wasmfx / specfx

WebAssembly specification, reference interpreter, and test suite.
https://webassembly.github.io/spec/
Other
7 stars 2 forks source link

fix parsing resume #53

Closed ahuoguo closed 4 days ago

ahuoguo commented 4 days ago

Fixes #52, resume_instr_instr is a single instruction so it should go to instr1

PS: I'm also a bit lost for which reference interpreter maintains the newest information. There's also https://github.com/WebAssembly/stack-switching

ahuoguo commented 4 days ago

Note that this doesn't fix https://github.com/wasmfx/wasmfx-tools/issues/96. The issue there is the reference interpreter rejects resume/resume_throw to be in tail position, an easier example is the following:

(module
  (type $task (;0;) (func))
  (type $cont (;1;) (cont $task))
  (type (;2;) (func (param i32)))
  (import "spectest" "print_i32" (func $print_i32 (;0;) (type 2)))
  (tag $yield (;0;) (type $task))
  (export "_start" (func $main))
  (start $main)
  (elem (;0;) declare func $task0)
  (func $task0 (;1;) (type $task)
    i32.const 1
    call $print_i32
  )
  (func $main (;2;) (type $task)
    ref.func $task0
    cont.new $cont
    resume $cont
  )
)
> ./wasm try.wast
try.wast:18.3-18.4: syntax error: unexpected token

This can be fixed by replacing resume_instr_handler_instr as follows:

resume_instr_handler_instr :
  | LPAR ON var var RPAR resume_instr_handler_instr
    { fun c -> let hs, es = $6 c in ($3 c tag, OnLabel ($4 c label)) :: hs, es }
  | LPAR ON var SWITCH RPAR resume_instr_handler_instr
    { fun c -> let hs, es = $6 c in ($3 c tag, OnSwitch) :: hs, es }
  | instr_list (* changed from `instr1`*)
    { fun c -> [], $1 c }

but it introduce A LOT OF shift/reduce conflicts

dhil commented 4 days ago

Thanks. I think this needs to be factored like try and catch for exceptions.

PS: I'm also a bit lost for which reference interpreter maintains the newest information. There's also https://github.com/WebAssembly/stack-switching

The upstream reference interpreter is https://github.com/WebAssembly/stack-switching. This reference interpreter is downstream and used for experimental features and research.

dhil commented 4 days ago

Thanks. I think this needs to be factored like try and catch for exceptions.

On reflection (or rather code inspection), I think we simply have to add a recursive instance of instr_list nonterminal in the production rule containing resume_instr_instr. It seems to pass your example. I will check the bug from wasm-tools next.

dhil commented 4 days ago

Thanks. I think this needs to be factored like try and catch for exceptions.

On reflection (or rather code inspection), I think we simply have to add a recursive instance of instr_list nonterminal in the production rule containing resume_instr_instr. It seems to pass your example. I will check the bug from wasm-tools next.

OK, so the proper fix is to push the instr_list nonterminal all the down to replace the instr1 in resume_instr_handler_instr. I can open a PR momentarily upstream with the fix.

dhil commented 4 days ago

I've opened the PR upstream https://github.com/WebAssembly/stack-switching/pull/100

ahuoguo commented 4 days ago

Thanks!