fix(gc): minor sweep must not finalize unmarked old-gen objects (#6892)

changelog.d/6933-minor-sweep-old-gen-finalize.md

@@ -0,0 +1,34 @@
+fix(gc): a minor sweep no longer finalizes unmarked old-generation objects (#6892).
+
+A minor trace never marks the old generation — old-gen parents are black
+leaves, visited only through dirty remembered-set pages — so "unmarked" in a
+minor means *unvisited*, not *dead*. `ArenaSweepObjectsState::process_object`
+sent every unmarked old-gen object down `reclaim_dead_object` anyway. A minor
+never frees old-gen memory, so this stayed latent; the damage was
+`finalize_dead_arena_payload`, whose `layout_clear_for_ptr` dropped the live
+object's `LAYOUT_SLOT_MASKS` entry (plus its payload side tables and external
+payload buffers).
+
+That left a live old-gen object in `GC_LAYOUT_SIDE_MASK` state with no mask.
+The next `layout_note_slot` rebuilt the mask from that single slot, so the
+following trace visited one element and skipped the object's other pointer
+slots — sweeping children that were still referenced. Reads then hit a
+recycled address and returned `undefined`, which is why the reported symptom
+was a varying `Cannot read properties of undefined (reading '<field>')`.
+
+`unmarked_is_provably_dead()` now gates reclaim on "the trace covered this
+object's generation": old-gen blocks are exempt during a minor sweep, except
+blocks selected for old-page defrag in the same cycle, whose live contents
+were evacuated by that cycle. Full traces are unchanged. The sweep's
+`retain_all_forwarded_stubs` flag is renamed `minor_sweep`, which is what
+every call site already passed and what both retention rules now key off.
+
+Found via the Milo compiler built by Perry: `emit-ir` over a file importing
+`std/fetch` threw, and its LLVM IR is now byte-identical to the same compiler
+under bun. 51/53 of milo's examples now match byte-for-byte.
+
+Side effect worth knowing: minors used to report every unmarked old object as
+dead bytes to `old_page_account_swept_object`, i.e. phantom fragmentation
+derived from mark bits that say nothing about the old generation. They now
+report those objects live, so the old-page defrag selector's dead-byte signal
+comes from full traces, where it is real.

crates/perry-runtime/src/gc/oldgen.rs

@@ -829,7 +829,7 @@ fn sweep_with_age_bump_and_old_reclaim_targets(
 ) -> SweepTraceStats {
     // These synchronous wrappers age-bump exactly when sweeping a MINOR
     // trace, so `do_age_bump` doubles as the minor-ness signal for the
-    // forwarded-stub retention rule (see `retain_all_forwarded_stubs`).
+    // old-gen retention rules (see `minor_sweep`).
     let mut state = IncrementalSweepState::new(
         do_age_bump,
         reclaim_dead_old_blocks,
@@ -1198,7 +1198,7 @@ impl IncrementalSweepState {
         reclaim_dead_old_blocks: bool,
         targeted_old_blocks: Option<crate::fast_hash::PtrHashSet<usize>>,
         sweep_malloc: bool,
-        retain_all_forwarded_stubs: bool,
+        minor_sweep: bool,
     ) -> Self {
         Self {
             subphase: SweepCycleSubphase::Malloc,
@@ -1210,7 +1210,8 @@ impl IncrementalSweepState {
             arena: ArenaSweepObjectsState::new(
                 do_age_bump,
                 reclaim_dead_old_blocks,
-                retain_all_forwarded_stubs,
+                minor_sweep,
+                targeted_old_blocks.clone(),
             ),
             cleanup: None,
             reclaim_dead_old_blocks,
@@ -1341,17 +1342,34 @@ struct ArenaSweepObjectsState {
     overflow_active: bool,
     do_age_bump: bool,
     reclaim_dead_old_blocks: bool,
-    /// Minor sweeps must retain EVERY forwarding stub: array growth installs
-    /// PERMANENT stubs (#6228 — stale pre-growth pointers keep resolving for
-    /// reads, references are never rewritten), and a minor treats old-gen
-    /// parents as black leaves whose slots are only visited via dirty pages.
-    /// An old parent (e.g. a long-lived Map's entries buffer) whose page is
-    /// no longer dirty never marks the stub its slot points at, so
-    /// "unmarked stub" does NOT imply "unreferenced" in a minor — reclaiming
-    /// it is a use-after-free (reads through the stale pointer return
-    /// reused-memory garbage). Full traces DO visit every live parent, so
-    /// mark-based stub reclaim stays sound (and bounds the accumulation).
-    retain_all_forwarded_stubs: bool,
+    /// This sweep follows a MINOR trace, whose mark bits say nothing about the
+    /// old generation: old-gen parents are black leaves whose slots are only
+    /// visited through dirty remembered-set pages, so an object reachable only
+    /// from a non-dirty old parent is never marked. "Unmarked" therefore does
+    /// NOT imply "dead" for anything in the old generation.
+    ///
+    /// Two consequences, both handled below:
+    ///
+    /// * Forwarding stubs must ALL be retained: array growth installs
+    ///   PERMANENT stubs (#6228 — stale pre-growth pointers keep resolving for
+    ///   reads, references are never rewritten). An old parent (e.g. a
+    ///   long-lived Map's entries buffer) whose page is no longer dirty never
+    ///   marks the stub its slot points at, so reclaiming it is a
+    ///   use-after-free.
+    /// * Ordinary old-gen objects must not be reclaimed either (#6892). The
+    ///   minor never frees their memory, but `reclaim_dead_object` still runs
+    ///   `finalize_dead_arena_payload` on them, which wipes a LIVE object's GC
+    ///   slot-layout mask and payload side tables and frees its external
+    ///   payload buffers.
+    ///
+    /// Full traces DO visit every live parent, so mark-based reclaim stays
+    /// sound there (and bounds the accumulation).
+    minor_sweep: bool,
+    /// Old-gen blocks selected for page defrag this cycle. Their live contents
+    /// were evacuated out during this same cycle, so what is left really is
+    /// reclaimable even in a minor — and the block-level reclaim needs
+    /// `block_has_live` to stay false for them.
+    targeted_old_blocks: Option<crate::fast_hash::PtrHashSet<usize>>,
     freed_bytes: u64,
     retained_forwarded_stub_objects: usize,
     retained_forwarded_stub_bytes: usize,
@@ -1361,7 +1379,8 @@ impl ArenaSweepObjectsState {
     fn new(
         do_age_bump: bool,
         reclaim_dead_old_blocks: bool,
-        retain_all_forwarded_stubs: bool,
+        minor_sweep: bool,
+        targeted_old_blocks: Option<crate::fast_hash::PtrHashSet<usize>>,
     ) -> Self {
         let n_blocks = crate::arena::arena_block_count();
         let block_snapshots = crate::arena::arena_block_snapshots();
@@ -1378,7 +1397,8 @@ impl ArenaSweepObjectsState {
                 || crate::closure::closure_dynamic_side_tables_nonempty(),
             do_age_bump,
             reclaim_dead_old_blocks,
-            retain_all_forwarded_stubs,
+            minor_sweep,
+            targeted_old_blocks,
             freed_bytes: 0,
             retained_forwarded_stub_objects: 0,
             retained_forwarded_stub_bytes: 0,
@@ -1443,13 +1463,36 @@ impl ArenaSweepObjectsState {
                 self.process_forwarded_object(header, block_idx, flags);
                 return;
             }
-            if flags & GC_FLAG_MARKED == 0 {
+            if flags & GC_FLAG_MARKED == 0 && self.unmarked_is_provably_dead(block_idx) {
                 self.reclaim_dead_object(header, block_idx);
             } else {
                 self.keep_live_object(header, block_idx, flags, age_bump_this, false);
             }
         }
     }
+
+    /// Does `flags & MARKED == 0` actually prove this object is garbage?
+    ///
+    /// Only when the trace that produced the marks covered the object's
+    /// generation. A minor trace never marks the old generation (see
+    /// `minor_sweep`), so an unmarked old-gen object is merely *unvisited* —
+    /// it stays live and must not be finalized. #6892: reclaiming one wiped
+    /// the GC slot-layout mask of a live old-gen array, after which the next
+    /// `layout_note_slot` rebuilt the mask from a single slot and the
+    /// following minor stopped tracing the array's other pointer elements,
+    /// sweeping objects that were still referenced.
+    ///
+    /// The old-page defrag targets are exempt: this cycle evacuated their live
+    /// contents, so the remainder is genuinely reclaimable.
+    #[inline]
+    fn unmarked_is_provably_dead(&self, block_idx: usize) -> bool {
+        if !self.minor_sweep || block_idx < self.old_block_start {
+            return true;
+        }
+        self.targeted_old_blocks
+            .as_ref()
+            .is_some_and(|selected| selected.contains(&block_idx))
+    }
 }
 
 impl ArenaSweepObjectsState {
@@ -1490,10 +1533,10 @@ impl ArenaSweepObjectsState {
         block_idx: usize,
         flags: u8,
     ) {
-        // See `retain_all_forwarded_stubs`: a minor cannot prove a stub
-        // unreferenced (old-gen parents are black leaves), so it must keep
-        // them all; a full trace reclaims the genuinely unreferenced ones.
-        let retain_stub = self.retain_all_forwarded_stubs
+        // See `minor_sweep`: a minor cannot prove a stub unreferenced (old-gen
+        // parents are black leaves), so it must keep them all; a full trace
+        // reclaims the genuinely unreferenced ones.
+        let retain_stub = self.minor_sweep
             || flags & GC_FLAG_MARKED != 0
             || (block_idx < self.resettable_general_n
                 && crate::arena::general_block_in_recent_window(block_idx));

crates/perry-runtime/src/gc/tests/oldgen.rs

@@ -1230,3 +1230,90 @@ fn test_minor_preserves_old_to_young_edge_across_minors() {
     clear_marks();
     remembered_set_clear();
 }
+
+/// #6892 — a MINOR sweep must not finalize an unmarked OLD-generation object.
+///
+/// Minor traces never mark the old generation (old-gen parents are black
+/// leaves, reached only through dirty remembered-set pages), so "unmarked" in
+/// a minor means "unvisited", not "dead". Treating such an object as garbage
+/// ran `finalize_dead_arena_payload` on a live object, whose
+/// `layout_clear_for_ptr` wiped its GC slot-layout mask. The next
+/// `layout_note_slot` then rebuilt the mask from a single slot, so the
+/// following trace stopped visiting the object's other pointer slots and swept
+/// children that were still referenced.
+#[test]
+fn test_minor_sweep_keeps_unmarked_old_object_layout_mask() {
+    let _isolation = copying_nursery_isolation_lock();
+    reset_remembered_set();
+    clear_marks();
+    clear_mark_seeds();
+    crate::arena::old_pages_begin_gc_cycle();
+
+    // A live old-gen object with one pointer slot, plus the slot-layout mask
+    // the collector reads to find that pointer.
+    let old_obj = crate::arena::arena_alloc_gc_old(3 * 8, 8, GC_TYPE_OBJECT) as usize;
+    unsafe {
+        std::ptr::write_bytes(old_obj as *mut u8, 0, 3 * 8);
+        // A freshly allocated payload starts pointer-free; the first pointer
+        // store is what promotes it to a side mask.
+        crate::gc::layout_init_pointer_free(old_obj as *mut u8);
+    }
+    let child = crate::arena::arena_alloc_gc_old(16, 8, GC_TYPE_STRING) as usize;
+    layout_note_slot(old_obj, 0, string_bits(child));
+    assert_eq!(
+        test_layout_pointer_slot_count(old_obj, 3),
+        Some(1),
+        "precondition: the old object starts with a one-pointer slot mask"
+    );
+
+    // A minor sweep. `old_obj` is deliberately left UNMARKED — that is exactly
+    // the state a minor trace leaves every old-gen object in.
+    let mut sweep = IncrementalSweepState::new(true, false, None, false, true);
+    let _ = sweep.finish_unbounded();
+
+    assert_eq!(
+        test_layout_pointer_slot_count(old_obj, 3),
+        Some(1),
+        "#6892: minor sweep wiped the slot-layout mask of a live old-gen object"
+    );
+
+    clear_marks();
+    remembered_set_clear();
+}
+
+/// Converse of `test_minor_sweep_keeps_unmarked_old_object_layout_mask`: a FULL
+/// trace does visit every live parent, so unmarked really does mean dead there
+/// and old-gen reclamation must still happen. Guards the #6892 fix against
+/// being widened into "never reclaim the old generation".
+#[test]
+fn test_full_sweep_still_finalizes_unmarked_old_object() {
+    let _isolation = copying_nursery_isolation_lock();
+    reset_remembered_set();
+    clear_marks();
+    clear_mark_seeds();
+    crate::arena::old_pages_begin_gc_cycle();
+
+    let old_obj = crate::arena::arena_alloc_gc_old(3 * 8, 8, GC_TYPE_OBJECT) as usize;
+    unsafe {
+        std::ptr::write_bytes(old_obj as *mut u8, 0, 3 * 8);
+        // A freshly allocated payload starts pointer-free; the first pointer
+        // store is what promotes it to a side mask.
+        crate::gc::layout_init_pointer_free(old_obj as *mut u8);
+    }
+    let child = crate::arena::arena_alloc_gc_old(16, 8, GC_TYPE_STRING) as usize;
+    layout_note_slot(old_obj, 0, string_bits(child));
+    assert_eq!(test_layout_pointer_slot_count(old_obj, 3), Some(1));
+
+    // Full trace (`minor_sweep = false`): unmarked is provably dead.
+    let mut sweep = IncrementalSweepState::new(false, true, None, false, false);
+    let _ = sweep.finish_unbounded();
+
+    assert_eq!(
+        test_layout_pointer_slot_count(old_obj, 3),
+        None,
+        "a full sweep must still finalize genuinely dead old-gen objects"
+    );
+
+    clear_marks();
+    remembered_set_clear();
+}