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

360

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

427–428

(10 - 2) -> 8

439–440

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

test/ExecutionEngine/RuntimeDyld/AArch64/ELF_ARM64_relocations.s

1–2 ↗

(On Diff #81225)

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

9–15 ↗

(On Diff #81225)

Can you add comments pointing out the relocations emitted?

[yuyichao]

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp

439–440

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

1–2 ↗

(On Diff #81225)

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.

9–15 ↗

(On Diff #81225)

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

9–15 ↗

(On Diff #81225)

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

439–440

Fine.

test/ExecutionEngine/RuntimeDyld/AArch64/ELF_ARM64_relocations.s

9–15 ↗

(On Diff #81225)

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

9–15 ↗

(On Diff #81225)

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. :)

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp

void RuntimeDyldELF::resolveAArch64Relocation(const SectionEntry &Section,
   }
   case ELF::R_AARCH64_MOVW_UABS_G3: {
     uint64_t Result = Value + Addend;
 
     // AArch64 code is emitted with .rela relocations. The data already in any
     // bits affected by the relocation on entry is garbage.
     *TargetPtr &= 0xffe0001fU;
     // Immediate goes in bits 20:5 of MOVZ/MOVK instruction
-    *TargetPtr |= Result >> (48 - 5);
+    *TargetPtr |= ((Result & 0xffff000000000000ULL) >> (48 - 5));
     // Shift must be "lsl #48", in bits 22:21
     assert((*TargetPtr >> 21 & 0x3) == 3 && "invalid shift for relocation");
     break;
   }
   case ELF::R_AARCH64_MOVW_UABS_G2_NC: {
     uint64_t Result = Value + Addend;
 
     // AArch64 code is emitted with .rela relocations. The data already in any
case ELF::R_AARCH64_LDST32_ABS_LO12_NC: {
     // Operation: S + A
     uint64_t Result = Value + Addend;
 
     // AArch64 code is emitted with .rela relocations. The data already in any
     // bits affected by the relocation on entry is garbage.
     *TargetPtr &= 0xffc003ffU;
     // Immediate goes in bits 21:10 of LD/ST instruction, taken
     // from bits 11:2 of X
     *TargetPtr |= ((Result & 0xffc) << (10 - 2));
     break;
   }
   case ELF::R_AARCH64_LDST64_ABS_LO12_NC: {
     // Operation: S + A
     uint64_t Result = Value + Addend;
 
     // AArch64 code is emitted with .rela relocations. The data already in any
     // bits affected by the relocation on entry is garbage.
     *TargetPtr &= 0xffc003ffU;
     // Immediate goes in bits 21:10 of LD/ST instruction, taken
     // from bits 11:3 of X
     *TargetPtr |= ((Result & 0xff8) << (10 - 3));
     break;
   }
   }
 }
 
 void RuntimeDyldELF::resolveARMRelocation(const SectionEntry &Section,
                                           uint64_t Offset, uint32_t Value,
                                           uint32_t Type, int32_t Addend) {
   // TODO: Add Thumb relocations.