Auto merge of #100603 - tmandry:zst-guards, r=dtolnay

bors · bors · commit 81b757c67048 · 2024-02-14T10:07:01.000Z
Optimize away poison guards when std is built with panic=abort

&gt; **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&lt;T&gt;` 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?
diff --git a/library/std/src/sync/poison.rs b/library/std/src/sync/poison.rs
@@ -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,
 }
 
@@ -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.
@@ -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,
 }
 
@@ -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
@@ -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.
     ///
@@ -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),
         }
@@ -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",
         }
@@ -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",
         }
@@ -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,
         }
@@ -267,6 +313,7 @@ where
 {
     match result {
         Ok(t) => Ok(f(t)),
+        #[cfg(panic = "unwind")]
         Err(PoisonError { guard }) => Err(PoisonError::new(f(guard))),
     }
 }
