wagon icon indicating copy to clipboard operation
wagon copied to clipboard

disasm: fix incorrect stack depth for end/discard

Open bkeroackdsc opened this issue 6 years ago • 7 comments

End/Discard ops were being misencoded with incorrect stack depths which caused stack underflows that manifested as vm.ctx.stack slice index out of bounds runtime panics.

Test case wasm (execute the exported function _ZN4enol4InitEv)

I'm not 100% familiar with the code in question, so please check if this is the correct fix but my test code runs correctly with this change applied.


This change is Reviewable

bkeroackdsc avatar Jan 03 '18 17:01 bkeroackdsc

@bkeroackdsc thanks for taking a stab at it! unfortunately, it seems your change brok' a few tests (the non-spec ones for exec/testdata seem to be ok, but the exec/testdata/spec do not)

I must admit, I am also a bit lagging with the complete understanding of this piece of code. @vibhavp ? do you have any input?

sbinet avatar Jan 03 '18 23:01 sbinet

Sorry for not replying, as I've been a little busy with classes. I'll try my best to address this issue in the coming weeks.

If you take a look at the comment above your change, the reason why we subtract 0 is to get the parent block of the target branch, which is what we want to unwind to.

vibhavp avatar Jan 09 '18 18:01 vibhavp

ping @vibhavp ?

sbinet avatar May 25 '18 06:05 sbinet

@sbinet and @vibhavp any update on this?

I am trying to run a function in a wasm file that looks like this in wat:

(func $transfer (export "transfer") (type $t4) (param $p0 i32) (param $p1 i32) (param $p2 i32)
    (local $l0 i32) (local $l1 i32)
    block $B0
      get_local $p0
      call $load
      tee_local $l0
      get_local $p2
      i32.lt_s
      br_if $B0
      get_local $p1
      call $load
      set_local $l1
      get_local $p0
      get_local $l0
      get_local $p2
      i32.sub
      call $store
      get_local $p1
      get_local $l1
      get_local $p2
      i32.add
      call $store
    end)

The disassemble code tries to do an OpDiscard at the end with a stack of length 0 and gets a "slice bounds out of range", but making the change in this pull request fixed it for me.

jeff1010322 avatar Aug 23 '18 21:08 jeff1010322

Does the PR still break the tests Sebastian was talking about?

vibhavp avatar Aug 24 '18 04:08 vibhavp

Yes it looks like it still fails some of the exec/TestSpec/testdata/spec tests:

--- FAIL: TestSpec (0.01s)
    --- PASS: TestSpec/testdata/spec/unreachable.wasm (0.00s)
    --- PASS: TestSpec/testdata/spec/traps_int_rem.wasm (0.00s)
    --- PASS: TestSpec/testdata/spec/traps_int_div.wasm (0.00s)
    --- PASS: TestSpec/testdata/spec/forward.wasm (0.00s)
    --- PASS: TestSpec/testdata/spec/traps_mem.wasm (0.00s)
    --- PASS: TestSpec/testdata/spec/address.wasm (0.00s)
    --- PASS: TestSpec/testdata/spec/switch.wasm (0.01s)
    --- PASS: TestSpec/testdata/spec/globals.wasm (0.00s)
    --- PASS: TestSpec/testdata/spec/break-drop.wasm (0.00s)
    --- PASS: TestSpec/testdata/spec/tee_local.wasm (0.01s)
    --- PASS: TestSpec/testdata/spec/unwind.wasm (0.01s)
    --- PASS: TestSpec/testdata/spec/get_local.wasm (0.00s)
    --- PASS: TestSpec/testdata/spec/memory_redundancy.wasm (0.00s)
    --- PASS: TestSpec/testdata/spec/nop.wasm (0.01s)
    --- PASS: TestSpec/testdata/spec/resizing.wasm (0.00s)
    --- FAIL: TestSpec/testdata/spec/br_if.wasm (0.01s)
    	exec_test.go:338: /Users/jeffsimpson/projects/go/src/github.com/go-interpreter/wagon/exec/testdata/spec/br_if.wasm, nested-br_table-value([i32:0]) (16): unexpected return value: got=uint32(8), want=uint32(5) (i32:5)
    --- PASS: TestSpec/testdata/spec/select.wasm (0.00s)
    --- FAIL: TestSpec/testdata/spec/loop.wasm (0.01s)
    	exec_test.go:273: /Users/jeffsimpson/projects/go/src/github.com/go-interpreter/wagon/exec/testdata/spec/loop.wasm: disasm: stack underflow
    --- PASS: TestSpec/testdata/spec/fac.wasm (0.00s)
    --- FAIL: TestSpec/testdata/spec/if.wasm (0.01s)
    	exec_test.go:273: /Users/jeffsimpson/projects/go/src/github.com/go-interpreter/wagon/exec/testdata/spec/if.wasm: disasm: stack underflow
    --- PASS: TestSpec/testdata/spec/return.wasm (0.01s)
    --- PASS: TestSpec/testdata/spec/endianness.wasm (0.01s)
    --- PASS: TestSpec/testdata/spec/call_indirect.wasm (0.02s)
    --- FAIL: TestSpec/testdata/spec/block.wasm (0.01s)
    	exec_test.go:273: /Users/jeffsimpson/projects/go/src/github.com/go-interpreter/wagon/exec/testdata/spec/block.wasm: disasm: stack underflow
    --- PASS: TestSpec/testdata/spec/br.wasm (0.01s)
    --- PASS: TestSpec/testdata/spec/br_table.wasm (0.15s)

I haven't has a chance yet to dig into why it fails these tests or what a true solution to this should be.

jeff1010322 avatar Aug 24 '18 13:08 jeff1010322

After further investigation I found that the problem was in my FunctionIndexSpace for 2 native functions that I added. I was using the wrong signature ParamTypes (ValueTypeI64 instead of ValueTypeI32) compared to what the reflected function was expecting.

Once I made the fix everything appears to be working as expected without including this pull request.

Also using the provided wagon-test.wasm I was able to execute the exported function with no problem: _ZN4enol4InitEv() i32 => 0 (uint32)

@bkeroackdsc If this is still giving you an error could you provide some of the test code you are using? Otherwise, do you think this pull request be closed?

jeff1010322 avatar Aug 24 '18 18:08 jeff1010322