[clang] fix generation of .debug_aranges with LTO

[azat]

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 <[e-mail]>

[azat]

Add test

[Orlando]

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]

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

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

[azat]

Adjust the test (to fix build on windows)

[azat]

Can someone take a look please?

[probinson]

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

512

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

clang/test/Driver/debug-options.c

379 ↗

(On Diff #457850)

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

[azat]

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

[azat]

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.

[dblaikie]

(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

513–514

[azat]

clang/lib/Driver/ToolChains/CommonArgs.cpp

513–514

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]

Adjust comments as suggested by @dblaikie

[azat]

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

[probinson]

I'm happy if @dblaikie is happy.

clang/lib/Driver/ToolChains/CommonArgs.cpp

516

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

[azat]

Update comment

[probinson]

clang/lib/Driver/ToolChains/CommonArgs.cpp

516

Typo, perfert -> perfect

517

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

[azat]

Update comment

[dblaikie]

Sounds good

[azat]

Is there anything left here?

[dblaikie]

In D133092#3787247, @azat wrote:

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]

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!

[probinson]

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.

[dblaikie]

In D133092#3787379, @azat wrote:

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]

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]

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

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 ([e-mail]: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"
...

[azat]

In D133092#3788734, @kaz7 wrote:

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]

In D133092#3788749, @azat wrote:

In D133092#3788734, @kaz7 wrote:

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]

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

[MaskRay]

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

[azat]

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]

I wasn't expecting that

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

[azat]

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

[dblaikie]

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]

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]

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]

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]

Resubmit with fixed tests and using --plugin-opt

[azat]

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]

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

[MaskRay]

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

6

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

[azat]

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

6

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]

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

6

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

[azat]

Update the test

[azat]

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

6

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

[azat]

Apparently test failures are not related?

Failed Tests (1):
  Clangd Unit Tests :: ./ClangdTests/TUSchedulerTests/IncluderCache

[azat]

Rerun CI

[MaskRay]

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

2

Remove // REQUIRES: lld

4

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.

5

This otherwise empty line is not useful.

[azat]

Update comment in the test

[azat]

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?

clang/lib/Driver/ToolChains/CommonArgs.cpp

#endif
     SmallString<1024> Plugin;
     llvm::sys::path::native(
         Twine(D.Dir) + "/../" CLANG_INSTALL_LIBDIR_BASENAME "/LLVMgold" +
             Suffix,
         Plugin);
     CmdArgs.push_back(Args.MakeArgString(Plugin));
   }
 
+  // Note, this solution is far from perfect, better to encode it into IR
+  // metadata, but this may not be worth it, since it looks like aranges is on
+  // the way out.
+  if (Args.hasArg(options::OPT_gdwarf_aranges)) {
+    CmdArgs.push_back(
+        Args.MakeArgString("--plugin-opt=-generate-arange-section"));
+  }
+
   // Try to pass driver level flags relevant to LTO code generation down to
   // the plugin.
 
   // Handle flags for selecting CPU variants.
   std::string CPU = getCPUName(D, Args, ToolChain.getTriple());
   if (!CPU.empty())
     CmdArgs.push_back(Args.MakeArgString(Twine("-plugin-opt=mcpu=") + CPU));
 

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

+// REQUIRES: lld
+
+/// Check that the linker plugin will get -generate-arange-section.
+// RUN: %clang -### -g --target=x86_64-linux -flto      -gdwarf-aranges %s 2>&1 | FileCheck %s
+// RUN: %clang -### -g --target=x86_64-linux -flto=thin -gdwarf-aranges %s 2>&1 | FileCheck %s
+// CHECK: --plugin-opt=-generate-arange-section