Page MenuHomePhabricator

Set up basic infrastructure for ELF/i386 backend support in JITLink .
ClosedPublic

Authored by jain98 on Aug 6 2022, 7:21 PM.

Details

Summary

Changes in this CR enable jit-linking a minimal ELF 32-bit object file. No relocations are supported yet.

Diff Detail

Event Timeline

jain98 created this revision.Aug 6 2022, 7:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2022, 7:21 PM
jain98 requested review of this revision.Aug 6 2022, 7:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2022, 7:21 PM
jain98 updated this revision to Diff 450685.Aug 7 2022, 7:30 PM

Runs clang-format

jain98 edited the summary of this revision. (Show Details)Aug 7 2022, 7:32 PM

Thanks very much for working on this Kshitij -- this looks like a great start to the i386 backend!

I have a couple of minor comments -- could you address them and update the review? Once that's done I think that this is ready to land. :)

llvm/lib/ExecutionEngine/JITLink/i386.cpp
2–10

Comments should be updated to i386.

llvm/test/ExecutionEngine/JITLink/i386/ELF_i386_minimal.s
4

Tests should be -noexec (except in special circumstances): We want them to be able to run on architectures other than the target.

I would simplify to:

# RUN: llvm-mc -triple=i386-unknown-linux-gnu -position-independent -filetype=obj -o %t.o %s
# RUN: llvm-jitlink -noexec %t.o
sunho added a subscriber: sunho.Aug 11 2022, 9:29 PM

Thanks for the patch! It's really exciting to see i386 backend coming up.

llvm/include/llvm/ExecutionEngine/JITLink/i386.h
27

The enum name here is better to be non platform-specific like in x86_64.h and aarch64.h. The edges are going to be reused across other platforms too. (e.g. COFF/i386)

llvm/lib/ExecutionEngine/JITLink/ELF_i386.cpp
50

If you were not aware of it, there is isInt<> function in support library.

jain98 marked 4 inline comments as done.Aug 13 2022, 10:56 AM
jain98 added inline comments.
llvm/include/llvm/ExecutionEngine/JITLink/i386.h
27

Makes sense. Changing it to NONE.

llvm/lib/ExecutionEngine/JITLink/ELF_i386.cpp
50

This is actually a remnant from the starter code that I used from the introductory aarch64 jitlink commit.

Removing the function entirely.

llvm/test/ExecutionEngine/JITLink/i386/ELF_i386_minimal.s
4

Does this also mean that the contents of this directory's lit.local.cfg should be changed? Currently this is what's in there -

if not 'i386' in config.root.targets:
  config.unsupported = True

I may be wrong, but I think it's saying run this test only if the current host architecture is i386?

4

Additionally, what the reasoning behind making these tests --noexec - it looks like most JitLink tests are? I'm wondering what makes the code in these tests non-runnable if we're able to load it into memory? I can see how the generated code for one architecture might be straight up rubbish for some other architecture, and therefore it does not make sense to actually try and execute the generated code. But then why go to the point where we're loading things into memory even? Why not stop after generating the LinkGraph?

jain98 updated this revision to Diff 452439.Aug 13 2022, 11:00 AM
jain98 marked 3 inline comments as done.

Addressed comments on revision 2

tschuett added inline comments.
llvm/include/llvm/ExecutionEngine/JITLink/i386.h
21

The codebase switched to C++17. You are allowed to use nested namespaces.

lhames accepted this revision.Aug 13 2022, 9:27 PM

@tschuett's suggestion is a good one.

Otherwise LGTM. Just let me know what name and email you would like me to use for attribution and I can land this tomorrow. :)

llvm/test/ExecutionEngine/JITLink/i386/ELF_i386_minimal.s
4

I may be wrong, but I think it's saying run this test only if the current host architecture is i386?

That check is saying "if this LLVM build isn't configured with i386 backend support". You want to keep it -- it tells us whether or not the tools can handle i386 assembly. :)

4

I can see how the generated code for one architecture might be straight up rubbish for some other architecture, and therefore it does not make sense to actually try and execute the generated code.

Yep -- we want to run this on all architectures, and that means not executing it when target arch != host arch. We just go one step further and never execute it at all.

But then why go to the point where we're loading things into memory even? Why not stop after generating the LinkGraph?

We want to check that all relocations have been applied correctly. Originally this testing system was designed for RuntimeDyld, which had no exposed data structure that we could evaluate, hence the decision to inspect final linked memory instead. Now that we have JITLink we could inspect the graph, but the advantage of checking the final linked memory is that it verifies that linking worked end-to-end, including layout in target memory.

It wouldn't hurt to rethink all this test infrastructure. It was written in a hurry back in 2015 and barely touched since. That said it has been remarkably effective -- a cleanup and new functionality would be welcome (LinkGraph inspection? Maybe a more user-friendly expression language and robust parser), but we'd definitely want to keep some variation of the end-to-end scheme that we have.

This revision is now accepted and ready to land.Aug 13 2022, 9:27 PM

@tschuett's suggestion is a good one.

I think so too. However, I was thinking maybe we should make that change separately and perhaps group it with similar namespace changes across other JitLink files? I'm happy to send out a code review for the same.

Otherwise LGTM. Just let me know what name and email you would like me to use for attribution and I can land this tomorrow. :)

Name: Kshitij Jain
Email: jkshtj@outlook.com

Thanks!

jain98 added inline comments.Aug 14 2022, 10:21 AM
llvm/test/ExecutionEngine/JITLink/i386/ELF_i386_minimal.s
4

But then why go to the point where we're loading things into memory even? Why not stop after generating the LinkGraph?

We want to check that all relocations have been applied correctly. Originally this testing system was designed for RuntimeDyld, which had no exposed data structure that we could evaluate, hence the decision to inspect final linked memory instead. Now that we have JITLink we could inspect the graph, but the advantage of checking the final linked memory is that it verifies that linking worked end-to-end, including layout in target memory.

Perhaps this is my lack of understanding/knowledge, but are we truly checking whether the relocations have been applied correctly? I'm thinking of a very simple Pointer32 type relocation for a non-static global var. Is it possible that we calculate the address of the variable to be relocated incorrectly and since we never run the program we never run into any issues?

I'm guessing perhaps the more concrete issues, such as, invalid memory access are checked for, but can the program be linked and still be logically incorrect?

jain98 marked 2 inline comments as done.Aug 14 2022, 10:22 AM

I ran out of time to test this today, but should be able to commit early tomorrow.

llvm/test/ExecutionEngine/JITLink/i386/ELF_i386_minimal.s
4

Is it possible that we calculate the address of the variable to be relocated incorrectly and since we never run the program we never run into any issues?

It is, but the address plumbing is shared infrastructure used by all backends and implicitly tested by the execution tests elsewhere in llvm/test/ExecutionEngine, so we don't need to re-test it in the JITLink backend tests.

This revision was landed with ongoing or failed builds.Aug 15 2022, 6:35 PM
This revision was automatically updated to reflect the committed changes.
chapuni added inline comments.
llvm/lib/ExecutionEngine/JITLink/ELF_i386.cpp
66

Unused?

jain98 added inline comments.Aug 16 2022, 6:32 AM
llvm/lib/ExecutionEngine/JITLink/ELF_i386.cpp
66

Yes. It looks like it's been fixed

66