ELF/AArch64: Refactor R_AARCH64_LDST{8,15,32,64,128}_ABS_LO12_NC Relocations

[zatrazz]

Summary

This patch refactor how to apply the R_AARCH64_LDST{8,16,32,64,128}_ABS_NC
relocations by adding a new function to correct extract the bits expected
by each relocation. This make is explicit which are the bits range expected
and simplify the code to mask and shift the deriable values.

It also fixes the R_AARCH64_LDST128_ABS_LO12_NC mask, although in pratice
the mask/shift always returns a 16 bytes aligned value.

Checked on AArch64 and with test-suite.

[ruiu]

ELF/Target.cpp

1329–1338

Maybe we should add this to llvm/Support/MathExtras.h? We have a bunch of bit manipulation functions there.

I don't see a reason to make it a template. Start and End can be passed as arguments.

Please run clang-format-diff to format in the LLVM style.

You don't need a parentheses around (End + 1) because End - Start + 1 is the same as (End + 1) - Start.

[ruiu]

ELF/Target.cpp

1360

This code is not the same as before. Was the previous code wrong?

[zatrazz]

ELF/Target.cpp

1329–1338

I do not have a strong preference where to add it, although I see this implementation kinda specialized as it extracts some bits and also shift to a value (not only apply a mask).

And I made it a template so compiler have a better time optimizing it depending of the mask. For instance on x86_64, getBits<0, 11> could optimized to just:

movq    %rdi, %rax
andl    $4095, %eax

While the argument passing counterpart would have much more instructions.

I will rerun clang-format-diff and remove the parentheses.

1360

Just the function name changed for this snippet, from updateAArch64Add to updateAArch64LdStrAdd (since I am now using it to update the immediate field to load/store instructions as well).

[zatrazz]

Updated patch based on previous comments.

[ruiu]

LGTM

ELF/Target.cpp

 
 static void updateAArch64Addr(uint8_t *L, uint64_t Imm) {
   uint32_t ImmLo = (Imm & 0x3) << 29;
   uint32_t ImmHi = (Imm & 0x1FFFFC) << 3;
   uint64_t Mask = (0x3 << 29) | (0x1FFFFC << 3);
   write32le(L, (read32le(L) & ~Mask) | ImmLo | ImmHi);
 }
 
-static inline void updateAArch64Add(uint8_t *L, uint64_t Imm) {
+// Return the bits [Start, End] from Val shifted Start bits.  For instance,
+// getBits<4,8>(0xF0) returns 0xF.
+template <uint8_t Start, uint8_t End> static uint64_t getBits(uint64_t Val) {
+  uint64_t Mask = ((uint64_t)1 << (End + 1 - Start)) - 1;
+  return (Val >> Start) & Mask;
+}
+
+// Update the immediate field in a ldr, str, and add instruction.
+static inline void updateAArch64LdStrAdd(uint8_t *L, uint64_t Imm) {
   or32le(L, (Imm & 0xFFF) << 10);
 }
 
 void AArch64TargetInfo::relocateOne(uint8_t *Loc, uint32_t Type,
                                     uint64_t Val) const {
   switch (Type) {
   case R_AARCH64_ABS16:
   case R_AARCH64_PREL16:
     checkIntUInt<16>(Loc, Val, Type);
     write16le(Loc, Val);
     break;
   case R_AARCH64_ABS32:
   case R_AARCH64_PREL32:
     checkIntUInt<32>(Loc, Val, Type);
     write32le(Loc, Val);
     break;
   case R_AARCH64_ABS64:
   case R_AARCH64_GLOB_DAT:
   case R_AARCH64_PREL64:
     write64le(Loc, Val);
     break;
   case R_AARCH64_ADD_ABS_LO12_NC:
-    updateAArch64Add(Loc, Val);
+    updateAArch64LdStrAdd(Loc, Val);
     break;
   case R_AARCH64_ADR_GOT_PAGE:
   case R_AARCH64_ADR_PREL_PG_HI21:
   case R_AARCH64_TLSIE_ADR_GOTTPREL_PAGE21:
   case R_AARCH64_TLSDESC_ADR_PAGE21:
     checkInt<33>(Loc, Val, Type);
     updateAArch64Addr(Loc, Val >> 12);
     break;
case R_AARCH64_CONDBR19:
     or32le(Loc, (Val & 0x1FFFFC) << 3);
     break;
   case R_AARCH64_LD64_GOT_LO12_NC:
   case R_AARCH64_TLSIE_LD64_GOTTPREL_LO12_NC:
   case R_AARCH64_TLSDESC_LD64_LO12_NC:
     checkAlignment<8>(Loc, Val, Type);
     or32le(Loc, (Val & 0xFF8) << 7);
     break;
-  case R_AARCH64_LDST128_ABS_LO12_NC:
-    or32le(Loc, (Val & 0x0FF8) << 6);
+  case R_AARCH64_LDST8_ABS_LO12_NC:
+    updateAArch64LdStrAdd(Loc, getBits<0, 11>(Val));
     break;
   case R_AARCH64_LDST16_ABS_LO12_NC:
-    or32le(Loc, (Val & 0x0FFE) << 9);
-    break;
-  case R_AARCH64_LDST8_ABS_LO12_NC:
-    or32le(Loc, (Val & 0xFFF) << 10);
+    updateAArch64LdStrAdd(Loc, getBits<1, 11>(Val));
     break;
   case R_AARCH64_LDST32_ABS_LO12_NC:
-    or32le(Loc, (Val & 0xFFC) << 8);
+    updateAArch64LdStrAdd(Loc, getBits<2, 11>(Val));
     break;
   case R_AARCH64_LDST64_ABS_LO12_NC:
-    or32le(Loc, (Val & 0xFF8) << 7);
+    updateAArch64LdStrAdd(Loc, getBits<3, 11>(Val));
+    break;
+  case R_AARCH64_LDST128_ABS_LO12_NC:
+    updateAArch64LdStrAdd(Loc, getBits<4, 11>(Val));
     break;
   case R_AARCH64_MOVW_UABS_G0_NC:
     or32le(Loc, (Val & 0xFFFF) << 5);
     break;
   case R_AARCH64_MOVW_UABS_G1_NC:
     or32le(Loc, (Val & 0xFFFF0000) >> 11);
     break;
   case R_AARCH64_MOVW_UABS_G2_NC:
     or32le(Loc, (Val & 0xFFFF00000000) >> 27);
     break;
   case R_AARCH64_MOVW_UABS_G3:
     or32le(Loc, (Val & 0xFFFF000000000000) >> 43);
     break;
   case R_AARCH64_TSTBR14:
     checkInt<16>(Loc, Val, Type);
     or32le(Loc, (Val & 0xFFFC) << 3);
     break;
   case R_AARCH64_TLSLE_ADD_TPREL_HI12:
     checkInt<24>(Loc, Val, Type);
-    updateAArch64Add(Loc, Val >> 12);
+    updateAArch64LdStrAdd(Loc, Val >> 12);
     break;
   case R_AARCH64_TLSLE_ADD_TPREL_LO12_NC:
   case R_AARCH64_TLSDESC_ADD_LO12_NC:
-    updateAArch64Add(Loc, Val);
+    updateAArch64LdStrAdd(Loc, Val);
     break;
   default:
     fatal(getErrorLocation(Loc) + "unrecognized reloc " + Twine(Type));
   }
 }
 
 void AArch64TargetInfo::relaxTlsGdToLe(uint8_t *Loc, uint32_t Type,
                                        uint64_t Val) const {