[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

160

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 ↗

(On Diff #94500)

Why do you need this?

1845–1847 ↗

(On Diff #94500)

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 ↗

(On Diff #94500)

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 ↗

(On Diff #94500)

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 ↗

(On Diff #94500)

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->Pic;
+  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, Target->TlsOffsetRel, 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.