[LLD][ELF] Fix ARM TLS global dynamic TlsOffsetRel for non-preemptible symbols

[peter.smith]

Summary

When the target of the TlsOffsetRel is non-preemptible we can write the offset directly into the GOT without needing a dynamic relocation. This is optional for dynamically linked executables but is required for static linking.

We used to rely on the GOTSection::writeTo member function to get the TLS offset of the symbol when writing the VA of the symbol. However in r288107 the GOTSection::writeTo was changed to only use relocations for writing values. Unfortunately we were missing the GOT relocation so all TLS offsets were being written as 0. This change adds the relocation to the GOT entry and a test case for non-0 offsets.

This was found as a result of investigating D31274

[ruiu]

ELF/Relocations.cpp

122

The new code is probably fine, but as a whole I think I don't fully understand this piece of code because it is entangled and complicated. What is this for?

132

Format.

ELF/Target.cpp

1845

Instead of adding Target->TlsOffsetRel, can you add R_ARM_ABS32, so that it is obvious that it is just an abs relocation?

[peter.smith]

I think at least some of the difficulty in understanding the code here is that there are too many somewhat arbitrary ARM and Mips specific bits of code here. I've updated the diff to split handleNoRelaxTlsRelocation() into an ARM and Mips specific function, this allows us to take out some of the special cases.

In the ARM case I've kept the relocation in Target as it allows the same relocation code to be given to the AddTlsReloc function.

I've also found out why the Body.isPreemptible() is for! When we are writing the module index we should only write a 1 if we know the symbol came from the executable. In the case when we are linking an executable but the TLS Symbol is defined in a Shared Library we still need a dynamic relocation for the module index.

Does this make the code any easier to follow? A relatively short but fairly complete description of ARM TLS can be found in Section 3 of http://infocenter.arm.com/help/topic/com.arm.doc.ihi0045e/IHI0045E_ABI_addenda.pdf

[• rafael]

It is now hard to read the fix. Can you split this in a refactoring patch and a fix patch?

ELF/Relocations.cpp

140

copy and paste of MIPS.

[ruiu]

Currently, we add a relocation to a GOT section to set a GOT slot for a TLS variable to 1. But that is an intricate way of setting it to 1, because when we add a relocation, we already have a reference to GOT. We should be able to directly set it to 1, instead of doing it indirectly using relocate().

What I was trying to do is to separate the GOT class into two classes -- one for the regular GOT and the other is for TLS variables. TLS variables use GOT slots very differently than regular variables, I think that will reduce complexity. (But doing that isn't easy as I don't understand some parts of MIPS/AArch64 TLS variables ABIs.)

[peter.smith]

I've split this into 3 reviews:
D31748 Split handleNoRelaxTlsRelocation into ARM and Mips specific impls
D31749 Fix ARM TLS global dynamic TlsOffsetRel for non-preemtible symbols
D31751 Tidy up handleARMTlsRelocation [NFC]
To separate out the refactoring from the fix.

I've also put some extra comments in the handleARMTlsRelocation to explain a bit more about what the ABI requires.

My understanding is that only ARM and Mips need to write TLS entries directly into the GOT this way is because the Local and Global Dynamic TLS can't be relaxed to Initial Exec and Local Exec, on all other Targets including AArch64 (ARM != AArch64) we never have to use the GOT for the cases where it is required on ARM and Mips (static linking) as it is these cases that can be relaxed. A possibility is to have an ARMGotSection as well as a MipsGotSection that handles TLS Entries separately. This would mean that ARM and Mips could probably share the same handleNoRelaxTlsRelocation without many extra conditions. It would mean duplicating some of the code in GotSection for ARM which is what put me off doing so at the time.

ELF/Relocations.cpp

static bool isPreemptible(const SymbolBody &Body, uint32_t Type) {
   // relocation types occupy eight bit. In case of N64 ABI we extract first
   // relocation from 3-in-1 packet because only the first relocation can
   // be against a real symbol.
   if (Config->EMachine == EM_MIPS && (Type & 0xff) == R_MIPS_GPREL16)
     return false;
   return Body.isPreemptible();
 }
 
-// This function is similar to the `handleTlsRelocation`. ARM and MIPS do not
-// support any relaxations for TLS relocations so by factoring out ARM and MIPS
+// This function is similar to the `handleTlsRelocation`. MIPS does not
+// support any relaxations for TLS relocations so by factoring out MIPS
 // handling in to the separate function we can simplify the code and do not
-// pollute `handleTlsRelocation` by ARM and MIPS `ifs` statements.
-template <class ELFT, class GOT>
-static unsigned handleNoRelaxTlsRelocation(GOT *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 we are linking an executable. For ARM we resolve the
-    // relocation when writing the Got. MIPS has a custom Got implementation
-    // that writes the Module index in directly.
-    if (!Body.isPreemptible() && !Config->Pic && Config->EMachine == EM_ARM)
-      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});
-    }
-  };
-
+// pollute other `handleTlsRelocation` by MIPS `ifs` statements.
+template <class ELFT>
+static unsigned handleMipsTlsRelocation(MipsGotSection *Got, uint32_t Type,
+                                        SymbolBody &Body, InputSectionBase &C,
+                                        uint64_t Offset, int64_t Addend,
+                                        RelExpr Expr) {
   if (isRelExprOneOf<R_MIPS_TLSLD, R_TLSLD_PC>(Expr)) {
-    if (Got->addTlsIndex() && (Config->Pic || Config->EMachine == EM_ARM))
-      addModuleReloc(Got->getTlsIndexOff(), true);
+    if (Got->addTlsIndex() && Config->Pic)
+      In<ELFT>::RelaDyn->addReloc({Target->TlsModuleIndexRel, Got,
+                                   Got->getTlsIndexOff(), false, nullptr, 0});
     C.Relocations.push_back({Expr, Type, Offset, Addend, &Body});
     return 1;
   }
 
   if (Target->isTlsGlobalDynamicRel(Type)) {
-    if (Got->addDynTlsEntry(Body) &&
-        (Body.isPreemptible() || Config->EMachine == EM_ARM)) {
+    if (Got->addDynTlsEntry(Body) && Body.isPreemptible()) {
       uint64_t Off = Got->getGlobalDynOffset(Body);
-      addModuleReloc(Off, false);
+      In<ELFT>::RelaDyn->addReloc(
+          {Target->TlsModuleIndexRel, Got, Off, false, &Body, 0});
       if (Body.isPreemptible())
         In<ELFT>::RelaDyn->addReloc({Target->TlsOffsetRel, Got,
                                      Off + Config->Wordsize, false, &Body, 0});
     }
     C.Relocations.push_back({Expr, Type, Offset, Addend, &Body});
     return 1;
   }
+  return 0;
+}
+
+// This function is similar to the `handleTlsRelocation`. ARM does not
+// support any relaxations for TLS relocations so by factoring out ARM
+// handling in to the separate function we can simplify the code and do not
+// pollute other `handleTlsRelocation` by MIPS `ifs` statements.
+// ARM is logically similar to MIPS but differs enough in the details to make
+// a combination of the two functions difficult to understand.
+template <class ELFT>
+static unsigned handleARMTlsRelocation(GotSection<ELFT> *Got, uint32_t Type,
+                                       SymbolBody &Body, InputSectionBase &C,
+                                       uint64_t Offset, int64_t Addend,
+                                       RelExpr Expr) {
+  // 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});
+  };
+
+  if (Expr == R_TLSLD_PC) {
+    if (Got->addTlsIndex())
+      AddTlsReloc(Got->getTlsIndexOff(), Target->TlsModuleIndexRel,
+                  NeedDynId ? nullptr : &Body, NeedDynId);
+    C.Relocations.push_back({Expr, Type, Offset, Addend, &Body});
+    return 1;
+  }
 
+  if (Target->isTlsGlobalDynamicRel(Type)) {
+    if (Got->addDynTlsEntry(Body)) {
+      uint64_t Off = Got->getGlobalDynOffset(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.
 template <class ELFT>
 static unsigned
 handleTlsRelocation(uint32_t Type, SymbolBody &Body, InputSectionBase &C,
                     typename ELFT::uint Offset, int64_t Addend, RelExpr Expr) {
   if (!(C.Flags & SHF_ALLOC))
     return 0;
 
   if (!Body.isTls())
     return 0;
 
   if (Config->EMachine == EM_ARM)
-    return handleNoRelaxTlsRelocation<ELFT>(In<ELFT>::Got, Type, Body, C,
-                                            Offset, Addend, Expr);
+    return handleARMTlsRelocation<ELFT>(In<ELFT>::Got, Type, Body, C, Offset,
+                                        Addend, Expr);
   if (Config->EMachine == EM_MIPS)
-    return handleNoRelaxTlsRelocation<ELFT>(In<ELFT>::MipsGot, Type, Body, C,
-                                            Offset, Addend, Expr);
+    return handleMipsTlsRelocation<ELFT>(In<ELFT>::MipsGot, Type, Body, C,
 
   bool IsPreemptible = isPreemptible(Body, Type);
   if (isRelExprOneOf<R_TLSDESC, R_TLSDESC_PAGE, R_TLSDESC_CALL>(Expr) &&
       Config->Shared) {
     if (In<ELFT>::Got->addDynTlsEntry(Body)) {
       uint64_t Off = In<ELFT>::Got->getGlobalDynOffset(Body);
       In<ELFT>::RelaDyn->addReloc({Target->TlsDescRel, In<ELFT>::Got, Off,
                                    !IsPreemptible, &Body, 0});

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));

arm-tls-gd-nonpreemptible.s

+// RUN: llvm-mc -filetype=obj -triple=armv7a-none-linux-gnueabi %s -o %t
+// RUN: ld.lld %t -o %t2
+// RUN: llvm-mc %s -o %t.o -filetype=obj -triple=armv7a-linux-gnueabi
+// RUN: llvm-objdump -s %t2 | FileCheck %s
+// RUN: ld.lld %t --shared -o %t3.so
+// RUN: llvm-objdump -s %t3.so | FileCheck -check-prefix=CHECK-SHARED %s
+// REQUIRES: arm
+
+// For an executable, we write the module index 1 and the offset into the TLS
+// directly into the GOT. For a shared library we can only write the offset
+// into the TLS directly if the symbol is non-preemptible
+
+ .text
+ .syntax unified
+ .globl __tls_get_addr
+ .type __tls_get_addr,%function
+__tls_get_addr:
+ bx lr
+
+ .globl  _start
+ .p2align        2
+ .type   _start,%function
+func:
+.L0:
+ nop
+.L1:
+ nop
+.L2:
+ nop
+.L3:
+ nop        
+ .p2align        2
+// Generate R_ARM_TLS_GD32 relocations
+// These can be resolved at static link time for executables as 1 is always the
+// module index and the offset into tls is known at static link time        
+.Lt0: .word   x1(TLSGD) + (. - .L0 - 8)
+.Lt1: .word   x2(TLSGD) + (. - .L1 - 8)
+.Lt2: .word   x3(TLSGD) + (. - .L2 - 8)
+.Lt3: .word   x4(TLSGD) + (. - .L3 - 8)
+ .hidden x1
+ .globl  x1
+ .hidden x2       
+ .globl  x2
+ .globl  x3
+ .globl  x4       
+
+ .section       .tdata,"awT",%progbits
+ .p2align  2
+.TLSSTART:
+ .type  x1, %object
+x1:
+ .word 10
+ .type  x2, %object
+x2:
+ .word 20
+        
+ .section       .tbss,"awT",%nobits
+ .p2align 2
+ .type  x3, %object
+x3:
+ .space 4
+ .type  x4, %object
+x4:
+ .space 4
+
+// CHECK: Contents of section .got:
+// CHECK-NEXT:  12008 01000000 00000000 01000000 04000000
+// CHECK-NEXT:  12018 01000000 08000000 01000000 0c000000
+
+// CHECK-SHARED: Contents of section .got:
+// CHECK-SHARED-NEXT:  2050 00000000 00000000 00000000 04000000
+// CHECK-SHARED-NEXT:  2060 00000000 00000000 00000000 00000000