From 6a6f5c87748f1d2ca06f34de938e204bf5489052 Mon Sep 17 00:00:00 2001 From: Frank Emrich Date: Fri, 9 Feb 2024 15:10:53 +0000 Subject: [PATCH 1/4] squashed --- crates/cli-flags/src/lib.rs | 8 ++++ crates/continuations/src/lib.rs | 10 +++++ crates/runtime/src/continuation.rs | 18 ++++---- crates/runtime/src/traphandlers/backtrace.rs | 6 +-- crates/wasmtime/src/config.rs | 8 ++++ tests/all/typed_continuations.rs | 45 ++++++++++++++++++++ 6 files changed, 81 insertions(+), 14 deletions(-) diff --git a/crates/cli-flags/src/lib.rs b/crates/cli-flags/src/lib.rs index f4397880310d..3c84eca95833 100644 --- a/crates/cli-flags/src/lib.rs +++ b/crates/cli-flags/src/lib.rs @@ -211,6 +211,9 @@ wasmtime_option_group! { pub timeout: Option, /// Size of stacks created with cont.new instructions pub wasmfx_stack_size: Option, + /// Space that must be left on stack when starting execution of a + /// function while running on a continuation stack. + pub wasmfx_red_zone_size: Option, /// Configures support for all WebAssembly proposals implemented. pub all_proposals: Option, /// Configure support for the bulk memory proposal. @@ -552,6 +555,11 @@ impl CommonOptions { if let Some(wasmfx_stack_size) = self.wasm.wasmfx_stack_size { config.wasmfx_stack_size(wasmfx_stack_size); } + if let Some(wasmfx_red_zone_size) = self.wasm.wasmfx_red_zone_size { + config.wasmfx_red_zone_size(wasmfx_red_zone_size); + } + + match_feature! { ["pooling-allocator" : self.opts.pooling_allocator] diff --git a/crates/continuations/src/lib.rs b/crates/continuations/src/lib.rs index 62ebedfa14f0..b235d626f0c5 100644 --- a/crates/continuations/src/lib.rs +++ b/crates/continuations/src/lib.rs @@ -3,6 +3,12 @@ use std::ptr; /// Default size for continuation stacks pub const DEFAULT_FIBER_SIZE: usize = 2097152; // 2MB = 512 pages of 4k +/// Default size of the red zone at the bottom of a fiber stack. This means that +/// whenever we are executing on a Fiber stack and starting (!) execution of a +/// wasm (!) function, the stack pointer must be at least this many bytes away +/// from the bottom of the fiber stack. +pub const DEFAULT_RED_ZONE_SIZE: usize = 32768; // 32K = 8 pages of 4k size + /// TODO #[allow(dead_code)] pub const ENABLE_DEBUG_PRINTING: bool = false; @@ -46,6 +52,10 @@ pub mod types { #[derive(Clone)] pub struct WasmFXConfig { pub stack_size: usize, + + /// Space that must be left on stack when starting execution of a + /// function while running on a continuation stack. + pub red_zone_size: usize, } /// This type is used to save (and subsequently restore) a subset of the data in diff --git a/crates/runtime/src/continuation.rs b/crates/runtime/src/continuation.rs index 5f0642507ff2..aea828826a29 100644 --- a/crates/runtime/src/continuation.rs +++ b/crates/runtime/src/continuation.rs @@ -314,9 +314,14 @@ pub fn cont_new( let capacity = cmp::max(param_count, result_count); let payload = Payloads::new(capacity); + let wasmfx_config = unsafe { &*(*instance.store()).wasmfx_config() }; + // TODO(frank-emrich) Currently, the general `stack_limit` configuration + // option of wasmtime is unrelated to the stack size of our fiber stack. + let stack_size = wasmfx_config.stack_size; + let red_zone_size = wasmfx_config.red_zone_size; + let fiber = { - let wasmfx_config = unsafe { &*(*instance.store()).wasmfx_config() }; - let stack = FiberStack::malloc(wasmfx_config.stack_size) + let stack = FiberStack::malloc(stack_size) .map_err(|error| TrapReason::user_without_backtrace(error.into()))?; let args_ptr = payload.data; let fiber = Fiber::new(stack, move |_first_val: (), _suspend: &Yield| unsafe { @@ -332,8 +337,9 @@ pub fn cont_new( }; let tsp = fiber.stack().top().unwrap(); + let stack_limit = unsafe { tsp.sub(stack_size - red_zone_size) } as usize; let contobj = Box::new(ContinuationObject { - limits: StackLimits::with_stack_limit(unsafe { tsp.sub(DEFAULT_FIBER_SIZE) } as usize), + limits: StackLimits::with_stack_limit(stack_limit), fiber: Box::into_raw(fiber), parent_chain: StackChain::Absent, args: payload, @@ -413,12 +419,6 @@ pub fn resume( *runtime_limits.last_wasm_entry_sp.get() = (*contobj).limits.last_wasm_entry_sp; } - unsafe { - (*(*(*instance.store()).vmruntime_limits()) - .stack_limit - .get_mut()) = 0 - }; - Ok(fiber.resume()) } diff --git a/crates/runtime/src/traphandlers/backtrace.rs b/crates/runtime/src/traphandlers/backtrace.rs index 6c612b186202..c7fc3348ea68 100644 --- a/crates/runtime/src/traphandlers/backtrace.rs +++ b/crates/runtime/src/traphandlers/backtrace.rs @@ -230,11 +230,7 @@ impl Backtrace { let stack_range = (*cont.fiber).stack().range().unwrap(); debug_assert!(stack_range.contains(&limits.last_wasm_exit_fp)); debug_assert!(stack_range.contains(&limits.last_wasm_entry_sp)); - // TODO(frank-emrich) Enable this assertion once we stop - // zero-ing the stack limit in - // `wasmtime_runtime::continuation::resume` - // - // debug_assert_eq!(stack_range.end, limits.stack_limit); + debug_assert!(stack_range.contains(&limits.stack_limit)); }, None => { // reached stack information for main stack diff --git a/crates/wasmtime/src/config.rs b/crates/wasmtime/src/config.rs index b836fff96fa7..4fd29999e9a1 100644 --- a/crates/wasmtime/src/config.rs +++ b/crates/wasmtime/src/config.rs @@ -223,6 +223,7 @@ impl Config { compiler_config: CompilerConfig::default(), wasmfx_config: WasmFXConfig { stack_size: wasmtime_continuations::DEFAULT_FIBER_SIZE, + red_zone_size: wasmtime_continuations::DEFAULT_RED_ZONE_SIZE, }, #[cfg(feature = "cache")] cache_config: CacheConfig::new_cache_disabled(), @@ -699,6 +700,13 @@ impl Config { self } + /// Configures the amount of space that must be left on stack when starting + /// execution of a function while running on a continuation stack. + pub fn wasmfx_red_zone_size(&mut self, size: usize) -> &mut Self { + self.wasmfx_config.red_zone_size = size; + self + } + /// Configures whether the WebAssembly tail calls proposal will be enabled /// for compilation or not. /// diff --git a/tests/all/typed_continuations.rs b/tests/all/typed_continuations.rs index a5ab92a9f130..ca9fd2246059 100644 --- a/tests/all/typed_continuations.rs +++ b/tests/all/typed_continuations.rs @@ -1116,4 +1116,49 @@ mod traps { Ok(()) } + + #[test] + #[cfg_attr(feature = "typed_continuations_baseline_implementation", ignore)] + fn stack_overflow_in_continuation() -> Result<()> { + let wat = r#" + (module + (type $ft (func (param i32))) + (type $ct (cont $ft)) + + (func $entry (export "entry") + (call $a) + ) + + (func $a (export "a") + ;; We ask for a billion recursive calls + (i32.const 1_000_000_000) + + (resume $ct (cont.new $ct (ref.func $overflow))) + ) + + (func $overflow (export "overflow") (param $i i32) + (block $continue + (local.get $i) + ;; return if $i == 0 + (br_if $continue) + (return) + ) + (i32.sub (local.get $i) (i32.const 1)) + (call $overflow) + ) + + ) + "#; + + let runner = Runner::new(); + + let error = runner + .run_test::<()>(wat, &[]) + .expect_err("Expecting execution to yield error"); + + assert!(error.root_cause().is::()); + assert_eq!(*error.downcast_ref::().unwrap(), Trap::StackOverflow); + + Ok(()) + } } From 250da1e0f2550688f057b34a081c270014134c18 Mon Sep 17 00:00:00 2001 From: Frank Emrich Date: Wed, 20 Mar 2024 14:38:59 +0000 Subject: [PATCH 2/4] typo --- crates/continuations/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/continuations/src/lib.rs b/crates/continuations/src/lib.rs index b235d626f0c5..4f2aa4f8c5f5 100644 --- a/crates/continuations/src/lib.rs +++ b/crates/continuations/src/lib.rs @@ -4,7 +4,7 @@ use std::ptr; pub const DEFAULT_FIBER_SIZE: usize = 2097152; // 2MB = 512 pages of 4k /// Default size of the red zone at the bottom of a fiber stack. This means that -/// whenever we are executing on a Fiber stack and starting (!) execution of a +/// whenever we are executing on a fiber stack and starting (!) execution of a /// wasm (!) function, the stack pointer must be at least this many bytes away /// from the bottom of the fiber stack. pub const DEFAULT_RED_ZONE_SIZE: usize = 32768; // 32K = 8 pages of 4k size From 153dd04bcd767bf3bcf6864db0245c794750921f Mon Sep 17 00:00:00 2001 From: Frank Emrich Date: Wed, 20 Mar 2024 15:26:13 +0000 Subject: [PATCH 3/4] cargo fmt --- crates/cli-flags/src/lib.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/cli-flags/src/lib.rs b/crates/cli-flags/src/lib.rs index 3c84eca95833..3dc5c9cb6efb 100644 --- a/crates/cli-flags/src/lib.rs +++ b/crates/cli-flags/src/lib.rs @@ -559,8 +559,6 @@ impl CommonOptions { config.wasmfx_red_zone_size(wasmfx_red_zone_size); } - - match_feature! { ["pooling-allocator" : self.opts.pooling_allocator] enable => { From dbda3bcfc825089eadc4a4ea2731ade669c82340 Mon Sep 17 00:00:00 2001 From: Frank Emrich Date: Thu, 21 Mar 2024 13:19:11 +0000 Subject: [PATCH 4/4] document relationship between stack size and red zone size options --- crates/cli-flags/src/lib.rs | 1 + crates/continuations/src/lib.rs | 1 + 2 files changed, 2 insertions(+) diff --git a/crates/cli-flags/src/lib.rs b/crates/cli-flags/src/lib.rs index 3dc5c9cb6efb..5a0ccea676c1 100644 --- a/crates/cli-flags/src/lib.rs +++ b/crates/cli-flags/src/lib.rs @@ -213,6 +213,7 @@ wasmtime_option_group! { pub wasmfx_stack_size: Option, /// Space that must be left on stack when starting execution of a /// function while running on a continuation stack. + /// Must be smaller than the `wasmfx_stack_size` option above. pub wasmfx_red_zone_size: Option, /// Configures support for all WebAssembly proposals implemented. pub all_proposals: Option, diff --git a/crates/continuations/src/lib.rs b/crates/continuations/src/lib.rs index 4f2aa4f8c5f5..960927e71cf9 100644 --- a/crates/continuations/src/lib.rs +++ b/crates/continuations/src/lib.rs @@ -55,6 +55,7 @@ pub struct WasmFXConfig { /// Space that must be left on stack when starting execution of a /// function while running on a continuation stack. + /// Must be smaller than the value of `stack_size` above. pub red_zone_size: usize, }