Stream: git-wasmtime

Topic: wasmtime / PR #10763 winch: Implement check_stack for aar...

view this post on Zulip Wasmtime GitHub notifications bot (May 10 2025 at 02:34):

graydon opened PR #10763 from graydon:winch-aarch64-check-stack to bytecodealliance:main:

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

view this post on Zulip Wasmtime GitHub notifications bot (May 10 2025 at 02:34):

graydon requested wasmtime-compiler-reviewers for a review on PR #10763.

view this post on Zulip Wasmtime GitHub notifications bot (May 10 2025 at 02:34):

graydon requested abrown for a review on PR #10763.

view this post on Zulip Wasmtime GitHub notifications bot (May 10 2025 at 02:34):

graydon requested alexcrichton for a review on PR #10763.

view this post on Zulip Wasmtime GitHub notifications bot (May 10 2025 at 02:34):

graydon requested wasmtime-core-reviewers for a review on PR #10763.

view this post on Zulip Wasmtime GitHub notifications bot (May 10 2025 at 03:49):

github-actions[bot] commented on PR #10763:

Subscribe to Label Action

cc @saulecabrera

<details>
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:

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

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (May 10 2025 at 23:16):

graydon updated PR #10763.

view this post on Zulip Wasmtime GitHub notifications bot (May 11 2025 at 00:01):

graydon updated PR #10763.

view this post on Zulip Wasmtime GitHub notifications bot (May 11 2025 at 20:02):

alexcrichton requested saulecabrera for a review on PR #10763.

view this post on Zulip Wasmtime GitHub notifications bot (May 11 2025 at 20:02):

alexcrichton commented on PR #10763:

(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 :+1:

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2025 at 15:22):

saulecabrera created PR review comment:

Perhaps we should update the documentation here, since the implementation here differs a bit from x64, e.g., we're emitting add rd, rm, rn

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2025 at 15:22):

saulecabrera created PR review comment:

Here we might want to use self.masm.trapif instead. Aarch64 requires the SP to be 16-byte aligned, and even though the SP is not used for memory addressing, if a trap is raised, signal handling code will segfault if SP is not correctly aligned. See https://github.com/bytecodealliance/wasmtime/pull/10146 for more details.

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2025 at 15:22):

saulecabrera submitted PR review:

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, https://github.com/bytecodealliance/wasmtime/pull/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.

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2025 at 15:22):

saulecabrera created PR review comment:

Should we perhaps change these two to be Writable<Reg>, we're changing both of them and it's easier to audit sites in which registers are changed in the MacroAssembler layer if those are annotated as writable.

view this post on Zulip Wasmtime GitHub notifications bot (May 13 2025 at 01:44):

graydon submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 13 2025 at 01:44):

graydon created PR review comment:

Oh, sure (self already is a MacroAssembler so I assume you mean self.trapif).

view this post on Zulip Wasmtime GitHub notifications bot (May 13 2025 at 01:45):

graydon submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 13 2025 at 01:45):

graydon created PR review comment:

Oh, ok. I already did alter it (note it describes an instruction _sequence_) but I can alter it to be a little more explicit.

view this post on Zulip Wasmtime GitHub notifications bot (May 13 2025 at 01:45):

graydon submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 13 2025 at 01:45):

graydon created PR review comment:

Sure, done.

view this post on Zulip Wasmtime GitHub notifications bot (May 13 2025 at 01:46):

graydon updated PR #10763.

view this post on Zulip Wasmtime GitHub notifications bot (May 13 2025 at 01:51):

graydon commented on PR #10763:

@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?)

view this post on Zulip Wasmtime GitHub notifications bot (May 13 2025 at 02:06):

graydon updated PR #10763.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2025 at 00:07):

saulecabrera commented on PR #10763:

@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.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2025 at 07:10):

graydon updated PR #10763.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2025 at 13:27):

saulecabrera submitted PR review:

LGTM, thanks!

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2025 at 13:48):

saulecabrera merged PR #10763.

Last updated: Dec 06 2025 at 07:03 UTC