c-api: Create RootScope where necessary
#8374
Conversation
This commit changes the `wasmtime_val_t::{from_val, to_val}` methods to
take a `RootScope` instead of any `AsContextMut`. This then required a
number of refactorings in callers to ensure that a `RootScope` was
created for any function that needed one. This is required to ensure
that GC references in the C API aren't forced to live for the entire
lifetime of the store.
This additionally added `*_unrooted` variants which do the same thing
but don't require `RootScope`. This was needed for when the C API calls
out to the embedder through a function call because a new `RootScope`
wouldn't work for return values (they're bound to a scope within the
closure when we want them to outlive the closure). In these situations
though we know a `RootScope` is already present at the entrypoint.
Closes bytecodealliance#8367
crates/c-api/src/val.rs
Outdated
| /// elsewhere on the stack. For example this is used when we call back out | ||
| /// to the embedder. In such a situation we know we previously entered with | ||
| /// some other call so the root scope is on the stack there. | ||
| pub fn from_val_unrooted(cx: impl AsContextMut, val: Val) -> wasmtime_val_t { |
There was a problem hiding this comment.
Maybe we can rename this from_val_unscoped or something? "unrooted" had me convinced that this was passing around ExternRef::to_raw results and stuff like that potentially across GCs which is not what's going on.
There was a problem hiding this comment.
Also, the wasmtime_val_t::from_val[_unrooted] constructors never create Rooteds so I don't think we actually need two variants of these ones? Its just creating a Val (which contains a Rooted and means we might be creating a Rooted ourselves) that needs the scoping.
There was a problem hiding this comment.
Good point about the naming, I'll switch to your suggestion.
I thought the same for from_val but I ended up realizing that APIs like wasmtime_global_get also will want a RootScope on them. That's only calling from_val but if we forgot a RootScope there the any value coming out of a global would forever-after be attached to the store (since the Rooted coming out has no other scope). That convinced me to leave the from_val{,_unscoped} distinction to continue to encourage the use of RootScope anywhere in the C API that might use it.
Subscribe to Label Actioncc @peterhuene
This issue or pull request has been labeled: "wasmtime:c-api"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
* c-api: Create `RootScope` where necessary
This commit changes the `wasmtime_val_t::{from_val, to_val}` methods to
take a `RootScope` instead of any `AsContextMut`. This then required a
number of refactorings in callers to ensure that a `RootScope` was
created for any function that needed one. This is required to ensure
that GC references in the C API aren't forced to live for the entire
lifetime of the store.
This additionally added `*_unrooted` variants which do the same thing
but don't require `RootScope`. This was needed for when the C API calls
out to the embedder through a function call because a new `RootScope`
wouldn't work for return values (they're bound to a scope within the
closure when we want them to outlive the closure). In these situations
though we know a `RootScope` is already present at the entrypoint.
Closes bytecodealliance#8367
* Review comments
* c-api: Create `RootScope` where necessary
This commit changes the `wasmtime_val_t::{from_val, to_val}` methods to
take a `RootScope` instead of any `AsContextMut`. This then required a
number of refactorings in callers to ensure that a `RootScope` was
created for any function that needed one. This is required to ensure
that GC references in the C API aren't forced to live for the entire
lifetime of the store.
This additionally added `*_unrooted` variants which do the same thing
but don't require `RootScope`. This was needed for when the C API calls
out to the embedder through a function call because a new `RootScope`
wouldn't work for return values (they're bound to a scope within the
closure when we want them to outlive the closure). In these situations
though we know a `RootScope` is already present at the entrypoint.
Closes #8367
* Review comments
This commit changes the
wasmtime_val_t::{from_val, to_val}methods to take aRootScopeinstead of anyAsContextMut. This then required a number of refactorings in callers to ensure that aRootScopewas created for any function that needed one. This is required to ensure that GC references in the C API aren't forced to live for the entire lifetime of the store.This additionally added
*_unrootedvariants which do the same thing but don't requireRootScope. This was needed for when the C API calls out to the embedder through a function call because a newRootScopewouldn't work for return values (they're bound to a scope within the closure when we want them to outlive the closure). In these situations though we know aRootScopeis already present at the entrypoint.Closes #8367