-
Notifications
You must be signed in to change notification settings - Fork 475
[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
Conversation
There was a problem hiding this 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?
test/core/data.wast
Outdated
(module | ||
(global (export "global-i32") i32 (i32.const 0)) | ||
(global (export "global-mut-i32") (mut i32) (i32.const 0)) | ||
) | ||
(register "test") | ||
|
There was a problem hiding this comment.
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.
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. |
1416df2
to
23f9c8d
Compare
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. |
23f9c8d
to
cca5a5e
Compare
Thanks for the clarification! I've removed all linking-related changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
No description provided.