diff --git a/changelog.d/7214-closure-calln-stale-registers.md b/changelog.d/7214-closure-calln-stale-registers.md new file mode 100644 index 0000000000..82c22a04a3 --- /dev/null +++ b/changelog.d/7214-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..dba208d8ee 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,23 +1330,43 @@ 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 @@ -1309,10 +1388,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 +1434,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 +1497,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 +1510,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 +1518,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()); 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