Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[llvm-jitlink] Support plain AArch32 range extension stubs in jitlink-check's stub_addr() expressions #73268

Merged
merged 3 commits into from
Nov 24, 2023

Conversation

weliveindetail
Copy link
Contributor

We want to use regular stub_addr() expressions in jitlink-check lines to test the generation of stubs in AArch32, but we don't want this to require a standardized GOT-based PLT implementation. In terms of performance and binary size it doesn't seem beneficial. And in terms of patching, we should be able to handle range-extension- and interworking-stubs without a lot of extra logic.

This patch adds a separate path for AArch32 stubs in llvm-jitlink-elf to allow it. The relocations in our stubs are not pointing to the GOT, but to the external symbol directly. Thus, we have to avoid access to the block of the edge target. Instead we only return the symbol name. This is enough to use stub_addr() expressions in tests.

In order to allow decoding of stub target addresses in the future, we mention the stub flavor in the section name.

Copy link
Contributor

@lhames lhames left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks Stefan!

Copy link
Contributor

@eymay eymay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that now in JITLink AArch32 we can have PLT and interworking/range-extension stubs in different sections so they are kept orthogonal features. Now the checker is also acknowledging this separation.

@@ -12,6 +12,7 @@

#include "llvm-jitlink.h"

#include "llvm/ExecutionEngine/Orc/Core.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, yes it's a leftover. Together with the orc::ExecutionSession &ES parameter below...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might play with section name here, in the future possibly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, ideally I'd like to have jitlink-check lines like this:

# jitlink-check: decode_operand(stub_addr(elf_stubs.o, fn) + 0, 1) = (fn & 0x0000ffff)
# jitlink-check: decode_operand(stub_addr(elf_stubs.o, fn) + 4, 2) = (fn & 0xffff0000) >> 16

We know it's a Thumv7 stub and we could check the movw + movt immediates directly, but decode_operand() doesn't support address expressions right now. We could add that, but it doesn't seem trivial.

One alternative might be to do it in the checker itself and then add something like:

# jitlink-check: decode_stub_target(elf_stubs.o, fn) = fn

In this case the section name would determine the stub type for the checker. Not urgent yet, but something to think about.

Copy link
Contributor Author

@weliveindetail weliveindetail left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that now in JITLink AArch32 we can have PLT and interworking/range-extension stubs in different sections so they are kept orthogonal features. Now the checker is also acknowledging this separation.

Yes, I should express that in the commit message. Thanks for your notes!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, ideally I'd like to have jitlink-check lines like this:

# jitlink-check: decode_operand(stub_addr(elf_stubs.o, fn) + 0, 1) = (fn & 0x0000ffff)
# jitlink-check: decode_operand(stub_addr(elf_stubs.o, fn) + 4, 2) = (fn & 0xffff0000) >> 16

We know it's a Thumv7 stub and we could check the movw + movt immediates directly, but decode_operand() doesn't support address expressions right now. We could add that, but it doesn't seem trivial.

One alternative might be to do it in the checker itself and then add something like:

# jitlink-check: decode_stub_target(elf_stubs.o, fn) = fn

In this case the section name would determine the stub type for the checker. Not urgent yet, but something to think about.

@@ -12,6 +12,7 @@

#include "llvm-jitlink.h"

#include "llvm/ExecutionEngine/Orc/Core.h"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, yes it's a leftover. Together with the orc::ExecutionSession &ES parameter below...

Copy link

github-actions bot commented Nov 24, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

@weliveindetail weliveindetail merged commit 95dcb8b into llvm:main Nov 24, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants