diff --git a/changelog.d/7206-stale-receiver-registers.md b/changelog.d/7206-stale-receiver-registers.md new file mode 100644 index 0000000000..0cea8a53a1 --- /dev/null +++ b/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. diff --git a/crates/perry-codegen/src/expr/index_get.rs b/crates/perry-codegen/src/expr/index_get.rs index 63fc244487..7f2ba03be3 100644 --- a/crates/perry-codegen/src/expr/index_get.rs +++ b/crates/perry-codegen/src/expr/index_get.rs @@ -1987,7 +1987,24 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { 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,11 +2016,13 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { "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, @@ -2011,7 +2030,13 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // 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 { 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 diff --git a/crates/perry-codegen/src/lower_call/console_promise.rs b/crates/perry-codegen/src/lower_call/console_promise.rs index dc49d5cebd..4bc662bb1f 100644 --- a/crates/perry-codegen/src/lower_call/console_promise.rs +++ b/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 = 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 = 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 = 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) diff --git a/scripts/gc_root_dominance_check.py b/scripts/gc_root_dominance_check.py index d15e390edc..9dcd084a18 100755 --- a/scripts/gc_root_dominance_check.py +++ b/scripts/gc_root_dominance_check.py @@ -85,6 +85,8 @@ real corpus. """ import argparse +import contextlib +import io import os import re import sys @@ -758,6 +760,251 @@ def scan(blk, lo, hi): """ +# ------------------------------------------------- stale-register invariant +# +# The check above anchors on a shadow-slot BIND, so it can only see values that +# are eventually rooted. The residual #7154 offender is not one of those: the +# receiver of `inst.check(f())` is read out of a closure capture cell, kept in a +# bare register across `f()`, and never rooted at all. `--stale-registers` +# checks the more general invariant the module header states: +# +# No register holding a GC pointer may be USED below a collection point. +# After the last thing that can collect, every operand is either re-read from +# a root the collector rewrote, or re-derived from immutable storage. +# +# A "heap-value source" is an instruction that yields a value which may be a GC +# pointer. Two kinds: +# +# * an ALLOCATION (ALLOC_RE) -- unambiguously a fresh heap object; +# * a ROOT READ -- a load of a location the collector rewrites (a shadow-slot +# alloca, a closure capture cell, a temp-root slot, a module global, a +# mutable-capture box). Reading one produces a register that is correct +# *now* and stale after the next evacuation, which is precisely the +# "property (2) without property (3)" failure `temp_root.rs` describes. +# +# `js_closure_get_capture_bits` is the load-bearing one and it is deliberately +# in NONCOLLECTING (closure/alloc.rs:463 is a raw slot read that cannot +# allocate) -- being non-collecting is exactly what makes it a root READ rather +# than a collection point. + +# Loads of these globals are root reads: module-level variables live in +# `@perry_global_*` and are registered roots that evacuation rewrites. +GLOBAL_ROOT_RE = re.compile(r"@perry_global_[\w.$]+") + +# Calls that READ a collector-rewritten location into a register. +ROOT_READ_CALLS = { + "js_closure_get_capture_bits", # closure/alloc.rs:463 capture cell read + "js_closure_get_capture_ptr", + "js_gc_temp_root_get", # a MUTABLE root: rewritten on evacuation + "js_box_get_bits", # box.rs mutable-capture cell read + "js_implicit_this_get", # object/this_binding.rs:160 thread-local + "js_new_target_get", +} + +# Argument positions that make a stale pointer FATAL rather than merely wrong: +# the value is dereferenced as an object. Used only for ranking, never to +# suppress -- the check stays one-sided. +RECEIVER_SINKS = re.compile( + r"^js_(" + r"typed_feedback_native_call_method\w*|native_call_method\w*|" + r"native_call_value|object_get_field\w*|object_set_field\w*|" + r"object_get_property|object_set_property|put_value_set_dyn_ic|" + r"get_value_dyn_ic|closure_call\w*|call_closure|call_function|" + r"call_value|invoke_closure|apply_function|" + r"array_\w+|map_\w+|set_\w+|typed_feedback_\w*call\w*" + r")$" +) + + +def heap_source_kind(ins, slot_of_alloca): + """Classify `ins` as a producer of a possibly-GC-pointer register.""" + if ins.result is None: + return None + if ins.callee is not None: + if ALLOC_RE.match(ins.callee): + return "alloc" + if ins.callee in ROOT_READ_CALLS: + return "capture" if "capture" in ins.callee else "rootread" + return None + if "= load " in ins.text: + if GLOBAL_ROOT_RE.search(ins.text): + return "global" + m = re.search(r"load\s+(?:i64|double)\s*,\s*ptr %([\w.$]+)", ins.text) + if m and m.group(1) in slot_of_alloca: + return "slotload" + return None + + +class StaleUse: + def __init__(self, module, func, src, kind, use, collectors, reg, + poll_reaching=frozenset()): + self.module = module + self.func = func + self.src = src + self.kind = kind + self.use = use + self.collectors = collectors + self.reg = reg + self.poll_reaching = poll_reaching + + @property + def movers(self): + return sorted({c.callee for c in self.collectors + if c.callee == MOVING_POLL or c.callee in self.poll_reaching + or c.callee in POLL_CAPABLE_RUNTIME}) + + @property + def moving(self): + return bool(self.movers) + + @property + def fatal_sink(self): + return bool(self.use.callee and RECEIVER_SINKS.match(self.use.callee)) + + +def check_func_stale(module, f, poll_reaching=frozenset(), moving_only=False): + """Report registers holding a heap value that are USED below a collection + point without being re-read from a root.""" + if not f.blocks: + return [] + slot_of_alloca = {} + for b in f.blocks: + for ins in f.insns[b]: + m = BIND_RE.search(ins.text) + if m: + slot_of_alloca[m.group(2)] = int(m.group(1)) + + def_of = {} + for b in f.blocks: + for ins in f.insns[b]: + if ins.result: + def_of[ins.result] = ins + + idom = dominators(f) + out = [] + + for b in f.blocks: + for src in f.insns[b]: + kind = heap_source_kind(src, slot_of_alloca) + if kind is None: + continue + # Forward closure over bit-level/identity ops: the untagged + # pointer, the nanbox and the bitcast are all the same address. + chain = {src.result} + grew = True + while grew: + grew = False + for bb in f.blocks: + for ins in f.insns[bb]: + if (ins.result and ins.result not in chain + and is_transparent(ins) + and uses(ins.text, chain)): + chain.add(ins.result) + grew = True + # First real (non-transparent) use of any register in the chain + # that sits below a collection point. + for bb in f.blocks: + if not dominates(idom, src.block, bb): + continue + for use in f.insns[bb]: + if use.result in chain or is_transparent(use): + continue + if not uses(use.text, chain): + continue + if use.block == src.block and use.idx <= src.idx: + continue + # `js_shadow_slot_bind(N, ptr %alloca)` names the alloca, + # not the value; a store THROUGH the chain is a use of the + # pointer operand only when the chain is the stored value. + hits = window_hits_generic(f, src, use) + if not hits: + continue + v = StaleUse(module, f.name, src, kind, use, hits, + src.result, poll_reaching) + if moving_only and not v.moving: + continue + out.append(v) + break + else: + continue + break + return out + + +def window_hits_generic(f, A, B): + """Collecting calls on some CFG path from just after A to just before B.""" + hits = [] + if A.block == B.block: + for c in f.insns[A.block]: + if is_collecting(c.callee) and A.idx < c.idx < B.idx: + hits.append(c) + return hits + for c in f.insns[A.block]: + if is_collecting(c.callee) and c.idx > A.idx: + hits.append(c) + for c in f.insns[B.block]: + if is_collecting(c.callee) and c.idx < B.idx: + hits.append(c) + for m_blk in between_blocks(f, A.block, B.block): + for c in f.insns[m_blk]: + if is_collecting(c.callee): + hits.append(c) + return hits + + +def run_stale(parsed, poll_reaching, verbose, moving_only, fatal_only, + max_stale=None): + total = 0 + per_kind = defaultdict(int) + per_sink = defaultdict(int) + out = [] + for mod, fs in parsed: + for f in fs: + for v in check_func_stale(mod, f, poll_reaching, moving_only): + if fatal_only and not v.fatal_sink: + continue + total += 1 + per_kind[v.kind] += 1 + per_sink[v.use.callee or "store"] += 1 + cs = sorted({c.callee for c in v.collectors}) + out.append( + f"{mod}::{f.name}\n" + f" source ({v.kind}): {v.src.text.strip()}\n" + f" stale use : {v.use.text.strip()}\n" + f" between : {', '.join(cs[:6])}" + f"{' (+%d more)' % (len(cs) - 6) if len(cs) > 6 else ''}\n" + f" MOVING : " + f"{('YES via ' + ', '.join(v.movers[:3])) if v.moving else 'no'}\n" + ) + if verbose: + print("\n".join(out)) + print(f"=== stale-register uses: {total}") + for k, n in sorted(per_kind.items(), key=lambda kv: -kv[1]): + print(f" {n:6d} source={k}") + for k, n in sorted(per_sink.items(), key=lambda kv: -kv[1])[:15]: + print(f" {n:6d} sink={k}") + + # This mode is a DIAGNOSTIC, and unlike the bind-anchored check it is not + # calibrated to zero. The count is dominated by values the checker cannot + # prove are pointers, and even the `--fatal-sinks` slice still carries a + # known-unfixed class (`js_closure_call*`, #7154), so `!= 0` is the normal + # state of a healthy tree. Exiting 1 on any hit would make this a check + # that can never pass, which is the same failure as the four "a gate that + # cannot fail" hazards in CLAUDE.md read backwards: every caller learns to + # ignore the exit status, and the day it means something nobody is looking. + # Ranked leads are the product. Gating is opt-in and explicit via + # --max-stale, which is how a calibrated slice becomes a ratchet later. + if max_stale is not None: + if total > max_stale: + print(f"error: {total} stale-register use(s), budget is " + f"{max_stale}. Lower the count or raise --max-stale " + "deliberately.", file=sys.stderr) + return 1 + print(f"within budget: {total} <= {max_stale}") + return 0 + + + # ------------------------------------------------- unrooted-alloca check --- # # The third way the invariant breaks (#7202), and the one the bind-anchored @@ -1018,6 +1265,20 @@ def _scan(paths, moving_only, anchor): return found, binds +def _stale_probe(path, max_stale): + """(reported uses, exit status) from --stale-registers over one file.""" + parsed = [(os.path.basename(path), parse_file(path))] + poll_reaching, _known = compute_poll_reaching( + [f for _m, fs in parsed for f in fs]) + buf = io.StringIO() + # Both streams: an over-budget probe is *expected* to print its error, and + # that line in the self-test transcript reads like a real failure. + with contextlib.redirect_stdout(buf), contextlib.redirect_stderr(buf): + rc = run_stale(parsed, poll_reaching, False, False, False, max_stale) + m = re.search(r"stale-register uses: (\d+)", buf.getvalue()) + return (int(m.group(1)) if m else -1), rc + + def self_test(): """Assert the checker reports the planted violation and clears the control. @@ -1062,6 +1323,32 @@ def self_test(): file=sys.stderr) ok = False + # --stale-registers is a diagnostic, so its exit status is asserted + # from both ends: the default must NOT go red on a corpus that has + # hits, and --max-stale must actually be able to. A budget nobody has + # watched fail is a budget that has not been shown to work. + n_planted, rc_planted = _stale_probe(planted, None) + if n_planted != 2: + print(f"self-test FAIL: --stale-registers over the planted fixture " + f"-> {n_planted} uses, expected 2", file=sys.stderr) + ok = False + if rc_planted != 0: + print("self-test FAIL: --stale-registers without --max-stale must " + f"exit 0 (diagnostic), got {rc_planted}", file=sys.stderr) + ok = False + if _stale_probe(planted, 0)[1] != 1: + print("self-test FAIL: --max-stale 0 over the planted fixture must " + "exit 1; the budget cannot fail", file=sys.stderr) + ok = False + if _stale_probe(planted, 2)[1] != 0: + print("self-test FAIL: --max-stale 2 over the planted fixture must " + "exit 0; the budget is off by one", file=sys.stderr) + ok = False + if _stale_probe(clean, 0) != (0, 0): + print("self-test FAIL: --max-stale 0 over the control fixture must " + "report 0 uses and exit 0", file=sys.stderr) + ok = False + try: _scan([broken], False, "alloc") except MalformedIR: @@ -1128,6 +1415,21 @@ def main(): ap.add_argument("-v", "--verbose", action="store_true") ap.add_argument("--self-test", action="store_true", help="run the built-in planted/clean fixtures and exit") + ap.add_argument("--stale-registers", action="store_true", + help="check the general stale-register invariant instead of " + "the bind-anchored one: report every register holding a " + "GC value that is USED below a collection point without " + "being re-read from a root (#7154). Diagnostic: this " + "mode exits 0 and reports counts unless --max-stale " + "gives it a budget") + ap.add_argument("--fatal-sinks", action="store_true", + help="with --stale-registers, keep only uses that " + "DEREFERENCE the stale value (a call receiver/callee), " + "where a relocation is fatal rather than merely wrong") + ap.add_argument("--max-stale", type=int, default=None, metavar="N", + help="with --stale-registers, exit 1 when more than N uses " + "are reported. Without it the mode is a ranked lead " + "list, not a pass/fail number, so it exits 0.") ap.add_argument("--min-files", type=int, default=1, metavar="N", help="fail unless at least N .ll files were scanned (default 1)") ap.add_argument("--min-binds", type=int, default=1, metavar="N", @@ -1140,6 +1442,15 @@ def main(): "Disjoint from the bind-anchored check by construction.") ns = ap.parse_args() + # A knob that is silently ignored is a disarmed knob: `--max-stale 0` + # without `--stale-registers` would run the bind-anchored check and look + # like it enforced a budget, and `--fatal-sinks` alone would look like it + # narrowed a report it never reached. Refuse instead (argparse exits 2). + if ns.max_stale is not None and not ns.stale_registers: + ap.error("--max-stale requires --stale-registers") + if ns.fatal_sinks and not ns.stale_registers: + ap.error("--fatal-sinks requires --stale-registers") + if ns.self_test: return self_test() @@ -1173,6 +1484,9 @@ def main(): parsed.append((os.path.basename(p), parse_file(p))) poll_reaching, _known = compute_poll_reaching( [f for _m, fs in parsed for f in fs]) + if ns.stale_registers: + return run_stale(parsed, poll_reaching, verbose, moving_only, + ns.fatal_sinks, ns.max_stale) n_binds = sum( 1 for _m, fs in parsed diff --git a/test-files/test_gap_gc_index_get_receiver_rooting.ts b/test-files/test_gap_gc_index_get_receiver_rooting.ts new file mode 100644 index 0000000000..3fb1ddfe0b --- /dev/null +++ b/test-files/test_gap_gc_index_get_receiver_rooting.ts @@ -0,0 +1,58 @@ +// #7154: the BASE of a computed property READ must be rooted across the +// evaluation of the key expression. +// +// `o[f()]` evaluates the base first and the key second — spec order, and +// codegen follows it — which left the base in a bare SSA register while `f()` +// was lowered. `f()` allocates, and under `PERRY_GC_MOVING_LOOP_POLLS=1` a loop +// back-edge poll inside it runs an evacuating minor. The base SURVIVES (the +// closure capture cell / module global holding it is a root), so it MOVES: the +// collector rewrites that location but not the register. The field read then +// walks the keys array of abandoned from-space memory — a SIGSEGV inside +// `get_field_by_name_object_tail`, or a silently wrong value. +// +// This is the READ counterpart of #7192's `index_set` / `property_set` receiver +// guard, which fixed only the STORE side (`o[f()] = v`). In the registry it is +// zod's `core/checks.ts:68`, +// `numericOriginMap[typeof def.value as "number" | "bigint" | "object"]` — +// a module-global base with a key expression that reads a property, and +// therefore can collect. +// +// LIVE BY CONSTRUCTION. The base is read out of a closure capture, so it is a +// young movable object; the key expression allocates long enough that the minor +// runs EARLY in it and the abandoned from-space copy is then reused by the rest +// of the key's own allocation; and the value read back is a heap object whose +// field is dereferenced, so a stale read is observable rather than latent. +// Clean under a non-moving collector, so the evacuating arms are the ones that +// bite. + +function keyOf(v: number): string { + const bits: any[] = []; + for (let i = 0; i < 4000; i++) { + bits.push({ i: i, s: "x", pad: [i, i + 1, i + 2] }); + } + return bits.length === 4000 ? "hit" : "miss"; +} + +function make(tag: number): (k: number) => number { + const originMap: any = { + hit: { v: tag, name: "hit" }, + miss: { v: -1, name: "miss" }, + other: { v: -2, name: "other" }, + spare: { v: -3, name: "spare" }, + }; + return (k: number) => (originMap[keyOf(k)] as any).v as number; +} + +function run(): number { + let bad = 0; + for (let r = 0; r < 200; r++) { + const f = make(r); + const got = f(r); + if (got !== r) { + bad++; + } + } + return bad; +} + +console.log("bad", run()); diff --git a/test-files/test_gap_gc_method_receiver_rooting.ts b/test-files/test_gap_gc_method_receiver_rooting.ts new file mode 100644 index 0000000000..72bb1d8dda --- /dev/null +++ b/test-files/test_gap_gc_method_receiver_rooting.ts @@ -0,0 +1,61 @@ +// #7154: the RECEIVER of a dynamic method call must be rooted across the +// evaluation of the call's arguments. +// +// `recv.m(f())` evaluates the receiver first and the arguments second — spec +// order, and codegen follows it — which left the receiver in a bare SSA +// register while `f()` was lowered. `f()` allocates, and under +// `PERRY_GC_MOVING_LOOP_POLLS=1` a loop back-edge poll inside it runs an +// evacuating minor. The receiver SURVIVES that minor (the closure capture cell +// holding it is a root), which means it MOVES: the collector rewrites the +// capture cell but not the caller's register. The dispatch then resolves `m` +// against abandoned from-space 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 fixed the four alloc-anchored +// sites. In the registry it is zod's `classic/schemas.ts:301`, +// `inst.regex = (...args) => inst.check(checks.regex(...args))` — `inst` read +// from the arrow's capture cell, held across a real user call, then used as the +// receiver of `.check`. +// +// Same invariant as #7192, #7184 and #7114: a GC value's root must dominate +// every subsequent collection point, and a rewritten location is worthless +// unless the code below the collection point READS that location again. +// +// LIVE BY CONSTRUCTION. `inst` is a plain object literal read out of a closure +// capture, so the call takes the dynamic by-name dispatch rather than a static +// class-method call, and the argument allocates hard enough to reach the +// collector. A non-moving collection cannot expose this, so the evacuating arms +// are the ones that bite. + +function churn(x: number): number { + const bits: any[] = []; + for (let i = 0; i < 600; i++) { + bits.push({ i: i, s: "x" }); + } + return x + bits.length - 600; +} + +function make(t: number): (p: number) => number { + const inst: any = { + tag: t, + check(v: number): number { + return this.tag + v; + }, + }; + return (p: number) => inst.check(churn(p)); +} + +function run(): number { + let bad = 0; + for (let r = 0; r < 400; r++) { + const f = make(r); + const got = f(1); + if (got !== r + 1) { + bad++; + } + } + return bad; +} + +console.log("bad", run()); diff --git a/test-parity/gc_repsel_corpus.txt b/test-parity/gc_repsel_corpus.txt index 77725895cb..2b953831e1 100644 --- a/test-parity/gc_repsel_corpus.txt +++ b/test-parity/gc_repsel_corpus.txt @@ -209,3 +209,26 @@ test_gap_gc_inline_ctor_this_rooting # `requires=move` arms. test_gap_gc_catch_param_rooting test_gap_gc_closure_this_capture_rooting + +# --- Stale REGISTERS: a receiver and a computed base (#7206) ----------------- +# A different failure from every entry above. The sites above are root stores +# that are late, mis-indexed or missing; these two values are never rooted at +# all. Spec order evaluates the object first and the sibling operand second, so +# `recv.m(f())` and `o[f()]` leave the base in a bare SSA register while `f()` +# runs. It SURVIVES the minor -- the capture cell or module global it was read +# from is a root -- and therefore MOVES, the collector rewrites that location, +# and the register keeps naming from-space. A bare register is not a root, so +# no runtime GC probe can see it. +# +# Measured on `origin/main` (91170973c), compiled AND run with +# `PERRY_GC_MOVING_LOOP_POLLS=1`, oracle node 26.5.1 (`bad 0` for both): +# method_receiver_rooting `TypeError: value is not a function`, and +# exit=139 (SIGSEGV) under PERRY_GC_ZEAL=1 + +# PERRY_GC_PROTECT_FROMSPACE=1 +# index_get_receiver_rooting `TypeError: Cannot read properties of undefined +# (reading 'v')` +# Both are `bad 0` with #7206 applied, and `bad 0` on the shipped default on +# BOTH sides -- the default cannot express the bug, which is why these belong on +# the `requires=move` arms and prove nothing on `default`. +test_gap_gc_method_receiver_rooting +test_gap_gc_index_get_receiver_rooting