🎬 Add explicit API to run start functions; don't run it implicitly #506
Conversation
Previously, the start function would be implicitly run upon instance creation or resetting. Since the environment can be different at instantiation time to when export functions are typically called (missing embedder contexts, etc), this has tripped people up. It can also just be surprising that any code runs at all at those points, which combined with the inherently confusing nature of Wasm start functions just led to too many messes. `Instance::run_start()` is now part of the public API, and will run the start function if it is present in that instance's Wasm module. It does nothing if there is no start function. `Instance::run()` will now return `Err(Error::InstanceNeedsStart)` if the start function is present but hasn't been run since the instance was created or reset. This commit bumps the development version to `0.7.0-dev`, because while the API changes are semver-compatible, the semantics are not.
| /// This is `true` if and only if this handle was referenced through the start section. | ||
| /// | ||
| /// The same function may be referenced via other contexts, and will appear with `false` in this | ||
| /// field in those cases. | ||
| pub is_start_func: bool, |
There was a problem hiding this comment.
This is a little weird; I could be convinced this belongs in lucet-runtime-internals only, but since FunctionHandle is only used downstream of lucet-module it seemed okay.
| /// | ||
| /// [run]: struct.Instance.html#method.run | ||
| /// [start]: https://webassembly.github.io/spec/core/syntax/modules.html#syntax-start | ||
| pub fn run_start(&mut self) -> Result<(), Error> { |
There was a problem hiding this comment.
should run_start return an error if it has already been run and the instance not reset? I don't think it's terribly likely to have been calling the start function manually before, so accidentally rerunning it was unlikely, but double-initialization seems more likely with this as public API.
There was a problem hiding this comment.
Yeah, I actually was just thinking this. Good call
There was a problem hiding this comment.
Not a blocking concern, just a question that I'm curious about: would it be feasible to return an error if any exports are called on an instance before its start function has run?
There was a problem hiding this comment.
Done in 21a8ff3 ✨
There was a problem hiding this comment.
Not a blocking concern, just a question that I'm curious about: would it be feasible to return an error if any exports are called on an instance before its start function has run?
Aaa, race condition on the comments, sorry. This is a good idea; I will add it while my brain is in this corner of the project.
There was a problem hiding this comment.
@sunfishcode your suggestion is implemented in 50f808f. Thank you!
There was a problem hiding this comment.
If I read the patch right, it's issuing an error if the start function calls any imports. My question was about issuing errors if any exported function in a module is called before the module's start function is called, which isn't normally possible.
There was a problem hiding this comment.
Well, I definitely read that wrong, whoops. Yes, an error is returned if you try to call any other entrypoints before running the start function, if it exists.
Should I keep the terminate-on-import-function behavior? Now that I'm looking at the spec again I can't actually figure out whether import functions can be called in the start section.
There was a problem hiding this comment.
Imports can be called from the start function.
There was a problem hiding this comment.
Where's my dunce cap emoji? Reverted in 1e84d31
Thank you for the suggestion, @sunfishcode!
`test_ex` doesn't do much good if the non-exclusive tests aren't trying to take the read lock 😬
|
There is now a fair amount of whitespace diff in the
|
I was really tired yesterday, and misinterpreted @sunfishcode's comment 🙃
This also separates instantiation from initialization in a manner similar to bytecodealliance/lucet#506.
This also separates instantiation from initialization in a manner similar to bytecodealliance/lucet#506.
This also separates instantiation from initialization in a manner similar to bytecodealliance/lucet#506.
This also separates instantiation from initialization in a manner similar to bytecodealliance/lucet#506.
* Reactor support. This implements the new WASI ABI described here: https://github.com/WebAssembly/WASI/blob/master/design/application-abi.md It adds APIs to `Instance` and `Linker` with support for running WASI programs, and also simplifies the process of instantiating WASI API modules. This currently only includes Rust API support. * Add comments and fix a typo in a comment. * Fix a rustdoc warning. * Tidy an unneeded `mut`. * Factor out instance initialization with `NewInstance`. This also separates instantiation from initialization in a manner similar to bytecodealliance/lucet#506. * Update fuzzing oracles for the API changes. * Remove `wasi_linker` and clarify that Commands/Reactors aren't connected to WASI. * Move Command/Reactor semantics into the Linker. * C API support. * Fix fuzzer build. * Update usage syntax from "::" to "=". * Remove `NewInstance` and `start()`. * Elaborate on Commands and Reactors and add a spec link. * Add more comments. * Fix wat syntax. * Fix wat. * Use the `Debug` formatter to format an anyhow::Error. * Fix wat.
Previously, the start function would be implicitly run upon instance creation or resetting. Since the environment can be different at instantiation time to when export functions are typically called (missing embedder contexts, etc), this has tripped people up. It can also just be surprising that any code runs at all at those points, which combined with the inherently confusing nature of Wasm start functions just led to too many messes.
Instance::run_start()is now part of the public API, and will run the start function if it is present in that instance's Wasm module. It does nothing if there is no start function.Instance::run()will now returnErr(Error::InstanceNeedsStart)if the start function is present but hasn't been run since the instance was created or reset.Instance::run_start()will now return an error if run on an instance more than once without an interveningInstance::reset().If a start function attempts to call a
#[lucet_hostcall]-decorated import function, it will be terminated.This commit bumps the development version to
0.7.0-dev, because while the API changes are semver-compatible, the semantics are not.