diff --git a/changelog.d/7147-gc-test-build-production-scan-mode.md b/changelog.d/7147-gc-test-build-production-scan-mode.md new file mode 100644 index 0000000000..a892a1ff12 --- /dev/null +++ b/changelog.d/7147-gc-test-build-production-scan-mode.md @@ -0,0 +1,58 @@ +**GC / tests:** the perry-runtime unit-test build ran a *different GC root +configuration from production*. `gc::roots::conservative_stack_scan_mode()` +defaulted the test build to `ConservativeStackScanMode::Full` while production +resolves `Auto -> SkipDisabled`, and every consequence pointed in the direction +that hides defects: a missing precise root (#7055's shape) was rescued by the +native scan under test while being a live wrong answer in production, so the +suite filtered out precisely the failure class it exists to catch; the copying +minor was ineligible under test (`CopiedMinorFallbackReason::ConservativeStack`) +so relocation went largely unexercised; and a conservative scan keeps arbitrary +native-stack garbage alive, which is the wrong direction for *"this object +should have been collected"* — degrading exactly the tests most dependent on +precision. The comment justifying the split described how some tests were +written, not a requirement of testing. The correct mechanism already existed and +the tests already used it (`RuntimeHandleScope` / `gc_temp_root_*`, plus the +isolation guards, which pin `Auto` themselves): **351 of the 531 gc tests were +already running in production's mode**, so flipping the default for the rest cost +exactly one test conversion. + +That conversion is `test_minor_preserves_old_to_young_edge_across_minors`, which +asserted the parent's slot still held `ptr_bits(child)` — the address captured +*before* the first minor. It only held because `Full` made the minor non-moving; +with precise roots the copying minor relocates the child and correctly rewrites +the slot (`left: 9222531965582835720` vs `right: 9222531965580476424`). Reading a +raw local across a relocating collection is the defect class this suite exists to +catch, so the test now does what generated code does and re-reads the child out +of the parent's slot after every minor. Measured: cycles 0–2 run `copied=1 +promoted=0` with the remembered-set edge intact; cycle 3 reports `promoted=1` and +the child leaves the nursery, at which point the edge is old→old and the +remembered set correctly retires it — handled explicitly now, with an +`rs_covered_cycles >= 2` floor so the loop cannot go vacuous. Evidence (Mac mini, +darwin-arm64, release, `--test-threads=1`): `PERRY_CONSERVATIVE_STACK_SCAN=0` went +1573/1 → **1574/0**, default unchanged at 1574/0. Sabotage-verified both ways — +injecting `remembered_set_clear()` after the minor (the #6181 dropped edge) turns +the converted test red in *both* scan modes. + +`ConservativeScanAutoGuard` is deleted: with the default already `Auto` it set the +value it was going to get anyway and could no longer fail, so per CLAUDE.md's +kill-policy it goes rather than lingering as an untested no-op. Its single user +moves to `ConservativeScanDisabledGuard`, which asserts the stronger property that +test needs (objects held only as native-stack locals must be *collected*). + +**GC / runtime (#7145):** `js_shadow_frame_pop`'s corrupted-handle guard used +`debug_assert!(false, …)` **inside an `extern "C"` fn**. In a debug build the +assert fires, the panic cannot unwind across the `extern "C"` boundary, and the +process aborts — so `cargo test -p perry-runtime --lib` in the dev profile +SIGABRTed the entire test binary at +`gc::tests::shadow_stack_ops::out_of_range_frame_pop_is_ignored`, which drives +that path on purpose. CI runs the suite `--release`, where `debug_assert!` +compiles out, so the gate was structurally blind to it: a whole profile of the +runtime test suite was un-runnable and no gate could go red. Both that site and +`js_gc_temp_root_push`'s identical overflow guard (unreachable in practice, hence +never caught) now use the `report_growth_stub_skipped_below_heap_min` pattern — +`#[cold]`, one line on stderr, once per process, so it cannot perturb a stdout +parity comparison. Skip behaviour is unchanged in both profiles. + +**Build:** `crates/perry-codegen/src/linker.rs:96` landed with a rustfmt violation +in #7135, leaving `cargo fmt --all -- --check` red on `main` as of `a3b31c0d8` and +`lint` failing on every open PR. Reformatted here; no behaviour change. diff --git a/crates/perry-codegen/src/linker.rs b/crates/perry-codegen/src/linker.rs index 4dad530820..38172041b4 100644 --- a/crates/perry-codegen/src/linker.rs +++ b/crates/perry-codegen/src/linker.rs @@ -96,9 +96,8 @@ fn write_ll_atomically(ll_path: &Path, ll_text: &str, counter: u64) -> Result<() return Ok(()); } } - fs::write(ll_path, ll_text.as_bytes()).with_context(|| { - format!("Failed to write temp .ll file at {}", ll_path.display()) - }) + fs::write(ll_path, ll_text.as_bytes()) + .with_context(|| format!("Failed to write temp .ll file at {}", ll_path.display())) } } } diff --git a/crates/perry-runtime/src/gc/roots.rs b/crates/perry-runtime/src/gc/roots.rs index 02169b39a4..0db36e456a 100644 --- a/crates/perry-runtime/src/gc/roots.rs +++ b/crates/perry-runtime/src/gc/roots.rs @@ -250,20 +250,31 @@ pub(super) fn conservative_stack_scan_mode() -> ConservativeStackScanMode { if let Some(mode) = CONSERVATIVE_STACK_SCAN_OVERRIDE.with(|c| c.get()) { return mode; } - // Unit tests root GC-managed pointers as raw locals on the *native* (Rust) - // stack, not the shadow stack, so without the conservative native scan any - // collection triggered mid-test (e.g. by an allocation in a helper) reclaims - // them and a later header deref faults. Default the test build to a full - // scan so those locals survive; production keeps live values on the shadow - // stack and skips the imprecise, costly native scan. - #[cfg(test)] - { - ConservativeStackScanMode::Full - } - #[cfg(not(test))] - { - ConservativeStackScanMode::Auto - } + // ONE default for every build, tests included. + // + // The test build used to default to `Full` on the grounds that unit tests + // root GC-managed pointers as raw locals on the *native* (Rust) stack + // rather than the shadow stack, so the conservative scan was what kept them + // alive. That is a description of how some tests were written, not a + // requirement of testing, and it had a cost that outweighed the + // convenience: it made the test build a DIFFERENT GC configuration from + // production. A missing precise root — the #7055 shape — was rescued by the + // native scan under test and was a live wrong answer in production, so the + // suite filtered out precisely the failure class it exists to catch. It + // also made the copying minor ineligible under test + // (`CopiedMinorFallbackReason::ConservativeStack`), so relocation was + // largely unexercised. + // + // The correct mechanism already existed and the tests already used it: + // `RuntimeHandleScope` / `gc_temp_root_*`, plus the isolation guards, which + // pin `Auto` themselves — 351 of the 531 gc tests were already running this + // way. Flipping the default cost exactly one test conversion + // (`test_minor_preserves_old_to_young_edge_across_minors`, which read a + // relocated child through a stale pre-collection local). + // + // A test that needs the scan *provably* off — not merely resolved off by + // today's `Auto` policy — should pin `ConservativeScanDisabledGuard`. + ConservativeStackScanMode::Auto } #[inline] diff --git a/crates/perry-runtime/src/gc/roots/shadow_stack.rs b/crates/perry-runtime/src/gc/roots/shadow_stack.rs index 6f51685fd1..3bd5c32995 100644 --- a/crates/perry-runtime/src/gc/roots/shadow_stack.rs +++ b/crates/perry-runtime/src/gc/roots/shadow_stack.rs @@ -528,7 +528,7 @@ pub extern "C" fn js_shadow_frame_pop(frame_handle: u64) { // wraps for a handle near `usize::MAX` and lets exactly the corrupted // handles this guard exists for slip through into an unchecked read. if base >= s.len { - debug_assert!(false, "shadow-stack pop past end (corrupted frame handle)"); + report_corrupt_frame_pop(frame_handle); return; } s.frame_top = (*s.ptr.add(base)).value as usize; @@ -537,6 +537,36 @@ pub extern "C" fn js_shadow_frame_pop(frame_handle: u64) { }); } +/// Report an out-of-range `js_shadow_frame_pop` handle without unwinding. +/// +/// #7145: this guard used to be `debug_assert!(false, …)`. That is unsound +/// *inside an `extern "C"` fn*: in a debug build the assert fires, the panic +/// cannot unwind across the `extern "C"` boundary, and the process aborts. +/// `gc::tests::shadow_stack_ops::out_of_range_frame_pop_is_ignored` calls this +/// path deliberately, so `cargo test -p perry-runtime --lib` in the dev profile +/// SIGABRTed the entire test binary — a whole profile of the runtime suite was +/// un-runnable, and CI could not see it because CI runs `--release`, where +/// `debug_assert!` compiles out. +/// +/// Skipping the malformed pop is the documented behaviour in both profiles now; +/// the diagnostic is one line on stderr, emitted once per process so it cannot +/// perturb a stdout parity comparison (the +/// `report_growth_stub_skipped_below_heap_min` pattern). +#[cold] +fn report_corrupt_frame_pop(frame_handle: u64) { + use std::sync::atomic::AtomicBool; + static REPORTED: AtomicBool = AtomicBool::new(false); + if REPORTED.swap(true, Ordering::Relaxed) { + return; + } + eprintln!( + "[perry-gc] shadow-stack pop past end: frame handle {frame_handle:#x} is out of \ +range; the pop was SKIPPED (memory-safe, over-approximates the root set). This \ +usually means a caller threaded a NaN-boxed value into js_shadow_frame_pop \ +instead of the index js_shadow_frame_push returned. Reported once per process." + ); +} + /// Update slot `idx` in the current frame with `value`. /// Codegen emits this at safepoints for each live pointer-typed /// local, and for the `value = 0` "local is dead from here" clear. diff --git a/crates/perry-runtime/src/gc/roots/temp_roots.rs b/crates/perry-runtime/src/gc/roots/temp_roots.rs index 577b76f82f..f19c957870 100644 --- a/crates/perry-runtime/src/gc/roots/temp_roots.rs +++ b/crates/perry-runtime/src/gc/roots/temp_roots.rs @@ -60,6 +60,27 @@ thread_local! { std::cell::UnsafeCell::new(Vec::with_capacity(TEMP_ROOT_RESERVE)); } +/// Report a temp-root stack overflow without unwinding. +/// +/// Same defect class as #7145's `js_shadow_frame_pop`: a `debug_assert!(false)` +/// inside an `extern "C"` fn aborts the process in a debug build instead of +/// unwinding. Unreachable in practice (it needs `u32::MAX` live temp roots), so +/// no test drives it — which is exactly why it should not be left as a latent +/// abort. One line on stderr, once per process. +#[cold] +fn report_temp_root_overflow(depth: usize) { + use std::sync::atomic::{AtomicBool, Ordering}; + static REPORTED: AtomicBool = AtomicBool::new(false); + if REPORTED.swap(true, Ordering::Relaxed) { + return; + } + eprintln!( + "[perry-gc] temp-root stack overflow at depth {depth}: refusing to grow \ +(codegen dropped a js_gc_temp_root_truncate). The returned index still addresses \ +a live slot. Reported once per process." + ); +} + /// Push `value` and return the index generated code must pass to /// `js_gc_temp_root_get` / `js_gc_temp_root_set` / `js_gc_temp_root_truncate`. #[no_mangle] @@ -71,7 +92,7 @@ pub extern "C" fn js_gc_temp_root_push(value: u64) -> u32 { // keeps a runaway from turning into unbounded retention. The returned // index still addresses a live slot, so get/set stay well-defined. if idx >= u32::MAX as usize { - debug_assert!(false, "temp-root stack overflow (unbalanced truncate)"); + report_temp_root_overflow(idx); return (idx - 1) as u32; } s.push(value); diff --git a/crates/perry-runtime/src/gc/tests/oldgen.rs b/crates/perry-runtime/src/gc/tests/oldgen.rs index a46883567b..5a96b33d85 100644 --- a/crates/perry-runtime/src/gc/tests/oldgen.rs +++ b/crates/perry-runtime/src/gc/tests/oldgen.rs @@ -751,7 +751,10 @@ fn test_old_page_defrag_re_remembers_young_child_after_collection_clear() { } let _reset = ResetGcTestState; - let _scan = ConservativeScanAutoGuard::new(); + // This test verifies objects it holds only as native-stack locals are + // COLLECTED, so the native scan must not rescue them. `Disabled` says that + // unconditionally; the default `Auto` merely resolves that way today. + let _scan = ConservativeScanDisabledGuard::new(); let _isolation = copying_nursery_isolation_lock(); let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); let _force = EnvVarGuard::set("PERRY_GC_FORCE_EVACUATE", "1"); @@ -1142,6 +1145,13 @@ fn test_minor_skips_whole_heap_old_to_young_rebuild() { /// `restore_surviving_dirty_coverage`. This is the under-remembering /// (use-after-free) guard for Fix 2: if any minor dropped the edge, the /// child would be swept and the RS root mark below would not reach it. +/// +/// The loop tracks the child through the parent's SLOT, never through the +/// pre-collection Rust local. With the conservative native-stack scan off — +/// precise roots only, which is what production runs — the copying minor is +/// eligible and RELOCATES the child on every cycle, rewriting the parent's +/// slot. A test that re-read the raw local would be reading a stale address, +/// which is the exact defect class this suite exists to catch. #[test] fn test_minor_preserves_old_to_young_edge_across_minors() { let _isolation = copying_nursery_isolation_lock(); @@ -1172,8 +1182,10 @@ fn test_minor_preserves_old_to_young_edge_across_minors() { other_old.push(h); } - let child = crate::arena::arena_alloc_gc(40, 8, GC_TYPE_OBJECT) as usize; - let child_header = unsafe { header_from_user_ptr(child as *const u8) }; + // Re-derived from the parent's slot after every minor (see the loop), so + // both are `mut`: a relocating minor moves the child. + let mut child = crate::arena::arena_alloc_gc(40, 8, GC_TYPE_OBJECT) as usize; + let mut child_header = unsafe { header_from_user_ptr(child as *const u8) }; assert!(crate::arena::pointer_in_nursery(child)); unsafe { *fields = ptr_bits(child); @@ -1185,23 +1197,58 @@ fn test_minor_preserves_old_to_young_edge_across_minors() { "barrier must record the old→young edge" ); + // Cycles that actually exercised the old→young RS edge. Asserted non-zero + // below so the loop cannot go vacuous if the child is tenured early. + let mut rs_covered_cycles = 0; + for cycle in 0..4 { let trace = collect_minor_trace(GcTriggerKind::Direct); assert_eq!( trace.old_to_young_rebuild_objects_scanned, 0, "cycle {cycle}: minor must skip the whole-heap RS rebuild" ); + + // Follow the edge through the parent's SLOT — the authoritative root — + // rather than through the pre-collection local. A relocating minor + // moves the child and rewrites this slot; that rewrite is the property + // under test, not a nuisance. + let slot = unsafe { *fields }; + let slot_child = (slot & POINTER_MASK) as usize; + assert_eq!( + slot, + ptr_bits(slot_child), + "cycle {cycle}: the parent's slot must still hold a tagged pointer" + ); + if trace.copying_nursery.copied_objects == 0 && trace.copying_nursery.promoted_objects == 0 + { + assert_eq!( + slot_child, child, + "cycle {cycle}: a minor that moved nothing must leave the parent's slot intact" + ); + } + child = slot_child; + child_header = unsafe { header_from_user_ptr(child as *const u8) }; + + if !crate::arena::pointer_in_nursery(child) { + // The child aged out: a relocating minor TENURED it into the old + // gen. The edge is now old→old, so the remembered set correctly + // retires it and RS coverage no longer applies. The + // under-remembering property still has to hold, and here it means + // the child is live old-gen memory rather than swept. + assert!( + crate::arena::pointer_in_old_gen(child), + "cycle {cycle}: a child that left the nursery must be in the old gen, \ + not swept out from under the parent's slot" + ); + break; + } + // The child (reachable only via the old parent) must still be covered // by the remembered set: RS root marking reaches and marks it. assert!( remembered_set_size() > 0, "cycle {cycle}: old→young edge must survive the minor" ); - assert_eq!( - unsafe { *fields }, - ptr_bits(child), - "cycle {cycle}: non-moving minor must leave the parent's slot intact" - ); clear_marks(); let valid_ptrs = build_valid_pointer_set(); let stats = mark_remembered_set_roots(&valid_ptrs); @@ -1219,8 +1266,15 @@ fn test_minor_preserves_old_to_young_edge_across_minors() { // from the remembered set alone (not a stale mark). (*child_header).gc_flags &= !GC_FLAG_MARKED; } + rs_covered_cycles += 1; } + assert!( + rs_covered_cycles >= 2, + "the loop must actually exercise the old→young edge across repeated \ + minors; only {rs_covered_cycles} cycle(s) did" + ); + unsafe { (*parent_header).gc_flags &= !GC_FLAG_PINNED; for h in other_old { diff --git a/crates/perry-runtime/src/gc/tests/support.rs b/crates/perry-runtime/src/gc/tests/support.rs index ce4cc20bf7..ffb18c086c 100644 --- a/crates/perry-runtime/src/gc/tests/support.rs +++ b/crates/perry-runtime/src/gc/tests/support.rs @@ -202,30 +202,14 @@ pub(super) fn root_scanner_registry_counts() -> (usize, usize, usize, usize) { (rust_roots, mutable_roots, ffi_roots, ffi_mutable_roots) } -/// Opt this thread out of the test build's full-conservative-scan default for -/// the guard's lifetime, restoring the prior override on drop. GC tests that -/// verify collection of objects they hold only as native-stack locals — and -/// don't go through `ScopedRootScannerRegistryGuard` — use this so the native -/// scan is *skipped* (production `Auto` behavior) and those locals are reclaimed. -pub(super) struct ConservativeScanAutoGuard { - prev: Option, -} - -impl ConservativeScanAutoGuard { - pub(super) fn new() -> Self { - Self { - prev: crate::gc::set_conservative_stack_scan_override(Some( - crate::gc::ConservativeStackScanMode::Auto, - )), - } - } -} - -impl Drop for ConservativeScanAutoGuard { - fn drop(&mut self) { - crate::gc::set_conservative_stack_scan_override(self.prev); - } -} +// `ConservativeScanAutoGuard` used to live here. It opted a thread out of the +// test build's `Full` conservative-scan default, back to production's `Auto`. +// The test build now defaults to `Auto` for every thread +// (`gc::roots::conservative_stack_scan_mode`), so the guard set the value it +// was already going to get — it could no longer fail, and per CLAUDE.md's +// kill-policy a mode that cannot be exercised is deleted rather than left for a +// future bisect to trust. Tests needing the scan *provably* off should pin +// `ConservativeScanDisabledGuard`, which asserts the stronger property. /// Put the runtime-handle mutable-root scanner back into this thread's /// registry.