Rollup merge of #77147 - fusion-engineering-forks:static-mutex, r=dtolnay

Split sys_common::Mutex in StaticMutex and MovableMutex.

The (unsafe) `Mutex` from `sys_common` had a rather complicated interface. You were supposed to call `init()` manually, unless you could guarantee it was neither moved nor used reentrantly.

Calling `destroy()` was also optional, although it was unclear if 1) resources might be leaked or not, and 2) if `destroy()` should only be called when `init()` was called.

This allowed for a number of interesting (confusing?) different ways to use this `Mutex`, all captured in a single type.

In practice, this type was only ever used in two ways:

1. As a static variable. In this case, neither `init()` nor `destroy()` are called. The variable is never moved, and it is never used reentrantly. It is only ever locked using the `LockGuard`, never with `raw_lock`.

2. As a `Box`ed variable. In this case, both `init()` and `destroy()` are called, it will be moved and possibly used reentrantly.

No other combinations are used anywhere in `std`.

This change simplifies things by splitting this `Mutex` type into two types matching the two use cases: `StaticMutex` and `MovableMutex`.

The interface of both new types is now both safer and simpler. The first one does not call nor expose `init`/`destroy`, and the second one calls those automatically in its `new()` and `Drop` functions. Also, the locking functions of `MovableMutex` are no longer unsafe.

---

This will also make it easier to conditionally box mutexes later, by moving that decision into sys/sys_common. Some of the mutex implementations (at least those of Wasm and 'sys/unsupported') are safe to move, so wouldn't need a box. ~~(But that's blocked on  #76932 for now.)~~ (See #77380.)

Author: JohnTitor

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.

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<T: ?Sized> {
-    // 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<sys::Mutex>,
+    inner: sys::MovableMutex,
     poison: poison::Flag,
     data: UnsafeCell<T>,
 }
@@ -218,15 +213,11 @@ impl<T> Mutex<T> {
     /// ```
     #[stable(feature = "rust1", since = "1.0.0")]
     pub fn new(t: T) -> Mutex<T> {
-        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<T: ?Sized> Mutex<T> {
                 (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<T: ?Sized> Mutex<T> {
     }
 }
 
-#[stable(feature = "rust1", since = "1.0.0")]
-unsafe impl<#[may_dangle] T: ?Sized> Drop for Mutex<T> {
-    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<T> From<T> for Mutex<T> {
     /// Creates a new mutex in an unlocked state ready for use.
@@ -509,7 +487,7 @@ impl<T: ?Sized + fmt::Display> 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
 }
 

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();

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();

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()
 }
 

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();

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()
 }
 

library/std/src/sys_common/at_exit_imp.rs

@@ -4,17 +4,16 @@
 
 use crate::mem;
 use crate::ptr;
-use crate::sys_common::mutex::Mutex;
+use crate::sys_common::mutex::StaticMutex;
 
 type Queue = Vec<Box<dyn FnOnce()>>;
 
 // NB these are specifically not types from `std::sync` as they currently rely
 // 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 _;

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.

library/std/src/sys_common/mutex.rs

@@ -1,101 +1,106 @@
 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 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 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 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())
+    pub const fn new() -> Self {
+        Self(imp::Mutex::new())
     }
 
-    /// Prepare the mutex for use.
+    /// Calls raw_lock() and then returns an RAII guard to guarantee the mutex
+    /// will be unlocked.
     ///
-    /// 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.
+    /// 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 init(&mut self) {
-        self.0.init()
+    pub unsafe fn lock(&self) -> StaticMutexGuard<'_> {
+        self.0.lock();
+        StaticMutexGuard(&self.0)
     }
+}
 
-    /// 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.
+#[must_use]
+pub struct StaticMutexGuard<'a>(&'a imp::Mutex);
+
+impl Drop for StaticMutexGuard<'_> {
     #[inline]
-    pub unsafe fn raw_lock(&self) {
-        self.0.lock()
+    fn drop(&mut self) {
+        unsafe {
+            self.0.unlock();
+        }
     }
+}
 
-    /// Calls raw_lock() and then returns an RAII guard to guarantee the mutex
-    /// will be unlocked.
+/// 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<imp::Mutex>`, to allow the object to be moved
+/// without moving the raw mutex.
+pub struct MovableMutex(Box<imp::Mutex>);
+
+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 unsafe fn lock(&self) -> MutexGuard<'_> {
-        self.raw_lock();
-        MutexGuard(&self.0)
+    pub fn raw_lock(&self) {
+        unsafe { self.0.lock() }
     }
 
     /// 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.
     #[inline]
-    pub unsafe fn try_lock(&self) -> bool {
-        self.0.try_lock()
+    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.
-    ///
-    /// 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);
-
-impl Drop for MutexGuard<'_> {
-    #[inline]
+impl Drop for MovableMutex {
     fn drop(&mut self) {
-        unsafe {
-            self.0.unlock();
-        }
+        unsafe { self.0.destroy() };
     }
 }