From be09a4a8b2eadddadc0f3c00e40917e254ba91ab Mon Sep 17 00:00:00 2001 From: joboet Date: Mon, 15 Aug 2022 14:33:12 +0200 Subject: [PATCH] std: use `sync::RwLock` for internal statics --- library/std/src/panicking.rs | 129 +++++++------------ library/std/src/sys/solid/os.rs | 4 +- library/std/src/sys/unix/locks/mod.rs | 6 +- library/std/src/sys/unix/os.rs | 8 +- library/std/src/sys/unsupported/locks/mod.rs | 2 +- library/std/src/sys/wasm/mod.rs | 2 +- library/std/src/sys/windows/locks/mod.rs | 2 +- library/std/src/sys_common/rwlock.rs | 60 --------- 8 files changed, 62 insertions(+), 151 deletions(-) diff --git a/library/std/src/panicking.rs b/library/std/src/panicking.rs index 25c9201f2ed..38dcf6cbf7d 100644 --- a/library/std/src/panicking.rs +++ b/library/std/src/panicking.rs @@ -18,9 +18,9 @@ use crate::intrinsics; use crate::mem::{self, ManuallyDrop}; use crate::process; use crate::sync::atomic::{AtomicBool, Ordering}; +use crate::sync::{PoisonError, RwLock}; use crate::sys::stdio::panic_output; use crate::sys_common::backtrace; -use crate::sys_common::rwlock::StaticRwLock; use crate::sys_common::thread_info; use crate::thread; @@ -71,20 +71,29 @@ extern "C" fn __rust_foreign_exception() -> ! { rtabort!("Rust cannot catch foreign exceptions"); } -#[derive(Copy, Clone)] enum Hook { Default, - Custom(*mut (dyn Fn(&PanicInfo<'_>) + 'static + Sync + Send)), + Custom(Box) + 'static + Sync + Send>), } impl Hook { - fn custom(f: impl Fn(&PanicInfo<'_>) + 'static + Sync + Send) -> Self { - Self::Custom(Box::into_raw(Box::new(f))) + #[inline] + fn into_box(self) -> Box) + 'static + Sync + Send> { + match self { + Hook::Default => Box::new(default_hook), + Hook::Custom(hook) => hook, + } } } -static HOOK_LOCK: StaticRwLock = StaticRwLock::new(); -static mut HOOK: Hook = Hook::Default; +impl Default for Hook { + #[inline] + fn default() -> Hook { + Hook::Default + } +} + +static HOOK: RwLock = RwLock::new(Hook::Default); /// Registers a custom panic hook, replacing any that was previously registered. /// @@ -125,24 +134,13 @@ pub fn set_hook(hook: Box) + 'static + Sync + Send>) { panic!("cannot modify the panic hook from a panicking thread"); } - // SAFETY: - // - // - `HOOK` can only be modified while holding write access to `HOOK_LOCK`. - // - The argument of `Box::from_raw` is always a valid pointer that was created using - // `Box::into_raw`. - unsafe { - let guard = HOOK_LOCK.write(); - let old_hook = HOOK; - HOOK = Hook::Custom(Box::into_raw(hook)); - drop(guard); - - if let Hook::Custom(ptr) = old_hook { - #[allow(unused_must_use)] - { - Box::from_raw(ptr); - } - } - } + let new = Hook::Custom(hook); + let mut hook = HOOK.write().unwrap_or_else(PoisonError::into_inner); + let old = mem::replace(&mut *hook, new); + drop(hook); + // Only drop the old hook after releasing the lock to avoid deadlocking + // if its destructor panics. + drop(old); } /// Unregisters the current panic hook, returning it. @@ -179,22 +177,11 @@ pub fn take_hook() -> Box) + 'static + Sync + Send> { panic!("cannot modify the panic hook from a panicking thread"); } - // SAFETY: - // - // - `HOOK` can only be modified while holding write access to `HOOK_LOCK`. - // - The argument of `Box::from_raw` is always a valid pointer that was created using - // `Box::into_raw`. - unsafe { - let guard = HOOK_LOCK.write(); - let hook = HOOK; - HOOK = Hook::Default; - drop(guard); + let mut hook = HOOK.write().unwrap_or_else(PoisonError::into_inner); + let old_hook = mem::take(&mut *hook); + drop(hook); - match hook { - Hook::Default => Box::new(default_hook), - Hook::Custom(ptr) => Box::from_raw(ptr), - } - } + old_hook.into_box() } /// Atomic combination of [`take_hook`] and [`set_hook`]. Use this to replace the panic handler with @@ -240,24 +227,9 @@ where panic!("cannot modify the panic hook from a panicking thread"); } - // SAFETY: - // - // - `HOOK` can only be modified while holding write access to `HOOK_LOCK`. - // - The argument of `Box::from_raw` is always a valid pointer that was created using - // `Box::into_raw`. - unsafe { - let guard = HOOK_LOCK.write(); - let old_hook = HOOK; - HOOK = Hook::Default; - - let prev = match old_hook { - Hook::Default => Box::new(default_hook), - Hook::Custom(ptr) => Box::from_raw(ptr), - }; - - HOOK = Hook::custom(move |info| hook_fn(&prev, info)); - drop(guard); - } + let mut hook = HOOK.write().unwrap_or_else(PoisonError::into_inner); + let prev = mem::take(&mut *hook).into_box(); + *hook = Hook::Custom(Box::new(move |info| hook_fn(&prev, info))); } fn default_hook(info: &PanicInfo<'_>) { @@ -682,27 +654,26 @@ fn rust_panic_with_hook( crate::sys::abort_internal(); } - unsafe { - let mut info = PanicInfo::internal_constructor(message, location, can_unwind); - let _guard = HOOK_LOCK.read(); - match HOOK { - // Some platforms (like wasm) know that printing to stderr won't ever actually - // print anything, and if that's the case we can skip the default - // hook. Since string formatting happens lazily when calling `payload` - // methods, this means we avoid formatting the string at all! - // (The panic runtime might still call `payload.take_box()` though and trigger - // formatting.) - Hook::Default if panic_output().is_none() => {} - Hook::Default => { - info.set_payload(payload.get()); - default_hook(&info); - } - Hook::Custom(ptr) => { - info.set_payload(payload.get()); - (*ptr)(&info); - } - }; - } + let mut info = PanicInfo::internal_constructor(message, location, can_unwind); + let hook = HOOK.read().unwrap_or_else(PoisonError::into_inner); + match *hook { + // Some platforms (like wasm) know that printing to stderr won't ever actually + // print anything, and if that's the case we can skip the default + // hook. Since string formatting happens lazily when calling `payload` + // methods, this means we avoid formatting the string at all! + // (The panic runtime might still call `payload.take_box()` though and trigger + // formatting.) + Hook::Default if panic_output().is_none() => {} + Hook::Default => { + info.set_payload(payload.get()); + default_hook(&info); + } + Hook::Custom(ref hook) => { + info.set_payload(payload.get()); + hook(&info); + } + }; + drop(hook); if panics > 1 || !can_unwind { // If a thread panics while it's already unwinding then we diff --git a/library/std/src/sys/solid/os.rs b/library/std/src/sys/solid/os.rs index b5649d6e0ff..eecb347e599 100644 --- a/library/std/src/sys/solid/os.rs +++ b/library/std/src/sys/solid/os.rs @@ -8,7 +8,7 @@ use crate::os::{ solid::ffi::{OsStrExt, OsStringExt}, }; use crate::path::{self, PathBuf}; -use crate::sys_common::rwlock::StaticRwLock; +use crate::sync::RwLock; use crate::vec; use super::{error, itron, memchr}; @@ -78,7 +78,7 @@ pub fn current_exe() -> io::Result { unsupported() } -static ENV_LOCK: StaticRwLock = StaticRwLock::new(); +static ENV_LOCK: RwLock<()> = RwLock::new(()); pub struct Env { iter: vec::IntoIter<(OsString, OsString)>, diff --git a/library/std/src/sys/unix/locks/mod.rs b/library/std/src/sys/unix/locks/mod.rs index f5f92f69358..9bb314b7010 100644 --- a/library/std/src/sys/unix/locks/mod.rs +++ b/library/std/src/sys/unix/locks/mod.rs @@ -11,21 +11,21 @@ cfg_if::cfg_if! { mod futex_rwlock; mod futex_condvar; pub(crate) use futex_mutex::{Mutex, MovableMutex}; - pub(crate) use futex_rwlock::{RwLock, MovableRwLock}; + pub(crate) use futex_rwlock::MovableRwLock; pub(crate) use futex_condvar::MovableCondvar; } else if #[cfg(target_os = "fuchsia")] { mod fuchsia_mutex; mod futex_rwlock; mod futex_condvar; pub(crate) use fuchsia_mutex::{Mutex, MovableMutex}; - pub(crate) use futex_rwlock::{RwLock, MovableRwLock}; + pub(crate) use futex_rwlock::MovableRwLock; pub(crate) use futex_condvar::MovableCondvar; } else { mod pthread_mutex; mod pthread_rwlock; mod pthread_condvar; pub(crate) use pthread_mutex::{Mutex, MovableMutex}; - pub(crate) use pthread_rwlock::{RwLock, MovableRwLock}; + pub(crate) use pthread_rwlock::MovableRwLock; pub(crate) use pthread_condvar::MovableCondvar; } } diff --git a/library/std/src/sys/unix/os.rs b/library/std/src/sys/unix/os.rs index 46545a0839f..3c3770708b1 100644 --- a/library/std/src/sys/unix/os.rs +++ b/library/std/src/sys/unix/os.rs @@ -17,10 +17,10 @@ use crate::path::{self, PathBuf}; use crate::ptr; use crate::slice; use crate::str; +use crate::sync::{PoisonError, RwLock}; use crate::sys::cvt; use crate::sys::fd; use crate::sys::memchr; -use crate::sys_common::rwlock::{StaticRwLock, StaticRwLockReadGuard}; use crate::vec; #[cfg(all(target_env = "gnu", not(target_os = "vxworks")))] @@ -501,10 +501,10 @@ pub unsafe fn environ() -> *mut *const *const c_char { ptr::addr_of_mut!(environ) } -static ENV_LOCK: StaticRwLock = StaticRwLock::new(); +static ENV_LOCK: RwLock<()> = RwLock::new(()); -pub fn env_read_lock() -> StaticRwLockReadGuard { - ENV_LOCK.read() +pub fn env_read_lock() -> impl Drop { + ENV_LOCK.read().unwrap_or_else(PoisonError::into_inner) } /// Returns a vector of (variable, value) byte-vector pairs for all the diff --git a/library/std/src/sys/unsupported/locks/mod.rs b/library/std/src/sys/unsupported/locks/mod.rs index d412ff152a0..602a2d6231a 100644 --- a/library/std/src/sys/unsupported/locks/mod.rs +++ b/library/std/src/sys/unsupported/locks/mod.rs @@ -3,4 +3,4 @@ mod mutex; mod rwlock; pub use condvar::{Condvar, MovableCondvar}; pub use mutex::{MovableMutex, Mutex}; -pub use rwlock::{MovableRwLock, RwLock}; +pub use rwlock::MovableRwLock; diff --git a/library/std/src/sys/wasm/mod.rs b/library/std/src/sys/wasm/mod.rs index 4159efe2a05..93838390bee 100644 --- a/library/std/src/sys/wasm/mod.rs +++ b/library/std/src/sys/wasm/mod.rs @@ -57,7 +57,7 @@ cfg_if::cfg_if! { mod futex_rwlock; pub(crate) use futex_condvar::{Condvar, MovableCondvar}; pub(crate) use futex_mutex::{Mutex, MovableMutex}; - pub(crate) use futex_rwlock::{RwLock, MovableRwLock}; + pub(crate) use futex_rwlock::MovableRwLock; } #[path = "atomics/futex.rs"] pub mod futex; diff --git a/library/std/src/sys/windows/locks/mod.rs b/library/std/src/sys/windows/locks/mod.rs index d412ff152a0..602a2d6231a 100644 --- a/library/std/src/sys/windows/locks/mod.rs +++ b/library/std/src/sys/windows/locks/mod.rs @@ -3,4 +3,4 @@ mod mutex; mod rwlock; pub use condvar::{Condvar, MovableCondvar}; pub use mutex::{MovableMutex, Mutex}; -pub use rwlock::{MovableRwLock, RwLock}; +pub use rwlock::MovableRwLock; diff --git a/library/std/src/sys_common/rwlock.rs b/library/std/src/sys_common/rwlock.rs index ba56f3a8f1b..36d721fcbcb 100644 --- a/library/std/src/sys_common/rwlock.rs +++ b/library/std/src/sys_common/rwlock.rs @@ -1,65 +1,5 @@ use crate::sys::locks as imp; -/// An OS-based reader-writer lock, meant for use in static variables. -/// -/// This rwlock does not implement poisoning. -/// -/// This rwlock has a const constructor ([`StaticRwLock::new`]), does not -/// implement `Drop` to cleanup resources. -pub struct StaticRwLock(imp::RwLock); - -impl StaticRwLock { - /// Creates a new rwlock for use. - #[inline] - pub const fn new() -> Self { - Self(imp::RwLock::new()) - } - - /// Acquires shared access to the underlying lock, blocking the current - /// thread to do so. - /// - /// The lock is automatically unlocked when the returned guard is dropped. - #[inline] - pub fn read(&'static self) -> StaticRwLockReadGuard { - unsafe { self.0.read() }; - StaticRwLockReadGuard(&self.0) - } - - /// Acquires write access to the underlying lock, blocking the current thread - /// to do so. - /// - /// The lock is automatically unlocked when the returned guard is dropped. - #[inline] - pub fn write(&'static self) -> StaticRwLockWriteGuard { - unsafe { self.0.write() }; - StaticRwLockWriteGuard(&self.0) - } -} - -#[must_use] -pub struct StaticRwLockReadGuard(&'static imp::RwLock); - -impl Drop for StaticRwLockReadGuard { - #[inline] - fn drop(&mut self) { - unsafe { - self.0.read_unlock(); - } - } -} - -#[must_use] -pub struct StaticRwLockWriteGuard(&'static imp::RwLock); - -impl Drop for StaticRwLockWriteGuard { - #[inline] - fn drop(&mut self) { - unsafe { - self.0.write_unlock(); - } - } -} - /// An OS-based reader-writer lock. /// /// This rwlock cleans up its resources in its `Drop` implementation and may