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

[David Blaikie via Phabricator via llvm-commits]

dblaikie 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) {
----------------
MaskRay wrote:
> 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.
yeah, I doubt it's a huge actual perf concern - but shorter scoped variables, etc, make for easier to understand code


================
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
----------------
MaskRay wrote:
> 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.
Yep, just local symbols - you can test the behavior between bfd/gold/lld with this example:

a.cpp:
```
inline void f1() { }
void f2() { f1(); }
```
b.cpp:
```
inline void f1() { }
void f2();
int main() {
  f1();
  f2();
}
```
```
$ clang++ f1.cpp f2.cpp -g
$ llvm-dwarfdump a.out
```

& you'll see that gold and bfd resolve all the relocations to the same comdat to the same address - though this only happens if the comdats are identical. If they differ (say, if you make f1 do some arithmetic and optimize one but don't optimize the other) then the behavior will match lld - keeping one but pointing only the matching DWARF to that definition, and all the other DWARF will point to the tombstone value (whatever that is in each linker).

Anyway - nothing new here/related to this patch, and the lld behavior's probably preferable for a lot of folks - means CUs don't start overlapping unintentionally, etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81784