This is an archive of the discontinued LLVM Phabricator instance.

[RuntimeDyld] Don't allocate unnecessary stub buffer space
ClosedPublic

Authored by sanjoy on Nov 13 2015, 7:04 PM.

Details

Summary

For relocation types that are known to not require stub functions, there
is no need to allocate extra space for the stub functions.

Diff Detail

Event Timeline

sanjoy updated this revision to Diff 40199.Nov 13 2015, 7:04 PM
sanjoy retitled this revision from to [RuntimeDyld] Don't allocate unnecessary stub buffer space.
sanjoy updated this object.
sanjoy added reviewers: lhames, reames, maksfb.
sanjoy added a subscriber: llvm-commits.
maksfb edited edge metadata.Nov 15 2015, 11:23 AM

Sanjoy, I'm not very familiar with all scenarios where stubs are used. Is it possible R_X86_64_PC32 relocation will need a stub?

Sanjoy, I'm not very familiar with all scenarios where stubs are used. Is it possible R_X86_64_PC32 relocation will need a stub?

I'm just going by reading the code (I'm not familiar with linkers in
general), but the relevant clause in
RuntimeDyldELF::processRelocationRef seems to be

} else if (RelType == ELF::R_X86_64_PC32) {
  Value.Addend += support::ulittle32_t::ref(computePlaceholderAddress(SectionID, Offset));
  processSimpleRelocation(SectionID, Offset, RelType, Value);

and it does not seem to require stub space. In fact, from reading the
code, pretty much the only relocation for x86_64/ELF that needs stub
space is ELF::R_X86_64_PLT32.

In any case, if I'm wrong, D14674 and D14675 should make it obvious an
obvious crash.

lhames edited edge metadata.Nov 21 2015, 2:34 AM

Hi Sanjoy,

Sorry for the delay in reviewing this - I'm still out on vacation for another week.

I think relocationNeedsStub should be a virtual function so that we don't need to check relocations for other formats/archs, but other than that this looks good to me. :)

Cheers,
Lang.

lhames accepted this revision.Nov 21 2015, 2:35 AM
lhames edited edge metadata.
This revision is now accepted and ready to land.Nov 21 2015, 2:35 AM
This revision was automatically updated to reflect the committed changes.

Sorry, for some reason phabricator emails escaped my inbox. The diff looks great!