[PATCH] D14713: [ELF2] - Optimization for R_X86_64_GOTTPOFF relocation.

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/Target.cpp:371
@@ +370,3 @@
+  if (Loc - 3 < BufStart)
+    error("Tls relocation optimization fail, buffer overrun !");
+  uint8_t *Prefix = &Loc[-3];
----------------
rafael wrote:
> Please add a test for this.
> 
> It is important to show that we can actually get here with broken input.
I bit doubt that we should check such things here. Broken input for that place with or without that patch will lead to broken output. With this optimization applied or without it. The difference only in what way it broken (broken + optimization == broken x 2, but still broken).
Broken input is a bug of compiler and should be covered by its tests or by our tests of broken imputs that are separate tests.
Second problem that if according manual this relocation must use only mov or add then in future llvm-mc for example may add check for that and do not generate broken output. That will make this test to fail. We can save from that only by placing precompiled corrupted binaries as inputs, but thats again not for that patch I think.
But anyways just in case I added what you asked for to test. So if you really think we should do that for some reason - I see nothing too much bad in that if it touches only the test. Its easy to remove from test at any time.

================
Comment at: ELF/Target.cpp:392
@@ +391,3 @@
+  relocateOne(Loc, BufEnd, R_X86_64_TPOFF32, P, SA);
+}
+
----------------
ruiu wrote:
> I applied you patch to my local repository to see if there's a room to make this simpler, only to find that this wouldn't be significantly simplified. I slightly updated the comments and code though. Please take a look.
> 
>   // In some conditions, R_X86_64_GOTTPOFF relocation can be optimized to
>   // R_X86_64_TPOFF32 so that R_X86_64_TPOFF32 so that it does not use GOT.
>   // This function does that. Read "ELF Handling For Thread-Local Storage,
>   // 5.5 x86-x64 linker optimizations" (http://www.akkadia.org/drepper/tls.pdf)
>   // by Ulrich Drepper for details.
>   void X86_64TargetInfo::relocateTlsOptimize(uint8_t *Loc, uint8_t *BufStart,
>                                              uint8_t *BufEnd, uint64_t P,
>                                              uint64_t SA) const {
>     // Ulrich's document section 6.5 says that @gottpoff(%rip) must be
>     // used in MOVQ or ADDQ instructions only.
>     // "MOVQ foo at GOTTPOFF(%RIP), %REG" is transformed to "MOVQ $foo, %REG".
>     // "ADDQ foo at GOTTPOFF(%RIP), %REG" is transformed to "LEAQ foo(%REG), %REG"
>     // (if the register is not RSP) or "ADDQ $foo, %RSP".
>     // Opcodes info can be found at http://ref.x86asm.net/coder64.html#x48.
>     if (Loc - 3 < BufStart)
>       error("TLS relocation optimization failed. Buffer overrun!");
>     uint8_t *Prefix = Loc - 3;
>     uint8_t *Inst = Loc - 2;
>     uint8_t *RegSlot = Loc - 1;
>     uint8_t Reg = Loc[-1] >> 3;
>     bool IsMov = *Inst == 0x8b;
>     bool RspAdd = !IsMov && Reg == 4;
>     // r12 and rsp registers requires special handling.
>     // Problem is that for other registers, for example leaq 0xXXXXXXXX(%r11),%r11
>     // result out is 7 bytes: 4d 8d 9b XX XX XX XX,
>     // but leaq 0xXXXXXXXX(%r12),%r12 is 8 bytes: 4d 8d a4 24 XX XX XX XX.
>     // The same true for rsp. So we convert to addq for them, saving 1 byte that
>     // we dont have.
>     if (RspAdd)
>       *Inst = 0x81;
>     else
>       *Inst = IsMov ? 0xc7 : 0x8d;
>     if (*Prefix == 0x4c)
>       *Prefix = (IsMov || RspAdd) ? 0x49 : 0x4d;
>     *RegSlot = (IsMov || RspAdd) ? (0xc0 | Reg) : (0x80 | Reg | (Reg << 3));
>     relocateOne(Loc, BufEnd, R_X86_64_TPOFF32, P, SA);
>   }
> 
Looks good. Applied your changes. Thanks !

I only added r12 to comment:
```
// (if the register is not RSP/R12) or "ADDQ $foo, %RSP".
```

================
Comment at: test/elf2/tls-opt.s:2
@@ +1,3 @@
+// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
+// RUN: ld.lld2 -e main %t.o -o %t1
+// RUN: llvm-readobj -r %t1 | FileCheck --check-prefix=NORELOC %s
----------------
rafael wrote:
> Just create a _start and drop the "-e main"
Done.

================
Comment at: test/elf2/tls-opt.s:13
@@ +12,3 @@
+// DISASM-NEXT: 11007: 49 c7 c7 f8 ff ff ff movq $-8, %r15
+// DISASM-NEXT: 1100e: 48 8d 80 f8 ff ff ff leaq -8(%rax), %rax
+// DISASM-NEXT: 11015: 4d 8d bf f8 ff ff ff leaq -8(%r15), %r15
----------------
rafael wrote:
> Do you really need the binary content? The text should be sufficient, no? I.E.:
> 
> // DISASM-NEXT: 11000: {{.*}} movq $-8, %rax
Any of them is sufficient I think but we are emiting binary output and not the text. So need to keep and check the binary. At the same time text helps to read the test so it should be there at least in comments for binary I think. But there is no point to move it to comments I believe. I would leave it as is because of that all.


http://reviews.llvm.org/D14713