fix(codegen): close #7192's own residual root-store hole, and make the dominance checker able to fail - #7198
Merged
proggeramlug merged 2 commits intoAug 1, 2026
Conversation
added 2 commits
August 1, 2026 18:20
…he checker able to fail Maintenance pass on PerryTS#7192. Four review findings adjudicated reproducer-first; three were real and are fixed here, one was rejected with evidence. REAL — `Expr::ObjectSpread` / `Expr::ClassExprFresh` protect predicates. `protect_handle` was computed from the operand expressions alone, so `{ ...src, tail: 7 }` over inert parts pushed no root at all — and `js_object_copy_own_fields` reads every own key of the source, which runs a getter, which is arbitrary JS that can reach a back-edge poll. Reproduced at the PR head under PERRY_GC_MOVING_LOOP_POLLS=1: `bad 10` deterministic 3/3, clean by default. Same for a class expression carrying `static { … }` with otherwise-inert statics: `bad 4` 3/3. A spread part and a static block now force protection on their own; `block_fns` moves above `rooted_handle_begin` so the predicate can see it. The rest of the predicate stays byte-identical to `Expr::Object`'s, because an allocation inside a runtime helper provably cannot INITIATE a moving collection — `gc_check_trigger`'s minor arm defers to the loop safepoint under polls and is conservative-scanned or budgeted non-moving otherwise. REAL — the inline-ctor result slot. `ctor_result_slot` is a plain `alloca_entry`: not a shadow slot, not a temp root, never rewritten. Seeding it with `obj_box` parked the PRE-constructor instance address in unrooted memory for the whole body, and on fall-through `js_ctor_return_override` saw an *object* in `raw` and returned THAT — discarding the re-read instance the new `reload_instance` had just recovered. The dominance fix was defeated at its last instruction, and the PR's own regression test could not see it because it walks the FIRST operand, which is the re-read one. The slot now starts at `undefined`, which is exactly equivalent on all four paths (fall-through, bare `return;`, `return <expr>`, inherited-symbol ctor) and carries no address. A regression test pins it. REAL — the key operand of a computed store. `o[k] = f()` and a class expression's `[sym]: init` lower the key before the value, leaving it in an SSA register across the same window the receiver had. Rooted with the same guard, released inside-out so the temp-root cuts nest. No runtime reproducer found (the value survives; the staleness is latent until the abandoned memory is reused, which is PerryTS#7154's fingerprint), so this rests on the same static argument as PerryTS#7114. REJECTED — `%entry.implicit` in the checker. No such code exists; the parser drops pre-label instructions rather than synthesising a block, and numeric labels already parse correctly. Perry's IR writer provably cannot emit either shape (`function.rs` emits a label before every block, always `name.counter`), confirmed over 214 real functions / 3608 labels. But the adjacent defect is real and worse: a label-less function parsed to zero blocks and was SILENTLY SKIPPED — a planted violation vanished and the run exited 0. Both that and a label-shaped line the strict regex declines now raise `MalformedIR`, and the `; preds =` form LLVM itself prints is accepted rather than mis-parsed. The checker could not fail in three other ways, all fixed: it exited 0 on no arguments, on `--help`, on a typo'd flag and on a directory holding no `.ll`; it had no liveness assertion, so a clean verdict over zero root stores was indistinguishable from a clean one over the real corpus; and `PERRY_SAVE_LL` was never honoured for modules past `MIN_CALLABLES_TO_SPLIT`, so `--trace llvm` silently emitted nothing for exactly the largest modules. `main` now uses argparse, requires `--min-files` / `--min-binds`, and grows a `--self-test` that plants a violation and asserts the checker reports it. The split path writes one `.ll` per codegen unit. Wires the gate into a new `gc-root-dominance` workflow — deliberately NOT in branch protection's required contexts yet, since a gate that has never been green blocks every open PR the day it is promoted. CLAUDE.md gains the invariant itself under known-weak areas, including the third way it breaks that is still open: a heap value in a plain `alloca_entry` (`lower_call/new.rs`'s inline-ctor `this_slot`). Refs PerryTS#7154, PerryTS#7184, PerryTS#7192, PerryTS#7114, PerryTS#6951.
…tSpread reachability claim PerryTS#7192's fragment is restored to what merged, minus nothing: the corrections and the follow-up's own entries move into a separate PR-keyed fragment, per changelog.d/README.md. The two corrections it records: * Expr::ObjectSpread is reachable ONLY from a JSX spread attribute (crates/perry-hir/src/jsx.rs:67, its sole construction site). Since PerryTS#809 an object literal containing a spread lowers to a source-ordered IIFE built on js_object_assign_one, so { ...a, k: v } never reaches that arm. The claim that zod's 269-key spread runs through it is wrong. * "At least one more site of this class remains" is replaced by three named residuals with reproducers, two of them SIGSEGVs under PERRY_GC_MOVING_LOOP_POLLS=1, all reproduced at 73a9084 (inherited, not caused) and all clean under the shipped default.
proggeramlug
force-pushed
the
fix/7192-followup-dominance
branch
from
August 1, 2026 16:25
9a7d52f to
79db46d
Compare
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThe PR updates GC-sensitive code generation, adds split LLVM IR emission, hardens the dominance checker, and introduces a macOS workflow that builds, analyzes, and preserves failed IR corpora. ChangesGC root-store dominance
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant PerryCodegen
participant LLVMCorpus
participant DominanceChecker
GitHubActions->>PerryCodegen: build compiler and runtime archives
PerryCodegen->>LLVMCorpus: emit fixture LLVM modules
GitHubActions->>DominanceChecker: run self-test and corpus validation
DominanceChecker->>LLVMCorpus: parse root stores and allocation sites
DominanceChecker-->>GitHubActions: return validation status
GitHubActions->>LLVMCorpus: upload corpus when the job fails
Possibly related issues
Possibly related PRs
Suggested reviewers: ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Aug 1, 2026
Contributor
Author
|
The three residuals in "Why #7161 cannot be reverted yet" are now filed granularly, each with its reproducer and the layer the fix belongs at:
All three reproduce at Repo hygiene, separate from this PR: three consecutive commits on |
Contributor
Author
|
Post-merge gate results for this PR, measured on
Per-target codegen failures, identical on both trees: Zero delta on every gate. Two things found while running them, both worth their own follow-up:
Separately: the ratchet's pinned baseline expects a moving minor ( |
This was referenced Aug 1, 2026
jdalton
added a commit
to jdalton/perry
that referenced
this pull request
Aug 1, 2026
…eir sibling operands Two more sites of PerryTS#7192's root-store-dominance class, found by extending its checker with the general stale-register invariant and each 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. 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 PerryTS#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 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 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: the READ counterpart of PerryTS#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 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 (PerryTS#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: test_gap_gc_method_receiver_rooting.ts POLLS=1 parent: TypeError 5/5 this: bad 0 10/10 test_gap_gc_index_get_receiver_rooting.ts POLLS=1 parent: TypeError 4/4 this: bad 0 10/10 both, POLLS=1 + PERRY_GEN_GC=0 bad 0 both, default (no polls) bad 0 5/5 scripts/gc_root_dominance_check.py grows --stale-registers. 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 classifies 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), follows it forward through bit-level identity ops, and reports the first real use 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. Over the 141-module sfw-registry corpus the fatal-sink slice went 986 -> 729 with this change and 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. That mode is a diagnostic, so it EXITS 0 and reports counts. It is not calibrated to zero — the raw count is dominated by values the checker cannot prove are pointers, and even the --fatal-sinks slice still carries the js_closure_callN class above — so returning 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 via --max-stale N, which exits 1 above the budget, so a calibrated slice can become a ratchet later without the raw mode pretending to be one. --max-stale without --stale-registers is a usage error (exit 2) rather than a silently ignored budget. --self-test asserts all of it: the default must report 2 uses on the planted fixture and still exit 0, --max-stale 0 must exit 1, --max-stale 2 must exit 0, and the control fixture must report zero. The bind-anchored gate gc-root-dominance.yml actually runs is untouched and still exits non-zero on any violation. sfw-registry --help under PERRY_GC_MOVING_LOOP_POLLS=1 (compiled AND run with the flag) is still red — 8/10 SIGSEGV before these fixes surfaced past the TypeError, 5/10 after — so PerryTS#7161's stopgap stays. Its default arm is clean 10/10. cargo test -p perry-codegen: the 6 loop_safepoint_purity failures from PerryTS#7161's default flip, identical to main. cargo test -p perry-runtime: unchanged — this commit touches only perry-codegen, which perry-runtime does not depend on. Refs PerryTS#7154, PerryTS#7192, PerryTS#7198, PerryTS#7184, PerryTS#7161, PerryTS#7114, PerryTS#6951.
jdalton
added a commit
to jdalton/perry
that referenced
this pull request
Aug 1, 2026
…eir sibling operands Two more sites of PerryTS#7192's root-store-dominance class, found by extending its checker with the general stale-register invariant and each 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. 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 PerryTS#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 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 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: the READ counterpart of PerryTS#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 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 (PerryTS#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: test_gap_gc_method_receiver_rooting.ts POLLS=1 parent: TypeError 5/5 this: bad 0 10/10 test_gap_gc_index_get_receiver_rooting.ts POLLS=1 parent: TypeError 4/4 this: bad 0 10/10 both, POLLS=1 + PERRY_GEN_GC=0 bad 0 both, default (no polls) bad 0 5/5 scripts/gc_root_dominance_check.py grows --stale-registers. 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 classifies 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), follows it forward through bit-level identity ops, and reports the first real use 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. Over the 141-module sfw-registry corpus the fatal-sink slice went 986 -> 729 with this change and 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. That mode is a diagnostic, so it EXITS 0 and reports counts. It is not calibrated to zero — the raw count is dominated by values the checker cannot prove are pointers, and even the --fatal-sinks slice still carries the js_closure_callN class above — so returning 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 via --max-stale N, which exits 1 above the budget, so a calibrated slice can become a ratchet later without the raw mode pretending to be one. --max-stale or --fatal-sinks without --stale-registers is a usage error (exit 2) rather than a silently ignored budget or an ignored filter. --self-test asserts all of it: the default must report 2 uses on the planted fixture and still exit 0, --max-stale 0 must exit 1, --max-stale 2 must exit 0, and the control fixture must report zero. The bind-anchored gate gc-root-dominance.yml actually runs is untouched and still exits non-zero on any violation. sfw-registry --help under PERRY_GC_MOVING_LOOP_POLLS=1 (compiled AND run with the flag) is still red — 8/10 SIGSEGV before these fixes surfaced past the TypeError, 5/10 after — so PerryTS#7161's stopgap stays. Its default arm is clean 10/10. cargo test -p perry-codegen: the 6 loop_safepoint_purity failures from PerryTS#7161's default flip, identical to main. cargo test -p perry-runtime: unchanged — this commit touches only perry-codegen, which perry-runtime does not depend on. Refs PerryTS#7154, PerryTS#7192, PerryTS#7198, PerryTS#7184, PerryTS#7161, PerryTS#7114, PerryTS#6951.
This was referenced Aug 1, 2026
proggeramlug
pushed a commit
that referenced
this pull request
Aug 1, 2026
…eir sibling operands (#7206) * fix(codegen): root a call receiver and a computed-read base across their sibling operands Two more sites of #7192's root-store-dominance class, found by extending its checker with the general stale-register invariant and each 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. 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 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 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: 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 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: test_gap_gc_method_receiver_rooting.ts POLLS=1 parent: TypeError 5/5 this: bad 0 10/10 test_gap_gc_index_get_receiver_rooting.ts POLLS=1 parent: TypeError 4/4 this: bad 0 10/10 both, POLLS=1 + PERRY_GEN_GC=0 bad 0 both, default (no polls) bad 0 5/5 scripts/gc_root_dominance_check.py grows --stale-registers. 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 classifies 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), follows it forward through bit-level identity ops, and reports the first real use 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. Over the 141-module sfw-registry corpus the fatal-sink slice went 986 -> 729 with this change and 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. That mode is a diagnostic, so it EXITS 0 and reports counts. It is not calibrated to zero — the raw count is dominated by values the checker cannot prove are pointers, and even the --fatal-sinks slice still carries the js_closure_callN class above — so returning 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 via --max-stale N, which exits 1 above the budget, so a calibrated slice can become a ratchet later without the raw mode pretending to be one. --max-stale or --fatal-sinks without --stale-registers is a usage error (exit 2) rather than a silently ignored budget or an ignored filter. --self-test asserts all of it: the default must report 2 uses on the planted fixture and still exit 0, --max-stale 0 must exit 1, --max-stale 2 must exit 0, and the control fixture must report zero. The bind-anchored gate gc-root-dominance.yml actually runs is untouched and still exits non-zero on any violation. sfw-registry --help under PERRY_GC_MOVING_LOOP_POLLS=1 (compiled AND run with the flag) is still red — 8/10 SIGSEGV before these fixes surfaced past the TypeError, 5/10 after — so #7161's stopgap stays. Its default arm is clean 10/10. cargo test -p perry-codegen: the 6 loop_safepoint_purity failures from #7161's default flip, identical to main. cargo test -p perry-runtime: unchanged — this commit touches only perry-codegen, which perry-runtime does not depend on. Refs #7154, #7192, #7198, #7184, #7161, #7114, #6951. * fix(codegen): adopt #7207's reread_store_operand signature at the two #7206 sites Merge-time fix, not a change of intent. #7207 (`1679e22b4`, merged after this PR branched) widened `reread_store_operand` to take the operand expression and return `anyhow::Result<String>` so its new `Reload` arm can re-lower a string-literal base instead of reusing a register taken before the collection point. The two call sites added here still passed the old three-argument form. git merged both files cleanly because the change is in a DIFFERENT file (`temp_root.rs`) from the call sites (`index_get.rs`) -- a textually clean merge that does not compile. Adopting the new signature also gives these two sites #7207's strictly better treatment of a literal base (`"abc"[k]`). * test(gc): register #7206's two witnesses in the GC x repsel corpus Both are moving-only: clean on the shipped default on both sides of the fix, so they certify nothing on the `default` arm and belong with the `requires=move` rows. Measured red-then-green numbers are in the comment block. --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
proggeramlug
pushed a commit
to jdalton/perry
that referenced
this pull request
Aug 1, 2026
…e_callN
The generic dynamic-value-call lowering held THREE classes of GC value in bare
SSA registers across work that can collect. `js_closure_callN` is the central
dispatch path -- `f(g())`, `o.m(g())`, `curry(1)(2)`, every call whose callee is
a value rather than a statically resolved function -- and this is the site
An SSA register is not a GC root. Under PERRY_GC_MOVING_LOOP_POLLS=1 a back-edge
poll inside an argument runs an evacuating minor: each held value SURVIVES (the
capture cell, shadow slot or module global it was read from is a root) and
therefore MOVES. The collector rewrites that location; the register keeps naming
from-space.
* the CALLEE, held across the whole argument list. The checked unbox masks a
pre-move address and js_closure_callN reads a closure header out of
abandoned memory: "TypeError: value is not a function".
* the `this` RECEIVER, held across the read of the callee off it AND the
argument list. PerryTS#7206 fixed this operand on the sibling
js_native_call_method_by_id dispatch; this is the generic one.
* each already-lowered ARGUMENT, held across the arguments after it AND
across the rebind unbox.
The three live windows differ, so they are computed separately:
receiver | the callee read + every argument
callee | every argument
argument i | the arguments after i + the rebind unbox
That last window is why this is not a copy of PerryTS#7206's fix.
js_closure_unbox_callee_checked_rebind calls clone_closure_rebind_this, which
ALLOCATES a replacement closure (closure/dynamic_props.rs:1040) when the callee
captures `this`. It sits below the last argument and above js_closure_callN, so
the arguments are re-read below it -- hence RootedOperands::reread_one, which
re-reads one operand at a caller-chosen point instead of the whole group at one.
Hoisting the unbox above the argument list would remove the window instead, but
its throw is observable and the spec evaluates arguments before it. On the
>16-arity path the argument stores into the stack buffer moved below the unbox
for the same reason: a stack buffer is not a root, so filling it above an
allocating rebind freezes pre-move addresses one indirection further out.
Receiverless calls take js_closure_unbox_callee_checked, a tag check and a mask
that allocates nothing, so `f(x, y)` on inert operands emits exactly its old IR.
Temp roots, not re-lowering: re-lowering the callee or receiver would observe an
assignment made by an argument, a miscompile rather than a rooting fix.
Three gap tests, one per held value, each red on the parent under a GENUINE
POLLS=1 build and green after. The flag is compile-time since PerryTS#7161 AND
runtime-armed (gc_moving_loop_polls_enabled(), gc/policy.rs:1759) -- setting
only one is a false green, and the first cut of these tests passed 10/10 for
exactly that reason.
callee / this / argument, POLLS=1 parent: TypeError 10/10 each
this: bad 0 10/10 each
all three, POLLS=1 + PERRY_GEN_GC=0 bad 0 5/5
all three, default (no polls) bad 0 5/5
Cost over the 141-module sfw-registry corpus, measured rather than assumed
because this is the hottest emitted call path. operand_protection emits nothing
for an operand whose window cannot collect, which is why the delta is small:
linked binary 39,216,688 -> 39,233,200 B (+0.042%), emitted IR 1,999,570 ->
2,001,607 lines (+0.10%), js_gc_temp_root_push sites 8,394 -> 8,885.
scripts/gc_root_dominance_check.py gains js_closure_unbox_callee_checked in
NONCOLLECTING, citing closure/unbox.rs:25 -- a tag check and a low-48 mask, no
allocation, no user code, no poll. It sits between every dynamic call's last
argument and its js_closure_callN, so its absence reported the whole argument
list of every 1-arg dynamic call as stale: 372 of the 729 fatal-sink hits were
that single false positive, all marked MOVING: no. The _rebind variant is
deliberately NOT added -- it allocates, and the fix above depends on it counting
as a collection point. Fatal-sink slice against the corrected list: 231 -> 205.
cargo test -p perry-codegen: failing set IDENTICAL to the parent (6
loop_safepoint_purity, 16 native_proof_regressions, 3 native_proof_buffer_views,
1 shadow_slot_hygiene, 1 typed_shape_descriptors), measured on the parent commit
rather than assumed; one lib unit test red on the parent passes here. The
bind-anchored gate reports the same single non-moving residual PerryTS#7192 left.
WHAT THIS DOES NOT CLOSE: sfw-registry --help under a genuine POLLS=1 build is
still red -- 3/10 pass, 7/10 SIGSEGV -- so PerryTS#7161's stopgap STAYS. Its default arm
is clean 10/10, so nothing was traded away. Two concrete leads are written up in
the changelog fragment: `prev_this` in this same lowering is the same bug
unfixed (js_implicit_this_set returns a value read from the scanned, rewritten
IMPLICIT_THIS cell and holds it across the entire user call), and the remaining
205 fatal sinks are no longer dominated by one class, with the spread dispatch
(expr/call_spread.rs) the obvious next site.
Refs PerryTS#7154, PerryTS#7206, PerryTS#7192, PerryTS#7198, PerryTS#7184, PerryTS#7161, PerryTS#7114, PerryTS#6951, PerryTS#519.
proggeramlug
pushed a commit
that referenced
this pull request
Aug 1, 2026
…e_callN (#7214) * fix(codegen): root the callee, `this` and every argument of js_closure_callN The generic dynamic-value-call lowering held THREE classes of GC value in bare SSA registers across work that can collect. `js_closure_callN` is the central dispatch path -- `f(g())`, `o.m(g())`, `curry(1)(2)`, every call whose callee is a value rather than a statically resolved function -- and this is the site An SSA register is not a GC root. Under PERRY_GC_MOVING_LOOP_POLLS=1 a back-edge poll inside an argument runs an evacuating minor: each held value SURVIVES (the capture cell, shadow slot or module global it was read from is a root) and therefore MOVES. The collector rewrites that location; the register keeps naming from-space. * the CALLEE, held across the whole argument list. The checked unbox masks a pre-move address and js_closure_callN reads a closure header out of abandoned memory: "TypeError: value is not a function". * the `this` RECEIVER, held across the read of the callee off it AND the argument list. #7206 fixed this operand on the sibling js_native_call_method_by_id dispatch; this is the generic one. * each already-lowered ARGUMENT, held across the arguments after it AND across the rebind unbox. The three live windows differ, so they are computed separately: receiver | the callee read + every argument callee | every argument argument i | the arguments after i + the rebind unbox That last window is why this is not a copy of #7206's fix. js_closure_unbox_callee_checked_rebind calls clone_closure_rebind_this, which ALLOCATES a replacement closure (closure/dynamic_props.rs:1040) when the callee captures `this`. It sits below the last argument and above js_closure_callN, so the arguments are re-read below it -- hence RootedOperands::reread_one, which re-reads one operand at a caller-chosen point instead of the whole group at one. Hoisting the unbox above the argument list would remove the window instead, but its throw is observable and the spec evaluates arguments before it. On the >16-arity path the argument stores into the stack buffer moved below the unbox for the same reason: a stack buffer is not a root, so filling it above an allocating rebind freezes pre-move addresses one indirection further out. Receiverless calls take js_closure_unbox_callee_checked, a tag check and a mask that allocates nothing, so `f(x, y)` on inert operands emits exactly its old IR. Temp roots, not re-lowering: re-lowering the callee or receiver would observe an assignment made by an argument, a miscompile rather than a rooting fix. Three gap tests, one per held value, each red on the parent under a GENUINE POLLS=1 build and green after. The flag is compile-time since #7161 AND runtime-armed (gc_moving_loop_polls_enabled(), gc/policy.rs:1759) -- setting only one is a false green, and the first cut of these tests passed 10/10 for exactly that reason. callee / this / argument, POLLS=1 parent: TypeError 10/10 each this: bad 0 10/10 each all three, POLLS=1 + PERRY_GEN_GC=0 bad 0 5/5 all three, default (no polls) bad 0 5/5 Cost over the 141-module sfw-registry corpus, measured rather than assumed because this is the hottest emitted call path. operand_protection emits nothing for an operand whose window cannot collect, which is why the delta is small: linked binary 39,216,688 -> 39,233,200 B (+0.042%), emitted IR 1,999,570 -> 2,001,607 lines (+0.10%), js_gc_temp_root_push sites 8,394 -> 8,885. scripts/gc_root_dominance_check.py gains js_closure_unbox_callee_checked in NONCOLLECTING, citing closure/unbox.rs:25 -- a tag check and a low-48 mask, no allocation, no user code, no poll. It sits between every dynamic call's last argument and its js_closure_callN, so its absence reported the whole argument list of every 1-arg dynamic call as stale: 372 of the 729 fatal-sink hits were that single false positive, all marked MOVING: no. The _rebind variant is deliberately NOT added -- it allocates, and the fix above depends on it counting as a collection point. Fatal-sink slice against the corrected list: 231 -> 205. cargo test -p perry-codegen: failing set IDENTICAL to the parent (6 loop_safepoint_purity, 16 native_proof_regressions, 3 native_proof_buffer_views, 1 shadow_slot_hygiene, 1 typed_shape_descriptors), measured on the parent commit rather than assumed; one lib unit test red on the parent passes here. The bind-anchored gate reports the same single non-moving residual #7192 left. WHAT THIS DOES NOT CLOSE: sfw-registry --help under a genuine POLLS=1 build is still red -- 3/10 pass, 7/10 SIGSEGV -- so #7161's stopgap STAYS. Its default arm is clean 10/10, so nothing was traded away. Two concrete leads are written up in the changelog fragment: `prev_this` in this same lowering is the same bug unfixed (js_implicit_this_set returns a value read from the scanned, rewritten IMPLICIT_THIS cell and holds it across the entire user call), and the remaining 205 fatal sinks are no longer dominated by one class, with the spread dispatch (expr/call_spread.rs) the obvious next site. Refs #7154, #7206, #7192, #7198, #7184, #7161, #7114, #6951, #519. * chore(7214): merge-time fixes — fragment name, rustfmt, corpus registration - changelog fragment was PR-misnumbered `7207-`; #7207 is a different, already merged change. Renamed to `7214-`. Content already referenced #7206 correctly and is unchanged. - `cargo fmt --all -- --check` is a required check on `lint`; one hand-wrapped `roots.push` call needed re-wrapping. No behaviour change. - Registered the three witnesses in the GC x repsel corpus, next to #7206's pair. All three are moving-only: clean on the shipped default on both sides of the fix, so they belong with the `requires=move` rows and prove nothing on `default`. --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
proggeramlug
pushed a commit
that referenced
this pull request
Aug 1, 2026
…ly, and document the invariant (#7212) * fix(codegen): root the callee, `this` and every argument of js_closure_callN The generic dynamic-value-call lowering held THREE classes of GC value in bare SSA registers across work that can collect. `js_closure_callN` is the central dispatch path -- `f(g())`, `o.m(g())`, `curry(1)(2)`, every call whose callee is a value rather than a statically resolved function -- and this is the site An SSA register is not a GC root. Under PERRY_GC_MOVING_LOOP_POLLS=1 a back-edge poll inside an argument runs an evacuating minor: each held value SURVIVES (the capture cell, shadow slot or module global it was read from is a root) and therefore MOVES. The collector rewrites that location; the register keeps naming from-space. * the CALLEE, held across the whole argument list. The checked unbox masks a pre-move address and js_closure_callN reads a closure header out of abandoned memory: "TypeError: value is not a function". * the `this` RECEIVER, held across the read of the callee off it AND the argument list. #7206 fixed this operand on the sibling js_native_call_method_by_id dispatch; this is the generic one. * each already-lowered ARGUMENT, held across the arguments after it AND across the rebind unbox. The three live windows differ, so they are computed separately: receiver | the callee read + every argument callee | every argument argument i | the arguments after i + the rebind unbox That last window is why this is not a copy of #7206's fix. js_closure_unbox_callee_checked_rebind calls clone_closure_rebind_this, which ALLOCATES a replacement closure (closure/dynamic_props.rs:1040) when the callee captures `this`. It sits below the last argument and above js_closure_callN, so the arguments are re-read below it -- hence RootedOperands::reread_one, which re-reads one operand at a caller-chosen point instead of the whole group at one. Hoisting the unbox above the argument list would remove the window instead, but its throw is observable and the spec evaluates arguments before it. On the >16-arity path the argument stores into the stack buffer moved below the unbox for the same reason: a stack buffer is not a root, so filling it above an allocating rebind freezes pre-move addresses one indirection further out. Receiverless calls take js_closure_unbox_callee_checked, a tag check and a mask that allocates nothing, so `f(x, y)` on inert operands emits exactly its old IR. Temp roots, not re-lowering: re-lowering the callee or receiver would observe an assignment made by an argument, a miscompile rather than a rooting fix. Three gap tests, one per held value, each red on the parent under a GENUINE POLLS=1 build and green after. The flag is compile-time since #7161 AND runtime-armed (gc_moving_loop_polls_enabled(), gc/policy.rs:1759) -- setting only one is a false green, and the first cut of these tests passed 10/10 for exactly that reason. callee / this / argument, POLLS=1 parent: TypeError 10/10 each this: bad 0 10/10 each all three, POLLS=1 + PERRY_GEN_GC=0 bad 0 5/5 all three, default (no polls) bad 0 5/5 Cost over the 141-module sfw-registry corpus, measured rather than assumed because this is the hottest emitted call path. operand_protection emits nothing for an operand whose window cannot collect, which is why the delta is small: linked binary 39,216,688 -> 39,233,200 B (+0.042%), emitted IR 1,999,570 -> 2,001,607 lines (+0.10%), js_gc_temp_root_push sites 8,394 -> 8,885. scripts/gc_root_dominance_check.py gains js_closure_unbox_callee_checked in NONCOLLECTING, citing closure/unbox.rs:25 -- a tag check and a low-48 mask, no allocation, no user code, no poll. It sits between every dynamic call's last argument and its js_closure_callN, so its absence reported the whole argument list of every 1-arg dynamic call as stale: 372 of the 729 fatal-sink hits were that single false positive, all marked MOVING: no. The _rebind variant is deliberately NOT added -- it allocates, and the fix above depends on it counting as a collection point. Fatal-sink slice against the corrected list: 231 -> 205. cargo test -p perry-codegen: failing set IDENTICAL to the parent (6 loop_safepoint_purity, 16 native_proof_regressions, 3 native_proof_buffer_views, 1 shadow_slot_hygiene, 1 typed_shape_descriptors), measured on the parent commit rather than assumed; one lib unit test red on the parent passes here. The bind-anchored gate reports the same single non-moving residual #7192 left. WHAT THIS DOES NOT CLOSE: sfw-registry --help under a genuine POLLS=1 build is still red -- 3/10 pass, 7/10 SIGSEGV -- so #7161's stopgap STAYS. Its default arm is clean 10/10, so nothing was traded away. Two concrete leads are written up in the changelog fragment: `prev_this` in this same lowering is the same bug unfixed (js_implicit_this_set returns a value read from the scanned, rewritten IMPLICIT_THIS cell and holds it across the entire user call), and the remaining 205 fatal sinks are no longer dominated by one class, with the spread dispatch (expr/call_spread.rs) the obvious next site. Refs #7154, #7206, #7192, #7198, #7184, #7161, #7114, #6951, #519. * chore(7214): merge-time fixes — fragment name, rustfmt, corpus registration - changelog fragment was PR-misnumbered `7207-`; #7207 is a different, already merged change. Renamed to `7214-`. Content already referenced #7206 correctly and is unchanged. - `cargo fmt --all -- --check` is a required check on `lint`; one hand-wrapped `roots.push` call needed re-wrapping. No behaviour change. - Registered the three witnesses in the GC x repsel corpus, next to #7206's pair. All three are moving-only: clean on the shipped default on both sides of the fix, so they belong with the `requires=move` rows and prove nothing on `default`. * ci(gc): make the root-dominance gate able to fail, baseline it honestly, and document the invariant Squashed. See PR description for the seeded-violation proof and the allowlist rationale. * docs(gc): correct the post-#7207 staleness in the rooting-invariant writeup Merge-time corrections to statements #7207 invalidated while this PR was in review: - CLAUDE.md called `lower_call/new.rs`'s inline-ctor `this_slot` "still open". #7207 closed it. Point at `--unrooted-allocas` as the detector for that shape and name #7210 as where its remaining hits are tracked. - The rooting-invariant doc documented `--stale-registers` but not `--unrooted-allocas`, so the one mode the bind-anchored check is structurally blind to had no entry. Add it, and state plainly that the gate does NOT run it and that its hits are deliberately outside the allowlist — the allowlist covers the bind-anchored shape only. - "all five known shapes" -> "every known shape", so the pointer cannot go stale the next time one is found. --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Post-merge follow-up to #7192, from its review round. Every finding was taken to a reproducer or to code before it was accepted or declined.
The four review findings
lower_call/new.rs:1870—ctor_result_slotlogical_collections.rs:927—protect_handleindex_set.rs:1494— the key operandgc_root_dominance_check.py:135—%entry.implicitgc_root_dominance_check.py:655— empty input exits 01. The inline-constructor result slot — the load-bearing one
ctor_result_slotis a plainalloca_entry: not a shadow slot, not a temp root, never rewritten by the collector. Seeding it withobj_boxparked the pre-constructor instance address in unrooted memory for the whole body. On fall-through,js_ctor_return_overridesaw an object inrawand returned that — discarding the instancereload_instancehad just re-read.Two things make it worse than it looks:
PERRY_INLINE_CTOR.force_ctor_callrequiresclass.constructor.is_some(), so any class with fields or heritage but no own constructor —class C { payload = mk() },class C extends B {}— takes the inline path by default.the_new_instance_is_rooted_across_the_constructor_bodywalks the first operand of the override; that is the one that was re-read. The value actually published on fall-through is the second.The slot now starts at
undefined, which is exactly equivalent on all four paths and carries no address:raw= staleobj_box, an object ⇒ returns the stale addressraw= undefined ⇒ returns the re-read instancereturn;return <expr>Stmt::Returnoverwrote the slotIt also removes a latent spurious
TypeError: a derived class whose instance's GC type is outsideconstructor_return_overrides_this's set previously fell through to "Derived constructors may only return object or undefined" on a plain fall-through. New testthe_inline_ctor_result_slot_never_carries_an_instance_addressasserts every store into the slot is a constant.2. The computed key of a store
o[k] = f()and a class expression's[sym]: initlower the key before the value, leaving it in exactly the window #7192 closed for the receiver. A non-literal string key is an ordinary heap string with no registered root of its own, sounbox_str_handlebelow the call would hand the setter a pre-moveStringHeader*. Rooted with the same guard, pushed after the receiver's and released before it so thetemp_root_truncatecuts nest.ReceiverGuard/guard_store_receiver→StoreOperandGuard/guard_store_operand, because rooting a key through something called "receiver" is the naming drift that let #7114's two predicates diverge.No runtime reproducer: the stale address stays self-consistent until the abandoned memory is reused, which is #7154's fingerprint. This rests on the static argument, and I am saying so rather than implying otherwise.
3. The protection predicates — half accepted
Declined: "
js_object_set_field_by_namecan transition the shape, therefore force the root." An allocation inside a runtime helper provably cannot initiate a moving collection.gc_check_trigger()'s minor arm defers to the loop safepoint under polls (GC_SAFEPOINT_PENDING), falls back to a conservative-scanned non-moving minor with polls off, and reaches a budgetedMutatorAssistwithevacuation_policy_allowed = falseon the shipped default; C4b skips non-tenured nursery objects outright. Forking these predicates away fromExpr::Object's on an unsound reason would recreate the two-copies-of-one-decision shape that produced #7114.Accepted: user-code re-entry inside the helper is the real route. A spread part forces protection on its own (
js_object_copy_own_fieldsreads every own key, so an accessor there runs arbitrary JS), and so does astatic { … }block — which is whyblock_fnsmoves aboverooted_handle_begin. Verified in the emitted IR: a class expression with one inert named static, one static block, no captures and no symbol statics answeredfalseto every term of the old predicate and emitted no instance root; it now pushes one and re-reads it at every use.4 & 5. The checker was a gate that could not fail
Audited against all four ways (CLAUDE.md). It could not fail in four of them:
--help, on a typo'd flag, and on a directory with no.ll— and the corpus is generated, so an empty trace directory is a routine outcome of a failed compile;violations: 0, exit 0;Now: argparse,
--min-files,--min-bindswithroot stores: Nin the summary,MalformedIRon both unparseable shapes, LLVM's own; preds =label form accepted rather than mis-parsed into the previous block, and--self-testthat plants same-block and cross-block violations and asserts both are reported while an otherwise-identical control clears.The
%entry.implicitsynthetic block does not exist —grep -n implicithas one hit and it isjs_implicit_this_setinNONCOLLECTING. Numeric labels already parse (\wincludes digits, verified). Perry cannot emit either shape:LlFunction::to_irwrites a label before every block andcreate_blockformats every label as"{name}.{counter}"— confirmed over 214 real functions / 3608 labels, 0 pre-label instructions, 0 numeric labels.One more hole in the same family:
PERRY_SAVE_LLwas never honoured for split modules.n_unitsis forced to 1 only foremit_ir_only, and then_units > 1pathreturns before the write — so--trace llvmemitted nothing for every module pastMIN_CALLABLES_TO_SPLIT, i.e. exactly the largest modules, which is where a static IR audit most needs to look. The comment above it claimed the opposite. The split path now writes one.llper unit.New workflow
gc-root-dominance:--self-testfirst (fast structural failure before a compiler build), then a real corpus underPERRY_GC_MOVING_LOOP_POLLS=1 PERRY_INLINE_SHADOW_SLOT=0with both liveness floors. Nocontinue-on-error, no pipe between the checker and the exit status,cancel-in-progressscoped topull_requestso a busymainqueue cannot starve it. Deliberately not a required context — a gate that has never been green blocks every open PR the day it is promoted. Promote after a clean week onmain.Correction to #7192's fragment
Expr::ObjectSpreadis JSX-only. Since #809 an object literal containing a spread lowers to a source-ordered IIFE built onjs_object_assign_one(lower/expr_object.rs:844);Expr::ObjectSpread's sole construction site iscrates/perry-hir/src/jsx.rs:67. The zod-269-key-spread claim is wrong. The fix is still right; its blast radius is JSX.Residuals — why #7161 cannot be reverted yet
All three reproduce at
73a9084ea(before #7184 and #7192), so they are inherited rather than caused, and all three are clean under the shipped default:alloca_entryslots the collector never rewrites — the inline-ctorthis_slot, and the[N x i64]closure-capture staging array. Fixingthis_slotmeans routingExpr::Thisthrough a root rather than an alloca.{ ...src, k: v }with an accessor source — SIGSEGV (exit=139) underPERRY_GC_MOVING_LOOP_POLLS=1, on thejs_object_assign_oneIIFE path. A lighter variant survives but silently drops the copied value 10 times in 400.static { … }block — SIGSEGV (exit=139) under polls. The class object is now correctly rooted; what is still stale is the valuejs_static_this_arm_valueparks in the runtime's static-thiscell.Summary by CodeRabbit
Bug Fixes
Quality & Validation
Documentation