RuntimeDyldELF: implement R_AARCH64_PREL64 reloc

[evgeny777]

Summary

Also simplified the whole function a little.

[davide]

Please split this in two independent patches and I'll review both of them, i.e. please separate the functional changes from the NFC ones.

[evgeny777]

Here is a patch with functional changes only. I'll post NFC changes when this one lands.

[davide]

If we support BE, then you should probably add a test which exercises this reloc for a big endian target.
Otherwise, just change that to be LE (if that's the case, ideally, we should change that it everywhere for R_AARCH64*)

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp

382–384

is it possible for this to be big-endian?
i.e. shouldn't this always be write64le() as we already do in lld?

[evgeny777]

is it possible for this to be big-endian?

Looks like aarch64_be is one of the targets supported by LLVM.
Are you saying that reloc values should still be written as little-endian on aarch64_be? If so who will convert them back to host byte order? CPU?

[davide]

In D28122#631779, @evgeny777 wrote:

is it possible for this to be big-endian?

Looks like aarch64_be is one of the targets supported by LLVM.
Are you saying that reloc values should still be written as little-endian on aarch64_be? If so who will convert them back to host byte order? CPU?

No, on aarch64-be we should write the bytes in big-endian order. But you should add a test for aarch64_be (for this, and all the other untested BE

[davide]

In D28122#631780, @davide wrote:

In D28122#631779, @evgeny777 wrote:

is it possible for this to be big-endian?

Looks like aarch64_be is one of the targets supported by LLVM.
Are you saying that reloc values should still be written as little-endian on aarch64_be? If so who will convert them back to host byte order? CPU?

No, on aarch64-be we should write the bytes in big-endian order. But you should add a test for aarch64_be (for this, and all the other untested BE

untested relocations on BE, if any (please add the tests in a separate commit/review to keep the diff readable).

[evgeny777]

Ah, got it. Let's stick to LE then

[davide]

In D28122#631780, @davide wrote:

In D28122#631779, @evgeny777 wrote:

is it possible for this to be big-endian?

Looks like aarch64_be is one of the targets supported by LLVM.
Are you saying that reloc values should still be written as little-endian on aarch64_be? If so who will convert them back to host byte order? CPU?

No, on aarch64-be we should write the bytes in big-endian order. But you should add a test for aarch64_be (for this, and all the other untested BE

untested relocations on BE, if any (please add the tests in a separate commit/review to keep the diff readable).

In D28122#631787, @evgeny777 wrote:

Ah, got it. Let's stick to LE then

ehm, that's not quite what I meant, sorry if it wasn't clear enough.What I'm suggesting is adding a test for BE for this relocation, and then audit all the other relocations not tested in aarch64-be configuration and add test for those.

The reason why lld uses write64le is that (I think) it supports only aarch64le so any aarch64be object will be rejected (or it's supposed to).
With your change you end up with a mix of endian-agnostic relocations and a relocation which will assume little-endian independently from the input.
If somebody passes a BE input that could result in a silent misapplication of a relocation, very painful to track down.

[evgeny777]

I've added BE test for PREL64 and BE/LE tests for PREL32 (feature was there already, but wasn't tested)

So currently supported AArch64 relocs are:
ABS64: LE/BE
ABS32: LE/BE
PREL32: LE/BE
PREL64: LE/BE
CALL26/JUMP26 : LE
MOVW_UABS_G3: LE (BE test is incorrect: it passes, but checks for invalid values)
MOVW_UABS_G2_NC: LE (BE test is incorrect: it passes, but checks for invalid values)
MOVW_UABS_G1_NC: LE (BE test is incorrect: it passes, but checks for invalid values)
MOVW_UABS_G0_NC: LE (BE test is incorrect: it passes, but checks for invalid values)
PREL_PG_HI21: LE (not tested)
ADD_ABS_LO12_NC: LE
LDST32_ABS_LO12_NC: LE (not tested)
LDST64_ABS_LO12_NC: LE (not tested)

Hope this helps

[davide]

In D28122#632151, @evgeny777 wrote:

I've added BE test for PREL64 and BE/LE tests for PREL32 (feature was there already, but wasn't tested)

So currently supported AArch64 relocs are:
ABS64: LE/BE
ABS32: LE/BE
PREL32: LE/BE
PREL64: LE/BE
CALL26/JUMP26 : LE
MOVW_UABS_G3: LE (BE test is incorrect: it passes, but checks for invalid values)
MOVW_UABS_G2_NC: LE (BE test is incorrect: it passes, but checks for invalid values)
MOVW_UABS_G1_NC: LE (BE test is incorrect: it passes, but checks for invalid values)
MOVW_UABS_G0_NC: LE (BE test is incorrect: it passes, but checks for invalid values)
PREL_PG_HI21: LE (not tested)
ADD_ABS_LO12_NC: LE
LDST32_ABS_LO12_NC: LE (not tested)
LDST64_ABS_LO12_NC: LE (not tested)

Hope this helps

I think we should really clean up all this technical debt before adding other new relocations/features to the JIT.
I think you may want to send reviews for fixing the tests/bugs in the existing BE relocations, add tests for the missing ones and then we can land this one. Thanks for your cooperation.

[evgeny777]

I think you may want to send reviews for fixing the tests/bugs in the existing BE relocations

Sorry I've misinformed you: A64 instructions are always little-endian. The difference occurs only for data relocations: ABS32/64 and PREL32/64. I've added all missing test cases in r291438

[evgeny777]

Diff updated to reflect latest changes

[davide]

LGTM. Lang?

[yuyichao]

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp

353

Sorry I've misinformed you: A64 instructions are always little-endian. The difference occurs only for data relocations: ABS32/64 and PREL32/64. I've added all missing test cases in r291438

FYI. I included this comment trying to avoid precisely this confusion......

Also FWIW, this was part of https://reviews.llvm.org/D27629 ......

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp

 }
 
 void RuntimeDyldELF::resolveAArch64Relocation(const SectionEntry &Section,
                                               uint64_t Offset, uint64_t Value,
                                               uint32_t Type, int64_t Addend) {
   uint32_t *TargetPtr =
       reinterpret_cast<uint32_t *>(Section.getAddressWithOffset(Offset));
   uint64_t FinalAddress = Section.getLoadAddressWithOffset(Offset);
   // Data should use target endian. Code should always use little endian.
   bool isBE = Arch == Triple::aarch64_be;
 
   DEBUG(dbgs() << "resolveAArch64Relocation, LocalAddress: 0x"
                << format("%llx", Section.getAddressWithOffset(Offset))
                << " FinalAddress: 0x" << format("%llx", FinalAddress)
                << " Value: 0x" << format("%llx", Value) << " Type: 0x"
                << format("%x", Type) << " Addend: 0x" << format("%llx", Addend)
                << "\n");
 
+  const uint64_t AbsVal = Value + Addend;
+  const uint64_t RelVal = AbsVal - FinalAddress;
+
   switch (Type) {
   default:
     llvm_unreachable("Relocation type not implemented yet!");
     break;
   case ELF::R_AARCH64_ABS64:
     write(isBE, TargetPtr, Value + Addend);
     break;
-  case ELF::R_AARCH64_PREL32: {
-    uint64_t Result = Value + Addend - FinalAddress;
-    assert(static_cast<int64_t>(Result) >= INT32_MIN &&
-           static_cast<int64_t>(Result) <= UINT32_MAX);
-    write(isBE, TargetPtr, static_cast<uint32_t>(Result & 0xffffffffU));
+  case ELF::R_AARCH64_PREL32:
+    assert(static_cast<int64_t>(RelVal) >= INT32_MIN &&
+           static_cast<int64_t>(RelVal) <= UINT32_MAX);
+    write(isBE, TargetPtr, static_cast<uint32_t>(RelVal & 0xffffffffU));
+    break;
+  case ELF::R_AARCH64_PREL64:
+    write(isBE, TargetPtr, RelVal);
     break;
-  }
   case ELF::R_AARCH64_CALL26: // fallthrough
   case ELF::R_AARCH64_JUMP26: {
     // Operation: S+A-P. Set Call or B immediate value to bits fff_fffc of the
     // calculation.
-    uint64_t BranchImm = Value + Addend - FinalAddress;
-
     // "Check that -2^27 <= result < 2^27".
-    assert(isInt<28>(BranchImm));
-    or32le(TargetPtr, (BranchImm & 0x0FFFFFFC) >> 2);
+    assert(isInt<28>(RelVal));
+    or32le(TargetPtr, (RelVal & 0x0FFFFFFC) >> 2);
     break;
   }
   case ELF::R_AARCH64_MOVW_UABS_G3:
-    or32le(TargetPtr, ((Value + Addend) & 0xFFFF000000000000) >> 43);
+    or32le(TargetPtr, (AbsVal & 0xFFFF000000000000) >> 43);
     break;
   case ELF::R_AARCH64_MOVW_UABS_G2_NC:
-    or32le(TargetPtr, ((Value + Addend) & 0xFFFF00000000) >> 27);
+    or32le(TargetPtr, (AbsVal & 0xFFFF00000000) >> 27);
     break;
   case ELF::R_AARCH64_MOVW_UABS_G1_NC:
-    or32le(TargetPtr, ((Value + Addend) & 0xFFFF0000) >> 11);
+    or32le(TargetPtr, (AbsVal & 0xFFFF0000) >> 11);
     break;
   case ELF::R_AARCH64_MOVW_UABS_G0_NC:
-    or32le(TargetPtr, ((Value + Addend) & 0xFFFF) << 5);
+    or32le(TargetPtr, (AbsVal & 0xFFFF) << 5);
     break;
   case ELF::R_AARCH64_ADR_PREL_PG_HI21: {
     // Operation: Page(S+A) - Page(P)
     uint64_t Result =
-        ((Value + Addend) & ~0xfffULL) - (FinalAddress & ~0xfffULL);
+        (AbsVal & ~0xfffULL) - (FinalAddress & ~0xfffULL);
 
     // Check that -2^32 <= X < 2^32
     assert(isInt<33>(Result) && "overflow check failed for relocation");
 
     // Immediate goes in bits 30:29 + 5:23 of ADRP instruction, taken
     // from bits 32:12 of X.
     write32AArch64Addr(TargetPtr, Result >> 12);
     break;
   }
   case ELF::R_AARCH64_ADD_ABS_LO12_NC:
     // Operation: S + A
     // Immediate goes in bits 21:10 of LD/ST instruction, taken
     // from bits 11:0 of X
-    or32AArch64Imm(TargetPtr, Value + Addend);
+    or32AArch64Imm(TargetPtr, AbsVal);
     break;
   case ELF::R_AARCH64_LDST32_ABS_LO12_NC:
     // Operation: S + A
     // Immediate goes in bits 21:10 of LD/ST instruction, taken
     // from bits 11:2 of X
-    or32AArch64Imm(TargetPtr, getBits(Value + Addend, 2, 11));
+    or32AArch64Imm(TargetPtr, getBits(AbsVal, 2, 11));
     break;
   case ELF::R_AARCH64_LDST64_ABS_LO12_NC:
     // Operation: S + A
     // Immediate goes in bits 21:10 of LD/ST instruction, taken
     // from bits 11:3 of X
-    or32AArch64Imm(TargetPtr, getBits(Value + Addend, 3, 11));
+    or32AArch64Imm(TargetPtr, getBits(AbsVal, 3, 11));
     break;
   }
 }
 
 void RuntimeDyldELF::resolveARMRelocation(const SectionEntry &Section,
                                           uint64_t Offset, uint32_t Value,
                                           uint32_t Type, int32_t Addend) {
   // TODO: Add Thumb relocations.

test/ExecutionEngine/RuntimeDyld/AArch64/ELF_ARM64_relocations.s

# R_AARCH64_ADD_ABS_LO12_NC
         .size   g, .Lfunc_end0-g
 
         .type   k,@object
         .data
         .globl  k
         .p2align        3
 k:
         .xword  f
-        .size   k, 8
+        .xword  f - .
+        .size   k, 16
 
 # rtdyld-check: *{4}(g) = 0xd2e02460
 # rtdyld-check: *{4}(g + 4) = 0xf2c8ace0
 # rtdyld-check: *{4}(g + 8) = 0xf2b13560
 # rtdyld-check: *{4}(g + 12) = 0xf299bde0
 # rtdyld-check: *{8}k = f
+# rtdyld-check: *{8}(k + 8) = f - (k + 8)
 
 ## f & 0xFFF = 0xdef (bits 11:0 of f)
 ## 0xdef << 10 = 0x37bc00
 # rtdyld-check: *{4}(a) = 0x9137bc00