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

Better offsets #6315

Merged
merged 2 commits into from Sep 9, 2022
Merged

Better offsets #6315

merged 2 commits into from Sep 9, 2022

Conversation

kddnewton
Copy link
Contributor

Two commits in this one. The first consolidates how we handle instruction offsets. The second uses a b instruction when the offset fits for the Jmp instruction. More detailed descriptions in the commits.

Copy link
Contributor

@maximecb maximecb left a comment

Choose a reason for hiding this comment

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

Some tests failing on arm, would have to validate if it's due to this change or not. Otherwise looks great 👌

There are a lot of instructions on AArch64 where we take an offset
from PC in terms of the number of instructions. This is for loading
a value relative to the PC or for jumping.

We were usually accepting an A64Opnd or an i32. It can get
confusing and inconsistent though because sometimes you would
divide by 4 to get the number of instructions or multiply by 4 to
get the number of bytes.

This commit adds a struct that wraps an i32 in order to keep all of
that logic in one place. It makes it much easier to read and reason
about how these offsets are getting used.
@maximecb maximecb merged commit 848037c into ruby:master Sep 9, 2022
@maximecb maximecb deleted the better-offsets branch September 9, 2022 15:37
@noahgibbs
Copy link
Contributor

Ah, this looks like it might be related to the ARM64 crash. I'll check if it's better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants