[PATCH] D15494: [ELF] - implemented @indntpoff (x86) relocation and its optimization.

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/Target.cpp:106
@@ -103,3 +105,3 @@
                          uint64_t SA) const;
-  void relocateTlsIeToLe(uint8_t *Loc, uint8_t *BufEnd, uint64_t P,
-                         uint64_t SA) const;
+  void relocateTlsIeToLe(uint32_t Type, uint8_t *Loc, uint8_t *BufEnd,
+                         uint64_t P, uint64_t SA) const;
----------------
ruiu wrote:
> In other places we use `unsigned` for the symbol type.
Changed to unsigned. Thats what we might want to change for all places I guess, since currently some methods uses uint32_t. others uses unsigned.
For example these ones uses uint32:

```
  virtual bool isRelRelative(uint32_t Type) const;
  virtual bool isSizeDynReloc(uint32_t Type, const SymbolBody &S) const;
  virtual bool relocNeedsCopy(uint32_t Type, const SymbolBody &S) const;
  virtual bool relocNeedsGot(uint32_t Type, const SymbolBody &S) const = 0;
  virtual bool relocNeedsPlt(uint32_t Type, const SymbolBody &S) const = 0
```

================
Comment at: ELF/Target.cpp:351-354
@@ -339,4 +350,6 @@
     return Target->isTlsOptimized(Type, &S) && canBePreempted(&S, true);
   if (Type == R_386_TLS_GOTIE)
     return !isTlsOptimized(Type, &S);
+  if (Type == R_386_TLS_IE)
+    return !isTlsOptimized(Type, &S);
   return Type == R_386_GOT32 || relocNeedsPlt(Type, S);
----------------
ruiu wrote:
> You can combine these two `if`s using ||.
Done.

================
Comment at: ELF/Target.cpp:519-520
@@ +518,4 @@
+    // For R_386_TLS_IE relocation we perform the next transformations:
+    // MOVL foo at INDNTPOFF,%EAX is transformed to MOVL $foo,%EAX
+    // MOVL foo at INDNTPOFF,%REG is transformed to MOVL $foo,%REG
+    // ADDL foo at INDNTPOFF,%REG is transformed to ADDL $foo,%REG
----------------
silvas wrote:
> ruiu wrote:
> > silvas wrote:
> > > ruiu wrote:
> > > > Isn't the former a special case of the latter? (EAX is a REG?)
> > > There is a special encoding for the EAX version, so from a linker's perspective (operations on machine code bytes) they are different. There are many x86 instructions for which eax is special (carry-over from old 8086 CISC stuff). Originally, ax = "accumulator" register and had a special "load accumulator" instruction which now is a special encoding pattern-matched by the assembler for assembly text `mov $foo,%eax`. Another example is use of cx "count" register as arg for variable bit shift.
> > But still it looks odd as a comment. Maybe we should add
> > 
> >   // 0xA1 is RAX. RAX encoding is different from others
> > 
> > to 
> > 
> >   if (*RegSlot == 0xA1) {
> > 
> > ?
> The comment does say "First one is special because when EAX is used the sequence is 5 bytes long, otherwise it is 6 bytes." 
> 
> Also, I think the proposed wording "0xA1 is RAX" is confusing. 0xA1 is simply the opcode byte for the instruction `MOV rAX, Ov` (see `Table A-2. One-byte Opcode Map: (00H — F7H)` in the intel manual Vol. 2).
> 
> Note that `movl $foo,%eax` actually can be encoded two ways; in one case, `*RegSlot` is just the opcode byte (see entry for 0xA1 in Table A-2), in the other, it is an opcode extension byte (see entry 0xC7 in Table A-2 and also row C7 (in group 11) column 000 in `Table A-6. Opcode Extensions for One- and Two-byte Opcodes by Group Number`). Perhaps `RegSlot` needs a different name to reflect this situation.
So movl $0x0,%eax can be encoded as
0xc7 0xc0 0x00 0x00 0x00 0x00
and
0xb8 0x00 0x00 0x00 0x00

We convert here from 0xA1 0x00 0x00 0x00 0x00 to 0xB8 0x00 0x00 0x00 0x00 (MOV eAX,Ov -> MOV eAX, see A.1.1, A.1.2, Table A-2).
So for us that A1 is always oppcode byte here it seems. Then I would call it OpCode but it used for other branches as well where has different meaning. Thus may be "Op" name would be fine here, combining both "Opcode" and "Operands" interpretations ?

================
Comment at: ELF/Target.h:48
@@ -46,1 +47,3 @@
   virtual unsigned getPltRefReloc(unsigned Type) const;
+  virtual unsigned getTlsGotReloc(unsigned Type = -1) const {
+    return TlsGotReloc;
----------------
ruiu wrote:
> Can you make the argument mandatory? Technically nothing wrong with the optional parameter, but that defines actually a shorthand for defining two different functions, getTlsGotReloc() and getTlsGotReloc(unsigned), thus it (slightly) increase program complexity.
Done.
Problem here is that getTlsGotReloc is used in several places but requires special handling of Type only in one of them (where relocation calculation actually happens, for other places method returns dynamic relocation which is single). 
That makes need of explicit using of arg = -1, what does not looks great for me.


http://reviews.llvm.org/D15494