diff --git a/benchmarks/gc_ratchet/probes/10_store_receiver_across_alloc.ts b/benchmarks/gc_ratchet/probes/10_store_receiver_across_alloc.ts new file mode 100644 index 0000000000..ba1e98be34 --- /dev/null +++ b/benchmarks/gc_ratchet/probes/10_store_receiver_across_alloc.ts @@ -0,0 +1,76 @@ +// GC ratchet probe: the store receiver held across an allocating RHS. +// +// `a[i] = v` evaluates the receiver first and the value last — spec order — so +// the receiver sits in an SSA register while the RHS runs. When the RHS +// allocates, an evacuating minor can relocate the array underneath it: the slot +// the register was loaded from is a registered root and evacuation rewrites it, +// but the register is not, so the store lands in retired from-space. +// +// THREE THINGS MAKE THIS PROBE BITE, and dropping any one of them makes it +// silently measure nothing: +// +// 1. The array is a MODULE-LEVEL binding. A local array lives in a shadow slot +// the collector rewrites, and the bug does not reproduce. +// 2. The store happens INSIDE A FUNCTION. The identical loop written at top +// level is clean. +// 3. The RHS ALLOCATES. `sink[i] = i` cannot collect, so codegen correctly +// emits no rooting at all and there is no window. +// +// This is NOT observable from output alone: evacuation copies rather than +// zeroes, so the stale address still holds the old bytes and the program prints +// the right answer. It takes `PERRY_GC_PROTECT_FROMSPACE=1` — which unmaps +// retired from-space — to turn the latent stale access into a fault. Run it as +// the other probes are run, plus: +// +// PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=64 +// +// Regression probe for the array-store receiver fix; found via #7341. +// +// Metric contract: stdout carries only `probe:`/`checksum:` lines and is diffed +// byte-for-byte against the pinned Node oracle. Retention metrics go to stderr +// as `#gcmetric` lines and are Perry-vs-Perry only. + +declare function gc(): void; + +const SLOTS = 1024; +const ITERATIONS = 200000; + +// (1) module-level, so the receiver is loaded from a global handle rather than +// a shadow slot. +const sink: ({ a: number; b: string } | null)[] = new Array(SLOTS); +for (let i = 0; i < SLOTS; i++) { + sink[i] = null; +} + +let checksum = 0; + +// (2) inside a function, and (3) an allocating RHS. +function churn(n: number): void { + for (let i = 0; i < n; i++) { + sink[i & (SLOTS - 1)] = { a: i, b: "s" + (i & 7) }; + } +} + +churn(ITERATIONS); + +// Read every slot back so a store that landed in abandoned memory shows up as a +// missing or wrong entry rather than being quietly dropped. +for (let i = 0; i < SLOTS; i++) { + const entry = sink[i]; + if (entry !== null) { + checksum = (checksum + entry.a + entry.b.length) | 0; + } +} + +for (let i = 0; i < SLOTS; i++) { + sink[i] = null; +} + +gc(); +const mu = process.memoryUsage(); + +console.log("probe:10_store_receiver_across_alloc"); +console.log("checksum:" + checksum); +console.error("#gcmetric heap_used_bytes=" + mu.heapUsed); +console.error("#gcmetric heap_total_bytes=" + mu.heapTotal); +console.error("#gcmetric rss_bytes=" + mu.rss); diff --git a/changelog.d/7342-array-store-receiver-rooting.md b/changelog.d/7342-array-store-receiver-rooting.md new file mode 100644 index 0000000000..616b637a71 --- /dev/null +++ b/changelog.d/7342-array-store-receiver-rooting.md @@ -0,0 +1,32 @@ +### Fixed + +- **`a[i] = v` could store through a pre-collection array address.** The + receiver is evaluated first and the value last (spec order), so it sits in an + SSA register while the RHS runs. When the RHS allocates, an evacuating minor + relocates the array — the slot the register was loaded from is a registered + root and evacuation rewrites it, but the register is not, so the store landed + in retired from-space. + + Codegen already had the machinery for exactly this (#7154's + `guard_store_operand_across` / `reread_store_operand`) and the generic object + paths used it; three array paths did not. All three now do. + + Reproducing it needs a **module-level** array written **inside a function** + with an **allocating RHS** — a local array, a top-level loop, or an inert RHS + are each clean on their own. It is invisible from program output because + evacuation copies rather than zeroes, so the stale address still reads the + correct old bytes; `PERRY_GC_PROTECT_FROMSPACE=1` is what turns it into a + fault. Both root backends failed identically, because the value was never + given a root slot at all. + + Nothing changes on the hot path: the guard is gated on the RHS being able to + collect, so `a[i] = i * 2` emits the same IR as before. + +- **The from-space quarantine now runs over real programs.** + `gc_instrument_smoke.sh` drove `PERRY_GC_PROTECT_FROMSPACE` with + `PERRY_GC_MOVING_LOOP_POLLS` over one synthetic fixture, and back-edge polls + fire only while user JS runs — so it structurally could not expose staleness + in runtime-internal code. A new arm runs every `gc_ratchet` probe by the + allocation-point route instead, with a non-vacuity check so an empty probe set + fails rather than reporting a clean sweep of nothing. This is what surfaced the + bug above (#7341). diff --git a/crates/perry-codegen/src/expr/index_set.rs b/crates/perry-codegen/src/expr/index_set.rs index 44bcde5006..ceac76e0e1 100644 --- a/crates/perry-codegen/src/expr/index_set.rs +++ b/crates/perry-codegen/src/expr/index_set.rs @@ -228,6 +228,13 @@ fn lower_array_index_set_via_runtime_key( source_label: &str, ) -> Result { let arr_box = lower_expr(ctx, object)?; + // #7341, same hazard as the packed path: the receiver is live across both + // `index` and `value` lowering, and an allocating RHS is a collection + // point. Without this the store writes through a pre-evacuation address. + let recv_collects = super::temp_root::expr_may_trigger_gc(ctx, index) + || super::temp_root::expr_may_trigger_gc(ctx, value); + let recv_guard = + super::temp_root::guard_store_operand_across(ctx, object, &arr_box, recv_collects); let idx_double = lower_expr(ctx, index)?; let value_needs_barrier = array_store_needs_write_barrier(ctx, value); let (val_double, val_bits) = lower_value_for_dynamic_index_set( @@ -236,6 +243,7 @@ fn lower_array_index_set_via_runtime_key( "index_set.array_runtime_key_value_bits", "array_runtime_key_set_helper_edge", )?; + let arr_box = super::temp_root::reread_store_operand(ctx, &recv_guard, object, &arr_box)?; let arr_handle = { let blk = ctx.block(); unbox_to_i64(blk, &arr_box) @@ -270,6 +278,8 @@ fn lower_array_index_set_via_runtime_key( let arr_bits = ctx.block().bitcast_double_to_i64(&arr_box); emit_write_barrier(ctx, &arr_bits, &val_bits); } + // After the store: the helper itself can grow the element array. + super::temp_root::release_store_operand(ctx, recv_guard); Ok(val_double) } @@ -979,9 +989,20 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // dispatch overhead). if is_array_expr(ctx, object) && !is_numeric_expr(ctx, index) { let arr_box = lower_expr(ctx, object)?; + // #7341: receiver live across `index` and `value` lowering. + let recv_collects = super::temp_root::expr_may_trigger_gc(ctx, index) + || super::temp_root::expr_may_trigger_gc(ctx, value); + let recv_guard = super::temp_root::guard_store_operand_across( + ctx, + object, + &arr_box, + recv_collects, + ); let idx_double = lower_expr(ctx, index)?; let value_needs_barrier = array_store_needs_write_barrier(ctx, value); let val_double = lower_expr(ctx, value)?; + let arr_box = + super::temp_root::reread_store_operand(ctx, &recv_guard, object, &arr_box)?; let arr_handle = { let blk = ctx.block(); unbox_to_i64(blk, &arr_box) @@ -1007,6 +1028,7 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { let arr_bits = ctx.block().bitcast_double_to_i64(&arr_box); emit_write_barrier(ctx, &arr_bits, &val_bits); } + super::temp_root::release_store_operand(ctx, recv_guard); return Ok(val_double); } if is_array_expr(ctx, object) @@ -1287,8 +1309,39 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { let require_numeric_layout = value_is_numeric && expr_has_numeric_pointer_free_array_layout(ctx, object); let arr_box = lower_expr(ctx, object)?; + // #7341: this array path skipped the store-operand guard the + // generic object paths below already apply (#7154). `a[i] = v` + // evaluates the receiver first and the value last — spec order + // — so the receiver sits in an SSA register while an allocating + // RHS runs. The *slot* it was loaded from is a registered root + // and evacuation rewrites it; the register is not, so the store + // lands in retired from-space. + // + // Reproduced deterministically (6/6) by a module-level array + // written in a loop from inside a function: + // + // const sink: unknown[] = new Array(1024); + // function churn(n) { for (...) sink[i & 1023] = { a: i }; } + // + // The same loop at top level is clean, and a *local* array is + // clean, which is why this went unnoticed. Both root backends + // fault identically — the value was never given a root slot at + // all, so neither lowering could have covered it. + // + // The window is the disjunction over `index` and `value` + // (#7201): the receiver is live across both. + let recv_collects = super::temp_root::expr_may_trigger_gc(ctx, index) + || super::temp_root::expr_may_trigger_gc(ctx, value); + let recv_guard = super::temp_root::guard_store_operand_across( + ctx, + object, + &arr_box, + recv_collects, + ); let idx_double = lower_expr(ctx, index)?; let val_double = lower_expr(ctx, value)?; + let arr_box = + super::temp_root::reread_store_operand(ctx, &recv_guard, object, &arr_box)?; let local_id = if let Expr::LocalGet(id) = object.as_ref() { Some(*id) } else { @@ -1425,6 +1478,10 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { emit_write_barrier(ctx, &arr_bits, &val_bits); } } + // After the store, never before: every branch above ends in a + // helper that can itself allocate (element-array growth, the + // extend path's realloc). + super::temp_root::release_store_operand(ctx, recv_guard); return Ok(val_double); } if let Expr::String(literal) = index.as_ref() { diff --git a/scripts/gc_instrument_smoke.sh b/scripts/gc_instrument_smoke.sh index e31063cb8a..ded4ee273e 100755 --- a/scripts/gc_instrument_smoke.sh +++ b/scripts/gc_instrument_smoke.sh @@ -135,6 +135,60 @@ if [[ "$zeal_retired" -le "$nozeal_retired" ]]; then exit 1 fi +# ---- arm 4: the quarantine, aimed at real programs -------------------------- +# +# #7341. Everything above drives the instrument with PERRY_GC_MOVING_LOOP_POLLS +# over ONE synthetic fixture. Back-edge polls fire only while user JS runs, so +# that configuration structurally CANNOT expose an unrooted pointer in runtime +# code that does not re-enter user JS -- which is most of the runtime. The gate +# was well built and pointed at the wrong workload; aiming it at the gc_ratchet +# probes by the ALLOCATION-POINT route instead found 55 stale from-space +# dereferences across the gap suite, 44 of them in programs that exit cleanly +# and print the right answer. +# +# It has to be a fault-based check, not an output check: evacuation copies +# rather than zeroes, so a stale address still reads the correct old bytes. +# Only unmapping retired from-space turns the latent access into a signal. +PROBES="$(dirname "$0")/../benchmarks/gc_ratchet/probes" +if [[ -d "$PROBES" ]]; then + echo + echo "== arm 4: quarantine over the gc_ratchet probes (allocation-point route) ==" + probe_count=0 + probe_failed=0 + for probe in "$PROBES"/*.ts; do + [[ -e "$probe" ]] || continue + name="$(basename "$probe" .ts)" + probe_count=$((probe_count + 1)) + "$PERRY_BIN" compile "$probe" -o "$WORK/$name" >/dev/null + set +e + PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=64 \ + PERRY_GC_HEAP_LIMIT=8 PERRY_GC_INCREMENTAL=0 PERRY_CONSERVATIVE_STACK_SCAN=off \ + "$WORK/$name" > "$WORK/$name.out" 2> "$WORK/$name.err" + rc=$? + set -e + if [[ $rc -ge 128 ]]; then + echo "FAIL [$name]: signalled $rc under from-space quarantine." >&2 + echo " A live value still pointed into retired from-space after an" >&2 + echo " evacuating minor. The handler's diagnosis:" >&2 + grep -A6 'gc-fromspace-protect. FAULT' "$WORK/$name.err" >&2 || true + probe_failed=$((probe_failed + 1)) + fi + done + # Non-vacuity: the arm must have had a subject. A probe set that silently + # stopped matching would otherwise report a clean sweep of nothing -- the + # exact shape of #6942 / #7024 / #7336. + if [[ "$probe_count" -eq 0 ]]; then + echo "FAIL: no probes matched $PROBES -- arm 4 ran on nothing." >&2 + exit 1 + fi + if [[ "$probe_failed" -ne 0 ]]; then + echo "FAIL: $probe_failed/$probe_count probes faulted under the quarantine." >&2 + exit 1 + fi + echo " $probe_count/$probe_count probes clean under from-space quarantine" +fi + echo echo "PASS: instruments inert when off (0 retirements), live when on" echo " (no-zeal=$nozeal_retired, zeal=$zeal_retired retirements), program correct in all arms." +echo " Quarantine clean over $probe_count real probes (allocation-point route)."