diff --git a/crates/vm/src/builtins/complex.rs b/crates/vm/src/builtins/complex.rs index f471c43c580..079b98046bc 100644 --- a/crates/vm/src/builtins/complex.rs +++ b/crates/vm/src/builtins/complex.rs @@ -41,6 +41,7 @@ thread_local! { impl PyPayload for PyComplex { const MAX_FREELIST: usize = 100; const HAS_FREELIST: bool = true; + const FREELIST_HAS_PAYLOAD: bool = false; #[inline] fn class(ctx: &Context) -> &'static Py { @@ -53,7 +54,7 @@ impl PyPayload for PyComplex { .try_with(|fl| { let mut list = fl.take(); let stored = if list.len() < Self::MAX_FREELIST { - list.push(obj); + unsafe { list.push(obj) }; true } else { false diff --git a/crates/vm/src/builtins/descriptor.rs b/crates/vm/src/builtins/descriptor.rs index b3189e592a8..f4bea6b351c 100644 --- a/crates/vm/src/builtins/descriptor.rs +++ b/crates/vm/src/builtins/descriptor.rs @@ -736,7 +736,7 @@ impl PyMemberSpec { } // = PyMemberDescrObject -#[pyclass(name = "member_descriptor", module = false)] +#[pyclass(name = "member_descriptor", module = false, traverse = "manual")] #[derive(Debug)] pub struct PyMemberDescriptor { #[pymember(name = "__objclass__", path = "typ")] @@ -746,6 +746,13 @@ pub struct PyMemberDescriptor { pub access: MemberAccess, } +// SAFETY: common.typ is owned; the other fields contain no owned Python objects. +unsafe impl crate::object::Traverse for PyMemberDescriptor { + fn traverse(&self, tracer_fn: &mut crate::object::TraverseFn<'_>) { + tracer_fn(self.common.typ.as_object()); + } +} + impl PyMemberDescriptor { /// Byte offset of an object-pointer member. `None` for bool, float, and /// struct-sequence indexes, which slot specialization must not treat as cells. diff --git a/crates/vm/src/builtins/dict.rs b/crates/vm/src/builtins/dict.rs index d709521ea6d..ae11932d7e7 100644 --- a/crates/vm/src/builtins/dict.rs +++ b/crates/vm/src/builtins/dict.rs @@ -69,6 +69,7 @@ thread_local! { impl PyPayload for PyDict { const MAX_FREELIST: usize = 80; const HAS_FREELIST: bool = true; + const FREELIST_HAS_PAYLOAD: bool = false; #[inline] fn class(ctx: &Context) -> &'static Py { @@ -81,7 +82,7 @@ impl PyPayload for PyDict { .try_with(|fl| { let mut list = fl.take(); let stored = if list.len() < Self::MAX_FREELIST { - list.push(obj); + unsafe { list.push(obj) }; true } else { false diff --git a/crates/vm/src/builtins/float.rs b/crates/vm/src/builtins/float.rs index 78d3bef7794..03b6fc92337 100644 --- a/crates/vm/src/builtins/float.rs +++ b/crates/vm/src/builtins/float.rs @@ -48,6 +48,7 @@ thread_local! { impl PyPayload for PyFloat { const MAX_FREELIST: usize = 100; const HAS_FREELIST: bool = true; + const FREELIST_HAS_PAYLOAD: bool = false; #[inline] fn class(ctx: &Context) -> &'static Py { @@ -60,7 +61,7 @@ impl PyPayload for PyFloat { .try_with(|fl| { let mut list = fl.take(); let stored = if list.len() < Self::MAX_FREELIST { - list.push(obj); + unsafe { list.push(obj) }; true } else { false diff --git a/crates/vm/src/builtins/function.rs b/crates/vm/src/builtins/function.rs index c6c4aaf2b79..fde1f6ef346 100644 --- a/crates/vm/src/builtins/function.rs +++ b/crates/vm/src/builtins/function.rs @@ -99,6 +99,8 @@ fn next_func_version() -> u32 { unsafe impl Traverse for PyFunction { fn traverse(&self, tracer_fn: &mut TraverseFn<'_>) { self.globals.traverse(tracer_fn); + // Both fields own a reference, even when builtins aliases globals. + self.builtins.traverse(tracer_fn); if let Some(closure) = self.closure.as_ref() { // Visit the closure tuple itself as an edge, not its cells: the // tuple is a tracked object that can join a reference cycle, and diff --git a/crates/vm/src/builtins/int.rs b/crates/vm/src/builtins/int.rs index d6799474582..d35196e0cea 100644 --- a/crates/vm/src/builtins/int.rs +++ b/crates/vm/src/builtins/int.rs @@ -58,6 +58,7 @@ thread_local! { impl PyPayload for PyInt { const MAX_FREELIST: usize = 100; const HAS_FREELIST: bool = true; + const FREELIST_HAS_PAYLOAD: bool = false; #[inline] fn class(ctx: &Context) -> &'static Py { @@ -74,7 +75,7 @@ impl PyPayload for PyInt { .try_with(|fl| { let mut list = fl.take(); let stored = if list.len() < Self::MAX_FREELIST { - list.push(obj); + unsafe { list.push(obj) }; true } else { false diff --git a/crates/vm/src/builtins/list.rs b/crates/vm/src/builtins/list.rs index d6a2c1669a7..8dec0d5d6a1 100644 --- a/crates/vm/src/builtins/list.rs +++ b/crates/vm/src/builtins/list.rs @@ -90,6 +90,7 @@ thread_local! { impl PyPayload for PyList { const MAX_FREELIST: usize = 80; const HAS_FREELIST: bool = true; + const FREELIST_HAS_PAYLOAD: bool = false; #[inline] fn class(ctx: &Context) -> &'static Py { @@ -102,7 +103,7 @@ impl PyPayload for PyList { .try_with(|fl| { let mut list = fl.take(); let stored = if list.len() < Self::MAX_FREELIST { - list.push(obj); + unsafe { list.push(obj) }; true } else { false diff --git a/crates/vm/src/builtins/range.rs b/crates/vm/src/builtins/range.rs index e8d2c30dbce..a8db4fab6ef 100644 --- a/crates/vm/src/builtins/range.rs +++ b/crates/vm/src/builtins/range.rs @@ -80,6 +80,7 @@ thread_local! { impl PyPayload for PyRange { const MAX_FREELIST: usize = 6; const HAS_FREELIST: bool = true; + const FREELIST_HAS_PAYLOAD: bool = false; #[inline] fn class(ctx: &Context) -> &'static Py { @@ -92,7 +93,7 @@ impl PyPayload for PyRange { .try_with(|fl| { let mut list = fl.take(); let stored = if list.len() < Self::MAX_FREELIST { - list.push(obj); + unsafe { list.push(obj) }; true } else { false diff --git a/crates/vm/src/builtins/slice.rs b/crates/vm/src/builtins/slice.rs index 8cc53fd16c1..3728d232869 100644 --- a/crates/vm/src/builtins/slice.rs +++ b/crates/vm/src/builtins/slice.rs @@ -40,7 +40,7 @@ unsafe impl crate::object::Traverse for PySlice { out.push(start); } // stop is not Option, so it will be freed when payload is dropped - // (via drop_in_place on freelist pop, or Box::from_raw on dealloc) + // (before freelist caching, or during deallocation) if let Some(step) = self.step.take() { out.push(step); } @@ -54,6 +54,7 @@ thread_local! { impl PyPayload for PySlice { const MAX_FREELIST: usize = 1; const HAS_FREELIST: bool = true; + const FREELIST_HAS_PAYLOAD: bool = false; #[inline] fn class(ctx: &Context) -> &'static Py { @@ -66,7 +67,7 @@ impl PyPayload for PySlice { .try_with(|fl| { let mut list = fl.take(); let stored = if list.len() < Self::MAX_FREELIST { - list.push(obj); + unsafe { list.push(obj) }; true } else { false diff --git a/crates/vm/src/builtins/tuple.rs b/crates/vm/src/builtins/tuple.rs index a1c06e49106..4168b6564d4 100644 --- a/crates/vm/src/builtins/tuple.rs +++ b/crates/vm/src/builtins/tuple.rs @@ -118,6 +118,7 @@ thread_local! { impl PyPayload for PyTuple { const MAX_FREELIST: usize = 2000; const HAS_FREELIST: bool = true; + const FREELIST_HAS_PAYLOAD: bool = false; #[inline] fn class(ctx: &Context) -> &'static Py { @@ -130,7 +131,7 @@ impl PyPayload for PyTuple { .try_with(|fl| { let mut list = fl.take(); let stored = if list.len() < Self::MAX_FREELIST { - list.push(obj); + unsafe { list.push(obj) }; true } else { false diff --git a/crates/vm/src/frame.rs b/crates/vm/src/frame.rs index dadc0476be8..fa71f80e4b5 100644 --- a/crates/vm/src/frame.rs +++ b/crates/vm/src/frame.rs @@ -1615,6 +1615,7 @@ thread_local! { impl PyPayload for FrameObject { const MAX_FREELIST: usize = 200; const HAS_FREELIST: bool = true; + const FREELIST_HAS_PAYLOAD: bool = false; // Ordinary call frames are created untracked and only enter the GC when // they escape (see `release_datastack_frame`); generator/coroutine frames // are tracked explicitly at creation in `invoke_with_locals`. @@ -1631,7 +1632,7 @@ impl PyPayload for FrameObject { .try_with(|fl| { let mut list = fl.take(); let stored = if list.len() < Self::MAX_FREELIST { - list.push(obj); + unsafe { list.push(obj) }; true } else { false diff --git a/crates/vm/src/object/core.rs b/crates/vm/src/object/core.rs index eb7edc93cca..a59857d66d4 100644 --- a/crates/vm/src/object/core.rs +++ b/crates/vm/src/object/core.rs @@ -1015,7 +1015,7 @@ unsafe impl Link for WeakLink { } // PyWeakReference: each weakref holds a direct pointer to its referent. -#[pyclass(name = "ReferenceType", module = "weakref")] +#[pyclass(name = "ReferenceType", module = "weakref", traverse = "manual")] #[derive(Debug)] pub struct PyWeak { pointers: Pointers>, @@ -1035,6 +1035,21 @@ cfg_select! { _ => {} } +// SAFETY: the stripe lock protects the owned callback; wr_object is a weak pointer. +unsafe impl Traverse for PyWeak { + fn traverse(&self, tracer_fn: &mut TraverseFn<'_>) { + let obj_ptr = self.wr_object.load(Ordering::Acquire); + if obj_ptr.is_null() { + return; + } + let _lock = weakref_lock::lock(obj_ptr as usize); + if !self.wr_object.load(Ordering::Relaxed).is_null() { + // SAFETY: the callback is read under the same lock used to consume it. + unsafe { (&*self.callback.get()).traverse(tracer_fn) }; + } + } +} + impl PyWeak { /// _PyWeakref_GET_REF: attempt to upgrade the weakref to a strong reference. pub(crate) fn upgrade(&self) -> Option { @@ -1247,9 +1262,11 @@ impl Py { /// object it points at until its own `Drop` unlinks it, and a thread /// walking that list reads the class off every node it passes, so the /// class has to outlive the payload. - unsafe fn drop_fields(ptr: *mut Self) { + unsafe fn drop_fields(ptr: *mut Self) { unsafe { - core::ptr::drop_in_place(&raw mut (*ptr).payload); + if DROP_PAYLOAD { + core::ptr::drop_in_place(&raw mut (*ptr).payload); + } core::ptr::drop_in_place(&raw mut (*ptr).typ); } } @@ -1260,8 +1277,15 @@ impl Py { /// # Safety /// `ptr` must be a valid pointer from `Py::new` and must not be used after this call. unsafe fn dealloc(ptr: *mut Self) { + unsafe { Self::dealloc_inner::(ptr) }; + } + + // Freelist entries have already had their payload destroyed. + unsafe fn dealloc_inner(ptr: *mut Self) { unsafe { - let (flags, member_count) = (*ptr).read_type_flags(); + // Read the initialized header, including for cached allocations + // whose payload has already been destroyed. + let (flags, member_count) = (*(ptr as *const PyObject)).0.read_type_flags(); let has_ext = flags.contains(&crate::types::PyTypeFlags::HAS_DICT) || member_count > 0; let has_weakref = flags.contains(&crate::types::PyTypeFlags::HAS_WEAKREF); // Objects published to lock-free caches keep their memory mapped @@ -1293,7 +1317,7 @@ impl Py { let alloc_ptr = (ptr as *mut u8).sub(inner_offset); - Self::drop_fields(ptr); + Self::drop_fields::(ptr); // Drop member cells, then ObjExt. WeakRefList is in front of the cells. let mut cursor = alloc_ptr; @@ -1321,10 +1345,10 @@ impl Py { } } else if published { let layout = core::alloc::Layout::new::(); - Self::drop_fields(ptr); + Self::drop_fields::(ptr); crate::object::qsbr::free_delayed(ptr as *mut u8, layout); } else { - Self::drop_fields(ptr); + Self::drop_fields::(ptr); // The fields are gone; the box is only here to free the memory // the matching `Box::new` in `new` allocated. drop(Box::from_raw(ptr.cast::>())); @@ -1459,15 +1483,28 @@ impl Py { /// Thread-local freelist storage for reusing object allocations. /// -/// Wraps a `Vec<*mut PyObject>`. On thread teardown, `Drop` frees raw -/// `Py` allocations without running payload destructors to avoid -/// accessing already-destroyed thread-local storage (GC state, other freelists). +/// Cached allocations retain their header, but not their payload. Payloads +/// are destroyed before caching, while thread-local deallocation state is +/// available. Thread teardown therefore only releases the allocation and its +/// reference to the static base class. pub(crate) struct FreeList { items: Vec<*mut PyObject>, _marker: core::marker::PhantomData, } impl FreeList { + /// Cache a dead allocation after destroying its payload. + /// + /// # Safety + /// `ptr` must point to an untracked, unpublished, exact base-type `Py` + /// with no outstanding payload borrows. Its payload must be initialized. + pub(crate) unsafe fn push(&mut self, ptr: *mut PyObject) { + debug_assert!(!T::FREELIST_HAS_PAYLOAD); + // Destruction can reenter allocation; publish the husk only afterwards. + unsafe { core::ptr::drop_in_place(&raw mut (*(ptr as *mut Py)).payload) }; + self.items.push(ptr); + } + pub(crate) const fn new() -> Self { Self { items: Vec::new(), @@ -1484,16 +1521,10 @@ impl Default for FreeList { impl Drop for FreeList { fn drop(&mut self) { - // During thread teardown, we cannot safely run destructors on cached - // objects because their Drop impls may access thread-local storage - // (GC state, other freelists) that is already destroyed. - // Instead, free just the raw allocation. The payload's heap fields - // (BigInt, PyObjectRef, etc.) are leaked, but this is bounded by - // MAX_FREELIST per type per thread. for ptr in self.items.drain(..) { - unsafe { - alloc::alloc::dealloc(ptr as *mut u8, core::alloc::Layout::new::>()); - } + // The payload is gone and the remaining class reference points to + // a static base type, so no Python destruction can run here. + unsafe { Py::::dealloc_inner::(ptr as *mut Py) }; } } } @@ -2827,8 +2858,10 @@ impl PyRef { unsafe { core::ptr::write(&mut (*inner).ref_count, RefCount::new()); (*inner).gc_bits.store(0, Ordering::Relaxed); - core::ptr::drop_in_place(&mut (*inner).payload); - core::ptr::write(&mut (*inner).payload, payload); + if T::FREELIST_HAS_PAYLOAD { + core::ptr::drop_in_place(&raw mut (*inner).payload); + } + core::ptr::write(&raw mut (*inner).payload, payload); // Freelist only stores exact base types (push-side filter), // but subtypes sharing the same Rust payload (e.g. structseq) // may pop entries. Update typ if it differs. diff --git a/crates/vm/src/object/payload.rs b/crates/vm/src/object/payload.rs index a5a4bf35b7f..09b6c86eb85 100644 --- a/crates/vm/src/object/payload.rs +++ b/crates/vm/src/object/payload.rs @@ -96,13 +96,18 @@ pub trait PyPayload: MaybeTraverse + PyThreadingConstraint + Sized + 'static { /// race conditions when the object is reused. const HAS_FREELIST: bool = false; + /// Whether cached allocations retain an initialized payload. + /// `FreeList` destroys payloads before caching and requires false. + const FREELIST_HAS_PAYLOAD: bool = true; + /// Maximum number of objects to keep in the freelist. const MAX_FREELIST: usize = 0; /// Try to push a dead object onto this type's freelist for reuse. /// Returns true if the object was stored (caller must NOT free the memory). /// Called after tp_clear, so the payload is a cleared husk; implementations - /// must not rely on its pre-clear contents. + /// must not rely on its pre-clear contents. If `FREELIST_HAS_PAYLOAD` is + /// false, destroy the payload before publishing the allocation for reuse. /// /// # Safety /// `obj` must be a valid pointer to a `Py` with refcount 0 @@ -114,13 +119,13 @@ pub trait PyPayload: MaybeTraverse + PyThreadingConstraint + Sized + 'static { } /// Try to pop a pre-allocated object from this type's freelist. - /// The returned pointer still has the old payload; the caller must + /// The caller must /// reinitialize `ref_count`, `gc_bits`, and `payload`. /// /// # Safety /// The returned pointer (if Some) must point to a valid `Py` - /// whose payload is still initialized from a previous allocation. The caller - /// will drop and overwrite `payload` before reuse. + /// whose payload is initialized if and only if `FREELIST_HAS_PAYLOAD` is + /// true. The caller will destroy it if initialized, then write a new payload. #[inline] unsafe fn freelist_pop(_payload: &Self) -> Option> { None diff --git a/extra_tests/snippets/stdlib_gc_function_builtins.py b/extra_tests/snippets/stdlib_gc_function_builtins.py new file mode 100644 index 00000000000..8f6b429142d --- /dev/null +++ b/extra_tests/snippets/stdlib_gc_function_builtins.py @@ -0,0 +1,28 @@ +"""Functions must report every owned builtins reference to the cycle collector.""" + +import gc +import weakref + + +def check_function_cycle(shared_namespace): + namespace = {} + builtins = namespace if shared_namespace else {} + namespace["__builtins__"] = builtins + exec("def temporary(): return 42", namespace) + keeper = namespace["temporary"] + builtins["temporary"] = keeper + reference = weakref.ref(keeper) + del namespace, builtins + + gc.collect() + assert reference() is keeper + assert keeper() == 42 + + del keeper + gc.collect() + assert reference() is None + + +check_function_cycle(shared_namespace=False) +check_function_cycle(shared_namespace=True) +print("ok") diff --git a/extra_tests/snippets/stdlib_gc_member_descriptor.py b/extra_tests/snippets/stdlib_gc_member_descriptor.py new file mode 100644 index 00000000000..0c1862e847e --- /dev/null +++ b/extra_tests/snippets/stdlib_gc_member_descriptor.py @@ -0,0 +1,24 @@ +"""A slot descriptor owns its class, including when the class becomes garbage.""" + +import gc +import weakref + + +def create(): + class Temporary: + __slots__ = ("value",) + + return weakref.ref(Temporary), Temporary.value + + +reference, descriptor = create() +gc.collect() +assert reference() is descriptor.__objclass__ +instance = descriptor.__objclass__() +descriptor.__set__(instance, 42) +assert descriptor.__get__(instance) == 42 + +del instance, descriptor +gc.collect() +assert reference() is None +print("ok") diff --git a/extra_tests/snippets/stdlib_gc_slice_payload.py b/extra_tests/snippets/stdlib_gc_slice_payload.py new file mode 100644 index 00000000000..735758f6f9e --- /dev/null +++ b/extra_tests/snippets/stdlib_gc_slice_payload.py @@ -0,0 +1,21 @@ +import gc +import weakref + + +class Target: + pass + + +target = Target() +reference = weakref.ref(target) +value = slice(None, target, None) +del target +gc.collect() +assert reference() is value.stop + +del value +gc.collect() +assert reference() is None, "a cached slice retained its stop argument" + +# Reusing cached storage must initialize a fresh payload. +assert slice(1, 7, 2).indices(10) == (1, 7, 2) diff --git a/extra_tests/snippets/stdlib_gc_weakref_callback.py b/extra_tests/snippets/stdlib_gc_weakref_callback.py new file mode 100644 index 00000000000..0909403f38b --- /dev/null +++ b/extra_tests/snippets/stdlib_gc_weakref_callback.py @@ -0,0 +1,44 @@ +"""Weak references own their callbacks, but never own their referents.""" + +import gc +import weakref + + +class Target: + pass + + +class CallableTarget: + def __call__(self): + return 42 + + +def check_callback_cycle(target, make_reference): + def create(): + holder = [] + + def callback(reference): + holder.append(reference) + + reference = make_reference(target, callback) + holder.append(reference) + return weakref.ref(callback) + + reference = create() + gc.collect() + assert reference() is None + + +check_callback_cycle(Target(), weakref.ref) +check_callback_cycle(Target(), weakref.proxy) +check_callback_cycle(CallableTarget(), weakref.proxy) + +called = [] +target = Target() +target.cycle = target +reference = weakref.ref(target, lambda ref: called.append(ref() is None)) +del target +gc.collect() +assert reference() is None +assert called == [True] +print("ok")