Skip to content
This repository has been archived by the owner on May 11, 2020. It is now read-only.

disasm: fix incorrect stack depth for end/discard #52

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bkeroackdsc
Copy link
Contributor

@bkeroackdsc bkeroackdsc commented Jan 3, 2018

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

End/Discard ops were being misencoded with incorrect stack depths which caused stack underflows that manifested as slice index out of bounds runtime panics.
@sbinet
Copy link
Contributor

sbinet commented Jan 3, 2018

@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 sbinet requested a review from vibhavp January 3, 2018 23:51
@vibhavp
Copy link
Collaborator

vibhavp commented Jan 9, 2018

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.

@sbinet
Copy link
Contributor

sbinet commented May 25, 2018

ping @vibhavp ?

@jeff1010322
Copy link

@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.

@vibhavp
Copy link
Collaborator

vibhavp commented Aug 24, 2018

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

@jeff1010322
Copy link

jeff1010322 commented Aug 24, 2018

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
Copy link

jeff1010322 commented Aug 24, 2018

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?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants