Fix R_AARCH64_MOVW_UABS_G3 relocation

[yuyichao]

Summary

The relocation is missing mask so an address that has non-zero bits in 47:43 may overwrite the register number. (Frequently shows up as target register changed to xzr....)

[yuyichao]

Tests added and endianess issue fixed.

[yuyichao]

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp

389

Now that the diff is much bigger I should point out that this line is the actual bug fix (adding mask). Other changes are necessary so that the added test can be run without causing trouble on big endian systems.

[yuyichao]

Use target endian for data relocation.

[yuyichao]

Ping.

[davide]

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp

466–468

(10 - 2) -> 8

480–482

any reason why (10 - 3) is not spelled 7 (here and everywhere else)

test/ExecutionEngine/RuntimeDyld/AArch64/ELF_ARM64_relocations.s

2–3

We need tests for both little and big endian, no?

10–16

Can you add comments pointing out the relocations emitted?

[yuyichao]

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp

480–482

I believe it's because these are the last bits of the source (3) and the destination (10). I'm fine with either but I think the 10 - 3 is reasonably clear what it is doing along with the comment above each of these.

test/ExecutionEngine/RuntimeDyld/AArch64/ELF_ARM64_relocations.s

2–3

Sure. I don't have hardware to actually check if it's actually the correct output but I'll just reference clang/gcc output in be mode.

10–16

Sorry, what exactly do you mean? The relocations are explicitly specified in the code (#:abs_g3: etc.). Do you mean to write the name like R_AARCH64_MOVW_UABS_G3? Or something else?

[yuyichao]

test/ExecutionEngine/RuntimeDyld/AArch64/ELF_ARM64_relocations.s

10–16

Or do you mean the disassemble of the relocated instructions?

[davide]

I don't have additional comments but please wait for Tim/Lang to sign this off.

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp

480–482

Fine.

test/ExecutionEngine/RuntimeDyld/AArch64/ELF_ARM64_relocations.s

10–16

The former (spelling the as R_AARCH64_FOO)

[davide]

(and add the required tests please)

[yuyichao]

(and add the required tests please)

Sure.

test/ExecutionEngine/RuntimeDyld/AArch64/ELF_ARM64_relocations.s

10–16

OK. Will do.

[yuyichao]

Tests added. Also discovered and had to fix two other bugs since the arch detection was wrong for aarch64_be ELF file....

[lhames]

LGTM. Thanks Yichao. :)

include/llvm/Object/ELFObjectFile.h

unsigned ELFObjectFile<ELFT>::getArch() const {
   bool IsLittleEndian = ELFT::TargetEndianness == support::little;
   switch (EF.getHeader()->e_machine) {
   case ELF::EM_386:
   case ELF::EM_IAMCU:
     return Triple::x86;
   case ELF::EM_X86_64:
     return Triple::x86_64;
   case ELF::EM_AARCH64:
-    return Triple::aarch64;
+    return IsLittleEndian ? Triple::aarch64 : Triple::aarch64_be;
   case ELF::EM_ARM:
     return Triple::arm;
   case ELF::EM_AVR:
     return Triple::avr;
   case ELF::EM_HEXAGON:
     return Triple::hexagon;
   case ELF::EM_LANAI:
     return Triple::lanai;

include/llvm/Object/RelocVisitor.h

if (ObjToVisit.getBytesInAddress() == 8) { // 64-bit object file
           return visitELF_X86_64_32(R, Value);
         case llvm::ELF::R_X86_64_32S:
           return visitELF_X86_64_32S(R, Value);
         default:
           HasError = true;
           return RelocToApply();
         }
       case Triple::aarch64:
+      case Triple::aarch64_be:
         switch (RelocType) {
         case llvm::ELF::R_AARCH64_ABS32:
           return visitELF_AARCH64_ABS32(R, Value);
         case llvm::ELF::R_AARCH64_ABS64:
           return visitELF_AARCH64_ABS64(R, Value);
         default:
           HasError = true;
           return RelocToApply();

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");
 
   switch (Type) {
   default:
     llvm_unreachable("Relocation type not implemented yet!");
     break;
   case ELF::R_AARCH64_ABS64: {
     uint64_t *TargetPtr =
         reinterpret_cast<uint64_t *>(Section.getAddressWithOffset(Offset));
-    *TargetPtr = Value + Addend;
+    if (isBE)
+      support::ubig64_t::ref{TargetPtr} = Value + Addend;
+    else
+      support::ulittle64_t::ref{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);
-    *TargetPtr = static_cast<uint32_t>(Result & 0xffffffffU);
+    if (isBE)
+      support::ubig32_t::ref{TargetPtr} =
+        static_cast<uint32_t>(Result & 0xffffffffU);
+    else
+      support::ulittle32_t::ref{TargetPtr} =
+        static_cast<uint32_t>(Result & 0xffffffffU);
     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;
+    uint32_t TargetValue = support::ulittle32_t::ref{TargetPtr};
 
     // "Check that -2^27 <= result < 2^27".
     assert(isInt<28>(BranchImm));
 
     // AArch64 code is emitted with .rela relocations. The data already in any
     // bits affected by the relocation on entry is garbage.
-    *TargetPtr &= 0xfc000000U;
+    TargetValue &= 0xfc000000U;
     // Immediate goes in bits 25:0 of B and BL.
-    *TargetPtr |= static_cast<uint32_t>(BranchImm & 0xffffffcU) >> 2;
+    TargetValue |= static_cast<uint32_t>(BranchImm & 0xffffffcU) >> 2;
+    support::ulittle32_t::ref{TargetPtr} = TargetValue;
     break;
   }
   case ELF::R_AARCH64_MOVW_UABS_G3: {
     uint64_t Result = Value + Addend;
+    uint32_t TargetValue = support::ulittle32_t::ref{TargetPtr};
 
     // AArch64 code is emitted with .rela relocations. The data already in any
     // bits affected by the relocation on entry is garbage.
-    *TargetPtr &= 0xffe0001fU;
+    TargetValue &= 0xffe0001fU;
     // Immediate goes in bits 20:5 of MOVZ/MOVK instruction
-    *TargetPtr |= Result >> (48 - 5);
+    TargetValue |= ((Result & 0xffff000000000000ULL) >> (48 - 5));
     // Shift must be "lsl #48", in bits 22:21
-    assert((*TargetPtr >> 21 & 0x3) == 3 && "invalid shift for relocation");
+    assert((TargetValue >> 21 & 0x3) == 3 && "invalid shift for relocation");
+    support::ulittle32_t::ref{TargetPtr} = TargetValue;
     break;
   }
   case ELF::R_AARCH64_MOVW_UABS_G2_NC: {
     uint64_t Result = Value + Addend;
+    uint32_t TargetValue = support::ulittle32_t::ref{TargetPtr};
 
     // AArch64 code is emitted with .rela relocations. The data already in any
     // bits affected by the relocation on entry is garbage.
-    *TargetPtr &= 0xffe0001fU;
+    TargetValue &= 0xffe0001fU;
     // Immediate goes in bits 20:5 of MOVZ/MOVK instruction
-    *TargetPtr |= ((Result & 0xffff00000000ULL) >> (32 - 5));
+    TargetValue |= ((Result & 0xffff00000000ULL) >> (32 - 5));
     // Shift must be "lsl #32", in bits 22:21
-    assert((*TargetPtr >> 21 & 0x3) == 2 && "invalid shift for relocation");
+    assert((TargetValue >> 21 & 0x3) == 2 && "invalid shift for relocation");
+    support::ulittle32_t::ref{TargetPtr} = TargetValue;
     break;
   }
   case ELF::R_AARCH64_MOVW_UABS_G1_NC: {
     uint64_t Result = Value + Addend;
+    uint32_t TargetValue = support::ulittle32_t::ref{TargetPtr};
 
     // AArch64 code is emitted with .rela relocations. The data already in any
     // bits affected by the relocation on entry is garbage.
-    *TargetPtr &= 0xffe0001fU;
+    TargetValue &= 0xffe0001fU;
     // Immediate goes in bits 20:5 of MOVZ/MOVK instruction
-    *TargetPtr |= ((Result & 0xffff0000U) >> (16 - 5));
+    TargetValue |= ((Result & 0xffff0000U) >> (16 - 5));
     // Shift must be "lsl #16", in bits 22:2
-    assert((*TargetPtr >> 21 & 0x3) == 1 && "invalid shift for relocation");
+    assert((TargetValue >> 21 & 0x3) == 1 && "invalid shift for relocation");
+    support::ulittle32_t::ref{TargetPtr} = TargetValue;
     break;
   }
   case ELF::R_AARCH64_MOVW_UABS_G0_NC: {
     uint64_t Result = Value + Addend;
+    uint32_t TargetValue = support::ulittle32_t::ref{TargetPtr};
 
     // AArch64 code is emitted with .rela relocations. The data already in any
     // bits affected by the relocation on entry is garbage.
-    *TargetPtr &= 0xffe0001fU;
+    TargetValue &= 0xffe0001fU;
     // Immediate goes in bits 20:5 of MOVZ/MOVK instruction
-    *TargetPtr |= ((Result & 0xffffU) << 5);
+    TargetValue |= ((Result & 0xffffU) << 5);
     // Shift must be "lsl #0", in bits 22:21.
-    assert((*TargetPtr >> 21 & 0x3) == 0 && "invalid shift for relocation");
+    assert((TargetValue >> 21 & 0x3) == 0 && "invalid shift for relocation");
+    support::ulittle32_t::ref{TargetPtr} = TargetValue;
     break;
   }
   case ELF::R_AARCH64_ADR_PREL_PG_HI21: {
     // Operation: Page(S+A) - Page(P)
     uint64_t Result =
         ((Value + Addend) & ~0xfffULL) - (FinalAddress & ~0xfffULL);
+    uint32_t TargetValue = support::ulittle32_t::ref{TargetPtr};
 
     // Check that -2^32 <= X < 2^32
     assert(isInt<33>(Result) && "overflow check failed for relocation");
 
     // AArch64 code is emitted with .rela relocations. The data already in any
     // bits affected by the relocation on entry is garbage.
-    *TargetPtr &= 0x9f00001fU;
+    TargetValue &= 0x9f00001fU;
     // Immediate goes in bits 30:29 + 5:23 of ADRP instruction, taken
     // from bits 32:12 of X.
-    *TargetPtr |= ((Result & 0x3000U) << (29 - 12));
-    *TargetPtr |= ((Result & 0x1ffffc000ULL) >> (14 - 5));
+    TargetValue |= ((Result & 0x3000U) << (29 - 12));
+    TargetValue |= ((Result & 0x1ffffc000ULL) >> (14 - 5));
+    support::ulittle32_t::ref{TargetPtr} = TargetValue;
     break;
   }
   case ELF::R_AARCH64_LDST32_ABS_LO12_NC: {
     // Operation: S + A
     uint64_t Result = Value + Addend;
+    uint32_t TargetValue = support::ulittle32_t::ref{TargetPtr};
 
     // AArch64 code is emitted with .rela relocations. The data already in any
     // bits affected by the relocation on entry is garbage.
-    *TargetPtr &= 0xffc003ffU;
+    TargetValue &= 0xffc003ffU;
     // Immediate goes in bits 21:10 of LD/ST instruction, taken
     // from bits 11:2 of X
-    *TargetPtr |= ((Result & 0xffc) << (10 - 2));
+    TargetValue |= ((Result & 0xffc) << (10 - 2));
+    support::ulittle32_t::ref{TargetPtr} = TargetValue;
     break;
   }
   case ELF::R_AARCH64_LDST64_ABS_LO12_NC: {
     // Operation: S + A
     uint64_t Result = Value + Addend;
+    uint32_t TargetValue = support::ulittle32_t::ref{TargetPtr};
 
     // AArch64 code is emitted with .rela relocations. The data already in any
     // bits affected by the relocation on entry is garbage.
-    *TargetPtr &= 0xffc003ffU;
+    TargetValue &= 0xffc003ffU;
     // Immediate goes in bits 21:10 of LD/ST instruction, taken
     // from bits 11:3 of X
-    *TargetPtr |= ((Result & 0xff8) << (10 - 3));
+    TargetValue |= ((Result & 0xff8) << (10 - 3));
+    support::ulittle32_t::ref{TargetPtr} = TargetValue;
     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_BE-relocations.s

+# RUN: llvm-mc -triple=aarch64_be-none-linux-gnu -filetype=obj -o %T/be-reloc.o %s
+# RUN: llvm-rtdyld -triple=aarch64_be-none-linux-gnu -verify -dummy-extern f=0x0123456789abcdef -check=%s %T/be-reloc.o
+
+        .text
+        .globl  g
+        .p2align        2
+        .type   g,@function
+g:
+# R_AARCH64_MOVW_UABS_G3
+        movz    x0, #:abs_g3:f
+# R_AARCH64_MOVW_UABS_G2_NC
+        movk    x0, #:abs_g2_nc:f
+# R_AARCH64_MOVW_UABS_G1_NC
+        movk    x0, #:abs_g1_nc:f
+# R_AARCH64_MOVW_UABS_G0_NC
+        movk    x0, #:abs_g0_nc:f
+        ret
+        .Lfunc_end0:
+        .size   g, .Lfunc_end0-g
+
+        .type   k,@object
+        .data
+        .globl  k
+        .p2align        3
+k:
+        .xword  f
+        .size   k, 8
+
+# LE instructions read as BE
+# rtdyld-check: *{4}(g) = 0x6024e0d2
+# rtdyld-check: *{4}(g + 4) = 0xe0acc8f2
+# rtdyld-check: *{4}(g + 8) = 0x6035b1f2
+# rtdyld-check: *{4}(g + 12) = 0xe0bd99f2
+# rtdyld-check: *{8}k = f

test/ExecutionEngine/RuntimeDyld/AArch64/ELF_ARM64_relocations.s

+# RUN: llvm-mc -triple=arm64-none-linux-gnu -filetype=obj -o %T/reloc.o %s
+# RUN: llvm-rtdyld -triple=arm64-none-linux-gnu -verify -dummy-extern f=0x0123456789abcdef -check=%s %T/reloc.o
+
+        .text
+        .globl  g
+        .p2align        2
+        .type   g,@function
+g:
+# R_AARCH64_MOVW_UABS_G3
+        movz    x0, #:abs_g3:f
+# R_AARCH64_MOVW_UABS_G2_NC
+        movk    x0, #:abs_g2_nc:f
+# R_AARCH64_MOVW_UABS_G1_NC
+        movk    x0, #:abs_g1_nc:f
+# R_AARCH64_MOVW_UABS_G0_NC
+        movk    x0, #:abs_g0_nc:f
+        ret
+        .Lfunc_end0:
+        .size   g, .Lfunc_end0-g
+
+        .type   k,@object
+        .data
+        .globl  k
+        .p2align        3
+k:
+        .xword  f
+        .size   k, 8
+
+# 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