Page MenuHomePhabricator

[ELF] - R_386_GOTOFF relocation implemented.
ClosedPublic

Authored by grimar on Dec 9 2015, 9:43 AM.

Details

Summary

R_386_GOTOFF is calculated as S + A - GOT, where:
S - Represents the value of the symbol whose index resides in the relocation entry.
A - Represents the addend used to compute the value of the relocatable field.
GOT - Represents the address of the global offset table.

This relocation needs got address.
Problem here was that got table was not created if had no entries. Had to change how GOT gets to the output, test covers that case.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar updated this revision to Diff 42311.Dec 9 2015, 9:43 AM
grimar retitled this revision from to [ELF] - R_386_GOTOFF relocation implemented..
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar.
ruiu added inline comments.Dec 9 2015, 10:01 AM
ELF/OutputSections.h
140 ↗(On Diff #42311)

Maybe we should have a separate global variable, HasGotoffRel, instead of this field.

ELF/Target.cpp
328–334 ↗(On Diff #42311)

I don't think this is the right place to write this code. This function is doing more than its name implies.

grimar updated this revision to Diff 42397.Dec 10 2015, 12:59 AM

Review comments addressed.

ELF/OutputSections.h
140 ↗(On Diff #42311)

I think HasGotOffRel is too specific, at least it can be used for mips as shown in this patch:

Out<ELFT>::Got->Demanded |=
    (isOutputDynamic() && Config->EMachine == EM_MIPS);

I can make it separate static but what the point for it ? It only should be used to keep got section now, but when separate static it looks like it can be used for something else, what is not true.

ELF/Target.cpp
328–334 ↗(On Diff #42311)

Yes, I didn`t like that place too, but then only suitable place for me would be adding Target->relocUsesGotAddr() and call it during iterations over relocations at upper level.
That adds one more method to Target thats only useful only for x86 now. Made that change.

ruiu added inline comments.Dec 10 2015, 3:01 PM
ELF/OutputSections.h
140 ↗(On Diff #42397)

Well, I think that is rather too specific. You set Demanded field in Writer.cpp, but that boolean value is consumed immediately in the following line in an obscure way. (You explicitly wrote to the boolean variable but read from it through leaveInOutput() function.)

Can we write like this in Writer.cpp?

bool needsGot = (isOutputDynamic() && Config->EMachine == EM_MIPS);

// If we have a relocation that is relative to GOT (such as GOTOFFREL),
// we need to emit a GOT even if it's empty.
if (HasGotoffRel)
  needsGot = true;

if (needsGotMips || !Out<ELFT>::Got->empty()) {
  ....
}
ELF/Target.h
62 ↗(On Diff #42397)

I'd name relocNeedsGot.

grimar updated this revision to Diff 42511.Dec 11 2015, 1:36 AM

Review comments addressed.

ELF/OutputSections.h
140 ↗(On Diff #42397)

Done.
(I actually don`t like idea of global variable at all. I would prefer static member of GotSection then. But if you insist..)

ELF/Target.h
62 ↗(On Diff #42397)

I`d too, but unfortunately we already have one bool relocNeedsGot(...) above :)
Which is used to determine if relocation needs got entry to be allocated. So I dont think you want to move functionality from relocUsesGotAddr there.
May be relocAddressesGot() ?

grimar updated this revision to Diff 43111.Dec 17 2015, 1:57 AM
  • Rebased.
ruiu accepted this revision.Dec 19 2015, 12:01 AM
ruiu edited edge metadata.

LGTM with a few nits.

ELF/OutputSections.cpp
24–28 ↗(On Diff #43111)

bool lld::elf2::HasGotOffRel = false;

ELF/OutputSections.h
39 ↗(On Diff #43111)

got -> GOT

ELF/Target.cpp
364 ↗(On Diff #43111)

I'd name this isGotRelative.

ELF/Writer.cpp
771–779 ↗(On Diff #43111)

I think this flow is slightly better in readability (because special cases are now in ifs.)

bool needsGot = !Out<ELFT>::Got->empty();
// We add the .got section to the result for dynamic MIPS target because
// its address and properties are mentioned in the .dynamic section.
if (Config->EMachine == EM_MIPS)
  needsGot = needsGot || isOutputDynamic();
// If we have a relocation that is relative to GOT (such as GOTOFFREL),
// we need to emit a GOT even if it's empty.
if (HasGotOffRel)
  needsGot = true;
This revision is now accepted and ready to land.Dec 19 2015, 12:01 AM
This revision was automatically updated to reflect the committed changes.
grimar added inline comments.Dec 21 2015, 2:06 AM
ELF/Writer.cpp
771–779 ↗(On Diff #43111)

Ok.
The only distinction - I used |= :

if (Config->EMachine == EM_MIPS)
  needsGot |= isOutputDynamic();
grimar added inline comments.Dec 26 2015, 1:41 PM
lld/trunk/ELF/Writer.cpp
214

btw, what about making this more generic ?

  1. Change HasGotOffRel to be not a global variable but a public member of target.
  2. remove isGotRelative(Type), create visitRelocation(Type) or something alike instead. Which purpose would be to review relocation and set flags like HasGotOffRel inside.