comments and slight refactoring

RalfJung · RalfJung · commit 82374ad9bd73 · 2019-11-19T14:51:08.000+01:00
diff --git a/rust-version b/rust-version
@@ -1 +1 @@
-8831d766ace89bc74714918a7d9fbd3ca5ec946a
+3e525e3f6d9e85d54fa4c49b52df85aa0c990100
diff --git a/src/helpers.rs b/src/helpers.rs
@@ -53,6 +53,7 @@ fn resolve_did<'mir, 'tcx>(tcx: TyCtxt<'tcx>, path: &[&str]) -> InterpResult<'tc
 
 pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> {
 
+    /// Gets an instance for a path.
     fn resolve_path(&self, path: &[&str]) -> InterpResult<'tcx, ty::Instance<'tcx>> {
         Ok(ty::Instance::mono(self.eval_context_ref().tcx.tcx, resolve_did(self.eval_context_ref().tcx.tcx, path)?))
     }
diff --git a/src/machine.rs b/src/machine.rs
@@ -25,10 +25,11 @@ pub const NUM_CPUS: u64 = 1;
 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. When this frame is popped during unwinding a panic,
-    /// we stop unwinding, and use the `CatchUnwindData` to
-    /// store the panic payload and continue execution in the parent frame.
+
+    /// 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>>,
 }
 
diff --git a/src/shims/foreign_items.rs b/src/shims/foreign_items.rs
@@ -130,27 +130,21 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
 
         // First: functions that diverge.
         match link_name {
-            // Note that this matches calls to the *foreign* item "__rust_start_panic" -
-            // that is, calls `extern "Rust" { fn __rust_start_panic(...) }`
-            // We forward this to the underlying *implementation* in "libpanic_unwind"
+            // Note that this matches calls to the *foreign* item `__rust_start_panic* -
+            // that is, calls to `extern "Rust" { fn __rust_start_panic(...) }`.
+            // We forward this to the underlying *implementation* in "libpanic_unwind".
             "__rust_start_panic" => {
                 let start_panic_instance = this.resolve_path(&["panic_unwind", "__rust_start_panic"])?;
                 return Ok(Some(this.load_mir(start_panic_instance.def, None)?));
             }
-
-            // During a normal (non-Miri) compilation,
-            // this gets resolved to the '#[panic_handler]` function at link time,
-            // which corresponds to the function with the `#[panic_handler]` attribute.
-            //
-            // Since we're interpreting mir, we forward it to the implementation of `panic_impl`
-            //
-            // This is used by libcore to forward panics to the actual
-            // panic impl
+            // Similarly, we forward calls to the `panic_impl` foreign item to its implementation.
+            // The implementation is provided by the function with the `#[panic_handler]` attribute.
             "panic_impl" => {
                 let panic_impl_id = this.tcx.lang_items().panic_impl().unwrap();
                 let panic_impl_instance = ty::Instance::mono(*this.tcx, panic_impl_id);
                 return Ok(Some(this.load_mir(panic_impl_instance.def, None)?));
             }
+
             "exit" | "ExitProcess" => {
                 // it's really u32 for ExitProcess, but we have to put it into the `Exit` error variant anyway
                 let code = this.read_scalar(args[0])?.to_i32()?;
diff --git a/src/shims/intrinsics.rs b/src/shims/intrinsics.rs
@@ -45,7 +45,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
 
         // Handle non-diverging intrinsics
         // The intrinsic itself cannot diverge (otherwise, we would have handled it above),
-        // so if we got here without a return place... (can happen e.g., for transmute returning `!`)
+        // so if we got here without a return place that's UB (can happen e.g., for transmute returning `!`).
         let dest = match dest {
             Some(dest) => dest,
             None => throw_ub!(Unreachable)
diff --git a/src/shims/panic.rs b/src/shims/panic.rs
@@ -1,33 +1,42 @@
+//! Panic runtime for Miri.
+//!
+//! The core pieces of the runtime are:
+//! - An implementation of `__rust_maybe_catch_panic` that pushes the invoked stack frame with
+//!   some extra metadata derived from the panic-catching arguments of `__rust_maybe_catch_panic`.
+//! - A hack in `libpanic_unwind` that calls the `miri_start_panic` intrinsic instead of the
+//!   target-native panic runtime. (This lives in the rustc repo.)
+//! - An implementation of `miri_start_panic` that stores its argument (the panic payload), and then
+//!   immediately returns, but on the *unwind* edge (not the normal return edge), thus initiating unwinding.
+//! - A hook executed each time a frame is popped, such that if the frame pushed by `__rust_maybe_catch_panic`
+//!   gets popped *during unwinding*, we take the panic payload and store it according to the extra
+//!   metadata we remembered when pushing said frame.
+
 use rustc::mir;
 use crate::*;
 use super::machine::FrameData;
 use rustc_target::spec::PanicStrategy;
 use crate::rustc_target::abi::LayoutOf;
 
 /// Holds all of the relevant data for a call to
-/// __rust_maybe_catch_panic
+/// `__rust_maybe_catch_panic`.
 ///
 /// If a panic occurs, we update this data with
-/// the information from the panic site
+/// the information from the panic site.
 #[derive(Debug)]
 pub struct CatchUnwindData<'tcx> {
-    /// The 'data' argument passed to `__rust_maybe_catch_panic`
-    pub data: Pointer<Tag>,
-    /// The `data_ptr` argument passed to `__rust_maybe_catch_panic`
+    /// The dereferenced `data_ptr` argument passed to `__rust_maybe_catch_panic`.
     pub data_place: MPlaceTy<'tcx, Tag>,
-    /// The `vtable_ptr` argument passed to `__rust_maybe_catch_panic`
+    /// 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`
+    /// The `dest` from the original call to `__rust_maybe_catch_panic`.
     pub dest: PlaceTy<'tcx, Tag>,
-    /// The `ret` from the original call to `__rust_maybe_catch_panic`
-    pub 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
-    /// by libpanic_unwind to delegate the actual unwinding process to Miri
+    /// by libpanic_unwind to delegate the actual unwinding process to Miri.
     #[inline(always)]
     fn handle_miri_start_panic(
         &mut self,
@@ -44,14 +53,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             throw_unsup_format!("the evaluated program panicked");
         }
 
-        // Get the raw pointer stored in arg[0]
+        // Get the raw pointer stored in arg[0] (the panic payload).
         let scalar = this.read_immediate(args[0])?;
+        assert!(this.machine.panic_payload.is_none(), "the panic runtime should avoid double-panics");
         this.machine.panic_payload = Some(scalar);
 
-        // Jump to the unwind block to begin unwinding
-        // We don't use 'goto_block' - if `unwind` is None,
-        // we'll end up immediately returning out of the
-        // function during the next step() call
+        // Jump to the unwind block to begin unwinding.
+        // We don't use `goto_block` as that is just meant for normal returns.
         let next_frame = this.frame_mut();
         next_frame.block = unwind;
         next_frame.stmt = 0;
@@ -74,16 +82,16 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         //     data_ptr: *mut usize,
         //     vtable_ptr: *mut usize,
         // ) -> u32
+
+        // Get all the arguments.
         let f = this.read_scalar(args[0])?.not_undef()?;
-        let data = this.read_scalar(args[1])?.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()?;
-        this.write_null(dest)?;
         trace!("__rust_maybe_catch_panic: {:?}", f_instance);
-
-
-        // Now we make a function call.
         // TODO: consider making this reusable? `InterpCx::step` does something similar
         // for the TLS destructors, and of course `eval_main`.
         let mir = this.load_mir(f_instance.def, None)?;
@@ -98,32 +106,30 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             StackPopCleanup::Goto { ret: Some(ret), unwind: None },
         )?;
 
+        let mut args = this.frame().body.args_iter();
+        // First argument.
+        let arg_local = args
+            .next()
+            .expect("Argument to __rust_maybe_catch_panic does not take enough arguments.");
+        let arg_dest = this.local_place(arg_local)?;
+        this.write_scalar(f_arg, arg_dest)?;
+        // No more arguments.
+        args.next().expect_none("__rust_maybe_catch_panic argument has more arguments than expected");
+
+        // 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.
         // 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: data.to_ptr()?,
                 data_place,
                 vtable_place,
                 dest,
-                ret,
             })
         }
 
-        let mut args = this.frame().body.args_iter();
-
-        let arg_local = args
-            .next()
-            .expect("Argument to __rust_maybe_catch_panic does not take enough arguments.");
-        let arg_dest = this.local_place(arg_local)?;
-        this.write_scalar(data, arg_dest)?;
-
-        args.next().expect_none("__rust_maybe_catch_panic argument has more arguments than expected");
-
-        // We ourselves will return `0`, eventually (because we will not return if we paniced).
-        this.write_null(dest)?;
-
         return Ok(());
     }
 
@@ -140,33 +146,27 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         // 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 the frame that was immediately above
-            // the frame which originally called `__rust_maybe_catch_panic`
-            trace!("unwinding: found catch_panic frame: {:?}", this.frame().span);
+            // We've just popped a frame that was pushed by `__rust_maybe_catch_panic`,
+            // 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)',
+            // `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 back
-            // into the panic handler frame
-            let real_ret = this.machine.panic_payload.take().unwrap();
-
-            let payload = this.ref_to_mplace(real_ret)?;
+            // 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.expect("Expected fat pointer");
 
-
-            let data_place = unwind_data.data_place;
-            let vtable_place = unwind_data.vtable_place;
-            let dest = unwind_data.dest;
-
-            // Here, we write to the pointers provided to the call
-            // to '__rust_maybe_catch_panic`.
-            this.write_scalar(payload_data_place, data_place.into())?;
-            this.write_scalar(payload_vtable_place, vtable_place.into())?;
+            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 `__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)?;
 
             StackPopInfo::StopUnwinding

Original file line number	Diff line number	Diff line change
`@@ -1 +1 @@`
`1`		`-8831d766ace89bc74714918a7d9fbd3ca5ec946a`
	`1`	`+3e525e3f6d9e85d54fa4c49b52df85aa0c990100`
Original file line number	Diff line number	Diff line change
`@@ -53,6 +53,7 @@ fn resolve_did<'mir, 'tcx>(tcx: TyCtxt<'tcx>, path: &[&str]) -> InterpResult<'tc`
`53`	`53`
`54`	`54`	`pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> {`
`55`	`55`
	`56`	`+ /// Gets an instance for a path.`
`56`	`57`	`fn resolve_path(&self, path: &[&str]) -> InterpResult<'tcx, ty::Instance<'tcx>> {`
`57`	`58`	`Ok(ty::Instance::mono(self.eval_context_ref().tcx.tcx, resolve_did(self.eval_context_ref().tcx.tcx, path)?))`
`58`	`59`	`}`