From fb5c3db0b03cc28035c947d317203d1a73e66858 Mon Sep 17 00:00:00 2001 From: jdalton Date: Sat, 1 Aug 2026 15:03:45 -0400 Subject: [PATCH 1/4] 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/4] =?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 From 32d2a015f75bb22eb4f202186a542b77e15e4c84 Mon Sep 17 00:00:00 2001 From: jdalton Date: Sat, 1 Aug 2026 14:40:29 -0400 Subject: [PATCH 3/4] ci(gc): make the root-dominance gate able to fail, baseline it honestly, and document the invariant Squashed. See PR description for the seeded-violation proof and the allowlist rationale. --- .github/workflows/gc-root-dominance.yml | 110 ++-- .gitignore | 2 + CLAUDE.md | 2 +- changelog.d/7212-gc-root-dominance-gate.md | 65 ++ docs/src/SUMMARY.md | 2 + docs/src/internals/gc-rooting-invariant.md | 250 ++++++++ .../internals/rfc-rooting-by-construction.md | 248 +++++++ scripts/gc_root_dominance_allowlist.json | 51 ++ scripts/gc_root_dominance_check.py | 605 +++++++++++++++++- scripts/gc_root_dominance_corpus.sh | 147 +++++ 10 files changed, 1397 insertions(+), 85 deletions(-) create mode 100644 changelog.d/7212-gc-root-dominance-gate.md create mode 100644 docs/src/internals/gc-rooting-invariant.md create mode 100644 docs/src/internals/rfc-rooting-by-construction.md create mode 100644 scripts/gc_root_dominance_allowlist.json create mode 100755 scripts/gc_root_dominance_corpus.sh diff --git a/.github/workflows/gc-root-dominance.yml b/.github/workflows/gc-root-dominance.yml index 9774979c33..541f28c2c6 100644 --- a/.github/workflows/gc-root-dominance.yml +++ b/.github/workflows/gc-root-dominance.yml @@ -19,16 +19,37 @@ name: GC Root Dominance # # 1. no `continue-on-error`, no `|| true`, no pipe between the checker and # the shell's exit status; -# 2. NOT yet in branch protection's required contexts — deliberately, because -# a new gate has never been green and promoting it immediately blocks every -# open PR. Promote after one clean week on `main`; +# 2. NOT yet in branch protection's required contexts. This was deliberate at +# first — a new gate has never been green, so promoting it immediately +# blocks every open PR — but the second step was never taken, and in the +# meantime the job WAS red on `main` and blocked nothing: the five +# ClassExprFresh hits now tracked in #7211 have been reported on every run +# since #7198 and went unread. That is hazard 2 doing exactly what the +# corollary in CLAUDE.md warns about. +# +# **ACTION REQUIRED, and it is not something this workflow can do to +# itself**: with the allowlist below the job is green on `main`, so a +# repo admin must add `gc-root-dominance` to branch protection's required +# contexts. Until that happens this file is documentation, not a gate. +# See docs/src/internals/gc-rooting-invariant.md, "Promoting this gate". # 3. `concurrency` cancels pull-request runs only, never `main` runs; -# 4. the subject is ASSERTED live, not assumed. `--self-test` proves the -# checker still reports a planted violation and still clears the control, -# and `--min-files` / `--min-binds` refuse a clean verdict over a corpus -# that contained no modules or no root stores. An empty `.perry-trace/llvm` -# is a routine outcome of a failed compile, so "0 violations" over 0 files -# must be an error rather than a pass. +# 4. the subject is ASSERTED live, not assumed, at three levels. +# `--self-test` proves the checker still reports a planted violation in +# hand-written IR and still clears the control. `--seeded-violations` +# goes further and plants collection points into the REAL corpus, +# requiring every one to be reported -- that is what catches the case +# where perry's emitted IR drifts to a shape the parser can no longer +# read, which frozen fixtures cannot detect. And `--min-files` / +# `--min-binds` / `--min-funcs` refuse a clean verdict over a corpus with +# too few modules, root stores or functions to have exercised anything. +# An empty `.perry-trace/llvm` is a routine outcome of a failed compile, +# so "0 violations" over 0 files must be an error rather than a pass. +# +# Known-remaining violations live in scripts/gc_root_dominance_allowlist.json, +# one named entry each with an issue and a written justification -- NOT a +# numeric threshold, which cannot tell a new violation from an old one. An +# entry that matches nothing fails the build, so a fixed bug's entry must be +# deleted rather than left to widen coverage later. on: pull_request: @@ -93,60 +114,35 @@ jobs: || { echo "::error::target/release/$artifact was not produced"; exit 1; } done + # The source list and the env knobs live in the script, not here, so that + # reproducing a CI failure is one command rather than a re-read of this + # YAML. A retyped invocation that drops PERRY_GC_MOVING_LOOP_POLLS + # produces IR in which the bug is not expressible at all, and the local + # run then "cannot reproduce" a real finding. - name: Emit the IR corpus - env: - # PERRY_GC_MOVING_LOOP_POLLS=1 is what puts `js_gc_loop_safepoint` in - # the IR, which is what the MOVING classification keys on. It is off - # by default (#7161 stopgap), so without it this gate would run over - # IR that cannot express the bug — hazard 4 again. - PERRY_GC_MOVING_LOOP_POLLS: "1" - # Makes every root store the @js_shadow_slot_bind call form. The #7088 - # inline diamond is equivalent but harder to anchor on. - PERRY_INLINE_SHADOW_SLOT: "0" - PERRY_NO_AUTO_OPTIMIZE: "1" - run: | - set -euo pipefail - mkdir -p ir-corpus - # A spread of shapes that exercise the lowerings this invariant runs - # through: construction, object/array literals and spreads, class - # expressions with statics, property and element stores, closures. - # Kept to test-files/ so the corpus is versioned with the repo rather - # than depending on a private workload. - shopt -s nullglob - sources=( - test-files/test_gap_gc_*.ts - test-files/test_gap_class*.ts - test-files/test_gap_object*.ts - test-files/test_gap_static*.ts - test-files/test_gap_prop*.ts - ) - if [ "${#sources[@]}" -eq 0 ]; then - echo "::error::no corpus sources matched; the glob is stale" - exit 1 - fi - for src in "${sources[@]}"; do - name="$(basename "$src" .ts)" - rm -rf .perry-trace/llvm - # A source that fails to compile must not silently shrink the - # corpus: --min-files below is the backstop, but say so here too. - if ! ./target/release/perry compile "$src" -o "/tmp/$name" --trace llvm >/dev/null 2>&1; then - echo "::warning::$src did not compile; skipping" - continue - fi - for ll in .perry-trace/llvm/*.ll; do - cp "$ll" "ir-corpus/${name}__$(basename "$ll")" - done - done - echo "corpus: $(find ir-corpus -name '*.ll' | wc -l) .ll files" + run: ./scripts/gc_root_dominance_corpus.sh ir-corpus - name: Check root-store dominance run: | set -euo pipefail - # No pipe: the checker's own exit status is the job's. --min-binds - # asserts the corpus actually contained root stores, so a green - # verdict cannot come from IR that never had a subject. + # No pipe: the checker's own exit status is the job's. + # + # The floors are asserted, not hoped for. On the corpus as of this + # commit the run reports ~1993 functions / 117 modules / 2501 root + # stores, so these sit below that with room for churn and well above + # "something compiled". Raise them when the corpus grows; never lower + # one to make a run pass -- a shrinking corpus is the finding. + # + # --seeded-violations plants 40 collection points into this very IR + # and requires all 40 to be reported. That is the arm that fails if + # the checker has stopped understanding perry's output, which is the + # only way a green verdict here could be a lie. python3 scripts/gc_root_dominance_check.py ir-corpus \ - --moving-only --min-files 5 --min-binds 50 -v + --moving-only \ + --min-files 90 --min-binds 1500 --min-funcs 1200 \ + --allowlist scripts/gc_root_dominance_allowlist.json \ + --seeded-violations 40 \ + -v - name: Upload the IR corpus on failure if: failure() diff --git a/.gitignore b/.gitignore index 227196cd75..f1a078cbc8 100644 --- a/.gitignore +++ b/.gitignore @@ -40,6 +40,8 @@ benchmarks/suite/assets/ # `perry compile --trace llvm` dumps per-module .ll files here. .perry-trace/ +# scripts/gc_root_dominance_corpus.sh output (regenerate, never commit) +ir-corpus/ # Compiled test executables in the project root (no extension) /test_* diff --git a/CLAUDE.md b/CLAUDE.md index 29fa33cb2a..ba0339693d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -250,4 +250,4 @@ Corollary: a *new* gate has never been green, so promoting it to required immedi - **Async-to-generator transform, body locals.** It boxes every body local into a shared mutable cell typed `Any`. Two consequences seen in the wild: per-iteration `let`/`const` bindings collapse for closures created in a loop, and computed numeric-key calls (`arr[i](x)`) lose their type proof and silently resolve by *method name*, evaporating the call. - **Native base-class subclassing.** A native base's surface is installed at `super()` time and its parent edge lives in the class registry; keying any of that on a literal `extends` name loses it for fieldless classes, indirect subclasses, and class expressions. - **Two prototype-resolution paths.** `CLASS_PROTOTYPE_OBJECTS` (synthetic: `Object.create`, plain-function ctors) vs `CLASS_DECL_PROTOTYPE_OBJECTS` (declared classes). `in`/`for…in` and `getPrototypeOf` have disagreed about the same chain. -- **Root-store dominance in codegen.** *A GC-managed value's root store must **dominate** every subsequent site that can collect.* Three ways it has broken, all shipped: the store's slot index fell outside the pushed shadow frame so `js_shadow_slot_bind` bounds-checked it into a silent no-op (#7184); the store was emitted in-frame but **after** a call that allocates (#7192); and the value lives in a plain `alloca_entry` that is neither a shadow slot nor a temp root, so the collector never rewrites it (`lower_call/new.rs`'s inline-ctor `this_slot`, still open). All three present identically — a *rooted* slot holding a dangling pointer, surfacing cycles later as `TypeError: value is not a function` — and **none is visible to any runtime GC probe**, because at the moment of the collection there is nothing for the collector to find. That is why #7154's from-space scan only ever saw offenders whose targets had already died. The instrument is static: `scripts/gc_root_dominance_check.py` over `--trace llvm` output (`--self-test` proves it can still fail). Only bites under `PERRY_GC_MOVING_LOOP_POLLS=1`, off by default since #7161 — so a green default run says nothing about this class. +- **Root-store dominance in codegen.** *A GC-managed value's root store must **dominate** every subsequent site that can collect.* Three ways it has broken, all shipped: the store's slot index fell outside the pushed shadow frame so `js_shadow_slot_bind` bounds-checked it into a silent no-op (#7184); the store was emitted in-frame but **after** a call that allocates (#7192); and the value lives in a plain `alloca_entry` that is neither a shadow slot nor a temp root, so the collector never rewrites it (`lower_call/new.rs`'s inline-ctor `this_slot`, still open). All three present identically — a *rooted* slot holding a dangling pointer, surfacing cycles later as `TypeError: value is not a function` — and **none is visible to any runtime GC probe**, because at the moment of the collection there is nothing for the collector to find. That is why #7154's from-space scan only ever saw offenders whose targets had already died. The instrument is static: `scripts/gc_root_dominance_check.py` over `--trace llvm` output (`--self-test` proves it can still fail). Only bites under `PERRY_GC_MOVING_LOOP_POLLS=1`, off by default since #7161 — so a green default run says nothing about this class. **Full writeup, all five known shapes and how to check your work: `docs/src/internals/gc-rooting-invariant.md`.** The CI gate is `gc-root-dominance.yml` over `scripts/gc_root_dominance_corpus.sh`; known-remaining hits are named one-per-entry in `scripts/gc_root_dominance_allowlist.json` (an entry that matches nothing FAILS, so a fix must delete its entry). A fifth shape is open as #7211: `ClassExprFresh` roots only when it thinks the static *initializers* collect, and never asks whether its own emitted `js_object_set_field_by_name` does — the sophisticated version of the mistake, where the author wrote a rooting predicate and it asked the wrong question. diff --git a/changelog.d/7212-gc-root-dominance-gate.md b/changelog.d/7212-gc-root-dominance-gate.md new file mode 100644 index 0000000000..1ddf161a7f --- /dev/null +++ b/changelog.d/7212-gc-root-dominance-gate.md @@ -0,0 +1,65 @@ +### CI: the GC root-dominance gate can now fail, and is baselined honestly + +The static root-dominance checker added in #7198 had been **red on `main` ever +since it merged**, and blocked nothing: `gc-root-dominance` is not in branch +protection's required contexts, so the job reported failure without being able +to turn a merge red. That is hazard 2 from CLAUDE.md's "four ways a gate can be +unable to fail", and the corollary it warns about — run a new gate once, *then* +promote it; leaving the second step undone is how hazard 2 happens. + +The five violations it had been reporting all along are real, and are now +tracked as #7211: `Expr::ClassExprFresh` roots its class object only when it +believes the static *initializers* can collect, and never asks whether the +lowering's own emitted `js_object_set_field_by_name` can. A class expression +whose statics are inert (`class C { static tag = tag }`) therefore holds the +object in a register across a collection point. `js_object_mark_class` does not +rescue it: that helper roots `CLASS_OBJECT_VALUES`' own copy, which keeps the +object alive and forwarded while leaving the register stale. Reachability is not +the invariant. + +**Corpus** (`scripts/gc_root_dominance_corpus.sh`, new) — emission moves out of +the workflow so that reproducing a CI failure is one command rather than a +re-read of the YAML; an invocation retyped without `PERRY_GC_MOVING_LOOP_POLLS=1` +produces IR in which the bug is not expressible at all. Grown from 41 to 117 +`.ll` files / 1993 functions / 2501 root stores over 99 sources, selected for the +lowerings this invariant runs through. A stale glob is a hard error and the +compiled-source count has an explicit floor. + +**Allowlist** (`scripts/gc_root_dominance_allowlist.json`, new) — one named entry +per known-remaining hit, each with an issue and a written justification, instead +of a numeric threshold. A threshold cannot distinguish a new violation from an +old one, and the cheapest way to green a red build is to raise it by one. The +checker enforces that an entry matching nothing **fails** (so a fix must delete +its entry — that is the ratchet), that an entry suppresses at most its `count`, +and that an unnamed violation fails regardless of the total. + +**Proof of failure** — `--seeded-violations N` splices synthetic collection +points into the *real* corpus IR between an allocation and its root store and +requires every one to be reported. `--self-test` only proves the checker fires on +frozen fixtures, which keeps passing even if perry's emitted IR drifts to a shape +the parser can no longer read; that is the case where the gate reports a serene +`violations: 0` over IR it is not analysing. `--self-test` additionally gained +arms covering the allowlist's anti-absorption properties. + +**Visibility** — `--min-funcs` plus a `checked N functions / M modules` summary +line, so a silently-empty or silently-shrunken run is impossible to mistake for a +clean one. + +Verified: green on `main` with the allowlist; red without it; red with any single +entry removed or its `count` lowered; exit 2 on a stale entry or an empty corpus; +40/40 seeded violations caught. Corpus emission ~80s, the check ~3s. + +**Still required, and not something the workflow can do to itself:** a repo admin +must add `gc-root-dominance` to branch protection's required contexts. + +### Docs + +- `docs/src/internals/gc-rooting-invariant.md` — the rule stated plainly for + codegen authors, with all five real bugs as case studies, the symptom each + produces, and how to check your work. Includes the false-green caveat on + `PERRY_GC_PROTECT_FROMSPACE_DEPTH` (the default of 4 is not enough; use 800). +- `docs/src/internals/rfc-rooting-by-construction.md` — design proposal for + making the bug unrepresentable: V8's `Handle`/`HandleScope` discipline + expressed through Rust's borrow checker, so that using an unrooted value + across a collection point is a compile error. Four of the five real bugs would + be caught by construction. Proposal only; nothing implemented. diff --git a/docs/src/SUMMARY.md b/docs/src/SUMMARY.md index 6c1d7678ac..9b5cc5cbdc 100644 --- a/docs/src/SUMMARY.md +++ b/docs/src/SUMMARY.md @@ -174,6 +174,8 @@ - [Memory Model](internals/memory-model.md) - [Explicit Memory Control](internals/explicit-memory.md) +- [The GC rooting invariant (codegen)](internals/gc-rooting-invariant.md) +- [RFC: rooting by construction](internals/rfc-rooting-by-construction.md) # Contributing diff --git a/docs/src/internals/gc-rooting-invariant.md b/docs/src/internals/gc-rooting-invariant.md new file mode 100644 index 0000000000..a9766123f9 --- /dev/null +++ b/docs/src/internals/gc-rooting-invariant.md @@ -0,0 +1,250 @@ +# The GC rooting invariant (codegen) + +Read this before you emit a call from a lowering. + +## The rule + +> **Any GC-managed value that is live across a collection point must be +> reachable from a root before that point.** +> +> A value read out of a root and held in an SSA register across a call **is not +> rooted**. It is a copy, and the collector cannot see copies. + +Perry's GC moves objects. When an evacuating minor runs, it walks the roots, +copies live objects to old-gen, and **rewrites every reference it can reach**. +Anything it cannot reach keeps the old address. That address now points into +from-space, which is about to be reused. + +A "collection point" is any of: + +- an allocation (`js_object_alloc`, `js_array_alloc`, `js_closure_alloc`, string + concatenation, boxing — anything that can take an arena block); +- a call that can allocate, which in practice means **almost every runtime + helper**. `js_object_set_field_by_name` allocates: it performs the keys-array + transition. `js_object_get_property` allocates: it can run a getter, which is + user code; +- `js_gc_loop_safepoint`, the back-edge poll (only emitted under + `PERRY_GC_MOVING_LOOP_POLLS=1`, off by default since #7161). + +The safe default is that **a call collects unless you have read the runtime +source and proved otherwise**. The checker described below encodes exactly this +bias: its `NONCOLLECTING` set is the only place a call is declared safe, and +every entry names the runtime line that proves it. + +## Why this class of bug is so expensive + +Every violation presents the same way and none of it points at the code that is +wrong: + +- the symptom is `TypeError: value is not a function`, or a SIGSEGV, **cycles + later and somewhere else** — wherever the stale pointer is finally + dereferenced; +- **no runtime GC probe can see it.** At the moment of the collection there is + nothing for the collector to find, so a from-space scan, a verify-roots pass + and a zeal run all come back clean. `PERRY_GC_VERIFY_EVACUATION` checks that + reachable slots were forwarded; it cannot check a register it does not know + exists; +- it is **invisible by default**, because the back-edge poll that triggers it is + off. A green default test run says nothing about this class. + +Four instances shipped in a single day. The detection lag, not the fix, was the +cost every time. + +## The four ways it has actually broken + +### 1. Slot index past the frame (#7184) + +The root store was emitted, and it looked right. But the slot index fell outside +the frame pushed by `js_shadow_frame_enter`, so `js_shadow_slot_bind` +bounds-checked it and **silently returned**. The value was never rooted; the IR +says it was. + +*Tell:* a `js_shadow_slot_bind(i32 N, …)` where `N >= the frame size`. There is +no diagnostic — the bind is a no-op by design, because a bounds-check that +panicked would be worse. + +### 2. Root store after a collecting call (#7192) + +The store was in-frame and correct, but emitted **after** a call that allocates. +Between the allocation and the store, the value lived only in a register. + +```llvm +%obj = call ptr @js_object_alloc(i32 4) +%ret = call double @js_call_function(double %a) ; can evacuate %obj +store ptr %obj, ptr %slot ; stores the OLD address +call void @js_shadow_slot_bind(i32 0, ptr %slot) ; roots a dangling pointer +``` + +*Tell:* the resulting slot is *rooted* and *dangling* at the same time, which is +why it survives every "is it rooted?" check. + +### 3. Method receiver across the argument list (#7206) + +The receiver was loaded out of its root, then the argument expressions were +lowered — each of which can allocate — and only then was the call emitted with +the receiver still in the register loaded before the arguments. + +*Tell:* a `load` from a root slot, followed by any lowering of a sub-expression, +followed by a use of the loaded register. **Re-read the root after every +collection point** instead of caching the load. + +### 4. Computed-read base across the key expression (#7206) + +`base[key]` — the base was materialized, then the *key* expression was lowered +(allocating a string, say), then the element read used the stale base. + +*Tell:* two operands where one is evaluated first and used last. + +### And the one that is still open (#7211) + +`Expr::ClassExprFresh` roots its class object only when it believes the static +*initializers* can collect: + +```rust +let protect_handle = !captured_args.is_empty() + || !symbol_statics.is_empty() + || !block_fns.is_empty() + || any_may_trigger_gc(ctx, named_statics.iter().map(|(_, v)| v)); +``` + +Every disjunct asks about code the *author* supplied. None asks whether the +lowering's **own emitted calls** collect — and the loop below unconditionally +emits one `js_object_set_field_by_name` per static, which does. So +`class C { static tag = tag }`, whose only initializer is an inert `LocalGet`, +takes `protect_handle == false` and goes stale. + +This one is worth internalising, because it is the sophisticated version of the +mistake: the author *did* think about rooting, wrote a predicate for it, and the +predicate asked the wrong question. + +> **`js_object_mark_class` does not save it.** That helper puts the object in +> `CLASS_OBJECT_VALUES`, which is a registered root and *is* forwarded. The +> object stays alive and the side table's copy stays correct — and the register +> is still stale, because the collector rewrote a different copy. +> +> **Reachability is not the invariant.** The invariant is that the register you +> are still going to use was rewritten. A side table roots *its* pointer, not +> yours. + +## How to check your work + +### 1. The static checker — run this one + +It is the only instrument that sees this class before it crashes. + +```bash +cargo build --release -p perry -p perry-runtime-static -p perry-stdlib-static +./scripts/gc_root_dominance_corpus.sh ir-corpus +python3 scripts/gc_root_dominance_check.py ir-corpus --moving-only \ + --allowlist scripts/gc_root_dominance_allowlist.json -v +``` + +It parses the emitted LLVM IR, builds per-function CFGs, computes real +Cooper/Harvey/Kennedy dominance, and reports any root store that does not +dominate a preceding collection point. It is one-sided: an unrecognised call +counts as collecting, so a gap in its model costs a false positive, never a +missed bug. + +For a single file you are iterating on: + +```bash +PERRY_GC_MOVING_LOOP_POLLS=1 PERRY_INLINE_SHADOW_SLOT=0 \ + ./target/release/perry compile mycase.ts -o /tmp/mycase --trace llvm +python3 scripts/gc_root_dominance_check.py .perry-trace/llvm -v +``` + +Both env knobs matter. `PERRY_GC_MOVING_LOOP_POLLS=1` is what puts +`js_gc_loop_safepoint` in the IR, which the `MOVING` classification keys on; +without it the corpus **cannot express the bug**. `PERRY_INLINE_SHADOW_SLOT=0` +makes every root store the `js_shadow_slot_bind` call form the checker anchors +on. + +`--stale-registers` (#7206) additionally catches values that are *never* rooted +— read out of a root and held in a register across a collection point. That is +the mode that found cases 3 and 4. + +### 2. The runtime instruments — second, and mind the depth + +From #7196: + +- `PERRY_GC_ZEAL=1` — collect at every safepoint. Slow, thorough. +- `PERRY_GC_PROTECT_FROMSPACE=1` — `mprotect` from-space after evacuation so a + stale read faults immediately instead of reading plausible garbage. +- `PERRY_GC_FROMSPACE_SCAN_ABORT` — now actually runs. + +> **`PERRY_GC_PROTECT_FROMSPACE_DEPTH` defaults to 4, and that default produces +> FALSE GREENS.** Four levels of retained from-space is not enough to still be +> holding the block your stale pointer is in by the time it is dereferenced. +> **Use 800.** A clean run at the default depth means nothing. + +And remember the ceiling on all of these: if the collection happens while the +only copy is in a register, there is nothing at that moment for any runtime +probe to notice. These instruments catch the *consequence*, later. The static +checker catches the *cause*, now. + +## The CI gate + +`.github/workflows/gc-root-dominance.yml` runs the checker on every PR over a +versioned corpus of ~99 `test-files/` sources chosen for the lowerings they +exercise (class expressions, constructors, object/array literals, computed +reads, property stores, closures, dynamic dispatch). The whole job is a few +minutes; the check itself is about three seconds over ~2000 functions. + +It is built to be able to fail, against all four hazards in CLAUDE.md: + +- the checker's exit status is the job's — no `continue-on-error`, no pipe; +- `concurrency` cancels pull-request runs only, so `main` runs are never starved; +- `--min-files` / `--min-binds` / `--min-funcs` refuse a clean verdict over a + corpus too thin to have exercised anything, and the run prints + `checked N functions / M modules` so a silently-empty run is visible; +- `--self-test` proves it still fires on planted fixtures, and + `--seeded-violations 40` splices collection points into the **real** corpus IR + and requires all 40 to be reported — that is the arm that catches the checker + silently losing the ability to read perry's output. + +### The allowlist, and why it is not a number + +Known-remaining violations live in `scripts/gc_root_dominance_allowlist.json`, +one entry each with a fingerprint, an issue, and a written justification. + +A numeric threshold cannot tell a new violation from an old one: fix one bug, +introduce another, and the total is unchanged and the gate stays green. Worse, +under deadline the cheapest way to green a red build is to raise the number by +one, and nothing in the diff says what was conceded. + +So the checker enforces three properties: + +1. **an entry that matches nothing fails the build.** When you fix the bug, + delete the entry in the same PR. That is the ratchet, and it is why a fixed + bug cannot leave a tombstone that quietly widens coverage later. +2. **an entry suppresses at most its `count`.** A second violation of the same + shape in the same function is new, and fails. +3. **a violation with no entry fails**, regardless of how many entries exist. + +Adding an entry is a code-review event. Bumping a `count` to green a build is +the exact thing this file exists to prevent. + +### Promoting this gate + +**As of this writing the job is NOT in branch protection's required contexts**, +which means it cannot turn a merge red — hazard 2, and the reason the #7211 hits +sat unread on `main` from #7198 onward while the job was visibly failing. + +With the allowlist the job is green on `main`, so the remaining step is for a +repo admin to add `gc-root-dominance` to the required contexts. A workflow +cannot do this to itself. Until it is done, this is documentation. + +## Rules of thumb + +- **Root before you call, not after.** If a value must survive a call, its root + store belongs above the call, unconditionally. Do not predicate it on a + cleverness about which callees collect — that is bug #5. +- **Re-read the root after every collection point.** Never cache a load out of a + root slot across a call. `rooted_handle_get` exists for this. +- **Evaluate-then-allocate is the hazard.** Any lowering with two or more + operands where one is materialized before another is lowered needs the first + one rooted. +- **`--trace llvm` and read it.** Three seconds of the checker beats a day of + bisecting a `not a function` five cycles downstream. +- **When in doubt, root it.** A redundant shadow slot costs a store. A missing + one costs a day, and it costs it to whoever hits the crash, not to you. diff --git a/docs/src/internals/rfc-rooting-by-construction.md b/docs/src/internals/rfc-rooting-by-construction.md new file mode 100644 index 0000000000..3132f9d891 --- /dev/null +++ b/docs/src/internals/rfc-rooting-by-construction.md @@ -0,0 +1,248 @@ +# RFC: rooting by construction + +**Status:** proposal. Nothing in this document is implemented. +**Problem:** [The GC rooting invariant](gc-rooting-invariant.md) — #7154, #7184, +#7192, #7206, #7211. + +## The case + +Five instances of one bug in about a day. Each was found by a different means, +each took hours to localise, and each fix was two lines. The fixes are not the +cost; the *representability* is. Today a lowering author can write the wrong +thing, and nothing between their keyboard and a crash five GC cycles later +objects. + +The current defences are all detection, and they run at increasing distance from +the mistake: + +| defence | catches | latency | +|---|---|---| +| code review | what a reviewer happens to notice | minutes, unreliable | +| `gc_root_dominance_check.py` | dominance violations in emitted IR | one CI run | +| `PERRY_GC_ZEAL` / from-space protect | the *consequence*, if timing cooperates | a test run, flaky | +| a user's crash | everything, eventually | days | + +The static checker is a genuine improvement and should stay. But it is still a +post-hoc pass over generated artefacts: it tells you the IR you produced is +wrong, not that the code you wrote cannot produce it. V8 made the opposite +choice with `Handle` / `HandleScope` / `DisallowGarbageCollection`, and the +reason is instructive — V8 has far more GC-touching call sites than perry, and +manages them with a type discipline rather than with a linter. + +**The question this RFC answers: can perry's Rust codegen make an unrooted live +value across a collection point fail to compile?** + +Short answer: yes, for four of the five real bugs, with a change that is +mechanical but wide. + +## Why the type system is currently absent + +Perry's codegen represents an SSA value as a **`String`**: + +```rust +let obj = ctx.block().call(I64, "js_object_alloc", &[(I32, &tcid), (I32, &n)]); +// obj: String -- the register name, e.g. "%r10" +ctx.block().call_void("js_object_set_field_by_name", &[(I64, &obj), ...]); +``` + +`String` is `Clone`, has no lifetime, and carries no information about what it +holds or whether it is still valid. Every value in the emitter — an `i32` loop +counter, a `double`, a GC pointer, a slot index — has the same type. There is +nothing for a rule to attach to. That is the root cause of the *class*, as +distinct from the root cause of any one bug. + +There are ~2500 builder call sites (`~2080` `.call(`, `~416` `.call_void(`) +across 35 files in `crates/perry-codegen/src`. + +## Proposed API + +Three types and one rule. + +```rust +/// A register holding a GC-managed value that is NOT rooted. +/// +/// Borrows the emitter immutably. Not Clone, not Copy. +pub struct Raw<'e> { + reg: String, + _emitter: PhantomData<&'e Emitter>, +} + +/// A shadow-slot root. Outlives collection points; cannot be read directly. +pub struct Rooted { + slot: SlotIdx, // only obtainable from ShadowFrame::alloc_slot +} + +/// A register holding something the GC does not manage: i32, double, bool, +/// a slot index. Freely copyable, no lifetime. +pub struct Plain(String); +``` + +The whole design rests on **splitting the emitter's methods by whether they can +collect**: + +```rust +impl Emitter { + /// Cannot collect. Takes &self, so outstanding `Raw` handles stay valid. + pub fn emit_pure(&self, ...) -> Plain { ... } + + /// CAN collect. Takes &mut self, which ends every outstanding `Raw` borrow. + pub fn emit_call(&mut self, sig: CollectingCall, args: &[Arg]) -> Raw<'_> { ... } +} + +impl Rooted { + /// Re-read the slot. The returned Raw is valid until the next &mut emit. + pub fn get<'e>(&self, e: &'e Emitter) -> Raw<'e> { ... } +} + +impl<'e> Raw<'e> { + /// Consume this register into a root. The only way to make a Rooted. + pub fn root(self, e: &mut Emitter, frame: &mut ShadowFrame) -> Rooted { ... } +} +``` + +The rule falls out of the borrow checker with no new machinery: + +> A `Raw<'e>` holds a shared borrow of the emitter. Emitting anything that can +> collect requires `&mut`. Therefore **a `Raw` cannot be used across a +> collection point** — the compiler rejects it. + +```rust +let obj = e.emit_call(OBJECT_ALLOC, &[..]); // Raw<'_>, borrows e +e.emit_call(SET_FIELD, &[obj.arg(), ..]); // needs &mut e +let boxed = obj.nanbox(&e); // ERROR: obj borrows e, + // which is mutably + // borrowed above +``` + +The fix is the correct code, and it is the shortest path out of the error: + +```rust +let obj = e.emit_call(OBJECT_ALLOC, &[..]).root(&mut e, &mut frame); +e.emit_call(SET_FIELD, &[obj.get(&e).arg(), ..]); +let boxed = obj.get(&e).nanbox(&e); // re-read, correct +``` + +Note that `Rooted::get` returning a fresh `Raw<'e>` also enforces the *second* +half of the contract that `temp_root.rs` documents today in prose: **re-read +after every collection point**, never cache the load. A cached `Raw` simply does +not survive the next `&mut`. + +### Implementation note + +`emit_pure` taking `&self` while appending to the instruction buffer needs +interior mutability — a `RefCell>` inside `Emitter`. That is the one +piece of real machinery this design requires, and it is contained to the +builder. The `RefCell` is never held across a call into user code, so the +runtime borrow panics are not a practical hazard. + +`CollectingCall` vs pure is decided by a table with the same one-sided bias the +checker already uses: **a callee is collecting unless it appears in a +`NON_COLLECTING` list whose every entry names the runtime line that proves it.** +That list already exists, in `gc_root_dominance_check.py`. It should move into +Rust and become the single source of truth both consume. + +## Would it have caught the real bugs? + +| bug | shape | caught? | +|---|---|---| +| **#7192** root store after a collecting call | `%obj` used after `js_call_function` | **Yes.** `Raw` used after `&mut` emit — borrow error. | +| **#7206a** method receiver across the argument list | receiver loaded, args lowered, receiver used | **Yes.** Lowering an argument is an `&mut` emit; the receiver `Raw` is dead. Author must hold a `Rooted` and `get()` after. | +| **#7206b** computed-read base across the key expression | base materialized, key lowered, base used | **Yes.** Identical mechanism. | +| **#7211** `ClassExprFresh` predicate asks the wrong question | rooted only if *initializers* can collect | **Yes, and most valuably.** There is no predicate to get wrong: `js_object_set_field_by_name` is a `CollectingCall`, so the class object's `Raw` cannot survive the loop. The author is forced to `root()` — the cleverness that caused the bug becomes unexpressible. | +| **#7184** slot index outside the pushed frame | `js_shadow_slot_bind(i32 N)` with `N >= frame size` | **Partly.** Not a liveness bug, so the borrow checker is silent. It *is* fixed by construction if `SlotIdx` is only obtainable from `ShadowFrame::alloc_slot()` and the frame's `enter(n)` count is derived from the number allocated, rather than both being written by hand. That is a worthwhile companion change and is cheap. | + +Four of five by construction, the fifth by making the frame own its own slot +numbering. That is a strong enough result to justify the work. + +## Migration cost + +The honest number is large but the distribution is favourable. + +- **~2500 builder call sites**, 35 files. Most are *not* GC-managed: loop + counters, `double` arithmetic, NaN-box bit twiddling, slot indices. Those + become `Plain`, which is `Copy` and imposes nothing. A rough read of the call + sites suggests **300–500 genuinely handle GC pointers** — the ones in + `expr/`, `lower_call/`, and the object/array/closure literal paths. +- **8 `rooted_handle_begin` sites** exist today, so the *explicit* rooting + surface is currently tiny. That is the point: the sites that need rooting and + do not have it are the bugs. +- The work is mechanical and the compiler drives it: change a signature, follow + the errors. It does not require understanding each lowering, only the local + data flow the compiler points at. + +**Incremental path, which matters more than the total:** + +1. Land the types with an explicit, greppable escape hatch: + `Raw::from_untrusted_register(String)` / `Raw::into_untrusted_register()`. + Every un-migrated caller uses it. Zero behaviour change, zero risk. +2. Migrate one family at a time, highest-risk first: `expr/temp_root.rs`'s + clients, then `lower_call/*`, then the literal paths. Each is its own PR. +3. `#[deny]` the escape hatch per-module as each module finishes, so migrated + code cannot regress. +4. Keep `gc_root_dominance_check.py` in CI permanently as the backstop for + whatever still goes through the escape hatch — and as the check on the + `NON_COLLECTING` table itself, which the type system trusts and cannot + verify. + +Steps 1 and 2-for-one-family are a plausible next PR. There is no point at which +a half-migrated tree is worse than today's. + +## Performance + +- **Emitted code: identical.** These are compile-time wrappers over register + names; the IR is unchanged. +- **Compiler runtime: neutral to slightly negative.** `Raw` is a newtype over + `String`, so no extra allocation. The `RefCell` adds a borrow flag check per + `emit_pure`, which is noise next to the `format!` calls already in the + builder. +- **Compiler build time: slightly up**, from monomorphisation over the added + lifetime. `perry-codegen` is already one of the slow crates; this is worth + measuring before the wide migration, not assuming. +- **Risk of *more* rooting than today:** yes, and that is a real cost worth + naming. When the borrow checker forces a `root()`, the author will insert one + rather than reason about whether it was needed, and some will be redundant. + A redundant shadow slot is one store and one bind. Given that the alternative + is the bug this document exists about, that is the right trade — but it should + be measured on the benchmark suite after the first family migrates, not waved + through. + +## What it cannot catch + +Stating these plainly, because a safety mechanism believed to be total is worse +than one known to be partial: + +- **A miscategorised callee.** If a genuinely-allocating helper is listed in + `NON_COLLECTING`, the type system will cheerfully allow a `Raw` across it. The + table is trusted input. This is why the checker must stay: it derives its + verdict from the emitted IR, so the two failure modes are not correlated. +- **The escape hatch**, for as long as any caller uses it. +- **Runtime-side rooting.** `RuntimeHandleScope` in `perry-runtime` is a + separate discipline over hand-written Rust; nothing here touches it. +- **Anything interprocedural.** A lowering that returns a `Raw` to a caller that + then collects is caught only if the lifetime actually propagates — which it + does for direct returns, but not across a `String` boundary or a struct field + that erases the lifetime. +- **Correctness of the shadow frame itself** — that `enter(n)` matches the slots + used, that the frame is popped on every path including unwinds. The + `SlotIdx`-from-`alloc_slot` change addresses the first; the rest is separate. +- **Values rooted in a side table rather than a slot.** As #7211 shows, + `CLASS_OBJECT_VALUES` roots *its own copy* and leaves the register stale. The + type system would treat such a value as unrooted, which is the correct and + conservative answer — but it means some code that is arguably fine today will + be forced to add a slot. + +## Recommendation + +Adopt, incrementally, starting with step 1 and one family. The decisive argument +is #7211: an author who was *actively thinking about GC rooting*, who wrote a +four-clause predicate to decide whether to root, still got it wrong — because +the predicate asked about the user's expressions and not about the lowering's +own emitted calls. No amount of care or review reliably catches that. A type +that makes the value unusable after the call does, and it does so at the moment +the mistake is made rather than five GC cycles later in someone else's program. + +**Not prototyped here.** `crates/perry-codegen/src/expr/` and `lower_call/` are +under concurrent edit (#7206 and the `js_closure_callN` work), and a +proof-of-concept worth anything has to touch exactly those files. The right +sequencing is: land the CI gate, let the in-flight lowering fixes merge, then +open step 1 as its own PR against a quiet tree. diff --git a/scripts/gc_root_dominance_allowlist.json b/scripts/gc_root_dominance_allowlist.json new file mode 100644 index 0000000000..936b642598 --- /dev/null +++ b/scripts/gc_root_dominance_allowlist.json @@ -0,0 +1,51 @@ +{ + "_readme": [ + "Known-remaining GC root-dominance violations, one entry per hit.", + "", + "This is deliberately NOT a numeric threshold. A count cannot tell a new", + "violation from an old one: fix one bug, introduce another, and the total is", + "unchanged and the gate stays green. Each entry names a fingerprint, the", + "issue tracking it, and why it is being tolerated.", + "", + "Three rules the checker enforces, so this file cannot decay into a blanket", + "suppression:", + " 1. an entry that matches nothing FAILS the build. When the bug is fixed,", + " delete the entry in the same PR. That is the ratchet.", + " 2. an entry suppresses at most `count` hits. A second violation of the", + " same shape in the same function is new, and fails.", + " 3. a violation with no entry fails, no matter how many entries exist.", + "", + "Adding an entry is a code-review event. Bumping `count` to make a red build", + "green is the thing this file exists to prevent -- if the count went up, a", + "new violation was introduced.", + "", + "Fingerprint format: .ll::::->", + "Get it from the checker's own output (`-v` prints one per violation)." + ], + "entries": [ + { + "fingerprint": "test_gap_class_expr_identity__test_gap_class_expr_identity_ts.ll::main::js_object_alloc->js_object_set_field_by_name", + "issue": "#7211", + "count": 2, + "justification": "Expr::ClassExprFresh (expr/static_field_meta.rs:432) holds the fresh class object in an SSA register across the js_object_set_field_by_name calls that install its named statics. Its protect_handle predicate only asks whether the AUTHOR'S initializer expressions can collect, never whether the lowering's own emitted field-store can -- so a class expression with inert statics gets no temp root. Two evaluations in this module, hence count 2. Pre-existing on main and reported red by this same gate since #7198; not fixed here because crates/perry-codegen/src/expr/ is being actively edited under #7206." + }, + { + "fingerprint": "test_gap_class_expr_instance_fields__test_gap_class_expr_instance_fields_ts.ll::main::js_object_alloc->js_object_set_field_by_name", + "issue": "#7211", + "count": 1, + "justification": "Same ClassExprFresh predicate gap as the entry above, reached through a class expression carrying instance fields. Tracked as one fix in #7211; listed separately because the fingerprint is per-module and a per-module entry is what keeps a NEW violation in this module from being absorbed." + }, + { + "fingerprint": "test_gap_class_expr_new_instanceof__test_gap_class_expr_new_instanceof_ts.ll::main::js_object_alloc->js_object_set_field_by_name", + "issue": "#7211", + "count": 1, + "justification": "Same ClassExprFresh predicate gap as the entries above, reached via `new`/`instanceof` on a class-expression value. Note that js_object_mark_class does put this object in CLASS_OBJECT_VALUES, which is scanned and forwarded -- that keeps the OBJECT alive but does not rewrite the register, so the violation is real rather than a false positive." + }, + { + "fingerprint": "test_gap_class_expr_static_this__test_gap_class_expr_static_this_ts.ll::main::js_object_alloc->js_object_set_field_by_name", + "issue": "#7211", + "count": 1, + "justification": "Same ClassExprFresh predicate gap as the entries above, reached through a static method that reads `this`. Same fix in #7211 retires all four entries at once; the checker will then fail on the stale entries, which is the intended prompt to delete them." + } + ] +} diff --git a/scripts/gc_root_dominance_check.py b/scripts/gc_root_dominance_check.py index 644e72ed3f..a6214460d4 100755 --- a/scripts/gc_root_dominance_check.py +++ b/scripts/gc_root_dominance_check.py @@ -87,6 +87,7 @@ import argparse import contextlib import io +import json import os import re import sys @@ -581,6 +582,30 @@ def movers(self): def moving(self): return bool(self.movers) + @property + def fingerprint(self): + """Identity of this violation for allowlist matching. + + Deliberately built from names, never from SSA registers, slot indices + or line numbers: `%12`, `slot 3` and "line 480" all move when an + unrelated lowering above them changes by one instruction, and a + fingerprint that churns is a fingerprint someone will replace with a + numeric threshold. What stays put across a recompile is *which + function*, *what it allocated*, and *what could collect before the + root store landed* -- which is also exactly the triple a human needs + to judge whether an entry is still the bug they signed off on. + + Coarser than one-per-instruction on purpose: two violations in the + same function with the same alloc/collector pair collapse to one + fingerprint, and the allowlist entry's `count` is what pins the + multiplicity. A third one appearing later therefore still fails. + """ + return "{}::{}::{}->{}".format( + self.module, self.func, self.alloc.callee or "?", + sorted({c.callee for c in self.collectors})[0] + if self.collectors else "?", + ) + def check_func(module, f, want_moving_only=False, poll_reaching=frozenset(), anchor_mode="alloc"): @@ -710,6 +735,337 @@ def scan(blk, lo, hi): return violations +# -------------------------------------------------------------- allowlist --- +# +# Why a file of named entries and not `--max-violations N`: +# +# A numeric threshold cannot tell a new violation from an old one. Fix one bug, +# introduce a different one, and the count is unchanged and the gate is green -- +# which is the failure mode this whole exercise exists to prevent. Worse, the +# number ratchets the wrong way under pressure: the cheapest way to make a red +# build green is to raise it by one, and nothing in the diff says what was +# conceded. +# +# So: one entry per known-remaining violation, each naming the issue that tracks +# it and carrying a written justification, and three properties that keep the +# file from decaying into a blanket suppression: +# +# * an entry that matches NOTHING is an error. When the bug is fixed the entry +# must be deleted in the same PR, so the allowlist cannot accumulate +# tombstones that quietly widen its coverage later. +# * an entry suppresses at most `count` violations. A second violation of the +# same shape in the same function is new, and fails. +# * a violation with no entry fails, regardless of how many entries exist. + +ALLOWLIST_MIN_JUSTIFICATION = 40 + + +class AllowlistError(Exception): + """The allowlist itself is malformed, stale, or under-documented.""" + + +class AllowEntry: + __slots__ = ("fingerprint", "count", "issue", "justification", "matched") + + def __init__(self, fingerprint, count, issue, justification): + self.fingerprint = fingerprint + self.count = count + self.issue = issue + self.justification = justification + self.matched = 0 + + +def load_allowlist(path): + """Parse and VALIDATE the allowlist. Every failure here is exit 2. + + Validation is strict because an allowlist is the one part of a gate whose + whole purpose is to make it not fail; a typo'd field that silently parses + to "suppress everything" would be indistinguishable from a working gate. + """ + try: + with open(path, "r") as fh: + doc = json.load(fh) + except FileNotFoundError: + raise AllowlistError(f"{path}: no such file") + except json.JSONDecodeError as exc: + raise AllowlistError(f"{path}: not valid JSON: {exc}") + + if not isinstance(doc, dict) or "entries" not in doc: + raise AllowlistError( + f"{path}: expected a JSON object with an \"entries\" array") + raw = doc["entries"] + if not isinstance(raw, list): + raise AllowlistError(f"{path}: \"entries\" must be an array") + + entries = {} + for i, e in enumerate(raw): + where = f"{path}: entries[{i}]" + if not isinstance(e, dict): + raise AllowlistError(f"{where}: must be an object") + for field in ("fingerprint", "issue", "justification"): + if not isinstance(e.get(field), str) or not e[field].strip(): + raise AllowlistError( + f"{where}: \"{field}\" is required and must be a " + "non-empty string") + count = e.get("count", 1) + if not isinstance(count, int) or isinstance(count, bool) or count < 1: + raise AllowlistError( + f"{where}: \"count\" must be a positive integer (got {count!r})") + # A justification of "known issue" is not a justification. The length + # floor is crude, but it is the difference between a reviewer having to + # read a sentence and having to go excavate the history themselves. + if len(e["justification"].strip()) < ALLOWLIST_MIN_JUSTIFICATION: + raise AllowlistError( + f"{where}: \"justification\" must be at least " + f"{ALLOWLIST_MIN_JUSTIFICATION} characters explaining WHY this " + "violation is known-safe or known-tracked. Suppressing a " + "GC-rooting violation without saying why is how the next one " + "gets waved through.") + fp = e["fingerprint"].strip() + if fp in entries: + raise AllowlistError( + f"{where}: duplicate fingerprint {fp!r}; merge the two entries " + "and set \"count\" instead, so the suppressed multiplicity is " + "stated in one place") + entries[fp] = AllowEntry(fp, count, e["issue"].strip(), + e["justification"].strip()) + return entries + + +def apply_allowlist(violations, entries): + """Split `violations` into (suppressed, remaining) and mark entries used. + + Over-quota violations land in `remaining`: an entry is a licence for a + stated number of hits, not for a shape. + """ + seen = defaultdict(int) + suppressed, remaining = [], [] + for v in violations: + fp = v.fingerprint + entry = entries.get(fp) + if entry is None: + remaining.append(v) + continue + seen[fp] += 1 + if seen[fp] <= entry.count: + entry.matched += 1 + suppressed.append(v) + else: + remaining.append(v) + return suppressed, remaining + + +def stale_entries(entries): + """Entries that matched nothing. Always an error -- see the header note.""" + return [e for e in entries.values() if e.matched == 0] + + +# --------------------------------------------------- seeded-violation proof --- +# +# `--self-test` proves the checker fires on HAND-WRITTEN IR. Necessary, not +# sufficient: the fixtures are frozen text, so they keep passing even if perry's +# emitted IR drifts to a shape the parser can no longer read -- at which point +# the checker reports a serene `violations: 0` over a corpus it is not actually +# analysing. That is hazard 4 (CLAUDE.md) wearing the self-test's clothes, and +# it is the failure mode this repo has hit most often. +# +# So the gate also seeds violations into the REAL corpus and requires every one +# to be caught. For each sampled site we take a value that IS correctly rooted +# today and splice a collection point into the gap between the allocation and +# its root store -- mechanically manufacturing #7192's exact shape out of code +# that is currently right: +# +# %v = call ptr @js_object_alloc(...) <-- real +# call double @js_call_function(...) <-- spliced in +# store ptr %v, ptr %slot <-- real +# call void @js_shadow_slot_bind(...) <-- real +# +# Only the collection point is synthetic. The function, its CFG, the allocation, +# the value's provenance chain through `or`/`bitcast`, the alloca, the store and +# the bind are all perry's own output, so the exercise covers the parser, the +# CFG builder, the provenance walk and the dominance computation against IR as +# it is actually emitted. Splicing rather than reordering also means a site +# always exists wherever a rooted allocation exists, instead of depending on the +# scheduler happening to leave a collecting call in a convenient place. +# +# If perry's IR stops having rooted allocations in the shape this matches, the +# gate goes red for "I could not seed a violation" rather than green for "I +# found none". + +_ANY_BIND_RE = re.compile( + r"^\s*call void @js_shadow_slot_bind\(i32 (\d+), ptr %([\w.$]+)\)") +_ANY_STORE_RE = re.compile( + r"^\s*store\s+[\w\[\]x* ]+?\s+%([\w.$]+),\s*ptr %([\w.$]+)") +_ANY_ASSIGN_RE = re.compile(r"^\s*%([\w.$]+)\s*=\s*(.*)$") +_ANY_CALL_RHS_RE = re.compile(r"^call\s+[^@]*@([\w.$]+)\(") + +# The spliced collection point. `js_call_function` is in POLL_CAPABLE_RUNTIME, +# so the planted violation is classified MOVING and survives `--moving-only` -- +# i.e. the proof is about the configuration the gate actually ships with, not a +# laxer one. The mutant only has to be parseable by this checker, never +# compiled, so no declaration is needed. +_SEED_CALL = " %gcseed.probe = call double @js_call_function(double 0.000000e+00)" + + +def _block_defs(lines, lo, hi): + """reg -> right-hand side, for definitions in lines[lo:hi].""" + defs = {} + for j in range(lo, hi): + m = _ANY_ASSIGN_RE.match(lines[j]) + if m: + defs[m.group(1)] = m.group(2) + return defs + + +def _reaches_alloc(defs, reg, limit=32): + """Does `reg` trace back to an allocation through transparent ops only? + + Mirrors `provenance()` deliberately: the seeded site has to be one the + checker's own anchoring would accept, otherwise a non-report is correct + behaviour being counted as a miss. + """ + seen = set() + q = deque([reg]) + while q and len(seen) < limit: + r = q.popleft() + if r in seen: + continue + seen.add(r) + rhs = defs.get(r) + if rhs is None: + continue + cm = _ANY_CALL_RHS_RE.match(rhs) + if cm: + if ALLOC_RE.match(cm.group(1)): + return True + continue + if any(op in rhs for op in TRANSPARENT_OPS): + q.extend(re.findall(r"%([\w.$]+)", rhs)) + return False + + +def _seed_sites(lines): + """Yield line indices at which to splice a collection point. + + A site is a `store`/`js_shadow_slot_bind` pair whose stored value traces + back to an allocation earlier in the same basic block, where: + + * the slot is bound exactly once in the enclosing function and never + `js_shadow_slot_set`, so it cannot already be active at the store -- + an already-active slot publishes through `bound_ptr` and the checker + correctly reports nothing; + * nothing in the gap roots the value another way, which would likewise + make a non-report correct. + + Both exclusions matter: without them the "misses" this test reports would + be the checker behaving properly, and the first person to see a red run + would learn to distrust it. + """ + fn_lo, fn_hi = 0, 0 + block_start = 0 + fn_text = "" + for i, line in enumerate(lines): + if line.startswith("define "): + fn_lo = i + fn_hi = next((k for k in range(i + 1, len(lines)) + if lines[k].startswith("}")), len(lines)) + fn_text = "\n".join(lines[fn_lo:fn_hi]) + block_start = i + 1 + continue + if LABEL_RE.match(line) or line.startswith("}"): + block_start = i + 1 + continue + if i < fn_lo or i >= fn_hi: + continue + bm = _ANY_BIND_RE.match(line) + if not bm: + continue + slot, alloca = bm.group(1), bm.group(2) + if i == 0: + continue + sm = _ANY_STORE_RE.match(lines[i - 1]) + if not sm or sm.group(2) != alloca: + continue + # the slot must not already be live at the store + if fn_text.count(f"js_shadow_slot_bind(i32 {slot}, ") != 1: + continue + if f"js_shadow_slot_set(i32 {slot}," in fn_text: + continue + defs = _block_defs(lines, block_start, i - 1) + if not _reaches_alloc(defs, sm.group(1)): + continue + # a rooting call already in the block would protect the mutant + if any(rc in lines[j] for j in range(block_start, i - 1) + for rc in ROOTING_CALLS): + continue + yield i - 1 + + +def _mutate(lines, at): + """Splice a collection point immediately above line `at`.""" + return lines[:at] + [_SEED_CALL] + lines[at:] + + +def seeded_violation_test(paths, moving_only, anchor, want_sites, verbose=False): + """Return 0 if every seeded violation was caught, non-zero otherwise.""" + caught = missed = sites = 0 + misses = [] + with tempfile.TemporaryDirectory() as td: + mutant = os.path.join(td, "mutant.ll") + for p in sorted(paths): + if sites >= want_sites: + break + with open(p, "r", errors="replace") as fh: + lines = fh.read().splitlines() + try: + base, _ = _scan([p], moving_only, anchor) + except MalformedIR: + continue + for at in _seed_sites(lines): + if sites >= want_sites: + break + with open(mutant, "w") as fh: + fh.write("\n".join(_mutate(lines, at)) + "\n") + try: + after, _ = _scan([mutant], moving_only, anchor) + except MalformedIR: + continue + sites += 1 + if len(after) > len(base): + caught += 1 + if verbose: + print(f" seeded {os.path.basename(p)}:{at + 1} -> caught") + else: + missed += 1 + misses.append(f"{p}:{at + 1}") + + print(f"=== seeded violations: {sites} planted, {caught} caught, " + f"{missed} MISSED") + if sites == 0: + print("error: could not seed a single violation into the corpus. The " + "mutator looks for a store/bind pair whose value traces back to " + "an allocation in the same block; finding none means perry's " + "emitted IR no longer has the shape this checker anchors on, so " + "a clean verdict from it would be meaningless.", file=sys.stderr) + return 2 + if sites < want_sites: + print(f"error: only {sites} seed site(s) found, wanted {want_sites}. " + "Too narrow a sample to claim the checker still fires.", + file=sys.stderr) + return 2 + if missed: + print("error: the checker did NOT report these seeded violations:", + file=sys.stderr) + for m in misses[:20]: + print(f" {m}", file=sys.stderr) + print("A planted collection point between an allocation and its root " + "store went unreported. The checker is not reading this IR the " + "way it thinks it is; a clean verdict from it means nothing " + "until this is explained.", file=sys.stderr) + return 1 + return 0 + + # ------------------------------------------------------------- self-test --- # # The gate's own "can it fail?" arm. `PLANTED` is the #7186 shape: the instance @@ -1268,7 +1624,7 @@ def _scan(paths, moving_only, anchor): if BIND_RE.search(ins.text) ) found = [ - (mod, v) + v for mod, fs in parsed for f in fs for v in check_func(mod, f, moving_only, poll_reaching, anchor) @@ -1318,7 +1674,7 @@ def self_test(): print(f"self-test FAIL: planted fixture -> {binds} binds, expected 2", file=sys.stderr) ok = False - if ok and not all(v.moving for _m, v in found): + if ok and not all(v.moving for v in found): print("self-test FAIL: both planted violations reach a moving minor " "(js_call_function / js_gc_loop_safepoint) and must be " "classified MOVING", file=sys.stderr) @@ -1408,6 +1764,122 @@ def self_test(): f"bind for every alloca, so the unrooted check must report 0, " f"got {len(found)}", file=sys.stderr) ok = False + # ---- the allowlist must not be able to absorb a new violation ------ + # + # These arms exist because the allowlist is the only component whose + # job is to stop the gate failing. Every one of them is a way it could + # quietly become a blanket suppression. + planted_hits, _ = _scan([planted], False, "alloc") + fps = sorted({v.fingerprint for v in planted_hits}) + if len(fps) != 2: + print(f"self-test FAIL: planted fixture -> {len(fps)} distinct " + "fingerprints, expected 2 (the two functions differ, so the " + "fingerprint must distinguish them)", file=sys.stderr) + ok = False + + def _write_allowlist(entries): + p = os.path.join(td, "allow.json") + with open(p, "w") as fh: + json.dump({"entries": entries}, fh) + return p + + good_reason = ("tracked in the issue above; the receiver is rooted by " + "the caller one frame up, verified by hand") + + if len(fps) == 2: + # 1. Full coverage suppresses. + al = load_allowlist(_write_allowlist([ + {"fingerprint": fp, "issue": "#0000", + "justification": good_reason} for fp in fps])) + _sup, rem = apply_allowlist(planted_hits, al) + if rem or stale_entries(al): + print("self-test FAIL: an allowlist naming every planted " + "fingerprint should suppress all of them and go stale on " + "none", file=sys.stderr) + ok = False + + # 2. Partial coverage still fails -- the uncovered one is "new". + al = load_allowlist(_write_allowlist([ + {"fingerprint": fps[0], "issue": "#0000", + "justification": good_reason}])) + _sup, rem = apply_allowlist(planted_hits, al) + if len(rem) != 1: + print(f"self-test FAIL: one entry covering one of two " + f"violations left {len(rem)} unallowed, expected 1. An " + "allowlist that suppresses a violation it does not name " + "is a blanket suppression.", file=sys.stderr) + ok = False + + # 3. `count` is a quota, not a licence for the shape. Feed the same + # violation twice against a count of 1 and the second must + # survive -- this is the "new violation while the total is + # unchanged" case that a numeric threshold cannot see. + al = load_allowlist(_write_allowlist([ + {"fingerprint": fps[0], "issue": "#0000", "count": 1, + "justification": good_reason}])) + same = [v for v in planted_hits if v.fingerprint == fps[0]] + _sup, rem = apply_allowlist(same + same, al) + if len(rem) != 1: + print(f"self-test FAIL: count=1 against two hits of the same " + f"fingerprint left {len(rem)} unallowed, expected 1", + file=sys.stderr) + ok = False + + # 4. An entry that matches nothing is an error, so a fixed bug's + # tombstone cannot linger and widen coverage later. + al = load_allowlist(_write_allowlist([ + {"fingerprint": "nosuch::nosuch::js_object_alloc->js_x", + "issue": "#0000", "justification": good_reason}])) + apply_allowlist(planted_hits, al) + if not stale_entries(al): + print("self-test FAIL: an allowlist entry matching nothing must " + "be reported stale", file=sys.stderr) + ok = False + + # 5. Under-documented and malformed entries are refused outright. + for bad, why in ( + ([{"fingerprint": "a::b::c->d", "issue": "#1", + "justification": "known"}], "a too-short justification"), + ([{"fingerprint": "a::b::c->d", "justification": good_reason}], + "a missing issue"), + ([{"fingerprint": "a::b::c->d", "issue": "#1", "count": 0, + "justification": good_reason}], "count=0"), + ([{"fingerprint": "a::b::c->d", "issue": "#1", + "justification": good_reason}, + {"fingerprint": "a::b::c->d", "issue": "#2", + "justification": good_reason}], "a duplicate fingerprint"), + ): + try: + load_allowlist(_write_allowlist(bad)) + except AllowlistError: + pass + else: + print(f"self-test FAIL: {why} must be rejected", file=sys.stderr) + ok = False + + # ---- the corpus mutator must be able to manufacture a violation ---- + # (proves seeded_violation_test's machinery works even before it is + # pointed at real IR, so a CI run that finds no sites is a real signal + # about the IR rather than a broken mutator.) + with open(clean, "r") as fh: + clean_lines = fh.read().splitlines() + sites = list(_seed_sites(clean_lines)) + if not sites: + print("self-test FAIL: the mutator found no seed site in the clean " + "fixture, which is a rooted alloc followed by a collecting " + "call — the exact shape it exists to find", file=sys.stderr) + ok = False + else: + mutant = os.path.join(td, "mutant.ll") + with open(mutant, "w") as fh: + fh.write("\n".join(_mutate(clean_lines, sites[0])) + "\n") + got, _ = _scan([mutant], False, "alloc") + if not got: + print("self-test FAIL: sinking the root store below the " + "collecting call in the CLEAN fixture must produce a " + "violation; the mutator or the checker is broken", + file=sys.stderr) + ok = False print("self-test OK" if ok else "self-test FAILED") return 0 if ok else 1 @@ -1451,6 +1923,21 @@ def main(): "holds a heap value across a collecting call and is " "loaded below it, with no js_shadow_slot_bind anywhere. " "Disjoint from the bind-anchored check by construction.") + ap.add_argument("--min-funcs", type=int, default=1, metavar="N", + help="fail unless at least N functions were checked (default 1). " + "Files and binds can both look healthy while the corpus " + "is one module deep; this asserts breadth.") + ap.add_argument("--allowlist", metavar="PATH", + help="JSON file of known-remaining violations, one entry each " + "with an issue and a written justification. An entry that " + "matches nothing is an error, and an entry suppresses at " + "most its `count` hits -- so a NEW violation fails even " + "when the total is below some previous number.") + ap.add_argument("--seeded-violations", type=int, default=0, metavar="N", + help="after checking, plant N synthetic violations into the " + "real corpus IR and require every one to be reported. " + "Proves the checker can still fail against the IR perry " + "actually emits, not just against frozen fixtures.") ns = ap.parse_args() # A knob that is silently ignored is a disarmed knob: `--max-stale 0` @@ -1465,6 +1952,14 @@ def main(): if ns.self_test: return self_test() + allowlist = {} + if ns.allowlist: + try: + allowlist = load_allowlist(ns.allowlist) + except AllowlistError as exc: + print(f"error: {exc}", file=sys.stderr) + return 2 + moving_only = ns.moving_only anchor = "any" if ns.any_def else "alloc" verbose = ns.verbose @@ -1552,42 +2047,98 @@ def main(): return 2 return 1 if total else 0 - total = 0 - moving_total = 0 - per_kind = defaultdict(int) - per_kind_moving = defaultdict(int) - out = [] + n_funcs = sum(len(fs) for _m, fs in parsed) + + found = [] for mod, fs in parsed: for f in fs: - for v in check_func(mod, f, moving_only, poll_reaching, anchor): - total += 1 - per_kind[v.alloc.callee] += 1 - if v.moving: - moving_total += 1 - per_kind_moving[v.alloc.callee] += 1 - cs = sorted({c.callee for c in v.collectors}) - out.append( - f"{mod}::{v.func}\n" - f" alloc : {v.alloc.text.strip()}\n" - f" store : {v.store.text.strip()}\n" - f" bind : slot {v.slot} {v.bind.text.strip()}\n" - f" between: {', '.join(cs[:8])}" - f"{' (+%d more)' % (len(cs) - 8) if len(cs) > 8 else ''}\n" - f" MOVING : {('YES via ' + ', '.join(v.movers[:3])) if v.moving else 'no'}\n" - ) + found.extend(check_func(mod, f, moving_only, poll_reaching, anchor)) + + suppressed, remaining = apply_allowlist(found, allowlist) + + def render(v): + cs = sorted({c.callee for c in v.collectors}) + return ( + f"{v.module}::{v.func}\n" + f" alloc : {v.alloc.text.strip()}\n" + f" store : {v.store.text.strip()}\n" + f" bind : slot {v.slot} {v.bind.text.strip()}\n" + f" between: {', '.join(cs[:8])}" + f"{' (+%d more)' % (len(cs) - 8) if len(cs) > 8 else ''}\n" + f" MOVING : {('YES via ' + ', '.join(v.movers[:3])) if v.moving else 'no'}\n" + f" fingerprint: {v.fingerprint}\n" + ) + + total = len(found) + moving_total = sum(1 for v in found if v.moving) + per_kind = defaultdict(int) + per_kind_moving = defaultdict(int) + for v in found: + per_kind[v.alloc.callee] += 1 + if v.moving: + per_kind_moving[v.alloc.callee] += 1 + if verbose: - print("\n".join(out)) - print(f"=== files: {len(paths)} root stores: {n_binds} violations: {total}" - f" (moving-minor reachable: {moving_total})") + print("\n".join(render(v) for v in found)) + + # The breadth line CI reads. Printing functions and modules -- not just a + # violation count -- is what makes a silently-empty or silently-shrunken run + # visible in the log instead of indistinguishable from a clean one. + print(f"=== checked {n_funcs} functions / {len(parsed)} modules " + f"({len(paths)} .ll files, {n_binds} root stores)") + print(f"=== violations: {total} (moving-minor reachable: {moving_total})") + if allowlist: + print(f"=== allowlisted: {len(suppressed)} hit(s) across " + f"{len(allowlist)} entr(y/ies); unallowed: {len(remaining)}") for k, n in sorted(per_kind.items(), key=lambda kv: -kv[1]): print(f" {n:6d} ({per_kind_moving.get(k, 0):5d} moving) {k}") + + # --- subject-liveness assertions, before any verdict is announced ------- if n_binds < ns.min_binds: print(f"error: {n_binds} root store(s) in the corpus, need at least " f"{ns.min_binds}. The subject of this check never ran — a clean " "verdict here means the IR was not the IR you think it is " "(compile with PERRY_INLINE_SHADOW_SLOT=0).", file=sys.stderr) return 2 - return 1 if total else 0 + if n_funcs < ns.min_funcs: + print(f"error: checked {n_funcs} function(s), need at least " + f"{ns.min_funcs}. The corpus compiled but is too thin to have " + "exercised the lowerings this invariant runs through.", + file=sys.stderr) + return 2 + + # --- allowlist hygiene -------------------------------------------------- + stale = stale_entries(allowlist) + if stale: + print("error: allowlist entries matched nothing:", file=sys.stderr) + for e in stale: + print(f" {e.fingerprint} ({e.issue})", file=sys.stderr) + print("Either the violation was fixed — delete the entry in the same " + "PR, that is the ratchet — or the corpus shrank and no longer " + "contains the module it names, which means this run checked less " + "than it claims to.", file=sys.stderr) + return 2 + + if remaining: + if not verbose: + print("\n".join(render(v) for v in remaining)) + print(f"error: {len(remaining)} GC root-dominance violation(s) not " + "covered by the allowlist.", file=sys.stderr) + print("A GC-managed value is live across a collection point without " + "being reachable from a root. See docs/src/internals/" + "gc-rooting-invariant.md. If this is genuinely known and tracked, " + "add an entry with an issue and a justification — never a count " + "bump.", file=sys.stderr) + return 1 + + # --- can this gate still fail? ------------------------------------------ + if ns.seeded_violations: + rc = seeded_violation_test(paths, moving_only, anchor, + ns.seeded_violations, verbose) + if rc: + return rc + + return 0 if __name__ == "__main__": diff --git a/scripts/gc_root_dominance_corpus.sh b/scripts/gc_root_dominance_corpus.sh new file mode 100755 index 0000000000..e06ba84714 --- /dev/null +++ b/scripts/gc_root_dominance_corpus.sh @@ -0,0 +1,147 @@ +#!/usr/bin/env bash +# +# Emit the LLVM IR corpus that scripts/gc_root_dominance_check.py gates on. +# +# This lives in a script rather than inline in the workflow so that the corpus +# CI checks and the corpus you check locally are the same corpus. The previous +# arrangement had the source list, the env knobs and the failure budget spelled +# out only in gc-root-dominance.yml, which made "reproduce the CI failure" +# mean "reread the YAML and retype it" -- and a retyped PERRY_GC_MOVING_LOOP_POLLS +# that gets dropped produces IR in which the bug is not expressible at all. +# +# ./scripts/gc_root_dominance_corpus.sh [OUTDIR] +# +# Requires target/release/perry plus the runtime archives (see the workflow, or +# `cargo build --release -p perry -p perry-runtime-static -p perry-stdlib-static`). +# +# Exit status is non-zero if fewer than MIN_COMPILED sources produced IR. A +# source that stops compiling is allowed -- test-files/ churns, and this gate is +# not the gap suite -- but the corpus is NOT allowed to quietly shrink to +# nothing, because "0 violations over 3 files" and "0 violations over 300 files" +# print the same verdict and mean opposite things. + +set -euo pipefail + +OUTDIR="${1:-ir-corpus}" +PERRY_BIN="${PERRY_BIN:-target/release/perry}" + +# Sources are chosen for the LOWERINGS they exercise, not for coverage of the +# language. Every one of #7154/#7184/#7192/#7206 was in one of these paths: +# +# gc/repsel - the rooting machinery itself, and representation selection +# (a value that changes representation changes who must root it) +# class - method receivers, `super()`, static initialisers. #7206's +# receiver-across-the-argument-list bug lives here. +# new - inline-constructor `this_slot`, the still-open alloca case +# object/ - object and array literals: long runs of element stores, each +# array one an allocation the accumulating literal must survive +# computed/ - computed reads. #7206's other bug is the base held in a +# index register across the key expression's own allocations +# prop - property stores +# static - static field initialisers, which run in a synthetic frame +# closure - captures, the js_closure_callN family +# dynamic - dynamic dispatch, where the receiver's type is not proven +# spread - spread/rest, which allocate per element +# map/set - collection literals +# +# Keep this list in sync with MIN_COMPILED below when you add a prefix. +PATTERNS=( + 'test_gap_gc_*.ts' + 'test_gap_repsel*.ts' + 'test_gap_class*.ts' + 'test_gap_new*.ts' + 'test_gap_object*.ts' + 'test_gap_array*.ts' + 'test_gap_computed*.ts' + 'test_gap_prop*.ts' + 'test_gap_static*.ts' + 'test_gap_closure*.ts' + 'test_gap_dynamic*.ts' + 'test_gap_map*.ts' + 'test_gap_set*.ts' +) + +# The floor, not a target. Raise it when the corpus grows; never lower it to +# make a run pass -- a shrinking corpus is the finding, not the obstacle. +MIN_COMPILED="${MIN_COMPILED:-90}" + +if [ ! -x "$PERRY_BIN" ]; then + echo "error: $PERRY_BIN not found or not executable." >&2 + echo " cargo build --release -p perry -p perry-runtime-static -p perry-stdlib-static" >&2 + exit 2 +fi + +shopt -s nullglob +sources=() +for pat in "${PATTERNS[@]}"; do + # shellcheck disable=SC2206 + matches=(test-files/$pat) + if [ "${#matches[@]}" -eq 0 ]; then + # Loud, because a stale pattern is how a corpus silently loses a whole + # lowering: the run stays green and nobody re-reads the glob list. + echo "::error::pattern '$pat' matched nothing; it is stale. Remove it or fix it." >&2 + exit 2 + fi + sources+=("${matches[@]}") +done + +if [ "${#sources[@]}" -eq 0 ]; then + echo "::error::no corpus sources matched any pattern; the glob list is stale" >&2 + exit 2 +fi + +rm -rf "$OUTDIR" +mkdir -p "$OUTDIR" + +compiled=0 +skipped=0 +skipped_names=() +scratch="$(mktemp -d)" +trap 'rm -rf "$scratch"' EXIT + +for src in "${sources[@]}"; do + name="$(basename "$src" .ts)" + rm -rf .perry-trace/llvm + # PERRY_GC_MOVING_LOOP_POLLS=1 is what puts `js_gc_loop_safepoint` in the IR, + # which is what the MOVING classification keys on. It is off by default + # (#7161), so without it this corpus cannot express the bug at all. + # PERRY_INLINE_SHADOW_SLOT=0 makes every root store the @js_shadow_slot_bind + # call form; the #7088 inline diamond is equivalent but harder to anchor on. + if ! env PERRY_GC_MOVING_LOOP_POLLS=1 \ + PERRY_INLINE_SHADOW_SLOT=0 \ + PERRY_NO_AUTO_OPTIMIZE=1 \ + "$PERRY_BIN" compile "$src" -o "$scratch/$name" --trace llvm \ + >/dev/null 2>&1; then + skipped=$((skipped + 1)) + skipped_names+=("$name") + continue + fi + emitted=0 + for ll in .perry-trace/llvm/*.ll; do + cp "$ll" "$OUTDIR/${name}__$(basename "$ll")" + emitted=1 + done + if [ "$emitted" -eq 1 ]; then + compiled=$((compiled + 1)) + else + # Compiled but emitted no IR: --trace llvm is wired to codegen, so this + # means codegen did not run (a fully cached build). Counting it as a + # success would let a cache hit stand in for a check. + skipped=$((skipped + 1)) + skipped_names+=("$name (no .ll emitted)") + fi +done + +files="$(find "$OUTDIR" -name '*.ll' | wc -l | tr -d ' ')" +echo "corpus: $compiled/${#sources[@]} sources compiled, $skipped skipped, $files .ll files" +if [ "$skipped" -gt 0 ]; then + printf ' skipped: %s\n' "${skipped_names[*]}" +fi + +if [ "$compiled" -lt "$MIN_COMPILED" ]; then + echo "::error::only $compiled sources compiled, need at least $MIN_COMPILED." >&2 + echo "The corpus shrank. Either fix the sources that stopped compiling, or -- if" >&2 + echo "they were removed on purpose -- adjust PATTERNS and MIN_COMPILED together" >&2 + echo "in this file, in the same PR, so the reduction is reviewable." >&2 + exit 1 +fi From 7e5d0e4bca435c26236a251bf2e3c6cbc630aa37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 23:46:15 +0200 Subject: [PATCH 4/4] docs(gc): correct the post-#7207 staleness in the rooting-invariant writeup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merge-time corrections to statements #7207 invalidated while this PR was in review: - CLAUDE.md called `lower_call/new.rs`'s inline-ctor `this_slot` "still open". #7207 closed it. Point at `--unrooted-allocas` as the detector for that shape and name #7210 as where its remaining hits are tracked. - The rooting-invariant doc documented `--stale-registers` but not `--unrooted-allocas`, so the one mode the bind-anchored check is structurally blind to had no entry. Add it, and state plainly that the gate does NOT run it and that its hits are deliberately outside the allowlist — the allowlist covers the bind-anchored shape only. - "all five known shapes" -> "every known shape", so the pointer cannot go stale the next time one is found. --- CLAUDE.md | 2 +- docs/src/internals/gc-rooting-invariant.md | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/CLAUDE.md b/CLAUDE.md index ba0339693d..65580f82a7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -250,4 +250,4 @@ Corollary: a *new* gate has never been green, so promoting it to required immedi - **Async-to-generator transform, body locals.** It boxes every body local into a shared mutable cell typed `Any`. Two consequences seen in the wild: per-iteration `let`/`const` bindings collapse for closures created in a loop, and computed numeric-key calls (`arr[i](x)`) lose their type proof and silently resolve by *method name*, evaporating the call. - **Native base-class subclassing.** A native base's surface is installed at `super()` time and its parent edge lives in the class registry; keying any of that on a literal `extends` name loses it for fieldless classes, indirect subclasses, and class expressions. - **Two prototype-resolution paths.** `CLASS_PROTOTYPE_OBJECTS` (synthetic: `Object.create`, plain-function ctors) vs `CLASS_DECL_PROTOTYPE_OBJECTS` (declared classes). `in`/`for…in` and `getPrototypeOf` have disagreed about the same chain. -- **Root-store dominance in codegen.** *A GC-managed value's root store must **dominate** every subsequent site that can collect.* Three ways it has broken, all shipped: the store's slot index fell outside the pushed shadow frame so `js_shadow_slot_bind` bounds-checked it into a silent no-op (#7184); the store was emitted in-frame but **after** a call that allocates (#7192); and the value lives in a plain `alloca_entry` that is neither a shadow slot nor a temp root, so the collector never rewrites it (`lower_call/new.rs`'s inline-ctor `this_slot`, still open). All three present identically — a *rooted* slot holding a dangling pointer, surfacing cycles later as `TypeError: value is not a function` — and **none is visible to any runtime GC probe**, because at the moment of the collection there is nothing for the collector to find. That is why #7154's from-space scan only ever saw offenders whose targets had already died. The instrument is static: `scripts/gc_root_dominance_check.py` over `--trace llvm` output (`--self-test` proves it can still fail). Only bites under `PERRY_GC_MOVING_LOOP_POLLS=1`, off by default since #7161 — so a green default run says nothing about this class. **Full writeup, all five known shapes and how to check your work: `docs/src/internals/gc-rooting-invariant.md`.** The CI gate is `gc-root-dominance.yml` over `scripts/gc_root_dominance_corpus.sh`; known-remaining hits are named one-per-entry in `scripts/gc_root_dominance_allowlist.json` (an entry that matches nothing FAILS, so a fix must delete its entry). A fifth shape is open as #7211: `ClassExprFresh` roots only when it thinks the static *initializers* collect, and never asks whether its own emitted `js_object_set_field_by_name` does — the sophisticated version of the mistake, where the author wrote a rooting predicate and it asked the wrong question. +- **Root-store dominance in codegen.** *A GC-managed value's root store must **dominate** every subsequent site that can collect.* Three ways it has broken, all shipped: the store's slot index fell outside the pushed shadow frame so `js_shadow_slot_bind` bounds-checked it into a silent no-op (#7184); the store was emitted in-frame but **after** a call that allocates (#7192); and the value lives in a plain `alloca_entry` that is neither a shadow slot nor a temp root, so the collector never rewrites it (`lower_call/new.rs`'s inline-ctor `this_slot`, closed by #7207; `--unrooted-allocas` is the detector for that shape, and its remaining hits are #7210's). All three present identically — a *rooted* slot holding a dangling pointer, surfacing cycles later as `TypeError: value is not a function` — and **none is visible to any runtime GC probe**, because at the moment of the collection there is nothing for the collector to find. That is why #7154's from-space scan only ever saw offenders whose targets had already died. The instrument is static: `scripts/gc_root_dominance_check.py` over `--trace llvm` output (`--self-test` proves it can still fail). Only bites under `PERRY_GC_MOVING_LOOP_POLLS=1`, off by default since #7161 — so a green default run says nothing about this class. **Full writeup, every known shape and how to check your work: `docs/src/internals/gc-rooting-invariant.md`.** The CI gate is `gc-root-dominance.yml` over `scripts/gc_root_dominance_corpus.sh`; known-remaining hits are named one-per-entry in `scripts/gc_root_dominance_allowlist.json` (an entry that matches nothing FAILS, so a fix must delete its entry). A fifth shape is open as #7211: `ClassExprFresh` roots only when it thinks the static *initializers* collect, and never asks whether its own emitted `js_object_set_field_by_name` does — the sophisticated version of the mistake, where the author wrote a rooting predicate and it asked the wrong question. diff --git a/docs/src/internals/gc-rooting-invariant.md b/docs/src/internals/gc-rooting-invariant.md index a9766123f9..df8ee4beda 100644 --- a/docs/src/internals/gc-rooting-invariant.md +++ b/docs/src/internals/gc-rooting-invariant.md @@ -163,6 +163,21 @@ on. — read out of a root and held in a register across a collection point. That is the mode that found cases 3 and 4. +`--unrooted-allocas` (#7207) covers the remaining shape, and is the one the +bind-anchored check is structurally blind to: the value lives in a plain +`alloca_entry` for its whole lifetime, so there is no `js_shadow_slot_bind` to +anchor on and a scan that starts from binds calls the function clean. It found +`lower_call/new.rs`'s inline-ctor `this_slot` independently of any runtime +probe. **The gate does not run this mode yet** — its remaining hits are the +caches, staging arrays and inlined-callee params tracked as #7210, and they are +deliberately not in the allowlist, which covers the bind-anchored shape only. +Run it by hand when you touch an `alloca_entry` site: + +```bash +python3 scripts/gc_root_dominance_check.py .perry-trace/llvm \ + --unrooted-allocas --moving-only -v +``` + ### 2. The runtime instruments — second, and mind the depth From #7196: