This is an archive of the discontinued LLVM Phabricator instance.

Handle relocations to thumb functions when dynamic linking COFF modules
ClosedPublic

Authored by wallace on Sep 30 2016, 6:17 PM.

Details

Summary

This adds the necessary logic to support relocations to thumb functions in the COFF dynamic linker.
The jumps to function addresses are mostly blx, which requires the ISA selection bit when jumping to a thumb function.

Note: I'm determining if the relocation requires the ISA bit when creating the relocation entries and not when resolving the relocation. I have to do that because I need the ObjectFile and the actual Symbol, which are available only when creating the entries. It would require a gross refactor if I do it otherwise, but I'm okay with doing it if you think it's better.

Event Timeline

wallace updated this revision to Diff 73183.Sep 30 2016, 6:17 PM
wallace retitled this revision from to Handle thumb relocations when dynamic linking COFF modules.
wallace updated this object.
wallace added a reviewer: compnerd.
wallace set the repository for this revision to rL LLVM.
wallace added a subscriber: sas.

Hi,

Maybe if you added a bit more comments on the code, a better commit message (and some tests :), it'd be easier to understand what you're aiming for. :)

cheers,
--renato

ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFThumb.h
40–41 ↗(On Diff #73183)

Is this the only Thumb/COFF relocation?

114 ↗(On Diff #73183)

You already do the exact same test inside isThumbRelocation, so it's redundant here.

193 ↗(On Diff #73183)

I'm not sure what this does...

269 ↗(On Diff #73183)

Nor this...

compnerd requested changes to this revision.Sep 30 2016, 10:10 PM
compnerd edited edge metadata.

This absolutely needs a test, right now its somewhat challenging to follow. I agree with @rengolin that the commit message isn't particularly helpful either.

ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFThumb.h
39 ↗(On Diff #73183)

Unnecessary braces.

What other type is there?

40–41 ↗(On Diff #73183)

Yes, the way that COFF identifies thumb is via the IMAGE_SCN_MEM_16BIT characteristic.

269 ↗(On Diff #73183)

This is a horrible way to handle this. The fact that RE.IsThumbRel is boolean, and therefore gets converted into an implicit 1 or 0 which happens to map to the appropriate ISA selection bit. Can we handle this inside of the Relocation handling perhaps?

This revision now requires changes to proceed.Sep 30 2016, 10:10 PM
peter.smith edited edge metadata.Oct 3 2016, 5:54 AM

Not much to add over what has already been said. From an ELF speaker's perspective it looks like you should only identify a symbol as Thumb if it is a function, but I don't know COFF well enough to be sure.

ExecutionEngine/RuntimeDyld/RuntimeDyldImpl.h
153 ↗(On Diff #73183)

Does Rel mean relative to a thumb symbol or relocation in this context? Looking at the code I think you mean relative. Could the name make this clearer.

My first thought was that IsThumbRel meant this is a Thumb Relocation, not this is a relocation to a Thumb function symbol.

ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFThumb.h
36 ↗(On Diff #73183)

Does a data symbol in a section marked as Thumb code always indicate a Thumb address? For example could there be a relocation to literal data in such a section.

I mention it as in ELF bit 0 is set only if the target symbol has type STT_FUNC which I'm guessing is ST_Function in COFF.

wallace updated this revision to Diff 73337.EditedOct 3 2016, 1:53 PM
wallace edited edge metadata.

Improved the code, added comments, improved the diff description, but test is still missing

wallace retitled this revision from Handle thumb relocations when dynamic linking COFF modules to Handle relocations to thumb functions when dynamic linking COFF modules.Oct 3 2016, 2:04 PM
wallace updated this object.
wallace edited edge metadata.
compnerd added inline comments.Oct 3 2016, 2:06 PM
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldImpl.h
153

I think that we care more if the target is a thumb function. IsTargetThumbFunc would be more descriptive, and happens to mirror the .thumb_func directive for the GNU assembler syntax.

lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFThumb.h
25

IsThumbFunc is shorter but just as descriptive IMO.

wallace added inline comments.Oct 3 2016, 2:09 PM
lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFThumb.h
25

will do, thanks!

wallace updated this revision to Diff 73341.Oct 3 2016, 2:12 PM

shortened long function names

wallace updated this revision to Diff 73365.Oct 3 2016, 3:52 PM

added a test for the mov32t relocation

wallace updated this revision to Diff 73373.Oct 3 2016, 4:52 PM

finished adding tests

wallace updated this revision to Diff 73374.Oct 3 2016, 4:57 PM

polished the test

wallace marked 2 inline comments as done.Oct 7 2016, 1:02 PM

ping

I've got one comment about the #TODO in the test. I don't know much about COFF and I'm not a contributor to this area so I think you'll need to get approval from someone like compnerd.

test/ExecutionEngine/RuntimeDyld/ARM/COFF_Thumb.s
129

What is the reason for the # TODO? What will it take for them to be done?

compnerd accepted this revision.Oct 10 2016, 8:24 PM
compnerd edited edge metadata.
compnerd added inline comments.
test/ExecutionEngine/RuntimeDyld/ARM/COFF_Thumb.s
124

Would be nice to give this a proper label. Eventually, it would be nice to be able to run this test against MCJIT.

129

Theres a single relocation that is emitted as per the PE/COFF specification. We need to be able to index through the relocation to process that component of the address.

142

Again, a label for this would be nice.

This revision is now accepted and ready to land.Oct 10 2016, 8:24 PM
wallace updated this revision to Diff 74285.Oct 11 2016, 1:00 PM
wallace edited edge metadata.

add labels, but I'm pretty sure you won't like them
I have literary problems...

This revision was automatically updated to reflect the committed changes.