Auto merge of #77380 - fusion-engineering-forks:unbox-the-mutex, r=dtolnay

Unbox mutexes and condvars on some platforms

Both mutexes and condition variables contained a Box containing the actual os-specific object. This was done because moving these objects may cause undefined behaviour on some platforms.

However, this is not needed on Windows[1], Wasm[2], cloudabi[2], and 'unsupported'[3], were the box was only needlessly making them less efficient.

This change gets rid of the box on those platforms.

On those platforms, `Condvar` can no longer verify it is only used with one `Mutex`, as mutexes no longer have a stable address. This was addressed and considered acceptable in #76932.

[1]\: https://docs.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-initializesrwlock
[2]\: These are just a single atomic integer together with futex wait/wake calls/instructions.
[3]\: The `unsupported` platform doesn't support multiple threads at all.

3 files changed, 83 insertions, 42 deletions

diff --git a/library/std/src/sys_common/condvar.rs b/library/std/src/sys_common/condvar.rs
index a48d301f812..2c02e1cd33c 100644
--- a/library/std/src/sys_common/condvar.rs
+++ b/library/std/src/sys_common/condvar.rs
@@ -1,72 +1,64 @@
 use crate::sys::condvar as imp;
+use crate::sys::mutex as mutex_imp;
 use crate::sys_common::mutex::MovableMutex;
 use crate::time::Duration;
 
+mod check;
+
+type CondvarCheck = <mutex_imp::MovableMutex as check::CondvarCheck>::Check;
+
 /// An OS-based condition variable.
-///
-/// This structure is the lowest layer possible on top of the OS-provided
-/// condition variables. It is consequently entirely unsafe to use. It is
-/// recommended to use the safer types at the top level of this crate instead of
-/// this type.
-pub struct Condvar(imp::Condvar);
+pub struct Condvar {
+    inner: imp::MovableCondvar,
+    check: CondvarCheck,
+}
 
 impl Condvar {
     /// Creates a new condition variable for use.
-    ///
-    /// Behavior is undefined if the condition variable is moved after it is
-    /// first used with any of the functions below.
-    pub const fn new() -> Condvar {
-        Condvar(imp::Condvar::new())
-    }
-
-    /// Prepares the condition variable for use.
-    ///
-    /// This should be called once the condition variable is at a stable memory
-    /// address.
-    #[inline]
-    pub unsafe fn init(&mut self) {
-        self.0.init()
+    pub fn new() -> Self {
+        let mut c = imp::MovableCondvar::from(imp::Condvar::new());
+        unsafe { c.init() };
+        Self { inner: c, check: CondvarCheck::new() }
     }
 
     /// Signals one waiter on this condition variable to wake up.
     #[inline]
-    pub unsafe fn notify_one(&self) {
-        self.0.notify_one()
+    pub fn notify_one(&self) {
+        unsafe { self.inner.notify_one() };
     }
 
     /// Awakens all current waiters on this condition variable.
     #[inline]
-    pub unsafe fn notify_all(&self) {
-        self.0.notify_all()
+    pub fn notify_all(&self) {
+        unsafe { self.inner.notify_all() };
     }
 
     /// Waits for a signal on the specified mutex.
     ///
     /// Behavior is undefined if the mutex is not locked by the current thread.
-    /// Behavior is also undefined if more than one mutex is used concurrently
-    /// on this condition variable.
+    ///
+    /// May panic if used with more than one mutex.
     #[inline]
     pub unsafe fn wait(&self, mutex: &MovableMutex) {
-        self.0.wait(mutex.raw())
+        self.check.verify(mutex);
+        self.inner.wait(mutex.raw())
     }
 
     /// Waits for a signal on the specified mutex with a timeout duration
     /// specified by `dur` (a relative time into the future).
     ///
     /// Behavior is undefined if the mutex is not locked by the current thread.
-    /// Behavior is also undefined if more than one mutex is used concurrently
-    /// on this condition variable.
+    ///
+    /// May panic if used with more than one mutex.
     #[inline]
     pub unsafe fn wait_timeout(&self, mutex: &MovableMutex, dur: Duration) -> bool {
-        self.0.wait_timeout(mutex.raw(), dur)
+        self.check.verify(mutex);
+        self.inner.wait_timeout(mutex.raw(), dur)
     }
+}
 
-    /// Deallocates all resources associated with this condition variable.
-    ///
-    /// Behavior is undefined if there are current or will be future users of
-    /// this condition variable.
-    #[inline]
-    pub unsafe fn destroy(&self) {
-        self.0.destroy()
+impl Drop for Condvar {
+    fn drop(&mut self) {
+        unsafe { self.inner.destroy() };
     }
 }
diff --git a/library/std/src/sys_common/condvar/check.rs b/library/std/src/sys_common/condvar/check.rs
new file mode 100644
index 00000000000..fecb732b910
--- /dev/null
+++ b/library/std/src/sys_common/condvar/check.rs
@@ -0,0 +1,48 @@
+use crate::sync::atomic::{AtomicUsize, Ordering};
+use crate::sys::mutex as mutex_imp;
+use crate::sys_common::mutex::MovableMutex;
+
+pub trait CondvarCheck {
+    type Check;
+}
+
+/// For boxed mutexes, a `Condvar` will check it's only ever used with the same
+/// mutex, based on its (stable) address.
+impl CondvarCheck for Box<mutex_imp::Mutex> {
+    type Check = SameMutexCheck;
+}
+
+pub struct SameMutexCheck {
+    addr: AtomicUsize,
+}
+
+#[allow(dead_code)]
+impl SameMutexCheck {
+    pub const fn new() -> Self {
+        Self { addr: AtomicUsize::new(0) }
+    }
+    pub fn verify(&self, mutex: &MovableMutex) {
+        let addr = mutex.raw() as *const mutex_imp::Mutex as usize;
+        match self.addr.compare_and_swap(0, addr, Ordering::SeqCst) {
+            0 => {}              // Stored the address
+            n if n == addr => {} // Lost a race to store the same address
+            _ => panic!("attempted to use a condition variable with two mutexes"),
+        }
+    }
+}
+
+/// Unboxed mutexes may move, so `Condvar` can not require its address to stay
+/// constant.
+impl CondvarCheck for mutex_imp::Mutex {
+    type Check = NoCheck;
+}
+
+pub struct NoCheck;
+
+#[allow(dead_code)]
+impl NoCheck {
+    pub const fn new() -> Self {
+        Self
+    }
+    pub fn verify(&self, _: &MovableMutex) {}
+}
diff --git a/library/std/src/sys_common/mutex.rs b/library/std/src/sys_common/mutex.rs
index 93ec7d89bc5..a1e11d24465 100644
--- a/library/std/src/sys_common/mutex.rs
+++ b/library/std/src/sys_common/mutex.rs
@@ -58,21 +58,22 @@ impl Drop for StaticMutexGuard<'_> {
 ///
 /// This mutex does not implement poisoning.
 ///
-/// This is a wrapper around `Box<imp::Mutex>`, to allow the object to be moved
-/// without moving the raw mutex.
-pub struct MovableMutex(Box<imp::Mutex>);
+/// This is either a wrapper around `Box<imp::Mutex>` or `imp::Mutex`,
+/// depending on the platform. It is boxed on platforms where `imp::Mutex` may
+/// not be moved.
+pub struct MovableMutex(imp::MovableMutex);
 
 unsafe impl Sync for MovableMutex {}
 
 impl MovableMutex {
     /// Creates a new mutex.
     pub fn new() -> Self {
-        let mut mutex = box imp::Mutex::new();
+        let mut mutex = imp::MovableMutex::from(imp::Mutex::new());
         unsafe { mutex.init() };
         Self(mutex)
     }
 
-    pub(crate) fn raw(&self) -> &imp::Mutex {
+    pub(super) fn raw(&self) -> &imp::Mutex {
         &self.0
     }