fix(codegen): root a call receiver and a computed-read base across their sibling operands

changelog.d/7206-stale-receiver-registers.md

@@ -0,0 +1,108 @@
+### Fixed
+
+- **A method call's receiver and a computed read's base are now rooted across
+  the expressions lowered between them and the dispatch.** Two more sites of
+  #7192's root-store-dominance class, both found by extending its checker and
+  both reproduced in ~30 lines of TypeScript.
+
+  `recv.m(f())` and `o[f()]` evaluate the reference first and the second
+  operand after — spec order, and codegen follows it. That left the reference
+  in a bare SSA register while `f()` was lowered, and `f()` allocates. Under
+  `PERRY_GC_MOVING_LOOP_POLLS=1` a loop back-edge poll inside it runs an
+  evacuating minor. The reference *survives* that minor — the closure capture
+  cell, shadow slot or module global holding it is a root — so it **moves**:
+  the collector rewrites that location and the register keeps naming
+  from-space. This is the same "property (2) — a rewritten location — is
+  worthless without property (3), reading that location again below the
+  collection point" that `expr/temp_root.rs`'s module header describes, and the
+  same fix #7192 applied to the property/element STORE receiver.
+
+  - **`lower_call/console_promise.rs`** — the `js_native_call_method_by_id`
+    dispatch. The stale receiver makes the method lookup resolve against
+    abandoned memory, so the call throws `TypeError: value is not a function`.
+    In `sfw-registry` this is zod's `classic/schemas.ts:301`,
+    `inst.regex = (...args) => inst.check(checks.regex(...args))`: `inst` is
+    read out of the arrow's capture cell, held across `checks.regex(...args)`
+    (a real user call, so it polls), then used as `.check`'s receiver. The
+    arguments are now rooted too — each before the *next* one is lowered, per
+    `RootedOperands`' incremental contract — so an earlier argument cannot go
+    stale across a later one either.
+  - **`expr/index_get.rs`** — the dynamic-string-key arm and the last-resort
+    runtime-tag-check arm, i.e. the READ counterpart of #7192's `index_set` /
+    `property_set` guard, which only covered the store side. The stale base
+    makes the field read walk the keys array of from-space memory: a SIGSEGV
+    inside `get_field_by_name_object_tail`, or a silently wrong value. In
+    `sfw-registry` this is zod's `core/checks.ts:68`,
+    `numericOriginMap[typeof def.value]` — a module-global base with a key
+    expression that reads a property and therefore can collect.
+
+  Both use `temp_root::guard_store_operand` / `reread_store_operand` /
+  `release_store_operand` (#7198's generalized naming). A temp root, not a
+  re-lower: re-lowering the reference would observe an assignment made by the
+  second operand itself, which is a miscompile rather than a rooting fix. The
+  guard emits nothing when the sibling expression cannot collect, so an inert
+  argument list or key keeps its previous IR exactly, and it is released
+  *after* the dispatch because the dispatcher allocates while reading these
+  values.
+
+  Verified by two new gap tests, each red on the parent commit and green after,
+  and each clean under a non-moving collector so the failure is proven to track
+  collector mode rather than luck:
+
+  | | parent | this change |
+  |---|---|---|
+  | `test_gap_gc_method_receiver_rooting.ts`, `POLLS=1` | `TypeError: value is not a function` 5/5 | `bad 0` **10/10** |
+  | `test_gap_gc_index_get_receiver_rooting.ts`, `POLLS=1` | `TypeError: Cannot read properties of undefined` 4/4 | `bad 0` **10/10** |
+  | both, `POLLS=1` + `PERRY_GEN_GC=0` | `bad 0` | `bad 0` |
+  | both, default (no polls) | `bad 0` | `bad 0` 5/5 |
+
+### Changed
+
+- **`scripts/gc_root_dominance_check.py` grows a `--stale-registers` mode.**
+  The shipped check anchors on a shadow-slot bind, so it can only see values
+  that are eventually rooted; neither site above is. The new mode checks the
+  more general invariant the module header already states — *no register
+  holding a GC pointer may be used below a collection point without being
+  re-read from a root* — by classifying every **heap-value source** (an
+  allocation, or a read of a collector-rewritten location: a shadow-slot load,
+  a closure capture cell, a temp-root slot, a module global, a mutable-capture
+  box), following it forward through bit-level identity ops, and reporting the
+  first real use that sits below a collecting call. `--fatal-sinks` narrows to
+  uses that *dereference* the value (a call receiver or callee), where a
+  relocation is fatal rather than merely wrong.
+
+  It reproduces both sites above independently of any runtime probe, and it is
+  how they were found. Over the 141-module `sfw-registry` corpus the
+  fatal-sink slice went **986 → 729** with this change; the entire
+  `js_typed_feedback_native_call_method_by_id` class (257) is gone. The
+  remaining 593 are `js_closure_callN` — the generic dynamic-value-call
+  lowering, which holds the callee AND the `this` receiver AND each argument in
+  registers across the argument list. That is the next site of this class and
+  it is not fixed here.
+
+  Like the bind-anchored check the mode is one-sided: `NONCOLLECTING` is the
+  only place a call is declared safe. It is a diagnostic, not a gate — the raw
+  (non-`--fatal-sinks`) count is dominated by values the checker cannot prove
+  are pointers, so it is a ranked lead list rather than a pass/fail number.
+
+  **The exit status says so.** `--stale-registers` prints its counts and exits
+  `0`; it is not calibrated to zero, and a mode that returned `1` on any hit
+  would be a check that can never pass — the mirror image of CLAUDE.md's four
+  "a gate that cannot fail" hazards, and just as reliably ignored. Gating is
+  opt-in through the new **`--max-stale N`**, which exits `1` when more than
+  `N` uses are reported, so a slice that *has* been calibrated (say the
+  `--fatal-sinks` count once `js_closure_call*` is fixed) can become a ratchet
+  without the raw mode pretending to be one. Passing `--max-stale` or
+  `--fatal-sinks` without `--stale-registers` is a usage error (exit 2) rather
+  than a silently ignored budget or an ignored filter — either one would run
+  the bind-anchored check while looking like it did something else.
+  Misconfiguration keeps its own status: `--min-files` still makes an
+  empty corpus exit 2 in this mode too, and the bind-anchored gate that
+  `gc-root-dominance.yml` actually runs is untouched — it still exits non-zero
+  on any violation.
+
+  `--self-test` grew four arms for this, so the exit status is asserted from
+  both ends rather than assumed: over the planted fixture the default mode
+  must report 2 uses and still exit `0`, `--max-stale 0` must exit `1`,
+  `--max-stale 2` must exit `0`, and the control fixture must report zero.
+  Reverting the default to `return 1 if total else 0` fails three of them.

crates/perry-codegen/src/expr/index_get.rs

@@ -1987,7 +1987,24 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
                 let preserve_class_ref_bits =
                     index_object_is_class_or_proto_ref(ctx, object.as_ref());
                 let obj_box = lower_expr(ctx, object)?;
+                // #7154: `o[f()]` evaluates the base first and the key second,
+                // leaving the base in a bare SSA register while `f()` runs. An
+                // evacuating minor inside `f()` relocates the base; the location
+                // it was read from is a root and gets rewritten, the register is
+                // not, and the field read then dereferences from-space memory.
+                //
+                // This is the READ counterpart of #7192's `index_set` /
+                // `property_set` receiver guard. zod's `core/checks.ts:68`
+                // (`numericOriginMap[typeof def.value]`) is the instance that
+                // SIGSEGV'd `sfw-registry --help` under
+                // `PERRY_GC_MOVING_LOOP_POLLS=1`: `numericOriginMap` is a module
+                // global (a registered root the collector rewrites) and the key
+                // `typeof def.value` is a property get that can collect.
+                let recv_guard =
+                    super::temp_root::guard_store_operand(ctx, object, &obj_box, index);
                 let key_box = lower_expr(ctx, index)?;
+                let obj_box =
+                    super::temp_root::reread_store_operand(ctx, &recv_guard, object, &obj_box)?;
                 let blk = ctx.block();
                 let obj_bits = blk.bitcast_double_to_i64(&obj_box);
                 let obj_handle =
@@ -1999,19 +2016,27 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
                     "object[index]",
                     TypedFeedbackContract::object_get_by_name(),
                 );
-                return Ok(ctx.block().call(
+                let out = ctx.block().call(
                     DOUBLE,
                     "js_typed_feedback_object_get_field_by_name_f64",
                     &[(I64, &site_id), (I64, &obj_handle), (I64, &key_handle)],
-                ));
+                );
+                super::temp_root::release_store_operand(ctx, recv_guard);
+                return Ok(out);
             }
             // Last-resort fallback with runtime tag checks on the index.
             // First runtime-check whether the index is a Symbol; if so,
             // dispatch to the symbol-property side table — mirrors the
             // IndexSet branch. Otherwise fall through to string/numeric.
             let preserve_class_ref_bits = index_object_is_class_or_proto_ref(ctx, object.as_ref());
             let obj_box = lower_expr(ctx, object)?;
+            // #7154: same window as the dynamic-string-key arm above — the base
+            // is live in a register while the key expression is lowered, and an
+            // evacuating minor inside the key relocates it.
+            let recv_guard = super::temp_root::guard_store_operand(ctx, object, &obj_box, index);
             let idx_box = lower_expr(ctx, index)?;
+            let obj_box =
+                super::temp_root::reread_store_operand(ctx, &recv_guard, object, &obj_box)?;
             // RequireObjectCoercible(base): `null[k]` / `undefined[k]` must throw
             // a TypeError per spec, NOT silently return undefined. The dotted
             // PropertyGet path already guards nullish receivers; the computed
@@ -2114,14 +2139,18 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
             ctx.block().br(&merge_lbl);
             // Merge.
             ctx.current_block = merge_idx;
-            Ok(ctx.block().phi(
+            let merged = ctx.block().phi(
                 DOUBLE,
                 &[
                     (&v_sym, &sym_end_lbl),
                     (&v_str, &str_end_lbl),
                     (&v_num, &num_end_lbl),
                 ],
-            ))
+            );
+            // Released in the merge block so every arm's getter (any of which
+            // can run a user getter and therefore collect) is still covered.
+            super::temp_root::release_store_operand(ctx, recv_guard);
+            Ok(merged)
         }
 
         // Phase H err: `agg.errors.length` — receiver is

crates/perry-codegen/src/lower_call/console_promise.rs

@@ -852,11 +852,52 @@ pub fn try_lower_native_method_str_dispatch(
                     return Ok(Some(reg));
                 }
             }
+            // #7154: `recv.m(f())` evaluates the receiver first and the
+            // arguments second — spec order, and codegen follows it. That left
+            // the receiver in a bare SSA register while every argument was
+            // lowered, and an argument that allocates reaches a back-edge poll
+            // and an evacuating minor. The minor RELOCATES the receiver: the
+            // location it was read from (a closure capture cell, a shadow slot,
+            // a module global) is a root and gets rewritten, but the register is
+            // not a root and keeps naming from-space. The dispatch below then
+            // resolves the method against abandoned memory and throws
+            // `TypeError: value is not a function`.
+            //
+            // This is the site that kept `sfw-registry --help` red under
+            // `PERRY_GC_MOVING_LOOP_POLLS=1` after #7192. zod's
+            // `classic/schemas.ts:301` builds
+            // `inst.regex = (...args) => inst.check(checks.regex(...args))`;
+            // `inst` is read out of the arrow's capture cell, held across
+            // `checks.regex(...args)` (a real user call, so it polls), and then
+            // used as the receiver of `.check`.
+            //
+            // Same shape, same fix as #7192's property/element STORE receiver:
+            // a temp root, not a re-lower. Re-lowering `object` would observe an
+            // assignment made by an argument, which is a miscompile rather than
+            // a rooting fix (see `temp_root::operand_is_reloadable`). Each
+            // argument is likewise rooted before the NEXT one is lowered, so an
+            // earlier argument cannot go stale across a later one.
+            let arg_collects: Vec<bool> = args
+                .iter()
+                .map(|a| crate::expr::temp_root::expr_may_trigger_gc(ctx, a))
+                .collect();
+            let any_arg_collects = arg_collects.iter().any(|&c| c);
+            let operand_exprs: Vec<&Expr> = std::iter::once(object.as_ref())
+                .chain(args.iter())
+                .collect();
+            let mut roots = crate::expr::temp_root::root_operands_begin(args.len() + 1);
             let recv_box = lower_expr(ctx, object)?;
-            let mut lowered_args: Vec<String> = Vec::with_capacity(args.len());
-            for a in args {
-                lowered_args.push(lower_expr(ctx, a)?);
+            roots.push(ctx, object.as_ref(), &recv_box, any_arg_collects);
+            for (i, a) in args.iter().enumerate() {
+                let v = lower_expr(ctx, a)?;
+                roots.push(ctx, a, &v, arg_collects[i + 1..].iter().any(|&c| c));
             }
+            // Re-read below the last collection point. Mandatory, not
+            // defensive: the temp-root slot is a MUTABLE root, so an evacuating
+            // cycle rewrites it and the register pushed beforehand is stale.
+            let rereads = roots.reread(ctx, &operand_exprs)?;
+            let recv_box = rereads[0].clone();
+            let lowered_args: Vec<String> = rereads[1..].to_vec();
             // Pass a tagged pointer to the immutable StringPool dispatch
             // descriptor. A GC-backed string handle belongs to the main
             // thread's arena and cannot be resolved safely by a
@@ -893,7 +934,7 @@ pub fn try_lower_native_method_str_dispatch(
             // call's location no longer shadows this one.
             crate::expr::calls::emit_call_location_at(ctx, call_byte_offset);
             let blk = ctx.block();
-            return Ok(Some(blk.call(
+            let result = blk.call(
                 DOUBLE,
                 "js_typed_feedback_native_call_method_by_id",
                 &[
@@ -903,7 +944,11 @@ pub fn try_lower_native_method_str_dispatch(
                     (PTR, &args_ptr),
                     (I64, &args_len_str),
                 ],
-            )));
+            );
+            // Release AFTER the dispatch, not before: the dispatcher allocates
+            // while it reads these values.
+            roots.release(ctx);
+            return Ok(Some(result));
         }
     }
     Ok(None)