Auto merge of #1227 - RalfJung:unwind, r=RalfJung

adjust Miri to needs of changed unwinding strategy

rust-version

@@ -1 +1 @@
-1572c433eed495d0ade41511ae106b180e02851d
+7cdbc87a49b0b705a41a004a6d486b0952521ae7

src/machine.rs

@@ -32,11 +32,10 @@ pub struct FrameData<'tcx> {
     /// Extra data for Stacked Borrows.
     pub call_id: stacked_borrows::CallId,
 
-    /// If this is Some(), then this is a special "catch unwind" frame (the frame of the closure
-    /// called by `__rustc_maybe_catch_panic`). When this frame is popped during unwinding a panic,
-    /// we stop unwinding, use the `CatchUnwindData` to
-    /// store the panic payload, and continue execution in the parent frame.
-    pub catch_panic: Option<CatchUnwindData<'tcx>>,
+    /// If this is Some(), then this is a special "catch unwind" frame (the frame of `try_fn`
+    /// called by `try`). When this frame is popped during unwinding a panic,
+    /// we stop unwinding, use the `CatchUnwindData` to handle catching.
+    pub catch_unwind: Option<CatchUnwindData<'tcx>>,
 }
 
 /// Extra memory kinds
@@ -163,7 +162,8 @@ pub struct Evaluator<'tcx> {
 
     /// The temporary used for storing the argument of
     /// the call to `miri_start_panic` (the panic payload) when unwinding.
-    pub(crate) panic_payload: Option<ImmTy<'tcx, Tag>>,
+    /// This is pointer-sized, and matches the `Payload` type in `src/libpanic_unwind/miri.rs`.
+    pub(crate) panic_payload: Option<Scalar<Tag>>,
 }
 
 impl<'tcx> Evaluator<'tcx> {
@@ -405,15 +405,15 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'tcx> {
         let call_id = stacked_borrows.map_or(NonZeroU64::new(1).unwrap(), |stacked_borrows| {
             stacked_borrows.borrow_mut().new_call()
         });
-        Ok(FrameData { call_id, catch_panic: None })
+        Ok(FrameData { call_id, catch_unwind: None })
     }
 
     #[inline(always)]
     fn stack_pop(
         ecx: &mut InterpCx<'mir, 'tcx, Self>,
         extra: FrameData<'tcx>,
         unwinding: bool,
-    ) -> InterpResult<'tcx, StackPopInfo> {
+    ) -> InterpResult<'tcx, StackPopJump> {
         ecx.handle_stack_pop(extra, unwinding)
     }
 

src/shims/foreign_items.rs

@@ -150,12 +150,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
 
         // Second: some functions that we forward to MIR implementations.
         match link_name {
-            // This matches calls to the *foreign* item `__rust_start_panic*, that is,
-            // calls to `extern "Rust" { fn __rust_start_panic(...) }`.
+            // This matches calls to the foreign item `__rust_start_panic`, that is,
+            // calls to `extern "Rust" { fn __rust_start_panic(...) }`
+            // (and `__rust_panic_cleanup`, respectively).
             // We forward this to the underlying *implementation* in the panic runtime crate.
             // Normally, this will be either `libpanic_unwind` or `libpanic_abort`, but it could
             // also be a custom user-provided implementation via `#![feature(panic_runtime)]`
-            "__rust_start_panic" => {
+            "__rust_start_panic" | "__rust_panic_cleanup"=> {
                 // FIXME we might want to cache this... but it's not really performance-critical.
                 let panic_runtime = tcx
                     .crates()
@@ -164,7 +165,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                     .expect("No panic runtime found!");
                 let panic_runtime = tcx.crate_name(*panic_runtime);
                 let start_panic_instance =
-                    this.resolve_path(&[&*panic_runtime.as_str(), "__rust_start_panic"])?;
+                    this.resolve_path(&[&*panic_runtime.as_str(), link_name])?;
                 return Ok(Some(&*this.load_mir(start_panic_instance.def, None)?));
             }
             _ => {}
@@ -291,11 +292,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                 this.write_scalar(new_ptr, dest)?;
             }
 
-            "__rust_maybe_catch_panic" => {
-                this.handle_catch_panic(args, dest, ret)?;
-                return Ok(false);
-            }
-
             "memcmp" => {
                 let left = this.read_scalar(args[0])?.not_undef()?;
                 let right = this.read_scalar(args[1])?.not_undef()?;

src/shims/intrinsics.rs

@@ -34,6 +34,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         // First handle intrinsics without return place.
         let (dest, ret) = match ret {
             None => match intrinsic_name {
+                "miri_start_panic" => return this.handle_miri_start_panic(args, unwind),
                 "abort" => {
                     // FIXME: remove, once the intrinsic on the rustc side is fixed.
                     throw_machine_stop!(TerminationInfo::Abort);
@@ -44,7 +45,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         };
 
         match intrinsic_name {
-            "miri_start_panic" => return this.handle_miri_start_panic(args, unwind),
+            "try" => return this.handle_try(args, dest, ret),
 
             "arith_offset" => {
                 let offset = this.read_scalar(args[1])?.to_machine_isize(this)?;

src/shims/panic.rs

@@ -17,24 +17,22 @@ use rustc_target::spec::PanicStrategy;
 
 use crate::*;
 
-/// Holds all of the relevant data for a call to
-/// `__rust_maybe_catch_panic`.
-///
-/// If a panic occurs, we update this data with
-/// the information from the panic site.
+/// Holds all of the relevant data for when unwinding hits a `try` frame.
 #[derive(Debug)]
 pub struct CatchUnwindData<'tcx> {
-    /// The dereferenced `data_ptr` argument passed to `__rust_maybe_catch_panic`.
-    pub data_place: MPlaceTy<'tcx, Tag>,
-    /// The dereferenced `vtable_ptr` argument passed to `__rust_maybe_catch_panic`.
-    pub vtable_place: MPlaceTy<'tcx, Tag>,
-    /// The `dest` from the original call to `__rust_maybe_catch_panic`.
-    pub dest: PlaceTy<'tcx, Tag>,
+    /// The `catch_fn` callback to call in case of a panic.
+    catch_fn: Scalar<Tag>,
+    /// The `data` argument for that callback.
+    data: Scalar<Tag>,
+    /// The return place from the original call to `try`.
+    dest: PlaceTy<'tcx, Tag>,
+    /// The return block from the original call to `try`.
+    ret: mir::BasicBlock,
 }
 
 impl<'mir, 'tcx> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {}
 pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> {
-    /// Handles the special "miri_start_panic" intrinsic, which is called
+    /// Handles the special `miri_start_panic` intrinsic, which is called
     /// by libpanic_unwind to delegate the actual unwinding process to Miri.
     fn handle_miri_start_panic(
         &mut self,
@@ -46,47 +44,50 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         trace!("miri_start_panic: {:?}", this.frame().span);
 
         // Get the raw pointer stored in arg[0] (the panic payload).
-        let scalar = this.read_immediate(args[0])?;
+        let payload = this.read_scalar(args[0])?.not_undef()?;
         assert!(
             this.machine.panic_payload.is_none(),
             "the panic runtime should avoid double-panics"
         );
-        this.machine.panic_payload = Some(scalar);
+        this.machine.panic_payload = Some(payload);
 
         // Jump to the unwind block to begin unwinding.
         this.unwind_to_block(unwind);
         return Ok(());
     }
 
-    fn handle_catch_panic(
+    /// Handles the `try` intrinsic, the underlying implementation of `std::panicking::try`.
+    fn handle_try(
         &mut self,
         args: &[OpTy<'tcx, Tag>],
         dest: PlaceTy<'tcx, Tag>,
         ret: mir::BasicBlock,
     ) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
-        let tcx = &{ this.tcx.tcx };
 
-        // fn __rust_maybe_catch_panic(
-        //     f: fn(*mut u8),
-        //     data: *mut u8,
-        //     data_ptr: *mut usize,
-        //     vtable_ptr: *mut usize,
-        // ) -> u32
+        // Signature:
+        //   fn r#try(try_fn: fn(*mut u8), data: *mut u8, catch_fn: fn(*mut u8, *mut u8)) -> i32
+        // Calls `try_fn` with `data` as argument. If that executes normally, returns 0.
+        // If that unwinds, calls `catch_fn` with the first argument being `data` and
+        // then second argument being a target-dependent `payload` (i.e. it is up to us to define
+        // what that is), and returns 1.
+        // The `payload` is passed (by libstd) to `__rust_panic_cleanup`, which is then expected to
+        // return a `Box<dyn Any + Send + 'static>`.
+        // In Miri, `miri_start_panic` is passed exactly that type, so we make the `payload` simply
+        // a pointer to `Box<dyn Any + Send + 'static>`.
 
         // Get all the arguments.
-        let f = this.read_scalar(args[0])?.not_undef()?;
-        let f_arg = this.read_scalar(args[1])?.not_undef()?;
-        let data_place = this.deref_operand(args[2])?;
-        let vtable_place = this.deref_operand(args[3])?;
-
-        // Now we make a function call, and pass `f_arg` as first and only argument.
-        let f_instance = this.memory.get_fn(f)?.as_instance()?;
-        trace!("__rust_maybe_catch_panic: {:?}", f_instance);
-        let ret_place = MPlaceTy::dangling(this.layout_of(tcx.mk_unit())?, this).into();
+        let try_fn = this.read_scalar(args[0])?.not_undef()?;
+        let data = this.read_scalar(args[1])?.not_undef()?;
+        let catch_fn = this.read_scalar(args[2])?.not_undef()?;
+
+        // Now we make a function call, and pass `data` as first and only argument.
+        let f_instance = this.memory.get_fn(try_fn)?.as_instance()?;
+        trace!("try_fn: {:?}", f_instance);
+        let ret_place = MPlaceTy::dangling(this.layout_of(this.tcx.mk_unit())?, this).into();
         this.call_function(
             f_instance,
-            &[f_arg.into()],
+            &[data.into()],
             Some(ret_place),
             // Directly return to caller.
             StackPopCleanup::Goto { ret: Some(ret), unwind: None },
@@ -95,12 +96,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         // We ourselves will return `0`, eventually (will be overwritten if we catch a panic).
         this.write_null(dest)?;
 
-        // In unwind mode, we tag this frame with some extra data.
+        // In unwind mode, we tag this frame with the extra data needed to catch unwinding.
         // This lets `handle_stack_pop` (below) know that we should stop unwinding
         // when we pop this frame.
-        if this.tcx.tcx.sess.panic_strategy() == PanicStrategy::Unwind {
-            this.frame_mut().extra.catch_panic =
-                Some(CatchUnwindData { data_place, vtable_place, dest })
+        if this.tcx.sess.panic_strategy() == PanicStrategy::Unwind {
+            this.frame_mut().extra.catch_unwind = Some(CatchUnwindData { catch_fn, data, dest, ret });
         }
 
         return Ok(());
@@ -110,45 +110,45 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         &mut self,
         mut extra: FrameData<'tcx>,
         unwinding: bool,
-    ) -> InterpResult<'tcx, StackPopInfo> {
+    ) -> InterpResult<'tcx, StackPopJump> {
         let this = self.eval_context_mut();
 
         trace!("handle_stack_pop(extra = {:?}, unwinding = {})", extra, unwinding);
+        if let Some(stacked_borrows) = this.memory.extra.stacked_borrows.as_ref() {
+            stacked_borrows.borrow_mut().end_call(extra.call_id);
+        }
 
         // We only care about `catch_panic` if we're unwinding - if we're doing a normal
         // return, then we don't need to do anything special.
-        let res = if let (true, Some(unwind_data)) = (unwinding, extra.catch_panic.take()) {
-            // We've just popped a frame that was pushed by `__rust_maybe_catch_panic`,
+        if let (true, Some(catch_unwind)) = (unwinding, extra.catch_unwind.take()) {
+            // We've just popped a frame that was pushed by `try`,
             // and we are unwinding, so we should catch that.
             trace!("unwinding: found catch_panic frame during unwinding: {:?}", this.frame().span);
 
-            // `panic_payload` now holds a `*mut (dyn Any + Send)`,
-            // provided by the `miri_start_panic` intrinsic.
-            // We want to split this into its consituient parts -
-            // the data and vtable pointers - and store them according to
-            // `unwind_data`, i.e., we store them where `__rust_maybe_catch_panic`
-            // was told to put them.
-            let payload = this.machine.panic_payload.take().unwrap();
-            let payload = this.ref_to_mplace(payload)?;
-            let payload_data_place = payload.ptr;
-            let payload_vtable_place = payload.meta.unwrap_meta();
-
-            this.write_scalar(payload_data_place, unwind_data.data_place.into())?;
-            this.write_scalar(payload_vtable_place, unwind_data.vtable_place.into())?;
+            // We set the return value of `try` to 1, since there was a panic.
+            this.write_scalar(Scalar::from_i32(1), catch_unwind.dest)?;
 
-            // We set the return value of `__rust_maybe_catch_panic` to 1,
-            // since there was a panic.
-            let dest = unwind_data.dest;
-            this.write_scalar(Scalar::from_int(1, dest.layout.size), dest)?;
+            // `panic_payload` holds what was passed to `miri_start_panic`.
+            // This is exactly the second argument we need to pass to `catch_fn`.
+            let payload = this.machine.panic_payload.take().unwrap();
 
-            StackPopInfo::StopUnwinding
+            // Push the `catch_fn` stackframe.
+            let f_instance = this.memory.get_fn(catch_unwind.catch_fn)?.as_instance()?;
+            trace!("catch_fn: {:?}", f_instance);
+            let ret_place = MPlaceTy::dangling(this.layout_of(this.tcx.mk_unit())?, this).into();
+            this.call_function(
+                f_instance,
+                &[catch_unwind.data.into(), payload.into()],
+                Some(ret_place),
+                // Directly return to caller of `try`.
+                StackPopCleanup::Goto { ret: Some(catch_unwind.ret), unwind: None },
+            )?;
+
+            // We pushed a new stack frame, the engine should not do any jumping now!
+            Ok(StackPopJump::NoJump)
         } else {
-            StackPopInfo::Normal
-        };
-        if let Some(stacked_borrows) = this.memory.extra.stacked_borrows.as_ref() {
-            stacked_borrows.borrow_mut().end_call(extra.call_id);
+            Ok(StackPopJump::Normal)
         }
-        Ok(res)
     }
 
     /// Starta a panic in the interpreter with the given message as payload.