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

[George Rimar via Phabricator via llvm-commits]

grimar 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),
----------------
bd1976llvm wrote:
> jhenderson wrote:
> > 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.
> Checking the section name (not symbol name) is not *hacky* in this case. Section names that start with the prefix ".debug" are reserved "for the system". Checking that the section name starts with the prefix ".debug" is the correct ELF way to determine if it is a debug information section. 
But testing the symbol name inside a hot relocation loop and changing the behavior depending on that is itself a bit hacky, isn't?
My main point I think is that if there is a chance we can just do not do it - I would try it. If that will not work for some reason - then it needs a good comment and also a test case explaining why the generic approach does not work.


================
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
----------------
avl wrote:
> jhenderson wrote:
> > Could you indent this like you were before, please? Same below.
> I did not get it. What exactly should be done ? 
> 
> this :
> 
> # RUN: echo '.section .text,"a"; .byte 0;  .section .debug_foo,"", at progbits; .quad .text' \
> # RUN: | llvm-mc --filetype=obj --arch=x86-64 - -o %t
> 
> should be change into this ?
> 
> # RUN: echo '.section .text,"a"; .byte 0;  .section .debug_foo,"", at progbits; .quad .text'  | \
> # RUN: llvm-mc --filetype=obj --arch=x86-64 - -o %t
I think James wanted you to add a double space (identation) before `llvm-mc`:

```
# RUN: echo '.section .text,"a"; .byte 0;  .section .debug_foo,"", at progbits; .quad .text' \
# RUN:   | llvm-mc --filetype=obj --arch=x86-64 - -o %t
```


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

https://reviews.llvm.org/D59553