Debug: clarify safety around ActivationBacktrace::new. (#12191)

* Debug: clarify safety around ActivationBacktrace::new.

As per [this discussion], we decided that we would handle potential
frame-cursor invalidation unsafety (with a hypothetical future
frame-editing debugger API) by putting `unsafe` on that future
hypothetical API rather than on the cursor construction. With the APIs
available today on the `Store`, there is no way to invalidate the frame
cursor while within its lifetime-bounded scope (with lifetime tied to
the `Store` that is passed into the hostcall creating the cursor), so
the API today should be completely safe. This PR makes it so.

[this discussion]: #12176 (comment)

* store_mut is safe as well.

Author: cfallin

crates/wasmtime/src/runtime/debug.rs

@@ -153,12 +153,7 @@ impl<'a, T: 'static> DebugFrameCursor<'a, T> {
             };
             self.frames = match next_frame {
                 FrameOrHostCode::Frame(frame) => VirtualFrame::decode(
-                    // SAFETY: we are using the Store only to decode
-                    // frames; the only mutable aspect used here is
-                    // rooting any new GC values that are read out,
-                    // and we do not remove frames that we may be
-                    // visiting.
-                    unsafe { self.iter.store_mut() }.0.as_store_opaque(),
+                    self.iter.store_mut().0.as_store_opaque(),
                     frame,
                     self.is_trapping_frame,
                 ),
@@ -203,14 +198,7 @@ impl<'a, T: 'static> DebugFrameCursor<'a, T> {
     /// Get the instance associated with the current frame.
     pub fn instance(&mut self) -> Instance {
         let instance = self.raw_instance();
-        Instance::from_wasmtime(
-            instance.id(),
-            // SAFETY: we are using the Store only
-            // to read the instance reference; we
-            // do not remove frames that we may be
-            // visiting.
-            unsafe { self.iter.store_mut() }.0.as_store_opaque(),
-        )
+        Instance::from_wasmtime(instance.id(), self.iter.store_mut().0.as_store_opaque())
     }
 
     /// Get the module associated with the current frame, if any
@@ -505,10 +493,7 @@ impl<'a, T: 'static> AsContext for DebugFrameCursor<'a, T> {
 }
 impl<'a, T: 'static> AsContextMut for DebugFrameCursor<'a, T> {
     fn as_context_mut(&mut self) -> StoreContextMut<'_, Self::Data> {
-        // SAFETY: `StoreContextMut` does not provide any methods that
-        // could remove frames from the stack, so the iterator remains
-        // valid.
-        unsafe { StoreContextMut(self.iter.store_mut().0) }
+        StoreContextMut(self.iter.store_mut().0)
     }
 }
 

crates/wasmtime/src/runtime/vm/traphandlers/backtrace.rs

@@ -407,19 +407,7 @@ impl<'a, T: 'static> ActivationBacktrace<'a, T> {
     /// This serves as an alternative to `Backtrace::trace()` and
     /// friends: it allows external iteration (and e.g. lazily walking
     /// through frames in a stack) rather than visiting via a closure.
-    ///
-    /// # Safety
-    ///
-    /// Although the iterator provides mutable store as a public
-    /// field, this *must not* be used to mutate the stack activation
-    /// itself that this iterator is visiting. While the `store`
-    /// technically owns the stack in question, the only way to do
-    /// this with the current API would be to return back into the
-    /// Wasm activation. As long as this iterator is held and used
-    /// while within host code called from that activation (which will
-    /// ordinarily be ensured if the `store`'s lifetime came from the
-    /// host entry point) then everything will be sound.
-    pub(crate) unsafe fn new(
+    pub(crate) fn new(
         store: StoreContextMut<'a, T>,
         activation: Activation,
     ) -> ActivationBacktrace<'a, T> {
@@ -506,9 +494,9 @@ impl<'a, T> StoreBacktrace<'a, T> {
         // the innermost activation that owns the store, or a sentinel
         // if there are no activations.
         let current = match activations.pop() {
-            Some(innermost) => unsafe {
+            Some(innermost) => {
                 StoreOrActivationBacktrace::Activation(ActivationBacktrace::new(store, innermost))
-            },
+            }
             None => StoreOrActivationBacktrace::Store(store),
         };
         StoreBacktrace {
@@ -526,14 +514,7 @@ impl<'a, T> StoreBacktrace<'a, T> {
     }
 
     /// Get the Store underlying this iteration.
-    ///
-    /// # Safety
-    ///
-    /// This mutable store access *must not* be used to mutate the
-    /// stack itself that this iterator is visiting by removing
-    /// frames, even though the `store` technically owns the stack in
-    /// question.
-    pub unsafe fn store_mut(&mut self) -> StoreContextMut<'_, T> {
+    pub fn store_mut(&mut self) -> StoreContextMut<'_, T> {
         match self.current.as_mut().unwrap() {
             StoreOrActivationBacktrace::Activation(activation) => {
                 StoreContextMut(activation.store.0)
@@ -554,9 +535,9 @@ impl<'a, T> StoreBacktrace<'a, T> {
         let activation = self.activations.pop();
         let store = self.take_store();
         self.current = Some(match activation {
-            Some(activation) => StoreOrActivationBacktrace::Activation(unsafe {
-                ActivationBacktrace::new(store, activation)
-            }),
+            Some(activation) => {
+                StoreOrActivationBacktrace::Activation(ActivationBacktrace::new(store, activation))
+            }
             None => StoreOrActivationBacktrace::Store(store),
         });
     }