Stream: git-wasmtime

Topic: wasmtime / PR #8374 c-api: Create `RootScope` where neces...

Wasmtime GitHub notifications bot (Apr 15 2024 at 21:26):

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 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

<!--
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.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Apr 15 2024 at 21:26):

alexcrichton requested fitzgen for a review on PR #8374.

Wasmtime GitHub notifications bot (Apr 15 2024 at 21:26):

alexcrichton requested wasmtime-core-reviewers for a review on PR #8374.

Wasmtime GitHub notifications bot (Apr 15 2024 at 21:41):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 15 2024 at 21:41):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 15 2024 at 21:41):

fitzgen created PR review 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.

Wasmtime GitHub notifications bot (Apr 15 2024 at 21:41):

fitzgen created PR review 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.

Wasmtime GitHub notifications bot (Apr 15 2024 at 21:44):

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>

Wasmtime GitHub notifications bot (Apr 15 2024 at 22:04):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 15 2024 at 22:04):

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 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.

Wasmtime GitHub notifications bot (Apr 15 2024 at 22:05):

alexcrichton updated PR #8374.

Wasmtime GitHub notifications bot (Apr 15 2024 at 22:05):

alexcrichton has enabled auto merge for PR #8374.

Wasmtime GitHub notifications bot (Apr 15 2024 at 22:43):

alexcrichton merged PR #8374.

Last updated: Nov 22 2024 at 16:03 UTC