Auto merge of #100576 - joboet:movable_const_remutex, r=Mark-Simulacrum

bors · bors · commit e7cdd4c0909b · 2022-09-04T22:53:58.000Z
Make `ReentrantMutex` movable and `const`

As `MovableMutex` is now `const`, it can be used to simplify the implementation and interface of the internal reentrant mutex type. Consequently, the standard error stream does not need to be wrapped in `OnceLock` and `OnceLock::get_or_init_pin()` can be removed.
diff --git a/library/std/src/io/stdio.rs b/library/std/src/io/stdio.rs
@@ -8,7 +8,6 @@ use crate::io::prelude::*;
 use crate::cell::{Cell, RefCell};
 use crate::fmt;
 use crate::io::{self, BufReader, IoSlice, IoSliceMut, LineWriter, Lines};
-use crate::pin::Pin;
 use crate::sync::atomic::{AtomicBool, Ordering};
 use crate::sync::{Arc, Mutex, MutexGuard, OnceLock};
 use crate::sys::stdio;
@@ -526,7 +525,7 @@ pub struct Stdout {
     // FIXME: this should be LineWriter or BufWriter depending on the state of
     //        stdout (tty or not). Note that if this is not line buffered it
     //        should also flush-on-panic or some form of flush-on-abort.
-    inner: Pin<&'static ReentrantMutex<RefCell<LineWriter<StdoutRaw>>>>,
+    inner: &'static ReentrantMutex<RefCell<LineWriter<StdoutRaw>>>,
 }
 
 /// A locked reference to the [`Stdout`] handle.
@@ -603,24 +602,20 @@ static STDOUT: OnceLock<ReentrantMutex<RefCell<LineWriter<StdoutRaw>>>> = OnceLo
 #[stable(feature = "rust1", since = "1.0.0")]
 pub fn stdout() -> Stdout {
     Stdout {
-        inner: Pin::static_ref(&STDOUT).get_or_init_pin(
-            || unsafe { ReentrantMutex::new(RefCell::new(LineWriter::new(stdout_raw()))) },
-            |mutex| unsafe { mutex.init() },
-        ),
+        inner: STDOUT
+            .get_or_init(|| ReentrantMutex::new(RefCell::new(LineWriter::new(stdout_raw())))),
     }
 }
 
 pub fn cleanup() {
-    if let Some(instance) = STDOUT.get() {
-        // Flush the data and disable buffering during shutdown
-        // by replacing the line writer by one with zero
-        // buffering capacity.
-        // We use try_lock() instead of lock(), because someone
-        // might have leaked a StdoutLock, which would
-        // otherwise cause a deadlock here.
-        if let Some(lock) = Pin::static_ref(instance).try_lock() {
-            *lock.borrow_mut() = LineWriter::with_capacity(0, stdout_raw());
-        }
+    // Flush the data and disable buffering during shutdown
+    // by replacing the line writer by one with zero
+    // buffering capacity.
+    // We use try_lock() instead of lock(), because someone
+    // might have leaked a StdoutLock, which would
+    // otherwise cause a deadlock here.
+    if let Some(lock) = STDOUT.get().and_then(ReentrantMutex::try_lock) {
+        *lock.borrow_mut() = LineWriter::with_capacity(0, stdout_raw());
     }
 }
 
@@ -761,7 +756,7 @@ impl fmt::Debug for StdoutLock<'_> {
 /// standard library or via raw Windows API calls, will fail.
 #[stable(feature = "rust1", since = "1.0.0")]
 pub struct Stderr {
-    inner: Pin<&'static ReentrantMutex<RefCell<StderrRaw>>>,
+    inner: &'static ReentrantMutex<RefCell<StderrRaw>>,
 }
 
 /// A locked reference to the [`Stderr`] handle.
@@ -834,16 +829,12 @@ pub struct StderrLock<'a> {
 #[stable(feature = "rust1", since = "1.0.0")]
 pub fn stderr() -> Stderr {
     // Note that unlike `stdout()` we don't use `at_exit` here to register a
-    // destructor. Stderr is not buffered , so there's no need to run a
+    // destructor. Stderr is not buffered, so there's no need to run a
     // destructor for flushing the buffer
-    static INSTANCE: OnceLock<ReentrantMutex<RefCell<StderrRaw>>> = OnceLock::new();
+    static INSTANCE: ReentrantMutex<RefCell<StderrRaw>> =
+        ReentrantMutex::new(RefCell::new(stderr_raw()));
 
-    Stderr {
-        inner: Pin::static_ref(&INSTANCE).get_or_init_pin(
-            || unsafe { ReentrantMutex::new(RefCell::new(stderr_raw())) },
-            |mutex| unsafe { mutex.init() },
-        ),
-    }
+    Stderr { inner: &INSTANCE }
 }
 
 impl Stderr {
diff --git a/library/std/src/sync/once_lock.rs b/library/std/src/sync/once_lock.rs
@@ -3,7 +3,6 @@ use crate::fmt;
 use crate::marker::PhantomData;
 use crate::mem::MaybeUninit;
 use crate::panic::{RefUnwindSafe, UnwindSafe};
-use crate::pin::Pin;
 use crate::sync::Once;
 
 /// A synchronization primitive which can be written to only once.
@@ -223,60 +222,6 @@ impl<T> OnceLock<T> {
         Ok(unsafe { self.get_unchecked() })
     }
 
-    /// Internal-only API that gets the contents of the cell, initializing it
-    /// in two steps with `f` and `g` if the cell was empty.
-    ///
-    /// `f` is called to construct the value, which is then moved into the cell
-    /// and given as a (pinned) mutable reference to `g` to finish
-    /// initialization.
-    ///
-    /// This allows `g` to inspect an manipulate the value after it has been
-    /// moved into its final place in the cell, but before the cell is
-    /// considered initialized.
-    ///
-    /// # Panics
-    ///
-    /// If `f` or `g` panics, the panic is propagated to the caller, and the
-    /// cell remains uninitialized.
-    ///
-    /// With the current implementation, if `g` panics, the value from `f` will
-    /// not be dropped. This should probably be fixed if this is ever used for
-    /// a type where this matters.
-    ///
-    /// It is an error to reentrantly initialize the cell from `f`. The exact
-    /// outcome is unspecified. Current implementation deadlocks, but this may
-    /// be changed to a panic in the future.
-    pub(crate) fn get_or_init_pin<F, G>(self: Pin<&Self>, f: F, g: G) -> Pin<&T>
-    where
-        F: FnOnce() -> T,
-        G: FnOnce(Pin<&mut T>),
-    {
-        if let Some(value) = self.get_ref().get() {
-            // SAFETY: The inner value was already initialized, and will not be
-            // moved anymore.
-            return unsafe { Pin::new_unchecked(value) };
-        }
-
-        let slot = &self.value;
-
-        // Ignore poisoning from other threads
-        // If another thread panics, then we'll be able to run our closure
-        self.once.call_once_force(|_| {
-            let value = f();
-            // SAFETY: We use the Once (self.once) to guarantee unique access
-            // to the UnsafeCell (slot).
-            let value: &mut T = unsafe { (&mut *slot.get()).write(value) };
-            // SAFETY: The value has been written to its final place in
-            // self.value. We do not to move it anymore, which we promise here
-            // with a Pin<&mut T>.
-            g(unsafe { Pin::new_unchecked(value) });
-        });
-
-        // SAFETY: The inner value has been initialized, and will not be moved
-        // anymore.
-        unsafe { Pin::new_unchecked(self.get_ref().get_unchecked()) }
-    }
-
     /// Consumes the `OnceLock`, returning the wrapped value. Returns
     /// `None` if the cell was empty.
     ///
diff --git a/library/std/src/sys/hermit/mutex.rs b/library/std/src/sys/hermit/mutex.rs
@@ -174,9 +174,6 @@ impl Mutex {
         Mutex { inner: Spinlock::new(MutexInner::new()) }
     }
 
-    #[inline]
-    pub unsafe fn init(&mut self) {}
-
     #[inline]
     pub unsafe fn lock(&self) {
         loop {
diff --git a/library/std/src/sys/itron/mutex.rs b/library/std/src/sys/itron/mutex.rs
@@ -31,12 +31,6 @@ impl Mutex {
         Mutex { mtx: SpinIdOnceCell::new() }
     }
 
-    pub unsafe fn init(&mut self) {
-        // Initialize `self.mtx` eagerly
-        let id = new_mtx().unwrap_or_else(|e| fail(e, &"acre_mtx"));
-        unsafe { self.mtx.set_unchecked((id, ())) };
-    }
-
     /// Get the inner mutex's ID, which is lazily created.
     fn raw(&self) -> abi::ID {
         match self.mtx.get_or_try_init(|| new_mtx().map(|id| (id, ()))) {
diff --git a/library/std/src/sys/sgx/mutex.rs b/library/std/src/sys/sgx/mutex.rs
@@ -20,9 +20,6 @@ impl Mutex {
         Mutex { inner: SpinMutex::new(WaitVariable::new(false)) }
     }
 
-    #[inline]
-    pub unsafe fn init(&mut self) {}
-
     #[inline]
     pub unsafe fn lock(&self) {
         let mut guard = self.inner.lock();
diff --git a/library/std/src/sys/unix/locks/fuchsia_mutex.rs b/library/std/src/sys/unix/locks/fuchsia_mutex.rs
@@ -85,9 +85,6 @@ impl Mutex {
         Mutex { futex: AtomicU32::new(UNLOCKED) }
     }
 
-    #[inline]
-    pub unsafe fn init(&mut self) {}
-
     #[inline]
     pub unsafe fn try_lock(&self) -> bool {
         let thread_self = zx_thread_self();
diff --git a/library/std/src/sys/unix/locks/futex_mutex.rs b/library/std/src/sys/unix/locks/futex_mutex.rs
@@ -19,9 +19,6 @@ impl Mutex {
         Self { futex: AtomicU32::new(0) }
     }
 
-    #[inline]
-    pub unsafe fn init(&mut self) {}
-
     #[inline]
     pub unsafe fn try_lock(&self) -> bool {
         self.futex.compare_exchange(0, 1, Acquire, Relaxed).is_ok()
diff --git a/library/std/src/sys/unix/locks/pthread_mutex.rs b/library/std/src/sys/unix/locks/pthread_mutex.rs
@@ -52,7 +52,7 @@ impl Mutex {
         Mutex { inner: UnsafeCell::new(libc::PTHREAD_MUTEX_INITIALIZER) }
     }
     #[inline]
-    pub unsafe fn init(&mut self) {
+    unsafe fn init(&mut self) {
         // Issue #33770
         //
         // A pthread mutex initialized with PTHREAD_MUTEX_INITIALIZER will have
diff --git a/library/std/src/sys/unsupported/locks/mutex.rs b/library/std/src/sys/unsupported/locks/mutex.rs
@@ -16,9 +16,6 @@ impl Mutex {
         Mutex { locked: Cell::new(false) }
     }
 
-    #[inline]
-    pub unsafe fn init(&mut self) {}
-
     #[inline]
     pub unsafe fn lock(&self) {
         assert_eq!(self.locked.replace(true), false, "cannot recursively acquire mutex");
diff --git a/library/std/src/sys/windows/locks/mutex.rs b/library/std/src/sys/windows/locks/mutex.rs
@@ -37,8 +37,6 @@ impl Mutex {
     pub const fn new() -> Mutex {
         Mutex { srwlock: UnsafeCell::new(c::SRWLOCK_INIT) }
     }
-    #[inline]
-    pub unsafe fn init(&mut self) {}
 
     #[inline]
     pub unsafe fn lock(&self) {
diff --git a/library/std/src/sys_common/remutex.rs b/library/std/src/sys_common/remutex.rs
@@ -1,13 +1,11 @@
 #[cfg(all(test, not(target_os = "emscripten")))]
 mod tests;
 
+use super::mutex as sys;
 use crate::cell::UnsafeCell;
-use crate::marker::PhantomPinned;
 use crate::ops::Deref;
 use crate::panic::{RefUnwindSafe, UnwindSafe};
-use crate::pin::Pin;
 use crate::sync::atomic::{AtomicUsize, Ordering::Relaxed};
-use crate::sys::locks as sys;
 
 /// A re-entrant mutual exclusion
 ///
@@ -41,11 +39,10 @@ use crate::sys::locks as sys;
 /// synchronization is left to the mutex, making relaxed memory ordering for
 /// the `owner` field fine in all cases.
 pub struct ReentrantMutex<T> {
-    mutex: sys::Mutex,
+    mutex: sys::MovableMutex,
     owner: AtomicUsize,
     lock_count: UnsafeCell<u32>,
     data: T,
-    _pinned: PhantomPinned,
 }
 
 unsafe impl<T: Send> Send for ReentrantMutex<T> {}
@@ -68,39 +65,22 @@ impl<T> RefUnwindSafe for ReentrantMutex<T> {}
 /// guarded data.
 #[must_use = "if unused the ReentrantMutex will immediately unlock"]
 pub struct ReentrantMutexGuard<'a, T: 'a> {
-    lock: Pin<&'a ReentrantMutex<T>>,
+    lock: &'a ReentrantMutex<T>,
 }
 
 impl<T> !Send for ReentrantMutexGuard<'_, T> {}
 
 impl<T> ReentrantMutex<T> {
     /// Creates a new reentrant mutex in an unlocked state.
-    ///
-    /// # Unsafety
-    ///
-    /// This function is unsafe because it is required that `init` is called
-    /// once this mutex is in its final resting place, and only then are the
-    /// lock/unlock methods safe.
-    pub const unsafe fn new(t: T) -> ReentrantMutex<T> {
+    pub const fn new(t: T) -> ReentrantMutex<T> {
         ReentrantMutex {
-            mutex: sys::Mutex::new(),
+            mutex: sys::MovableMutex::new(),
             owner: AtomicUsize::new(0),
             lock_count: UnsafeCell::new(0),
             data: t,
-            _pinned: PhantomPinned,
         }
     }
 
-    /// Initializes this mutex so it's ready for use.
-    ///
-    /// # Unsafety
-    ///
-    /// Unsafe to call more than once, and must be called after this will no
-    /// longer move in memory.
-    pub unsafe fn init(self: Pin<&mut Self>) {
-        self.get_unchecked_mut().mutex.init()
-    }
-
     /// Acquires a mutex, blocking the current thread until it is able to do so.
     ///
     /// This function will block the caller until it is available to acquire the mutex.
@@ -113,15 +93,14 @@ impl<T> ReentrantMutex<T> {
     /// If another user of this mutex panicked while holding the mutex, then
     /// this call will return failure if the mutex would otherwise be
     /// acquired.
-    pub fn lock(self: Pin<&Self>) -> ReentrantMutexGuard<'_, T> {
+    pub fn lock(&self) -> ReentrantMutexGuard<'_, T> {
         let this_thread = current_thread_unique_ptr();
-        // Safety: We only touch lock_count when we own the lock,
-        // and since self is pinned we can safely call the lock() on the mutex.
+        // Safety: We only touch lock_count when we own the lock.
         unsafe {
             if self.owner.load(Relaxed) == this_thread {
                 self.increment_lock_count();
             } else {
-                self.mutex.lock();
+                self.mutex.raw_lock();
                 self.owner.store(this_thread, Relaxed);
                 debug_assert_eq!(*self.lock_count.get(), 0);
                 *self.lock_count.get() = 1;
@@ -142,10 +121,9 @@ impl<T> ReentrantMutex<T> {
     /// If another user of this mutex panicked while holding the mutex, then
     /// this call will return failure if the mutex would otherwise be
     /// acquired.
-    pub fn try_lock(self: Pin<&Self>) -> Option<ReentrantMutexGuard<'_, T>> {
+    pub fn try_lock(&self) -> Option<ReentrantMutexGuard<'_, T>> {
         let this_thread = current_thread_unique_ptr();
-        // Safety: We only touch lock_count when we own the lock,
-        // and since self is pinned we can safely call the try_lock on the mutex.
+        // Safety: We only touch lock_count when we own the lock.
         unsafe {
             if self.owner.load(Relaxed) == this_thread {
                 self.increment_lock_count();
@@ -179,12 +157,12 @@ impl<T> Deref for ReentrantMutexGuard<'_, T> {
 impl<T> Drop for ReentrantMutexGuard<'_, T> {
     #[inline]
     fn drop(&mut self) {
-        // Safety: We own the lock, and the lock is pinned.
+        // Safety: We own the lock.
         unsafe {
             *self.lock.lock_count.get() -= 1;
             if *self.lock.lock_count.get() == 0 {
                 self.lock.owner.store(0, Relaxed);
-                self.lock.mutex.unlock();
+                self.lock.mutex.raw_unlock();
             }
         }
     }
diff --git a/library/std/src/sys_common/remutex/tests.rs b/library/std/src/sys_common/remutex/tests.rs

Original file line number	Diff line number	Diff line change
`@@ -174,9 +174,6 @@ impl Mutex {`
`174`	`174`	`Mutex { inner: Spinlock::new(MutexInner::new()) }`
`175`	`175`	`}`
`176`	`176`
`177`		`- #[inline]`
`178`		`- pub unsafe fn init(&mut self) {}`
`179`		`-`
`180`	`177`	`#[inline]`
`181`	`178`	`pub unsafe fn lock(&self) {`
`182`	`179`	`loop {`
Original file line number	Diff line number	Diff line change
`@@ -20,9 +20,6 @@ impl Mutex {`
`20`	`20`	`Mutex { inner: SpinMutex::new(WaitVariable::new(false)) }`
`21`	`21`	`}`
`22`	`22`
`23`		`- #[inline]`
`24`		`- pub unsafe fn init(&mut self) {}`
`25`		`-`
`26`	`23`	`#[inline]`
`27`	`24`	`pub unsafe fn lock(&self) {`
`28`	`25`	`let mut guard = self.inner.lock();`
Original file line number	Diff line number	Diff line change
`@@ -85,9 +85,6 @@ impl Mutex {`
`85`	`85`	`Mutex { futex: AtomicU32::new(UNLOCKED) }`
`86`	`86`	`}`
`87`	`87`
`88`		`- #[inline]`
`89`		`- pub unsafe fn init(&mut self) {}`
`90`		`-`
`91`	`88`	`#[inline]`
`92`	`89`	`pub unsafe fn try_lock(&self) -> bool {`
`93`	`90`	`let thread_self = zx_thread_self();`
Original file line number	Diff line number	Diff line change
`@@ -19,9 +19,6 @@ impl Mutex {`
`19`	`19`	`Self { futex: AtomicU32::new(0) }`
`20`	`20`	`}`
`21`	`21`
`22`		`- #[inline]`
`23`		`- pub unsafe fn init(&mut self) {}`
`24`		`-`
`25`	`22`	`#[inline]`
`26`	`23`	`pub unsafe fn try_lock(&self) -> bool {`
`27`	`24`	`self.futex.compare_exchange(0, 1, Acquire, Relaxed).is_ok()`
Original file line number	Diff line number	Diff line change
`@@ -52,7 +52,7 @@ impl Mutex {`
`52`	`52`	`Mutex { inner: UnsafeCell::new(libc::PTHREAD_MUTEX_INITIALIZER) }`
`53`	`53`	`}`
`54`	`54`	`#[inline]`
`55`		`- pub unsafe fn init(&mut self) {`
	`55`	`+ unsafe fn init(&mut self) {`
`56`	`56`	`// Issue #33770`
`57`	`57`	`//`
`58`	`58`	`// A pthread mutex initialized with PTHREAD_MUTEX_INITIALIZER will have`
Original file line number	Diff line number	Diff line change
`@@ -16,9 +16,6 @@ impl Mutex {`
`16`	`16`	`Mutex { locked: Cell::new(false) }`
`17`	`17`	`}`
`18`	`18`
`19`		`- #[inline]`
`20`		`- pub unsafe fn init(&mut self) {}`
`21`		`-`
`22`	`19`	`#[inline]`
`23`	`20`	`pub unsafe fn lock(&self) {`
`24`	`21`	`assert_eq!(self.locked.replace(true), false, "cannot recursively acquire mutex");`
Original file line number	Diff line number	Diff line change
`@@ -37,8 +37,6 @@ impl Mutex {`
`37`	`37`	`pub const fn new() -> Mutex {`
`38`	`38`	`Mutex { srwlock: UnsafeCell::new(c::SRWLOCK_INIT) }`
`39`	`39`	`}`
`40`		`- #[inline]`
`41`		`- pub unsafe fn init(&mut self) {}`
`42`	`40`
`43`	`41`	`#[inline]`
`44`	`42`	`pub unsafe fn lock(&self) {`