Small fixes.

Author: vakaras

src/shims/sync.rs

@@ -301,6 +301,8 @@ fn post_cond_signal<'mir, 'tcx: 'mir>(
     mutex: MutexId,
 ) -> InterpResult<'tcx> {
     reacquire_cond_mutex(ecx, thread, mutex)?;
+    // Waiting for the mutex is not included in the waiting time because we need
+    // to acquire the mutex always even if we get a timeout.
     ecx.unregister_timeout_callback_if_exists(thread)
 }
 
@@ -343,10 +345,17 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         let kind = this.read_scalar(kind_op)?.not_undef()?;
         if kind == this.eval_libc("PTHREAD_MUTEX_NORMAL")? {
             // In `glibc` implementation, the numeric values of
-            // `PTHREAD_MUTEX_NORMAL` and `PTHREAD_MUTEX_DEFAULT` are equal, but
-            // they have different behaviour in some cases. Therefore, we add
-            // this flag to ensure that we can distinguish
-            // `PTHREAD_MUTEX_NORMAL` from `PTHREAD_MUTEX_DEFAULT`.
+            // `PTHREAD_MUTEX_NORMAL` and `PTHREAD_MUTEX_DEFAULT` are equal.
+            // However, a mutex created by explicitly passing
+            // `PTHREAD_MUTEX_NORMAL` type has in some cases different behaviour
+            // from the default mutex for which the type was not explicitly
+            // specified. For a more detailed discussion, please see
+            // https://github.com/rust-lang/miri/issues/1419.
+            //
+            // To distinguish these two cases in already constructed mutexes, we
+            // use the same trick as glibc: for the case when
+            // `pthread_mutexattr_settype` is caled explicitly, we set the
+            // `PTHREAD_MUTEX_NORMAL_FLAG` flag.
             let normal_kind = kind.to_i32()? | PTHREAD_MUTEX_NORMAL_FLAG;
             // Check that after setting the flag, the kind is distinguishable
             // from all other kinds.
@@ -414,7 +423,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             } else {
                 // Trying to acquire the same mutex again.
                 if is_mutex_kind_default(this, kind)? {
-                    throw_ub_format!("trying to acquire already locked PTHREAD_MUTEX_DEFAULT");
+                    throw_ub_format!("trying to acquire already locked default mutex");
                 } else if is_mutex_kind_normal(this, kind)? {
                     throw_machine_stop!(TerminationInfo::Deadlock);
                 } else if kind == this.eval_libc("PTHREAD_MUTEX_ERRORCHECK")? {
@@ -484,7 +493,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             // https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_mutex_unlock.html.
             if is_mutex_kind_default(this, kind)? {
                 throw_ub_format!(
-                    "unlocked a PTHREAD_MUTEX_DEFAULT mutex that was not locked by the current thread"
+                    "unlocked a default mutex that was not locked by the current thread"
                 );
             } else if is_mutex_kind_normal(this, kind)? {
                 throw_ub_format!(

src/thread.rs

@@ -377,6 +377,9 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> {
     }
 
     /// Register the given `callback` to be called once the `call_time` passes.
+    ///
+    /// The callback will be called with `thread` being the active thread, and
+    /// the callback may not change the active thread.
     fn register_timeout_callback(
         &mut self,
         thread: ThreadId,
@@ -452,10 +455,8 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> {
         // https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_cond_timedwait.html#
         let potential_sleep_time =
             self.timeout_callbacks.values().map(|info| info.call_time.get_wait_time()).min();
-        if let Some(sleep_time) = potential_sleep_time {
-            if sleep_time == Duration::new(0, 0) {
-                return Ok(SchedulingAction::ExecuteTimeoutCallback);
-            }
+        if potential_sleep_time == Some(Duration::new(0, 0)) {
+            return Ok(SchedulingAction::ExecuteTimeoutCallback);
         }
         // No callbacks scheduled, pick a regular thread to execute.
         if self.threads[self.active_thread].state == ThreadState::Enabled
@@ -699,6 +700,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         let this = self.eval_context_mut();
         let (thread, callback) =
             this.machine.threads.get_ready_callback().expect("no callback found");
+        // This back-and-forth with `set_active_thread` is here because of two
+        // design decisions:
+        // 1. Make the caller and not the callback responsible for changing
+        //    thread.
+        // 2. Make the scheduler the only place that can change the active
+        //    thread.
         let old_thread = this.set_active_thread(thread)?;
         callback(this)?;
         this.set_active_thread(old_thread)?;

tests/compile-fail/sync/libc_pthread_mutex_NULL_deadlock.rs

@@ -0,0 +1,16 @@
+// ignore-windows: No libc on Windows
+//
+// Check that if we pass NULL attribute, then we get the default mutex type.
+
+#![feature(rustc_private)]
+
+extern crate libc;
+
+fn main() {
+    unsafe {
+        let mut mutex: libc::pthread_mutex_t = std::mem::zeroed();
+        assert_eq!(libc::pthread_mutex_init(&mut mutex as *mut _, std::ptr::null() as *const _), 0);
+        assert_eq!(libc::pthread_mutex_lock(&mut mutex as *mut _), 0);
+        libc::pthread_mutex_lock(&mut mutex as *mut _); //~ ERROR Undefined Behavior: trying to acquire already locked default mutex
+    }
+}

tests/compile-fail/sync/libc_pthread_mutex_default_deadlock.rs

@@ -0,0 +1,17 @@
+// ignore-windows: No libc on Windows
+//
+// Check that if we do not set the mutex type, it is the default.
+
+#![feature(rustc_private)]
+
+extern crate libc;
+
+fn main() {
+    unsafe {
+        let mutexattr: libc::pthread_mutexattr_t = std::mem::zeroed();
+        let mut mutex: libc::pthread_mutex_t = std::mem::zeroed();
+        assert_eq!(libc::pthread_mutex_init(&mut mutex as *mut _, &mutexattr as *const _), 0);
+        assert_eq!(libc::pthread_mutex_lock(&mut mutex as *mut _), 0);
+        libc::pthread_mutex_lock(&mut mutex as *mut _); //~ ERROR Undefined Behavior: trying to acquire already locked default mutex
+    }
+}

tests/compile-fail/sync/libc_pthread_mutex_wrong_owner.rs

@@ -24,7 +24,7 @@ fn main() {
 
         let lock_copy = lock.clone();
         thread::spawn(move || {
-            assert_eq!(libc::pthread_mutex_unlock(lock_copy.0.get() as *mut _), 0); //~ ERROR: Undefined Behavior: unlocked a PTHREAD_MUTEX_DEFAULT mutex that was not locked
+            assert_eq!(libc::pthread_mutex_unlock(lock_copy.0.get() as *mut _), 0); //~ ERROR: Undefined Behavior: unlocked a default mutex that was not locked by the current thread
         })
         .join()
         .unwrap();