Small changes.

Author: vakaras

src/shims/sync.rs

@@ -202,6 +202,7 @@ fn condattr_set_clock_id<'mir, 'tcx: 'mir>(
 // Our chosen memory layout for the emulated conditional variable (does not have
 // to match the platform layout!):
 
+// bytes 0-3: reserved for signature on macOS
 // bytes 4-7: the conditional variable id as u32 or 0 if id is not assigned yet.
 // bytes 8-11: the clock id constant as i32
 
@@ -275,19 +276,13 @@ fn release_cond_mutex<'mir, 'tcx: 'mir>(
     active_thread: ThreadId,
     mutex: MutexId,
 ) -> InterpResult<'tcx> {
-    if let Some((owner_thread, current_locked_count)) = ecx.mutex_unlock(mutex) {
-        if current_locked_count != 0 {
-            throw_unsup_format!("awaiting on multiple times acquired lock is not supported");
+    if let Some((old_owner_thread, old_locked_count)) = ecx.mutex_unlock(mutex)? {
+        if old_locked_count != 1 {
+            throw_unsup_format!("awaiting on a lock acquired multiple times is not supported");
         }
-        if owner_thread != active_thread {
+        if old_owner_thread != active_thread {
             throw_ub_format!("awaiting on a mutex owned by a different thread");
         }
-        if let Some(thread) = ecx.mutex_dequeue(mutex) {
-            // We have at least one thread waiting on this mutex. Transfer
-            // ownership to it.
-            ecx.mutex_lock(mutex, thread);
-            ecx.unblock_thread(thread)?;
-        }
     } else {
         throw_ub_format!("awaiting on unlocked mutex");
     }
@@ -349,7 +344,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             mutexattr_get_kind(this, attr_op)?.not_undef()?
         };
 
-        let _ = mutex_get_or_create_id(this, mutex_op)?;
+        // Write 0 to use the same code path as the static initializers.
+        mutex_set_id(this, mutex_op, Scalar::from_i32(0))?;
+
         mutex_set_kind(this, mutex_op, kind)?;
 
         Ok(0)
@@ -427,19 +424,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         let kind = mutex_get_kind(this, mutex_op)?.not_undef()?;
         let id = mutex_get_or_create_id(this, mutex_op)?;
 
-        if let Some((owner_thread, current_locked_count)) = this.mutex_unlock(id) {
-            if owner_thread != this.get_active_thread()? {
+        if let Some((old_owner_thread, _old_locked_count)) = this.mutex_unlock(id)? {
+            if old_owner_thread != this.get_active_thread()? {
                 throw_ub_format!("called pthread_mutex_unlock on a mutex owned by another thread");
             }
-            if current_locked_count == 0 {
-                // The mutex is unlocked.
-                if let Some(thread) = this.mutex_dequeue(id) {
-                    // We have at least one thread waiting on this mutex. Transfer
-                    // ownership to it.
-                    this.mutex_lock(id, thread);
-                    this.unblock_thread(thread)?;
-                }
-            }
             Ok(0)
         } else {
             if kind == this.eval_libc("PTHREAD_MUTEX_NORMAL")? {
@@ -476,11 +464,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         let active_thread = this.get_active_thread()?;
 
         if this.rwlock_is_write_locked(id) {
-            this.rwlock_enqueue_reader(id, active_thread);
-            this.block_thread(active_thread)?;
+            this.rwlock_enqueue_and_block_reader(id, active_thread)?;
             Ok(0)
         } else {
-            this.rwlock_reader_add(id, active_thread);
+            this.rwlock_reader_lock(id, active_thread);
             Ok(0)
         }
     }
@@ -494,7 +481,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         if this.rwlock_is_write_locked(id) {
             this.eval_libc_i32("EBUSY")
         } else {
-            this.rwlock_reader_add(id, active_thread);
+            this.rwlock_reader_lock(id, active_thread);
             Ok(0)
         }
     }
@@ -506,10 +493,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         let active_thread = this.get_active_thread()?;
 
         if this.rwlock_is_locked(id) {
-            this.block_thread(active_thread)?;
-            this.rwlock_enqueue_writer(id, active_thread);
+            this.rwlock_enqueue_and_block_writer(id, active_thread)?;
         } else {
-            this.rwlock_writer_set(id, active_thread);
+            this.rwlock_writer_lock(id, active_thread);
         }
 
         Ok(0)
@@ -524,7 +510,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         if this.rwlock_is_locked(id) {
             this.eval_libc_i32("EBUSY")
         } else {
-            this.rwlock_writer_set(id, active_thread);
+            this.rwlock_writer_lock(id, active_thread);
             Ok(0)
         }
     }
@@ -535,18 +521,18 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         let id = rwlock_get_or_create_id(this, rwlock_op)?;
         let active_thread = this.get_active_thread()?;
 
-        if this.rwlock_reader_remove(id, active_thread) {
+        if this.rwlock_reader_unlock(id, active_thread) {
             // The thread was a reader.
             if this.rwlock_is_locked(id) {
                 // No more readers owning the lock. Give it to a writer if there
                 // is any.
                 if let Some(writer) = this.rwlock_dequeue_writer(id) {
                     this.unblock_thread(writer)?;
-                    this.rwlock_writer_set(id, writer);
+                    this.rwlock_writer_lock(id, writer);
                 }
             }
             Ok(0)
-        } else if Some(active_thread) == this.rwlock_writer_remove(id) {
+        } else if Some(active_thread) == this.rwlock_writer_unlock(id) {
             // The thread was a writer.
             //
             // We are prioritizing writers here against the readers. As a
@@ -555,12 +541,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             if let Some(writer) = this.rwlock_dequeue_writer(id) {
                 // Give the lock to another writer.
                 this.unblock_thread(writer)?;
-                this.rwlock_writer_set(id, writer);
+                this.rwlock_writer_lock(id, writer);
             } else {
                 // Give the lock to all readers.
                 while let Some(reader) = this.rwlock_dequeue_reader(id) {
                     this.unblock_thread(reader)?;
-                    this.rwlock_reader_add(id, reader);
+                    this.rwlock_reader_lock(id, reader);
                 }
             }
             Ok(0)
@@ -586,6 +572,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
     fn pthread_condattr_init(&mut self, attr_op: OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> {
         let this = self.eval_context_mut();
 
+        // The default value of the clock attribute shall refer to the system
+        // clock.
+        // https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_condattr_setclock.html
         let default_clock_id = this.eval_libc("CLOCK_REALTIME")?;
         condattr_set_clock_id(this, attr_op, default_clock_id)?;
 
@@ -647,7 +636,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             condattr_get_clock_id(this, attr_op)?.not_undef()?
         };
 
-        let _ = cond_get_or_create_id(this, cond_op)?;
+        // Write 0 to use the same code path as the static initializers.
+        cond_set_id(this, cond_op, Scalar::from_i32(0))?;
+
         cond_set_clock_id(this, cond_op, clock_id)?;
 
         Ok(0)

src/sync.rs

@@ -1,6 +1,7 @@
 use std::collections::{hash_map::Entry, HashMap, VecDeque};
 use std::convert::TryFrom;
 use std::num::NonZeroU32;
+use std::ops::Not;
 
 use rustc_index::vec::{Idx, IndexVec};
 
@@ -142,34 +143,47 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         mutex.lock_count = mutex.lock_count.checked_add(1).unwrap();
     }
 
-    /// Unlock by decreasing the lock count. If the lock count reaches 0, unset
-    /// the owner.
-    fn mutex_unlock(&mut self, id: MutexId) -> Option<(ThreadId, usize)> {
+    /// Try unlocking by decreasing the lock count and returning the old owner
+    /// and the old lock count. If the lock count reaches 0, release the lock
+    /// and potentially give to a new owner. If the lock was not locked, return
+    /// `None`.
+    ///
+    /// Note: It is the caller's responsibility to check that the thread that
+    /// unlocked the lock actually is the same one, which owned it.
+    fn mutex_unlock(&mut self, id: MutexId) -> InterpResult<'tcx, Option<(ThreadId, usize)>> {
         let this = self.eval_context_mut();
         let mutex = &mut this.machine.threads.sync.mutexes[id];
         if let Some(current_owner) = mutex.owner {
-            mutex.lock_count = mutex
-                .lock_count
+            // Mutex is locked.
+            let old_lock_count = mutex.lock_count;
+            mutex.lock_count = old_lock_count
                 .checked_sub(1)
                 .expect("invariant violation: lock_count == 0 iff the thread is unlocked");
             if mutex.lock_count == 0 {
                 mutex.owner = None;
+                // The mutex is completely unlocked. Try transfering ownership
+                // to another thread.
+                if let Some(new_owner) = this.mutex_dequeue(id) {
+                    this.mutex_lock(id, new_owner);
+                    this.unblock_thread(new_owner)?;
+                }
             }
-            Some((current_owner, mutex.lock_count))
+            Ok(Some((current_owner, old_lock_count)))
         } else {
-            None
+            // Mutex is unlocked.
+            Ok(None)
         }
     }
 
     #[inline]
-    /// Take a thread out the queue waiting for the lock.
+    /// Put the thread into the queue waiting for the lock.
     fn mutex_enqueue(&mut self, id: MutexId, thread: ThreadId) {
         let this = self.eval_context_mut();
         this.machine.threads.sync.mutexes[id].queue.push_back(thread);
     }
 
     #[inline]
-    /// Take a thread out the queue waiting for the lock.
+    /// Take a thread out of the queue waiting for the lock.
     fn mutex_dequeue(&mut self, id: MutexId) -> Option<ThreadId> {
         let this = self.eval_context_mut();
         this.machine.threads.sync.mutexes[id].queue.pop_front()
@@ -187,7 +201,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
     fn rwlock_is_locked(&mut self, id: RwLockId) -> bool {
         let this = self.eval_context_mut();
         this.machine.threads.sync.rwlocks[id].writer.is_some()
-            || !this.machine.threads.sync.rwlocks[id].readers.is_empty()
+            || this.machine.threads.sync.rwlocks[id].readers.is_empty().not()
     }
 
     #[inline]
@@ -197,16 +211,18 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         this.machine.threads.sync.rwlocks[id].writer.is_some()
     }
 
-    /// Add a reader that collectively with other readers owns the lock.
-    fn rwlock_reader_add(&mut self, id: RwLockId, reader: ThreadId) {
+    /// Read-lock the lock by adding the `reader` the list of threads that own
+    /// this lock.
+    fn rwlock_reader_lock(&mut self, id: RwLockId, reader: ThreadId) {
         let this = self.eval_context_mut();
         assert!(!this.rwlock_is_write_locked(id), "the lock is write locked");
         let count = this.machine.threads.sync.rwlocks[id].readers.entry(reader).or_insert(0);
-        *count += 1;
+        *count = count.checked_add(1).expect("the reader counter overflowed");
     }
 
-    /// Try removing the reader. Returns `true` if succeeded.
-    fn rwlock_reader_remove(&mut self, id: RwLockId, reader: ThreadId) -> bool {
+    /// Try read-unlock the lock for `reader`. Returns `true` if succeeded,
+    /// `false` if this `reader` did not hold the lock.
+    fn rwlock_reader_unlock(&mut self, id: RwLockId, reader: ThreadId) -> bool {
         let this = self.eval_context_mut();
         match this.machine.threads.sync.rwlocks[id].readers.entry(reader) {
             Entry::Occupied(mut entry) => {
@@ -222,41 +238,51 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
     }
 
     #[inline]
-    /// Put the reader in the queue waiting for the lock.
-    fn rwlock_enqueue_reader(&mut self, id: RwLockId, reader: ThreadId) {
+    /// Put the reader in the queue waiting for the lock and block it.
+    fn rwlock_enqueue_and_block_reader(
+        &mut self,
+        id: RwLockId,
+        reader: ThreadId,
+    ) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
         assert!(this.rwlock_is_write_locked(id), "queueing on not write locked lock");
         this.machine.threads.sync.rwlocks[id].reader_queue.push_back(reader);
+        this.block_thread(reader)
     }
 
     #[inline]
-    /// Take the reader out the queue waiting for the lock.
+    /// Take a reader out the queue waiting for the lock.
     fn rwlock_dequeue_reader(&mut self, id: RwLockId) -> Option<ThreadId> {
         let this = self.eval_context_mut();
         this.machine.threads.sync.rwlocks[id].reader_queue.pop_front()
     }
 
     #[inline]
     /// Lock by setting the writer that owns the lock.
-    fn rwlock_writer_set(&mut self, id: RwLockId, writer: ThreadId) {
+    fn rwlock_writer_lock(&mut self, id: RwLockId, writer: ThreadId) {
         let this = self.eval_context_mut();
         assert!(!this.rwlock_is_locked(id), "the lock is already locked");
         this.machine.threads.sync.rwlocks[id].writer = Some(writer);
     }
 
     #[inline]
-    /// Try removing the writer.
-    fn rwlock_writer_remove(&mut self, id: RwLockId) -> Option<ThreadId> {
+    /// Try to unlock by removing the writer.
+    fn rwlock_writer_unlock(&mut self, id: RwLockId) -> Option<ThreadId> {
         let this = self.eval_context_mut();
         this.machine.threads.sync.rwlocks[id].writer.take()
     }
 
     #[inline]
     /// Put the writer in the queue waiting for the lock.
-    fn rwlock_enqueue_writer(&mut self, id: RwLockId, writer: ThreadId) {
+    fn rwlock_enqueue_and_block_writer(
+        &mut self,
+        id: RwLockId,
+        writer: ThreadId,
+    ) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
         assert!(this.rwlock_is_locked(id), "queueing on unlocked lock");
         this.machine.threads.sync.rwlocks[id].writer_queue.push_back(writer);
+        this.block_thread(writer)
     }
 
     #[inline]

src/thread.rs

@@ -32,7 +32,7 @@ pub enum SchedulingAction {
     Stop,
 }
 
-/// Timeout timeout_callbacks can be created by synchronization primitives to tell the
+/// Timeout callbacks can be created by synchronization primitives to tell the
 /// scheduler that they should be called once some period of time passes.
 type TimeoutCallback<'mir, 'tcx> =
     Box<dyn FnOnce(&mut InterpCx<'mir, 'tcx, Evaluator<'mir, 'tcx>>) -> InterpResult<'tcx> + 'tcx>;
@@ -189,7 +189,7 @@ struct TimeoutCallbackInfo<'mir, 'tcx> {
 
 impl<'mir, 'tcx> std::fmt::Debug for TimeoutCallbackInfo<'mir, 'tcx> {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        write!(f, "CallBack({:?})", self.call_time)
+        write!(f, "TimeoutCallback({:?})", self.call_time)
     }
 }
 
@@ -394,7 +394,8 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> {
 
     /// Get a callback that is ready to be called.
     fn get_ready_callback(&mut self) -> Option<(ThreadId, TimeoutCallback<'mir, 'tcx>)> {
-        // We use a for loop here to make the scheduler more deterministic.
+        // We iterate over all threads in the order of their indices because
+        // this allows us to have a deterministic scheduler.
         for thread in self.threads.indices() {
             match self.timeout_callbacks.entry(thread) {
                 Entry::Occupied(entry) =>

tests/compile-fail/sync/libc_pthread_rwlock_read_wrong_owner.rs

@@ -0,0 +1,32 @@
+// ignore-windows: No libc on Windows
+
+#![feature(rustc_private)]
+
+extern crate libc;
+
+use std::cell::UnsafeCell;
+use std::sync::Arc;
+use std::thread;
+
+struct RwLock(UnsafeCell<libc::pthread_rwlock_t>);
+
+unsafe impl Send for RwLock {}
+unsafe impl Sync for RwLock {}
+
+fn new_lock() -> Arc<RwLock> {
+    Arc::new(RwLock(UnsafeCell::new(libc::PTHREAD_RWLOCK_INITIALIZER)))
+}
+
+fn main() {
+    unsafe {
+        let lock = new_lock();
+        assert_eq!(libc::pthread_rwlock_rdlock(lock.0.get() as *mut _), 0);
+
+        let lock_copy = lock.clone();
+        thread::spawn(move || {
+            assert_eq!(libc::pthread_rwlock_unlock(lock_copy.0.get() as *mut _), 0); //~ ERROR: Undefined Behavior: unlocked an rwlock that was not locked by the active thread
+        })
+        .join()
+        .unwrap();
+    }
+}