fix(gc): root optional parameters, and the instance in runtime-dispatched new (#7280) - #7291
Merged
proggeramlug merged 1 commit intoAug 3, 2026
Conversation
…ched `new` (PerryTS#7280) PerryTS#7154's surviving residual, the one holding PerryTS#7161's evacuating-minor stopgap in place. zod `src/v4/core/util.ts`'s `clone(inst, def?, params?: { parent: boolean })` gave `params` no shadow slot even though HIR types it `Object(...)` and `type_is_pointer_bearing` says true. It crossed `new inst._zod.constr(...)` in callee-saved `d8` and `params?.parent` dereferenced retired from-space. The cause is the OPTIONAL MARKER, not the object type. The pointer-locals refinement fixpoint proves a local non-pointer from its WRITES, and for a PARAMETER the write list is a strict subset of its definitions -- the incoming argument is not a write. The optional-parameter desugaring then donates the one write that completes the false proof, for free, on every optional parameter in the program: if (p === undefined) { p = undefined; } `Void` is definitely-non-pointer, so "every write is non-pointer" held. Both of that loop's conclusions are unsound for a parameter for the same reason, and both are excluded here; each has its own reproducer, the second measured red 200/200 with only the first exclusion applied. Found while bisecting toward it: four runtime-side siblings. Every `new` route codegen cannot resolve statically hands construction to perry-runtime, and all four held the instance in a bare Rust local across the user constructor body, returning the pre-move address. Routed through `RuntimeHandleScope` -- which is what this file's own `CURRENT_NEW_TARGET` doc-comment already said needed doing. Also corrects `gc/policy.rs`'s "sound by construction" claim for the loop-polls route (PerryTS#7280 ask 2): deferring to `js_gc_loop_safepoint` makes the collection point precise for CODEGEN frames and says nothing about a runtime Rust frame. Measured, release, `PERRY_GC_MOVING_LOOP_POLLS=1` at compile: sfw-registry --help, PROTECT_FROMSPACE=1 DEPTH=800 (no zeal) FAULT 10/10 -> 40/40 clean sfw-registry --help, plain polls ~2/60 fail -> 59/60 6 unit reproducers, POLLS=1 ZEAL=1 200/200 wrong -> clean (PERRY_GEN_GC=0 clean both sides) Codegen cost on the sfw-registry binary: +33,088 bytes (+0.1231%), all of it the `all_non_pointer` exclusion; the other two changes are +0. Two witnesses added and registered in test-parity/gc_repsel_corpus.txt. PerryTS#7280 STAYS OPEN. The plain arm is 59/60, and the surviving failure is a `TypeError: Cannot read properties of undefined (reading 'has')` -- a different symptom from the pre-fix `object is not a function` / SIGSEGV -- so at least one more unrooted holder remains on that workload. PerryTS#7161's stopgap must stay.
📝 WalkthroughWalkthroughChangesThe PR preserves pointer parameters during codegen refinement and roots relocated values across four dynamic construction paths. It adds GC stress tests for optional parameters and dynamic construction, updates GC policy documentation, and registers both tests as moving-GC witnesses. Moving GC rooting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ConstructionPath
participant RuntimeHandleScope
participant UserConstructor
participant MovingGC
Caller->>ConstructionPath: invoke dynamic construction
ConstructionPath->>RuntimeHandleScope: root instance and construction state
ConstructionPath->>UserConstructor: execute constructor
UserConstructor->>MovingGC: allocate objects
MovingGC-->>RuntimeHandleScope: relocate rooted values
ConstructionPath->>RuntimeHandleScope: reread relocated values
ConstructionPath-->>Caller: return relocated instance
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-runtime/src/object/class_registry/construct.rs (1)
985-1078: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRoot the instance before prototype lookup in both construct paths.
At
crates/perry-runtime/src/object/class_registry/construct.rs#L985-L1078,obj_ptris allocated beforeclosure_get_dynamic_prop(..., "prototype")andensure_function_prototype_objectcan install a prototype. Atcrates/perry-runtime/src/object/class_registry/construct.rs#L1669-L1680,obj_ptris allocated beforeconstructor_prototype_bits(nt)readsnewTarget.prototypevia object allocation access. MoveRuntimeHandleScope::new()immediately after allocation, root the instance, and use a refreshed handle for the subsequent prototype-linking call(s) to satisfy GC-managed-pointer rooting before user-visible lookups/allocation.🤖 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-runtime/src/object/class_registry/construct.rs` around lines 985 - 1078, Root the newly allocated instance before any prototype lookup or allocation in both construct paths. In crates/perry-runtime/src/object/class_registry/construct.rs lines 985-1078, and again at lines 1669-1680, create RuntimeHandleScope immediately after obj_ptr allocation, register the instance, and use the refreshed rooted handle for subsequent object_link_class_default_prototype and related calls; preserve the existing prototype-selection behavior while ensuring the pointer remains valid across GC.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.
Inline comments:
In `@crates/perry-runtime/src/gc/policy.rs`:
- Around line 1420-1437: Update the policy comment near the loop-poll deferral
explanation, specifically the statement at the later “sound by construction”
reference, to remove that claim. State instead that deferral makes the
collection point precise, while values held in runtime Rust frames still require
explicit rooting across calls into user JavaScript; keep the surrounding
explanation consistent.
---
Outside diff comments:
In `@crates/perry-runtime/src/object/class_registry/construct.rs`:
- Around line 985-1078: Root the newly allocated instance before any prototype
lookup or allocation in both construct paths. In
crates/perry-runtime/src/object/class_registry/construct.rs lines 985-1078, and
again at lines 1669-1680, create RuntimeHandleScope immediately after obj_ptr
allocation, register the instance, and use the refreshed rooted handle for
subsequent object_link_class_default_prototype and related calls; preserve the
existing prototype-selection behavior while ensuring the pointer remains valid
across GC.
🪄 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: 9d42a6e3-0cb5-4951-b2d6-2381327ade7b
📒 Files selected for processing (7)
changelog.d/7280-optional-param-and-dynamic-construct-rooting.mdcrates/perry-codegen/src/collectors/pointer_locals.rscrates/perry-runtime/src/gc/policy.rscrates/perry-runtime/src/object/class_registry/construct.rstest-files/test_gap_gc_dynamic_construct_receiver_rooting.tstest-files/test_gap_gc_optional_param_receiver_rooting.tstest-parity/gc_repsel_corpus.txt
Comment on lines
+1420
to
+1437
| // #7280: that used to read "so it is sound by construction". IT IS NOT, and | ||
| // the overclaim is the kind that stops the next person looking. What | ||
| // deferring to `js_gc_loop_safepoint` buys is precise *codegen* roots — the | ||
| // loop body has completed, so every live value the COMPILED frame holds is a | ||
| // named local on the shadow stack. It buys nothing for a value parked in a | ||
| // RUNTIME (Rust) frame, which the precise walk does not visit at all: no | ||
| // shadow slot, no temp root, no registered scanner. A back-edge poll that | ||
| // fires while `js_new_function_construct` is midway through a user | ||
| // constructor body relocates the instance that helper is holding in a plain | ||
| // `let`, and no safepoint's root set covers it. That was measured, not | ||
| // argued — four reproducers in | ||
| // `test-files/test_gap_gc_dynamic_construct_receiver_rooting.ts`, 200/200 | ||
| // iterations wrong per route before the `RuntimeHandleScope` routing in | ||
| // `object/class_registry/construct.rs`. The correct statement is: the | ||
| // loop-polls route makes the COLLECTION POINT precise; keeping runtime | ||
| // frames rooted across it is a separate obligation, discharged by | ||
| // `RuntimeHandleScope`, and every runtime helper that calls back into user | ||
| // JS owes it. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the stale “sound by construction” claim.
These lines correctly state that loop-poll deferral does not root values held in runtime Rust frames. However, Line 1499 still calls the same deferral “sound by construction.” Keep one consistent guarantee in this policy comment.
Update Line 1499 to state that deferral makes the collection point precise, while runtime values still require explicit rooting across user-JS calls.
Suggested wording
- // The deferral above is the primary path and is sound by
- // construction; reaching here means the slack expired without the
+ // The deferral above is the primary path and makes the collection
+ // point precise; runtime Rust values still require explicit rooting
+ // across user-JS calls. Reaching here means the slack expired withoutAs per coding guidelines, GC-managed pointers must remain rooted across every operation that can collect; the supplied file context shows that Line 1499 still contradicts this requirement.
🤖 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-runtime/src/gc/policy.rs` around lines 1420 - 1437, Update the
policy comment near the loop-poll deferral explanation, specifically the
statement at the later “sound by construction” reference, to remove that claim.
State instead that deferral makes the collection point precise, while values
held in runtime Rust frames still require explicit rooting across calls into
user JavaScript; keep the surrounding explanation consistent.
Source: Coding guidelines
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.
Closes part of #7280. #7280 itself stays open — see "What this does not fix".
The bug
sfw-registry --helpunderPERRY_GC_MOVING_LOOP_POLLS=1faults in zodsrc/v4/core/util.ts:485:paramslives in callee-savedd8acrossnew inst._zod.constr(...)— a userconstructor crossing ~180 copying minors — with no shadow slot, so the
collector never rewrites it and
params?.parentdereferences retiredfrom-space. The nullish guard doesn't help: a stale pointer is neither
nullnor
undefined.Why parameter #2 was classified non-pointer — the HIR answer
Not the type.
--trace hir --focussays it plainly:Object(...)—type_is_pointer_bearingreturns true, sois_ptr_typed(&p.ty)atpointer_locals.rs:908was never the problem. The slotwas dropped by
non_pointer_locals, and statement[1]is why.The refinement fixpoint proves a local non-pointer from its WRITES. For a
PARAMETER the write list is a strict subset of its definitions — the incoming
argument is not a write. The optional-parameter desugaring then donates the
one write that completes the false proof, for free, on every optional
parameter in the program:
Voidis definitely-non-pointer, soall_non_pointerstayed true and theparameter was proven non-pointer while its declared type said
Object.That is also why a hand-written witness with
params: any(no?) stays greenand misled earlier attempts: without the optional marker there is no write, and
!local_writes.is_empty()already guarded that case.The fix
Site (1),
collectors/pointer_locals.rs— exclude parameters from bothconclusions the fixpoint draws from that subset. Each half has its own
reproducer:
all_non_pointerparams?: { parent: boolean }— zod'sprecise_inferencep?: anyplus a local aliased from it — the loop instead concludeslocal_value_types[p] = Void, the alias inherits it and loses its slotOne-sided in the safe direction: a parameter that would have been proven
non-pointer keeps a root the collector rewrites harmlessly. Body
lets areuntouched — their
Stmt::Letinit is inwrites, so for them the write listreally is every definition.
Not site (2).
lower_generic_property_getis where the fault lands, notwhere the bug is. The receiver arrives already stale; re-reading it inside the
property-get diamond would re-read the same dead register. I disassembled the
faulting frame to confirm this rather than inferring it:
clone's prologue isjs_shadow_frame_enter(2), slot 0 ←inst, slot 1 ←cl, andfmov d8, d2isnever followed by a store. Site (2) needs no change, and
temp_root::lower_exprs_rootedis indeed the wrong tool — it is for argument lists.
Four runtime-side siblings, found while bisecting toward site (1) and each
independently reproducible. Every
newroute codegen cannot resolve staticallyhands construction to perry-runtime, and all four held the instance in a bare
Rust local across the user constructor body, returning the pre-move address:
construct_registered_class_refnew obj.ctor(n),ctora declared classjs_new_function_construct, class-object armctora class expression valuejs_new_function_construct, closure tailctora plain functionjs_new_function_construct_with_new_targetReflect.construct(fn, args, newTarget)A runtime frame is not visited by the precise root walk and the conservative
stack scan resolves to
SkipDisabledin shipped builds. Routed throughRuntimeHandleScope— which this file's ownCURRENT_NEW_TARGETdoc-commentalready said needed doing and deferred. The same scope roots the displaced
prev_this/prev_new_target/prev_current_new_targetvalues named in thatcomment.
gc/policy.rs, #7280 ask 2. "Sound by construction" is corrected. Deferringto
js_gc_loop_safepointmakes the collection point precise for codegenframes; it says nothing about a value parked in a runtime Rust frame. The four
sites above are exactly that gap.
Evidence
Release build,
PERRY_GC_MOVING_LOOP_POLLS=1at compile time.sfw-registry --help,PROTECT_FROMSPACE=1 DEPTH=800, no zealsfw-registry --help, plain pollswparam(zod shape),POLLS=1 ZEAL=1bad 200(200/200)bad 0wanyparam(p?: any+ alias)bad 200bad 0+ PERRY_GEN_GC=0Two witnesses added, both registered in
test-parity/gc_repsel_corpus.txtsogc-moving-witnessesruns them (not left dark — #7278's failure mode):test_gap_gc_optional_param_receiver_rooting— both parameter shapes.test_gap_gc_dynamic_construct_receiver_rooting— all four runtime routes.Both verified green in the exact
loop_pollsarm env(
HEAP_LIMIT=8 INCREMENTAL=0 CONSERVATIVE_STACK_SCAN=off POLLS=1 FORCE_EVACUATE=1)and byte-identical to the Node oracle.
Codegen cost,
sfw-registrybinary, measured againstorigin/main:origin/mainRuntimeHandleScopefixall_non_pointerexclusionprecise_inferenceexclusionAll of the cost is the
all_non_pointerexclusion. This is above the +0.042%reference from #7214 and I am flagging it rather than burying it: it is the
price of not proving a parameter non-pointer from a subset of its definitions,
and the cheaper variants are the ones that leave the bug in.
Tests.
loop_safepoint_purityis 1 passed / 6 failed both on this branchand on the parent
97c69211d— measured, not assumed, by stashing the threesource changes and re-running. It asserts back-edge polls survive, and polls are
default-off since #7161, so it needs
PERRY_GC_MOVING_LOOP_POLLS=1; with thatset it is 7/7 on this branch. Not mine, but worth its own issue — it is a test
that cannot pass in the default configuration.
What this does not fix, and why there is no #7161 revert here
The plain-polls arm is 59/60, not 60/60. The surviving failure is
which is a different symptom from the pre-fix
object is not a function/SIGSEGV, so at least one more unrooted holder remains on that workload. The
revert is not earnable and I have not opened it. #7161's stopgap must stay.
Separately, and orthogonally: the zeal-only
node-machine-idTypeError: Cannot convert undefined or null to objectreported during triageis real and reproduces only under
PERRY_GC_ZEAL=1, which is supposed to besemantically transparent. It deserves its own issue.
★ The gate blind spot
--stale-registersclassifies the faulting siteMOVING: no, sogc-root-dominance's--moving-onlyarm cannot see this bug while thefrom-space reporter faults on it every time. Both are right, and that is the
problem.
The checker reads emitted LLVM IR. At the property-get it sees a receiver loaded
from a correctly shadow-bound alloca, and the collector really does rewrite
that slot — so
MOVING: nois a true statement about the IR. The staleness wasinjected from outside the IR: for site (1) by a slot the collector was never
told about, and for the four runtime sites by a Rust frame the IR does not
contain at all. A rooted slot holding a dangling pointer.
So this is not a misclassification to tune. It is a scope boundary:
gc_root_dominance_check.pycan only ever see defects expressible in emittedIR, and CLAUDE.md's "runtime-side cache" note is one instance of a larger class —
any value a runtime helper holds across a call back into user JS. The four
sites here are that class, and no static IR pass will ever find them. The
detector that does is
PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800on a realworkload. Worth writing into
docs/src/internals/gc-rooting-invariant.mdas astated limit, so the next person does not read a green
--moving-onlyascoverage.
One protocol note for whoever picks up the remaining 1/60:
perry compilestrips symbols, so use
PERRY_DEBUG_SYMBOLS=1or the protector backtrace isunreadable. And
PERRY_GC_DIAG=1is inert in a locally auto-optimized build(the linked runtime lacks the
diagnosticsfeature), so liveness has to comefrom CI or from a red-then-green pair.
No version bump in this PR, per instruction; the maintainer bumps at merge.
Summary by CodeRabbit
Bug Fixes
this, andnew.targetvalues during moving garbage collection.Tests
Documentation