Auto merge of rust-lang#100579 - joboet:sync_mutex_everywhere, r=thomcc

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

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

I changed the program argument implementation on Hermit, it does not need `Mutex` but can use atomics like some UNIX systems (ping `@mkroening` `@stlankes).`

Author: bors

library/std/src/backtrace.rs

@@ -325,8 +325,7 @@ impl Backtrace {
     // Capture a backtrace which start just before the function addressed by
     // `ip`
     fn create(ip: usize) -> Backtrace {
-        // SAFETY: We don't attempt to lock this reentrantly.
-        let _lock = unsafe { lock() };
+        let _lock = lock();
         let mut frames = Vec::new();
         let mut actual_start = None;
         unsafe {
@@ -469,8 +468,7 @@ impl Capture {
         // Use the global backtrace lock to synchronize this as it's a
         // requirement of the `backtrace` crate, and then actually resolve
         // everything.
-        // SAFETY: We don't attempt to lock this reentrantly.
-        let _lock = unsafe { lock() };
+        let _lock = lock();
         for frame in self.frames.iter_mut() {
             let symbols = &mut frame.symbols;
             let frame = match &frame.frame {

library/std/src/sys/hermit/args.rs

@@ -1,20 +1,37 @@
-use crate::ffi::OsString;
+use crate::ffi::{c_char, CStr, OsString};
 use crate::fmt;
+use crate::os::unix::ffi::OsStringExt;
+use crate::ptr;
+use crate::sync::atomic::{
+    AtomicIsize, AtomicPtr,
+    Ordering::{Acquire, Relaxed, Release},
+};
 use crate::vec;
 
+static ARGC: AtomicIsize = AtomicIsize::new(0);
+static ARGV: AtomicPtr<*const u8> = AtomicPtr::new(ptr::null_mut());
+
 /// One-time global initialization.
 pub unsafe fn init(argc: isize, argv: *const *const u8) {
-    imp::init(argc, argv)
-}
-
-/// One-time global cleanup.
-pub unsafe fn cleanup() {
-    imp::cleanup()
+    ARGC.store(argc, Relaxed);
+    // Use release ordering here to broadcast writes by the OS.
+    ARGV.store(argv as *mut *const u8, Release);
 }
 
 /// Returns the command line arguments
 pub fn args() -> Args {
-    imp::args()
+    // Synchronize with the store above.
+    let argv = ARGV.load(Acquire);
+    // If argv has not been initialized yet, do not return any arguments.
+    let argc = if argv.is_null() { 0 } else { ARGC.load(Relaxed) };
+    let args: Vec<OsString> = (0..argc)
+        .map(|i| unsafe {
+            let cstr = CStr::from_ptr(*argv.offset(i) as *const c_char);
+            OsStringExt::from_vec(cstr.to_bytes().to_vec())
+        })
+        .collect();
+
+    Args { iter: args.into_iter() }
 }
 
 pub struct Args {
@@ -51,44 +68,3 @@ impl DoubleEndedIterator for Args {
         self.iter.next_back()
     }
 }
-
-mod imp {
-    use super::Args;
-    use crate::ffi::{CStr, OsString};
-    use crate::os::unix::ffi::OsStringExt;
-    use crate::ptr;
-
-    use crate::sys_common::mutex::StaticMutex;
-
-    static mut ARGC: isize = 0;
-    static mut ARGV: *const *const u8 = ptr::null();
-    static LOCK: StaticMutex = StaticMutex::new();
-
-    pub unsafe fn init(argc: isize, argv: *const *const u8) {
-        let _guard = LOCK.lock();
-        ARGC = argc;
-        ARGV = argv;
-    }
-
-    pub unsafe fn cleanup() {
-        let _guard = LOCK.lock();
-        ARGC = 0;
-        ARGV = ptr::null();
-    }
-
-    pub fn args() -> Args {
-        Args { iter: clone().into_iter() }
-    }
-
-    fn clone() -> Vec<OsString> {
-        unsafe {
-            let _guard = LOCK.lock();
-            (0..ARGC)
-                .map(|i| {
-                    let cstr = CStr::from_ptr(*ARGV.offset(i) as *const i8);
-                    OsStringExt::from_vec(cstr.to_bytes().to_vec())
-                })
-                .collect()
-        }
-    }
-}

library/std/src/sys/hermit/mod.rs

@@ -106,9 +106,7 @@ pub unsafe fn init(argc: isize, argv: *const *const u8, _sigpipe: u8) {
 
 // SAFETY: must be called only once during runtime cleanup.
 // NOTE: this is not guaranteed to run, for example when the program aborts.
-pub unsafe fn cleanup() {
-    args::cleanup();
-}
+pub unsafe fn cleanup() {}
 
 #[cfg(not(test))]
 #[no_mangle]

library/std/src/sys/windows/process.rs

@@ -16,6 +16,7 @@ use crate::os::windows::ffi::{OsStrExt, OsStringExt};
 use crate::os::windows::io::{AsHandle, AsRawHandle, BorrowedHandle, FromRawHandle, IntoRawHandle};
 use crate::path::{Path, PathBuf};
 use crate::ptr;
+use crate::sync::Mutex;
 use crate::sys::args::{self, Arg};
 use crate::sys::c;
 use crate::sys::c::NonZeroDWORD;
@@ -25,7 +26,6 @@ use crate::sys::handle::Handle;
 use crate::sys::path;
 use crate::sys::pipe::{self, AnonPipe};
 use crate::sys::stdio;
-use crate::sys_common::mutex::StaticMutex;
 use crate::sys_common::process::{CommandEnv, CommandEnvs};
 use crate::sys_common::IntoInner;
 
@@ -301,9 +301,9 @@ impl Command {
         //
         // For more information, msdn also has an article about this race:
         // https://support.microsoft.com/kb/315939
-        static CREATE_PROCESS_LOCK: StaticMutex = StaticMutex::new();
+        static CREATE_PROCESS_LOCK: Mutex<()> = Mutex::new(());
 
-        let _guard = unsafe { CREATE_PROCESS_LOCK.lock() };
+        let _guard = CREATE_PROCESS_LOCK.lock();
 
         let mut pipes = StdioPipes { stdin: None, stdout: None, stderr: None };
         let null = Stdio::Null;

library/std/src/sys_common/backtrace.rs

@@ -7,15 +7,14 @@ use crate::fmt;
 use crate::io;
 use crate::io::prelude::*;
 use crate::path::{self, Path, PathBuf};
-use crate::sys_common::mutex::StaticMutex;
+use crate::sync::{Mutex, PoisonError};
 
 /// Max number of frames to print.
 const MAX_NB_FRAMES: usize = 100;
 
-// SAFETY: Don't attempt to lock this reentrantly.
-pub unsafe fn lock() -> impl Drop {
-    static LOCK: StaticMutex = StaticMutex::new();
-    LOCK.lock()
+pub fn lock() -> impl Drop {
+    static LOCK: Mutex<()> = Mutex::new(());
+    LOCK.lock().unwrap_or_else(PoisonError::into_inner)
 }
 
 /// Prints the current backtrace.

library/std/src/sys_common/mutex.rs

@@ -1,49 +1,5 @@
 use crate::sys::locks as imp;
 
-/// 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 used reentrantly.
-///
-/// This mutex does not implement poisoning.
-///
-/// This is a wrapper around `imp::Mutex` that does *not* call `init()` and
-/// `destroy()`.
-pub struct StaticMutex(imp::Mutex);
-
-unsafe impl Sync for StaticMutex {}
-
-impl StaticMutex {
-    /// Creates a new mutex for use.
-    #[inline]
-    pub const fn new() -> Self {
-        Self(imp::Mutex::new())
-    }
-
-    /// Calls raw_lock() and then returns an RAII guard to guarantee the mutex
-    /// will be unlocked.
-    ///
-    /// It is undefined behaviour to call this function while locked by the
-    /// same thread.
-    #[inline]
-    pub unsafe fn lock(&'static self) -> StaticMutexGuard {
-        self.0.lock();
-        StaticMutexGuard(&self.0)
-    }
-}
-
-#[must_use]
-pub struct StaticMutexGuard(&'static imp::Mutex);
-
-impl Drop for StaticMutexGuard {
-    #[inline]
-    fn drop(&mut self) {
-        unsafe {
-            self.0.unlock();
-        }
-    }
-}
-
 /// An OS-based mutual exclusion lock.
 ///
 /// This mutex cleans up its resources in its `Drop` implementation, may safely

library/std/src/thread/mod.rs

@@ -1128,24 +1128,21 @@ impl ThreadId {
                     }
                 }
             } else {
-                use crate::sys_common::mutex::StaticMutex;
+                use crate::sync::{Mutex, PoisonError};
 
-                // It is UB to attempt to acquire this mutex reentrantly!
-                static GUARD: StaticMutex = StaticMutex::new();
-                static mut COUNTER: u64 = 0;
+                static COUNTER: Mutex<u64> = Mutex::new(0);
 
-                unsafe {
-                    let guard = GUARD.lock();
+                let mut counter = COUNTER.lock().unwrap_or_else(PoisonError::into_inner);
+                let Some(id) = counter.checked_add(1) else {
+                    // in case the panic handler ends up calling `ThreadId::new()`,
+                    // avoid reentrant lock acquire.
+                    drop(counter);
+                    exhausted();
+                };
 
-                    let Some(id) = COUNTER.checked_add(1) else {
-                        drop(guard); // in case the panic handler ends up calling `ThreadId::new()`, avoid reentrant lock acquire.
-                        exhausted();
-                    };
-
-                    COUNTER = id;
-                    drop(guard);
-                    ThreadId(NonZeroU64::new(id).unwrap())
-                }
+                *counter = id;
+                drop(counter);
+                ThreadId(NonZeroU64::new(id).unwrap())
             }
         }
     }