diff --git a/changelog.d/7041-gc-whole-heap-fromspace-scan.md b/changelog.d/7041-gc-whole-heap-fromspace-scan.md new file mode 100644 index 0000000000..fea903a8b2 --- /dev/null +++ b/changelog.d/7041-gc-whole-heap-fromspace-scan.md @@ -0,0 +1,44 @@ +### GC: whole-heap from-space scan (`PERRY_GC_FROMSPACE_SCAN`) + +Adds a debug-only verification pass that answers, after the copying minor's +rewrite pass and before from-space is reset: + +> does any live object OUTSIDE from-space still contain a word that decodes to a +> from-space address? + +It walks the whole heap — the arena census plus the malloc-tracked registry — +and decodes every payload word through `decode_root_word`, the decoder the mark +and rewrite paths already share (#6910). Objects that are themselves in +from-space are skipped: a dead object legitimately still points at its dead +peers. + +**Why this is not redundant with `PERRY_GC_VERIFY_EVACUATION` (#7035).** The +existing verifier walks the same surfaces the *rewrite pass* walks, so it can +only check that the rewrite pass agreed with itself; a holder the rewrite pass +never enumerated is invisible to both. On the #7022 reproducer the verifier +reports clean while the program dies of malloc-heap corruption. This pass does +not depend on any root enumeration, and on that same reproducer it names the +offending holders on the second collection. + +Each offender is classified along two axes that have different fixes: + +- **missing rewrite** (target carries `GC_FLAG_FORWARDED` — it moved and this + reference was not updated) vs **dangling** (target was never evacuated and is + about to be recycled); +- remembered-set coverage of the slot's page — `never_dirty` (a store path + skipped the write barrier), `lost_dirty` (the edge was recorded and then + lost), `dirty_but_missed` (the page is dirty and the slot was still missed). + This reuses the existing `EVER_DIRTY_OLD_PAGES` tracking, which is now enabled + by this scan as well as by `PERRY_GC_VERIFY_EVACUATION`. + +`PERRY_GC_FROMSPACE_SCAN_ABORT=1` additionally aborts on the first offender so a +crash report captures the offending cycle rather than the downstream corruption. + +O(live heap) per collection, so it is strictly opt-in and off by default. + +Three unit tests cover both directions — that the scan finds a planted +un-rewritten old→young reference, that it stops reporting once the reference is +removed, that it separates dangling from missing-rewrite, and that it ignores +references held *by* from-space objects. Teeth verified by sabotage: blinding +the from-space predicate turns two of the three red with the exact +"planted=0" signature. diff --git a/crates/perry-runtime/src/gc/barrier.rs b/crates/perry-runtime/src/gc/barrier.rs index a8ac92d9fe..a4d93f6d89 100644 --- a/crates/perry-runtime/src/gc/barrier.rs +++ b/crates/perry-runtime/src/gc/barrier.rs @@ -1296,7 +1296,10 @@ thread_local! { fn ever_dirty_tracking_enabled() -> bool { use std::sync::OnceLock; static CACHED: OnceLock = OnceLock::new(); - *CACHED.get_or_init(|| std::env::var_os("PERRY_GC_VERIFY_EVACUATION").is_some()) + *CACHED.get_or_init(|| { + std::env::var_os("PERRY_GC_VERIFY_EVACUATION").is_some() + || super::fromspace_scan::fromspace_scan_enabled() + }) } fn ever_dirty_note(page: usize) { @@ -1859,3 +1862,17 @@ pub fn remembered_set_clear() { let mut state = RememberedSetClearState::new(); while !state.step(usize::MAX) {} } + +/// #7035: is `addr`'s old page currently in the remembered set? +pub(super) fn dirty_now_for_addr(addr: usize) -> bool { + DIRTY_OLD_PAGES.with(|s| { + s.borrow() + .contains(&crate::arena::generation_page_for_addr(addr)) + }) +} + +/// #7035: was `addr`'s old page EVER dirtied? Distinguishes "barrier never ran" +/// from "edge was recorded then lost". +pub(super) fn ever_dirty_for_addr(addr: usize) -> bool { + ever_dirty_old_page(crate::arena::generation_page_for_addr(addr)) +} diff --git a/crates/perry-runtime/src/gc/copying.rs b/crates/perry-runtime/src/gc/copying.rs index cebb8e5c74..62f9ca5094 100644 --- a/crates/perry-runtime/src/gc/copying.rs +++ b/crates/perry-runtime/src/gc/copying.rs @@ -1192,6 +1192,11 @@ pub(super) fn gc_collect_minor_copying_fast_path_with_eligibility( super::verify::verify_marked_heap_report_nonfatal("copying-minor"); } + // #7035: whole-heap from-space scan. MUST run here — after the rewrite + // pass, before from-space is reset — and it is deliberately independent of + // the root enumeration the rewrite pass and the evacuation verifier share. + super::fromspace_scan::run_fromspace_scan(); + crate::promise::cleanup_copied_minor_promise_contexts_for_gc(); finalize_dead_copied_minor_from_space_side_allocations(); let reset = crate::arena::copying_reset_from_spaces_and_flip(); diff --git a/crates/perry-runtime/src/gc/fromspace_scan.rs b/crates/perry-runtime/src/gc/fromspace_scan.rs new file mode 100644 index 0000000000..a472bae291 --- /dev/null +++ b/crates/perry-runtime/src/gc/fromspace_scan.rs @@ -0,0 +1,305 @@ +//! Whole-heap from-space scan (#7035). +//! +//! # Why this exists +//! +//! `PERRY_GC_VERIFY_EVACUATION` walks the same surfaces the *rewrite pass* +//! walks — `visit_shadow_stack_root_slots`, `visit_global_root_slots`, and the +//! registered side-table scanners. Both derive from one enumeration, so the +//! verifier can only check that the rewrite pass agreed with itself: a holder +//! the rewrite pass never knew about is invisible to the verifier too. On the +//! #7022 reproducer the verifier reports clean while the program dies of malloc +//! heap corruption, which is exactly that blind spot. +//! +//! This pass takes the opposite approach and asks a question that does not +//! depend on any root enumeration: +//! +//! > After the rewrite pass, does any live object OUTSIDE from-space still +//! > contain a word that decodes to a from-space address? +//! +//! It answers it by walking the whole heap — the arena census (`Address`-order +//! `ArenaObjectCursor`, covering Eden / both survivor semispaces / old / long- +//! lived) plus the malloc-tracked registry — and decoding every payload word +//! through `decode_root_word`, the same decoder the mark and rewrite paths +//! share (#6910). Objects that are themselves in from-space are skipped: they +//! are about to be reclaimed, and a dead object legitimately still points at +//! its dead peers. +//! +//! A word whose target carries `GC_FLAG_FORWARDED` is an unambiguous **missing +//! rewrite**: the object moved this cycle and this reference was not updated. +//! A word pointing at a non-forwarded from-space object is a **dangling** +//! reference — the target was not evacuated at all, so it is about to be +//! recycled underneath this holder. +//! +//! # Cost and placement +//! +//! O(live heap) per collection, so it is strictly a debug instrument behind +//! `PERRY_GC_FROMSPACE_SCAN=1`. It must run after the rewrite pass and +//! **before** `copying_reset_from_spaces_and_flip`, while from-space is still +//! intact and page-registered — the same window the evacuation verifier uses. +//! +//! `PERRY_GC_FROMSPACE_SCAN_ABORT=1` additionally aborts on the first offender +//! so a debugger/crash report captures the offending cycle rather than the +//! downstream corruption. + +use super::*; + +/// One offending word found by the scan. +#[derive(Clone, Copy)] +pub(crate) struct FromSpaceRef { + /// Header of the object that CONTAINS the stale word. + pub(super) owner_header: usize, + pub(super) owner_obj_type: u8, + pub(super) owner_space: crate::arena::HeapSpace, + /// Byte offset of the stale word within the owner's payload. + pub(super) slot_offset: usize, + /// The from-space address the word decodes to. + pub(super) target: usize, + pub(super) target_space: crate::arena::HeapSpace, + /// True when the target carries `GC_FLAG_FORWARDED` — i.e. it MOVED and + /// this reference was simply not rewritten. + pub(super) target_forwarded: bool, + /// True when the word was NaN-boxed, false when it was a bare address. + pub(super) nanboxed: bool, + /// Remembered-set coverage of THIS SLOT's page, the decisive split: + /// `ever_dirty == false` -> a store path skipped the write barrier; + /// `ever_dirty && !dirty_now` -> the edge was recorded and then LOST. + pub(super) slot_dirty_now: bool, + pub(super) slot_ever_dirty: bool, + /// Owner's raw `gc_flags`, reported uninterpreted. NOTE: during a minor, + /// old-gen parents are NOT marked even when their slots are scanned through + /// the remembered set, so `GC_FLAG_MARKED` here does not mean "was scanned". + pub(super) owner_flags: u8, +} + +#[derive(Default)] +pub(crate) struct FromSpaceScanReport { + pub(crate) objects_scanned: usize, + pub(crate) words_scanned: usize, + pub(crate) missing_rewrites: usize, + pub(crate) dangling: usize, + /// Offending slots whose page was NEVER dirtied -> a store path skipped the + /// write barrier entirely. + pub(crate) never_dirty: usize, + /// Offending slots whose page was dirtied at some point but is not dirty + /// now -> the edge was recorded and then lost by a clear/restore gap. + pub(crate) lost_dirty: usize, + /// Offending slots whose page IS currently dirty -> the remembered set had + /// it and the scan still missed the slot. + pub(crate) dirty_but_missed: usize, + pub(crate) distinct_owners: crate::fast_hash::PtrHashSet, + pub(crate) samples: Vec, +} + +const MAX_SAMPLES: usize = 32; + +pub(super) fn fromspace_scan_enabled() -> bool { + use std::sync::OnceLock; + static CACHED: OnceLock = OnceLock::new(); + *CACHED.get_or_init(|| { + matches!( + std::env::var("PERRY_GC_FROMSPACE_SCAN").as_deref(), + Ok("1") | Ok("on") | Ok("true") + ) + }) +} + +fn fromspace_scan_abort() -> bool { + use std::sync::OnceLock; + static CACHED: OnceLock = OnceLock::new(); + *CACHED.get_or_init(|| { + matches!( + std::env::var("PERRY_GC_FROMSPACE_SCAN_ABORT").as_deref(), + Ok("1") | Ok("on") | Ok("true") + ) + }) +} + +/// Is `space` a from-space for the copying minor that just ran? Eden always is; +/// so is whichever survivor semispace was active going INTO the cycle (the +/// flip happens later, inside `copying_reset_from_spaces_and_flip`). +#[inline] +fn is_from_space(space: crate::arena::HeapSpace) -> bool { + space == crate::arena::HeapSpace::NurseryEden || space == crate::arena::active_survivor_space() +} + +/// Scan one object's payload for from-space references. +/// +/// # Safety +/// `header` must point at a live, walkable `GcHeader` whose `size` field covers +/// the allocation, which is what both the arena cursor and the malloc registry +/// guarantee. +unsafe fn scan_object(header: *mut GcHeader, report: &mut FromSpaceScanReport) { + let total = (*header).size as usize; + if total <= GC_HEADER_SIZE { + return; + } + let user = (header as *mut u8).add(GC_HEADER_SIZE); + let owner_space = crate::arena::classify_heap_space(user as usize); + // A from-space object is about to be reclaimed; it may legitimately still + // reference its dead peers. Only holders that SURVIVE the cycle matter. + if is_from_space(owner_space) { + return; + } + report.objects_scanned += 1; + + let payload_words = (total - GC_HEADER_SIZE) / 8; + let words = user as *const u64; + for i in 0..payload_words { + let bits = *words.add(i); + report.words_scanned += 1; + let Some(word) = super::root_words::decode_root_word(bits) else { + continue; + }; + let target = word.addr(); + let target_space = crate::arena::classify_heap_space(target); + if !is_from_space(target_space) { + continue; + } + // The target is in from-space. Did it move (missing rewrite) or was it + // never evacuated (dangling)? + let target_forwarded = if target >= GC_HEADER_SIZE { + let th = (target - GC_HEADER_SIZE) as *const GcHeader; + (*th).gc_flags & GC_FLAG_FORWARDED != 0 + } else { + false + }; + if target_forwarded { + report.missing_rewrites += 1; + } else { + report.dangling += 1; + } + report.distinct_owners.insert(header as usize); + let slot_addr = words.add(i) as usize; + let dirty_now = super::barrier::dirty_now_for_addr(slot_addr); + let ever_dirty = super::barrier::ever_dirty_for_addr(slot_addr); + if !ever_dirty { + report.never_dirty += 1; + } else if !dirty_now { + report.lost_dirty += 1; + } else { + report.dirty_but_missed += 1; + } + if report.samples.len() < MAX_SAMPLES { + report.samples.push(FromSpaceRef { + owner_header: header as usize, + owner_obj_type: (*header).obj_type, + owner_space, + slot_offset: i * 8, + target, + target_space, + target_forwarded, + nanboxed: matches!(word, super::root_words::RootWord::Nanboxed { .. }), + slot_dirty_now: super::barrier::dirty_now_for_addr(words.add(i) as usize), + slot_ever_dirty: super::barrier::ever_dirty_for_addr(words.add(i) as usize), + owner_flags: (*header).gc_flags, + }); + } + if fromspace_scan_abort() { + report_and_abort(report); + } + } +} + +/// Walk the whole heap and report every surviving reference into from-space. +pub(crate) fn scan_heap_for_fromspace_refs() -> FromSpaceScanReport { + let mut report = FromSpaceScanReport::default(); + + // --- arena census (Eden / survivors / old / longlived) ------------------ + let mut builder = + crate::arena::ArenaObjectCursorBuilder::new(crate::arena::ArenaWalkOrder::Address); + let mut cursor = loop { + let mut budget = usize::MAX; + if let Some(cursor) = builder.step(&mut budget) { + break cursor; + } + }; + loop { + let mut budget = usize::MAX; + match cursor.next_budgeted(&mut budget) { + Some((header_ptr, _block_idx)) => unsafe { + scan_object(header_ptr as *mut GcHeader, &mut report); + }, + None => { + if cursor.is_finished() { + break; + } + } + } + } + + // --- malloc-tracked objects -------------------------------------------- + let malloc_headers: Vec<*mut GcHeader> = + MALLOC_STATE.with(|s| s.borrow().objects.iter().copied().collect()); + for header in malloc_headers { + if header.is_null() { + continue; + } + unsafe { + scan_object(header, &mut report); + } + } + + report +} + +fn describe(r: &FromSpaceRef) -> String { + format!( + " owner={:#x} type={} space={:?} +{} {} -> {:#x} ({:?}) {} [slot dirty_now={} ever_dirty={} owner_flags={:#x} marked={}]", + r.owner_header, + r.owner_obj_type, + r.owner_space, + r.slot_offset, + if r.nanboxed { "nanbox" } else { "bare" }, + r.target, + r.target_space, + if r.target_forwarded { + "MISSING-REWRITE (target moved)" + } else { + "DANGLING (target not evacuated)" + }, + r.slot_dirty_now, + r.slot_ever_dirty, + r.owner_flags, + r.owner_flags & GC_FLAG_MARKED != 0 + ) +} + +fn report_and_abort(report: &FromSpaceScanReport) -> ! { + emit_report(report, "abort"); + panic!( + "gc from-space scan: {} missing rewrite(s), {} dangling reference(s) survived the rewrite pass", + report.missing_rewrites, report.dangling + ); +} + +pub(super) fn emit_report(report: &FromSpaceScanReport, phase: &str) { + eprintln!( + "[gc-fromspace-scan {}] objects={} words={} missing_rewrites={} dangling={} owners={} | never_dirty={} lost_dirty={} dirty_but_missed={}", + phase, + report.objects_scanned, + report.words_scanned, + report.missing_rewrites, + report.dangling, + report.distinct_owners.len(), + report.never_dirty, + report.lost_dirty, + report.dirty_but_missed + ); + for sample in &report.samples { + eprintln!("{}", describe(sample)); + } +} + +/// Entry point called from the copying minor, after the rewrite pass and before +/// `copying_reset_from_spaces_and_flip`. +pub(super) fn run_fromspace_scan() { + if !fromspace_scan_enabled() { + return; + } + let report = scan_heap_for_fromspace_refs(); + if report.missing_rewrites > 0 || report.dangling > 0 { + emit_report(&report, "OFFENDERS"); + } else { + emit_report(&report, "clean"); + } +} diff --git a/crates/perry-runtime/src/gc/mod.rs b/crates/perry-runtime/src/gc/mod.rs index 50ae8b0e70..8690685564 100644 --- a/crates/perry-runtime/src/gc/mod.rs +++ b/crates/perry-runtime/src/gc/mod.rs @@ -72,6 +72,11 @@ use oldgen::*; mod cycle; use cycle::*; mod verify; + +/// #7035: whole-heap from-space scan — verification that does NOT depend on +/// the rewrite pass own root enumeration. Debug-only +/// (`PERRY_GC_FROMSPACE_SCAN=1`). +mod fromspace_scan; pub use verify::*; #[cfg(feature = "diagnostics")] mod heap_snapshot; diff --git a/crates/perry-runtime/src/gc/tests/fromspace_scan.rs b/crates/perry-runtime/src/gc/tests/fromspace_scan.rs new file mode 100644 index 0000000000..4785999419 --- /dev/null +++ b/crates/perry-runtime/src/gc/tests/fromspace_scan.rs @@ -0,0 +1,145 @@ +//! Teeth for the whole-heap from-space scan (#7035). +//! +//! Both directions are asserted deliberately. A verification instrument that +//! silently reports clean is worse than no instrument at all — that is the +//! defect #7035 records about `PERRY_GC_VERIFY_EVACUATION`, and it cost real +//! bisect cycles on #7022 because "verifier clean" was read as a negative +//! result. So these tests check that the scan (a) FINDS a planted un-rewritten +//! old->young reference, and (b) does NOT report it once the reference is gone. +//! +//! Deltas rather than absolute counts: a test process shares one thread-local +//! heap with whatever else the surrounding test allocated, so only the change +//! attributable to the planted reference is meaningful. + +use super::super::fromspace_scan::*; +use super::super::*; + +/// Plant a NaN-boxed reference to `young` in `holder`'s first payload word. +/// +/// # Safety +/// `holder` must have at least 8 bytes of payload. +unsafe fn plant_reference(holder: *mut u8, young: *mut u8) { + *(holder as *mut u64) = crate::value::POINTER_TAG | (young as u64 & crate::value::POINTER_MASK); +} + +unsafe fn clear_reference(holder: *mut u8) { + *(holder as *mut u64) = 0; +} + +#[test] +fn fromspace_scan_finds_an_unrewritten_old_to_young_reference() { + // An old-gen holder is outside from-space, so the scan inspects it; a + // nursery target is inside from-space, so a surviving reference to it after + // the rewrite pass is exactly what the scan exists to catch. + let holder = crate::arena::arena_alloc_gc_old(64, 8, GC_TYPE_OBJECT); + let young = crate::arena::arena_alloc_gc(64, 8, GC_TYPE_OBJECT); + unsafe { + std::ptr::write_bytes(holder, 0, 64); + } + + let baseline = scan_heap_for_fromspace_refs(); + + // Plant the reference AND mark the target forwarded — i.e. the object moved + // this cycle and this reference was not updated. + unsafe { + plant_reference(holder, young); + let young_header = header_from_user_ptr(young) as *mut GcHeader; + (*young_header).gc_flags |= GC_FLAG_FORWARDED; + } + + let planted = scan_heap_for_fromspace_refs(); + assert!( + planted.missing_rewrites > baseline.missing_rewrites, + "the scan must report a planted un-rewritten old->young reference \ + (baseline missing_rewrites={}, planted={})", + baseline.missing_rewrites, + planted.missing_rewrites + ); + + // The negative direction: remove the reference and the report must fall + // back. Without this half, a scan that reported every word as an offender + // would pass the assertion above. + unsafe { + clear_reference(holder); + } + let cleared = scan_heap_for_fromspace_refs(); + assert!( + cleared.missing_rewrites < planted.missing_rewrites, + "removing the reference must lower the count again \ + (planted={}, cleared={})", + planted.missing_rewrites, + cleared.missing_rewrites + ); + + unsafe { + let young_header = header_from_user_ptr(young) as *mut GcHeader; + (*young_header).gc_flags &= !GC_FLAG_FORWARDED; + } +} + +#[test] +fn fromspace_scan_separates_dangling_from_missing_rewrite() { + // A reference to a young object that was NOT forwarded is a different + // defect class (the target was never evacuated and is about to be recycled) + // and must be counted separately, because the two have different fixes. + let holder = crate::arena::arena_alloc_gc_old(64, 8, GC_TYPE_OBJECT); + let young = crate::arena::arena_alloc_gc(64, 8, GC_TYPE_OBJECT); + unsafe { + std::ptr::write_bytes(holder, 0, 64); + let young_header = header_from_user_ptr(young) as *mut GcHeader; + (*young_header).gc_flags &= !GC_FLAG_FORWARDED; + } + + let baseline = scan_heap_for_fromspace_refs(); + unsafe { + plant_reference(holder, young); + } + let planted = scan_heap_for_fromspace_refs(); + + assert!( + planted.dangling > baseline.dangling, + "a reference to a NON-forwarded from-space object must be counted as \ + dangling (baseline={}, planted={})", + baseline.dangling, + planted.dangling + ); + assert_eq!( + planted.missing_rewrites, baseline.missing_rewrites, + "a non-forwarded target must NOT be counted as a missing rewrite" + ); + + unsafe { + clear_reference(holder); + } +} + +#[test] +fn fromspace_scan_ignores_references_held_by_from_space_objects() { + // A dead nursery object legitimately still points at its dead peers. If the + // scan reported those it would drown the real signal — on the #7022 + // reproducer from-space holds tens of thousands of such objects. + let dead_holder = crate::arena::arena_alloc_gc(64, 8, GC_TYPE_OBJECT); + let young = crate::arena::arena_alloc_gc(64, 8, GC_TYPE_OBJECT); + unsafe { + std::ptr::write_bytes(dead_holder, 0, 64); + } + + let baseline = scan_heap_for_fromspace_refs(); + unsafe { + plant_reference(dead_holder, young); + let young_header = header_from_user_ptr(young) as *mut GcHeader; + (*young_header).gc_flags |= GC_FLAG_FORWARDED; + } + let planted = scan_heap_for_fromspace_refs(); + + assert_eq!( + planted.missing_rewrites, baseline.missing_rewrites, + "a from-space holder's reference into from-space must be ignored" + ); + + unsafe { + clear_reference(dead_holder); + let young_header = header_from_user_ptr(young) as *mut GcHeader; + (*young_header).gc_flags &= !GC_FLAG_FORWARDED; + } +} diff --git a/crates/perry-runtime/src/gc/tests/mod.rs b/crates/perry-runtime/src/gc/tests/mod.rs index c425ed7b9d..33c1fefb32 100644 --- a/crates/perry-runtime/src/gc/tests/mod.rs +++ b/crates/perry-runtime/src/gc/tests/mod.rs @@ -10,6 +10,7 @@ mod dead_owner_side_tables; mod debt_pacer; mod error_side_tables; mod evacuation; +mod fromspace_scan; mod helper_stores; mod host_safepoints; mod incremental_sweep_reclaim;