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

bors · bors · commit 32cbc65e6bf7 · 2020-10-04T06:48:17.000Z
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.
diff --git a/library/std/src/sync/condvar.rs b/library/std/src/sync/condvar.rs
@@ -2,10 +2,8 @@
 mod tests;
 
 use crate::fmt;
-use crate::sync::atomic::{AtomicUsize, Ordering};
 use crate::sync::{mutex, MutexGuard, PoisonError};
 use crate::sys_common::condvar as sys;
-use crate::sys_common::mutex as sys_mutex;
 use crate::sys_common::poison::{self, LockResult};
 use crate::time::{Duration, Instant};
 
@@ -109,8 +107,7 @@ impl WaitTimeoutResult {
 /// ```
 #[stable(feature = "rust1", since = "1.0.0")]
 pub struct Condvar {
-    inner: Box<sys::Condvar>,
-    mutex: AtomicUsize,
+    inner: sys::Condvar,
 }
 
 impl Condvar {
@@ -126,11 +123,7 @@ impl Condvar {
     /// ```
     #[stable(feature = "rust1", since = "1.0.0")]
     pub fn new() -> Condvar {
-        let mut c = Condvar { inner: box sys::Condvar::new(), mutex: AtomicUsize::new(0) };
-        unsafe {
-            c.inner.init();
-        }
-        c
+        Condvar { inner: sys::Condvar::new() }
     }
 
     /// Blocks the current thread until this condition variable receives a
@@ -192,7 +185,6 @@ impl Condvar {
     pub fn wait<'a, T>(&self, guard: MutexGuard<'a, T>) -> LockResult<MutexGuard<'a, T>> {
         let poisoned = unsafe {
             let lock = mutex::guard_lock(&guard);
-            self.verify(lock);
             self.inner.wait(lock);
             mutex::guard_poison(&guard).get()
         };
@@ -389,7 +381,6 @@ impl Condvar {
     ) -> LockResult<(MutexGuard<'a, T>, WaitTimeoutResult)> {
         let (poisoned, result) = unsafe {
             let lock = mutex::guard_lock(&guard);
-            self.verify(lock);
             let success = self.inner.wait_timeout(lock, dur);
             (mutex::guard_poison(&guard).get(), WaitTimeoutResult(!success))
         };
@@ -510,7 +501,7 @@ impl Condvar {
     /// ```
     #[stable(feature = "rust1", since = "1.0.0")]
     pub fn notify_one(&self) {
-        unsafe { self.inner.notify_one() }
+        self.inner.notify_one()
     }
 
     /// Wakes up all blocked threads on this condvar.
@@ -550,27 +541,7 @@ impl Condvar {
     /// ```
     #[stable(feature = "rust1", since = "1.0.0")]
     pub fn notify_all(&self) {
-        unsafe { self.inner.notify_all() }
-    }
-
-    fn verify(&self, mutex: &sys_mutex::MovableMutex) {
-        let addr = mutex.raw() as *const _ as usize;
-        match self.mutex.compare_and_swap(0, addr, Ordering::SeqCst) {
-            // If we got out 0, then we have successfully bound the mutex to
-            // this cvar.
-            0 => {}
-
-            // If we get out a value that's the same as `addr`, then someone
-            // already beat us to the punch.
-            n if n == addr => {}
-
-            // Anything else and we're using more than one mutex on this cvar,
-            // which is currently disallowed.
-            _ => panic!(
-                "attempted to use a condition variable with two \
-                         mutexes"
-            ),
-        }
+        self.inner.notify_all()
     }
 }
 
@@ -588,10 +559,3 @@ impl Default for Condvar {
         Condvar::new()
     }
 }
-
-#[stable(feature = "rust1", since = "1.0.0")]
-impl Drop for Condvar {
-    fn drop(&mut self) {
-        unsafe { self.inner.destroy() }
-    }
-}
diff --git a/library/std/src/sync/condvar/tests.rs b/library/std/src/sync/condvar/tests.rs
@@ -191,7 +191,7 @@ fn wait_timeout_wake() {
 
 #[test]
 #[should_panic]
-#[cfg_attr(target_os = "emscripten", ignore)]
+#[cfg_attr(not(unix), ignore)]
 fn two_mutexes() {
     let m = Arc::new(Mutex::new(()));
     let m2 = m.clone();
diff --git a/library/std/src/sys/cloudabi/condvar.rs b/library/std/src/sys/cloudabi/condvar.rs
@@ -15,6 +15,8 @@ pub struct Condvar {
     condvar: UnsafeCell<AtomicU32>,
 }
 
+pub type MovableCondvar = Condvar;
+
 unsafe impl Send for Condvar {}
 unsafe impl Sync for Condvar {}
 
diff --git a/library/std/src/sys/cloudabi/mutex.rs b/library/std/src/sys/cloudabi/mutex.rs
@@ -15,6 +15,8 @@ extern "C" {
 // implemented identically.
 pub struct Mutex(RWLock);
 
+pub type MovableMutex = Mutex;
+
 pub unsafe fn raw(m: &Mutex) -> *mut AtomicU32 {
     rwlock::raw(&m.0)
 }
diff --git a/library/std/src/sys/hermit/condvar.rs b/library/std/src/sys/hermit/condvar.rs
@@ -14,6 +14,8 @@ pub struct Condvar {
     sem2: *const c_void,
 }
 
+pub type MovableCondvar = Box<Condvar>;
+
 unsafe impl Send for Condvar {}
 unsafe impl Sync for Condvar {}
 
diff --git a/library/std/src/sys/sgx/condvar.rs b/library/std/src/sys/sgx/condvar.rs
@@ -7,6 +7,8 @@ pub struct Condvar {
     inner: SpinMutex<WaitVariable<()>>,
 }
 
+pub type MovableCondvar = Box<Condvar>;
+
 impl Condvar {
     pub const fn new() -> Condvar {
         Condvar { inner: SpinMutex::new(WaitVariable::new(())) }
diff --git a/library/std/src/sys/sgx/mutex.rs b/library/std/src/sys/sgx/mutex.rs
@@ -8,6 +8,8 @@ pub struct Mutex {
     inner: SpinMutex<WaitVariable<bool>>,
 }
 
+pub type MovableMutex = Box<Mutex>;
+
 // Implementation according to “Operating Systems: Three Easy Pieces”, chapter 28
 impl Mutex {
     pub const fn new() -> Mutex {
diff --git a/library/std/src/sys/unix/condvar.rs b/library/std/src/sys/unix/condvar.rs
@@ -6,6 +6,8 @@ pub struct Condvar {
     inner: UnsafeCell<libc::pthread_cond_t>,
 }
 
+pub type MovableCondvar = Box<Condvar>;
+
 unsafe impl Send for Condvar {}
 unsafe impl Sync for Condvar {}
 
diff --git a/library/std/src/sys/unix/mutex.rs b/library/std/src/sys/unix/mutex.rs
@@ -5,6 +5,8 @@ pub struct Mutex {
     inner: UnsafeCell<libc::pthread_mutex_t>,
 }
 
+pub type MovableMutex = Box<Mutex>;
+
 #[inline]
 pub unsafe fn raw(m: &Mutex) -> *mut libc::pthread_mutex_t {
     m.inner.get()
diff --git a/library/std/src/sys/unsupported/condvar.rs b/library/std/src/sys/unsupported/condvar.rs
@@ -3,6 +3,8 @@ use crate::time::Duration;
 
 pub struct Condvar {}
 
+pub type MovableCondvar = Condvar;
+
 impl Condvar {
     pub const fn new() -> Condvar {
         Condvar {}
diff --git a/library/std/src/sys/unsupported/mutex.rs b/library/std/src/sys/unsupported/mutex.rs
@@ -4,6 +4,8 @@ pub struct Mutex {
     locked: UnsafeCell<bool>,
 }
 
+pub type MovableMutex = Mutex;
+
 unsafe impl Send for Mutex {}
 unsafe impl Sync for Mutex {} // no threads on this platform
 
diff --git a/library/std/src/sys/vxworks/condvar.rs b/library/std/src/sys/vxworks/condvar.rs
@@ -6,6 +6,8 @@ pub struct Condvar {
     inner: UnsafeCell<libc::pthread_cond_t>,
 }
 
+pub type MovableCondvar = Box<Condvar>;
+
 unsafe impl Send for Condvar {}
 unsafe impl Sync for Condvar {}
 
diff --git a/library/std/src/sys/vxworks/mutex.rs b/library/std/src/sys/vxworks/mutex.rs
@@ -5,6 +5,8 @@ pub struct Mutex {
     inner: UnsafeCell<libc::pthread_mutex_t>,
 }
 
+pub type MovableMutex = Box<Mutex>;
+
 #[inline]
 pub unsafe fn raw(m: &Mutex) -> *mut libc::pthread_mutex_t {
     m.inner.get()
diff --git a/library/std/src/sys/wasm/condvar_atomics.rs b/library/std/src/sys/wasm/condvar_atomics.rs
@@ -9,6 +9,8 @@ pub struct Condvar {
     cnt: AtomicUsize,
 }
 
+pub type MovableCondvar = Condvar;
+
 // Condition variables are implemented with a simple counter internally that is
 // likely to cause spurious wakeups. Blocking on a condition variable will first
 // read the value of the internal counter, unlock the given mutex, and then
diff --git a/library/std/src/sys/wasm/mutex_atomics.rs b/library/std/src/sys/wasm/mutex_atomics.rs
@@ -8,6 +8,8 @@ pub struct Mutex {
     locked: AtomicUsize,
 }
 
+pub type MovableMutex = Mutex;
+
 // Mutexes have a pretty simple implementation where they contain an `i32`
 // internally that is 0 when unlocked and 1 when the mutex is locked.
 // Acquisition has a fast path where it attempts to cmpxchg the 0 to a 1, and
diff --git a/library/std/src/sys/windows/condvar.rs b/library/std/src/sys/windows/condvar.rs
@@ -8,6 +8,8 @@ pub struct Condvar {
     inner: UnsafeCell<c::CONDITION_VARIABLE>,
 }
 
+pub type MovableCondvar = Condvar;
+
 unsafe impl Send for Condvar {}
 unsafe impl Sync for Condvar {}
 
diff --git a/library/std/src/sys/windows/mutex.rs b/library/std/src/sys/windows/mutex.rs
@@ -29,6 +29,11 @@ pub struct Mutex {
     lock: AtomicUsize,
 }
 
+// Windows SRW Locks are movable (while not borrowed).
+// ReentrantMutexes (in Inner) are not, but those are stored indirectly through
+// a Box, so do not move when the Mutex it self is moved.
+pub type MovableMutex = Mutex;
+
 unsafe impl Send for Mutex {}
 unsafe impl Sync for Mutex {}
 
diff --git 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
@@ -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

Original file line number	Diff line number	Diff line change
`@@ -15,6 +15,8 @@ pub struct Condvar {`
`15`	`15`	`condvar: UnsafeCell<AtomicU32>,`
`16`	`16`	`}`
`17`	`17`
	`18`	`+pub type MovableCondvar = Condvar;`
	`19`	`+`
`18`	`20`	`unsafe impl Send for Condvar {}`
`19`	`21`	`unsafe impl Sync for Condvar {}`
`20`	`22`
Original file line number	Diff line number	Diff line change
`@@ -15,6 +15,8 @@ extern "C" {`
`15`	`15`	`// implemented identically.`
`16`	`16`	`pub struct Mutex(RWLock);`
`17`	`17`
	`18`	`+pub type MovableMutex = Mutex;`
	`19`	`+`
`18`	`20`	`pub unsafe fn raw(m: &Mutex) -> *mut AtomicU32 {`
`19`	`21`	`rwlock::raw(&m.0)`
`20`	`22`	`}`
Original file line number	Diff line number	Diff line change
`@@ -14,6 +14,8 @@ pub struct Condvar {`
`14`	`14`	`sem2: *const c_void,`
`15`	`15`	`}`
`16`	`16`
	`17`	`+pub type MovableCondvar = Box<Condvar>;`
	`18`	`+`
`17`	`19`	`unsafe impl Send for Condvar {}`
`18`	`20`	`unsafe impl Sync for Condvar {}`
`19`	`21`
Original file line number	Diff line number	Diff line change
`@@ -7,6 +7,8 @@ pub struct Condvar {`
`7`	`7`	`inner: SpinMutex<WaitVariable<()>>,`
`8`	`8`	`}`
`9`	`9`
	`10`	`+pub type MovableCondvar = Box<Condvar>;`
	`11`	`+`
`10`	`12`	`impl Condvar {`
`11`	`13`	`pub const fn new() -> Condvar {`
`12`	`14`	`Condvar { inner: SpinMutex::new(WaitVariable::new(())) }`
Original file line number	Diff line number	Diff line change
`@@ -8,6 +8,8 @@ pub struct Mutex {`
`8`	`8`	`inner: SpinMutex<WaitVariable<bool>>,`
`9`	`9`	`}`
`10`	`10`
	`11`	`+pub type MovableMutex = Box<Mutex>;`
	`12`	`+`
`11`	`13`	`// Implementation according to “Operating Systems: Three Easy Pieces”, chapter 28`
`12`	`14`	`impl Mutex {`
`13`	`15`	`pub const fn new() -> Mutex {`
Original file line number	Diff line number	Diff line change
`@@ -6,6 +6,8 @@ pub struct Condvar {`
`6`	`6`	`inner: UnsafeCell<libc::pthread_cond_t>,`
`7`	`7`	`}`
`8`	`8`
	`9`	`+pub type MovableCondvar = Box<Condvar>;`
	`10`	`+`
`9`	`11`	`unsafe impl Send for Condvar {}`
`10`	`12`	`unsafe impl Sync for Condvar {}`
`11`	`13`
Original file line number	Diff line number	Diff line change
`@@ -5,6 +5,8 @@ pub struct Mutex {`
`5`	`5`	`inner: UnsafeCell<libc::pthread_mutex_t>,`
`6`	`6`	`}`
`7`	`7`
	`8`	`+pub type MovableMutex = Box<Mutex>;`
	`9`	`+`
`8`	`10`	`#[inline]`
`9`	`11`	`pub unsafe fn raw(m: &Mutex) -> *mut libc::pthread_mutex_t {`
`10`	`12`	`m.inner.get()`