move rwlock dequeuing to shared code, and use that code for Windows rwlocks

Author: RalfJung

src/shims/posix/sync.rs

@@ -1,6 +1,5 @@
 use std::convert::TryInto;
 use std::time::{Duration, SystemTime};
-use std::ops::Not;
 
 use crate::*;
 use stacked_borrows::Tag;
@@ -548,27 +547,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         let active_thread = this.get_active_thread();
 
         if this.rwlock_reader_unlock(id, active_thread) {
-            // The thread was a reader.
-            if this.rwlock_is_locked(id).not() {
-                // No more readers owning the lock. Give it to a writer if there
-                // is any.
-                this.rwlock_dequeue_and_lock_writer(id);
-            }
             Ok(0)
-        } 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
-            // result, not only readers can starve writers, but also writers can
-            // starve readers.
-            if this.rwlock_dequeue_and_lock_writer(id) {
-                // Someone got the write lock, nice.
-            } else {
-                // Give the lock to all readers.
-                while this.rwlock_dequeue_and_lock_reader(id) {
-                    // Rinse and repeat.
-                }
-            }
+        } else if this.rwlock_writer_unlock(id, active_thread) {
             Ok(0)
         } else {
             throw_ub_format!("unlocked an rwlock that was not locked by the active thread");

src/shims/windows/foreign_items.rs

@@ -257,7 +257,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
 
             // Better error for attempts to create a thread
             "CreateThread" => {
-                throw_unsup_format!("Miri does not support threading");
+                throw_unsup_format!("Miri does not support concurrency on Windows");
             }
 
             // Incomplete shims that we "stub out" just to get pre-main initialization code to work.
@@ -292,7 +292,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             if this.frame().instance.to_string().starts_with("std::sys::windows::") => {
                 #[allow(non_snake_case)]
                 let &[_lpCriticalSection] = check_arg_count(args)?;
-                assert_eq!(this.get_total_thread_count(), 1, "concurrency on Windows not supported");
+                assert_eq!(this.get_total_thread_count(), 1, "concurrency on Windows is not supported");
                 // Nothing to do, not even a return value.
                 // (Windows locks are reentrant, and we have only 1 thread,
                 // so not doing any futher checks here is at least not incorrect.)
@@ -301,7 +301,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             if this.frame().instance.to_string().starts_with("std::sys::windows::") => {
                 #[allow(non_snake_case)]
                 let &[_lpCriticalSection] = check_arg_count(args)?;
-                assert_eq!(this.get_total_thread_count(), 1, "concurrency on Windows not supported");
+                assert_eq!(this.get_total_thread_count(), 1, "concurrency on Windows is not supported");
                 // There is only one thread, so this always succeeds and returns TRUE.
                 this.write_scalar(Scalar::from_i32(1), dest)?;
             }

src/shims/windows/sync.rs

@@ -1,19 +1,22 @@
-use rustc_target::abi::Size;
-
 use crate::*;
 
 // Locks are pointer-sized pieces of data, initialized to 0.
-// We use them to count readers, with usize::MAX representing the write-locked state.
+// We use the first 4 bytes to store the RwLockId.
 
-fn deref_lock<'mir, 'tcx: 'mir>(
+fn srwlock_get_or_create_id<'mir, 'tcx: 'mir>(
     ecx: &mut MiriEvalContext<'mir, 'tcx>,
     lock_op: OpTy<'tcx, Tag>,
-) -> InterpResult<'tcx, MPlaceTy<'tcx, Tag>> {
-    // `lock` is a pointer to `void*`; cast it to a pointer to `usize`.
-    let lock = ecx.deref_operand(lock_op)?;
-    let usize = ecx.machine.layouts.usize;
-    assert_eq!(lock.layout.size, usize.size);
-    Ok(lock.offset(Size::ZERO, MemPlaceMeta::None, usize, ecx)?)
+) -> InterpResult<'tcx, RwLockId> {
+    let id = ecx.read_scalar_at_offset(lock_op, 0, ecx.machine.layouts.u32)?.to_u32()?;
+    if id == 0 {
+        // 0 is a default value and also not a valid rwlock id. Need to allocate
+        // a new rwlock.
+        let id = ecx.rwlock_create();
+        ecx.write_scalar_at_offset(lock_op, 0, id.to_u32_scalar(), ecx.machine.layouts.u32)?;
+        Ok(id)
+    } else {
+        Ok(RwLockId::from_u32(id))
+    }
 }
 
 impl<'mir, 'tcx> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {}
@@ -24,17 +27,20 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         lock_op: OpTy<'tcx, Tag>,
     ) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
-        assert_eq!(this.get_total_thread_count(), 1, "concurrency on Windows is not supported");
-
-        let lock = deref_lock(this, lock_op)?;
-        let lock_val = this.read_scalar(lock.into())?.to_machine_usize(this)?;
-        if lock_val == 0 {
-            // Currently not locked. Lock it.
-            let new_val = Scalar::from_machine_usize(this.machine_usize_max(), this);
-            this.write_scalar(new_val, lock.into())?;
+        let id = srwlock_get_or_create_id(this, lock_op)?;
+        let active_thread = this.get_active_thread();
+
+        if this.rwlock_is_locked(id) {
+            // Note: this will deadlock if the lock is already locked by this
+            // thread in any way.
+            //
+            // FIXME: Detect and report the deadlock proactively. (We currently
+            // report the deadlock only when no thread can continue execution,
+            // but we could detect that this lock is already locked and report
+            // an error.)
+            this.rwlock_enqueue_and_block_writer(id, active_thread);
         } else {
-            // Lock is already held. This is a deadlock.
-            throw_machine_stop!(TerminationInfo::Deadlock);
+            this.rwlock_writer_lock(id, active_thread);
         }
 
         Ok(())
@@ -46,18 +52,15 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         lock_op: OpTy<'tcx, Tag>,
     ) -> InterpResult<'tcx, u8> {
         let this = self.eval_context_mut();
-        assert_eq!(this.get_total_thread_count(), 1, "concurrency on Windows is not supported");
-
-        let lock = deref_lock(this, lock_op)?;
-        let lock_val = this.read_scalar(lock.into())?.to_machine_usize(this)?;
-        if lock_val == 0 {
-            // Currently not locked. Lock it.
-            let new_val = this.machine_usize_max();
-            this.write_scalar(Scalar::from_machine_usize(new_val, this), lock.into())?;
-            Ok(1)
-        } else {
+        let id = srwlock_get_or_create_id(this, lock_op)?;
+        let active_thread = this.get_active_thread();
+
+        if this.rwlock_is_locked(id) {
             // Lock is already held.
             Ok(0)
+        } else {
+            this.rwlock_writer_lock(id, active_thread);
+            Ok(1)
         }
     }
 
@@ -67,17 +70,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         lock_op: OpTy<'tcx, Tag>,
     ) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
-        assert_eq!(this.get_total_thread_count(), 1, "concurrency on Windows is not supported");
-
-        let lock = deref_lock(this, lock_op)?;
-        let lock_val = this.read_scalar(lock.into())?.to_machine_usize(this)?;
-        if lock_val == this.machine_usize_max() {
-            // Currently locked. Unlock it.
-            let new_val = 0;
-            this.write_scalar(Scalar::from_machine_usize(new_val, this), lock.into())?;
-        } else {
-            // Lock is not locked.
-            throw_ub_format!("calling ReleaseSRWLockExclusive on an SRWLock that is not exclusively locked");
+        let id = srwlock_get_or_create_id(this, lock_op)?;
+        let active_thread = this.get_active_thread();
+
+        if !this.rwlock_writer_unlock(id, active_thread) {
+            // The docs do not say anything about this case, but it seems better to not allow it.
+            throw_ub_format!("calling ReleaseSRWLockExclusive on an SRWLock that is not exclusively locked by the current thread");
         }
 
         Ok(())
@@ -89,21 +87,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         lock_op: OpTy<'tcx, Tag>,
     ) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
-        assert_eq!(this.get_total_thread_count(), 1, "concurrency on Windows is not supported");
+        let id = srwlock_get_or_create_id(this, lock_op)?;
+        let active_thread = this.get_active_thread();
 
-        let lock = deref_lock(this, lock_op)?;
-        let lock_val = this.read_scalar(lock.into())?.to_machine_usize(this)?;
-        if lock_val == this.machine_usize_max() {
-            // Currently write locked. This is a deadlock.
-            throw_machine_stop!(TerminationInfo::Deadlock);
+        if this.rwlock_is_write_locked(id) {
+            this.rwlock_enqueue_and_block_reader(id, active_thread);
         } else {
-            // Bump up read counter (cannot overflow as we just checkd against usize::MAX);
-            let new_val = lock_val+1;
-            // Make sure this does not reach the "write locked" flag.
-            if new_val == this.machine_usize_max() {
-                throw_unsup_format!("SRWLock read-acquired too many times");
-            }
-            this.write_scalar(Scalar::from_machine_usize(new_val, this), lock.into())?;
+            this.rwlock_reader_lock(id, active_thread);
         }
 
         Ok(())
@@ -115,21 +105,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         lock_op: OpTy<'tcx, Tag>,
     ) -> InterpResult<'tcx, u8> {
         let this = self.eval_context_mut();
-        assert_eq!(this.get_total_thread_count(), 1, "concurrency on Windows is not supported");
+        let id = srwlock_get_or_create_id(this, lock_op)?;
+        let active_thread = this.get_active_thread();
 
-        let lock = deref_lock(this, lock_op)?;
-        let lock_val = this.read_scalar(lock.into())?.to_machine_usize(this)?;
-        if lock_val == this.machine_usize_max() {
-            // Currently write locked.
+        if this.rwlock_is_write_locked(id) {
             Ok(0)
         } else {
-            // Bump up read counter (cannot overflow as we just checkd against usize::MAX);
-            let new_val = lock_val+1;
-            // Make sure this does not reach the "write locked" flag.
-            if new_val == this.machine_usize_max() {
-                throw_unsup_format!("SRWLock read-acquired too many times");
-            }
-            this.write_scalar(Scalar::from_machine_usize(new_val, this), lock.into())?;
+            this.rwlock_reader_lock(id, active_thread);
             Ok(1)
         }
     }
@@ -140,20 +122,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         lock_op: OpTy<'tcx, Tag>,
     ) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
-        assert_eq!(this.get_total_thread_count(), 1, "concurrency on Windows is not supported");
-
-        let lock = deref_lock(this, lock_op)?;
-        let lock_val = this.read_scalar(lock.into())?.to_machine_usize(this)?;
-        if lock_val == this.machine_usize_max() {
-            // Currently write locked. This is a UB.
-            throw_ub_format!("calling ReleaseSRWLockShared on write-locked SRWLock");
-        } else if lock_val == 0 {
-            // Currently not locked at all.
-            throw_ub_format!("calling ReleaseSRWLockShared on unlocked SRWLock");
-        } else {
-            // Decrement read counter (cannot overflow as we just checkd against 0);
-            let new_val = lock_val-1;
-            this.write_scalar(Scalar::from_machine_usize(new_val, this), lock.into())?;
+        let id = srwlock_get_or_create_id(this, lock_op)?;
+        let active_thread = this.get_active_thread();
+
+        if !this.rwlock_reader_unlock(id, active_thread) {
+            // The docs do not say anything about this case, but it seems better to not allow it.
+            throw_ub_format!("calling ReleaseSRWLockShared on an SRWLock that is not locked by the current thread");
         }
 
         Ok(())