8000 [test] Account for one-pass implementations in more places by YerinAlexey · Pull Request #1904 · WebAssembly/spec · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[test] Account for one-pass implementations in more places #1904

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

Merged
merged 1 commit into from
May 13, 2025

Conversation

YerinAlexey
Copy link
Contributor

No description provided.

Copy link
Member
@rossberg rossberg left a comment

Choose a reason for hiding this comment

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

Hm, to be honest, I don't follow the reason for most of the changes in this PR. Can you elaborate why you think they're needed and how they relate to streaming compilation?

Comment on lines 415 to 420
(module
(global (export "global-i32") i32 (i32.const 0))
(global (export "global-mut-i32") (mut i32) (i32.const 0))
)
(register "test")

Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be needed. Validation has to happen before linking, so it mustn't matter for any of the tests whether the imported module actually exists.

Same with the other tests.

@YerinAlexey
Copy link
Contributor Author

Sorry, I mixed up the terminology in the title - I was referring to implementations that do decoding and validation in one pass.

I was generally following the idea that failure tests should only contain one type of error.

This test change specifically permits implementations to immediately resolve imports rather than doing that during instantiation. I think this is reasonable and can even be beneficial for some of them. If this addition is undesirable, I can remove that part.

@YerinAlexey YerinAlexey force-pushed the streaming-test-fix branch from 1416df2 to 23f9c8d Compare May 12, 2025 21:35
@YerinAlexey YerinAlexey changed the title [test] Account for streaming implementations in more places [test] Account for one-pass implementations in more places May 12, 2025
@rossberg
Copy link
Member

With Wasm's model of separate compilation, imports are not available during validation or compilation. Providing them, i.e., linking, is part of instantiation, and a compiled module can be instantiated multiple times with different imports. Even for API that combines the two phases, an invalid module must be treated as if linking was never started.

Because of that, the presence of module imports should never matter for pure validation tests, and we in fact want to test that validation fails regardless.

@YerinAlexey YerinAlexey force-pushed the streaming-test-fix branch from 23f9c8d to cca5a5e Compare May 13, 2025 14:16
@YerinAlexey
Copy link
Contributor Author

Thanks for the clarification! I've removed all linking-related changes.

Copy link
Member
@rossberg rossberg left a comment

Choose a reason for hiding this comment

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

Thanks!

@rossberg rossberg merged commit 8d6792e into WebAssembly:main May 13, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0