[X86] Support -fno-plt __tls_get_addr calls

[MaskRay]

Summary

In general dynamic/local dynamic TLS models, with -fno-plt,

• x86: emit calll *___tls_get_addr@GOT(%ebx) instead of calll ___tls_get_addr@PLT Note, on x86, if we can get rid of %ebx as the PIC register, it may be better to use a register not preserved across function calls.
• x86_64: emit callq *__tls_get_addr@GOTPCREL(%rip) instead of callq __tls_get_addr@PLT

Reorganize the code by separating 32-bit and 64-bit.

[MaskRay]

Fix markdown in the description...

[MaskRay]

.

[dalias]

Hardcoding ebx defeats most of the purpose of no-plt, which was motivated by: https://ewontfix.com/18/

[MaskRay]

https://ewontfix.com/18/ was enlightening. I read it two or three years ago for the first time but apparently I lacked background knowledge to understand these stuff then :) Now I understand how -fno-plt benefits people who don't care about lazy binding...

call    __x86.get_pc_thunk.cx
jmp *bar@GOT+_GLOBAL_OFFSET_TABLE_(%ecx)

I think the relocation type is unavailable. This requires something like G+GOT+A-* (think R_X86_64_GOTPCREL G+GOT+A-P) but that is not in the psABI.

We can still do better by switching to %eax, %ecx or %edx, but I think this function does not have information to make the decision. We should probably propagate the -fno-plt (RtLibUseGot) information to several levels above but I know really little about the x86 codegen to achieve that...

I have to add a TODO item in the description to note down this future improvement.

[MaskRay]

Add a TODO

[MaskRay]

🏓

[dalias]

I'm not sure how LLVM internals work, but if it's anything like GCC's, I think you just need to make it take a register operand for the GOT address as an input/operand. Then the higher-level code should ensure it's available, and then you just use it in the code emitted. Touching higher-level code shouldn't be needed.

[rnk]

Looks good to me.

I'm not 100% following the discussion of using EBX as the PIC register, but I think it's a pre-existing issue for 32-bit code. I think it's unlikely that anyone will ever care enough to improve our 32-bit codegen, so it seems reasonable to leave it alone for now (or forever).

lib/Target/X86/X86MCInstLower.cpp

690 ↗

(On Diff #200222)

I think it would be more readable to retain this NeedsPadding variable, but make it evaluate to SRVK == MCSymbolRefExpr::VK_TLSGD.

[MaskRay]

Address review comment

[nikic]

I'm seeing linker failures that I suspect are related to this change:

/usr/bin/ld: /home/nikic/rust/foo/libtest2.rlib(test2.test2.3a1fbbbh-cgu.0.rcgu.o): TLS transition from R_X86_64_TLSLD to R_X86_64_TPOFF32 against `_ZN5test24TEST17h9cebda1c5e0bffdeE' at 0x4 in section `.text._ZN5test28get_test17h7831513ccc936b83E' failed

Old:

0000000000000000 <_ZN5test28get_test17h7831513ccc936b83E>:
   0:   50                      push   %rax
   1:   48 8d 3d 00 00 00 00    lea    0x0(%rip),%rdi        # 8 <_ZN5test28get_test17h7831513ccc936b83E+0x8>
            4: R_X86_64_TLSLD   _ZN5test24TEST17h9cebda1c5e0bffdeE-0x4
   8:   e8 00 00 00 00          callq  d <_ZN5test28get_test17h7831513ccc936b83E+0xd>
            9: R_X86_64_PLT32   __tls_get_addr-0x4
   d:   48 8b 80 00 00 00 00    mov    0x0(%rax),%rax
            10: R_X86_64_DTPOFF32   _ZN5test24TEST17h9cebda1c5e0bffdeE
  14:   59                      pop    %rcx
  15:   c3                      retq

New:

0000000000000000 <_ZN5test28get_test17h7831513ccc936b83E>:
   0:   50                      push   %rax
   1:   48 8d 3d 00 00 00 00    lea    0x0(%rip),%rdi        # 8 <_ZN5test28get_test17h7831513ccc936b83E+0x8>
            4: R_X86_64_TLSLD   _ZN5test24TEST17h9cebda1c5e0bffdeE-0x4
   8:   ff 15 00 00 00 00       callq  *0x0(%rip)        # e <_ZN5test28get_test17h7831513ccc936b83E+0xe>
            a: R_X86_64_GOTPCREL    __tls_get_addr-0x4
   e:   48 8b 80 00 00 00 00    mov    0x0(%rax),%rax
            11: R_X86_64_DTPOFF32   _ZN5test24TEST17h9cebda1c5e0bffdeE
  15:   59                      pop    %rcx
  16:   c3                      retq

I'm not really familiar with TLS relocations -- do you have any pointers on what could be going wrong here?

[MaskRay]

In D62106#1572575, @nikic wrote:

I'm seeing linker failures that I suspect are related to this change:

/usr/bin/ld: /home/nikic/rust/foo/libtest2.rlib(test2.test2.3a1fbbbh-cgu.0.rcgu.o): TLS transition from R_X86_64_TLSLD to R_X86_64_TPOFF32 against `_ZN5test24TEST17h9cebda1c5e0bffdeE' at 0x4 in section `.text._ZN5test28get_test17h7831513ccc936b83E' failed

Old:

0000000000000000 <_ZN5test28get_test17h7831513ccc936b83E>:
   0:   50                      push   %rax
   1:   48 8d 3d 00 00 00 00    lea    0x0(%rip),%rdi        # 8 <_ZN5test28get_test17h7831513ccc936b83E+0x8>
            4: R_X86_64_TLSLD   _ZN5test24TEST17h9cebda1c5e0bffdeE-0x4
   8:   e8 00 00 00 00          callq  d <_ZN5test28get_test17h7831513ccc936b83E+0xd>
            9: R_X86_64_PLT32   __tls_get_addr-0x4
   d:   48 8b 80 00 00 00 00    mov    0x0(%rax),%rax
            10: R_X86_64_DTPOFF32   _ZN5test24TEST17h9cebda1c5e0bffdeE
  14:   59                      pop    %rcx
  15:   c3                      retq

New:

0000000000000000 <_ZN5test28get_test17h7831513ccc936b83E>:
   0:   50                      push   %rax
   1:   48 8d 3d 00 00 00 00    lea    0x0(%rip),%rdi        # 8 <_ZN5test28get_test17h7831513ccc936b83E+0x8>
            4: R_X86_64_TLSLD   _ZN5test24TEST17h9cebda1c5e0bffdeE-0x4
   8:   ff 15 00 00 00 00       callq  *0x0(%rip)        # e <_ZN5test28get_test17h7831513ccc936b83E+0xe>
            a: R_X86_64_GOTPCREL    __tls_get_addr-0x4
   e:   48 8b 80 00 00 00 00    mov    0x0(%rax),%rax
            11: R_X86_64_DTPOFF32   _ZN5test24TEST17h9cebda1c5e0bffdeE
  15:   59                      pop    %rcx
  16:   c3                      retq

I'm not really familiar with TLS relocations -- do you have any pointers on what could be going wrong here?

% cat a.cc
static thread_local int a;
int main() { return a; }
% clang -fno-plt -fpic a.cc -Wa,-mrelax-relocations=no -fuse-ld=gold
% clang -fno-plt -fpic a.cc -Wa,-mrelax-relocations=no -fuse-ld=lld
% clang -fno-plt -fpic a.cc -Wa,-mrelax-relocations=no -fuse-ld=bfd
/usr/bin/ld.bfd: /tmp/a-9fd9f7.o: TLS transition from R_X86_64_TLSLD to R_X86_64_TPOFF32 against `_ZL1a' at 0x17 in section `.text' failed

# bfd/elf-x86-64.c:elf_x86_64_check_tls_transition
...
    if (largepic)
      return r_type == R_X86_64_PLTOFF64;
    else if (indirect_call)
      return r_type == R_X86_64_GOTPCRELX; // it doesn't allow R_X86_64_GOTPCREL
    else
      return (r_type == R_X86_64_PC32 || r_type == R_X86_64_PLT32);

ld.bfd doesn't allow R_X86_64_GOTPCREL. You can work around the issue with:

% clang -fno-plt -fpic a.cc -Wa,-mrelax-relocations=yes -fuse-ld=bfd

Filed a bug against binutils-gdb: https://sourceware.org/bugzilla/show_bug.cgi?id=24784

[nikic]

@MaskRay Thanks for checking! Can you tell me which binutils version you are using? It looks like using plain ld works for you, but I'm seeing this error with binutils 2.30 ld (gold 1.15 works though).

Specifically I'm seeing this with /usr/bin/ld -o test file1.o file2.rlib with the files F9486230 and F9486238.

Edit: Ooops, never mind. I did not realize that ld and ld.bfd are the same thing.

[nikic]

ld.bfd doesn't allow R_X86_64_GOTPCREL. You can work around the issue with:

% clang -fno-plt -fpic a.cc -Wa,-mrelax-relocations=yes -fuse-ld=bfd

Thanks, I've verified that setting RelaxELFRelocations in TargetOptions indeed works around the issue. I'm a bit concerned about what other compatibility issues this will cause though. If I understood correctly, relax relocations is a relatively recent feature and not compatible with older linkers, so may not be a suitable compiler default.

[MaskRay]

In D62106#1572600, @nikic wrote:

ld.bfd doesn't allow R_X86_64_GOTPCREL. You can work around the issue with:

% clang -fno-plt -fpic a.cc -Wa,-mrelax-relocations=yes -fuse-ld=bfd

Thanks, I've verified that setting RelaxELFRelocations in TargetOptions indeed works around the issue. I'm a bit concerned about what other compatibility issues this will cause though. If I understood correctly, relax relocations is a relatively recent feature and not compatible with older linkers, so may not be a suitable compiler default.

I think R_X86_64_GOTPCRELX should be usable in ld.bfd and gold since 2016. In gold, -fno-plt R_X86_64_GOTPCREL was supported in Aug 2016 (https://sourceware.org/bugzilla/show_bug.cgi?id=20216).

Do you know why the Rust lib uses -fno-plt TLS code sequence? I'm thinking if its depended llvm should be configured with -DENABLE_X86_RELAX_RELOCATIONS=ON so that you don't need -Wa,-mrelax-relocations=yes

% llvm-objdump -dr file2.rlib
...
       8: ff 15 00 00 00 00             callq   *(%rip)
                000000000000000a:  R_X86_64_GOTPCREL    __tls_get_addr-4

[nikic]

In D62106#1572612, @MaskRay wrote:

In D62106#1572600, @nikic wrote:

ld.bfd doesn't allow R_X86_64_GOTPCREL. You can work around the issue with:

% clang -fno-plt -fpic a.cc -Wa,-mrelax-relocations=yes -fuse-ld=bfd

Thanks, I've verified that setting RelaxELFRelocations in TargetOptions indeed works around the issue. I'm a bit concerned about what other compatibility issues this will cause though. If I understood correctly, relax relocations is a relatively recent feature and not compatible with older linkers, so may not be a suitable compiler default.

I think R_X86_64_GOTPCRELX should be usable in ld.bfd and gold since 2016. In gold, -fno-plt R_X86_64_GOTPCREL was supported in Aug 2016 (https://sourceware.org/bugzilla/show_bug.cgi?id=20216).

Thanks for the information. As 2016 is still quite recent, I don't think we'll be able to turn on relax relocations by default and need some other solution. Given the flaky linker support, would it be possible to add an option to keep using PLT for __tls_get_addr despite RtLibUseGOT?

Do you know why the Rust lib uses -fno-plt TLS code sequence?

As rust uses full relro, using PLT doesn't really have any advantage, so it is disabled by default nowadays.

[dalias]

Have you checked what gcc does? It should be safe to do the same, no? Or is the issue that gcc leaves it to gas, and it's reasonably assumed that gas and ld.bfd versions will match, whereas clang uses an internal assembler that might not match?

[MaskRay]

In D62106#1572702, @dalias wrote:

Have you checked what gcc does? It should be safe to do the same, no? Or is the issue that gcc leaves it to gas, and it's reasonably assumed that gas and ld.bfd versions will match, whereas clang uses an internal assembler that might not match?

gcc does something like:

(define_insn "*tls_global_dynamic_64_<mode>"
...
if (flag_plt || !HAVE_AS_IX86_TLS_GET_ADDR_GOT)
  return "call\t%P2";
return "call\t{*%p2@GOT(%1)|[DWORD PTR %p2@GOT[%1]]}";

I haven't built it but it apparently it guards the issue with the configure-time check. I'll have to work around the bug with a source change.. D64304

@nikic Can you share a reproduce of the Rust code?

llvm/trunk/lib/Target/X86/X86MCInstLower.cpp

void X86MCInstLower::Lower(const MachineInstr *MI, MCInst &OutMI) const {
   case X86::MOVSX64rr32:
     SimplifyMOVSX(OutMI);
     break;
   }
 }
 
 void X86AsmPrinter::LowerTlsAddr(X86MCInstLower &MCInstLowering,
                                  const MachineInstr &MI) {
-
-  bool is64Bits = MI.getOpcode() == X86::TLS_addr64 ||
+  bool Is64Bits = MI.getOpcode() == X86::TLS_addr64 ||
                   MI.getOpcode() == X86::TLS_base_addr64;
-
-  bool needsPadding = MI.getOpcode() == X86::TLS_addr64;
-
-  MCContext &context = OutStreamer->getContext();
-
-  if (needsPadding)
-    EmitAndCountInstruction(MCInstBuilder(X86::DATA16_PREFIX));
+  MCContext &Ctx = OutStreamer->getContext();
 
   MCSymbolRefExpr::VariantKind SRVK;
   switch (MI.getOpcode()) {
   case X86::TLS_addr32:
   case X86::TLS_addr64:
     SRVK = MCSymbolRefExpr::VK_TLSGD;
     break;
   case X86::TLS_base_addr32:
     SRVK = MCSymbolRefExpr::VK_TLSLDM;
     break;
   case X86::TLS_base_addr64:
     SRVK = MCSymbolRefExpr::VK_TLSLD;
     break;
   default:
     llvm_unreachable("unexpected opcode");
   }
 
-  MCSymbol *sym = MCInstLowering.GetSymbolFromOperand(MI.getOperand(3));
-  const MCSymbolRefExpr *symRef = MCSymbolRefExpr::create(sym, SRVK, context);
-
-  MCInst LEA;
-  if (is64Bits) {
-    LEA.setOpcode(X86::LEA64r);
-    LEA.addOperand(MCOperand::createReg(X86::RDI)); // dest
-    LEA.addOperand(MCOperand::createReg(X86::RIP)); // base
-    LEA.addOperand(MCOperand::createImm(1));        // scale
-    LEA.addOperand(MCOperand::createReg(0));        // index
-    LEA.addOperand(MCOperand::createExpr(symRef));  // disp
-    LEA.addOperand(MCOperand::createReg(0));        // seg
-  } else if (SRVK == MCSymbolRefExpr::VK_TLSLDM) {
-    LEA.setOpcode(X86::LEA32r);
-    LEA.addOperand(MCOperand::createReg(X86::EAX)); // dest
-    LEA.addOperand(MCOperand::createReg(X86::EBX)); // base
-    LEA.addOperand(MCOperand::createImm(1));        // scale
-    LEA.addOperand(MCOperand::createReg(0));        // index
-    LEA.addOperand(MCOperand::createExpr(symRef));  // disp
-    LEA.addOperand(MCOperand::createReg(0));        // seg
-  } else {
-    LEA.setOpcode(X86::LEA32r);
-    LEA.addOperand(MCOperand::createReg(X86::EAX)); // dest
-    LEA.addOperand(MCOperand::createReg(0));        // base
-    LEA.addOperand(MCOperand::createImm(1));        // scale
-    LEA.addOperand(MCOperand::createReg(X86::EBX)); // index
-    LEA.addOperand(MCOperand::createExpr(symRef));  // disp
-    LEA.addOperand(MCOperand::createReg(0));        // seg
-  }
-  EmitAndCountInstruction(LEA);
-
-  if (needsPadding) {
+  const MCSymbolRefExpr *Sym = MCSymbolRefExpr::create(
+      MCInstLowering.GetSymbolFromOperand(MI.getOperand(3)), SRVK, Ctx);
+  bool UseGot = MMI->getModule()->getRtLibUseGOT();
+
+  if (Is64Bits) {
+    bool NeedsPadding = SRVK == MCSymbolRefExpr::VK_TLSGD;
+    if (NeedsPadding)
+      EmitAndCountInstruction(MCInstBuilder(X86::DATA16_PREFIX));
+    EmitAndCountInstruction(MCInstBuilder(X86::LEA64r)
+                                .addReg(X86::RDI)
+                                .addReg(X86::RIP)
+                                .addImm(1)
+                                .addReg(0)
+                                .addExpr(Sym)
+                                .addReg(0));
+    const MCSymbol *TlsGetAddr = Ctx.getOrCreateSymbol("__tls_get_addr");
+    if (NeedsPadding) {
+      if (!UseGot)
         EmitAndCountInstruction(MCInstBuilder(X86::DATA16_PREFIX));
       EmitAndCountInstruction(MCInstBuilder(X86::DATA16_PREFIX));
       EmitAndCountInstruction(MCInstBuilder(X86::REX64_PREFIX));
     }
-
-  StringRef name = is64Bits ? "__tls_get_addr" : "___tls_get_addr";
-  MCSymbol *tlsGetAddr = context.getOrCreateSymbol(name);
-  const MCSymbolRefExpr *tlsRef =
-      MCSymbolRefExpr::create(tlsGetAddr, MCSymbolRefExpr::VK_PLT, context);
-
+    if (UseGot) {
+      const MCExpr *Expr = MCSymbolRefExpr::create(
+          TlsGetAddr, MCSymbolRefExpr::VK_GOTPCREL, Ctx);
+      EmitAndCountInstruction(MCInstBuilder(X86::CALL64m)
+                                  .addReg(X86::RIP)
+                                  .addImm(1)
+                                  .addReg(0)
+                                  .addExpr(Expr)
+                                  .addReg(0));
+    } else {
       EmitAndCountInstruction(
-      MCInstBuilder(is64Bits ? X86::CALL64pcrel32 : X86::CALLpcrel32)
-          .addExpr(tlsRef));
+          MCInstBuilder(X86::CALL64pcrel32)
+              .addExpr(MCSymbolRefExpr::create(TlsGetAddr,
+                                               MCSymbolRefExpr::VK_PLT, Ctx)));
+    }
+  } else {
+    if (SRVK == MCSymbolRefExpr::VK_TLSGD && !UseGot) {
+      EmitAndCountInstruction(MCInstBuilder(X86::LEA32r)
+                                  .addReg(X86::EAX)
+                                  .addReg(0)
+                                  .addImm(1)
+                                  .addReg(X86::EBX)
+                                  .addExpr(Sym)
+                                  .addReg(0));
+    } else {
+      EmitAndCountInstruction(MCInstBuilder(X86::LEA32r)
+                                  .addReg(X86::EAX)
+                                  .addReg(X86::EBX)
+                                  .addImm(1)
+                                  .addReg(0)
+                                  .addExpr(Sym)
+                                  .addReg(0));
+    }
+
+    const MCSymbol *TlsGetAddr = Ctx.getOrCreateSymbol("___tls_get_addr");
+    if (UseGot) {
+      const MCExpr *Expr =
+          MCSymbolRefExpr::create(TlsGetAddr, MCSymbolRefExpr::VK_GOT, Ctx);
+      EmitAndCountInstruction(MCInstBuilder(X86::CALL32m)
+                                  .addReg(X86::EBX)
+                                  .addImm(1)
+                                  .addReg(0)
+                                  .addExpr(Expr)
+                                  .addReg(0));
+    } else {
+      EmitAndCountInstruction(
+          MCInstBuilder(X86::CALLpcrel32)
+              .addExpr(MCSymbolRefExpr::create(TlsGetAddr,
+                                               MCSymbolRefExpr::VK_PLT, Ctx)));
+    }
+  }
 }
 
 /// Emit the largest nop instruction smaller than or equal to \p NumBytes
 /// bytes.  Return the size of nop emitted.
 static unsigned EmitNop(MCStreamer &OS, unsigned NumBytes, bool Is64Bit,
                         const MCSubtargetInfo &STI) {
   // This works only for 64bit. For 32bit we have to do additional checking if
   // the CPU supports multi-byte nops.

llvm/trunk/test/CodeGen/X86/tls-no-plt.ll

+; RUN: llc < %s -mtriple=i386-linux-musl -relocation-model=pic | FileCheck --check-prefixes=CHECK,X86 %s
+; RUN: llc < %s -mtriple=x86_64-linux-musl -relocation-model=pic | FileCheck --check-prefixes=CHECK,X64 %s
+
+@gd = thread_local global i32 0
+@ld = internal thread_local global i32 0
+
+define i32* @get_gd() {
+entry:
+; CHECK-LABEL: get_gd:
+; X86: leal gd@TLSGD(%ebx), %eax
+; X86: calll *___tls_get_addr@GOT(%ebx)
+
+; X64: leaq gd@TLSGD(%rip), %rdi
+; X64: callq *__tls_get_addr@GOTPCREL(%rip)
+  ret i32* @gd
+}
+
+define i32* @get_ld() {
+; FIXME: This function uses a single thread-local variable, we might want to fall back to general-dynamic.
+; CHECK-LABEL: get_ld:
+; X86: leal ld@TLSLDM(%ebx), %eax
+; X86: calll *___tls_get_addr@GOT(%ebx)
+
+; X64: leaq ld@TLSLD(%rip), %rdi
+; X64: callq *__tls_get_addr@GOTPCREL(%rip)
+  ret i32* @ld
+}
+
+!llvm.module.flags = !{!1}
+!1 = !{i32 7, !"RtLibUseGOT", i32 1}