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

wasm: allocate module.TableIndexSpace when importing table from resolver #84

Closed
wants to merge 1 commit into from

Conversation

duanbing
Copy link

@duanbing duanbing commented Aug 22, 2018

When I run this case math, I got an error as below:

panic: runtime error: index out of range

goroutine 1 [running]:
github.com/go-interpreter/wagon/wasm.(*Module).resolveImports(0xc4200a6120, 0x115b720, 0x1, 0xc4200842c0)
	/Users/XXXX/Project/Go/src/github.com/go-interpreter/wagon/wasm/imports.go:187 +0xcd2
github.com/go-interpreter/wagon/wasm.ReadModule(0x116fd00, 0xc42007c6c0, 0x115b720, 0x13e, 0x33e, 0x0)
	/Users/XXXX/Project/Go/src/github.com/go-interpreter/wagon/wasm/module.go:143 +0x2a8
main.main()
	/Users/XXXX/Project/Go/src/github.com/duanbing/wasm-test/go/math.go:33 +0xf5
exit status 2

After tracing the source code, I found module.TableIndexSpace in file wasm/imports.go line 187, didn't get allocated.

So I went back to function DecodeModule in ReadModule to figure out whether module.TableIndexSpace is allocated, or m.Table is allocated.

In ReadModule, we firstly DecodeModule math.wasm, coz I don't declare table, so m.TableIndexSpace is not be allocated, but m.LinearMemoryIndexSpace is. Then go to resolveImports, travel module.Import.Entries, which is Import Section of math.wasm, and use Export Section from env.wasm to fill in those sections. Unfortunately, when initializing m.TableIndexSpace, we got panic.

So, this is all I caught. I have no idea if it's a bug, or my env.wasm is not written correctly.

@codecov-io
Copy link

codecov-io commented Aug 22, 2018

Codecov Report

Merging #84 into master will decrease coverage by 0.04%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #84      +/-   ##
==========================================
- Coverage   67.52%   67.48%   -0.05%     
==========================================
  Files          32       32              
  Lines        2987     2989       +2     
==========================================
  Hits         2017     2017              
- Misses        736      738       +2     
  Partials      234      234
Impacted Files Coverage Δ
wasm/imports.go 19.76% <0%> (-0.48%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 876a973...5c56cc7. Read the comment docs.

@sbinet
Copy link
Contributor

sbinet commented Aug 22, 2018

Looks good.

2 things:

  • could you add a test that would fail w/o this PR?
  • could you reword your commit message so its header looks like:
wasm: allocate module.TableIndexSpace when importing table from resolver

thanks.

@duanbing duanbing changed the title Allocate module.TableIndexSpace when importing table from resolver wasm: allocate module.TableIndexSpace when importing table from resolver Aug 23, 2018
@duanbing
Copy link
Author

duanbing commented Aug 23, 2018

I had changed the commit message header.

For the exception test, I use an import instruction. AFAIK, there is no import unit test except for some examples in vm_example_test, but I don't think this PR is an example. And All tests in testdata/modules.json seems being designed for the single module. Do you have any suggestions for the test's placement?

@duanbing duanbing closed this Apr 10, 2020
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.

None yet

3 participants