Auto merge of #1532 - divergentdave:thread-panic-payload, r=RalfJung

Move panic payload state from Machine to Thread

This PR moves the panic payload storage from the `Machine` state to per-thread state. Prior to this change, if one thread panicked while another was still unwinding, Miri would fail with `thread 'rustc' panicked at 'the panic runtime should avoid double-panics', src/shims/panic.rs:51:9`. I ran into this issue while prototyping a round-robin scheduler, but it's also reachable with the current scheduler and contrived programs that use blocking API calls to cause thread switching during unwinding. I wrote a test case along those lines for this change.

Author: bors

src/eval.rs

@@ -193,12 +193,6 @@ pub fn create_ecx<'mir, 'tcx: 'mir>(
         StackPopCleanup::None { cleanup: true },
     )?;
 
-    // Set the last_error to 0
-    let errno_layout = ecx.machine.layouts.u32;
-    let errno_place = ecx.allocate(errno_layout, MiriMemoryKind::Machine.into());
-    ecx.write_scalar(Scalar::from_u32(0), errno_place.into())?;
-    ecx.machine.last_error = Some(errno_place);
-
     Ok((ecx, ret_place))
 }
 

src/helpers.rs

@@ -394,17 +394,33 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         )
     }
 
+    /// Get last error variable as a place, lazily allocating thread-local storage for it if
+    /// necessary.
+    fn last_error_place(&mut self) -> InterpResult<'tcx, MPlaceTy<'tcx, Tag>> {
+        let this = self.eval_context_mut();
+        if let Some(errno_place) = this.active_thread_ref().last_error {
+            Ok(errno_place)
+        } else {
+            // Allocate new place, set initial value to 0.
+            let errno_layout = this.machine.layouts.u32;
+            let errno_place = this.allocate(errno_layout, MiriMemoryKind::Machine.into());
+            this.write_scalar(Scalar::from_u32(0), errno_place.into())?;
+            this.active_thread_mut().last_error = Some(errno_place);
+            Ok(errno_place)
+        }
+    }
+
     /// Sets the last error variable.
     fn set_last_error(&mut self, scalar: Scalar<Tag>) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
-        let errno_place = this.machine.last_error.unwrap();
+        let errno_place = this.last_error_place()?;
         this.write_scalar(scalar, errno_place.into())
     }
 
     /// Gets the last error variable.
-    fn get_last_error(&self) -> InterpResult<'tcx, Scalar<Tag>> {
-        let this = self.eval_context_ref();
-        let errno_place = this.machine.last_error.unwrap();
+    fn get_last_error(&mut self) -> InterpResult<'tcx, Scalar<Tag>> {
+        let this = self.eval_context_mut();
+        let errno_place = this.last_error_place()?;
         this.read_scalar(errno_place.into())?.check_init()
     }
 

src/machine.rs

@@ -236,9 +236,6 @@ pub struct Evaluator<'mir, 'tcx> {
     pub(crate) argv: Option<Scalar<Tag>>,
     pub(crate) cmd_line: Option<Scalar<Tag>>,
 
-    /// Last OS error location in memory. It is a 32-bit integer.
-    pub(crate) last_error: Option<MPlaceTy<'tcx, Tag>>,
-
     /// TLS state.
     pub(crate) tls: TlsData<'tcx>,
 
@@ -252,11 +249,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,
 
@@ -285,13 +277,11 @@ impl<'mir, 'tcx> Evaluator<'mir, 'tcx> {
             argc: None,
             argv: None,
             cmd_line: None,
-            last_error: None,
             tls: TlsData::default(),
             communicate,
             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,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()?;
+        let thread = this.active_thread_mut();
         assert!(
-            this.machine.panic_payload.is_none(),
+            thread.panic_payload.is_none(),
             "the panic runtime should avoid double-panics"
         );
-        this.machine.panic_payload = Some(payload);
+        thread.panic_payload = Some(payload);
 
         // Jump to the unwind block to begin unwinding.
         this.unwind_to_block(unwind);
@@ -132,9 +133,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.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/shims/posix/linux/foreign_items.rs

@@ -21,7 +21,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             // errno
             "__errno_location" => {
                 let &[] = check_arg_count(args)?;
-                let errno_place = this.machine.last_error.unwrap();
+                let errno_place = this.last_error_place()?;
                 this.write_scalar(errno_place.to_ref().to_scalar()?, dest)?;
             }
 

src/shims/posix/macos/foreign_items.rs

@@ -20,7 +20,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             // errno
             "__error" => {
                 let &[] = check_arg_count(args)?;
-                let errno_place = this.machine.last_error.unwrap();
+                let errno_place = this.last_error_place()?;
                 this.write_scalar(errno_place.to_ref().to_scalar()?, dest)?;
             }
 

src/thread.rs

@@ -106,12 +106,23 @@ 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`.
+    pub(crate) panic_payload: Option<Scalar<Tag>>,
+
+    /// Last OS error location in memory. It is a 32-bit integer.
+    pub(crate) last_error: Option<MPlaceTy<'tcx, Tag>>,
 }
 
 impl<'mir, 'tcx> Thread<'mir, 'tcx> {
@@ -150,6 +161,8 @@ impl<'mir, 'tcx> Default for Thread<'mir, 'tcx> {
             thread_name: None,
             stack: Vec::new(),
             join_status: ThreadJoinStatus::Joinable,
+            panic_payload: None,
+            last_error: None,
         }
     }
 }
@@ -568,6 +581,18 @@ 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 active_thread_ref(&self) -> &Thread<'mir, 'tcx> {
+        let this = self.eval_context_ref();
+        this.machine.threads.active_thread_ref()
+    }
+
     #[inline]
     fn get_total_thread_count(&self) -> usize {
         let this = self.eval_context_ref();

tests/run-pass/libc.rs

@@ -212,6 +212,24 @@ fn test_prctl_thread_name() {
     }
 }
 
+/// Tests whether each thread has its own `__errno_location`.
+fn test_thread_local_errno() {
+    #[cfg(not(target_os = "macos"))]
+    use libc::__errno_location;
+    #[cfg(target_os = "macos")]
+    use libc::__error as __errno_location;
+
+    unsafe {
+        *__errno_location() = 0xBEEF;
+        std::thread::spawn(|| {
+            assert_eq!(*__errno_location(), 0);
+            *__errno_location() = 0xBAD1DEA;
+            assert_eq!(*__errno_location(), 0xBAD1DEA);
+        }).join().unwrap();
+        assert_eq!(*__errno_location(), 0xBEEF);
+    }
+}
+
 fn main() {
     #[cfg(target_os = "linux")]
     test_posix_fadvise();
@@ -229,4 +247,6 @@ fn main() {
 
     #[cfg(target_os = "linux")]
     test_prctl_thread_name();
+
+    test_thread_local_errno();
 }

tests/run-pass/libc.stderr

@@ -0,0 +1,2 @@
+warning: thread support is experimental. For example, Miri does not detect data races yet.
+

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

@@ -0,0 +1,91 @@
+// 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};
+
+struct BlockOnDrop(Option<JoinHandle<()>>);
+
+impl BlockOnDrop {
+    fn new(handle: JoinHandle<()>) -> BlockOnDrop {
+        BlockOnDrop(Some(handle))
+    }
+}
+
+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");
+    }
+}
+
+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 || {
+            eprintln!("Thread 1 starting, will block on mutex");
+            let (mutex, condvar) = &*t1_started_pair;
+            *mutex.lock().unwrap() = true;
+            condvar.notify_one();
+
+            drop(t1_continue_mutex.lock());
+            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);
+        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 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
+    // 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());
+    eprintln!("Thread 2 has exited");
+}

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

@@ -0,0 +1,11 @@
+warning: thread support is experimental. For example, Miri does not detect data races yet.
+
+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