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 borrow scope for store in WrappedCallable impl for WasmtimeFn. #366

Conversation

peterhuene
Copy link
Member

@peterhuene peterhuene commented Sep 24, 2019

This PR fixes the borrow scope of store in the WrappedCallable impl of
WasmTimeFn such that it does not remain borrowed across the call to wasmtime_call_trampoline. By limiting the scope of the borrow, the implementation can be reentered if an exported function calls an imported function, which in turn calls another exported function.

Fixes #365.

@sunfishcode
Copy link
Member

This looks good; would it be feasible to convert your example from #365 into a testcase? wasmtime-api/examples/gcd.rs may be useful as a starting point.

@peterhuene
Copy link
Member Author

peterhuene commented Sep 25, 2019

I didn't see formal tests for the API crate yet. Should I add a unit test or an test case as an example?

@sunfishcode
Copy link
Member

I think it makes sense to add a tests directory, and add this as a test there.

@peterhuene peterhuene changed the title Fix borrow scope for store in wasm_func_call. Fix borrow scope for store in WrappedCallable impl for WasmtimeFn. Sep 25, 2019
@peterhuene peterhuene force-pushed the fix-wasm-func-call-borrow-scope branch from 9a57d95 to 491466a Compare September 25, 2019 19:50
@peterhuene
Copy link
Member Author

@sunfishcode I've amended the commit with a test case.

@peterhuene
Copy link
Member Author

@sunfishcode @yurydelendik It appears that there's a calling convention mismatch for the import in the new test case on Windows. Is there something I should be doing in the Rust API or is this a current limitation?

@yurydelendik
Copy link
Contributor

It appears that there's a calling convention mismatch for the import in the new test case on Windows. Is there something I should be doing in the Rust API or is this a current limitation?

Let's disable tests on Windows, so it will not block landing of this PR -- I'll look into that calling convention problem shortly.

@peterhuene
Copy link
Member Author

peterhuene commented Sep 25, 2019

Should I file a new issue to track the calling convention mismatch panic?

@peterhuene
Copy link
Member Author

Filed as #378.

@peterhuene
Copy link
Member Author

cache::tests::test_write_read_cache from wasmtime-environ is failing on Windows and appears unrelated to these changes. Are there any known issues with this test?

@sunfishcode
Copy link
Member

@mrowqa Would you mind taking a look at this test failure?

This PR fixes the borrow scope of store in the `WrappedCallable` impl of
`WasmTimeFn` such that it does not remain borrowed across the call to
`wasmtime_call_trampoline`. By limiting the scope of the borrow, the
implementation can be reentered if an exported function calls an imported
function, which in turn calls another exported function.

Fixes bytecodealliance#365.
@peterhuene peterhuene force-pushed the fix-wasm-func-call-borrow-scope branch from 72ef81d to 66e29cf Compare September 26, 2019 18:30
@peterhuene
Copy link
Member Author

@mrowqa the cache::tests::test_write_read_cache failure is from this build: https://dev.azure.com/CraneStation/Wasmtime/_build/results?buildId=1064.

@mrowqa
Copy link
Contributor

mrowqa commented Sep 26, 2019

@peterhuene I can't reproduce the CI failure. I have updated the Rust version in the meantime, though. Since the test changed in the meantime, my suggestion was to rebase your PR onto master, but I can see you already did that when I was writing this comment 😃 . Other weird thing is the stack trace didn't make sense. It pointed to an empty line.

@peterhuene
Copy link
Member Author

@mrowqa thanks for looking into it. I'll keep an eye out in case it fails again.

@mrowqa
Copy link
Contributor

mrowqa commented Sep 26, 2019

Looks like everything works. The formatting problem is within lightbeam. I have already opened an PR there CraneStation/lightbeam#28.

@sunfishcode
Copy link
Member

Thanks!

@sunfishcode sunfishcode merged commit 4288f33 into bytecodealliance:master Sep 26, 2019
@peterhuene peterhuene deleted the fix-wasm-func-call-borrow-scope branch September 26, 2019 19:56
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.

Panic when reentering wasm_func_call from C API.
4 participants