From 4f7014537f60f48bea7ab2501b88be15787491f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Tue, 28 Jul 2026 19:00:13 +0200 Subject: [PATCH 1/2] fix(gc): minor sweep must not finalize unmarked old-gen objects (#6892) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A minor trace never marks the old generation: old-gen parents are black leaves, reached only through dirty remembered-set pages. "Unmarked" in a minor therefore means "unvisited", not "dead" — the sweep's own `retain_all_forwarded_stubs` rule already spells this out for forwarding stubs, and cycle.rs says it again ("Minor traces never mark the old generation, so deadness there is only trusted for untenured nursery headers"). `ArenaSweepObjectsState::process_object` did not honour it for ordinary objects: any unmarked old-gen object took the `reclaim_dead_object` path. A minor never frees old-gen memory, so this looked harmless — but `reclaim_dead_object` still runs `finalize_dead_arena_payload` on the object, and that calls `layout_clear_for_ptr`, which drops the object's entry from `LAYOUT_SLOT_MASKS` (plus its payload side tables and external payload buffers). The live old-gen object is left in state `GC_LAYOUT_SIDE_MASK` with no mask. The next `layout_note_slot` on it rebuilds a mask from that single slot, so the following trace visits only that one element and skips every other pointer slot — and the children behind them are swept while still referenced. The mutator then reads a recycled address and gets `undefined`, which is why the reported symptom was a varying `Cannot read properties of undefined (reading '')`. Fix: `unmarked_is_provably_dead()` — an unmarked object is only provably dead when the trace covered its generation. Old-gen blocks are exempt in a minor sweep, except blocks selected for old-page defrag this cycle, whose live contents were evacuated during the same cycle (and whose block-level reclaim needs `block_has_live` to stay false). Full traces are unchanged: they visit every live parent, so mark-based old-gen reclaim stays sound. Renames `retain_all_forwarded_stubs` to `minor_sweep`, which is what every call site already passes and what both rules now key off. Repro (issue #6892): the Milo compiler built by Perry, running `emit-ir` over a file importing `std/fetch`. Before: throws. After: exits 0 and its LLVM IR is byte-identical to the same compiler under bun. The env matrix that motivated the report (`PERRY_GEN_GC=0` and `PERRY_WRITE_BARRIERS=0` pass, `PERRY_GEN_GC_EVACUATE=0` and `PERRY_GC_VERIFY_EVACUATION=1` fail) is now uniformly green, as are `PERRY_GC_FORCE_EVACUATE=1` and the two stress flags combined. Note on old-page defrag input: minors previously fed `old_page_account_swept_object(live=false)` for every unmarked old object, i.e. phantom dead bytes derived from mark bits that say nothing about the old generation. They now report those objects live, so the defrag selector's dead-byte signal comes from full traces, where it is real. --- crates/perry-runtime/src/gc/oldgen.rs | 85 +++++++++++++++----- crates/perry-runtime/src/gc/tests/oldgen.rs | 87 +++++++++++++++++++++ 2 files changed, 151 insertions(+), 21 deletions(-) diff --git a/crates/perry-runtime/src/gc/oldgen.rs b/crates/perry-runtime/src/gc/oldgen.rs index 1ffe7c376a..28f9b4dbfd 100644 --- a/crates/perry-runtime/src/gc/oldgen.rs +++ b/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>, 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>, 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>, ) -> 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)); diff --git a/crates/perry-runtime/src/gc/tests/oldgen.rs b/crates/perry-runtime/src/gc/tests/oldgen.rs index 188a60c44c..a46883567b 100644 --- a/crates/perry-runtime/src/gc/tests/oldgen.rs +++ b/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(); +} From 9a222726372fb5c622a6ddc4c40f5bc43105435c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Tue, 28 Jul 2026 19:14:42 +0200 Subject: [PATCH 2/2] docs: changelog fragment for #6933 --- .../6933-minor-sweep-old-gen-finalize.md | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 changelog.d/6933-minor-sweep-old-gen-finalize.md diff --git a/changelog.d/6933-minor-sweep-old-gen-finalize.md b/changelog.d/6933-minor-sweep-old-gen-finalize.md new file mode 100644 index 0000000000..70c9600dfd --- /dev/null +++ b/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 '')`. + +`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.