Skip to content
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.

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

Winch support for AArch64 tough to integrate with tests/wast.rs #9566

Open
alexcrichton opened this issue Nov 5, 2024 · 3 comments
Open
Labels
winch Winch issues or pull requests

Comments

@alexcrichton
Copy link
Member

I've done some recent refactoring over the past month or so of the tests/wast.rs test suite. The general idea is that it should not only assert that passing tests pass but it should additionally assert that failing tests fail. This ensures that we keep an accurate picture of where tests are at and we know to enable spec tests once support is implemented.

This is problematic on Winch for AArch64, however. The problem is that when a test is supposed to fail it's run anyway and it's expected to return an error through Result. Failures such as panics and segfaults are not currently recoverable. Currently, though, tests on the AArch64 backend for Winch often panic (e.g. todo!() items) or segfault (likely because something isn't finished yet).

The way that tests/wast.rs works for now is that it pretends that all tests require the reference-types feature for wasm which is hardcoded as "doesn't work on aarch64 winch". This causes all tests to fail early in Config::validate before the backend is used. This isn't great though because some tests do indeed pass on Winch right now and it might be reasonable to start ratcheting support to ensure it doesn't regress.

cc @saulecabrera and @vulc41n as y'all may be interested in this. I'll clarify that this is not an urgent issue by any means -- I don't intend to put pressure on either of y'all. Instead I wanted to make y'all aware of this if you're looking for future aarch64 work.

@alexcrichton alexcrichton added the winch Winch issues or pull requests label Nov 5, 2024
Copy link

github-actions bot commented Nov 5, 2024

Subscribe to Label Action

cc @saulecabrera

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 .github/subscribe-to-label.json configuration file.

Learn more.

@saulecabrera
Copy link
Member

This isn't great though because some tests do indeed pass on Winch right now and it might be reasonable to start ratcheting support to ensure it doesn't regress.

Agreed, thanks for putting this issue together (I'll add it to the Winch board if you don't mind) -- we're almost at parity with the x64 backend (see #8321 (comment)). After finishing epoch my plan is to finalize the 11 instructions needed in order to have the aarch64 backend be at parity with x64, which will enable running spec tests in aarch64.

For the unrecoverable errors, I hope that #8096, will address that issue and make it easier to integrate with your refactoring for test handling. I will most likely address that one after having finished support for aarch64.

@alexcrichton
Copy link
Member Author

That all sounds great to me 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
winch Winch issues or pull requests
Projects
Status: Todo
Development

No branches or pull requests

2 participants