Wasmtime GitHub notifications bot (May 07 2025 at 22:35): Wasmtime GitHub notifications bot (May 07 2025 at 22:35):saulecabrera requested fitzgen for a review on PR #10750.
Wasmtime GitHub notifications bot (May 07 2025 at 22:35):saulecabrera opened PR #10750 from saulecabrera:winch-aarch64-run-more-tests to bytecodealliance:main:
Closes https://github.com/bytecodealliance/wasmtime/issues/9566
This commit takes inspiration from
https://github.com/bytecodealliance/wasmtime/pull/10738/filesDuring the 2025-05-07 Cranelift meeting we discussed potential avenues to start running wast tests for winch-aarch64. One potential idea was to declare an allowlist of tests that should be exectued and potentially ignore everyhing else. Although that idea works, I decided to diverge a bit from it, in favor of introducing a very strict classification of the state of tests for aarch64, namely, in this commit tests are classified as:
- Run and expected to pass
- Run and exepected to fail in a recoverable way
- Don't run since it's known that they produce an unrecoverable error e.g., segafault.
This approach is probably more verbose than the one discussed in the meeting, however, I think it's easier to have a global view of that status for aarch64 and potentially other backends in the future.
@alexcrichton curious to see what do you think.
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (May 07 2025 at 22:35):saulecabrera requested wasmtime-core-reviewers for a review on PR #10750.
Wasmtime GitHub notifications bot (May 07 2025 at 22:37):saulecabrera updated PR #10750.
Wasmtime GitHub notifications bot (May 07 2025 at 22:47):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (May 07 2025 at 22:47):alexcrichton created PR review comment:
Could the platform-independent tests, the ones that fail on all platforms, be deduplicated between here and x64?
Wasmtime GitHub notifications bot (May 07 2025 at 23:20):saulecabrera updated PR #10750.
Wasmtime GitHub notifications bot (May 07 2025 at 23:20):saulecabrera created PR review comment:
Sure, yeah. Added that in https://github.com/bytecodealliance/wasmtime/pull/10750/commits/0d4784bc61f6b7a5350b2e91d0ec7df85880cae3
Wasmtime GitHub notifications bot (May 07 2025 at 23:20):saulecabrera submitted PR review.
Wasmtime GitHub notifications bot (May 08 2025 at 01:05):github-actions[bot] commented on PR #10750:
Label Messager: wasmtime:config
It looks like you are changing Wasmtime's configuration options. Make sure to
complete this check list:
[ ] If you added a new
Configmethod, you wrote extensive documentation for
it.<details>
Our documentation should be of the following form:
```text
Short, simple summary sentence.More details. These details can be multiple paragraphs. There should be
information about not just the method, but its parameters and results as
well.Is this method fallible? If so, when can it return an error?
Can this method panic? If so, when does it panic?
Example
Optional example here.
```</details>
[ ] If you added a new
Configmethod, or modified an existing one, you
ensured that this configuration is exercised by the fuzz targets.<details>
For example, if you expose a new strategy for allocating the next instance
slot inside the pooling allocator, you should ensure that at least one of our
fuzz targets exercises that new strategy.Often, all that is required of you is to ensure that there is a knob for this
configuration option in [wasmtime_fuzzing::Config][fuzzing-config] (or one
of its nestedstructs).Rarely, this may require authoring a new fuzz target to specifically test this
configuration. See [our docs on fuzzing][fuzzing-docs] for more details.</details>
[ ] If you are enabling a configuration option by default, make sure that it
has been fuzzed for at least two weeks before turning it on by default.[fuzzing-config]: https://github.com/bytecodealliance/wasmtime/blob/ca0e8d0a1d8cefc0496dba2f77a670571d8fdcab/crates/fuzzing/src/generators.rs#L182-L194
[fuzzing-docs]: https://docs.wasmtime.dev/contributing-fuzzing.html
<details>
To modify this label's message, edit the <code>.github/label-messager/wasmtime-config.md</code> file.
To add new label messages or remove existing label messages, edit the
<code>.github/label-messager.json</code> configuration file.</details>
Wasmtime GitHub notifications bot (May 08 2025 at 15:30):saulecabrera commented on PR #10750:
The failure in CI happens because
wasmtime_test_util::wast::Compiler::should_failgets invoked fromwasmtime_testmacro, and as far as I understand,#[cfg(target_arch="...")]in macro code will resolve to the host, not to the target, which ends up returningtrueforshould_fail, but since we're cross compiling (aarch64 -> x64), in reality the test passes and the#[should_panic]predicate is not met.Prior to this change this was not an issue, since we were only taking x64 into account for Winch.
Wasmtime GitHub notifications bot (May 08 2025 at 15:30):saulecabrera edited a comment on PR #10750:
The failure in CI happens because
wasmtime_test_util::wast::Compiler::should_failgets invoked from thewasmtime_testmacro, and as far as I understand,#[cfg(target_arch="...")]in macro code will resolve to the host, not to the target, which ends up returningtrueforshould_fail, but since we're cross compiling (aarch64 -> x64), in reality the test passes and the#[should_panic]predicate is not met.Prior to this change this was not an issue, since we were only taking x64 into account for Winch.
Wasmtime GitHub notifications bot (May 08 2025 at 15:50):alexcrichton commented on PR #10750:
Bah I was afraid of this happening at some point...
Could we perhaps delay this to happening at runtime? We've got the two structures there at test-run-time so we should be able to run the same code. I'd prefer to not catch panics so it means though that all tests would have to return
Result<()>and the runtime result ofshould_failwould result in expecting an error or not, however.
Wasmtime GitHub notifications bot (May 08 2025 at 16:33):saulecabrera commented on PR #10750:
Yeah, I'll try that. I remember we had discussed this, I just wasn't sure if there was a strong preference regarding catching panics.
Wasmtime GitHub notifications bot (May 08 2025 at 16:35):alexcrichton commented on PR #10750:
Personally I'd say I have a weak preference, if that's the best solution maintenance-wise I'd go with catching panics
Wasmtime GitHub notifications bot (May 08 2025 at 18:40):saulecabrera updated PR #10750.
Wasmtime GitHub notifications bot (May 08 2025 at 18:41):saulecabrera updated PR #10750.
Wasmtime GitHub notifications bot (May 08 2025 at 18:42):saulecabrera commented on PR #10750:
Ok, ended up going with emitting the call to
Compiler::should_failat runtime. Would you mind taking another look?
Wasmtime GitHub notifications bot (May 08 2025 at 22:59):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (May 08 2025 at 22:59):alexcrichton created PR review comment:
Could this use
result.unwrap()to print the error on failure?
Wasmtime GitHub notifications bot (May 09 2025 at 12:46):saulecabrera updated PR #10750.
Wasmtime GitHub notifications bot (May 09 2025 at 13:06):saulecabrera updated PR #10750.
Wasmtime GitHub notifications bot (May 09 2025 at 13:43):saulecabrera merged PR #10750.
Last updated: Dec 06 2025 at 06:05 UTC