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

[Peter Wu via Phabricator via llvm-commits]

Lekensteyn marked 12 inline comments as done.
Lekensteyn added inline comments.


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


================
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;
----------------
grimar wrote:
> 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.
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?


================
Comment at: ELF/Arch/X86_64.cpp:305
+    memcpy(Loc - 2, Inst, sizeof(Inst));
+  } else
+    error(getErrorLocation(Loc - 3) +
----------------
grimar wrote:
> 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");
> ```
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


================
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
----------------
grimar wrote:
> Do you need `--hash-style=sysv`?
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?


================
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
----------------
grimar wrote:
> We often skip checking the exact section index when it is not an intention:
> 
> ```
> Section {{.*}} .rela.dyn
> ```
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?


================
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
----------------
grimar wrote:
> 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?
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.)


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D56779