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

[Adhemerval Zanella via llvm-commits]

zatrazz 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;
----------------
ruiu wrote:
> Can you rename this function to updateAArch64Addr? (That is not directly related to your change, but having Adr and Add are too subtle.)
I will change that.

================
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;
----------------
ruiu wrote:
> 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;
This seems better, I will change that.

================
Comment at: ELF/Target.cpp:1424
@@ +1423,3 @@
+    checkInt<24>(SA, Type);
+    updateAArch64Add(Loc, (SA & 0xFFF000) >> 12);
+    break;
----------------
ruiu wrote:
> 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.
I though about it as well, but I decided to add to be explicit the relocation is handling a add instruction directly (although the name already says it). Anyway, related to inlining I will add an inline modifier to function definition.

================
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);
----------------
ruiu wrote:
> Is this the same as isTlsGlobalDynamicRel? If so, is there any reason to not use the function here?
Indeed, I will change that.

================
Comment at: ELF/Target.cpp:1365
@@ -1317,3 +1364,3 @@
     uint64_t X = getAArch64Page(SA) - getAArch64Page(P);
-    checkInt<33>(X, Type);
-    updateAArch64Adr(Loc, (X >> 12) & 0x1FFFFF); // X[32:12]
+    checkInt<32>(X, Type);
+    updateAArch64Addr(Loc, (X >> 12) & 0x1FFFFF); // X[32:12]
----------------
ruiu wrote:
> Why did you change this?
Because on AArch64 ELF specification states that R_AARCH64_ADR_PREL_PG_HI21 and  R_AARCH64_TLSIE_ADR_GOTTPREL_PAGE21 should check if the value is within 2^-32 and 2^32. My understanding is currently checking against 2^-33 and 2^33.

================
Comment at: ELF/Target.cpp:1439
@@ +1438,3 @@
+
+  // Global-Dynamic relocs can be relaxed to to Initial-Exec if the target is a
+  // an executable.  And if the target is local it can also be fully relaxed to
----------------
ruiu wrote:
> s/to to/to/
> s/a an/an/
I will change that.

================
Comment at: ELF/Target.cpp:1462
@@ +1461,3 @@
+  case R_AARCH64_TLSDESC_CALL:
+    if (canBePreempted(S, true))
+      relocateTlsGdToIe(Type, Loc, BufEnd, P, SA);
----------------
ruiu wrote:
> Please add {}
Ok.

================
Comment at: ELF/Target.cpp:1465
@@ +1464,3 @@
+    else {
+      uint64_t X= S ? S->getVA<ELF64LE>() : SA;
+      relocateTlsGdToLe(Type, Loc, BufEnd, P, X);
----------------
ruiu wrote:
> Insert space after '='.
Ok.

================
Comment at: ELF/Target.cpp:1501
@@ +1500,3 @@
+
+  uint64_t tpOff = llvm::alignTo(TcbSize, Out<ELF64LE>::TlsPhdr->p_align);
+  uint64_t X = SA + tpOff;
----------------
ruiu wrote:
> Please follow the LLVM naming convention.
> 
> tpOff -> TPOff
Right, I will change that.

================
Comment at: ELF/Target.cpp:1539
@@ +1538,3 @@
+    // Generate movz.
+    unsigned int RegNo = (Inst & 0x1f);
+    NewInst = (0xd2a00000 | RegNo) | (((X >> 16) & 0xffff) << 5);
----------------
ruiu wrote:
> s/unsigned int/unsigned/
Ok.


http://reviews.llvm.org/D16892