[ELF] Fix LLD performing an invalid IE->LE relaxation for a thread-local variable on x86-64

[artem]

Summary

Reproducer

main.cpp:

void do_some_fun();

thread_local double kek;

int main() {
  kek = 42.0;
  do_some_fun();
}

bug.cpp:

#include <cstdio>

extern thread_local __attribute__((weak)) double kek;

void do_some_fun() {
  if (&kek != nullptr) {
    printf("The address is not nullptr!\n");
  } else {
    printf("The address is nullptr :(\n");
  }
  printf("Actually, the address is %p\n", &kek);
  printf("And the value is %f\n", kek);
}

Commands to reproduce:

clang++ -O3 -c main.cpp -o main.o
clang++ -O3 -c bug.cpp -o bug.o
clang++ -fuse-ld=lld main.o bug.o -o program

Expected behavior

The address is not nullptr!
Actually, the address is 0x7fcb82adb738
And the value is 42.000000

Actual behavior

The address is nullptr :(
Actually, the address is 0x7fa226ab5738
And the value is 42.000000

Explanation

Clang generates the following assembly:

movq    %fs:0, %rax
addq    kek@GOTTPOFF(%rip), %rax ; ADDQ sets ZF if the address is NULL
leaq    .Lstr(%rip), %rax        ; "The value is nullptr :("
leaq    .Lstr.3(%rip), %rdi      ; "The value is not nullptr!"
cmoveq  %rax, %rdi               ; Checks ZF
callq   puts@PLT

and a R_X86_64_GOTTPOFF relocation.

Then, when LLD discovers location of kek, it tries to relax the relocation to R_X86_64_TPOFF32. The transformation uses LEAQ thus proper ZF is lost and CMOVEQ misbehaves.

[MaskRay]

Clang generates the following assembly:

Which Clang version do you use? I cannot reproduce with clang trunk, 14.0, or 15.0 with clang++ main.cpp bug.cpp; ./a.out.

Anyway I don't think there is a bug in lld. lld's behavior matches gold while GNU ld has an assertion failure for some cases in tls-opt.s: assertion fail ../../bfd/elf64-x86-64.c:4159.
The GNU ld issue suggests that it doesn't expect the TLS code sequence provided by your Clang.

The weak attribute for TLS can essentially be ignored as undefined weak TLS doesn't really have the "if undefined, resolve to zero" semantics.
The user code extern thread_local __attribute__((weak)) double kek; probably should lead to an error.

[artem]

In D145062#4162016, @MaskRay wrote:

Clang generates the following assembly:

Which Clang version do you use? I cannot reproduce with clang trunk, 14.0, or 15.0 with clang++ main.cpp bug.cpp; ./a.out.

Ubuntu clang version 15.0.6. I think you need to specify -O3 for compiler to produce such code

Anyway I don't think there is a bug in lld. lld's behavior matches gold while GNU ld has an assertion failure for some cases in tls-opt.s: assertion fail ../../bfd/elf64-x86-64.c:4159.
The GNU ld issue suggests that it doesn't expect the TLS code sequence provided by your Clang.

I used GNU ld (GNU Binutils for Ubuntu) 2.39. Without any extra flags, it successfully links the binary, which shows exactly the same wrong behavior. HOWERVER, passing -rdynamic to BFD magically fixes the issue, it emits ADD instruction ¯\_(ツ)_/¯

The weak attribute for TLS can essentially be ignored as undefined weak TLS doesn't really have the "if undefined, resolve to zero" semantics.
The user code extern thread_local __attribute__((weak)) double kek; probably should lead to an error.

I understand, but I added the __attribute__((weak)) with the only reason for Clang to not compile out the condition check.

P.S. I have a reproducer for GCC+LLD, it does not declare the variable as weak, but requires code to be instrumented with UBSan and is much less concise. Actually, that's the piece of code which originally helped me to discover the issue (the code in this differential's summary is hand-crafted). You can view it in attachments (tested with gcc version 12.2.0):

gcc-lld-tls-bug.zip3 KBDownload

[MaskRay]

In D145062#4162915, @artem wrote:

In D145062#4162016, @MaskRay wrote:

Clang generates the following assembly:

Which Clang version do you use? I cannot reproduce with clang trunk, 14.0, or 15.0 with clang++ main.cpp bug.cpp; ./a.out.

Ubuntu clang version 15.0.6. I think you need to specify -O3 for compiler to produce such code

I forgot to say but I have tried -O0/-O2/-O3 for all of 14.0, 15.0, and trunk from official releases. Not reproducible...
I don't know whether Ubuntu has made some patching which may change the behavior.

Anyway I don't think there is a bug in lld. lld's behavior matches gold while GNU ld has an assertion failure for some cases in tls-opt.s: assertion fail ../../bfd/elf64-x86-64.c:4159.
The GNU ld issue suggests that it doesn't expect the TLS code sequence provided by your Clang.

I used GNU ld (GNU Binutils for Ubuntu) 2.39. Without any extra flags, it successfully links the binary, which shows exactly the same wrong behavior. HOWERVER, passing -rdynamic to BFD magically fixes the issue, it emits ADD instruction ¯\_(ツ)_/¯

Thanks. I confirm this behavior with and without -rdynamic, so I think GNU ls is doing something strange...
Whether or not the TLS symbol or its associated TLS init function for is exported or not, the TLS optimization behavior should not change.

The weak attribute for TLS can essentially be ignored as undefined weak TLS doesn't really have the "if undefined, resolve to zero" semantics.
The user code extern thread_local __attribute__((weak)) double kek; probably should lead to an error.

I understand, but I added the __attribute__((weak)) with the only reason for Clang to not compile out the condition check.

P.S. I have a reproducer for GCC+LLD, it does not declare the variable as weak, but requires code to be instrumented with UBSan and is much less concise. Actually, that's the piece of code which originally helped me to discover the issue (the code in this differential's summary is hand-crafted). You can view it in attachments (tested with gcc version 12.2.0):

gcc-lld-tls-bug.zip3 KBDownload

That said, I believe this is more of a GCC bug. If you can reproduce it with Clang, it is a Clang bug as well.

Link-time TLS optimization is a protocol between the compiler and the linker.
The linker just blindly trusts that the compiler output is legal and causes the situation: not updating eflags due to add=>lea transition causes a condition to fail.
For your reproduce, it is a bug of GCC's -fsanitize=undefined.
Since lld's behavior matches GNU ld's relaxation scheme (even if it is just the non-export-dynamic case), I don't think this a lld bug.

I will investigate more and add more information about _ZTH*, weak on my https://maskray.me/blog/2021-02-14-all-about-thread-local-storage

With these said, since this patch looks like a code simplification, if we can get GNU ld do something similar, we can revisit this patch.
I am not so sure whether changing lea to add (which will update eflags) may cause other unknown problems, though.

[MaskRay]

If you can get a smaller reproduce, you can report this on https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93623

The key is extern thread_local double precise_now (__thread will paper over the bug) which has a undefined weak symbol _ZTH* which will be instrumented by -fsanitize=undefined.

[artem]

In D145062#4163217, @MaskRay wrote:

I forgot to say but I have tried -O0/-O2/-O3 for all of 14.0, 15.0, and trunk from official releases. Not reproducible...
I don't know whether Ubuntu has made some patching which may change the behavior.

Here's a godbolt link: https://godbolt.org/z/3vMW8vz1s

Link-time TLS optimization is a protocol between the compiler and the linker.
The linker just blindly trusts that the compiler output is legal and causes the situation: not updating eflags due to add=>lea transition causes a condition to fail.

While I tend agree with this statement, I have to admit ELF ABI for TLS seems to be language agnostic. If it is possible to write an assembly program that does not violate the ABI but triggers the problem, it is outside the scope of both GCC and Clang. I quickly listed through the "ELF Handling For Thread-Local Storage" and couldn't find an explicit prohibition against dependency on the side effects of accessing IE variables.

With these said, since this patch looks like a code simplification, if we can get GNU ld do something similar, we can revisit this patch.
I am not so sure whether changing lea to add (which will update eflags) may cause other unknown problems, though.

Should I submit a patch or discuss it with someone somewhere first? I am not very competent in this kind of topics and how it's usually being done.

In D145062#4163222, @MaskRay wrote:

If you can get a smaller reproduce, you can report this on https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93623
The key is extern thread_local double precise_now (__thread will paper over the bug) which has a undefined weak symbol _ZTH* which will be instrumented by -fsanitize=undefined.

Thanks for the suggestion. Do you mean that specific issue (it does not seem to be related to our subject at first glance) or reporting to GCC's Bugzilla in general?
As for __thread, it's a nice catch. I think can be used as a workaround until the issue is solved. Also, it exaplains why GCC has decided to instrument a non-weak symbol.

[MaskRay]

In D145062#4164723, @artem wrote:

In D145062#4163217, @MaskRay wrote:

I forgot to say but I have tried -O0/-O2/-O3 for all of 14.0, 15.0, and trunk from official releases. Not reproducible...
I don't know whether Ubuntu has made some patching which may change the behavior.

Here's a godbolt link: https://godbolt.org/z/3vMW8vz1s

OK, I used 15.0.6 which doesn't use addq kek@GOTTPOFF(%rip), %rbx.
On Clang&LLVM side, D131716 fixed the bug.

Link-time TLS optimization is a protocol between the compiler and the linker.
The linker just blindly trusts that the compiler output is legal and causes the situation: not updating eflags due to add=>lea transition causes a condition to fail.

While I tend agree with this statement, I have to admit ELF ABI for TLS seems to be language agnostic. If it is possible to write an assembly program that does not violate the ABI but triggers the problem, it is outside the scope of both GCC and Clang. I quickly listed through the "ELF Handling For Thread-Local Storage" and couldn't find an explicit prohibition against dependency on the side effects of accessing IE variables.

No, "ELF Handling For Thread-Local Storage" is not authoritative for x86-64. We should use "System V Application Binary Interface AMD64 Architecture Processor Supplement" "TLS linker optimization" as the canonical reference, which mentions Table 10.17: IE -> LE Code Transition With Lea.

With these said, since this patch looks like a code simplification, if we can get GNU ld do something similar, we can revisit this patch.
I am not so sure whether changing lea to add (which will update eflags) may cause other unknown problems, though.

Should I submit a patch or discuss it with someone somewhere first? I am not very competent in this kind of topics and how it's usually being done.

OK, now that I re-checked the x86-64 ABI, I think this change will likely not be accepted on binutils side :(
You can just file a bug on https://gcc.gnu.org/bugzilla/show_bug.cgi and there are folks working on binutils, too.

In D145062#4163222, @MaskRay wrote:

If you can get a smaller reproduce, you can report this on https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93623
The key is extern thread_local double precise_now (__thread will paper over the bug) which has a undefined weak symbol _ZTH* which will be instrumented by -fsanitize=undefined.

Thanks for the suggestion. Do you mean that specific issue (it does not seem to be related to our subject at first glance) or reporting to GCC's Bugzilla in general?
As for __thread, it's a nice catch. I think can be used as a workaround until the issue is solved. Also, it exaplains why GCC has decided to instrument a non-weak symbol.

lld/ELF/Arch/X86_64.cpp

 
 // In some conditions, R_X86_64_GOTTPOFF relocation can be optimized to
 // R_X86_64_TPOFF32 so that it does not use GOT.
 static void relaxTlsIeToLe(uint8_t *loc, const Relocation &, uint64_t val) {
   uint8_t *inst = loc - 3;
   uint8_t reg = loc[-1] >> 3;
   uint8_t *regSlot = loc - 1;
 
-  // Note that ADD with RSP or R12 is converted to ADD instead of LEA
-  // because LEA with these registers needs 4 bytes to encode and thus
-  // wouldn't fit the space.
-
-  if (memcmp(inst, "\x48\x03\x25", 3) == 0) {
-    // "addq foo@gottpoff(%rip),%rsp" -> "addq $foo,%rsp"
-    memcpy(inst, "\x48\x81\xc4", 3);
-  } else if (memcmp(inst, "\x4c\x03\x25", 3) == 0) {
-    // "addq foo@gottpoff(%rip),%r12" -> "addq $foo,%r12"
-    memcpy(inst, "\x49\x81\xc4", 3);
-  } else if (memcmp(inst, "\x4c\x03", 2) == 0) {
-    // "addq foo@gottpoff(%rip),%r[8-15]" -> "leaq foo(%r[8-15]),%r[8-15]"
-    memcpy(inst, "\x4d\x8d", 2);
-    *regSlot = 0x80 | (reg << 3) | reg;
+  // Note that ADD cannot be optimized to LEA because it does not set flags
+
+  if (memcmp(inst, "\x4c\x03", 2) == 0) {
+    // "addq foo@gottpoff(%rip),%r[8-15]" -> "addq $foo,%r[8-15]"
+    memcpy(inst, "\x49\x81", 2);
+    *regSlot = 0xc0 | reg;
   } else if (memcmp(inst, "\x48\x03", 2) == 0) {
-    // "addq foo@gottpoff(%rip),%reg -> "leaq foo(%reg),%reg"
-    memcpy(inst, "\x48\x8d", 2);
-    *regSlot = 0x80 | (reg << 3) | reg;
+    // "addq foo@gottpoff(%rip),%reg -> "addq $foo,%reg"
+    memcpy(inst, "\x48\x81", 2);
+    *regSlot = 0xc0 | reg;
   } else if (memcmp(inst, "\x4c\x8b", 2) == 0) {
     // "movq foo@gottpoff(%rip),%r[8-15]" -> "movq $foo,%r[8-15]"
     memcpy(inst, "\x49\xc7", 2);
     *regSlot = 0xc0 | reg;
   } else if (memcmp(inst, "\x48\x8b", 2) == 0) {
     // "movq foo@gottpoff(%rip),%reg" -> "movq $foo,%reg"
     memcpy(inst, "\x48\xc7", 2);
     *regSlot = 0xc0 | reg;

lld/test/ELF/tls-opt.s

 // REQUIRES: x86
 // RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
 // RUN: ld.lld %t.o -o %t1
 // RUN: llvm-readobj -r %t1 | FileCheck --check-prefix=NORELOC %s
 // RUN: llvm-objdump --no-print-imm-hex -d --no-show-raw-insn %t1 | FileCheck --check-prefix=DISASM %s
 
 // NORELOC:      Relocations [
 // NORELOC-NEXT: ]
 
 // DISASM:      <_start>:
 // DISASM-NEXT:   movq $-8, %rax
 // DISASM-NEXT:   movq $-8, %r15
-// DISASM-NEXT:   leaq -8(%rax), %rax
-// DISASM-NEXT:   leaq -8(%r15), %r15
+// DISASM-NEXT:   addq $-8, %rax
+// DISASM-NEXT:   addq $-8, %r15
 // DISASM-NEXT:   addq $-8, %rsp
 // DISASM-NEXT:   addq $-8, %r12
 // DISASM-NEXT:   movq $-4, %rax
 // DISASM-NEXT:   movq $-4, %r15
-// DISASM-NEXT:   leaq -4(%rax), %rax
-// DISASM-NEXT:   leaq -4(%r15), %r15
+// DISASM-NEXT:   addq $-4, %rax
+// DISASM-NEXT:   addq $-4, %r15
 // DISASM-NEXT:   addq $-4, %rsp
 // DISASM-NEXT:   addq $-4, %r12
 
 // LD to LE:
 // DISASM-NEXT:   movq %fs:0, %rax
 // DISASM-NEXT:   leaq -8(%rax), %rcx
 // DISASM-NEXT:   movq %fs:0, %rax
 // DISASM-NEXT:   leaq -4(%rax), %rcx

lld/test/ELF/tls-weak-undef.s

 ## An undefined weak TLS symbol does not fetch a lazy definition.
 # RUN: ld.lld %texec.o --start-lib %tdef.o --end-lib -o %tlazy
 # RUN: llvm-objdump --no-print-imm-hex -d %tlazy | FileCheck %s --check-prefix=EXEC
 
 ## Undefined TLS symbols arbitrarily resolve to 0.
 # EXEC:   leaq 16(%rax), %rdx
 ## Initial-exec references to undefined weak symbols can be relaxed to LE
 ## references.
-# EXEC:   leaq 32(%rax), %rax
+# EXEC:   addq $32, %rax
 # SHARED: leaq 48(%rax), %rcx
 
 # RUN: ld.lld -shared %tdef.o -o %tdef.so
 # RUN: not ld.lld %texec.o %tdef.so -o /dev/null 2>&1 | FileCheck --check-prefix=ERROR %s
 
 # ERROR: error: relocation R_X86_64_TPOFF32 cannot be used against symbol 'le'; recompile with -fPIC
 
 #--- ledef.s

lld/test/ELF/x86-64-tls-ie-opt-local.s

 // REQUIRES: x86
 // RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
 // RUN: ld.lld %t.o -o %t1
 // RUN: llvm-readobj -r %t1 | FileCheck --check-prefix=NORELOC %s
 // RUN: llvm-objdump --no-print-imm-hex -d %t1 | FileCheck --check-prefix=DISASM %s
 
 // NORELOC:      Relocations [
 // NORELOC-NEXT: ]
 
 // DISASM:      Disassembly of section .text:
 // DISASM-EMPTY:
 // DISASM-NEXT: <_start>:
 // DISASM-NEXT:   movq $-8, %rax
 // DISASM-NEXT:   movq $-8, %r15
-// DISASM-NEXT:   leaq -8(%rax), %rax
-// DISASM-NEXT:   leaq -8(%r15), %r15
+// DISASM-NEXT:   addq $-8, %rax
+// DISASM-NEXT:   addq $-8, %r15
 // DISASM-NEXT:   addq $-8, %rsp
 // DISASM-NEXT:   addq $-8, %r12
 // DISASM-NEXT:   movq $-4, %rax
 // DISASM-NEXT:   movq $-4, %r15
-// DISASM-NEXT:   leaq -4(%rax), %rax
-// DISASM-NEXT:   leaq -4(%r15), %r15
+// DISASM-NEXT:   addq $-4, %rax
+// DISASM-NEXT:   addq $-4, %r15
 // DISASM-NEXT:   addq $-4, %rsp
 // DISASM-NEXT:   addq $-4, %r12
 
 .section .tbss,"awT",@nobits
 
 .type tls0,@object
 .align 4
 tls0: