Auto merge of #100581 - joboet:sync_rwlock_everywhere, r=thomcc

std: use `sync::RwLock` for internal statics

Since `sync::RwLock` is now `const`-constructible, it can be used for internal statics, removing the need for `sys_common::StaticRwLock`. This adds some extra allocations on platforms which need to box their locks (currently SGX and some UNIX), but these will become unnecessary with the lock improvements tracked in #93740.

Author: bors

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<dyn Fn(&PanicInfo<'_>) + '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<dyn Fn(&PanicInfo<'_>) + '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<Hook> = 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<dyn Fn(&PanicInfo<'_>) + '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<dyn Fn(&PanicInfo<'_>) + '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

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<PathBuf> {
     unsupported()
 }
 
-static ENV_LOCK: StaticRwLock = StaticRwLock::new();
+static ENV_LOCK: RwLock<()> = RwLock::new(());
 
 pub struct Env {
     iter: vec::IntoIter<(OsString, OsString)>,

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;
     }
 }

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

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;

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;

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;

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