[PATCH] D59553: [LLD][ELF][DebugInfo] llvm-symbolizer shows incorrect source line info if --gc-sections used

Thu Mar 28 06:07:01 PDT 2019

jhenderson added a comment.

A resolution on the compiler/assembler side of things (basically i.e. not rewriting DWARF) will still need something in the linker for a while, due to old objects, if we want to have a perfect experience.

================
Comment at: lld/ELF/InputSection.cpp:834
       Target->relocateOne(BufLoc, Type, SignExtend64<Bits>(Sym.getVA(Addend)));
+    else
+      // If relocation points to the deleted section (-Wl,--gc-sections),
----------------
This will patch any non-alloc section that is relocated, not just .debug* sections. I think it should be scoped to those kinds of sections only, as it could break the behaviour for other unknown sections that people may have.

================
Comment at: lld/ELF/InputSection.cpp:835
+    else
+      // If relocation points to the deleted section (-Wl,--gc-sections),
+      // then it is neccessary to make start offset to be out of
----------------
If relocation -> If the relocation

You don't need to put "-Wl," in the switch, since we're in the linker here already. Just use --gc-sections directly.

================
Comment at: lld/ELF/InputSection.cpp:836
+      // If relocation points to the deleted section (-Wl,--gc-sections),
+      // then it is neccessary to make start offset to be out of
+      // module address range to not to overlap with module's ranges.
----------------
to make start offset to be out of -> to make the start offset be outside the

================
Comment at: lld/ELF/InputSection.cpp:837
+      // then it is neccessary to make start offset to be out of
+      // module address range to not to overlap with module's ranges.
+      // UINT64_MAX-1 used as maximal value of an address. UINT64_MAX has
----------------
to not to overlap with -> to not overlap with the

================
Comment at: lld/ELF/InputSection.cpp:838
+      // module address range to not to overlap with module's ranges.
+      // UINT64_MAX-1 used as maximal value of an address. UINT64_MAX has
+      // special usage in .debug_ranges so we use UINT64_MAX minus one.
----------------
This bit doesn't quite make sense. Perhaps "We use UINT64_MAX-1 because..."

================
Comment at: lld/test/ELF/gc-sections-debuginfo.s:1
+# REQUIRES: x86
+
----------------
General nit throughout this test: you are using a mixture of '-' and '--' for long switches, which is a bit jarring. Please switch to using '--' throughout, if possible.

This test should test other debug section patching too, as the problem is not specific to .debug_info. The ones I'm thinking of in particular are .debug_types, .debug_ranges, .debug_line and .debug_aranges. Since the behaviour is actually patching of any debug section, perhaps it would be better to just call the section .debug_foo or similar?

================
Comment at: lld/test/ELF/gc-sections-debuginfo.s:4
+# RUN: echo '.section .text,"a"; .byte 0;  .section .debug_info,"", at progbits; .quad .text' | \
+# RUN:   llvm-mc -filetype=obj -arch=x86-64 - -o %t
+# RUN: ld.lld %t -o %t2 --gc-sections
----------------
I don't know what the common format in LLD tests is off the top of my head, but I find it easier if the '|' is on the following line:

```
# RUN: echo .... \
# RUN:   | llvm-mc ...
```

================
Comment at: lld/test/ELF/gc-sections-debuginfo.s:6
+# RUN: ld.lld %t -o %t2 --gc-sections
+# RUN: llvm-readobj -sections  -section-data %t2 | FileCheck %s -check-prefix=CHECK -check-prefix=CHECK64
+
----------------
Nit: spurious extra space between -sections and -section-data

================
Comment at: lld/test/ELF/gc-sections-debuginfo.s:11
+# RUN: ld.lld %t -o %t2 --gc-sections
+# RUN: llvm-readobj -sections  -section-data %t2 | FileCheck %s -check-prefix=CHECK -check-prefix=CHECK32
+
----------------
grimar wrote:
> Hint: you can use `-check-prefixes=CHECK,CHECK32`
Also applies above.

================
Comment at: lld/test/ELF/gc-sections-debuginfo.s:14
+
+# When -Wl,--gc-sections used - linker deletes unused text sections.
+# But their debug info left in the binary. That debug_info could have relocations
----------------
Replace:
"When -Wl,--gc-sections used - linker ..."
with:
"When --gc-sections is used, the linker ..."

================
Comment at: lld/test/ELF/gc-sections-debuginfo.s:15
+# When -Wl,--gc-sections used - linker deletes unused text sections.
+# But their debug info left in the binary. That debug_info could have relocations
+# pointing to deleted sections. Address ranges of such debug_info could overlapp
----------------
sections. But their... -> sections, but their

Use "debug data" instead of "debug_info" throughout here, because .debug_info is just one section in the debug data.

================
Comment at: lld/test/ELF/gc-sections-debuginfo.s:17
+# pointing to deleted sections. Address ranges of such debug_info could overlapp
+# with other address ranges for correct debug_info. To prevent address'es clashing 
+# it is neccessary to resolve such relocations to an address which is out of module 
----------------
address'es -> addresses

================
Comment at: lld/test/ELF/gc-sections-debuginfo.s:18
+# with other address ranges for correct debug_info. To prevent address'es clashing 
+# it is neccessary to resolve such relocations to an address which is out of module 
+# address space. That test checks that relocation from .debug_info section pointing
----------------
out of module -> out of the module

================
Comment at: lld/test/ELF/gc-sections-debuginfo.s:19
+# it is neccessary to resolve such relocations to an address which is out of module 
+# address space. That test checks that relocation from .debug_info section pointing
+# to deleted section .text would be resolved into 0xfffffffffffffffe.
----------------
relocation from -> relocations from the

================
Comment at: lld/test/ELF/gc-sections-debuginfo.s:20
+# address space. That test checks that relocation from .debug_info section pointing
+# to deleted section .text would be resolved into 0xfffffffffffffffe.
+
----------------
to deleted section .text -> to a deleted .text section
into -> to

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

https://reviews.llvm.org/D59553