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

Fix table index bug #21

Merged
merged 3 commits into from
Mar 11, 2021
Merged

Fix table index bug #21

merged 3 commits into from
Mar 11, 2021

Conversation

Gaboose
Copy link
Contributor

@Gaboose Gaboose commented Mar 11, 2021

Hi @mathetake! First of all, nice work with this project!

I tried running a rust-compiled hello world program with the wasm32-wasi target, started adding the required methods into the wasi package, and was pretty excited to get it working, but then kept getting a function signature mismatch panic from the callIndirect function in vm_call.go. It took me quite a while to debug, and I got to understand the wasm binary format way better because of it, but finally found the culprit:

There's a classic bug in the code that populates the table index space. This loop:

for i, b := range elem.Init {
	table[i+offset] = &b
}

Takes the pointer of the temporary variable, which is reused on every iteration. The fix is just to use table[i+offset] = &elem.Init[i].

Hope you see this pull request, and merge it! I want to contribute some more code to the wasi package afterwards.

Copy link
Member

@mathetake mathetake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Gaboose thank you so much! Actually I have been aware of this Go-newbie bug since #16 is submitted, but never got to have a time to fix til now.

LGTM modulo some nits. Thank you!

@mathetake mathetake merged commit fcd295b into tetratelabs:master Mar 11, 2021
@Gaboose Gaboose mentioned this pull request Mar 18, 2021
@Gaboose Gaboose deleted the fix-table-index-bug branch March 21, 2021 14:24
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

Successfully merging this pull request may close these issues.

2 participants