[PATCH] D16892: [lld] [ELF/AArch64] Add support to some GD/LE/IS TLS relocation

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/Target.cpp:1311
@@ -1267,3 +1310,3 @@
 
 static void updateAArch64Adr(uint8_t *L, uint64_t Imm) {
   uint32_t ImmLo = (Imm & 0x3) << 29;
----------------
Can you rename this function to updateAArch64Addr? (That is not directly related to your change, but having Adr and Add are too subtle.)

================
Comment at: ELF/Target.cpp:1421
@@ +1420,3 @@
+  case R_AARCH64_TLSLE_ADD_TPREL_HI12: {
+    uint64_t tpOff = llvm::alignTo(TcbSize, Out<ELF64LE>::TlsPhdr->p_align);
+    SA += tpOff;
----------------
tpOff -> TPOff

but I think you don't need this variable -- instead you can just

  uint64_t V = llvm::alignTo(TcbSize, Out<ELF64LE>::TlsPhdr->p_align) + SA;

================
Comment at: ELF/Target.cpp:1424
@@ +1423,3 @@
+    checkInt<24>(SA, Type);
+    updateAArch64Add(Loc, (SA & 0xFFF000) >> 12);
+    break;
----------------
Hm, I don't know if updateAArch64Add worth to be defined in the first place. It may be better to inline it because it is short and you used it only twice.

================
Comment at: ELF/Target.cpp:1445-1448
@@ +1444,6 @@
+  // Local-Exec.
+  if (Type == R_AARCH64_TLSDESC_ADR_PAGE21 ||
+      Type == R_AARCH64_TLSDESC_LD64_LO12_NC ||
+      Type == R_AARCH64_TLSDESC_ADD_LO12_NC ||
+      Type == R_AARCH64_TLSDESC_CALL)
+    return !canBePreempted (S, true);
----------------
Is this the same as isTlsGlobalDynamicRel? If so, is there any reason to not use the function here?


Repository:
  rL LLVM

http://reviews.llvm.org/D16892