From 50b60dc85c77ad7f027b6989fde2bcb14cdc8a72 Mon Sep 17 00:00:00 2001 From: Shahar Naveh <50263213+ShaharNaveh@users.noreply.github.com> Date: Sat, 22 Aug 2026 14:54:18 +0300 Subject: [PATCH] Remove `#[repr(C)]` from LinkedList (#8575) * fix doc * drop `repr(C)]` * remove needless generics from LinkedList * clippy --- crates/common/src/linked_list.rs | 190 ++++++++++++++----------------- crates/vm/src/gc_state.rs | 11 +- 2 files changed, 93 insertions(+), 108 deletions(-) diff --git a/crates/common/src/linked_list.rs b/crates/common/src/linked_list.rs index e8d362470..75e0b5234 100644 --- a/crates/common/src/linked_list.rs +++ b/crates/common/src/linked_list.rs @@ -38,10 +38,15 @@ //! specified node is actually contained by the list. #![allow(clippy::new_without_default, clippy::missing_safety_doc)] +// It doesn't make sense to enforce `unsafe_op_in_unsafe_fn` for this module because +// +// * The intrusive linked list naturally relies on unsafe operations. +// * Excessive `unsafe {}` blocks hurt readability significantly. +#![expect(unsafe_op_in_unsafe_fn)] use core::cell::UnsafeCell; use core::fmt; -use core::marker::{PhantomData, PhantomPinned}; +use core::marker::PhantomPinned; use core::mem::ManuallyDrop; use core::ptr::{self, NonNull}; @@ -49,18 +54,15 @@ use core::ptr::{self, NonNull}; /// /// Currently, the list is not emptied on drop. It is the caller's /// responsibility to ensure the list is empty before dropping it. -pub struct LinkedList { +pub struct LinkedList { /// Linked list head - head: Option>, - + head: Option>, // /// Linked list tail // tail: Option>, - /// Node type marker. - _marker: PhantomData<*const L>, } -unsafe impl Send for LinkedList where L::Target: Send {} -unsafe impl Sync for LinkedList where L::Target: Sync {} +unsafe impl Send for LinkedList where L::Target: Send {} +unsafe impl Sync for LinkedList where L::Target: Sync {} /// Defines how a type is tracked within a linked list. /// @@ -82,13 +84,21 @@ pub unsafe trait Link { type Target; /// Convert the handle to a raw pointer without consuming the handle. - #[allow(clippy::wrong_self_convention)] fn as_raw(handle: &Self::Handle) -> NonNull; /// Convert the raw pointer to a handle unsafe fn from_raw(ptr: NonNull) -> Self::Handle; /// Return the pointers for a node + /// + /// # Safety + /// + /// The resulting pointer should have the same tag in the stacked-borrows + /// stack as the argument. In particular, the method may not create an + /// intermediate reference in the process of creating the resulting raw + /// pointer. + /// + /// The `target` pointer must be valid. unsafe fn pointers(target: NonNull) -> NonNull>; } @@ -96,6 +106,7 @@ pub unsafe trait Link { pub struct Pointers { inner: UnsafeCell>, } + /// We do not want the compiler to put the `noalias` attribute on mutable /// references to this type, so the type has been made `!Unpin` with a /// `PhantomPinned` field. @@ -103,26 +114,19 @@ pub struct Pointers { /// Additionally, we never access the `prev` or `next` fields directly, as any /// such access would implicitly involve the creation of a reference to the /// field, which we want to avoid since the fields are not `!Unpin`, and would -/// hence be given the `noalias` attribute if we were to do such an access. -/// As an alternative to accessing the fields directly, the `Pointers` type +/// hence be given the `noalias` attribute if we were to do such an access. As +/// an alternative to accessing the fields directly, the [`Pointers`] type /// provides getters and setters for the two fields, and those are implemented -/// using raw pointer casts and offsets, which is valid since the struct is -/// #[repr(C)]. +/// using `ptr`-specific methods which avoids the creation of intermediate +/// references. /// /// See this link for more information: /// -#[repr(C)] struct PointersInner { - /// The previous node in the list. null if there is no previous node. - /// - /// This field is accessed through pointer manipulation, so it is not dead code. - #[allow(dead_code)] + /// The previous node in the list. [`None`] if there is no previous node. prev: Option>, - /// The next node in the list. null if there is no previous node. - /// - /// This field is accessed through pointer manipulation, so it is not dead code. - #[allow(dead_code)] + /// The next node in the list. [`None`] if there is no previous node. next: Option>, /// This type is !Unpin due to the heuristic from: @@ -130,27 +134,21 @@ struct PointersInner { _pin: PhantomPinned, } -unsafe impl Send for PointersInner {} -unsafe impl Sync for PointersInner {} - unsafe impl Send for Pointers {} unsafe impl Sync for Pointers {} // ===== impl LinkedList ===== -impl LinkedList { +impl LinkedList { /// Creates an empty linked list. #[must_use] pub const fn new() -> Self { Self { head: None, // tail: None, - _marker: PhantomData, } } -} -impl LinkedList { /// Adds an element first in the list. pub fn push_front(&mut self, val: L::Handle) { // The value should not be dropped, it is being inserted into the list @@ -167,6 +165,7 @@ impl LinkedList { L::pointers(ptr).as_ref().get_next().is_none(), "push_front: node already has next pointer (double-insert?)" ); + L::pointers(ptr).as_mut().set_next(self.head); L::pointers(ptr).as_mut().set_prev(None); @@ -210,6 +209,7 @@ impl LinkedList { if let Some(new_head) = self.head { L::pointers(new_head).as_mut().set_prev(None); } + L::pointers(head).as_mut().set_next(None); L::pointers(head).as_mut().set_prev(None); Some(L::from_raw(head)) @@ -235,47 +235,45 @@ impl LinkedList { /// The caller **must** ensure that `node` is currently contained by /// `self` or not contained by any other list. pub unsafe fn remove(&mut self, node: NonNull) -> Option { - unsafe { - if let Some(prev) = L::pointers(node).as_ref().get_prev() { - debug_assert_eq!( - L::pointers(prev).as_ref().get_next(), - Some(node), - "linked list corruption: prev->next != node (prev={prev:p}, node={node:p})" - ); - L::pointers(prev) - .as_mut() - .set_next(L::pointers(node).as_ref().get_next()); - } else { - if self.head != Some(node) { - return None; - } - - self.head = L::pointers(node).as_ref().get_next(); + if let Some(prev) = L::pointers(node).as_ref().get_prev() { + debug_assert_eq!( + L::pointers(prev).as_ref().get_next(), + Some(node), + "linked list corruption: prev->next != node (prev={prev:p}, node={node:p})" + ); + L::pointers(prev) + .as_mut() + .set_next(L::pointers(node).as_ref().get_next()); + } else { + if self.head != Some(node) { + return None; } - if let Some(next) = L::pointers(node).as_ref().get_next() { - debug_assert_eq!( - L::pointers(next).as_ref().get_prev(), - Some(node), - "linked list corruption: next->prev != node (next={next:p}, node={node:p})" - ); - L::pointers(next) - .as_mut() - .set_prev(L::pointers(node).as_ref().get_prev()); - } else { - // // This might be the last item in the list - // if self.tail != Some(node) { - // return None; - // } - - // self.tail = L::pointers(node).as_ref().get_prev(); - } - - L::pointers(node).as_mut().set_next(None); - L::pointers(node).as_mut().set_prev(None); - - Some(L::from_raw(node)) + self.head = L::pointers(node).as_ref().get_next(); } + + if let Some(next) = L::pointers(node).as_ref().get_next() { + debug_assert_eq!( + L::pointers(next).as_ref().get_prev(), + Some(node), + "linked list corruption: next->prev != node (next={next:p}, node={node:p})" + ); + L::pointers(next) + .as_mut() + .set_prev(L::pointers(node).as_ref().get_prev()); + } else { + // // This might be the last item in the list + // if self.tail != Some(node) { + // return None; + // } + + // self.tail = L::pointers(node).as_ref().get_prev(); + } + + L::pointers(node).as_mut().set_next(None); + L::pointers(node).as_mut().set_prev(None); + + Some(L::from_raw(node)) } // pub fn last(&self) -> Option<&L::Target> { @@ -293,7 +291,7 @@ impl LinkedList { } } -impl fmt::Debug for LinkedList { +impl fmt::Debug for LinkedList { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("LinkedList") .field("head", &self.head) @@ -302,7 +300,7 @@ impl fmt::Debug for LinkedList { } } -impl Default for LinkedList { +impl Default for LinkedList { fn default() -> Self { Self::new() } @@ -310,16 +308,16 @@ impl Default for LinkedList { // ===== impl DrainFilter ===== -pub struct DrainFilter<'a, T: Link, F> { - list: &'a mut LinkedList, +pub struct DrainFilter<'a, L: Link, F> { + list: &'a mut LinkedList, filter: F, - curr: Option>, + curr: Option>, } -impl LinkedList { - pub const fn drain_filter(&mut self, filter: F) -> DrainFilter<'_, T, F> +impl LinkedList { + pub const fn drain_filter(&mut self, filter: F) -> DrainFilter<'_, L, F> where - F: FnMut(&mut T::Target) -> bool, + F: FnMut(&L::Target) -> bool, { let curr = self.head; DrainFilter { @@ -330,17 +328,17 @@ impl LinkedList { } } -impl Iterator for DrainFilter<'_, T, F> +impl Iterator for DrainFilter<'_, L, F> where - T: Link, - F: FnMut(&mut T::Target) -> bool, + L: Link, + F: FnMut(&L::Target) -> bool, { - type Item = T::Handle; + type Item = L::Handle; fn next(&mut self) -> Option { while let Some(curr) = self.curr { // safety: the pointer references data contained by the list - self.curr = unsafe { T::pointers(curr).as_ref() }.get_next(); + self.curr = unsafe { L::pointers(curr).as_ref() }.get_next(); // safety: the value is still owned by the linked list. if (self.filter)(unsafe { &mut *curr.as_ptr() }) { @@ -368,38 +366,26 @@ impl Pointers { } pub const fn get_prev(&self) -> Option> { - // SAFETY: prev is the first field in PointersInner, which is #[repr(C)]. - unsafe { - let inner = self.inner.get(); - let prev = inner as *const Option>; - ptr::read(prev) - } + // SAFETY: Field is accessed immutably through a reference. + unsafe { ptr::addr_of!((*self.inner.get()).prev).read() } } + pub const fn get_next(&self) -> Option> { - // SAFETY: next is the second field in PointersInner, which is #[repr(C)]. - unsafe { - let inner = self.inner.get(); - let prev = inner as *const Option>; - let next = prev.add(1); - ptr::read(next) - } + // SAFETY: Field is accessed immutably through a reference. + unsafe { ptr::addr_of!((*self.inner.get()).next).read() } } pub const fn set_prev(&mut self, value: Option>) { - // SAFETY: prev is the first field in PointersInner, which is #[repr(C)]. + // SAFETY: Field is accessed mutably through a mutable reference. unsafe { - let inner = self.inner.get(); - let prev = inner as *mut Option>; - ptr::write(prev, value); + ptr::addr_of_mut!((*self.inner.get()).prev).write(value); } } + pub const fn set_next(&mut self, value: Option>) { - // SAFETY: next is the second field in PointersInner, which is #[repr(C)]. + // SAFETY: Field is accessed mutably through a mutable reference. unsafe { - let inner = self.inner.get(); - let prev = inner as *mut Option>; - let next = prev.add(1); - ptr::write(next, value); + ptr::addr_of_mut!((*self.inner.get()).next).write(value); } } } diff --git a/crates/vm/src/gc_state.rs b/crates/vm/src/gc_state.rs index 6cfeebe97..3a3440fb5 100644 --- a/crates/vm/src/gc_state.rs +++ b/crates/vm/src/gc_state.rs @@ -299,9 +299,9 @@ impl Drop for CollectStopTheWorld { pub struct GcState { /// Per-generation intrusive linked lists for object tracking. /// Objects start in gen0, survivors are promoted to gen1, then gen2. - generation_lists: [PyRwLock>; 3], + generation_lists: [PyRwLock>; 3], /// Frozen/permanent objects (excluded from normal GC) - permanent_list: PyRwLock>, + permanent_list: PyRwLock>, /// Number of tracked objects per generation, across all interpreters. /// /// Advisory: they drive the collection threshold and `gc.get_count()`, and @@ -322,7 +322,7 @@ pub struct GcState { } // SAFETY: All fields are either inherently Send/Sync (atomics, RwLock, Mutex) or protected by PyMutex. -// LinkedList is Send+Sync because GcLink's Target (PyObject) is Send+Sync. +// LinkedList is Send+Sync because GcLink's Target (PyObject) is Send+Sync. #[cfg(feature = "threading")] unsafe impl Send for GcState {} #[cfg(feature = "threading")] @@ -418,8 +418,7 @@ impl GcState { let (list_lock, count) = if obj_gen <= 2 { ( - &self.generation_lists[obj_gen as usize] - as &PyRwLock>, + &self.generation_lists[obj_gen as usize] as &PyRwLock>, &self.counts[obj_gen as usize], ) } else if obj_gen == GC_PERMANENT { @@ -459,7 +458,7 @@ impl GcState { /// If generation is Some(n), returns those in generation n only. pub fn get_objects(&self, generation: Option, owner: GcOwner) -> Vec { fn collect_from_list( - list: &LinkedList, + list: &LinkedList, owner: GcOwner, ) -> impl Iterator + '_ { list.iter()