[LLD][ELF] Tidy up handleARMTlsRelocation [NFC]

[peter.smith]

Summary

Replace addModuleReloc with AddTlsReloc so that we can use it for both the module relocation and the offset relocation. This is a refactoring of the patch in D31749 that doesn't obscure the fix (as observed in D31672).

[• rafael]

ELF/Relocations.cpp

154

Why Config->Pic? Is it not always 1 for position independent executables?

[peter.smith]

Yes, we can use Config->Shared instead, thanks for pointing that out. I've updated to use Config->Shared and have put back the use of TlsOffsetRel in the Got, it could be possible to add another field to make it use a more regular relocation code, but I'm not sure that is a net win in readability. I'm happy to make the change if it is important to others though.

[ruiu]

ELF/Target.cpp

1842

Why do you need this?

1845–1847

This is not a comment to this particular patch, but we need to remove this piece of code and instead create a way to just set "1" to .got without using this type of relocation in an odd way.

[peter.smith]

ELF/Target.cpp

1842

It is so the same relocation code can be used in:

AddTlsReloc(Off + Config->Wordsize, Target->TlsOffsetRel, &Body, NeedDynOff);

Without it, it would need to be

AddTlsReloc(Off + Config->Wordsize, NeedDynOff ? Target->TlsOffsetRel : R_ARM_ABS32, NeedDynOff);

As when we statically come to resolve the R_ARM_TLS_DTPOFF32 we get unknown relocation code.

I thought that the trade off in adding R_ARM_TLS_DTPOFF32 was worth it as it emphasized that the dynamic relocation was just being evaluated at static link time. I'm not hugely fussed about doing it this way.

1845–1847

These possibilities come to mind:

• Add a function to add arbitrary constants to the GotSection, the module index becomes In<ELFT>::Got->addConstant(Off, 1);
• Add a function to add the executables module index at an offset, In<ELFT>::Got->addModuleIndex(Off)
• Make an ARMGotSection that handles the module index in a similar way to the MipsGotSection

I'd like if we have a solution that both you and Rafael are happy with. I have a weak preference for resolving the Relocation as 1 until there are other cases where we need to write arbitrary constants to the GotSection, but as I say this is a weak preference and I'm happy to change it.

[ruiu]

LGTM

ELF/Target.cpp

1842

Got it. Thank you for explaining. I think I'm slightly inclined to go with R_ARM_ABS32, but because we already have TLS relocations here, I"m OK with the new code.

ELF/Relocations.cpp

 // The pair of GOT entries created are of the form
 // GOT[e0] Module Index (Used to find pointer to TLS block at run-time)
 // GOT[e1] Offset of symbol in TLS block
 template <class ELFT>
 static unsigned handleARMTlsRelocation(GotSection<ELFT> *Got, uint32_t Type,
                                        SymbolBody &Body, InputSectionBase &C,
                                        uint64_t Offset, int64_t Addend,
                                        RelExpr Expr) {
-  auto addModuleReloc = [&](uint64_t Off, bool LD) {
-    // The Dynamic TLS Module Index Relocation can be statically resolved to 1
-    // if we know that the TLS Symbol is in an executable.
-    if (!Body.isPreemptible() && !Config->Pic)
-      Got->Relocations.push_back(
-          {R_ABS, Target->TlsModuleIndexRel, Off, 0, &Body});
-    else {
-      SymbolBody *Dest = LD ? nullptr : &Body;
-      In<ELFT>::RelaDyn->addReloc(
-          {Target->TlsModuleIndexRel, Got, Off, false, Dest, 0});
-    }
+  // The Dynamic TLS Module Index Relocation for a symbol defined in an
+  // executable is always 1. If the target Symbol is not preemtible then
+  // we know the offset into the TLS block at static link time.
+  bool NeedDynId = Body.isPreemptible() || Config->Shared;
+  bool NeedDynOff = Body.isPreemptible();
+
+  auto AddTlsReloc = [&](uint64_t Off, uint32_t Type, SymbolBody *Dest,
+                         bool Dyn) {
+    if (Dyn)
+      In<ELFT>::RelaDyn->addReloc({Type, Got, Off, false, Dest, 0});
+    else
+      Got->Relocations.push_back({R_ABS, Type, Off, 0, Dest});
   };
 
   // Local Dynamic is for access to module local TLS variables, while still
   // being suitable for being dynamically loaded via dlopen.
   // GOT[e0] is the module index, with a special value of 0 for the current
   // module. GOT[e1] is unused. There only needs to be one module index entry.
   if (Expr == R_TLSLD_PC && Got->addTlsIndex()) {
-    addModuleReloc(Got->getTlsIndexOff(), true);
+    AddTlsReloc(Got->getTlsIndexOff(), Target->TlsModuleIndexRel,
+                NeedDynId ? nullptr : &Body, NeedDynId);
     C.Relocations.push_back({Expr, Type, Offset, Addend, &Body});
     return 1;
   }
 
   // Global Dynamic is the most general purpose access model. When we know
   // the module index and offset of symbol in TLS block we can fill these in
   // using static GOT relocations.
   if (Expr == R_TLSGD_PC) {
     if (Got->addDynTlsEntry(Body)) {
       uint64_t Off = Got->getGlobalDynOffset(Body);
-      addModuleReloc(Off, false);
-      if (Body.isPreemptible())
-        In<ELFT>::RelaDyn->addReloc({Target->TlsOffsetRel, Got,
-                                     Off + Config->Wordsize, false, &Body, 0});
-      else
-        Got->Relocations.push_back(
-            {R_ABS, R_ARM_ABS32, Off + Config->Wordsize, 0, &Body});
+      AddTlsReloc(Off, Target->TlsModuleIndexRel, &Body, NeedDynId);
+      AddTlsReloc(Off + Config->Wordsize, Target->TlsOffsetRel, &Body,
+                  NeedDynOff);
     }
     C.Relocations.push_back({Expr, Type, Offset, Addend, &Body});
     return 1;
   }
   return 0;
 }
 
 // Returns the number of relocations processed.

ELF/Target.cpp

void ARMTargetInfo::relocateOne(uint8_t *Loc, uint32_t Type,
   case R_ARM_TARGET1:
   case R_ARM_TARGET2:
   case R_ARM_TLS_GD32:
   case R_ARM_TLS_IE32:
   case R_ARM_TLS_LDM32:
   case R_ARM_TLS_LDO32:
   case R_ARM_TLS_LE32:
   case R_ARM_TLS_TPOFF32:
+  case R_ARM_TLS_DTPOFF32:
     write32le(Loc, Val);
     break;
   case R_ARM_TLS_DTPMOD32:
     write32le(Loc, 1);
     break;
   case R_ARM_PREL31:
     checkInt<31>(Loc, Val, Type);
     write32le(Loc, (read32le(Loc) & 0x80000000) | (Val & ~0x80000000));
     break;
   case R_ARM_CALL:
     // R_ARM_CALL is used for BL and BLX instructions, depending on the
     // value of bit 0 of Val, we must select a BL or BLX instruction
     if (Val & 1) {