Move panic payload state from Machine to Thread

Author: divergentdave

src/machine.rs

@@ -252,11 +252,6 @@ pub struct Evaluator<'mir, 'tcx> {
     pub(crate) file_handler: shims::posix::FileHandler,
     pub(crate) dir_handler: shims::posix::DirHandler,
 
-    /// The temporary used for storing the argument of
-    /// the call to `miri_start_panic` (the panic payload) when unwinding.
-    /// This is pointer-sized, and matches the `Payload` type in `src/libpanic_unwind/miri.rs`.
-    pub(crate) panic_payload: Option<Scalar<Tag>>,
-
     /// The "time anchor" for this machine's monotone clock (for `Instant` simulation).
     pub(crate) time_anchor: Instant,
 
@@ -291,7 +286,6 @@ impl<'mir, 'tcx> Evaluator<'mir, 'tcx> {
             validate,
             file_handler: Default::default(),
             dir_handler: Default::default(),
-            panic_payload: None,
             time_anchor: Instant::now(),
             layouts,
             threads: ThreadManager::default(),

src/shims/panic.rs

@@ -48,11 +48,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         // Get the raw pointer stored in arg[0] (the panic payload).
         let &[payload] = check_arg_count(args)?;
         let payload = this.read_scalar(payload)?.check_init()?;
-        assert!(
-            this.machine.panic_payload.is_none(),
-            "the panic runtime should avoid double-panics"
-        );
-        this.machine.panic_payload = Some(payload);
+        this.set_panic_payload(payload);
 
         // Jump to the unwind block to begin unwinding.
         this.unwind_to_block(unwind);
@@ -132,9 +128,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             // We set the return value of `try` to 1, since there was a panic.
             this.write_scalar(Scalar::from_i32(1), catch_unwind.dest)?;
 
-            // `panic_payload` holds what was passed to `miri_start_panic`.
+            // The Thread's `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();
+            let payload = this.take_panic_payload();
 
             // Push the `catch_fn` stackframe.
             let f_instance = this.memory.get_fn(catch_unwind.catch_fn)?.as_instance()?;

src/thread.rs

@@ -106,12 +106,21 @@ enum ThreadJoinStatus {
 /// A thread.
 pub struct Thread<'mir, 'tcx> {
     state: ThreadState,
+
     /// Name of the thread.
     thread_name: Option<Vec<u8>>,
+
     /// The virtual call stack.
     stack: Vec<Frame<'mir, 'tcx, Tag, FrameData<'tcx>>>,
+
     /// The join status.
     join_status: ThreadJoinStatus,
+
+    /// The temporary used for storing the argument of
+    /// the call to `miri_start_panic` (the panic payload) when unwinding.
+    /// This is pointer-sized, and matches the `Payload` type in `src/libpanic_unwind/miri.rs`.
+    panic_payload: Option<Scalar<Tag>>,
+
 }
 
 impl<'mir, 'tcx> Thread<'mir, 'tcx> {
@@ -150,6 +159,7 @@ impl<'mir, 'tcx> Default for Thread<'mir, 'tcx> {
             thread_name: None,
             stack: Vec::new(),
             join_status: ThreadJoinStatus::Joinable,
+            panic_payload: None,
         }
     }
 }
@@ -509,6 +519,21 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> {
             throw_machine_stop!(TerminationInfo::Deadlock);
         }
     }
+
+    /// Store the panic payload when beginning unwinding.
+    fn set_panic_payload(&mut self, payload: Scalar<Tag>) {
+        let thread = self.active_thread_mut();
+        assert!(
+            thread.panic_payload.is_none(),
+            "the panic runtime should avoid double-panics"
+        );
+        thread.panic_payload = Some(payload);
+    }
+
+    /// Retrieve the panic payload, for use in `catch_unwind`.
+    fn take_panic_payload(&mut self) -> Scalar<Tag> {
+        self.active_thread_mut().panic_payload.take().unwrap()
+    }
 }
 
 // Public interface to thread management.
@@ -686,4 +711,16 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         }
         Ok(())
     }
+
+    /// Store the panic payload when beginning unwinding.
+    fn set_panic_payload(&mut self, payload: Scalar<Tag>) {
+        let this = self.eval_context_mut();
+        this.machine.threads.set_panic_payload(payload);
+    }
+
+    /// Retrieve the panic payload, for use in `catch_unwind`.
+    fn take_panic_payload(&mut self) -> Scalar<Tag> {
+        let this = self.eval_context_mut();
+        this.machine.threads.take_panic_payload()
+    }
 }

tests/run-pass/panic/concurrent-panic.rs

@@ -0,0 +1,80 @@
+// ignore-windows: Concurrency on Windows is not supported yet.
+use std::sync::{Arc, Condvar, Mutex};
+use std::thread::{spawn, JoinHandle};
+
+struct BlockOnDrop(Option<JoinHandle<()>>);
+
+impl BlockOnDrop {
+    fn new(handle: JoinHandle<()>) -> BlockOnDrop {
+        BlockOnDrop(Some(handle))
+    }
+}
+
+impl Drop for BlockOnDrop {
+    fn drop(&mut self) {
+        let _ = self.0.take().unwrap().join();
+    }
+}
+
+/// Cause a panic in one thread while another thread is unwinding.
+fn main() {
+    let t1_started_pair = Arc::new((Mutex::new(false), Condvar::new()));
+    let t2_started_pair = Arc::new((Mutex::new(false), Condvar::new()));
+
+    let t1_continue_mutex = Arc::new(Mutex::new(()));
+    let t1_continue_guard = t1_continue_mutex.lock();
+
+    let t1 = {
+        let t1_started_pair = t1_started_pair.clone();
+        let t1_continue_mutex = t1_continue_mutex.clone();
+        spawn(move || {
+            let (mutex, condvar) = &*t1_started_pair;
+            *mutex.lock().unwrap() = true;
+            condvar.notify_one();
+
+            drop(t1_continue_mutex.lock());
+            panic!("panic in thread 1");
+        })
+    };
+    let t2 = {
+        let t2_started_pair = t2_started_pair.clone();
+        let block_on_drop = BlockOnDrop::new(t1);
+        spawn(move || {
+            let _ = block_on_drop;
+
+            let (mutex, condvar) = &*t2_started_pair;
+            *mutex.lock().unwrap() = true;
+            condvar.notify_one();
+
+            panic!("panic in thread 2");
+        })
+    };
+
+    // Wait for thread 1 to signal it has started.
+    let (t1_started_mutex, t1_started_condvar) = &*t1_started_pair;
+    let mut t1_started_guard = t1_started_mutex.lock().unwrap();
+    while !*t1_started_guard {
+        t1_started_guard = t1_started_condvar.wait(t1_started_guard).unwrap();
+    }
+    // Thread 1 should now be blocked waiting on t1_continue_mutex.
+
+    // Wait for thread 2 to signal it has started.
+    let (t2_started_mutex, t2_started_condvar) = &*t2_started_pair;
+    let mut t2_started_guard = t2_started_mutex.lock().unwrap();
+    while !*t2_started_guard {
+        t2_started_guard = t2_started_condvar.wait(t2_started_guard).unwrap();
+    }
+    // Thread 2 should now have already panicked and be in the middle of
+    // unwinding. It should now be blocked on joining thread 1.
+
+    // Unlock t1_continue_mutex, and allow thread 1 to proceed.
+    drop(t1_continue_guard);
+    // Thread 1 will panic the next time it is scheduled. This will test the
+    // behavior of interest to this test, whether Miri properly handles
+    // concurrent panics in two different threads.
+
+    // Block the main thread on waiting to join thread 2. Thread 2 should
+    // already be blocked on joining thread 1, so thread 1 will be scheduled
+    // to run next, as it is the only ready thread.
+    assert!(t2.join().is_err());
+}

tests/run-pass/panic/concurrent-panic.stderr

@@ -0,0 +1,4 @@
+warning: thread support is experimental. For example, Miri does not detect data races yet.
+
+thread '<unnamed>' panicked at 'panic in thread 2', $DIR/concurrent-panic.rs:49:13
+thread '<unnamed>' panicked at 'panic in thread 1', $DIR/concurrent-panic.rs:36:13