Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WasmFX reference interpreter: Errorenous treatment of non-final types #62

Closed
frank-emrich opened this issue Jul 24, 2024 · 3 comments
Closed
Assignees

Comments

@frank-emrich
Copy link
Collaborator

There seems to be a validation problem when WasmFX instructions non-final types.

Consider the following program:

(module
  (type $ft1 (func (param i32)))
  (type $ct1 (sub (cont $ft1)))

  (type $ft0 (func))
  (type $ct0 (sub (cont $ft0)))

  (func $test (param $x (ref $ct1))
    (i32.const 123)
    (local.get $x)
    (cont.bind $ct1 $ct0)
    (drop)
  )
)

This should be accepted, but fails with the following error message:

invalid module: type mismatch: instruction requires [i32 (ref null (cont 0))] but stack has [i32 (ref 1)]

Removing the "sub" from the definitions of $ct0 and $ct1 makes the problem go away.

The culprit seems to be our usage of heap_type_of_str_type in valid.ml. This conversion function was introduced by us and is only used for the validation of WasmFX instructions. It turns a structural type into a heap type by turning it into a final subtype with an empty list of supertypes, and then puts that subtype definition into a singleton group of recursive types.

I suspect that this also affects code combining recursive types and WasmFX instruction, as it throws away information about the members of recursive type groups. We can probably get rid of heap_type_of_str_type altogether and obtain the desired heap types in a different way.

@dhil
Copy link
Member

dhil commented Jul 24, 2024

Thanks. I think before fixing this issue, we should make sure the reference interpreter is in sync with the wasm-3.0+exn on WebAssembly/spec.

@rossberg
Copy link
Member

rossberg commented Jul 24, 2024

I agree, that function seems wrong and should never be used. Instead of heap_type_of_str_type (FuncT/ContT t), these rules should use VarHT (StatX x), where x is the type index from which the structural type was expanded before.

Note that the wasm-3.0+exn branch is no longer needed and now subsumed by wasm-3.0.

PS: I think syncing with wasm-3.0 is somewhat independent of this particular issue.

@dhil
Copy link
Member

dhil commented Aug 9, 2024

Fixed by #72.

@dhil dhil closed this as completed Aug 9, 2024
dhil pushed a commit to dhil/wasm-stack-switching that referenced this issue Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants