Fix issues with stubbing WASI imports #185
Conversation
Specifically, without this patch WASIp2 imports where stubbed even if they are part of the target world, because of a bug in how the target world imports were checked. Additionally, many WASIp1 imports where unconditionally stubbed, making it impossible to use them after resuming a wizer snapshot. With this commit, they're only stubbed if the respective WASIp2 functionality is stubbed as well. Finally, ComponentizeJS now emits a warning for any imports that exist, but end up being stubbed. Without this, it's exceedingly difficult to debug why something doesn't work at runtime.
| @@ -174,22 +175,6 @@ fn target_world_requires_io(target_world_imports: &HashSet<String>) -> bool { | |||
|
|
|||
| const PREVIEW1: &str = "wasi_snapshot_preview1"; | |||
| fn stub_preview1(module: &mut Module) -> Result<()> { | |||
There was a problem hiding this comment.
The assumption here is effectively that none of StarlingMonkey calls are needed at runtime, so in a sense this code path binds to the StarlingMonkey internals.
This was before we had full support for per-subsystem stubbing. Preview1 as special here because it is implicit in the C++ bindings and pulled in automatically over the explicit preview2 host API.
Now that we have better subsystem management I agree we can integrate this into the main stubbing routines, based on the single assumption that the filesystem usage from core starlingmonkey is disabled always by default (at least until we implement an FS..!).
There was a problem hiding this comment.
For binding custom builtins, if we wanted to better support FS, happy to discuss this further too.
There was a problem hiding this comment.
Context for this PR is my work on debugging support in StarlingMonkey, which needs post-wizening access to env vars (to check if the debugger should be activated) and to socket APIs (to make debugging work at all.)
I think now that we have the subsystem management it makes sense to lean into it in this way, and allow custom builtins to make use of WASI APIs at their own pace, without a need to have changes applied to ComponentizeJS.
There was a problem hiding this comment.
if we wanted to better support FS, happy to discuss this further too.
I'm not sure we really need much else from ComponentizeJS, at least for now: as long as the stubs are only applied to things not in the target world, everything should work well
The warning added in bytecodealliance#185 turns out to be too noisy: there are too many situations in which an import exists, but should be stubbed. In particular once StarlingMonkey is updated to include bytecodealliance/StarlingMonkey#218, the warning would essentially be shown for every component, because in most cases, the target world won't contain the necessary interfaces, by design.
The warning added in #185 turns out to be too noisy: there are too many situations in which an import exists, but should be stubbed. In particular once StarlingMonkey is updated to include bytecodealliance/StarlingMonkey#218, the warning would essentially be shown for every component, because in most cases, the target world won't contain the necessary interfaces, by design.
Specifically, without this patch WASIp2 imports where stubbed even if they are part of the target world, because of a bug in how the target world imports were checked.
Additionally, many WASIp1 imports where unconditionally stubbed, making it impossible to use them after resuming a wizer snapshot. With this commit, they're only stubbed if the respective WASIp2 functionality is stubbed as well.
Finally, ComponentizeJS now emits a warning for any imports that exist, but end up being stubbed. Without this, it's exceedingly difficult to debug why something doesn't work at runtime.