bytecodealliance/wat

Parsing invalid lane indexes

Closed this issue · 4 comments

abrown commented

In a call to wast::parser::parse::<wast::Wast>, the wast parser fails too early. In the spec test simd_lane.wast, I see:

Error: invalid u32 number: constant out of range
     --> tests/spec_testsuite/proposals/simd/simd_lane.wast:409:66
      |
  409 | (assert_invalid (module (func (result i32) (i8x16.extract_lane_s -1 (v128.const i8x16 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0)))) "invalid lane index")
      |                                                                  ^

The wast parser is failing correctly (see the integer-width logic here) but perhaps it needs to be lazier when failing, since it should be failing from some call in the AssertInvalid block.

alexcrichton commented

I think this happens because when we parse this instruction the payload is a u32, not an i32, and -1 doesn't fit inside of a u32 (at least not currently)

We can probably easily fix this though by switching that to i32 and I think the spec tests should pass?

abrown commented

I tried that but then the spec tests fail for other reasons:

expected a u8
     --> tests/spec_testsuite/proposals/simd/simd_lane.wast:493:52
      |
  493 |   (v8x16.shuffle 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14)
      |                                                    ^
And this is after I changed a bunch of stuff in the test to avoid further failures (@Honry, you may be interested in this): 

$ git diff proposals/simd/simd_lane.wast
diff --git a/proposals/simd/simd_lane.wast b/proposals/simd/simd_lane.wast
index 24b2d5a..b2a8a24 100644
--- a/proposals/simd/simd_lane.wast
+++ b/proposals/simd/simd_lane.wast
@@ -419,7 +419,7 @@
 (assert_invalid (module (func (result f32) (f32x4.extract_lane -1 (v128.const f32x4 0 0 0 0)))) "invalid lane index")
 (assert_invalid (module (func (result f32) (f32x4.extract_lane 4 (v128.const f32x4 0 0 0 0)))) "invalid lane index")
 (assert_invalid (module (func (result v128) (i8x16.replace_lane -1 (v128.const i8x16 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0) (i32.const 1)))) "invalid lane index")
-(assert_invalid (module (func (result v128) (i8x16.replace_lane 16 (v128.const ii8x16 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0) (i32.const 1)))) "invalid lane index")
+(assert_invalid (module (func (result v128) (i8x16.replace_lane 16 (v128.const i8x16 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0) (i32.const 1)))) "invalid lane index")
 (assert_invalid (module (func (result v128) (i16x8.replace_lane -1 (v128.const i16x8 0 0 0 0 0 0 0 0) (i32.const 1)))) "invalid lane index")
 (assert_invalid (module (func (result v128) (i16x8.replace_lane 8 (v128.const i16x8 0 0 0 0 0 0 0 0) (i32.const 1)))) "invalid lane index")
 (assert_invalid (module (func (result v128) (i32x4.replace_lane -1 (v128.const i32x4 0 0 0 0) (i32.const 1)))) "invalid lane index")
@@ -444,10 +444,10 @@
 (assert_invalid (module (func (result v128) (i16x8.replace_lane 8 (v128.const i8x16 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0) (i32.const 1)))) "invalid lane index")
 (assert_invalid (module (func (result v128) (i32x4.replace_lane 4 (v128.const i8x16 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0) (i32.const 1)))) "invalid lane index")
 (assert_invalid (module (func (result v128) (f32x4.replace_lane 4 (v128.const i8x16 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0) (i32.const 1)))) "invalid lane index")
-(assert_invalid (module (func (result i64) (i64x2.extract_lane 2 (v128.const i64x2 0 0 0 0)))) "invalid lane index")
-(assert_invalid (module (func (result f64) (f64x2.extract_lane 2 (v128.const f64x2 0 0 0 0)))) "invalid lane index")
-(assert_invalid (module (func (result v128) (i64x2.replace_lane 2 (v128.const i64x2 0 0 0 0) (i64.const 1)))) "invalid lane index")
-(assert_invalid (module (func (result v128) (f64x2.replace_lane 2 (v128.const f64x2 0 0 0 0) (f64.const 1.0)))) "invalid lane index")
+(assert_invalid (module (func (result i64) (i64x2.extract_lane 2 (v128.const i64x2 0 0)))) "invalid lane index")
+(assert_invalid (module (func (result f64) (f64x2.extract_lane 2 (v128.const f64x2 0 0)))) "invalid lane index")
+(assert_invalid (module (func (result v128) (i64x2.replace_lane 2 (v128.const i64x2 0 0) (i64.const 1)))) "invalid lane index")
+(assert_invalid (module (func (result v128) (f64x2.replace_lane 2 (v128.const f64x2 0 0) (f64.const 1.0)))) "invalid lane index")
 
 ;; Invalid parameters: required v128 but pass other types
 
@@ -854,4 +854,4 @@
   (v128.const i8x16 -16 -15 -14 -13 -12 -11 -10 -9 8 7 6 5 4 3 2 1))
 
 (assert_return (invoke "as-local_set-value-1" (v128.const i64x2 -1 -1)) (i64.const -1))
-(assert_return (invoke "as-global_set-value-3" (v128.const f64x2 0 0)(f64.const 3.14)) (v128.const f64x2 3.14 0))
\ No newline at end of file
+(assert_return (invoke "as-global_set-value-3" (v128.const f64x2 0 0)(f64.const 3.14)) (v128.const f64x2 3.14 0))

abrown commented

I guess this brings up a test design issue: are the *.wast spec tests supposed to only allow correct WAT inside an assert_invalid? Or do they allow incorrect WAT and then we must potentially parse each inner module separately?