LLD: Avoid segfault with --emit-relocs

[dstolfa]

Summary

When passing an --emit-relocs flag to lld, there seems to be a possibility for the output section to be NULL. This causes lld to crash which results in the executable not being generated. This patch works around this issue by simply checking if the output section exists and returns "" as the name if it doesn't. The check of the return value then prevents propagation of "" in other bits of the code.

I've inspected a number of resulting ELF files generated with this and it looks correct and it breaks no additional tests.

[dstolfa]

Generate a minimal test case for the breakage. It seems like the root cause of this are the cfi directives.

[arichardson]

ELF/Writer.cpp

101

It seems like the problem here is that when this is called with .rela.eh_frame getRelocatedSection() returns an EhInputSection where ->getParent() returns null.

If I link the test file with ld.bfd it contains relocations so we should probably be emitting the same output and not just omit the relocations:

Relocations [
  Section (3) .rela.eh_frame {
    0x4000A0 R_X86_64_PC32 .text 0x0
  }
]

test/ELF/emit-relocs-freebsd.s

2

Possibly add a llvm-readobj invocation here to show that the input file contains relocations:

Relocations [
  Section (4) .rela.eh_frame {
    0x20 R_X86_64_PC32 .text 0x0
  }
]

6

I don't there is much point in testing the aliases for --emit-relocs here. It is already done in emit-relocs.s

8

This line is not actually running llvm-readobj

24

I don't think we need to check the symbols in this test. This is about not crashing on relocations in .eh_frame and --emit-relocs

[dstolfa]

I've verified the breakage on Linux as well. It seems to be caused by InX::EhFrame->getParent() being nullptr when EmitRelocs is set. This is because near the start of the InputSection vector, there are additional input sections that end up pointing to InX::EhFrame as the parent and getOutputSectionName returns nullptr.

[dstolfa]

Bump the diff to reflect things that @arichardson has helped me find.

The relocations are still not checked in the test itself because it's not clear what the section number should be yet (I'm getting different outputs from different linkers at the moment).

This diff removes the unnecessary duplicate runs that are checked by emit-relocs.s already and renames emit-relocs-freebsd.s to emit-relocs-eh.s as the breakage is not unique to FreeBSD. It does not provide a correct fix yet and still just works around the issue by simply ignoring the problematic entries.

[grimar]

It is https://bugs.llvm.org//show_bug.cgi?id=36367

[dstolfa]

Closing this due to a better patch in https://reviews.llvm.org/D44622.

[dstolfa]

Fix the issue and add a sensible test.

EDIT: This breaks a couple of tests (my environment didn't update the first time around).

[dstolfa]

I believe that this fully fixes the issue. It adds a check for Config->Relocatable and checks for OutsecName correctly.

All tests pass.

[dstolfa]

Import the test case from @grimar's D44679 to verify that this patch works with the linkerscript test as well.

Thanks for thinking of this case!

[• espindola]

This was fixed by r328299, right?

[dstolfa]

In D44601#1047427, @espindola wrote:

This was fixed by r328299, right?

Correct. I seem to have missed the commit when it happened so it remained open. Closing this diff because of the commit.

ELF/LinkerScript.cpp

void LinkerScript::addOrphanSections() {
   StringMap<OutputSection *> Map;
 
   std::vector<OutputSection *> V;
   for (InputSectionBase *S : InputSections) {
     if (!S->Live || S->Parent)
       continue;
 
     StringRef Name = getOutputSectionName(S);
+    if (Name == "")
+      continue;
 
     if (Config->OrphanHandling == OrphanHandlingPolicy::Error)
       error(toString(S) + " is being placed in '" + Name + "'");
     else if (Config->OrphanHandling == OrphanHandlingPolicy::Warn)
       warn(toString(S) + " is being placed in '" + Name + "'");
 
     if (OutputSection *Sec =
             findByName(makeArrayRef(SectionCommands).slice(0, End), Name)) {

ELF/Writer.cpp

StringRef elf::getOutputSectionName(InputSectionBase *S) {
 
   // This is for --emit-relocs. If .text.foo is emitted as .text.bar, we want
   // to emit .rela.text.foo as .rela.text.bar for consistency (this is not
   // technically required, but not doing it is odd). This code guarantees that.
   if ((S->Type == SHT_REL || S->Type == SHT_RELA) &&
       !isa<SyntheticSection>(S)) {
     OutputSection *Out =
         cast<InputSection>(S)->getRelocatedSection()->getOutputSection();
+    if (!Out)
+      return "";
     if (S->Type == SHT_RELA)
       return Saver.save(".rela" + Out->Name);
     return Saver.save(".rel" + Out->Name);
   }
 
   for (StringRef V :
        {".text.", ".rodata.", ".data.rel.ro.", ".data.", ".bss.rel.ro.",
         ".bss.", ".init_array.", ".fini_array.", ".ctors.", ".dtors.", ".tbss.",

test/ELF/emit-relocs-freebsd.s

+# REQUIRES: x86
+# RUN: llvm-mc -triple=x86_64-unknown-freebsd -filetype=obj %s -o %t1.o
+# RUN: ld.lld %t1.o --emit-relocs -o %t
+# RUN: llvm-readobj -t -r -s %t | FileCheck %s
+
+## Check single dash form.
+# RUN: ld.lld -emit-relocs %t1.o -o %t1
+# RUN llvm-read-obj -t -r -s %t1 | FileCheck %s
+
+## Check alias.
+# RUN: ld.lld -q %t1.o -o %t2
+# RUN: llvm-readobj -t -r -s %t2 | FileCheck %s
+
+# CHECK:        Section {
+# CHECK:          Index: 2
+# CHECK-NEXT:     Name: .text
+# CHECK-NEXT:     Type: SHT_PROGBITS
+# CHECK-NEXT:     Flags [
+# CHECK-NEXT:       SHF_ALLOC
+# CHECK-NEXT:       SHF_EXECINSTR
+# CHECK-NEXT:     ]
+# CHECK:        Relocations [
+# CHECK-NEXT:   ] 
+# CHECK-NEXT:   Symbols [
+# CHECK-NEXT:     Symbol {
+# CHECK-NEXT:       Name:
+# CHECK-NEXT:       Value: 0x0
+# CHECK-NEXT:       Size: 0
+# CHECK-NEXT:       Binding: Local
+# CHECK-NEXT:       Type: None
+# CHECK-NEXT:       Other: 0
+# CHECK-NEXT:       Section: Undefined
+# CHECK-NEXT:     }
+# CHECK-NEXT:     Symbol {
+# CHECK-NEXT:       Name:
+# CHECK-NEXT:       Value: 0x201000
+# CHECK-NEXT:       Size: 0
+# CHECK-NEXT:       Binding: Local
+# CHECK-NEXT:       Type: Section
+# CHECK-NEXT:       Other: 0
+# CHECK-NEXT:       Section: .text
+# CHECK-NEXT:     }
+# CHECK-NEXT:     Symbol {
+# CHECK-NEXT:       Name:
+# CHECK-NEXT:       Value: 0x0
+# CHECK-NEXT:       Size: 0
+# CHECK-NEXT:       Binding: Local
+# CHECK-NEXT:       Type: Section
+# CHECK-NEXT:       Other: 0
+# CHECK-NEXT:       Section: .comment
+# CHECK-NEXT:     }
+# CHECK-NEXT:     Symbol {
+# CHECK-NEXT:       Name: _start
+# CHECK-NEXT:       Value: 0x201000
+# CHECK-NEXT:       Size: 0
+# CHECK-NEXT:       Binding: Global
+# CHECK-NEXT:       Type: None
+# CHECK-NEXT:       Other: 0
+# CHECK-NEXT:       Section: .text
+# CHECK-NEXT:     }
+# CHECK-NEXT:   ]
+
+.text
+.globl _start
+_start:
+.cfi_startproc
+  nop
+.cfi_endproc