Is `(v128.const)` supposed to parse in the text format?

[alexcrichton]

Some recently added tests look like they're asserting that various text pieces such as:

• (v128.const)
• (v128.const 0 0 0 0)
• (v128.const i32x4)

I would have expected these to be assert_malformed since they're ideally invalid syntax, but wanted to confirm. @Honry was this perhaps a mistake in the test case generation?

[alexcrichton]

It looks like this is sort of similar to lane-related tests which assert that i8x16.extract_lane_s parses (no immediate argument), but is that perhaps a mistake?

[abrown]

Along these lines, should (v8x16.shuffle -1 ...) parse?

[tlively]

I agree the examples with missing arguments and immediates should be assert_malformed rather than assert_invalid. Not sure about invalid indices, though. It looks like invalid indices on br_table parse but are invalid: https://github.com/WebAssembly/spec/blob/master/test/core/br_table.wast#L1556-L1592

[rossberg]

+1, these should be syntax errors, including negative indices, because indices are unsigned. OOB indices on the other hand are typically a validation error, since the static bounds check requires contextual information.

[AndrewScheidecker]

The problem is that these tests are being developed against WAVM's implementation, and WAVM doesn't strictly enforce the difference between an assert_invalid and an assert_malformed. It does not do validation as a separate stage like the spec interpreter, so sometimes it will hit a validation error before a malformed error that the spec interpreter would have produced. WAVM also doesn't bother trying to distinguish between malformed and invalid initializer expressions as the spec interpreter does.

I've made a PR (#255) on the WAVM repo to fix these tests, and to make WAVM strictly enforce its interpretation of malformed vs validation errors on the SIMD tests, so it should be harder to introduce tests like this that will fail in the spec interpreter.

OOB indices on the other hand are typically a validation error, since the static bounds check requires contextual information.

The lane indices for *.extract_lane, *.replace_lane, and v8x16.shuffle can all be range checked without any information other than which instruction is being parsed, so I think it's more natural for them to be range checked during parsing.

[rossberg]

The lane indices for *.extract_lane, *.replace_lane, and v8x16.shuffle can all be range checked without any information other than which instruction is being parsed, so I think it's more natural for them to be range checked during parsing.

Yes, they need much less context, only the surrounding instruction itself. However, unless you can conveniently classify the legal values by a grammar, treating them via a validation-time check is more appropriate. For range checking free-formed numbers the grammar approach is not very natural, at least not if you want to allow flexible notation like 01 or 0x0 etc. There is precedence for handling similar cases as validation, e.g. the alignment value on loads/stores etc.

[rossberg]

PS: More importantly in the case Wasm is whether the binary format allows free-formed numbers. In particular, the idea is that the Wasm AST is a (more or less) accurate representation of the set of values that the binary format can encode. If something is, say, an i32 in the binary format then it should be parsed as such and range checks would be validation. If the binary format only uses bit patterns to represent a small set of discrete values, then the story is different.

IIUC, a lane index is a u8 or u16 value, so it seems to fall into the former case.

[AndrewScheidecker]

PS: More importantly in the case Wasm is whether the binary format allows free-formed numbers. In particular, the idea is that the Wasm AST is a (more or less) accurate representation of the set of values that the binary format can encode.

There isn't formal spec text/code for the binary format or abstract syntax of lane indices yet, but WAVM uses u8 for the abstract syntax, and serializes it as a u7 (i.e. a u8 whose top bit must be zero).

Based on that, I would say that a text lane index < 0 or >= 128 should be malformed, but lane index < 128 && lane index >= num lanes should be invalid. Does that make sense?

[rossberg]

Yes, that's consistent with constants in other places.

[AndrewScheidecker]

WAVM uses a LEB128-encoded u7 (i.e. it treats a u8>127 as malformed), but the other implementations I looked at (V8, wabt, wasmparser) just serialize lane indices as u8. I prefer the reserved extensibility of the LEB128-encoded u7, but since there are so many implementations already that just use a u8, I'll just change WAVM to do that.

i.e. lane index < 0 || >= 256 should be malformed, but lane index < 256 && lane index >= num lanes should be invalid.

[Honry]

Thanks all, tests have been fixed at #183, PTAL.

[binji]

In a similar vein; should an optional plus be allowed before a lane index? (see

simd/test/core/simd/simd_lane.wast

Line 888 in 7fe545c

(module (func (result i32) (i8x16.extract_lane_u +0x0f (v128.const i8x16 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0))))

)

In general we haven't allowed an optional plus for values that can't be negative. (e.g. indexes are defined as u32)