diff --git a/crates/common/src/linked_list.rs b/crates/common/src/linked_list.rs index 48cdb4feb95..223d9ce0eb1 100644 --- a/crates/common/src/linked_list.rs +++ b/crates/common/src/linked_list.rs @@ -157,6 +157,15 @@ impl LinkedList { let ptr = L::as_raw(&val); assert_ne!(self.head, Some(ptr)); unsafe { + // Verify the node is not already in a list (pointers must be clean) + debug_assert!( + L::pointers(ptr).as_ref().get_prev().is_none(), + "push_front: node already has prev pointer (double-insert?)" + ); + debug_assert!( + 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); @@ -192,6 +201,20 @@ impl LinkedList { // } // } + /// Removes the first element from the list and returns it, or None if empty. + pub fn pop_front(&mut self) -> Option { + let head = self.head?; + unsafe { + self.head = L::pointers(head).as_ref().get_next(); + 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)) + } + } + /// Returns whether the linked list does not contain any node pub const fn is_empty(&self) -> bool { self.head.is_none() @@ -212,7 +235,11 @@ impl LinkedList { 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)); + 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()); @@ -225,7 +252,11 @@ impl LinkedList { } if let Some(next) = L::pointers(node).as_ref().get_next() { - debug_assert_eq!(L::pointers(next).as_ref().get_prev(), Some(node)); + 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()); diff --git a/crates/vm/src/gc_state.rs b/crates/vm/src/gc_state.rs index a25c8d6c42a..2b3b3ccd3a2 100644 --- a/crates/vm/src/gc_state.rs +++ b/crates/vm/src/gc_state.rs @@ -3,7 +3,9 @@ //! This module implements CPython-compatible generational garbage collection //! for RustPython, using an intrusive doubly-linked list approach. +use crate::common::linked_list::LinkedList; use crate::common::lock::{PyMutex, PyRwLock}; +use crate::object::{GC_PERMANENT, GC_UNTRACKED, GcLink}; use crate::{AsObject, PyObject, PyObjectRef}; use core::ptr::NonNull; use core::sync::atomic::{AtomicBool, AtomicU32, AtomicUsize, Ordering}; @@ -96,13 +98,10 @@ impl GcGeneration { } } -/// Wrapper for raw pointer to make it Send + Sync +/// Wrapper for NonNull to impl Hash/Eq for use in temporary collection sets. +/// Only used within collect_inner, never shared across threads. #[derive(Clone, Copy, PartialEq, Eq, Hash)] -struct GcObjectPtr(NonNull); - -// SAFETY: We only use this for tracking objects, and proper synchronization is used -unsafe impl Send for GcObjectPtr {} -unsafe impl Sync for GcObjectPtr {} +struct GcPtr(NonNull); /// Global GC state pub struct GcState { @@ -112,11 +111,11 @@ pub struct GcState { pub permanent: GcGeneration, /// GC enabled flag pub enabled: AtomicBool, - /// Per-generation object tracking (for correct gc_refs algorithm) - /// Objects start in gen0, survivors move to gen1, then gen2 - generation_objects: [PyRwLock>; 3], + /// Per-generation intrusive linked lists for object tracking. + /// Objects start in gen0, survivors are promoted to gen1, then gen2. + generation_lists: [PyRwLock>; 3], /// Frozen/permanent objects (excluded from normal GC) - permanent_objects: PyRwLock>, + permanent_list: PyRwLock>, /// Debug flags pub debug: AtomicU32, /// gc.garbage list (uncollectable objects with __del__) @@ -127,17 +126,10 @@ pub struct GcState { collecting: PyMutex<()>, /// Allocation counter for gen0 alloc_count: AtomicUsize, - /// Registry of all tracked objects (for cycle detection) - tracked_objects: PyRwLock>, - /// Objects that have been finalized (__del__ already called) - /// Prevents calling __del__ multiple times on resurrected objects - finalized_objects: PyRwLock>, } -// SAFETY: GcObjectPtr wraps NonNull which is !Send/!Sync, but we only use it as an opaque -// hash key. PyObjectRef is Send. In threading mode, PyRwLock/PyMutex are parking_lot based -// and genuinely Sync. In non-threading mode, gc_state() is thread-local so neither Send -// nor Sync is needed. +// 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. #[cfg(feature = "threading")] unsafe impl Send for GcState {} #[cfg(feature = "threading")] @@ -159,19 +151,17 @@ impl GcState { ], permanent: GcGeneration::new(0), enabled: AtomicBool::new(true), - generation_objects: [ - PyRwLock::new(HashSet::new()), - PyRwLock::new(HashSet::new()), - PyRwLock::new(HashSet::new()), + generation_lists: [ + PyRwLock::new(LinkedList::new()), + PyRwLock::new(LinkedList::new()), + PyRwLock::new(LinkedList::new()), ], - permanent_objects: PyRwLock::new(HashSet::new()), + permanent_list: PyRwLock::new(LinkedList::new()), debug: AtomicU32::new(0), garbage: PyMutex::new(Vec::new()), callbacks: PyMutex::new(Vec::new()), collecting: PyMutex::new(()), alloc_count: AtomicUsize::new(0), - tracked_objects: PyRwLock::new(HashSet::new()), - finalized_objects: PyRwLock::new(HashSet::new()), } } @@ -238,120 +228,99 @@ impl GcState { ] } - /// Track a new object (add to gen0) - /// Called when IS_TRACE objects are created + /// Track a new object (add to gen0). + /// O(1) — intrusive linked list push_front, no hashing. /// /// # Safety /// obj must be a valid pointer to a PyObject pub unsafe fn track_object(&self, obj: NonNull) { - let gc_ptr = GcObjectPtr(obj); - - // _PyObject_GC_TRACK let obj_ref = unsafe { obj.as_ref() }; obj_ref.set_gc_tracked(); + obj_ref.set_gc_generation(0); - // Add to generation 0 tracking first (for correct gc_refs algorithm) - // Only increment count if we successfully add to the set - { - let mut gen0 = self.generation_objects[0].write(); - if gen0.insert(gc_ptr) { - self.generations[0].count.fetch_add(1, Ordering::SeqCst); - self.alloc_count.fetch_add(1, Ordering::SeqCst); - } - } - - // Also add to global tracking (for get_objects, etc.) - self.tracked_objects.write().insert(gc_ptr); + self.generation_lists[0].write().push_front(obj); + self.generations[0].count.fetch_add(1, Ordering::SeqCst); + self.alloc_count.fetch_add(1, Ordering::SeqCst); } - /// Untrack an object (remove from GC lists) - /// Called when objects are deallocated + /// Untrack an object (remove from GC lists). + /// O(1) — intrusive linked list remove by node pointer. /// /// # Safety - /// obj must be a valid pointer to a PyObject + /// obj must be a valid pointer to a PyObject that is currently tracked. + /// The object's memory must still be valid (pointers are read). pub unsafe fn untrack_object(&self, obj: NonNull) { - let gc_ptr = GcObjectPtr(obj); - - // Remove from generation tracking lists and decrement the correct generation's count - for (gen_idx, generation) in self.generation_objects.iter().enumerate() { - let mut gen_set = generation.write(); - if gen_set.remove(&gc_ptr) { - // Decrement count for the generation we removed from - let count = self.generations[gen_idx].count.load(Ordering::SeqCst); - if count > 0 { - self.generations[gen_idx] - .count - .fetch_sub(1, Ordering::SeqCst); - } - break; // Object can only be in one generation - } - } - - // Remove from global tracking - self.tracked_objects.write().remove(&gc_ptr); + let obj_ref = unsafe { obj.as_ref() }; - // Remove from permanent tracking - { - let mut permanent = self.permanent_objects.write(); - if permanent.remove(&gc_ptr) { - let count = self.permanent.count.load(Ordering::SeqCst); - if count > 0 { - self.permanent.count.fetch_sub(1, Ordering::SeqCst); - } + loop { + let obj_gen = obj_ref.gc_generation(); + + let (list_lock, count) = if obj_gen <= 2 { + ( + &self.generation_lists[obj_gen as usize] + as &PyRwLock>, + &self.generations[obj_gen as usize].count, + ) + } else if obj_gen == GC_PERMANENT { + (&self.permanent_list, &self.permanent.count) + } else { + return; // GC_UNTRACKED or unknown — already untracked + }; + + let mut list = list_lock.write(); + // Re-check generation under lock (may have changed due to promotion) + if obj_ref.gc_generation() != obj_gen { + drop(list); + continue; // Retry with the updated generation + } + if unsafe { list.remove(obj) }.is_some() { + count.fetch_sub(1, Ordering::SeqCst); + obj_ref.clear_gc_tracked(); + obj_ref.set_gc_generation(GC_UNTRACKED); + } else { + // Object claims to be in this generation but wasn't found in the list. + // This indicates a bug: the object was already removed from the list + // without updating gc_generation, or was never inserted. + eprintln!( + "GC WARNING: untrack_object failed to remove obj={obj:p} from gen={obj_gen}, \ + tracked={}, gc_gen={}", + obj_ref.is_gc_tracked(), + obj_ref.gc_generation() + ); } + return; } - - // Remove from finalized set - self.finalized_objects.write().remove(&gc_ptr); - } - - /// Check if an object has been finalized - pub fn is_finalized(&self, obj: NonNull) -> bool { - let gc_ptr = GcObjectPtr(obj); - self.finalized_objects.read().contains(&gc_ptr) - } - - /// Mark an object as finalized - pub fn mark_finalized(&self, obj: NonNull) { - let gc_ptr = GcObjectPtr(obj); - self.finalized_objects.write().insert(gc_ptr); } /// Get tracked objects (for gc.get_objects) /// If generation is None, returns all tracked objects. /// If generation is Some(n), returns objects in generation n only. pub fn get_objects(&self, generation: Option) -> Vec { + fn collect_from_list( + list: &LinkedList, + ) -> impl Iterator + '_ { + list.iter().filter_map(|obj| { + if obj.strong_count() > 0 { + Some(obj.to_owned()) + } else { + None + } + }) + } + match generation { None => { - // Return all tracked objects - self.tracked_objects - .read() - .iter() - .filter_map(|ptr| { - let obj = unsafe { ptr.0.as_ref() }; - if obj.strong_count() > 0 { - Some(obj.to_owned()) - } else { - None - } - }) - .collect() + // Return all tracked objects from all generations + permanent + let mut result = Vec::new(); + for gen_list in &self.generation_lists { + result.extend(collect_from_list(&gen_list.read())); + } + result.extend(collect_from_list(&self.permanent_list.read())); + result } Some(g) if (0..=2).contains(&g) => { - // Return objects in specific generation - let gen_idx = g as usize; - self.generation_objects[gen_idx] - .read() - .iter() - .filter_map(|ptr| { - let obj = unsafe { ptr.0.as_ref() }; - if obj.strong_count() > 0 { - Some(obj.to_owned()) - } else { - None - } - }) - .collect() + let guard = self.generation_lists[g as usize].read(); + collect_from_list(&guard).collect() } _ => Vec::new(), } @@ -365,8 +334,6 @@ impl GcState { return false; } - // _PyObject_GC_Alloc checks thresholds - // Check gen0 threshold let count0 = self.generations[0].count.load(Ordering::SeqCst) as u32; let threshold0 = self.generations[0].threshold(); @@ -379,14 +346,6 @@ impl GcState { } /// Perform garbage collection on the given generation - /// Returns (collected_count, uncollectable_count) - /// - /// Implements CPython-compatible generational GC algorithm: - /// - Only collects objects from generations 0 to `generation` - /// - Uses gc_refs algorithm: gc_refs = strong_count - internal_refs - /// - Only subtracts references between objects IN THE SAME COLLECTION - /// - /// If `force` is true, collection runs even if GC is disabled (for manual gc.collect() calls) pub fn collect(&self, generation: usize) -> (usize, usize) { self.collect_inner(generation, false) } @@ -418,24 +377,21 @@ impl GcState { crate::builtins::type_::type_cache_clear(); // Step 1: Gather objects from generations 0..=generation - // Hold read locks for the entire collection to prevent other threads - // from untracking objects while we're iterating. + // Hold read locks for the entire scan to prevent concurrent modifications. let gen_locks: Vec<_> = (0..=generation) - .map(|i| self.generation_objects[i].read()) + .map(|i| self.generation_lists[i].read()) .collect(); - let mut collecting: HashSet = HashSet::new(); - for gen_set in &gen_locks { - for &ptr in gen_set.iter() { - let obj = unsafe { ptr.0.as_ref() }; + let mut collecting: HashSet = HashSet::new(); + for gen_list in &gen_locks { + for obj in gen_list.iter() { if obj.strong_count() > 0 { - collecting.insert(ptr); + collecting.insert(GcPtr(NonNull::from(obj))); } } } if collecting.is_empty() { - // Reset gen0 count even if nothing to collect self.generations[0].count.store(0, Ordering::SeqCst); self.generations[generation].update_stats(0, 0); return (0, 0); @@ -450,25 +406,21 @@ impl GcState { } // Step 2: Build gc_refs map (copy reference counts) - let mut gc_refs: std::collections::HashMap = - std::collections::HashMap::new(); + let mut gc_refs: std::collections::HashMap = std::collections::HashMap::new(); for &ptr in &collecting { let obj = unsafe { ptr.0.as_ref() }; gc_refs.insert(ptr, obj.strong_count()); } // Step 3: Subtract internal references - // CRITICAL: Only subtract refs to objects IN THE COLLECTING SET for &ptr in &collecting { let obj = unsafe { ptr.0.as_ref() }; - // Double-check object is still alive if obj.strong_count() == 0 { continue; } let referent_ptrs = unsafe { obj.gc_get_referent_ptrs() }; for child_ptr in referent_ptrs { - let gc_ptr = GcObjectPtr(child_ptr); - // Only decrement if child is also in the collecting set! + let gc_ptr = GcPtr(child_ptr); if collecting.contains(&gc_ptr) && let Some(refs) = gc_refs.get_mut(&gc_ptr) { @@ -478,12 +430,9 @@ impl GcState { } // Step 4: Find reachable objects (gc_refs > 0) and traverse from them - // Objects with gc_refs > 0 are definitely reachable from outside. - // We need to mark all objects reachable from them as also reachable. - let mut reachable: HashSet = HashSet::new(); - let mut worklist: Vec = Vec::new(); + let mut reachable: HashSet = HashSet::new(); + let mut worklist: Vec = Vec::new(); - // Start with objects that have gc_refs > 0 for (&ptr, &refs) in &gc_refs { if refs > 0 { reachable.insert(ptr); @@ -491,14 +440,12 @@ impl GcState { } } - // Traverse reachable objects to find more reachable ones while let Some(ptr) = worklist.pop() { let obj = unsafe { ptr.0.as_ref() }; if obj.is_gc_tracked() { let referent_ptrs = unsafe { obj.gc_get_referent_ptrs() }; for child_ptr in referent_ptrs { - let gc_ptr = GcObjectPtr(child_ptr); - // If child is in collecting set and not yet marked reachable + let gc_ptr = GcPtr(child_ptr); if collecting.contains(&gc_ptr) && reachable.insert(gc_ptr) { worklist.push(gc_ptr); } @@ -506,8 +453,8 @@ impl GcState { } } - // Step 5: Find unreachable objects (in collecting but not in reachable) - let unreachable: Vec = collecting.difference(&reachable).copied().collect(); + // Step 5: Find unreachable objects + let unreachable: Vec = collecting.difference(&reachable).copied().collect(); if debug.contains(GcDebugFlags::STATS) { eprintln!( @@ -517,23 +464,22 @@ impl GcState { ); } - if unreachable.is_empty() { - // No cycles found - promote survivors to next generation - drop(gen_locks); // Release read locks before promoting - self.promote_survivors(generation, &collecting); - // Reset gen0 count - self.generations[0].count.store(0, Ordering::SeqCst); - self.generations[generation].update_stats(0, 0); - return (0, 0); - } - - // Release read locks before finalization phase. - // This allows other threads to untrack objects while we finalize. - drop(gen_locks); - - // Step 6: Finalize unreachable objects and handle resurrection + // Create strong references while read locks are still held. + // After dropping gen_locks, other threads can untrack+free objects, + // making the raw pointers in `reachable`/`unreachable` dangling. + // Strong refs keep objects alive for later phases. + let survivor_refs: Vec = reachable + .iter() + .filter_map(|ptr| { + let obj = unsafe { ptr.0.as_ref() }; + if obj.strong_count() > 0 { + Some(obj.to_owned()) + } else { + None + } + }) + .collect(); - // 6a: Get references to all unreachable objects let unreachable_refs: Vec = unreachable .iter() .filter_map(|ptr| { @@ -546,114 +492,98 @@ impl GcState { }) .collect(); + if unreachable.is_empty() { + drop(gen_locks); + self.promote_survivors(generation, &survivor_refs); + self.generations[0].count.store(0, Ordering::SeqCst); + self.generations[generation].update_stats(0, 0); + return (0, 0); + } + + // Release read locks before finalization phase. + drop(gen_locks); + + // Step 6: Finalize unreachable objects and handle resurrection + if unreachable_refs.is_empty() { - self.promote_survivors(generation, &reachable); - // Reset gen0 count + self.promote_survivors(generation, &survivor_refs); self.generations[0].count.store(0, Ordering::SeqCst); self.generations[generation].update_stats(0, 0); return (0, 0); } // 6b: Record initial strong counts (for resurrection detection) - // Each object has +1 from unreachable_refs, so initial count includes that - let initial_counts: std::collections::HashMap = unreachable_refs + let initial_counts: std::collections::HashMap = unreachable_refs .iter() .map(|obj| { - let ptr = GcObjectPtr(core::ptr::NonNull::from(obj.as_ref())); + let ptr = GcPtr(core::ptr::NonNull::from(obj.as_ref())); (ptr, obj.strong_count()) }) .collect(); // 6c: Clear existing weakrefs BEFORE calling __del__ - // This invalidates existing weakrefs, but new weakrefs created during __del__ - // will still work (WeakRefList::add restores inner.obj if cleared) - // - // CRITICAL: We use a two-phase approach to match CPython behavior: - // Phase 1: Clear ALL weakrefs (set inner.obj = None) and collect callbacks - // Phase 2: Invoke ALL callbacks - // This ensures that when a callback runs, ALL weakrefs to unreachable objects - // are already dead (return None when called). let mut all_callbacks: Vec<(crate::PyRef, crate::PyObjectRef)> = Vec::new(); for obj_ref in &unreachable_refs { let callbacks = obj_ref.gc_clear_weakrefs_collect_callbacks(); all_callbacks.extend(callbacks); } - // Phase 2: Now call all callbacks - at this point ALL weakrefs are cleared for (wr, cb) in all_callbacks { if let Some(Err(e)) = crate::vm::thread::with_vm(&cb, |vm| cb.call((wr.clone(),), vm)) { - // Report the exception via run_unraisable crate::vm::thread::with_vm(&cb, |vm| { vm.run_unraisable(e.clone(), Some("weakref callback".to_owned()), cb.clone()); }); } - // If with_vm returns None, we silently skip - no VM available to handle errors } - // 6d: Call __del__ on all unreachable objects - // This allows resurrection to work correctly - // Skip objects that have already been finalized (prevents multiple __del__ calls) + // 6d: Call __del__ on unreachable objects (skip already-finalized). + // try_call_finalizer() internally checks gc_finalized() and sets it, + // so we must NOT set it beforehand. for obj_ref in &unreachable_refs { - let ptr = GcObjectPtr(core::ptr::NonNull::from(obj_ref.as_ref())); - let already_finalized = self.finalized_objects.read().contains(&ptr); - - if !already_finalized { - // Mark as finalized BEFORE calling __del__ - // This ensures is_finalized() returns True inside __del__ - self.finalized_objects.write().insert(ptr); - obj_ref.try_call_finalizer(); - } + obj_ref.try_call_finalizer(); } - // 6d: Detect resurrection - strong_count increased means object was resurrected - // Step 1: Find directly resurrected objects (strong_count increased) - let mut resurrected_set: HashSet = HashSet::new(); - let unreachable_set: HashSet = unreachable.iter().copied().collect(); + // Detect resurrection + let mut resurrected_set: HashSet = HashSet::new(); + let unreachable_set: HashSet = unreachable.iter().copied().collect(); for obj in &unreachable_refs { - let ptr = GcObjectPtr(core::ptr::NonNull::from(obj.as_ref())); + let ptr = GcPtr(core::ptr::NonNull::from(obj.as_ref())); let initial = initial_counts.get(&ptr).copied().unwrap_or(1); if obj.strong_count() > initial { resurrected_set.insert(ptr); } } - // Step 2: Transitive resurrection - objects reachable from resurrected are also resurrected - // This is critical for cases like: Lazarus resurrects itself, its cargo should also survive - let mut worklist: Vec = resurrected_set.iter().copied().collect(); + // Transitive resurrection + let mut worklist: Vec = resurrected_set.iter().copied().collect(); while let Some(ptr) = worklist.pop() { let obj = unsafe { ptr.0.as_ref() }; let referent_ptrs = unsafe { obj.gc_get_referent_ptrs() }; for child_ptr in referent_ptrs { - let child_gc_ptr = GcObjectPtr(child_ptr); - // If child is in unreachable set and not yet marked as resurrected + let child_gc_ptr = GcPtr(child_ptr); if unreachable_set.contains(&child_gc_ptr) && resurrected_set.insert(child_gc_ptr) { worklist.push(child_gc_ptr); } } } - // Step 3: Partition into resurrected and truly dead + // Partition into resurrected and truly dead let (resurrected, truly_dead): (Vec<_>, Vec<_>) = unreachable_refs.into_iter().partition(|obj| { - let ptr = GcObjectPtr(core::ptr::NonNull::from(obj.as_ref())); + let ptr = GcPtr(core::ptr::NonNull::from(obj.as_ref())); resurrected_set.contains(&ptr) }); - let resurrected_count = resurrected.len(); - if debug.contains(GcDebugFlags::STATS) { eprintln!( "gc: {} resurrected, {} truly dead", - resurrected_count, + resurrected.len(), truly_dead.len() ); } - // 6e: Break cycles ONLY for truly dead objects (not resurrected) - // Compute collected count: exclude instance dicts that are also in truly_dead. - // In CPython 3.12+, instance dicts are managed inline and not separately tracked, - // so they don't count toward the collected total. + // Compute collected count (exclude instance dicts in truly_dead) let collected = { let dead_ptrs: HashSet = truly_dead .iter() @@ -672,7 +602,16 @@ impl GcState { truly_dead.len() - instance_dict_count }; - // 6e-1: If DEBUG_SAVEALL is set, save truly dead objects to garbage + // Promote survivors to next generation BEFORE tp_clear. + // This matches CPython's order (move_legacy_finalizer_reachable → delete_garbage) + // and ensures survivor_refs are dropped before tp_clear, so reachable objects + // (e.g. LateFin) aren't kept alive beyond the deferred-drop phase. + self.promote_survivors(generation, &survivor_refs); + drop(survivor_refs); + + // Resurrected objects stay tracked — just drop our references + drop(resurrected); + if debug.contains(GcDebugFlags::SAVEALL) { let mut garbage_guard = self.garbage.lock(); for obj_ref in truly_dead.iter() { @@ -681,12 +620,8 @@ impl GcState { } if !truly_dead.is_empty() { - // 6g: Break cycles by clearing references (tp_clear) - // Weakrefs were already cleared in step 6c, but new weakrefs created - // during __del__ (step 6d) can still be upgraded. - // - // Clear and destroy objects within a deferred drop context. - // This prevents deadlocks from untrack calls during destruction. + // Break cycles by clearing references (tp_clear) + // Use deferred drop context to prevent stack overflow. rustpython_common::refcount::with_deferred_drops(|| { for obj_ref in truly_dead.iter() { if obj_ref.gc_has_clear() { @@ -694,19 +629,11 @@ impl GcState { drop(edges); } } - // Drop truly_dead references, triggering actual deallocation drop(truly_dead); }); } - // 6f: Resurrected objects stay in tracked_objects (they're still alive) - // Just drop our references to them - drop(resurrected); - - // Promote survivors (reachable objects) to next generation - self.promote_survivors(generation, &reachable); - - // Reset gen0 count after collection (enables automatic GC to trigger again) + // Reset gen0 count self.generations[0].count.store(0, Ordering::SeqCst); self.generations[generation].update_stats(collected, 0); @@ -714,39 +641,49 @@ impl GcState { (collected, 0) } - /// Promote surviving objects to the next generation - fn promote_survivors(&self, from_gen: usize, survivors: &HashSet) { + /// Promote surviving objects to the next generation. + /// + /// `survivors` must be strong references (`PyObjectRef`) to keep objects alive, + /// since the generation read locks are released before this is called. + /// + /// Holds both source and destination list locks simultaneously to prevent + /// a race where concurrent `untrack_object` reads a stale `gc_generation` + /// and operates on the wrong list. + fn promote_survivors(&self, from_gen: usize, survivors: &[PyObjectRef]) { if from_gen >= 2 { return; // Already in oldest generation } let next_gen = from_gen + 1; - for &ptr in survivors { - // Remove from current generation - for gen_idx in 0..=from_gen { - let mut gen_set = self.generation_objects[gen_idx].write(); - if gen_set.remove(&ptr) { - // Decrement count for source generation - let count = self.generations[gen_idx].count.load(Ordering::SeqCst); - if count > 0 { - self.generations[gen_idx] - .count - .fetch_sub(1, Ordering::SeqCst); - } + for obj_ref in survivors { + let obj = obj_ref.as_ref(); + let ptr = NonNull::from(obj); + let obj_gen = obj.gc_generation(); + if obj_gen as usize <= from_gen && obj_gen <= 2 { + let src_gen = obj_gen as usize; + + // Lock both source and destination lists simultaneously. + // Always ascending order (src_gen < next_gen) → no deadlock. + let mut src = self.generation_lists[src_gen].write(); + let mut dst = self.generation_lists[next_gen].write(); + + // Re-check under locks: object might have been untracked concurrently + if obj.gc_generation() != obj_gen || !obj.is_gc_tracked() { + continue; + } - // Release before acquiring next lock - drop(gen_set); + if unsafe { src.remove(ptr) }.is_some() { + self.generations[src_gen] + .count + .fetch_sub(1, Ordering::SeqCst); - // Add to next generation - let mut next_set = self.generation_objects[next_gen].write(); - if next_set.insert(ptr) { - // Increment count for target generation - self.generations[next_gen] - .count - .fetch_add(1, Ordering::SeqCst); - } - break; + dst.push_front(ptr); + self.generations[next_gen] + .count + .fetch_add(1, Ordering::SeqCst); + + obj.set_gc_generation(next_gen as u8); } } } @@ -757,47 +694,44 @@ impl GcState { self.permanent.count() } - /// Freeze all tracked objects (move to permanent generation) + /// Freeze all tracked objects (move to permanent generation). + /// Lock order: generation_lists[i] → permanent_list (consistent with unfreeze). pub fn freeze(&self) { - // Move all objects from gen0-2 to permanent - let mut objects_to_freeze: Vec = Vec::new(); - - for (gen_idx, generation) in self.generation_objects.iter().enumerate() { - let mut gen_set = generation.write(); - objects_to_freeze.extend(gen_set.drain()); + let mut count = 0usize; + + for (gen_idx, gen_list) in self.generation_lists.iter().enumerate() { + let mut list = gen_list.write(); + let mut perm = self.permanent_list.write(); + while let Some(ptr) = list.pop_front() { + perm.push_front(ptr); + unsafe { ptr.as_ref().set_gc_generation(GC_PERMANENT) }; + count += 1; + } self.generations[gen_idx].count.store(0, Ordering::SeqCst); } - // Add to permanent set - let mut permanent = self.permanent_objects.write(); - let count = objects_to_freeze.len(); - for ptr in objects_to_freeze { - permanent.insert(ptr); - } self.permanent.count.fetch_add(count, Ordering::SeqCst); } - /// Unfreeze all objects (move from permanent to gen2) + /// Unfreeze all objects (move from permanent to gen2). + /// Lock order: generation_lists[2] → permanent_list (consistent with freeze). pub fn unfreeze(&self) { - let mut objects_to_unfreeze: Vec = Vec::new(); + let mut count = 0usize; { - let mut permanent = self.permanent_objects.write(); - objects_to_unfreeze.extend(permanent.drain()); + let mut gen2 = self.generation_lists[2].write(); + let mut perm_list = self.permanent_list.write(); + while let Some(ptr) = perm_list.pop_front() { + gen2.push_front(ptr); + unsafe { ptr.as_ref().set_gc_generation(2) }; + count += 1; + } self.permanent.count.store(0, Ordering::SeqCst); } - // Add to generation 2 - let mut gen2 = self.generation_objects[2].write(); - let count = objects_to_unfreeze.len(); - for ptr in objects_to_unfreeze { - gen2.insert(ptr); - } self.generations[2].count.fetch_add(count, Ordering::SeqCst); } - /// Force-unlock all locks after fork() in the child process. - /// /// Reset all locks to unlocked state after fork(). /// /// After fork(), only the forking thread survives. Any lock held by another @@ -820,12 +754,10 @@ impl GcState { } self.permanent.reinit_stats_after_fork(); - for rw in &self.generation_objects { + for rw in &self.generation_lists { reinit_rwlock_after_fork(rw); } - reinit_rwlock_after_fork(&self.permanent_objects); - reinit_rwlock_after_fork(&self.tracked_objects); - reinit_rwlock_after_fork(&self.finalized_objects); + reinit_rwlock_after_fork(&self.permanent_list); } } } diff --git a/crates/vm/src/object/core.rs b/crates/vm/src/object/core.rs index f4e458e1484..fce1eaf7e35 100644 --- a/crates/vm/src/object/core.rs +++ b/crates/vm/src/object/core.rs @@ -169,21 +169,23 @@ pub(super) unsafe fn default_dealloc(obj: *mut PyObject) { let vtable = obj_ref.0.vtable; // Untrack from GC BEFORE deallocation. + // Must happen before memory is freed because intrusive list removal + // reads the object's gc_pointers (prev/next). if obj_ref.is_gc_tracked() { let ptr = unsafe { NonNull::new_unchecked(obj) }; - if T::HAS_FREELIST { - // Freelist types must untrack immediately to avoid race conditions: - // a deferred untrack could remove a re-tracked entry after reuse. - unsafe { crate::gc_state::gc_state().untrack_object(ptr) }; - } else { - rustpython_common::refcount::try_defer_drop(move || { - // untrack_object only removes the pointer address from a HashSet. - // It does NOT dereference the pointer, so it's safe even after deallocation. - unsafe { - crate::gc_state::gc_state().untrack_object(ptr); - } - }); + unsafe { + crate::gc_state::gc_state().untrack_object(ptr); } + // Verify untrack cleared the tracked flag and generation + debug_assert!( + !obj_ref.is_gc_tracked(), + "object still tracked after untrack_object" + ); + debug_assert_eq!( + obj_ref.gc_generation(), + crate::object::GC_UNTRACKED, + "gc_generation not reset after untrack_object" + ); } // Extract child references before deallocation to break circular refs (tp_clear) @@ -257,6 +259,33 @@ bitflags::bitflags! { } } +/// GC generation constants +pub(crate) const GC_UNTRACKED: u8 = 0xFF; +pub(crate) const GC_PERMANENT: u8 = 3; + +/// Link implementation for GC intrusive linked list tracking +pub(crate) struct GcLink; + +// SAFETY: PyObject (PyInner) is heap-allocated and pinned in memory +// once created. gc_pointers is at a fixed offset in PyInner. +unsafe impl Link for GcLink { + type Handle = NonNull; + type Target = PyObject; + + fn as_raw(handle: &NonNull) -> NonNull { + *handle + } + + unsafe fn from_raw(ptr: NonNull) -> NonNull { + ptr + } + + unsafe fn pointers(target: NonNull) -> NonNull> { + let inner_ptr = target.as_ptr() as *mut PyInner; + unsafe { NonNull::new_unchecked(&raw mut (*inner_ptr).gc_pointers) } + } +} + /// This is an actual python object. It consists of a `typ` which is the /// python class, and carries some rust payload optionally. This rust /// payload can be a rust float or rust int in case of float and int objects. @@ -266,6 +295,11 @@ pub(super) struct PyInner { pub(super) vtable: &'static PyObjVTable, /// GC bits for free-threading (like ob_gc_bits) pub(super) gc_bits: PyAtomic, + /// GC generation index (0-2=gen, GC_PERMANENT=permanent, GC_UNTRACKED=not tracked). + /// Uses PyAtomic for interior mutability (writes happen through &self under list locks). + pub(super) gc_generation: PyAtomic, + /// Intrusive linked list pointers for GC generational tracking + pub(super) gc_pointers: Pointers, pub(super) typ: PyAtomicRef, // __class__ member pub(super) dict: Option, @@ -810,6 +844,8 @@ impl PyInner { ref_count: RefCount::new(), vtable: PyObjVTable::of::(), gc_bits: Radium::new(0), + gc_generation: Radium::new(GC_UNTRACKED), + gc_pointers: Pointers::new(), typ: PyAtomicRef::from(typ), dict: dict.map(InstanceDict::new), weak_list: WeakRefList::new(), @@ -1199,7 +1235,7 @@ impl PyObject { /// Mark the object as finalized. Should be called before __del__. /// _PyGC_SET_FINALIZED in Py_GIL_DISABLED mode. #[inline] - fn set_gc_finalized(&self) { + pub(crate) fn set_gc_finalized(&self) { self.set_gc_bit(GcBits::FINALIZED); } @@ -1209,6 +1245,19 @@ impl PyObject { self.0.gc_bits.fetch_or(bit.bits(), Ordering::Relaxed); } + /// Get the GC generation index for this object. + #[inline] + pub(crate) fn gc_generation(&self) -> u8 { + self.0.gc_generation.load(Ordering::Relaxed) + } + + /// Set the GC generation index for this object. + /// Must only be called while holding the generation list's write lock. + #[inline] + pub(crate) fn set_gc_generation(&self, generation: u8) { + self.0.gc_generation.store(generation, Ordering::Relaxed); + } + /// _PyObject_GC_TRACK #[inline] pub(crate) fn set_gc_tracked(&self) { @@ -2028,6 +2077,8 @@ pub(crate) fn init_type_hierarchy() -> (PyTypeRef, PyTypeRef, PyTypeRef) { ref_count: RefCount::new(), vtable: PyObjVTable::of::(), gc_bits: Radium::new(0), + gc_generation: Radium::new(GC_UNTRACKED), + gc_pointers: Pointers::new(), dict: None, weak_list: WeakRefList::new(), payload: type_payload, @@ -2040,6 +2091,8 @@ pub(crate) fn init_type_hierarchy() -> (PyTypeRef, PyTypeRef, PyTypeRef) { ref_count: RefCount::new(), vtable: PyObjVTable::of::(), gc_bits: Radium::new(0), + gc_generation: Radium::new(GC_UNTRACKED), + gc_pointers: Pointers::new(), dict: None, weak_list: WeakRefList::new(), payload: object_payload, diff --git a/crates/vm/src/object/mod.rs b/crates/vm/src/object/mod.rs index a279201a0b0..56db97aef1d 100644 --- a/crates/vm/src/object/mod.rs +++ b/crates/vm/src/object/mod.rs @@ -8,4 +8,5 @@ pub use self::core::*; pub use self::ext::*; pub use self::payload::*; pub(crate) use core::SIZEOF_PYOBJECT_HEAD; +pub(crate) use core::{GC_PERMANENT, GC_UNTRACKED, GcLink}; pub use traverse::{MaybeTraverse, Traverse, TraverseFn};