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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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.
----------------
Nit: spurious space before the full stop.


================
Comment at: lld/test/ELF/debug-dead-reloc-32.s:2
+# REQUIRES: x86
+## Test we resolve symbolic relocations in .debug_* sections to a tombstone
+## if the referenced symbol is discarded (--gc-sections, non-prevailing
----------------
tombstone -> tombstone value

Here and in other tests.


================
Comment at: lld/test/ELF/debug-dead-reloc-32.s:17
+
+.section .text.1,"axe"
+  .byte 0
----------------
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.


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


================
Comment at: lld/test/ELF/debug-dead-reloc-icf.s:12
+# CHECK:      Contents of section .debug_info:
+# CHECK-NEXT:  0000 {{.*}} 00000000 ffffffff ffffffff
+
----------------
Perhaps it would be better to spell out all the zeros for the first relocation target, as it was a bit confusing seeing one set of 8 followed by a set of 16.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81784