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

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

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



================
Comment at: ELF/Arch/X86_64.cpp:275
   //   mov %fs:0,%rax
   //   leaq bar at tpoff(%rax), %rcx
   if (Type == R_X86_64_DTPOFF64) {
----------------
Should this comment be moved under the `if (NextOp == 0xe8)` check below it seems?


================
Comment at: ELF/Arch/X86_64.cpp:291
+  const uint8_t NextOp = Loc[4];
+  const uint8_t NextModRm = Loc[5];
+  if (NextOp == 0xe8)
----------------
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];
...
```


================
Comment at: ELF/Arch/X86_64.cpp:302
+    // 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;
----------------
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.


================
Comment at: ELF/Arch/X86_64.cpp:305
+    memcpy(Loc - 2, Inst, sizeof(Inst));
+  } else
+    error(getErrorLocation(Loc - 3) +
----------------
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 at tlsld(%rip),%rdi               # 48 8d 3d <Loc>
    //   call *__tls_get_addr at 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");
```


================
Comment at: ELF/Arch/X86_64.cpp:307
+    error(getErrorLocation(Loc - 3) +
+          "Expected R_X86_64_PLT32 or R_X86_64_GOTPCRELX after R_X86_64_TLSLD");
 }
----------------
`expected` should be lowercase I think.


================
Comment at: test/ELF/tls-opt-x86_64-noplt.s:5
+// RUN: ld.lld -shared %tso.o -o %t.so
+// RUN: ld.lld --hash-style=sysv %t.o %t.so -o %t1
+// RUN: llvm-readobj -r %t1 | FileCheck --check-prefix=RELOC %s
----------------
Do you need `--hash-style=sysv`?


================
Comment at: test/ELF/tls-opt-x86_64-noplt.s:10
+// RELOC:      Relocations [
+// RELOC-NEXT:  Section (4) .rela.dyn {
+// RELOC-NEXT:    0x2020B0 R_X86_64_TPOFF64 tlsshared0 0x0
----------------
We often skip checking the exact section index when it is not an intention:

```
Section {{.*}} .rela.dyn
```


================
Comment at: test/ELF/tls-opt-x86_64-noplt.s:18
+
+// Table 11.5: GD -> IE Code Transition (LP64)
+// DISASM-NEXT: 201000: 64 48 8b 04 25 00 00 00 00      movq %fs:0, %rax
----------------
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?


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56779/new/

https://reviews.llvm.org/D56779