This is an archive of the discontinued LLVM Phabricator instance.

[clang] fix generation of .debug_aranges with LTO
ClosedPublic

Authored by azat on Aug 31 2022, 10:42 PM.

Details

Summary

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 Timeline

azat created this revision.Aug 31 2022, 10:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 31 2022, 10:42 PM
Herald added a subscriber: inglorion. · View Herald Transcript
azat requested review of this revision.Aug 31 2022, 10:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 31 2022, 10:42 PM
azat updated this revision to Diff 457182.Aug 31 2022, 11:18 PM

Add test

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.

azat added a comment.Sep 2 2022, 7:43 AM

@Orlando thanks for taking a look!

When constructing the compiler command line (here in Clang.cpp) there's a special case for SCE debugger tuning:

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

Refs: https://reviews.llvm.org/D81970

azat updated this revision to Diff 457678.Sep 2 2022, 12:49 PM

v3: test only linux (to avoid failures on windows)
v4: add a comment about SCE

azat updated this revision to Diff 457850.Sep 4 2022, 4:38 AM

Adjust the test (to fix build on windows)

azat added a comment.Sep 6 2022, 6:35 AM

Can someone take a look please?

Yeah, but it seems that it is not possible to use lld for PS4

It's correct that PS4 does not use lld. PS5 does use lld (under a different name).
But, we don't use addLTOOptions() for either PS4 or PS5, so this patch doesn't affect (or help) them. I'm okay with saying we'll need to deal with that case separately.

P.S. although this looks like a hack, since none of -mllvm was passed to the lld before.

PS4CPU.cpp actually does have a case where we pass "-mllvm -enable-jmc-instrument" to lld, so you are not breaking new ground here.

clang/lib/Driver/ToolChains/CommonArgs.cpp
511

The note should really say "PS4/PS5 do not go through this path, hence ..."

clang/test/Driver/debug-options.c
379

You don't need to use -DAG on LLDGARANGE, because it's used only once and not combined with other prefixes.

azat updated this revision to Diff 458573.Sep 7 2022, 2:58 PM

Update test and comments in the code (suggestions from @probinson)

azat marked 2 inline comments as done.Sep 7 2022, 2:59 PM

PS4CPU.cpp actually does have a case where we pass "-mllvm -enable-jmc-instrument" to lld, so you are not breaking new ground here.

Great, thanks for pointing out!
The patch had been rebased with addressed comments.

(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)

clang/lib/Driver/ToolChains/CommonArgs.cpp
512–513
azat added inline comments.Sep 7 2022, 11:58 PM
clang/lib/Driver/ToolChains/CommonArgs.cpp
512–513

I've added extra NeedAranges like in clang/lib/Driver/ToolChains/Clang.cpp for better grep'ability, but no problem, I will change this.

azat updated this revision to Diff 458663.Sep 8 2022, 12:03 AM

Adjust comments as suggested by @dblaikie

azat marked an inline comment as done.Sep 8 2022, 12:05 AM

@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!

I'm happy if @dblaikie is happy.

clang/lib/Driver/ToolChains/CommonArgs.cpp
515

We don't usually cite people by name in code comments. Credit for the thought is clear in the review.

azat updated this revision to Diff 458717.Sep 8 2022, 6:38 AM

Update comment

probinson added inline comments.Sep 8 2022, 6:48 AM
clang/lib/Driver/ToolChains/CommonArgs.cpp
515

Typo, perfert -> perfect

516

...this may not _be_ worth it, since _it_ looks like aranges _is_ on the way out.

azat updated this revision to Diff 458723.Sep 8 2022, 6:53 AM
azat marked an inline comment as done.

Update comment

azat marked 2 inline comments as done.Sep 8 2022, 6:53 AM
dblaikie accepted this revision.Sep 8 2022, 7:13 AM

Sounds good

This revision is now accepted and ready to land.Sep 8 2022, 7:13 AM
azat added a comment.Sep 13 2022, 11:43 AM

Is there anything left here?

Is there anything left here?

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.

azat added a comment.Sep 13 2022, 12:34 PM

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?

Yeah, I don't
I though that only trusted/experienced/... llvm devs can commit.

In which case I can commit this on your behalf.

Yes, please!

we don't use addLTOOptions() for either PS4 or PS5, so this patch doesn't affect (or help) them. I'm okay with saying we'll need to deal with that case separately.

FTR, I've raised an internal ticket about this, and someone from Sony will handle it.

This revision was automatically updated to reflect the committed changes.

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?

Yeah, I don't
I though that only trusted/experienced/... llvm devs can commit.

~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.

azat added a comment.Sep 13 2022, 11:51 PM

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)

azat added a comment.Sep 13 2022, 11:52 PM

~sort of. The process is described here: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

Yep, already looked through it, thanks for the link.

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.

Noted, thank you!

kaz7 added a subscriber: kaz7.EditedSep 14 2022, 12:04 AM

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"
...

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.

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

kaz7 added a comment.Sep 14 2022, 9:15 AM

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.

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

Thank you. I've not noticed it.

thakis added a subscriber: thakis.Sep 14 2022, 9:44 AM

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 :)

This is not correct. GNU ld and gold don't accept -mllvm. You need to use -plugin-opt=-generate...

MaskRay reopened this revision.Sep 14 2022, 10:50 AM
This revision is now accepted and ready to land.Sep 14 2022, 10:50 AM
MaskRay requested changes to this revision.Sep 14 2022, 10:50 AM
This revision now requires changes to proceed.Sep 14 2022, 10:50 AM
azat added a comment.Sep 14 2022, 1:41 PM

This is not correct. GNU ld and gold don't accept -mllvm. You need to use -plugin-opt=-generate...

@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:

  • it does not require additional condition
  • it works for gold and bfd too (so they do interpret --plugin-opt=-generate-arange-section option too! I wasn't expecting that)

The new version of the patch will be available here - https://reviews.llvm.org/D133875

azat added a comment.Sep 14 2022, 1:43 PM

I wasn't expecting that

Oh, I see, it wasn't the linker, but the plugin.

azat added a comment.Sep 20 2022, 1:46 AM

Can someone do a final review on this? And apply it if everything is OK.

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)

azat abandoned this revision.Sep 20 2022, 1:12 PM

Which patch needs review, this one or D133875? Or are they both relevant?

Sorry, wrote in the wrong thread. The only patch that needs review is resubmitted version - https://reviews.llvm.org/D133875

(their names are basically identical, so I'm not sure which is needed/how they're different)

The relevant contains resubmit in the title.
I also will abandoned this revision (hope this will make this slightly clear).

MaskRay added a comment.EditedSep 20 2022, 1:15 PM

If the two patches are basically identical with just some fixes for the problem, the convention is to reuse the original Differential.
You can click "Add Action - Open" (or Reopen?), then you can upload a new patch to update the Differential.
This keeps all discussions in one place and keeps are reviewers and interested folks aware.

No need to change the subject to (resubmit).

azat added a comment.Sep 20 2022, 1:49 PM

If the two patches are basically identical with just some fixes for the problem, the convention is to reuse the original Differential.

Ok, did not know that, will keep it in mind, thanks!

You can click "Add Action - Open" (or Reopen?), then you can upload a new patch to update the Differential.

Actually I don't have such actions.

This keeps all discussions in one place and keeps are reviewers and interested folks aware.

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.

azat reclaimed this revision.Sep 25 2022, 1:06 PM
This revision now requires changes to proceed.Sep 25 2022, 1:06 PM
azat updated this revision to Diff 462756.Sep 25 2022, 1:08 PM

Resubmit with fixed tests and using --plugin-opt

azat edited the summary of this revision. (Show Details)Sep 25 2022, 1:09 PM

If the two patches are basically identical with just some fixes for the problem, the convention is to reuse the original Differential.

I've updated this revision.

azat added a comment.Sep 28 2022, 3:07 AM

Can someone make a final review please? Thanks in advance.

MaskRay added inline comments.Sep 29 2022, 12:02 AM
clang/test/Driver/debug-options-aranges.c
5 ↗(On Diff #462756)

Avoid legacy -target . Delete -fuse-ld=lld which is unneeded.

azat added inline comments.Sep 29 2022, 12:09 AM
clang/test/Driver/debug-options-aranges.c
5 ↗(On Diff #462756)

Delete -fuse-ld=lld which is unneeded

I've tried without it and in some env it may try to use gold, which will not be available on CI and the test will fail.
Do you have suggestions on how to overcome this? Required gold so as ld? Or leaving -fuse-ld=lld is fine?

MaskRay added inline comments.Sep 29 2022, 12:17 AM
clang/test/Driver/debug-options-aranges.c
5 ↗(On Diff #462756)

-### does not run the command. This shall pass in any environment.

azat updated this revision to Diff 463778.Sep 29 2022, 12:59 AM

Update the test

azat marked 2 inline comments as done.Sep 29 2022, 1:00 AM
azat added inline comments.
clang/test/Driver/debug-options-aranges.c
5 ↗(On Diff #462756)

You right, I forgot that real execution of commands had been removed in the latest version of the patch. Thanks. Rebased.

azat marked an inline comment as done.Sep 29 2022, 1:55 PM

Apparently test failures are not related?

Failed Tests (1):
  Clangd Unit Tests :: ./ClangdTests/TUSchedulerTests/IncluderCache
azat updated this revision to Diff 464486.Oct 1 2022, 4:09 AM

Rerun CI

MaskRay accepted this revision.Oct 1 2022, 3:15 PM
MaskRay added inline comments.
clang/test/Driver/debug-options-aranges.c
1 ↗(On Diff #464486)

Remove // REQUIRES: lld

3 ↗(On Diff #464486)

Optional: End full sentences with a period. Some directories use the convention that non-RUN-non-CHECK lines use /// as the comment marker. The idea is to make comments stand out and foster possible future lit/FileCheck improvement that check unused prefixes.

4 ↗(On Diff #464486)

This otherwise empty line is not useful.

This revision is now accepted and ready to land.Oct 1 2022, 3:15 PM
azat updated this revision to Diff 464542.Oct 2 2022, 2:53 AM
azat marked an inline comment as done.

Update comment in the test

azat marked an inline comment as done.Oct 2 2022, 2:53 AM

Optional: End full sentences with a period. Some directories use the convention that non-RUN-non-CHECK lines use /// as the comment marker. The idea is to make comments stand out and foster possible future lit/FileCheck improvement that check unused prefixes.

Ok, updated.

Can someone pick this patch please?

This revision was automatically updated to reflect the committed changes.