[ELF] - Fix for "LLD crashes with --emit-relocs when trying to proccess .eh_frame"

[grimar]

Summary

This fixes PR36367 which is about segfault when --emit-relocs is
used together with .eh_frame sections.

That happens because we reorder .eh_frame sections and corresponding
.rela.eh_frame sections when combining .eh_frames.
Usually, objects do not do that and we error out on such objects (I added the test case),
so the situation with .eh_frame is unique.

The patch fixes it.

[dstolfa]

Seems to apply cleanly and works on FreeBSD. Thanks @grimar!

[• espindola]

ELF/Writer.cpp

161

This is a useful invariant. Could we do this even when Config->EmitRelocs is false?

1391

This comment is now out of date.

test/ELF/emit-relocs-eh-frame.s

17

I think you can delete the .size and the .Lfunc_end0.

[• espindola]

The crash is from

// 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();

Where Out is null because we call this for the relocation section before .eh_frame. The caller is

std::vector<OutputSection *> V;
for (InputSectionBase *S : InputSections) {
  if (!S->Live || S->Parent)
    continue;

  StringRef Name = getOutputSectionName(S);

So this suggests two options that might be simpler.

• Just handle Out being null. We are just trying to get a better looking output section name. It is not critical.
• Do two passes over the input section in LinkerScript::addOrphanSections.

[dstolfa]

In D44622#1042429, @espindola wrote:

The crash is from

// 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();

Where Out is null because we call this for the relocation section before .eh_frame. The caller is

std::vector<OutputSection *> V;
for (InputSectionBase *S : InputSections) {
  if (!S->Live || S->Parent)
    continue;

  StringRef Name = getOutputSectionName(S);

So this suggests two options that might be simpler.

• Just handle Out being null. We are just trying to get a better looking output section name. It is not critical.
• Do two passes over the input section in LinkerScript::addOrphanSections.

This is the approach I've taken in https://reviews.llvm.org/D44601 (though it was still a workaround at that point). @arichardson pointed out that we should probably have an output similar to ld.bfd and gold, so I suppose the fix should be guided by that as well as where it would be best to handle this case?

[• rafael]

Domagoj Stolfa via Phabricator via llvm-commits
<[e-mail]> writes:

dstolfa added a comment.

In https://reviews.llvm.org/D44622#1042429, @espindola wrote:

The crash is from

// 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();

Where Out is null because we call this for the relocation section before .eh_frame. The caller is

std::vector<OutputSection *> V;
for (InputSectionBase *S : InputSections) {
  if (!S->Live || S->Parent)
    continue;

  StringRef Name = getOutputSectionName(S);

So this suggests two options that might be simpler.

• Just handle Out being null. We are just trying to get a better looking output section name. It is not critical.
• Do two passes over the input section in LinkerScript::addOrphanSections.

This is the approach I've taken in https://reviews.llvm.org/D44601 (though it was still a workaround at that point). @arichardson pointed out that we should probably have an output similar to ld.bfd and gold, so I suppose the fix should be guided by that as well as where it would be best to handle this case?

Instead of returning "" it could return S->Name and that would handle
all real world cases I think.

I think doing two passes in LinkerScript::addOrphanSections might be the
best solution as it is simple and handles even synthetic tests like using
a linker script to assign .eh_frame to an output section with a
different name.

Cheers,
Rafael

[dstolfa]

In D44622#1042505, @rafael wrote:

Domagoj Stolfa via Phabricator via llvm-commits
<[e-mail]> writes:

dstolfa added a comment.

In https://reviews.llvm.org/D44622#1042429, @espindola wrote:

The crash is from

// 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();

Where Out is null because we call this for the relocation section before .eh_frame. The caller is

std::vector<OutputSection *> V;
for (InputSectionBase *S : InputSections) {
  if (!S->Live || S->Parent)
    continue;

  StringRef Name = getOutputSectionName(S);

So this suggests two options that might be simpler.

• Just handle Out being null. We are just trying to get a better looking output section name. It is not critical.
• Do two passes over the input section in LinkerScript::addOrphanSections.

This is the approach I've taken in https://reviews.llvm.org/D44601 (though it was still a workaround at that point). @arichardson pointed out that we should probably have an output similar to ld.bfd and gold, so I suppose the fix should be guided by that as well as where it would be best to handle this case?

Instead of returning "" it could return S->Name and that would handle
all real world cases I think.

I think doing two passes in LinkerScript::addOrphanSections might be the
best solution as it is simple and handles even synthetic tests like using
a linker script to assign .eh_frame to an output section with a
different name.

Cheers,
Rafael

I believe the change I've just added in the review fixes the issue in the most minimal way possible.

[grimar]

In D44622#1042505, @rafael wrote:

I think doing two passes in LinkerScript::addOrphanSections might be the
best solution as it is simple and handles even synthetic tests like using
a linker script to assign .eh_frame to an output section with a
different name.

Cheers,
Rafael

I tried to do this while investigated the ways to fix an issue.
What I did not like with such approach was:

1. Amount of changes was larger than in this patch.
2. Changes were in a generic code, but --emit-relocs + .eh_frame is a very specific corner case.

And I think it can not be used for something else because we
explicitly do not support reordered objects.
So I tried to find a way to isolate the fix from the common flow.
(like this patch do)

I'll post the patch for above to demonstrate results I had though.

I also tried one more way: during combining of eh_frames I moved
synthetic EhFrame in place of the first .eh_frame. With that, it also guaranteed
the proper order, though broke a large number of test cases and particularly
moved EhFrame before EhFrameHdr. So I refused from such approach too.

[grimar]

Abandoning, going to commit D44679.

ELF/Writer.cpp

if (!ES || !ES->Live)
       continue;
 
     InX::EhFrame->addSection<ELFT>(ES);
     S = nullptr;
   }
 
   std::vector<InputSectionBase *> &V = InputSections;
   V.erase(std::remove(V.begin(), V.end(), nullptr), V.end());
+
+  // For futher --emit-reloc handling code we need target output section to be
+  // created before we create relocation output section, so we want target to be
+  // listed first. We do not support objects that break this order, but code
+  // above do that, because InX::EhFrame synthetic section is placed after
+  // all other regular sections. Here we want to sort sections list to restore
+  // this general ordering rule. We do not need or want to move synthetic
+  // sections because them are special.
+  if (Config->EmitRelocs)
+    std::stable_partition(V.begin(), V.end(), [](InputSectionBase *IS) {
+      return (IS->Type != SHT_REL && IS->Type != SHT_RELA) ||
+             isa<SyntheticSection>(IS);
+    });
 }
 
 static Defined *addOptionalRegular(StringRef Name, SectionBase *Sec,
                                    uint64_t Val, uint8_t StOther = STV_HIDDEN,
                                    uint8_t Binding = STB_GLOBAL) {
   Symbol *S = Symtab->find(Name);
   if (!S || S->isDefined())
     return nullptr;

 // Doing it has an unintended side effects. If it turns out that we
 // don't need a .got (for example) at all because there's no
 // relocation that needs a .got, we don't want to emit .got.
 //
 // To deal with the above problem, this function is called after
 // scanRelocations is called to remove synthetic sections that turn
 // out to be empty.
 static void removeUnusedSyntheticSections() {
   // All input synthetic sections that can be empty are placed after
   // all regular ones. We iterate over them all and exit at first
   // non-synthetic.
-  for (InputSectionBase *S : llvm::reverse(InputSections)) {
+  for (InputSectionBase *S : InputSections) {
     SyntheticSection *SS = dyn_cast<SyntheticSection>(S);
-    if (!SS)
-      return;
-    OutputSection *OS = SS->getParent();
+    OutputSection *OS = SS ? SS->getParent() : nullptr;
     if (!OS || !SS->empty())
       continue;
 
     // If we reach here, then SS is an unused synthetic section and we want to
     // remove it from corresponding input section description of output section.
     for (BaseCommand *B : OS->SectionCommands)
       if (auto *ISD = dyn_cast<InputSectionDescription>(B))
         llvm::erase_if(ISD->Sections,

test/ELF/emit-relocs-eh-frame.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t1.o
+# RUN: ld.lld --emit-relocs %t1.o -o %t
+# RUN: llvm-readobj -r %t | FileCheck %s
+
+# CHECK:      Relocations [
+# CHECK-NEXT:   Section {{.*}} .rela.eh_frame {
+# CHECK-NEXT:     0x{{.*}} R_X86_64_PC32 .text 0x0
+# CHECK-NEXT:   }
+# CHECK-NEXT: ]
+
+.text
+.globl foo
+foo:
+ .cfi_startproc
+ .Lfunc_end0:
+ .size foo, .Lfunc_end0-foo
+ .cfi_endproc

test/ELF/emit-relocs-shared.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
 # RUN: ld.lld --hash-style=sysv --emit-relocs %t.o -o %t.so -shared
 # RUN: llvm-readobj -r %t.so | FileCheck %s
 
 .data
 .quad foo
 
 # CHECK:      Relocations [
-# CHECK-NEXT:   Section (4) .rela.dyn {
+# CHECK-NEXT:   Section {{.*}} .rela.dyn {
 # CHECK-NEXT:     0x1000 R_X86_64_64 foo 0x0
 # CHECK-NEXT:   }
-# CHECK-NEXT:   Section (8) .rela.data {
+# CHECK-NEXT:   Section {{.*}} .rela.data {
 # CHECK-NEXT:     0x1000 R_X86_64_64 foo 0x0
 # CHECK-NEXT:   }
 # CHECK-NEXT: ]

test/ELF/emit-relocs.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t1.o
 # RUN: ld.lld --emit-relocs %t1.o -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-readobj -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: .rela.text
+# CHECK:        Index:
+# CHECK:        Name: .rela.text
 # CHECK-NEXT:   Type: SHT_RELA
 # CHECK-NEXT:   Flags [
 # CHECK-NEXT:     SHF_INFO_LINK
 # CHECK-NEXT:   ]
 # CHECK:      Relocations [
 # CHECK-NEXT:   Section ({{.*}}) .rela.text {
 # CHECK-NEXT:     0x201002 R_X86_64_32 .text 0x1
 # CHECK-NEXT:     0x201007 R_X86_64_PLT32 fn 0xFFFFFFFFFFFFFFFC

test/ELF/invalid/emit-relocs-reordered.test

+# REQUIRES: x86
+
+# RUN: yaml2obj %s -o %t.o
+# RUN: not ld.lld %t.o -o %t.exe 2>&1 | FileCheck %s
+# CHECK: unsupported relocation reference
+
+## YAML below lists .rela.text before .text, we do not support it.
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  OSABI:           ELFOSABI_FREEBSD
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:
+  - Type:            SHT_REL
+    Name:            .rela.text
+    Link:            .symtab
+    Info:            .text
+    AddressAlign:    0x04
+    Relocations:
+      - Offset:          0
+        Symbol:          .text
+        Type:            R_X86_64_NONE
+  - Type:            SHT_PROGBITS
+    Name:            .text
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x04
+    Content:         "FFFFFFFFFFFFFFFF"