[ELF][X86_64] Fix corrupted LD -> LE optimization for TLS without PLT

[Lekensteyn]

Summary

The LD -> LE optimization for Thread-Local Storage without PLT requires
an additional "66" prefix, otherwise the next instruction will be
corrupted, causing runtime misbehavior (crashes) of the linked object.

The other (GD -> IE/LD) optimizations are the same with or without PLT,
but at tests for completeness. Those instructions are copied from
https://raw.githubusercontent.com/wiki/hjl-tools/x86-psABI/x86-64-psABI-1.0.pdf#subsection.11.1.2

This does not try to address ILP32 (x32) support.

Fixes https://bugs.llvm.org/show_bug.cgi?id=37303

[grimar]

I think it is good. My comments are below (please wait for Rui's ones though, he might have different opinions I guess).

ELF/Arch/X86_64.cpp

275 ↗

(On Diff #182018)

Should this comment be moved under the if (NextOp == 0xe8) check below it seems?

291 ↗

(On Diff #182018)

I would probably do an early return here (to avoid unneccessary read of the Loc[5]):

const uint8_t NextOp = Loc[4];
if (NextOp == 0xe8) {
  memcpy(Loc - 3, Inst, sizeof(Inst));
  return;
}

const uint8_t NextModRm = Loc[5];
...

302 ↗

(On Diff #182018)

I do not think we included the links for the code that do relaxations. At least I did not find it around, so I would remove the last 2 lines for consistency.

305 ↗

(On Diff #182018)

LLD code style suggests that you should wrap all branches into `{ ... }' here.

To avoid doing that for single error(... line I would also use early return for this place then:

if (NextOp == 0xff && NextModRm == 0x15) {
    // Convert
    //   leaq  x@tlsld(%rip),%rdi               # 48 8d 3d <Loc>
    //   call *__tls_get_addr@GOTPCREL(%rip)    # ff 15 <disp32>
    // to
    //   .long  0x66666666
    //   movq   %fs:0,%rax
    // See "Table 11.9: LD -> LE Code Transition (LP64)" in
    // https://raw.githubusercontent.com/wiki/hjl-tools/x86-psABI/x86-64-psABI-1.0.pdf
    Loc[-3] = 0x66;
    memcpy(Loc - 2, Inst, sizeof(Inst));
    return;
}

error(getErrorLocation(Loc - 3) +
          "Expected R_X86_64_PLT32 or R_X86_64_GOTPCRELX after R_X86_64_TLSLD");

307 ↗

(On Diff #182018)

expected should be lowercase I think.

test/ELF/tls-opt-x86_64-noplt.s

5 ↗

(On Diff #182018)

Do you need --hash-style=sysv?

10 ↗

(On Diff #182018)

We often skip checking the exact section index when it is not an intention:

Section {{.*}} .rela.dyn

18 ↗

(On Diff #182018)

I do not remember we referred to the spec tables in the tests anywhere earlier, it seems not a too bad idea though,
because TLS optimizations is a pain.

I would either add a link to an x86-64-psABI-1.0.pdf in this test or maybe inlined the tables right here?

Rui, what do you think?

[Lekensteyn]

ELF/Arch/X86_64.cpp

275 ↗

(On Diff #182018)

Not entirely, only the first two instructions (lea and call) are patched by the code below.

I thought that the third instruction is handled by the DTPOFF blocks below, but I could be mistaken though.

302 ↗

(On Diff #182018)

It used to be documented in the past, but then it got moved to ELF/Target.cpp.

Would you like it to be removed here?

305 ↗

(On Diff #182018)

Will be fixed in the next patch.

In the past I received different feedback in the past regarding single statement blocks, is there any documention about the style? I can't find guidance on the use of curly braces in blocks in any of:
https://llvm.org/docs/CodingStandards.html
https://llvm.org/docs/ProgrammersManual.html
https://clang.llvm.org/docs/ClangFormatStyleOptions.html

test/ELF/tls-opt-x86_64-noplt.s

5 ↗

(On Diff #182018)

It does not hurt and seems to be used in other tests as well (the default seems --hash-style=sysv on MIPS and --hash-style=both on others).

If I drop it, then the section number and offsets below will also have to be updated. Any preference whether to keep it or not?

10 ↗

(On Diff #182018)

If the section changes, then the offsets will likely have to be updated as well, so it might not save that much. I was inspired by test/ELF/tls-opt-gdie.s. Should I apply your suggestion here?

18 ↗

(On Diff #182018)

The references were helpful as I could cross-reference the actual output with the expected output :)
I'll add the URL to this file as well. The left side of the table is listed below while the right side of the table appears in the objdump output here.

(During development of this patch, I did not link with an external symbol which resulted in GD -> IE not being tested correctly, it was treated as GD -> LE.)

[grimar]

ELF/Arch/X86_64.cpp

302 ↗

(On Diff #182018)

We should wait for Rui's opinion here I think. He usually has own preferences about comments and documenting the code.

305 ↗

(On Diff #182018)

LLD can have its own minor differences/tweaks of style. Usually - if you doubt, just look at the code around,
we are very consistent I believe.

A rule for braces in LLD: if any branch has them, then all of the branches should.
We do not use braces for single lines of code. But see above.

So the following example are correct for LLD:

if (XXX)
  do();

if (XXX) {
 do1();
 do2();
} else {
 do3();
}

test/ELF/tls-opt-x86_64-noplt.s

5 ↗

(On Diff #182018)

We do not include the options that are not needed for the test usually.
Some of our tests contains --hash-style=sysv because when we switched style used by default we did not want to update offsets and things for all tests. So we had to do that. But for a new test, I see no reason to include it.

10 ↗

(On Diff #182018)

Up to you.

You can just omit the offsets here, I think it is the best way. See znotext-plt-relocations.s for example.
Using of {{.*}} is generally very common, but I know not of our tests do that.

[Lekensteyn]

Updated coding style as suggested. Only one issue is still open (whether to remove/change the comment in the source file). Thank you for the feedback!

[ruiu]

ELF/Arch/X86_64.cpp

267–275 ↗

(On Diff #182054)

Now this comment is at a wrong place? It looks like this comment explains the entire function, but I believe now this comment explains on the first "if" condition.

[Lekensteyn]

ELF/Arch/X86_64.cpp

267–275 ↗

(On Diff #182054)

The comment indeed describes the if (NextOp == 0xe8) { case below, except for the third instruction (related to dtpoff). How is that last instruction handled?

I kept the comment here because I thought that the if (Type == R_X86_DTPOFF*) { blocks took care of it. I can move it down, but this part I did not understand.

[ruiu]

ELF/Arch/X86_64.cpp

267–275 ↗

(On Diff #182054)

I mean why don't you move this comment inside if (NextOp == 0xe8) {?

[ruiu]

ELF/Arch/X86_64.cpp

290–291 ↗

(On Diff #182054)

When we are rewriting instructions, we don't have to understand the instruction encoding or anything but can just follow the rule. So I'd remove NextOp varaible and use Loc[4] instead. Likewise, remove NextModRm and just use Loc[5].

[Lekensteyn]

I mean why don't you move this comment inside if (NextOp == 0xe8) {?

Because memcpy only patches the first two instructions:

// Convert
//   leaq bar@tlsld(%rip), %rdi     # 48 8d 3d <Loc>
//   callq __tls_get_addr@PLT       # e8 <disp32>

but not this one (which is why I initially left the comment on top):

//   leaq bar@dtpoff(%rax), %rcx

I've now moved the comment verbatim and inlined the NextOp and NextModRm names.

[ruiu]

LGTM

ELF/Arch/X86_64.cpp

281 ↗

(On Diff #182150)

Add a blank line.

[Lekensteyn]

Can I backport this to the 8.0 release branch? Are there any special rules for that?

(Should release notes also be updated?)

ELF/Arch/X86_64.cpp

281 ↗

(On Diff #182150)

Before the if? I will also fix the wrong indentation for the comment.

[efriedma]

See http://llvm.org/docs/HowToReleaseLLVM.html#merge-requests for merging patches to the release branch.

[ruiu]

We should cherrypick this patch to 8.0 branch so that 8.0 will be shipped with this fix included. I'll file a bug and do, so no worries.

ELF/Arch/X86_64.cpp

281 ↗

(On Diff #182150)

Yes. I would add a blank line before if so that the variable definition and if doesn't come too close.

[Lekensteyn]

We should cherrypick this patch to 8.0 branch so that 8.0 will be shipped with this fix included. I'll file a bug and do, so no worries.

Thanks Eli for the information and thank you Rui for taking care of backporting! I'll merge this version now to trunk.

[ruiu]

Cherried pick to 8.0 branch as r351401.

lld/trunk/ELF/Arch/X86_64.cpp

void X86_64<ELFT>::relaxTlsIeToLe(uint8_t *Loc, RelType Type,
   // The original code used a PC relative relocation.
   // Need to compensate for the -4 it had in the addend.
   write32le(Loc, Val + 4);
 }
 
 template <class ELFT>
 void X86_64<ELFT>::relaxTlsLdToLe(uint8_t *Loc, RelType Type,
                                   uint64_t Val) const {
-  // Convert
-  //   leaq bar@tlsld(%rip), %rdi
-  //   callq __tls_get_addr@PLT
-  //   leaq bar@dtpoff(%rax), %rcx
-  // to
-  //   .word 0x6666
-  //   .byte 0x66
-  //   mov %fs:0,%rax
-  //   leaq bar@tpoff(%rax), %rcx
   if (Type == R_X86_64_DTPOFF64) {
     write64le(Loc, Val);
     return;
   }
   if (Type == R_X86_64_DTPOFF32) {
     write32le(Loc, Val);
     return;
   }
 
   const uint8_t Inst[] = {
       0x66, 0x66,                                           // .word 0x6666
       0x66,                                                 // .byte 0x66
       0x64, 0x48, 0x8b, 0x04, 0x25, 0x00, 0x00, 0x00, 0x00, // mov %fs:0,%rax
   };
+
+  if (Loc[4] == 0xe8) {
+    // Convert
+    //   leaq bar@tlsld(%rip), %rdi           # 48 8d 3d <Loc>
+    //   callq __tls_get_addr@PLT             # e8 <disp32>
+    //   leaq bar@dtpoff(%rax), %rcx
+    // to
+    //   .word 0x6666
+    //   .byte 0x66
+    //   mov %fs:0,%rax
+    //   leaq bar@tpoff(%rax), %rcx
     memcpy(Loc - 3, Inst, sizeof(Inst));
+    return;
+  }
+
+  if (Loc[4] == 0xff && Loc[5] == 0x15) {
+    // Convert
+    //   leaq  x@tlsld(%rip),%rdi               # 48 8d 3d <Loc>
+    //   call *__tls_get_addr@GOTPCREL(%rip)    # ff 15 <disp32>
+    // to
+    //   .long  0x66666666
+    //   movq   %fs:0,%rax
+    // See "Table 11.9: LD -> LE Code Transition (LP64)" in
+    // https://raw.githubusercontent.com/wiki/hjl-tools/x86-psABI/x86-64-psABI-1.0.pdf
+    Loc[-3] = 0x66;
+    memcpy(Loc - 2, Inst, sizeof(Inst));
+    return;
+  }
+
+  error(getErrorLocation(Loc - 3) +
+        "expected R_X86_64_PLT32 or R_X86_64_GOTPCRELX after R_X86_64_TLSLD");
 }
 
 template <class ELFT>
 void X86_64<ELFT>::relocateOne(uint8_t *Loc, RelType Type, uint64_t Val) const {
   switch (Type) {
   case R_X86_64_8:
     checkUInt(Loc, Val, 8, Type);
     *Loc = Val;

lld/trunk/test/ELF/tls-opt-x86_64-noplt.s

+// REQUIRES: x86
+
+// Checks whether the TLS optimizations match the cases in Chapter 11 of
+// https://raw.githubusercontent.com/wiki/hjl-tools/x86-psABI/x86-64-psABI-1.0.pdf
+
+// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
+// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/tls-opt-gdie.s -o %tso.o
+// RUN: ld.lld -shared %tso.o -o %t.so
+// RUN: ld.lld %t.o %t.so -o %t1
+// RUN: llvm-readobj -r %t1 | FileCheck --check-prefix=RELOC %s
+// RUN: llvm-objdump -d %t1 | FileCheck --check-prefix=DISASM %s
+
+// RELOC:      Relocations [
+// RELOC-NEXT:  Section {{.*}} .rela.dyn {
+// RELOC-NEXT:    0x2020C0 R_X86_64_TPOFF64 tlsshared0 0x0
+// RELOC-NEXT:    0x2020C8 R_X86_64_TPOFF64 tlsshared1 0x0
+// RELOC-NEXT:  }
+// RELOC-NEXT: ]
+
+// DISASM:      _start:
+
+// Table 11.5: GD -> IE Code Transition (LP64)
+// DISASM-NEXT: 201000: 64 48 8b 04 25 00 00 00 00      movq %fs:0, %rax
+// DISASM-NEXT: 201009: 48 03 05 b0 10 00 00            addq 4272(%rip), %rax
+// DISASM-NEXT: 201010: 64 48 8b 04 25 00 00 00 00      movq %fs:0, %rax
+// DISASM-NEXT: 201019: 48 03 05 a8 10 00 00            addq 4264(%rip), %rax
+
+// Table 11.7: GD -> LE Code Transition (LP64)
+// DISASM-NEXT: 201020: 64 48 8b 04 25 00 00 00 00      movq %fs:0, %rax
+// DISASM-NEXT: 201029: 48 8d 80 f8 ff ff ff            leaq -8(%rax), %rax
+// DISASM-NEXT: 201030: 64 48 8b 04 25 00 00 00 00      movq %fs:0, %rax
+// DISASM-NEXT: 201039: 48 8d 80 fc ff ff ff            leaq -4(%rax), %rax
+
+
+// Table 11.9: LD -> LE Code Transition (LP64)
+// DISASM-NEXT: 201040: 66 66 66 66 64 48 8b 04 25 00 00 00 00  movq %fs:0, %rax
+// DISASM-NEXT: 20104d: 66 66 66 66 64 48 8b 04 25 00 00 00 00  movq %fs:0, %rax
+
+.type tls0,@object
+.section .tbss,"awT",@nobits
+.globl tls0
+.align 4
+tls0:
+ .long 0
+ .size tls0, 4
+
+.type  tls1,@object
+.globl tls1
+.align 4
+tls1:
+ .long 0
+ .size tls1, 4
+
+.section .text
+.globl _start
+_start:
+ // Table 11.5: GD -> IE Code Transition (LP64)
+ .byte  0x66
+ leaq   tlsshared0@tlsgd(%rip),%rdi
+ .byte  0x66
+ rex64
+ call   *__tls_get_addr@GOTPCREL(%rip)
+
+ .byte  0x66
+ leaq   tlsshared1@tlsgd(%rip),%rdi
+ .byte  0x66
+ rex64
+ call   *__tls_get_addr@GOTPCREL(%rip)
+
+ // Table 11.7: GD -> LE Code Transition (LP64)
+ .byte  0x66
+ leaq   tls0@tlsgd(%rip),%rdi
+ .byte  0x66
+ rex64
+ call   *__tls_get_addr@GOTPCREL(%rip)
+
+ .byte  0x66
+ leaq   tls1@tlsgd(%rip),%rdi
+ .byte  0x66
+ rex64
+ call   *__tls_get_addr@GOTPCREL(%rip)
+
+ // Table 11.9: LD -> LE Code Transition (LP64)
+ leaq   tls0@tlsld(%rip),%rdi
+ call   *__tls_get_addr@GOTPCREL(%rip)
+
+ leaq   tls1@tlsld(%rip),%rdi
+ call   *__tls_get_addr@GOTPCREL(%rip)