Re: [PATCH v2 2/9] rust: list: add tracking for ListArc

From: Benno Lossin
Date: Mon May 27 2024 - 05:42:04 EST


On 06.05.24 11:53, Alice Ryhl wrote:
> @@ -32,9 +33,24 @@ pub trait ListArcSafe<const ID: u64 = 0> {
> unsafe fn on_drop_list_arc(&self);
> }
>
> +/// Declares that this type is able to safely attempt to create `ListArc`s at any time.
> +///
> +/// # Safety
> +///
> +/// Implementers must ensure that `try_new_list_arc` does not return `true` if a `ListArc` already
> +/// exists.
> +pub unsafe trait TryNewListArc<const ID: u64 = 0>: ListArcSafe<ID> {
> + /// Attempts to convert an `Arc<Self>` into an `ListArc<Self>`. Returns `true` if the
> + /// conversion was successful.
> + fn try_new_list_arc(&self) -> bool;
> +}
> +
> /// Declares that this type supports [`ListArc`].
> ///
> -/// When using this macro, it will only be possible to create a [`ListArc`] from a [`UniqueArc`].
> +/// When using this macro, you may choose between the `untracked` strategy where it is not tracked
> +/// whether a [`ListArc`] exists, and the `tracked_by` strategy where the tracking is deferred to a
> +/// field of the struct. The `tracked_by` strategy can be combined with a field of type
> +/// [`AtomicListArcTracker`] to track whether a [`ListArc`] exists.

I think it would make sense to use bullet points here.
Also, you should mention that in the `tracked_by` strategy, the field is
required to implement `TryNewListArc`.

> #[macro_export]
> macro_rules! impl_list_arc_safe {
> (impl$({$($generics:tt)*})? ListArcSafe<$num:tt> for $t:ty { untracked; } $($rest:tt)*) => {
> @@ -45,6 +61,37 @@ unsafe fn on_drop_list_arc(&self) {}
> $crate::list::impl_list_arc_safe! { $($rest)* }
> };
>
> + (impl$({$($generics:tt)*})? ListArcSafe<$num:tt> for $t:ty {
> + tracked_by $field:ident : $fty:ty;
> + } $($rest:tt)*) => {
> + impl$(<$($generics)*>)? $crate::list::ListArcSafe<$num> for $t {
> + unsafe fn on_create_list_arc_from_unique(self: ::core::pin::Pin<&mut Self>) {
> + // SAFETY: This field is structurally pinned.

Who ensures this? This is not documented on the macro.
The only way that I see to fix this would be to make the `tracked_by`
strategy `unsafe`. At least until we implement proper structural pinning
of fields.

> + let field = unsafe {
> + ::core::pin::Pin::map_unchecked_mut(self, |me| &mut me.$field)
> + };
> + // SAFETY: The caller promises that there is no `ListArc`.
> + unsafe {
> + <$fty as $crate::list::ListArcSafe<$num>>::on_create_list_arc_from_unique(field)
> + };
> + }
> + unsafe fn on_drop_list_arc(&self) {
> + // SAFETY: The caller promises that there is no `ListArc` reference, and also
> + // promises that the tracking thinks there is a `ListArc` reference.
> + unsafe { <$fty as $crate::list::ListArcSafe<$num>>::on_drop_list_arc(&self.$field) };
> + }
> + }
> + unsafe impl$(<$($generics)*>)? $crate::list::TryNewListArc<$num> for $t
> + where
> + $fty: TryNewListArc<$num>,
> + {
> + fn try_new_list_arc(&self) -> bool {
> + <$fty as $crate::list::TryNewListArc<$num>>::try_new_list_arc(&self.field)
> + }
> + }
> + $crate::list::impl_list_arc_safe! { $($rest)* }
> + };
> +
> () => {};
> }
> pub use impl_list_arc_safe;

[...]

> @@ -313,3 +406,60 @@ impl<T, U, const ID: u64> core::ops::DispatchFromDyn<ListArc<U, ID>> for ListArc
> U: ListArcSafe<ID> + ?Sized,
> {
> }
> +
> +/// A utility for tracking whether a [`ListArc`] exists using an atomic.
> +///
> +/// # Invariant
> +///
> +/// If the boolean is `false`, then there is no [`ListArc`] for this value.

"If `inner` is `false`, ..."

---
Cheers,
Benno

> +#[repr(transparent)]
> +pub struct AtomicListArcTracker<const ID: u64 = 0> {
> + inner: AtomicBool,
> + _pin: PhantomPinned,
> +}

[...]