[PATCH] D81784: [ELF] Resolve relocations in .debug_* referencing (discarded symbols or ICF folded section symbols) to tombstone values

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/InputSection.cpp:908
 
-    if (sym.isTls() && !Out::tlsPhdr)
+    auto *ds = dyn_cast<Defined>(&sym);
+    if (sym.isTls() && !Out::tlsPhdr) {
----------------
dblaikie wrote:
> Probably not a big deal, but curious to do this test when it might not be needed. Ah, partly because fo the if/else if/else structure - if this used early-return instead, perhaps it would be easier to write it like this:
> 
> ```
> if (sym.isTls() && !Out::tlsPhdr) {
>   target->relocateNoSym(bufLoc, type, 0);
>   continue;
> }
> if (isDebug && type == target->symbolicRel) {
>   auto *ds = dyn_cast<Defined>(&sym);
>   if (!sym.getOutputSection() ||
>         (ds && ds->section->repl != ds->section)) {
>     target->relocateNoSym(bufLoc, type, isDebugLocOrRanges ? UINT64_MAX - 1 : UINT64_MAX);
>     continue;
>   }
> }
> target->relocateNoSym(bufLoc, type,
>                             SignExtend64<bits>(sym.getVA(addend)));
Thanks for the suggestion! I thought about the additional test a normal code path would run. It seems that you're also concerned - so I'll refactor the if else if else structure as you suggested to avoid an unneeded dyn_cast.


================
Comment at: lld/ELF/InputSection.cpp:915-919
+      // folded section symbols) to a tombstone value. The normal behavior
+      // (resolving to addend) is unsatisfactory because the result address
+      // range may collide with a valid range of low address, or leave multiple
+      // CUs claiming ownership of the same range of code, which may confuse
+      // consumers.
----------------
dblaikie wrote:
> "normal behavior" might be overstating it - it was the behavior of lld and is the behavior of gold, but isn't the behavior of bfd.ld.
> 
> maybe "Other options such as zero (1 for debug_ranges) (bfd.ld) or zero+addend (gold) result in overlaps with valid low-address ranges, or deeper problems terminating debug_loc and debug_range lists early"?
> 
> 
I'll just say "Resolving to addend". I may send a patch to GNU ld, and a statement about GNU ld's behavior may be soon outdated.


================
Comment at: lld/test/ELF/debug-dead-reloc.s:37
+
+## .text.3 from The second %t1.o is discarded. Resolved to UINT64_C(-1).
+.section .debug_addr
----------------
dblaikie wrote:
> So even if the comdat group is identical, this will resolve only one of them & use -1 for the other?
> 
> I think that's a regression/separate change that probably should have a sepraate discussion compared to the -1 rather than 0+addend change.
> 
> Seems there could be some value in resolving all the function descriptions to the same code where possible - but I also see the problem with having CUs ranges overlap (which I think @probinson is especially concerned about). So the change may well be merited, but I think deserves a separate review/discussion.
> 
> (eg: currently... oh, bfd and gold currently do the "point all the DWARF to the one definition" but it seems lld doesn't do that and, as you say, already does zero (well, zero+addend) out the duplicate references to the same deduplicated comdat) Fair enough then... curious, but not a change in behavior, so guess we slipped that in somewhere along the way & hasn't broken anything that I've heard.
A relocation referencing a local symbol defined in a non-prevailing comdat group currently resolves to 0+addend, instead of   prevailing_symbol_address+addend. We change it from 0+addend to -1. This is not a regression.

A relocation referencing a global symbol defined in a non-prevailing comdat group resolves prevailing_symbol_address+addend (ELF rule). This patch does not change the behavior. Though, I can't find any global symbol reference in .debug_*
Added a test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81784/new/

https://reviews.llvm.org/D81784