This is an archive of the discontinued LLVM Phabricator instance.

Use relocations to fill statically known got entries
ClosedPublic

Authored by rafael on Nov 27 2016, 11:53 PM.

Details

Reviewers
ruiu
peter.smith
Summary

Right now we just remember a SymbolBody for each got entry and duplicate a bit of logic to decide what value, if any, should be written for that SymbolBody.

With ARM there will be more complicated values, and it seems better to just use the relocation code to fill the got entries. This makes it clear that each entry is filled by the dynamic linker or by the static linker.

Diff Detail

Event Timeline

rafael retitled this revision from to Use relocations to fill statically known got entries.
rafael updated this object.
rafael added reviewers: peter.smith, rui314.
rafael added a subscriber: llvm-commits.
peter.smith edited edge metadata.Nov 28 2016, 5:55 AM

I think the approach looks good.

One possible problem is that we have missed out on the possibilities that other architectures will need these relocations. For example in aarch64 it is possible to generate got slot generating relocations that would be resolved statically in some cases:

extern int foo __attribute__((weak));

int _start(void)
{
    return foo;
}

clang --target=aarch64-linux-gnu -c foo.c
ld.lld foo.o -o foo.axf
bin/ld.lld: error: unrecognized reloc 1025

I think it could be prudent to add the R*_GLOB_DAT to the other targets as well.

I'll merge in my downstream changes for static TLS in ARM and run it past some additional tests, will let you know if I find anything.

atanasyan added a subscriber: atanasyan.
atanasyan resigned from this revision.Nov 28 2016, 6:16 AM
atanasyan removed a reviewer: atanasyan.
ruiu edited reviewers, added: ruiu; removed: rui314.Nov 28 2016, 6:32 AM
emaste added inline comments.Nov 28 2016, 6:37 AM
lld/ELF/Target.cpp
1700–1701

these were previously in alpha(ish) order

peter.smith added inline comments.Nov 28 2016, 6:39 AM
lld/ELF/Target.cpp
1699

For ARM (and Mips if it were to use this mechanism) I think we are missing the Target::moduleIndexRel R_ARM_TLS_DTPMOD32 (Always resolves to 1 for an executable). This is currently not showing up in the tests as for ARM as the current implementation forces TLS to use dynamic relocations, but will need to be fixed for static linking. The handleNoRelaxTlsRelocation will most likely need to generate this. I think this could be added in a later patch though.

ruiu accepted this revision.Nov 28 2016, 7:21 AM
ruiu edited edge metadata.

LGTM with all comments addressed.

lld/ELF/SyntheticSections.cpp
405

Thank you for doing this. push_back'ing two nullptrs looked odd.

432

nit: you can just inline this.

lld/ELF/Target.cpp
805

sort

This revision is now accepted and ready to land.Nov 28 2016, 7:21 AM
espindola closed this revision.Mar 14 2018, 4:37 PM
espindola added a subscriber: espindola.

288107