[PATCH] D112451: implemented R_AARCH64_ADR_PREL_PG_HI21, R_AARCH64_ADR_PREL_PG_HI21_NC, R_AARCH64_ADD_ABS_LO12_NC

[Lang Hames via Phabricator via llvm-commits]

lhames added inline comments.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_aarch64.cpp:103-105
+  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;
+  }
----------------
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).


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_aarch64.cpp:107-109
+  constexpr JITTargetAddress getPageAddress(JITTargetAddress expr) const {
+    return expr & ~0xfff;
+  }
----------------
Are arm64 pages always 4k on Linux? On Darwin they're 16k.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_aarch64.cpp:111-113
   template <uint8_t Bits> static bool fitsRangeSignedInt(int64_t Value) {
     return Value >= -(1 << Bits) && Value < (1 << Bits);
   }
----------------
This should be moved into the aarch64.h header and renamed `isInRangeForImmSN` for consistency with the existing x86_64 functions.


================
Comment at: llvm/test/ExecutionEngine/JITLink/AArch64/ELF_aarch64_test1.s:3
+# RUN: llvm-mc -triple=aarch64 -filetype=obj -o %t %s
+# RUN: llvm-jitlink -debug-only=jitlink -noexec %t 2>&1 | FileCheck %s
+	.text
----------------
sgraenitz wrote:
> sgraenitz wrote:
> > 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
> @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..
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


================
Comment at: llvm/test/ExecutionEngine/JITLink/AArch64/ELF_aarch64_test1.s:11-21
+	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]
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112451/new/

https://reviews.llvm.org/D112451