-
Notifications
You must be signed in to change notification settings - Fork 106
Refine recursion check to handle child import/export forwarding #589
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
base: main
Are you sure you want to change the base?
Conversation
dicej
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; thanks!
|
I updated the PR with a second commit to make the recursion check more conservative and even easier to optimize (see the new text in CanonicalABI.md as for why). As the original post says, we're not aware of any producer toolchains that can generate components that hit this case, so the update minimizes runtime complexity until we properly implement recursive reentrance. PTAL! |
This commit makes a few changes related to recursive reentrance checks, instance poisoning, etc.: - Implements the more restrictive lift/lower rules described in WebAssembly/component-model#589 such that a component instance may not lower a function lifted by one of its ancestors, nor vice-versa. Any such lower will result in a fused adapter which traps unconditionally, preventing guest-to-guest recursive reentrance without requiring data flow analysis. - Note that this required updating several WAST tests which were violating the new rule. - This is handled entirely in the `fact` module now; I've removed the `AlwaysTrap` case previously handled by `wasmtime-cranelift`. - Removes `FLAG_MAY_ENTER` from `InstanceFlags`. It is no longer needed for guest-to-guest calls due to the above, and for guest-to-host-to-guest calls we can rely on either `FLAG_NEEDS_POST_RETURN` for sync-lifted functions or the `GuestTask` call stack for async-lifted functions. - Adds a `StoreOpaque::trapped` field which is set when _any_ instance belonging to that store traps, at which point the entire store is considered poisoned, meaning no instance belonging to it may be entered. This prevents indeterminant concurrent task state left over from the trapping instance from leaking into other instances. Note that this does _not_ include code to push and pop `GuestTask` instances for guest-to-guest sync-to-sync calls, nor for host-to-guest calls using e.g. the synchronous `Func::call` API, so certain intrinsics which expect a `GuestTask` to be present such as `backpressure.inc` will still fail in such cases. I'll address that in a later PR. Fixes bytecodealliance#12128
This commit makes a few changes related to recursive reentrance checks, instance poisoning, etc.: - Implements the more restrictive lift/lower rules described in WebAssembly/component-model#589 such that a component instance may not lower a function lifted by one of its ancestors, nor vice-versa. Any such lower will result in a fused adapter which traps unconditionally, preventing guest-to-guest recursive reentrance without requiring data flow analysis. - Note that this required updating several WAST tests which were violating the new rule. - This is handled entirely in the `fact` module now; I've removed the `AlwaysTrap` case previously handled by `wasmtime-cranelift`. - Removes `FLAG_MAY_ENTER` from `InstanceFlags`. It is no longer needed for guest-to-guest calls due to the above, and for guest-to-host-to-guest calls we can rely on either `FLAG_NEEDS_POST_RETURN` for sync-lifted functions or the `GuestTask` call stack for async-lifted functions. - Adds a `StoreOpaque::trapped` field which is set when _any_ instance belonging to that store traps, at which point the entire store is considered poisoned, meaning no instance belonging to it may be entered. This prevents indeterminant concurrent task state left over from the trapping instance from leaking into other instances. Note that this does _not_ include code to push and pop `GuestTask` instances for guest-to-guest sync-to-sync calls, nor for host-to-guest calls using e.g. the synchronous `Func::call` API, so certain intrinsics which expect a `GuestTask` to be present such as `backpressure.inc` will still fail in such cases. I'll address that in a later PR. Fixes bytecodealliance#12128
This commit makes a few changes related to recursive reentrance checks, instance poisoning, etc.: - Implements the more restrictive lift/lower rules described in WebAssembly/component-model#589 such that a component instance may not lower a function lifted by one of its ancestors, nor vice-versa. Any such lower will result in a fused adapter which traps unconditionally, preventing guest-to-guest recursive reentrance without requiring data flow analysis. - Note that this required updating several WAST tests which were violating the new rule. - This is handled entirely in the `fact` module now; I've removed the `AlwaysTrap` case previously handled by `wasmtime-cranelift`. - Removes `FLAG_MAY_ENTER` from `InstanceFlags`. It is no longer needed for guest-to-guest calls due to the above, and for guest-to-host-to-guest calls we can rely on either `FLAG_NEEDS_POST_RETURN` for sync-lifted functions or the `GuestTask` call stack for async-lifted functions. - Adds a `StoreOpaque::trapped` field which is set when _any_ instance belonging to that store traps, at which point the entire store is considered poisoned, meaning no instance belonging to it may be entered. This prevents indeterminant concurrent task state left over from the trapping instance from leaking into other instances. Note that this does _not_ include code to push and pop `GuestTask` instances for guest-to-guest sync-to-sync calls, nor for host-to-guest calls using e.g. the synchronous `Func::call` API, so certain intrinsics which expect a `GuestTask` to be present such as `backpressure.inc` will still fail in such cases. I'll address that in a later PR. Fixes bytecodealliance#12128
This commit makes a few changes related to recursive reentrance checks, instance poisoning, etc.: - Implements the more restrictive lift/lower rules described in WebAssembly/component-model#589 such that a component instance may not lower a function lifted by one of its ancestors, nor vice-versa. Any such lower will result in a fused adapter which traps unconditionally, preventing guest-to-guest recursive reentrance without requiring data flow analysis. - Note that this required updating several WAST tests which were violating the new rule. - This is handled entirely in the `fact` module now; I've removed the `AlwaysTrap` case previously handled by `wasmtime-cranelift`. - Removes `FLAG_MAY_ENTER` from `InstanceFlags`. It is no longer needed for guest-to-guest calls due to the above, and for guest-to-host-to-guest calls we can rely on either `FLAG_NEEDS_POST_RETURN` for sync-lifted functions or the `GuestTask` call stack for async-lifted functions. - Adds a `StoreOpaque::trapped` field which is set when _any_ instance belonging to that store traps, at which point the entire store is considered poisoned, meaning no instance belonging to it may be entered. This prevents indeterminant concurrent task state left over from the trapping instance from leaking into other instances. Note that this does _not_ include code to push and pop `GuestTask` instances for guest-to-guest sync-to-sync calls, nor for host-to-guest calls using e.g. the synchronous `Func::call` API, so certain intrinsics which expect a `GuestTask` to be present such as `backpressure.inc` will still fail in such cases. I'll address that in a later PR. Fixes bytecodealliance#12128
This commit makes a few changes related to recursive reentrance checks, instance poisoning, etc.: - Implements the more restrictive lift/lower rules described in WebAssembly/component-model#589 such that a component instance may not lower a function lifted by one of its ancestors, nor vice-versa. Any such lower will result in a fused adapter which traps unconditionally, preventing guest-to-guest recursive reentrance without requiring data flow analysis. - Note that this required updating several WAST tests which were violating the new rule. - This is handled entirely in the `fact` module now; I've removed the `AlwaysTrap` case previously handled by `wasmtime-cranelift`. - Removes `FLAG_MAY_ENTER` from `InstanceFlags`. It is no longer needed for guest-to-guest calls due to the above, and for guest-to-host-to-guest calls we can rely on either `FLAG_NEEDS_POST_RETURN` for sync-lifted functions or the `GuestTask` call stack for async-lifted functions. - Adds a `StoreOpaque::trapped` field which is set when _any_ instance belonging to that store traps, at which point the entire store is considered poisoned, meaning no instance belonging to it may be entered. This prevents indeterminant concurrent task state left over from the trapping instance from leaking into other instances. Note that this does _not_ include code to push and pop `GuestTask` instances for guest-to-guest sync-to-sync calls, nor for host-to-guest calls using e.g. the synchronous `Func::call` API, so certain intrinsics which expect a `GuestTask` to be present such as `backpressure.inc` will still fail in such cases. I'll address that in a later PR. Also note that I made a small change to `wasmtime-wit-bindgen`, adding a `Send` bound on the `T` type parameter for `store | async` functions. This allowed me to recursively call `{Typed}Func::call_concurrent` from inside a host function, and it doesn't have any downsides AFAICT. Fixes bytecodealliance#12128
This commit makes a few changes related to recursive reentrance checks, instance poisoning, etc.: - Implements the more restrictive lift/lower rules described in WebAssembly/component-model#589 such that a component instance may not lower a function lifted by one of its ancestors, nor vice-versa. Any such lower will result in a fused adapter which traps unconditionally, preventing guest-to-guest recursive reentrance without requiring data flow analysis. - Note that this required updating several WAST tests which were violating the new rule, including some in the `tests/component-model` Git submodule, which I've updated. - This is handled entirely in the `fact` module now; I've removed the `AlwaysTrap` case previously handled by `wasmtime-cranelift`. - Removes `FLAG_MAY_ENTER` from `InstanceFlags`. It is no longer needed for guest-to-guest calls due to the above, and for guest-to-host-to-guest calls we can rely on either `FLAG_NEEDS_POST_RETURN` for sync-lifted functions or the `GuestTask` call stack for async-lifted functions. - Adds a `StoreOpaque::trapped` field which is set when _any_ instance belonging to that store traps, at which point the entire store is considered poisoned, meaning no instance belonging to it may be entered. This prevents indeterminant concurrent task state left over from the trapping instance from leaking into other instances. Note that this does _not_ include code to push and pop `GuestTask` instances for guest-to-guest sync-to-sync calls, nor for host-to-guest calls using e.g. the synchronous `Func::call` API, so certain intrinsics which expect a `GuestTask` to be present such as `backpressure.inc` will still fail in such cases. I'll address that in a later PR. Also note that I made a small change to `wasmtime-wit-bindgen`, adding a `Send` bound on the `T` type parameter for `store | async` functions. This allowed me to recursively call `{Typed}Func::call_concurrent` from inside a host function, and it doesn't have any downsides AFAICT. Fixes bytecodealliance#12128
The current
trap_if_on_stackrecursion check is meant to trap if a single component instance is called recursively (as this can lead to unpredictable deadlock due to backpressure as well as some other problems enumerated in this PR). (There's a TODO to add a way for components to opt-in to allowing recursive reentrance in a way that avoids both the trap and the deadlock.) However, the current simplistic Python definition misses the case where a single parent component is reentered via its child components (that are re-exported or receive forwarded imports). The reason is that, when calling into a component instance A, one must also consider the call to be entering any parent of A that is not also a parent of the caller's component. This PR adds aparentfield toComponentInstanceand then uses it to fix recursion check (which is now calledcall_is_recursive).A nice side-effect of this change (which actually helped point out the problem) is that it ensures that component-to-component recursion checks can be eliminated at compile time in the vast majority of cases where parent components do not also contain core module instances (which allow recursion via
call_indirect), leaving only the pre-existing host-to-component recursion checks (which in most cases should be a single "ah, the stack is empty, nothing to do here" branch).This change also shouldn't break any existing code since it only impacts obscure corner cases that we don't think producer toolchains can even emit atm.