[AArch64] Force relocations for all ADRP instructions

[mstorsjo]

Summary

This generalizes an existing fix from ELF to MachO and COFF.

Test that an ADRP to a local symbol whose offset is known at assembly time still produces relocations, both for MachO and COFF. Test that an ADRP without a @page modifier on MachO fails (previously it didn't).

[mstorsjo]

I'd be interested in knowing why this isn't done by default (i.e. why this doesn't need to be done at all for MachO) btw, if someone has got time to explain.

[t.p.northover]

I'd be interested in knowing why this isn't done by default (i.e. why this doesn't need to be done at all for MachO) btw, if someone has got time to explain.

I'm pretty sure that's a bug, I get the wrong answer when I actually try out this function on iOS:

        .text
        .global _foo
        .p2align 12
_foo:
        .fill 1023, 4, 0xd503201f
        adrp x0, Lthere
Lthere:
        ret

I suspect it hasn't come up on MachO because CodeGen never used ADRP to address objects in the __text section so you need assembly to trigger it.

So would you mind putting that function into the base AArch64AsmBackend?

[mgrang]

lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp

585

Period at the end of comment.

test/MC/AArch64/adrp-relocation-coff.s

1 ↗

(On Diff #107053)

There's already a test case for coff relocations: test/MC/AArch64/coff-relocations.s. Would you mind adding to the same file instead of creating a new one just for adrp relocs?

[mstorsjo]

lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp

585

I'll move the ELFAArch64AsmBackend implementation up to the superclass, so this comment will go away altogether.

test/MC/AArch64/adrp-relocation-coff.s

1 ↗

(On Diff #107053)

Sure, I can do that. Although since this fix is getting generalized for MachO as well, I'd like to test that as well. Should I try to add that into some MachO specific file as well, or have an adrp specific test file? The current one (adrp-relocation.s) tests some relocations that I don't think all are supported in COFF at least, which was why I tried to create a new one here.

[mstorsjo]

In D35544#812979, @t.p.northover wrote:

I'd be interested in knowing why this isn't done by default (i.e. why this doesn't need to be done at all for MachO) btw, if someone has got time to explain.

I'm pretty sure that's a bug, I get the wrong answer when I actually try out this function on iOS:

        .text
        .global _foo
        .p2align 12
_foo:
        .fill 1023, 4, 0xd503201f
        adrp x0, Lthere
Lthere:
        ret

I suspect it hasn't come up on MachO because CodeGen never used ADRP to address objects in the __text section so you need assembly to trigger it.

I did a quick test like this:

static int the_func(int a);
                
void *get_func(void) {
        return the_func;
}

static int the_func(int a) {
        return 2*a;
}

and got this:

$ clang test.c -c -o test.o -O3 -target arm64-apple-darwin
$ llvm-objdump -d -r test.o

test.o:  file format Mach-O arm64

Disassembly of section __TEXT,__text:
ltmp0:
       0:       00 00 00 90     adrp    x0, #0
                0000000000000000:  ARM64_RELOC_PAGE21   _the_func
       4:       00 00 00 91     add     x0, x0, #0
                0000000000000004:  ARM64_RELOC_PAGEOFF12        _the_func
       8:       c0 03 5f d6     ret

_the_func:
       c:       00 78 1f 53     lsl     w0, w0, #1
      10:       c0 03 5f d6     ret

So it seems to me like it actually does use ADRP, but probably via some other codepath, since it does generate a relocation. But fixing it for handwritten assembly obviously also is worthwhile.

So would you mind putting that function into the base AArch64AsmBackend?

Sure, will do.

[mstorsjo]

In D35544#812979, @t.p.northover wrote:

I'd be interested in knowing why this isn't done by default (i.e. why this doesn't need to be done at all for MachO) btw, if someone has got time to explain.

I'm pretty sure that's a bug, I get the wrong answer when I actually try out this function on iOS:

        .text
        .global _foo
        .p2align 12
_foo:
        .fill 1023, 4, 0xd503201f
        adrp x0, Lthere
Lthere:
        ret

I suspect it hasn't come up on MachO because CodeGen never used ADRP to address objects in the __text section so you need assembly to trigger it.

So would you mind putting that function into the base AArch64AsmBackend?

Actually, when I try to move the function from ELFAArch64AsmBackend to AArch64AsmBackend, I suddenly started getting the following errors when assembling your test snippet:

test.s:7:9: error: ADR/ADRP relocations must be GOT relative
        adrp x0, Lthere
        ^
test.s:7:9: error: unknown AArch64 fixup kind!
        adrp x0, Lthere

[t.p.northover]

So it seems to me like it actually does use ADRP, but probably via some other codepath, since it does generate a relocation. But fixing it for handwritten assembly obviously also is worthwhile.

MachO always splits up the __text section by its global symbols (sorry, I realise I was a bit vague earlier), so it emits a relocation for those reasons. You'd need to convince LLVM to use ADRP for something in the same function to see the effect. I'm not aware of any LLVM or C code that would do that (aside from inline asm, of course).

Actually, when I try to move the function from ELFAArch64AsmBackend to AArch64AsmBackend, I suddenly started getting the following errors when assembling your test snippet:

Ah, interesting! I actually think that's correct behaviour. The MachO adrp always has a textual tag for the kind of relocation you want. For example adrp x0, Lthere@PAGE. So without that function we silently miscompiled invalid code instead of diagnosing it; with your change it survives to the relocation phase to be reported.

Technically we should probably diagnose it earlier (AsmParser), but for your purposes just checking that there is an error in the MachO case is probably the right thing to do.

[mstorsjo]

In D35544#813711, @t.p.northover wrote:

Actually, when I try to move the function from ELFAArch64AsmBackend to AArch64AsmBackend, I suddenly started getting the following errors when assembling your test snippet:

Ah, interesting! I actually think that's correct behaviour. The MachO adrp always has a textual tag for the kind of relocation you want. For example adrp x0, Lthere@PAGE. So without that function we silently miscompiled invalid code instead of diagnosing it; with your change it survives to the relocation phase to be reported.

Technically we should probably diagnose it earlier (AsmParser), but for your purposes just checking that there is an error in the MachO case is probably the right thing to do.

Oh, indeed, I forgot that MachO has a different syntax for these. I'll update with tests for both of these cases.

[mstorsjo]

Moved the fix from ELF to the base class, added tests for COFF and MachO.

[t.p.northover]

Looks fine to me other than a nit about the MachO test. Unless the fix to that gets weird and you want to confirm it's right you can commit without uploading another revision.

test/MC/AArch64/macho-adrp-missing-reloc.s

3

This isn't really a failure. The usual way to check error messages is something like:

; RUN: not llvm-mc < %s -triple arm64-apple-darwin 2>&1 | FileCheck %s
; CHECK: error: ADR/ADRP relocations must be GOT relative

The "not" means it's acceptable for llvm-mc to "fail", and the FileCheck makes sure we get a vaguely sensible error message rather than a crash or some other nonsense.

lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp

void relaxInstruction(const MCInst &Inst, const MCSubtargetInfo &STI,
                         MCInst &Res) const override;
   bool writeNopData(uint64_t Count, MCObjectWriter *OW) const override;
 
   void HandleAssemblerFlag(MCAssemblerFlag Flag) {}
 
   unsigned getPointerSize() const { return 8; }
 
   unsigned getFixupKindContainereSizeInBytes(unsigned Kind) const;
+
+  bool shouldForceRelocation(const MCAssembler &Asm, const MCFixup &Fixup,
+                             const MCValue &Target) override;
 };
 
 } // end anonymous namespace
 
 /// \brief The number of bytes the fixup may change.
 static unsigned getFixupKindNumBytes(unsigned Kind) {
   switch (Kind) {
   default:
bool AArch64AsmBackend::writeNopData(uint64_t Count, MCObjectWriter *OW) const {
 
   // We are properly aligned, so write NOPs as requested.
   Count /= 4;
   for (uint64_t i = 0; i != Count; ++i)
     OW->write32(0xd503201f);
   return true;
 }
 
+bool AArch64AsmBackend::shouldForceRelocation(const MCAssembler &Asm,
+                                              const MCFixup &Fixup,
+                                              const MCValue &Target) {
+  // The ADRP instruction adds some multiple of 0x1000 to the current PC &
+  // ~0xfff. This means that the required offset to reach a symbol can vary by
+  // up to one step depending on where the ADRP is in memory. For example:
+  //
+  //     ADRP x0, there
+  //  there:
+  //
+  // If the ADRP occurs at address 0xffc then "there" will be at 0x1000 and
+  // we'll need that as an offset. At any other address "there" will be in the
+  // same page as the ADRP and the instruction should encode 0x0. Assuming the
+  // section isn't 0x1000-aligned, we therefore need to delegate this decision
+  // to the linker -- a relocation!
+  if ((uint32_t)Fixup.getKind() == AArch64::fixup_aarch64_pcrel_adrp_imm21)
+    return true;
+  return false;
+}
+
 namespace {
 
 namespace CU {
 
 /// \brief Compact unwind encoding values.
 enum CompactUnwindEncodings {
   /// \brief A "frameless" leaf function, where no non-volatile registers are
   /// saved. The return remains in LR throughout the function.
public:
 
   ELFAArch64AsmBackend(const Target &T, uint8_t OSABI, bool IsLittleEndian,
                        bool IsILP32)
     : AArch64AsmBackend(T, IsLittleEndian), OSABI(OSABI), IsILP32(IsILP32) {}
 
   MCObjectWriter *createObjectWriter(raw_pwrite_stream &OS) const override {
     return createAArch64ELFObjectWriter(OS, OSABI, IsLittleEndian, IsILP32);
   }
-
-  bool shouldForceRelocation(const MCAssembler &Asm, const MCFixup &Fixup,
-                             const MCValue &Target) override;
 };
 
-bool ELFAArch64AsmBackend::shouldForceRelocation(const MCAssembler &Asm,
-                                                 const MCFixup &Fixup,
-                                                 const MCValue &Target) {
-  // The ADRP instruction adds some multiple of 0x1000 to the current PC &
-  // ~0xfff. This means that the required offset to reach a symbol can vary by
-  // up to one step depending on where the ADRP is in memory. For example:
-  //
-  //     ADRP x0, there
-  //  there:
-  //
-  // If the ADRP occurs at address 0xffc then "there" will be at 0x1000 and
-  // we'll need that as an offset. At any other address "there" will be in the
-  // same page as the ADRP and the instruction should encode 0x0. Assuming the
-  // section isn't 0x1000-aligned, we therefore need to delegate this decision
-  // to the linker -- a relocation!
-  if ((uint32_t)Fixup.getKind() == AArch64::fixup_aarch64_pcrel_adrp_imm21)
-    return true;
-  return false;
-}
-
 }
 
 namespace {
 class COFFAArch64AsmBackend : public AArch64AsmBackend {
 public:
   COFFAArch64AsmBackend(const Target &T, const Triple &TheTriple)
       : AArch64AsmBackend(T, /*IsLittleEndian*/true) {}
 
   MCObjectWriter *createObjectWriter(raw_pwrite_stream &OS) const override {
     return createAArch64WinCOFFObjectWriter(OS);
   }
 };
 }
 
 MCAsmBackend *llvm::createAArch64leAsmBackend(const Target &T,
                                               const MCRegisterInfo &MRI,
                                               const Triple &TheTriple,
                                               StringRef CPU,
                                               const MCTargetOptions &Options) {
   if (TheTriple.isOSBinFormatMachO())
     return new DarwinAArch64AsmBackend(T, MRI);
 

test/MC/AArch64/coff-relocations.s

 adrp x0, foo
 
 ; IMAGE_REL_ARM64_PAGEOFFSET_12A
 add x0, x0, :lo12:foo
 
 ; IMAGE_REL_ARM64_PAGEOFFSET_12L
 ldr x0, [x0, :lo12:foo]
 
+; IMAGE_REL_ARM64_PAGEBASE_REL21, even if the symbol offset is known
+adrp x0, bar
+bar:
+
 ; IMAGE_REL_ARM64_SECREL
 .secrel32 .Linfo_bar
 .Linfo_bar:
 
 ; IMAGE_REL_ARM64_SECTION
 .secidx func
 
-
 ; CHECK: Format: COFF-ARM64
 ; CHECK: Arch: aarch64
 ; CHECK: AddressSize: 64bit
 ; CHECK: Relocations [
 ; CHECK:   Section (1) .text {
 ; CHECK: 0x4 IMAGE_REL_ARM64_ADDR32 foo
 ; CHECK: 0x8 IMAGE_REL_ARM64_ADDR32NB func
 ; CHECK: 0xC IMAGE_REL_ARM64_ADDR64 arr
 ; CHECK: 0x14 IMAGE_REL_ARM64_BRANCH26 target
 ; CHECK: 0x18 IMAGE_REL_ARM64_PAGEBASE_REL21 foo
 ; CHECK: 0x1C IMAGE_REL_ARM64_PAGEOFFSET_12A foo
 ; CHECK: 0x20 IMAGE_REL_ARM64_PAGEOFFSET_12L foo
-; CHECK: 0x24 IMAGE_REL_ARM64_SECREL .text
-; CHECK: 0x28 IMAGE_REL_ARM64_SECTION func
+; CHECK: 0x24 IMAGE_REL_ARM64_PAGEBASE_REL21 bar
+; CHECK: 0x28 IMAGE_REL_ARM64_SECREL .text
+; CHECK: 0x2C IMAGE_REL_ARM64_SECTION func
 ; CHECK:   }
 ; CHECK: ]

test/MC/AArch64/macho-adrp-missing-reloc.s

+; RUN: llvm-mc < %s -triple arm64-apple-darwin -filetype=obj -o - | llvm-readobj -r - | FileCheck %s
+
+; XFAIL: *
+; ADR/ADRP relocations must be GOT relative
+; unknown AArch64 fixup kind!
+  adrp x3, Lbar
+Lbar:
+  ret

test/MC/AArch64/macho-adrp-page.s

+; RUN: llvm-mc < %s -triple arm64-apple-darwin -filetype=obj -o - | llvm-readobj -r - | FileCheck %s
+
+  adrp x3, Lbar@page
+; CHECK: ARM64_RELOC_PAGE21
+Lbar:
+  ret