Differential D133092
[clang] fix generation of .debug_aranges with LTO Authored by azat on Aug 31 2022, 10:42 PM.
Details Right now in case of LTO the section is not emited: $ cat test.c void __attribute__((optnone)) bar() { } void __attribute__((optnone)) foo() { bar(); } int main() { foo(); } $ clang -flto=thin -gdwarf-aranges -g -O3 test.c $ eu-readelf -waranges a.out | fgrep -c -e foo -e bar 0 $ clang -gdwarf-aranges -g -O3 test.c $ eu-readelf -waranges a.out | fgrep -c -e foo -e bar 2 Fix this by passing explicitly --plugin-opt=-generate-arange-section. Suggested-by: OCHyams <orlando.hyams@sony.com>
Diff Detail
Event TimelineComment Actions When constructing the compiler command line (here in Clang.cpp) there's a special case for SCE debugger tuning: // -gdwarf-aranges turns on the emission of the aranges section in the // backend. // Always enabled for SCE tuning. bool NeedAranges = DebuggerTuning == llvm::DebuggerKind::SCE; This isn't an area I've looked at before but It looks like we (Sony - cc @probinson), at least historically, have DWARF consumers that want to see .debug_aranges. Given there's this bit of code for the compiler command, IMO it would be reasonable to add the special case for the linker command too. Please could you add that? I mentioned on the discourse thread but will repeat here for other reviewers: This approach SGTM but I would feel more comfortable if someone with more experience reviewed this. Comment Actions @Orlando thanks for taking a look!
Yeah, but it seems that it is not possible to use lld for PS4 (only orbis-ld), according to https://github.com/llvm/llvm-project/blob/0dcbe0e1df407ce0e6d9e3df3d177a669723faa8/clang/lib/Driver/ToolChains/PS4CPU.cpp#L203-L206 Comment Actions
It's correct that PS4 does not use lld. PS5 does use lld (under a different name).
PS4CPU.cpp actually does have a case where we pass "-mllvm -enable-jmc-instrument" to lld, so you are not breaking new ground here.
Comment Actions
Great, thanks for pointing out! Comment Actions (generally seems OK - maybe include a comment about this being a less-than-perfect solution (the better solution would be to encode this in IR metadata (DICompileUnit) if it's reasonable to respect this option on a per-CU basis (which it probably is, though that'd be a bit of backend work) - or to set it as IR global metadata (like how the DWARF version is encoded - probably using the same IR linker merging strategy, of choosing the highest value (so if any module has aranges, then a linked module has aranges too - even for the CUs that had it turned off) if it's really not feasible to support on a per-CU basis) but probably OK-enough, given that aranges is hopefully on the way out & not worth all the work of the deeper fix)
Comment Actions @dblaikie Rebased. Brief of you comment had been added into the code, and the whole comment included into the commit message. Thanks for taking a look! Comment Actions Generally not. the usual flow would be once you have approval to go ahead and submit the patch yourself - I take it you don't have commit access? In which case I can commit this on your behalf. Comment Actions
Yeah, I don't
Yes, please! Comment Actions
FTR, I've raised an internal ticket about this, and someone from Sony will handle it. Comment Actions ~sort of. The process is described here: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access But generally not every developer is immediately aware of who has/doesn't have commit access - so if your patch gets approved and you can't commit it yourself, please follow up on the review/ask that someone commits it for you. Comment Actions Hm, by some reason on CI there is lld, any clue? (I've looked through other tests, and cannot find anything different that will show path to lld or require it explicitly) Comment Actions
Yep, already looked through it, thanks for the link.
Noted, thank you! Comment Actions For your information, after this patch check-clang fails with following errors if there is no lld (lld is not enable in CMake, and lld is not installed previously). I appreciate if you run debug-options.c test with -fuse-ld=lld if the lld is existing. Thanks. /home/jam/llvm-upstream/llvm-project/clang/test/Driver/debug-options.c:379:16: error: LLDGARANGE: expected string not found in input
// LLDGARANGE: {{".*lld.*"}} {{.*}} "-generate-arange-section"
^
<stdin>:1:1: note: scanning from here
clang version 16.0.0 (git@kaz7.github.com:llvm/llvm-project.git 734843ebc7c348a154182da00d4f0e215932d64e)
^
<stdin>:7:546: note: possible intended match here
"/home/jam/llvm-upstream/build/bin/clang-16" "-cc1" "-triple" "x86_64-unknown-linux" "-emit-llvm-bc" "-flto=full" "-flto-unit" "-disable-free" "-clear-ast-before-backend" "-disable-llvm-verifier" "-discard-value-names" "-main-file-name" "debug-options.c" "-mrelocation-model" "pic" "-pic-level" "2" "-pic-is-pie" "-mframe-pointer=all" "-fmath-errno" "-ffp-contract=on" "-fno-rounding-math" "-mconstructor-aliases" "-funwind-tables=2" "-target-cpu" "x86-64" "-tune-cpu" "generic" "-mllvm" "-treat-scalable-fixed-error-as-warning" "-debugger-tuning=gdb" "-mllvm" "-generate-arange-section" "-fcoverage-compilation-dir=/home/jam/llvm-upstream/build/tools/clang/test/Driver" "-resource-dir" "/home/jam/llvm-upstream/build/lib/clang/16.0.0" "-internal-isystem" "/home/jam/llvm-upstream/build/lib/clang/16.0.0/include" "-internal-isystem" "/usr/local/include" "-internal-isystem" "/usr/lib/gcc/x86_64-linux-gnu/11/../../../../x86_64-linux-gnu/include" "-internal-externc-isystem" "/usr/include/x86_64-linux-gnu" "-internal-externc-isystem" "/include" "-internal-externc-isystem" "/usr/include" "-fdebug-compilation-dir=/home/jam/llvm-upstream/build/tools/clang/test/Driver" "-ferror-limit" "19" "-fgnuc-version=4.2.1" "-faddrsig" "-D__GCC_HAVE_DWARF2_CFI_ASM=1" "-o" "/tmp/lit-tmp-l8cchoyh/debug-options-1436f9.o" "-x" "c" "/home/jam/llvm-upstream/llvm-project/clang/test/Driver/debug-options.c"
...Comment Actions Yep, already worked on it, fix - https://reviews.llvm.org/D133841 P.S. I don't have commit rights so if someone can commit it after CI that will be great Comment Actions Reverted this in 5631d20bfc9f77d15435badf6ce34e1a56c27e1c since the fix for the test failures this change here caused got reverted due to breaking other tests. Let's regroup and make a new patch that lands this change with all test failures addressed :) Comment Actions This is not correct. GNU ld and gold don't accept -mllvm. You need to use -plugin-opt=-generate... Comment Actions
@MaskRay actually those options will be added only if the name matches lld/lld.exe (but I guess I need to provide more context so that it will be easier to understand), but you your approach is better anyway, since:
The new version of the patch will be available here - https://reviews.llvm.org/D133875 Comment Actions Which patch needs review, this one or D133875? Or are they both relevant? (their names are basically identical, so I'm not sure which is needed/how they're different) Comment Actions
Sorry, wrote in the wrong thread. The only patch that needs review is resubmitted version - https://reviews.llvm.org/D133875
The relevant contains resubmit in the title. Comment Actions If the two patches are basically identical with just some fixes for the problem, the convention is to reuse the original Differential. No need to change the subject to (resubmit). Comment Actions
Ok, did not know that, will keep it in mind, thanks!
Actually I don't have such actions.
Yeah, that's a good point, but by some reason I though that it is not possible/good to update already landed revision... P.S. the resubmit version already have some comments too, maybe it will be better to continue there? But I'm not against close it and move it here, if this is preferable and someone can reopen it. Comment Actions
I've updated this revision.
Comment Actions Apparently test failures are not related? Failed Tests (1): Clangd Unit Tests :: ./ClangdTests/TUSchedulerTests/IncluderCache
Comment Actions
Ok, updated. Can someone pick this patch please? | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
The note should really say "PS4/PS5 do not go through this path, hence ..."