From 82bc685297f914d056596f2f0b0be0bfde96de3f Mon Sep 17 00:00:00 2001 From: Jethro Beekman Date: Thu, 6 Aug 2026 12:00:08 +0000 Subject: [PATCH 01/12] Fix unsoundness issues in std::sys::pal::sgx::waitqueue::unsafe_list --- library/std/src/sys/pal/sgx/waitqueue/mod.rs | 21 ++ .../src/sys/pal/sgx/waitqueue/unsafe_list.rs | 192 ++++++++++++++---- 2 files changed, 172 insertions(+), 41 deletions(-) diff --git a/library/std/src/sys/pal/sgx/waitqueue/mod.rs b/library/std/src/sys/pal/sgx/waitqueue/mod.rs index 41d1413fcdee9..14cb2f6f67ec3 100644 --- a/library/std/src/sys/pal/sgx/waitqueue/mod.rs +++ b/library/std/src/sys/pal/sgx/waitqueue/mod.rs @@ -79,6 +79,27 @@ pub struct WaitGuard<'a, T: 'a> { /// safe because the waiting thread will not return from that stack frame until /// after it is notified. The notifying thread ensures to clean up any /// references to the list entries before sending the wakeup event. +// The safety requirements of `UnsafeList` are upheld as follows: +// +// * All list operations are performed while holding the lock of the +// `SpinMutex` around the `WaitVariable` containing the list. +// * A waiting thread pushes a stack-allocated entry and does not invalidate +// it while it is in the list: it only accesses the entry through the +// reference `push` returned, reading `wake` under the `WaitEntry`'s own +// `SpinMutex`. +// * `push` -> `pop`: a notifying thread pops the entry and sets `wake` under +// the `WaitEntry`'s `SpinMutex`; when that mutex is released, the thread +// will no longer access the entry (guaranteed by the mutex guard). The +// waiting thread only returns from the stack frame containing the entry +// once it observes `wake == true` under that same mutex, so the entry is +// only deallocated after the notifying thread's last access to it. +// * `push` -> `remove`: on a timeout, `wait_timeout` re-acquires the queue +// lock and checks `wake`: the entry is still in the list if and only if +// `wake` is not set, because notifying threads always `pop` an entry +// before setting its `wake`. Only if the entry is still in the list is it +// removed. +// * Besides as described, no other exclusive references to the entry are +// taken. pub struct WaitQueue { // We use an inner Mutex here to protect the data in the face of spurious // wakeups. diff --git a/library/std/src/sys/pal/sgx/waitqueue/unsafe_list.rs b/library/std/src/sys/pal/sgx/waitqueue/unsafe_list.rs index c736cab576e4d..e6b46f2cd07af 100644 --- a/library/std/src/sys/pal/sgx/waitqueue/unsafe_list.rs +++ b/library/std/src/sys/pal/sgx/waitqueue/unsafe_list.rs @@ -1,12 +1,78 @@ //! A doubly-linked list where callers are in charge of memory allocation //! of the nodes in the list. +//! +//! # Safety +//! +//! `UnsafeList` itself does not synchronize any of its memory accesses, so +//! callers must serialize all operations on a list, e.g. with a lock. +//! +//! While an entry passed to `push` is in the list, it must not be invalidated, +//! with one exception explained below. Invalidation of the entry, by creating a new +//! exclusive reference to it, would invalidate the pointers to the entry stored +//! in the list. The entry goes through one of two flows (see also each operation's +//! safety documentation): +//! +//! * `push` -> `pop`, usually with `pop` on another thread: the entry pointer +//! stored in the list keeps its `push`-time provenance. As mentioned, for it to +//! still be valid to dereference in `pop`, the pushing caller must not access +//! the entry in between. After `pop`, the references into `value` returned by +//! `push` and `pop` are held concurrently, possibly by two threads. This is +//! valid as they are shared references, but mutating `value` requires interior +//! mutability and synchronization. That synchronization must also ensure the +//! entry is only deallocated after the popping thread's last access to it. +//! * `push` -> `remove`, on the thread that pushed: the caller reclaims a pushed +//! entry by passing a reference to the entry to `remove`. The entry must still +//! be in the list. The caller of `remove` must create a new exclusive reference +//! to the entry, which invalidates the pointers to the entry stored in the list. +//! This is fine in this case because `remove` only overwrites those pointers, +//! and never dereferences them. + +// # Aliasing +// +// The list is self-referential: it stores pointers to its own `head_tail` +// field in the list entries' links. `UnsafePinned` is used to ensure pointer +// validity. +// +// Pointers to the other entries are derived from the exclusive reference passed +// to `push` and stay valid while the entry is in the list (see the safety +// requirements in the module documentation). Multiple immutable references may +// exist to values of entries in the list, while the links in the list may be +// mutated simultaneously. Creating mutable references to entries to update the +// links would invalidate any outstanding shared references. As such, all links +// are updated via raw-pointer place expressions instead, keeping the value +// references valid. +// +// # Pointer dereferencing +// +// All pointers stored in the list are valid to dereference: +// +// 1. The head/tail pointer is derived from `head_tail`'s `UnsafePinned` +// wherever it is needed. Because of the `UnsafePinned` wrapper, no +// exclusive reference to the list (or a structure containing it) makes an +// aliasing claim on `head_tail`, so every derived pointer and every copy +// of it stored in the links stay valid for the list's lifetime. +// 2. Pointers to other entries, stored in the links, are derived from the +// exclusive reference passed to `push` and stay valid while the entry is +// in the list, as ensured by the safety requirements in the module +// documentation. +// +// Both points rely on this code never creating references to entries, as +// those would make their own aliasing claims on the entries. #[cfg(test)] mod tests; -use crate::mem; -use crate::ptr::NonNull; +use crate::pin::UnsafePinned; +use crate::ptr::{self, NonNull}; +/// A caller-allocated list entry. +/// +/// While the entry is in a list, the list holds a pointer derived from the +/// exclusive reference passed to `UnsafeList::push`, so the caller must not +/// access the entry until it is removed from the list. `UnsafeList::push` +/// returns a reference borrowing the entry, and `UnsafeList::remove` +/// reborrows it exclusively, so the borrow checker enforces this for safe +/// accesses. pub struct UnsafeListEntry { next: NonNull>, prev: NonNull>, @@ -14,7 +80,7 @@ pub struct UnsafeListEntry { } impl UnsafeListEntry { - fn dummy() -> Self { + const fn dummy() -> Self { UnsafeListEntry { next: NonNull::dangling(), prev: NonNull::dangling(), value: None } } @@ -25,37 +91,55 @@ impl UnsafeListEntry { // WARNING: self-referential struct! pub struct UnsafeList { - head_tail: NonNull>, - head_tail_entry: Option>, + // UnsafePinned isn't required to implement this code, but it makes it a lot + // simpler. Without UnsafePinned, the provenance of each entry link pointer + // would need to be re-established prior to dereferencing, whenever it points + // to `head_tail`. + head_tail: UnsafePinned>, + init: bool, } impl UnsafeList { pub const fn new() -> Self { - unsafe { UnsafeList { head_tail: NonNull::new_unchecked(1 as _), head_tail_entry: None } } + UnsafeList { head_tail: UnsafePinned::new(UnsafeListEntry::dummy()), init: false } + } + + fn head_tail(&mut self) -> NonNull> { + // SAFETY: `get_mut_unchecked` returns the address of `head_tail`, + // which is non-null. + unsafe { NonNull::new_unchecked(self.head_tail.get_mut_unchecked()) } } /// # Safety + /// + /// The caller must ensure the list is never moved after this call: the + /// list becomes self-referential. unsafe fn init(&mut self) { - if self.head_tail_entry.is_none() { - self.head_tail_entry = Some(UnsafeListEntry::dummy()); - // SAFETY: `head_tail_entry` must be non-null, which it is because we assign it above. - self.head_tail = - unsafe { NonNull::new_unchecked(self.head_tail_entry.as_mut().unwrap()) }; - // SAFETY: `self.head_tail` must meet all requirements for a mutable reference. - unsafe { self.head_tail.as_mut() }.next = self.head_tail; - unsafe { self.head_tail.as_mut() }.prev = self.head_tail; + if !self.init { + let head_tail = self.head_tail(); + // SAFETY: `head_tail` is valid to dereference (see point 1 of the + // `Pointer dereferencing` explanation at the top of the file). + unsafe { (*head_tail.as_ptr()).next = head_tail }; + unsafe { (*head_tail.as_ptr()).prev = head_tail }; + self.init = true; } } pub fn is_empty(&self) -> bool { - if self.head_tail_entry.is_some() { - let first = unsafe { self.head_tail.as_ref() }.next; - if first == self.head_tail { + if self.init { + // SAFETY: `get` returns the address of `head_tail`, which is + // non-null. + let head_tail = unsafe { NonNull::new_unchecked(self.head_tail.get()) }; + // SAFETY: `head_tail` is valid to dereference (see point 1 + // of the `Pointer dereferencing` explanation at the top of the + // file). + let first = unsafe { (*head_tail.as_ptr()).next }; + if first == head_tail { // ,-------> /---------\ next ---, // | |head_tail| | // `--- prev \---------/ <-------` - // SAFETY: `self.head_tail` must meet all requirements for a reference. - unsafe { rtassert!(self.head_tail.as_ref().prev == first) }; + // SAFETY: `head_tail` is valid to dereference. + unsafe { rtassert!((*head_tail.as_ptr()).prev == first) }; true } else { false @@ -72,7 +156,9 @@ impl UnsafeList { /// The entry must remain allocated until the entry is removed from the /// list AND the caller who popped is done using the entry. Special /// care must be taken in the caller of `push` to ensure unwinding does - /// not destroy the stack frame containing the entry. + /// not destroy the stack frame containing the entry. While the entry is + /// in the list, it must not be accessed except through the reference + /// returned here or by passing the entry to `remove`. pub unsafe fn push<'a>(&mut self, entry: &'a mut UnsafeListEntry) -> &'a T { unsafe { self.init() }; @@ -85,13 +171,20 @@ impl UnsafeList { // /---------\ next ---> /-----\ next ---> /---------\ // ... |prev_tail| |entry| |head_tail| ... // \---------/ <--- prev \-----/ <--- prev \---------/ - let mut entry = unsafe { NonNull::new_unchecked(entry) }; - let mut prev_tail = mem::replace(&mut unsafe { self.head_tail.as_mut() }.prev, entry); - // SAFETY: `entry` must meet all requirements for a mutable reference. - unsafe { entry.as_mut() }.prev = prev_tail; - unsafe { entry.as_mut() }.next = self.head_tail; - // SAFETY: `prev_tail` must meet all requirements for a mutable reference. - unsafe { prev_tail.as_mut() }.next = entry; + let entry = unsafe { NonNull::new_unchecked(entry) }; + let head_tail = self.head_tail(); + // SAFETY: `head_tail` is valid to dereference (see point 1 + // of the `Pointer dereferencing` explanation at the top of the + // file). + let prev_tail = unsafe { ptr::replace(&raw mut (*head_tail.as_ptr()).prev, entry) }; + // SAFETY: `entry` is valid to dereference: it was derived from an + // exclusive reference above. + unsafe { (*entry.as_ptr()).prev = prev_tail }; + unsafe { (*entry.as_ptr()).next = head_tail }; + // SAFETY: `prev_tail` was loaded from the list's links, so it is + // valid to dereference (see points 1 and 2 of the + // `Pointer dereferencing` explanation at the top of the file). + unsafe { (*prev_tail.as_ptr()).next = entry }; // unwrap ok: always `Some` on non-dummy entries unsafe { (*entry.as_ptr()).value.as_ref() }.unwrap() } @@ -103,8 +196,6 @@ impl UnsafeList { /// The caller must make sure to synchronize ending the borrow of the /// return value and deallocation of the containing entry. pub unsafe fn pop<'a>(&mut self) -> Option<&'a T> { - unsafe { self.init() }; - if self.is_empty() { None } else { @@ -117,12 +208,23 @@ impl UnsafeList { // /---------\ next ---> /------\ // ... |head_tail| |second| ... // \---------/ <--- prev \------/ - let mut first = unsafe { self.head_tail.as_mut() }.next; - let mut second = unsafe { first.as_mut() }.next; - unsafe { self.head_tail.as_mut() }.next = second; - unsafe { second.as_mut() }.prev = self.head_tail; - unsafe { first.as_mut() }.next = NonNull::dangling(); - unsafe { first.as_mut() }.prev = NonNull::dangling(); + + let head_tail = self.head_tail(); + // SAFETY: `head_tail` is valid to dereference (see point 1 + // of the `Pointer dereferencing` explanation at the top of the + // file). + let first = unsafe { (*head_tail.as_ptr()).next }; + // SAFETY: `first` was loaded from the list's links, so it is + // valid to dereference (see point 2 of the + // `Pointer dereferencing` explanation at the top of the file). + let second = unsafe { (*first.as_ptr()).next }; + unsafe { (*head_tail.as_ptr()).next = second }; + // SAFETY: `second` was loaded from the list's links, so it is + // valid to dereference (see points 1 and 2 of the + // `Pointer dereferencing` explanation at the top of the file). + unsafe { (*second.as_ptr()).prev = head_tail }; + unsafe { (*first.as_ptr()).next = NonNull::dangling() }; + unsafe { (*first.as_ptr()).prev = NonNull::dangling() }; // unwrap ok: always `Some` on non-dummy entries Some(unsafe { (*first.as_ptr()).value.as_ref() }.unwrap()) } @@ -133,7 +235,8 @@ impl UnsafeList { /// # Safety /// /// The caller must ensure that `entry` has been pushed onto `self` - /// prior to this call and has not moved since then. + /// prior to this call, has not been removed from the list since then + /// (by `pop` or `remove`), and has not moved since it was pushed. pub unsafe fn remove(&mut self, entry: &mut UnsafeListEntry) { rtassert!(!self.is_empty()); // BEFORE: @@ -145,11 +248,18 @@ impl UnsafeList { // /----\ next ---> /----\ // ... |prev| |next| ... // \----/ <--- prev \----/ - let mut prev = entry.prev; - let mut next = entry.next; - // SAFETY: `prev` and `next` must meet all requirements for a mutable reference.entry - unsafe { prev.as_mut() }.next = next; - unsafe { next.as_mut() }.prev = prev; + + // The exclusive reference `entry`, created by the caller, has + // invalidated the pointers to `entry` stored in its neighbors (see + // the module documentation); those are only overwritten below, + // never dereferenced. + let prev = entry.prev; + let next = entry.next; + // SAFETY: `prev` and `next` were loaded from `entry`'s links, so + // they are valid to dereference (see points 1 and 2 of the + // `Pointer dereferencing` explanation at the top of the file). + unsafe { (*prev.as_ptr()).next = next }; + unsafe { (*next.as_ptr()).prev = prev }; entry.next = NonNull::dangling(); entry.prev = NonNull::dangling(); } From 994ae4e6a331a73d837f3059b2d53283c7e24d3d Mon Sep 17 00:00:00 2001 From: Jethro Beekman Date: Thu, 6 Aug 2026 17:51:58 +0000 Subject: [PATCH 02/12] Implementing pinning in std::sys::pal::sgx::waitqueue::unsafe_list --- library/std/src/sys/pal/sgx/waitqueue/mod.rs | 80 ++++++----- .../src/sys/pal/sgx/waitqueue/spin_mutex.rs | 17 ++- .../std/src/sys/pal/sgx/waitqueue/tests.rs | 6 +- .../src/sys/pal/sgx/waitqueue/unsafe_list.rs | 131 ++++++++++++------ .../pal/sgx/waitqueue/unsafe_list/tests.rs | 101 ++++++++------ library/std/src/sys/sync/condvar/sgx.rs | 11 +- library/std/src/sys/sync/mutex/sgx.rs | 19 +-- 7 files changed, 225 insertions(+), 140 deletions(-) diff --git a/library/std/src/sys/pal/sgx/waitqueue/mod.rs b/library/std/src/sys/pal/sgx/waitqueue/mod.rs index 14cb2f6f67ec3..76c6ae62be796 100644 --- a/library/std/src/sys/pal/sgx/waitqueue/mod.rs +++ b/library/std/src/sys/pal/sgx/waitqueue/mod.rs @@ -18,12 +18,13 @@ mod unsafe_list; use fortanix_sgx_abi::{EV_UNPARK, Tcs, WAIT_INDEFINITE}; -pub use self::spin_mutex::{SpinMutex, SpinMutexGuard, try_lock_or_false}; +pub use self::spin_mutex::{SpinMutex, SpinMutexGuard}; use self::unsafe_list::{UnsafeList, UnsafeListEntry}; use super::abi::{thread, usercalls}; use crate::num::NonZero; use crate::ops::{Deref, DerefMut}; use crate::panic::{self, AssertUnwindSafe}; +use crate::pin::Pin; use crate::time::Duration; /// An queue entry in a `WaitQueue`. @@ -38,24 +39,28 @@ struct WaitEntry { /// queue and the data are synchronized, since the type itself is not `Sync`. /// /// Consumers of this API should use a synchronization primitive for shared -/// access, such as `SpinMutex`. -#[derive(Default)] +/// access. `WaitVariable::new` is the only constructor and provides that +/// with `SpinMutex`. pub struct WaitVariable { queue: WaitQueue, lock: T, } impl WaitVariable { - pub const fn new(var: T) -> Self { - WaitVariable { queue: WaitQueue::new(), lock: var } - } - pub fn lock_var(&self) -> &T { &self.lock } - pub fn lock_var_mut(&mut self) -> &mut T { - &mut self.lock + pub fn lock_var_mut(self: Pin<&mut Self>) -> &mut T { + // SAFETY: `lock` is not structurally pinned: a pinned `WaitVariable` + // makes no promise that `T` is pinned. + unsafe { &mut self.get_unchecked_mut().lock } + } + + fn queue(self: Pin<&mut Self>) -> Pin<&mut WaitQueue> { + // SAFETY: `queue` is structurally pinned: a pinned `WaitVariable` + // pins it, and it is never moved out of it. + unsafe { self.map_unchecked_mut(|this| &mut this.queue) } } } @@ -68,7 +73,7 @@ pub enum NotifiedTcs { /// An RAII guard that will notify a set of target threads as well as unlock /// a mutex on drop. pub struct WaitGuard<'a, T: 'a> { - mutex_guard: Option>>, + mutex_guard: Option>>>, notified_tcs: NotifiedTcs, } @@ -107,14 +112,8 @@ pub struct WaitQueue { } unsafe impl Send for WaitQueue {} -impl Default for WaitQueue { - fn default() -> Self { - Self::new() - } -} - impl<'a, T> Deref for WaitGuard<'a, T> { - type Target = SpinMutexGuard<'a, WaitVariable>; + type Target = Pin>>; fn deref(&self) -> &Self::Target { self.mutex_guard.as_ref().unwrap() @@ -139,8 +138,24 @@ impl<'a, T> Drop for WaitGuard<'a, T> { } impl WaitQueue { - pub const fn new() -> Self { - WaitQueue { inner: UnsafeList::new() } + /// Creates a new queue. + /// + /// # Safety + /// + /// The caller must initialize the queue's list (`UnsafeList::init`) + /// before any other use of the queue, including dropping it. + /// `WaitVariable::new`, the sole constructor of the containing + /// structure, does this. + pub const unsafe fn new() -> Self { + // SAFETY: the caller upholds `UnsafeList::new`'s contract (see this + // function's safety requirements). + WaitQueue { inner: unsafe { UnsafeList::new() } } + } + + fn inner(self: Pin<&mut Self>) -> Pin<&mut UnsafeList>> { + // SAFETY: `inner` is structurally pinned: a pinned `WaitQueue` pins + // it, and it is never moved out of it. + unsafe { self.map_unchecked_mut(|this| &mut this.inner) } } /// Adds the calling thread to the `WaitVariable`'s wait queue, then wait @@ -148,14 +163,17 @@ impl WaitQueue { /// /// This function does not return until this thread has been awoken. When `before_wait` panics, /// this function will abort. - pub fn wait(mut guard: SpinMutexGuard<'_, WaitVariable>, before_wait: F) { + pub fn wait( + mut guard: Pin>>, + before_wait: F, + ) { // very unsafe: check requirements of UnsafeList::push unsafe { let mut entry = UnsafeListEntry::new(SpinMutex::new(WaitEntry { tcs: thread::current(), wake: false, })); - let entry = guard.queue.inner.push(&mut entry); + let entry = guard.as_mut().queue().inner().push(&mut entry); drop(guard); if let Err(_e) = panic::catch_unwind(AssertUnwindSafe(|| before_wait())) { rtabort!("Panic before wait on wakeup event") @@ -176,7 +194,7 @@ impl WaitQueue { /// If not, it will remove the calling thread from the wait queue. /// When `before_wait` panics, this function will abort. pub fn wait_timeout( - lock: &SpinMutex>, + lock: Pin<&SpinMutex>>, timeout: Duration, before_wait: F, ) -> bool { @@ -186,7 +204,7 @@ impl WaitQueue { tcs: thread::current(), wake: false, })); - let entry_lock = lock.lock().queue.inner.push(&mut entry); + let entry_lock = lock.lock_pinned().as_mut().queue().inner().push(&mut entry); if let Err(_e) = panic::catch_unwind(AssertUnwindSafe(|| before_wait())) { rtabort!("Panic before wait on wakeup event or timeout") } @@ -194,11 +212,11 @@ impl WaitQueue { // acquire the wait queue's lock first to avoid deadlock // and ensure no other function can simultaneously access the list // (e.g., `notify_one` or `notify_all`) - let mut guard = lock.lock(); + let mut guard = lock.lock_pinned(); let success = entry_lock.lock().wake; if !success { // nobody is waking us up, so remove our entry from the wait queue. - guard.queue.inner.remove(&mut entry); + guard.as_mut().queue().inner().remove(&mut entry); } success } @@ -210,14 +228,14 @@ impl WaitQueue { /// If a waiter is found, a `WaitGuard` is returned which will notify the /// waiter when it is dropped. pub fn notify_one( - mut guard: SpinMutexGuard<'_, WaitVariable>, - ) -> Result, SpinMutexGuard<'_, WaitVariable>> { + mut guard: Pin>>, + ) -> Result, Pin>>> { // SAFETY: lifetime of the pop() return value is limited to the map // closure (The closure return value is 'static). The underlying // stack frame won't be freed until after the lock on the queue is released // (i.e., `guard` is dropped). unsafe { - let tcs = guard.queue.inner.pop().map(|entry| -> Tcs { + let tcs = guard.as_mut().queue().inner().pop().map(|entry| -> Tcs { let mut entry_guard = entry.lock(); entry_guard.wake = true; entry_guard.tcs @@ -237,14 +255,14 @@ impl WaitQueue { /// If at least one waiter is found, a `WaitGuard` is returned which will /// notify all waiters when it is dropped. pub fn notify_all( - mut guard: SpinMutexGuard<'_, WaitVariable>, - ) -> Result, SpinMutexGuard<'_, WaitVariable>> { + mut guard: Pin>>, + ) -> Result, Pin>>> { // SAFETY: lifetime of the pop() return values are limited to the // while loop body. The underlying stack frames won't be freed until // after the lock on the queue is released (i.e., `guard` is dropped). unsafe { let mut count = 0; - while let Some(entry) = guard.queue.inner.pop() { + while let Some(entry) = guard.as_mut().queue().inner().pop() { count += 1; let mut entry_guard = entry.lock(); entry_guard.wake = true; diff --git a/library/std/src/sys/pal/sgx/waitqueue/spin_mutex.rs b/library/std/src/sys/pal/sgx/waitqueue/spin_mutex.rs index 73c7a101d601d..f052c73115015 100644 --- a/library/std/src/sys/pal/sgx/waitqueue/spin_mutex.rs +++ b/library/std/src/sys/pal/sgx/waitqueue/spin_mutex.rs @@ -7,6 +7,7 @@ mod tests; use crate::cell::UnsafeCell; use crate::hint; use crate::ops::{Deref, DerefMut}; +use crate::pin::Pin; use crate::sync::atomic::{Atomic, AtomicBool, Ordering}; #[derive(Default)] @@ -52,11 +53,19 @@ impl SpinMutex { None } } -} -/// Lock the Mutex or return false. -pub macro try_lock_or_false($e:expr) { - if let Some(v) = $e.try_lock() { v } else { return false } + #[inline(always)] + pub fn lock_pinned(self: Pin<&Self>) -> Pin> { + // SAFETY: `value` is structurally pinned: a pinned mutex pins its + // contents, and `SpinMutexGuard` never moves the value. + unsafe { Pin::new_unchecked(self.get_ref().lock()) } + } + + #[inline(always)] + pub fn try_lock_pinned(self: Pin<&Self>) -> Option>> { + // SAFETY: see `lock_pinned` + self.get_ref().try_lock().map(|guard| unsafe { Pin::new_unchecked(guard) }) + } } impl<'a, T> Deref for SpinMutexGuard<'a, T> { diff --git a/library/std/src/sys/pal/sgx/waitqueue/tests.rs b/library/std/src/sys/pal/sgx/waitqueue/tests.rs index bf91fdd08ed54..05ade6b0b5d17 100644 --- a/library/std/src/sys/pal/sgx/waitqueue/tests.rs +++ b/library/std/src/sys/pal/sgx/waitqueue/tests.rs @@ -4,14 +4,14 @@ use crate::thread; #[test] fn queue() { - let wq = Arc::new(SpinMutex::>::default()); + let wq = Arc::new(WaitVariable::new(())); let wq2 = wq.clone(); - let locked = wq.lock(); + let locked = (*wq).as_ref().lock_pinned(); let t1 = thread::spawn(move || { // if we obtain the lock, the main thread should be waiting - assert!(WaitQueue::notify_one(wq2.lock()).is_ok()); + assert!(WaitQueue::notify_one((*wq2).as_ref().lock_pinned()).is_ok()); }); WaitQueue::wait(locked, || {}); diff --git a/library/std/src/sys/pal/sgx/waitqueue/unsafe_list.rs b/library/std/src/sys/pal/sgx/waitqueue/unsafe_list.rs index e6b46f2cd07af..d5e9904cbee3e 100644 --- a/library/std/src/sys/pal/sgx/waitqueue/unsafe_list.rs +++ b/library/std/src/sys/pal/sgx/waitqueue/unsafe_list.rs @@ -58,11 +58,21 @@ // // Both points rely on this code never creating references to entries, as // those would make their own aliasing claims on the entries. +// +// # Pinning +// +// Once initialized, the list is self-referential, so it must not be moved. +// `UnsafeList` is `!Unpin` and the operations take `Pin<&mut Self>`, letting +// the compiler enforce this. Dropping the list while entries are still +// linked would leave those entries dangling; `Drop` checks this, and `Pin`'s +// drop guarantee ensures every path that invalidates the list's storage +// (including in-place replacement with `Pin::set`) runs the check. #[cfg(test)] mod tests; -use crate::pin::UnsafePinned; +use super::{SpinMutex, WaitQueue, WaitVariable}; +use crate::pin::{Pin, UnsafePinned}; use crate::ptr::{self, NonNull}; /// A caller-allocated list entry. @@ -89,19 +99,31 @@ impl UnsafeListEntry { } } -// WARNING: self-referential struct! +// WARNING: self-referential struct! Must not be moved once initialized, see +// the `Pinning` explanation at the top of the file. pub struct UnsafeList { // UnsafePinned isn't required to implement this code, but it makes it a lot // simpler. Without UnsafePinned, the provenance of each entry link pointer // would need to be re-established prior to dereferencing, whenever it points // to `head_tail`. head_tail: UnsafePinned>, - init: bool, } impl UnsafeList { - pub const fn new() -> Self { - UnsafeList { head_tail: UnsafePinned::new(UnsafeListEntry::dummy()), init: false } + /// Creates a new list. + /// + /// Before use, the list must be placed in its final location and + /// initialized with `init`, making it self-referential; from then on + /// it must not be moved and can only be operated on through + /// `Pin<&mut Self>` (see the `Pinning` explanation at the top of the + /// file). `WaitVariable::new` performs this sequence. + /// + /// # Safety + /// + /// The caller must initialize the list with `init` before any other use, + /// including dropping it. + pub(super) const unsafe fn new() -> Self { + UnsafeList { head_tail: UnsafePinned::new(UnsafeListEntry::dummy()) } } fn head_tail(&mut self) -> NonNull> { @@ -110,42 +132,35 @@ impl UnsafeList { unsafe { NonNull::new_unchecked(self.head_tail.get_mut_unchecked()) } } - /// # Safety - /// - /// The caller must ensure the list is never moved after this call: the - /// list becomes self-referential. - unsafe fn init(&mut self) { - if !self.init { - let head_tail = self.head_tail(); - // SAFETY: `head_tail` is valid to dereference (see point 1 of the - // `Pointer dereferencing` explanation at the top of the file). - unsafe { (*head_tail.as_ptr()).next = head_tail }; - unsafe { (*head_tail.as_ptr()).prev = head_tail }; - self.init = true; - } + /// Makes the list self-referential: the list must be in its final + /// location and must never be moved afterwards. Called exactly once per + /// list, during construction (`WaitVariable::new`), so lists + /// are always initialized before use. + fn init(&mut self) { + let head_tail = self.head_tail(); + // SAFETY: `head_tail` is valid to dereference (see point 1 of the + // `Pointer dereferencing` explanation at the top of the file). + unsafe { (*head_tail.as_ptr()).next = head_tail }; + unsafe { (*head_tail.as_ptr()).prev = head_tail }; } pub fn is_empty(&self) -> bool { - if self.init { - // SAFETY: `get` returns the address of `head_tail`, which is - // non-null. - let head_tail = unsafe { NonNull::new_unchecked(self.head_tail.get()) }; - // SAFETY: `head_tail` is valid to dereference (see point 1 - // of the `Pointer dereferencing` explanation at the top of the - // file). - let first = unsafe { (*head_tail.as_ptr()).next }; - if first == head_tail { - // ,-------> /---------\ next ---, - // | |head_tail| | - // `--- prev \---------/ <-------` - // SAFETY: `head_tail` is valid to dereference. - unsafe { rtassert!((*head_tail.as_ptr()).prev == first) }; - true - } else { - false - } - } else { + // SAFETY: `get` returns the address of `head_tail`, which is + // non-null. + let head_tail = unsafe { NonNull::new_unchecked(self.head_tail.get()) }; + // SAFETY: `head_tail` is valid to dereference (see point 1 + // of the `Pointer dereferencing` explanation at the top of the + // file). + let first = unsafe { (*head_tail.as_ptr()).next }; + if first == head_tail { + // ,-------> /---------\ next ---, + // | |head_tail| | + // `--- prev \---------/ <-------` + // SAFETY: `head_tail` is valid to dereference. + unsafe { rtassert!((*head_tail.as_ptr()).prev == first) }; true + } else { + false } } @@ -159,8 +174,9 @@ impl UnsafeList { /// not destroy the stack frame containing the entry. While the entry is /// in the list, it must not be accessed except through the reference /// returned here or by passing the entry to `remove`. - pub unsafe fn push<'a>(&mut self, entry: &'a mut UnsafeListEntry) -> &'a T { - unsafe { self.init() }; + pub unsafe fn push<'a>(self: Pin<&mut Self>, entry: &'a mut UnsafeListEntry) -> &'a T { + // SAFETY: the list is not moved out of the pinned reference. + let this = unsafe { self.get_unchecked_mut() }; // BEFORE: // /---------\ next ---> /---------\ @@ -172,7 +188,7 @@ impl UnsafeList { // ... |prev_tail| |entry| |head_tail| ... // \---------/ <--- prev \-----/ <--- prev \---------/ let entry = unsafe { NonNull::new_unchecked(entry) }; - let head_tail = self.head_tail(); + let head_tail = this.head_tail(); // SAFETY: `head_tail` is valid to dereference (see point 1 // of the `Pointer dereferencing` explanation at the top of the // file). @@ -195,10 +211,13 @@ impl UnsafeList { /// /// The caller must make sure to synchronize ending the borrow of the /// return value and deallocation of the containing entry. - pub unsafe fn pop<'a>(&mut self) -> Option<&'a T> { + pub unsafe fn pop<'a>(self: Pin<&mut Self>) -> Option<&'a T> { if self.is_empty() { None } else { + // SAFETY: the list is not moved out of the pinned reference. + let this = unsafe { self.get_unchecked_mut() }; + // BEFORE: // /---------\ next ---> /-----\ next ---> /------\ // ... |head_tail| |first| |second| ... @@ -209,7 +228,7 @@ impl UnsafeList { // ... |head_tail| |second| ... // \---------/ <--- prev \------/ - let head_tail = self.head_tail(); + let head_tail = this.head_tail(); // SAFETY: `head_tail` is valid to dereference (see point 1 // of the `Pointer dereferencing` explanation at the top of the // file). @@ -237,8 +256,9 @@ impl UnsafeList { /// The caller must ensure that `entry` has been pushed onto `self` /// prior to this call, has not been removed from the list since then /// (by `pop` or `remove`), and has not moved since it was pushed. - pub unsafe fn remove(&mut self, entry: &mut UnsafeListEntry) { + pub unsafe fn remove(self: Pin<&mut Self>, entry: &mut UnsafeListEntry) { rtassert!(!self.is_empty()); + // BEFORE: // /----\ next ---> /-----\ next ---> /----\ // ... |prev| |entry| |next| ... @@ -264,3 +284,28 @@ impl UnsafeList { entry.prev = NonNull::dangling(); } } + +impl Drop for UnsafeList { + fn drop(&mut self) { + // A non-empty list would leave its entries with dangling links. + // `Pin`'s drop guarantee routes every path that invalidates the + // list's storage (including in-place replacement via `Pin::set`) + // through this check. + rtassert!(self.is_empty()); + } +} + +impl super::WaitVariable { + /// Creates a mutex-protected `WaitVariable` on the heap, with its queue's + /// list initialized. Initialization makes the list self-referential and + /// happens before pinning: only the `Box` pointer is moved into the + /// `Pin`, the heap allocation itself never moves. + pub fn new(value: T) -> Pin>>> { + // SAFETY: `init` is called below, before the queue is otherwise used + // or dropped. + let queue = unsafe { WaitQueue::new() }; + let result = Box::new(SpinMutex::new(WaitVariable { queue, lock: value })); + result.lock().queue.inner.init(); + Box::into_pin(result) + } +} diff --git a/library/std/src/sys/pal/sgx/waitqueue/unsafe_list/tests.rs b/library/std/src/sys/pal/sgx/waitqueue/unsafe_list/tests.rs index c653dee17bc36..6d2a0322d63bb 100644 --- a/library/std/src/sys/pal/sgx/waitqueue/unsafe_list/tests.rs +++ b/library/std/src/sys/pal/sgx/waitqueue/unsafe_list/tests.rs @@ -1,16 +1,27 @@ use super::*; use crate::cell::Cell; +use crate::pin::Pin; + +/// All lists are constructed by `WaitVariable::new`; this test +/// stand-in likewise initializes the list before pinning it. +fn new_list() -> Pin>> { + // SAFETY: `init` is called below, before the list is otherwise used or + // dropped. + let mut list = Box::new(unsafe { UnsafeList::new() }); + list.init(); + Box::into_pin(list) +} /// # Safety /// List must be valid. -unsafe fn assert_empty(list: &mut UnsafeList) { +unsafe fn assert_empty(list: Pin<&mut UnsafeList>) { assert!(unsafe { list.pop() }.is_none(), "assertion failed: list is not empty"); } #[test] fn init_empty() { unsafe { - assert_empty(&mut UnsafeList::::new()); + assert_empty(new_list::().as_mut()); } } @@ -18,10 +29,10 @@ fn init_empty() { fn push_pop() { unsafe { let mut node = UnsafeListEntry::new(1234); - let mut list = UnsafeList::new(); - assert_eq!(list.push(&mut node), &1234); - assert_eq!(list.pop().unwrap(), &1234); - assert_empty(&mut list); + let mut list = new_list(); + assert_eq!(list.as_mut().push(&mut node), &1234); + assert_eq!(list.as_mut().pop().unwrap(), &1234); + assert_empty(list.as_mut()); } } @@ -29,10 +40,10 @@ fn push_pop() { fn push_remove() { unsafe { let mut node = UnsafeListEntry::new(1234); - let mut list = UnsafeList::new(); - assert_eq!(list.push(&mut node), &1234); - list.remove(&mut node); - assert_empty(&mut list); + let mut list = new_list(); + assert_eq!(list.as_mut().push(&mut node), &1234); + list.as_mut().remove(&mut node); + assert_empty(list.as_mut()); } } @@ -44,29 +55,29 @@ fn push_remove_pop() { let mut node3 = UnsafeListEntry::new(13); let mut node4 = UnsafeListEntry::new(14); let mut node5 = UnsafeListEntry::new(15); - let mut list = UnsafeList::new(); - assert_eq!(list.push(&mut node1), &11); - assert_eq!(list.push(&mut node2), &12); - assert_eq!(list.push(&mut node3), &13); - assert_eq!(list.push(&mut node4), &14); - assert_eq!(list.push(&mut node5), &15); + let mut list = new_list(); + assert_eq!(list.as_mut().push(&mut node1), &11); + assert_eq!(list.as_mut().push(&mut node2), &12); + assert_eq!(list.as_mut().push(&mut node3), &13); + assert_eq!(list.as_mut().push(&mut node4), &14); + assert_eq!(list.as_mut().push(&mut node5), &15); - list.remove(&mut node1); - assert_eq!(list.pop().unwrap(), &12); - list.remove(&mut node3); - assert_eq!(list.pop().unwrap(), &14); - list.remove(&mut node5); - assert_empty(&mut list); + list.as_mut().remove(&mut node1); + assert_eq!(list.as_mut().pop().unwrap(), &12); + list.as_mut().remove(&mut node3); + assert_eq!(list.as_mut().pop().unwrap(), &14); + list.as_mut().remove(&mut node5); + assert_empty(list.as_mut()); - assert_eq!(list.push(&mut node1), &11); - assert_eq!(list.pop().unwrap(), &11); - assert_empty(&mut list); + assert_eq!(list.as_mut().push(&mut node1), &11); + assert_eq!(list.as_mut().pop().unwrap(), &11); + assert_empty(list.as_mut()); - assert_eq!(list.push(&mut node3), &13); - assert_eq!(list.push(&mut node4), &14); - list.remove(&mut node3); - list.remove(&mut node4); - assert_empty(&mut list); + assert_eq!(list.as_mut().push(&mut node3), &13); + assert_eq!(list.as_mut().push(&mut node4), &14); + list.as_mut().remove(&mut node3); + list.as_mut().remove(&mut node4); + assert_empty(list.as_mut()); } } @@ -77,17 +88,17 @@ fn complex_pushes_pops() { let mut node2 = UnsafeListEntry::new(4567); let mut node3 = UnsafeListEntry::new(9999); let mut node4 = UnsafeListEntry::new(8642); - let mut list = UnsafeList::new(); - list.push(&mut node1); - list.push(&mut node2); - assert_eq!(list.pop().unwrap(), &1234); - list.push(&mut node3); - assert_eq!(list.pop().unwrap(), &4567); - assert_eq!(list.pop().unwrap(), &9999); - assert_empty(&mut list); - list.push(&mut node4); - assert_eq!(list.pop().unwrap(), &8642); - assert_empty(&mut list); + let mut list = new_list(); + list.as_mut().push(&mut node1); + list.as_mut().push(&mut node2); + assert_eq!(list.as_mut().pop().unwrap(), &1234); + list.as_mut().push(&mut node3); + assert_eq!(list.as_mut().pop().unwrap(), &4567); + assert_eq!(list.as_mut().pop().unwrap(), &9999); + assert_empty(list.as_mut()); + list.as_mut().push(&mut node4); + assert_eq!(list.as_mut().pop().unwrap(), &8642); + assert_empty(list.as_mut()); } } @@ -95,11 +106,11 @@ fn complex_pushes_pops() { fn cell() { unsafe { let mut node = UnsafeListEntry::new(Cell::new(0)); - let mut list = UnsafeList::new(); - let noderef = list.push(&mut node); + let mut list = new_list(); + let noderef = list.as_mut().push(&mut node); assert_eq!(noderef.get(), 0); - list.pop().unwrap().set(1); - assert_empty(&mut list); + list.as_mut().pop().unwrap().set(1); + assert_empty(list.as_mut()); assert_eq!(noderef.get(), 1); } } diff --git a/library/std/src/sys/sync/condvar/sgx.rs b/library/std/src/sys/sync/condvar/sgx.rs index 2bde9d0694eda..77866bf773c65 100644 --- a/library/std/src/sys/sync/condvar/sgx.rs +++ b/library/std/src/sys/sync/condvar/sgx.rs @@ -1,3 +1,4 @@ +use crate::pin::Pin; use crate::sys::pal::waitqueue::{SpinMutex, WaitQueue, WaitVariable}; use crate::sys::sync::{Mutex, OnceBox}; use crate::time::Duration; @@ -12,24 +13,24 @@ impl Condvar { Condvar { inner: OnceBox::new() } } - fn get(&self) -> &SpinMutex> { - self.inner.get_or_init(|| Box::pin(SpinMutex::new(WaitVariable::new(())))).get_ref() + fn get(&self) -> Pin<&SpinMutex>> { + self.inner.get_or_init(|| WaitVariable::new(())) } #[inline] pub fn notify_one(&self) { - let guard = self.get().lock(); + let guard = self.get().lock_pinned(); let _ = WaitQueue::notify_one(guard); } #[inline] pub fn notify_all(&self) { - let guard = self.get().lock(); + let guard = self.get().lock_pinned(); let _ = WaitQueue::notify_all(guard); } pub unsafe fn wait(&self, mutex: &Mutex) { - let guard = self.get().lock(); + let guard = self.get().lock_pinned(); WaitQueue::wait(guard, || unsafe { mutex.unlock() }); mutex.lock() } diff --git a/library/std/src/sys/sync/mutex/sgx.rs b/library/std/src/sys/sync/mutex/sgx.rs index 3eb981bc65af6..cd348a5f2e60e 100644 --- a/library/std/src/sys/sync/mutex/sgx.rs +++ b/library/std/src/sys/sync/mutex/sgx.rs @@ -1,4 +1,5 @@ -use crate::sys::pal::waitqueue::{SpinMutex, WaitQueue, WaitVariable, try_lock_or_false}; +use crate::pin::Pin; +use crate::sys::pal::waitqueue::{SpinMutex, WaitQueue, WaitVariable}; use crate::sys::sync::OnceBox; pub struct Mutex { @@ -12,20 +13,20 @@ impl Mutex { Mutex { inner: OnceBox::new() } } - fn get(&self) -> &SpinMutex> { - self.inner.get_or_init(|| Box::pin(SpinMutex::new(WaitVariable::new(false)))).get_ref() + fn get(&self) -> Pin<&SpinMutex>> { + self.inner.get_or_init(|| WaitVariable::new(false)) } #[inline] pub fn lock(&self) { - let mut guard = self.get().lock(); + let mut guard = self.get().lock_pinned(); if *guard.lock_var() { // Another thread has the lock, wait WaitQueue::wait(guard, || {}) // Another thread has passed the lock to us } else { // We are just now obtaining the lock - *guard.lock_var_mut() = true; + *guard.as_mut().lock_var_mut() = true; } } @@ -33,10 +34,10 @@ impl Mutex { pub unsafe fn unlock(&self) { // SAFETY: the mutex was locked by the current thread, so it has been // initialized already. - let guard = unsafe { self.inner.get_unchecked().get_ref().lock() }; + let guard = unsafe { self.inner.get_unchecked().lock_pinned() }; if let Err(mut guard) = WaitQueue::notify_one(guard) { // No other waiters, unlock - *guard.lock_var_mut() = false; + *guard.as_mut().lock_var_mut() = false; } else { // There was a thread waiting, just pass the lock } @@ -44,13 +45,13 @@ impl Mutex { #[inline] pub fn try_lock(&self) -> bool { - let mut guard = try_lock_or_false!(self.get()); + let Some(mut guard) = self.get().try_lock_pinned() else { return false }; if *guard.lock_var() { // Another thread has the lock false } else { // We are just now obtaining the lock - *guard.lock_var_mut() = true; + *guard.as_mut().lock_var_mut() = true; true } } From 0246333ae9ca1c28786be25f503786372a446d10 Mon Sep 17 00:00:00 2001 From: Jethro Beekman Date: Tue, 18 Aug 2026 23:00:03 +0000 Subject: [PATCH 03/12] Move std::sys::pal::sgx::waitqueue::unsafe_list to a platform-agnostic location and add more unit tests. --- library/std/src/sys/pal/sgx/waitqueue/mod.rs | 16 +- .../pal/sgx/waitqueue/unsafe_list/tests.rs | 116 ------- library/std/src/sys/sync/mod.rs | 3 + .../sgx/waitqueue => sync}/unsafe_list.rs | 37 +-- library/std/src/sys/sync/unsafe_list/tests.rs | 285 ++++++++++++++++++ 5 files changed, 314 insertions(+), 143 deletions(-) delete mode 100644 library/std/src/sys/pal/sgx/waitqueue/unsafe_list/tests.rs rename library/std/src/sys/{pal/sgx/waitqueue => sync}/unsafe_list.rs (91%) create mode 100644 library/std/src/sys/sync/unsafe_list/tests.rs diff --git a/library/std/src/sys/pal/sgx/waitqueue/mod.rs b/library/std/src/sys/pal/sgx/waitqueue/mod.rs index 76c6ae62be796..7f7f20116308d 100644 --- a/library/std/src/sys/pal/sgx/waitqueue/mod.rs +++ b/library/std/src/sys/pal/sgx/waitqueue/mod.rs @@ -14,17 +14,16 @@ mod tests; mod spin_mutex; -mod unsafe_list; use fortanix_sgx_abi::{EV_UNPARK, Tcs, WAIT_INDEFINITE}; pub use self::spin_mutex::{SpinMutex, SpinMutexGuard}; -use self::unsafe_list::{UnsafeList, UnsafeListEntry}; use super::abi::{thread, usercalls}; use crate::num::NonZero; use crate::ops::{Deref, DerefMut}; use crate::panic::{self, AssertUnwindSafe}; use crate::pin::Pin; +use crate::sys::sync::unsafe_list::{UnsafeList, UnsafeListEntry}; use crate::time::Duration; /// An queue entry in a `WaitQueue`. @@ -62,6 +61,19 @@ impl WaitVariable { // pins it, and it is never moved out of it. unsafe { self.map_unchecked_mut(|this| &mut this.queue) } } + + /// Creates a mutex-protected `WaitVariable` on the heap, with its queue's + /// list initialized. Initialization makes the list self-referential and + /// happens before pinning: only the `Box` pointer is moved into the + /// `Pin`, the heap allocation itself never moves. + pub fn new(value: T) -> Pin>>> { + // SAFETY: `init` is called below, before the queue is otherwise used + // or dropped. + let queue = unsafe { WaitQueue::new() }; + let result = Box::new(SpinMutex::new(WaitVariable { queue, lock: value })); + result.lock().queue.inner.init(); + Box::into_pin(result) + } } #[derive(Copy, Clone)] diff --git a/library/std/src/sys/pal/sgx/waitqueue/unsafe_list/tests.rs b/library/std/src/sys/pal/sgx/waitqueue/unsafe_list/tests.rs deleted file mode 100644 index 6d2a0322d63bb..0000000000000 --- a/library/std/src/sys/pal/sgx/waitqueue/unsafe_list/tests.rs +++ /dev/null @@ -1,116 +0,0 @@ -use super::*; -use crate::cell::Cell; -use crate::pin::Pin; - -/// All lists are constructed by `WaitVariable::new`; this test -/// stand-in likewise initializes the list before pinning it. -fn new_list() -> Pin>> { - // SAFETY: `init` is called below, before the list is otherwise used or - // dropped. - let mut list = Box::new(unsafe { UnsafeList::new() }); - list.init(); - Box::into_pin(list) -} - -/// # Safety -/// List must be valid. -unsafe fn assert_empty(list: Pin<&mut UnsafeList>) { - assert!(unsafe { list.pop() }.is_none(), "assertion failed: list is not empty"); -} - -#[test] -fn init_empty() { - unsafe { - assert_empty(new_list::().as_mut()); - } -} - -#[test] -fn push_pop() { - unsafe { - let mut node = UnsafeListEntry::new(1234); - let mut list = new_list(); - assert_eq!(list.as_mut().push(&mut node), &1234); - assert_eq!(list.as_mut().pop().unwrap(), &1234); - assert_empty(list.as_mut()); - } -} - -#[test] -fn push_remove() { - unsafe { - let mut node = UnsafeListEntry::new(1234); - let mut list = new_list(); - assert_eq!(list.as_mut().push(&mut node), &1234); - list.as_mut().remove(&mut node); - assert_empty(list.as_mut()); - } -} - -#[test] -fn push_remove_pop() { - unsafe { - let mut node1 = UnsafeListEntry::new(11); - let mut node2 = UnsafeListEntry::new(12); - let mut node3 = UnsafeListEntry::new(13); - let mut node4 = UnsafeListEntry::new(14); - let mut node5 = UnsafeListEntry::new(15); - let mut list = new_list(); - assert_eq!(list.as_mut().push(&mut node1), &11); - assert_eq!(list.as_mut().push(&mut node2), &12); - assert_eq!(list.as_mut().push(&mut node3), &13); - assert_eq!(list.as_mut().push(&mut node4), &14); - assert_eq!(list.as_mut().push(&mut node5), &15); - - list.as_mut().remove(&mut node1); - assert_eq!(list.as_mut().pop().unwrap(), &12); - list.as_mut().remove(&mut node3); - assert_eq!(list.as_mut().pop().unwrap(), &14); - list.as_mut().remove(&mut node5); - assert_empty(list.as_mut()); - - assert_eq!(list.as_mut().push(&mut node1), &11); - assert_eq!(list.as_mut().pop().unwrap(), &11); - assert_empty(list.as_mut()); - - assert_eq!(list.as_mut().push(&mut node3), &13); - assert_eq!(list.as_mut().push(&mut node4), &14); - list.as_mut().remove(&mut node3); - list.as_mut().remove(&mut node4); - assert_empty(list.as_mut()); - } -} - -#[test] -fn complex_pushes_pops() { - unsafe { - let mut node1 = UnsafeListEntry::new(1234); - let mut node2 = UnsafeListEntry::new(4567); - let mut node3 = UnsafeListEntry::new(9999); - let mut node4 = UnsafeListEntry::new(8642); - let mut list = new_list(); - list.as_mut().push(&mut node1); - list.as_mut().push(&mut node2); - assert_eq!(list.as_mut().pop().unwrap(), &1234); - list.as_mut().push(&mut node3); - assert_eq!(list.as_mut().pop().unwrap(), &4567); - assert_eq!(list.as_mut().pop().unwrap(), &9999); - assert_empty(list.as_mut()); - list.as_mut().push(&mut node4); - assert_eq!(list.as_mut().pop().unwrap(), &8642); - assert_empty(list.as_mut()); - } -} - -#[test] -fn cell() { - unsafe { - let mut node = UnsafeListEntry::new(Cell::new(0)); - let mut list = new_list(); - let noderef = list.as_mut().push(&mut node); - assert_eq!(noderef.get(), 0); - list.as_mut().pop().unwrap().set(1); - assert_empty(list.as_mut()); - assert_eq!(noderef.get(), 1); - } -} diff --git a/library/std/src/sys/sync/mod.rs b/library/std/src/sys/sync/mod.rs index 8ee0b2649ed3d..ff675d22f1dc3 100644 --- a/library/std/src/sys/sync/mod.rs +++ b/library/std/src/sys/sync/mod.rs @@ -5,6 +5,9 @@ mod once; mod once_box; mod rwlock; mod thread_parking; +#[cfg(any(all(target_vendor = "fortanix", target_env = "sgx"), test))] +#[cfg_attr(not(all(target_vendor = "fortanix", target_env = "sgx")), allow(dead_code))] +pub(crate) mod unsafe_list; pub use condvar::Condvar; pub use mutex::Mutex; diff --git a/library/std/src/sys/pal/sgx/waitqueue/unsafe_list.rs b/library/std/src/sys/sync/unsafe_list.rs similarity index 91% rename from library/std/src/sys/pal/sgx/waitqueue/unsafe_list.rs rename to library/std/src/sys/sync/unsafe_list.rs index d5e9904cbee3e..c9d065279f294 100644 --- a/library/std/src/sys/pal/sgx/waitqueue/unsafe_list.rs +++ b/library/std/src/sys/sync/unsafe_list.rs @@ -71,7 +71,6 @@ #[cfg(test)] mod tests; -use super::{SpinMutex, WaitQueue, WaitVariable}; use crate::pin::{Pin, UnsafePinned}; use crate::ptr::{self, NonNull}; @@ -83,7 +82,7 @@ use crate::ptr::{self, NonNull}; /// returns a reference borrowing the entry, and `UnsafeList::remove` /// reborrows it exclusively, so the borrow checker enforces this for safe /// accesses. -pub struct UnsafeListEntry { +pub(crate) struct UnsafeListEntry { next: NonNull>, prev: NonNull>, value: Option, @@ -94,14 +93,14 @@ impl UnsafeListEntry { UnsafeListEntry { next: NonNull::dangling(), prev: NonNull::dangling(), value: None } } - pub fn new(value: T) -> Self { + pub(crate) fn new(value: T) -> Self { UnsafeListEntry { value: Some(value), ..Self::dummy() } } } // WARNING: self-referential struct! Must not be moved once initialized, see // the `Pinning` explanation at the top of the file. -pub struct UnsafeList { +pub(crate) struct UnsafeList { // UnsafePinned isn't required to implement this code, but it makes it a lot // simpler. Without UnsafePinned, the provenance of each entry link pointer // would need to be re-established prior to dereferencing, whenever it points @@ -122,7 +121,7 @@ impl UnsafeList { /// /// The caller must initialize the list with `init` before any other use, /// including dropping it. - pub(super) const unsafe fn new() -> Self { + pub(crate) const unsafe fn new() -> Self { UnsafeList { head_tail: UnsafePinned::new(UnsafeListEntry::dummy()) } } @@ -136,7 +135,7 @@ impl UnsafeList { /// location and must never be moved afterwards. Called exactly once per /// list, during construction (`WaitVariable::new`), so lists /// are always initialized before use. - fn init(&mut self) { + pub(crate) fn init(&mut self) { let head_tail = self.head_tail(); // SAFETY: `head_tail` is valid to dereference (see point 1 of the // `Pointer dereferencing` explanation at the top of the file). @@ -144,7 +143,7 @@ impl UnsafeList { unsafe { (*head_tail.as_ptr()).prev = head_tail }; } - pub fn is_empty(&self) -> bool { + pub(crate) fn is_empty(&self) -> bool { // SAFETY: `get` returns the address of `head_tail`, which is // non-null. let head_tail = unsafe { NonNull::new_unchecked(self.head_tail.get()) }; @@ -174,7 +173,10 @@ impl UnsafeList { /// not destroy the stack frame containing the entry. While the entry is /// in the list, it must not be accessed except through the reference /// returned here or by passing the entry to `remove`. - pub unsafe fn push<'a>(self: Pin<&mut Self>, entry: &'a mut UnsafeListEntry) -> &'a T { + pub(crate) unsafe fn push<'a>( + self: Pin<&mut Self>, + entry: &'a mut UnsafeListEntry, + ) -> &'a T { // SAFETY: the list is not moved out of the pinned reference. let this = unsafe { self.get_unchecked_mut() }; @@ -211,7 +213,7 @@ impl UnsafeList { /// /// The caller must make sure to synchronize ending the borrow of the /// return value and deallocation of the containing entry. - pub unsafe fn pop<'a>(self: Pin<&mut Self>) -> Option<&'a T> { + pub(crate) unsafe fn pop<'a>(self: Pin<&mut Self>) -> Option<&'a T> { if self.is_empty() { None } else { @@ -256,7 +258,7 @@ impl UnsafeList { /// The caller must ensure that `entry` has been pushed onto `self` /// prior to this call, has not been removed from the list since then /// (by `pop` or `remove`), and has not moved since it was pushed. - pub unsafe fn remove(self: Pin<&mut Self>, entry: &mut UnsafeListEntry) { + pub(crate) unsafe fn remove(self: Pin<&mut Self>, entry: &mut UnsafeListEntry) { rtassert!(!self.is_empty()); // BEFORE: @@ -294,18 +296,3 @@ impl Drop for UnsafeList { rtassert!(self.is_empty()); } } - -impl super::WaitVariable { - /// Creates a mutex-protected `WaitVariable` on the heap, with its queue's - /// list initialized. Initialization makes the list self-referential and - /// happens before pinning: only the `Box` pointer is moved into the - /// `Pin`, the heap allocation itself never moves. - pub fn new(value: T) -> Pin>>> { - // SAFETY: `init` is called below, before the queue is otherwise used - // or dropped. - let queue = unsafe { WaitQueue::new() }; - let result = Box::new(SpinMutex::new(WaitVariable { queue, lock: value })); - result.lock().queue.inner.init(); - Box::into_pin(result) - } -} diff --git a/library/std/src/sys/sync/unsafe_list/tests.rs b/library/std/src/sys/sync/unsafe_list/tests.rs new file mode 100644 index 0000000000000..4376b2870d426 --- /dev/null +++ b/library/std/src/sys/sync/unsafe_list/tests.rs @@ -0,0 +1,285 @@ +use super::*; +use crate::cell::Cell; +use crate::pin::Pin; + +/// All lists are constructed by `WaitVariable::new`; this test +/// stand-in likewise initializes the list before pinning it. +fn new_list() -> Pin>> { + // SAFETY: `init` is called below, before the list is otherwise used or + // dropped. + let mut list = Box::new(unsafe { UnsafeList::new() }); + list.init(); + Box::into_pin(list) +} + +/// # Safety +/// List must be valid. +unsafe fn assert_empty(list: Pin<&mut UnsafeList>) { + assert!(unsafe { list.pop() }.is_none(), "assertion failed: list is not empty"); +} + +#[test] +fn init_empty() { + unsafe { + assert_empty(new_list::().as_mut()); + } +} + +#[test] +fn push_pop() { + unsafe { + let mut node = UnsafeListEntry::new(1234); + let mut list = new_list(); + assert_eq!(list.as_mut().push(&mut node), &1234); + assert_eq!(list.as_mut().pop().unwrap(), &1234); + assert_empty(list.as_mut()); + } +} + +#[test] +fn push_remove() { + unsafe { + let mut node = UnsafeListEntry::new(1234); + let mut list = new_list(); + assert_eq!(list.as_mut().push(&mut node), &1234); + list.as_mut().remove(&mut node); + assert_empty(list.as_mut()); + } +} + +#[test] +fn push_remove_pop() { + unsafe { + let mut node1 = UnsafeListEntry::new(11); + let mut node2 = UnsafeListEntry::new(12); + let mut node3 = UnsafeListEntry::new(13); + let mut node4 = UnsafeListEntry::new(14); + let mut node5 = UnsafeListEntry::new(15); + let mut list = new_list(); + assert_eq!(list.as_mut().push(&mut node1), &11); + assert_eq!(list.as_mut().push(&mut node2), &12); + assert_eq!(list.as_mut().push(&mut node3), &13); + assert_eq!(list.as_mut().push(&mut node4), &14); + assert_eq!(list.as_mut().push(&mut node5), &15); + + list.as_mut().remove(&mut node1); + assert_eq!(list.as_mut().pop().unwrap(), &12); + list.as_mut().remove(&mut node3); + assert_eq!(list.as_mut().pop().unwrap(), &14); + list.as_mut().remove(&mut node5); + assert_empty(list.as_mut()); + + assert_eq!(list.as_mut().push(&mut node1), &11); + assert_eq!(list.as_mut().pop().unwrap(), &11); + assert_empty(list.as_mut()); + + assert_eq!(list.as_mut().push(&mut node3), &13); + assert_eq!(list.as_mut().push(&mut node4), &14); + list.as_mut().remove(&mut node3); + list.as_mut().remove(&mut node4); + assert_empty(list.as_mut()); + } +} + +#[test] +fn complex_pushes_pops() { + unsafe { + let mut node1 = UnsafeListEntry::new(1234); + let mut node2 = UnsafeListEntry::new(4567); + let mut node3 = UnsafeListEntry::new(9999); + let mut node4 = UnsafeListEntry::new(8642); + let mut list = new_list(); + list.as_mut().push(&mut node1); + list.as_mut().push(&mut node2); + assert_eq!(list.as_mut().pop().unwrap(), &1234); + list.as_mut().push(&mut node3); + assert_eq!(list.as_mut().pop().unwrap(), &4567); + assert_eq!(list.as_mut().pop().unwrap(), &9999); + assert_empty(list.as_mut()); + list.as_mut().push(&mut node4); + assert_eq!(list.as_mut().pop().unwrap(), &8642); + assert_empty(list.as_mut()); + } +} + +#[test] +fn cell() { + unsafe { + let mut node = UnsafeListEntry::new(Cell::new(0)); + let mut list = new_list(); + let noderef = list.as_mut().push(&mut node); + assert_eq!(noderef.get(), 0); + list.as_mut().pop().unwrap().set(1); + assert_empty(list.as_mut()); + assert_eq!(noderef.get(), 1); + } +} + +// Regression tests for the aliasing issues in rust-lang/rust#160603, +// exercising the usage patterns of the SGX `WaitQueue`. `hostile_reborrow` +// mirrors safe code reborrowing the structure containing the list between +// list operations (as `WaitVariable::lock_var_mut` and the pin projections +// do). + +struct Wrapper { + list: UnsafeList, + other: u32, +} + +impl Wrapper { + fn new() -> Pin>> { + // SAFETY: `init` is called below, before the list is otherwise used + // or dropped. + let mut wrapper = Box::new(Wrapper { list: unsafe { UnsafeList::new() }, other: 0 }); + wrapper.list.init(); + Box::into_pin(wrapper) + } + + fn list(self: Pin<&mut Self>) -> Pin<&mut UnsafeList> { + // SAFETY: `list` is structurally pinned: a pinned `Wrapper` pins it, + // and it is never moved out of it. + unsafe { self.map_unchecked_mut(|this| &mut this.list) } + } + + fn hostile_reborrow(self: Pin<&mut Self>) { + // SAFETY: nothing is moved; `other` is not structurally pinned. + let this = unsafe { self.get_unchecked_mut() }; + this.other = this.other.wrapping_add(1); + } +} + +// The `wait_timeout` fallback path: push an entry, use the returned +// reference, then remove the entry. +#[test] +fn wait_timeout_fallback() { + unsafe { + let mut w = Wrapper::new(); + let mut entry = UnsafeListEntry::new(1234); + let value = w.as_mut().list().push(&mut entry); + assert_eq!(*value, 1234); + + w.as_mut().hostile_reborrow(); + + // Not woken up: remove our own entry, as `wait_timeout` does. + w.as_mut().list().remove(&mut entry); + assert_empty(w.as_mut().list()); + } +} + +// Removing the first entry while others are present. +#[test] +fn remove_first_of_many() { + unsafe { + let mut w = Wrapper::new(); + let mut e1 = UnsafeListEntry::new(1); + let mut e2 = UnsafeListEntry::new(2); + let mut e3 = UnsafeListEntry::new(3); + w.as_mut().list().push(&mut e1); + w.as_mut().list().push(&mut e2); + w.as_mut().list().push(&mut e3); + w.as_mut().list().remove(&mut e1); + assert_eq!(w.as_mut().list().pop().unwrap(), &2); + assert_eq!(w.as_mut().list().pop().unwrap(), &3); + assert_empty(w.as_mut().list()); + } +} + +// Entries pushed from different "stack frames" and popped by a "notifier" +// (like `notify_all`), with hostile reborrows between every operation. +#[test] +fn notify_all_pattern() { + unsafe { + let mut w = Wrapper::new(); + let mut e1 = UnsafeListEntry::new(1); + let mut e2 = UnsafeListEntry::new(2); + w.as_mut().list().push(&mut e1); + w.as_mut().hostile_reborrow(); + w.as_mut().list().push(&mut e2); + w.as_mut().hostile_reborrow(); + + let mut count = 0; + while let Some(v) = w.as_mut().list().pop() { + count += *v; + w.as_mut().hostile_reborrow(); + } + assert_eq!(count, 3); + } +} + +// Empty-list churn: repeated push/pop cycles with reborrows in between. +#[test] +fn empty_churn() { + unsafe { + let mut w = Wrapper::new(); + for i in 0..4 { + let mut e = UnsafeListEntry::new(i); + w.as_mut().list().push(&mut e); + w.as_mut().hostile_reborrow(); + assert_eq!(w.as_mut().list().pop().unwrap(), &i); + w.as_mut().hostile_reborrow(); + assert!(w.list.is_empty()); + } + } +} + +// Cross-thread `wait`/`notify_one` pattern: the waiting thread pushes a +// stack-allocated entry and keeps reading through the reference returned by +// `push` while the notifying thread pops the entry and stores through the +// reference returned by `pop`. +#[test] +fn cross_thread_wait_notify() { + use crate::sync::atomic::{AtomicBool, Ordering}; + use crate::sync::{Arc, Mutex}; + use crate::thread; + + struct Queue { + list: UnsafeList, + } + // SAFETY: like the real `WaitQueue`, the list is only accessed while + // holding the mutex. + unsafe impl Send for Queue {} + + let queue = Arc::new(Mutex::new(Queue { + // SAFETY: `init` is called below, before the list is otherwise used + // or dropped. + list: unsafe { UnsafeList::new() }, + })); + queue.lock().unwrap().list.init(); + + for _ in 0..3 { + let waiter = { + let queue = Arc::clone(&queue); + thread::spawn(move || { + let mut entry = UnsafeListEntry::new(AtomicBool::new(false)); + let mut guard = queue.lock().unwrap(); + // SAFETY: the list lives in the heap allocation behind the + // `Arc` and is never moved. + let list = unsafe { Pin::new_unchecked(&mut guard.list) }; + // SAFETY: `entry` is only dropped after the notifier popped + // it and set the flag, and is not otherwise accessed while it + // is in the list. + let wake = unsafe { list.push(&mut entry) }; + drop(guard); + while !wake.load(Ordering::Acquire) { + thread::yield_now(); + } + }) + }; + loop { + let mut guard = queue.lock().unwrap(); + // SAFETY: the list lives in the heap allocation behind the `Arc` + // and is never moved. + let list = unsafe { Pin::new_unchecked(&mut guard.list) }; + // SAFETY: the entry is not deallocated until the waiting thread + // observes the flag, which is only set below. + if let Some(wake) = unsafe { list.pop() } { + // Set under the queue lock, like `notify_one`. + wake.store(true, Ordering::Release); + break; + } + drop(guard); + thread::yield_now(); + } + waiter.join().unwrap(); + } +} From 8770fd646cdb10b71783828d479835ee5b252e90 Mon Sep 17 00:00:00 2001 From: Jethro Beekman Date: Mon, 7 Sep 2026 17:51:19 +0200 Subject: [PATCH 04/12] SGX: Abort instead of panic on randomness generation failure Randomness generation failure is an abnormal circumstance that should lead to program termination. It's not reasonable to let consumers of `std` functionality catch such failures and resume from the. --- library/std/src/sys/random/sgx.rs | 47 +++++++++++++++---------------- 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/library/std/src/sys/random/sgx.rs b/library/std/src/sys/random/sgx.rs index 462b19003fad2..5b834f5742615 100644 --- a/library/std/src/sys/random/sgx.rs +++ b/library/std/src/sys/random/sgx.rs @@ -3,46 +3,43 @@ use crate::arch::x86_64::{_rdrand16_step, _rdrand32_step, _rdrand64_step}; const RETRIES: u32 = 10; fn fail() -> ! { - panic!("failed to generate random data"); + rtabort!("failed to generate random data"); } fn rdrand64() -> u64 { - unsafe { - let mut ret: u64 = 0; - for _ in 0..RETRIES { - if _rdrand64_step(&mut ret) == 1 { - return ret; - } + let mut ret: u64 = 0; + for _ in 0..RETRIES { + // SAFETY: the rdrand feature is enabled on SGX targets + if unsafe { _rdrand64_step(&mut ret) } == 1 { + return ret; } - - fail(); } + + fail(); } fn rdrand32() -> u32 { - unsafe { - let mut ret: u32 = 0; - for _ in 0..RETRIES { - if _rdrand32_step(&mut ret) == 1 { - return ret; - } + let mut ret: u32 = 0; + for _ in 0..RETRIES { + // SAFETY: the rdrand feature is enabled on SGX targets + if unsafe { _rdrand32_step(&mut ret) } == 1 { + return ret; } - - fail(); } + + fail(); } fn rdrand16() -> u16 { - unsafe { - let mut ret: u16 = 0; - for _ in 0..RETRIES { - if _rdrand16_step(&mut ret) == 1 { - return ret; - } + let mut ret: u16 = 0; + for _ in 0..RETRIES { + // SAFETY: the rdrand feature is enabled on SGX targets + if unsafe { _rdrand16_step(&mut ret) } == 1 { + return ret; } - - fail(); } + + fail(); } pub fn fill_bytes(bytes: &mut [u8]) { From 998113d9aa02449af639ebb77c6782c29fa16d35 Mon Sep 17 00:00:00 2001 From: Jethro Beekman Date: Mon, 7 Sep 2026 17:53:15 +0200 Subject: [PATCH 05/12] SGX: don't abort on randomness failure in usercalls::wait --- library/std/src/sys/pal/sgx/abi/usercalls/mod.rs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/library/std/src/sys/pal/sgx/abi/usercalls/mod.rs b/library/std/src/sys/pal/sgx/abi/usercalls/mod.rs index 2378028ccab92..236b918bfd063 100644 --- a/library/std/src/sys/pal/sgx/abi/usercalls/mod.rs +++ b/library/std/src/sys/pal/sgx/abi/usercalls/mod.rs @@ -1,6 +1,6 @@ +use crate::arch::x86_64::_rdrand64_step; use crate::cmp; use crate::io::{self, BorrowedCursor, IoSlice, IoSliceMut}; -use crate::random::random; use crate::time::{Duration, Instant}; pub(crate) mod alloc; @@ -167,6 +167,12 @@ pub fn exit(panic: bool) -> ! { /// Usercall `wait`. See the ABI documentation for more information. #[unstable(feature = "sgx_platform", issue = "56975")] pub fn wait(event_mask: u64, mut timeout: u64) -> io::Result { + fn try_rdrand() -> Option { + let mut val: u64 = 0; + // SAFETY: the rdrand feature is enabled on SGX targets + if unsafe { _rdrand64_step(&mut val) } == 1 { Some(val) } else { None } + } + if timeout != WAIT_NO && timeout != WAIT_INDEFINITE { // We don't want people to rely on accuracy of timeouts to make // security decisions in an SGX enclave. That's why we add a random @@ -175,9 +181,14 @@ pub fn wait(event_mask: u64, mut timeout: u64) -> io::Result { // to make things work in other cases. Note that in the SGX threat // model the enclave runner which is serving the wait usercall is not // trusted to ensure accurate timeouts. + // + // Since the random timeout is only intended as defense-in-depth + // protection at development/testing time, it's ok to continue if + // randomness generation fails. if let Ok(timeout_signed) = i64::try_from(timeout) { let tenth = timeout_signed / 10; - let deviation = random::(..).checked_rem(tenth).unwrap_or(0); + let deviation = + try_rdrand().and_then(|rnd| (rnd as i64).checked_rem(tenth)).unwrap_or(0); timeout = timeout_signed.saturating_add(deviation) as _; } } From 1e18d10e2b593a057f59d36ab8a4486e988f2d32 Mon Sep 17 00:00:00 2001 From: Xing Xue Date: Wed, 9 Sep 2026 10:39:58 -0400 Subject: [PATCH 06/12] Gate ELF code in metadata.rs for ELF only. --- .../rustc_codegen_ssa/src/back/metadata.rs | 28 ++++++++----------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/compiler/rustc_codegen_ssa/src/back/metadata.rs b/compiler/rustc_codegen_ssa/src/back/metadata.rs index a43bf72b6a27d..32b80b621038b 100644 --- a/compiler/rustc_codegen_ssa/src/back/metadata.rs +++ b/compiler/rustc_codegen_ssa/src/back/metadata.rs @@ -129,13 +129,10 @@ pub(super) fn search_for_section<'a>( fn add_gnu_property_note( file: &mut write::Object<'static>, architecture: Architecture, - binary_format: BinaryFormat, endianness: Endianness, ) { - // check bti protection - if binary_format != BinaryFormat::Elf - || !matches!(architecture, Architecture::X86_64 | Architecture::Aarch64) - { + // Only X86_64 and Aarch64 require a GNU property note. + if !matches!(architecture, Architecture::X86_64 | Architecture::Aarch64) { return; } @@ -253,12 +250,14 @@ pub(crate) fn create_object_file(sess: &Session) -> Option u32 { } } Architecture::PowerPc64 => { - const EF_PPC64_ABI_UNKNOWN: u32 = 0; const EF_PPC64_ABI_ELF_V1: u32 = 1; const EF_PPC64_ABI_ELF_V2: u32 = 2; @@ -392,11 +390,7 @@ pub(super) fn elf_e_flags(architecture: Architecture, sess: &Session) -> u32 { // which leads to broken binaries if ELFv1 is used for the object files. LlvmAbi::ElfV1 => EF_PPC64_ABI_ELF_V1, LlvmAbi::ElfV2 => EF_PPC64_ABI_ELF_V2, - _ if sess.target.options.binary_format.to_object() == BinaryFormat::Elf => { - bug!("invalid ABI specified for this PPC64 ELF target"); - } - // Fall back - _ => EF_PPC64_ABI_UNKNOWN, + _ => bug!("invalid ABI specified for this PPC64 ELF target"), } } Architecture::Sparc32Plus => elf::EF_SPARC_32PLUS, From 978c25012d9591ce28e4ee19bca88e1b75255ba1 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 8 Sep 2026 16:10:31 +1000 Subject: [PATCH 07/12] Remove most `Style` variants These are dead since the old emitter was removed in Jan 2026 (3ccabc6a8dc). Also removed: `BRIGHT_BLUE`, `Level::color`, and a couple of `level` fn parameters. --- .../src/annotate_snippet_emitter_writer.rs | 15 +++------- compiler/rustc_errors/src/emitter.rs | 25 +---------------- compiler/rustc_errors/src/lib.rs | 28 ------------------- 3 files changed, 5 insertions(+), 63 deletions(-) diff --git a/compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs b/compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs index 7e7f72943c7cd..42c22ffc0f5f3 100644 --- a/compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs +++ b/compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs @@ -166,9 +166,7 @@ impl AnnotateSnippetEmitter { // If at least one portion of the message is styled, we need to // "pre-style" the message let mut title = if msgs.iter().any(|(_, style)| style != &crate::Style::NoStyle) { - annotation_level - .clone() - .secondary_title(Cow::Owned(self.pre_style_msgs(msgs, *level, args))) + annotation_level.clone().secondary_title(Cow::Owned(self.pre_style_msgs(msgs, args))) } else { annotation_level.clone().primary_title(format_diag_messages(msgs, args)) }; @@ -255,7 +253,7 @@ impl AnnotateSnippetEmitter { // If at least one portion of the message is styled, we need to // "pre-style" the message let msg = if c.messages.iter().any(|(_, style)| style != &crate::Style::NoStyle) { - Cow::Owned(self.pre_style_msgs(&c.messages, c.level, args)) + Cow::Owned(self.pre_style_msgs(&c.messages, args)) } else { format_diag_messages(&c.messages, args) }; @@ -544,16 +542,11 @@ impl AnnotateSnippetEmitter { .short_message(self.short_message) } - fn pre_style_msgs( - &self, - msgs: &[(DiagMessage, Style)], - level: Level, - args: &DiagArgMap, - ) -> String { + fn pre_style_msgs(&self, msgs: &[(DiagMessage, Style)], args: &DiagArgMap) -> String { msgs.iter() .filter_map(|(m, style)| { let text = format_diag_message(m, args); - let style = style.anstyle(level); + let style = style.anstyle(); if text.is_empty() { None } else { Some(format!("{style}{text}{style:#}")) } }) .collect() diff --git a/compiler/rustc_errors/src/emitter.rs b/compiler/rustc_errors/src/emitter.rs index 6d5f8462ff496..e3decd29b594a 100644 --- a/compiler/rustc_errors/src/emitter.rs +++ b/compiler/rustc_errors/src/emitter.rs @@ -555,33 +555,10 @@ pub fn get_stderr_color_choice(color: ColorConfig, stderr: &std::io::Stderr) -> if matches!(choice, ColorChoice::Auto) { AutoStream::choice(stderr) } else { choice } } -/// On Windows, BRIGHT_BLUE is hard to read on black. Use cyan instead. -/// -/// See #36178. -const BRIGHT_BLUE: anstyle::Style = if cfg!(windows) { - AnsiColor::BrightCyan.on_default() -} else { - AnsiColor::BrightBlue.on_default() -}; - impl Style { - pub(crate) fn anstyle(&self, lvl: Level) -> anstyle::Style { + pub(crate) fn anstyle(&self) -> anstyle::Style { match self { - Style::Addition => AnsiColor::BrightGreen.on_default(), - Style::Removal => AnsiColor::BrightRed.on_default(), - Style::LineAndColumn => anstyle::Style::new(), - Style::LineNumber => BRIGHT_BLUE.effects(Effects::BOLD), - Style::Quotation => anstyle::Style::new(), - Style::MainHeaderMsg => if cfg!(windows) { - AnsiColor::BrightWhite.on_default() - } else { - anstyle::Style::new() - } - .effects(Effects::BOLD), - Style::UnderlinePrimary | Style::LabelPrimary => lvl.color().effects(Effects::BOLD), - Style::UnderlineSecondary | Style::LabelSecondary => BRIGHT_BLUE.effects(Effects::BOLD), Style::HeaderMsg | Style::NoStyle => anstyle::Style::new(), - Style::Level(lvl) => lvl.color().effects(Effects::BOLD), Style::Highlight => AnsiColor::Magenta.on_default().effects(Effects::BOLD), } } diff --git a/compiler/rustc_errors/src/lib.rs b/compiler/rustc_errors/src/lib.rs index 98a5b32e5d902..150ac1b93d6df 100644 --- a/compiler/rustc_errors/src/lib.rs +++ b/compiler/rustc_errors/src/lib.rs @@ -1656,23 +1656,6 @@ impl fmt::Display for Level { } impl Level { - fn color(self) -> anstyle::Style { - match self { - Bug | Fatal | Error | DelayedBug => AnsiColor::BrightRed.on_default(), - ForceWarning | Warning => { - if cfg!(windows) { - AnsiColor::BrightYellow.on_default() - } else { - AnsiColor::Yellow.on_default() - } - } - Note | OnceNote => AnsiColor::BrightGreen.on_default(), - Help | OnceHelp => AnsiColor::BrightCyan.on_default(), - FailureNote => anstyle::Style::new(), - Allow | Expect => unreachable!(), - } - } - pub fn to_str(self) -> &'static str { match self { Bug | DelayedBug => "error: internal compiler error", @@ -1698,20 +1681,9 @@ impl IntoDiagArg for Level { #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash, Encodable, Decodable)] pub enum Style { - MainHeaderMsg, HeaderMsg, - LineAndColumn, - LineNumber, - Quotation, - UnderlinePrimary, - UnderlineSecondary, - LabelPrimary, - LabelSecondary, NoStyle, - Level(Level), Highlight, - Addition, - Removal, } // FIXME(eddyb) this doesn't belong here AFAICT, should be moved to callsite. From 7f2354c3585471ef50a2ea110c9fe164e2dd8c49 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 8 Sep 2026 16:21:06 +1000 Subject: [PATCH 08/12] Remove `Style::HeaderMsg` It has a single use, where it is passed in to `format_diag_messages` which then discards it. So that use can be replaced with `Style::NoStyle`. --- .../rustc_errors/src/annotate_snippet_emitter_writer.rs | 6 ++---- compiler/rustc_errors/src/emitter.rs | 2 +- compiler/rustc_errors/src/lib.rs | 1 - 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs b/compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs index 42c22ffc0f5f3..df9e43fc345f0 100644 --- a/compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs +++ b/compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs @@ -307,10 +307,8 @@ impl AnnotateSnippetEmitter { // do not display this suggestion, it is meant only for tools } SuggestionStyle::HideCodeAlways => { - let msg = format_diag_messages( - &[(suggestion.msg.to_owned(), Style::HeaderMsg)], - args, - ); + let msg = + format_diag_messages(&[(suggestion.msg.to_owned(), Style::NoStyle)], args); group = group.element(annotate_snippets::Level::HELP.message(msg)); } SuggestionStyle::HideCodeInline diff --git a/compiler/rustc_errors/src/emitter.rs b/compiler/rustc_errors/src/emitter.rs index e3decd29b594a..60102a7ce8092 100644 --- a/compiler/rustc_errors/src/emitter.rs +++ b/compiler/rustc_errors/src/emitter.rs @@ -558,7 +558,7 @@ pub fn get_stderr_color_choice(color: ColorConfig, stderr: &std::io::Stderr) -> impl Style { pub(crate) fn anstyle(&self) -> anstyle::Style { match self { - Style::HeaderMsg | Style::NoStyle => anstyle::Style::new(), + Style::NoStyle => anstyle::Style::new(), Style::Highlight => AnsiColor::Magenta.on_default().effects(Effects::BOLD), } } diff --git a/compiler/rustc_errors/src/lib.rs b/compiler/rustc_errors/src/lib.rs index 150ac1b93d6df..8b15db856d65d 100644 --- a/compiler/rustc_errors/src/lib.rs +++ b/compiler/rustc_errors/src/lib.rs @@ -1681,7 +1681,6 @@ impl IntoDiagArg for Level { #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash, Encodable, Decodable)] pub enum Style { - HeaderMsg, NoStyle, Highlight, } From 22d747df1cf8a19f75cbd2f8790657e933491854 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 8 Sep 2026 18:54:13 +1000 Subject: [PATCH 09/12] Reduce unnecessary `Cow` use in error formatting `format_diag_messages` always constructs a `String`, so it can just return that. This simplifies various call sites. --- .../src/annotate_snippet_emitter_writer.rs | 7 +++---- compiler/rustc_errors/src/formatting.rs | 7 ++----- compiler/rustc_errors/src/json.rs | 11 ++--------- 3 files changed, 7 insertions(+), 18 deletions(-) diff --git a/compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs b/compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs index df9e43fc345f0..c772ffb69ab22 100644 --- a/compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs +++ b/compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs @@ -5,7 +5,6 @@ //! //! [annotate_snippets]: https://docs.rs/crate/annotate-snippets/ -use std::borrow::Cow; use std::fmt::Debug; use std::io; use std::io::Write; @@ -166,7 +165,7 @@ impl AnnotateSnippetEmitter { // If at least one portion of the message is styled, we need to // "pre-style" the message let mut title = if msgs.iter().any(|(_, style)| style != &crate::Style::NoStyle) { - annotation_level.clone().secondary_title(Cow::Owned(self.pre_style_msgs(msgs, args))) + annotation_level.clone().secondary_title(self.pre_style_msgs(msgs, args)) } else { annotation_level.clone().primary_title(format_diag_messages(msgs, args)) }; @@ -184,7 +183,7 @@ impl AnnotateSnippetEmitter { // If we don't have span information, emit and exit let Some(sm) = self.sm.as_ref() else { group = group.elements(children.iter().map(|c| { - let msg = format_diag_messages(&c.messages, args).to_string(); + let msg = format_diag_messages(&c.messages, args); let level = annotation_level_for_level(c.level); level.message(msg) })); @@ -253,7 +252,7 @@ impl AnnotateSnippetEmitter { // If at least one portion of the message is styled, we need to // "pre-style" the message let msg = if c.messages.iter().any(|(_, style)| style != &crate::Style::NoStyle) { - Cow::Owned(self.pre_style_msgs(&c.messages, args)) + self.pre_style_msgs(&c.messages, args) } else { format_diag_messages(&c.messages, args) }; diff --git a/compiler/rustc_errors/src/formatting.rs b/compiler/rustc_errors/src/formatting.rs index 7b617031d6c8e..a56b45c729887 100644 --- a/compiler/rustc_errors/src/formatting.rs +++ b/compiler/rustc_errors/src/formatting.rs @@ -24,11 +24,8 @@ fn to_fluent_args<'iter>(iter: impl Iterator>) -> FluentAr } /// Convert `DiagMessage`s to a string -pub fn format_diag_messages( - messages: &[(DiagMessage, Style)], - args: &DiagArgMap, -) -> Cow<'static, str> { - Cow::Owned(messages.iter().map(|(m, _)| format_diag_message(m, args)).collect::()) +pub fn format_diag_messages(messages: &[(DiagMessage, Style)], args: &DiagArgMap) -> String { + messages.iter().map(|(m, _)| format_diag_message(m, args)).collect::() } /// Convert a `DiagMessage` to a string diff --git a/compiler/rustc_errors/src/json.rs b/compiler/rustc_errors/src/json.rs index 04ac140f33261..1f5a8c2fe94a6 100644 --- a/compiler/rustc_errors/src/json.rs +++ b/compiler/rustc_errors/src/json.rs @@ -379,20 +379,13 @@ impl Diagnostic { let buf = Arc::try_unwrap(buf.0).unwrap().into_inner().unwrap(); let buf = String::from_utf8(buf).unwrap(); - Diagnostic { - message: formatted_message.to_string(), - code, - level, - spans, - children, - rendered: Some(buf), - } + Diagnostic { message: formatted_message, code, level, spans, children, rendered: Some(buf) } } fn from_sub_diagnostic(subdiag: &Subdiag, args: &DiagArgMap, je: &JsonEmitter) -> Diagnostic { let formatted_message = format_diag_messages(&subdiag.messages, args); Diagnostic { - message: formatted_message.to_string(), + message: formatted_message, code: None, level: subdiag.level.to_str(), spans: DiagnosticSpan::from_multispan(&subdiag.span, args, je), From 1e5fc291b1d91faadc03e9d9add3fdc42b4b3323 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 8 Sep 2026 19:03:28 +1000 Subject: [PATCH 10/12] Replace a `format_diag_messages` use When there's a single message, `format_diag_message` suffices. --- compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs b/compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs index c772ffb69ab22..e82b5bd8e4370 100644 --- a/compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs +++ b/compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs @@ -306,8 +306,7 @@ impl AnnotateSnippetEmitter { // do not display this suggestion, it is meant only for tools } SuggestionStyle::HideCodeAlways => { - let msg = - format_diag_messages(&[(suggestion.msg.to_owned(), Style::NoStyle)], args); + let msg = format_diag_message(&suggestion.msg, args).into_owned(); group = group.element(annotate_snippets::Level::HELP.message(msg)); } SuggestionStyle::HideCodeInline From 62c9a99e50cb06971d8680a87724c2361702d01a Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 8 Sep 2026 19:09:01 +1000 Subject: [PATCH 11/12] Introduce `Sublevel` for errors Current `Level` is used for both diagnostics and subdiagnostics. But both diagnostics and subdiagnostics only use some of the levels. This commit introduces `Sublevel` to tighten up the representation and prevent impossible combinations. --- compiler/rustc_codegen_ssa/src/back/write.rs | 4 +- .../src/annotate_snippet_emitter_writer.rs | 19 +++- compiler/rustc_errors/src/diagnostic.rs | 46 +++++----- compiler/rustc_errors/src/emitter.rs | 7 +- compiler/rustc_errors/src/lib.rs | 91 ++++++++++++------- .../rustc_expand/src/proc_macro_server.rs | 12 +++ .../traits/fulfillment_errors.rs | 6 +- 7 files changed, 120 insertions(+), 65 deletions(-) diff --git a/compiler/rustc_codegen_ssa/src/back/write.rs b/compiler/rustc_codegen_ssa/src/back/write.rs index e8b25eb4359d0..60ac8a2663b90 100644 --- a/compiler/rustc_codegen_ssa/src/back/write.rs +++ b/compiler/rustc_codegen_ssa/src/back/write.rs @@ -12,7 +12,7 @@ use rustc_data_structures::profiling::{SelfProfilerRef, VerboseTimingGuard}; use rustc_errors::emitter::Emitter; use rustc_errors::{ Diag, DiagArgMap, DiagCtxt, DiagCtxtHandle, DiagMessage, ErrCode, FatalError, FatalErrorMarker, - Level, MultiSpan, Style, Suggestions, catch_fatal_errors, + Level, MultiSpan, Style, Sublevel, Suggestions, catch_fatal_errors, }; use rustc_fs_util::link_or_copy; use rustc_incremental::{copy_cgu_workproduct_to_incr_comp_cache_dir, in_incr_comp_dir_sess}; @@ -1217,7 +1217,7 @@ struct Diagnostic { // missing the following fields from `rustc_errors::Subdiag`. // - `span`: it doesn't impl `Send`. struct Subdiagnostic { - level: Level, + level: Sublevel, messages: Vec<(DiagMessage, Style)>, } diff --git a/compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs b/compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs index e82b5bd8e4370..f075ff21bc7bc 100644 --- a/compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs +++ b/compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs @@ -28,7 +28,7 @@ use crate::emitter::{ use crate::formatting::{format_diag_message, format_diag_messages}; use crate::{ CodeSuggestion, DiagInner, DiagMessage, Emitter, ErrCode, Level, MultiSpan, Style, Subdiag, - SuggestionStyle, TerminalUrl, + Sublevel, SuggestionStyle, TerminalUrl, }; /// Generates diagnostics using annotate-snippet @@ -125,14 +125,23 @@ fn annotation_level_for_level(level: Level) -> annotate_snippets::level::Level<' } Level::Fatal | Level::Error => annotate_snippets::level::ERROR, Level::ForceWarning | Level::Warning => annotate_snippets::Level::WARNING, - Level::Note | Level::OnceNote => annotate_snippets::Level::NOTE, - Level::Help | Level::OnceHelp => annotate_snippets::Level::HELP, + Level::Note => annotate_snippets::Level::NOTE, + Level::Help => annotate_snippets::Level::HELP, Level::FailureNote => annotate_snippets::Level::NOTE.no_name(), Level::Allow => panic!("Should not call with Allow"), Level::Expect => panic!("Should not call with Expect"), } } +fn annotation_level_for_sublevel(level: Sublevel) -> annotate_snippets::level::Level<'static> { + match level { + Sublevel::Error => annotate_snippets::Level::ERROR, + Sublevel::Warning => annotate_snippets::Level::WARNING, + Sublevel::Note | Sublevel::OnceNote => annotate_snippets::Level::NOTE, + Sublevel::Help | Sublevel::OnceHelp => annotate_snippets::Level::HELP, + } +} + impl AnnotateSnippetEmitter { pub fn new(dst: Destination) -> Self { Self { @@ -184,7 +193,7 @@ impl AnnotateSnippetEmitter { let Some(sm) = self.sm.as_ref() else { group = group.elements(children.iter().map(|c| { let msg = format_diag_messages(&c.messages, args); - let level = annotation_level_for_level(c.level); + let level = annotation_level_for_sublevel(c.level); level.message(msg) })); @@ -247,7 +256,7 @@ impl AnnotateSnippetEmitter { } for c in children { - let level = annotation_level_for_level(c.level); + let level = annotation_level_for_sublevel(c.level); // If at least one portion of the message is styled, we need to // "pre-style" the message diff --git a/compiler/rustc_errors/src/diagnostic.rs b/compiler/rustc_errors/src/diagnostic.rs index 2874b85e9b67a..461e310d67896 100644 --- a/compiler/rustc_errors/src/diagnostic.rs +++ b/compiler/rustc_errors/src/diagnostic.rs @@ -16,7 +16,8 @@ use tracing::debug; use crate::{ CodeSuggestion, DiagCtxtHandle, DiagMessage, ErrCode, ErrorGuaranteed, ExplicitBug, Level, - MultiSpan, StashKey, Style, Substitution, SubstitutionPart, SuggestionStyle, Suggestions, + MultiSpan, StashKey, Style, Sublevel, Substitution, SubstitutionPart, SuggestionStyle, + Suggestions, }; /// Trait for types that `Diag::emit` can return as a "guarantee" (or "proof") @@ -321,9 +322,7 @@ impl DiagInner { Level::ForceWarning | Level::Warning | Level::Note - | Level::OnceNote | Level::Help - | Level::OnceHelp | Level::FailureNote | Level::Allow | Level::Expect => false, @@ -350,7 +349,12 @@ impl DiagInner { } } - pub(crate) fn sub(&mut self, level: Level, message: impl Into, span: MultiSpan) { + pub(crate) fn sub( + &mut self, + level: Sublevel, + message: impl Into, + span: MultiSpan, + ) { let sub = Subdiag { level, messages: vec![(message.into(), Style::NoStyle)], span }; self.children.push(sub); } @@ -374,7 +378,7 @@ impl DiagInner { pub fn emitted_at_sub_diag(&self) -> Subdiag { let track = format!("-Ztrack-diagnostics: created at {}", self.emitted_at); Subdiag { - level: crate::Level::Note, + level: crate::Sublevel::Note, messages: vec![(DiagMessage::Str(Cow::Owned(track)), Style::NoStyle)], span: MultiSpan::new(), } @@ -427,7 +431,7 @@ impl PartialEq for DiagInner { /// For example, a note attached to an error. #[derive(Clone, Debug, PartialEq, Hash, Encodable, Decodable)] pub struct Subdiag { - pub level: Level, + pub level: Sublevel, pub messages: Vec<(DiagMessage, Style)>, pub span: MultiSpan, } @@ -708,12 +712,12 @@ impl<'a, G: EmissionGuarantee> Diag<'a, G> { with_fn! { with_note, /// Add a note attached to this diagnostic. pub fn note(&mut self, msg: impl Into) -> &mut Self { - self.sub(Level::Note, msg, MultiSpan::new()); + self.sub(Sublevel::Note, msg, MultiSpan::new()); self } } pub fn highlighted_note(&mut self, msg: Vec) -> &mut Self { - self.sub_with_highlights(Level::Note, msg, MultiSpan::new()); + self.sub_with_highlights(Sublevel::Note, msg, MultiSpan::new()); self } @@ -722,13 +726,13 @@ impl<'a, G: EmissionGuarantee> Diag<'a, G> { span: impl Into, msg: Vec, ) -> &mut Self { - self.sub_with_highlights(Level::Note, msg, span.into()); + self.sub_with_highlights(Sublevel::Note, msg, span.into()); self } /// This is like [`Diag::note()`], but it's only printed once. pub fn note_once(&mut self, msg: impl Into) -> &mut Self { - self.sub(Level::OnceNote, msg, MultiSpan::new()); + self.sub(Sublevel::OnceNote, msg, MultiSpan::new()); self } @@ -740,7 +744,7 @@ impl<'a, G: EmissionGuarantee> Diag<'a, G> { sp: impl Into, msg: impl Into, ) -> &mut Self { - self.sub(Level::Note, msg, sp.into()); + self.sub(Sublevel::Note, msg, sp.into()); self } } @@ -751,14 +755,14 @@ impl<'a, G: EmissionGuarantee> Diag<'a, G> { sp: S, msg: impl Into, ) -> &mut Self { - self.sub(Level::OnceNote, msg, sp.into()); + self.sub(Sublevel::OnceNote, msg, sp.into()); self } with_fn! { with_warn, /// Add a warning attached to this diagnostic. pub fn warn(&mut self, msg: impl Into) -> &mut Self { - self.sub(Level::Warning, msg, MultiSpan::new()); + self.sub(Sublevel::Warning, msg, MultiSpan::new()); self } } @@ -769,26 +773,26 @@ impl<'a, G: EmissionGuarantee> Diag<'a, G> { sp: S, msg: impl Into, ) -> &mut Self { - self.sub(Level::Warning, msg, sp.into()); + self.sub(Sublevel::Warning, msg, sp.into()); self } with_fn! { with_help, /// Add a help message attached to this diagnostic. pub fn help(&mut self, msg: impl Into) -> &mut Self { - self.sub(Level::Help, msg, MultiSpan::new()); + self.sub(Sublevel::Help, msg, MultiSpan::new()); self } } /// This is like [`Diag::help()`], but it's only printed once. pub fn help_once(&mut self, msg: impl Into) -> &mut Self { - self.sub(Level::OnceHelp, msg, MultiSpan::new()); + self.sub(Sublevel::OnceHelp, msg, MultiSpan::new()); self } /// Add a help message attached to this diagnostic with a customizable highlighted message. pub fn highlighted_help(&mut self, msg: Vec) -> &mut Self { - self.sub_with_highlights(Level::Help, msg, MultiSpan::new()); + self.sub_with_highlights(Sublevel::Help, msg, MultiSpan::new()); self } @@ -798,7 +802,7 @@ impl<'a, G: EmissionGuarantee> Diag<'a, G> { span: impl Into, msg: Vec, ) -> &mut Self { - self.sub_with_highlights(Level::Help, msg, span.into()); + self.sub_with_highlights(Sublevel::Help, msg, span.into()); self } @@ -810,7 +814,7 @@ impl<'a, G: EmissionGuarantee> Diag<'a, G> { sp: impl Into, msg: impl Into, ) -> &mut Self { - self.sub(Level::Help, msg, sp.into()); + self.sub(Sublevel::Help, msg, sp.into()); self } } @@ -1233,13 +1237,13 @@ impl<'a, G: EmissionGuarantee> Diag<'a, G> { /// public methods above. /// /// Used by `proc_macro_server` for implementing `server::Diagnostic`. - pub fn sub(&mut self, level: Level, message: impl Into, span: MultiSpan) { + pub fn sub(&mut self, level: Sublevel, message: impl Into, span: MultiSpan) { self.deref_mut().sub(level, message, span); } /// Convenience function for internal use, clients should use one of the /// public methods above. - fn sub_with_highlights(&mut self, level: Level, messages: Vec, span: MultiSpan) { + fn sub_with_highlights(&mut self, level: Sublevel, messages: Vec, span: MultiSpan) { let messages = messages.into_iter().map(|m| (m.content.into(), m.style)).collect(); let sub = Subdiag { level, messages, span }; self.children.push(sub); diff --git a/compiler/rustc_errors/src/emitter.rs b/compiler/rustc_errors/src/emitter.rs index 60102a7ce8092..749b58e5d4b82 100644 --- a/compiler/rustc_errors/src/emitter.rs +++ b/compiler/rustc_errors/src/emitter.rs @@ -26,7 +26,8 @@ use tracing::{debug, warn}; use crate::formatting::format_diag_message; use crate::timings::TimingRecord; use crate::{ - CodeSuggestion, DiagInner, DiagMessage, Level, MultiSpan, Style, Subdiag, SuggestionStyle, + CodeSuggestion, DiagInner, DiagMessage, Level, MultiSpan, Style, Subdiag, Sublevel, + SuggestionStyle, }; /// Describes the way the content of the `rendered` field of the json output is generated @@ -209,7 +210,7 @@ pub trait Emitter { ); children.push(Subdiag { - level: Level::Note, + level: Sublevel::Note, messages: vec![(DiagMessage::from(msg), Style::NoStyle)], span: MultiSpan::new(), }); @@ -379,7 +380,7 @@ impl Emitter for EmitterWithNote { } fn emit_diagnostic(&mut self, mut diag: DiagInner) { - diag.sub(Level::Note, self.note.clone(), MultiSpan::new()); + diag.sub(Sublevel::Note, self.note.clone(), MultiSpan::new()); self.emitter.emit_diagnostic(diag); } } diff --git a/compiler/rustc_errors/src/lib.rs b/compiler/rustc_errors/src/lib.rs index 8b15db856d65d..0951189dd3256 100644 --- a/compiler/rustc_errors/src/lib.rs +++ b/compiler/rustc_errors/src/lib.rs @@ -617,8 +617,7 @@ impl<'a> DiagCtxtHandle<'a> { DelayedBug => { return self.dcx.inner.borrow_mut().emit_diagnostic(diag, self.tainted_with_errors); } - ForceWarning | Warning | Note | OnceNote | Help | OnceHelp | FailureNote | Allow - | Expect => None, + ForceWarning | Warning | Note | Help | FailureNote | Allow | Expect => None, }; // FIXME(Centril, #69537): Consider reintroducing panic on overwriting a stashed diagnostic @@ -1300,7 +1299,6 @@ impl DiagCtxtInner { } } Note | Help | FailureNote => {} - OnceNote | OnceHelp => panic!("bad level: {:?}", diagnostic.level), Allow => { // Nothing emitted for allowed lints. if diagnostic.has_future_breakage() { @@ -1359,8 +1357,11 @@ impl DiagCtxtInner { let not_yet_emitted = |sub: &mut Subdiag| { debug!(?sub); - if sub.level != OnceNote && sub.level != OnceHelp { - return true; + match sub.level { + Sublevel::Error | Sublevel::Warning | Sublevel::Note | Sublevel::Help => { + return true; + } + Sublevel::OnceNote | Sublevel::OnceHelp => {} } let mut hasher = StableHasher::new(); sub.hash(&mut hasher); @@ -1371,7 +1372,7 @@ impl DiagCtxtInner { diagnostic.children.retain_mut(not_yet_emitted); if already_emitted { let msg = "duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`"; - diagnostic.sub(Note, msg, MultiSpan::new()); + diagnostic.sub(Sublevel::Note, msg, MultiSpan::new()); } if is_error { @@ -1521,7 +1522,7 @@ impl DiagCtxtInner { let msg = msg!( "`flushed_delayed` got diagnostic with level {$level}, instead of the expected `DelayedBug`" ).arg("level", bug.level).format(); - bug.sub(Note, msg, bug.span.primary_span().unwrap().into()); + bug.sub(Sublevel::Note, msg, bug.span.primary_span().unwrap().into()); } bug.level = Bug; @@ -1572,26 +1573,24 @@ impl DelayedDiagInner { .arg("emitted_at", diag.emitted_at.clone()) .arg("note", self.note) .format(); - diag.sub(Note, msg, diag.span.primary_span().unwrap_or(DUMMY_SP).into()); + diag.sub(Sublevel::Note, msg, diag.span.primary_span().unwrap_or(DUMMY_SP).into()); diag } } -/// | Level | is_error | EmissionGuarantee | Top-level | Sub | Used in lints? -/// | ----- | -------- | ----------------- | --------- | --- | -------------- -/// | Bug | yes | BugAbort | yes | - | - -/// | Fatal | yes | FatalAbort/FatalError[^star] | yes | - | - -/// | Error | yes | ErrorGuaranteed | yes | - | yes -/// | DelayedBug | yes | ErrorGuaranteed | yes | - | - -/// | ForceWarning | - | () | yes | - | lint-only -/// | Warning | - | () | yes | yes | yes -/// | Note | - | () | rare | yes | - -/// | OnceNote | - | () | - | yes | lint-only -/// | Help | - | () | rare | yes | - -/// | OnceHelp | - | () | - | yes | lint-only -/// | FailureNote | - | () | rare | - | - -/// | Allow | - | () | yes | - | lint-only -/// | Expect | - | () | yes | - | lint-only +/// | Level | is_error | EmissionGuarantee | Top-level | Used in lints? +/// | ----- | -------- | ----------------- | --------- | -------------- +/// | Bug | yes | BugAbort | yes | - +/// | Fatal | yes | FatalAbort/FatalError[^star] | yes | - +/// | Error | yes | ErrorGuaranteed | yes | yes +/// | DelayedBug | yes | ErrorGuaranteed | yes | - +/// | ForceWarning | - | () | yes | lint-only +/// | Warning | - | () | yes | yes +/// | Note | - | () | rare | - +/// | Help | - | () | rare | - +/// | FailureNote | - | () | rare | - +/// | Allow | - | () | yes | lint-only +/// | Expect | - | () | yes | lint-only /// /// [^star]: `FatalAbort` normally, `FatalError` in the non-aborting "almost fatal" case that is /// occasionally used. @@ -1629,15 +1628,9 @@ pub enum Level { /// A message giving additional context. Note, - /// A note that is only emitted once. - OnceNote, - /// A message suggesting how to fix something. Help, - /// A help that is only emitted once. - OnceHelp, - /// Similar to `Note`, but used in cases where compilation has failed. When printed for human /// consumption, it doesn't have any kind of `note:` label. FailureNote, @@ -1661,8 +1654,8 @@ impl Level { Bug | DelayedBug => "error: internal compiler error", Fatal | Error => "error", ForceWarning | Warning => "warning", - Note | OnceNote => "note", - Help | OnceHelp => "help", + Note => "note", + Help => "help", FailureNote => "failure-note", Allow | Expect => unreachable!(), } @@ -1679,6 +1672,42 @@ impl IntoDiagArg for Level { } } +/// The level for a subdiagnostic. +#[derive(Copy, PartialEq, Eq, Clone, Hash, Debug, Encodable, Decodable)] +pub enum Sublevel { + /// See `Level::Error`. + /// + /// The compiler never uses this level in a subdiagnostic, but it can be produced by proc + /// macros. See tests/ui/proc-macro/sub-error-diag.rs for details. + Error, + + /// See `Level::Warning`. + Warning, + + /// See `Level::Note`. + Note, + + /// A note that is only emitted once. + OnceNote, + + /// See `Level::Help`. + Help, + + /// A help that is only emitted once. + OnceHelp, +} + +impl Sublevel { + pub fn to_str(self) -> &'static str { + match self { + Sublevel::Error => "error", + Sublevel::Warning => "warning", + Sublevel::Note | Sublevel::OnceNote => "note", + Sublevel::Help | Sublevel::OnceHelp => "help", + } + } +} + #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash, Encodable, Decodable)] pub enum Style { NoStyle, diff --git a/compiler/rustc_expand/src/proc_macro_server.rs b/compiler/rustc_expand/src/proc_macro_server.rs index c522626b39562..c0a9a43c64cdf 100644 --- a/compiler/rustc_expand/src/proc_macro_server.rs +++ b/compiler/rustc_expand/src/proc_macro_server.rs @@ -414,6 +414,18 @@ impl ToInternal for Level { } } +impl ToInternal for Level { + fn to_internal(self) -> rustc_errors::Sublevel { + match self { + Level::Error => rustc_errors::Sublevel::Error, + Level::Warning => rustc_errors::Sublevel::Warning, + Level::Note => rustc_errors::Sublevel::Note, + Level::Help => rustc_errors::Sublevel::Help, + _ => unreachable!("unknown proc_macro::Level variant: {:?}", self), + } + } +} + fn cancel_diags_into_string(diags: Vec>) -> String { let mut messages = diags.into_iter().flat_map(Diag::cancel_into_message); let msg = messages.next().expect("no diagnostic has a message"); diff --git a/compiler/rustc_trait_selection/src/error_reporting/traits/fulfillment_errors.rs b/compiler/rustc_trait_selection/src/error_reporting/traits/fulfillment_errors.rs index d8a22e745bcc2..5b029d6fad5d4 100644 --- a/compiler/rustc_trait_selection/src/error_reporting/traits/fulfillment_errors.rs +++ b/compiler/rustc_trait_selection/src/error_reporting/traits/fulfillment_errors.rs @@ -10,8 +10,8 @@ use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_data_structures::unord::UnordSet; use rustc_errors::codes::*; use rustc_errors::{ - Applicability, Diag, ErrorGuaranteed, Level, MultiSpan, StashKey, StringPart, Suggestions, msg, - pluralize, struct_span_code_err, + Applicability, Diag, ErrorGuaranteed, MultiSpan, StashKey, StringPart, Sublevel, Suggestions, + msg, pluralize, struct_span_code_err, }; use rustc_hir::attrs::diagnostic::CustomDiagnostic; use rustc_hir::attrs::lang_items::LangItem; @@ -2286,7 +2286,7 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> { } if let [child, ..] = &err.children[..] - && child.level == Level::Help + && child.level == Sublevel::Help && let Some(line) = child.messages.get(0) && let Some(line) = line.0.as_str() && line.starts_with("the trait") From 01e1453fcfab1fabf1703b033e91176fc3da18b0 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 9 Sep 2026 08:53:27 +1000 Subject: [PATCH 12/12] Tweak a `use` item Use the `FatalError` re-export from `rustc_errors` instead of `rustc_span`, because that's what's normally done. --- compiler/rustc_expand/src/module.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/compiler/rustc_expand/src/module.rs b/compiler/rustc_expand/src/module.rs index febe5e16cf446..d49eae9830b3f 100644 --- a/compiler/rustc_expand/src/module.rs +++ b/compiler/rustc_expand/src/module.rs @@ -4,12 +4,11 @@ use std::path::{self, Path, PathBuf}; use rustc_ast::{AttrVec, Attribute, Inline, Item, ModSpans}; use rustc_attr_parsing::template; use rustc_attr_parsing::validate_attr::emit_malformed_attribute; -use rustc_errors::{Diag, ErrorGuaranteed}; +use rustc_errors::{Diag, ErrorGuaranteed, FatalError}; use rustc_parse::lexer::StripTokens; use rustc_parse::{exp, new_parser_from_file, unwrap_or_emit_fatal}; use rustc_session::Session; use rustc_session::parse::ParseSess; -use rustc_span::fatal_error::FatalError; use rustc_span::{Ident, Span, sym}; use thin_vec::ThinVec;