This is an archive of the discontinued LLVM Phabricator instance.

[RuntimeDyldELF] Improve GOT support
ClosedPublic

Authored by loladiro on Apr 2 2015, 6:44 PM.

Details

Summary

This is the first in a series of patches to eventually add support for TLS relocations to RuntimeDyld. This patch resolves an issue in the current GOT handling, where GOT entries would be reused between object files, which leads to the same situation that necessitates the GOT in the first place, i.e. that the 32-bit offset can not cover all of the address space. Thus this patch makes the GOT object-file-local.
Unfortunately, this still isn't quite enough, because the MemoryManager does not yet guarantee that sections are allocated sufficiently close to each other, even if they belong to the same object file. To address this concern, this patch also adds a small API abstraction on top of the GOT allocation mechanism that will allow (temporarily, until the MemoryManager is improved) using the stub mechanism instead of allocating a different section. The actual switch from separate section to stub mechanism will be part of a follow-on commit, so that it can be easily reverted independently at the appropriate time.

Diff Detail

Repository
rL LLVM

Event Timeline

loladiro updated this revision to Diff 23195.Apr 2 2015, 6:44 PM
loladiro retitled this revision from to [RuntimeDyldELF] Improve GOT support.
loladiro updated this object.
loladiro edited the test plan for this revision. (Show Details)
loladiro added a reviewer: lhames.
loladiro set the repository for this revision to rL LLVM.
loladiro added a subscriber: Unknown Object (MLST).
lhames edited edge metadata.Apr 6 2015, 12:34 PM

Hi Keno,

This sounds great. Sorry for the delay in review - I'm hoping to get to
them very soon (i.e. in a day or two).

Cheers,
Lang.

lhames added inline comments.Apr 6 2015, 12:41 PM
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
255–258 ↗(On Diff #23195)

Would it be better to zero out the GOT entry when it's reserved so that you don't need to special-case the placeholder logic here?

270–273 ↗(On Diff #23195)

Ditto here.

loladiro added inline comments.Apr 6 2015, 12:45 PM
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
255–258 ↗(On Diff #23195)

No, that doesn't work because the placeholder pull values out of the original (unrelocated) object file, which does not have the stub. The better direction to go is to encode everything that's needed into the Addend, but that's a separate and more invasive change.

lhames accepted this revision.Apr 6 2015, 12:51 PM
lhames edited edge metadata.
lhames added inline comments.
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
255–258 ↗(On Diff #23195)

Ahh, of course. This is another good argument for getting rid of our reliance on the original file.

Given this, your existing scheme looks good to me.

This revision is now accepted and ready to land.Apr 6 2015, 12:51 PM
This revision was automatically updated to reflect the committed changes.