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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
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),
----------------
avl wrote:
> grimar wrote:
> > jhenderson wrote:
> > > 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.
> > Hmmm... I do not agree here. I think if the user's section assumes that linker resolves the relocation to a removed section as a zero, that does not seem the correct assumption to me. I doubt we should care that we switched the behavior here (or at least we probably want to know about such cases before explicitly supporting them, I think).
> In some previous version of the patch there was a check for Name.startswith(".debug") which I was asked to remove because "checking the symbol name is very hacky". Would it be OK to return it here ? Or probably solution similar to https://reviews.llvm.org/D54747 would be OK ? (create a flag Debug, set it Debug = Name.startswith(".debug"), and check it here)
I don't feel strongly about it, so if there's opposition, what is here is probably fine, I think.


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


================
Comment at: lld/test/ELF/gc-sections-debuginfo.s:4
+# RUN: echo '.section .text,"a"; .byte 0;  .section .debug_foo,"", at progbits; .quad .text' \
+# RUN: | llvm-mc --filetype=obj --arch=x86-64 - -o %t
+# RUN: ld.lld %t -o %t2 --gc-sections
----------------
Could you indent this like you were before, please? Same below.


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


================
Comment at: lld/test/ELF/gc-sections-debuginfo.s:16
+# but their debug info left in the binary. That debug data could have relocations
+# pointing to deleted sections. Address ranges of such debug data could overlapp
+# with other address ranges for correct debug data. To prevent addresses clashing 
----------------
overlapp -> overlap


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

https://reviews.llvm.org/D59553