fix(gc): stop the collector moving objects at register-imprecise allocation points (#7682)

changelog.d/7687-alloc-point-collections-must-not-move.md

@@ -0,0 +1,78 @@
+### Fixed — an allocation-point collection can no longer MOVE anything (#7682)
+
+A 189-statement tree-walking interpreter — ordinary TypeScript, no exotic
+construct, `scriptc coverage` reports it fully static — returned a **silently
+wrong number** on default settings, every run: `1708662` where Node and a fully
+static build both give `1708840`. No crash, no `TypeError`, no diagnostic.
+
+**Root cause.** `gc_check_trigger`'s nursery-churn arm collects from inside
+`arena_cell_alloc`, i.e. at whatever half-finished expression happened to need a
+fresh arena block. That program point is described by neither root lowering: the
+shadow stack names only values codegen has already stored to a slot, and RS4GC
+relocates only what it can type as `ptr addrspace(1)`, which a NaN-boxed
+`double` operand in an SSA register is not. The arm therefore took
+`ManualGcScanGuard::force_full_scan()`, whose job there is not retention but
+*immobility* — a conservative native-stack scan makes the copying minor
+ineligible (`CopiedMinorFallbackReason::ConservativeStack`), so the non-moving
+in-place minor runs and nothing relocates.
+
+`PERRY_GC_SCAVENGE` gated that guard off. The guard is now unconditional.
+
+**Why the gate was ever conditional, and why both halves of the reason were
+false.** The flag's doc comment said "Phase-1 de-risking flag (OFF by default) …
+NOT sound as a production default yet — the alloc point can be
+register-imprecise — so it stays behind this flag for measurement only". Eight
+lines below it, the body said `ON BY DEFAULT (#7056)`. That is the #6987 shape
+CLAUDE.md warns about, and this time the stale half was the one carrying the
+soundness argument. The body's own claim — "enabling this also defers
+alloc-point collections to a precise safepoint" — was false too: that deferral
+is gated on `gc_moving_loop_polls_enabled()`, OFF by default since #7161. In the
+shipped configuration the two flags disagree, the deferral is dead code, and the
+alloc-point minor ran right there with neither a scan nor a safepoint.
+
+**The failure, end to end.** `evalNode` lowers `{ names: [n.name], … }` by
+reading `n.name` into a register, then inline-bump-allocating the one-element
+array. The bump overflows its block, `js_inline_arena_slow_alloc` →
+`arena_cell_alloc` → `gc_check_trigger` runs an *evacuating* minor, and the
+string moves. Control returns to the shared merge block, which stores the
+pre-move address into the new array. `lookup` then compares `names[i] === name`
+— a live string against a moved one — falls through to its default, and naive
+`fib`, which is just a count of leaves returning `1`, comes back short by
+exactly the number of missed lookups.
+
+**Why every existing gate was green.** `PERRY_GC_VERIFY_MARK` reports OK
+(marking is correct; it is the post-move holder that is wrong, and it is a
+register, so there is nothing in the heap to find). The heap-wide
+`PERRY_GC_FROMSPACE_SCAN` finds no live offender for the same reason — every
+owner it reports is `marked=false`, i.e. already dead. `scripts/gc_root_dominance_check.py`
+reports 0 violations over 380 root stores: the value is never bound to a slot at
+all, so there is no store whose dominance it could question. And the GC probe
+corpus holds its subjects across an *explicit* churn call; none holds one across
+the allocation of the literal being built.
+
+**Cost.** Alloc-point nursery collections are non-moving again, which is what
+they were before #7056. Copying minors continue to run at the precise safepoints
+(`gc_safepoint_moving_minor`), where the root set is real. `PERRY_GC_SCAVENGE`
+keeps its other job — routing nursery-churn triggers to the direct minor instead
+of the budgeted non-moving stepper — and is documented as the pacing knob it is.
+The `gc-ratchet` artifact was measured under the shipped default and pins the
+old evacuating behaviour; it needs regenerating on the pinned host.
+
+**Tests.**
+
+- `gc::tests::scan_fallback::the_alloc_point_nursery_minor_retains_native_stack_values_under_shipped_pacing`
+  drives the arm with a value reachable only from a live native-stack word and
+  asserts it survives, that a collection actually ran, and that the census
+  counted the forced scan. Its sabotage control
+  (`the_alloc_point_plant_dies_when_the_scan_is_pinned_off`) runs the identical
+  plant with the scan pinned off and asserts the plant DIES — without it, "the
+  malloc sweep never ran" and "the guard held" are the same green.
+- `policy::force_shipped_default_gc_pacing` pins polls OFF + scavenge ON. That
+  combination had no test guard: `force_legacy_gc_pacing` pins both off and
+  `force_moving_gc_pacing` pins both on, so every test in the crate declared a
+  pacing mode in which the two flags agreed — and the interaction that broke is
+  exactly the one where they disagree.
+- `test-files/test_gap_gc_alloc_point_no_move.ts` is the interpreter itself,
+  compared byte-for-byte against Node. Verified non-vacuous: it prints `1708662`
+  against the pre-fix runtime under the gap harness's own
+  `PERRY_NO_AUTO_OPTIMIZE=1` configuration, and `1708840` after.

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

@@ -326,18 +326,27 @@ fn gc_verify_evacuation_enabled() -> bool {
     )
 }
 
-/// Phase-1 de-risking flag (OFF by default). When set, the alloc-point
-/// nursery-churn arm (`gc_check_trigger`) runs its direct minor with the
-/// PRECISE shadow-stack roots instead of forcing the conservative native
-/// scan. The conservative scan makes the copying fast path ineligible
-/// (`CopiedMinorFallbackReason::ConservativeStack`), pinning the minor to the
-/// non-moving in-place sweep that cannot reclaim array-growth stubs; skipping
-/// it lets the evacuating scavenge run and reset the whole young arena in
-/// O(live). NOT sound as a production default yet — the alloc point can be
-/// register-imprecise — so it stays behind this flag for measurement +
-/// `PERRY_GC_VERIFY_EVACUATION` probing only. Pairs with
-/// `PERRY_GC_MAJOR_PACING_FLOOR_MB=0` so the #6939 pacing doesn't escalate the
-/// minor to a full before the copying path is reached.
+/// `PERRY_GC_SCAVENGE` — **ON by default since #7056**, kill switch
+/// `PERRY_GC_SCAVENGE=0`/`off`/`false`. It is a PACING knob: it routes
+/// nursery-churn triggers to the direct minor in `gc_check_trigger` instead of
+/// the budgeted non-moving stepper, which on a reallocation-heavy loop frees
+/// nothing. Paired with the nursery cap in `policy::effective_next_arena_trigger`
+/// that is the -69% RSS result quoted on the getter below.
+///
+/// It does **not** decide whether the alloc-point minor may move, and #7682 is
+/// what that confusion cost. The flag used to gate the `force_full_scan()` on
+/// that arm off, so the shipped default ran an EVACUATING minor at an arbitrary
+/// allocation point — a program point neither root lowering describes — and
+/// values held only in registers were relocated behind their holders' backs.
+/// The guard is now unconditional; see the comment at its site in
+/// `policy::gc_check_trigger` for why no pacing knob can answer the question it
+/// asks.
+///
+/// This doc comment previously read "Phase-1 de-risking flag (OFF by default)
+/// … NOT sound as a production default yet". Both halves were false for two
+/// hundred releases, eight lines above a body comment saying "ON BY DEFAULT" —
+/// the #6987 shape CLAUDE.md warns about, and this time the stale half was the
+/// one carrying the soundness argument.
 #[cfg(test)]
 thread_local! {
     /// Test-only override, consulted BEFORE the process-wide OnceLock so a
@@ -373,10 +382,14 @@ pub(super) fn gc_scavenge_enabled() -> bool {
         // them evacuating (O(live) copying) rather than O(heap) sweeps — so the
         // frequency is cheap instead of expensive.
         //
-        // Enabling this also defers alloc-point collections to a precise
-        // safepoint rather than collecting behind a forced conservative scan.
-        // That is newly reasonable: native roots became the default in #7370, so
-        // a precise safepoint is what the shipped configuration now has.
+        // What this does NOT do, despite what this comment used to claim: it
+        // does not defer alloc-point collections to a precise safepoint. That
+        // deferral is gated on `gc_moving_loop_polls_enabled()`, which has been
+        // OFF by default since #7161 — so in the shipped configuration the two
+        // flags disagree, the deferral is dead, and the alloc-point minor runs
+        // right there. It is sound because that minor is non-moving
+        // (`force_full_scan`), not because it was moved somewhere precise
+        // (#7682).
         !matches!(
             std::env::var("PERRY_GC_SCAVENGE").as_deref(),
             Ok("0") | Ok("off") | Ok("false")

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

@@ -568,6 +568,30 @@ pub(super) fn force_moving_gc_pacing() -> LegacyGcPacingGuard {
     }
 }
 
+/// Pin the pacing combination a **shipped binary actually runs**: moving-loop
+/// polls OFF (`gc_moving_loop_polls_enabled`, default OFF since #7161) and
+/// scavenge ON (`gc_scavenge_enabled`, default ON since #7056).
+///
+/// This is a third combination, and its absence is part of why #7682 shipped.
+/// [`force_legacy_gc_pacing`] pins polls OFF *and* scavenge OFF;
+/// [`force_moving_gc_pacing`] pins both ON. Every test in this crate therefore
+/// declared a pacing mode in which the two flags agreed — and the
+/// alloc-point/deferral interaction that broke is precisely the one where they
+/// DISAGREE: scavenge routes nursery pressure to the direct alloc-point minor,
+/// while the deferral that was supposed to move that collection to a precise
+/// safepoint is gated on the polls flag and never runs.
+#[cfg(test)]
+pub(super) fn force_shipped_default_gc_pacing() -> LegacyGcPacingGuard {
+    let previous = GC_MOVING_LOOP_POLLS_TEST_OVERRIDE.with(|cell| cell.replace(Some(false)));
+    let cap_previous = GC_NURSERY_CAP_TEST_SUPPRESSED.with(|cell| cell.replace(false));
+    let scavenge_previous = super::GC_SCAVENGE_TEST_OVERRIDE.with(|cell| cell.replace(Some(true)));
+    LegacyGcPacingGuard {
+        previous,
+        cap_previous,
+        scavenge_previous,
+    }
+}
+
 pub(super) fn gc_trace_enabled() -> bool {
     #[cfg(test)]
     if GC_TRACE_TEST_FORCE.with(Cell::get) {
@@ -1822,14 +1846,23 @@ pub fn gc_check_trigger() {
             }
             let pre_in_use = crate::arena::arena_in_use_bytes();
             let pre_malloc_count = malloc_object_count();
-            // PERRY_GC_SCAVENGE (Phase-1 de-risking, OFF by default): skip the
-            // conservative native-stack scan so this direct minor runs with the
-            // PRECISE shadow-stack roots and the copying fast path becomes
-            // eligible (an evacuating scavenge that resets the whole young arena
-            // in O(live)). The default path keeps `force_full_scan` — at an
-            // arbitrary alloc point a value mid-construction may live only in
-            // registers, which the conservative scan retains (and which makes
-            // copied-minor ineligible, so the non-moving minor runs).
+            // THE ALLOC POINT IS REGISTER-IMPRECISE, SO THIS MINOR MUST NOT
+            // MOVE. Unconditional, and the unconditionality is the fix for
+            // #7682.
+            //
+            // Reaching this line means the collection is happening HERE, at an
+            // arbitrary allocation point inside a half-built expression — not
+            // at a declared safepoint. Neither root lowering describes that
+            // point: the shadow stack only names values codegen has already
+            // stored to a slot, and RS4GC only relocates values it can type as
+            // `ptr addrspace(1)`, which a NaN-boxed `double` operand in an SSA
+            // register is not. A value that exists ONLY in a register here is
+            // therefore invisible to both, so an evacuating minor relocates the
+            // object and leaves the register naming the pre-move address. The
+            // conservative native-stack scan is what covers exactly that gap:
+            // it retains such values AND makes the copying minor ineligible
+            // (`CopiedMinorFallbackReason::ConservativeStack`), so the
+            // non-moving in-place minor runs and nothing relocates.
             //
             // ★ #7148 disposition: **keep as the bounded valve, now counted.**
             // The deferral above is the primary path and is sound by
@@ -1842,16 +1875,31 @@ pub fn gc_check_trigger() {
             // reached" is: this arm runs, and it is the reason RSS stays
             // bounded. Making it *imprecise* instead (collecting without the
             // scan) is the one thing #7148 rules out — it would trade a cost
-            // problem for a soundness problem. Making it **countable** is what
-            // turns "unreachable in practice" into a measurement:
-            // `ConservativeScanSite::NurseryChurnSlackValve` is 0 on all eight
-            // ratchet probes and across the stress matrix, and the drain
-            // counter proves the deferral ran instead.
-            let _scan = (!super::gc_scavenge_enabled()).then(|| {
-                super::roots::ManualGcScanGuard::force_full_scan(
-                    super::ConservativeScanSite::NurseryChurnSlackValve,
-                )
-            });
+            // problem for a soundness problem.
+            //
+            // #7682 is that trade, shipped. `PERRY_GC_SCAVENGE` used to gate
+            // this guard off, on the strength of a doc comment claiming the
+            // flag was "OFF by default … for measurement only" and a body
+            // comment saying it also "defers alloc-point collections to a
+            // precise safepoint". Neither held in the shipped configuration:
+            // the flag has been ON by default since #7056, and the deferral
+            // above is gated on `gc_moving_loop_polls_enabled()`, which is OFF
+            // by default since #7161. So the default build collected — and
+            // EVACUATED — right here, with no scan and no deferral. A
+            // tree-walking interpreter (`test_gap_gc_alloc_point_no_move.ts`)
+            // then read a relocated heap string out of a stale register and
+            // silently returned the wrong number.
+            //
+            // The scan-skip cannot be recovered by asking "is scavenge on?":
+            // that question is about pacing, and the precondition being
+            // asserted here is about the PRECISION OF THIS PROGRAM POINT,
+            // which no pacing knob can change. Scavenge keeps its other job —
+            // routing nursery-churn triggers to this direct minor instead of
+            // the budgeted non-moving stepper — and the moving minor keeps
+            // running at the precise safepoints, where the root set is real.
+            let _scan = super::roots::ManualGcScanGuard::force_full_scan(
+                super::ConservativeScanSite::NurseryChurnSlackValve,
+            );
             let outcome = super::gc_collect_minor_with_trigger(GcTriggerSnapshot::capture(kind));
             // Re-baseline the arming trigger after the direct minor, mirroring
             // `gc_finish_budgeted_cycle`. This arm is taken whenever