[PATCH] D46502: [ELF] - Fix for "LLD can create incorrect debug PC ranges for functions in Comdat groups."

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

If I've understood correctly (from test/ELF/comdat.s) this will affect relocations from all sections to discarded local symbols. If I'm right I think that this is too wide a use case. My understanding is that in gold and bfd this case is specifically for relocations from Dwarf debug sections to discarded local symbols. Relocations from non-dwarf debug sections to discarded sections will be either an error or ignored depending on the use case.

For example
file1.s

          .section .text.1, "axG", %progbits, foo, comdat, unique, 0
  foo:    nop
  
          .section .text.2, "ax", %progbits
          .globl _start
  _start:        
          .quad foo
  
          .section .debug_info
          .quad foo

file2.s

          .section .text.1, "axG", %progbits, foo, comdat, unique, 0
  foo:    nop
  
          .section .text.3, "ax", %progbits
          .globl func
  func:
          // This is a reference to a local definition inside group foo, ld.bfd will give an error message, ld.gold a warning, here when group is discarded, LLD will not.
          .quad foo
          nop
  
          .section .debug_info
          // This is a reference to a local definition inside group foo from a debug section, relocation is resolved to foo in kept group
          .quad foo
          // This is a reference to a local definition inside group foo, but does not exist in group foo in file1.s, resolved to 0.

llvm-mc -triple=x86_64-pc-linux t.s -filetype=obj -o t.o
llvm-mc -triple=x86_64-pc-linux t2.s -filetype=obj -o t2.o
ld.bfd t.o t2.o -o t.exe
`.text.1' referenced in section `.text.3' of t2.o: defined in discarded section `.text.1[foo]' of t2.o
ld.gold t.o t2.o -o t.exe
t2.o(.text.3+0x0): warning: relocation refers to discarded section
ld.lld t.o t2.o -o t.exe
// no errors

So I think that we are potentially doing two things wrong here:

- I think we should be giving a warning or error when there are relocations from non-debug sections to discarded local symbols.
- If we have a relocation from a debug section to a discarded local symbol, we should use the definition of the symbol from the section in the kept group or 0 if there is no such symbol.

Could this be implemented by something like:

  If (relocation to local defined in discarded group section) {
    if (section containing relocation is debug) {
      if (find local definition in kept group)
          use that definition.
      else
          0   
    }
    else
       warn or error





================
Comment at: ELF/InputFiles.cpp:439
+      if (It.second /* IsNew */) {
+        if (Entries.size() == 1)
+          It.first->second = &this->Sections[Entries[0]];
----------------
Unfortunately I don't think that this will work on all targets. For example with --target=armv7a-linux-gnueabihf clang puts the exception table information (.ARM.exdix.name_of_function> and .rel.ARM.exidx.text.name_of_function) in the same group as name_of_function. I think it will be possible to select _a_ candidate section for these groups by choosing the section with SHF_EXECINSTR. 


================
Comment at: ELF/InputFiles.cpp:466
+      InputSectionBase **SingleLeaderSec = It.first->second;
+      if (Entries.size() == 1 && SingleLeaderSec) {
+        InputSection *IS = createDiscardedProxy(*SingleLeaderSec);
----------------
Same comment about number of entries as above.


================
Comment at: ELF/InputSection.cpp:298
+    return true;
+  return IS && !IS->File && !IS->Flags && !IS->Type && IS->Data.empty() &&
+         IS->Name.empty();
----------------
I'm guessing this is to match the createDiscardedProxy? I'm thinking that it might be better to extract that into a isDiscardedProxy() function to make it clearer. Might be worth checking that Repl is non null as well.


================
Comment at: ELF/InputSection.h:53
+  // when creating discarded proxy section. See createDiscardedProxy().
   SectionBase *Repl;
 
----------------
What happens if the Repl section for a discardedProxy is merged by ICF? Can we have a Repl -> Repl -> Repl needing to follow to the end of the Repl chain to find the candidate section. 


================
Comment at: test/ELF/comdat-debug.s:1
+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
----------------
We should add a test for ld -r as well as that could easily get broken in the future.


================
Comment at: test/ELF/comdat.s:32
 // 0x1000 - 0x1001 - 5 = -6
-// 0      - 0x1006 - 5 = -4107
+// 0x1000 - 0x1006 - 5 = -11
 // CHECK-NEXT:   1001:	{{.*}}  callq  -6
----------------
I think this should be an error or warning (see main comment)


https://reviews.llvm.org/D46502