Auto merge of #1441 - RalfJung:sync-cleanup, r=RalfJung

Synchronization primitive cleanup

Make some methods infallible, move a bit more work into the platform-independent `sync.rs`, and fix a bug in rwlock unlocking.

Author: bors

README.md

@@ -112,7 +112,7 @@ There is no way to list all the infinite things Miri cannot do, but the
 interpreter will explicitly tell you when it finds something unsupported:
 
 ```
-error: unsupported operation: Miri does not support threading
+error: unsupported operation: can't call foreign function: bind
     ...
     = help: this is likely not a bug in the program; it indicates that the program \
             performed an operation that the interpreter does not support

src/shims/foreign_items/posix.rs

@@ -254,14 +254,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             "pthread_getspecific" => {
                 let &[key] = check_arg_count(args)?;
                 let key = this.force_bits(this.read_scalar(key)?.not_undef()?, key.layout.size)?;
-                let active_thread = this.get_active_thread()?;
+                let active_thread = this.get_active_thread();
                 let ptr = this.machine.tls.load_tls(key, active_thread, this)?;
                 this.write_scalar(ptr, dest)?;
             }
             "pthread_setspecific" => {
                 let &[key, new_ptr] = check_arg_count(args)?;
                 let key = this.force_bits(this.read_scalar(key)?.not_undef()?, key.layout.size)?;
-                let active_thread = this.get_active_thread()?;
+                let active_thread = this.get_active_thread();
                 let new_ptr = this.read_scalar(new_ptr)?.not_undef()?;
                 this.machine.tls.store_tls(key, active_thread, this.test_null(new_ptr)?)?;
 

src/shims/foreign_items/posix/macos.rs

@@ -98,7 +98,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                 let dtor = this.read_scalar(dtor)?.not_undef()?;
                 let dtor = this.memory.get_fn(dtor)?.as_instance()?;
                 let data = this.read_scalar(data)?.not_undef()?;
-                let active_thread = this.get_active_thread()?;
+                let active_thread = this.get_active_thread();
                 this.machine.tls.set_macos_thread_dtor(active_thread, dtor, data)?;
             }
 

src/shims/foreign_items/windows.rs

@@ -163,14 +163,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             "TlsGetValue" => {
                 let &[key] = check_arg_count(args)?;
                 let key = u128::from(this.read_scalar(key)?.to_u32()?);
-                let active_thread = this.get_active_thread()?;
+                let active_thread = this.get_active_thread();
                 let ptr = this.machine.tls.load_tls(key, active_thread, this)?;
                 this.write_scalar(ptr, dest)?;
             }
             "TlsSetValue" => {
                 let &[key, new_ptr] = check_arg_count(args)?;
                 let key = u128::from(this.read_scalar(key)?.to_u32()?);
-                let active_thread = this.get_active_thread()?;
+                let active_thread = this.get_active_thread();
                 let new_ptr = this.read_scalar(new_ptr)?.not_undef()?;
                 this.machine.tls.store_tls(key, active_thread, this.test_null(new_ptr)?)?;
 
@@ -291,7 +291,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 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.)
@@ -300,7 +300,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 not supported");
                 // There is only one thread, so this always succeeds and returns TRUE
                 this.write_scalar(Scalar::from_i32(1), dest)?;
             }

src/shims/sync.rs

@@ -1,5 +1,6 @@
 use std::convert::TryInto;
 use std::time::{Duration, SystemTime};
+use std::ops::Not;
 
 use rustc_middle::ty::{layout::TyAndLayout, TyKind, TypeAndMut};
 use rustc_target::abi::{LayoutOf, Size};
@@ -284,15 +285,16 @@ fn reacquire_cond_mutex<'mir, 'tcx: 'mir>(
     thread: ThreadId,
     mutex: MutexId,
 ) -> InterpResult<'tcx> {
+    ecx.unblock_thread(thread);
     if ecx.mutex_is_locked(mutex) {
-        ecx.mutex_enqueue(mutex, thread);
+        ecx.mutex_enqueue_and_block(mutex, thread);
     } else {
         ecx.mutex_lock(mutex, thread);
-        ecx.unblock_thread(thread)?;
     }
     Ok(())
 }
 
+/// After a thread waiting on a condvar was signalled:
 /// Reacquire the conditional variable and remove the timeout callback if any
 /// was registered.
 fn post_cond_signal<'mir, 'tcx: 'mir>(
@@ -303,24 +305,25 @@ fn post_cond_signal<'mir, 'tcx: 'mir>(
     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)
+    ecx.unregister_timeout_callback_if_exists(thread);
+    Ok(())
 }
 
 /// Release the mutex associated with the condition variable because we are
 /// entering the waiting state.
-fn release_cond_mutex<'mir, 'tcx: 'mir>(
+fn release_cond_mutex_and_block<'mir, 'tcx: 'mir>(
     ecx: &mut MiriEvalContext<'mir, 'tcx>,
     active_thread: ThreadId,
     mutex: MutexId,
 ) -> InterpResult<'tcx> {
-    if let Some(old_locked_count) = ecx.mutex_unlock(mutex, active_thread)? {
+    if let Some(old_locked_count) = ecx.mutex_unlock(mutex, active_thread) {
         if old_locked_count != 1 {
             throw_unsup_format!("awaiting on a lock acquired multiple times is not supported");
         }
     } else {
         throw_ub_format!("awaiting on unlocked or owned by a different thread mutex");
     }
-    ecx.block_thread(active_thread)?;
+    ecx.block_thread(active_thread);
     Ok(())
 }
 
@@ -411,14 +414,13 @@ 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)?;
-        let active_thread = this.get_active_thread()?;
+        let active_thread = this.get_active_thread();
 
         if this.mutex_is_locked(id) {
             let owner_thread = this.mutex_get_owner(id);
             if owner_thread != active_thread {
-                // Block the active thread.
-                this.block_thread(active_thread)?;
-                this.mutex_enqueue(id, active_thread);
+                // Enqueue the active thread.
+                this.mutex_enqueue_and_block(id, active_thread);
                 Ok(0)
             } else {
                 // Trying to acquire the same mutex again.
@@ -449,7 +451,7 @@ 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)?;
-        let active_thread = this.get_active_thread()?;
+        let active_thread = this.get_active_thread();
 
         if this.mutex_is_locked(id) {
             let owner_thread = this.mutex_get_owner(id);
@@ -482,9 +484,9 @@ 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)?;
-        let active_thread = this.get_active_thread()?;
+        let active_thread = this.get_active_thread();
 
-        if let Some(_old_locked_count) = this.mutex_unlock(id, active_thread)? {
+        if let Some(_old_locked_count) = this.mutex_unlock(id, active_thread) {
             // The mutex was locked by the current thread.
             Ok(0)
         } else {
@@ -528,10 +530,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         let this = self.eval_context_mut();
 
         let id = rwlock_get_or_create_id(this, rwlock_op)?;
-        let active_thread = this.get_active_thread()?;
+        let active_thread = this.get_active_thread();
 
         if this.rwlock_is_write_locked(id) {
-            this.rwlock_enqueue_and_block_reader(id, active_thread)?;
+            this.rwlock_enqueue_and_block_reader(id, active_thread);
             Ok(0)
         } else {
             this.rwlock_reader_lock(id, active_thread);
@@ -543,7 +545,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         let this = self.eval_context_mut();
 
         let id = rwlock_get_or_create_id(this, rwlock_op)?;
-        let active_thread = this.get_active_thread()?;
+        let active_thread = this.get_active_thread();
 
         if this.rwlock_is_write_locked(id) {
             this.eval_libc_i32("EBUSY")
@@ -557,22 +559,22 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         let this = self.eval_context_mut();
 
         let id = rwlock_get_or_create_id(this, rwlock_op)?;
-        let active_thread = this.get_active_thread()?;
+        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.
             //
             // Relevant documentation:
             // https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_rwlock_wrlock.html
-            // An in depth discussion on this topic:
+            // An in-depth discussion on this topic:
             // https://github.com/rust-lang/rust/issues/53127
             //
             // 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)?;
+            this.rwlock_enqueue_and_block_writer(id, active_thread);
         } else {
             this.rwlock_writer_lock(id, active_thread);
         }
@@ -584,7 +586,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         let this = self.eval_context_mut();
 
         let id = rwlock_get_or_create_id(this, rwlock_op)?;
-        let active_thread = this.get_active_thread()?;
+        let active_thread = this.get_active_thread();
 
         if this.rwlock_is_locked(id) {
             this.eval_libc_i32("EBUSY")
@@ -598,17 +600,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         let this = self.eval_context_mut();
 
         let id = rwlock_get_or_create_id(this, rwlock_op)?;
-        let active_thread = this.get_active_thread()?;
+        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) {
+            if this.rwlock_is_locked(id).not() {
                 // 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_lock(id, writer);
-                }
+                this.rwlock_dequeue_and_lock_writer(id);
             }
             Ok(0)
         } else if Some(active_thread) == this.rwlock_writer_unlock(id) {
@@ -617,15 +616,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             // We are prioritizing writers here against the readers. As a
             // result, not only readers can starve writers, but also writers can
             // starve readers.
-            if let Some(writer) = this.rwlock_dequeue_writer(id) {
-                // Give the lock to another writer.
-                this.unblock_thread(writer)?;
-                this.rwlock_writer_lock(id, writer);
+            if this.rwlock_dequeue_and_lock_writer(id) {
+                // Someone got the write lock, nice.
             } else {
                 // Give the lock to all readers.
-                while let Some(reader) = this.rwlock_dequeue_reader(id) {
-                    this.unblock_thread(reader)?;
-                    this.rwlock_reader_lock(id, reader);
+                while this.rwlock_dequeue_and_lock_reader(id) {
+                    // Rinse and repeat.
                 }
             }
             Ok(0)
@@ -753,9 +749,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
 
         let id = cond_get_or_create_id(this, cond_op)?;
         let mutex_id = mutex_get_or_create_id(this, mutex_op)?;
-        let active_thread = this.get_active_thread()?;
+        let active_thread = this.get_active_thread();
 
-        release_cond_mutex(this, active_thread, mutex_id)?;
+        release_cond_mutex_and_block(this, active_thread, mutex_id)?;
         this.condvar_wait(id, active_thread, mutex_id);
 
         Ok(0)
@@ -774,9 +770,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
 
         let id = cond_get_or_create_id(this, cond_op)?;
         let mutex_id = mutex_get_or_create_id(this, mutex_op)?;
-        let active_thread = this.get_active_thread()?;
+        let active_thread = this.get_active_thread();
 
-        release_cond_mutex(this, active_thread, mutex_id)?;
+        release_cond_mutex_and_block(this, active_thread, mutex_id)?;
         this.condvar_wait(id, active_thread, mutex_id);
 
         // We return success for now and override it in the timeout callback.
@@ -823,7 +819,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
 
                 Ok(())
             }),
-        )?;
+        );
 
         Ok(())
     }
@@ -833,7 +829,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
 
         let id = cond_get_or_create_id(this, cond_op)?;
         if this.condvar_is_awaited(id) {
-            throw_ub_format!("destroyed an awaited conditional variable");
+            throw_ub_format!("destroying an awaited conditional variable");
         }
         cond_set_id(this, cond_op, ScalarMaybeUninit::Uninit)?;
         cond_set_clock_id(this, cond_op, ScalarMaybeUninit::Uninit)?;

src/shims/thread.rs

@@ -19,9 +19,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
              For example, Miri does not detect data races yet.",
         );
 
-        let new_thread_id = this.create_thread()?;
+        let new_thread_id = this.create_thread();
         // Also switch to new thread so that we can push the first stackframe.
-        let old_thread_id = this.set_active_thread(new_thread_id)?;
+        let old_thread_id = this.set_active_thread(new_thread_id);
 
         let thread_info_place = this.deref_operand(thread)?;
         this.write_scalar(
@@ -47,7 +47,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             StackPopCleanup::None { cleanup: true },
         )?;
 
-        this.set_active_thread(old_thread_id)?;
+        this.set_active_thread(old_thread_id);
 
         Ok(0)
     }
@@ -82,7 +82,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
     fn pthread_self(&mut self, dest: PlaceTy<'tcx, Tag>) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
 
-        let thread_id = this.get_active_thread()?;
+        let thread_id = this.get_active_thread();
         this.write_scalar(Scalar::from_uint(thread_id.to_u32(), dest.layout.size), dest)
     }
 
@@ -105,10 +105,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             // byte. Since `read_c_str` returns the string without the null
             // byte, we need to truncate to 15.
             name.truncate(15);
-            this.set_active_thread_name(name)?;
+            this.set_active_thread_name(name);
         } else if option == this.eval_libc_i32("PR_GET_NAME")? {
             let address = this.read_scalar(arg2)?.not_undef()?;
-            let mut name = this.get_active_thread_name()?.to_vec();
+            let mut name = this.get_active_thread_name().to_vec();
             name.push(0u8);
             assert!(name.len() <= 16);
             this.memory.write_bytes(address, name)?;
@@ -127,15 +127,15 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         this.assert_target_os("macos", "pthread_setname_np");
 
         let name = this.memory.read_c_str(name)?.to_owned();
-        this.set_active_thread_name(name)?;
+        this.set_active_thread_name(name);
 
         Ok(())
     }
 
     fn sched_yield(&mut self) -> InterpResult<'tcx, i32> {
         let this = self.eval_context_mut();
 
-        this.yield_active_thread()?;
+        this.yield_active_thread();
 
         Ok(0)
     }