Page MenuHomePhabricator

[ELF] - implemented @indntpoff (x86) relocation and its optimization.
ClosedPublic

Authored by grimar on Dec 14 2015, 7:32 AM.

Details

Summary

@indntpoff is similar to @gotntpoff, but for use in position dependent code. While @gotntpoff resolves to GOT slot address relative to the
start of the GOT in the movl or addl instructions, @indntpoff resolves to the
absolute GOT slot address. ("ELF Handling For Thread-Local Storage", Ulrich Drepper).

Since this relocation resolves to the absolute GOT slot address it requires R_386_RELATIVE relocation for each place of using when linking shared library. Had to add Target::relocNeedsDynRelative() method that checks if a relocation requires R_386_RELATIVE relocation to be emited either.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar updated this revision to Diff 42717.Dec 14 2015, 7:32 AM
grimar retitled this revision from to [ELF] - implemented @indntpoff (x86) relocation and its optimization..
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: grimar, llvm-commits.
grimar added inline comments.
ELF/OutputSections.cpp
281 ↗(On Diff #42717)

Changes above mostly belongs to dependency patch http://reviews.llvm.org/D15453.
The difference is that IsDynRelative was added to conditions.

ruiu added inline comments.Dec 14 2015, 12:45 PM
ELF/Target.cpp
106 ↗(On Diff #42717)

In other places we use unsigned for the symbol type.

351–354 ↗(On Diff #42717)

You can combine these two ifs using ||.

519–520 ↗(On Diff #42717)

Isn't the former a special case of the latter? (EAX is a REG?)

ELF/Target.h
48 ↗(On Diff #42717)

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.

silvas added a subscriber: silvas.Dec 14 2015, 4:22 PM
silvas added inline comments.
ELF/Target.cpp
519–520 ↗(On Diff #42717)

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.

ruiu added inline comments.Dec 14 2015, 4:32 PM
ELF/Target.cpp
519–520 ↗(On Diff #42717)

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) {

?

silvas added inline comments.Dec 14 2015, 4:53 PM
ELF/Target.cpp
519–520 ↗(On Diff #42717)

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.

grimar updated this revision to Diff 42828.Dec 15 2015, 2:38 AM
  • Rebased
  • Review comments addressed.
ELF/Target.cpp
106 ↗(On Diff #42717)

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
351–354 ↗(On Diff #42717)

Done.

519–520 ↗(On Diff #42717)

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 ?

ELF/Target.h
48 ↗(On Diff #42717)

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.

ruiu added inline comments.Dec 15 2015, 11:25 AM
ELF/OutputSections.cpp
225 ↗(On Diff #42828)

Oh this is worse than before. I didn't expect that you had to use a magic number instead of a default parameter. Default parameter is better.

grimar updated this revision to Diff 42970.Dec 16 2015, 1:23 AM

Review comments addressed.

ELF/OutputSections.cpp
225 ↗(On Diff #42828)

Yep. Reverted back.

ruiu accepted this revision.Dec 16 2015, 10:47 AM
ruiu edited edge metadata.

LGTM

This revision is now accepted and ready to land.Dec 16 2015, 10:47 AM
This revision was automatically updated to reflect the committed changes.