This is an archive of the discontinued LLVM Phabricator instance.

[ELF][MIPS] Handle R_MIPS_HI16/LO16 relocations against _gp_disp symbol
ClosedPublic

Authored by atanasyan on Dec 12 2015, 10:54 PM.

Details

Summary

The _gp_disp is a magic symbol designates offset between start of function and gp pointer into GOT. Only R_MIPS_HI16 and R_MIPS_LO16 relocations are permitted with _gp_disp. The patch adds the _gp_disp as an ignored symbol and adjusts symbol value before call the relocateOne for R_MIPS_HI16/LO16 relocations.

I put code which adjust symbol value in case of R_MIPS_HI16/LO16 relocations into the InputSectionBase<ELFT>::relocate routine to escape passing SymboBody to each relocateOne methods while it is necessary for MIPS target only.

It is possible to optimize the current solution a bit. We can save a reference to the SymbolBody instance created by the addIgnoredSym call and replace "_gp_disp" string comparison by comparison of SymbolBody references. But I cannot find a good place to keep the reference to the "_gp_disp" SymbolBody.

Diff Detail

Repository
rL LLVM

Event Timeline

atanasyan updated this revision to Diff 42650.Dec 12 2015, 10:54 PM
atanasyan retitled this revision from to [ELF][MIPS] Handle R_MIPS_HI16/LO16 relocations against _gp_disp symbol.
atanasyan updated this object.
atanasyan added reviewers: ruiu, rafael.
atanasyan set the repository for this revision to rL LLVM.
atanasyan added a project: lld.
atanasyan added a subscriber: llvm-commits.
ruiu added inline comments.Dec 14 2015, 1:02 PM
ELF/InputSection.cpp
209–212 ↗(On Diff #42650)

How about creating a new field, MipsGpDisp of type SymbolBody*, to InputSection class as a public static member to avoid string comparison?

Or, you can modify SymbolTable::addIgnoredSym to return a symbol body which is just added, and assign it to Config or somewhere. And then compare that value's repl() with Body here.

I don't know which is better, or no strong preference.

atanasyan updated this revision to Diff 42899.Dec 15 2015, 1:31 PM

Save SymbolBody for the _gp_disp symbol into the Config::MipsGpDisp field and use the saved pointer instead of string "_gp_disp" comparison.

ruiu accepted this revision.Dec 15 2015, 1:34 PM
ruiu edited edge metadata.

LGTM

This revision is now accepted and ready to land.Dec 15 2015, 1:34 PM
This revision was automatically updated to reflect the committed changes.
grimar added a subscriber: grimar.Dec 24 2015, 3:04 AM
grimar added inline comments.
lld/trunk/ELF/InputSection.cpp
213

I thinking about that place last time.
May be we should add A argument to Target->relocateOne() ?
That can help to move that calculation to mips target, since it looks like absence of A inside relocateOne() is the only stoppable for doing that.

I thinking about that place last time.
May be we should add A argument to Target->relocateOne() ?
That can help to move that calculation to mips target, since it looks like absence of A inside relocateOne() is the only stoppable for doing that.

Currently S+A is a single parameter SA. Are you suggesting split that as two parameters, S and A?

I don't know if that makes code simpler, but it may worth trying.

Not sure about splitting. That seems to be more complicated. Just adding A can save from complicating logic outside allowing relocateOne to use it if it is needed for concrete relocation`s code like here for mips.


llvm-commits mailing list
llvm-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits