alexcrichton opened PR #8374 from alexcrichton:fix-c-api-and-gc
to bytecodealliance:main
:
This commit changes the
wasmtime_val_t::{from_val, to_val}
methods to take aRootScope
instead of anyAsContextMut
. This then required a number of refactorings in callers to ensure that aRootScope
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 requireRootScope
. This was needed for when the C API calls out to the embedder through a function call because a newRootScope
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 aRootScope
is already present at the entrypoint.Closes #8367
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
alexcrichton requested fitzgen for a review on PR #8374.
alexcrichton requested wasmtime-core-reviewers for a review on PR #8374.
fitzgen submitted PR review.
fitzgen submitted PR review.
fitzgen created PR review comment:
Also, the
wasmtime_val_t::from_val[_unrooted]
constructors never createRooted
s so I don't think we actually need two variants of these ones? Its just creating aVal
(which contains aRooted
and means we might be creating aRooted
ourselves) that needs the scoping.
fitzgen created PR review comment:
Maybe we can rename this
from_val_unscoped
or something? "unrooted" had me convinced that this was passing aroundExternRef::to_raw
results and stuff like that potentially across GCs which is not what's going on.
github-actions[bot] commented on PR #8374:
Subscribe to Label Action
cc @peterhuene
<details>
This issue or pull request has been labeled: "wasmtime:c-api"Thus the following users have been cc'd because of the following labels:
- peterhuene: wasmtime:c-api
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
alexcrichton submitted PR review.
alexcrichton created PR review 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 likewasmtime_global_get
also will want aRootScope
on them. That's only callingfrom_val
but if we forgot aRootScope
there the any value coming out of a global would forever-after be attached to the store (since theRooted
coming out has no other scope). That convinced me to leave thefrom_val{,_unscoped}
distinction to continue to encourage the use ofRootScope
anywhere in the C API that might use it.
alexcrichton updated PR #8374.
alexcrichton has enabled auto merge for PR #8374.
alexcrichton merged PR #8374.
Last updated: Dec 23 2024 at 12:05 UTC