This is an archive of the discontinued LLVM Phabricator instance.

Adding support for PCRel32GOTLoad in ELF x86 for the jitlinker
ClosedPublic

Authored by jaredwy on Jul 14 2020, 12:06 AM.

Details

Summary

This adds the basic support for GOT in elf x86. Was able to just get away using the macho code. So there will be a follow up patch to turn that into a generic utility for both of the x86

Diff Detail

Event Timeline

jaredwy created this revision.Jul 14 2020, 12:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2020, 12:06 AM
jaredwy updated this revision to Diff 278072.Jul 14 2020, 10:49 PM
jaredwy marked an inline comment as done.

This includes support for section relative relocations as well as responding to the clang-tidy issues

lhames accepted this revision.Jul 14 2020, 11:00 PM

LGTM -- Thanks very much Jared!

This revision is now accepted and ready to land.Jul 14 2020, 11:00 PM
MaskRay added inline comments.
llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp
489

LLVM coding style uses ++SymbolIndex

637

llvm/Support/Endian.h llvm::support::endian::write64le

MaskRay added inline comments.Jul 14 2020, 11:07 PM
llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp
35

PCRel32GOT & PCRel32GOTLoad seem to match the names of Mach-O X86_64_RELOC_GOT & X86_64_RELOC_GOT_LOAD. The ELF relocation types are named R_X86_64_GOTPCREL & R_X86_64_GOTPCRELX

jaredwy marked 3 inline comments as done.Jul 15 2020, 12:07 AM
jaredwy added inline comments.
llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp
35

This was done on purpose, this code is lifted verbatim from the Mach-O side. By normalising the edges of the graph it allows us to write more generic code.

The next commit after this, will merge all this to a base class that both Mach-O and elf can use.

208

Not sure what this got left in or not flagged as unused :)

489

Thanks. Fixed up.

637

Thanks, That actually can remove the comment above as well.

jaredwy updated this revision to Diff 278082.Jul 15 2020, 12:09 AM

Using write64le instead of a cast. Thanks @MaskRay
and fixing up a style issue

jaredwy marked an inline comment as not done.Jul 15 2020, 12:10 AM
lhames added inline comments.Jul 15 2020, 9:58 AM
llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp
35

Yep. The long term plan for JITLink is to make the graph edge kinds per-architecture rather than per-(format,architecture), so I think these names are fine for now.

MaskRay added inline comments.Jul 15 2020, 10:08 AM
llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp
408
lhames added inline comments.Jul 15 2020, 12:12 PM
llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp
408

I'm comfortable with auto being used where the variable name clearly implies valid usage, in this case as an index.

I understand there's room for differences in opinion there, but I don't want to delay this landing: We want to get it into LLVM 11 so that people can start testing/living on it.

jaredwy marked an inline comment as done.Jul 15 2020, 1:15 PM
jaredwy added inline comments.
llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp
408

I share langs view, to me that the name is index the type almost doesn't matter, but understand there could be a difference of opinion there.

There is also the consideration that this just supports x86 at the moment, Given that the we want to follow up with 32 bit support and this class is going to become very templated to match the ElfFile<T> so keeping it auto will help with reducing code churn.

Are you ok with landing it as is?

lhames added inline comments.Jul 15 2020, 1:21 PM
llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp
408

Are you ok with landing it as is?

Yep. Please go ahead and commit.

We can address style issues in follow up patches. The focus for now should be on getting as much ELF functionality as we reasonably can into the LLVM 11 release branch so that potential clients can start testing and providing feedback.

This revision was automatically updated to reflect the committed changes.