fix(codegen): never let a shadow-frame slot index escape the pushed frame (#7154) - #7184
Merged
proggeramlug merged 1 commit intoAug 1, 2026
Merged
Conversation
📝 WalkthroughWalkthroughShadow-frame slot allocation now deduplicates repeated HIR local IDs across parameters, declarations, and catch parameters. A debug assertion checks frame sizing. A regression fixture verifies bounded indices and correct allocation for a later pointer local. ChangesShadow-slot deduplication
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
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 |
…rame Duplicate `var` declarations share one HIR local id but keep a Stmt::Let per declaration site. collect_pointer_typed_locals burned a slot index for every Let while the map kept one entry per id, and every caller sizes the frame with map.len() — so functions with redeclarations pushed a frame smaller than the highest index handed out (lodash's runInContext: frame 598, binds up to 769). js_shadow_slot_bind and the PerryTS#7088 inline store both bounds-check and silently no-op on an out-of-frame index, so those locals were live but invisible to the precise-root moving minor: an evacuating collection at a loop back-edge poll relocated the object and left the compiled local slot pointing into from-space — the mutator then re-injected the stale address through ordinary stores ("TypeError: value is not a function" once called). This is the sfw-registry --help crash under PERRY_GC_MOVING_LOOP_POLLS=1 traced in PerryTS#7154 (lldb: lodash.js:10751, castRest read from an unrooted local). Assign at most one slot per id (entry().or_insert_with), restoring the invariant map.len() == slots handed out == max index + 1, and assert it. Regression test compiles a duplicate-var function and asserts every emitted slot index is inside the pushed frame. Refs PerryTS#7154.
jdalton
force-pushed
the
fix/7154-shadow-frame-slot-overflow
branch
from
August 1, 2026 12:26
a9191ad to
9407445
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-codegen/tests/shadow_slot_hygiene.rs (1)
689-705: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that the duplicate local uses slot 0.
The current assertions prove only that indices are bounded and that the trailing local uses slot 1. They do not prove that both
id: 1declarations reuse the first slot. A future allocator could alias both locals at slot 1 and still satisfy these assertions.Proposed assertion
assert!( fn_ir.contains("`@js_shadow_slot_bind`(i32 1, ptr %"), "trailing pointer local must be rooted at the deduped index; fn IR:\n{fn_ir}" ); + assert!( + fn_ir.contains("`@js_shadow_slot_bind`(i32 0, ptr %"), + "duplicate local must use the first deduplicated slot; fn IR:\n{fn_ir}" + );🤖 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/tests/shadow_slot_hygiene.rs` around lines 689 - 705, Extend the assertions in the shadow-slot hygiene test to verify that both duplicate id: 1 locals bind to slot 0. Keep the existing bounds and trailing-local slot-1 assertions, and inspect the parsed `@js_shadow_slot_bind` entries so the duplicate declarations cannot both be incorrectly assigned slot 1.
🤖 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.
Nitpick comments:
In `@crates/perry-codegen/tests/shadow_slot_hygiene.rs`:
- Around line 689-705: Extend the assertions in the shadow-slot hygiene test to
verify that both duplicate id: 1 locals bind to slot 0. Keep the existing bounds
and trailing-local slot-1 assertions, and inspect the parsed
`@js_shadow_slot_bind` entries so the duplicate declarations cannot both be
incorrectly assigned slot 1.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ab45e265-3b66-47e1-8408-222d03f27286
📒 Files selected for processing (3)
changelog.d/7184-shadow-frame-slot-overflow.mdcrates/perry-codegen/src/collectors/pointer_locals.rscrates/perry-codegen/tests/shadow_slot_hygiene.rs
jdalton
added a commit
to jdalton/perry
that referenced
this pull request
Aug 1, 2026
… after it PerryTS#7184 fixed one instance of "a live GC value is invisible to the moving minor because its root store silently no-ops": the store's slot index fell outside the pushed shadow frame, so js_shadow_slot_bind bounds-checked it away. This is its sibling — the store is emitted in-frame, but LATE. The invariant is that a GC-managed value's root store must DOMINATE every subsequent site that can trigger a collection; four lowerings broke it by keeping the value in a bare SSA register across a call that allocates, which under PERRY_GC_MOVING_LOOP_POLLS=1 reaches a back-edge poll and an evacuating minor. new C(...) is the load-bearing one. The instance was a raw register while the constructor body ran. It is rooted inside the callee (the `this` parameter has a shadow slot), so the minor does not free it — it MOVES it and rewrites the callee's root, leaving the caller's register naming from-space. js_gc_init_typed_shape_layout then installed the layout descriptor on the abandoned copy and js_ctor_return_override published that dead address into the caller's shadow slot: a *rooted* slot holding a dangling pointer, which is exactly why PerryTS#7154's from-space scan only ever saw offenders one or more cycles after the target died, with correct layout coverage. The instance is now temp-rooted across the constructor and re-read afterwards, on both the standalone-<Class>_constructor symbol path and the inline-ctor path. A class with no constructor, no fields and no heritage runs no user code in that window and keeps its previous IR exactly. Three more sites of the same shape: * Expr::ObjectSpread — `{ ...a, k: f() }` allocated the object and then wrote every field through a register held across each part's lowering, with no rooting at all. Expr::Object has used RootedHandle for this since PerryTS#6951; the spread form never got it. * Expr::ClassExprFresh — same for the fresh class object built by a class-expression factory, across its static-field initializers, captured arguments, symbol statics and `static { … }` blocks. * property / element stores — `o.k = f()` evaluates the reference first and the value second (spec order), leaving the receiver in a register across `f()`. The slot it was loaded from is a root and gets rewritten; the register does not, so the store landed in from-space and the field never appeared on the object the program kept. This is PerryTS#7114 with a receiver instead of a string literal. temp_root::ReceiverGuard roots it only when the value expression can collect, so an inert RHS keeps its previous IR. temp_root_scope_begin now takes the caller's extra reason to open a scope, because `new C()` with no arguments still needs a marker to cut against. Verified by test-files/test_gap_gc_new_instance_rooting.ts: wrong 9 times in 400 iterations at pure 73a9084 under PERRY_GC_MOVING_LOOP_POLLS=1 (3/3 runs, deterministic), clean 5/5 by default, clean 10/10 with this fix. A codegen regression test pins the def-use chain: the value js_ctor_return_override publishes must be re-derived from the instance's temp root. Also adds scripts/gc_root_dominance_check.py, the static checker that found these — it builds each function's CFG from the emitted LLVM, computes real Cooper/Harvey/Kennedy dominance, and reports every root store that does not dominate a preceding collection point. Both shipped bugs of this class are invisible to runtime GC probes, because at the moment of the collection there is nothing for the collector to find. Over the 196-module sfw-registry corpus it reported 234 violations before this change and 1 after. sfw-registry --help under PERRY_GC_MOVING_LOOP_POLLS=1 is still red — at least one more site of this class remains — so this does not close PerryTS#7154 and stopgap PerryTS#7161 stays. Its default arm is unchanged (clean 5/5). Refs PerryTS#7154, PerryTS#7184, PerryTS#7161, PerryTS#7114, PerryTS#6951.
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
2 participants
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.
What
collect_pointer_typed_localsassigns shadow-frame slot indices with a bareout.insert(id, next_slot); next_slot += 1;. A local id that appears in more than oneStmt::Let— duplicatevardeclarations share a single HIR binding and lowering keeps aLetat each declaration site — replaces the map entry but still burns an index. Every caller (codegen/function.rs,codegen/closure.rs,codegen/entry.rs,codegen/helpers.rs) sizes the frame withmap.len(), so a function with redeclarations pushes a frame smaller than the highest slot index it emits.lodash's
runInContexthas 170+varredeclarations: it pushesjs_shadow_frame_enter(i32 598)and then binds slots up to 769. Bothjs_shadow_slot_bindand the #7088 inline store bounds-check the index and silently no-op when it is at or beyond the frame length — so slots 598–769 are live locals that are invisible to the precise-root moving minor. An evacuating collection at a loop back-edge poll relocates the referent, rewrites every root it knows about, and leaves the compiled local slot pointing into from-space. The mutator then re-injects that stale address through ordinary stores — exactly the mutator-reinjection mechanism established at the end of the #7154 trail.Evidence
sfw-registry --help(the GC: evacuating minor drops an old-to-young field[1] edge, crashing with 'value is not a function' #7154 repro workload) at currentmain(6406ed66e),PERRY_GC_MOVING_LOOP_POLLS=1compile+run: crash 5/5TypeError: value is not a function. lldb backtrace of the throw:lodash.js:10751—var overArgs = castRest(...)— the calleecastRestloaded from stack slot[sp,#0x3320], whose inline shadow store targets slot index0x29c(668) in a 598-slot frame.var+ a closure local past the burned index + an allocating loop): red at pure6406ed66eunderPERRY_GC_MOVING_LOOP_POLLS=1(3/3 withPERRY_GC_FORCE_EVACUATE=1, 1/1 without), clean under the default (polls off), clean 3/3 with this fix — the exact collector-mode-tracking fingerprint of GC: evacuating minor drops an old-to-young field[1] edge, crashing with 'value is not a function' #7154.Fix
Assign at most one slot per id (
entry().or_insert_with), restoring the invariant the frame sizing depends on:map.len() == slots handed out == max index + 1, with adebug_assertso it cannot silently regress. Re-binding the same slot from each duplicate declaration site is correct — the id names one alloca.The regression test compiles a duplicate-var function and asserts every emitted
js_shadow_slot_bindindex (which the #7088 inline store mirrors on its fallback arm) is inside the pushed frame.Verification
main@6406ed66e(polls=1)sfw-registry --help, default armsfw-registry --help, polls=1, this branchcargo test -p perry-codegen --no-fail-fastmain(loop_safepoint_purity is red onmainsince #7161's default flip; native_proof_* targets are order-flaky onmain— verified by isolated re-runs passing on both trees); the new test passesRefs #7154. Keeps stopgap #7161 in place — the registry's polls-on arm is not clean yet.
Summary by CodeRabbit
Bug Fixes
Tests