[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 marked an inline comment as done.
MaskRay added inline comments.


================
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:
> 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.
This was previously discussed on https://bugs.llvm.org/show_bug.cgi?id=37212
I agree with you that it deserves its own email thread.

See `PRETEND` and `_bfd_elf_check_kept_section` in binutils-gdb/bfd/elflink.c for GNU ld's behavior. GNU ld finds a replacement if the sizes are equal.

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

I agree. I think a tombstone value is better than `PRETEND` logic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81784