fix(codegen): root the callee, this and every argument of js_closure_callN - #7214
Conversation
📝 WalkthroughWalkthroughDynamic closure-call lowering now roots and rereads receivers, callees, and arguments across allocation windows. It adds per-operand rereading, updates root-dominance classification, and adds moving-GC regression coverage and corpus entries. ChangesMoving-GC rooting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ClosureCallLowering
participant RootedOperands
participant MovingGC
participant RuntimeDispatch
ClosureCallLowering->>RootedOperands: root receiver, callee, and arguments
ClosureCallLowering->>MovingGC: evaluate allocating arguments and rebind receiver
MovingGC-->>RootedOperands: relocate rooted values
ClosureCallLowering->>RootedOperands: reread receiver, callee, and arguments
ClosureCallLowering->>RuntimeDispatch: dispatch closure call
RuntimeDispatch-->>ClosureCallLowering: return result
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@changelog.d/7207-closure-calln-stale-registers.md`:
- Around line 116-117: Escape the leading hash in the wrapped changelog sentence
beginning with “#7161” so markdownlint MD018 no longer interprets it as a
malformed heading, while preserving the rendered text and surrounding
formatting.
In `@crates/perry-codegen/src/expr/index_get.rs`:
- Around line 2030-2037: Update the dynamic-string-key path in the index-get
lowering to root the receiver and key material across key materialization and
any allocation points. After unbox_str_handle and receiver-root rereads,
re-derive the key and receiver handles from their current rooted values before
calling js_typed_feedback_object_get_field_by_name_f64; do not pass stale bare
obj_handle or key_handle values after the root is released.
In `@crates/perry-codegen/src/lower_call/console_promise.rs`:
- Around line 1526-1529: Root the value returned by js_implicit_this_set in a
TempRoot slot before the allocating rebind unbox and user-code dispatch, then
reread that rooted value immediately before the restore at the dispatch cleanup.
Release the prev_this root after roots so nested temporary roots are released
inner-to-outer, and use the existing valid temporary-root APIs rather than an
untracked alloca.
In `@scripts/gc_root_dominance_check.py`:
- Around line 1091-1099: Move the n_binds computation and --min-binds validation
in the main argument-handling flow before dispatching to run_stale or the
bind-anchored path. Ensure both --stale-registers and the default check reject
corpora with fewer than ns.min_binds root stores, while preserving each path’s
existing behavior after the shared guard.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 87d369df-fa8e-4fda-b3dc-a609be0d12cc
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
CLAUDE.mdCargo.tomlchangelog.d/7200-stale-receiver-registers.mdchangelog.d/7207-closure-calln-stale-registers.mdcrates/perry-codegen/src/expr/index_get.rscrates/perry-codegen/src/expr/temp_root.rscrates/perry-codegen/src/lower_call/console_promise.rsscripts/gc_root_dominance_check.pytest-files/test_gap_gc_closure_call_argument_rooting.tstest-files/test_gap_gc_closure_call_callee_rooting.tstest-files/test_gap_gc_closure_call_this_rooting.tstest-files/test_gap_gc_index_get_receiver_rooting.tstest-files/test_gap_gc_method_receiver_rooting.ts
| **`sfw-registry --help` under a genuine `POLLS=1` build is still red, so | ||
| #7161's stopgap stays.** Measured on this build, compiled *and* run with the |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Escape the leading # to avoid an MD018 markdownlint hit.
Line 117 starts with #7161's stopgap stays.** after the wrap. markdownlint's MD018 rule treats a line-leading # without a following space as a malformed ATX heading. Escape it so the sentence still reads correctly without triggering the lint rule.
📝 Proposed fix to escape the leading hash
**`sfw-registry --help` under a genuine `POLLS=1` build is still red, so
-#7161's stopgap stays.** Measured on this build, compiled *and* run with the
+\`#7161`'s stopgap stays.** Measured on this build, compiled *and* run with the📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| **`sfw-registry --help` under a genuine `POLLS=1` build is still red, so | |
| #7161's stopgap stays.** Measured on this build, compiled *and* run with the | |
| **`sfw-registry --help` under a genuine `POLLS=1` build is still red, so | |
| \`#7161`'s stopgap stays.** Measured on this build, compiled *and* run with the |
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 117-117: No space after hash on atx style heading
(MD018, no-missing-space-atx)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@changelog.d/7207-closure-calln-stale-registers.md` around lines 116 - 117,
Escape the leading hash in the wrapped changelog sentence beginning with “#7161”
so markdownlint MD018 no longer interprets it as a malformed heading, while
preserving the rendered text and surrounding formatting.
Source: Linters/SAST tools
| 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)") | ||
| 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") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
--stale-registers bypasses the --min-binds safety check.
run_stale is dispatched at Line 1140 before n_binds is computed (Line 1143) and before the --min-binds guard (Line 1181). The bind-anchored path validates that at least ns.min_binds root stores exist in the corpus, because a corpus with zero root stores means the IR was not compiled the way the checker assumes (for example PERRY_INLINE_SHADOW_SLOT=0), and a clean verdict over such a corpus proves nothing.
The stale-register path relies on the same BIND_RE-derived slot_of_alloca map for slotload classification, so it has the identical failure mode: a misconfigured build produces zero heap-value sources, run_stale reports total == 0, and the script exits 0 — a false-clean CI result for the exact invariant this PR depends on to catch GC rooting regressions.
Compute n_binds and apply the --min-binds guard before dispatching to either code path.
🐛 Proposed fix to run the min-binds guard before either code path
parsed = []
for p in sorted(paths):
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)
n_binds = sum(
1
for _m, fs in parsed
for f in fs
for b in f.blocks
for ins in f.insns[b]
if BIND_RE.search(ins.text)
)
+ if n_binds < ns.min_binds:
+ print(f"error: {n_binds} root store(s) in the corpus, need at least "
+ f"{ns.min_binds}. A clean verdict here means the IR was not the "
+ "IR you think it is (compile with PERRY_INLINE_SHADOW_SLOT=0).",
+ file=sys.stderr)
+ return 2
+ if ns.stale_registers:
+ return run_stale(parsed, poll_reaching, verbose, moving_only,
+ ns.fatal_sinks)Also applies to: 1135-1150, 1181-1186
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/gc_root_dominance_check.py` around lines 1091 - 1099, Move the
n_binds computation and --min-binds validation in the main argument-handling
flow before dispatching to run_stale or the bind-anchored path. Ensure both
--stale-registers and the default check reject corpora with fewer than
ns.min_binds root stores, while preserving each path’s existing behavior after
the shared guard.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/gc_root_dominance_check.py (1)
1493-1500: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
--stale-registersstill bypasses the--min-bindssafety check.Line 1498 dispatches
run_stalebeforen_bindsis computed at Line 1501 and before the--min-bindsguard at Line 1584. This is the same gap raised on a previous commit: the bind-anchored path validates that at leastns.min_bindsroot stores exist in the corpus, because a corpus with zero root stores means the IR was not compiled the way the checker assumes. The stale-register path has the identical exposure, but nothing enforces the floor beforerun_stalereturns.A related, second gap in the same dispatch block: if a user passes both
--stale-registersand--unrooted-allocas,--stale-registerssilently wins at Line 1498 and the--unrooted-allocasbranch at Line 1510 never executes, with no error. Compare this to the explicitap.errorguards already added for--max-stale/--fatal-sinksat Lines 1460-1463, which exist specifically because "a knob that is silently ignored is a disarmed knob."Compute
n_bindsand apply the--min-bindsguard before dispatching torun_stale, and reject the--stale-registers/--unrooted-allocascombination explicitly.🐛 Proposed fix to gate `--stale-registers` on `--min-binds`
parsed = [] for p in sorted(paths): 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 for f in fs for b in f.blocks for ins in f.insns[b] if BIND_RE.search(ins.text) ) + if ns.stale_registers: + if n_binds < ns.min_binds: + print(f"error: {n_binds} root store(s) in the corpus, need at " + f"least {ns.min_binds}. A clean verdict here means the IR " + "was not the IR you think it is (compile with " + "PERRY_INLINE_SHADOW_SLOT=0).", file=sys.stderr) + return 2 + return run_stale(parsed, poll_reaching, verbose, moving_only, + ns.fatal_sinks, ns.max_stale)Also applies to: 1584-1589
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/gc_root_dominance_check.py` around lines 1493 - 1500, Update the dispatch logic around run_stale to compute n_binds and enforce the existing ns.min_binds safety check before returning from the --stale-registers path. Add an explicit argument error rejecting simultaneous --stale-registers and --unrooted-allocas, so neither mode silently overrides the other; preserve the existing validation behavior for the remaining options.
♻️ Duplicate comments (1)
changelog.d/7214-closure-calln-stale-registers.md (1)
116-117: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winEscape the leading
#to avoid the MD018 markdownlint hit.Line 117 still starts with
#7161's stopgap stays.**after the wrap. markdownlint's MD018 rule treats a line-leading#without a following space as a malformed ATX heading, and the static analysis hint confirms this fires again at Line 117 in this commit. This is the same issue raised on a previous commit of this fragment (then named7207-closure-calln-stale-registers.md) and it remains unresolved.📝 Proposed fix to escape the leading hash
**`sfw-registry --help` under a genuine `POLLS=1` build is still red, so -#7161's stopgap stays.** Measured on this build, compiled *and* run with the +`#7161`'s stopgap stays.** Measured on this build, compiled *and* run with the🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@changelog.d/7214-closure-calln-stale-registers.md` around lines 116 - 117, Escape the leading hash in the wrapped changelog sentence so the line containing “#7161's stopgap stays.” no longer begins with a raw # character; preserve the existing wording and Markdown formatting.Source: Linters/SAST tools
🧹 Nitpick comments (1)
scripts/gc_root_dominance_check.py (1)
1239-1247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated GC-capable-alloca counting loop.
The loop that counts GC-capable allocas at Lines 1516-1523 (inside the CLI
--unrooted-allocasbranch) duplicates the same loop already in_scan_unrootedat Lines 1239-1247. Extract a small helper, e.g._count_gc_capable_allocas(parsed), and call it from both sites.♻️ Proposed extraction
+def _count_gc_capable_allocas(parsed): + n = 0 + for _m, fs in parsed: + for f in fs: + for b in f.blocks: + for ins in f.insns[b]: + am = ALLOCA_RE.match(ins.text) + if am and any(am.group(2).strip().startswith(t) + for t in GC_CAPABLE_ALLOCA_TYPES): + n += 1 + return n + + def _scan_unrooted(paths): """(violations, n_gc_capable_allocas) over `paths`.""" parsed = [(os.path.basename(p), parse_file(p)) for p in sorted(paths)] poll_reaching, _known = compute_poll_reaching( [f for _m, fs in parsed for f in fs]) - n = 0 - for _m, fs in parsed: - for f in fs: - for b in f.blocks: - for ins in f.insns[b]: - am = ALLOCA_RE.match(ins.text) - if am and any(am.group(2).strip().startswith(t) - for t in GC_CAPABLE_ALLOCA_TYPES): - n += 1 + n = _count_gc_capable_allocas(parsed)Also applies to: 1510-1523
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/gc_root_dominance_check.py` around lines 1239 - 1247, Extract the duplicated GC-capable-alloca counting logic into a helper such as _count_gc_capable_allocas(parsed), preserving the existing traversal and ALLOCA_RE/GC_CAPABLE_ALLOCA_TYPES checks. Replace the inline loop in _scan_unrooted and the corresponding CLI --unrooted-allocas branch with calls to this helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/gc_root_dominance_check.py`:
- Around line 1493-1500: Update the dispatch logic around run_stale to compute
n_binds and enforce the existing ns.min_binds safety check before returning from
the --stale-registers path. Add an explicit argument error rejecting
simultaneous --stale-registers and --unrooted-allocas, so neither mode silently
overrides the other; preserve the existing validation behavior for the remaining
options.
---
Duplicate comments:
In `@changelog.d/7214-closure-calln-stale-registers.md`:
- Around line 116-117: Escape the leading hash in the wrapped changelog sentence
so the line containing “#7161's stopgap stays.” no longer begins with a raw #
character; preserve the existing wording and Markdown formatting.
---
Nitpick comments:
In `@scripts/gc_root_dominance_check.py`:
- Around line 1239-1247: Extract the duplicated GC-capable-alloca counting logic
into a helper such as _count_gc_capable_allocas(parsed), preserving the existing
traversal and ALLOCA_RE/GC_CAPABLE_ALLOCA_TYPES checks. Replace the inline loop
in _scan_unrooted and the corresponding CLI --unrooted-allocas branch with calls
to this helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a2ac677-634d-4bdf-8fac-8eb19d081af2
📒 Files selected for processing (6)
changelog.d/7206-stale-receiver-registers.mdchangelog.d/7214-closure-calln-stale-registers.mdcrates/perry-codegen/src/expr/index_get.rscrates/perry-codegen/src/expr/temp_root.rscrates/perry-codegen/src/lower_call/console_promise.rsscripts/gc_root_dominance_check.py
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/perry-codegen/src/expr/index_get.rs
- crates/perry-codegen/src/expr/temp_root.rs
- crates/perry-codegen/src/lower_call/console_promise.rs
…corpus Moving-only, like PerryTS#7206's pair: clean on the shipped default on both sides of the fix. The comment block records the three distinct rooting windows and the red-then-green measured against a PerryTS#7206-only build, which is what makes these 'the sites PerryTS#7206 left open' rather than a restatement of it.
…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.
…ration - changelog fragment was PR-misnumbered `7207-`; PerryTS#7207 is a different, already merged change. Renamed to `7214-`. Content already referenced PerryTS#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 PerryTS#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`.
8d2a264 to
e0fce62
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/perry-codegen/src/lower_call/console_promise.rs (1)
1416-1430: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
prev_thisis still an unrooted GC sink.
js_implicit_this_setreturnsprev_thisat Line 1418 or Line 1427. That value stays in a bare SSA register across the allocating rebind unbox at Lines 1461-1475 and across the whole dispatch at Lines 1486-1519. Neither location is a GC root. The restore at Lines 1525-1529 can write a pre-move address back into the implicit-thiscell after a moving collection.Push a temp root right after
js_implicit_this_setproducesprev_this. Re-read it just before the restore. Release that root afterrootsso nested releases run inner to outer.As per coding guidelines: "Ensure GC-managed values are rooted before every possible collection point and that the root store dominates all such sites; use shadow slots or temporary roots rather than untracked allocas."
Also applies to: 1525-1529
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-codegen/src/lower_call/console_promise.rs` around lines 1416 - 1430, Root the value returned by js_implicit_this_set immediately after each prev_this assignment, including both method_recv and receiverless-call branches. Re-read the rooted value immediately before the implicit-this restore around the dispatch cleanup, then release this temporary root after releasing roots so nested roots unwind inner-to-outer; do not use the stale SSA value across the allocating rebind or dispatch.Source: Coding guidelines
scripts/gc_root_dominance_check.py (1)
1498-1500: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
--stale-registersstill bypasses the--min-bindssafety check.
run_staleis dispatched here beforen_bindsis computed (Line 1501) and before the--min-bindsguard (Line 1584).heap_source_kind's "slotload" classification depends onslot_of_alloca, which is derived from the sameBIND_REmatches used to computen_binds. A corpus with zero root stores (a misconfigured build, e.g.PERRY_INLINE_SHADOW_SLOT=1) makescheck_func_stalefind fewer or no violations, so--stale-registers --max-stale Ncan report a false-clean result over IR that was never checked.Compute
n_bindsand apply the--min-bindsguard before dispatching to either code path.As per coding guidelines: "A CI gate must assert that the behavior it measures actually executed, not merely that the test completed without throwing; verify gate exit codes directly rather than relying on piped wrapper commands."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/gc_root_dominance_check.py` around lines 1498 - 1500, Move the n_binds computation and --min-binds guard ahead of the ns.stale_registers dispatch, so both run_stale and the normal path validate the bind-count safety threshold before proceeding. Preserve the existing run_stale arguments and behavior after this guard, and ensure the CI gate checks the underlying command’s exit status directly rather than relying on a piped wrapper result.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/perry-codegen/src/lower_call/console_promise.rs`:
- Around line 1416-1430: Root the value returned by js_implicit_this_set
immediately after each prev_this assignment, including both method_recv and
receiverless-call branches. Re-read the rooted value immediately before the
implicit-this restore around the dispatch cleanup, then release this temporary
root after releasing roots so nested roots unwind inner-to-outer; do not use the
stale SSA value across the allocating rebind or dispatch.
In `@scripts/gc_root_dominance_check.py`:
- Around line 1498-1500: Move the n_binds computation and --min-binds guard
ahead of the ns.stale_registers dispatch, so both run_stale and the normal path
validate the bind-count safety threshold before proceeding. Preserve the
existing run_stale arguments and behavior after this guard, and ensure the CI
gate checks the underlying command’s exit status directly rather than relying on
a piped wrapper result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e398d9b9-757a-487e-ab71-129b2469eed0
📒 Files selected for processing (8)
changelog.d/7214-closure-calln-stale-registers.mdcrates/perry-codegen/src/expr/temp_root.rscrates/perry-codegen/src/lower_call/console_promise.rsscripts/gc_root_dominance_check.pytest-files/test_gap_gc_closure_call_argument_rooting.tstest-files/test_gap_gc_closure_call_callee_rooting.tstest-files/test_gap_gc_closure_call_this_rooting.tstest-parity/gc_repsel_corpus.txt
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/perry-codegen/src/expr/temp_root.rs
- test-files/test_gap_gc_closure_call_this_rooting.ts
- test-files/test_gap_gc_closure_call_callee_rooting.ts
2 participants
Stacked on #7206 — branch is based on
fix/7154-stale-register-roots, so review that first. Base ismain; the diff will shrink to just this commit once #7206 merges.The site #7206 named and deliberately left open:
js_closure_callN, the generic dynamic-value-call lowering, which held the callee, thethisreceiver and every already-lowered argument in bare SSA registers across the argument list. Three stale registers in the single hottest emitted call path, and the last known instance of #7192's root-store-dominance class.The three registers
An SSA register is not a GC root. Under
PERRY_GC_MOVING_LOOP_POLLS=1a 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.js_closure_unbox_callee_checkedmasks a pre-move address,js_closure_callNreads a closure header out of abandoned memory →TypeError: value is not a functionthisreceiverthis.xreads garbageWhy this is not a copy of #7206's fix
The three windows are different, so they are computed separately rather than protected as one block. The argument window is the subtle one:
js_closure_unbox_callee_checked_rebindcallsclone_closure_rebind_this, which allocates a replacement closure (closure/dynamic_props.rs:1040) when the callee capturesthis. It sits below the last argument and abovejs_closure_callN— so the arguments must be re-read below it, while the callee and receiver are consumed by it.RootedOperands::rereadre-reads the whole group at one point, which is wrong for this shape. Hencereread_one: re-read a single operand at a caller-chosen point.Hoisting the unbox above the argument list would remove the window instead, and was rejected: 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 just 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, sof(x, y)on inert operands emits exactly the IR it emitted before.Temp roots, not re-lowering: re-lowering the callee or receiver would observe an assignment made by an argument, which is a miscompile rather than a rooting fix.
The false green that nearly shipped
PERRY_GC_MOVING_LOOP_POLLSis compile-time since #7161 and runtime-armed (gc_moving_loop_polls_enabled(),gc/policy.rs:1759). The first cut of these three gap tests passed 10/10 on the unfixed parent because the harness set the flag only at compile time: the safepoint calls were emitted and then returned immediately. Both, or the test proves nothing.Verification
test_gap_gc_closure_call_callee_rooting.ts,POLLS=1TypeError: value is not a function10/10bad 010/10test_gap_gc_closure_call_this_rooting.ts,POLLS=1TypeError: value is not a function10/10bad 010/10test_gap_gc_closure_call_argument_rooting.ts,POLLS=1TypeError: value is not a function10/10bad 010/10POLLS=1+PERRY_GEN_GC=0bad 0bad 05/5bad 0bad 05/5cargo test -p perry-codegencargo testdetail: 6loop_safepoint_purity(#7161's flip), 16native_proof_regressions, 3native_proof_buffer_views, 1shadow_slot_hygiene, 1typed_shape_descriptors— all pre-existing on the parent. Oneperry-codegenlib unit test that is red on the parent passes here.Cost — measured, not assumed
This is the central dispatch path, so it was measured over the 141-module
sfw-registrycorpus.operand_protectionemits nothing for an operand whose window cannot collect, which is why the delta is this small:js_gc_temp_root_pushsitesChecker: a cited
NONCOLLECTINGentry, and what it revealedjs_closure_unbox_callee_checkedis now inNONCOLLECTING, citingclosure/unbox.rs:25— a tag check on the NaN-boxed callee 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 entire argument list of every 1-arg dynamic call as stale. 372 of the 729 fatal-sink hits were that single false positive, all markedMOVING: no— which is why the raw count barely moved after the fix and why the corrected numbers are the ones quoted. This is the checker's one-sided discipline working as designed: a missing entry costs false positives, never a missed bug.js_closure_unbox_callee_checked_rebindis 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.
sfw-registry --helpunder a genuinePOLLS=1build is still red, so #7161's stopgap stays. Compiled and run with the flag on this build: 3/10 pass, 7/10 SIGSEGV. The default arm is clean 10/10, so nothing was traded away.The three registers were real and are now provably rooted, but they are not the last thing standing between the registry and a clean evacuating minor. Two concrete leads, both found while fixing the above and neither speculative:
prev_thisin this same lowering is the same bug, unfixed.js_implicit_this_setreturns the previous implicitthis, read out of theIMPLICIT_THIScell — whichobject/this_binding.rs:176documents as a scanned mutable root the collector rewrites. That value is then held in a bare register across the entire user call and written back afterwards, so a collection anywhere inside the callee makes the restore publish a from-space pointer back into a root. It is invisible to the current checker on both ends:js_implicit_this_setis not inROOT_READ_CALLS, and it is not aRECEIVER_SINKSfatal sink. Fixing it costs a temp root on every dynamic call, so it was measured and left rather than folded in here.js_closure_call1, 22js_closure_call2, 18js_closure_call_apply_with_spread, 17js_array_spread_append, 15js_object_set_field_by_name, 15js_array_concat, and a long tail. The spread path (expr/call_spread.rs) is the obvious next site: same dispatch family, untouched by fix(codegen): root a call receiver and a computed-read base across their sibling operands #7206 and by this PR.Refs #7154, #7206, #7192, #7198, #7184, #7161, #7114, #6951, #519.
Summary by CodeRabbit
Bug Fixes
thisbinding and variadic argument handling during dynamic calls.Tests
Documentation