From 2bc5d44ca963a4dccf09c993fe9813f6a795aedf Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sat, 10 Oct 2020 20:13:03 +0200 Subject: [PATCH 1/4] Fix outdated comment about not needing to flush stderr. --- library/std/src/io/stdio.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/library/std/src/io/stdio.rs b/library/std/src/io/stdio.rs index 6ea7704d42213..b5594c8e093bd 100644 --- a/library/std/src/io/stdio.rs +++ b/library/std/src/io/stdio.rs @@ -756,13 +756,9 @@ pub struct StderrLock<'a> { /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub fn stderr() -> Stderr { - // Note that unlike `stdout()` we don't use `Lazy` here which registers a - // destructor. Stderr is not buffered nor does the `stderr_raw` type consume - // any owned resources, so there's no need to run any destructors at some - // point in the future. - // - // This has the added benefit of allowing `stderr` to be usable during - // process shutdown as well! + // 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 for flushing the buffer static INSTANCE: SyncOnceCell>> = SyncOnceCell::new(); Stderr { From 9dc7f13c39febc6466c8f79ad4c270ab8a63881f Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sat, 10 Oct 2020 20:15:55 +0200 Subject: [PATCH 2/4] Remove unnecessary import of `crate::marker` in std::sys_common::remutex. It was used for marker::Send, but Send is already in scope. --- library/std/src/sys_common/remutex.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/library/std/src/sys_common/remutex.rs b/library/std/src/sys_common/remutex.rs index 162eab2388d55..35d6701efd0bd 100644 --- a/library/std/src/sys_common/remutex.rs +++ b/library/std/src/sys_common/remutex.rs @@ -2,7 +2,6 @@ mod tests; use crate::fmt; -use crate::marker; use crate::ops::Deref; use crate::panic::{RefUnwindSafe, UnwindSafe}; use crate::sys::mutex as sys; @@ -40,7 +39,7 @@ pub struct ReentrantMutexGuard<'a, T: 'a> { lock: &'a ReentrantMutex, } -impl !marker::Send for ReentrantMutexGuard<'_, T> {} +impl !Send for ReentrantMutexGuard<'_, T> {} impl ReentrantMutex { /// Creates a new reentrant mutex in an unlocked state. From 8fe90966e19a4c3b6d05484d5368e5f9b444eb8b Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Wed, 7 Oct 2020 17:19:38 +0200 Subject: [PATCH 3/4] Add (internal-only) SyncOnceCell::get_or_init_pin. --- library/std/src/lazy.rs | 55 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/library/std/src/lazy.rs b/library/std/src/lazy.rs index e0095e64faf31..68f57958bb233 100644 --- a/library/std/src/lazy.rs +++ b/library/std/src/lazy.rs @@ -10,6 +10,7 @@ use crate::{ mem::MaybeUninit, ops::{Deref, Drop}, panic::{RefUnwindSafe, UnwindSafe}, + pin::Pin, sync::Once, }; @@ -297,6 +298,60 @@ impl SyncOnceCell { 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(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 `SyncOnceCell`, returning the wrapped value. Returns /// `None` if the cell was empty. /// From 67c18fdec59cf313818b8606c3847a8af6bcf684 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sat, 10 Oct 2020 20:20:14 +0200 Subject: [PATCH 4/4] Use Pin for the 'don't move' requirement of ReentrantMutex. The code in io::stdio before this change misused the ReentrantMutexes, by calling init() on them and moving them afterwards. Now that ReentrantMutex requires Pin for init(), this mistake is no longer easy to make. --- library/std/src/io/stdio.rs | 54 +++++++++++---------- library/std/src/lib.rs | 2 + library/std/src/sys_common/remutex.rs | 52 ++++++++------------ library/std/src/sys_common/remutex/tests.rs | 35 +++++++------ 4 files changed, 70 insertions(+), 73 deletions(-) diff --git a/library/std/src/io/stdio.rs b/library/std/src/io/stdio.rs index b5594c8e093bd..1160011f35287 100644 --- a/library/std/src/io/stdio.rs +++ b/library/std/src/io/stdio.rs @@ -9,6 +9,7 @@ use crate::cell::{Cell, RefCell}; use crate::fmt; use crate::io::{self, BufReader, Initializer, IoSlice, IoSliceMut, LineWriter}; use crate::lazy::SyncOnceCell; +use crate::pin::Pin; use crate::sync::atomic::{AtomicBool, Ordering}; use crate::sync::{Arc, Mutex, MutexGuard}; use crate::sys::stdio; @@ -490,7 +491,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: &'static ReentrantMutex>>, + inner: Pin<&'static ReentrantMutex>>>, } /// A locked reference to the `Stdout` handle. @@ -550,25 +551,29 @@ pub struct StdoutLock<'a> { pub fn stdout() -> Stdout { static INSTANCE: SyncOnceCell>>> = SyncOnceCell::new(); + + fn cleanup() { + if let Some(instance) = INSTANCE.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()); + } + } + } + Stdout { - inner: INSTANCE.get_or_init(|| unsafe { - let _ = sys_common::at_exit(|| { - if let Some(instance) = INSTANCE.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) = instance.try_lock() { - *lock.borrow_mut() = LineWriter::with_capacity(0, stdout_raw()); - } - } - }); - let r = ReentrantMutex::new(RefCell::new(LineWriter::new(stdout_raw()))); - r.init(); - r - }), + inner: Pin::static_ref(&INSTANCE).get_or_init_pin( + || unsafe { + let _ = sys_common::at_exit(cleanup); + ReentrantMutex::new(RefCell::new(LineWriter::new(stdout_raw()))) + }, + |mutex| unsafe { mutex.init() }, + ), } } @@ -700,7 +705,7 @@ impl fmt::Debug for StdoutLock<'_> { /// an error. #[stable(feature = "rust1", since = "1.0.0")] pub struct Stderr { - inner: &'static ReentrantMutex>, + inner: Pin<&'static ReentrantMutex>>, } /// A locked reference to the `Stderr` handle. @@ -762,11 +767,10 @@ pub fn stderr() -> Stderr { static INSTANCE: SyncOnceCell>> = SyncOnceCell::new(); Stderr { - inner: INSTANCE.get_or_init(|| unsafe { - let r = ReentrantMutex::new(RefCell::new(stderr_raw())); - r.init(); - r - }), + inner: Pin::static_ref(&INSTANCE).get_or_init_pin( + || unsafe { ReentrantMutex::new(RefCell::new(stderr_raw())) }, + |mutex| unsafe { mutex.init() }, + ), } } diff --git a/library/std/src/lib.rs b/library/std/src/lib.rs index 6c240cb4c3ed9..def8db1f45c37 100644 --- a/library/std/src/lib.rs +++ b/library/std/src/lib.rs @@ -266,6 +266,7 @@ #![feature(format_args_nl)] #![feature(gen_future)] #![feature(generator_trait)] +#![feature(get_mut_unchecked)] #![feature(global_asm)] #![feature(hashmap_internals)] #![feature(int_error_internals)] @@ -293,6 +294,7 @@ #![feature(panic_info_message)] #![feature(panic_internals)] #![feature(panic_unwind)] +#![feature(pin_static_ref)] #![feature(prelude_import)] #![feature(ptr_internals)] #![feature(raw)] diff --git a/library/std/src/sys_common/remutex.rs b/library/std/src/sys_common/remutex.rs index 35d6701efd0bd..475bfca9b6dcc 100644 --- a/library/std/src/sys_common/remutex.rs +++ b/library/std/src/sys_common/remutex.rs @@ -1,9 +1,10 @@ #[cfg(all(test, not(target_os = "emscripten")))] mod tests; -use crate::fmt; +use crate::marker::PhantomPinned; use crate::ops::Deref; use crate::panic::{RefUnwindSafe, UnwindSafe}; +use crate::pin::Pin; use crate::sys::mutex as sys; /// A re-entrant mutual exclusion @@ -14,6 +15,7 @@ use crate::sys::mutex as sys; pub struct ReentrantMutex { inner: sys::ReentrantMutex, data: T, + _pinned: PhantomPinned, } unsafe impl Send for ReentrantMutex {} @@ -36,7 +38,7 @@ impl RefUnwindSafe for ReentrantMutex {} /// guarded data. #[must_use = "if unused the ReentrantMutex will immediately unlock"] pub struct ReentrantMutexGuard<'a, T: 'a> { - lock: &'a ReentrantMutex, + lock: Pin<&'a ReentrantMutex>, } impl !Send for ReentrantMutexGuard<'_, T> {} @@ -50,7 +52,11 @@ impl ReentrantMutex { /// 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 { - ReentrantMutex { inner: sys::ReentrantMutex::uninitialized(), data: t } + ReentrantMutex { + inner: sys::ReentrantMutex::uninitialized(), + data: t, + _pinned: PhantomPinned, + } } /// Initializes this mutex so it's ready for use. @@ -59,8 +65,8 @@ impl ReentrantMutex { /// /// Unsafe to call more than once, and must be called after this will no /// longer move in memory. - pub unsafe fn init(&self) { - self.inner.init(); + pub unsafe fn init(self: Pin<&mut Self>) { + self.get_unchecked_mut().inner.init() } /// Acquires a mutex, blocking the current thread until it is able to do so. @@ -75,9 +81,9 @@ impl ReentrantMutex { /// 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) -> ReentrantMutexGuard<'_, T> { + pub fn lock(self: Pin<&Self>) -> ReentrantMutexGuard<'_, T> { unsafe { self.inner.lock() } - ReentrantMutexGuard::new(&self) + ReentrantMutexGuard { lock: self } } /// Attempts to acquire this lock. @@ -92,8 +98,12 @@ impl ReentrantMutex { /// 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) -> Option> { - if unsafe { self.inner.try_lock() } { Some(ReentrantMutexGuard::new(&self)) } else { None } + pub fn try_lock(self: Pin<&Self>) -> Option> { + if unsafe { self.inner.try_lock() } { + Some(ReentrantMutexGuard { lock: self }) + } else { + None + } } } @@ -106,30 +116,6 @@ impl Drop for ReentrantMutex { } } -impl fmt::Debug for ReentrantMutex { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self.try_lock() { - Some(guard) => f.debug_struct("ReentrantMutex").field("data", &*guard).finish(), - None => { - struct LockedPlaceholder; - impl fmt::Debug for LockedPlaceholder { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.write_str("") - } - } - - f.debug_struct("ReentrantMutex").field("data", &LockedPlaceholder).finish() - } - } - } -} - -impl<'mutex, T> ReentrantMutexGuard<'mutex, T> { - fn new(lock: &'mutex ReentrantMutex) -> ReentrantMutexGuard<'mutex, T> { - ReentrantMutexGuard { lock } - } -} - impl Deref for ReentrantMutexGuard<'_, T> { type Target = T; diff --git a/library/std/src/sys_common/remutex/tests.rs b/library/std/src/sys_common/remutex/tests.rs index 9c686e579d735..88453ded2f9fb 100644 --- a/library/std/src/sys_common/remutex/tests.rs +++ b/library/std/src/sys_common/remutex/tests.rs @@ -1,4 +1,6 @@ +use crate::boxed::Box; use crate::cell::RefCell; +use crate::pin::Pin; use crate::sync::Arc; use crate::sys_common::remutex::{ReentrantMutex, ReentrantMutexGuard}; use crate::thread; @@ -6,10 +8,11 @@ use crate::thread; #[test] fn smoke() { let m = unsafe { - let m = ReentrantMutex::new(()); - m.init(); + let mut m = Box::pin(ReentrantMutex::new(())); + m.as_mut().init(); m }; + let m = m.as_ref(); { let a = m.lock(); { @@ -27,18 +30,19 @@ fn smoke() { #[test] fn is_mutex() { let m = unsafe { - let m = Arc::new(ReentrantMutex::new(RefCell::new(0))); - m.init(); - m + // FIXME: Simplify this if Arc gets a Arc::get_pin_mut. + let mut m = Arc::new(ReentrantMutex::new(RefCell::new(0))); + Pin::new_unchecked(Arc::get_mut_unchecked(&mut m)).init(); + Pin::new_unchecked(m) }; let m2 = m.clone(); - let lock = m.lock(); + let lock = m.as_ref().lock(); let child = thread::spawn(move || { - let lock = m2.lock(); + let lock = m2.as_ref().lock(); assert_eq!(*lock.borrow(), 4950); }); for i in 0..100 { - let lock = m.lock(); + let lock = m.as_ref().lock(); *lock.borrow_mut() += i; } drop(lock); @@ -48,20 +52,21 @@ fn is_mutex() { #[test] fn trylock_works() { let m = unsafe { - let m = Arc::new(ReentrantMutex::new(())); - m.init(); - m + // FIXME: Simplify this if Arc gets a Arc::get_pin_mut. + let mut m = Arc::new(ReentrantMutex::new(())); + Pin::new_unchecked(Arc::get_mut_unchecked(&mut m)).init(); + Pin::new_unchecked(m) }; let m2 = m.clone(); - let _lock = m.try_lock(); - let _lock2 = m.try_lock(); + let _lock = m.as_ref().try_lock(); + let _lock2 = m.as_ref().try_lock(); thread::spawn(move || { - let lock = m2.try_lock(); + let lock = m2.as_ref().try_lock(); assert!(lock.is_none()); }) .join() .unwrap(); - let _lock3 = m.try_lock(); + let _lock3 = m.as_ref().try_lock(); } pub struct Answer<'a>(pub ReentrantMutexGuard<'a, RefCell>);