[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

1190

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 ↗

(On Diff #126937)

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/SyntheticSections.h

 public:
   MipsRldMapSection();
   size_t getSize() const override { return Config->Wordsize; }
   void writeTo(uint8_t *Buf) override {}
 };
 
 class ARMExidxSentinelSection : public SyntheticSection {
 public:
+  static ARMExidxSentinelSection *create();
+
   ARMExidxSentinelSection();
   size_t getSize() const override { return 8; }
   void writeTo(uint8_t *Buf) override;
+
+  InputSection *Highest = 0;
 };
 
 // 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::create() {
+  if (Config->Relocatable)
+    return nullptr;
+
+  // Find if there are live ARM exidx input sections.
+  if (InputSections.end() ==
+      llvm::find_if(InputSections, [](InputSectionBase *Sec) {
+        return Sec->Type == SHT_ARM_EXIDX && Sec->Live;
+      }))
+    return nullptr;
+
+  return make<ARMExidxSentinelSection>();
+}
+
 ARMExidxSentinelSection::ARMExidxSentinelSection()
     : SyntheticSection(SHF_ALLOC | SHF_LINK_ORDER, SHT_ARM_EXIDX,
                        Config->Wordsize, ".ARM.exidx") {}
 
 // 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();
if (!Config->Relocatable) {
     Add(InX::EhFrame);
   }
 
   if (InX::SymTab)
     Add(InX::SymTab);
   Add(InX::ShStrTab);
   if (InX::StrTab)
     Add(InX::StrTab);
+
+  if (Config->EMachine == EM_ARM)
+    // Add a sentinel to terminate .ARM.exidx. It helps an unwinder
+    // to find the exact address range of the last entry.
+    if (auto *Sec = ARMExidxSentinelSection::create())
+      Add(Sec);
 }
 
 // The main function of the writer.
 template <class ELFT> void Writer<ELFT>::run() {
   // Create linker-synthesized sections such as .got or .plt.
   // Such sections are of type input section.
   createSyntheticSections<ELFT>();
 
while (NonScriptI != E) {
     std::rotate(Pos, NonScriptI, End);
     NonScriptI = End;
   }
 
   Script->adjustSectionsAfterSorting();
 }
 
 static bool compareByFilePosition(InputSection *A, InputSection *B) {
-  // Synthetic doesn't have link order dependecy, stable_sort will keep it last
+  // Synthetic, i. e. a sentinel section, should go last.
   if (A->kind() == InputSectionBase::Synthetic ||
       B->kind() == InputSectionBase::Synthetic)
-    return false;
+    return A->kind() != InputSectionBase::Synthetic;
   InputSection *LA = A->getLinkOrderDep();
   InputSection *LB = B->getLinkOrderDep();
   OutputSection *AOut = LA->getParent();
   OutputSection *BOut = LB->getParent();
   if (AOut != BOut)
     return AOut->SectionIndex < BOut->SectionIndex;
   return LA->OutSecOff < LB->OutSecOff;
 }
for (OutputSection *Sec : OutputSections) {
     for (BaseCommand *Base : Sec->SectionCommands) {
       if (auto *ISD = dyn_cast<InputSectionDescription>(Base)) {
         for (InputSection *&IS : ISD->Sections) {
           ScriptSections.push_back(&IS);
           Sections.push_back(IS);
         }
       }
     }
+    if (Sections.empty())
+      continue;
+
     std::stable_sort(Sections.begin(), Sections.end(), compareByFilePosition);
     for (int I = 0, N = Sections.size(); I < N; ++I)
       *ScriptSections[I] = Sections[I];
+
+    // The last actual entry is required to fill the sentinel.
+    // Remember it here so that we don't have to find it again.
+    if (auto *Sentinel = dyn_cast<ARMExidxSentinelSection>(Sections.back())) {
+      assert(Sections.size() >= 2 &&
+             "We should create a sentinel section only if there are "
+             "alive regular exidx sections.");
+      Sentinel->Highest = Sections[Sections.size() - 2];
+    }
   }
 }
 
 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