binaryen icon indicating copy to clipboard operation
binaryen copied to clipboard

Incorrect validation of `br_on_cast` and type difference computation

Open fitzgen opened this issue 6 months ago • 2 comments

Originally reported as https://github.com/WebAssembly/gc/issues/510

This test case should fail validation with type mismatch: expression has type (ref any) but expected eqref. FWIW, Firefox, wasmparser, and the reference interpreter all give that validation error, however binaryen incorrectly claims it is valid.

The br_on_cast should be typed [externref eqref] -> [externref (ref any)] because diff(anyref, arrayref) = (ref any), but it seems like binaryen is typing it as an identity function.

(module
  (rec
    (type (;0;) (sub (func (result externref eqref))))
    (type (;1;) (array (mut externref)))
  )
  (import "" "" (global (;0;) (mut f32)))
  (func (;0;) (type 0) (result externref eqref)
    ;; []
    block (type 0) (result externref eqref) ;; label = @1
      ;; []
      block ;; label = @2
        ;; []
        loop (result (ref null 0)) ;; label = @3
          ;; []
          ref.null i31
          ;; [i31ref]
          call 0
          ;; [i31ref externref eqref]
          br 2 (;@1;)
          ;; [i31ref externref eqref]
          global.get 0
          ;; [i31ref externref eqref f32]
          f32.ceil
          ;; [i31ref externref eqref f32]
          call 0
          ;; [i31ref externref eqref f32 externref eqref]
          call 2
          ;; [i31ref externref eqref f32 externref eqref externref eqref]
          br_on_cast 2 (;@1;) anyref arrayref
          ;; [i31ref externref eqref f32 externref eqref externref (ref any)]
          br_on_non_null 3 (;@0;) ;; <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< validation error should be here
          ref.is_null
          nop
          unreachable
        end
        unreachable
      end
      unreachable
    end
  )
  (func (;1;) (type 0) (result externref eqref)
    unreachable
  )
  (func (;2;) (type 0) (result externref eqref)
    unreachable
  )
  (memory (;0;) 15 18996)
)

fitzgen avatar Jan 19 '24 16:01 fitzgen

This is likely due to br_on_* not supporting a sent value in addition to the reference itself, which is a current limitation. No real-world code so far does that AFAIK so it has not been a priority to add (and due to it requiring multivalue it has downsides in our IR).

Separately it is odd that this does not fail to validate on the br_on_cast going to a multivalue block. That suggests we are also missing validation there. We only fuzz valid testcases so that was missed I supposed. Adding that validation should be simple.

kripken avatar Jan 19 '24 17:01 kripken

FWIW we do report a validation error when parsing this wat directly with --new-wat-parser due to the sent type of br_on_cast not matching the target block type. I assume the original repro was parsed from binary instead; perhaps our binary parser is doing something odd that masks the problem.

tlively avatar Jan 22 '24 21:01 tlively