Skip to content

winch: Implement check_stack for aarch64 #10763

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
May 15, 2025
Merged

winch: Implement check_stack for aarch64 #10763

merged 7 commits into from
May 15, 2025

Conversation

graydon
Copy link
Contributor

@graydon graydon commented May 10, 2025

This is my first attempt at contributing to winch or cranelift at all -- wide open to any feedback/suggestions. I was just poking around looking for things I could do to help push winch over the finish line on aarch64. @saulecabrera suggested the missing check_stack implementation might be a welcome change.

(This is also my first time working with aarch64 instructions in any detail; hopefully I got the semantics not-too-wrong)

Before:

$ cargo run -- wast -Ccompiler=winch tests/spec_testsuite/call.wast
[...]
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.41s
     Running `target/debug/wasmtime wast -Ccompiler=winch tests/spec_testsuite/call.wast`
execution on async fiber has overflowed its stackzsh: abort      cargo run -- wast -Ccompiler=winch tests/spec_testsuite/call.wast

After:

$ cargo run -- wast -Ccompiler=winch tests/spec_testsuite/call.wast
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.41s
     Running `target/debug/wasmtime wast -Ccompiler=winch tests/spec_testsuite/call.wast`
Error: failed to run script file 'tests/spec_testsuite/call.wast'

Caused by:
    0: failed directive on tests/spec_testsuite/call.wast:285:1
    1: error while executing at wasm backtrace:
           0: <unknown>!<wasm function 17>
    2: wasm trap: call stack exhausted

@graydon graydon requested review from a team as code owners May 10, 2025 02:34
@graydon graydon requested review from abrown and alexcrichton and removed request for a team May 10, 2025 02:34
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:aarch64 Issues related to AArch64 backend. winch Winch issues or pull requests labels May 10, 2025
@github-actions GitHub Actions
Copy link

Subscribe to Label Action

cc @saulecabrera

This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "winch"

Thus the following users have been cc'd because of the following labels:

  • saulecabrera: winch

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@graydon graydon force-pushed the winch-aarch64-check-stack branch from a1a87f5 to 8a8bccc Compare May 10, 2025 23:16
@alexcrichton alexcrichton requested review from saulecabrera and removed request for abrown and alexcrichton May 11, 2025 20:02
@alexcrichton
Copy link
Member

(moving review over to @saulecabrera)

Looks like you found WASMTIME_TEST_BLESS=1 though which is indeed the recommend way to fix the original test failures 👍

saulecabrera
saulecabrera reviewed May 12, 2025
View reviewed changes
Copy link
Member

@saulecabrera saulecabrera left a comment

Choose a reason for hiding this comment

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

This is looking really good, thanks! I left a couple of comments inline, which I think is probably worth addressing.

In terms of tests, since our last discussion via Zulip, #10750 landed, which enables running some aarch64 tests in CI. It's probably a good idea to enable at least the call.wast tests here https://github.com/bytecodealliance/wasmtime/blob/main/crates/test-util/src/wast.rs#L422. Ideally, we should enable all of them, but it's possible that some of them will fail, due to some of the existing bugs that I'm currently in the process of fixing.

@graydon
Copy link
Contributor Author

graydon commented May 13, 2025

@saulecabrera thanks for the review! one thing worth pointing out is that this change doesn't include the bit about emitting unwind instructions that's in the x86_64 masm. I got the impression that's a larger project though (there doesn't seem to be any unwind support in the aarch64 backend at all right now -- is that something else that needs doing?)

@saulecabrera
Copy link
Member

@graydon You're right that there is no unwind support right now, it's not expected to land that functionality as part of this PR. I think that once that (any passing) tests are updated here https://github.com/bytecodealliance/wasmtime/blob/main/crates/test-util/src/wast.rs#L422, we should be able to land this one.

@graydon graydon force-pushed the winch-aarch64-check-stack branch from d6fb23b to 08b2402 Compare May 15, 2025 07:10
saulecabrera
saulecabrera approved these changes May 15, 2025
View reviewed changes
Copy link
Member

@saulecabrera saulecabrera 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!

@saulecabrera saulecabrera added this pull request to the merge queue May 15, 2025
Merged via the queue into bytecodealliance:main with commit 3f876fe May 15, 2025
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saulecabrera saulecabrera saulecabrera approved these changes

Assignees
No one assigned
Labels
cranelift:area:aarch64 Issues related to AArch64 backend. cranelift Issues related to the Cranelift code generator winch Winch issues or pull requests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@graydon @alexcrichton @saulecabrera