gc: root a closure's own this_closure pointer on the shadow stack (#7055) - #7065
Merged
Conversation
…7055) A closure body reaches its captures through `js_closure_get_capture_bits(%this_closure, idx)`. `%this_closure` is an LLVM parameter — a register value no root enumeration can see — and the shipped default runs an evacuating young collection at loop back-edge polls (`js_gc_loop_safepoint`) with precise roots and no conservative native-stack scan. A closure relocated while its own body is on the stack therefore left that register pointing into from-space, which the same cycle resets and hands straight back to the mutator: the next capture access read a foreign object's `capture_count`, judged the index out of range and returned 0, so every later boxed-capture read yielded `undefined` and every write was silently dropped. In an `async fn` the casualty was the generator state machine's own `__gen_state` box, so the `state = <next>` store at the end of a resumed state body went nowhere and the following `await` resumed into the state it had just finished — replaying one loop iteration and folding its contribution into the accumulator twice. Closure bodies with captures now spill `%this_closure` into a NaN-boxed entry alloca bound to a shadow-stack slot, and every capture access reloads the pointer from that slot, so the collector rewrites it like any other precise root. Capture-less closures are unaffected: they emit no capture access, so they keep their frame-free prologue.
📝 WalkthroughWalkthroughClosure code generation now roots captured closures’ ChangesClosure pointer rooting and access
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant compile_closure
participant shadow_stack
participant capture_access
participant relocating_gc
compile_closure->>shadow_stack: bind NaN-boxed this_closure root
capture_access->>shadow_stack: reload current closure pointer
relocating_gc->>shadow_stack: update rooted pointer during evacuation
capture_access->>shadow_stack: use relocated closure for capture access
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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
🤖 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/codegen/closure.rs`:
- Around line 1114-1115: Update one_capture_closure_ir() to mark the closure
mutable, include capture index 0 in mutable_captures, and emit a captured
LocalSet(0, ...) in its body while retaining the existing capture writer absence
assertion. Keep the fixture focused on exercising mutable boxed-capture writes.
In `@crates/perry-codegen/src/expr/literals_vars.rs`:
- Line 706: Update the captured-local numeric update flow around
current_closure_ptr_value, js_to_numeric, and js_numeric_step so both pointers
are refreshed after coercion/stepping: reload closure_ptr immediately before the
final capture access, then re-fetch box_ptr from that fresh closure before
calling js_box_set_bits.
In `@crates/perry/tests/gc_closure_self_pointer_root_7055.rs`:
- Around line 132-137: Update the command setup in the arm execution loop to
remove inherited PERRY_GEN_GC settings before applying each arm’s environment
overrides. Ensure the default and nursery-capacity arms cannot be skipped or
altered by the test runner environment, while preserving each arm’s explicit
settings from arm.
🪄 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: ff4a6d7e-e417-4c69-a260-19e0f78de09b
📒 Files selected for processing (15)
changelog.d/7065-closure-self-pointer-gc-root.mdcrates/perry-codegen/src/codegen/arguments.rscrates/perry-codegen/src/codegen/closure.rscrates/perry-codegen/src/codegen/entry.rscrates/perry-codegen/src/codegen/function.rscrates/perry-codegen/src/codegen/method.rscrates/perry-codegen/src/expr/array_push.rscrates/perry-codegen/src/expr/closure.rscrates/perry-codegen/src/expr/instance_misc1.rscrates/perry-codegen/src/expr/literals_vars.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/shadow_slot.rscrates/perry-codegen/src/expr/static_field_meta.rscrates/perry-codegen/src/lower_array_method.rscrates/perry/tests/gc_closure_self_pointer_root_7055.rs
CodeRabbit review of 84ce786, three findings. **Major (`literals_vars.rs`), valid in part.** In the captured-`Update` arm the *unboxed* branch reused `closure_ptr` after `coerce_old` / `step_new`, which emit `js_to_numeric` / `js_numeric_step`. The first of those runs a user `valueOf` — arbitrary JS that can reach a `js_gc_loop_safepoint` and relocate this closure — and `js_closure_set_capture_bits` does NOT validate its pointer, so the write would store into whatever the mutator had since put at that recycled from-space address. The pointer is now re-read from the rooted slot before the write, only on the coercing path. The other half of the finding — "the boxed path also retains a stale `box_ptr`" — is not a hazard. A box is `std::alloc::alloc`'d by `js_box_alloc_bits`, is never freed and is never relocated (`scan_box_roots_mut` rewrites the JSValue *inside* the box, not its address), so an address read before a collection still names the same live cell after it. Documented at the site instead of adding a redundant re-fetch. **Minor (fixture vacuity), valid.** The IR fixture only emitted `LocalGet`, so its negative assertion on the capture *writer* could not fail for the reason it claimed. It now emits a captured `LocalSet` and a captured `x++`, and asserts positively that every capture access reloads the rooted slot. A second fixture reaches the unboxed writer (the arm the Major fix touches) by omitting the capture's declaration site, which is what keeps `collect_boxed_vars` from boxing it. **Minor (inherited env), valid.** `Command` inherits the test runner's environment, so an exported `PERRY_GEN_GC=0` would have turned every arm of the e2e suite into the never-relocates control and let it pass against the unfixed compiler. The whole collector-knob family is now cleared before each arm applies its own settings.
Contributor
Author
Review round 1 (CodeRabbit on
|
| test | sabotage | result |
|---|---|---|
closure_body_roots_…_reads_captures_through_it |
drop the rooted slot | no shadow-framed closure body in IR → FAILED |
closure_body_roots_…_reads_captures_through_it |
read the raw %this_closure |
capture reads must reload … from its rooted slot → FAILED |
unboxed_capture_write_reloads_the_closure_pointer_after_the_coercion |
drop the post-coercion reload | must re-read the closure pointer from its rooted slot %r3 after js_to_numeric → FAILED |
relocating_minor_does_not_replay_an_async_loop_iteration (e2e) |
either codegen sabotage | calls:151 vs calls:150 → FAILED |
cargo test -p perry-codegen --lib: 284 passed, 0 failed. e2e suite green over all 10 arms. cargo fmt --all -- --check clean.
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/codegen/closure.rs`:
- Around line 1325-1357: Update the reload assertions around the slot checks and
the corresponding checks near the later capture-access validation to match the
complete SSA slot token, not raw prefixes; require the slot reference to end at
the next non-digit character so `%5` cannot match `%52`. Preserve the existing
count and post-`js_to_numeric` validation behavior while applying the same
boundary-aware matching in both locations.
🪄 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: 20266d33-75c3-4082-bf84-2cf15bfdbfb9
📒 Files selected for processing (3)
crates/perry-codegen/src/codegen/closure.rscrates/perry-codegen/src/expr/literals_vars.rscrates/perry/tests/gc_closure_self_pointer_root_7055.rs
Comment on lines
+1325
to
+1357
| let slot = { | ||
| let bind = body | ||
| .find("@js_shadow_slot_bind(") | ||
| .map(|i| &body[i..]) | ||
| .and_then(|t| t.split_once("ptr ")) | ||
| .map(|(_, rest)| rest) | ||
| .unwrap_or_else(|| panic!("no shadow-slot bind in body:\n{body}")); | ||
| bind.split(')').next().expect("bind operand").to_string() | ||
| }; | ||
| let reload = format!("load i64, ptr {slot}"); | ||
| assert!( | ||
| body.matches(reload.as_str()).count() >= accesses, | ||
| "every one of the {accesses} capture accesses must reload the \ | ||
| closure pointer from its rooted slot {slot}; body:\n{body}" | ||
| ); | ||
|
|
||
| // #7055 (CodeRabbit): the sharp case — `js_to_numeric` runs a user | ||
| // `valueOf`, i.e. arbitrary JS that can reach a loop poll and relocate | ||
| // this closure. The capture access that FOLLOWS it must come from a | ||
| // fresh load, never from a pointer materialized before the call. | ||
| let coerce_at = body.find("@js_to_numeric").expect("coercion call"); | ||
| let next_access = body[coerce_at..] | ||
| .find("@js_closure_get_capture_bits(") | ||
| .map(|i| coerce_at + i) | ||
| .unwrap_or_else(|| { | ||
| panic!("fixture must access the capture after the coercion; body:\n{body}") | ||
| }); | ||
| assert!( | ||
| body[coerce_at..next_access].contains(reload.as_str()), | ||
| "a capture access after `js_to_numeric` (which can run a user \ | ||
| `valueOf` and relocate the closure) must re-read the closure \ | ||
| pointer from its rooted slot {slot}; body:\n{body}" | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)closure\.rs$|perry-codegen/src/codegen/closure.rs' || true
echo "== around target lines =="
wc -l crates/perry-codegen/src/codegen/closure.rs
sed -n '1280,1435p' crates/perry-codegen/src/codegen/closure.rs | nl -ba -v1280
echo "== search match patterns =="
rg -n 'uses i64, ptr|`@js_shadow_slot_bind`|`@js_to_numeric`|`@js_closure_get_capture_bits`|matches|contains\(reload|count\(\) >= accesses' crates/perry-codegen/src/codegen/closure.rsRepository: PerryTS/perry
Length of output: 433
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target block =="
sed -n '1280,1435p' crates/perry-codegen/src/codegen/closure.rs
echo "== exact match strings in target file =="
rg -n '`@js_shadow_slot_bind`\(|`@js_to_numeric`|`@js_closure_get_capture_bits`\(|load i64, ptr|\.matches\(|\.contains\(reload|assertions' crates/perry-codegen/src/codegen/closure.rs || trueRepository: PerryTS/perry
Length of output: 7976
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
def occurrences(haystack, needle):
count = 0
start = 0
while True:
idx = haystack.find(needle, start)
if idx == -1:
break
count += 1
start = idx + 1
return count
def boundary_count(haystack, needle):
hbytes = haystack.encode()
nlen = len(needle)
count = 0
start = 0
while True:
idx = haystack.find(needle, start)
if idx == -1:
break
if idx + nlen >= len(haystack) or not haystack[idx + nlen].isdigit():
count += 1
start = idx + 1
return count
cases = [
("load i64, ptr %5 next", "load i64, ptr %5", 1, 1),
("load i64, ptr %52 next\nload i64, ptr %5", "load i64, ptr %5", 2, 1),
("load i64, ptr %5x\nload i64, ptr %5", "load i64, ptr %5", 2, 1),
("no instruction", "load i64, ptr %5", 0, 0),
]
for body, needle, _raw_expected, _boundary_expected in cases:
print(repr(body), needle, occurrences(body, needle), boundary_count(body, needle))
PY
echo "== IR generator and fixture context =="
rg -n 'fn unboxed_capture_update_ir|fn .*closure.*ir|`@js_shadow_slot_bind`|`@load` i64|perry_closure|shadow_slot_bind|load i64, ptr' crates/perry-codegen/src/codegen/closure.rs crates/perry-codegen -g '!target'Repository: PerryTS/perry
Length of output: 22249
Apply a trailing boundary to reload checks.
load i64, ptr {slot} currently matches longer SSA names that share the same numeric prefix, e.g. %52 when slot is %5; the same concern applies at lines 1394-1417. Match up to the next non-digit character instead of using raw substring search for these assertions.
🤖 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/codegen/closure.rs` around lines 1325 - 1357, Update
the reload assertions around the slot checks and the corresponding checks near
the later capture-access validation to match the complete SSA slot token, not
raw prefixes; require the slot reference to end at the next non-digit character
so `%5` cannot match `%52`. Preserve the existing count and post-`js_to_numeric`
validation behavior while applying the same boundary-aware matching in both
locations.
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
1 participant
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 #7055.
The defect
A closure body reaches its captured variables through
js_closure_get_capture_bits(%this_closure, idx).%this_closureis the LLVMparameter the caller passed — a register value no root enumeration can see.
The shipped default runs an evacuating young collection at loop back-edge polls
(
js_gc_loop_safepoint) with PRECISE roots and no conservative native-stackscan. So when a closure is relocated while its own body is on the stack, every
root the collector knows about is rewritten and that register is not. From-space
is reset at the end of the same cycle and handed straight back to the mutator,
so the next capture access reads a different object's header:
js_closure_get_capture_bitsfindsidx >= real_capture_count(...)and returns0. Pointer 0 is not a registered box, so from that instant on every
boxed-capture read in the body yields
undefinedand every boxed-capture writeis silently dropped (
js_box_set/js_i32_box_setno-op on an unregisteredpointer).
In an
async fnthe casualty is the generator state machine itself. Theasync-to-generator transform boxes every body local, including
__gen_state, sothe
state = <next>store at the end of a resumed state body went nowhere. Thefollowing
awaitthen resumed into the state it had just finished and ran oneloop iteration twice — folding one request's contribution into the accumulator
a second time. That is #7055's signature exactly: a deterministic checksum error
of fixed magnitude, identical at 150 / 300 / 600 / 1200 requests, present only
when
copied_objects > 0.Minimal reproducer
Down from the issue's ~60-line
w5_srv_scale.tsto 11 lines.callsmust be 3:It needs neither
setImmediatenorJSON.stringifynorargv— only anasync fnwhose loop body allocates objects (w6, the same program pushingnumbers, does not reproduce: no pointer to relocate) and a relocating minor. The
nursery cap is a pacing knob only; the issue's default-configuration failure is
the same program at 150 × 1500 rows.
Forward trace of the wrong value
Runtime instrumentation on the 11-line program, one run, in order:
step closure is evacuated
…3eb0050 → …4f00008; every root the collectorowns (
INLINE_TRAP.current_step,CURRENT_MICROTASK_CALLBACK,TASK_QUEUE)is rewritten. The JS frame's
%this_closureregister still holds…3eb0050.{ id: i }lands at…3eb0050(see the value bits of the first dropped box write — the oldclosure address, now an ordinary object).
js_closure_get_capture_bits(…3eb0050, idx)returns 0, sojs_i32_box_set(0, 1)— the__gen_state = 1store — is dropped.step=…4f00008,cur_cbmatches), reads
stateas 0, and re-runs state 0.Exactly one duplicate, because only the one body execution overlapped the move.
The fix
crates/perry-codegen/src/codegen/closure.rs: a closure body that has capturesspills
%this_closureinto an entry-block alloca, NaN-boxed withPOINTER_TAG,and binds it to a shadow-stack slot (
reserve_shadow_slot+js_shadow_slot_bind).expr::current_closure_ptr_valuereloads the pointerfrom that slot at every capture access, so the collector marks and rewrites it
like any other precise root. All 12
ctx.current_closure_ptrconsumers routethrough the helper.
Emitted IR, before → after:
Scope kept deliberately tight:
(a, b) => a - bemits no captureaccess, so no slot is reserved and no
js_shadow_frame_push/poppair isforced onto a body that needs no frame.
lower_typed_f64_body_*bails on anything that is not straight-linearithmetic (
"typed-f64 clone cannot lower non-straight-line statement"), soa typed body contains no loop poll and no allocation — its
%this_closureregister cannot go stale. Same for the public trampoline, which only forwards
the parameter.
this/new.targetcapture reads keep using the raw parameter: they runin the entry-block prologue, ahead of any statement that can collect.
crates/perry-runtimeis byte-identical tomain.Verification
Oracle is node
v26.5.0(.node-version), asserted before every run.Release build,
PERRY_NO_AUTO_OPTIMIZEnot set (production path).The issue's own
w5_srv_scale.ts, one binary, checksum per arm:-341887099-198241022❌-341887099✅-1598210268-1454564191❌-1598210268✅17352663231878912400❌1735266323✅(The before-column error is the constant
143646077at every length — thistree's magnitude for the constant the issue reports as
251646625ate279b2d54. Same defect, same shape: one request's fold counted twice.)The reduced 150-request pump (
calls+checksum), all node-exact after:calls:150checksum:-342133776PERRY_GC_SCAVENGE_NURSERY_MB=1calls:151checksum:-340242326❌PERRY_GC_SCAVENGE_NURSERY_MB=2calls:151❌PERRY_GC_SCAVENGE_NURSERY_MB=8calls:151❌PERRY_GC_MOVING_SAFEPOINT=0PERRY_GC_SCAVENGE=1PERRY_GC_FORCE_EVACUATE=1PERRY_GEN_GC=0(control)The fix is not inertness: the instrumented run still shows
MOVE closure … -> …on the fixed build. The closure is still relocated; thebody just follows it.
Regression coverage
Two tests, one per tier, both sabotage-verified in both directions.
perry-codegenunit test (runs in the per-PRcargo-testgate)—
codegen::closure::tests::closure_body_roots_its_own_closure_pointer_and_reads_captures_through_it.Compiles a one-capture closure to IR (
emit_ir_only) and asserts the bodyNaN-boxes
%this_closureinto a shadow-bound slot and that nojs_closure_get_capture_bits(i64 %this_closure/set_capture_bits(i64 %this_closureremains. Non-vacuous: it also asserts the body reads a captureat all.
crates/perry/tests/gc_closure_self_pointer_root_7055.rs(runs per-PR viathe
e2e-scopedjob, CI: PR cargo-test never executes crates/perry integration suites; main-push full run starves under merge trains (documented near-miss) #5960). Compiles the reduced pump and runs the samebinary under 10 collector configurations — default, a
PERRY_GC_SCAVENGE_NURSERY_MBsweep of 1/2/3/4/5/8/12/16, andPERRY_GEN_GC=0— each of which must reproduce node byte-for-byte. The sweep is what gives it
teeth without flakiness: whether a relocating minor lands inside the state
body depends on where the nursery happens to fill, so any single cap is a coin
flip on a given host, while across the sweep the unfixed compiler fails at
1, 2 and 8 MB.
cargo test -p perry-codegen --lib: 283 passed, 0 failed.cargo fmt --all -- --check: clean.Summary by CodeRabbit