assert_invalid and assert_malformed produce inconsistent JS tests

[backes]

I observed this on the address.wast test, line 213:

(assert_malformed
  (module quote
    "(memory 1)"
    "(func (drop (i32.load offset=4294967296 (i32.const 0))))"
  )
  "i32 constant"
)

Converted to JS via the spec interpreter (./wasm -d address.wast -o address.js), it produces this check:

assert_malformed("\x3c\x6d\x61\x6c\x66\x6f\x72\x6d\x65\x64\x20\x71\x75\x6f\x74\x65\x3e");

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.

(assert_invalid
  (module quote "(foobar)")
  "foobar"
)

[rossberg]

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.

[backes]

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.

[rossberg]

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.

[backes]

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

[rossberg]

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?

[backes]

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 "" payload. As that's hex-encoded, it looks like valid test, but does not test anything reasonable.

Definitely not high priority, but might avoid more confusion in the future.