This is an archive of the discontinued LLVM Phabricator instance.

Adds absolute and pc relative relocation support for ELF/i386
ClosedPublic

Authored by jain98 on Oct 8 2022, 6:56 PM.

Details

Summary

This commit adds support for 32 bit absolute and pc relative relocations in ELF/i386 objects, along with simple unit tests.

Diff Detail

Event Timeline

jain98 created this revision.Oct 8 2022, 6:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 8 2022, 6:56 PM
jain98 requested review of this revision.Oct 8 2022, 6:56 PM
jain98 edited the summary of this revision. (Show Details)Oct 8 2022, 6:59 PM
jain98 added reviewers: lhames, sunho, sgraenitz.
jain98 updated this revision to Diff 466327.Oct 8 2022, 7:03 PM

Adds absolute and pc relative relocation support for ELF/i386

sgraenitz accepted this revision.Oct 9 2022, 11:15 PM

Thanks for working on this! Apart from the naming detail, the changes in ELFLinkGraphBuilder.h are looking good to me.

llvm/lib/ExecutionEngine/JITLink/ELFLinkGraphBuilder.h
517

Nit: We should keep typename Function here as well as for forEachRelRelocation()

This revision is now accepted and ready to land.Oct 9 2022, 11:15 PM

There are missing newlines at end of files.

jain98 added inline comments.Oct 15 2022, 7:22 PM
llvm/lib/ExecutionEngine/JITLink/ELFLinkGraphBuilder.h
517

I can change it back to "RelocHandlerFunction". I purposely changed it to "RelocHandlerMethod" because "Func" argument was a member function (which at least in Rust are just referred to as methods. Perhaps they're not called the same thing in C++). Is there a reason why you named the template parameter "RelocHandlerFunction"?

sgraenitz added inline comments.Oct 17 2022, 2:04 AM
llvm/lib/ExecutionEngine/JITLink/ELFLinkGraphBuilder.h
517

Yes, they are not the same thing. Methods can be subject to dynamic dispatch. Just to clarify, there are two overloads and the typenames are chosen on purpose:
(1) forEachRelocation<RelocHandlerFunction> takes a function (static member or freestanding function)
(2) forEachRelocation<ClassT, RelocHandlerMethod> takes an object and a method (regular member function)

The one we talk about here is (1) and I think it should remain unchanged. forEachRelRelocation() should follow this pattern.

lhames accepted this revision.Oct 30 2022, 4:17 PM

LGTM too. :)

This revision was landed with ongoing or failed builds.Oct 30 2022, 5:35 PM
This revision was automatically updated to reflect the committed changes.