Make backtrace creation more accurate to support GC (#268)

When creating a backtrace while running some continuation `c` with
parent `p` (another continuation or the main stack), the following
happens at the moment:
After traversing the stack frames of `c`, we need to traverse the stack
frames of `p`. To this end, we look at the `StackLimits` object of `p`.
The `last_wasm_exit_{fp, pc}` fields therein tell us the frame pointer
and program counter at the point where we stopped execution in `p`. This
must have been at a `resume` Wasm instruction that resumed `c`. This FP
and PC allows us to travese the stack frames of `p`.
Note that the fields in `StackLimits` are updated by the various stack
switching instructions. The `last_wasm_exit_{fp,pc}` fields are only
ever used for creating backtraces, they do not influence control flow at
all.

Since #223, the `last_wasm_exit_pc` field in `StackLimits` is set on
`resume` by using the CLIF instruction `get_instruction_pointer`, which
I introduced specifically for that use case. This CLIF instruction will
give us a PC value that will be associated with the Wasm `resume`
instruction under consideration. This ensures that the backtraces we
create mention the right Wasm instructions.

That's good enough for our current use case, where backtraces are used
mostly to show where things went wrong on a trap. However, in the
future, when we want to support GC, we *also* need to use backtraces to
obtain stack maps, do identify live objects on continuation stacks. At
that point, the current approach becomes too coarse: Currently, the
`last_wasm_exit_pc` value we create is associated with the right *Wasm*
instruction (namely, a `resume`), but with the wrong *CLIF* instruction:
The PC saved in `last_wasm_exit_pc` will be associated with the
`get_instruction_pointer` CLIF instruction that created it. However,
logically, it must be associated with the `stack_switch` CLIF
instruction where execution actually switched from `p` to `c`, and where
it would subsequently continue if we were to switch back to the parent
`p`. Only the `stack_switch` instruction will have the most
recent/correct stack map describing where live values are located in the
stack frame that executed `resume`.

This PR rectifies this situation as follows: Instead of maintaining the
fields `last_wasm_exit_{fp, pc}` in `StackLimits`, we now load the
required PC and FP values directly from the control context of the
corresponding Fiber (i.e., the memory area that saves PC, SP, FP used to
actually switch stacks). In other words, to create backtraces, we get
the necessary information about where execution continues in the parent
stack directly from the source of truth (i.e., the control context),
rather than duplicating this information in `StackLimits` just for
backtrace creation.

Thus, the fields `last_wasm_exit_{fp, pc}` are removed from
`StackLimits` and no longer need to be updated on `resume`. Further, the
`get_instruction_pointer` CLIF instruction, and corresponding `GetRip`
x64 `MInst`, which I introduced in #223, are removed.

cranelift/codegen/meta/src/shared/instructions.rs

@@ -1322,32 +1322,6 @@ pub(crate) fn define(
         .operands_out(vec![Operand::new("addr", iAddr)]),
     );
 
-    // NOTE(frank-emrich)
-    // This is only used as part of a temporary workaround while our
-    // cross-continuation implementation of backtraces is built around the
-    // assumption that we "exit" Wasm on resume.
-    // The returned instruction pointer is never used for actual control flow
-    // (i.e., we never branch to it), but only for the construction of
-    // backtraces.
-    //
-    // We conservatively give it all kinds of side-effects to avoid it being
-    // moved around too much, but all that matters anyway is that during the
-    // Wasm -> CLIF translation, this CLIF instruction is associated with the
-    // current Wasm instruction.
-    ig.push(
-        Inst::new(
-            "get_instruction_pointer",
-            r#"
-        Get the instruction pointer at this instruction.
-        "#,
-            &formats.nullary,
-        )
-        .operands_out(vec![Operand::new("addr", iAddr)])
-        .other_side_effects()
-        .can_load()
-        .can_store(),
-    );
-
     ig.push(
         Inst::new(
             "iconst",

cranelift/codegen/src/isa/x64/inst.isle

@@ -811,9 +811,6 @@
        ;; this program point.
        (Unwind (inst UnwindInst))
 
-       ;; Writes the current PC into dst
-       (GetRip (dst WritableGpr))
-
        ;; A pseudoinstruction that just keeps a value alive.
        (DummyUse (reg Reg))))
 

cranelift/codegen/src/isa/x64/inst/emit.rs

@@ -4626,15 +4626,6 @@ pub(crate) fn emit(
         Inst::DummyUse { .. } => {
             // Nothing.
         }
-
-        Inst::GetRip { dst } => {
-            let here = sink.get_label();
-            sink.bind_label(here, state.ctrl_plane_mut());
-            let amode = Amode::RipRelative { target: here };
-            let dst = dst.map(|gpr| Reg::from(gpr));
-            let inst = Inst::lea(amode, dst);
-            inst.emit(sink, info, state);
-        }
     }
 
     state.clear_post_insn();

cranelift/codegen/src/isa/x64/inst/mod.rs

@@ -139,7 +139,6 @@ impl Inst {
             | Inst::CoffTlsGetAddr { .. }
             | Inst::Unwind { .. }
             | Inst::DummyUse { .. }
-            | Inst::GetRip { .. }
             | Inst::AluConstOp { .. } => smallvec![],
 
             Inst::LockCmpxchg16b { .. }
@@ -1957,10 +1956,6 @@ impl PrettyPrint for Inst {
                 let reg = pretty_print_reg(*reg, 8);
                 format!("dummy_use {reg}")
             }
-
-            Inst::GetRip { .. } => {
-                format!("get_rip")
-            }
         }
     }
 }
@@ -2707,10 +2702,6 @@ fn x64_get_operands(inst: &mut Inst, collector: &mut impl OperandVisitor) {
         Inst::DummyUse { reg } => {
             collector.reg_use(reg);
         }
-
-        Inst::GetRip { dst } => {
-            collector.reg_def(dst);
-        }
     }
 }
 

cranelift/codegen/src/isa/x64/lower.isle

@@ -3483,11 +3483,6 @@
                 (Amode.ImmReg 8 (x64_rbp) (mem_flags_trusted))
                 (ExtKind.None)))
 
-(rule (lower (get_instruction_pointer))
-      (let ((dst WritableGpr (temp_writable_gpr))
-            (_ Unit (emit (MInst.GetRip dst))))
-        (value_reg dst)))
-
 ;; Rules for `jump` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
 (rule (lower_branch (jump _) (single_target target))

cranelift/codegen/src/isa/x64/pcc.rs

@@ -979,8 +979,6 @@ pub(crate) fn check(
         Inst::Unwind { .. } | Inst::DummyUse { .. } => Ok(()),
 
         Inst::StackSwitchBasic { .. } => Err(PccError::UnimplementedInst),
-
-        Inst::GetRip { .. } => Err(PccError::UnimplementedInst),
     }
 }
 

cranelift/interpreter/src/step.rs

@@ -1285,7 +1285,6 @@ where
         Opcode::X86Pmulhrsw => unimplemented!("X86Pmulhrsw"),
         Opcode::X86Pmaddubsw => unimplemented!("X86Pmaddubsw"),
         Opcode::X86Cvtt2dq => unimplemented!("X86Cvtt2dq"),
-        Opcode::GetInstructionPointer => unimplemented!("GetInstructionPointer"),
         Opcode::StackSwitch => unimplemented!("StackSwitch"),
     })
 }

crates/continuations/src/lib.rs

@@ -57,8 +57,6 @@ pub struct WasmFXConfig {
 #[derive(Debug, Default, Clone)]
 pub struct StackLimits {
     pub stack_limit: usize,
-    pub last_wasm_exit_fp: usize,
-    pub last_wasm_exit_pc: usize,
     pub last_wasm_entry_fp: usize,
 }
 
@@ -301,8 +299,6 @@ pub mod offsets {
         use memoffset::offset_of;
 
         pub const STACK_LIMIT: usize = offset_of!(StackLimits, stack_limit);
-        pub const LAST_WASM_EXIT_FP: usize = offset_of!(StackLimits, last_wasm_exit_fp);
-        pub const LAST_WASM_EXIT_PC: usize = offset_of!(StackLimits, last_wasm_exit_pc);
         pub const LAST_WASM_ENTRY_FP: usize = offset_of!(StackLimits, last_wasm_entry_fp);
     }
 

crates/cranelift/src/wasmfx/optimized.rs

@@ -1274,22 +1274,18 @@ pub(crate) mod typed_continuation_helpers {
             );
         }
 
-        /// Overwrites the `last_wasm_entry_sp` field of the `StackLimits`
+        /// Overwrites the `last_wasm_entry_fp` field of the `StackLimits`
         /// object in the `StackLimits` of this object by loading the corresponding
         /// field from the `VMRuntimeLimits`.
         /// If `load_stack_limit` is true, we do the same for the `stack_limit`
         /// field.
-        /// If `wasm_exit_fp`/`wasm_exit_pc` values are provided, we use them to
-        /// overwrite the respective fields in the `StackLimits`.
         #[allow(clippy::cast_possible_truncation, reason = "TODO")]
         pub fn load_limits_from_vmcontext<'a>(
             &self,
             env: &mut crate::func_environ::FuncEnvironment<'a>,
             builder: &mut FunctionBuilder,
             vmruntime_limits_ptr: ir::Value,
             load_stack_limit: bool,
-            wasm_exit_fp: Option<ir::Value>,
-            wasm_exit_pc: Option<ir::Value>,
         ) {
             use wasmtime_continuations::offsets as o;
 
@@ -1323,24 +1319,6 @@ pub(crate) mod typed_continuation_helpers {
                     o::stack_limits::STACK_LIMIT,
                 );
             }
-
-            wasm_exit_fp.inspect(|wasm_exit_fp| {
-                builder.ins().store(
-                    memflags,
-                    *wasm_exit_fp,
-                    stack_limits_ptr,
-                    o::stack_limits::LAST_WASM_EXIT_FP as i32,
-                );
-            });
-
-            wasm_exit_pc.inspect(|wasm_exit_pc| {
-                builder.ins().store(
-                    memflags,
-                    *wasm_exit_pc,
-                    stack_limits_ptr,
-                    o::stack_limits::LAST_WASM_EXIT_PC as i32,
-                );
-            });
         }
     }
 
@@ -1966,29 +1944,8 @@ pub(crate) fn translate_resume<'a>(
         // as well as the `VMRuntimeLimits`.
         // See the comment on `wasmtime_continuations::StackChain` for a description
         // of the invariants that we maintain for the various stack limits.
-        // NOTE(frank-emrich) The `last_wasm_exit_pc` field in the `StackLimits`
-        // of the active continuation is only used for the purposes of backtrace
-        // creation, it does not affect control flow at all.
-        // All that matters is that it must contain an arbitrary PC that
-        // Wasmtime has associated with the current Wasm `resume` instruction
-        // being translated. Previously, the value for this field was obtained
-        // inside the `tc_resume` libcall: The `tc_libcall` would automaticall
-        // set libcall `lasm_wasm_exit_pc` in the `VMRuntimeLimits` to the
-        // return address of the libcall, which would indeed be a PC within the
-        // translation of `resume`. We now set the value of `last_wasm_exit_pc`
-        // directly in generated code by using the get_instruction_pointer CLIF
-        // instruction.
         let vm_runtime_limits_ptr = vmctx.load_vm_runtime_limits_ptr(env, builder);
-        let last_wasm_exit_fp = builder.ins().get_frame_pointer(env.pointer_type());
-        let last_wasm_exit_pc = builder.ins().get_instruction_pointer(env.pointer_type());
-        parent_csi.load_limits_from_vmcontext(
-            env,
-            builder,
-            vm_runtime_limits_ptr,
-            true,
-            Some(last_wasm_exit_fp),
-            Some(last_wasm_exit_pc),
-        );
+        parent_csi.load_limits_from_vmcontext(env, builder, vm_runtime_limits_ptr, true);
         resume_csi.write_limits_to_vmcontext(env, builder, vm_runtime_limits_ptr);
 
         // Install handlers in (soon to be) parent's HandlerList:
@@ -2110,14 +2067,7 @@ pub(crate) fn translate_resume<'a>(
         // 3. Broke the continuation chain at suspended_continuation.last_ancestor
 
         // We store parts of the VMRuntimeLimits into the continuation that just suspended.
-        suspended_csi.load_limits_from_vmcontext(
-            env,
-            builder,
-            vm_runtime_limits_ptr,
-            false,
-            None,
-            None,
-        );
+        suspended_csi.load_limits_from_vmcontext(env, builder, vm_runtime_limits_ptr, false);
 
         // Afterwards (!), restore parts of the VMRuntimeLimits from the
         // parent of the suspended continuation (which is now active).
@@ -2390,14 +2340,7 @@ pub(crate) fn translate_switch<'a>(
         // Load current runtime limits from `VMContext` and store in the
         // switcher continuation.
         let vm_runtime_limits_ptr = vmctx.load_vm_runtime_limits_ptr(env, builder);
-        switcher_contref_csi.load_limits_from_vmcontext(
-            env,
-            builder,
-            vm_runtime_limits_ptr,
-            false,
-            None,
-            None,
-        );
+        switcher_contref_csi.load_limits_from_vmcontext(env, builder, vm_runtime_limits_ptr, false);
 
         let revision = switcher_contref.get_revision(env, builder);
         let new_contobj =

crates/wasmtime/src/runtime/vm/continuation.rs

@@ -779,7 +779,7 @@ pub mod stack_chain {
             matches!(self, StackChain::MainStack(_))
         }
 
-        /// Returns an iterator over the stacks in this chain.
+        /// Returns an iterator over the continuations in this chain.
         /// We don't implement `IntoIterator` because our iterator is unsafe, so at
         /// least this gives us some way of indicating this, even though the actual
         /// unsafety lies in the `next` function.
@@ -788,40 +788,71 @@ pub mod stack_chain {
         ///
         /// This function is not unsafe per see, but it returns an object
         /// whose usage is unsafe.
-        pub unsafe fn into_iter(self) -> ContinuationChainIterator {
-            ContinuationChainIterator(self)
+        pub unsafe fn into_continuation_iter(self) -> ContinuationIterator {
+            ContinuationIterator(self)
+        }
+
+        /// Returns an iterator over the stack limits in this chain.
+        /// We don't implement `IntoIterator` because our iterator is unsafe, so at
+        /// least this gives us some way of indicating this, even though the actual
+        /// unsafety lies in the `next` function.
+        ///
+        /// # Safety
+        ///
+        /// This function is not unsafe per see, but it returns an object
+        /// whose usage is unsafe.
+        pub unsafe fn into_stack_limits_iter(self) -> StackLimitsIterator {
+            StackLimitsIterator(self)
         }
     }
 
-    /// Iterator for stacks in a stack chain.
-    /// Each stack is represented by a tuple `(co_opt, sl)`, where sl is a pointer
-    /// to the stack's `StackLimits` object and `co_opt` is a pointer to the
-    /// corresponding `VMContRef`, or None for the main stack.
+    /// Iterator for Continuations in a stack chain.
+    #[cfg_attr(feature = "wasmfx_baseline", allow(dead_code))]
+    pub struct ContinuationIterator(StackChain);
+
+    /// Iterator for StackLimits in a stack chain.
     #[cfg_attr(feature = "wasmfx_baseline", allow(dead_code))]
-    pub struct ContinuationChainIterator(StackChain);
+    pub struct StackLimitsIterator(StackChain);
+
+    impl Iterator for ContinuationIterator {
+        type Item = *mut VMContRef;
+
+        #[cfg(any(not(feature = "wasmfx_baseline"), feature = "wasmfx_no_baseline"))]
+        fn next(&mut self) -> Option<Self::Item> {
+            match self.0 {
+                StackChain::Absent | StackChain::MainStack(_) => None,
+                StackChain::Continuation(ptr) => {
+                    let continuation = unsafe { ptr.as_mut().unwrap() };
+                    self.0 = continuation.parent_chain.clone();
+                    Some(ptr)
+                }
+            }
+        }
 
-    impl Iterator for ContinuationChainIterator {
-        type Item = (Option<*mut VMContRef>, *mut StackLimits);
+        #[cfg(all(feature = "wasmfx_baseline", not(feature = "wasmfx_no_baseline")))]
+        fn next(&mut self) -> Option<Self::Item> {
+            unimplemented!()
+        }
+    }
+
+    impl Iterator for StackLimitsIterator {
+        type Item = *mut StackLimits;
 
         #[cfg(any(not(feature = "wasmfx_baseline"), feature = "wasmfx_no_baseline"))]
         fn next(&mut self) -> Option<Self::Item> {
             match self.0 {
                 StackChain::Absent => None,
                 StackChain::MainStack(csi) => {
                     let stack_limits = unsafe { &mut (*csi).limits } as *mut StackLimits;
-
-                    let next = (None, stack_limits);
                     self.0 = StackChain::Absent;
-                    Some(next)
+                    Some(stack_limits)
                 }
                 StackChain::Continuation(ptr) => {
                     let continuation = unsafe { ptr.as_mut().unwrap() };
-                    let next = (
-                        Some(ptr),
-                        (&mut continuation.common_stack_information.limits) as *mut StackLimits,
-                    );
+                    let stack_limits =
+                        (&mut continuation.common_stack_information.limits) as *mut StackLimits;
                     self.0 = continuation.parent_chain.clone();
-                    Some(next)
+                    Some(stack_limits)
                 }
             }
         }

crates/wasmtime/src/runtime/vm/fibre/mod.rs

@@ -82,6 +82,16 @@ cfg_if::cfg_if! {
                 self.0.range()
             }
 
+            /// Returns the instruction pointer stored in the Fiber's ControlContext.
+            pub fn control_context_instruction_pointer(&self) -> usize {
+                self.0.control_context_instruction_pointer()
+            }
+
+            /// Returns the frame pointer stored in the Fiber's ControlContext.
+            pub fn control_context_frame_pointer(&self) -> usize {
+                self.0.control_context_frame_pointer()
+            }
+
             pub fn initialize(
                 &self,
                 func_ref: *const VMFuncRef,

crates/wasmtime/src/runtime/vm/fibre/unix.rs

@@ -158,6 +158,24 @@ impl FiberStack {
         Some(base..base + self.len)
     }
 
+    pub fn control_context_instruction_pointer(&self) -> usize {
+        // See picture at top of this file:
+        // RIP is stored 8 bytes below top of stack.
+        unsafe {
+            let ptr = self.top.sub(8) as *mut usize;
+            *ptr
+        }
+    }
+
+    pub fn control_context_frame_pointer(&self) -> usize {
+        // See picture at top of this file:
+        // RBP is stored 16 bytes below top of stack.
+        unsafe {
+            let ptr = self.top.sub(16) as *mut usize;
+            *ptr
+        }
+    }
+
     /// This function installs the launchpad for the computation to run on the
     /// fiber, such that executing a `stack_switch` instruction on the stack
     /// actually runs the desired computation.