fix(codegen): root pointer locals of constructor bodies inlined into other frames - #9102
Conversation
…other frames A constructor body spliced inline into another function — the super() parent-body inline in a derived constructor, the new-site own/inherited ctor inlines, and let_stmt's scalar-ctor variants — declared its locals into plain entry allocas: the enclosing function's shadow-slot map is computed by collect_pointer_typed_locals over that function's OWN params and body, so the spliced body's pointer locals (and its bound ctor params) were invisible to the collector. A moving minor between a local's store and a later use left it holding the pre-move address. This is issue PerryTS#9081's three.js failure: RenderTarget's ctor body, spliced into WebGLRenderTarget_constructor by the super() inline, holds `const texture = new Texture(image)` across `this.textures = []` and the attachment loop. Under a 1 MiB nursery the texture local went stale, and Texture.copy() read `source.mipmaps` as undefined ("Cannot read properties of undefined (reading 'slice')"). The from-space quarantine pinpointed the stale header deref inside the constructor; the emitted IR showed the spliced texture alloca had no js_shadow_slot_bind while the standalone RenderTarget_constructor (whose own compile pass saw the body) rooted it at slot 4. Fix: expr/shadow_slot.rs gains root_inlined_ctor_pointer_locals, called at all five splice sites before lowering the spliced body. It runs the same pointer-locals collector over the spliced params+body and extends the frame through reserve_shadow_slot, which grows the already-emitted frame in place on both root backends (native stack maps and shadow frames) — matching the bug reproducing under both PERRY_RS4GC arms. Let/assignment sites then mirror stores through the ordinary bind path; an id already bound in ctx.locals (a ctor param) is bound immediately, and rebound on a repeated inline of the same constructor so the slot tracks the newest alloca. Local ids are module-unique, so extending the map never aliases an enclosing local. Also moves is_global_this_value from let_stmt.rs to let_stmt_facts.rs (pure relocation) to stay under the 2000-line cap. Regression test: test_gap_gc_inlined_ctor_body_locals_rooting.ts (registered in the gc-repsel corpus). Its parity-env runs the seeded every-poll evacuating schedule with the from-space quarantine armed: on the unfixed compiler it SIGSEGVs at minor #0 (stale spliced-local deref); fixed, it is byte-exact with the oracle. The write-after-move stamp discriminator also catches the bug by value when quarantine pages were recycled. Verified against the original three.js reproduction on the remote host: default heap, forced 1 MiB nursery, quarantine, and the PERRY_RS4GC=0 arm all pass; PERRY_GC_DIAG confirms 2 copying minors so the stress is live. Fixes PerryTS#9081 Claude-Session: https://claude.ai/code/session_0131AxDes1mwPJJ343CxS4oX
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe change adds shadow-slot rooting for pointer locals and parameters from inlined constructor bodies. It wires the helper into five constructor-splicing paths, relocates ChangesInline Constructor GC Rooting
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR roots pointer locals in inlined constructor bodies to prevent stale references after moving garbage collection. No actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description provides detailed root-cause, fix, regression-test, linked-issue, and validation information. It does not use every template heading or include the checklist, but it is substantially complete. Full details: Linked Issues checkExplanation The implementation addresses the core issue by rooting pointer locals and constructor parameters at all five splice sites, and the regression test verifies moving-GC behavior. However, the linked issue specifically requests a repository regression using compilePackages with Three.js 0.180.0 and new WebGLRenderTarget() under a forced small nursery; the provided test is a focused constructor-codegen test instead. Resolution Add or reference an automated repository regression that compiles Three.js 0.180.0 through compilePackages and executes new WebGLRenderTarget() with a forced small scavenging nursery, while asserting successful output and confirming that evacuation occurred. Full details: Docstring CoverageExplanation Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 7 files. (2 skipped: 2 unsupported.)
✨ 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 |
|
Merged. This is the highest-consequence class of bug in the codebase and the diagnosis is exactly right, so I validated it with both instruments rather than just the test. The root cause statement is the valuable part:
That is the third shape in CLAUDE.md's root-store-dominance list — "the value lives in a plain The behavioural A/B, under the knobs the test itself declares (
Main's failure is reported by the quarantine in as many words: Which is the point worth underlining: without GC pressure both arms print identical correct output. A reviewer running this test plainly would have concluded there was nothing to fix. The The static instrument agrees independently.
The 46 is what makes the zero meaningful — the checker had subjects rather than being blind to the file. (The root-dominance check proper reports 0 root stores on this fixture and says so rather than returning a green; that is the tool being honest about not applying here, not evidence.) The three.js provenance in the header — Covering all four splice sites (the Validation: codegen 1347 passed, runtime 2819 ( |
1 participant
Fixes #9081
Root cause
Codegen splices constructor bodies inline into other functions at five sites: the
super(...)parent-body inline in a derived constructor (expr/this_super_call.rs), thenew-site own-ctor and inherited-ctor inlines (lower_call/new.rs), andlet_stmt's scalar-ctor variants. The enclosing function's shadow-slot map is computed bycollect_pointer_typed_localsover that function's own params and body, so a spliced body's pointer locals — and its ctor params, bound bybind_inline_constructor_params— landed in plain entry allocas that are neither shadow slots nor temp roots. The collector never rewrites them, and a moving minor between the local's store and a later use leaves it holding the pre-move address (the CLAUDE.md "root-store dominance" class,#7207shape, introduced by body splicing).That is exactly how three.js died under
compilePackageswith a small nursery:RenderTarget's ctor body, spliced by the super() inline into the standaloneWebGLRenderTarget_constructor(which the entry module calls as a symbol), holdsconst texture = new Texture(image)acrossthis.textures = []and the attachment loop. The from-space quarantine (PERRY_GC_PROTECT_FROMSPACE=1, depth 800) pinpointed the stale header deref inside that constructor; the emitted IR shows the splicedtexturealloca has nojs_shadow_slot_bind, while the standaloneRenderTarget_constructor— whose own compile pass saw the body — roots the same local at slot 4. Downstream,Texture.copy()readssource.mipmapsoff the retired address asundefined→TypeError: Cannot read properties of undefined (reading 'slice').Fix
expr/shadow_slot.rsgainsroot_inlined_ctor_pointer_locals, called at all five splice sites before lowering the spliced body. It runs the same pointer-locals collector over the spliced params+body and extends the frame viareserve_shadow_slot, which grows the already-emitted frame in place on both root backends (native stack maps and shadow frames) — matching the bug reproducing under bothPERRY_RS4GCarms.Let/assignment sites then mirror stores through the ordinary bind path; an id already bound inctx.locals(a spliced ctor param) is bound immediately, and re-bound on a repeated inline of the same constructor so the slot tracks the newest alloca. Local ids are module-unique (one counter per lowering context), so extending the map never aliases an enclosing local, and a nestedsuper()chain recurses through the same site naturally.is_global_this_valuemoved fromlet_stmt.rstolet_stmt_facts.rs(pure relocation) to stay under the 2000-line cap.Regression test
test-files/test_gap_gc_inlined_ctor_body_locals_rooting.ts, registered in the gc-repsel corpus. Itsparity-envruns the seeded every-poll evacuating schedule with the from-space quarantine armed, and its stamp discriminator makes a stale spliced local observable by value after a single move (write through the collector-rewritten instance field, read back through the spliced local). Four arms: the dynamic-construct route that replays the standalone derived ctor (the exact three.js configuration), directnewof the derived/default/base classes.Validation (on the issue's reproduction host)
CRASHED), quarantine naming the stale spliced-local deref. Fixed: byte-exact with the oracle under default env and parity-env, on bothPERRY_RS4GCbackends.new WebGLRenderTarget(), three 0.180.0 viacompilePackages): passes on default heap, forced 1 MiB nursery, forced nursery + quarantine (depth 800), and thePERRY_RS4GC=0build.PERRY_GC_DIAGconfirms 2 copying collections, so the stress is live, not vacuous.gc_repsel_matrix.sh --arms pron the new test: 7/7 cells byte-exact vs pinned node 26.5.1; the moving arms (evac_minor,force_verify) are live (3,469 objects moved).test_gap_sweep A/B on the same host, same oracle: failure lists byte-identical between the unfixed and fixed compilers except the new test (crash → pass). The pre-existing deltas on that host are its ext-feature link gaps (http2/net) and known snapshot entries.cargo test -p perry-codegen: 1347 passed.scripts/run_lint_gates.sh: green except the known worktree-onlycargo fmt --allmanifest quirk (changed files are rustfmt-clean); the test-registration gate passes with the new corpus entry.https://claude.ai/code/session_0131AxDes1mwPJJ343CxS4oX
Summary by CodeRabbit
Bug Fixes
Tests