[LLD][ELF] - Simplify TLS LD handling code.

[grimar]

Summary

Our code in handleTlsRelocation is hard to read.
It is possible to rewrite it and reduce a bit.
That is what this patch does. It should be NFC.

[ikudrin]

If I calculate it right, your code has the same cyclomatic complexity as the original one. At the same time, it is one level deeper, taking into account the number of conditions for the particular lines.

[grimar]

Rui, what do you think? For me, it looks simpler than the original code.

[ruiu]

ELF/Relocations.cpp

189–191

I like the new code because it's shorter, but this part looks odd to me, because only R_ABS is not a TLSLD type, though other relocations are all TLSLD. So it is hard to judge.

[grimar]

• Addressed review comment.

ELF/Relocations.cpp

189–191

What if I keep logic about R_ABS outside? Like in the updated version.

[grimar]

Ping?

[grimar]

Rui, what do you think about this one?
I think the new code is better because it groups handling of the R_TLSLD_* relocations at one place.

• Rebased.

[ruiu]

ELF/Arch/PPC64.cpp

742–743

The old code doesn't say anything about these relocations, but your new code contains code for these relocations. Where did they come from? What is special about them?

[grimar]

ELF/Arch/PPC64.cpp

742–743

The original code did that:

if (Expr == R_TLSLD_GOT_OFF) {
  // Local-Dynamic relocs can be relaxed to local-exec
  if (!Config->Shared) {
    C.Relocations.push_back({R_RELAX_TLS_LD_TO_LE, Type, Offset, Addend, &Sym});

The new code does:

if (isRelExprOneOf<R_TLSLD_* ..., R_TLSLD_GOT_OFF>(Expr)) {
    // Local-Dynamic relocs can be relaxed to Local-Exec.
    if (!Config->Shared) {
      C.Relocations.push_back(
          {Target->adjustRelaxExpr(Type, nullptr, R_RELAX_TLS_LD_TO_LE), Type,
           Offset, Addend, &Sym});

See, the new code calls Target->adjustRelaxExpr before pushing the relocations
(btw, that is consistent with the other places in this method).
Target->adjustRelaxExpr should leave some relocations along than
(not convert the expression for them).

(Note, such an approach is not new, for example, we are doing the same on AArch64 I think:
https://github.com/llvm-mirror/lld/blob/master/ELF/Arch/AArch64.cpp#L125)

[grimar]

Ping.

[ruiu]

I'd like to see more drastic refactoring of this code. Honestly we should completely rewrite this code because the current code is really hard to read. As to this patch, I'm not sure if this is an improvement from readability perspective, as both the current code and the new code are hard to understand to me.

[grimar]

Ok, abandoned then.

ELF/Arch/PPC64.cpp

bool PPC64::inBranchRange(RelType Type, uint64_t Src, uint64_t Dst) const {
   int64_t Offset = Dst - Src;
   return isInt<26>(Offset);
 }
 
 RelExpr PPC64::adjustRelaxExpr(RelType Type, const uint8_t *Data,
                                RelExpr Expr) const {
   if (Expr == R_RELAX_TLS_GD_TO_IE)
     return R_RELAX_TLS_GD_TO_IE_GOT_OFF;
-  if (Expr == R_RELAX_TLS_LD_TO_LE)
+
+  if (Expr == R_RELAX_TLS_LD_TO_LE) {
+    if (Type == R_PPC64_GOT_DTPREL16_HA || Type == R_PPC64_GOT_DTPREL16_LO_DS ||
+        Type == R_PPC64_GOT_DTPREL16_DS || Type == R_PPC64_GOT_DTPREL16_HI)
+      return Expr;
     return R_RELAX_TLS_LD_TO_LE_ABS;
+  }
   return Expr;
 }
 
 // Reference: 3.7.4.1 of the 64-bit ELF V2 abi supplement.
 // The general dynamic code sequence for a global `x` uses 4 instructions.
 // Instruction                    Relocation                Symbol
 // addis r3, r2, x@got@tlsgd@ha   R_PPC64_GOT_TLSGD16_HA      x
 // addi  r3, r3, x@got@tlsgd@l    R_PPC64_GOT_TLSGD16_LO      x

ELF/Relocations.cpp

if (In.Got->addDynTlsEntry(Sym)) {
       In.RelaDyn->addReloc(
           {Target->TlsDescRel, In.Got, Off, !Sym.IsPreemptible, &Sym, 0});
     }
     if (Expr != R_TLSDESC_CALL)
       C.Relocations.push_back({Expr, Type, Offset, Addend, &Sym});
     return 1;
   }
 
   if (isRelExprOneOf<R_TLSLD_GOT, R_TLSLD_GOT_FROM_END, R_TLSLD_PC,
-                     R_TLSLD_HINT>(Expr)) {
+                     R_TLSLD_HINT, R_TLSLD_GOT_OFF>(Expr)) {
     // Local-Dynamic relocs can be relaxed to Local-Exec.
     if (!Config->Shared) {
       C.Relocations.push_back(
           {Target->adjustRelaxExpr(Type, nullptr, R_RELAX_TLS_LD_TO_LE), Type,
            Offset, Addend, &Sym});
       return Target->TlsGdRelaxSkip;
     }
+
     if (Expr == R_TLSLD_HINT)
       return 1;
+
+    if (Expr == R_TLSLD_GOT_OFF) {
+      if (!Sym.isInGot()) {
+        In.Got->addEntry(Sym);
+        In.Got->Relocations.push_back(
+            {R_ABS, Target->TlsOffsetRel, Sym.getGotOffset(), 0, &Sym});
+      }
+      C.Relocations.push_back({Expr, Type, Offset, Addend, &Sym});
+      return 1;
+    }
+
     if (In.Got->addTlsIndex())
       In.RelaDyn->addReloc(Target->TlsModuleIndexRel, In.Got,
                            In.Got->getTlsIndexOff(), nullptr);
     C.Relocations.push_back({Expr, Type, Offset, Addend, &Sym});
     return 1;
   }
 
   // Local-Dynamic relocs can be relaxed to Local-Exec.
   if (Expr == R_ABS && !Config->Shared) {
     C.Relocations.push_back(
         {Target->adjustRelaxExpr(Type, nullptr, R_RELAX_TLS_LD_TO_LE), Type,
          Offset, Addend, &Sym});
     return 1;
   }
 
-  // Local-Dynamic sequence where offset of tls variable relative to dynamic
-  // thread pointer is stored in the got.
-  if (Expr == R_TLSLD_GOT_OFF) {
-    // Local-Dynamic relocs can be relaxed to local-exec
-    if (!Config->Shared) {
-      C.Relocations.push_back({R_RELAX_TLS_LD_TO_LE, Type, Offset, Addend, &Sym});
-      return 1;
-    }
-    if (!Sym.isInGot()) {
-      In.Got->addEntry(Sym);
-      uint64_t Off = Sym.getGotOffset();
-      In.Got->Relocations.push_back(
-          {R_ABS, Target->TlsOffsetRel, Off, 0, &Sym});
-    }
-    C.Relocations.push_back({Expr, Type, Offset, Addend, &Sym});
-    return 1;
-  }
-
   if (isRelExprOneOf<R_TLSDESC, R_AARCH64_TLSDESC_PAGE, R_TLSDESC_CALL,
                      R_TLSGD_GOT, R_TLSGD_GOT_FROM_END, R_TLSGD_PC>(Expr)) {
     if (Config->Shared) {
       if (In.Got->addDynTlsEntry(Sym)) {
         uint64_t Off = In.Got->getGlobalDynOffset(Sym);
         In.RelaDyn->addReloc(Target->TlsModuleIndexRel, In.Got, Off, &Sym);
 
         // If the symbol is preemptible we need the dynamic linker to write