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, 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 _; } } diff --git a/library/std/src/sys/pal/sgx/waitqueue/mod.rs b/library/std/src/sys/pal/sgx/waitqueue/mod.rs index 41d1413fcdee9..7f7f20116308d 100644 --- a/library/std/src/sys/pal/sgx/waitqueue/mod.rs +++ b/library/std/src/sys/pal/sgx/waitqueue/mod.rs @@ -14,16 +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, try_lock_or_false}; -use self::unsafe_list::{UnsafeList, UnsafeListEntry}; +pub use self::spin_mutex::{SpinMutex, SpinMutexGuard}; 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`. @@ -38,24 +38,41 @@ 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) } + } + + /// 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) } } @@ -68,7 +85,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, } @@ -79,6 +96,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. @@ -86,14 +124,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() @@ -118,8 +150,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 @@ -127,14 +175,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") @@ -155,7 +206,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 { @@ -165,7 +216,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") } @@ -173,11 +224,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 } @@ -189,14 +240,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 @@ -216,14 +267,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 deleted file mode 100644 index c736cab576e4d..0000000000000 --- a/library/std/src/sys/pal/sgx/waitqueue/unsafe_list.rs +++ /dev/null @@ -1,156 +0,0 @@ -//! A doubly-linked list where callers are in charge of memory allocation -//! of the nodes in the list. - -#[cfg(test)] -mod tests; - -use crate::mem; -use crate::ptr::NonNull; - -pub struct UnsafeListEntry { - next: NonNull>, - prev: NonNull>, - value: Option, -} - -impl UnsafeListEntry { - fn dummy() -> Self { - UnsafeListEntry { next: NonNull::dangling(), prev: NonNull::dangling(), value: None } - } - - pub fn new(value: T) -> Self { - UnsafeListEntry { value: Some(value), ..Self::dummy() } - } -} - -// WARNING: self-referential struct! -pub struct UnsafeList { - head_tail: NonNull>, - head_tail_entry: Option>, -} - -impl UnsafeList { - pub const fn new() -> Self { - unsafe { UnsafeList { head_tail: NonNull::new_unchecked(1 as _), head_tail_entry: None } } - } - - /// # Safety - 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; - } - } - - 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 { - // ,-------> /---------\ next ---, - // | |head_tail| | - // `--- prev \---------/ <-------` - // SAFETY: `self.head_tail` must meet all requirements for a reference. - unsafe { rtassert!(self.head_tail.as_ref().prev == first) }; - true - } else { - false - } - } else { - true - } - } - - /// Pushes an entry onto the back of the list. - /// - /// # Safety - /// - /// 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. - pub unsafe fn push<'a>(&mut self, entry: &'a mut UnsafeListEntry) -> &'a T { - unsafe { self.init() }; - - // BEFORE: - // /---------\ next ---> /---------\ - // ... |prev_tail| |head_tail| ... - // \---------/ <--- prev \---------/ - // - // AFTER: - // /---------\ 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; - // unwrap ok: always `Some` on non-dummy entries - unsafe { (*entry.as_ptr()).value.as_ref() }.unwrap() - } - - /// Pops an entry from the front of the list. - /// - /// # Safety - /// - /// 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 { - // BEFORE: - // /---------\ next ---> /-----\ next ---> /------\ - // ... |head_tail| |first| |second| ... - // \---------/ <--- prev \-----/ <--- prev \------/ - // - // AFTER: - // /---------\ 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(); - // unwrap ok: always `Some` on non-dummy entries - Some(unsafe { (*first.as_ptr()).value.as_ref() }.unwrap()) - } - } - - /// Removes an entry from the list. - /// - /// # Safety - /// - /// The caller must ensure that `entry` has been pushed onto `self` - /// prior to this call and has not moved since then. - pub unsafe fn remove(&mut self, entry: &mut UnsafeListEntry) { - rtassert!(!self.is_empty()); - // BEFORE: - // /----\ next ---> /-----\ next ---> /----\ - // ... |prev| |entry| |next| ... - // \----/ <--- prev \-----/ <--- prev \----/ - // - // AFTER: - // /----\ 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; - entry.next = NonNull::dangling(); - entry.prev = NonNull::dangling(); - } -} 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 c653dee17bc36..0000000000000 --- a/library/std/src/sys/pal/sgx/waitqueue/unsafe_list/tests.rs +++ /dev/null @@ -1,105 +0,0 @@ -use super::*; -use crate::cell::Cell; - -/// # Safety -/// List must be valid. -unsafe fn assert_empty(list: &mut UnsafeList) { - assert!(unsafe { list.pop() }.is_none(), "assertion failed: list is not empty"); -} - -#[test] -fn init_empty() { - unsafe { - assert_empty(&mut UnsafeList::::new()); - } -} - -#[test] -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); - } -} - -#[test] -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); - } -} - -#[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 = 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); - - 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); - - assert_eq!(list.push(&mut node1), &11); - assert_eq!(list.pop().unwrap(), &11); - assert_empty(&mut list); - - 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); - } -} - -#[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 = 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); - } -} - -#[test] -fn cell() { - unsafe { - let mut node = UnsafeListEntry::new(Cell::new(0)); - let mut list = UnsafeList::new(); - let noderef = list.push(&mut node); - assert_eq!(noderef.get(), 0); - list.pop().unwrap().set(1); - assert_empty(&mut list); - assert_eq!(noderef.get(), 1); - } -} 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]) { 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/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/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 } } diff --git a/library/std/src/sys/sync/unsafe_list.rs b/library/std/src/sys/sync/unsafe_list.rs new file mode 100644 index 0000000000000..c9d065279f294 --- /dev/null +++ b/library/std/src/sys/sync/unsafe_list.rs @@ -0,0 +1,298 @@ +//! 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. +// +// # 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::{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(crate) struct UnsafeListEntry { + next: NonNull>, + prev: NonNull>, + value: Option, +} + +impl UnsafeListEntry { + const fn dummy() -> Self { + UnsafeListEntry { next: NonNull::dangling(), prev: NonNull::dangling(), value: None } + } + + 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(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 + // to `head_tail`. + head_tail: UnsafePinned>, +} + +impl UnsafeList { + /// 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(crate) const unsafe fn new() -> Self { + UnsafeList { head_tail: UnsafePinned::new(UnsafeListEntry::dummy()) } + } + + 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()) } + } + + /// 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. + 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). + unsafe { (*head_tail.as_ptr()).next = head_tail }; + unsafe { (*head_tail.as_ptr()).prev = head_tail }; + } + + 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()) }; + // 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 + } + } + + /// Pushes an entry onto the back of the list. + /// + /// # Safety + /// + /// 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. 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(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() }; + + // BEFORE: + // /---------\ next ---> /---------\ + // ... |prev_tail| |head_tail| ... + // \---------/ <--- prev \---------/ + // + // AFTER: + // /---------\ next ---> /-----\ next ---> /---------\ + // ... |prev_tail| |entry| |head_tail| ... + // \---------/ <--- prev \-----/ <--- prev \---------/ + let entry = unsafe { NonNull::new_unchecked(entry) }; + 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). + 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() + } + + /// Pops an entry from the front of the list. + /// + /// # Safety + /// + /// The caller must make sure to synchronize ending the borrow of the + /// return value and deallocation of the containing entry. + pub(crate) 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| ... + // \---------/ <--- prev \-----/ <--- prev \------/ + // + // AFTER: + // /---------\ next ---> /------\ + // ... |head_tail| |second| ... + // \---------/ <--- prev \------/ + + 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). + 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()) + } + } + + /// Removes an entry from the list. + /// + /// # Safety + /// + /// 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(crate) unsafe fn remove(self: Pin<&mut Self>, entry: &mut UnsafeListEntry) { + rtassert!(!self.is_empty()); + + // BEFORE: + // /----\ next ---> /-----\ next ---> /----\ + // ... |prev| |entry| |next| ... + // \----/ <--- prev \-----/ <--- prev \----/ + // + // AFTER: + // /----\ next ---> /----\ + // ... |prev| |next| ... + // \----/ <--- 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(); + } +} + +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()); + } +} 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(); + } +} diff --git a/src/doc/rustc/src/lints/levels.md b/src/doc/rustc/src/lints/levels.md index 5b23ac9e09c1b..57c5a8dbe4c61 100644 --- a/src/doc/rustc/src/lints/levels.md +++ b/src/doc/rustc/src/lints/levels.md @@ -104,7 +104,7 @@ level is capped via cap-lints. ## deny A 'deny' lint produces an error if you violate it. For example, this code -runs into the `exceeding_bitshifts` lint. +runs into the `arithmetic_overflow` lint. ```rust,no_run fn main() { @@ -114,13 +114,13 @@ fn main() { ```bash $ rustc main.rs -error: bitshift exceeds the type's number of bits - --> main.rs:2:13 +error: this arithmetic operation will overflow + --> main.rs:2:5 | 2 | 100u8 << 10; - | ^^^^^^^^^^^ + | ^^^^^^^^^^^ attempt to shift left by `10_i32`, which would overflow | - = note: `#[deny(exceeding_bitshifts)]` on by default + = note: `#[deny(arithmetic_overflow)]` on by default ``` What's the difference between an error from a lint and a regular old error? @@ -306,19 +306,13 @@ And we compile it, capping lints to warn: ```bash $ rustc lib.rs --cap-lints warn -warning: bitshift exceeds the type's number of bits +warning: this arithmetic operation will overflow --> lib.rs:2:5 | 2 | 100u8 << 10; - | ^^^^^^^^^^^ + | ^^^^^^^^^^^ attempt to shift left by `10_i32`, which would overflow | - = note: `#[warn(exceeding_bitshifts)]` on by default - -warning: this expression will panic at run-time - --> lib.rs:2:5 - | -2 | 100u8 << 10; - | ^^^^^^^^^^^ attempt to shift left with overflow + = note: `#[warn(arithmetic_overflow)]` on by default ``` It now only warns, rather than errors. We can go further and allow all lints: diff --git a/src/librustdoc/html/render/search_index.rs b/src/librustdoc/html/render/search_index.rs index 0991778f87d79..4c93e632ab467 100644 --- a/src/librustdoc/html/render/search_index.rs +++ b/src/librustdoc/html/render/search_index.rs @@ -315,6 +315,29 @@ impl SerializedSearchIndex { let other_entryid_offset = self.names.len(); let mut map_other_pathid_to_self_pathid = Vec::new(); let mut skips = FxHashSet::default(); + + fn remap_entry_data( + other_entry_data: &EntryData, + map_other_pathid_to_self_pathid: &[usize], + ) -> EntryData { + EntryData { + parent: other_entry_data + .parent + .map(|parent| map_other_pathid_to_self_pathid[parent]) + .clone(), + module_path: other_entry_data + .module_path + .map(|path| map_other_pathid_to_self_pathid[path]) + .clone(), + exact_module_path: other_entry_data + .exact_module_path + .map(|exact_path| map_other_pathid_to_self_pathid[exact_path]) + .clone(), + krate: map_other_pathid_to_self_pathid[other_entry_data.krate], + ..other_entry_data.clone() + } + } + for (other_pathid, other_path_data) in other.path_data.iter().enumerate() { if let Some(other_path_data) = other_path_data { let name = Symbol::intern(&other.names[other_pathid]); @@ -439,87 +462,72 @@ impl SerializedSearchIndex { } } for other_entryid in 0..other.names.len() { - if skips.contains(&other_entryid) { - // we push tombstone entries to keep the IDs lined up - self.push(String::new(), None, None, String::new(), None, None, None); - } else { - self.push( - other.names[other_entryid].clone(), - other.path_data[other_entryid].clone(), - other.entry_data[other_entryid].as_ref().map(|other_entry_data| EntryData { - parent: other_entry_data - .parent - .map(|parent| map_other_pathid_to_self_pathid[parent]) - .clone(), - module_path: other_entry_data - .module_path - .map(|path| map_other_pathid_to_self_pathid[path]) - .clone(), - exact_module_path: other_entry_data - .exact_module_path - .map(|exact_path| map_other_pathid_to_self_pathid[exact_path]) - .clone(), - krate: map_other_pathid_to_self_pathid[other_entry_data.krate], - ..other_entry_data.clone() - }), - other.descs[other_entryid].clone(), - other.function_data[other_entryid].clone().map(|mut func| { - fn map_fn_sig_item( - map_other_pathid_to_self_pathid: &Vec, - ty: &mut RenderType, - ) { - match ty.id { - None => {} - Some(RenderTypeId::Index(generic)) if generic < 0 => {} - Some(RenderTypeId::Index(id)) => { - let id = usize::try_from(id).unwrap(); - let id = map_other_pathid_to_self_pathid[id]; - assert!(id != !0); - ty.id = Some(RenderTypeId::Index(isize::try_from(id).unwrap())); - } - _ => unreachable!(), + self.push( + other.names[other_entryid].clone(), + if skips.contains(&other_entryid) { + None + } else { + other.path_data[other_entryid].clone() + }, + other.entry_data[other_entryid].as_ref().map(|other_entry_data| { + remap_entry_data(other_entry_data, &map_other_pathid_to_self_pathid) + }), + other.descs[other_entryid].clone(), + other.function_data[other_entryid].clone().map(|mut func| { + fn map_fn_sig_item( + map_other_pathid_to_self_pathid: &Vec, + ty: &mut RenderType, + ) { + match ty.id { + None => {} + Some(RenderTypeId::Index(generic)) if generic < 0 => {} + Some(RenderTypeId::Index(id)) => { + let id = usize::try_from(id).unwrap(); + let id = map_other_pathid_to_self_pathid[id]; + assert!(id != !0); + ty.id = Some(RenderTypeId::Index(isize::try_from(id).unwrap())); } - if let Some(generics) = &mut ty.generics { - for generic in generics { - map_fn_sig_item(map_other_pathid_to_self_pathid, generic); - } + _ => unreachable!(), + } + if let Some(generics) = &mut ty.generics { + for generic in generics { + map_fn_sig_item(map_other_pathid_to_self_pathid, generic); } - if let Some(bindings) = &mut ty.bindings { - for (param, constraints) in bindings { - *param = match *param { - param @ RenderTypeId::Index(generic) if generic < 0 => { - param - } - RenderTypeId::Index(id) => { - let id = usize::try_from(id).unwrap(); - let id = map_other_pathid_to_self_pathid[id]; - assert!(id != !0); - RenderTypeId::Index(isize::try_from(id).unwrap()) - } - _ => unreachable!(), - }; - for constraint in constraints { - map_fn_sig_item( - map_other_pathid_to_self_pathid, - constraint, - ); + } + if let Some(bindings) = &mut ty.bindings { + for (param, constraints) in bindings { + *param = match *param { + param @ RenderTypeId::Index(generic) if generic < 0 => param, + RenderTypeId::Index(id) => { + let id = usize::try_from(id).unwrap(); + let id = map_other_pathid_to_self_pathid[id]; + assert!(id != !0); + RenderTypeId::Index(isize::try_from(id).unwrap()) } + _ => unreachable!(), + }; + for constraint in constraints { + map_fn_sig_item(map_other_pathid_to_self_pathid, constraint); } } } - for input in &mut func.inputs { - map_fn_sig_item(&map_other_pathid_to_self_pathid, input); - } - for output in &mut func.output { - map_fn_sig_item(&map_other_pathid_to_self_pathid, output); - } - for clause in &mut func.where_clause { - for entry in clause { - map_fn_sig_item(&map_other_pathid_to_self_pathid, entry); - } + } + for input in &mut func.inputs { + map_fn_sig_item(&map_other_pathid_to_self_pathid, input); + } + for output in &mut func.output { + map_fn_sig_item(&map_other_pathid_to_self_pathid, output); + } + for clause in &mut func.where_clause { + for entry in clause { + map_fn_sig_item(&map_other_pathid_to_self_pathid, entry); } - func - }), + } + func + }), + if skips.contains(&other_entryid) { + None + } else { other.type_data[other_entryid].as_ref().map(|type_data| TypeData { inverted_function_inputs_index: type_data .inverted_function_inputs_index @@ -556,11 +564,11 @@ impl SerializedSearchIndex { }) .collect(), search_unbox: type_data.search_unbox, - }), - other.alias_pointers[other_entryid] - .map(|alias_pointer| alias_pointer + other_entryid_offset), - ); - } + }) + }, + other.alias_pointers[other_entryid] + .map(|alias_pointer| alias_pointer + other_entryid_offset), + ); } if other.generic_inverted_index.len() > self.generic_inverted_index.len() { self.generic_inverted_index.resize(other.generic_inverted_index.len(), Vec::new()); diff --git a/src/librustdoc/html/render/write_shared.rs b/src/librustdoc/html/render/write_shared.rs index b3c2563aa3f97..ab72edacae296 100644 --- a/src/librustdoc/html/render/write_shared.rs +++ b/src/librustdoc/html/render/write_shared.rs @@ -375,15 +375,20 @@ impl CrateInfo { .fold(Ok(Vec::new()), |acc, parts_path| { let mut acc = acc?; let dir = &parts_path.0; - acc.append(&mut try_err!(std::fs::read_dir(dir), dir.as_path()) + let mut files: Vec> = try_err!(std::fs::read_dir(dir), dir.as_path()) + .map(|file| Ok(file?.path())) + .collect(); + files.sort_by_key(|p| p.as_ref().map_or(PathBuf::new(), |p| p.clone())); + acc.append(&mut files + .into_iter() .filter_map(|file| { - let to_crate_info = |file: Result| -> Result, Error> { + let to_crate_info = |file: Result| -> Result, Error> { let file = try_err!(file, dir.as_path()); - if file.path().extension() != Some(OsStr::new("json")) { + if file.extension() != Some(OsStr::new("json")) { return Ok(None); } - let parts = try_err!(fs::read(file.path()), file.path()); - let parts: CrateInfo = try_err!(serde_json::from_slice(&parts), file.path()); + let parts = try_err!(fs::read(&file), &file); + let parts: CrateInfo = try_err!(serde_json::from_slice(&parts), &file); Ok(Some(parts)) }; to_crate_info(file).transpose() diff --git a/src/tools/compiletest/src/directives.rs b/src/tools/compiletest/src/directives.rs index 3459273c922a1..24e933c516f81 100644 --- a/src/tools/compiletest/src/directives.rs +++ b/src/tools/compiletest/src/directives.rs @@ -215,6 +215,8 @@ pub(crate) struct TestProps { pub(crate) disable_gdb_pretty_printers: bool, /// Compare the output by lines, rather than as a single string. pub(crate) compare_output_by_lines: bool, + /// Use CCI (`--read-doc-meta` and `--write-doc-meta`) merge mode. + pub(crate) use_rustdoc_cci_doc_meta_merge: bool, } mod directives { @@ -262,6 +264,7 @@ mod directives { pub(crate) const MINICORE_COMPILE_FLAGS: &str = "minicore-compile-flags"; pub(crate) const DISABLE_GDB_PRETTY_PRINTERS: &str = "disable-gdb-pretty-printers"; pub(crate) const COMPARE_OUTPUT_BY_LINES: &str = "compare-output-by-lines"; + pub(crate) const USE_RUSTDOC_CCI_DOC_META_MERGE: &str = "use-rustdoc-cci-doc-meta-merge"; } impl TestProps { @@ -319,6 +322,7 @@ impl TestProps { dont_require_annotations: Default::default(), disable_gdb_pretty_printers: false, compare_output_by_lines: false, + use_rustdoc_cci_doc_meta_merge: false, } } diff --git a/src/tools/compiletest/src/directives/directive_names.rs b/src/tools/compiletest/src/directives/directive_names.rs index d305aaaf9453f..eb7020a00aa29 100644 --- a/src/tools/compiletest/src/directives/directive_names.rs +++ b/src/tools/compiletest/src/directives/directive_names.rs @@ -310,6 +310,7 @@ pub(crate) const KNOWN_DIRECTIVE_NAMES: &[&str] = &[ "unset-rustc-env", // Used by the tidy check `unknown_revision`. "unused-revision-names", + "use-rustdoc-cci-doc-meta-merge", // tidy-alphabetical-end ]; diff --git a/src/tools/compiletest/src/directives/handlers.rs b/src/tools/compiletest/src/directives/handlers.rs index 3848bb4854e75..59656bd15ab15 100644 --- a/src/tools/compiletest/src/directives/handlers.rs +++ b/src/tools/compiletest/src/directives/handlers.rs @@ -364,6 +364,13 @@ fn make_directive_handlers_map() -> HashMap<&'static str, Handler> { &mut props.compare_output_by_lines, ); }), + handler(USE_RUSTDOC_CCI_DOC_META_MERGE, |config, ln, props| { + config.set_name_directive( + ln, + USE_RUSTDOC_CCI_DOC_META_MERGE, + &mut props.use_rustdoc_cci_doc_meta_merge, + ); + }), ]; handlers diff --git a/src/tools/compiletest/src/runtest.rs b/src/tools/compiletest/src/runtest.rs index a08a96f0d7be5..c728e56cf639d 100644 --- a/src/tools/compiletest/src/runtest.rs +++ b/src/tools/compiletest/src/runtest.rs @@ -1047,14 +1047,21 @@ impl<'test> TestCx<'test> { .args(&self.props.doc_flags); match kind { - DocKind::Html => {} + DocKind::Html => { + if self.props.use_rustdoc_cci_doc_meta_merge { + rustdoc.arg("--write-doc-meta-dir").arg(out_dir.as_ref().join("doc.meta")); + } + } DocKind::Json => { rustdoc.arg("--output-format").arg("json"); } } // Both JSON output and `--disable-minification` are unstable rustdoc options. - if matches!(kind, DocKind::Json) || self.config.disable_minification { + if matches!(kind, DocKind::Json) + || self.config.disable_minification + || self.props.use_rustdoc_cci_doc_meta_merge + { rustdoc.arg("-Zunstable-options"); } if self.config.disable_minification { @@ -1065,7 +1072,31 @@ impl<'test> TestCx<'test> { rustdoc.arg(format!("-Clinker={}", linker)); } - self.compose_and_run_compiler(rustdoc, None) + let docres = self.compose_and_run_compiler(rustdoc, None); + if !docres.status.success() { + return docres; + } + if kind == DocKind::Html && self.props.use_rustdoc_cci_doc_meta_merge { + let mut rustdoc_merge = Command::new(rustdoc_path); + let current_dir = self.output_base_dir(); + rustdoc_merge.current_dir(current_dir); + rustdoc_merge + .arg("-o") + .arg(out_dir.as_ref()) + .args(&self.props.compile_flags) + .args(&self.props.doc_flags) + .arg("--read-doc-meta-dir") + .arg(out_dir.as_ref().join("doc.meta")) + .arg("-Zunstable-options"); + if self.config.disable_minification { + rustdoc_merge.arg("--disable-minification"); + } + let docmerge = self.compose_and_run_compiler(rustdoc_merge, None); + if !docmerge.status.success() { + return docmerge; + } + } + docres } fn exec_compiled_test(&self) -> ProcRes { diff --git a/tests/rustdoc-js-std/pathbuf.js b/tests/rustdoc-js-std/pathbuf.js new file mode 100644 index 0000000000000..97b11fabe0fb8 --- /dev/null +++ b/tests/rustdoc-js-std/pathbuf.js @@ -0,0 +1,17 @@ +// The PathBuf type is defined in std, +// but used in proc_macro. This means both crates' +// search indexes contain TypeData for it, +// but only std defines EntryData. +// This test case ensures we can merge them. +// +// https://github.com/rust-lang/rust/issues/162334 + + +const EXPECTED = [ + { + query: 'PathBuf', + others: [ + { 'path': 'std::path', 'name': 'PathBuf' }, + ], + }, +]; diff --git a/tests/rustdoc-js/auxiliary/upstream-type.rs b/tests/rustdoc-js/auxiliary/upstream-type.rs new file mode 100644 index 0000000000000..155d9c1626284 --- /dev/null +++ b/tests/rustdoc-js/auxiliary/upstream-type.rs @@ -0,0 +1,15 @@ +//@ use-rustdoc-cci-doc-meta-merge + +/// +pub struct FooBar; + +/// Test case for overlapping struct and function name +#[allow(nonstandard_style)] +pub struct overlapping_name { + _inner: (), +} + +/// Test case for overlapping function and struct name +pub fn overlapping_name() -> FooBar { + FooBar +} diff --git a/tests/rustdoc-js/downstream-type.js b/tests/rustdoc-js/downstream-type.js new file mode 100644 index 0000000000000..557042bb0ef53 --- /dev/null +++ b/tests/rustdoc-js/downstream-type.js @@ -0,0 +1,58 @@ +// exact-check +// ignore-order + +// The FooBar type is defined in upstream_type, +// but used in downstream_type. This means both crates' +// search indexes contain TypeData for it, +// but only upstream_type defines EntryData. +// This test case ensures we can merge them +// when running in CCI mode. +// https://github.com/rust-lang/rust/issues/162334 +const EXPECTED = [ + { + 'query': 'FooBar', + 'others': [ + { + 'path': 'upstream_type', + 'name': 'FooBar', + }, + ], + 'in_args': [ + { + 'path': 'downstream_type', + 'name': 'downstream_fn', + 'desc': 'https://github.com/rust-lang/rust/issues/162334', + }, + ], + 'returned': [ + { + 'path': 'upstream_type', + 'name': 'overlapping_name', + 'desc': 'Test case for overlapping function and struct name', + }, + ], + }, + { + 'query': 'overlapping_name', + 'others': [ + { + 'path': 'upstream_type', + 'name': 'overlapping_name', + 'ty': 5, + }, + { + 'path': 'upstream_type', + 'name': 'overlapping_name', + 'ty': 7, + }, + ], + 'returned': [], + 'in_args': [ + { + 'path': 'downstream_type', + 'name': 'with_overlap', + 'desc': '', + }, + ] + }, +]; diff --git a/tests/rustdoc-js/downstream-type.rs b/tests/rustdoc-js/downstream-type.rs new file mode 100644 index 0000000000000..16bba39aa1865 --- /dev/null +++ b/tests/rustdoc-js/downstream-type.rs @@ -0,0 +1,9 @@ +//@ aux-crate:upstream_type=upstream-type.rs +//@ aux-build:upstream-type.rs +//@ build-aux-docs +//@ use-rustdoc-cci-doc-meta-merge + +/// +pub fn downstream_fn(f: upstream_type::FooBar) {} + +pub fn with_overlap(f: upstream_type::overlapping_name) {}