[ELF] Emit a diagnostic if there's reference to a local symbol in a discarded COMDAT section

[davide]

Summary

Continuing the discussion from: https://reviews.llvm.org/D24750

[ruiu]

This patch adds an error check but the erroneous relocations are processed as before. The relocations points to the Discarded section, so I think the latter process cannot handle them properly. How did it work in the first place?

[davide]

I'm not sure it did. This is the disassembly of the generated lld output (in particular the second callq).

Disassembly of section .text2:
foo:
    1000:       90      nop
Disassembly of section bar:
bar:
    1001:       e8 fa ff ff ff  callq   -6 <foo>
    1006:       e8 f5 ef ff ff  callq   -4107

This is, instead, what gold generates:

./patatino:     file format elf64-x86-64


Disassembly of section .text2:

00000000000001c4 <foo>:
 1c4:   90                      nop

Disassembly of section bar:

00000000000001c5 <bar>:
 1c5:   e8 fa ff ff ff          callq  1c4 <foo>
 1ca:   e8 31 fe ff ff          callq  0 <foo-0x1c4>

I'm not quite sure we ought to produce a valid output, given that the input is not a valid ELF.

[davide]

IMHO, emitting this warning is a good idea as it informs people that their object are not ELF-compliant. I also think it's very unfortunate we can't turn this into an error (at least not in its current form) because .eh_frame relies on this behavior to work correctly.

[• rafael]

Is this producing a warning for know broken special cases like .eh_frame and .debug_line?

[davide]

In D24837#550970, @rafael wrote:

Is this producing a warning for know broken special cases like .eh_frame and .debug_line?

Yes, do we want to skip them?

[davide]

I think that when we arrive at this point we don't have anymore this information (if it comes from .eh_frame or something else), so we might need to record this earlier.

ELF/Relocations.cpp

       continue;
 
     // Skip a relocation that points to a dead piece
     // in a eh_frame section.
     while (PieceI != PieceE &&
            (PieceI->InputOff + PieceI->size() <= RI.r_offset))
       ++PieceI;
 
+    // If this a reference to a local symbol in a discarded COMDAT section,
+    // emit a warning.
+    if (Body.isLocal()) {
+      auto *D = dyn_cast<DefinedRegular<ELFT>>(&Body);
+      if (D && D->Section == &InputSection<ELFT>::Discarded)
+        warning("relocation from " + File.getName() + " with offset " +
+                Twine(RI.r_offset) + " refers to discarded section");
+    }
+
     // Compute the offset of this section in the output section. We do it here
     // to try to compute it only once.
     uintX_t Offset;
     if (PieceI != PieceE) {
       assert(PieceI->InputOff <= RI.r_offset && "Relocation not in any piece");
       if (PieceI->OutputOff == (size_t)-1)
         continue;
       Offset = PieceI->OutputOff + RI.r_offset - PieceI->InputOff;

test/ELF/comdat.s

 // RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
 // RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %p/Inputs/comdat.s -o %t2.o
-// RUN: ld.lld -shared %t.o %t.o %t2.o -o %t
+// RUN: ld.lld -shared %t.o %t.o %t2.o -o %t 2>&1 | FileCheck --check-prefix=WARN %s
 // RUN: llvm-objdump -d %t | FileCheck %s
 // RUN: llvm-readobj -s -t %t | FileCheck --check-prefix=READ %s
 // REQUIRES: x86
 
         .section  .text2,"axG",@progbits,foo,comdat,unique,0
 foo:
         nop
 
+// WARN: relocation from {{.*}}comdat.s{{.*}}.o with offset 1 refers to discarded section
+
 // CHECK: Disassembly of section .text2:
 // CHECK-NEXT: foo:
 // CHECK-NEXT:   1000: {{.*}}  nop
 // CHECK-NOT: nop
 
         .section bar, "ax"
         call foo