fix(gc): Layer 1 rooting slice 7 — child_process, Proxy/Reflect, await (#7615) - #7662
Open
proggeramlug wants to merge 1 commit into
Open
fix(gc): Layer 1 rooting slice 7 — child_process, Proxy/Reflect, await (#7615)#7662proggeramlug wants to merge 1 commit into
proggeramlug wants to merge 1 commit into
Conversation
#7615) Migrate `expr/child_proc.rs`, `expr/proxy_reflect.rs` and `expr/fs_await.rs` onto `crate::rooting` and list them in the `MIGRATED_MODULES` ledger. All three named `expr::temp_root` before the migration, so all three lines are load-bearing on the committed source. One node-visible bug: `child_process` validated each argument the instant it was lowered, so a bad `command` threw before the later arguments were evaluated. JS evaluates a call's whole argument list before the callee is entered. A probe over all seven entry points diffs empty against node 26.5.1 with the fix and drops 14 side effects without it. Rooting: nine windows across five `child_process` arms (raw `StringHeader*` carried across user lowerings, plus `fork`'s `js_jsvalue_to_string_coerce` result), twenty-eight unprotected `Proxy.*` / `Reflect.*` lowerings, the `process.env[k] = v` key in both its literal and computed forms, and `proxy_build_args_array`'s bare-register accumulator (deleted; its four call sites now build the array inside a `RootedGroup` that also holds the receiver). `fs_await.rs`'s await-loop root was correct and never released. Adds `RootedGroup::adopt_emitted` — the combinator `rooting.rs` refused to add ahead of a caller — for a GC-managed value produced by an emitted step rather than by lowering an `Expr`.
proggeramlug
force-pushed
the
gc/7615-layer1-slice7-expr-modules
branch
from
August 8, 2026 20:06
387f2d8 to
a1a8b90
Compare
📝 WalkthroughWalkthroughLayer 1 rooting migration updates child-process, Proxy/Reflect, environment, and await lowering to use ChangesExpression rooting migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ExpressionLowering
participant RootedGroup
participant RuntimeCall
ExpressionLowering->>RootedGroup: lower and root collecting operands
ExpressionLowering->>RootedGroup: reread values after collection points
RootedGroup->>RuntimeCall: provide current pointer or boxed operands
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-codegen/src/expr/proxy_reflect.rs (1)
1602-1626: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRoot
inst_handlebefore calling intoemit_class_capture_writeback.
inst_handleis an unrooted raw i64 pointer.emit_class_capture_writebackemitsjs_box_set, and thejs_box_get/js_box_sethelper family is treated as a rooted accessor path that requires the box pointer to remain valid across its allocation/reentrancy window. Root the object pointer, emitjs_box_setthrough that rooted state, then release the root.🤖 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/expr/proxy_reflect.rs` around lines 1602 - 1626, In the class write-back block of the Reflect.construct lowering, root inst_handle before calling emit_class_capture_writeback, since that helper emits js_box_set across an allocation/reentrancy window. Release the root after the write-back completes, preserving the existing pointer derivation and class capture behavior.
🧹 Nitpick comments (1)
crates/perry-codegen/src/expr/slice7_rooting_tests.rs (1)
106-119: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueApply the
declarefilter tolast_alloc_beforefor consistency.
call_lineat line 69 andtemp_root_callsat line 125 both excludedeclarelines, and the module doc at lines 63-65 states that the exclusion is load-bearing.last_alloc_beforedoes not apply it, so adeclareforjs_object_alloccan be counted as an allocation site.The current tests cannot pass falsely because every case lowers a real allocating operand. The omission is a latent trap for the next test added to this file.
♻️ Proposed change
fn last_alloc_before(ir: &str, before: usize) -> usize { ir.lines() .enumerate() .take(before) + .filter(|(_, l)| !l.trim_start().starts_with("declare")) .filter(|(_, l)| l.contains("`@js_object_alloc`")) .map(|(i, _)| i) .last() .unwrap_or_else(|| panic!("no object allocation above line {before} in:\n{ir}")) }🤖 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/expr/slice7_rooting_tests.rs` around lines 106 - 119, Update last_alloc_before to exclude IR lines containing “declare” before filtering for “@js_object_alloc”, matching the existing call_line and temp_root_calls behavior. Preserve the current line-index tracking and no-allocation panic behavior, and keep last_alloc delegating through last_alloc_before.
🤖 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/expr/child_proc.rs`:
- Around line 82-96: Update lower_cp_args so operands remain rooted across
child-process validator calls, especially the spawnSync(cmd, args, opts) path
where opts is the final operand consumed by the validator. Ensure the
validator-emitting arms pass across_call = true, or otherwise make collects
cover the full validator window while preserving discarded-operand handling and
slot_ptr register expectations.
In `@crates/perry-codegen/src/expr/proxy_reflect.rs`:
- Around line 1236-1263: The assignment result is returned from a pre-call
register that may be invalidated by GC or re-entry. In
crates/perry-codegen/src/expr/proxy_reflect.rs lines 1236-1263, root val_double
across js_setenv and return its reread register below the call in both
computed-key and literal branches, adding the needed RootedGroup for the literal
branch. In lines 1278-1288, root the value operand across js_proxy_set and
return its post-call reread register instead of the pre-trap v[2].
- Around line 1513-1563: In the explicit-receiver branch of the surrounding
lowering function, root the lowered value using a collects flag derived from
receiver before calling lower_expr on receiver. Re-read the rooted value after
lower_expr(ctx, receiver)? and pass that reread value to js_put_value_set; leave
the same-receiver branch unchanged.
In `@crates/perry-codegen/src/expr/slice7_rooting_tests.rs`:
- Around line 172-188: The three zero-cost rooting tests use temp_root_calls,
which is blind to statepoint/RS4GC lowering; replace it with
temp_root_slot_width so the assertions cover all lowerings. In
crates/perry-codegen/src/expr/slice7_rooting_tests.rs at lines 172-188, 259-271,
and 437-442, update the no-options execSync, single-operand Reflect.ownKeys, and
string-literal process.env key tests to assert temp_root_slot_width(&ir) == 0.
---
Outside diff comments:
In `@crates/perry-codegen/src/expr/proxy_reflect.rs`:
- Around line 1602-1626: In the class write-back block of the Reflect.construct
lowering, root inst_handle before calling emit_class_capture_writeback, since
that helper emits js_box_set across an allocation/reentrancy window. Release the
root after the write-back completes, preserving the existing pointer derivation
and class capture behavior.
---
Nitpick comments:
In `@crates/perry-codegen/src/expr/slice7_rooting_tests.rs`:
- Around line 106-119: Update last_alloc_before to exclude IR lines containing
“declare” before filtering for “@js_object_alloc”, matching the existing
call_line and temp_root_calls behavior. Preserve the current line-index tracking
and no-allocation panic behavior, and keep last_alloc delegating through
last_alloc_before.
🪄 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: a040028b-1278-43ac-9a71-7a8146c3d3d4
📒 Files selected for processing (8)
changelog.d/7662-layer1-slice7-expr-modules.mdcrates/perry-codegen/src/expr/child_proc.rscrates/perry-codegen/src/expr/fs_await.rscrates/perry-codegen/src/expr/helpers.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/proxy_reflect.rscrates/perry-codegen/src/expr/slice7_rooting_tests.rscrates/perry-codegen/src/rooting.rs
Comment on lines
+82
to
+96
| fn lower_cp_args<'a>( | ||
| ctx: &mut FnCtx<'_>, | ||
| group: &mut RootedGroup<'a>, | ||
| exprs: &[&'a Expr], | ||
| discarded: Option<usize>, | ||
| across_call: bool, | ||
| ) -> Result<()> { | ||
| for (i, expr) in exprs.iter().enumerate() { | ||
| let consumed = Some(i) != discarded; | ||
| let collects = consumed | ||
| && (across_call || any_operand_may_collect(ctx, exprs[i + 1..].iter().copied())); | ||
| group.lower(ctx, expr, collects)?; | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the child-process validator runtime implementations for own-property reads that can invoke user accessors.
set -euo pipefail
fd -t f . crates/perry-runtime/src --exec rg -l 'js_child_process_validate_options' {} \; | while IFS= read -r f; do
echo "=== $f ==="
rg -n -A60 'fn js_child_process_validate_options' "$f"
done
echo
echo "=== validate_spawn_args / validate_args / validate_command ==="
rg -n -A40 'fn js_child_process_validate_(spawn_args|args|command)\b' crates/perry-runtime/srcRepository: PerryTS/perry
Length of output: 205
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== candidate child process files ==="
fd -t f 'child_process' crates/perry-runtime crates/perry-codegen crates/perry 2>/dev/null || true
echo
echo "=== relevant symbols in runtime ==="
rg -n 'js_child_process_validate_options|js_child_process_validate_(spawn_args|args|command)\b|validate.*spawn|validate.*args|validate.*command|ownProperties|Object\.getOwnProperty|readOwnProperty' crates/perry-runtime/src || true
echo
echo "=== child_proc outline and relevant snippets ==="
ast-grep outline crates/perry-codegen/src/expr/child_proc.rs || true
sed -n '1,140p' crates/perry-codegen/src/expr/child_proc.rs | cat -nRepository: PerryTS/perry
Length of output: 25054
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== validate.rs relevant sections ==="
sed -n '1,425p' crates/perry-runtime/src/child_process/validate.rs | cat -n
echo
echo "=== child_process usages in codegen arms ==="
rg -n 'lower_cp_args|emit_cp_validators|js_jsvalue_to_string_coerce|across_call|slot_ptr|emit_cp_validate_options|emit_cp_validate_spawn_args|emit_cp_validate_args|emit_cp_validate_command|emit_cp_validate_fork_module' crates/perry-codegen/src/expr/child_proc.rs
echo
echo "=== child_proc emit validators and lower call sites ==="
sed -n '160,310p' crates/perry-codegen/src/expr/child_proc.rs | cat -nRepository: PerryTS/perry
Length of output: 30670
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== locate cp_get_field / cp_value_to_string implementations ==="
rg -n 'fn cp_get_field|fn cp_value_to_string|fn cp_array_ptr|fn cp_object_ptr|fn extract_closure_ptr|is_registered_buffer|is_url_object_shape|js_array_get_f64' crates/perry-runtime/src crates/perry-codegen/src || true
echo
echo "=== files containing cp_get_field definitions ==="
rg -l 'fn cp_get_field|cp_get_field' crates/perry-runtime/src/child_process crates/perry-runtime/src | while IFS= read -r f; do
echo "--- $f"
rg -n -A35 'fn cp_get_field|fn cp_value_to_string|fn cp_array_ptr|fn cp_object_ptr|fn extract_closure_ptr' "$f" || true
done
echo
echo "=== read-write verifier for validate own-property reads ==="
python3 - <<'PY'
from pathlib import Path
p = Path("crates/perry-runtime/src/child_process/validate.rs")
s = p.read_text()
fn_name = "cp_validate_options"
idx = s.index("fn " + fn_name)
end = s.index("\n}", idx)
body = s[idx:end]
checks = [
("cp_get_field", "field reads"),
("cp_array_ptr", "array reads (loop element reads)"),
("js_array_get_f64", "array element reads"),
("js_is_symbol", "own property reflection"),
("extract_closure_ptr", "own property reflection"),
("js_validate_signal", "own property reflection"),
]
for needle, label in checks:
count = body.count(needle)
print(f"{needle}: {count} occurrences -> {label}")
print("body_len", len(body))
print("contains cp_throw_option_type", "cp_throw_option_type" in body)
print("contains cp_signal_is_valid", "cp_signal_is_valid" in body)
PYRepository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== child_process helper definitions ==="
fd -t f . crates/perry-runtime/src/child_process \
| while IFS= read -r f; do
echo "--- $f"
rg -n -A45 'pub \(crate\) fn cp_get_field|pub \(crate\) fn cp_value_to_string|fn cp_get_field|fn cp_value_to_string|fn cp_array_ptr|fn cp_object_ptr|fn cp_validate_stdio_entry|fn cp_signal_is_valid|fn cp_throw_option_type' "$f" || true
done
echo
echo "=== rooting/reread behavior ==="
fd -t f . crates/perry-codegen/src/rooting.rs crates/perry-codegen/src -i 'rooting' \
| while IFS= read -r f; do
echo "--- $f"
rg -n -A60 'struct RootedGroup|fn reread|fn lower|with_operands_rooted_across_call|fn with_rooted_group|any_operand_may_collect' "$f" || true
doneRepository: PerryTS/perry
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== cp_get_field / cp_value_to_string definitions in child_process ==="
rg -n -A80 'pub \(crate\) fn cp_get_field|pub \(crate\) fn cp_value_to_string|fn cp_get_field|fn cp_value_to_string|fn cp_array_ptr|fn cp_object_ptr|pub \(crate\) fn cp_signal_is_valid|fn cp_signal_is_valid' crates/perry-runtime/src/child_process/*
echo
echo "=== focused validate helper calls in cp_validate_options ==="
python3 - <<'PY'
from pathlib import Path
s = Path("crates/perry-runtime/src/child_process/validate.rs").read_text()
start = s.index("fn cp_validate_options")
end = s.index("\n}", start)
body = s[start:end]
for f in ["cp_get_field", "cp_value_to_string", "cp_array_ptr", "js_array_get_f64", "is_registered_buffer", "extract_closure_ptr", "is_url_object_shape"]:
print(f, body.count(f))
print("option field reads:", len([line for line in body.splitlines() if "cp_get_field(value" in line]))
PYRepository: PerryTS/perry
Length of output: 19030
Root the operands across all child-process validators.
js_child_process_validate_options calls js_object_get_field_by_name_f64 for each option field, so unprotected operands can be relocated through slot_box while the validator is still reading them. spawnSync(cmd, args, opts) leaves opts unprotected as the last operand, then slot_ptr(ctx, g, at[2]) names the pre-validator register for the consuming call. Set across_call = true for arms that emit validators, or make collects include the validator window.
🤖 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/expr/child_proc.rs` around lines 82 - 96, Update
lower_cp_args so operands remain rooted across child-process validator calls,
especially the spawnSync(cmd, args, opts) path where opts is the final operand
consumed by the validator. Ensure the validator-emitting arms pass across_call =
true, or otherwise make collects cover the full validator window while
preserving discarded-operand handling and slot_ptr register expectations.
Source: Learnings
Comment on lines
+1236
to
+1263
| let val_double = lower_expr(ctx, value)?; | ||
| let key_idx = ctx.strings.intern(property); | ||
| let key_handle_global = format!("@{}", ctx.strings.entry(key_idx).handle_global); | ||
| let blk = ctx.block(); | ||
| let key_box = blk.load(DOUBLE, &key_handle_global); | ||
| let key_handle = unbox_to_i64(blk, &key_box); | ||
| blk.call_void("js_setenv", &[(I64, &key_handle), (DOUBLE, &val_double)]); | ||
| return Ok(Some(val_double)); | ||
| } | ||
| // Computed key. `js_to_property_key` must run ABOVE the value's evaluation | ||
| // — ES2022 moved `ToPropertyKey` before the RHS — so the value that has to | ||
| // survive that evaluation is the COERCED key, a fresh heap string produced | ||
| // by an emitted call rather than by lowering an expression. That is what | ||
| // `RootedGroup::adopt_emitted` is for. | ||
| with_rooted_group(ctx, 1, |ctx, g| { | ||
| let key_box = lower_expr(ctx, key)?; | ||
| let property_key = ctx | ||
| .block() | ||
| .call(DOUBLE, "js_to_property_key", &[(DOUBLE, &key_box)]); | ||
| let key_slot = g.adopt_emitted(ctx, Repr::Boxed, &property_key); | ||
| let val_double = lower_expr(ctx, value)?; | ||
| // The strip happens BELOW the window, never above it. | ||
| let key_box = g.reread_emitted(ctx, key_slot); | ||
| let key_handle = unbox_str_handle(ctx.block(), &key_box); | ||
| ctx.block() | ||
| .call_void("js_setenv", &[(I64, &key_handle), (DOUBLE, &val_double)]); | ||
| Ok(Some(val_double)) | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Assignment-result registers are read above the consuming call that runs user code. Both arms return the value register produced before a helper that can allocate or re-enter user code, so an evacuating collection inside that helper leaves the returned register naming from-space whenever the assignment is used as an expression.
crates/perry-codegen/src/expr/proxy_reflect.rs#L1236-L1263: adoptval_doubleinto the openRootedGroupand returnreread_emittedbelow thejs_setenvcall; apply the same treatment to the literal branch, which currently has no group.crates/perry-codegen/src/expr/proxy_reflect.rs#L1278-L1288: root thevalueoperand acrossjs_proxy_setand return the re-read register instead of the pre-trapv[2].
📍 Affects 1 file
crates/perry-codegen/src/expr/proxy_reflect.rs#L1236-L1263(this comment)crates/perry-codegen/src/expr/proxy_reflect.rs#L1278-L1288
🤖 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/expr/proxy_reflect.rs` around lines 1236 - 1263, The
assignment result is returned from a pre-call register that may be invalidated
by GC or re-entry. In crates/perry-codegen/src/expr/proxy_reflect.rs lines
1236-1263, root val_double across js_setenv and return its reread register below
the call in both computed-key and literal branches, adding the needed
RootedGroup for the literal branch. In lines 1278-1288, root the value operand
across js_proxy_set and return its post-call reread register instead of the
pre-trap v[2].
Comment on lines
+1513
to
+1563
| with_rooted_group(ctx, 2, |ctx, g| { | ||
| // The receiver's window covers BOTH the key's lowering and the | ||
| // value's, so its `collects` is the disjunction — `o[f()] = 1` | ||
| // has an inert value and a collecting key. | ||
| let recv_collects = | ||
| any_operand_may_collect(ctx, [key.as_ref(), value.as_ref(), receiver.as_ref()]); | ||
| let recv_slot = g.lower(ctx, target, recv_collects)?; | ||
| let key_collects = | ||
| any_operand_may_collect(ctx, [value.as_ref(), receiver.as_ref()]); | ||
| let key_slot = g.lower(ctx, key, key_collects)?; | ||
| let v = lower_expr(ctx, value)?; | ||
| // #6812 (w12): same-receiver dynamic-key stores that failed the | ||
| // inline gate (computed target expressions) still take the | ||
| // outlined 3-way IC helper. | ||
| if same_put_value_receiver_expr(target, receiver) { | ||
| let k = g.reread(ctx, key_slot)?; | ||
| let t = g.reread(ctx, recv_slot)?; | ||
| let site_id = ctx.ic_site_counter; | ||
| ctx.ic_site_counter += 1; | ||
| let cache_name = format!("perry_ic_{}", site_id); | ||
| ctx.ic_globals.push(cache_name.clone()); | ||
| let cache_ref = format!("@{}", cache_name); | ||
| Ok(ctx.block().call( | ||
| DOUBLE, | ||
| "js_put_value_set_dyn_ic", | ||
| &[ | ||
| (crate::types::PTR, &cache_ref), | ||
| (DOUBLE, &t), | ||
| (DOUBLE, &k), | ||
| (DOUBLE, &v), | ||
| (I32, strict_i32), | ||
| ], | ||
| )) | ||
| } else { | ||
| // The explicit-receiver form lowers a FOURTH operand, so the | ||
| // re-reads have to sit below it, not above. | ||
| let r = lower_expr(ctx, receiver)?; | ||
| let k = g.reread(ctx, key_slot)?; | ||
| let t = g.reread(ctx, recv_slot)?; | ||
| Ok(ctx.block().call( | ||
| DOUBLE, | ||
| "js_put_value_set", | ||
| &[ | ||
| (DOUBLE, &t), | ||
| (DOUBLE, &k), | ||
| (DOUBLE, &v), | ||
| (DOUBLE, &r), | ||
| (I32, strict_i32), | ||
| ], | ||
| )) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
value is not rooted across the explicit-receiver lowering.
The group holds target and key. v is lowered at line 1523 and, in the else branch, is carried across lower_expr(ctx, receiver)? at line 1549. receiver is an arbitrary user expression that can allocate and run user code. v then reaches js_put_value_set as a stale register.
This is operand-to-operand, the taxonomy (c) shape the module doc says the slice removed. The same-receiver branch is unaffected because nothing is lowered after v there.
Root value with a collects derived from receiver, and re-read it below the receiver's lowering.
🐛 Proposed fix
- with_rooted_group(ctx, 2, |ctx, g| {
+ with_rooted_group(ctx, 3, |ctx, g| {
// The receiver's window covers BOTH the key's lowering and the
// value's, so its `collects` is the disjunction — `o[f()] = 1`
// has an inert value and a collecting key.
let recv_collects =
any_operand_may_collect(ctx, [key.as_ref(), value.as_ref(), receiver.as_ref()]);
let recv_slot = g.lower(ctx, target, recv_collects)?;
let key_collects =
any_operand_may_collect(ctx, [value.as_ref(), receiver.as_ref()]);
let key_slot = g.lower(ctx, key, key_collects)?;
- let v = lower_expr(ctx, value)?;
+ // The value is live across the explicit-receiver lowering
+ // below, so it needs the same protection the other operands
+ // get.
+ let explicit_receiver = !same_put_value_receiver_expr(target, receiver);
+ let value_collects = explicit_receiver
+ && any_operand_may_collect(ctx, std::iter::once(receiver.as_ref()));
+ let value_slot = g.lower(ctx, value, value_collects)?;
// `#6812` (w12): same-receiver dynamic-key stores that failed the
// inline gate (computed target expressions) still take the
// outlined 3-way IC helper.
if same_put_value_receiver_expr(target, receiver) {
+ let v = g.reread(ctx, value_slot)?;
let k = g.reread(ctx, key_slot)?;
let t = g.reread(ctx, recv_slot)?;
@@
} else {
// The explicit-receiver form lowers a FOURTH operand, so the
// re-reads have to sit below it, not above.
let r = lower_expr(ctx, receiver)?;
+ let v = g.reread(ctx, value_slot)?;
let k = g.reread(ctx, key_slot)?;
let t = g.reread(ctx, recv_slot)?;📝 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.
Suggested change
| with_rooted_group(ctx, 2, |ctx, g| { | |
| // The receiver's window covers BOTH the key's lowering and the | |
| // value's, so its `collects` is the disjunction — `o[f()] = 1` | |
| // has an inert value and a collecting key. | |
| let recv_collects = | |
| any_operand_may_collect(ctx, [key.as_ref(), value.as_ref(), receiver.as_ref()]); | |
| let recv_slot = g.lower(ctx, target, recv_collects)?; | |
| let key_collects = | |
| any_operand_may_collect(ctx, [value.as_ref(), receiver.as_ref()]); | |
| let key_slot = g.lower(ctx, key, key_collects)?; | |
| let v = lower_expr(ctx, value)?; | |
| // #6812 (w12): same-receiver dynamic-key stores that failed the | |
| // inline gate (computed target expressions) still take the | |
| // outlined 3-way IC helper. | |
| if same_put_value_receiver_expr(target, receiver) { | |
| let k = g.reread(ctx, key_slot)?; | |
| let t = g.reread(ctx, recv_slot)?; | |
| let site_id = ctx.ic_site_counter; | |
| ctx.ic_site_counter += 1; | |
| let cache_name = format!("perry_ic_{}", site_id); | |
| ctx.ic_globals.push(cache_name.clone()); | |
| let cache_ref = format!("@{}", cache_name); | |
| Ok(ctx.block().call( | |
| DOUBLE, | |
| "js_put_value_set_dyn_ic", | |
| &[ | |
| (crate::types::PTR, &cache_ref), | |
| (DOUBLE, &t), | |
| (DOUBLE, &k), | |
| (DOUBLE, &v), | |
| (I32, strict_i32), | |
| ], | |
| )) | |
| } else { | |
| // The explicit-receiver form lowers a FOURTH operand, so the | |
| // re-reads have to sit below it, not above. | |
| let r = lower_expr(ctx, receiver)?; | |
| let k = g.reread(ctx, key_slot)?; | |
| let t = g.reread(ctx, recv_slot)?; | |
| Ok(ctx.block().call( | |
| DOUBLE, | |
| "js_put_value_set", | |
| &[ | |
| (DOUBLE, &t), | |
| (DOUBLE, &k), | |
| (DOUBLE, &v), | |
| (DOUBLE, &r), | |
| (I32, strict_i32), | |
| ], | |
| )) | |
| } | |
| with_rooted_group(ctx, 3, |ctx, g| { | |
| // The receiver's window covers BOTH the key's lowering and the | |
| // value's, so its `collects` is the disjunction — `o[f()] = 1` | |
| // has an inert value and a collecting key. | |
| let recv_collects = | |
| any_operand_may_collect(ctx, [key.as_ref(), value.as_ref(), receiver.as_ref()]); | |
| let recv_slot = g.lower(ctx, target, recv_collects)?; | |
| let key_collects = | |
| any_operand_may_collect(ctx, [value.as_ref(), receiver.as_ref()]); | |
| let key_slot = g.lower(ctx, key, key_collects)?; | |
| // The value is live across the explicit-receiver lowering | |
| // below, so it needs the same protection the other operands | |
| // get. | |
| let explicit_receiver = !same_put_value_receiver_expr(target, receiver); | |
| let value_collects = explicit_receiver | |
| && any_operand_may_collect(ctx, std::iter::once(receiver.as_ref())); | |
| let value_slot = g.lower(ctx, value, value_collects)?; | |
| // `#6812` (w12): same-receiver dynamic-key stores that failed the | |
| // inline gate (computed target expressions) still take the | |
| // outlined 3-way IC helper. | |
| if same_put_value_receiver_expr(target, receiver) { | |
| let v = g.reread(ctx, value_slot)?; | |
| let k = g.reread(ctx, key_slot)?; | |
| let t = g.reread(ctx, recv_slot)?; | |
| let site_id = ctx.ic_site_counter; | |
| ctx.ic_site_counter += 1; | |
| let cache_name = format!("perry_ic_{}", site_id); | |
| ctx.ic_globals.push(cache_name.clone()); | |
| let cache_ref = format!("@{}", cache_name); | |
| Ok(ctx.block().call( | |
| DOUBLE, | |
| "js_put_value_set_dyn_ic", | |
| &[ | |
| (crate::types::PTR, &cache_ref), | |
| (DOUBLE, &t), | |
| (DOUBLE, &k), | |
| (DOUBLE, &v), | |
| (I32, strict_i32), | |
| ], | |
| )) | |
| } else { | |
| // The explicit-receiver form lowers a FOURTH operand, so the | |
| // re-reads have to sit below it, not above. | |
| let r = lower_expr(ctx, receiver)?; | |
| let v = g.reread(ctx, value_slot)?; | |
| let k = g.reread(ctx, key_slot)?; | |
| let t = g.reread(ctx, recv_slot)?; | |
| Ok(ctx.block().call( | |
| DOUBLE, | |
| "js_put_value_set", | |
| &[ | |
| (DOUBLE, &t), | |
| (DOUBLE, &k), | |
| (DOUBLE, &v), | |
| (DOUBLE, &r), | |
| (I32, strict_i32), | |
| ], | |
| )) | |
| } |
🤖 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/expr/proxy_reflect.rs` around lines 1513 - 1563, In
the explicit-receiver branch of the surrounding lowering function, root the
lowered value using a collects flag derived from receiver before calling
lower_expr on receiver. Re-read the rooted value after lower_expr(ctx,
receiver)? and pass that reread value to js_put_value_set; leave the
same-receiver branch unchanged.
Comment on lines
+172
to
+188
| #[test] | ||
| fn exec_sync_without_options_emits_no_rooting_traffic() { | ||
| let ir = compile_body( | ||
| "exec_sync_cold", | ||
| vec![Stmt::Expr(Expr::ChildProcessExecSync { | ||
| command: Box::new(Expr::String("ls".to_string())), | ||
| options: None, | ||
| })], | ||
| ); | ||
| require_call_line(&ir, "js_child_process_exec_sync"); | ||
| assert_eq!( | ||
| temp_root_calls(&ir), | ||
| 0, | ||
| "a no-options execSync cannot collect between its operand and the call, so \ | ||
| operand_protection must route it to Reuse\n{ir}" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The three zero-cost pins use a measure that is blind to the build's rooting lowering. temp_root_calls counts only js_gc_temp_root text, which the FFI fallback emits. The file's own temp_root_slot_width at lines 492-528 records that statepoint / RS4GC is the lowering this build uses, and under it a pooled slot is an entry alloca with no runtime call. All three pins therefore read 0 whether or not the operand was rooted. Replace temp_root_calls with temp_root_slot_width, which covers all three lowerings.
crates/perry-codegen/src/expr/slice7_rooting_tests.rs#L172-L188: asserttemp_root_slot_width(&ir) == 0for the no-optionsexecSync.crates/perry-codegen/src/expr/slice7_rooting_tests.rs#L259-L271: asserttemp_root_slot_width(&ir) == 0for the single-operandReflect.ownKeys.crates/perry-codegen/src/expr/slice7_rooting_tests.rs#L437-L442: asserttemp_root_slot_width(&ir) == 0for the string-literalprocess.envkey.
📍 Affects 1 file
crates/perry-codegen/src/expr/slice7_rooting_tests.rs#L172-L188(this comment)crates/perry-codegen/src/expr/slice7_rooting_tests.rs#L259-L271crates/perry-codegen/src/expr/slice7_rooting_tests.rs#L437-L442
🤖 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/expr/slice7_rooting_tests.rs` around lines 172 -
188, The three zero-cost rooting tests use temp_root_calls, which is blind to
statepoint/RS4GC lowering; replace it with temp_root_slot_width so the
assertions cover all lowerings. In
crates/perry-codegen/src/expr/slice7_rooting_tests.rs at lines 172-188, 259-271,
and 437-442, update the no-options execSync, single-operand Reflect.ownKeys, and
string-literal process.env key tests to assert temp_root_slot_width(&ir) == 0.
proggeramlug
added a commit
that referenced
this pull request
Aug 8, 2026
…undaries (#7665) * fix(opt-report): narrow test recording to the Session's own thread opt_report's gate (FORCED) and sink are process-global, but Session's lock only serialises tests that TAKE a Session. A concurrently-running test that lowers code without one therefore emitted into the holder's sink, and the holder's snapshot saw a neighbour's rows. Observed, not theoretical: only_the_return_position_is_marked_served asserts rows.len() == 2 and failed at 3 about one run in six once #7662 added lowering tests, which changed the parallel schedule. 0/14 on main before that PR, 2/18 on it -- the extra row is what identified the mechanism, not the timing. Recording is now additionally narrowed to the thread that opened the Session. Production is unaffected: the thread check is inside the #[cfg(test)] forced branch, and the env-var path is untouched. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix * fix(ext-registry): narrow provider recording to the test holding the lock USED_PROVIDERS is process-wide but PROVIDER_TEST_LOCK only serialises tests that TAKE it, so a concurrently-running test that lowers any code touching an ext symbol added providers to the holder's set. Observed: ext_prefix_net_does_not_over_match asserts the set is empty after an unlisted symbol and failed 1 run in 20 once #7662 added child_process lowering tests, which call record_ffi_call. ProviderTestGuard takes the lock AND claims the thread; record_ffi_call skips the global set when another thread holds a provider test. Production is unaffected -- PROVIDER_TEST_THREAD is #[cfg(test)] and the predicate returns true whenever no provider test is running. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix * docs(changelog): fragment for the test-isolation fixes Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix * chore: bump version to 0.5.1377 Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
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.
Slice 7 of the Layer 1 emitter migration (#7615). Migrates three
expr/modulesonto
crate::rooting, adds them to theMIGRATED_MODULESledger, and fixes whatreading them turned up.
What is here
expr/child_proc.rslower_cp_args), validators below the whole listexpr/proxy_reflect.rsreflect-metadataarms share one bodyexpr/fs_await.rswith_rooted_group, released in the merge blockThe node-visible bug
child_processvalidated each argument the instant it was lowered — lowercommand, throwERR_INVALID_ARG_TYPE, never evaluateoptions. JS evaluates acall's whole argument list before the callee is entered, so node runs those side
effects and only then throws.
A probe over all seven entry points, A/B'd against node 26.5.1 with a baseline
compiler built from
mainin a separate target directory (identical-pset,identical flags,
PERRY_NO_AUTO_OPTIMIZE=1, noPERRY_GC_MOVING_LOOP_POLLSoneither arm):
The rooting bugs
Taxonomy (a) — a raw pointer above a collection point, which
root_reloadstructurally cannot repair:
execSync/spawnSync/spawn/execFile/execFileSyncstrippedcommand/fileto a bareStringHeader*and then loweredargsandoptions;forkis fix(codegen): root the URL constructor's coerced string across base lowering (Layer 1) #7453's shape at a second site —js_jsvalue_to_string_coerceruns auser
toStringand its raw result crossed two more lowerings;spawnBackgroundcarriedlog_file's stripped pointer acrossenv_json;process.env[k] = vin both branches. The computed one coerced the key withjs_to_property_key— a fresh heap string with no other root — stripped it,and only then lowered the value. The literal one is String literal operand is not GC-rooted across an allocating call in the same expression (stale handle after evacuation) #7114 one operand over from
the
PutValueSetkey GC: a class expression with astatic { … }block SIGSEGVs under PERRY_GC_MOVING_LOOP_POLLS=1 (static-thiscell is not a rewritten root) #7201 fixed. ES2022 movedToPropertyKeybefore the RHS,so the coercion cannot be sunk; the coerced key is what must survive.
Taxonomy (c) — operand-to-operand: 28
Proxy.*/Reflect.*arms.Reflect.has(target, key)handed the pre-collectiontargetregister tojs_reflect_has;Reflect.setdoes it with four operands.#7154 accumulator:
proxy_build_args_arraythreaded the argument array's raw*mut ArrayHeaderthrough its push loop in a bare SSA register, and had no wayto root its caller's receiver across the same loop. Deleted; the four call
sites now build the array inside a
RootedGroupthat holds both.Never released:
fs_await.rspushed the await-loop root unconditionally andemitted no truncate on any path. Over-retention in the alloca lowering; in the
FFI fallback a
js_gc_temp_root_pushper execution with no truncate — #7462'sshape for an
awaitin a loop.API
RootedGroup::adopt_emitted— the combinatorrooting.rsdeleted unused andsaid could return "with its caller and with a written argument for why
call_rootedcannot serve". Two callers turned up, same shape: a GC-managedvalue produced by an emitted step rather than by lowering an
Expr. Theargument, and what it weakens, are in its doc.
Verification
lintcommands extracted fromtest.yml— pass.cargo test -p perry-codegen --lib --no-fail-fast738 pass;cargo test -p perry-runtime --lib --no-fail-fast1917 pass;cargo check --all-targetsclean; the 14native_root_coveragetests pass.gc_root_dominance_check.py --moving-only --seeded-violations 40over a 149-module corpus — 0 violations, 40 planted / 40 caught / 0 missed.
--unrooted-allocas --moving-only0 over 7864 gc-capable allocas.committed so a future "root everything" change goes red.
HEAD—error[count 0,Running unittestspresent, 11 of 13 red. The 2that stay green are the deliberate zero-cost pins.
temp_root_push_double/temp_root_truncatepair planted in each,error[count 0 in all three, ledger red and naming both planted lines.
No runtime fault is claimed. Every window is demonstrated in IR; whether a
stale pointer is observably wrong depends on what is recycled into those bytes.
Caveat on the dominance gate
It compiles its corpus under
PERRY_RS4GC=0— the shadow lowering, which isnot the default since #7370. Its green verdict says nothing about the native
lowering that ships; the
native_root_coveragesuite is what covers that, and itis run above.
Left for later slices, with reasons
expr/static_field_meta.rs,expr/math_simple.rs,expr/dyn_extern_i18n.rs—not audited here. A survey flagged candidates in all three (
ClassExprFresh'scaps_arraccumulator,ArrayMap's receiver unboxed below its own window, anda raw
path_handlereused across a dynamic-import compare loop that runs module__initbodies) but I have not verified them myself, so they are leads, notfindings.
lower_call/new.rs— unblocked by slice 6'sRootedGroupbut at 1988/2000lines against the file-size gate; needs its own slice with a split.
Summary by CodeRabbit
Bug Fixes
Tests