From fb5c3db0b03cc28035c947d317203d1a73e66858 Mon Sep 17 00:00:00 2001 From: jdalton Date: Sat, 1 Aug 2026 15:03:45 -0400 Subject: [PATCH 1/2] fix(codegen): root the callee, `this` and every argument of js_closure_callN The generic dynamic-value-call lowering held THREE classes of GC value in bare SSA registers across work that can collect. `js_closure_callN` is the central dispatch path -- `f(g())`, `o.m(g())`, `curry(1)(2)`, every call whose callee is a value rather than a statically resolved function -- and this is the site An SSA register is not a GC root. Under PERRY_GC_MOVING_LOOP_POLLS=1 a back-edge poll inside an argument runs an evacuating minor: each held value SURVIVES (the capture cell, shadow slot or module global it was read from is a root) and therefore MOVES. The collector rewrites that location; the register keeps naming from-space. * the CALLEE, held across the whole argument list. The checked unbox masks a pre-move address and js_closure_callN reads a closure header out of abandoned memory: "TypeError: value is not a function". * the `this` RECEIVER, held across the read of the callee off it AND the argument list. #7206 fixed this operand on the sibling js_native_call_method_by_id dispatch; this is the generic one. * each already-lowered ARGUMENT, held across the arguments after it AND across the rebind unbox. The three live windows differ, so they are computed separately: receiver | the callee read + every argument callee | every argument argument i | the arguments after i + the rebind unbox That last window is why this is not a copy of #7206's fix. js_closure_unbox_callee_checked_rebind calls clone_closure_rebind_this, which ALLOCATES a replacement closure (closure/dynamic_props.rs:1040) when the callee captures `this`. It sits below the last argument and above js_closure_callN, so the arguments are re-read below it -- hence RootedOperands::reread_one, which re-reads one operand at a caller-chosen point instead of the whole group at one. Hoisting the unbox above the argument list would remove the window instead, but its throw is observable and the spec evaluates arguments before it. On the >16-arity path the argument stores into the stack buffer moved below the unbox for the same reason: a stack buffer is not a root, so filling it above an allocating rebind freezes pre-move addresses one indirection further out. Receiverless calls take js_closure_unbox_callee_checked, a tag check and a mask that allocates nothing, so `f(x, y)` on inert operands emits exactly its old IR. Temp roots, not re-lowering: re-lowering the callee or receiver would observe an assignment made by an argument, a miscompile rather than a rooting fix. Three gap tests, one per held value, each red on the parent under a GENUINE POLLS=1 build and green after. The flag is compile-time since #7161 AND runtime-armed (gc_moving_loop_polls_enabled(), gc/policy.rs:1759) -- setting only one is a false green, and the first cut of these tests passed 10/10 for exactly that reason. callee / this / argument, POLLS=1 parent: TypeError 10/10 each this: bad 0 10/10 each all three, POLLS=1 + PERRY_GEN_GC=0 bad 0 5/5 all three, default (no polls) bad 0 5/5 Cost over the 141-module sfw-registry corpus, measured rather than assumed because this is the hottest emitted call path. operand_protection emits nothing for an operand whose window cannot collect, which is why the delta is small: linked binary 39,216,688 -> 39,233,200 B (+0.042%), emitted IR 1,999,570 -> 2,001,607 lines (+0.10%), js_gc_temp_root_push sites 8,394 -> 8,885. scripts/gc_root_dominance_check.py gains js_closure_unbox_callee_checked in NONCOLLECTING, citing closure/unbox.rs:25 -- a tag check and a low-48 mask, no allocation, no user code, no poll. It sits between every dynamic call's last argument and its js_closure_callN, so its absence reported the whole argument list of every 1-arg dynamic call as stale: 372 of the 729 fatal-sink hits were that single false positive, all marked MOVING: no. The _rebind variant is deliberately NOT added -- it allocates, and the fix above depends on it counting as a collection point. Fatal-sink slice against the corrected list: 231 -> 205. cargo test -p perry-codegen: failing set IDENTICAL to the parent (6 loop_safepoint_purity, 16 native_proof_regressions, 3 native_proof_buffer_views, 1 shadow_slot_hygiene, 1 typed_shape_descriptors), measured on the parent commit rather than assumed; one lib unit test red on the parent passes here. The bind-anchored gate reports the same single non-moving residual #7192 left. WHAT THIS DOES NOT CLOSE: sfw-registry --help under a genuine POLLS=1 build is still red -- 3/10 pass, 7/10 SIGSEGV -- so #7161's stopgap STAYS. Its default arm is clean 10/10, so nothing was traded away. Two concrete leads are written up in the changelog fragment: `prev_this` in this same lowering is the same bug unfixed (js_implicit_this_set returns a value read from the scanned, rewritten IMPLICIT_THIS cell and holds it across the entire user call), and the remaining 205 fatal sinks are no longer dominated by one class, with the spread dispatch (expr/call_spread.rs) the obvious next site. Refs #7154, #7206, #7192, #7198, #7184, #7161, #7114, #6951, #519. --- .../7207-closure-calln-stale-registers.md | 143 ++++++++++ crates/perry-codegen/src/expr/temp_root.rs | 42 ++- .../src/lower_call/console_promise.rs | 248 +++++++++++++----- scripts/gc_root_dominance_check.py | 11 + ...st_gap_gc_closure_call_argument_rooting.ts | 52 ++++ ...test_gap_gc_closure_call_callee_rooting.ts | 50 ++++ .../test_gap_gc_closure_call_this_rooting.ts | 54 ++++ 7 files changed, 519 insertions(+), 81 deletions(-) create mode 100644 changelog.d/7207-closure-calln-stale-registers.md create mode 100644 test-files/test_gap_gc_closure_call_argument_rooting.ts create mode 100644 test-files/test_gap_gc_closure_call_callee_rooting.ts create mode 100644 test-files/test_gap_gc_closure_call_this_rooting.ts diff --git a/changelog.d/7207-closure-calln-stale-registers.md b/changelog.d/7207-closure-calln-stale-registers.md new file mode 100644 index 0000000000..82c22a04a3 --- /dev/null +++ b/changelog.d/7207-closure-calln-stale-registers.md @@ -0,0 +1,143 @@ +### Fixed + +- **The generic dynamic-call lowering no longer holds its callee, its `this` + receiver or its already-lowered arguments in bare registers across the + argument list.** `js_closure_callN` is the central dispatch path — `f(g())`, + `o.m(g())`, `curry(1)(2)`, every call whose callee is a value rather than a + statically resolved function — and it held **three** classes of GC value in + SSA registers across work that can collect. This is the site #7206 named and + deliberately left open, and the last known instance of #7192's + root-store-dominance class. + + An SSA register is not a GC root. Under `PERRY_GC_MOVING_LOOP_POLLS=1` a + back-edge poll inside an argument runs an evacuating minor: each held value + *survives* — the capture cell, shadow slot or module global it was read from + is a root — and therefore **moves**. The collector rewrites that location; + the register keeps naming from-space. + + - **the callee**, held across the whole argument list. The checked unbox then + masks a pre-move address and `js_closure_callN` reads a closure header out + of abandoned memory: `TypeError: value is not a function`, the failure + shape #7154 has worn since #7184. + - **the `this` receiver**, held across the read of the callee off it *and* + the argument list. #7206 fixed this operand on the sibling + `js_native_call_method_by_id` dispatch; this is the same operand on the + generic one — the dispatch a closure-valued property takes (hono's + `RegExpRouter.match = match`, the #519 shape). + - **each already-lowered argument**, held across the arguments after it *and* + across the rebind unbox. + + The three live windows are different, so they are computed separately rather + than protected as one block: + + | operand | window | + |---|---| + | receiver | the callee read + every argument | + | callee | every argument | + | argument *i* | the arguments after *i* + the rebind unbox | + + That last window is the subtle one, and it is why this could not be a + copy of #7206's fix. `js_closure_unbox_callee_checked_rebind` calls + `clone_closure_rebind_this`, which **allocates** a replacement closure + (`closure/dynamic_props.rs:1040`) when the callee captures `this`. It sits + *below* the last argument and *above* `js_closure_callN`, so the arguments + are re-read below it — hence `RootedOperands::reread_one`, which re-reads one + operand at a caller-chosen point instead of re-reading the whole group at + one. Hoisting the unbox above the argument list would remove the window + instead, but its throw is observable and the spec evaluates arguments before + it. For the >16-arity path the argument stores into the stack buffer moved + below the unbox for the same reason: a stack buffer is not a root, so filling + it above an allocating rebind just freezes pre-move addresses one indirection + further out. + + The receiverless path takes `js_closure_unbox_callee_checked`, which is a tag + check and a mask and allocates nothing, so `f(x, y)` on inert operands emits + exactly the IR it emitted before. Temp roots, not re-lowering: re-lowering + the callee or receiver would observe an assignment made by an argument, which + is a miscompile rather than a rooting fix. + + Three new gap tests, one per held value, each red on the parent under a + genuine `POLLS=1` build and green after. **The flag is compile-time since + #7161 *and* runtime-armed (`gc_moving_loop_polls_enabled()`, + `gc/policy.rs:1759`) — setting only one of the two is a false green, and the + first cut of these tests passed 10/10 for exactly that reason.** + + | | parent | this change | + |---|---|---| + | `test_gap_gc_closure_call_callee_rooting.ts`, `POLLS=1` | `TypeError: value is not a function` **10/10** | `bad 0` **10/10** | + | `test_gap_gc_closure_call_this_rooting.ts`, `POLLS=1` | `TypeError: value is not a function` **10/10** | `bad 0` **10/10** | + | `test_gap_gc_closure_call_argument_rooting.ts`, `POLLS=1` | `TypeError: value is not a function` **10/10** | `bad 0` **10/10** | + | all three, `POLLS=1` + `PERRY_GEN_GC=0` | `bad 0` | `bad 0` 5/5 | + | all three, default (no polls) | `bad 0` | `bad 0` 5/5 | + + **Cost, measured over the 141-module `sfw-registry` corpus** — this is the + hottest call path in the compiler's output, so it was measured rather than + assumed. The `operand_protection` gate means an operand whose window cannot + collect emits nothing at all, which is why the delta is this small: + + | | before | after | delta | + |---|---|---|---| + | linked binary | 39,216,688 B | 39,233,200 B | **+0.042 %** | + | emitted IR | 1,999,570 lines | 2,001,607 lines | **+0.10 %** | + | `js_gc_temp_root_push` sites | 8,394 | 8,885 | +491 | + +### Changed + +- **`scripts/gc_root_dominance_check.py`: `js_closure_unbox_callee_checked` is + now in `NONCOLLECTING`**, citing `closure/unbox.rs:25` — it is a tag check on + the NaN-boxed callee and a low-48 mask, with no allocation, no user code and + no poll. It sits between every dynamic call's last argument and its + `js_closure_callN`, so its absence reported the entire argument list of every + 1-arg dynamic call as stale: **372 of the 729 fatal-sink hits were this one + false positive**, all of them marked `MOVING: no`. This is the checker's + stated one-sided discipline working as designed — a missing entry costs false + positives, never a missed bug — and it is why the raw before/after counts + below are quoted against the corrected list. + + `js_closure_unbox_callee_checked_rebind` is deliberately **not** added: it + allocates, and the fix above depends on it counting as a collection point. + +## Verification + +Over the 141-module `sfw-registry` corpus, fatal-sink slice, with the corrected +`NONCOLLECTING`: **231 → 205**. + +`cargo test -p perry-codegen`: failing set **identical to the parent** — +6 `loop_safepoint_purity` (#7161's default flip), 16 `native_proof_regressions`, +3 `native_proof_buffer_views`, 1 `shadow_slot_hygiene`, 1 +`typed_shape_descriptors`, all pre-existing and measured directly on the parent +commit rather than assumed. One `perry-codegen` lib unit test that is red on the +parent passes here. The bind-anchored gate reports the same single non-moving +residual #7192 left (`js_closure_alloc_with_captures_singleton`, 0 +moving-reachable). + +## What this does NOT close + +**`sfw-registry --help` under a genuine `POLLS=1` build is still red, so +#7161's stopgap stays.** Measured on this build, compiled *and* run with the +flag: **3/10 pass, 7/10 SIGSEGV**. Its default arm is clean **10/10**, so +nothing was traded away. The three fixed registers were real and are now +provably rooted, but they are not the last thing standing between the registry +and a clean evacuating minor. + +Two concrete leads for whoever picks this up, both found while fixing the above +and neither speculative: + +1. **`prev_this` in the same lowering is the same bug, unfixed.** + `js_implicit_this_set` returns the *previous* implicit `this`, read out of + the `IMPLICIT_THIS` cell — which `object/this_binding.rs:176` documents as a + scanned mutable root the collector rewrites. That value is then held in a + bare register across the entire user call and written back afterwards, so a + collection anywhere inside the callee makes the restore publish a from-space + pointer back into a root. It is invisible to the current checker on both + ends: `js_implicit_this_set` is not in `ROOT_READ_CALLS`, and it is not a + `RECEIVER_SINKS` fatal sink. Fixing it costs a temp root on every dynamic + call, which is why it was measured and left rather than folded in here. +2. **205 fatal-sink hits remain**, no longer dominated by any single class — + 37 `js_closure_call1`, 22 `js_closure_call2`, 18 + `js_closure_call_apply_with_spread`, 17 `js_array_spread_append`, 15 + `js_object_set_field_by_name`, 15 `js_array_concat`, and a long tail. The + spread path (`expr/call_spread.rs`) is the obvious next one: it is the same + dispatch family and was never touched by #7206 or this change. + +Refs #7154, #7206, #7192, #7198, #7184, #7161, #7114, #6951, #519. diff --git a/crates/perry-codegen/src/expr/temp_root.rs b/crates/perry-codegen/src/expr/temp_root.rs index 81bec6d35a..00bcaa3797 100644 --- a/crates/perry-codegen/src/expr/temp_root.rs +++ b/crates/perry-codegen/src/expr/temp_root.rs @@ -524,20 +524,42 @@ impl RootedOperands { operands: &[&Expr], ) -> anyhow::Result> { let mut out = Vec::with_capacity(self.values.len()); - for (i, original) in self.values.iter().enumerate() { - let value = match &self.slots[i] { - Some(idx) => { - let idx = idx.clone(); - temp_root_get_double(ctx, &idx) - } - None if self.reloadable[i] => super::lower_expr(ctx, operands[i])?, - None => original.clone(), - }; - out.push(value); + for i in 0..self.values.len() { + out.push(self.reread_one(ctx, operands, i)?); } Ok(out) } + /// Re-read ONE operand, at a point the caller picks. + /// + /// [`RootedOperands::reread`] re-reads the whole group at a single point, + /// which is right when one collection point separates the group from its + /// consumer. It is wrong when the operands are consumed by *different* + /// instructions with a collection point between them — the generic + /// dynamic-call lowering is exactly that shape (#7154): the callee and the + /// `this` receiver are consumed by `js_closure_unbox_callee_checked_rebind`, + /// that rebind CLONES a `this`-capturing closure and therefore allocates, + /// and only then does `js_closure_callN` consume the arguments. Re-reading + /// the arguments above the rebind would put them right back in the window + /// the roots exist to close. + /// + /// Same three cases as [`RootedOperands::reread`]; see its documentation. + pub(crate) fn reread_one( + &self, + ctx: &mut FnCtx<'_>, + operands: &[&Expr], + i: usize, + ) -> anyhow::Result { + Ok(match &self.slots[i] { + Some(idx) => { + let idx = idx.clone(); + temp_root_get_double(ctx, &idx) + } + None if self.reloadable[i] => super::lower_expr(ctx, operands[i])?, + None => self.values[i].clone(), + }) + } + /// True when this group actually pushed slots — the signal a caller uses to /// keep an eager unbox (and therefore its exact register numbering) on the /// unprotected path. diff --git a/crates/perry-codegen/src/lower_call/console_promise.rs b/crates/perry-codegen/src/lower_call/console_promise.rs index 4bc662bb1f..3d8da99098 100644 --- a/crates/perry-codegen/src/lower_call/console_promise.rs +++ b/crates/perry-codegen/src/lower_call/console_promise.rs @@ -1257,6 +1257,65 @@ pub fn try_lower_closure_call_fallthrough( // throw an `at :` frame under `--debug-symbols`. `0` (and the // default build) → no emission, unchanged `` frame. let call_byte_offset = ctx.strings.pending_call_offset(); + + // #7154: this lowering held THREE classes of GC value in bare SSA + // registers across work that can collect, and a bare register is not a GC + // root. Under `PERRY_GC_MOVING_LOOP_POLLS=1` a back-edge poll inside any of + // that work runs an evacuating minor: each value SURVIVES (the capture + // cell, shadow slot or module global it was read from is a root) and + // therefore MOVES, the collector rewrites that location, and the register + // keeps naming from-space. + // + // * the CALLEE, held across the whole argument list. The checked unbox + // then masks a pre-move address and `js_closure_callN` reads a closure + // header out of abandoned memory — `TypeError: value is not a + // function`, the failure shape #7154 has worn since #7184. + // * the `this` RECEIVER, held across the read of the callee off it AND + // the argument list. #7206 fixed this operand on the sibling + // `js_native_call_method_by_id` dispatch; this is the same operand on + // the generic one. + // * each already-lowered ARGUMENT, held across the arguments that follow + // it and across the rebind unbox below. + // + // The three windows are NOT the same, which is why they are computed + // separately rather than protected as one block: + // + // receiver | the callee read + every argument + // callee | every argument + // argument | the arguments after it + the rebind unbox + // + // The rebind unbox is a collection point that only exists on the + // member-shaped path: `js_closure_unbox_callee_checked_rebind` calls + // `clone_closure_rebind_this`, which allocates a fresh closure + // (`closure/dynamic_props.rs:1040`) when the callee captures `this`. It + // sits BELOW the last argument and ABOVE `js_closure_callN`, so the + // arguments are re-read after it, not before — see + // `RootedOperands::reread_one`. The receiverless path takes + // `js_closure_unbox_callee_checked`, which is a tag check and a mask and + // allocates nothing, so `f(x, y)` on inert operands emits exactly the IR it + // emitted before this change. + // + // Temp roots, not re-lowering: re-lowering the callee or the receiver would + // observe an assignment made by an argument, which is a miscompile rather + // than a rooting fix (`temp_root::operand_is_reloadable`). + let arg_collects: Vec = args + .iter() + .map(|a| crate::expr::temp_root::expr_may_trigger_gc(ctx, a)) + .collect(); + let any_arg_collects = arg_collects.iter().any(|&c| c); + // Reading the callee off the receiver: a by-name property get walks a + // prototype chain and can run an accessor, so it is a collection point in + // the receiver's window (and only in the receiver's). + let callee_read_collects = crate::expr::temp_root::expr_may_trigger_gc(ctx, callee); + + // Operands are recorded in the order their values are produced — receiver, + // callee, then arguments — because `RootedOperands` roots each one BEFORE + // the next is lowered. Rooting a finished list afterwards is worse than + // doing nothing: by then an earlier operand may already have been swept and + // the push publishes a dangling pointer into a slot the collector scans. + let mut roots = crate::expr::temp_root::root_operands_begin(args.len() + 2); + let mut operand_exprs: Vec<&Expr> = Vec::with_capacity(args.len() + 2); + let prelowered_recv: Option<(String, String)> = if let Expr::PropertyGet { object, property, .. @@ -1271,24 +1330,49 @@ pub fn try_lower_closure_call_fallthrough( None }; - let method_recv: Option = if let Some((ref obj_v, _)) = prelowered_recv { - Some(obj_v.clone()) - } else if let Expr::PropertyGet { object, .. } = callee { - // Skip the method-binding when the receiver is a global, - // namespace import, or NativeModuleRef — those aren't - // user objects and shouldn't influence `this`. - if matches!( - object.as_ref(), - Expr::GlobalGet(_) | Expr::NativeModuleRef(_) | Expr::ExternFuncRef { .. } - ) { - None - } else { - Some(lower_expr(ctx, object)?) + // The receiver expression, when this call binds one. Skip the + // method-binding when the receiver is a global, namespace import, or + // NativeModuleRef — those aren't user objects and shouldn't influence + // `this`. (`receiver_must_eval_once` never matches those forms, so the + // prelowered arm cannot disagree with this test.) + let method_recv_expr: Option<&Expr> = match callee { + Expr::PropertyGet { object, .. } => { + if prelowered_recv.is_some() { + Some(object.as_ref()) + } else if matches!( + object.as_ref(), + Expr::GlobalGet(_) | Expr::NativeModuleRef(_) | Expr::ExternFuncRef { .. } + ) { + None + } else { + Some(object.as_ref()) + } } - } else { - None + _ => None, }; + let method_recv: Option = match method_recv_expr { + Some(obj_expr) => { + let v = match prelowered_recv { + Some((ref obj_v, _)) => obj_v.clone(), + None => lower_expr(ctx, obj_expr)?, + }; + // Rooted here, before the callee read below: nothing has collected + // between the lowering above and this push. + roots.push( + ctx, + obj_expr, + &v, + callee_read_collects || any_arg_collects, + ); + operand_exprs.push(obj_expr); + Some(v) + } + None => None, + }; + let recv_slot = method_recv.as_ref().map(|_| 0usize); + let callee_slot = if recv_slot.is_some() { 1 } else { 0 }; + let recv_box = if let Some((ref obj_v, ref property)) = prelowered_recv { // Read `property` off the once-lowered receiver value via the // generic by-name getter (walks the prototype chain, so @@ -1309,10 +1393,31 @@ pub fn try_lower_closure_call_fallthrough( } else { lower_expr(ctx, callee)? }; - let mut lowered_args: Vec = Vec::with_capacity(args.len()); - for a in args { - lowered_args.push(lower_expr(ctx, a)?); + roots.push(ctx, callee, &recv_box, any_arg_collects); + operand_exprs.push(callee); + + // The rebind unbox allocates (see the header above), and it sits between + // the last argument and `js_closure_callN`, so every argument's window + // includes it. Receiverless calls take the non-allocating unbox and keep + // their old IR. + let rebind_allocates = method_recv.is_some(); + for (i, a) in args.iter().enumerate() { + let v = lower_expr(ctx, a)?; + let collects = rebind_allocates || arg_collects[i + 1..].iter().any(|&c| c); + roots.push(ctx, a, &v, collects); + operand_exprs.push(a); } + + // Re-read the receiver and the callee HERE: below every argument, above the + // unbox that consumes them. Mandatory, not defensive — the temp-root slot + // is a MUTABLE root, so an evacuating cycle rewrites it and the register + // pushed beforehand is stale. + let method_recv: Option = match recv_slot { + Some(i) => Some(roots.reread_one(ctx, &operand_exprs, i)?), + None => None, + }; + let recv_box = roots.reread_one(ctx, &operand_exprs, callee_slot)?; + let prev_this: Option = if let Some(ref this_val) = method_recv { let blk = ctx.block(); Some(blk.call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, this_val)])) @@ -1334,38 +1439,58 @@ pub fn try_lower_closure_call_fallthrough( // argument's location no longer shadows this one. Applies to both arity // branches below (the checked unbox throws in either). No-op default build. crate::expr::calls::emit_call_location_at(ctx, call_byte_offset); - let result = if lowered_args.len() <= 16 { + + // #5504: tag-check the callee before masking to a closure pointer. + // A non-callable value (number/string/bool/null/undefined) whose + // low-48 bits form an in-range address would otherwise be handed to + // `js_closure_callN` as a wild `*const ClosureHeader` and SIGSEGV on + // the header read. The checked unbox throws `TypeError: value is not + // a function` for any non-`POINTER_TAG` value. + // #6475: a member-shaped call (`o.m(args)`) must rebind an + // object-literal method's baked `this` capture slot to the receiver — + // the slot wins over the IMPLICIT_THIS cell set above, so a method + // inherited via `Object.setPrototypeOf(obj, proto)` otherwise runs + // with `this` bound to the proto literal (effect's Pipeable + // `TagClass.pipe(...)` composed against the wrong `this` and + // HttpApiBuilder.group returned a curried function instead of a + // Layer). The rebind variant is a no-op for closures that don't + // capture `this`, so plain functions and arrows are untouched; + // receiverless calls keep the plain checked unbox. + // + // #7154: this is also the collection point that sits between the arguments + // and the dispatch. `clone_closure_rebind_this` ALLOCATES a replacement + // closure when the callee captures `this`, so the arguments are re-read + // below it — hoisting the unbox above the argument list instead is not an + // option, because its throw is observable and the spec evaluates arguments + // before it. + let closure_handle = { let blk = ctx.block(); - // #5504: tag-check the callee before masking to a closure pointer. - // A non-callable value (number/string/bool/null/undefined) whose - // low-48 bits form an in-range address would otherwise be handed to - // `js_closure_callN` as a wild `*const ClosureHeader` and SIGSEGV on - // the header read. The checked unbox throws `TypeError: value is not - // a function` for any non-`POINTER_TAG` value. - // #6475: a member-shaped call (`o.m(args)`) must rebind an - // object-literal method's baked `this` capture slot to the receiver — - // the slot wins over the IMPLICIT_THIS cell set above, so a method - // inherited via `Object.setPrototypeOf(obj, proto)` otherwise runs - // with `this` bound to the proto literal (effect's Pipeable - // `TagClass.pipe(...)` composed against the wrong `this` and - // HttpApiBuilder.group returned a curried function instead of a - // Layer). The rebind variant is a no-op for closures that don't - // capture `this`, so plain functions and arrows are untouched; - // receiverless calls keep the plain checked unbox. - let closure_handle = if let Some(ref this_val) = method_recv { - blk.call( + match method_recv { + Some(ref this_val) => blk.call( I64, "js_closure_unbox_callee_checked_rebind", &[(DOUBLE, &recv_box), (DOUBLE, this_val)], - ) - } else { - blk.call( + ), + None => blk.call( I64, "js_closure_unbox_callee_checked", &[(DOUBLE, &recv_box)], - ) - }; + ), + } + }; + + // Re-read the arguments BELOW the unbox. `closure_handle` itself is a raw + // pointer in a register, but nothing between here and the dispatch can + // collect, so it needs no protection of its own. + let arg_base = callee_slot + 1; + let mut lowered_args: Vec = Vec::with_capacity(args.len()); + for i in 0..args.len() { + lowered_args.push(roots.reread_one(ctx, &operand_exprs, arg_base + i)?); + } + + let result = if lowered_args.len() <= 16 { let runtime_fn = format!("js_closure_call{}", lowered_args.len()); + let blk = ctx.block(); let mut call_args: Vec<(crate::types::LlvmType, &str)> = vec![(I64, &closure_handle)]; for v in &lowered_args { call_args.push((DOUBLE, v.as_str())); @@ -1377,6 +1502,12 @@ pub fn try_lower_closure_call_fallthrough( // variadic `js_closure_call_array(closure_i64, args_ptr, argc)`. This // mirrors the `js_native_call_value` marshaling used elsewhere in // lower_call. `args_ptr` is non-null here since argc > 16 > 0. + // + // #7154: the stores happen below the unbox now. A stack buffer is not a + // GC root, so filling it above an allocating rebind would freeze + // pre-move addresses into it — the same staleness one indirection + // further out. The stores have no observable effect, so moving them + // below the throw-capable unbox changes nothing else. let n = lowered_args.len(); let buf = ctx.func.alloca_entry_array(DOUBLE, n); let blk = ctx.block(); @@ -1384,35 +1515,6 @@ pub fn try_lower_closure_call_fallthrough( let slot = blk.gep(DOUBLE, &buf, &[(I64, &format!("{}", i))]); blk.store(DOUBLE, v, &slot); } - // #5504: tag-check the callee before masking to a closure pointer. - // A non-callable value (number/string/bool/null/undefined) whose - // low-48 bits form an in-range address would otherwise be handed to - // `js_closure_callN` as a wild `*const ClosureHeader` and SIGSEGV on - // the header read. The checked unbox throws `TypeError: value is not - // a function` for any non-`POINTER_TAG` value. - // #6475: a member-shaped call (`o.m(args)`) must rebind an - // object-literal method's baked `this` capture slot to the receiver — - // the slot wins over the IMPLICIT_THIS cell set above, so a method - // inherited via `Object.setPrototypeOf(obj, proto)` otherwise runs - // with `this` bound to the proto literal (effect's Pipeable - // `TagClass.pipe(...)` composed against the wrong `this` and - // HttpApiBuilder.group returned a curried function instead of a - // Layer). The rebind variant is a no-op for closures that don't - // capture `this`, so plain functions and arrows are untouched; - // receiverless calls keep the plain checked unbox. - let closure_handle = if let Some(ref this_val) = method_recv { - blk.call( - I64, - "js_closure_unbox_callee_checked_rebind", - &[(DOUBLE, &recv_box), (DOUBLE, this_val)], - ) - } else { - blk.call( - I64, - "js_closure_unbox_callee_checked", - &[(DOUBLE, &recv_box)], - ) - }; let argc = n.to_string(); blk.call( DOUBLE, @@ -1421,6 +1523,10 @@ pub fn try_lower_closure_call_fallthrough( ) }; + // Released AFTER the dispatch, not before: the dispatcher allocates while + // it reads these values. + roots.release(ctx); + if let Some(prev) = prev_this { ctx.block() .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, &prev)]); diff --git a/scripts/gc_root_dominance_check.py b/scripts/gc_root_dominance_check.py index 9dcd084a18..644e72ed3f 100755 --- a/scripts/gc_root_dominance_check.py +++ b/scripts/gc_root_dominance_check.py @@ -257,6 +257,17 @@ def build_cfg(f): "js_array_length", # array/indexing.rs:537 "js_object_mark_class", "js_class_object_pin_parent", "js_new_target_get", "js_new_target_set", + # closure/unbox.rs:25 -- a tag check on the NaN-boxed callee and a low-48 + # mask. No allocation, no user code, no poll. It sits between every + # dynamic call's last argument and its `js_closure_callN`, so leaving it + # out reported the whole argument list of every 1-arg dynamic call as + # stale (372 of the 729 fatal sinks) with `MOVING: no`. + # + # `js_closure_unbox_callee_checked_rebind` is deliberately NOT here: it + # calls `clone_closure_rebind_this`, which allocates a replacement closure + # (closure/dynamic_props.rs:1040) when the callee captures `this`. That one + # IS a collection point, and #7154's fix re-reads the arguments below it. + "js_closure_unbox_callee_checked", # object/this_binding.rs:160 -- a thread-local cell swap "js_implicit_this_set", "js_implicit_this_get", "js_gc_note_slot_layout", "js_string_addref_if_heap_string", diff --git a/test-files/test_gap_gc_closure_call_argument_rooting.ts b/test-files/test_gap_gc_closure_call_argument_rooting.ts new file mode 100644 index 0000000000..3a55ad5467 --- /dev/null +++ b/test-files/test_gap_gc_closure_call_argument_rooting.ts @@ -0,0 +1,52 @@ +// #7154: an already-lowered ARGUMENT of a dynamic value-call must be rooted +// across the evaluation of the arguments that follow it. +// +// `f(a, g())` lowers `a` into a bare SSA register and then lowers `g()`. `g()` +// allocates, and under `PERRY_GC_MOVING_LOOP_POLLS=1` a loop back-edge poll +// inside it runs an evacuating minor: `a` survives (the capture cell holding +// it is a root) and therefore MOVES, the collector rewrites the cell, and the +// register keeps naming from-space. `js_closure_callN` then passes the +// pre-move address as argument 0 and the callee reads its fields out of +// abandoned memory. +// +// This is the third register the generic dynamic-call lowering held across its +// own argument list; #7206 named all three and fixed none of them. +// +// ISOLATED ON PURPOSE. The callee is a module-level binding assigned a plain +// function declaration, so its function object is created once at module init +// and is long tenured by the time the loop runs — it is not what moves. `inst` +// is allocated fresh per iteration and is squarely in the nursery, so argument +// 0 is the operand under test. + +function churn(x: number): number { + const bits: any[] = []; + for (let i = 0; i < 600; i++) { + bits.push({ i: i, s: "x" }); + } + return x + bits.length - 600; +} + +function add(o: any, v: number): number { + return o.tag + v; +} + +const fn: any = add; + +function make(t: number): (p: number) => number { + const inst: any = { tag: t }; + return (p: number) => fn(inst, churn(p)); +} + +function run(): number { + let bad = 0; + for (let r = 0; r < 400; r++) { + const f = make(r); + const got = f(1); + if (got !== r + 1) { + bad++; + } + } + return bad; +} + +console.log("bad", run()); diff --git a/test-files/test_gap_gc_closure_call_callee_rooting.ts b/test-files/test_gap_gc_closure_call_callee_rooting.ts new file mode 100644 index 0000000000..13aef97b14 --- /dev/null +++ b/test-files/test_gap_gc_closure_call_callee_rooting.ts @@ -0,0 +1,50 @@ +// #7154: the CALLEE of a dynamic value-call must be rooted across the +// evaluation of the call's arguments. +// +// `f(g())` evaluates the callee first and the arguments second — spec order, +// and codegen follows it — which left the callee in a bare SSA register while +// `g()` was lowered. `g()` allocates, and under `PERRY_GC_MOVING_LOOP_POLLS=1` +// a loop back-edge poll inside it runs an evacuating minor. The callee +// SURVIVES that minor (the closure capture cell holding it is a root), which +// means it MOVES: the collector rewrites the capture cell but not the caller's +// register. `js_closure_unbox_callee_checked` then masks a from-space address +// and `js_closure_callN` reads its header out of abandoned memory — +// `TypeError: value is not a function`. +// +// Same invariant as #7206, #7192, #7184 and #7114, one operand over: a GC +// value's root must dominate every subsequent collection point, and a +// rewritten location is worthless unless the code below the collection point +// READS that location again. +// +// LIVE BY CONSTRUCTION. `fn` is an `any`-typed closure read out of a capture +// cell, so the call takes the generic `js_closure_callN` fallthrough rather +// than a static direct call, and the argument allocates hard enough to reach +// the collector. A non-moving collection cannot expose this, so the evacuating +// arms are the ones that bite. + +function churn(x: number): number { + const bits: any[] = []; + for (let i = 0; i < 600; i++) { + bits.push({ i: i, s: "x" }); + } + return x + bits.length - 600; +} + +function make(t: number): (p: number) => number { + const fn: any = (v: number): number => t + v; + return (p: number) => fn(churn(p)); +} + +function run(): number { + let bad = 0; + for (let r = 0; r < 400; r++) { + const f = make(r); + const got = f(1); + if (got !== r + 1) { + bad++; + } + } + return bad; +} + +console.log("bad", run()); diff --git a/test-files/test_gap_gc_closure_call_this_rooting.ts b/test-files/test_gap_gc_closure_call_this_rooting.ts new file mode 100644 index 0000000000..b8fc4c07ac --- /dev/null +++ b/test-files/test_gap_gc_closure_call_this_rooting.ts @@ -0,0 +1,54 @@ +// #7154: the `this` RECEIVER of a dynamic value-call must be rooted across the +// read of the callee off it and across the call's argument list. +// +// `recv.m(g())` where `recv.m` is a closure-VALUED property (not a native +// method) lowers through the generic `js_closure_callN` fallthrough. That path +// evaluates the receiver, reads the callee off it, lowers the arguments, and +// only then binds the receiver as the implicit `this` and hands it to +// `js_closure_unbox_callee_checked_rebind`. The receiver sat in a bare SSA +// register for that whole span: an evacuating minor inside `g()` rewrites the +// capture cell it was read from and leaves the register naming from-space, so +// the rebind clones captures out of abandoned memory and the body's `this.tag` +// reads garbage. +// +// #7206 fixed this same operand on the sibling `js_native_call_method_by_id` +// dispatch. This is the `js_closure_callN` one — the dispatch a closure-valued +// property takes (hono's `RegExpRouter.match = match`, the #519 shape). +// +// LIVE BY CONSTRUCTION. `m` is a non-arrow function declaration assigned onto +// an object literal, so it reads `this` through the implicit-this binding and +// the call takes the fallthrough rather than the by-name method dispatch. The +// receiver, the callee and the argument list are all in flight at once here — +// this lowering held all three in registers, and the three tests in this group +// name them one at a time. + +function churn(x: number): number { + const bits: any[] = []; + for (let i = 0; i < 600; i++) { + bits.push({ i: i, s: "x" }); + } + return x + bits.length - 600; +} + +function meth(this: any, v: number): number { + return this.tag + v; +} + +function make(t: number): (p: number) => number { + const inst: any = { tag: t, m: meth }; + return (p: number) => inst.m(churn(p)); +} + +function run(): number { + let bad = 0; + for (let r = 0; r < 400; r++) { + const f = make(r); + const got = f(1); + if (got !== r + 1) { + bad++; + } + } + return bad; +} + +console.log("bad", run()); From e0fce625247810878c9bd4aac3ed074c85792fb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 23:43:53 +0200 Subject: [PATCH 2/2] =?UTF-8?q?chore(7214):=20merge-time=20fixes=20?= =?UTF-8?q?=E2=80=94=20fragment=20name,=20rustfmt,=20corpus=20registration?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - changelog fragment was PR-misnumbered `7207-`; #7207 is a different, already merged change. Renamed to `7214-`. Content already referenced #7206 correctly and is unchanged. - `cargo fmt --all -- --check` is a required check on `lint`; one hand-wrapped `roots.push` call needed re-wrapping. No behaviour change. - Registered the three witnesses in the GC x repsel corpus, next to #7206's pair. All three are moving-only: clean on the shipped default on both sides of the fix, so they belong with the `requires=move` rows and prove nothing on `default`. --- ... => 7214-closure-calln-stale-registers.md} | 0 .../src/lower_call/console_promise.rs | 7 +---- test-parity/gc_repsel_corpus.txt | 30 +++++++++++++++++++ 3 files changed, 31 insertions(+), 6 deletions(-) rename changelog.d/{7207-closure-calln-stale-registers.md => 7214-closure-calln-stale-registers.md} (100%) diff --git a/changelog.d/7207-closure-calln-stale-registers.md b/changelog.d/7214-closure-calln-stale-registers.md similarity index 100% rename from changelog.d/7207-closure-calln-stale-registers.md rename to changelog.d/7214-closure-calln-stale-registers.md diff --git a/crates/perry-codegen/src/lower_call/console_promise.rs b/crates/perry-codegen/src/lower_call/console_promise.rs index 3d8da99098..dba208d8ee 100644 --- a/crates/perry-codegen/src/lower_call/console_promise.rs +++ b/crates/perry-codegen/src/lower_call/console_promise.rs @@ -1359,12 +1359,7 @@ pub fn try_lower_closure_call_fallthrough( }; // Rooted here, before the callee read below: nothing has collected // between the lowering above and this push. - roots.push( - ctx, - obj_expr, - &v, - callee_read_collects || any_arg_collects, - ); + roots.push(ctx, obj_expr, &v, callee_read_collects || any_arg_collects); operand_exprs.push(obj_expr); Some(v) } diff --git a/test-parity/gc_repsel_corpus.txt b/test-parity/gc_repsel_corpus.txt index 2b953831e1..083b437818 100644 --- a/test-parity/gc_repsel_corpus.txt +++ b/test-parity/gc_repsel_corpus.txt @@ -232,3 +232,33 @@ test_gap_gc_closure_this_capture_rooting # the `requires=move` arms and prove nothing on `default`. test_gap_gc_method_receiver_rooting test_gap_gc_index_get_receiver_rooting + +# --- Stale REGISTERS on the generic dynamic call (#7214) --------------------- +# The same class as #7206 above, on the hottest call path the compiler emits. +# `js_closure_callN` held THREE classes of GC value in bare registers, each +# across a DIFFERENT window: +# +# receiver | the callee read off it + every argument +# callee | every argument +# argument | the arguments after it + the rebind unbox +# +# The rebind unbox is a collection point that exists only on the member-shaped +# path: `js_closure_unbox_callee_checked_rebind` calls +# `clone_closure_rebind_this`, which allocates a replacement closure when the +# callee captures `this`. It sits BELOW the last argument and ABOVE the +# dispatch, so the arguments are re-read after it. The receiverless path takes +# `js_closure_unbox_callee_checked` -- a tag check and a mask that allocates +# nothing -- so `f(x, y)` on inert operands emits byte-identical IR. +# +# Measured with #7206 applied but NOT #7214, i.e. these are exactly the sites +# #7206 left open. Compiled AND run with `PERRY_GC_MOVING_LOOP_POLLS=1`, +# oracle node 26.5.1 (`bad 0` for all three): +# closure_call_callee_rooting `TypeError: value is not a function` +# closure_call_this_rooting `TypeError: value is not a function` +# closure_call_argument_rooting `TypeError: value is not a function` +# All three are `bad 0` with #7214 applied, including under PERRY_GC_ZEAL=1, +# and clean on the shipped default on BOTH sides -- so they certify nothing on +# the `default` arm and belong with the `requires=move` rows. +test_gap_gc_closure_call_callee_rooting +test_gap_gc_closure_call_this_rooting +test_gap_gc_closure_call_argument_rooting