wasmtime / PR #11031 winch(aarch64) Run integration tests... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #11031 winch(aarch64) Run integration tests...

Wasmtime GitHub notifications bot (Jun 12 2025 at 19:35):

saulecabrera opened PR #11031 from saulecabrera:winch-aarch64-enable-integration-tests to bytecodealliance:main:

This commit closes
https://github.com/bytecodealliance/wasmtime/issues/8321

With https://github.com/bytecodealliance/wasmtime/pull/11013, Winch passes all spec tests for Core Wasm on Aarch64.

Wasmtime GitHub notifications bot (Jun 12 2025 at 19:35):

saulecabrera requested alexcrichton for a review on PR #11031.

Wasmtime GitHub notifications bot (Jun 12 2025 at 19:35):

saulecabrera requested wasmtime-compiler-reviewers for a review on PR #11031.

Wasmtime GitHub notifications bot (Jun 12 2025 at 19:35):

saulecabrera requested wasmtime-core-reviewers for a review on PR #11031.

Wasmtime GitHub notifications bot (Jun 12 2025 at 19:36):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Jun 12 2025 at 19:36):

saulecabrera created PR review comment:

We need to gracefully handle compilation errors to correctly assert that a test should fail in case of missing features (e.g., SIMD on aarch64).

Wasmtime GitHub notifications bot (Jun 12 2025 at 19:37):

saulecabrera updated PR #11031.

Wasmtime GitHub notifications bot (Jun 12 2025 at 19:41):

saulecabrera edited PR #11031:

This commit closes https://github.com/bytecodealliance/wasmtime/issues/8321

With https://github.com/bytecodealliance/wasmtime/pull/11013, Winch passes all spec tests for Core Wasm on Aarch64.

Wasmtime GitHub notifications bot (Jun 12 2025 at 20:08):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 12 2025 at 20:08):

alexcrichton created PR review comment:

I think that this actually causes lots of test to "fail" with:
...

---- Winch/./tests/spec_testsuite/float_exprs.wast ----
this test is flagged as should-fail but it succeeded

---- Winch/./tests/spec_testsuite/table_copy.wast ----
this test is flagged as should-fail but it succeeded

---- Winch/./tests/spec_testsuite/const.wast ----
this test is flagged as should-fail but it succeeded

...
the reason being that when we classify spec features required by spec tests we do that by directory as opposed to per-test so the entire spec test suite is considered as "requires simd". That means that all spec tests are expected to fail despite many of them passing.

One possible fix is we could split into config.simd into config.really_needs_simd and config.maybe_needs_simd so this could return "should fail" for config.really_needs_simd but for spec tests they'd just be flagged with maybe_needs_simd which means we'd still run them (and allow-list failures below)

Wasmtime GitHub notifications bot (Jun 12 2025 at 20:19):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Jun 12 2025 at 20:19):

saulecabrera created PR review comment:

Ah right, I didn't test this change with x64 and I had forgotten that the should_fail code is now shared. Let me explore your idea and see where I land.

Wasmtime GitHub notifications bot (Jun 12 2025 at 21:46):

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

Subscribe to Label Action

cc @saulecabrera

<details>
This issue or pull request has been labeled: "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 <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Jun 13 2025 at 11:25):

saulecabrera edited PR review comment.

Wasmtime GitHub notifications bot (Jun 13 2025 at 15:28):

saulecabrera updated PR #11031.

Wasmtime GitHub notifications bot (Jun 13 2025 at 15:32):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Jun 13 2025 at 15:32):

saulecabrera created PR review comment:

@alexcrichton I've pushed a variation of your suggestion in https://github.com/bytecodealliance/wasmtime/pull/11031/commits/99e648766a9ebb41b864b92f96fe2eb16669d397 -- let me know what you think. Given that this configuration struct is shared among two tests contexts, I thought it might be easier to reason through if we instead introduce a proper option to flag the test as integration. Your suggested approach works, but I found it a bit challenging to reason about since it modifies to a certain extent the semantic meaning of the Wasm config depending on the testing context. Happy to revisit that last commit if you think that dividing the simd option is better though.

Wasmtime GitHub notifications bot (Jun 13 2025 at 15:34):

saulecabrera requested fitzgen for a review on PR #11031.

Wasmtime GitHub notifications bot (Jun 13 2025 at 15:34):

saulecabrera requested wasmtime-fuzz-reviewers for a review on PR #11031.

Wasmtime GitHub notifications bot (Jun 13 2025 at 15:34):

saulecabrera updated PR #11031.

Wasmtime GitHub notifications bot (Jun 13 2025 at 15:36):

saulecabrera updated PR #11031.

Wasmtime GitHub notifications bot (Jun 13 2025 at 15:53):

alexcrichton submitted PR review:

I like it :+1:

If you'll indulge a minor bikeshed though, what about inverting this from integration: bool to spec_test: bool? I'm a little worried that only the macro-defined tests are integration: true where all of tests/misc_testsuite/* should in theory be automatically integration: true as well. By inverting this to spec_test: bool it should be easy to audit the single location that classifies spec tests and ensure it sets that and then everything else can ignore it being set.

Also, as a minor nit, instead of adding .. to patterns can you add spec_test: _ to explicitly ignore the field? The exhaustive match helps ensure that we update the patterns/etc when a new field is added.

Wasmtime GitHub notifications bot (Jun 13 2025 at 17:25):

saulecabrera updated PR #11031.

Wasmtime GitHub notifications bot (Jun 13 2025 at 17:26):

saulecabrera updated PR #11031.

Wasmtime GitHub notifications bot (Jun 13 2025 at 17:27):

saulecabrera commented on PR #11031:

@alexcrichton all that make sense, I've pushed the fixes in https://github.com/bytecodealliance/wasmtime/pull/11031/commits/93bd9d4530203806e1ae1efe76359e879dda3733

Wasmtime GitHub notifications bot (Jun 13 2025 at 17:29):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 13 2025 at 17:29):

alexcrichton has enabled auto merge for PR #11031.

Wasmtime GitHub notifications bot (Jun 13 2025 at 17:58):

saulecabrera updated PR #11031.

Wasmtime GitHub notifications bot (Jun 13 2025 at 18:07):

saulecabrera has enabled auto merge for PR #11031.

Wasmtime GitHub notifications bot (Jun 13 2025 at 18:48):

saulecabrera updated PR #11031.

Wasmtime GitHub notifications bot (Jun 13 2025 at 19:35):

saulecabrera commented on PR #11031:

Hmm there seems to be a legit segfault for macOS-arm64, taking a look.

Wasmtime GitHub notifications bot (Jun 13 2025 at 21:45):

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

Subscribe to Label Action

cc @fitzgen

<details>
This issue or pull request has been labeled: "fuzzing"

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

fitzgen: fuzzing

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

Learn more.
</details>

Wasmtime GitHub notifications bot (Jun 16 2025 at 13:58):

saulecabrera merged PR #11031.

Wasmtime GitHub notifications bot (Jun 16 2025 at 14:00):

saulecabrera commented on PR #11031:

It didn't seem like a segfault in the test itself, nor I was able to reproduce after running the job multiple times (locally and in CI).

Last updated: Dec 06 2025 at 06:05 UTC