Skip to content

Do not wrap platform locks in sys_common #100505

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions library/std/src/backtrace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
74 changes: 37 additions & 37 deletions library/std/src/io/stdio.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<Option<LineWriter<StdoutRaw>>>>,
}

/// A locked reference to the [`Stdout`] handle.
Expand All @@ -548,10 +547,11 @@ pub struct Stdout {
#[must_use = "if unused stdout will immediately unlock"]
#[stable(feature = "rust1", since = "1.0.0")]
pub struct StdoutLock<'a> {
inner: ReentrantMutexGuard<'a, RefCell<LineWriter<StdoutRaw>>>,
inner: ReentrantMutexGuard<'a, RefCell<Option<LineWriter<StdoutRaw>>>>,
}

static STDOUT: OnceLock<ReentrantMutex<RefCell<LineWriter<StdoutRaw>>>> = OnceLock::new();
static STDOUT: ReentrantMutex<RefCell<Option<LineWriter<StdoutRaw>>>> =
ReentrantMutex::new(RefCell::new(None));

/// Constructs a new handle to the standard output of the current process.
///
Expand Down Expand Up @@ -602,25 +602,18 @@ static STDOUT: OnceLock<ReentrantMutex<RefCell<LineWriter<StdoutRaw>>>> = OnceLo
#[must_use]
#[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() },
),
}
Stdout { inner: &STDOUT }
}

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.try_lock() {
*lock.borrow_mut() = Some(LineWriter::with_capacity(0, stdout_raw()));
}
}

Expand Down Expand Up @@ -670,7 +663,7 @@ impl Write for Stdout {
}
#[inline]
fn is_write_vectored(&self) -> bool {
io::Write::is_write_vectored(&&*self)
stdout_raw().is_write_vectored()
}
fn flush(&mut self) -> io::Result<()> {
(&*self).flush()
Expand All @@ -696,7 +689,7 @@ impl Write for &Stdout {
}
#[inline]
fn is_write_vectored(&self) -> bool {
self.lock().is_write_vectored()
stdout_raw().is_write_vectored()
}
fn flush(&mut self) -> io::Result<()> {
self.lock().flush()
Expand All @@ -712,26 +705,37 @@ impl Write for &Stdout {
}
}

impl StdoutLock<'_> {
#[inline]
fn with_inner<F, R>(&mut self, f: F) -> R
where
F: FnOnce(&mut LineWriter<StdoutRaw>) -> R,
{
let mut inner = self.inner.borrow_mut();
f(inner.get_or_insert_with(|| LineWriter::new(stdout_raw())))
}
}

#[stable(feature = "rust1", since = "1.0.0")]
impl Write for StdoutLock<'_> {
fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
self.inner.borrow_mut().write(buf)
self.with_inner(|stdout| stdout.write(buf))
}
fn write_vectored(&mut self, bufs: &[IoSlice<'_>]) -> io::Result<usize> {
self.inner.borrow_mut().write_vectored(bufs)
self.with_inner(|stdout| stdout.write_vectored(bufs))
}
#[inline]
fn is_write_vectored(&self) -> bool {
self.inner.borrow_mut().is_write_vectored()
stdout_raw().is_write_vectored()
}
fn flush(&mut self) -> io::Result<()> {
self.inner.borrow_mut().flush()
self.with_inner(|stdout| stdout.flush())
}
fn write_all(&mut self, buf: &[u8]) -> io::Result<()> {
self.inner.borrow_mut().write_all(buf)
self.with_inner(|stdout| stdout.write_all(buf))
}
fn write_all_vectored(&mut self, bufs: &mut [IoSlice<'_>]) -> io::Result<()> {
self.inner.borrow_mut().write_all_vectored(bufs)
self.with_inner(|stdout| stdout.write_all_vectored(bufs))
}
}

Expand Down Expand Up @@ -761,7 +765,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.
Expand Down Expand Up @@ -834,16 +838,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 {
Expand Down
124 changes: 46 additions & 78 deletions library/std/src/panicking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -71,20 +71,31 @@ 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)))
Self::Custom(Box::new(f))
}

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 {
fn default() -> Hook {
Hook::Default
}
}

static HOOK: RwLock<Hook> = RwLock::new(Hook::Default);

/// Registers a custom panic hook, replacing any that was previously registered.
///
Expand Down Expand Up @@ -125,24 +136,7 @@ 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);
}
}
}
*HOOK.write().unwrap_or_else(PoisonError::into_inner) = Hook::custom(hook);
}

/// Unregisters the current panic hook, returning it.
Expand Down Expand Up @@ -179,22 +173,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
Expand Down Expand Up @@ -240,24 +223,10 @@ 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 old_hook = mem::take(&mut *hook);
let prev = old_hook.into_box();
*hook = Hook::custom(move |info| hook_fn(&prev, info));
}

fn default_hook(info: &PanicInfo<'_>) {
Expand Down Expand Up @@ -682,27 +651,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
Expand Down
2 changes: 1 addition & 1 deletion library/std/src/sync/condvar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ mod tests;

use crate::fmt;
use crate::sync::{mutex, poison, LockResult, MutexGuard, PoisonError};
use crate::sys_common::condvar as sys;
use crate::sys::locks as sys;
use crate::time::{Duration, Instant};

/// A type indicating whether a timed wait on a condition variable returned
Expand Down
Loading