From 3b4893b52e0b4b9f518999d2d89aedd977733789 Mon Sep 17 00:00:00 2001 From: Taiki Endo Date: Sun, 24 Dec 2023 05:30:26 +0900 Subject: [PATCH] epoch: Fix stacked borrows violations --- ci/miri.sh | 13 +++---- crossbeam-epoch/src/atomic.rs | 64 +++++++++++++++++++------------- crossbeam-epoch/src/internal.rs | 26 ++++++------- crossbeam-epoch/src/sync/list.rs | 37 +++++++++--------- 4 files changed, 76 insertions(+), 64 deletions(-) diff --git a/ci/miri.sh b/ci/miri.sh index c42943e22..69063de33 100755 --- a/ci/miri.sh +++ b/ci/miri.sh @@ -14,25 +14,24 @@ MIRIFLAGS="-Zmiri-strict-provenance -Zmiri-symbolic-alignment-check -Zmiri-disab MIRI_LEAK_CHECK='1' \ cargo miri test \ -p crossbeam-channel \ + -p crossbeam-epoch \ -p crossbeam-queue \ - -p crossbeam-utils 2>&1 | ts -i '%.s ' + -p crossbeam-utils \ + -p crossbeam 2>&1 | ts -i '%.s ' # -Zmiri-ignore-leaks is needed because we use detached threads in tests in tests/golang.rs: https://github.com/rust-lang/miri/issues/1371 MIRIFLAGS="-Zmiri-strict-provenance -Zmiri-symbolic-alignment-check -Zmiri-disable-isolation -Zmiri-ignore-leaks" \ cargo miri test \ -p crossbeam-channel --test golang 2>&1 | ts -i '%.s ' -# Use Tree Borrows instead of Stacked Borrows because epoch is not compatible with Stacked Borrows: https://github.com/crossbeam-rs/crossbeam/issues/545#issuecomment-1192785003 +# Use Tree Borrows instead of Stacked Borrows because skiplist is not compatible with Stacked Borrows: https://github.com/crossbeam-rs/crossbeam/issues/878 MIRIFLAGS="-Zmiri-strict-provenance -Zmiri-symbolic-alignment-check -Zmiri-disable-isolation -Zmiri-tree-borrows" \ cargo miri test \ - -p crossbeam-epoch \ - -p crossbeam-skiplist \ - -p crossbeam 2>&1 | ts -i '%.s ' + -p crossbeam-skiplist 2>&1 | ts -i '%.s ' -# Use Tree Borrows instead of Stacked Borrows because epoch is not compatible with Stacked Borrows: https://github.com/crossbeam-rs/crossbeam/issues/545#issuecomment-1192785003 # -Zmiri-compare-exchange-weak-failure-rate=0.0 is needed because some sequential tests (e.g., # doctest of Stealer::steal) incorrectly assume that sequential weak CAS will never fail. # -Zmiri-preemption-rate=0 is needed because this code technically has UB and Miri catches that. -MIRIFLAGS="-Zmiri-strict-provenance -Zmiri-symbolic-alignment-check -Zmiri-disable-isolation -Zmiri-tree-borrows -Zmiri-compare-exchange-weak-failure-rate=0.0 -Zmiri-preemption-rate=0" \ +MIRIFLAGS="-Zmiri-strict-provenance -Zmiri-symbolic-alignment-check -Zmiri-disable-isolation -Zmiri-compare-exchange-weak-failure-rate=0.0 -Zmiri-preemption-rate=0" \ cargo miri test \ -p crossbeam-deque 2>&1 | ts -i '%.s ' diff --git a/crossbeam-epoch/src/atomic.rs b/crossbeam-epoch/src/atomic.rs index b31926dc1..f2d907cb9 100644 --- a/crossbeam-epoch/src/atomic.rs +++ b/crossbeam-epoch/src/atomic.rs @@ -7,7 +7,6 @@ use core::marker::PhantomData; use core::mem::{self, MaybeUninit}; use core::ops::{Deref, DerefMut}; use core::ptr; -use core::slice; use crate::guard::Guard; #[cfg(not(miri))] @@ -125,8 +124,8 @@ pub trait Pointable { /// /// - The given `ptr` should have been initialized with [`Pointable::init`]. /// - `ptr` should not have yet been dropped by [`Pointable::drop`]. - /// - `ptr` should not be mutably dereferenced by [`Pointable::deref_mut`] concurrently. - unsafe fn deref<'a>(ptr: *mut ()) -> &'a Self; + /// - `ptr` should not be mutably dereferenced by [`Pointable::as_mut_ptr`] concurrently. + unsafe fn as_ptr(ptr: *mut ()) -> *const Self; /// Mutably dereferences the given pointer. /// @@ -134,9 +133,9 @@ pub trait Pointable { /// /// - The given `ptr` should have been initialized with [`Pointable::init`]. /// - `ptr` should not have yet been dropped by [`Pointable::drop`]. - /// - `ptr` should not be dereferenced by [`Pointable::deref`] or [`Pointable::deref_mut`] + /// - `ptr` should not be dereferenced by [`Pointable::as_ptr`] or [`Pointable::as_mut_ptr`] /// concurrently. - unsafe fn deref_mut<'a>(ptr: *mut ()) -> &'a mut Self; + unsafe fn as_mut_ptr(ptr: *mut ()) -> *mut Self; /// Drops the object pointed to by the given pointer. /// @@ -144,7 +143,7 @@ pub trait Pointable { /// /// - The given `ptr` should have been initialized with [`Pointable::init`]. /// - `ptr` should not have yet been dropped by [`Pointable::drop`]. - /// - `ptr` should not be dereferenced by [`Pointable::deref`] or [`Pointable::deref_mut`] + /// - `ptr` should not be dereferenced by [`Pointable::as_ptr`] or [`Pointable::as_mut_ptr`] /// concurrently. unsafe fn drop(ptr: *mut ()); } @@ -158,12 +157,12 @@ impl Pointable for T { Box::into_raw(Box::new(init)).cast::<()>() } - unsafe fn deref<'a>(ptr: *mut ()) -> &'a Self { - unsafe { &*(ptr as *const T) } + unsafe fn as_ptr(ptr: *mut ()) -> *const Self { + ptr as *const T } - unsafe fn deref_mut<'a>(ptr: *mut ()) -> &'a mut Self { - unsafe { &mut *ptr.cast::() } + unsafe fn as_mut_ptr(ptr: *mut ()) -> *mut Self { + ptr.cast::() } unsafe fn drop(ptr: *mut ()) { @@ -225,17 +224,22 @@ impl Pointable for [MaybeUninit] { } } - unsafe fn deref<'a>(ptr: *mut ()) -> &'a Self { + unsafe fn as_ptr(ptr: *mut ()) -> *const Self { unsafe { - let array = &*(ptr as *const Array); - slice::from_raw_parts(array.elements.as_ptr(), array.len) + let len = (*ptr.cast::>()).len; + // Use addr_of_mut for stacked borrows: https://github.com/rust-lang/miri/issues/1976 + let elements = + ptr::addr_of_mut!((*ptr.cast::>()).elements).cast::>(); + ptr::slice_from_raw_parts(elements, len) } } - unsafe fn deref_mut<'a>(ptr: *mut ()) -> &'a mut Self { + unsafe fn as_mut_ptr(ptr: *mut ()) -> *mut Self { unsafe { - let array = &mut *ptr.cast::>(); - slice::from_raw_parts_mut(array.elements.as_mut_ptr(), array.len) + let len = (*ptr.cast::>()).len; + let elements = + ptr::addr_of_mut!((*ptr.cast::>()).elements).cast::>(); + ptr::slice_from_raw_parts_mut(elements, len) } } @@ -846,7 +850,7 @@ impl fmt::Pointer for Atomic { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let data = self.data.load(Ordering::SeqCst); let (raw, _) = decompose_tag::(data); - fmt::Pointer::fmt(&(unsafe { T::deref(raw) as *const _ }), f) + fmt::Pointer::fmt(&(unsafe { T::as_ptr(raw) }), f) } } @@ -1134,14 +1138,14 @@ impl Deref for Owned { fn deref(&self) -> &T { let (raw, _) = decompose_tag::(self.data); - unsafe { T::deref(raw) } + unsafe { &*T::as_ptr(raw) } } } impl DerefMut for Owned { fn deref_mut(&mut self) -> &mut T { let (raw, _) = decompose_tag::(self.data); - unsafe { T::deref_mut(raw) } + unsafe { &mut *T::as_mut_ptr(raw) } } } @@ -1291,6 +1295,18 @@ impl<'g, T: ?Sized + Pointable> Shared<'g, T> { raw.is_null() } + #[doc(hidden)] + pub unsafe fn as_ptr(&self) -> *const T { + let (raw, _) = decompose_tag::(self.data); + unsafe { T::as_ptr(raw) } + } + + #[doc(hidden)] + pub unsafe fn as_mut_ptr(&self) -> *mut T { + let (raw, _) = decompose_tag::(self.data); + unsafe { T::as_mut_ptr(raw) } + } + /// Dereferences the pointer. /// /// Returns a reference to the pointee that is valid during the lifetime `'g`. @@ -1324,8 +1340,7 @@ impl<'g, T: ?Sized + Pointable> Shared<'g, T> { /// # unsafe { drop(a.into_owned()); } // avoid leak /// ``` pub unsafe fn deref(&self) -> &'g T { - let (raw, _) = decompose_tag::(self.data); - unsafe { T::deref(raw) } + unsafe { &*self.as_ptr() } } /// Dereferences the pointer. @@ -1366,8 +1381,7 @@ impl<'g, T: ?Sized + Pointable> Shared<'g, T> { /// # unsafe { drop(a.into_owned()); } // avoid leak /// ``` pub unsafe fn deref_mut(&mut self) -> &'g mut T { - let (raw, _) = decompose_tag::(self.data); - unsafe { T::deref_mut(raw) } + unsafe { &mut *self.as_mut_ptr() } } /// Converts the pointer to a reference. @@ -1407,7 +1421,7 @@ impl<'g, T: ?Sized + Pointable> Shared<'g, T> { if raw.is_null() { None } else { - Some(unsafe { T::deref(raw) }) + Some(unsafe { &*T::as_ptr(raw) }) } } @@ -1569,7 +1583,7 @@ impl fmt::Debug for Shared<'_, T> { impl fmt::Pointer for Shared<'_, T> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - fmt::Pointer::fmt(&(unsafe { self.deref() as *const _ }), f) + fmt::Pointer::fmt(&(unsafe { self.as_ptr() }), f) } } diff --git a/crossbeam-epoch/src/internal.rs b/crossbeam-epoch/src/internal.rs index bfc706ac3..e09393f97 100644 --- a/crossbeam-epoch/src/internal.rs +++ b/crossbeam-epoch/src/internal.rs @@ -292,7 +292,9 @@ pub(crate) struct Local { pin_count: Cell>, /// The local epoch. - epoch: CachePadded, + // TODO + // epoch: CachePadded, + epoch: AtomicEpoch, } // Make sure `Local` is less than or equal to 2048 bytes. @@ -324,7 +326,9 @@ impl Local { guard_count: Cell::new(0), handle_count: Cell::new(1), pin_count: Cell::new(Wrapping(0)), - epoch: CachePadded::new(AtomicEpoch::new(Epoch::starting())), + // TODO + // epoch: CachePadded::new(AtomicEpoch::new(Epoch::starting())), + epoch: AtomicEpoch::new(Epoch::starting()), }) .into_shared(unprotected()); collector.global.locals.insert(local, unprotected()); @@ -535,24 +539,18 @@ impl Local { } impl IsElement for Local { - fn entry_of(local: &Self) -> &Entry { + fn entry_of(local: *const Self) -> *const Entry { // SAFETY: `Local` is `repr(C)` and `entry` is the first field of it. - unsafe { - let entry_ptr = (local as *const Self).cast::(); - &*entry_ptr - } + local.cast::() } - unsafe fn element_of(entry: &Entry) -> &Self { + unsafe fn element_of(entry: *const Entry) -> *const Self { // SAFETY: `Local` is `repr(C)` and `entry` is the first field of it. - unsafe { - let local_ptr = (entry as *const Entry).cast::(); - &*local_ptr - } + entry.cast::() } - unsafe fn finalize(entry: &Entry, guard: &Guard) { - unsafe { guard.defer_destroy(Shared::from(Self::element_of(entry) as *const _)) } + unsafe fn finalize(entry: *const Entry, guard: &Guard) { + unsafe { guard.defer_destroy(Shared::from(Self::element_of(entry))) } } } diff --git a/crossbeam-epoch/src/sync/list.rs b/crossbeam-epoch/src/sync/list.rs index e30cff724..461a07663 100644 --- a/crossbeam-epoch/src/sync/list.rs +++ b/crossbeam-epoch/src/sync/list.rs @@ -4,6 +4,7 @@ //! 2002. use core::marker::PhantomData; +use core::ptr::NonNull; use core::sync::atomic::Ordering::{Acquire, Relaxed, Release}; use crate::{unprotected, Atomic, Guard, Shared}; @@ -66,7 +67,7 @@ pub(crate) struct Entry { /// pub(crate) trait IsElement { /// Returns a reference to this element's `Entry`. - fn entry_of(_: &T) -> &Entry; + fn entry_of(_: *const T) -> *const Entry; /// Given a reference to an element's entry, returns that element. /// @@ -80,7 +81,7 @@ pub(crate) trait IsElement { /// /// The caller has to guarantee that the `Entry` is called with was retrieved from an instance /// of the element type (`T`). - unsafe fn element_of(_: &Entry) -> &T; + unsafe fn element_of(_: *const Entry) -> *const T; /// The function that is called when an entry is unlinked from list. /// @@ -88,7 +89,7 @@ pub(crate) trait IsElement { /// /// The caller has to guarantee that the `Entry` is called with was retrieved from an instance /// of the element type (`T`). - unsafe fn finalize(_: &Entry, _: &Guard); + unsafe fn finalize(_: *const Entry, _: &Guard); } /// A lock-free, intrusive linked list of type `T`. @@ -173,16 +174,16 @@ impl> List { // Insert right after head, i.e. at the beginning of the list. let to = &self.head; // Get the intrusively stored Entry of the new element to insert. - let entry: &Entry = C::entry_of(unsafe { container.deref() }); + let entry: *const Entry = C::entry_of(unsafe { container.as_ptr() }); // Make a Shared ptr to that Entry. - let entry_ptr = Shared::from(entry as *const _); + let entry_ptr = Shared::from(entry); // Read the current successor of where we want to insert. let mut next = to.load(Relaxed, guard); loop { // Set the Entry of the to-be-inserted element to point to the previous successor of // `to`. - entry.next.store(next, Relaxed); + unsafe { (*entry).next.store(next, Relaxed) } match to.compare_exchange_weak(next, entry_ptr, Release, Relaxed, guard) { Ok(_) => break, // We lost the race or weak CAS failed spuriously. Update the successor and try @@ -220,12 +221,12 @@ impl> Drop for List { unsafe { let guard = unprotected(); let mut curr = self.head.load(Relaxed, guard); - while let Some(c) = curr.as_ref() { - let succ = c.next.load(Relaxed, guard); + while let Some(c) = NonNull::new(curr.as_ptr() as *mut Entry) { + let succ = c.as_ref().next.load(Relaxed, guard); // Verify that all elements have been removed from the list. assert_eq!(succ.tag(), 1); - C::finalize(curr.deref(), guard); + C::finalize(curr.as_ptr(), guard); curr = succ; } } @@ -236,8 +237,8 @@ impl<'g, T: 'g, C: IsElement> Iterator for Iter<'g, T, C> { type Item = Result<&'g T, IterError>; fn next(&mut self) -> Option { - while let Some(c) = unsafe { self.curr.as_ref() } { - let succ = c.next.load(Acquire, self.guard); + while let Some(c) = unsafe { NonNull::new(self.curr.as_ptr() as *mut Entry) } { + let succ = unsafe { c.as_ref().next.load(Acquire, self.guard) }; if succ.tag() == 1 { // This entry was removed. Try unlinking it from the list. @@ -257,7 +258,7 @@ impl<'g, T: 'g, C: IsElement> Iterator for Iter<'g, T, C> { // deallocation. Deferred drop is okay, because `list.delete()` can only be // called if `T: 'static`. unsafe { - C::finalize(self.curr.deref(), self.guard); + C::finalize(self.curr.as_ptr(), self.guard); } // `succ` is the new value of `self.pred`. @@ -284,10 +285,10 @@ impl<'g, T: 'g, C: IsElement> Iterator for Iter<'g, T, C> { } // Move one step forward. - self.pred = &c.next; + self.pred = unsafe { &(*c.as_ptr()).next }; self.curr = succ; - return Some(Ok(unsafe { C::element_of(c) })); + return Some(Ok(unsafe { &*C::element_of(c.as_ptr()) })); } // We reached the end of the list. @@ -303,16 +304,16 @@ mod tests { use std::sync::Barrier; impl IsElement for Entry { - fn entry_of(entry: &Self) -> &Entry { + fn entry_of(entry: *const Self) -> *const Entry { entry } - unsafe fn element_of(entry: &Entry) -> &Self { + unsafe fn element_of(entry: *const Entry) -> *const Self { entry } - unsafe fn finalize(entry: &Entry, guard: &Guard) { - unsafe { guard.defer_destroy(Shared::from(Self::element_of(entry) as *const _)) } + unsafe fn finalize(entry: *const Entry, guard: &Guard) { + unsafe { guard.defer_destroy(Shared::from(Self::element_of(entry))) } } }