[stack-switching] GC interaction - #14140
Conversation
c32ebbc to
40100cd
Compare
github-actions
Bot
added
cranelift
Subscribe to Label Actioncc @fitzgen DetailsThis issue or pull request has been labeled: "cranelift", "cranelift:area:x64", "wasmtime:api", "wasmtime:ref-types"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
This patch adds support for using Wasm GC when using the stack switching extension. The contents of this patch: * Trace GC roots across continuations Emit stack maps at stack-switch resume points and track GC-reference metadata for continuation payload buffers, allowing suspended stacks and bound arguments to be traced correctly. Allocate and update this metadata only for GC-capable payloads, leaving ordinary stack-switching paths largely unchanged. * Support `contref`s in GC types This patch enables storing `contref` values in GC structs and arrays. The core idea behind the implementation is the same as for `funcref`: a store-local interning table is used to map 16 bytes continuation identifiers to 4 bytes GC types. * Fixes issue 13750 Retain Wasm type information when loading continuation payloads and mark GC-reference results as requiring stack maps. This keeps references returned by `resume`, `suspend`, and `switch` alive across subsequent GC safepoints. Added the POC as a regression test. * Fixes issue 13021 Added the POC as a regression test. * Fixes issue 13022 Added the POC as a regression test.
40100cd to
22bc217
Compare
fitzgen
left a comment
fitzgen
left a comment
There was a problem hiding this comment.
Sorry for the delay. A few thoughts below.
| // We create a stack slot to hold the continuation values (16 | ||
| // bytes), and pass the address of this slot as the out parameter | ||
| // to the builtin. | ||
| let pointer_type = func_env.pointer_type(); | ||
| let pointer_bytes = pointer_type.bytes(); | ||
| let slot = builder.create_sized_stack_slot(ir::StackSlotData::new( | ||
| ir::StackSlotKind::ExplicitSlot, | ||
| 2 * pointer_bytes, | ||
| u8::try_from(pointer_bytes.trailing_zeros()).unwrap(), | ||
| )); | ||
| let out_result = builder.ins().stack_addr(pointer_type, slot, 0); |
There was a problem hiding this comment.
This will create a new stack slot each time we read a contref from a GC object, correct? But each stack slot is only live across one read? It seems like we should have a get_or_create_contref_stack_slot method in that case, or else repeatedly reading a contref could create a whole bunch of stack slots.
| let contref = builder.ins().load(pointer_type, flags, out_result, 0); | ||
| let revision = builder.ins().load( | ||
| pointer_type, | ||
| flags, | ||
| out_result, | ||
| i32::try_from(pointer_bytes).unwrap(), | ||
| ); |
There was a problem hiding this comment.
Can these offsets use constants defined in wasmtime-environ?
| //! Continuation references are sixteen bytes values, while every | ||
| //! reference field in the GC heap is four bytes. GC objects therefore | ||
| //! store an ID into this table rather than storing a `VMContObj` | ||
| //! directly. |
There was a problem hiding this comment.
This is not the reason to use a side table, the real reason is to avoid trusting native addresses that come out of the GC heap.
| let ans = if gc_refs != 0 { | ||
| crate::vm::stack_switching::cont_new::<true>( | ||
| store, | ||
| instance, | ||
| func, | ||
| param_count, | ||
| result_count, | ||
| )? | ||
| } else { | ||
| crate::vm::stack_switching::cont_new::<false>( | ||
| store, | ||
| instance, | ||
| func, | ||
| param_count, | ||
| result_count, | ||
| )? | ||
| }; |
There was a problem hiding this comment.
Is the generic parameter pulling its weight here? I doubt that a couple tests are going to make much of a difference compared to allocating a whole new stack...
| pub buffer: VMHostArray<u128>, | ||
|
|
||
| #[cfg(feature = "gc")] | ||
| pub gc_ref_data: *mut u8, |
There was a problem hiding this comment.
This should probably be VmPtr<u8>, evem though existing stack-switching code doesn't use that yet (but it should, given the fullness of time).
|
Hi, @dhil , I would like this PR to get merged, if you don't have time to pursue this, I can try to wrap this up while addressing the feedback. Thanks. |
3 participants
This patch adds support for using Wasm GC when using the stack switching extension. The noteworthy contents of this patch:
Emit stack maps at stack-switch resume points and track GC-reference metadata for continuation payload buffers, allowing suspended stacks and bound arguments to be traced correctly. Allocate and update this metadata only for GC-capable payloads, leaving ordinary stack-switching paths largely unchanged.
contrefs in GC typesThis patch enables storing
contrefvalues in GC structs and arrays. The core idea behind the implementation is the same as forfuncref: a store-local interning table is used to map 16 bytes continuation identifiers to 4 bytes GC types.Retain Wasm type information when loading continuation payloads and mark GC-reference results as requiring stack maps. This keeps references returned by
resume,suspend, andswitchalive across subsequent GC safepoints.Added the POC as a regression test.
Added the POC as a regression test.
Added the POC as a regression test.
The POC exits successfully with code 0 now. I've not added the POC as a regression test, because it is undistilled.
Resolves #12941. Resolves #13021. Resolves #13022. Resolves #13750.