dhil requested alexcrichton for a review on PR #11717.
dhil opened PR #11717 from dhil:continuation-trapping to bytecodealliance:main:
This patch fixes a problem with traps on continuations, which would otherwise allow a Wasm program to continue running after invoking a trapping instruction. Currently, a fresh trap handler is installed per continuation stack, meaning that the effects of a trap is delimited by the stack segment on which the trap occurred -- whereas it really ought to be delimited by the top-level of the program (i.e. the part just before host/engine frames).
dhil requested wasmtime-core-reviewers for a review on PR #11717.
posborne submitted PR review.
posborne created PR review comment:
Should be able to be simplified slightly to just
matches!(*stack_chain, VMStackChain::Continuation(_))
alexcrichton requested fitzgen for a review on PR #11717.
alexcrichton commented on PR #11717:
I'm not familiar enough with the stack-switching code currently to review this myself. For example I don't know if this is accidentally skipping over native frames at the base of other continuations. Given that I'm going to defer to @fitzgen and @posborne as they're more familiar with the details
For example I don't know if this is accidentally skipping over native frames at the base of other continuations.
Excellent point. I think it may skip over intermediate
invoke_wasm_and_catch_trapsframes, suggesting that a "bubbling" semantics of trapping up through continuation stacks may be the right thing to do.
fitzgen submitted PR review:
Thanks for fixing this bug!
Can we add a test that spawns an N deep stack chain with M frames where every other frame is a host frame, and the last frame (whether host or Wasm) triggers a trap? Then we can run that test exhaustively for small N and M.
Something like
(module ;; The imported host function. (import "host" "func" (func $host_func (param i32 i32))) ;; A global that is incremented after calling the host ;; function, which should trap, and therefore the ;; increment should never happen. (global $g (export "g") (mut i32) (i32.const 0)) (func (export "run") (param $frames-per-stack i32) (param $fuel i32) ;; Trap on out-of-fuel for frames. if (i32.eqz (local.get $fuel)) unreachable end ;; Decrement frame fuel. (local.set $fuel (i32.sub (local.get $fuel) (i32.const 1))) if (i32.eqz (i32.rem (local.get $fuel) (local.get $frames-per-stack)))) ;; TODO: Spawn a new stack, starting either with `run` ;; or our host function (based on another param or a ;; global or something), and switch to it... else ;; Call the host function to continue our mutual recursion. (call $host_func (local.get $frames-per-stack) (local.get $frame-fuel)) end ;; Increment the global. Should never execute, dynamically. (global.set $g (i32.add (global.get $g) (i32.const 1)) ) )let host_func = Func::wrap( &mut store, |mut caller: Caller<'_, ()>, frames_per_stack: u32, fuel: u32| -> Result<()> { ;; Trap on out-of-fuel for frames. if fuel == 0 { bail!("out of frame fuel"); } ;; Mutual recursion back into the Wasm function. let run = instance.get_typed_func::<(u32, u32)>(&mut caller).unwrap(); run.call(&mut caller, (frames_per_stack, fuel - 1))?; ;; Increment the global. Should never execute, dynamically. let g = instance.get_global("g").unwrap(); let g_val = g.get(&mut caller).unwrap_i32(); g.set(&mut caller, Val::I32(g_val + 1))?; Ok(()) }, ); // ... for frames_per_stack in 1..4 { for fuel in 0..frames_per_stack * 3 { let mut store = Store::new(&engine, ()); let instance = Instance::new(...)?; let run = instance.get_typed_func::<(u32, u32)>(&mut store)?; run.call(&mut store, (frames_per_stack, fuel))?; let g = instance.get_global(&mut store, "g").unwrap(); assert_eq!(g.unwrap_i32(), 0); } }This would give me a lot more confidence that we are properly handling traps across stacks, regardless of the stack chain, host functions, and what kind of frame is youngest or oldest.
(And when we add embedder API support for spawning stacks, we should also extend the host function in this new test to use that support)
dhil updated PR #11717.
dhil updated PR #11717.
dhil requested cfallin for a review on PR #11717.
dhil requested wasmtime-compiler-reviewers for a review on PR #11717.
dhil updated PR #11717.
dhil updated PR #11717.
dhil updated PR #11717.
dhil updated PR #11717.
dhil updated PR #11717.
@fitzgen PTAL when its convenient -- and apologies for the latency! I have added two new commits:
- https://github.com/bytecodealliance/wasmtime/pull/11717/commits/9bae46738f14a78aec65a3adb724cf873bbbf34e: The test you asked for.
- https://github.com/bytecodealliance/wasmtime/pull/11717/commits/00715fd28d884e4aead5b0613281c409eebd49cd: A more robust way to handle trapping in nested continuations by way of "bubbling" traps through each continuation rather than jumping straight to the nearest delimiting host/engine frame. It is more robust in the sense that it will run any Rust destructor that may be in the initial continuation frame (currently there are none, I believe, but I suppose this may change in the future).
dhil edited a comment on PR #11717:
@fitzgen PTAL when its convenient -- and apologies for the latency! I have added two new commits:
- https://github.com/bytecodealliance/wasmtime/pull/11717/commits/9bae46738f14a78aec65a3adb724cf873bbbf34e: The test you asked for.
- https://github.com/bytecodealliance/wasmtime/pull/11717/commits/00715fd28d884e4aead5b0613281c409eebd49cd: A more robust way to handle trapping in nested continuations by way of "bubbling" traps through each continuation rather than jumping straight to the nearest delimiting host/engine frame. It is more robust in the sense that it will run any Rust destructor that may be in the initial continuation frame (currently there are none, I believe, but I suppose this may change in the future).
If we go with this "bubbling" approach, then I will drop the first two commits of this PR.
:thumbs_up: fitzgen submitted PR review:
Thanks! LGTM with nitpicks below addressed
:speech_balloon: fitzgen created PR review comment:
And similar here
:speech_balloon: fitzgen created PR review comment:
Can you define
AliasRegions::last_wasm_entry_spand use it viaenv.alias_regions.last_wasm_entry_sphere? That will ensure that we are using the correct alias region for this load.I know it is currently a little tedious, but I plan on tackling https://github.com/bytecodealliance/wasmtime/issues/13707 soon, which should remove all the boilerplate.
dhil updated PR #11717.
:memo: dhil submitted PR review.
:speech_balloon: dhil created PR review comment:
Yes! That's an oversight from me. I have added them in https://github.com/bytecodealliance/wasmtime/pull/11717/commits/71b933fef854120bb859ff4700c5fe7205dc3e6c.
dhil updated PR #11717.
:speech_balloon: dhil edited PR review comment.
:thumbs_up: fitzgen submitted PR review:
Thanks!
fitzgen added PR #11717 Continuation trapping semantics to the merge queue.
github-merge-queue[bot] removed PR #11717 Continuation trapping semantics from the merge queue.
fitzgen commented on PR #11717:
@dhil I think you need to mark the new tests as
#[cfg_attr(miri, ignore)], since they run Wasm and MIRI can't handle that, except in very limited cases.https://github.com/bytecodealliance/wasmtime/actions/runs/30111260793/job/89541246907#step:6:654
dhil updated PR #11717.
@dhil I think you need to mark the new tests as
#[cfg_attr(miri, ignore)], since they run Wasm and MIRI can't handle that, except in very limited cases.https://github.com/bytecodealliance/wasmtime/actions/runs/30111260793/job/89541246907#step:6:654
I've added the directive to the continuation trap tests in commit https://github.com/bytecodealliance/wasmtime/pull/11717/commits/ec6cf0b17afcaa6adc3f9fd50c596c5dde6725f6.
:thumbs_up: fitzgen submitted PR review.
fitzgen added PR #11717 Continuation trapping semantics to the merge queue.
github-merge-queue[bot] removed PR #11717 Continuation trapping semantics from the merge queue.
@fitzgen looks like the merge queue failed on
Test macOS arm64 (wasmtime)-- I am not sure how it is related to this PR, are there more conditional compilation directives that I need to add?
fitzgen added PR #11717 Continuation trapping semantics to the merge queue.
fitzgen commented on PR #11717:
I re-enqueued to see if that was some flaky test, but I'm not aware of any flaky segfaults (that would be pretty concerning) so I suspect that the segfault is related to this PR
github-merge-queue[bot] removed PR #11717 Continuation trapping semantics from the merge queue.
alexcrichton added PR #11717 Continuation trapping semantics to the merge queue.
github-merge-queue[bot] removed PR #11717 Continuation trapping semantics from the merge queue.
dhil updated PR #11717.
dhil updated PR #11717.
@fitzgen The continuation trapping tests were failing with ASAN build. I have added
ignore(asan)directives to the two tests too. We need to instrument the stack switching runtime with asan markers in a similar way to the async fiber runtime. Though, I prefer to defer this to a subsequent patch.There is a
wasi-nntest that keeps failing, but I see it failed onmainduring the night too https://github.com/bytecodealliance/wasmtime/actions/runs/30326568601/job/90173165029 -- I suppose this failure is unrelated to this PR.
alexcrichton added PR #11717 Continuation trapping semantics to the merge queue.
alexcrichton commented on PR #11717:
Yeah wasi-nn is no longer gated on right now while it's flaky
:check: alexcrichton merged PR #11717.
alexcrichton removed PR #11717 Continuation trapping semantics from the merge queue.
Last updated: Jul 29 2026 at 05:03 UTC