Skip to content

Commit

Permalink
Rollup merge of #77147 - fusion-engineering-forks:static-mutex, r=dto…
Browse files Browse the repository at this point in the history
…lnay

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.)
  • Loading branch information
JohnTitor authored Oct 1, 2020
2 parents 9eaf536 + 825dda8 commit 1c4a5f8
Show file tree
Hide file tree
Showing 15 changed files with 104 additions and 133 deletions.
4 changes: 2 additions & 2 deletions library/std/src/sync/condvar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
30 changes: 4 additions & 26 deletions library/std/src/sync/mutex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
}
Expand Down Expand Up @@ -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
}
}

Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
}

Expand Down
4 changes: 2 additions & 2 deletions library/std/src/sys/hermit/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
4 changes: 2 additions & 2 deletions library/std/src/sys/unix/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
9 changes: 4 additions & 5 deletions library/std/src/sys/unix/os.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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()
}

Expand Down
4 changes: 2 additions & 2 deletions library/std/src/sys/vxworks/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
9 changes: 4 additions & 5 deletions library/std/src/sys/vxworks/os.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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()
}

Expand Down
7 changes: 3 additions & 4 deletions library/std/src/sys_common/at_exit_imp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 _;
Expand Down
10 changes: 5 additions & 5 deletions library/std/src/sys_common/condvar.rs
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -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
Expand All @@ -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.
Expand Down
127 changes: 66 additions & 61 deletions library/std/src/sys_common/mutex.rs
Original file line number Diff line number Diff line change
@@ -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() };
}
}
Loading

0 comments on commit 1c4a5f8

Please sign in to comment.