fix(gc): root the generator instance across its own prototype wiring (#7577) - #7584
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughGenerator prototype and async-generator wrapper code now roots heap objects and re-reads relocated pointers across allocating operations. Runtime-root tests and TypeScript stress tests cover synchronous and asynchronous generator attachment under copying GC. ChangesGenerator prototype GC rooting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GeneratorAttachment
participant RuntimeHandleScope
participant CopyingMinorGC
participant AsyncGeneratorQueue
GeneratorAttachment->>RuntimeHandleScope: root receiver and closures
GeneratorAttachment->>CopyingMinorGC: allocate prototype and wrappers
CopyingMinorGC-->>RuntimeHandleScope: relocate rooted objects
GeneratorAttachment->>RuntimeHandleScope: re-read current pointers
GeneratorAttachment->>AsyncGeneratorQueue: install wrappers with current pointers
AsyncGeneratorQueue-->>GeneratorAttachment: return current receiver
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 3
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/global_this/generator.rs (1)
523-546: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftThe closure operand in
js_generator_attach_closure_prototypeis not rooted. The receiver is rooted throughobj_h, but theclosurelocal remains a raw*const ClosureHeaderacrosswrap_async_generator_instance, which allocates three closures and three keys-array transitions. A raw Rust local is not a GC root and the collector does not rewrite it, sogenerator_function_prototype_ofcan receive a pre-move closure address and cacheg.prototypeon a dead closure. This one gap explains the missing test coverage and the overstated changelog claim.
crates/perry-runtime/src/object/global_this/generator.rs#L523-L546: rootclosurein the existingRuntimeHandleScopeafter Line 523 and re-read it from the handle before thegenerator_function_prototype_ofcall at Line 544.crates/perry-runtime/src/gc/tests/runtime_roots/generator_attach_prototype.rs#L133-L155: register the test function withjs_register_closure_async_generator_functionand install ownnext,return, andthrowclosures on the receiver, sowrap_async_generator_instanceruns under the armed collection and the closure-movement window is exercised.changelog.d/7584-generator-attach-prototype-rooting.md#L26-L30: correct the sentence "Every pointer now comes back out of aRuntimeHandleScopehandle after the call that could have moved it" once the closure operand is rooted, or scope the claim to the pointers that are actually rooted.🤖 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/global_this/generator.rs` around lines 523 - 546, Root the closure operand in the existing RuntimeHandleScope within crates/perry-runtime/src/object/global_this/generator.rs#L523-L546, then re-read it from the handle after wrap_async_generator_instance and before generator_function_prototype_of. In crates/perry-runtime/src/gc/tests/runtime_roots/generator_attach_prototype.rs#L133-L155, register the function with js_register_closure_async_generator_function and install own next, return, and throw closures to exercise the movement window. In changelog.d/7584-generator-attach-prototype-rooting.md#L26-L30, revise the claim about every pointer being re-read from rooted handles to match the corrected implementation.
🧹 Nitpick comments (1)
crates/perry-runtime/src/gc/tests/runtime_roots/generator_attach_prototype.rs (1)
91-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the recorded prototype's identity, not only its address class.
The check confirms that the recorded
[[Prototype]]is a plausible heap address. It does not confirm that the address is the correct prototype object. A link to any surviving heap object passes.Compare the recorded value against the expected prototype. For
js_generator_attach_prototypethat is the intermediate whose own prototype isgenerator_prototype_ptr(false). Forjs_generator_attach_closure_prototypethat is the value returned bygenerator_function_prototype_offor the same closure. Passing the expected address into the helper would close the gap.Using
crate::value::addr_class::is_plausible_heap_addrfor the address-class check is correct and matches the canonical predicate.🤖 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/tests/runtime_roots/generator_attach_prototype.rs` around lines 91 - 103, Update the prototype-recording assertion helper around object_static_prototype to accept and compare an expected prototype address, not just validate heap-address plausibility. Pass the intermediate whose own prototype is generator_prototype_ptr(false) from js_generator_attach_prototype, and pass generator_function_prototype_of for the same closure from js_generator_attach_closure_prototype; retain the existing is_plausible_heap_addr check.Source: Learnings
🤖 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/tests/runtime_roots/generator_attach_prototype.rs`:
- Around line 37-46: Update warm_generator_intrinsics to call the generator
intrinsic tower builder directly, using the visible ensure_generator_intrinsics
or equivalent builder symbol, instead of passing TAG_UNDEFINED to
js_generator_attach_prototype. Confirm the builder’s visibility from the test
module and preserve the helper’s purpose of constructing the tower before the
allocation-triggered call under test.
- Around line 133-155: Add async coverage alongside
attach_closure_prototype_survives_a_copying_minor_inside_the_call: register the
fake function with js_register_closure_async_generator_function, install own
next, return, and throw closures on the receiver, and exercise
js_generator_attach_closure_prototype with collection armed so
wrap_async_generator_instance runs. Add the corresponding is_async = 1 case for
js_generator_attach_prototype while preserving the existing synchronous test.
In `@test-files/test_gap_7577_generator_prototype_rooting.ts`:
- Around line 31-34: Update the prototype checks around the generator examples:
store two separate reads of small.prototype in distinct bindings before
comparing them, and store the results of two separate small(1) calls in distinct
bindings before comparing their prototypes. Preserve both identity checks while
avoiding direct self-comparisons flagged by Biome.
---
Outside diff comments:
In `@crates/perry-runtime/src/object/global_this/generator.rs`:
- Around line 523-546: Root the closure operand in the existing
RuntimeHandleScope within
crates/perry-runtime/src/object/global_this/generator.rs#L523-L546, then re-read
it from the handle after wrap_async_generator_instance and before
generator_function_prototype_of. In
crates/perry-runtime/src/gc/tests/runtime_roots/generator_attach_prototype.rs#L133-L155,
register the function with js_register_closure_async_generator_function and
install own next, return, and throw closures to exercise the movement window. In
changelog.d/7584-generator-attach-prototype-rooting.md#L26-L30, revise the claim
about every pointer being re-read from rooted handles to match the corrected
implementation.
---
Nitpick comments:
In
`@crates/perry-runtime/src/gc/tests/runtime_roots/generator_attach_prototype.rs`:
- Around line 91-103: Update the prototype-recording assertion helper around
object_static_prototype to accept and compare an expected prototype address, not
just validate heap-address plausibility. Pass the intermediate whose own
prototype is generator_prototype_ptr(false) from js_generator_attach_prototype,
and pass generator_function_prototype_of for the same closure from
js_generator_attach_closure_prototype; retain the existing
is_plausible_heap_addr check.
🪄 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: d003a491-e958-4a76-ab74-c0d54474c742
📒 Files selected for processing (6)
changelog.d/7584-generator-attach-prototype-rooting.mdcrates/perry-runtime/src/gc/tests/runtime_roots.rscrates/perry-runtime/src/gc/tests/runtime_roots/generator_attach_prototype.rscrates/perry-runtime/src/object/async_generator_queue.rscrates/perry-runtime/src/object/global_this/generator.rstest-files/test_gap_7577_generator_prototype_rooting.ts
| /// Build the generator intrinsic tower up front. It is lazily constructed on | ||
| /// the first `generator_prototype_ptr` call and costs dozens of allocations; | ||
| /// paying it here keeps the call under test down to its own two, so the | ||
| /// injected trigger lands where we intend. | ||
| fn warm_generator_intrinsics() { | ||
| let _ = crate::object::js_generator_attach_prototype( | ||
| f64::from_bits(crate::value::TAG_UNDEFINED), | ||
| 0, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
warm_generator_intrinsics does not warm anything.
The helper calls js_generator_attach_prototype with TAG_UNDEFINED. That value fails the !jv.is_pointer() guard at the top of js_generator_attach_prototype, so the function returns immediately. It never reaches generator_prototype_ptr, so the intrinsic tower is never built.
The doc at Lines 37-40 states that warming "keeps the call under test down to its own two" allocations. That premise does not hold. The tower is still built lazily inside the call under test, which adds many allocations before the intended window. The armed trigger then fires during tower construction, not in the window the module doc describes at Lines 21-24.
Call the intrinsic builder directly instead.
💚 Proposed fix to actually build the tower
fn warm_generator_intrinsics() {
- let _ = crate::object::js_generator_attach_prototype(
- f64::from_bits(crate::value::TAG_UNDEFINED),
- 0,
- );
+ // Force the lazy tower build for both brands before the trigger is armed.
+ let _ = crate::object::global_this::generator::generator_prototype_ptr(false);
+ let _ = crate::object::global_this::generator::generator_prototype_ptr(true);
}Confirm the visibility of the builder from the test module:
#!/bin/bash
# Locate the intrinsic-tower entry points and their visibility.
rg -nP -C 3 'fn\s+(ensure_generator_intrinsics|generator_prototype_ptr)\s*\(' --type=rust
rg -nP -C 3 '\b(pub(\(crate\))?\s+)?mod\s+generator\b' --type=rust -g '**/global_this/**'🤖 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/tests/runtime_roots/generator_attach_prototype.rs`
around lines 37 - 46, Update warm_generator_intrinsics to call the generator
intrinsic tower builder directly, using the visible ensure_generator_intrinsics
or equivalent builder symbol, instead of passing TAG_UNDEFINED to
js_generator_attach_prototype. Confirm the builder’s visibility from the test
module and preserve the helper’s purpose of constructing the tower before the
allocation-triggered call under test.
| console.log("B stable:", small.prototype === small.prototype); | ||
|
|
||
| // Every instance of the same generator function shares one `g.prototype`. | ||
| console.log("C shared:", Object.getPrototypeOf(small(1)) === Object.getPrototypeOf(small(1))); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the self-comparison at Line 31 and suppress the false positive at Line 34.
Biome reports lint/suspicious/noSelfCompare on both lines.
Line 31 is a genuine self-comparison. small.prototype === small.prototype is always true in any conforming engine and an optimizer may fold it. It does not test identity stability across reads. Capture two separate reads into bindings.
Line 34 is a false positive. The two small(1) calls produce distinct generator instances, so the comparison is meaningful. Bind the two results to make the intent explicit and to satisfy the rule.
💚 Proposed fix
// `g.prototype` identity is stable across reads.
-console.log("B stable:", small.prototype === small.prototype);
+const protoRead1 = small.prototype;
+const protoRead2 = small.prototype;
+console.log("B stable:", protoRead1 === protoRead2);
// Every instance of the same generator function shares one `g.prototype`.
-console.log("C shared:", Object.getPrototypeOf(small(1)) === Object.getPrototypeOf(small(1)));
+const shared1 = Object.getPrototypeOf(small(1));
+const shared2 = Object.getPrototypeOf(small(1));
+console.log("C shared:", shared1 === shared2);📝 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.
| console.log("B stable:", small.prototype === small.prototype); | |
| // Every instance of the same generator function shares one `g.prototype`. | |
| console.log("C shared:", Object.getPrototypeOf(small(1)) === Object.getPrototypeOf(small(1))); | |
| // `g.prototype` identity is stable across reads. | |
| const protoRead1 = small.prototype; | |
| const protoRead2 = small.prototype; | |
| console.log("B stable:", protoRead1 === protoRead2); | |
| // Every instance of the same generator function shares one `g.prototype`. | |
| const shared1 = Object.getPrototypeOf(small(1)); | |
| const shared2 = Object.getPrototypeOf(small(1)); | |
| console.log("C shared:", shared1 === shared2); |
🧰 Tools
🪛 Biome (2.5.6)
[error] 31-31: This comparison uses the same expression on both sides.
(lint/suspicious/noSelfCompare)
[error] 34-34: This comparison uses the same expression on both sides.
(lint/suspicious/noSelfCompare)
🤖 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 `@test-files/test_gap_7577_generator_prototype_rooting.ts` around lines 31 -
34, Update the prototype checks around the generator examples: store two
separate reads of small.prototype in distinct bindings before comparing them,
and store the results of two separate small(1) calls in distinct bindings before
comparing their prototypes. Preserve both identity checks while avoiding direct
self-comparisons flagged by Biome.
Source: Linters/SAST tools
…7577) `js_generator_attach_prototype` bound the receiver's address at function entry and used it at the tail, with four allocating calls in between. Same shape in `js_generator_attach_closure_prototype`, `generator_function_prototype_of`, and `wrap_async_generator_instance` / `set_method` below them. This frame owns the only reference. Codegen drops the caller's root immediately before the call: %r67 = bitcast i64 %r66 to double store ptr addrspace(1) null, ptr %r28 ; the caller's root, dropped %r68 = call double @js_generator_attach_prototype(double %r67, i32 0) which is the ordinary contract — a runtime helper roots its own arguments — except this one did not. And every call it makes allocates: `wrap_async_generator_instance` (three closures, three field sets), `generator_prototype_ptr` (lazily builds the whole generator intrinsic tower on first call), `js_object_alloc`, and `object_set_static_prototype` itself, whose `object_meta_ensure` mints the object's meta record out of the arena. A copying minor in any of those windows produced two wrong answers with no diagnostic: the `[[Prototype]]` link was recorded against the PRE-MOVE address, so `Object.getPrototypeOf(gen())` on the live object found nothing; and the function RETURNED that pre-move address, so the caller's generator was a dangling pointer into retired from-space. That is #7577's SIGBUS; without the instruments it is a wrong answer and exit 0. Every pointer is now re-read from a `RuntimeHandleScope` handle after the call that could have moved it. `generator_prototype_ptr` is deliberately called a second time rather than cached — it reads a GC-rooted atomic the collector rewrites, so a fresh call IS the re-read. `make_method_wrapper` is deleted rather than fixed: its signature was the defect. Taking `original` as a parameter forces every caller to bind that pointer before the `js_closure_alloc` inside, so the capture store writes a pre-collection address. The one caller now allocates first and reads `original` out of its handle, which leaves no correct way to call the helper. Reproduced DETERMINISTICALLY rather than under zeal. The issue's end-to-end reproducer needs a moving collection to land in a window that no safepoint reaches, so it does not fault reliably (measured: instrument live — `mode=ProtectPages retired_set=#0 blocks=2 bytes_protected=2097152` — and `copied_objects` up to 6053, but no fault). Instead the new tests inject the collection exactly where it belongs: `force_next_general_arena_alloc_slow()` + `GcTriggerThresholdTestGuard::make_arena_trigger_due()` make the next arena block allocation collect, and the next one is the callee's own. Pre-fix, both assertions fire; the receiver moved 0x20001280008 → 0x200014c0008 and the function returned 0x20001280008. Coverage: * `crates/perry-runtime/src/gc/tests/runtime_roots/generator_attach_prototype.rs` — two `cargo test -p perry-runtime` tests, one per entry point. Each asserts its subject was live (the receiver actually moved) before asserting anything else, per CLAUDE.md's "a gate must assert its subject was live"; each calls `register_runtime_handle_root_scanner_for_tests()`, without which the `RuntimeHandleScope` under test is decorative and the test would pass for the wrong reason. * `test-files/test_gap_7577_generator_prototype_rooting.ts` — the observable half (`Object.getPrototypeOf(gen())`, its identity, and its stability over 4000 sync + 200 async constructions with allocation churn), byte-identical to `node --experimental-strip-types` 26.5.1, and clean under `PERRY_GC_MOVING_LOOP_POLLS=1 PERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800`. No new runtime cache of a heap pointer, so no `gc_register_mutable_root_scanner` entry is needed; the handles are covered by the existing runtime-handle scanner. Sabotage-verified: restoring the entry-bound `obj_ptr` in both functions turns both tests red on "must return the receiver's CURRENT address".
1dcb551 to
dcc039d
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Audit before merge — verified, merged as v0.5.1331Sabotage-verified the fix myself: replacing the tail
Full runtime suite 1836 passed / 0 failed. All four The fix is the right shape for this family — root once, then re-read from the One gap, found by sabotaging the wrong function firstMy first sabotage attempt patched So of the functions this PR fixes — Two things from the report worth keepingThe issue's own reproducer cannot reproduce. Its program discards
Why that frame owns the only reference: codegen nulls the caller's |
1 participant
Closes #7577.
Root cause: an ordering bug, not a missing root
js_generator_attach_prototype(crates/perry-runtime/src/object/global_this/generator.rs:381)bound the receiver's address at function entry and used it at the tail:
This frame owns the only reference. Codegen drops the caller's root
immediately before the call — from the emitted IR for the issue's own
reproducer, in
perry_fn_..._small, the frame the fault report names:That is the ordinary contract (a runtime helper roots its own arguments) and
this one did not. Every call it makes allocates:
wrap_async_generator_instancejs_object_set_field_by_namekeys-array transitionsgenerator_prototype_ptrjs_object_allocobject_set_static_prototypeobject_meta_ensuremints the object's meta record out of the arenaA copying minor in any of those windows gave two wrong answers with no
diagnostic:
[[Prototype]]link was recorded against the pre-move address, soObject.getPrototypeOf(gen())on the live object found nothing;a dangling pointer into retired from-space.
That is #7577's SIGBUS. Without the instruments it is a wrong answer and exit 0,
which is the expensive half.
Same shape, same fix, in three more places reachable from that call:
js_generator_attach_closure_prototype,generator_function_prototype_of, andwrap_async_generator_instance/set_methodbelow them. Fixing only the namedfunction would have handed a freshly re-read receiver to a callee that lets it
go stale again.
Every pointer is now re-read from a
RuntimeHandleScopehandle after thecall that could have moved it.
generator_prototype_ptris deliberately calleda second time rather than cached: it reads a GC-rooted atomic the collector
rewrites, so a fresh call is the re-read.
make_method_wrapperis deleted rather than fixed. Its signature was thedefect — taking
originalas a parameter forces every caller to bind thatpointer before the
js_closure_allocinside, so the capture store writes apre-collection address. Its one caller now allocates the wrapper first and reads
originalout of its handle afterwards, which leaves no correct way to call thehelper; per CLAUDE.md's kill-policy the losing shape stops compiling rather than
waiting for a future caller.
No new runtime cache of a heap pointer, so no
gc_register_mutable_root_scannerentry is required — the handles are covered by the existing runtime-handle
scanner.
Reproduction: deterministic, not zeal-dependent
The issue's end-to-end reproducer does not fault reliably on
mainas of1e971d2, and I want to be explicit about that rather than claim a repro I did
not get. A moving collection needs a safepoint, and there is none inside
js_generator_attach_prototype— allocation-triggered minors force aconservative stack scan, which makes the copying minor ineligible. Measured, with
the instrument proven live:
(The issue's own version of the program is worse than that:
churn(i)'s resultis discarded and dead-code-eliminated, so every minor reports
copied_objects=0— the instrument is live and the collector is movingnothing. The
--pressurefailure mode from #7024, in miniature.)So the tests here inject the collection into the window instead of hoping
one lands there.
force_next_general_arena_alloc_slow()+GcTriggerThresholdTestGuard::make_arena_trigger_due()arm the next arena blockallocation to collect, and the next one is the callee's own. Pre-fix, measured:
The receiver moved during the call; the link went to the dead address and the
dead address came back.
Coverage
crates/perry-runtime/src/gc/tests/runtime_roots/generator_attach_prototype.rs— two
cargo test -p perry-runtimetests, one per entry point. Two details thatare load-bearing rather than decorative:
asserting anything else, per CLAUDE.md's "a gate must assert its subject was
live" — a run in which nothing moved says so instead of passing;
register_runtime_handle_root_scanner_for_tests(). Without it theRuntimeHandleScopeinside the function under test is decorative(
CopyingNurseryTestGuardmem::takes the thread's mutable-root scanners),and I hit exactly that: the first run of the fixed code still reported
ret_is_current=false, and the fix looked absent.test-files/test_gap_7577_generator_prototype_rooting.ts— the observablehalf:
Object.getPrototypeOf(gen()), its identity across reads, and itsstability over 4000 sync + 200 async constructions with allocation churn.
Byte-identical to
node --experimental-strip-typeson the pinned 26.5.1, andclean under
PERRY_GC_MOVING_LOOP_POLLS=1 PERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800with theinstrument printing
mode=ProtectPages retired_set=#0 blocks=2 bytes_protected=2097152andcopied_objectsup to 6053.Sabotage evidence
Restoring the entry-bound
obj_ptrin both functions (the pre-#7577 shape)turns both tests red on the precise defect:
Validation
Local; CI has a deep backlog and has not reported.
cargo test -p perry-runtime --no-fail-fast— 1820 passed, 0 failed.against Node: 24/24 pass. (A 25th,
test_gap_iterator_helpers_2874, is astanding known failure fixed by fix(runtime): give iterator helpers their own class id (#7576) #7583 on a separate branch.)
python3 scripts/raw_handle_debt.py→ 998 (baseline 998) — the fix usesNaN-boxed handles throughout, so the ledger does not move.
scripts/check_file_size.sh→ OK.cargo fmt --all -- --check→ clean.python3 scripts/addr_class_inventory.py→ fails, pre-existing: it flagscrates/perry-runtime/src/iter_result.rs:139(agcheader-castintroduced byperf(iterator): build one object per .next(), not five (#7564) #7579), and I verified it fails identically on a clean tree at
origin/main1e971d2. Not touched here. It needs a mutable header write, so
try_read_gc_header(which returns&'static) is not a drop-in — it wants anallowlist entry or a
gc_flagssetter.Summary by CodeRabbit
Bug Fixes
Tests