Add options -m[no-]zos-ppa1-name to remove the function name in PPA1 on z/OS. - #2
Conversation
|
Hello, I adjusted the implementation in commit b87a9a4
Would you please take another look? Thanks! |
| if (T.isOSAIX() && (Args.hasArg(OPT_mignore_xcoff_visibility))) | ||
| Opts.IgnoreXCOFFVisibility = 1; | ||
|
|
||
| if (const Arg *A = Args.getLastArg(OPT_mzos_ppa1_name, |
There was a problem hiding this comment.
Follow the pattern AIX has above. If it is z/OS query the option and set the Opts. You don't need to handle the unsupported error if you follow that pattern.
If hasArg, getLastArg etc are not called you will get the unused arg warning on other platforms. That is consistent with other target specific options.
There was a problem hiding this comment.
If hasArg, getLastArg etc are not called you will get the unused arg warning on other platforms. That is consistent with other target specific options.
What mechanism guarantees this in clang cc1?
My understanding is Flags<[TargetSpecific]> guarantees this in clang Driver, but not in cc1.
For the above AIX option, I tried this
>/plex/sujian/llvm/main/build/bin/clang -mignore-xcoff-visibility -target powerpc-unknown-linux main.c
clang: error: unsupported option '-mignore-xcoff-visibility' for target 'powerpc-unknown-linux'
>/plex/sujian/llvm/main/build/bin/clang -cc1 -mignore-xcoff-visibility -triple powerpc-unknown-linux main.c
>
The definition of this option in clang/include/clang/Options/Options.td is
def mignore_xcoff_visibility : Flag<["-"], "mignore-xcoff-visibility">, Group<m_Group>,
HelpText<"Not emit the visibility attribute for asm in AIX OS or give all symbols 'unspecified' visibility in XCOFF object file">,
Flags<[TargetSpecific]>, Visibility<[ClangOption, CC1Option]>;
It is handled in clang/lib/Driver/ToolChains/AIX.cpp
void AIX::addClangTargetOptions(
const llvm::opt::ArgList &Args, llvm::opt::ArgStringList &CC1Args,
BoundArch BA, Action::OffloadKind DeviceOffloadingKind) const {
Args.AddLastArg(CC1Args, options::OPT_mignore_xcoff_visibility);
I think Flags<[TargetSpecific]> and AIX::addClangTargetOptions() guarantees an error is emitted on non-aix platforms when the option is specified to clang.
But there is no such mechanism in cc1. That aix option is silently ignored in cc1 if specified on non-aix platforms.
Thanks!
There was a problem hiding this comment.
AIX:addClangTargetOptions() doesn't generate the error message. That passes the option on to cc1. Verify this but, it looks like the behaviour is any unclaimed options marked as target specific generate that error message.
Don't worry about generating an error from -cc1 for not z/OS systems. Just make sure you only check for the option if the target is z/OS. That will leave the option as unclaimed for non z/OS systems.
There was a problem hiding this comment.
AIX:addClangTargetOptions() doesn't generate the error message.
I agree.
Any options with Flags<[TargetSpecific]> is only accepted on platforms handling it. An error is emitted on platforms that do not handle this option. This is done in clang Driver.
AIX:addClangTargetOptions() calls Args.AddLastArg(CC1Args, options::OPT_mignore_xcoff_visibility);, which makes this option handled on AIX. This avoids the error on AIX. Other platforms do not handle this option, and they would get the error.
So
I think Flags<[TargetSpecific]> and AIX::addClangTargetOptions() guarantees an error is emitted on non-aix platforms when the option is specified to clang.
There was a problem hiding this comment.
Verify this but, it looks like the behaviour is any unclaimed options marked as target specific generate that error message.
Yes, I agree. This happens only in clang Driver, but not in cc1.
There was a problem hiding this comment.
Don't worry about generating an error from -cc1 for not z/OS systems. Just make sure you only check for the option if the target is z/OS.
I am emitting an error from cc1 because most downstream zos-only options do this.
We actually do not emit an error in Driver. We let Driver pass the option to cc1 and check there.
But upstream checks in Driver and let cc1 silently ignore the option if it is not on the platforms supporting it.
I will follow the upstream pattern. It also matches with lit test.
There was a problem hiding this comment.
Just make sure you only check for the option if the target is z/OS. That will leave the option as unclaimed for non z/OS systems.
I am not sure whether you are mixing clang Driver and cc1.
To "leave the option as unclaimed for non z/OS systems"
- I do it in Driver by putting the handling in clang/lib/Driver/ToolChains/ZOS.cpp
- I feel "unclaimed" is not a concept applied in cc1
There was a problem hiding this comment.
Don't worry about generating an error from -cc1 for not z/OS systems. Just make sure you only check for the option if the target is z/OS.
I have made this change.
There was a problem hiding this comment.
Just make sure you only check for the option if the target is z/OS. That will leave the option as unclaimed for non z/OS systems.
I am not sure whether you are mixing clang Driver and cc1. To "leave the option as unclaimed for non z/OS systems"
- I do it in Driver by putting the handling in clang/lib/Driver/ToolChains/ZOS.cpp
- I feel "unclaimed" is not a concept applied in cc1
That may be the case, but it's good to always follow the practice of checking the context before checking the option. Use "if zos and hasArg" vs "if hasArg and zos".
| if (auto *Fn = dyn_cast<llvm::Function>(GV)) { | ||
| auto ZOSPPA1Name = M.getLangOpts().getZOSPPA1Name(); | ||
| if (ZOSPPA1Name == clang::LangOptions::ZOSPPA1NameKind::Emit) | ||
| Fn->addFnAttr("zos-ppa1-name", "true"); |
There was a problem hiding this comment.
If this is a string, I'd use values more descriptive like "include"/"exclude". That will leave room for other values (eg. leaf or not). If it's a value can't you use true/false constants?
There was a problem hiding this comment.
This is a string with a value.
My understanding (mainly from copilot)
LLVM supports 3 main kinds:
- Enum attributes (most common): Backed by Attribute::AttrKind enum
Attribute::NoInline
Attribute::ReadOnly
- String (target-dependent)
“target-cpu"="x86-64"
- Key-value pairs
align = 16
I am using the 2nd one.
The first one is over-killing in our case.
The 3rd one, I think the underneath type is int.
I will make the values more descriptive. Is it ok to use "emit" and "no-emit"? They match with LangOpts.
There was a problem hiding this comment.
Thanks. I'm fine with different values as long as those values that make the code in llvm that use them self documenting. A names like "always" and "none" could be better.
The llvm code never uses the false value. It would be good to add an assert to make sure the value is one of the two set here.
There was a problem hiding this comment.
I'm fine with different values as long as those values that make the code in llvm that use them self documenting. A names like "always" and "none" could be better.
Do you mean "zos-ppa1-name"=["always"|"none"]?
I actually feel they are quite confusing.
To start with, they are not a pair. "always" should be grouped with "sometimes" and "never", and "none" should be grouped with "single" and "multiple".
I don't think "emit" and "no-emit" are perfect, but they match with the LangOpts option. I'd like to use them if you don't object.
It would be good to add an assert to make sure the value is one of the two set here.
I don't follow very well.
Do you mean to add an assert of ZOSPPA1Name = M.getLangOpts().getZOSPPA1Name() only having clang::LangOptions::ZOSPPA1NameKind::Emit and clang::LangOptions::ZOSPPA1NameKind::NoEmit here in clang/lib/CodeGen/Targets/SystemZ.cpp?
I think the values ZOSPPA1Name can have are limited by enum class ZOSPPA1NameKind.
Or you want to add an assert in SystemZAsmPrinter::calculatePPA1() that the "zos-ppa1-name" attribute only has two values, in case this attribute is set somewhere else by mistake?
Thanks!
|
|
||
| if (const Arg *A = Args.getLastArg(OPT_mzos_ppa1_name, | ||
| OPT_mno_zos_ppa1_name)) { | ||
| if (T.isOSzOS()) { |
There was a problem hiding this comment.
use following as it's faster, follows the usual convention and avoids marking an option as claimed when it isn't used.
if (T.isOSzOS()) {
if (const Arg *A = Args.getLastArg(OPT_mzos_ppa1_name, OPT_mno_zos_ppa1_name)) {
...
}
}There was a problem hiding this comment.
Sure! I will make the change. Thanks!
|
Hello, I addressed the above comments:
Would you please take another look? Thanks! |
| if (auto *Fn = dyn_cast<llvm::Function>(GV)) { | ||
| auto ZOSPPA1Name = M.getLangOpts().getZOSPPA1Name(); | ||
| if (ZOSPPA1Name == clang::LangOptions::ZOSPPA1NameKind::Emit) | ||
| Fn->addFnAttr("zos-ppa1-name", "emit"); |
There was a problem hiding this comment.
Looks good. Just a minor tweak on the attribute values.
Let's use all and none. I see these values are used for frame-pointer. This leaves us the ability to add behaviour like non-leaf down the road (i.e. don't add the name to ppa1 if this function doesn't call anything).
There was a problem hiding this comment.
Hello, I have made the suggested change. Would you please take another look? Thanks!
…lvm#191275)" (llvm#206816) This reverts commit 0f51760. A test fails with the commit (llvm#191275 (comment)): ``` Traceback (most recent call last): File "/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/llvm-project/lldb/test/API/functionalities/scripted_frame_provider/TestScriptedFrameProvider.py", line 596, in test_python_source_frames self.assertNotIn("0xffffffffffffffff", output.lower()) AssertionError: '0xffffffffffffffff' unexpectedly found in "* thread #2, name = 'a.out', stop reason = breakpoint 1.1\n * frame #0: compute_fibonacci at python_helper.py:7 [synthetic]\n frame #1: process_data at python_helper.py:16 [synthetic]\n frame #2: main at python_helper.py:27 [synthetic]\n frame #3: 0x0000badc2de81358 a.out`thread_func(thread_num=0) at main.cpp:44:13\n frame #4: 0x0000badc2de81f9c a.out`void std::__invoke_impl<void, void (*)(int), int>((null)=__invoke_other @ 0x0000f1555ebae74f, __f=0x0000badc66845ec0, __args=0x0000badc66845eb8) at invoke.h:61:14\n frame #5: 0x0000badc2de81f18 a.out`std::__invoke_result<void (*)(int), int>::type std::__invoke<void (*)(int), int>(__fn=0x0000badc66845ec0, __args=0x0000badc66845eb8) at invoke.h:96:14\n frame llvm#6: 0x0000badc2de81ee4 a.out`void std::thread::_invoker<std::tuple<void (*)(int), int>>::_m_invoke<0ul, 1ul>(this=0x0000badc66845eb8, (null)=_index_tuple<0ul, 1ul> @ 0x0000f1555ebae7af) at std_thread.h:259:13\n frame llvm#7: 0x0000badc2de81e98 a.out`std::thread::_invoker<std::tuple<void (*)(int), int>>::operator()(this=0x0000badc66845eb8) at std_thread.h:266:11\n frame llvm#8: 0x0000badc2de81d70 a.out`std::thread::_state_impl<std::thread::_invoker<std::tuple<void (*)(int), int>>>::_m_run(this=0xffffffffffffffff) at std_thread.h:211:13\n frame llvm#9: 0x0000f1555ef029cc libstdc++.so.6`___lldb_unnamed_symbol_d29b0 + 28\n frame llvm#10: 0x0000f1555ec30398 libc.so.6`___lldb_unnamed_symbol_800c0 + 728\n frame llvm#11: 0x0000f1555ec99e9c libc.so.6`___lldb_unnamed_symbol_e9e90 + 12\n" ``` I don't know why this test fails with the PR, but I don't have time to fix it now, so revert it to unblock CI. The backtrace was ``` frame #0: compute_fibonacci at python_helper.py:7 [synthetic] frame #1: process_data at python_helper.py:16 [synthetic] frame #2: main at python_helper.py:27 [synthetic] frame #3: 0x0000badc2de81358 a.out`thread_func(thread_num=0) at main.cpp:44:13 frame #4: 0x0000badc2de81f9c a.out`void std::__invoke_impl<void, void (*)(int), int>((null)=__invoke_other @ 0x0000f1555ebae74f, __f=0x0000badc66845ec0, __args=0x0000badc66845eb8) at invoke.h:61:14 frame #5: 0x0000badc2de81f18 a.out`std::__invoke_result<void (*)(int), int>::type std::__invoke<void (*)(int), int>(__fn=0x0000badc66845ec0, __args=0x0000badc66845eb8) at invoke.h:96:14 frame llvm#6: 0x0000badc2de81ee4 a.out`void std::thread::_invoker<std::tuple<void (*)(int), int>>::_m_invoke<0ul, 1ul>(this=0x0000badc66845eb8, (null)=_index_tuple<0ul, 1ul> @ 0x0000f1555ebae7af) at std_thread.h:259:13 frame llvm#7: 0x0000badc2de81e98 a.out`std::thread::_invoker<std::tuple<void (*)(int), int>>::operator()(this=0x0000badc66845eb8) at std_thread.h:266:11 frame llvm#8: 0x0000badc2de81d70 a.out`std::thread::_state_impl<std::thread::_invoker<std::tuple<void (*)(int), int>>>::_m_run(this=0xffffffffffffffff) at std_thread.h:211:13 frame llvm#9: 0x0000f1555ef029cc libstdc++.so.6`___lldb_unnamed_symbol_d29b0 + 28 frame llvm#10: 0x0000f1555ec30398 libc.so.6`___lldb_unnamed_symbol_800c0 + 728 frame llvm#11: 0x0000f1555ec99e9c libc.so.6`___lldb_unnamed_symbol_e9e90 + 12 ``` This contains 0xffffffffffffffff in frame 8.
… movemask (llvm#199081) The existing lowering in vectorToScalarBitmask() creates a 1 bit per lane movemask using a powers of 2 reduction (and+addv with a constant pool entry). This patch adds a DAG combine on ISD::CTTZ that recognizes cttz(bitcast <N x i1> to iN) and produces a compressed movemask with shrn (for i8 lanes) or xtn (for wider lanes) then runs scalar cttz on a 64- or 128-bit value. Dividing by bits per lane gives the lane index. Supports lane counts {2, 4, 8, 16, 32} (one or two NEON registers) For the example in the issue (`<16 x i8> -> i16`): Before: ```asm adrp x8, .LCPI0_0 cmlt v0.16b, v0.16b, #0 ldr q1, [x8, :lo12:.LCPI0_0] and v0.16b, v0.16b, v1.16b ext v1.16b, v0.16b, v0.16b, llvm#8 zip1 v0.16b, v0.16b, v1.16b addv h0, v0.8h fmov w8, s0 orr w8, w8, #0x10000 rbit w8, w8 clz w0, w8 ret ``` After: ```asm cmlt v0.16b, v0.16b, #0 shrn v0.8b, v0.8h, #4 fmov x8, d0 rbit x8, x8 clz x8, x8 lsr x0, x8, #2 ret ``` Also created a new test file with 24 functions covering both endianness - 7 basic widths (`<16 x i8>` down through `<2 x i32>`) - 4 wider cases that span two NEON registers (`<32 x i8>` etc) - 2 narrow cases that get sext'd up to fit one register - 2 cases where the bitcast has two users - 4 alternative comparison operators (`eq`, `ne`, etc) - 1 `cttz` with `is_zero_undef=true` - 4 negative tests that bails for unsupported shapes Fixes llvm#186295
…on z/OS. (#2) This PR adds options -m[no-]zos-ppa1-name to remove the function name in PPA1 on z/OS.
…lvm#207008) `Evaluate_DW_OP_convert` dereferenced `eval_ctx.dwarf_cu` (the `DWARFExpression` Delegate) whenever the operand DIE offset was non-zero, and unconditionally read `eval_ctx.stack.back()`. When a DWARF expression is evaluated without a DWARF unit (as the lldb-dwarf-expression-fuzzer does), two operand shapes crash: - `DW_OP_convert` with a non-zero offset calls `dwarf_cu->GetDIEBitSizeAndSign(...)` on a null Delegate. - `DW_OP_convert` with nothing on the stack reads the back of an empty vector. The unit test feeds both with `dwarf_cu == nullptr` and crashes: ``` [ RUN ] DWARFExpression.DW_OP_convert #2 SignalHandler(int, __siginfo*, void*) #4 DWARFExpression::Evaluate(...) #5 Evaluate(ArrayRef<unsigned char>, ...) ``` (SIGSEGV, the process aborts.) Bail out with an error when the stack is empty, and when a non-zero DIE offset is requested without a DWARF unit, instead of crashing. Extends `DWARFExpression.DW_OP_convert` with these two cases, which crash without the fix. --------- Co-authored-by: Michael Buch <michaelbuch12@gmail.com>
…long encodings (llvm#205907) When a (signed or unsigned) LEB128 value is encoded with extra trailing bytes that only carry zero- or sign-extension, the decode loop could keep running with the shift amount at 64 or beyond and then evaluate `Slice << Shift`, which is undefined behavior for a 64-bit type. The DWARF expression parser feeds attacker-controlled LEB128 operands (such as `DW_OP_bregN` / `DW_OP_constu`) through `DataExtractor::getULEB128` / `getSLEB128`, so the `lldb-dwarf-expression-fuzzer` reaches this under UBSan. The unsigned case: ``` LEB128.h:152:20: runtime error: shift exponent 70 is too large for 64-bit type 'uint64_t' (aka 'unsigned long long') #0 llvm::decodeULEB128(...) LEB128.h:152 #1 getLEB128<unsigned long long>(...) DataExtractor.cpp:227 #2 llvm::DataExtractor::getULEB128(...) DataExtractor.cpp:241 #3 llvm::DWARFExpression::Operation::extract(...) DWARFExpression.cpp:218 llvm#7 lldb_private::DWARFExpression::Evaluate(...) DWARFExpression.cpp:1333 llvm#9 LLVMFuzzerTestOneInput lldb-dwarf-expression-fuzzer.cpp:83 SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior LEB128.h:152:20 ``` and the signed case, reached the same way: ``` LEB128.h:190:20: runtime error: shift exponent 70 is too large for 64-bit type 'uint64_t' (aka 'unsigned long long') #0 llvm::decodeSLEB128(...) LEB128.h:190 #1 getLEB128<long long>(...) DataExtractor.cpp:227 #2 llvm::DataExtractor::getSLEB128(...) DataExtractor.cpp:245 #3 llvm::DWARFExpression::Operation::extract(...) DWARFExpression.cpp:216 llvm#7 lldb_private::DWARFExpression::Evaluate(...) DWARFExpression.cpp:1333 ``` The existing range checks already guarantee that once `Shift` reaches 64 the remaining bytes are pure extension and contribute nothing to the result, so skip the accumulating shift in that case. Decoded values are unchanged for all well-formed inputs. Adds overlong-encoding regression cases to `LEB128Test`. Without the fix the signed case is the UBSan diagnostic above in a sanitizer build, and in a normal build the unsigned case also decodes to the wrong value (the overlong `1` decodes as `11`).
This is a PR for internal review.
After the PR is approved I will use the base branch sj_mzos-ppa1-name to create an upstream PR.