implemented R_AARCH64_ADR_PREL_PG_HI21, R_AARCH64_ADR_PREL_PG_HI21_NC, R_AARCH64_ADD_ABS_LO12_NC

[Kelvinyu1117]

Summary

I have implemented serveral relocation types with a test case (for R_AARCH64_ADR_PREL_PG_HI21 and R_AARCH64_ADD_ABS_LO12_NC). But it's a bit buggy that for
the R_AARCH64_ADR_PREL_PG_HI21_NC, it seems that it always fall into OutOfRangeError, my logic should be correct accroding to the specification of ARM. Therefore,
I would like to ask for a review and gain some insights for debugging the code.

[sgraenitz]

Hi Kelvin, thanks for working on this. I will run your patch through my pi tonight and see how I can help.

[sgraenitz]

it always fall into OutOfRangeError

I just fixed that with cdb335ffaff2. The issue didn't show up earlier, because the shift value was always smaller 32. I guess you want to either rebase on this commit or cherry-pick it to your development branch. Also, it would be good to do the same for your extractBit function.

Apart from that, I see two relocations in your test case, both for g: R_AARCH64_ADD_ABS_LO12_NC (offset 0x4) and R_AARCH64_ADD_ABS_LO12_NC (offset 0x8). Is this looking correct?

Eventually, your test still fails, because it has no CHECK lines. See inline comment.

llvm/test/ExecutionEngine/JITLink/AArch64/ELF_aarch64_test1.s

3

My initial recommendation to use FileCheck was not good. JITLink tests actually use jitlink-check, which accepts some limited expressions to aid validation. I have to look up the details again as well.

Maybe this example can give you some idea how it works: https://reviews.llvm.org/D90331#change-XdnNV6F7d9o7

[sgraenitz]

llvm/test/ExecutionEngine/JITLink/AArch64/ELF_aarch64_test1.s

3

@lhames Is there documentation for jitlink-check somewhere? I remember that it's using RuntimeDyldChecker under the hood and that it's a little complicated..

[lhames]

llvm/lib/ExecutionEngine/JITLink/ELF_aarch64.cpp

103–105

Do we have any existing bit-slicing utilities like this? It feels like we should, but I don't actually recall seeing them anywhere, and I don't think I've used anything like them in the MachO implementation.

It would be good to check. If there are existing ones we should use them. If there aren't then we should aim to move these into a place where they can be re-used (that can be done separately from this patch).

107–109

Are arm64 pages always 4k on Linux? On Darwin they're 16k.

111–113

This should be moved into the aarch64.h header and renamed isInRangeForImmSN for consistency with the existing x86_64 functions.

llvm/test/ExecutionEngine/JITLink/AArch64/ELF_aarch64_test1.s

3

There isn't any good documentation. There is a comment describing the language at https://github.com/llvm/llvm-project/blob/d1fdd745d510f40d8741d44ce39f5ae24ee7f91a/llvm/include/llvm/ExecutionEngine/RuntimeDyldChecker.h#L32

Usually the best thing to do is look for a comparable existing test. In this case the best one would be the MachO / arm64 test case:

https://github.com/llvm/llvm-project/blob/main/llvm/test/ExecutionEngine/JITLink/AArch64/MachO_arm64_relocations.s

11–21

I see a test for R_AARCH64_ADD_ABS_LO12_NC here, but not the other relocations -- it would be good to add tests for all of them.

The tests don't execute so the loads/stores/arithmetic can usually be omitted, and only the instructions that actually have relocations attached included.

[sgraenitz]

llvm/lib/ExecutionEngine/JITLink/ELF_aarch64.cpp

103–105

Not that I am aware of. Agree to put a little more effort into it. In fact, for range checks, both RuntimeDyld and LLD make frequent use of SignExtend64 from llvm/Support/MathExtras.h.

Looking at the LLD code, it mostly uses raw shifts and typically it won't extract bits. Instead it tends to keep existing bits untouched and just overwrites the ones it needs to modify. I think this approach would work here as well. If so, we probably should get rid of extractBit altogether.

107–109

Can we access the actual page size reported from the executor? ExecutorProcessControl has it and we could make it accessible from here either via JITLinkContext or JITLinkMemoryManager. Both appear out of scope for this patch, but we should at least add a FIXME here.

111–113

Yes it has been part of my initial patch. I will move it as suggested or remove it if we go the SignExtend way.

[sgraenitz]

@sunho implemented that in D126287. I think this review should be closed.

llvm/include/llvm/ExecutionEngine/JITLink/aarch64.h

 namespace jitlink {
 namespace aarch64 {
 
 /// Represets aarch64 fixups
 enum EdgeKind_aarch64 : Edge::Kind {
 
   /// Set a CALL immediate field to bits [27:2] of X = Target - Fixup + Addend
   R_AARCH64_CALL26 = Edge::FirstRelocation,
+  // Set an ADRP immediate value to bits [32:12] of the X  = Page(Target +
+  // Addend) - Page(Fixup), Page(x) = x & ~0xFFF
+  R_AARCH64_ADR_PREL_PG_HI21,
+  // Set an ADRP immediate value to bits [32:12] of the X  = Page(Target +
+  // Addend) - Page(Fixup), Page(x) = x & ~0xFFF
+  R_AARCH64_ADR_PREL_PG_HI21_NC,
+  // Set an ADD immediate value to bits [11:0] of X = Target + Addend
+  R_AARCH64_ADD_ABS_LO12_NC
 
 };
 
 /// Returns a string name for the given aarch64 edge. For debugging purposes
 /// only
 const char *getEdgeKindName(Edge::Kind K);
 
 } // namespace aarch64
 } // namespace jitlink
 } // namespace llvm
 
 #endif // LLVM_EXECUTIONENGINE_JITLINK_AARCH64_H

llvm/lib/ExecutionEngine/JITLink/ELF_aarch64.cpp

case aarch64::R_AARCH64_CALL26: {
         return make_error<JITLinkError>("Call target is not 32-bit aligned");
 
       if (!fitsRangeSignedInt<27>(Value))
         return makeTargetOutOfRangeError(G, B, E);
 
       uint32_t RawInstr = *(little32_t *)FixupPtr;
       assert((RawInstr & 0x7fffffff) == 0x14000000 &&
              "RawInstr isn't a B or BR immediate instruction");
-      uint32_t Imm = (static_cast<uint32_t>(Value) & ((1 << 28) - 1)) >> 2;
+      uint32_t Imm = extractBit<27, 2>(Value);
       uint32_t FixedInstr = RawInstr | Imm;
       *(little32_t *)FixupPtr = FixedInstr;
       break;
     }
+    case aarch64::R_AARCH64_ADR_PREL_PG_HI21: {
+      int64_t value =
+          getPageAddress(E.getTarget().getAddress() + E.getAddend()) -
+          getPageAddress(FixupAddress);
+
+      if (!fitsRangeSignedInt<32>(value))
+        return makeTargetOutOfRangeError(G, B, E);
+
+      uint32_t Imm = extractBit<32, 12>(value);
+      uint32_t RawInstr = *(little32_t *)FixupPtr;
+      uint32_t result = RawInstr | Imm;
+
+      *(little32_t *)FixupPtr = result;
+      break;
+    }
+    case aarch64::R_AARCH64_ADR_PREL_PG_HI21_NC: {
+      int64_t value =
+          getPageAddress(E.getTarget().getAddress() + E.getAddend()) -
+          getPageAddress(FixupAddress);
+
+      uint32_t Imm = extractBit<32, 12>(value);
+      uint32_t RawInstr = *(little32_t *)FixupPtr;
+      uint32_t result = RawInstr | Imm;
+
+      *(little32_t *)FixupPtr = result;
+      break;
+    }
+    case aarch64::R_AARCH64_ADD_ABS_LO12_NC: {
+      int64_t value = E.getTarget().getAddress() + E.getAddend();
+
+      uint32_t Imm = extractBit<11, 0>(value);
+      uint32_t RawInstr = *(little32_t *)FixupPtr;
+      uint32_t result = RawInstr | Imm;
+      break;
+    }
     }
     return Error::success();
   }
 
+  template <uint8_t hi, uint8_t lo> static uint32_t extractBit(uint64_t num) {
+    return (static_cast<uint32_t>(num) & ((1 << (hi + 1)) - 1)) >> lo;
+  }
+
+  constexpr JITTargetAddress getPageAddress(JITTargetAddress expr) const {
+    return expr & ~0xfff;
+  }
+
   template <uint8_t Bits> static bool fitsRangeSignedInt(int64_t Value) {
     return Value >= -(1 << Bits) && Value < (1 << Bits);
   }
 };
 
 template <typename ELFT>
 class ELFLinkGraphBuilder_aarch64 : public ELFLinkGraphBuilder<ELFT> {
 private:
   static Expected<aarch64::EdgeKind_aarch64>
   getRelocationKind(const uint32_t Type) {
     using namespace aarch64;
     switch (Type) {
     case ELF::R_AARCH64_CALL26:
       return EdgeKind_aarch64::R_AARCH64_CALL26;
+    case ELF::R_AARCH64_ADR_PREL_PG_HI21:
+      return EdgeKind_aarch64::R_AARCH64_ADR_PREL_PG_HI21;
+    case ELF::R_AARCH64_ADR_PREL_PG_HI21_NC:
+      return EdgeKind_aarch64::R_AARCH64_ADR_PREL_PG_HI21_NC;
+    case ELF::R_AARCH64_ADD_ABS_LO12_NC:
+      return EdgeKind_aarch64::R_AARCH64_ADD_ABS_LO12_NC;
     }
 
     return make_error<JITLinkError>("Unsupported aarch64 relocation:" +
                                     formatv("{0:d}", Type));
   }
 
   Error addRelocations() override {
     LLVM_DEBUG(dbgs() << "Processing relocations:\n");

llvm/test/ExecutionEngine/JITLink/AArch64/ELF_aarch64_test1.s

+# REQUIRES: asserts
+# RUN: llvm-mc -triple=aarch64 -filetype=obj -o %t %s
+# RUN: llvm-jitlink -debug-only=jitlink -noexec %t 2>&1 | FileCheck %s
+  .text
+  .file "test1.c"
+  .globl  main                    // -- Begin function main
+  .p2align  2
+  .type main,@function
+main:                                   // @main
+// %bb.0:
+  sub sp, sp, #16             // =16
+  adrp  x8, g
+  add x8, x8, :lo12:g
+  mov w9, wzr
+  str w0, [sp, #12]
+  str x1, [sp]
+  ldr w10, [x8]
+  add w10, w10, #1            // =1
+  str w10, [x8]
+  mov w0, w9
+  add sp, sp, #16             // =16
+  ret
+.Lfunc_end0:
+  .size main, .Lfunc_end0-main
+                                        // -- End function
+  .type g,@object               // @g
+  .data
+  .globl  g
+  .p2align  2
+g:
+  .word 1                       // 0x1
+  .size g, 4
+
+  .ident  "clang version 10.0.0-4ubuntu1 "
+  .section  ".note.GNU-stack","",@progbits
+  .addrsig
+  .addrsig_sym g