fix(gc): root builtin.rs's constructor arguments across each other's lowering (#6986) - #7719
Conversation
|
Warning Review limit reached
Next review available in: 28 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughBuilt-in constructor lowering now adopts constructor arguments into the active ChangesBuilt-in constructor rooting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ExprNew
participant lower_builtin_new
participant RootedGroup
participant RuntimeConstructor
ExprNew->>lower_builtin_new: Lower built-in constructor
lower_builtin_new->>RootedGroup: Adopt evaluated arguments
lower_builtin_new->>lower_builtin_new: Lower later arguments and side effects
lower_builtin_new->>RootedGroup: Reread preserved arguments
lower_builtin_new->>RuntimeConstructor: Invoke constructor with rooted values
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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
🤖 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-codegen/src/lower_call/builtin.rs`:
- Around line 230-239: Update the affected constructor arms in
crates/perry-codegen/src/lower_call/builtin.rs:230-239, 264-273, and 298-312 to
lower and discard args[3..] after the optional length; at 332-340, 633-642,
655-664, 677-686, and 851-860, lower and discard args[2..] after the supported
flags, options, or arg1; and at 899-913, lower and discard args[3..] after the
message. Ensure every extra argument is evaluated for side effects while
preserving the existing supported-argument lowering.
🪄 Autofix
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: effc7429-9d58-4f7e-8278-f6b3725f284c
📒 Files selected for processing (5)
changelog.d/7719-builtin-ctor-roots.mdcrates/perry-codegen/src/lower_call/builtin.rscrates/perry-codegen/src/lower_call/new.rscrates/perry-codegen/src/temp_root_coverage/builtin_ctor.rscrates/perry-codegen/src/temp_root_coverage/mod.rs
| let source_collects = rooting::any_operand_may_collect(ctx, args[1..].iter()); | ||
| let source_idx = group.lower(ctx, &args[0], source_collects)?; | ||
| let offset_collects = rooting::any_operand_may_collect(ctx, args[2..].iter()); | ||
| let offset_idx = group.lower(ctx, &args[1], offset_collects)?; | ||
| let length_idx = adopt_optional_arg(ctx, args, 2, group)?; | ||
| let source = group.reread(ctx, source_idx)?; | ||
| let offset_box = group.reread(ctx, offset_idx)?; | ||
| let length_box = match length_idx { | ||
| Some(i) => group.reread(ctx, i)?, | ||
| None => double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Evaluate ignored constructor arguments.
These arms lower only their supported arguments. They do not lower later arguments for side effects. JavaScript evaluates every argument before a constructor call. Add a discard loop after the last supported argument in each arm.
crates/perry-codegen/src/lower_call/builtin.rs#L230-L239: Lowerargs[3..]after the optional length.crates/perry-codegen/src/lower_call/builtin.rs#L264-L273: Lowerargs[3..]after the optional length.crates/perry-codegen/src/lower_call/builtin.rs#L298-L312: Lowerargs[3..]after the optional length.crates/perry-codegen/src/lower_call/builtin.rs#L332-L340: Lowerargs[2..]after flags.crates/perry-codegen/src/lower_call/builtin.rs#L633-L642: Lowerargs[2..]after options.crates/perry-codegen/src/lower_call/builtin.rs#L655-L664: Lowerargs[2..]afterarg1.crates/perry-codegen/src/lower_call/builtin.rs#L677-L686: Lowerargs[2..]afterarg1.crates/perry-codegen/src/lower_call/builtin.rs#L851-L860: Lowerargs[2..]after options.crates/perry-codegen/src/lower_call/builtin.rs#L899-L913: Lowerargs[3..]after message.
📍 Affects 1 file
crates/perry-codegen/src/lower_call/builtin.rs#L230-L239(this comment)crates/perry-codegen/src/lower_call/builtin.rs#L264-L273crates/perry-codegen/src/lower_call/builtin.rs#L298-L312crates/perry-codegen/src/lower_call/builtin.rs#L332-L340crates/perry-codegen/src/lower_call/builtin.rs#L633-L642crates/perry-codegen/src/lower_call/builtin.rs#L655-L664crates/perry-codegen/src/lower_call/builtin.rs#L677-L686crates/perry-codegen/src/lower_call/builtin.rs#L851-L860crates/perry-codegen/src/lower_call/builtin.rs#L899-L913
🤖 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/builtin.rs` around lines 230 - 239,
Update the affected constructor arms in
crates/perry-codegen/src/lower_call/builtin.rs:230-239, 264-273, and 298-312 to
lower and discard args[3..] after the optional length; at 332-340, 633-642,
655-664, 677-686, and 851-860, lower and discard args[2..] after the supported
flags, options, or arg1; and at 899-913, lower and discard args[3..] after the
message. Ensure every extra argument is evaluated for side effects while
preserving the existing supported-argument lowering.
…lowering (#6986) 30 arms of lower_call/builtin.rs's lower_builtin_new lowered args[0] then args[1] (then, for several, discarded the rest for side effects) with plain lower_expr and no rooting decision — the same #6969 shape #7699 fixed in lower_new.rs's three non-class branches, left open by that PR for this file. WeakMap/WeakSet are a variant: the iterable was lowered, then js_weakmap_new (an unconditional allocation) ran, and only then was the iterable's now-possibly-stale register read. lower_builtin_new now takes the caller's RootedGroup (threaded in from lower_new_impl_inner, which already opens one per #6969/#7699) and three helpers adopt each operand into it as it is produced, never after the fact. CronJob needed a bespoke ordering: its raw-pointer derivation can itself allocate, so it has to run before the other two operands are re-read. Left out of scope: the extract_options_fields-based arms (Response, Request, Blob, File, Headers, ReadableStream, WritableStream, TransformStream) share the hazard but are a structurally different shape.
96c2553 to
9589f94
Compare
Merging as v0.5.141630 arms, not ~22 — the issue's own author flagged that estimate as unaudited, and it was low. Fixed by threading the caller's The honest part, and the reason I'm confident in thisThe static checker showed 0/0 both ways on the existing gate corpus — because that corpus contains no source exercising these constructors, so it cannot show a reduction. A naive TS-literal probe also read 0/0, and the report explains why rather than shrugging: Phase-3 object-literal synthesis and the array-literal inline-bump allocator each bind their own unrelated shadow slot that happens to also cover a plain constructor argument end-to-end. Only on switching to the project's own reproducer shape ( That sequence is worth keeping. A checker reporting 0/0 is the single easiest way to conclude "no bug here", and three separate reasons for a false zero showed up before the real signal did. Primary verification is therefore 6 sabotage-confirmed unit tests in
Named, not silently skippedThe |
…receiver hazard (#7664) scripts/gc_root_dominance_check.py: the native/--statepoints chain treated a phi as unconditionally transparent, so one tainted incoming edge blanket- tainted the phi's result and a downstream use was checked against ANY CFG path between source and use (between_blocks is deliberately path-insensitive, sound for an ordinary register but not for a phi, whose dynamic value depends on which edge was actually taken). All four reported unmasked hits were the same &&/|| short-circuit join: the tainted edge never crosses a safepoint, the OTHER edge does, and the checker reported that. _cast_closure gains phi_all_edges: a phi joins `chain` only once every incoming edge is independently in it. That closes the false positive and deliberately excludes the case of a single tainted edge with its own intervening safepoint before its predecessor's terminator; _phi_edge_hazard covers that separately, checking each edge's own window. Two new self-test fixtures (phi_safe_edge / phi_hazard_edge) pin both directions, each verified against a sabotaged copy of the checker to confirm it can still fail. lower_call/property_get/static_dispatch.rs: (Lexer as any).lex(...) reads a module-global receiver, then held it raw across arg-bundling logic that can allocate (a rest-param bundle always allocates; an object-literal argument can too) before implicit_this_save/js_static_this_arm_value read the stale copy -- the same #6969/#6986 shape #7719 just fixed in lower_call/builtin.rs, here on the receiver. Wrapped it in RootedGroup::adopt/reread. Re-verified against the current corpus: the checker fix eliminates exactly the four phi false positives with nothing else changing. The static-dispatch fix was not yet re-verified against a fresh corpus run after this rebase (disk pressure and box load made prior corpus runs unreliable) -- see the PR description for exactly what is and isn't confirmed.
…7664) Re-verifying the checker fix found 9 real+false hits, not the 8 the prior snapshot recorded -- test_gap_static_method_value_name_collision joined the population after #7691 without the budget being re-measured. Of the 9: 4 were the checker's own phi-edge false positives (fixed in the prior commit), 3 were unrooted:global (2 already fixed upstream by #7719, 1 fixed in the prior commit's static_dispatch.rs change), and 2 are unrooted:capture -- real, diagnosed, and tracked as this budget's referent rather than rushed. Measured on the native corpus, both arms of --moving-only, stale still 0.
…--max-unrooted to 2 (#7664) (#7724) * gc: fix the phi-edge checker false positives and the static-dispatch receiver hazard (#7664) scripts/gc_root_dominance_check.py: the native/--statepoints chain treated a phi as unconditionally transparent, so one tainted incoming edge blanket- tainted the phi's result and a downstream use was checked against ANY CFG path between source and use (between_blocks is deliberately path-insensitive, sound for an ordinary register but not for a phi, whose dynamic value depends on which edge was actually taken). All four reported unmasked hits were the same &&/|| short-circuit join: the tainted edge never crosses a safepoint, the OTHER edge does, and the checker reported that. _cast_closure gains phi_all_edges: a phi joins `chain` only once every incoming edge is independently in it. That closes the false positive and deliberately excludes the case of a single tainted edge with its own intervening safepoint before its predecessor's terminator; _phi_edge_hazard covers that separately, checking each edge's own window. Two new self-test fixtures (phi_safe_edge / phi_hazard_edge) pin both directions, each verified against a sabotaged copy of the checker to confirm it can still fail. lower_call/property_get/static_dispatch.rs: (Lexer as any).lex(...) reads a module-global receiver, then held it raw across arg-bundling logic that can allocate (a rest-param bundle always allocates; an object-literal argument can too) before implicit_this_save/js_static_this_arm_value read the stale copy -- the same #6969/#6986 shape #7719 just fixed in lower_call/builtin.rs, here on the receiver. Wrapped it in RootedGroup::adopt/reread. Re-verified against the current corpus: the checker fix eliminates exactly the four phi false positives with nothing else changing. The static-dispatch fix was not yet re-verified against a fresh corpus run after this rebase (disk pressure and box load made prior corpus runs unreliable) -- see the PR description for exactly what is and isn't confirmed. * gate(gc): lower gc-root-dominance-statepoints' --max-unrooted to 2 (#7664) Re-verifying the checker fix found 9 real+false hits, not the 8 the prior snapshot recorded -- test_gap_static_method_value_name_collision joined the population after #7691 without the budget being re-measured. Of the 9: 4 were the checker's own phi-edge false positives (fixed in the prior commit), 3 were unrooted:global (2 already fixed upstream by #7719, 1 fixed in the prior commit's static_dispatch.rs change), and 2 are unrooted:capture -- real, diagnosed, and tracked as this budget's referent rather than rushed. Measured on the native corpus, both arms of --moving-only, stale still 0. * chore: key the changelog fragment on PR #7724 * chore: point the budget referent at the split-out #7725 * chore: bump version to 0.5.1420 Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix * style: cargo fmt Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
1 participant
Summary
Closes #6986. #7699 fixed the three non-class branches of
lower_new_impl_inner(lower_call/new.rs) named in the issue and explicitly leftlower_call/builtin.rs's arms open, with the note "builtin.rs's ~22 arms stay open, with the inventory on the issue."An arm-by-arm audit found 30 match arms (not the rough "~22" estimate — the issue's own author said "I did not audit each one") that lower
args[0]thenargs[1](then, for several, discard the rest for side effects) with plainlower_exprand no rooting decision at all:Utf8Stream,EvalError/URIError,Uint8Array, the typed-array-view family,DataView,RegExp,EventEmitter,EventEmitterAsyncResource,SocketAddress,BroadcastChannel,Event,CustomEvent,DOMException,Console,StringDecoder, theReadable/Writable/Duplex/Transform/PassThroughfamily,LRUCache,DatabaseSync,StatementSync,Session,RateLimiterMemory,CronJob,AsyncResource,SuppressedError,WeakMap,WeakSet,TextDecoderStream,CompressionStream/DecompressionStream,ReadableStreamBYOBReader,CountQueuingStrategy/ByteLengthQueuingStrategy.WeakMap/WeakSetare a variant of the same bug rather than the textbook shape: the iterable argument was lowered (eagerly, via.map(lower_expr)), thenjs_weakmap_new/js_weakset_new— an unconditional allocation — ran, and only then was the iterable's now-possibly-stale register read back out.What changed
lower_builtin_newnow takes the caller'sRootedGroup(threaded in fromlower_new_impl_inner, which already opens one per #6969/#7699 — both oflower_builtin_new's call sites innew.rspass it through). Three small helpers do the adoption, following #7699's own stated discipline exactly — "adopt as the value is produced, never after the fact; rooting a finished list publishes an already-dangling argument 0, which is worse than not rooting at all":adopt_optional_arg— the one-operand-at-a-time primitive: lowersargs[idx]if present, rooted across everything fromargs[idx+1..].adopt_leading_arg_discard_rest— the single-leading-argument-plus-discard-loop shape (>12 arms).adopt_two_leading_args_discard_rest— the two-leading-arguments-plus-discard-loop shape (Event,CustomEvent,DOMException,Console,TextDecoderStream).CronJobneeded a bespoke ordering rather than the generic helpers: itscronTimeargument's raw-pointer derivation (js_get_string_pointer_unified) can itself allocate (SSO materialize), so it has to run beforeonTick/startare re-read from their slots, not after — otherwise the fix would trade one unrooted register for another.Explicitly out of scope, and why: the
extract_options_fields-based arms (Response,Request,Blob,File,Headers,ReadableStream,WritableStream,TransformStream) share the same underlying hazard — an earlier field's lowered value can sit unrooted across a later field's — but their per-property-match loop over a dynamicVec<(String, Expr)>is a structurally different shape from the fixedargs[0]/args[1]/args[2]sequence. Reusing these three helpers there isn't a good fit; it needs its own audit.Verification
Unit tests (
crates/perry-codegen/src/temp_root_coverage/builtin_ctor.rs, so they run in the per-PR--lib --binsgate, not the nightly-onlytests/*.rstier): six tests covering all three helper shapes plus theWeakMapvariant, throughperry_codegen::testing::temp_slots's codegen-contract assertions (same infrastructure #6969/#6983 used). Sabotage-confirmed: copied onto a cleanorigin/maincheckout, the five positive assertions fail against the pre-fix code (%r1 is never stored into a rooted slot, etc.); the paired negative gate (RegExpwith two non-allocating arguments) passes on both, so the positives aren't vacuously satisfied by a compiler that roots everything.Static checker (
scripts/gc_root_dominance_check.py), both lowerings:scripts/gc_root_dominance_corpus.sh,test_gap_gc_*/test_gap_new*/etc.) reads 0 violations before and after — it contains no source that constructs any of these built-ins, so it can't show a reduction either way.fresh(k)/"x" + churn(N), matching gc: constructor arguments (new C(a, b)) are not precise roots across the instance allocation #6969's ownnew Function(fresh(0), "return " + churn(N))) found the checker's actual reach: onnew DataView(fresh(1), "o"+churn(N), "l"+churn(N))andnew SuppressedError(fresh(3), "y"+churn(N), "z"+churn(N)), the middle argument's own producing call (js_string_concat_value) is itself inALLOC_RE, and the checker reports it stale.--stale-registers(shadow,PERRY_RS4GC=0): 2 → 0.--statepoints(native,PERRY_RS4GC=1+ productionrewrite-statepoints-for-gc): 2 unrooted → 0.RegExp,EventEmitter) read 0 both ways in this probe because their first argument comes from a plain user-function call (fresh(k)), whichALLOC_RE— by design, it matchesjs_*runtime symbols — doesn't recognize as a source. Their fix is the identical shape and is covered by the unit tests above.cargo test -p perry-codegen --no-fail-fast(full suite including the nightly-onlytests/*.rstier, which per-PR CI does not run): 6 pre-existing failures, identical on a cleanorigin/mainbaseline built in a separate worktree —large_local_array_push_inbounds_store_emits_precise_slot_barrier,proven_buffer_and_typed_array_reads_are_numeric_operands,reassigned_typed_array_store_records_runtime_fallback,integer_modulo::i32_counter_mod_unsafe_or_nonliteral_divisors_keep_frem,typed_f64_receiver_method_clone_raw_loads_after_composed_guards,integer_arithmetic_array_push_omits_inbounds_layout_note_and_barrier(tracked in #7708). No new failures.--lib --bins(the per-PR gate): 803 passed, 0 failed (797 pre-existing + 6 new).cargo fmt --all -- --checkandcargo clippy -p perry-codegen --lib --bins --no-depsare both clean on the touched files.Test plan
cargo test -p perry-codegen --lib --bins --no-fail-fast— 803 passedcargo test -p perry-codegen --no-fail-fast(full suite) — same 6 pre-existing failures as cleanorigin/main, no new onestemp_root_coverage::builtin_ctorscripts/gc_root_dominance_check.pybefore/after on a scoped probe, both lowerings (2→0)cargo fmt --all -- --checkcargo clippy -p perry-codegen --lib --bins --no-depsSummary by CodeRabbit
Bug Fixes
Tests