-
Notifications
You must be signed in to change notification settings - Fork 493
[interpreter] Fix JS conversion in assertions #1989
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
A refactoring missed the comma before the argument.
Arguably we should have some testing in this repo, but that would require a JS engine. And in the general case, a JS engine that supports all the wasm features that the interpreter does. Not sure if it's worth adding. |
I also have a PR out to fix the bikeshed boilerplate errors. |
Thanks for the fix! JS translation actually used to be tested, but I had to deactivate it in CI when Node didn't fully support Wasm 3.0. I just tried to reactivate it in a756f17, but it looks like current Node still does not correctly support the Table constructor for i64 address type (https://github.com/WebAssembly/spec/actions/runs/18094250858/job/51481617389), so I had to roll back. |
We have similar issues for Fuzzilli where we test features that might not be in node's stable version, yet (like the In our case we switched to using nightly versions (e.g. |
I just tried with 25-nightly, but there still is one odd failure on ref_null.wast: https://github.com/WebAssembly/spec/actions/runs/18100009934/job/51500259650 Not sure what's going on with that one. |
We skip this test case in V8 with the following comment:
(as |
Ah, so this actually is a bug in the translator, since it should wrap such calls into Wasm. Perhaps this mechanism wasn't extended for the exception proposal. I'll have a look. |
Okay, that was a one-line fix, the translator simply forgot to filter Strangely enough, it now fails on a different test, namely table.wast producing some validation error. |
Yeah, That PR was closed with a comment:
So one difficulty of using a web engine to validate the spec tests is that it requires a web engine that passes the spec tests... 🙃 |
The offending test appears to be this one: (module definition (table 0xffff_ffff funcref)) Now that I look at it, the problem seems to go a bit deeper: The underlying cause of this failure apparently is that the JS-API spec squarely demands a violation of the core spec! In particular, it says that a CompileError (i.e., validation error) has to be produced when certain bounds for declared memory or table limits are exceeded. But that behaviour is not actually allowed by the core spec, which only allows an instantiation-time error for a module like the above (which explicitly is not instantiated by the test). I think this needs fixing in the JS spec (and JS engines). |
Perhaps #1863 has struck again? It may be time to prioritize that and get these conditions cleaned up. |
Yes, indeed! Would be good to make progress on that. |
I tried to replace Node with latest SpiderMonkey's JS shell, but that also failed, on memory64. |
A refactoring missed the comma before the argument.
Fixes the error seen in web-platform-tests/wpt#54969 (comment)