Open backes opened 1 year ago
Yes, malformed tests are not convertible. Generating the string <malformed quote>
is producing something equivalent, with the idea that this leaves a trace of any assert_malformed test in the output file for reference.
Tests with assert_invalid should always be convertible. Can you point to an example where one uses a malformed quote? If so, that's a mistake.
I didn't spot any non-convertible modules with assert_invalid
, I just noticed that this produces the same output. I would have expected the wast->JS conversion to fail if assert_invalid
is used with a malformed module.
Given that there are (currently) 1791 instances of assert_malformed
in the core tests, I wonder if it makes sense to generate the same test for all of them, or if we should just skip them in the wast->JS conversion. We could output a comment instead to note in the JS file that this test cannot be translated into a JS test and was thus skipped.
The conversions of assertions and of modules are performed independently, so there is no checking or special casing for specific combinations. Could be added, but there isn't much value to it.
Combining assert_invalid
with a malformed module should not pass when run in the reference interpreter itself, so we ought to be able to assume that meaningless combinations like that cannot actually arise during conversion.
Ack, let's ignore the assert_invalid
case.
How much work would it be to skip the assert_malformed
tests in the JS generation? We currently execute 2869 assert_malformed
tests in every test run of V8, which seems unnecessary.
We could also filter those out as part of our script to import spec tests, but I would prefer to fix this at the root.
@gahaas FYI
You certainly don't want to skip all assert_malformed tests, only those with textual modules. Special-casing that should not be difficult, only slightly ugly, I believe.
Why do you care much about these cases? Aren't they cheap, since they fail immediately at decoding the first byte?
The original motivation for filing this was that I was confused that we didn't fail the test quoted above (V8 was missing a validation and produced a trap at runtime instead of a validation error). Before fixing this I was checking if there is a test for this and I found the assert_malformed
test. Then I found out that this test was not executed properly in the JS test because it couldn't be translated to a wasm binary.
Hence the request to just skip those tests (maybe with a comment in the JS file) instead of including them but with the "
Definitely not high priority, but might avoid more confusion in the future.
I observed this on the address.wast test, line 213:
Converted to JS via the spec interpreter (
./wasm -d address.wast -o address.js
), it produces this check:This checks that the string
<malformed quote>
is considered invalid Wasm, which is not a sensible test.I am not sure what we should do instead, I guess the underlying problem is that the interpreter cannot convert the WAT to binary, so there is not much it can do. Maybe it should skip the test instead?
The same happens with
assert_invalid
and any invalid string, e.g.