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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
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;
----------------
Lekensteyn wrote:
> 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?
We should wait for Rui's opinion here I think. He usually has own preferences about comments and documenting the code.


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





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


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


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D56779