[LLD][ELF][ARM] Fix crash when discarding all of the InputSections that have .ARM.exidx sections

[peter.smith]

Summary

While experimenting with some test cases for D67761 earlier today I found that I could crash the linker when I discarded all the InputSections, but not the .ARM.exidx sections that had a dependency on them. This was ostensibly down to the OutputSection code using a non-existing InputSection to obtain a OutputSection for the OutputSection link order dependency. Attempts to clumsily fix this resulted in other crashes as the executableSections and exidxSections weren't in synch.

The fix makes sure that we only add to the exidxSections when the section it depends on would be added to the executableSections. This allows us to remove the empty flag and then implement a more sophisticated needed() so that we discard the .ARM.exidx section if it wouldn't be valid to write it.

This is independent of the fix for D67761.

[MaskRay]

lld/ELF/SyntheticSections.cpp

3228 ↗

(On Diff #221068)

if (isec->type == SHT_ARM_EXIDX) {
  exidxSections.push_back(isec);
  return true;
}

probably also works. finalizeContents() has logic to filter dead InputSections, so I'm thinking whether we should simplify the logic here.

lld/test/ELF/arm-exidx-discard-all.s

1 ↗

(On Diff #221068)

How about moving this to linkerscript/? There are 2 arm-exidx* tests.

[peter.smith]

lld/ELF/SyntheticSections.cpp

3228 ↗

(On Diff #221068)

I tried that initially but got test failures (crashes) for the case where there was a zero sized .text section with a non-zero size .ARM.exidx section (garbage collection off). The .ARM.exidx section is added, but the executable section isn't, isNeeded() returns true. This leaves the executableSections and exidxSections out of synch.

It is possible to account for this case by making isNeeded() more complex by making it search for both executable and exidx sections, but I thought it better to keep the executableSections and exidxSections synchronised at the start.

I can make the change if you prefer.

lld/test/ELF/arm-exidx-discard-all.s

1 ↗

(On Diff #221068)

Sure I'll update the patch.

[peter.smith]

Changes from last version, moved exidx discard tests to linkerscripts subdir.

[MaskRay]

lld/ELF/SyntheticSections.cpp

3228 ↗

(On Diff #221068)

OK. I think having the check here is fine.

I thought it better to keep the executableSections and exidxSections synchronised at the start.

Keeping them synchronized is a nice property to have.

lld/trunk/ELF/SyntheticSections.h

public:
   ARMExidxSyntheticSection();
 
   // Add an input section to the ARMExidxSyntheticSection. Returns whether the
   // section needs to be removed from the main input section list.
   bool addSection(InputSection *isec);
 
   size_t getSize() const override { return size; }
   void writeTo(uint8_t *buf) override;
-  bool isNeeded() const override { return !empty; }
+  bool isNeeded() const override;
   // Sort and remove duplicate entries.
   void finalizeContents() override;
   InputSection *getLinkOrderDep() const;
 
   static bool classof(const SectionBase *d);
 
   // Links to the ARMExidxSections so we can transfer the relocations once the
   // layout is known.
   std::vector<InputSection *> exidxSections;
 
 private:
   size_t size;
 
-  // Empty if ExecutableSections contains no dependent .ARM.exidx sections.
-  bool empty = true;
-
   // Instead of storing pointers to the .ARM.exidx InputSections from
   // InputObjects, we store pointers to the executable sections that need
   // .ARM.exidx sections. We can then use the dependentSections of these to
   // either find the .ARM.exidx section or know that we need to generate one.
   std::vector<InputSection *> executableSections;
 
   // The executable InputSection with the highest address to use for the
   // sentinel. We store separately from ExecutableSections as merging of

lld/trunk/ELF/SyntheticSections.cpp

 
 static InputSection *findExidxSection(InputSection *isec) {
   for (InputSection *d : isec->dependentSections)
     if (d->type == SHT_ARM_EXIDX)
       return d;
   return nullptr;
 }
 
+static bool isValidExidxSectionDep(InputSection *isec) {
+  return (isec->flags & SHF_ALLOC) && (isec->flags & SHF_EXECINSTR) &&
+         isec->getSize() > 0;
+}
+
 bool ARMExidxSyntheticSection::addSection(InputSection *isec) {
   if (isec->type == SHT_ARM_EXIDX) {
+    if (InputSection* dep = isec->getLinkOrderDep())
+      if (isValidExidxSectionDep(dep)) {
         exidxSections.push_back(isec);
         return true;
       }
+    return false;
+  }
 
-  if ((isec->flags & SHF_ALLOC) && (isec->flags & SHF_EXECINSTR) &&
-      isec->getSize() > 0) {
+  if (isValidExidxSectionDep(isec)) {
     executableSections.push_back(isec);
-    if (empty && findExidxSection(isec))
-      empty = false;
     return false;
   }
 
   // FIXME: we do not output a relocation section when --emit-relocs is used
   // as we do not have relocation sections for linker generated table entries
   // and we would have to erase at a late stage relocations from merged entries.
   // Given that exception tables are already position independent and a binary
   // analyzer could derive the relocations we choose to erase the relocations.
void ARMExidxSyntheticSection::writeTo(uint8_t *buf) {
   // Write Sentinel.
   memcpy(buf + offset, cantUnwindData, sizeof(cantUnwindData));
   uint64_t s = sentinel->getVA(sentinel->getSize());
   uint64_t p = getVA() + offset;
   target->relocateOne(buf + offset, R_ARM_PREL31, s - p);
   assert(size == offset + 8);
 }
 
+bool ARMExidxSyntheticSection::isNeeded() const {
+  return llvm::find_if(exidxSections, [](InputSection *isec) {
+           return isec->isLive();
+         }) != exidxSections.end();
+}
+
 bool ARMExidxSyntheticSection::classof(const SectionBase *d) {
   return d->kind() == InputSectionBase::Synthetic && d->type == SHT_ARM_EXIDX;
 }
 
 ThunkSection::ThunkSection(OutputSection *os, uint64_t off)
     : SyntheticSection(SHF_ALLOC | SHF_EXECINSTR, SHT_PROGBITS,
                        config->wordsize, ".text.thunk") {
   this->parent = os;

lld/trunk/test/ELF/arm-exidx-discard.s

-// REQUIRES: arm
-// RUN: llvm-mc -filetype=obj -triple arm-gnu-linux-eabi -mcpu cortex-a7 -arm-add-build-attributes %s -o %t.o
-// RUN: echo "ENTRY(__entrypoint) SECTIONS { . = 0x10000; .text : { *(.text .text.*) } /DISCARD/ : { *(.ARM.exidx*) *(.gnu.linkonce.armexidx.*) } }" > %t.script
-// RUN: ld.lld -T %t.script %t.o -o %t.elf
-// RUN: llvm-readobj --sections %t.elf | FileCheck %s
-
-.globl  __entrypoint
-__entrypoint:
-.fnstart
-    bx  lr
- .save {r7, lr}
- .setfp r7, sp, #0
- .fnend
-// Check that .ARM.exidx/.gnu.linkonce.armexidx
-// are correctly removed if they were added.
-// CHECK-NOT: .ARM.exidx
-// CHECK-NOT: .gnu.linkonce.armexidx.

lld/trunk/test/ELF/linkerscript/arm-exidx-discard-all.s

+// REQUIRES: arm
+// RUN: llvm-mc -filetype=obj -triple arm-gnu-linux-eabi -mcpu cortex-a7 -arm-add-build-attributes %s -o %t.o
+// RUN: echo "ENTRY(__entrypoint) SECTIONS { /DISCARD/ : { *(.text.1) } }" > %t.script
+// RUN: ld.lld -T %t.script %t.o -o %t.elf
+// RUN: llvm-readobj --sections %t.elf | FileCheck %s
+
+/// Test that when we /DISCARD/ all the input sections with associated
+/// .ARM.exidx sections then we also discard all the .ARM.exidx sections.
+
+ .section .text.1, "ax", %progbits
+ .global foo
+ .type foo, %function
+ .fnstart
+foo:
+  bx lr
+  .cantunwind
+  .fnend
+
+// CHECK-NOT: .ARM.exidx

lld/trunk/test/ELF/linkerscript/arm-exidx-discard.s

+// REQUIRES: arm
+// RUN: llvm-mc -filetype=obj -triple arm-gnu-linux-eabi -mcpu cortex-a7 -arm-add-build-attributes %s -o %t.o
+// RUN: echo "ENTRY(__entrypoint) SECTIONS { . = 0x10000; .text : { *(.text .text.*) } /DISCARD/ : { *(.ARM.exidx*) *(.gnu.linkonce.armexidx.*) } }" > %t.script
+// RUN: ld.lld -T %t.script %t.o -o %t.elf
+// RUN: llvm-readobj --sections %t.elf | FileCheck %s
+
+.globl  __entrypoint
+__entrypoint:
+.fnstart
+    bx  lr
+ .save {r7, lr}
+ .setfp r7, sp, #0
+ .fnend
+// Check that .ARM.exidx/.gnu.linkonce.armexidx
+// are correctly removed if they were added.
+// CHECK-NOT: .ARM.exidx
+// CHECK-NOT: .gnu.linkonce.armexidx.