[ELF] Ensure that .init/.fini sections are padded with nops instead of traps

[arichardson]

Summary

The .init/.fini section is executed from start to finish so inserting traps
between the individual sections will cause programs to crash if the
.init/.fini sections are not filled up to the requested alignment.

I discovered this issue linking FreeBSD for MIPS n64 with LLD. When linking
with BFD 2.17 all binaries ran as expected but LLD was inserting 0xefefefef
instructions into the .fini section so every binary would crash on exit.

[ruiu]

I wonder whether this is a new issue or not. lld used to leave the gap in .init/.fini (or any other sections) as null bytes, so unless your processor handles null bytes as nop instructions, it didn't work before. So, this issue existed before lld started filling gaps with trap instructions. Is my understanding correct?

[arichardson]

In D38167#879015, @ruiu wrote:

I wonder whether this is a new issue or not. lld used to leave the gap in .init/.fini (or any other sections) as null bytes, so unless your processor handles null bytes as nop instructions, it didn't work before. So, this issue existed before lld started filling gaps with trap instructions. Is my understanding correct?

I guess the issue existed but it was not causing problems because null bytes are nops on MIPS (and according to https://en.wikipedia.org/wiki/NOP also x86 and ARM and possibly others).

[ruiu]

At least for x86, null bytes are not nop instructions, so it feels like it just happened to work for MIPS but it wasn't really guaranteed. How did you get a gap in .fini section?

[arichardson]

In D38167#879065, @ruiu wrote:

At least for x86, null bytes are not nop instructions, so it feels like it just happened to work for MIPS but it wasn't really guaranteed. How did you get a gap in .fini section?

Ah yes I was looking at the wrong column in the table, so I guess for x86 I should set the NopInstr to 0x90909090. The problem was that the FreeBSD startup assembly used a .align 4 directive which on MIPS is 2^4 instead of 4 bytes. I fixed this in https://github.com/CTSRD-CHERI/cheribsd/commit/d15dd021ce59f54996ce9c823b2b0d47f8d7039b and will submit it upstream as well. However, if we try to link older versions it will still break and I think this is a very simple fix.

[ruiu]

Well, this is not a bug, so this patch is not a fix but a workaround for existing FreeBSD/MIPS kernel's issue.

So the question is whether or not the issue is so significant and persistent that we need to extend our ABI to make a promise that lld fills gaps in .init/.fini sections with NOP bytes. I think the answer is no in order to make it simple. We shouldn't make any assumption on gap bytes (we shouldn't even assume that they are null or trap instructions) just like we shouldn't use uninitialized variables in C++.

[arichardson]

In D38167#879089, @ruiu wrote:

Well, this is not a bug, so this patch is not a fix but a workaround for existing FreeBSD/MIPS kernel's issue.

So the question is whether or not the issue is so significant and persistent that we need to extend our ABI to make a promise that lld fills gaps in .init/.fini sections with NOP bytes. I think the answer is no in order to make it simple. We shouldn't make any assumption on gap bytes (we shouldn't even assume that they are null or trap instructions) just like we shouldn't use uninitialized variables in C++.

I agree it is a bug in the CRT startup assembly code. However, it can be quite hard to track down where the trap instruction originated from (especially if /sbin/init faults). Would adding a warning or error if .init/.fini requires padding be an acceptable patch?

[arichardson]

LLD should not attempt to workaround bugs in crt*.o files

ELF/OutputSections.h

public:
   ConstraintKind Constraint = ConstraintKind::NoConstraint;
   std::string Location;
   std::string MemoryRegionName;
   bool Noload = false;
 
   template <class ELFT> void finalize();
   template <class ELFT> void writeTo(uint8_t *Buf);
   template <class ELFT> void maybeCompress();
-  uint32_t getFiller();
+  llvm::Optional<uint32_t> getFiller();
 
   void sort(std::function<int(InputSectionBase *S)> Order);
   void sortInitFini();
   void sortCtorsDtors();
 };
 
 int getPriority(StringRef S);
 

ELF/OutputSections.cpp

template <class ELFT> void OutputSection::writeTo(uint8_t *Buf) {
 
   // Write leading padding.
   std::vector<InputSection *> Sections;
   for (BaseCommand *Cmd : Commands)
     if (auto *ISD = dyn_cast<InputSectionDescription>(Cmd))
       for (InputSection *IS : ISD->Sections)
         if (IS->Live)
           Sections.push_back(IS);
-  uint32_t Filler = getFiller();
+  llvm::Optional<uint32_t> Filler = getFiller();
   if (Filler)
-    fill(Buf, Sections.empty() ? Size : Sections[0]->OutSecOff, Filler);
+    fill(Buf, Sections.empty() ? Size : Sections[0]->OutSecOff, *Filler);
 
   parallelForEachN(0, Sections.size(), [=](size_t I) {
     InputSection *IS = Sections[I];
     IS->writeTo<ELFT>(Buf);
 
     // Fill gaps between sections.
     if (Filler) {
       uint8_t *Start = Buf + IS->OutSecOff + IS->getSize();
       uint8_t *End;
       if (I + 1 == Sections.size())
         End = Buf + Size;
       else
         End = Buf + Sections[I + 1]->OutSecOff;
-      fill(Start, End - Start, Filler);
+      fill(Start, End - Start, *Filler);
     }
   });
 
   // Linker scripts may have BYTE()-family commands with which you
   // can write arbitrary bytes to the output. Process them if any.
   for (BaseCommand *Base : Commands)
     if (auto *Data = dyn_cast<BytesDataCommand>(Base))
       writeInt(Buf + Data->Offset, Data->Expression().getValue(), Data->Size);

 // because the compiler keeps the original initialization order in a
 // translation unit and we need to respect that.
 // For more detail, read the section of the GCC's manual about init_priority.
 void OutputSection::sortInitFini() {
   // Sort sections by priority.
   sort([](InputSectionBase *S) { return getPriority(S->Name); });
 }
 
-uint32_t OutputSection::getFiller() {
+llvm::Optional<uint32_t> OutputSection::getFiller() {
   if (Filler)
     return *Filler;
-  if (Flags & SHF_EXECINSTR)
+  if (Flags & SHF_EXECINSTR) {
+    // .init and .fini are run from start to end, so we need to pad it with nops
+    // instead of trap instructions
+    if (Name == ".init" || Name == ".fini")
+      return Target->NopInstr;
     return Target->TrapInstr;
-  return 0;
+  }
+  return None;
 }
 
 template void OutputSection::writeHeaderTo<ELF32LE>(ELF32LE::Shdr *Shdr);
 template void OutputSection::writeHeaderTo<ELF32BE>(ELF32BE::Shdr *Shdr);
 template void OutputSection::writeHeaderTo<ELF64LE>(ELF64LE::Shdr *Shdr);
 template void OutputSection::writeHeaderTo<ELF64BE>(ELF64BE::Shdr *Shdr);
 
 template void OutputSection::writeTo<ELF32LE>(uint8_t *Buf);

ELF/Target.h

public:
   // Set to 0 for variant 2
   unsigned TcbSize = 0;
 
   bool NeedsThunks = false;
 
   // A 4-byte field corresponding to one or more trap instructions, used to pad
   // executable OutputSections.
   uint32_t TrapInstr = 0;
+  // NOP instruction used to pad .init/.fini sections
+  uint32_t NopInstr = 0;
 
   virtual RelExpr adjustRelaxExpr(uint32_t Type, const uint8_t *Data,
                                   RelExpr Expr) const;
   virtual void relaxGot(uint8_t *Loc, uint64_t Val) const;
   virtual void relaxTlsGdToIe(uint8_t *Loc, uint32_t Type, uint64_t Val) const;
   virtual void relaxTlsGdToLe(uint8_t *Loc, uint32_t Type, uint64_t Val) const;
   virtual void relaxTlsIeToLe(uint8_t *Loc, uint32_t Type, uint64_t Val) const;
   virtual void relaxTlsLdToLe(uint8_t *Loc, uint32_t Type, uint64_t Val) const;

test/ELF/Inputs/freebsd-mips-crtn.s

+# Check that aligning the .fini section doesn't insert trap instructions
+
+.section .fini,"ax",%progbits
+.p2align 4
+.set noreorder
+sd $gp, 0($sp)
+sd $ra, 8($sp)
+jr $ra
+daddu $sp, $sp, 32
+.set reorder

test/ELF/fini-padding.s

+# RUN: llvm-mc -filetype=obj -triple=mips64-unknown-freebsd %s -o %t.o
+# RUN: llvm-mc -filetype=obj -triple=mips64-unknown-freebsd %S/Inputs/freebsd-mips-crtn.s -o %t-crtn.o
+# RUN: ld.lld -Bstatic %t.o %t-crtn.o -o %t.exe
+# RUN: llvm-readobj -s %t.exe | FileCheck %s -check-prefix SECTIONS
+# RUN: llvm-objdump -d -p -h %t.exe | FileCheck %s
+
+# LLD was inserting trap instructions into FreeBSD binaries .init/.fini sections
+# The crtn.s file wrongly aligned .init to 2**4 instead of 4 so lld was filling
+# in padding with trap instructions
+
+# Even though the FreeBSD code was wrong we should still not insert trap
+# instructions into .init/.fini. It contains concatenated snippets of code that
+# runs in order. We should insert nops here the same way BFD does it
+
+.text
+.global __start
+__start:
+  jr $ra
+  nop
+
+.global dtor
+dtor:
+  jr $ra
+  nop
+
+.section .fini,"ax",%progbits
+.set noreorder
+dla $t9, dtor
+li $a0, 42
+jalr $t9
+nop
+li $a0, 0x1234
+
+.section .init,"ax",%progbits
+.p2align 4
+.set noreorder
+li $a0, 42
+
+.section .init,"ax",%progbits
+.p2align 4
+.set noreorder
+li $a0, 43
+
+
+# SECTIONS:      Section {
+# SECTIONS:           Name: .fini (36)
+# SECTIONS-NEXT:      Type: SHT_PROGBITS (0x1)
+# SECTIONS-NEXT:      Flags [ (0x6)
+# SECTIONS-NEXT:        SHF_ALLOC (0x2)
+# SECTIONS-NEXT:        SHF_EXECINSTR (0x4)
+# SECTIONS-NEXT:      ]
+# SECTIONS-NEXT:      Address: 0x20020
+# SECTIONS-NEXT:      Offset: 0x10020
+# SECTIONS-NEXT:      Size: 64
+# SECTIONS-NEXT:      Link: 0
+# SECTIONS-NEXT:      Info: 0
+# SECTIONS-NEXT:      AddressAlignment: 16
+# SECTIONS-NEXT:      EntrySize: 0
+# SECTIONS-NEXT:    }
+
+
+# We don't want any trap instructions inserted between the .fini and .init sections
+
+# CHECK-LABEL: Disassembly of section .fini:
+# CHECK-NEXT: .fini:
+# CHECK-NEXT: 20020: 3c 19 00 00     lui     $25, 0
+# CHECK:      2003c: 03 20 f8 09     jalr    $25
+# CHECK-NEXT: 20040: 00 00 00 00     nop
+# CHECK-NEXT: 20044: 24 04 12 34     addiu   $4, $zero, 4660
+# The following two instructions were previously filled with traps:
+# CHECK-NEXT: 20048: 00 00 00 00     nop
+# CHECK-NEXT: 2004c: 00 00 00 00     nop
+# now the code from crtn.o:
+# CHECK-NEXT: 20050: ff bc 00 00     sd      $gp, 0($sp)
+# CHECK-NEXT: 20054: ff bf 00 08     sd      $ra, 8($sp)
+# CHECK-NEXT: 20058: 03 e0 00 08     jr      $ra
+# CHECK-NEXT: 2005c: 67 bd 00 20     daddiu  $sp, $sp, 32
+
+
+# CHECK-LABEL: Disassembly of section .init:
+# CHECK-NEXT: .init:
+# CHECK-NEXT: 24 04 00 2a     addiu   $4, $zero, 42
+# CHECK-NEXT: 00 00 00 00     nop
+# CHECK-NEXT: 00 00 00 00     nop
+# CHECK-NEXT: 00 00 00 00     nop
+# CHECK-NEXT: 24 04 00 2b     addiu   $4, $zero, 43