test(gc): run the unit-test build in production's conservative-scan mode

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.

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()))
         }
     }
 }

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]

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.

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);

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 {

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<crate::gc::ConservativeStackScanMode>,
-}
-
-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.