[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:930
+      // For an empty function, a pair of 0 can terminate pre-DWARF-v5
+      // .debug_loc and .debug_ranges . -1 is a reserved value (base address
+      // selection entry). Use -2 instead.
----------------
jhenderson wrote:
> Nit: spurious space before the full stop.
I tend to not write `.debug_ranges.` I will add `sections`


================
Comment at: lld/test/ELF/debug-dead-reloc-32.s:17
+
+.section .text.1,"axe"
+  .byte 0
----------------
jhenderson wrote:
> Is the 'e' for SHF_EXCLUDE? If so, do we actually want it and --gc-sections? Seems like one or the other would be sufficient to me.
I'll drop --gc-sections.


================
Comment at: lld/test/ELF/debug-dead-reloc-icf.s:2-5
+## Test we resolve symbolic relocations in .debug_* sections to a tombstone
+## if the referenced section symbol is folded into another section by ICF.
+## Otherwise, we would leave entries in multiple CUs claiming ownership of the
+## same range of code, which can confuse consumers.
----------------
dblaikie wrote:
> jhenderson wrote:
> > One issue that I don't recall whether it was brought up previously: if we resolve one to -1, and the other has no debug data (e.g. they came from two different CUs, only one of which was compiled with -g), we could actually lose debug data for the function completely.
> > 
> > I don't know what the right thing to do is in this situation, as I don't know whether it's better than the risk of confusing consumers. I don't think we can reasonably attempt to identify whether there is matching debug data for the kept version after all.
> Yeah, I'd accept that as an existing problem/not one that I think this tombstone work should necessarily address - the old solution or the new solution would be compatible with a linker making a more nuanced approach to this problem.
> 
> (but, in that separate discussion: I'm not sure this would be appropriate to fix - fixing this, I think, would mean having debug info influence which version of a function the linker chooses - increasing the risk of heisenbugs)
https://reviews.llvm.org/D59553#2083857

My understanding is that: the prevailing definition should have associated debug info. If not, the linker should not do extra work to attach debug info from other copies to the prevailing definition.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81784