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

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/InputSection.cpp:150-154
@@ -149,3 +149,7 @@
       continue;
+    } else if (Body.isTLS() &&
+               Target->getTlsOptimization(Type, Body) == TargetInfo::ToLE) {
+      Target->relocateTlsToLe(BufLoc, BufEnd, Type, AddrLoc, SymVA);
+      continue;
     }
     Target->relocateOne(BufLoc, BufEnd, Type, AddrLoc,
----------------
ruiu wrote:
> grimar wrote:
> > ruiu wrote:
> > > Can you move this above line 137 just like "if (Target->isTlsGlobalDynamicReloc(Type)) { ... }"?
> > > 
> > > Also, do you think you can merge getTlsOptimization and relocateTlsToLe into one function? I'd probably write like this.
> > > 
> > >   // Some TLS relocations are always compiled to use GOT, but the linker
> > >   // is sometimes able to rewrite it so that it doesn't use GOT. This may not
> > >   // only apply relocations but also modify preceding instructions.
> > >   if (applyOptimizeTls(BufLoc, Type, AddrLoc, Body))
> > >     continue;
> > About merging into one.
> > getTlsOptimization() is also now used also in another place, in bool X86_64TargetInfo::relocNeedsGot().
> > So such merging probably not possible.
> Fair. Then please rename relocateTlsOptimize. (Readers don't really want to know how it is going to be optimized in this context, so "ToLe" is too detailed.)
Renaming is done, but I did not move it because I need SymVA variable. Also this part of code became shorter now. And it uses continue like Target->relocNeedsCopy, so it looks to be in the most consistent place now, no ?

================
Comment at: ELF/Target.cpp:362-368
@@ +361,9 @@
+  uint8_t Reg = Loc[-1] >> 3;
+  // Originally it can be one of two:
+  // 1) movq foo at gottpoff(%rip), %reg
+  // We change it into one of:
+  // movq $foo, %reg
+  // addq $foo, %rsp (addressing with %rsp is special).
+  // 2) addq foo at gottpoff(%rip), %reg
+  // We change it into leaq foo(%reg), %reg.
+  bool RspAdd = (Ins != 0x8b && Reg == 4);
----------------
ruiu wrote:
> grimar wrote:
> > ruiu wrote:
> > > grimar wrote:
> > > > I dont sure what to do here. How to check out of bound for negative idx. Should I check (will need BufBegin arg I guess) or we can assume its never be out ?
> > > Is that actually a possible scenario that one wants to add a TLS value to SP register? Do you have to take care of that?
> > I did not analyse how much people do that :) 
> > gold and bfd do the same. And it is still possible scenario even if chance is low. I jsut want to be on a safe side with this single line.
> As long as it can be written simple enough, I'm fine, but currently it looks a bit too cryptic.
> 
> There are six instructions this function may emit. Could you explain what each instruction mean?
> 
> 0x49 0x81 0xc?
> 0x49 0xc7 0xc?
> 0x?? 0x81 0xc?
> 0x?? 0xc7 0xc?
> 0x4d 0x8d 0x8?
> 0x?? 0x8d 0x8?
That is (but I simplified code, so actual table is below in comments)

1) 0x49 0x81 0xc?
This is 0x49 0x81 0xC? XX XX XX XX, add $0xXXXXXXXX, %r?
Ex: 49 81 C0 00 00 00 00 = add $0x0, %r8

2) 0x49 0xc7 0xc?
This is 0x49 0xC7 0xC? XX XX XX XX, mov $0xXXXXXXXX, %r?
Ex: 49 C7 C0 00 00 00 00 = mov $0x0, %r8

3) 0x?? 0x81 0xc?
Never saw not the 0x48 as prefix for that cases, so
This is 0x48 0x81 0xC? XX XX XX XX, add $0xXXXXXXXX, %r??
Ex: 48 81 C0 00 00 00 00 = add $0x0, %rax

4) 0x?? 0xc7 0xc? 
Never saw not the 0x48 as prefix for that cases, so
This is 0x48 0xc7 0xc? XX XX XX XX, mov $0xXXXXXXXX, %r??
Ex: 48 C7 C0 00 00 00 00 = mov $0x0, %rax

5) 0x4d 0x8d 0x8?
This is 0x4D 0x8D 0x8? XX XX XX XX, lea 0xXXXXXXXX(%r?),%r?
Ex: 4D 8D 89 00 00 00 00 = lea 0x0(%r9),%r9

6) 0x?? 0x8d 0x8?
Never saw not the 0x48 as prefix for that cases, so
This is 0x48 0x8d 0x8? XX XX XX XX, lea 0xXXXXXXXX(%r??),%r??
Ex: 48 8D 89 00 00 00 00 = lea 0x0(%rcx),%rcx

================
Comment at: ELF/Target.cpp:362-368
@@ +361,9 @@
+  uint8_t Reg = Loc[-1] >> 3;
+  // Originally it can be one of two:
+  // 1) movq foo at gottpoff(%rip), %reg
+  // We change it into one of:
+  // movq $foo, %reg
+  // addq $foo, %rsp (addressing with %rsp is special).
+  // 2) addq foo at gottpoff(%rip), %reg
+  // We change it into leaq foo(%reg), %reg.
+  bool RspAdd = (Ins != 0x8b && Reg == 4);
----------------
grimar wrote:
> ruiu wrote:
> > grimar wrote:
> > > ruiu wrote:
> > > > grimar wrote:
> > > > > I dont sure what to do here. How to check out of bound for negative idx. Should I check (will need BufBegin arg I guess) or we can assume its never be out ?
> > > > Is that actually a possible scenario that one wants to add a TLS value to SP register? Do you have to take care of that?
> > > I did not analyse how much people do that :) 
> > > gold and bfd do the same. And it is still possible scenario even if chance is low. I jsut want to be on a safe side with this single line.
> > As long as it can be written simple enough, I'm fine, but currently it looks a bit too cryptic.
> > 
> > There are six instructions this function may emit. Could you explain what each instruction mean?
> > 
> > 0x49 0x81 0xc?
> > 0x49 0xc7 0xc?
> > 0x?? 0x81 0xc?
> > 0x?? 0xc7 0xc?
> > 0x4d 0x8d 0x8?
> > 0x?? 0x8d 0x8?
> That is (but I simplified code, so actual table is below in comments)
> 
> 1) 0x49 0x81 0xc?
> This is 0x49 0x81 0xC? XX XX XX XX, add $0xXXXXXXXX, %r?
> Ex: 49 81 C0 00 00 00 00 = add $0x0, %r8
> 
> 2) 0x49 0xc7 0xc?
> This is 0x49 0xC7 0xC? XX XX XX XX, mov $0xXXXXXXXX, %r?
> Ex: 49 C7 C0 00 00 00 00 = mov $0x0, %r8
> 
> 3) 0x?? 0x81 0xc?
> Never saw not the 0x48 as prefix for that cases, so
> This is 0x48 0x81 0xC? XX XX XX XX, add $0xXXXXXXXX, %r??
> Ex: 48 81 C0 00 00 00 00 = add $0x0, %rax
> 
> 4) 0x?? 0xc7 0xc? 
> Never saw not the 0x48 as prefix for that cases, so
> This is 0x48 0xc7 0xc? XX XX XX XX, mov $0xXXXXXXXX, %r??
> Ex: 48 C7 C0 00 00 00 00 = mov $0x0, %rax
> 
> 5) 0x4d 0x8d 0x8?
> This is 0x4D 0x8D 0x8? XX XX XX XX, lea 0xXXXXXXXX(%r?),%r?
> Ex: 4D 8D 89 00 00 00 00 = lea 0x0(%r9),%r9
> 
> 6) 0x?? 0x8d 0x8?
> Never saw not the 0x48 as prefix for that cases, so
> This is 0x48 0x8d 0x8? XX XX XX XX, lea 0xXXXXXXXX(%r??),%r??
> Ex: 48 8D 89 00 00 00 00 = lea 0x0(%rcx),%rcx
gold and bfd contain misleading comment about that special rsp handling. In fact that also touch r12 for them:

```
asm:
 addq tls0 at GOTTPOFF(%rip), %rsp
 addq tls0 at GOTTPOFF(%rip), %r8 
...
 addq tls0 at GOTTPOFF(%rip), %r12
...
 addq tls0 at GOTTPOFF(%rip), %r15 

00000000004000f0 <main>:
  400105:	48 81 c4 f8 ff ff ff 	add    $0xfffffffffffffff8,%rsp
  40010c:	4d 8d 80 f8 ff ff ff 	lea    -0x8(%r8),%r8
...
  400128:	49 81 c4 f8 ff ff ff 	add    $0xfffffffffffffff8,%r12
...
  40013d:	4d 8d bf f8 ff ff ff 	lea    -0x8(%r15),%r15
```

Looks like a bug of gold/bfd for me.
I rewrited the code to simplify it and removed special handling for rsp for now. I think we can add that if it will be needed later.

Updated possible emit table is:
2. 0x49 0xC7 0xC?
4. 0x?? 0xC7 0xC?
5. 0x4D 0x8D 0x8?
6. 0x?? 0x8D 0x8?

================
Comment at: ELF/Target.h:61
@@ -59,2 +60,3 @@
                            uint64_t P, uint64_t SA) const = 0;
-
+  virtual TlsOpt getTlsOptimization(unsigned Type, const SymbolBody &S) const;
+  virtual void relocateTlsToLe(uint8_t *Loc, uint8_t *BufEnd, uint32_t Type,
----------------
grimar wrote:
> ruiu wrote:
> > Currently it only returns None or ToLE, so can you change the signature of the function so that it returns a boolean value?
> Will do.
Done.


http://reviews.llvm.org/D14713