fix(gc): bind the catch parameter and the closure this/new.target slots, and snapshot the Object.assign string source - #7216
Conversation
|
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 (1)
📝 WalkthroughWalkthroughThe PR fixes moving-GC rooting for closure-captured ChangesGarbage-collection rooting
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
🚥 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
🧹 Nitpick comments (1)
test-files/test_gap_gc_closure_this_capture_rooting.ts (1)
36-41: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a
new.targetrooting regression.This test exercises captured
this, but it does not execute thecaptures_new_targetbranch. Add a closure that capturesnew.target, runschurn(), and reads the captured value after collection. Register that test intest-parity/gc_repsel_corpus.txt.Based on PR objectives: closure root slots cover both captured
thisandnew.target.🤖 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_gc_closure_this_capture_rooting.ts` around lines 36 - 41, Add a regression case alongside the existing closure factory in the test, where a returned closure captures new.target, invokes churn(), and reads the captured value after garbage collection; then register the new test in test-parity/gc_repsel_corpus.txt so it runs with the corpus.
🤖 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 `@test-files/test_gap_gc_assign_string_source_rooting.ts`:
- Around line 19-36: Update run and its Object.assign source-copy path so an
explicit moving-minor GC is triggered while object_assign_string_source still
holds the source view, rather than relying on repeated allocations to induce
collection. Place the trigger within the source-copy operation and preserve the
test’s existing assertions and per-iteration fresh-string setup.
---
Nitpick comments:
In `@test-files/test_gap_gc_closure_this_capture_rooting.ts`:
- Around line 36-41: Add a regression case alongside the existing closure
factory in the test, where a returned closure captures new.target, invokes
churn(), and reads the captured value after garbage collection; then register
the new test in test-parity/gc_repsel_corpus.txt so it runs with the corpus.
🪄 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: c44fe5b8-d7aa-43f8-9bf9-ab99bc9a64ae
📒 Files selected for processing (7)
crates/perry-codegen/src/codegen/closure.rscrates/perry-codegen/src/stmt/try_stmt.rscrates/perry-runtime/src/object/alloc.rstest-files/test_gap_gc_assign_string_source_rooting.tstest-files/test_gap_gc_catch_param_rooting.tstest-files/test_gap_gc_closure_this_capture_rooting.tstest-parity/gc_repsel_corpus.txt
| // LIVE BY CONSTRUCTION AND ONLY ON THE MOVING ARMS. The source is a FRESH heap | ||
| // string per iteration (built with `repeat`, well past the SSO limit), reachable | ||
| // only from a shadow-bound local — so it survives the minor, which means it | ||
| // MOVES. A non-moving collection leaves the bytes where they are and the borrow | ||
| // stays accidentally valid. | ||
|
|
||
| const ALPHA = "abcdefghijklmnopqrstuvwxyz"; | ||
| const WIDTH = 208; | ||
|
|
||
| function run(): string { | ||
| let badChar = 0; | ||
| let badCount = 0; | ||
| for (let r = 0; r < 300; r++) { | ||
| // Past SHORT_STRING_MAX_LEN, so this is a real heap `StringHeader` in the | ||
| // nursery rather than an inline short string copied into the caller's | ||
| // scratch buffer. | ||
| const src: string = ALPHA.repeat(8); | ||
| const out: any = Object.assign({}, src); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate test registration and any moving-GC configuration.
fd -a -t f 'run_gap_tests\.sh|run_parity_tests\.sh|gc_repsel_corpus\.txt' . | sort
rg -n -C4 \
'test_gap_gc_assign_string_source_rooting|PERRY_GC_MOVING_LOOP_POLLS|gc_repsel|moving' \
scripts test-parity test-files || trueRepository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the targeted test and its corpus registration around the relevant lines.
sed -n '1,80p' test-files/test_gap_gc_assign_string_source_rooting.ts
printf '\n--- corpus entries ---\n'
grep -n -C3 'test_gap_gc_assign_string_source_rooting\|requires=move\|cons_scan_off' test-parity/gc_repsel_corpus.txtRepository: PerryTS/perry
Length of output: 5064
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect similar GC rooting tests that already establish a moving-GC trigger.
sed -n '1,90p' test-files/test_gap_gc_spread_accessor_rooting.ts
sed -n '1,55p' test-files/test_gap_gc_inline_ctor_this_rooting.tsRepository: PerryTS/perry
Length of output: 4192
Make the GC trigger happen during the source copy.
This test relies on repeated allocation, but a non-moving minor leaves the source string bytes in place, so the alias can remain valid without a copy-time collection. Add an explicit moving-minor trigger that occurs while object_assign_string_source still holds the source view; otherwise regression coverage for this rooting bug depends on allocation pressure rather than the moving-GC root-liveness path.
🤖 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_gc_assign_string_source_rooting.ts` around lines 19 - 36,
Update run and its Object.assign source-copy path so an explicit moving-minor GC
is triggered while object_assign_string_source still holds the source view,
rather than relying on repeated allocations to induce collection. Place the
trigger within the source-copy operation and preserve the test’s existing
assertions and per-iteration fresh-string setup.
Verification updateRed-then-green on the final tree, all three armsRebuilt at the squashed commit (
Base ( Full runtime + codegen suites — no regressions
The three that stopped failing are Note on frame sizing (#7208)
|
Matrix — the first run was contended, and the number is not usableThe My idle gate was Targeted A/B instead — decisive, and it exonerates this PRRather than trust either number, all nine files were run standalone under the
Byte-identical on every one, exit code included. Whatever these nine are, this PR does not cause them and does not change them. An idle re-run is queued behind the other agent's build and will be posted when the box frees; the A/B above is the load-bearing evidence either way, since it holds base and fix to identical conditions rather than comparing a number against a different day's number. What the A/B exposed, which is more interesting than the FAIL count
|
1 participant
Fix-forward for #7207's review round plus the two next sites from its enumeration. Every one is reproducer-first; the one without a runtime witness says so in its own comment.
#7209 — the
catch (e)slot is RESERVED and never BOUNDcollect_pointer_typed_localsassigns the catch parameter an index (implicitlyAny, i.e. pointer-possible), sojs_shadow_frame_enter's count already included it.stmt/try_stmt.rsallocated the storage withalloca_entry, stored the exception, and never emittedjs_shadow_slot_bind— the frame was sized for a root that did not exist.Sharper than an ordinary missing root because
js_clear_exception()runs two lines earlier, dropping the runtime's own reference. From there the exception is reachable only through the unrooted alloca, and the catch body is arbitrary user code — so a precise-roots collection can sweep it, not merely move it.Fixed with
emit_shadow_slot_bind_for_local, which reuses the reserved index rather than growing the frame. Emitted after the store rather than hoisted to entry setup, deliberately: on the non-throwing path the alloca is never written, and an entry-hoisted bind would make the slot active over uninitialized stack bytes.#7208 — closures reserve no slot for captured
this/new.targetmethod.rsthen binds that slot; closures had no index to bind. Both slots areblk.alloca(DOUBLE)holding a heap receiver for the whole body, read by everyctx.this_stack.last()consumer. The in-tree comment claiming the capture reads are "exempt … they run in the entry-block prologue, ahead of any statement that could collect" justifies the timing of the read; it says nothing about the lifetime of the slot.Now reserves one index per captured
this/new.targetand binds each inline in the entry block, right after its store.#7214 — the
Object.assignstring source was a raw borrowCodeRabbit Critical on #7207.
str_bytes_from_jsvaluehands back a pointer into the sourceStringHeader(header + payload are one contiguousarena_alloc_gcblock;GC_TYPE_STRINGismovable), and its own safety note forbids holding it across a GC cycle. The loop held it across three allocation points per character. #7207 rooted the target, key and value on that arm and missed the source, because the source is a borrow rather than a JSValue in that frame.Snapshotted once before the loop. Full evidence and the measurement in #7215.
Verification
Both defaults, on the pinned host, hashed binaries. Base is
origin/main1679e22b4.test_gap_gc_closure_this_capture_rooting,POLLS=1bad 05/5bad 4, 3/3bad 03/3test_gap_gc_catch_param_rooting,POLLS=1message 8 field 8, 3/3bad churn 0 message 0 field 05/5test_gap_gc_assign_string_source_rootingnode --experimental-strip-types26.5.1#7214 has no runtime witness and the fix comment says so. An instrumented build rooted both the source and the target inside the loop and counted relocations:
chars=26001 src_moves=0 tgt_moves=1. The target moved — so collections genuinely happen in that function — but that source could not, because at 26 KB it is overLARGE_OBJECT_THRESHOLD_BYTESand is born tenured in the non-moving old generation. Under the threshold it is movable but one call allocates too little to reliably span a collection; four shapes across four arm configurations (up to 120 cycles) stayed clean. The exposure is the band just under 16 KiB, and the margin rests on a tunable constant.Both new gap files are registered in
test-parity/gc_repsel_corpus.txt, with a note thatcatch_param_rootingis the one corpus member that can also go wrong under a non-moving precise-roots collection (the sweep case), so it belongs oncons_scan_offand not only on therequires=movearms.Refs #7209, #7208, #7215, #7202, #7207, #7154, #7161.
Summary by CodeRabbit
Bug Fixes
Object.assignoperations involving longer strings.thisvalues orcatchexception parameters to become invalid during memory cleanup.Tests