diff --git a/library/std/src/sync/condvar.rs b/library/std/src/sync/condvar.rs index 7e2155dae6f..1376d8ebe8f 100644 --- a/library/std/src/sync/condvar.rs +++ b/library/std/src/sync/condvar.rs @@ -553,8 +553,8 @@ impl Condvar { unsafe { self.inner.notify_all() } } - fn verify(&self, mutex: &sys_mutex::Mutex) { - let addr = mutex as *const _ as usize; + 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. diff --git a/library/std/src/sync/mutex.rs b/library/std/src/sync/mutex.rs index a1703c731d4..e8f5a6f4294 100644 --- a/library/std/src/sync/mutex.rs +++ b/library/std/src/sync/mutex.rs @@ -166,12 +166,7 @@ use crate::sys_common::poison::{self, LockResult, TryLockError, TryLockResult}; #[stable(feature = "rust1", since = "1.0.0")] #[cfg_attr(not(test), rustc_diagnostic_item = "mutex_type")] pub struct Mutex { - // Note that this mutex is in a *box*, not inlined into the struct itself. - // Once a native mutex has been used once, its address can never change (it - // can't be moved). This mutex type can be safely moved at any time, so to - // ensure that the native mutex is used correctly we box the inner mutex to - // give it a constant address. - inner: Box, + inner: sys::MovableMutex, poison: poison::Flag, data: UnsafeCell, } @@ -218,15 +213,11 @@ impl Mutex { /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub fn new(t: T) -> Mutex { - let mut m = Mutex { - inner: box sys::Mutex::new(), + Mutex { + inner: sys::MovableMutex::new(), poison: poison::Flag::new(), data: UnsafeCell::new(t), - }; - unsafe { - m.inner.init(); } - m } } @@ -378,7 +369,6 @@ impl Mutex { (ptr::read(inner), ptr::read(poison), ptr::read(data)) }; mem::forget(self); - inner.destroy(); // Keep in sync with the `Drop` impl. drop(inner); poison::map_result(poison.borrow(), |_| data.into_inner()) @@ -411,18 +401,6 @@ impl Mutex { } } -#[stable(feature = "rust1", since = "1.0.0")] -unsafe impl<#[may_dangle] T: ?Sized> Drop for Mutex { - fn drop(&mut self) { - // This is actually safe b/c we know that there is no further usage of - // this mutex (it's up to the user to arrange for a mutex to get - // dropped, that's not our job) - // - // IMPORTANT: This code must be kept in sync with `Mutex::into_inner`. - unsafe { self.inner.destroy() } - } -} - #[stable(feature = "mutex_from", since = "1.24.0")] impl From for Mutex { /// Creates a new mutex in an unlocked state ready for use. @@ -509,7 +487,7 @@ impl fmt::Display for MutexGuard<'_, T> { } } -pub fn guard_lock<'a, T: ?Sized>(guard: &MutexGuard<'a, T>) -> &'a sys::Mutex { +pub fn guard_lock<'a, T: ?Sized>(guard: &MutexGuard<'a, T>) -> &'a sys::MovableMutex { &guard.lock.inner } diff --git a/library/std/src/sys/hermit/args.rs b/library/std/src/sys/hermit/args.rs index 72c1b8511ca..77272939272 100644 --- a/library/std/src/sys/hermit/args.rs +++ b/library/std/src/sys/hermit/args.rs @@ -57,11 +57,11 @@ mod imp { use crate::ptr; use crate::sys_common::os_str_bytes::*; - use crate::sys_common::mutex::Mutex; + use crate::sys_common::mutex::StaticMutex; static mut ARGC: isize = 0; static mut ARGV: *const *const u8 = ptr::null(); - static LOCK: Mutex = Mutex::new(); + static LOCK: StaticMutex = StaticMutex::new(); pub unsafe fn init(argc: isize, argv: *const *const u8) { let _guard = LOCK.lock(); diff --git a/library/std/src/sys/unix/args.rs b/library/std/src/sys/unix/args.rs index 9bc44a59482..f7c3f163718 100644 --- a/library/std/src/sys/unix/args.rs +++ b/library/std/src/sys/unix/args.rs @@ -80,13 +80,13 @@ mod imp { use crate::ptr; use crate::sync::atomic::{AtomicIsize, AtomicPtr, Ordering}; - use crate::sys_common::mutex::Mutex; + use crate::sys_common::mutex::StaticMutex; static ARGC: AtomicIsize = AtomicIsize::new(0); static ARGV: AtomicPtr<*const u8> = AtomicPtr::new(ptr::null_mut()); // We never call `ENV_LOCK.init()`, so it is UB to attempt to // acquire this mutex reentrantly! - static LOCK: Mutex = Mutex::new(); + static LOCK: StaticMutex = StaticMutex::new(); unsafe fn really_init(argc: isize, argv: *const *const u8) { let _guard = LOCK.lock(); diff --git a/library/std/src/sys/unix/os.rs b/library/std/src/sys/unix/os.rs index 4aa61fc5bf6..c9f9ed01e12 100644 --- a/library/std/src/sys/unix/os.rs +++ b/library/std/src/sys/unix/os.rs @@ -21,7 +21,7 @@ use crate::slice; use crate::str; use crate::sys::cvt; use crate::sys::fd; -use crate::sys_common::mutex::{Mutex, MutexGuard}; +use crate::sys_common::mutex::{StaticMutex, StaticMutexGuard}; use crate::vec; use libc::{c_char, c_int, c_void}; @@ -470,10 +470,9 @@ pub unsafe fn environ() -> *mut *const *const c_char { &mut environ } -pub unsafe fn env_lock() -> MutexGuard<'static> { - // We never call `ENV_LOCK.init()`, so it is UB to attempt to - // acquire this mutex reentrantly! - static ENV_LOCK: Mutex = Mutex::new(); +pub unsafe fn env_lock() -> StaticMutexGuard<'static> { + // It is UB to attempt to acquire this mutex reentrantly! + static ENV_LOCK: StaticMutex = StaticMutex::new(); ENV_LOCK.lock() } diff --git a/library/std/src/sys/vxworks/args.rs b/library/std/src/sys/vxworks/args.rs index adff6c489bb..30cf7a707c7 100644 --- a/library/std/src/sys/vxworks/args.rs +++ b/library/std/src/sys/vxworks/args.rs @@ -57,11 +57,11 @@ mod imp { use crate::marker::PhantomData; use crate::ptr; - use crate::sys_common::mutex::Mutex; + use crate::sys_common::mutex::StaticMutex; static mut ARGC: isize = 0; static mut ARGV: *const *const u8 = ptr::null(); - static LOCK: Mutex = Mutex::new(); + static LOCK: StaticMutex = StaticMutex::new(); pub unsafe fn init(argc: isize, argv: *const *const u8) { let _guard = LOCK.lock(); diff --git a/library/std/src/sys/vxworks/os.rs b/library/std/src/sys/vxworks/os.rs index 1fadf716135..08394a8d29d 100644 --- a/library/std/src/sys/vxworks/os.rs +++ b/library/std/src/sys/vxworks/os.rs @@ -10,7 +10,7 @@ use crate::path::{self, Path, PathBuf}; use crate::slice; use crate::str; use crate::sys::cvt; -use crate::sys_common::mutex::{Mutex, MutexGuard}; +use crate::sys_common::mutex::{StaticMutex, StaticMutexGuard}; use libc::{self, c_char /*,c_void */, c_int}; /*use sys::fd; this one is probably important */ use crate::vec; @@ -212,10 +212,9 @@ pub unsafe fn environ() -> *mut *const *const c_char { &mut environ } -pub unsafe fn env_lock() -> MutexGuard<'static> { - // We never call `ENV_LOCK.init()`, so it is UB to attempt to - // acquire this mutex reentrantly! - static ENV_LOCK: Mutex = Mutex::new(); +pub unsafe fn env_lock() -> StaticMutexGuard<'static> { + // It is UB to attempt to acquire this mutex reentrantly! + static ENV_LOCK: StaticMutex = StaticMutex::new(); ENV_LOCK.lock() } diff --git a/library/std/src/sys_common/at_exit_imp.rs b/library/std/src/sys_common/at_exit_imp.rs index 6b799db856e..90d5d3a7898 100644 --- a/library/std/src/sys_common/at_exit_imp.rs +++ b/library/std/src/sys_common/at_exit_imp.rs @@ -4,7 +4,7 @@ use crate::mem; use crate::ptr; -use crate::sys_common::mutex::Mutex; +use crate::sys_common::mutex::StaticMutex; type Queue = Vec>; @@ -12,9 +12,8 @@ type Queue = Vec>; // on poisoning and this module needs to operate at a lower level than requiring // the thread infrastructure to be in place (useful on the borders of // initialization/destruction). -// We never call `LOCK.init()`, so it is UB to attempt to -// acquire this mutex reentrantly! -static LOCK: Mutex = Mutex::new(); +// It is UB to attempt to acquire this mutex reentrantly! +static LOCK: StaticMutex = StaticMutex::new(); static mut QUEUE: *mut Queue = ptr::null_mut(); const DONE: *mut Queue = 1_usize as *mut _; diff --git a/library/std/src/sys_common/condvar.rs b/library/std/src/sys_common/condvar.rs index f9611bc6f7b..a48d301f812 100644 --- a/library/std/src/sys_common/condvar.rs +++ b/library/std/src/sys_common/condvar.rs @@ -1,5 +1,5 @@ use crate::sys::condvar as imp; -use crate::sys_common::mutex::{self, Mutex}; +use crate::sys_common::mutex::MovableMutex; use crate::time::Duration; /// An OS-based condition variable. @@ -46,8 +46,8 @@ impl Condvar { /// Behavior is also undefined if more than one mutex is used concurrently /// on this condition variable. #[inline] - pub unsafe fn wait(&self, mutex: &Mutex) { - self.0.wait(mutex::raw(mutex)) + pub unsafe fn wait(&self, mutex: &MovableMutex) { + self.0.wait(mutex.raw()) } /// Waits for a signal on the specified mutex with a timeout duration @@ -57,8 +57,8 @@ impl Condvar { /// Behavior is also undefined if more than one mutex is used concurrently /// on this condition variable. #[inline] - pub unsafe fn wait_timeout(&self, mutex: &Mutex, dur: Duration) -> bool { - self.0.wait_timeout(mutex::raw(mutex), dur) + pub unsafe fn wait_timeout(&self, mutex: &MovableMutex, dur: Duration) -> bool { + self.0.wait_timeout(mutex.raw(), dur) } /// Deallocates all resources associated with this condition variable. diff --git a/library/std/src/sys_common/mutex.rs b/library/std/src/sys_common/mutex.rs index e66d8994147..93ec7d89bc5 100644 --- a/library/std/src/sys_common/mutex.rs +++ b/library/std/src/sys_common/mutex.rs @@ -1,97 +1,47 @@ use crate::sys::mutex as imp; -/// An OS-based mutual exclusion lock. +/// An OS-based mutual exclusion lock, meant for use in static variables. /// -/// This is the thinnest cross-platform wrapper around OS mutexes. All usage of -/// this mutex is unsafe and it is recommended to instead use the safe wrapper -/// at the top level of the crate instead of this type. -pub struct Mutex(imp::Mutex); +/// This mutex has a const constructor ([`StaticMutex::new`]), does not +/// implement `Drop` to cleanup resources, and causes UB when moved or used +/// reentrantly. +/// +/// This mutex does not implement poisoning. +/// +/// This is a wrapper around `imp::Mutex` that does *not* call `init()` and +/// `destroy()`. +pub struct StaticMutex(imp::Mutex); -unsafe impl Sync for Mutex {} +unsafe impl Sync for StaticMutex {} -impl Mutex { +impl StaticMutex { /// Creates a new mutex for use. /// /// Behavior is undefined if the mutex is moved after it is /// first used with any of the functions below. - /// Also, until `init` is called, behavior is undefined if this - /// mutex is ever used reentrantly, i.e., `raw_lock` or `try_lock` - /// are called by the thread currently holding the lock. + /// Also, the behavior is undefined if this mutex is ever used reentrantly, + /// i.e., `lock` is called by the thread currently holding the lock. #[rustc_const_stable(feature = "const_sys_mutex_new", since = "1.0.0")] - pub const fn new() -> Mutex { - Mutex(imp::Mutex::new()) - } - - /// Prepare the mutex for use. - /// - /// This should be called once the mutex is at a stable memory address. - /// If called, this must be the very first thing that happens to the mutex. - /// Calling it in parallel with or after any operation (including another - /// `init()`) is undefined behavior. - #[inline] - pub unsafe fn init(&mut self) { - self.0.init() - } - - /// Locks the mutex blocking the current thread until it is available. - /// - /// Behavior is undefined if the mutex has been moved between this and any - /// previous function call. - #[inline] - pub unsafe fn raw_lock(&self) { - self.0.lock() + pub const fn new() -> Self { + Self(imp::Mutex::new()) } /// Calls raw_lock() and then returns an RAII guard to guarantee the mutex /// will be unlocked. - #[inline] - pub unsafe fn lock(&self) -> MutexGuard<'_> { - self.raw_lock(); - MutexGuard(&self.0) - } - - /// Attempts to lock the mutex without blocking, returning whether it was - /// successfully acquired or not. /// - /// Behavior is undefined if the mutex has been moved between this and any - /// previous function call. + /// It is undefined behaviour to call this function while locked, or if the + /// mutex has been moved since the last time this was called. #[inline] - pub unsafe fn try_lock(&self) -> bool { - self.0.try_lock() + pub unsafe fn lock(&self) -> StaticMutexGuard<'_> { + self.0.lock(); + StaticMutexGuard(&self.0) } - - /// Unlocks the mutex. - /// - /// Behavior is undefined if the current thread does not actually hold the - /// mutex. - /// - /// Consider switching from the pair of raw_lock() and raw_unlock() to - /// lock() whenever possible. - #[inline] - pub unsafe fn raw_unlock(&self) { - self.0.unlock() - } - - /// Deallocates all resources associated with this mutex. - /// - /// Behavior is undefined if there are current or will be future users of - /// this mutex. - #[inline] - pub unsafe fn destroy(&self) { - self.0.destroy() - } -} - -// not meant to be exported to the outside world, just the containing module -pub fn raw(mutex: &Mutex) -> &imp::Mutex { - &mutex.0 } #[must_use] -/// A simple RAII utility for the above Mutex without the poisoning semantics. -pub struct MutexGuard<'a>(&'a imp::Mutex); +pub struct StaticMutexGuard<'a>(&'a imp::Mutex); -impl Drop for MutexGuard<'_> { +impl Drop for StaticMutexGuard<'_> { #[inline] fn drop(&mut self) { unsafe { @@ -99,3 +49,58 @@ impl Drop for MutexGuard<'_> { } } } + +/// An OS-based mutual exclusion lock. +/// +/// This mutex does *not* have a const constructor, cleans up its resources in +/// its `Drop` implementation, may safely be moved (when not borrowed), and +/// does not cause UB when used reentrantly. +/// +/// 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); + +unsafe impl Sync for MovableMutex {} + +impl MovableMutex { + /// Creates a new mutex. + pub fn new() -> Self { + let mut mutex = box imp::Mutex::new(); + unsafe { mutex.init() }; + Self(mutex) + } + + pub(crate) fn raw(&self) -> &imp::Mutex { + &self.0 + } + + /// Locks the mutex blocking the current thread until it is available. + #[inline] + pub fn raw_lock(&self) { + unsafe { self.0.lock() } + } + + /// Attempts to lock the mutex without blocking, returning whether it was + /// successfully acquired or not. + #[inline] + pub fn try_lock(&self) -> bool { + unsafe { self.0.try_lock() } + } + + /// Unlocks the mutex. + /// + /// Behavior is undefined if the current thread does not actually hold the + /// mutex. + #[inline] + pub unsafe fn raw_unlock(&self) { + self.0.unlock() + } +} + +impl Drop for MovableMutex { + fn drop(&mut self) { + unsafe { self.0.destroy() }; + } +} diff --git a/library/std/src/sys_common/thread_local_key.rs b/library/std/src/sys_common/thread_local_key.rs index 676eadd1fac..dbcb7b36265 100644 --- a/library/std/src/sys_common/thread_local_key.rs +++ b/library/std/src/sys_common/thread_local_key.rs @@ -53,7 +53,7 @@ mod tests; use crate::sync::atomic::{self, AtomicUsize, Ordering}; use crate::sys::thread_local_key as imp; -use crate::sys_common::mutex::Mutex; +use crate::sys_common::mutex::StaticMutex; /// A type for TLS keys that are statically allocated. /// @@ -157,7 +157,7 @@ impl StaticKey { if imp::requires_synchronized_create() { // We never call `INIT_LOCK.init()`, so it is UB to attempt to // acquire this mutex reentrantly! - static INIT_LOCK: Mutex = Mutex::new(); + static INIT_LOCK: StaticMutex = StaticMutex::new(); let _guard = INIT_LOCK.lock(); let mut key = self.key.load(Ordering::SeqCst); if key == 0 { diff --git a/library/std/src/thread/mod.rs b/library/std/src/thread/mod.rs index fb2fbb5bf2d..087175bb92a 100644 --- a/library/std/src/thread/mod.rs +++ b/library/std/src/thread/mod.rs @@ -972,9 +972,8 @@ pub struct ThreadId(NonZeroU64); impl ThreadId { // Generate a new unique thread ID. fn new() -> ThreadId { - // We never call `GUARD.init()`, so it is UB to attempt to - // acquire this mutex reentrantly! - static GUARD: mutex::Mutex = mutex::Mutex::new(); + // It is UB to attempt to acquire this mutex reentrantly! + static GUARD: mutex::StaticMutex = mutex::StaticMutex::new(); static mut COUNTER: u64 = 1; unsafe { diff --git a/library/std/src/time.rs b/library/std/src/time.rs index 18e38c6299b..e7df3841147 100644 --- a/library/std/src/time.rs +++ b/library/std/src/time.rs @@ -20,7 +20,7 @@ use crate::error::Error; use crate::fmt; use crate::ops::{Add, AddAssign, Sub, SubAssign}; use crate::sys::time; -use crate::sys_common::mutex::Mutex; +use crate::sys_common::mutex::StaticMutex; use crate::sys_common::FromInner; #[stable(feature = "time", since = "1.3.0")] @@ -243,7 +243,7 @@ impl Instant { return Instant(os_now); } - static LOCK: Mutex = Mutex::new(); + static LOCK: StaticMutex = StaticMutex::new(); static mut LAST_NOW: time::Instant = time::Instant::zero(); unsafe { let _lock = LOCK.lock(); diff --git a/src/test/ui/issues/issue-54062.rs b/src/test/ui/issues/issue-54062.rs index 60a9b00d5d4..093d6601d4e 100644 --- a/src/test/ui/issues/issue-54062.rs +++ b/src/test/ui/issues/issue-54062.rs @@ -7,7 +7,6 @@ struct Test { fn main() {} fn testing(test: Test) { - let _ = test.comps.inner.lock().unwrap(); + let _ = test.comps.inner.try_lock(); //~^ ERROR: field `inner` of struct `Mutex` is private - //~| ERROR: no method named `unwrap` found } diff --git a/src/test/ui/issues/issue-54062.stderr b/src/test/ui/issues/issue-54062.stderr index e9e8080d467..5361ee1d345 100644 --- a/src/test/ui/issues/issue-54062.stderr +++ b/src/test/ui/issues/issue-54062.stderr @@ -1,16 +1,9 @@ error[E0616]: field `inner` of struct `Mutex` is private --> $DIR/issue-54062.rs:10:24 | -LL | let _ = test.comps.inner.lock().unwrap(); +LL | let _ = test.comps.inner.try_lock(); | ^^^^^ private field -error[E0599]: no method named `unwrap` found for struct `std::sys_common::mutex::MutexGuard<'_>` in the current scope - --> $DIR/issue-54062.rs:10:37 - | -LL | let _ = test.comps.inner.lock().unwrap(); - | ^^^^^^ method not found in `std::sys_common::mutex::MutexGuard<'_>` +error: aborting due to previous error -error: aborting due to 2 previous errors - -Some errors have detailed explanations: E0599, E0616. -For more information about an error, try `rustc --explain E0599`. +For more information about this error, try `rustc --explain E0616`.