Stream: git-wasmtime

Topic: wasmtime / PR #15051 CM+GC: Add support for tuples of sca...

view this post on Zulip Wasmtime GitHub notifications bot (Oct 28 2026 at 20:27):

fitzgen requested pchickey for a review on PR #15051.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 28 2026 at 20:27):

fitzgen removed the review request for wasmtime-core-reviewers from PR #15051.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 28 2026 at 20:27):

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

view this post on Zulip Wasmtime GitHub notifications bot (Oct 28 2026 at 20:27):

fitzgen opened PR #15051 from fitzgen:cm-gc-tuples-of-scalars to bytecodealliance:main:

The first bit of actually interacting with reference types in the CM+GC work. This forces us to build out a bunch of infrastructure, so I'm trying to keep this as minimal as possible by only supporting tuples of scalars. Follow up commits will introduce tuples of tuples and other reference-typed things like records and strings and arrays and all that.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 28 2026 at 21:46):

pchickey requested alexcrichton for a review on PR #15051.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 28 2026 at 21:46):

pchickey removed the review request for pchickey from PR #15051.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 16:24):

:memo: alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 16:24):

:speech_balloon: alexcrichton created PR review comment:

Might be worth pointing out that false is a lie here, but it kind of doesn't matter. Or perhaps alternatively, could there be a separate struct_get_{s,u} helper and then there's no param here at all? It's a bit odd to have that flag as well for floats for example.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 16:24):

:speech_balloon: alexcrichton created PR review comment:

Technically speaking one theoretical optimization here could be if in the GC ABI the src/destination are immutable and equal the exact struct could be plumbed through as-is, right? (fine to leave for a future PR/issue, just want to confirm my understanding)

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 16:24):

:speech_balloon: alexcrichton created PR review comment:

In theory these are pretty easy, right? Mostly just abstract_(...)?

IIRC ConcreteExn won't show up here, although I'm not sure about continuations.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 16:24):

:speech_balloon: alexcrichton created PR review comment:

Instead of .unwrap() could this be let Ok(()) = ...?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 16:24):

:speech_balloon: alexcrichton created PR review comment:

There's no max for the GC ABI, right?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 16:24):

:speech_balloon: alexcrichton created PR review comment:

Instead of duplicating this comment in all blocks, could there be an explanatory comment in one location and the others briefly redirect to that?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 16:24):

:speech_balloon: alexcrichton created PR review comment:

This is a linear search in a loop which might lead to quadratic behavior I think -- could idx here, since it's a module-local thing, be compare to the first index in the rec group instead?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 16:24):

:speech_balloon: alexcrichton created PR review comment:

Not that it matters for this test, but technically the (rec ...) here isn't necessary right?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 16:24):

:speech_balloon: alexcrichton created PR review comment:

FWIW I think that the concurrent code is going to need some relatively invasive changes to handle the GC ABI. Not only because of rooting everywhere but also because limits/maximums in params/results are pretty heavily baked in to the implementation on the host side. It might be best to just reject this for now and have a separate todo! or component-model-async integration being a big TODO item.

For example there's code like this which is definitely not correct for GC.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 16:24):

:speech_balloon: alexcrichton created PR review comment:

I'm a bit confused by this, aren't all interface types representable with a single core wasm type in the GC ABI meaning that flat_val_types should always succeed for GC?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 16:24):

:speech_balloon: alexcrichton created PR review comment:

This feels relatively confusing to me because GC tuples do indeed have a single flat type, so this method is kind of a lie in that respect. Could this method grow documentation about how it's supposed to be used or perhaps renamed to cater to the use case for how this works at call-sites?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 16:24):

:speech_balloon: alexcrichton created PR review comment:

How come these extra vecs are made?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 16:24):

:speech_balloon: alexcrichton created PR review comment:

Should this previous types.flat_types method get deleted/renamed?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 16:24):

:speech_balloon: alexcrichton created PR review comment:

Could this contain documentation that this panics if ty's flat representation overflows the limits for linear memory?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 16:24):

:speech_balloon: alexcrichton created PR review comment:

Why use Destination::Struct here when Destination::Stack pretty much does the same thing? Could Destination::Struct be removed entirely?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 16:24):

:speech_balloon: alexcrichton created PR review comment:

What's 525 here refer to?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 16:24):

:speech_balloon: alexcrichton created PR review comment:

Could this be handled in the record_field_{srcs,dsts} helper instead? That'll probably be needed for structs anyway and it's already trying to handle some of this boilerplate

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 16:24):

:speech_balloon: alexcrichton created PR review comment:

I think this limitation is fixed as of https://github.com/bytecodealliance/wasmtime/pull/15044

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 19:56):

fitzgen updated PR #15051.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:00):

:memo: fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:00):

:speech_balloon: fitzgen created PR review comment:

Good eye, fixed in latest commit.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:00):

:memo: fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:00):

:speech_balloon: fitzgen created PR review comment:

Done

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:01):

:memo: fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:01):

:speech_balloon: fitzgen created PR review comment:

Yep, done

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:01):

:memo: fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:01):

:speech_balloon: fitzgen created PR review comment:

Correct, callers passed usize::MAX, but I just made the Some return unconditional.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:03):

:memo: fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:03):

:speech_balloon: fitzgen created PR review comment:

Yes, but FlatType cannot represent reference types. I renamed Options::flat_types to Options::scalar_flat_types to help clarify.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:05):

:memo: fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:05):

:speech_balloon: fitzgen created PR review comment:

Oof, yeah. Okay, I disabled CM-async + GC for now, filed https://github.com/bytecodealliance/wasmtime/issues/15080 to track this.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:06):

:speech_balloon: fitzgen created PR review comment:

Refactored to have struct_get_{s,u} helpers that do the signed or unsigned extension when operating on a packed field.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:06):

:memo: fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:06):

:memo: fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:06):

:speech_balloon: fitzgen created PR review comment:

Done

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:07):

:memo: fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:07):

:speech_balloon: fitzgen created PR review comment:

A borrow issue that was actually pretty easily avoidable. Gone in the latest commit.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:07):

:memo: fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:07):

:speech_balloon: fitzgen created PR review comment:

That is correct, and yes, I am leaving this for the future.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:09):

:memo: fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:09):

:speech_balloon: fitzgen created PR review comment:

Filed https://github.com/bytecodealliance/wasmtime/issues/15081 to track this.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:10):

:memo: fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:10):

:speech_balloon: fitzgen created PR review comment:

Done.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:18):

:memo: fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:18):

:speech_balloon: fitzgen created PR review comment:

Not really because Destination::Stack holds &[ValType] which doesn't work for i8/i16, but even if we turned it into &[StorageType] then we would also need to know the which field of the struct it is and at this point the sharing is not really that helpful. I think Destination::Struct starts making more sense as we start supporting nested tuples in https://github.com/bytecodealliance/wasmtime/pull/15054, e.g.:

https://github.com/fitzgen/wasmtime/blob/1706336c1f96c0ff5ffcb6a347511b3c339acb55/crates/environ/src/fact/trampoline.rs#L1242-L1248

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:19):

:memo: fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:19):

:speech_balloon: fitzgen created PR review comment:

Renamed, as mentioned above.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:19):

:memo: fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:19):

:speech_balloon: fitzgen created PR review comment:

Done

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:19):

:memo: fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:19):

:speech_balloon: fitzgen created PR review comment:

Done

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:19):

:memo: fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:19):

:speech_balloon: fitzgen created PR review comment:

Yep, unnecessary. Removed so it isn't a distraction.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:20):

:memo: fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:20):

:speech_balloon: fitzgen created PR review comment:

https://github.com/WebAssembly/component-model/issues/525

I just removed the reference, since it will just become out of date once CM+GC is actually spec'd.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 20:21):

fitzgen commented on PR #15051:

@alexcrichton I believe I have addressed all of your review feedback with the latest commit

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 22:07):

:memo: alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 22:07):

:speech_balloon: alexcrichton created PR review comment:

Could this return not a GcStruct but perhaps the internals of the type, such as a local/type? It feels a bit odd to need sentinel values for field and e.g. FIELD_UNSET in the next PR where I think it would be best to model this as the GcStruct destination is directly created in aggregate_field_srcs' iterator.

Also, to confirm, I'd expect in the next PR that this method would take &mut Compiler to fill out the struct todo! here by injecting a struct.get instruction to get a struct-from-a-struct

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 22:07):

:speech_balloon: alexcrichton created PR review comment:

I'm not totally sold on this myself just yet, it seems like this would ideally use Destination::Stack for GC types and translate_{tuple,record} would have custom follow-ups for the GC ABI which inserts a struct.new with the desired type. The Destination::Stack variant has all the types expected as well as Options which seems like a good fit for all of this.

Furthermore the handling of Destination::Struct already does nothing everywhere which is basically the same for stack_set when the types line up (which they always would for the GC ABI).

Did you have other plans in mind for Destination::Struct in the future though?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2026 at 22:07):

:speech_balloon: alexcrichton created PR review comment:

Question on this: why is this needed? I would naively expect that all GC structs are required to store a component-model u8 in a storage-type i8 field, so wouldn't this be dead code in such a situation? Or is this one where a storage of i32 matches a component model u8 type?

Last updated: Oct 31 2026 at 23:03 UTC