diff --git a/benches/benches/binary_trees_with_parent_pointers.rs b/benches/benches/binary_trees_with_parent_pointers.rs index 2572397..1e72001 100644 --- a/benches/benches/binary_trees_with_parent_pointers.rs +++ b/benches/benches/binary_trees_with_parent_pointers.rs @@ -1,5 +1,6 @@ //! Benchmark adapted from the shredder crate, released under MIT license. Src: https://github.com/Others/shredder/blob/266de5a3775567463ee82febc42eed1c9a8b6197/benches/shredder_benchmark.rs +use std::cell::RefCell; use std::hint::black_box; use rust_cc::*; @@ -32,7 +33,7 @@ enum TreeNodeWithParent { right: Cc, }, Nested { - parent: Cc, + parent: RefCell>>, left: Cc, right: Cc, }, @@ -64,22 +65,46 @@ impl TreeNodeWithParent { return Cc::new(Self::End); } - Cc::::new_cyclic(|cc| Self::Root { - left: Self::new_nested(depth - 1, cc.clone()), - right: Self::new_nested(depth - 1, cc.clone()), - }) + let root = Cc::new(Self::Root { + left: Self::new_nested(depth - 1), + right: Self::new_nested(depth - 1), + }); + + if let Self::Root{ left, right } = &*root { + if let Self::Nested { parent, ..} = &**left { + *parent.borrow_mut() = Some(root.clone()); + } + + if let Self::Nested { parent, ..} = &**right { + *parent.borrow_mut() = Some(root.clone()); + } + } + + root } - fn new_nested(depth: usize, parent: Cc) -> Cc { + fn new_nested(depth: usize) -> Cc { if depth == 0 { return Cc::new(Self::End); } - Cc::::new_cyclic(|cc| Self::Nested { - left: Self::new_nested(depth - 1, cc.clone()), - right: Self::new_nested(depth - 1, cc.clone()), - parent, - }) + let cc = Cc::new(Self::Nested { + left: Self::new_nested(depth - 1), + right: Self::new_nested(depth - 1), + parent: RefCell::new(None), + }); + + if let Self::Nested{ left, right, .. } = &*cc { + if let Self::Nested { parent, ..} = &**left { + *parent.borrow_mut() = Some(cc.clone()); + } + + if let Self::Nested { parent, ..} = &**right { + *parent.borrow_mut() = Some(cc.clone()); + } + } + + cc } fn check(&self) -> usize { diff --git a/src/cc.rs b/src/cc.rs index 6e0c8e9..236795a 100644 --- a/src/cc.rs +++ b/src/cc.rs @@ -3,7 +3,7 @@ use std::cell::UnsafeCell; use std::marker::PhantomData; use std::mem::{self, MaybeUninit}; use std::ops::Deref; -use std::ptr::{self, addr_of, drop_in_place, NonNull}; +use std::ptr::{self, drop_in_place, NonNull}; use std::rc::Rc; #[cfg(feature = "nightly")] use std::{ @@ -51,7 +51,7 @@ impl Cc { } } - #[must_use = "newly created Cc is immediately dropped"] + /*#[must_use = "newly created Cc is immediately dropped"] #[track_caller] pub fn new_cyclic(f: F) -> Cc where @@ -81,32 +81,30 @@ impl Cc { // Return cc, since it is now valid cc - } + }*/ /// Takes out the value inside this [`Cc`]. /// /// This is safe since this function panics if this [`Cc`] is not unique (see [`is_unique`]). /// /// # Panics - /// This function panics if this [`Cc`] is not valid (see [`is_valid`]) or is not unique (see [`is_unique`]). + /// This function panics if this [`Cc`] is not unique (see [`is_unique`]). /// - /// [`is_valid`]: fn@Cc::is_valid /// [`is_unique`]: fn@Cc::is_unique #[inline] #[track_caller] pub fn into_inner(self) -> T { - assert!(self.is_valid(), "Cc<_> is not valid"); assert!(self.is_unique(), "Cc<_> is not unique"); assert!( - !self.counter_marker().is_traced_or_invalid(), + !self.counter_marker().is_traced(), "Cc<_> is being used by the collector and inner value cannot be taken out (this might have happen inside Trace, Finalize or Drop implementations)." ); // Make sure self is not into POSSIBLE_CYCLES before deallocating remove_from_list(self.inner.cast()); - // SAFETY: self is unique, valid and is not inside any list + // SAFETY: self is unique and is not inside any list unsafe { let t = ptr::read(self.inner().get_elem()); let layout = self.inner().layout(); @@ -124,11 +122,10 @@ impl Cc> { /// See [`MaybeUninit::assume_init`]. /// /// # Panics - /// This function panics if called while tracing or this [`Cc`] isn't valid (see [`is_valid`]). + /// This function panics if called while tracing. /// /// [`MaybeUninit::assume_init`]: fn@MaybeUninit::assume_init /// [`is_unique`]: fn@Cc::is_unique - /// [`is_valid`]: fn@Cc::is_valid #[inline] #[track_caller] pub unsafe fn assume_init(self) -> Cc { @@ -136,8 +133,6 @@ impl Cc> { panic!("Cannot initialize a Cc while tracing!"); } - assert!(self.is_valid()); - remove_from_list(self.inner.cast()); // The counter should not be updated since we're taking self @@ -155,12 +150,11 @@ impl Cc> { /// This is safe since this function panics if this [`Cc`] is not unique (see [`is_unique`]). /// /// # Panics - /// This function panics if this [`Cc`] is not valid (see [`is_valid`]), is not unique (see [`is_unique`]) or if it's called while tracing. + /// This function panics if this [`Cc`] is not unique (see [`is_unique`]) or if it's called while tracing. /// /// [`Cc`]: struct@Cc /// [`MaybeUninit::assume_init`]: fn@MaybeUninit::assume_init /// [`is_unique`]: fn@Cc::is_unique - /// [`is_valid`]: fn@Cc::is_valid #[inline] #[track_caller] pub fn init(mut self, value: T) -> Cc { @@ -168,8 +162,6 @@ impl Cc> { panic!("Cannot initialize a Cc while tracing!"); } - assert!(self.is_valid()); - // This prevents race conditions assert!(self.is_unique(), "Cc is not unique"); @@ -206,11 +198,6 @@ impl Cc { self.strong_count() == 1 } - #[inline] - pub fn is_valid(&self) -> bool { - self.counter_marker().is_valid() - } - #[cfg(feature = "finalization")] #[inline] #[track_caller] @@ -228,16 +215,11 @@ impl Cc { #[inline(always)] fn counter_marker(&self) -> &CounterMarker { - // SAFETY: It's always safe to access the counter_marker if we're not dereferencing anything else - unsafe { - &*addr_of!((*self.inner.as_ptr()).counter_marker) - } + &self.inner().counter_marker } - /// Note: don't call if CcOnHeap is not valid! #[inline(always)] fn inner(&self) -> &CcOnHeap { - // SAFETY: since Cc is alive and the underlying CcOnHeap is valid then we can access it unsafe { self.inner.as_ref() } } } @@ -257,7 +239,7 @@ impl Clone for Cc { remove_from_list(self.inner.cast()); - // It's always safe to clone a Cc, even if the underlying CcOnHeap is invalid + // It's always safe to clone a Cc Cc { inner: self.inner, _phantom: PhantomData, @@ -276,36 +258,31 @@ impl Deref for Cc { panic!("Cannot deref while tracing!"); } - // Make sure we don't access an invalid Cc - assert!(self.is_valid(), "This Cc is not valid!"); + //remove_from_list(self.inner.cast()); - remove_from_list(self.inner.cast()); - // We can do this since self is valid self.inner().get_elem() } } impl Drop for Cc { fn drop(&mut self) { - let counter_marker = self.counter_marker(); - // Always decrement the counter - let res = counter_marker.decrement_counter(); + let res = self.counter_marker().decrement_counter(); debug_assert!(res.is_ok()); // A CcOnHeap can be marked traced only during collections while being into a list different than POSSIBLE_CYCLES. - // In this case, or when invalid, no further action has to be taken, since the counter has been already decremented. - if counter_marker.is_traced_or_invalid() { + // In this case, no further action has to be taken, since the counter has been already decremented. + if self.counter_marker().is_traced() { return; } - if counter_marker.counter() == 0 { + if self.counter_marker().counter() == 0 { // Only us have a pointer to this allocation, deallocate! remove_from_list(self.inner.cast()); state(|state| { - let to_drop = if cfg!(feature = "finalization") && counter_marker.needs_finalization() { + let to_drop = if cfg!(feature = "finalization") && self.counter_marker().needs_finalization() { // This cfg is necessary since the cfg! above still compiles the line below, // however state doesn't contain the finalizing field when the finalization feature is off, // so removing this cfg makes the crate to fail compilation @@ -313,10 +290,10 @@ impl Drop for Cc { let _finalizing_guard = replace_state_field!(finalizing, true, state); // Set finalized - counter_marker.set_finalized(true); + self.counter_marker().set_finalized(true); self.inner().get_elem().finalize(); - counter_marker.counter() == 0 + self.counter_marker().counter() == 0 // _finalizing_guard is dropped here, resetting state.finalizing } else { true @@ -326,7 +303,7 @@ impl Drop for Cc { let _dropping_guard = replace_state_field!(dropping, true, state); let layout = self.inner().layout(); - // SAFETY: we're the only one to have a pointer to this allocation and we checked that inner is valid + // SAFETY: we're the only one to have a pointer to this allocation unsafe { drop_in_place(self.inner().elem.get()); cc_dealloc(self.inner, layout, state); @@ -335,9 +312,8 @@ impl Drop for Cc { } }); } else { - // SAFETY: we checked that inner is valid // We also know that we're not part of either root_list or non_root_list, since we haven't returned earlier - unsafe { add_to_list(self.inner.cast()) }; + add_to_list(self.inner.cast()); } } } @@ -346,14 +322,8 @@ unsafe impl Trace for Cc { #[inline] #[track_caller] fn trace(&self, ctx: &mut Context<'_>) { - // This must be done, since it is possible to call this function on an invalid instance - assert!(self.is_valid()); - - // SAFETY: we have just checked that self is valid - unsafe { - if CcOnHeap::trace(self.inner.cast(), ctx) { - self.inner().get_elem().trace(ctx); - } + if CcOnHeap::trace(self.inner.cast(), ctx) { + self.inner().get_elem().trace(ctx); } } } @@ -410,47 +380,11 @@ impl CcOnHeap { pub(crate) fn new_for_tests(t: T) -> NonNull> { CcOnHeap::new(t) } - - #[inline] - #[must_use] - fn new_invalid() -> NonNull>> { - let layout = Layout::new::>>(); - unsafe { - let ptr: NonNull>> = cc_alloc(layout); - ptr::write( - ptr.as_ptr(), - CcOnHeap { - next: UnsafeCell::new(None), - prev: UnsafeCell::new(None), - #[cfg(feature = "nightly")] - vtable: metadata(ptr.cast::>().as_ptr() as *mut dyn InternalTrace), - #[cfg(not(feature = "nightly"))] - fat_ptr: NonNull::new_unchecked( - ptr.cast::>().as_ptr() as *mut dyn InternalTrace - ), - counter_marker: { - let cm = CounterMarker::new_with_counter_to_one(); - cm.mark(Mark::Invalid); - cm - }, - _phantom: PhantomData, - elem: UnsafeCell::new(MaybeUninit::uninit()), - }, - ); - ptr - } - } } impl CcOnHeap { - #[inline] - pub(crate) fn is_valid(&self) -> bool { - self.counter_marker().is_valid() - } - #[inline] pub(crate) fn get_elem(&self) -> &T { - debug_assert!(self.is_valid()); unsafe { &*self.elem.get() } } @@ -486,10 +420,6 @@ impl CcOnHeap { unsafe impl Trace for CcOnHeap { #[inline(always)] fn trace(&self, ctx: &mut Context<'_>) { - // This should never be called on an invalid instance. - // The debug_assert should catch any bug related to this - debug_assert!(self.is_valid()); - self.get_elem().trace(ctx); } } @@ -497,55 +427,43 @@ unsafe impl Trace for CcOnHeap { impl Finalize for CcOnHeap { #[inline(always)] fn finalize(&self) { - // This should never be called on an invalid instance. - // The debug_assert should catch any bug related to this - debug_assert!(self.is_valid()); - self.get_elem().finalize(); } } #[inline] pub(crate) fn remove_from_list(ptr: NonNull>) { - unsafe { - let counter_marker = ptr.as_ref().counter_marker(); - - // Check if ptr is in possible_cycles list. Note that if ptr points to an invalid CcOnHeap<_>, - // then the if guard should never be true, since it is always marked as Mark::Invalid. - // This is also the reason why this function is not marked as unsafe. - if counter_marker.is_in_possible_cycles() { - // ptr is in the list, remove it - let _ = POSSIBLE_CYCLES.try_with(|pc| { - let mut list = pc.borrow_mut(); - // Confirm is_in_possible_cycles() in debug builds - #[cfg(feature = "pedantic-debug-assertions")] - debug_assert!(list.contains(ptr)); - - counter_marker.mark(Mark::NonMarked); - list.remove(ptr); - }); - } else { - // ptr is not in the list + let counter_marker = unsafe { ptr.as_ref() }.counter_marker(); - // Confirm !is_in_possible_cycles() in debug builds. - // This is safe to do since we're not putting the CcOnHeap into the list + // Check if ptr is in possible_cycles list + if counter_marker.is_in_possible_cycles() { + // ptr is in the list, remove it + let _ = POSSIBLE_CYCLES.try_with(|pc| { + let mut list = pc.borrow_mut(); + // Confirm is_in_possible_cycles() in debug builds #[cfg(feature = "pedantic-debug-assertions")] - debug_assert! { - POSSIBLE_CYCLES.try_with(|pc| { - !pc.borrow().contains(ptr) - }).unwrap_or(true) - }; - } + debug_assert!(list.contains(ptr)); + + counter_marker.mark(Mark::NonMarked); + list.remove(ptr); + }); + } else { + // ptr is not in the list + + // Confirm !is_in_possible_cycles() in debug builds. + // This is safe to do since we're not putting the CcOnHeap into the list + #[cfg(feature = "pedantic-debug-assertions")] + debug_assert! { + POSSIBLE_CYCLES.try_with(|pc| { + !pc.borrow().contains(ptr) + }).unwrap_or(true) + }; } } -/// SAFETY: ptr must be pointing to a valid CcOnHeap<_>. More formally, `ptr.as_ref().is_valid()` must return `true`. #[inline] -pub(crate) unsafe fn add_to_list(ptr: NonNull>) { - let counter_marker = ptr.as_ref().counter_marker(); - - // Check if ptr can be added safely - debug_assert!(counter_marker.is_valid()); +pub(crate) fn add_to_list(ptr: NonNull>) { + let counter_marker = unsafe { ptr.as_ref() }.counter_marker(); let _ = POSSIBLE_CYCLES.try_with(|pc| { let mut list = pc.borrow_mut(); @@ -577,56 +495,51 @@ pub(crate) unsafe fn add_to_list(ptr: NonNull>) { // Functions in common between every CcOnHeap<_> impl CcOnHeap<()> { - /// SAFETY: ptr must be pointing to a valid CcOnHeap<_>. More formally, `ptr.as_ref().is_valid()` must return `true`. #[inline] - pub(super) unsafe fn trace_inner(ptr: NonNull, ctx: &mut Context<'_>) { - CcOnHeap::get_traceable(ptr).as_ref().trace(ctx); + pub(super) fn trace_inner(ptr: NonNull, ctx: &mut Context<'_>) { + unsafe { + CcOnHeap::get_traceable(ptr).as_ref().trace(ctx); + } } - /// SAFETY: ptr must be pointing to a valid CcOnHeap<_>. More formally, `ptr.as_ref().is_valid()` must return `true`. #[cfg(feature = "finalization")] #[inline] - pub(super) unsafe fn finalize_inner(ptr: NonNull) -> bool { - if ptr.as_ref().counter_marker().needs_finalization() { - // Set finalized - ptr.as_ref().counter_marker().set_finalized(true); - - CcOnHeap::get_traceable(ptr).as_ref().finalize_elem(); - true - } else { - false + pub(super) fn finalize_inner(ptr: NonNull) -> bool { + unsafe { + if ptr.as_ref().counter_marker().needs_finalization() { + // Set finalized + ptr.as_ref().counter_marker().set_finalized(true); + + CcOnHeap::get_traceable(ptr).as_ref().finalize_elem(); + true + } else { + false + } } } - /// SAFETY: `drop_in_place` conditions must be true and ptr must be pointing to a valid CcOnHeap<_>. - /// More formally, `ptr.as_ref().is_valid()` must return `true`. + /// SAFETY: `drop_in_place` conditions must be true. #[inline] pub(super) unsafe fn drop_inner(ptr: NonNull) { CcOnHeap::get_traceable(ptr).as_mut().drop_elem(); } - /// SAFETY: ptr must be pointing to a valid CcOnHeap<_>. More formally, `ptr.as_ref().is_valid()` must return `true`. #[inline] - unsafe fn get_traceable(ptr: NonNull) -> NonNull { - debug_assert!(ptr.as_ref().is_valid()); // Just to be sure - + fn get_traceable(ptr: NonNull) -> NonNull { #[cfg(feature = "nightly")] - { + unsafe { let vtable = ptr.as_ref().vtable; NonNull::from_raw_parts(ptr.cast(), vtable) } #[cfg(not(feature = "nightly"))] - { + unsafe { ptr.as_ref().fat_ptr } } - /// SAFETY: ptr must be pointing to a valid CcOnHeap<_>. More formally, `ptr.as_ref().is_valid()` must return `true`. - pub(super) unsafe fn start_tracing(ptr: NonNull, ctx: &mut Context<'_>) { - debug_assert!(ptr.as_ref().is_valid()); - - let counter_marker = ptr.as_ref().counter_marker(); + pub(super) fn start_tracing(ptr: NonNull, ctx: &mut Context<'_>) { + let counter_marker = unsafe { ptr.as_ref() }.counter_marker(); match ctx.inner() { ContextInner::Counting { root_list, .. } => { // ptr is NOT into POSSIBLE_CYCLES list: ptr has just been removed from @@ -652,8 +565,6 @@ impl CcOnHeap<()> { // // This function is called from collect_cycles(), which doesn't know the // exact type of the element inside CcOnHeap, so trace it using the vtable - // - // SAFETY: we require that ptr points to a valid CcOnHeap<_> CcOnHeap::trace_inner(ptr, ctx); } @@ -662,19 +573,15 @@ impl CcOnHeap<()> { /// This function returns a `bool` instead of directly tracing the element inside the CcOnHeap, since this way /// we can avoid using the vtable most of the times (the responsibility of tracing the inner element is passed /// to the caller, which *might* have more information on the type inside CcOnHeap than us). - /// - /// SAFETY: ptr must be pointing to a valid CcOnHeap<_>. More formally, `ptr.as_ref().is_valid()` must return `true`. #[inline(never)] // Don't inline this function, it's huge #[must_use = "the element inside ptr is not traced by CcOnHeap::trace"] - unsafe fn trace(ptr: NonNull, ctx: &mut Context<'_>) -> bool { - debug_assert!(ptr.as_ref().is_valid()); - + fn trace(ptr: NonNull, ctx: &mut Context<'_>) -> bool { #[inline(always)] fn non_root(counter_marker: &CounterMarker) -> bool { counter_marker.tracing_counter() == counter_marker.counter() } - let counter_marker = ptr.as_ref().counter_marker(); + let counter_marker = unsafe { ptr.as_ref() }.counter_marker(); match ctx.inner() { ContextInner::Counting { root_list, diff --git a/src/counter_marker.rs b/src/counter_marker.rs index 0da102d..07b5c80 100644 --- a/src/counter_marker.rs +++ b/src/counter_marker.rs @@ -5,7 +5,6 @@ use crate::utils; const NON_MARKED: u32 = 0u32; const IN_POSSIBLE_CYCLES: u32 = 1u32 << (u32::BITS - 3); const TRACED: u32 = 2u32 << (u32::BITS - 3); -const INVALID: u32 = 0b111u32 << (u32::BITS - 3); const COUNTER_MASK: u32 = 0b11111111111111u32; // First 14 bits set to 1 const TRACING_COUNTER_MASK: u32 = COUNTER_MASK << 14; // 14 bits set to 1 followed by 14 bits set to 0 @@ -15,7 +14,8 @@ const FIRST_TWO_BITS_MASK: u32 = 3u32 << (u32::BITS - 2); const INITIAL_VALUE: u32 = COUNTER_MASK + 2; // +2 means that tracing counter and counter are both set to 1 -const MAX: u32 = COUNTER_MASK; +// pub(crate) to make it available in tests +pub(crate) const MAX: u32 = COUNTER_MASK; /// Internal representation: /// ```text @@ -28,7 +28,6 @@ const MAX: u32 = COUNTER_MASK; /// * `NON_MARKED` /// * `IN_POSSIBLE_CYCLES` (this implies `NON_MARKED`) /// * `TRACED` -/// * `INVALID` (`CcOnHeap` is invalid) /// * `B` is `1` when the element inside `CcOnHeap` has already been finalized, `0` otherwise /// * `C` is the tracing counter /// * `D` is the counter (last one for sum/subtraction efficiency) @@ -136,24 +135,11 @@ impl CounterMarker { (self.counter.get() & FIRST_TWO_BITS_MASK) == 0u32 } - /// Returns whether this CounterMarker is traced or not valid (exclusive or). - #[inline] - pub(crate) fn is_traced_or_invalid(&self) -> bool { - // true if (self.counter & BITS_MASK) is equal to 010, 011, 100, 101 or 111, - // so if the first two bits aren't both 0 - (self.counter.get() & FIRST_TWO_BITS_MASK) != 0u32 - } - #[inline] pub(crate) fn is_traced(&self) -> bool { (self.counter.get() & BITS_MASK) == TRACED } - #[inline] - pub(crate) fn is_valid(&self) -> bool { - (self.counter.get() & BITS_MASK) != INVALID - } - #[inline] pub(crate) fn mark(&self, new_mark: Mark) { self.counter.set((self.counter.get() & !BITS_MASK) | (new_mark as u32)); @@ -166,5 +152,4 @@ pub(crate) enum Mark { NonMarked = NON_MARKED, PossibleCycles = IN_POSSIBLE_CYCLES, Traced = TRACED, - Invalid = INVALID, } diff --git a/src/lib.rs b/src/lib.rs index fb40ad6..4f4c108 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -98,11 +98,7 @@ fn __collect(state: &State) { while let Some(ptr) = POSSIBLE_CYCLES.with(|pc| pc.borrow_mut().remove_first()) { // remove_first already marks ptr as NonMarked - - // SAFETY: ptr comes from POSSIBLE_CYCLES list, so it is surely valid since lists contain only pointers to valid CcOnHeap<_> - unsafe { - trace_counting(ptr, &mut root_list, &mut non_root_list); - } + trace_counting(ptr, &mut root_list, &mut non_root_list); } trace_roots(root_list, &mut non_root_list); @@ -110,8 +106,8 @@ fn __collect(state: &State) { if !non_root_list.is_empty() { #[cfg(feature = "pedantic-debug-assertions")] - non_root_list.iter().for_each(|ptr| unsafe { - let counter_marker = ptr.as_ref().counter_marker(); + non_root_list.iter().for_each(|ptr| { + let counter_marker = unsafe { ptr.as_ref() }.counter_marker(); debug_assert_eq!( counter_marker.tracing_counter(), @@ -122,15 +118,12 @@ fn __collect(state: &State) { #[cfg(feature = "finalization")] { - let mut has_finalized = false; + let has_finalized: bool; { let _finalizing_guard = replace_state_field!(finalizing, true, state); - non_root_list.iter().for_each(|ptr| { - // SAFETY: ptr comes from non_root_list, so it is surely valid since lists contain only pointers to valid CcOnHeap<_> - if unsafe { CcOnHeap::finalize_inner(ptr.cast()) } { - has_finalized = true; - } + has_finalized = non_root_list.iter().fold(false, |has_finalized, ptr| { + CcOnHeap::finalize_inner(ptr.cast()) | has_finalized }); // _finalizing_guard is dropped here, resetting state.finalizing @@ -170,8 +163,7 @@ fn deallocate_list(to_deallocate_list: List, state: &State) { // Drop every CcOnHeap before deallocating them (see comment below) to_deallocate_list.iter().for_each(|ptr| { - // SAFETY: ptr comes from non_root_list, so it is surely valid since lists contain only pointers to valid CcOnHeap<_>. - // Also, it's valid to drop in place ptr + // SAFETY: ptr is valid to access and drop in place unsafe { debug_assert!(ptr.as_ref().counter_marker().is_traced()); @@ -198,8 +190,7 @@ fn deallocate_list(to_deallocate_list: List, state: &State) { // _dropping_guard is dropped here, resetting state.dropping } -/// SAFETY: ptr must be pointing to a valid CcOnHeap<_>. More formally, `ptr.as_ref().is_valid()` must return `true`. -unsafe fn trace_counting( +fn trace_counting( ptr: NonNull>, root_list: &mut List, non_root_list: &mut List, @@ -209,17 +200,13 @@ unsafe fn trace_counting( non_root_list, }); - // SAFETY: ptr is required to be valid CcOnHeap::start_tracing(ptr, &mut ctx); } fn trace_roots(mut root_list: List, non_root_list: &mut List) { while let Some(ptr) = root_list.remove_first() { let mut ctx = Context::new(ContextInner::RootTracing { non_root_list, root_list: &mut root_list }); - // SAFETY: ptr comes from a list, so it is surely valid since lists contain only pointers to valid CcOnHeap<_> - unsafe { - CcOnHeap::start_tracing(ptr, &mut ctx); - } + CcOnHeap::start_tracing(ptr, &mut ctx); } mem::forget(root_list); // root_list is empty, no need run List::drop diff --git a/src/list.rs b/src/list.rs index 8ebad66..6bf34d2 100644 --- a/src/list.rs +++ b/src/list.rs @@ -21,9 +21,6 @@ impl List { #[inline] pub(crate) fn add(&mut self, ptr: NonNull>) { - // Check if ptr can be added safely - unsafe { debug_assert!(ptr.as_ref().is_valid()) }; - if let Some(first) = &mut self.first { unsafe { *first.as_ref().get_prev() = Some(ptr); @@ -43,10 +40,6 @@ impl List { #[inline] pub(crate) fn remove(&mut self, ptr: NonNull>) { - // Make sure ptr is valid. Since a pointer to an invalid CcOnHeap<_> cannot - // be added to any list, if ptr is invalid then this fn shouldn't have been called - unsafe { debug_assert!(ptr.as_ref().is_valid()) }; - // Remove from possible_cycles list unsafe { match (*ptr.as_ref().get_next(), *ptr.as_ref().get_prev()) { diff --git a/src/tests/cc.rs b/src/tests/cc.rs index 81e0b96..67f731a 100644 --- a/src/tests/cc.rs +++ b/src/tests/cc.rs @@ -1,6 +1,4 @@ use std::mem::MaybeUninit; -use std::ops::Deref; -use std::panic::{catch_unwind, AssertUnwindSafe}; use super::*; use crate::*; @@ -223,7 +221,7 @@ fn test_trait_object() { ); } -#[test] +/*#[test] fn test_cyclic() { reset_state(); @@ -282,14 +280,14 @@ fn test_cyclic() { drop(cc); collect_cycles(); -} +}*/ #[test] fn test_cyclic_finalization_aliasing() { reset_state(); struct Circular { - cc: Cc, + cc: RefCell>>, } unsafe impl Trace for Circular { @@ -304,15 +302,20 @@ fn test_cyclic_finalization_aliasing() { #[allow(unused_comparisons)] fn finalize(&self) { // The scope of this comparison is to recursively access the same allocation during finalization - assert!(self.cc.cc.cc.cc.cc.strong_count() >= 0); + assert!(self.cc.borrow().as_ref().unwrap().cc.borrow().as_ref().unwrap().cc.borrow().as_ref().unwrap().cc.borrow().as_ref().unwrap().cc.borrow().as_ref().unwrap().strong_count() >= 0); } } - let _ = Cc::::new_cyclic(|cc| Circular { - cc: Cc::new(Circular { - cc: cc.clone(), - }), - }); + { + let cc = Cc::new(Circular { + cc: RefCell::new(None), + }); + + *cc.cc.borrow_mut() = Some(Cc::new(Circular { + cc: RefCell::new(Some(cc.clone())), + })); + } + collect_cycles(); } @@ -321,7 +324,7 @@ fn test_self_loop_finalization_aliasing() { reset_state(); struct Circular { - cc: Cc, + cc: RefCell>>, } unsafe impl Trace for Circular { @@ -336,13 +339,17 @@ fn test_self_loop_finalization_aliasing() { #[allow(unused_comparisons)] fn finalize(&self) { // The scope of this comparison is to recursively access the same allocation during finalization - assert!(self.cc.cc.cc.cc.strong_count() >= 0); + assert!(self.cc.borrow().as_ref().unwrap().cc.borrow().as_ref().unwrap().cc.borrow().as_ref().unwrap().cc.borrow().as_ref().unwrap().strong_count() >= 0); } } - let _ = Cc::::new_cyclic(|cc| Circular { - cc: cc.clone(), - }); + { + let cc = Cc::new(Circular { + cc: RefCell::new(None), + }); + *cc.cc.borrow_mut() = Some(cc.clone()); + } + collect_cycles(); } diff --git a/src/tests/counter_marker.rs b/src/tests/counter_marker.rs index 0bbb889..8ae907c 100644 --- a/src/tests/counter_marker.rs +++ b/src/tests/counter_marker.rs @@ -1,19 +1,11 @@ use crate::counter_marker::*; -// This const is used only in a part of test_increment_decrement() which is -// disabled under MIRI (since it is very slow and doesn't really use unsafe). -// Thus, the cfg attribute removes the constant when running under MIRI to -// disable the "unused const" warning -#[cfg(not(miri))] -const MAX: u32 = 0b11111111111111; // 14 ones - #[test] fn test_new() { fn test(counter: CounterMarker) { assert!(counter.is_not_marked()); assert!(!counter.is_in_possible_cycles()); assert!(!counter.is_traced()); - assert!(counter.is_valid()); assert_eq!(counter.counter(), 1); assert_eq!(counter.tracing_counter(), 1); @@ -44,7 +36,6 @@ fn test_increment_decrement() { assert!(counter.is_not_marked()); assert!(!counter.is_in_possible_cycles()); assert!(!counter.is_traced()); - assert!(counter.is_valid()); } assert_not_marked(&counter); @@ -121,42 +112,30 @@ fn test_marks() { assert!(counter.is_not_marked()); assert!(!counter.is_in_possible_cycles()); assert!(!counter.is_traced()); - assert!(counter.is_valid()); counter.mark(Mark::NonMarked); assert!(counter.is_not_marked()); assert!(!counter.is_in_possible_cycles()); assert!(!counter.is_traced()); - assert!(counter.is_valid()); counter.mark(Mark::PossibleCycles); assert!(counter.is_not_marked()); assert!(counter.is_in_possible_cycles()); assert!(!counter.is_traced()); - assert!(counter.is_valid()); counter.mark(Mark::Traced); assert!(!counter.is_not_marked()); assert!(!counter.is_in_possible_cycles()); assert!(counter.is_traced()); - assert!(counter.is_valid()); counter.mark(Mark::NonMarked); assert!(counter.is_not_marked()); assert!(!counter.is_in_possible_cycles()); assert!(!counter.is_traced()); - assert!(counter.is_valid()); - - counter.mark(Mark::Invalid); - - assert!(!counter.is_not_marked()); - assert!(!counter.is_in_possible_cycles()); - assert!(!counter.is_traced()); - assert!(!counter.is_valid()); } test(CounterMarker::new_with_counter_to_one()); diff --git a/src/tests/list.rs b/src/tests/list.rs index 7367942..138a223 100644 --- a/src/tests/list.rs +++ b/src/tests/list.rs @@ -151,7 +151,6 @@ fn test_for_each_clearing_panic() { let counter_marker = counter_marker(it); - assert!(counter_marker.is_valid()); assert!(counter_marker.is_not_marked()); assert!(!counter_marker.is_in_possible_cycles()); } diff --git a/src/tests/panicking.rs b/src/tests/panicking.rs index 9fa6b5d..fc263c0 100644 --- a/src/tests/panicking.rs +++ b/src/tests/panicking.rs @@ -1,10 +1,10 @@ -use std::cell::Cell; +use std::cell::{Cell, RefCell}; use std::mem; use crate::tests::{assert_state_not_collecting, reset_state}; use crate::{collect_cycles, Cc, Context, Finalize, Trace}; -fn register_panicking(cc: &Cc) { +fn register_panicking(#[allow(unused_variables)] cc: &Cc) { // The unused_variables warning is triggered when not running on Miri #[cfg(miri)] extern "Rust" { /// From Miri documentation:
@@ -14,8 +14,6 @@ fn register_panicking(cc: &Cc) { fn miri_static_root(ptr: *const u8); } - assert!(cc.is_valid()); - #[cfg(miri)] // Use miri_static_root to avoid failures caused by leaks. Leaks are expected, // since this module tests panics (which leaks memory to prevent UB) @@ -56,7 +54,7 @@ struct Panicking { panic_on_trace: Cell, panic_on_finalize: Cell, panic_on_drop: Cell, - cc: Cc, + cc: RefCell>>, } unsafe impl Trace for Panicking { @@ -95,14 +93,15 @@ fn test_panicking_tracing_counting() { reset_state(); { - register_panicking(&Cc::::new_cyclic(|cc| Panicking { + let cc = Cc::new(Panicking { trace_counter: Cell::new(0), panic_on_trace: Cell::new(true), panic_on_finalize: Cell::new(false), panic_on_drop: Cell::new(false), - cc: cc.clone(), - })); - + cc: RefCell::new(None), + }); + *cc.cc.borrow_mut() = Some(cc.clone()); + register_panicking(&cc); } panicking_collect_cycles(assert_state_not_collecting); } @@ -114,21 +113,22 @@ fn test_panicking_tracing_root() { // Leave a root alive to trigger root tracing let _root = { - let root = Cc::::new_cyclic(|cc| Panicking { + let root = Cc::new(Panicking { trace_counter: Cell::new(1), panic_on_trace: Cell::new(true), panic_on_finalize: Cell::new(false), panic_on_drop: Cell::new(false), - cc: Cc::new(Panicking { - trace_counter: Cell::new(usize::MAX), - panic_on_trace: Cell::new(false), - panic_on_finalize: Cell::new(false), - panic_on_drop: Cell::new(false), - cc: cc.clone(), - }), + cc: RefCell::new(None), }); + *root.cc.borrow_mut() = Some(Cc::new(Panicking { + trace_counter: Cell::new(usize::MAX), + panic_on_trace: Cell::new(false), + panic_on_finalize: Cell::new(false), + panic_on_drop: Cell::new(false), + cc: RefCell::new(Some(root.clone())), + })); register_panicking(&root); - register_panicking(&root.cc); + register_panicking(root.cc.borrow().as_ref().unwrap()); #[allow(clippy::redundant_clone)] root.clone() }; @@ -141,13 +141,15 @@ fn test_panicking_finalize() { reset_state(); { - register_panicking(&Cc::::new_cyclic(|cc| Panicking { + let cc = Cc::new(Panicking { trace_counter: Cell::new(usize::MAX), panic_on_trace: Cell::new(false), panic_on_finalize: Cell::new(true), panic_on_drop: Cell::new(false), - cc: cc.clone(), - })); + cc: RefCell::new(None), + }); + *cc.cc.borrow_mut() = Some(cc.clone()); + register_panicking(&cc); } panicking_collect_cycles(assert_state_not_collecting); } @@ -159,7 +161,7 @@ fn test_panicking_drop() { // Cannot use Panicking since Panicking implements Finalize struct DropPanicking { - cyclic: Cc, + cyclic: RefCell>>, } unsafe impl Trace for DropPanicking { @@ -178,7 +180,11 @@ fn test_panicking_drop() { } { - register_panicking(&Cc::::new_cyclic(|cc| DropPanicking { cyclic: cc.clone() })); + let cc = Cc::new(DropPanicking { + cyclic: RefCell::new(None), + }); + *cc.cyclic.borrow_mut() = Some(cc.clone()); + register_panicking(&cc); } panicking_collect_cycles(assert_state_not_collecting); } @@ -189,13 +195,15 @@ fn test_panicking_drop_and_finalize() { reset_state(); { - register_panicking(&Cc::::new_cyclic(|cc| Panicking { + let cc = Cc::new(Panicking { trace_counter: Cell::new(usize::MAX), panic_on_trace: Cell::new(false), panic_on_finalize: Cell::new(false), panic_on_drop: Cell::new(true), - cc: cc.clone(), - })); + cc: RefCell::new(None), + }); + *cc.cc.borrow_mut() = Some(cc.clone()); + register_panicking(&cc); } panicking_collect_cycles(assert_state_not_collecting); } @@ -213,22 +221,23 @@ fn test_panicking_tracing_drop() { // call will panic, which is during dropping tracing const CELL_VALUE: usize = 1; - let root = Cc::::new_cyclic(|cc| Panicking { + let root = Cc::new(Panicking { trace_counter: Cell::new(CELL_VALUE), panic_on_trace: Cell::new(true), panic_on_finalize: Cell::new(false), panic_on_drop: Cell::new(false), - cc: Cc::new(Panicking { - trace_counter: Cell::new(CELL_VALUE), - panic_on_trace: Cell::new(true), - panic_on_finalize: Cell::new(false), - panic_on_drop: Cell::new(false), - cc: cc.clone(), - }), + cc: RefCell::new(None), }); + *root.cc.borrow_mut() = Some(Cc::new(Panicking { + trace_counter: Cell::new(CELL_VALUE), + panic_on_trace: Cell::new(true), + panic_on_finalize: Cell::new(false), + panic_on_drop: Cell::new(false), + cc: RefCell::new(Some(root.clone())), + })); register_panicking(&root); - register_panicking(&root.cc); + register_panicking(root.cc.borrow().as_ref().unwrap()); } panicking_collect_cycles(assert_state_not_collecting); } @@ -254,7 +263,7 @@ fn test_panicking_tracing_resurrecting() { if let Some(replaced) = RESURRECTED.with(|cell| cell.replace(None)) { reset_panicking(&replaced); - reset_panicking(&replaced.cc); + reset_panicking(replaced.cc.borrow().as_ref().unwrap()); // replaced is dropped here } collect_cycles(); // Reclaim memory @@ -265,7 +274,7 @@ fn test_panicking_tracing_resurrecting() { struct Resurrecter { panicking: Cc, - cyclic: Cc, + cyclic: RefCell>>, } unsafe impl Trace for Resurrecter { @@ -282,25 +291,28 @@ fn test_panicking_tracing_resurrecting() { } { - let a = Cc::::new_cyclic(|cc_res| Resurrecter { - panicking: Cc::::new_cyclic(|cc| Panicking { + let a = Cc::new(Resurrecter { + panicking: Cc::new(Panicking { trace_counter: Cell::new(2), panic_on_trace: Cell::new(true), panic_on_finalize: Cell::new(false), panic_on_drop: Cell::new(false), - cc: Cc::new(Panicking { - trace_counter: Cell::new(2), - panic_on_trace: Cell::new(true), - panic_on_finalize: Cell::new(false), - panic_on_drop: Cell::new(false), - cc: cc.clone(), - }), + cc: RefCell::new(None), }), - cyclic: cc_res.clone(), + cyclic: RefCell::new(None), }); + *a.panicking.cc.borrow_mut() = Some(Cc::new(Panicking { + trace_counter: Cell::new(2), + panic_on_trace: Cell::new(true), + panic_on_finalize: Cell::new(false), + panic_on_drop: Cell::new(false), + cc: RefCell::new(Some(a.panicking.clone())), + })); + *a.cyclic.borrow_mut() = Some(a.clone()); + register_panicking(&a); register_panicking(&a.panicking); - register_panicking(&a.panicking.cc); + register_panicking(a.panicking.cc.borrow().as_ref().unwrap()); drop(a); } diff --git a/tests/auto_collect.rs b/tests/auto_collect.rs index e7551da..5017e48 100644 --- a/tests/auto_collect.rs +++ b/tests/auto_collect.rs @@ -1,49 +1,59 @@ #![cfg(feature = "auto-collect")] -use rust_cc::state::executions_count; -use rust_cc::{collect_cycles, Cc, Context, Finalize, Trace}; +use std::cell::RefCell; + +use rust_cc::{Cc, collect_cycles, Context, Finalize, Trace}; use rust_cc::config::config; +use rust_cc::state::executions_count; -#[test] -fn test_auto_collect() { - struct Traceable { - inner: Option>, - _big: Big, - } +struct Traceable { + inner: RefCell>>, + _big: Big, +} - struct Big { - _array: [i64; 4096], - } +struct Big { + _array: [i64; 4096], +} - impl Default for Big { - fn default() -> Self { - Big { _array: [0; 4096] } - } +impl Default for Big { + fn default() -> Self { + Big { _array: [0; 4096] } } +} - unsafe impl Trace for Traceable { - fn trace(&self, ctx: &mut Context<'_>) { - self.inner.trace(ctx); - } +unsafe impl Trace for Traceable { + fn trace(&self, ctx: &mut Context<'_>) { + self.inner.trace(ctx); } +} - impl Finalize for Traceable {} +impl Finalize for Traceable {} - { - let _traceable = Cc::::new_cyclic(|cc| Traceable { - inner: Some(Cc::new(Traceable { - inner: Some(cc.clone()), - _big: Default::default(), - })), +impl Traceable { + fn new() -> Cc { + let traceable = Cc::new(Traceable { + inner: RefCell::new(None), _big: Default::default(), }); + *traceable.inner.borrow_mut() = Some(Cc::new(Traceable { + inner: RefCell::new(Some(traceable.clone())), + _big: Default::default(), + })); + traceable + } +} + +#[test] +fn test_auto_collect() { + { + let traceable = Traceable::new(); let executions_counter = executions_count().unwrap(); - drop(_traceable); + drop(traceable); assert_eq!(executions_counter, executions_count().unwrap(), "Collected but shouldn't have collected."); let _ = Cc::new(Traceable { - inner: None, + inner: RefCell::new(None), _big: Default::default(), }); // Collection should be triggered by allocations assert_ne!(executions_counter, executions_count().unwrap(), "Didn't collected"); @@ -64,45 +74,16 @@ fn test_disable_auto_collect() { } let _drop_guard = DropGuard; - struct Traceable { - inner: Option>, - _big: Big, - } - - struct Big { - _array: [i64; 4096], - } - - impl Default for Big { - fn default() -> Self { - Big { _array: [0; 4096] } - } - } - - unsafe impl Trace for Traceable { - fn trace(&self, ctx: &mut Context<'_>) { - self.inner.trace(ctx); - } - } - - impl Finalize for Traceable {} - { let executions_counter = executions_count().unwrap(); - let _traceable = Cc::::new_cyclic(|cc| Traceable { - inner: Some(Cc::new(Traceable { - inner: Some(cc.clone()), - _big: Default::default(), - })), - _big: Default::default(), - }); + let traceable = Traceable::new(); assert_eq!(executions_counter, executions_count().unwrap(), "Collected but shouldn't have collected."); - drop(_traceable); + drop(traceable); assert_eq!(executions_counter, executions_count().unwrap(), "Collected but shouldn't have collected."); let _ = Cc::new(Traceable { - inner: None, + inner: RefCell::new(None), _big: Default::default(), }); // Collection should be triggered by allocations assert_eq!(executions_counter, executions_count().unwrap(), "Collected but shouldn't have collected."); diff --git a/tests/cc.rs b/tests/cc.rs index 31f3afa..4c42710 100644 --- a/tests/cc.rs +++ b/tests/cc.rs @@ -15,7 +15,7 @@ fn test_complex() { struct C { a: RefCell>>, - b: Cc, + b: RefCell>>, } struct D { @@ -57,14 +57,16 @@ fn test_complex() { impl Finalize for C {} - let a = Cc::::new_cyclic(|a| A { - b: Cc::::new_cyclic(|b| B { + let a = Cc::new(A { + b: Cc::new(B { c: Cc::new(C { - a: RefCell::new(Some(a.clone())), - b: b.clone(), + a: RefCell::new(None), + b: RefCell::new(None), }), }), }); + *a.b.c.a.borrow_mut() = Some(a.clone()); + *a.b.c.b.borrow_mut() = Some(a.b.clone()); let d = Cc::new(D { c: a.b.c.clone() }); drop(a); collect_cycles(); @@ -75,7 +77,7 @@ fn test_complex() { collect_cycles(); } -#[test] +/*#[test] fn useless_cyclic() { let _cc = Cc::::new_cyclic(|_| { collect_cycles(); @@ -88,7 +90,7 @@ fn useless_cyclic() { collect_cycles(); drop(_cc); collect_cycles(); -} +}*/ #[cfg(feature = "finalization")] #[test] @@ -175,14 +177,15 @@ fn test_finalization() { a: RefCell::new(None), }); - let a = Cc::::new_cyclic(|a| A { + let a = Cc::new(A { dropped: Cell::new(false), c: Rc::downgrade(&c1), - b: RefCell::new(Some(Cc::new(B { - dropped: Cell::new(false), - a: a.clone(), - }))), + b: RefCell::new(None), }); + *a.b.borrow_mut() = Some(Cc::new(B { + dropped: Cell::new(false), + a: a.clone(), + })); assert_eq!(a.strong_count(), 2); @@ -318,7 +321,7 @@ fn test_finalize_drop() { _a: RefCell::new(Some(A { dropped: Cell::new(false), _c: weak.clone(), - b: RefCell::new(Some(Cc::new_cyclic(|_| B { + b: RefCell::new(Some(Cc::new(B { dropped: Cell::new(false), a: Cc::new(A { dropped: Cell::new(false), @@ -348,7 +351,7 @@ fn test_finalize_drop() { #[test] fn finalization_test() { struct Circular { - next: RefCell>, + next: RefCell>>, does_finalization: Cell, } @@ -361,23 +364,24 @@ fn finalization_test() { impl Finalize for Circular { fn finalize(&self) { if self.does_finalization.get() { - *self.next.borrow_mut() = Cc::new(Circular { + *self.next.borrow_mut() = Some(Cc::new(Circular { next: self.next.clone(), does_finalization: Cell::new(false), - }); + })); self.does_finalization.set(false); } } } { - let _ = Cc::::new_cyclic(|cc| Circular { - next: RefCell::new(Cc::new(Circular { - next: RefCell::new(cc.clone()), + let cc = Cc::new(Circular { + next: RefCell::new(Some(Cc::new(Circular { + next: RefCell::new(None), does_finalization: Cell::new(false), - })), + }))), does_finalization: Cell::new(true), }); + *cc.next.borrow().as_ref().unwrap().next.borrow_mut() = Some(cc.clone()); } collect_cycles(); } @@ -404,9 +408,10 @@ fn rc_test() { #[test] fn box_test() { { - let _cc = Cc::>::new_cyclic(|cc| { - Box::new(Cc::new(cc.clone())) as Box - }); + let cc = Cc::new(RefCell::new(None)); + { + *cc.borrow_mut() = Some(Box::new(cc.clone()) as Box); + } } collect_cycles(); @@ -419,7 +424,7 @@ fn alignment_test() { $({ #[repr(align($i))] struct A { - cc: Cc, + cc: RefCell>>, } unsafe impl Trace for A { @@ -431,9 +436,10 @@ fn alignment_test() { impl Finalize for A {} fn use_struct() { - let _ = Cc::::new_cyclic(|cc| A { - cc: cc.clone(), + let cc = Cc::new(A { + cc: RefCell::new(None), }); + *cc.cc.borrow_mut() = Some(cc.clone()); } use_struct(); })+