Skip to content

Commit

Permalink
Auto merge of #100603 - tmandry:zst-guards, r=dtolnay
Browse files Browse the repository at this point in the history
Optimize away poison guards when std is built with panic=abort

> **Note**: To take advantage of this PR, you will have to use `-Zbuild-std` or build your own toolchain. rustup toolchains always link to a libstd that was compiled with `panic=unwind`, since it's compatible with `panic=abort` code.

When std is compiled with `panic=abort` we can remove a lot of the poison machinery from the locks. This changes the `Flag` and `Guard` types to be ZSTs. It also adds an uninhabited member to `PoisonError` so the compiler knows it can optimize away the `Result::Err` paths, and make `LockResult<T>` layout-equivalent to `T`.

### Is this a breaking change?

`PoisonError::new` now panics if invoked from a libstd built with `panic="abort"` (or any non-`unwind` strategy). It is unclear to me whether to consider this a breaking change.

In order to encounter this behavior, **both of the following must be true**:

#### Using a libstd with `panic="abort"`

This is pretty uncommon. We don't build libstd with that in rustup, except in (Tier 2-3) platforms that do not support unwinding, **most notably wasm**.

Most people who do this are using cargo's `-Z build-std` feature, which is unstable.

`panic="abort"` is not a supported option in Rust's build system. It is possible to configure it using `CARGO_TARGET_xxx_RUSTFLAGS`, but I believe this only works on **non-host** platforms.

#### Creating `PoisonError` manually

This is also unlikely. The only common use case I can think of is in tests, and you can't run tests with `panic="abort"` without the unstable `-Z panic_abort_tests` flag.

It's possible that someone is implementing their own locks using std's `PoisonError` **and** defining "thread failure" to mean something other than "panic". If this is the case then we would break their code if it was used with a `panic="abort"` libstd. The locking crates I know of don't replicate std's poison API, but I haven't done much research into this yet.

I've touched on a fair number of considerations here. Which ones do people consider relevant?
  • Loading branch information
bors committed Feb 14, 2024
2 parents bb89df6 + 6b9289c commit 81b757c
Showing 1 changed file with 49 additions and 2 deletions.
51 changes: 49 additions & 2 deletions library/std/src/sync/poison.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
use crate::error::Error;
use crate::fmt;

#[cfg(panic = "unwind")]
use crate::sync::atomic::{AtomicBool, Ordering};
#[cfg(panic = "unwind")]
use crate::thread;

pub struct Flag {
#[cfg(panic = "unwind")]
failed: AtomicBool,
}

Expand All @@ -21,7 +25,10 @@ pub struct Flag {
impl Flag {
#[inline]
pub const fn new() -> Flag {
Flag { failed: AtomicBool::new(false) }
Flag {
#[cfg(panic = "unwind")]
failed: AtomicBool::new(false),
}
}

/// Check the flag for an unguarded borrow, where we only care about existing poison.
Expand All @@ -33,29 +40,46 @@ impl Flag {
/// Check the flag for a guarded borrow, where we may also set poison when `done`.
#[inline]
pub fn guard(&self) -> LockResult<Guard> {
let ret = Guard { panicking: thread::panicking() };
let ret = Guard {
#[cfg(panic = "unwind")]
panicking: thread::panicking(),
};
if self.get() { Err(PoisonError::new(ret)) } else { Ok(ret) }
}

#[inline]
#[cfg(panic = "unwind")]
pub fn done(&self, guard: &Guard) {
if !guard.panicking && thread::panicking() {
self.failed.store(true, Ordering::Relaxed);
}
}

#[inline]
#[cfg(not(panic = "unwind"))]
pub fn done(&self, _guard: &Guard) {}

#[inline]
#[cfg(panic = "unwind")]
pub fn get(&self) -> bool {
self.failed.load(Ordering::Relaxed)
}

#[inline(always)]
#[cfg(not(panic = "unwind"))]
pub fn get(&self) -> bool {
false
}

#[inline]
pub fn clear(&self) {
#[cfg(panic = "unwind")]
self.failed.store(false, Ordering::Relaxed)
}
}

pub struct Guard {
#[cfg(panic = "unwind")]
panicking: bool,
}

Expand Down Expand Up @@ -95,6 +119,8 @@ pub struct Guard {
#[stable(feature = "rust1", since = "1.0.0")]
pub struct PoisonError<T> {
guard: T,
#[cfg(not(panic = "unwind"))]
_never: !,
}

/// An enumeration of possible errors associated with a [`TryLockResult`] which
Expand Down Expand Up @@ -165,11 +191,27 @@ impl<T> PoisonError<T> {
///
/// This is generally created by methods like [`Mutex::lock`](crate::sync::Mutex::lock)
/// or [`RwLock::read`](crate::sync::RwLock::read).
///
/// This method may panic if std was built with `panic="abort"`.
#[cfg(panic = "unwind")]
#[stable(feature = "sync_poison", since = "1.2.0")]
pub fn new(guard: T) -> PoisonError<T> {
PoisonError { guard }
}

/// Creates a `PoisonError`.
///
/// This is generally created by methods like [`Mutex::lock`](crate::sync::Mutex::lock)
/// or [`RwLock::read`](crate::sync::RwLock::read).
///
/// This method may panic if std was built with `panic="abort"`.
#[cfg(not(panic = "unwind"))]
#[stable(feature = "sync_poison", since = "1.2.0")]
#[track_caller]
pub fn new(_guard: T) -> PoisonError<T> {
panic!("PoisonError created in a libstd built with panic=\"abort\"")
}

/// Consumes this error indicating that a lock is poisoned, returning the
/// underlying guard to allow access regardless.
///
Expand Down Expand Up @@ -225,6 +267,7 @@ impl<T> From<PoisonError<T>> for TryLockError<T> {
impl<T> fmt::Debug for TryLockError<T> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match *self {
#[cfg(panic = "unwind")]
TryLockError::Poisoned(..) => "Poisoned(..)".fmt(f),
TryLockError::WouldBlock => "WouldBlock".fmt(f),
}
Expand All @@ -235,6 +278,7 @@ impl<T> fmt::Debug for TryLockError<T> {
impl<T> fmt::Display for TryLockError<T> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match *self {
#[cfg(panic = "unwind")]
TryLockError::Poisoned(..) => "poisoned lock: another task failed inside",
TryLockError::WouldBlock => "try_lock failed because the operation would block",
}
Expand All @@ -247,6 +291,7 @@ impl<T> Error for TryLockError<T> {
#[allow(deprecated, deprecated_in_future)]
fn description(&self) -> &str {
match *self {
#[cfg(panic = "unwind")]
TryLockError::Poisoned(ref p) => p.description(),
TryLockError::WouldBlock => "try_lock failed because the operation would block",
}
Expand All @@ -255,6 +300,7 @@ impl<T> Error for TryLockError<T> {
#[allow(deprecated)]
fn cause(&self) -> Option<&dyn Error> {
match *self {
#[cfg(panic = "unwind")]
TryLockError::Poisoned(ref p) => Some(p),
_ => None,
}
Expand All @@ -267,6 +313,7 @@ where
{
match result {
Ok(t) => Ok(f(t)),
#[cfg(panic = "unwind")]
Err(PoisonError { guard }) => Err(PoisonError::new(f(guard))),
}
}

0 comments on commit 81b757c

Please sign in to comment.