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

Make module validation deterministic in case of multiple errors #9947

Conversation

michael-weigelt
Copy link
Contributor

@michael-weigelt michael-weigelt commented Jan 8, 2025

If a Wasm binary is invalid in several ways, parallel compilation will return a non-deterministic result. We would like to get a deterministic result even in the presence of multiple errors.

This PR addresses that by first materializing all validation results and then returning the first error, if any ('first' with regard to the order of the input vector).

The downside is that we cannot return early when an error is encountered. This slows down the validation for invalid binaries. Since that is not the happy path anyway, I propose to accept the slowdown.

The provided test does not prove determinism, which is hard to do. But it gives some confidence and ensures the behaviours of parallel and sequential compilation do not deviate.

@michael-weigelt michael-weigelt requested a review from a team as a code owner January 8, 2025 12:15
@michael-weigelt michael-weigelt requested review from alexcrichton and removed request for a team January 8, 2025 12:15
@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Jan 8, 2025
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Thanks for this!

@alexcrichton alexcrichton added this pull request to the merge queue Jan 8, 2025
alexcrichton added a commit to alexcrichton/wasm-tools that referenced this pull request Jan 8, 2025
Inspired by bytecodealliance/wasmtime#9947 this helps keep the output of
`wasm-tools validate` more deterministic by ensuring that the first
error is reported instead of any error within a module.
github-merge-queue bot pushed a commit to bytecodealliance/wasm-tools that referenced this pull request Jan 8, 2025
Inspired by bytecodealliance/wasmtime#9947 this helps keep the output of
`wasm-tools validate` more deterministic by ensuring that the first
error is reported instead of any error within a module.
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jan 8, 2025
@alexcrichton
Copy link
Member

Ah the timeout there looks like the new validate_deterministic test is taking awhile in miri, so mind putting #[cfg(not(miri))] on the test?

@alexcrichton
Copy link
Member

er, actually, #[cfg_attr(miri, ignore)] would probably be better

Michael Weigelt added 2 commits January 9, 2025 08:37
@michael-weigelt
Copy link
Contributor Author

@alexcrichton done, and I made the test smaller too.

@alexcrichton alexcrichton added this pull request to the merge queue Jan 9, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 9, 2025
@alexcrichton alexcrichton added this pull request to the merge queue Jan 9, 2025
Merged via the queue into bytecodealliance:main with commit 1c97044 Jan 9, 2025
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants