[PATCH] D118346: [ORC] Support ADR_PREL_PG_HI21 & ADD_ABS_LO12_NC for aarch64

[Lang Hames via Phabricator via llvm-commits]

lhames added a comment.

Reviewing the patch as-is because I think it's a great addition. If work on the ELF/aarch64 backend is starting in earnest though I would prefer to create a new generic aarch64.h header (along the lines of riscv.h and x86_64.h) and populate it with the existing MachO relocations -- the fixup logic is generally sharable, so there shouldn't be any need to duplicate it, though ELF will probably require some extra relocations beyond the set MachO uses.



================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_aarch64.cpp:46-48
+    uint32_t RawInstr = *(little32_t *)FixupPtr;
+    auto Addend = E.getAddend();
+    auto SymbolAddress = E.getTarget().getAddress();
----------------
To keep the fixup code idiomatic I would reference these values directly in each of the switch cases that use them, rather than naming them before the start of the switch.

This is particularly true for `RawInstr`: The fixup content isn't used for every relocation, and not all relocations are fixing up instructions.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_aarch64.cpp:84
+      uint32_t Mask = ((0x1FFFFC >> 2) << 5) | (0x3U << 29);
+      RawInstr |= ~Mask;
+      *(little32_t *)FixupPtr = RawInstr | ImmLo | ImmHi;
----------------
Should this be ` RawInstr &= ~Mask`?


================
Comment at: llvm/test/ExecutionEngine/JITLink/AArch64/ELF_aarch64_adrp.s:8
+# Test Case 1: R_AARCH64_ADR_PREL_PG_HI21
+# jitlink-check: decode_operand(main, 1) = (((str + 0) & 0xFFFFFFFFFFFFF000) - (main & 0xFFFFFFFFFFFFF000))[32:12]
+#
----------------
dongAxis1944 wrote:
> Hello, since I do not write raw assembly by hand, this case might be optimized.
> 
> 1.  The addend of this case is 0, I do not know how to set it to the other value;
> 2.  The target and str are on the same page, so "decode_operand(main, 1)" is always zero, should I put the two symbols on different pages?
This doesn't pass on my machine. Is it passing for you locally?

1. I think you could do it by referencing an offset from str? (That's how I'd do it on MachO, but I haven't checked on ELF).

2. You could use a fill directive to insert some filler in a way that would guarantee that str would be on a different page. That's worth testing too (both the "same page" and "different page" cases.

Your testcase doesn't actually need to run, so I'd make main separate and name each instruction to emphasize the test;

```
# RUN: rm -rf %t && mkdir -p %t
# RUN: llvm-mc -triple=aarch64-unknown-linux-gnu -relax-relocations=false -position-independent -filetype=obj -o %t/elf_aarch64_adrp.o %s
# RUN: llvm-jitlink -noexec -check %s %t/elf_aarch64_adrp.o

        .text

# Add a no-op main as an entry-point.
        .globl    main
        .p2align        2
main:
        ret

# Test R_AARCH64_ADR_PREL_PG_HI21
# jitlink-check: decode_operand(test_page21, 1) = str[32:12] - test_page21[32:12]
        .globl  test_page21
        .p2align        2
        .type test_page21, at function
test_page21:
        adrp    x0, str
        .size   test_page21, .-test_page21

# Test R_AARCH64_ADD_ABS_LO12_NC
# jitlink-check: decode_operand(test_pageoff12, 2) = str[11:0]
        .globl  test_pageoff12
        .p2align        2
        .type test_pageoff12, at function
test_pageoff12:
        add     x0, x0, :lo12:str
        .size   test_pageoff12, .-test_pageoff12

        .globl  str
        .type   str, at object
str:
        .asciz  "hello world"
        .size   str, 12
```


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

https://reviews.llvm.org/D118346