Page MenuHomePhabricator

Don't omit dynamic relocations for the GOT.
ClosedPublic

Authored by ed on Apr 3 2016, 3:47 AM.

Details

Summary

Where Clang's AArch64 backend seems to differ from the X86 backend is
that it tends to use the GOT more aggressively.

After getting CloudABI PIEs working on x86-64, I noticed that accessing
global variables would still crash on aarch64. Tracing it down, it turns
out that the GOT was filled with entries assuming the base address was
zero.

It turns out that we skip generating relocations for GOT entries in case
the relocation pointing towards the GOT is relative. This is probably
wrong. Whether the thing pointing to the GOT is absolute or relative
shouldn't make any difference; the GOT entry itself should contain the
absolute address, thus needs a relocation regardless.

Before turning this into a full patch containing tests, I'm first
sending this out for review, because again I don't really know what I'm
doing. Is my interpretation on this matter correct?

Diff Detail

Repository
rL LLVM

Event Timeline

ed updated this revision to Diff 52495.Apr 3 2016, 3:47 AM
ed retitled this revision from to Don't omit dynamic relocations for the GOT..
ed updated this object.
ruiu edited edge metadata.Apr 4 2016, 2:33 PM

This doesn't seem correct, but Rafael is updating this part of code. Rafael, can you take a look?

rafael accepted this revision.Apr 4 2016, 5:27 PM
rafael edited edge metadata.

The code does look wrong. It happens to work given the current implementations of isRelRelative. You should completely delete Dynrel, if we need a relocation for the got entry itself should only depend on the symbol and whether the output is relocatable (Config->Pic).

Do you have a testcase for which it fails it currently fails? If so, please add a testcase. If not, this is still a nice cleanup

This revision is now accepted and ready to land.Apr 4 2016, 5:27 PM
rafael edited edge metadata.Apr 4 2016, 5:27 PM
rafael added a subscriber: llvm-commits.

I accepted it, but phab is not sending emails.

ruiu added a comment.Apr 5 2016, 10:26 AM

Did you accept it although you are saying that this looks wrong?

ed updated this revision to Diff 52719.Apr 5 2016, 11:52 AM

Simplify, as suggested by Rafael. Also add a test.

ed added a comment.Apr 5 2016, 11:54 AM

The code does look wrong. It happens to work given the current implementations of isRelRelative. You should completely delete Dynrel, if we need a relocation for the got entry itself should only depend on the symbol and whether the output is relocatable (Config->Pic).

Done! Does this look all right?

Do you have a testcase for which it fails it currently fails? If so, please add a testcase. If not, this is still a nice cleanup

Yes! I've just written a test.

LGTM. Do you have commit access?

ed added a comment.Apr 5 2016, 1:23 PM

LGTM. Do you have commit access?

Yes! Committed as r265453.

This revision was automatically updated to reflect the committed changes.