[LLD][ELF]{ARM][AArch64] Add missing classof to patch sections.

[peter.smith]

Summary

The code to insert patch section merges them with a comparison function that uses logic of the form:

return (isa<PatchSection>(a) && !isa<PatchSection>(b));

The PatchSections don't implement classof so this check fails if a is a PatchSection and b (is not a PatchSection, but is a SyntheticSection). This can result in relocations in the patches being out of range if the SyntheticSection is big, for example a ThunkSection with lots of thunks.

fixes part of pr44071 https://bugs.llvm.org/show_bug.cgi?id=44071 . Namely the relocation out of range errors. The convergence failures in pr44071 are a separate problem that will be addressed in a follow up patch.

The test case is a simulation of the Chromium build reported in pr44071 . It will fail with relocation out of range errors without classof implemented. It isn't easy to replicate the out of range errors, it needs large thunk sections and a large number of patches. These won't be seen in the vast majority of AArch64 programs. The Chromium build in pr44071 is fully instrumented.

I've not got a real world example of the Arm Cortex-A8 patch failing, but it has the same code sequence so I've implemented the classof function. It is possible to write a test case, but it is difficult to find the right conditions to actually make it fail, I've erred on submitting the fix without one to unblock the Chromium team running into the problem.

[grimar]

LGTM. Please wait for another approval(s) too.

[hans]

lgtm

[grimar]

nit (patch title): "[LLD][ELF]{ARM]" -> ""[LLD][ELF][ARM]

[MaskRay]

[LLD][ELF]{ARM][AArch64] Add missing classof to patch sections.

{ -> [

lld/ELF/ARMErrataFix.cpp

86

A C function named patch will reside in .text.patch when compiled with -ffunction-sections. This does not matter, because it is only used to break a tie:

auto mergeCmp = [](const InputSection *a, const InputSection *b) {
  if (a->outSecOff != b->outSecOff)
    return a->outSecOff < b->outSecOff;
  return isa<Patch657417Section>(a) && !isa<Patch657417Section>(b);
};

[MaskRay]

lld/test/ELF/aarch64-cortex-a53-843419-thunk-range.s

7

%t2 -> /dev/null

[ruiu]

classof is an LLVM's way of implementing a dynamic casting, and the code is idiomatic -- we almost always dispatch only by an enum field. So this could be a bit confusing to those who are expecting that idiomatic mechanism? I wonder if it is better to define a new function instead of overloading classof.

[MaskRay]

In D71242#1778767, @ruiu wrote:

classof is an LLVM's way of implementing a dynamic casting, and the code is idiomatic -- we almost always dispatch only by an enum field. So this could be a bit confusing to those who are expecting that idiomatic mechanism? I wonder if it is better to define a new function instead of overloading classof.

We already have such a code pattern

class EhFrameSection final : public SyntheticSection {
public:
  EhFrameSection();
  void writeTo(uint8_t *buf) override;
  void finalizeContents() override;
  bool isNeeded() const override { return !sections.empty(); }
  size_t getSize() const override { return size; }

  static bool classof(const SectionBase *d) {
    return SyntheticSection::classof(d) && d->name == ".eh_frame";
  }

lld/ELF/AArch64ErrataFix.cpp

385

Probably return SyntheticSection::classof(d) && d->name == ".text.patch";

See EhFrameSection::classof.

[ruiu]

In D71242#1778777, @MaskRay wrote:

In D71242#1778767, @ruiu wrote:

classof is an LLVM's way of implementing a dynamic casting, and the code is idiomatic -- we almost always dispatch only by an enum field. So this could be a bit confusing to those who are expecting that idiomatic mechanism? I wonder if it is better to define a new function instead of overloading classof.

We already have such a code pattern

class EhFrameSection final : public SyntheticSection {
public:
  EhFrameSection();
  void writeTo(uint8_t *buf) override;
  void finalizeContents() override;
  bool isNeeded() const override { return !sections.empty(); }
  size_t getSize() const override { return size; }

  static bool classof(const SectionBase *d) {
    return SyntheticSection::classof(d) && d->name == ".eh_frame";
  }

Ah, that's true. Not sure if it's a good pattern, but it seems it doesn't cause a confusion, so I'm fine with that.

lld/ELF/AArch64ErrataFix.cpp

public:
   Patch843419Section(InputSection *p, uint64_t off);
 
   void writeTo(uint8_t *buf) override;
 
   size_t getSize() const override { return 8; }
 
   uint64_t getLDSTAddr() const;
 
+  static bool classof(const SectionBase *d) {
+    return d->kind() == InputSectionBase::Synthetic && d->name == ".text.patch";
+  }
+
   // The Section we are patching.
   const InputSection *patchee;
   // The offset of the instruction in the patchee section we are patching.
   uint64_t patcheeOffset;
   // A label for the start of the Patch that we can use as a relocation target.
   Symbol *patchSym;
 };
 

lld/ELF/ARMErrataFix.cpp

public:
 
   void writeTo(uint8_t *buf) override;
 
   size_t getSize() const override { return 4; }
 
   // Get the virtual address of the branch instruction at patcheeOffset.
   uint64_t getBranchAddr() const;
 
+  static bool classof(const SectionBase *d) {
+    return d->kind() == InputSectionBase::Synthetic && d->name ==".text.patch";
+  }
+
   // The Section we are patching.
   const InputSection *patchee;
   // The offset of the instruction in the Patchee section we are patching.
   uint64_t patcheeOffset;
   // A label for the start of the Patch that we can use as a relocation target.
   Symbol *patchSym;
   // A decoding of the branch instruction at patcheeOffset.
   uint32_t instr;

lld/test/ELF/aarch64-cortex-a53-843419-thunk-range.s

+// REQUIRES: aarch64
+// RUN: llvm-mc -filetype=obj -triple=aarch64-none-linux --asm-macro-max-nesting-depth=40000 %s -o %t.o
+// RUN: echo "SECTIONS { \
+// RUN:          .text1 0x10000 : { *(.text.*) } \
+// RUN:          .text2 0xf010000 : { *(.target) } } " > %t.script
+// RUN: ld.lld --script %t.script -fix-cortex-a53-843419 %t.o -o %t2 --print-map 2>&1 | FileCheck %s
+
+/// We use %(\parameter) to evaluate expression, which requires .altmacro.
+ .altmacro
+
+/// Test to reproduce the conditions that trigger R_AARCH64_JUMP26 out of range
+/// errors in pr44071. We create a large number of patches and thunks, with an
+/// LLD with the fault, the patches will be inserted after the thunks and due
+/// to the size of the thunk section some of the patches go out of range.
+/// With a fixed LLD the patches are inserted before the thunks.
+
+// CHECK: <internal>:(.text.patch)
+// CHECK: <internal>:(.text.thunk)
+
+/// Macro to generate the cortex-a53-843419 patch sequence
+ .macro ERRATA from, to
+   .balign 4096
+   .space 4096 - 8
+   adrp x0, dat1
+   ldr x1, [x1, #0]
+   ldr x0, [x0, :got_lo12:dat1]
+   ret
+   .if (\to-\from)
+     ERRATA %(\from+1),\to
+   .endif
+ .endm
+
+ .section .text.01, "ax", %progbits
+ .balign 4096
+ .globl _start
+ .type _start, %function
+ .space 4096 - 8
+_start:
+/// Generate lots of patches.
+ ERRATA 0, 4000
+
+ .macro CALLS from, to
+   bl far\from
+   .if (\to-\from)
+     CALLS %(\from+1),\to
+   .endif
+ .endm
+
+ /// Generate long range thunks. These are inserted before the patches. Generate
+ /// a sufficient number such that the patches must be placed before the
+ /// .text.thunk section, and if they aren't some of the patches go out of
+ /// range.
+ .section .text.02, "ax", %progbits
+ .global func
+ .type func, %function
+func:
+ CALLS 0, 20000
+
+ .section .text.03, "ax", %progbits
+ .global space1
+space1:
+ .space (1024 * 1024 * 96) + (120 * 4 * 1024)
+ .balign 4096
+
+ .section .text.04, "ax", %progbits
+ .global space2
+space2:
+ .space 1024 * 1024
+
+ .macro DEFS from, to
+   .global far\from
+   .type far\from, %function
+far\from:
+   ret
+   .if (\to-\from)
+     DEFS %(\from+1),\to
+   .endif
+ .endm
+
+ /// Define the thunk targets
+ .section .target, "ax", %progbits
+ DEFS 0, 20000
+
+ .data
+ .global dat1
+dat1:
+ .xword 0