[ELF] Fix placement of a sentinel entry in the .ARM.exidx section.

[ikudrin]

Summary

If an output section description for .ARM.exidx ends with an assignment statement,
the value of the symbol should include the sentinel entry.

[peter.smith]

This looks sensible to me. One minor nit with the comment as the ABI does not mention a sentinel. We add it as some recent versions of libunwind depend on it being there (due to gold and ld.bfd generating one).

ELF/Writer.cpp

1187

Strictly speaking the ABI doesn't require the .ARM.exidx to be terminated by a sentinel, but without one it does mean that the address range of the last entry would only be terminated by the end of the address space. Until recently libunwind depended on the presence of a sentinel.

[ruiu]

ELF/OutputSections.h

85

I don't think we should change this function just fixing an edge case for .ARM.exidx and linker script. Isn't there any way that is less intrusive? Also, please avoid using a parameter with a default value in lld as much as possible.

[ruiu]

I think a better way would be to add a terminator before processing linker scripts.

[ikudrin]

• Add a sentinel entry earlier so that it can be arranged in a regular way.

[ikudrin]

Thanks for the comments!

• Rebased on the top to embrace changes to deduplicate entries.
• Implemented method {{empty()}} to remove a sentinel if there are no other entries.
• Added a test to check that a sentinel is not removed by deduplication.

}

+ if (Sections.empty())
+ continue;

This is never true because of the sentinel, no?

I still have similar checks as a precaution for malicious linker scripts. See {{arm-data-prel.s}} as the example.

[ikudrin]

Thanks for the review, @rafael!

• RUN: ld.lld --no-merge-exidx-entries -T %t.script %t.o -shared -o %t.so
• RUN: llvm-objdump -s -triple=armv7a-none-linux-gnueabi %t.so | FileCheck %s

+// REQUIRES: arm

Why the change from // to #?

I see that you have unified the comment style, so, in this update I use the same, minimizing the diff.

+// RUN: llvm-objdump -s -triple=armv7a-none-linux-gnueabi %t.so | FileCheck %s --check-prefix=DUMP

Please use a prefix on the new FileCheck run instead to reduce the diff
(or commit the prefix change first).

Fixed.

+ if (!Sections.empty() && isa<ARMExidxSentinelSection>(Sections.back())) {

All tests pass if I move the !Sections.empty() to an assert.

As removeUnusedSyntheticSections() is not perfect, I can cook up a malicious script like this:

--- a/test/ELF/arm-data-prel.s
+++ b/test/ELF/arm-data-prel.s
@@ -1,7 +1,8 @@
 // RUN: llvm-mc %s -triple=armv7-unknown-linux-gnueabi -filetype=obj -o %t.o
 // RUN: echo "SECTIONS { \
-// RUN:          .text : { *(.text) } \
-// RUN:          .prel.test : { *(.ARM.exidx) } \
+// RUN:          .text : { *(.text.0) } \
+// RUN:          .prel.test : { *(.ARM.exidx.0) } \
+// RUN:          .trap : { *(.ARM.exidx) *(.dummy) } \
 // RUN:          .prel.test.TEST1 : { *(.ARM.exidx.TEST1) } \
 // RUN:          .TEST1 : { *(.TEST1) } } " > %t.script
 // RUN: ld.lld --script %t.script %t.o -o %t
@@ -32,7 +33,7 @@ _start:
  mov     pc, lr
  .fnend
 
- .section .text, "ax",%progbits
+ .section .text.0, "ax",%progbits
 // The generated .ARM.exidx section will refer to the personality
 // routine __aeabi_unwind_cpp_pr0. Provide a dummy implementation
 // to stop an undefined symbol error

Generally, I prefer my code to be safe. Although I must admit, that, with this script, lld still crashes, just a bit later, in OutputSection::finalize().

+ARMExidxSentinelSection *ARMExidxSentinelSection::create() {
+ if (Config->Relocatable)
+ return nullptr;
+
+ return make<ARMExidxSentinelSection>();
+}

This is now trivial, please inline.

Done.

LGTM with that.

Thanks! I'll commit this if there will be no further comments.

[ruiu]

lld/trunk/ELF/SyntheticSections.cpp

2573 ↗

(On Diff #127666)

It looks like this function returns true if the parent section of this section is empty. That's different from what you'd expect for emptyfunction. empty should return true if this is empty, not other object.

lld/trunk/ELF/SyntheticSections.h

790 ↗

(On Diff #127666)

= 0 is too C-ish. Please use nullptr. Also this needs comment because it is not obvious what this member is. What is Highest?

ELF/OutputSections.h

public:
   uint64_t Size = 0;
 
   // The following fields correspond to Elf_Shdr members.
   uint64_t Offset = 0;
   uint64_t LMAOffset = 0;
   uint64_t Addr = 0;
   uint32_t ShName = 0;
 
-  void addSection(InputSection *IS);
+  void addSection(InputSection *IS, InputSectionDescription *Hint = nullptr);
 
   // Location in the output buffer.
   uint8_t *Loc = nullptr;
 
   // The following members are normally only used in linker scripts.
   MemoryRegion *MemRegion = nullptr;
   Expr AddrExpr;
   Expr AlignExpr;

ELF/OutputSections.cpp

 // any special treatment on top of progbits, so there doesn't
 // seem to be a harm in merging them.
 static bool canMergeToProgbits(unsigned Type) {
   return Type == SHT_NOBITS || Type == SHT_PROGBITS || Type == SHT_INIT_ARRAY ||
          Type == SHT_PREINIT_ARRAY || Type == SHT_FINI_ARRAY ||
          Type == SHT_NOTE;
 }
 
-void OutputSection::addSection(InputSection *IS) {
+// Add IS to the output section.
+// If Hint is provided, it will be used to keep IS.
+// Overwise, a new InputSectionDescription will be added.
+void OutputSection::addSection(InputSection *IS,
+                               InputSectionDescription *Hint) {
   if (!Live) {
     // If IS is the first section to be added to this section,
     // initialize Type and Entsize from IS.
     Live = true;
     Type = IS->Type;
     Entsize = IS->Entsize;
   } else {
     // Otherwise, check if new type or flags are compatible with existing ones.
void OutputSection::addSection(InputSection *IS,
   // If this section contains a table of fixed-size entries, sh_entsize
   // holds the element size. If it contains elements of different size we
   // set sh_entsize to 0.
   if (Entsize != IS->Entsize)
     Entsize = 0;
 
   if (!IS->Assigned) {
     IS->Assigned = true;
+    auto ISD = Hint;
+    if (!ISD) {
       if (SectionCommands.empty() ||
           !isa<InputSectionDescription>(SectionCommands.back()))
         SectionCommands.push_back(make<InputSectionDescription>(""));
-    auto *ISD = cast<InputSectionDescription>(SectionCommands.back());
+      ISD = cast<InputSectionDescription>(SectionCommands.back());
+    }
     ISD->Sections.push_back(IS);
   }
 }
 
 void elf::sortByOrder(MutableArrayRef<InputSection *> In,
                       std::function<int(InputSectionBase *S)> Order) {
   typedef std::pair<int, InputSection *> Pair;
   auto Comp = [](const Pair &A, const Pair &B) { return A.first < B.first; };

ELF/SyntheticSections.h

 public:
   MipsRldMapSection();
   size_t getSize() const override { return Config->Wordsize; }
   void writeTo(uint8_t *Buf) override {}
 };
 
 class ARMExidxSentinelSection : public SyntheticSection {
 public:
-  ARMExidxSentinelSection();
+  ARMExidxSentinelSection(InputSection *Highest);
   size_t getSize() const override { return 8; }
   void writeTo(uint8_t *Buf) override;
+
+private:
+  InputSection *Highest;
 };
 
 // A container for one or more linker generated thunks. Instances of these
 // thunks including ARM interworking and Mips LA25 PI to non-PI thunks.
 class ThunkSection : public SyntheticSection {
 public:
   // ThunkSection in OS, with desired OutSecOff of Off
   ThunkSection(OutputSection *OS, uint64_t Off);

ELF/SyntheticSections.cpp

void elf::mergeSections() {
   std::vector<InputSectionBase *> &V = InputSections;
   V.erase(std::remove(V.begin(), V.end(), nullptr), V.end());
 }
 
 MipsRldMapSection::MipsRldMapSection()
     : SyntheticSection(SHF_ALLOC | SHF_WRITE, SHT_PROGBITS, Config->Wordsize,
                        ".rld_map") {}
 
-ARMExidxSentinelSection::ARMExidxSentinelSection()
+ARMExidxSentinelSection::ARMExidxSentinelSection(InputSection *Highest)
     : SyntheticSection(SHF_ALLOC | SHF_LINK_ORDER, SHT_ARM_EXIDX,
-                       Config->Wordsize, ".ARM.exidx") {}
+                       Config->Wordsize, ".ARM.exidx"),
+      Highest(Highest) {}
 
 // Write a terminating sentinel entry to the end of the .ARM.exidx table.
 // This section will have been sorted last in the .ARM.exidx table.
 // This table entry will have the form:
 // | PREL31 upper bound of code that has exception tables | EXIDX_CANTUNWIND |
 // The sentinel must have the PREL31 value of an address higher than any
 // address described by any other table entry.
 void ARMExidxSentinelSection::writeTo(uint8_t *Buf) {
-  // The Sections are sorted in order of ascending PREL31 address with the
-  // sentinel last. We need to find the InputSection that precedes the
-  // sentinel.
-  OutputSection *C = getParent();
-  InputSection *Highest = nullptr;
-  unsigned Skip = 1;
-  for (const BaseCommand *Base : llvm::reverse(C->SectionCommands)) {
-    if (!isa<InputSectionDescription>(Base))
-      continue;
-    auto L = cast<InputSectionDescription>(Base);
-    if (Skip >= L->Sections.size()) {
-      Skip -= L->Sections.size();
-      continue;
-    }
-    Highest = L->Sections[L->Sections.size() - Skip - 1];
-    break;
-  }
   assert(Highest);
   InputSection *LS = Highest->getLinkOrderDep();
   uint64_t S = LS->getParent()->Addr + LS->getOffset(LS->getSize());
   uint64_t P = getVA();
   Target->relocateOne(Buf, R_ARM_PREL31, S - P);
   write32le(Buf + 4, 1);
 }
 

ELF/Writer.cpp

 private:
   void copyLocalSymbols();
   void addSectionSymbols();
   void forEachRelSec(std::function<void(InputSectionBase &)> Fn);
   void sortSections();
   void resolveShfLinkOrder();
   void sortInputSections();
   void finalizeSections();
-  void addPredefinedSections();
   void setReservedSymbolSections();
 
   std::vector<PhdrEntry *> createPhdrs();
   void removeEmptyPTLoad();
   void addPtArmExid(std::vector<PhdrEntry *> &Phdrs);
   void assignFileOffsets();
   void assignFileOffsetsBinary();
   void setPhdrs();
static bool compareByFilePosition(InputSection *A, InputSection *B) {
   OutputSection *BOut = LB->getParent();
   if (AOut != BOut)
     return AOut->SectionIndex < BOut->SectionIndex;
   return LA->OutSecOff < LB->OutSecOff;
 }
 
 template <class ELFT> void Writer<ELFT>::resolveShfLinkOrder() {
   for (OutputSection *Sec : OutputSections) {
-    if (!(Sec->Flags & SHF_LINK_ORDER))
+    if (!(Sec->Flags & SHF_LINK_ORDER) || !Sec->Live)
       continue;
 
     // Link order may be distributed across several InputSectionDescriptions
     // but sort must consider them all at once.
     std::vector<InputSection **> ScriptSections;
     std::vector<InputSection *> Sections;
+    InputSectionDescription *LastISD = nullptr;
     for (BaseCommand *Base : Sec->SectionCommands) {
       if (auto *ISD = dyn_cast<InputSectionDescription>(Base)) {
+        if (ISD->Sections.empty())
+          continue;
+        LastISD = ISD;
         for (InputSection *&IS : ISD->Sections) {
           ScriptSections.push_back(&IS);
           Sections.push_back(IS);
         }
       }
     }
+    if (!LastISD)
+      continue;
     std::stable_sort(Sections.begin(), Sections.end(), compareByFilePosition);
     for (int I = 0, N = Sections.size(); I < N; ++I)
       *ScriptSections[I] = Sections[I];
+
+    // ARM ABI requires .ARM.exidx to be terminated by some piece of data.
+    // We have the terminater synthetic section class. Add that at the end.
+    if (!Config->Relocatable && Sec->Name == ".ARM.exidx") {
+      auto *Sentinel = make<ARMExidxSentinelSection>(Sections.back());
+      Sec->addSection(Sentinel, LastISD);
+    }
   }
 }
 
 static void applySynthetic(const std::vector<SyntheticSection *> &Sections,
                            std::function<void(SyntheticSection *)> Fn) {
   for (SyntheticSection *SS : Sections)
     if (SS && SS->getParent() && !SS->empty())
       Fn(SS);
if (InX::DynSymTab && Sym->includeInDynsym()) {
           In<ELFT>::VerNeed->addSymbol(SS);
     }
   }
 
   // Do not proceed if there was an undefined symbol.
   if (errorCount())
     return;
 
-  addPredefinedSections();
   removeUnusedSyntheticSections();
 
   sortSections();
   Script->removeEmptyCommands();
 
   // Now that we have the final list, create a list of all the
   // OutputSections for convenience.
   for (BaseCommand *Base : Script->SectionCommands)
template <class ELFT> void Writer<ELFT>::finalizeSections() {
   for (OutputSection *Sec : OutputSections)
     Sec->finalize<ELFT>();
 
   // createThunks may have added local symbols to the static symbol table
   applySynthetic({InX::SymTab},
                  [](SyntheticSection *SS) { SS->postThunkContents(); });
 }
 
-template <class ELFT> void Writer<ELFT>::addPredefinedSections() {
-  // ARM ABI requires .ARM.exidx to be terminated by some piece of data.
-  // We have the terminater synthetic section class. Add that at the end.
-  OutputSection *Cmd = findSection(".ARM.exidx");
-  if (!Cmd || !Cmd->Live || Config->Relocatable)
-    return;
-
-  auto *Sentinel = make<ARMExidxSentinelSection>();
-  Cmd->addSection(Sentinel);
-}
-
 // The linker is expected to define SECNAME_start and SECNAME_end
 // symbols for a few sections. This function defines them.
 template <class ELFT> void Writer<ELFT>::addStartEndSymbols() {
   auto Define = [&](StringRef Start, StringRef End, OutputSection *OS) {
     // These symbols resolve to the image base if the section does not exist.
     // A special value -1 indicates end of the section.
     if (OS) {
       addOptionalRegular<ELFT>(Start, OS, 0);

test/ELF/linkerscript/arm-exidx-sentinel-and-assignment.s

 # REQUIRES: arm
 # RUN: llvm-mc -filetype=obj -triple=armv7a-none-linux-gnueabi %s -o %t.o
 # RUN: echo "SECTIONS {                                        \
 # RUN:         .ARM.exidx 0x1000 : { *(.ARM.exidx*) foo = .; } \
 # RUN:         .text      0x2000 : { *(.text*) }               \
 # RUN:       }" > %t.script
 ## We used to crash if the last output section command for .ARM.exidx
 ## was anything but an input section description.
 # RUN: ld.lld -T %t.script %t.o -shared -o %t.so
-# RUN: llvm-objdump -s -triple=armv7a-none-linux-gnueabi %t.so | FileCheck %s
+# RUN: llvm-objdump -s -triple=armv7a-none-linux-gnueabi %t.so | FileCheck %s --check-prefix=DUMP
+# RUN: llvm-readobj -s -t %t.so | FileCheck %s
 
  .syntax unified
  .text
  .global _start
 _start:
  .fnstart
  .cantunwind
  bx lr
  .fnend
 
-// CHECK: Contents of section .ARM.exidx:
-// 1000 + 1000 = 0x2000 = _start
-// 1008 + 0ffc = 0x2004 = _start + sizeof(_start)
-// CHECK-NEXT: 1000 00100000 01000000 fc0f0000 01000000
+# DUMP: Contents of section .ARM.exidx:
+# 1000 + 1000 = 0x2000 = _start
+# 1008 + 0ffc = 0x2004 = _start + sizeof(_start)
+# DUMP-NEXT: 1000 00100000 01000000 fc0f0000 01000000
+
+# CHECK:       Section {
+# CHECK:         Name: .ARM.exidx
+# CHECK-NEXT:    Type: SHT_ARM_EXIDX
+# CHECK-NEXT:    Flags [
+# CHECK-NEXT:      SHF_ALLOC
+# CHECK-NEXT:      SHF_LINK_ORDER
+# CHECK-NEXT:    ]
+# CHECK-NEXT:    Address: 0x1000
+# CHECK-NEXT:    Offset:
+# CHECK-NEXT:    Size: 16
+
+# Symbol 'foo' is expected to point at the end of the section.
+# CHECK:       Symbol {
+# CHECK:         Name: foo
+# CHECK-NEXT:    Value: 0x1010