Changes in this CR enable jit-linking a minimal ELF 32-bit object file. No relocations are supported yet.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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 |
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. |
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? |
llvm/include/llvm/ExecutionEngine/JITLink/i386.h | ||
---|---|---|
21 | The codebase switched to C++17. You are allowed to use nested namespaces. |
@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 |
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 |
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.
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. |
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!
llvm/test/ExecutionEngine/JITLink/i386/ELF_i386_minimal.s | ||
---|---|---|
4 |
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? |
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 |
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. |
llvm/lib/ExecutionEngine/JITLink/ELF_i386.cpp | ||
---|---|---|
66 | Unused? |
llvm/lib/ExecutionEngine/JITLink/ELF_i386.cpp | ||
---|---|---|
66 | Yes. It looks like it's been fixed | |
66 | Yes looks like it's been fixed here - https://github.com/llvm/llvm-project/commit/eca990702d5676151419655098afb57b22cab600 |
The codebase switched to C++17. You are allowed to use nested namespaces.