From 15f4b19d8a80c2419ff28d53d6aa6da2cd6a06c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 6 Sep 2026 09:33:01 +0200 Subject: [PATCH] fix(codegen): root_reload's cost cap counts root loads, not derived values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `MAX_BLOCK_LOAD_PRODUCT` guards the reachability walk in `apply_to_function`, and that walk runs once per `groups` entry — one per ROOT LOAD. The check multiplied by `values.len()` instead, which since #7664 counts every pure-bit-op derivation as its own `Reloadable`. On a wide function that is several times the group count, so the pass declined on functions whose real cost was well inside the bound. Declining is not correctness-neutral under the native root lowering. The constant's comment claimed "the pass is an improvement, not a correctness precondition, so declining is safe"; that is false, and it is why the cliff went unnoticed. When the pass does not run, nothing re-reads the slot: a receiver read out of its root, unmasked to an i64/double and carried across a call is a value RS4GC cannot relocate, so the store lands in a from-space object. Found on Claude-of-Duty's `Arm.constructor` (4924 blocks, 747 root loads, 2102 values): 10,350,248 by the old metric against an 8M cap, 3,678,228 by the new one. It declined, and `this.upper = buildSleeve(...)` wrote through a stale receiver — a SIGBUS under `PERRY_GC_PROTECT_FROMSPACE=1`, and silent field corruption without it (`THREE.Object3D.add: object not an instance of THREE.Object3D. undefined` two frames later). The bound itself is unchanged; only the term it is measured against. A function that genuinely exceeds `blocks x groups` still declines and can still carry a stale register — that residual risk is now stated at the constant rather than denied. Note that `scripts/gc_root_dominance_check.py --stale-registers --moving-only` does NOT flag this shape: it reported 0 stale uses on the faulting module (17 found, all `source=global`), so it cannot serve as a guard here. The regression test replicates the `masked_receiver` shape across 1100 blocks with a MAX_RECIPE-length derivation, sized to clear the cap by the old metric and sit an order of magnitude inside it by the new one. It inserts 0 reloads before this change and 1100 after. --- crates/perry-codegen/src/root_reload.rs | 67 +++++++++++----- crates/perry-codegen/src/root_reload_tests.rs | 76 +++++++++++++++++++ 2 files changed, 124 insertions(+), 19 deletions(-) diff --git a/crates/perry-codegen/src/root_reload.rs b/crates/perry-codegen/src/root_reload.rs index 6f2f8df37c..efc23f6f5f 100644 --- a/crates/perry-codegen/src/root_reload.rs +++ b/crates/perry-codegen/src/root_reload.rs @@ -253,13 +253,37 @@ const NON_COLLECTING: &[&str] = &[ ]; /// Guard against a pathological function turning this pass into the compile's -/// bottleneck: the reachability walk is O(blocks) per slot load, so the product -/// is what matters. Above the cap the function keeps today's IR — the pass is -/// an improvement, not a correctness precondition, so declining is safe. +/// bottleneck: the reachability walk is O(blocks) per ROOT LOAD — one walk per +/// `groups` entry below, not one per reloadable value — so `blocks × groups` is +/// the product that bounds the cost. /// /// Sized from the corpora: the largest function in the dependency-scale corpus /// (`zod`'s parse core) is ~1400 blocks with ~90 slot loads, an order of /// magnitude under this. +/// +/// # Declining is NOT correctness-neutral (#9135 follow-up) +/// +/// The previous comment here read "the pass is an improvement, not a +/// correctness precondition, so declining is safe". Under the NATIVE root +/// lowering that is false, and Claude-of-Duty's `Arm.constructor` is the +/// counterexample: the receiver of an inline class-field store is read out of +/// its slot, unmasked to an `i64`/`double`, and carried across `buildSleeve`. +/// RS4GC relocates the `addrspace(1)` load but cannot touch the unmasked copy +/// (the "case 2" shape in this module's header), so when this pass declines +/// NOTHING re-reads the slot and the store lands in a from-space object. It +/// faults under `PERRY_GC_PROTECT_FROMSPACE=1` and silently corrupts the field +/// without it. +/// +/// That function measured 4924 blocks × 747 root loads = 3.7M — comfortably +/// under this cap — but the check multiplied by `values.len()` (2102, every +/// pure-bit-op derivation counted separately since #7664) for 10.4M, so it +/// declined on a metric ~2.8x larger than the cost it was guarding. +/// +/// So the bound stays (a real pathological function must still be able to opt +/// out), but it is measured against the walk's actual driver. A function that +/// genuinely exceeds it still keeps today's IR and can still carry a stale +/// register: the cap trades a compile-time cliff for a correctness risk, and +/// that residual risk is why the number is generous rather than tight. const MAX_BLOCK_LOAD_PRODUCT: usize = 8_000_000; /// How long a derivation may get before the pass declines to re-materialise it. @@ -549,7 +573,27 @@ pub(crate) fn apply_to_function(func: &mut LlFunction) -> usize { break; } } - if blocks.len().saturating_mul(values.len()) > MAX_BLOCK_LOAD_PRODUCT { + // Built BEFORE the cap check: `groups` is what the reachability walk below + // iterates (one walk per root load), so it is the term the cap must be + // measured against. See `MAX_BLOCK_LOAD_PRODUCT`. + // + // Keyed on `(recipe[0], root_ptr)`, not `recipe[0]` alone — #7725's capture-GET extension is + // the one case where a chain's `root_ptr` CHANGES partway through (the closure-ptr slot + // becomes a synthetic per-index key once the derivation passes through + // `js_closure_get_capture_bits`), so two sub-chains sharing one root LOAD can need two + // different invalidation conditions. Before #7725 every member of a `recipe[0]` group had + // the identical `root_ptr` by construction (it was always inherited, never overridden), so + // this is additive: it can only split a group that would otherwise have mixed two + // conditions under one, never change the grouping of any existing (non-capture) chain. + let mut groups: HashMap<((usize, usize), String), Vec> = HashMap::new(); + for (i, v) in values.iter().enumerate() { + groups + .entry((v.recipe[0], v.root_ptr.clone())) + .or_default() + .push(i); + } + + if blocks.len().saturating_mul(groups.len()) > MAX_BLOCK_LOAD_PRODUCT { return 0; } @@ -604,21 +648,6 @@ pub(crate) fn apply_to_function(func: &mut LlFunction) -> usize { // // Grouping by root load also puts the cost back at O(blocks × loads). // - // Keyed on `(recipe[0], root_ptr)`, not `recipe[0]` alone — #7725's capture-GET extension is - // the one case where a chain's `root_ptr` CHANGES partway through (the closure-ptr slot - // becomes a synthetic per-index key once the derivation passes through - // `js_closure_get_capture_bits`), so two sub-chains sharing one root LOAD can need two - // different invalidation conditions. Before #7725 every member of a `recipe[0]` group had - // the identical `root_ptr` by construction (it was always inherited, never overridden), so - // this is additive: it can only split a group that would otherwise have mixed two - // conditions under one, never change the grouping of any existing (non-capture) chain. - let mut groups: HashMap<((usize, usize), String), Vec> = HashMap::new(); - for (i, v) in values.iter().enumerate() { - groups - .entry((v.recipe[0], v.root_ptr.clone())) - .or_default() - .push(i); - } let mut group_keys: Vec<((usize, usize), String)> = groups.keys().cloned().collect(); group_keys.sort_unstable(); diff --git a/crates/perry-codegen/src/root_reload_tests.rs b/crates/perry-codegen/src/root_reload_tests.rs index 47303a4e73..28b192c378 100644 --- a/crates/perry-codegen/src/root_reload_tests.rs +++ b/crates/perry-codegen/src/root_reload_tests.rs @@ -1076,3 +1076,79 @@ fn the_slot_layout_note_helpers_are_non_collecting() { transposition of js_gc_note_slot_layout" ); } + +/// The cost cap must be measured against the number of ROOT LOADS, not the +/// number of reloadable values. +/// +/// `MAX_BLOCK_LOAD_PRODUCT` guards the reachability walk, and that walk runs +/// once per `groups` entry — one per root load — not once per reloadable +/// value. Since #7664 extended a recipe through pure bit ops, `values` counts +/// every derivation separately, so `blocks * values` can be several times +/// `blocks * groups`. Measuring the cap against `values` declined functions +/// whose real cost was well inside the bound, and a declined function keeps +/// every stale register: under the native root lowering nothing else re-reads +/// the slot. +/// +/// Each block below carries one root load plus a `MAX_RECIPE`-length pure +/// derivation off it — the `masked_receiver` shape, replicated — so `values` +/// is 8x `groups`. At 1100 blocks that is ~9.7M by the old metric against an +/// 8M cap, and ~1.2M by the new one. The reloads must still be inserted. +/// +/// Found on Claude-of-Duty's `Arm.constructor` (4924 blocks, 747 root loads, +/// 2102 values): 10.35M by the old metric, 3.68M by the new. Declining there +/// left the receiver of an inline class-field store unrooted across a call, +/// and the store landed in a from-space object. +#[test] +fn the_cap_counts_root_loads_not_derived_values() { + // `load + bitcast + 6 * and` is exactly MAX_RECIPE (8) steps. A longer + // chain is refused outright by the recipe fixpoint, which is a different + // decline and would not exercise the cap at all. + const BLOCKS: usize = 1100; + const MASKS: usize = 6; + + let mut f = LlFunction::new("wide", DOUBLE, vec![(DOUBLE, "%arg".into())]); + let mut labels = Vec::with_capacity(BLOCKS + 1); + labels.push(f.create_block("entry").label.clone()); + for i in 0..BLOCKS { + labels.push(f.create_block(&format!("b{i}")).label.clone()); + } + + let slot; + { + let b = f.block_mut(0).unwrap(); + slot = b.alloca(DOUBLE); + b.store(DOUBLE, "%arg", &slot); + b.call_void( + "js_shadow_slot_bind", + &[(crate::types::I32, "0"), (PTR, &slot)], + ); + b.br(&labels[1]); + } + + for i in 0..BLOCKS { + let next = labels.get(i + 2).cloned(); + let b = f.block_mut(i + 1).unwrap(); + let boxed = b.load(DOUBLE, &slot); + let mut cur = b.bitcast_double_to_i64(&boxed); + for _ in 0..MASKS { + cur = b.and(I64, &cur, "281474976710655"); + } + b.call(DOUBLE, "js_object_get_field_by_name_f64", &[]); + b.call_void( + "js_object_set_field_by_name", + &[(I64, &cur), (DOUBLE, "0.0")], + ); + match next { + Some(n) => b.br(&n), + None => b.ret(DOUBLE, "0.0"), + } + } + + let rewrites = apply_to_function(&mut f); + assert_eq!( + rewrites, BLOCKS, + "every block holds one stale masked receiver across a collecting call; \ + measuring the cap against the derived-value count declines the whole \ + function and silently leaves all of them in place" + ); +}