Skip to content

Conversation

alexcrichton
Copy link
Member

This commit fixes a bug in the new assembler which was surfaced through the changes in #10782 but was a pre-existing issue. Specifically the encoding of a RIP-relative addressing mode required knowing the number of bytes at the end of an instruction but this was accidentally hardcoded to 0. In #10782 imul instructions were added where a RIP-relative address mode can be used in conjunction with an immediate which cause the RIP-relative addressing to load from the wrong address.

This bug can in theory affect other instructions in the new assembler as well, but auditing the list of instructions it looks like imul is the only one that can possibly have an immediate after a RIP-relative addressing mode. That means that prior instructions using the new assembler should not be affected.

This commit fixes a bug in the new assembler which was surfaced through
the changes in bytecodealliance#10782 but was a pre-existing issue. Specifically the
encoding of a RIP-relative addressing mode required knowing the number
of bytes at the end of an instruction but this was accidentally
hardcoded to 0. In bytecodealliance#10782 `imul` instructions were added where a
RIP-relative address mode can be used in conjunction with an immediate
which cause the RIP-relative addressing to load from the wrong address.

This bug can in theory affect other instructions in the new assembler as
well, but auditing the list of instructions it looks like `imul` is the
only one that can possibly have an immediate after a RIP-relative
addressing mode. That means that prior instructions using the new
assembler should not be affected.
@alexcrichton alexcrichton requested a review from a team as a code owner May 20, 2025 21:12
@alexcrichton alexcrichton requested review from abrown and removed request for a team May 20, 2025 21:12
Copy link
Member

@abrown abrown left a comment

Choose a reason for hiding this comment

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

LGTM! (Though it looks like something will need to change relative to pretty-printing...)

@alexcrichton
Copy link
Member Author

@abrown mind double-checking the last commit I just added?

Copy link
Member

@abrown abrown left a comment

Choose a reason for hiding this comment

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

Yeah, there must surely be a different way to sort this all out but this is great for now.

@alexcrichton alexcrichton enabled auto-merge May 20, 2025 22:20
@alexcrichton alexcrichton added this pull request to the merge queue May 20, 2025
Merged via the queue into bytecodealliance:main with commit 83fe01e May 20, 2025
71 checks passed
@alexcrichton alexcrichton deleted the fix-rip-relative-addressing branch May 20, 2025 23:02
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.

2 participants