-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Stack switching: Infrastructure and runtime support #10388
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
0715390
175d189
476ba8a
f8162ae
d03246b
47da5a4
0c6c58a
cb0df54
8f0ba05
f93903a
4237cdc
85f593f
f3a1cab
257f958
ba91bd0
7fbb3fa
27fb201
ffd8a1d
e823b74
b63e072
f69a569
e3e8d6a
12b2d3f
fc60266
7e77662
17645fa
da37984
5db7836
7b72aa3
22bf039
2f994c3
6a60fe6
4ebb04b
1daeb0f
e3a3d4c
9bab3fd
3f3e366
f6dd45f
3957e50
d93132e
ce4e088
d6a3ae3
ca5bcb6
60c9cdb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,6 +66,16 @@ WASM_API_EXTERN void wasmtime_linker_delete(wasmtime_linker_t *linker); | |
| WASM_API_EXTERN void wasmtime_linker_allow_shadowing(wasmtime_linker_t *linker, | ||
| bool allow_shadowing); | ||
|
|
||
| /** | ||
| * \brief Configures whether the given Linker will allow unknown exports from | ||
| * command modules. | ||
| * | ||
| * By default this setting is `false`. | ||
| */ | ||
| WASM_API_EXTERN void | ||
| wasmtime_linker_allow_unknown_exports(wasmtime_linker_t *linker, | ||
| bool allow_unknown_exports); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see a corresponding function definition in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is a stray declaration from some of our downstream experiments. It has been needed to capture the C shadow stack pointer, that clang generates. It must be kept in sync with the stack switching done by the engine. To be clear, this is just due to the fact that clang doesn't know about stack switching.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW, exposing these things the C API would be good in general, so if you want to do it here or in a new PR, you're more than welcome to!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes sure! I am leaning towards adding this in a follow-up PR focused on the C API. What do you think @frank-emrich ? |
||
|
|
||
| /** | ||
| * \brief Defines a new item in this linker. | ||
| * | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -331,6 +331,8 @@ wasmtime_option_group! { | |
| pub trap_on_grow_failure: Option<bool>, | ||
| /// Maximum execution time of wasm code before timing out (1, 2s, 100ms, etc) | ||
| pub timeout: Option<Duration>, | ||
| /// Size of stacks created with cont.new instructions | ||
| pub stack_switching_stack_size: Option<usize>, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At least for now, I'm not sure it is worth having separate config knobs for stacks created via
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an interesting question, which Frank and I have discussed in the past. If my memory serves me right, then our layout is slightly different. However perhaps we can unify the layouts (or we can perhaps simply adopt the fibers' layout). I am not too sure about the implications, I think @frank-emrich has the key knowledge to best answer this question.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The automatic pooling allocator integration would also get you fast allocation of new stacks from a pool, which would look nice on your benchmarks and what have you ;)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, agreed! I totally see the appeal!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From a technical perspective, there is no need for us to have our own config setting for the stack size.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We are generally a little loosey-goosey with our nomenclature here, but to make things precise for a moment:
libcalls are part of Wasmtime's trusted compute base, host functions are not (modulo some hand waving around their usage of Given all that: we will execute libcalls on async fibers, but will never execute host functions on async fibers. When Wasm calls an imported host function, we first switch from the async fiber to the native stack, then run the host function, and finally switch back to the Wasm's async fiber stack to pass the function results back to Wasm JIT code. A libcall should never access stack memory beyond the This is all a little bit of an aside and I'm mostly just explaining all this to make sure we are on the same page here. I don't think any of this needs to change for stack-switching, and I would also expect that we would call libcalls, but never host functions, on stack-switching stacks, same way we do things on our async fibers. |
||
| /// Configures support for all WebAssembly proposals implemented. | ||
| pub all_proposals: Option<bool>, | ||
| /// Configure support for the bulk memory proposal. | ||
|
|
@@ -366,6 +368,8 @@ wasmtime_option_group! { | |
| pub component_model_async: Option<bool>, | ||
| /// Configure support for the function-references proposal. | ||
| pub function_references: Option<bool>, | ||
| /// Configure support for the stack-switching proposal. | ||
| pub stack_switching: Option<bool>, | ||
| /// Configure support for the GC proposal. | ||
| pub gc: Option<bool>, | ||
| /// Configure support for the custom-page-sizes proposal. | ||
|
|
@@ -803,6 +807,12 @@ impl CommonOptions { | |
| config.native_unwind_info(enable); | ||
| } | ||
|
|
||
| match_feature! { | ||
| ["stack-switching" : self.wasm.stack_switching_stack_size] | ||
| size => config.stack_switching_stack_size(size), | ||
| _ => err, | ||
| } | ||
|
|
||
| match_feature! { | ||
| ["pooling-allocator" : self.opts.pooling_allocator.or(pooling_allocator_default)] | ||
| enable => { | ||
|
|
@@ -964,6 +974,9 @@ impl CommonOptions { | |
| if let Some(enable) = self.wasm.memory64.or(all) { | ||
| config.wasm_memory64(enable); | ||
| } | ||
| if let Some(enable) = self.wasm.stack_switching { | ||
| config.wasm_stack_switching(enable); | ||
| } | ||
| if let Some(enable) = self.wasm.custom_page_sizes.or(all) { | ||
| config.wasm_custom_page_sizes(enable); | ||
| } | ||
|
|
@@ -994,6 +1007,7 @@ impl CommonOptions { | |
| ("gc", gc, wasm_gc) | ||
| ("gc", reference_types, wasm_reference_types) | ||
| ("gc", function_references, wasm_function_references) | ||
| ("stack-switching", stack_switching, wasm_stack_switching) | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -148,7 +148,12 @@ fn read_field_at_addr( | |
| .call(get_interned_func_ref, &[vmctx, func_ref_id, expected_ty]); | ||
| builder.func.dfg.first_result(call_inst) | ||
| } | ||
| WasmHeapTopType::Cont => todo!(), // FIXME: #10248 stack switching support. | ||
| WasmHeapTopType::Cont => { | ||
| // TODO(#10248) GC integration for stack switching | ||
| return Err(wasmtime_environ::WasmError::Unsupported( | ||
| "Stack switching feature not compatbile with GC, yet".to_string(), | ||
| )); | ||
|
Comment on lines
+152
to
+
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not something that needs to be handled in this PR, but there is no reason we can't enable GC and also maintain the existing memory management for continuations, where they have the same lifetime as the |
||
| } | ||
| }, | ||
| }, | ||
| }; | ||
|
|
@@ -1011,6 +1016,8 @@ pub fn translate_ref_test( | |
| | WasmHeapType::NoExtern | ||
| | WasmHeapType::Func | ||
| | WasmHeapType::NoFunc | ||
| | WasmHeapType::Cont | ||
| | WasmHeapType::NoCont | ||
| | WasmHeapType::I31 => unreachable!("handled top, bottom, and i31 types above"), | ||
|
|
||
| // For these abstract but non-top and non-bottom types, we check the | ||
|
|
@@ -1063,8 +1070,12 @@ pub fn translate_ref_test( | |
|
|
||
| func_env.is_subtype(builder, actual_shared_ty, expected_shared_ty) | ||
| } | ||
|
|
||
| WasmHeapType::Cont | WasmHeapType::ConcreteCont(_) | WasmHeapType::NoCont => todo!(), // FIXME: #10248 stack switching support. | ||
| WasmHeapType::ConcreteCont(_) => { | ||
| // TODO(#10248) GC integration for stack switching | ||
| return Err(wasmtime_environ::WasmError::Unsupported( | ||
| "Stack switching feature not compatbile with GC, yet".to_string(), | ||
| )); | ||
| } | ||
| }; | ||
| builder.ins().jump(continue_block, &[result]); | ||
|
|
||
|
|
@@ -1403,8 +1414,9 @@ impl FuncEnvironment<'_> { | |
| WasmHeapType::Func | WasmHeapType::ConcreteFunc(_) | WasmHeapType::NoFunc => { | ||
| unreachable!() | ||
| } | ||
|
|
||
| WasmHeapType::Cont | WasmHeapType::ConcreteCont(_) | WasmHeapType::NoCont => todo!(), // FIXME: #10248 stack switching support. | ||
| WasmHeapType::Cont | WasmHeapType::ConcreteCont(_) | WasmHeapType::NoCont => { | ||
| unreachable!() | ||
| } | ||
| }; | ||
|
|
||
| match (ty.nullable, might_be_i31) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -61,6 +61,12 @@ pub const TRAP_HEAP_MISALIGNED: TrapCode = | |
| TrapCode::unwrap_user(Trap::HeapMisaligned as u8 + TRAP_OFFSET); | ||
| pub const TRAP_TABLE_OUT_OF_BOUNDS: TrapCode = | ||
| TrapCode::unwrap_user(Trap::TableOutOfBounds as u8 + TRAP_OFFSET); | ||
| pub const TRAP_UNHANDLED_TAG: TrapCode = | ||
| TrapCode::unwrap_user(Trap::UnhandledTag as u8 + TRAP_OFFSET); | ||
| pub const TRAP_CONTINUATION_ALREADY_CONSUMED: TrapCode = | ||
| TrapCode::unwrap_user(Trap::ContinuationAlreadyConsumed as u8 + TRAP_OFFSET); | ||
| pub const TRAP_DELETE_ME_DEBUG_ASSERTION: TrapCode = | ||
| TrapCode::unwrap_user(Trap::DeleteMeDebugAssertion as u8 + TRAP_OFFSET); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW, we already have |
||
| pub const TRAP_CAST_FAILURE: TrapCode = | ||
| TrapCode::unwrap_user(Trap::CastFailure as u8 + TRAP_OFFSET); | ||
|
|
||
|
|
@@ -202,7 +208,11 @@ fn reference_type(wasm_ht: WasmHeapType, pointer_type: ir::Type) -> ir::Type { | |
| match wasm_ht.top() { | ||
| WasmHeapTopType::Func => pointer_type, | ||
| WasmHeapTopType::Any | WasmHeapTopType::Extern => ir::types::I32, | ||
| WasmHeapTopType::Cont => todo!(), // FIXME: #10248 stack switching support. | ||
| WasmHeapTopType::Cont => | ||
| // TODO(10248) This is added in a follow-up PR | ||
| { | ||
| unimplemented!("codegen for stack switching types not implemented, yet") | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -206,6 +206,32 @@ macro_rules! foreach_builtin_function { | |
| // Raises an unconditional trap where the trap information must have | ||
| // been previously filled in. | ||
| raise(vmctx: vmctx); | ||
|
|
||
| // Creates a new continuation from a funcref. | ||
| cont_new(vmctx: vmctx, r: pointer, param_count: u32, result_count: u32) -> pointer; | ||
|
|
||
| // FIXME(frank-emrich) The next three builtins are used by the debug printing mechanism. | ||
| // They are not supposed to be part of the final upstreamed code. | ||
| // | ||
| // Prints a 'static str, represented as a | ||
| // pointer and a length. | ||
| delete_me_print_str(vmctx: vmctx, s: pointer, len : u64); | ||
| // Prints integer | ||
| delete_me_print_int(vmctx: vmctx, arg : u64); | ||
| // Prints pointer, formatted as hex. | ||
| delete_me_print_pointer(vmctx: vmctx, arg : pointer); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Were these going to be removed in the next PR, or should they be removed now? Maybe throw an item into the meta task list if not now.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My take is that they shouldn't make it into the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question! These only exist for some hacky debug printing code that should not end up in The reason why we don't want to land this is explained here: On the codegen side, we just emit the addresses of Rust string literals as literals into generated code, with no relocation information. So this crashes and burns if you ever try to re-use the same generated code from a different invocation of
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ah okay! Yes, definitely should not land these bits in that case. |
||
|
|
||
| // Returns an index for Wasm's `table.grow` instruction | ||
| // for `contobj`s. Note that the initial | ||
| // Option<VMContObj> (i.e., the value to fill the new | ||
| // slots with) is split into two arguments: The underlying | ||
| // continuation reference and the revision count. To | ||
| // denote the continuation being `None`, `init_contref` | ||
| // may be 0. | ||
| table_grow_cont_obj(vmctx: vmctx, table: u32, delta: u64, init_contref: pointer, init_revision: u64) -> pointer; | ||
| // `value_contref` and `value_revision` together encode | ||
| // the Option<VMContObj>, as in previous libcall. | ||
| table_fill_cont_obj(vmctx: vmctx, table: u32, dst: u64, value_contref: pointer, value_revision: u64, len: u64) -> bool; | ||
| } | ||
| }; | ||
| } | ||
|
|
@@ -347,6 +373,7 @@ impl BuiltinFunctionIndex { | |
| (@get memory32_grow pointer) => (TrapSentinel::NegativeTwo); | ||
| (@get table_grow_func_ref pointer) => (TrapSentinel::NegativeTwo); | ||
| (@get table_grow_gc_ref pointer) => (TrapSentinel::NegativeTwo); | ||
| (@get table_grow_cont_obj pointer) => (TrapSentinel::NegativeTwo); | ||
|
|
||
| // Atomics-related functions return a negative value indicating trap | ||
| // indicate a trap. | ||
|
|
@@ -371,6 +398,8 @@ impl BuiltinFunctionIndex { | |
| (@get intern_func_ref_for_gc_heap u64) => (return None); | ||
| (@get is_subtype u32) => (return None); | ||
|
|
||
| (@get cont_new pointer) => (TrapSentinel::Negative); | ||
|
|
||
| // Bool-returning functions use `false` as an indicator of a trap. | ||
| (@get $name:ident bool) => (TrapSentinel::Falsy); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, I don't see a function definition for this one either.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same reason as above. Should just be deleted for now, I think.