Review comments

Author: divergentdave

src/shims/panic.rs

@@ -48,7 +48,12 @@ 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()?;
-        this.set_panic_payload(payload);
+        let thread = this.active_thread_mut();
+        assert!(
+            thread.panic_payload.is_none(),
+            "the panic runtime should avoid double-panics"
+        );
+        thread.panic_payload = Some(payload);
 
         // Jump to the unwind block to begin unwinding.
         this.unwind_to_block(unwind);
@@ -130,7 +135,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
 
             // 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.take_panic_payload();
+            let payload = this.active_thread_mut().panic_payload.take().unwrap();
 
             // Push the `catch_fn` stackframe.
             let f_instance = this.memory.get_fn(catch_unwind.catch_fn)?.as_instance()?;

src/thread.rs

@@ -119,8 +119,7 @@ pub struct Thread<'mir, 'tcx> {
     /// 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>>,
-
+    pub(crate) panic_payload: Option<Scalar<Tag>>,
 }
 
 impl<'mir, 'tcx> Thread<'mir, 'tcx> {
@@ -519,21 +518,6 @@ 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.
@@ -593,6 +577,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         this.machine.threads.get_active_thread_id()
     }
 
+    #[inline]
+    fn active_thread_mut(&mut self) -> &mut Thread<'mir, 'tcx> {
+        let this = self.eval_context_mut();
+        this.machine.threads.active_thread_mut()
+    }
+
     #[inline]
     fn get_total_thread_count(&self) -> usize {
         let this = self.eval_context_ref();
@@ -711,16 +701,4 @@ 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

@@ -1,4 +1,8 @@
 // ignore-windows: Concurrency on Windows is not supported yet.
+
+//! Cause a panic in one thread while another thread is unwinding. This checks
+//! that separate threads have their own panicking state.
+
 use std::sync::{Arc, Condvar, Mutex};
 use std::thread::{spawn, JoinHandle};
 
@@ -12,11 +16,12 @@ impl BlockOnDrop {
 
 impl Drop for BlockOnDrop {
     fn drop(&mut self) {
+        eprintln!("Thread 2 blocking on thread 1");
         let _ = self.0.take().unwrap().join();
+        eprintln!("Thread 1 has exited");
     }
 }
 
-/// 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()));
@@ -28,6 +33,7 @@ fn main() {
         let t1_started_pair = t1_started_pair.clone();
         let t1_continue_mutex = t1_continue_mutex.clone();
         spawn(move || {
+            eprintln!("Thread 1 starting, will block on mutex");
             let (mutex, condvar) = &*t1_started_pair;
             *mutex.lock().unwrap() = true;
             condvar.notify_one();
@@ -36,6 +42,16 @@ fn main() {
             panic!("panic in thread 1");
         })
     };
+
+    // 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();
+    }
+    eprintln!("Thread 1 reported it has started");
+    // Thread 1 should now be blocked waiting on t1_continue_mutex.
+
     let t2 = {
         let t2_started_pair = t2_started_pair.clone();
         let block_on_drop = BlockOnDrop::new(t1);
@@ -50,24 +66,18 @@ fn main() {
         })
     };
 
-    // 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();
     }
+    eprintln!("Thread 2 reported it has started");
     // 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.
+    eprintln!("Unlocking mutex");
     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
@@ -77,4 +87,5 @@ fn main() {
     // 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());
+    eprintln!("Thread 2 has exited");
 }

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

@@ -1,4 +1,11 @@
 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
+Thread 1 starting, will block on mutex
+Thread 1 reported it has started
+thread '<unnamed>' panicked at 'panic in thread 2', $DIR/concurrent-panic.rs:65:13
+Thread 2 blocking on thread 1
+Thread 2 reported it has started
+Unlocking mutex
+thread '<unnamed>' panicked at 'panic in thread 1', $DIR/concurrent-panic.rs:42:13
+Thread 1 has exited
+Thread 2 has exited