diff --git a/library/std/src/sync/condvar.rs b/library/std/src/sync/condvar.rs index 1376d8ebe8f4a..ffc1e57f4e03f 100644 --- 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, - 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> { 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 index 86d099ee3a19c..6757707cd9513 100644 --- 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 index dabdc0c9b510a..82d89b260fafd 100644 --- 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, } +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 index 580ab0e8ad863..66839e05bf076 100644 --- 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 index 52c8c3b17e826..b45e8718f088e 100644 --- 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; + 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 index ed6dbcf497147..dfe22e6a1b375 100644 --- 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>, } +pub type MovableCondvar = Box; + 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 index 4911c2f538769..8874517dac60c 100644 --- 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>, } +pub type MovableMutex = Box; + // 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 index 9f1847943f326..e38f91af9f0b9 100644 --- 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, } +pub type MovableCondvar = Box; + 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 index 45c600f75f5cf..ebc737b75ae0b 100644 --- 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, } +pub type MovableMutex = Box; + #[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 index a578eee8ccce2..35d12a69c8aa5 100644 --- 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 index 9ef8af52eb5c2..06ea9a1e2c109 100644 --- 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, } +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 index 5a77966d97468..b4724be7c7c3b 100644 --- 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, } +pub type MovableCondvar = Box; + 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 index 103d87e3d2f91..dd7582c21a727 100644 --- 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, } +pub type MovableMutex = Box; + #[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 index d86bb60507be2..a96bb18e6ef1a 100644 --- 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 index 4b1a7c9b48141..2970fcf806cbf 100644 --- 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 index 8f7f6854cc22c..44547a5c51a34 100644 --- 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, } +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 index e2aaca59fe2f3..fa51b006c346f 100644 --- 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 index a48d301f8127b..2c02e1cd33c81 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 = ::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 0000000000000..fecb732b910ce --- /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 { + 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 93ec7d89bc5c7..a1e11d24465ea 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`, to allow the object to be moved -/// without moving the raw mutex. -pub struct MovableMutex(Box); +/// This is either a wrapper around `Box` 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 }