wasmtime / PR #11020 Initial component model and GC suppo... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #11020 Initial component model and GC suppo...

Wasmtime GitHub notifications bot (Jun 11 2025 at 23:35):

fitzgen requested alexcrichton for a review on PR #11020.

Wasmtime GitHub notifications bot (Jun 11 2025 at 23:35):

fitzgen opened PR #11020 from fitzgen:cm-gc-initial-fact-integration to bytecodealliance:main:

This lays down the initial infrastructure for support for GC in our fused adapters for the component model. We keep track of whether each lifted/lowered function wants args/results as GC values or in linear memory. We additionally plumb through the core function types of the functions being lifted and lowered for (eventual) use with GC adapters.

Ultimately, this commit is enough to fuse together lifted and lowered functions where one or both are using the GC variant of the canonical ABI. Attempting to actually pass arguments will hit todo!()s. The work of implementing those todo!()s is left to future commits.

Wasmtime GitHub notifications bot (Jun 11 2025 at 23:37):

fitzgen created PR review comment:

Marking this PR as a draft because I am pretty unsure about this method. It feels like there has to be a better way? Also I am not sure how to grab the type for imports or named exports.

cc @alexcrichton

Wasmtime GitHub notifications bot (Jun 11 2025 at 23:37):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jun 12 2025 at 15:14):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 12 2025 at 15:14):

alexcrichton created PR review comment:

s/allow/expect/

(that way you're forced to delete this once it's used)

Wasmtime GitHub notifications bot (Jun 12 2025 at 15:14):

alexcrichton created PR review comment:

I'd recommend renaming this to core_ty_of_func_def and panicking internally if it's not a function (e.g. InstanceFlags would panic saying "not a function").

Either that or I might recommend replacing the one caller of this function (right?), AliasExportFunc, by inlining the definition of core_def_of_module_instance_export into that location. The core_def_of_module_instance_export has all the type information available to it and might be able to simplify this as you wouldn't need to match-then-match of sorts.

Also I am not sure how to grab the type for imports or named exports.

Named exports aren't actually possible so you can ignore those. By inlining core_def_of_module_instance_export into AliasExportFunc that should become apparent as the only constructor of ExportItem::Name is with imports. For imports you'll also have the module type available and you can look up the named export of the module type and that'll be the type information you want.

Wasmtime GitHub notifications bot (Jun 12 2025 at 15:14):

alexcrichton created PR review comment:

Question on this, would it be possible to plumb the type information around to avoid ever needing to reimplement lowering here? In that if we always had a type available could we always run the case of the Gc case here?

Wasmtime GitHub notifications bot (Jun 12 2025 at 15:14):

alexcrichton created PR review comment:

Possible alternative to this, and a number of other locations, could Gc be empty? The type of a function is something that's always known regardless of whether it's gc or linear memory, so could the type be adjacent to the "function item" and Gc here be empty? That way in the gc case there's still always a type available but it would also be available in the linear memory case too in case it's needed. (e.g. elsewhere when calculating the lowered signature we could avoid reimplementing algorithms in wasmtime I think?)

Wasmtime GitHub notifications bot (Jun 13 2025 at 16:29):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jun 13 2025 at 16:29):

fitzgen created PR review comment:

You mean plumb in the wasmparser types? I can't think of any reason that wouldn't be possible in theory, but I don't really feel comfortable enough with this corner of the code base to actually do that myself with confidence yet. FWIW, the GC signature is just the core signature that we did already plumb through, so this change isn't making the problem any worse, and is more similar to where we would like to eventually end up than the existing code is.

Wasmtime GitHub notifications bot (Jun 13 2025 at 16:37):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 13 2025 at 16:37):

alexcrichton created PR review comment:

Nah not wasmparser::types::Types, but ModuleInternedTypeIndex. Basically how the GC branch has the type available to it, no lowering necessary, so there's not really any reason we can't also carry along the type index in the linear memory case, removing the need for lowering.

Wasmtime GitHub notifications bot (Jun 13 2025 at 18:20):

fitzgen updated PR #11020.

Wasmtime GitHub notifications bot (Jun 13 2025 at 18:20):

fitzgen has marked PR #11020 as ready for review.

Wasmtime GitHub notifications bot (Jun 13 2025 at 18:20):

fitzgen requested alexcrichton for a review on PR #11020.

Wasmtime GitHub notifications bot (Jun 13 2025 at 18:20):

fitzgen requested wasmtime-compiler-reviewers for a review on PR #11020.

Wasmtime GitHub notifications bot (Jun 13 2025 at 18:20):

fitzgen requested wasmtime-core-reviewers for a review on PR #11020.

Wasmtime GitHub notifications bot (Jun 13 2025 at 18:21):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jun 13 2025 at 18:21):

fitzgen created PR review comment:

I didn't remove this method yet, but it should be possible in follow ups.

Wasmtime GitHub notifications bot (Jun 13 2025 at 18:22):

fitzgen commented on PR #11020:

Making it so that the options always have a core type was actually really nice, and a bunch of initializers that previously had options and a core type no longer need a separate core type.

Anyways, I think this is ready for another round of review.

Wasmtime GitHub notifications bot (Jun 13 2025 at 18:37):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 13 2025 at 18:38):

alexcrichton created PR review comment:

Given that this live in CanonicalOptions I don't think there's any need for it to live in LiftedFunction any more?

Wasmtime GitHub notifications bot (Jun 13 2025 at 18:38):

alexcrichton created PR review comment:

Would it be possible to use this branch independent of whatever data model is used now?

Wasmtime GitHub notifications bot (Jun 13 2025 at 22:44):

fitzgen requested alexcrichton for a review on PR #11020.

Wasmtime GitHub notifications bot (Jun 13 2025 at 22:44):

fitzgen updated PR #11020.

Wasmtime GitHub notifications bot (Jun 13 2025 at 22:44):

fitzgen requested wasmtime-fuzz-reviewers for a review on PR #11020.

Wasmtime GitHub notifications bot (Jun 13 2025 at 22:46):

fitzgen updated PR #11020.

Wasmtime GitHub notifications bot (Jun 13 2025 at 22:47):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jun 13 2025 at 22:47):

fitzgen created PR review comment:

Ah I see what you were getting at this whole time now. Yes, done.

Wasmtime GitHub notifications bot (Jun 13 2025 at 23:00):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 14 2025 at 01:05):

github-actions[bot] commented on PR #11020:

Subscribe to Label Action

cc @fitzgen

<details>
This issue or pull request has been labeled: "fuzzing", "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

fitzgen: fuzzing

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 (Jun 18 2025 at 20:23):

fitzgen updated PR #11020.

Wasmtime GitHub notifications bot (Jun 18 2025 at 20:23):

fitzgen has enabled auto merge for PR #11020.

Wasmtime GitHub notifications bot (Jun 18 2025 at 20:57):

fitzgen merged PR #11020.

Last updated: Dec 06 2025 at 06:05 UTC