From 2fb75d8e40ef1159c82cd0e522bb6d396bb4e248 Mon Sep 17 00:00:00 2001 From: jdalton Date: Sun, 2 Aug 2026 14:21:45 -0400 Subject: [PATCH 1/4] fix(gc): root the rest-argument and same-module direct-call paths #7240 fixed `lower_call/extern_func.rs`'s cross-module NON-rest arm and named two siblings it would not ship unmeasured. These are those two. `extern_func.rs`'s `has_rest` arm had TWO unprotected registers where the non-rest arm had one. The fixed parameters, as before -- except their window does not close when the last argument is lowered, because the rest array is materialized afterwards and materializing it runs `js_array_alloc` plus one `js_array_push_f64` per trailing argument. And the ACCUMULATOR, which has no analogue in the non-rest arm: `current` is a raw `*mut ArrayHeader` in a bare SSA register, threaded through the push loop, holding the only reference to every argument pushed so far while the next argument's expression -- arbitrary user code -- is lowered. Nothing rooted it, so a minor landing in that window was free to SWEEP the array, not merely move it. `func_ref.rs`'s same-module arms, all four, had the identical defect. #7240's regression test needed a two-file fixture precisely because a same-file callee does not reach `extern_func.rs` at all: it resolves through `Expr::FuncRef(fid)` into `func_ref.rs`, so the bug sat one `else` away, unreached by that PR's test. It was not folded into #7240 because `func_ref.rs` threads its lowered arguments through four specialized-ABI dispatch paths, each a fast/fallback diamond with a phi at the merge; the temp-root release has to sit in the merge block that post-dominates all five call sites. The release is emitted AFTER `implicit_this_restore`, and that order is load-bearing. `implicit_this_save` (#7211) runs below the argument lowering, so its slot sits ABOVE this group, and `js_gc_temp_root_truncate` drops `base` and everything above it. Releasing first drops the saved receiver, and `js_gc_temp_root_get` answers an out-of-range read with `0` -- so the restore would rebind the enclosing method's `this` to the NUMBER 0. That is a miscompile, not a rooting bug, and it fires whenever a same-module callee reads dynamic `this` and at least one argument takes a real slot. All five arms now share one `lower_call/mod.rs` helper. Each argument is still gated by `temp_root::operand_protection`, so a list of scalars emits the IR it emitted before. Measured per gap test, compiled AND run with `PERRY_GC_MOVING_LOOP_POLLS=1`: arm parent (6aeef5baf) this commit polls only bad 0 10/10 bad 0 10/10 polls + PERRY_GC_ZEAL=1 0/10, SIGSEGV bad 0 10/10 polls + zeal + PERRY_GEN_GC=0 bad 0 10/10 bad 0 10/10 The first row is why both test files carry a `parity-env:` line: without it the harness runs them in the default configuration, the broken compiler prints `bad 0`, and the files gate nothing. Polls are off by default since #7161, so the IR has no back-edge safepoint to collect on, and without zeal the only collections are allocation-triggered, which take `ManualGcScanGuard::force_full_scan` and make the copying minor ineligible -- nothing moves, so a stale register still names a live object. `run_parity_tests.sh` applies `parity-env` to the perry compile AND the perry run, which is what `PERRY_GC_MOVING_LOOP_POLLS` needs, since it is read at both. The `PERRY_GEN_GC=0` row is the control that proves the tests track collector mode rather than being flaky. Statically, over the 116-source corpus emitted by the parent compiler and read with the parent checker (so this is the codegen delta alone): `--stale-registers --moving-only` 110 -> 62, and `--moving-only --fatal-sinks` 32 -> 0. Those 32 were all `source=alloc sink=js_array_push_f64` -- the unrooted rest accumulator. Refs #7154. --- crates/perry-codegen/src/expr/temp_root.rs | 14 ++ .../src/lower_call/extern_func.rs | 53 +++--- .../perry-codegen/src/lower_call/func_ref.rs | 167 +++++++++--------- crates/perry-codegen/src/lower_call/mod.rs | 131 ++++++++++++++ .../gc_call_arg_rooting_pkg/rest_callee.ts | 31 ++++ .../test_gap_gc_rest_argument_rooting.ts | 120 +++++++++++++ ...ap_gc_same_module_call_argument_rooting.ts | 131 ++++++++++++++ 7 files changed, 532 insertions(+), 115 deletions(-) create mode 100644 test-files/fixtures/gc_call_arg_rooting_pkg/rest_callee.ts create mode 100644 test-files/test_gap_gc_rest_argument_rooting.ts create mode 100644 test-files/test_gap_gc_same_module_call_argument_rooting.ts diff --git a/crates/perry-codegen/src/expr/temp_root.rs b/crates/perry-codegen/src/expr/temp_root.rs index 3c9bed3de6..28421a3916 100644 --- a/crates/perry-codegen/src/expr/temp_root.rs +++ b/crates/perry-codegen/src/expr/temp_root.rs @@ -628,6 +628,20 @@ impl RootedOperands { pub(crate) fn release(self, ctx: &mut FnCtx<'_>) { temp_root_release(ctx, self.guard); } + + /// The group's guard slot, for a caller that must release it together with + /// slots it pushed ITSELF. + /// + /// [`RootedOperands::release`] is the ordinary exit and consumes the group. + /// The rest-argument lowering cannot use it: it pushes accumulator slots + /// ([`rooted_array_begin`]) *above* this group, and because + /// [`temp_root_truncate`] is a stack cut, one truncate at the LOWEST index + /// drops both. So that caller needs the index rather than the act — and it + /// must not release early, since the accumulator has to stay rooted across + /// the consuming call too. + pub(crate) fn guard(&self) -> Option { + self.guard.clone() + } } /// Release a guard returned by [`lower_exprs_rooted`]. Call it *after* the diff --git a/crates/perry-codegen/src/lower_call/extern_func.rs b/crates/perry-codegen/src/lower_call/extern_func.rs index 94dfcaa18b..fdc74b2802 100644 --- a/crates/perry-codegen/src/lower_call/extern_func.rs +++ b/crates/perry-codegen/src/lower_call/extern_func.rs @@ -12,7 +12,7 @@ use perry_api_manifest::{ use perry_hir::types::Type as HirType; use perry_hir::Expr; -use crate::expr::{lower_expr, nanbox_pointer_inline, nanbox_string_inline, unbox_to_i64, FnCtx}; +use crate::expr::{lower_expr, nanbox_string_inline, unbox_to_i64, FnCtx}; use crate::nanbox::{double_literal, POINTER_MASK_I64}; use crate::native_value::{ layout_for_manifest_pod, layout_runtime_id, llvm_type_for_native_rep, materialize_js_value, @@ -1757,38 +1757,31 @@ pub fn try_lower_extern_func_call( ctx.pending_declares .push((fname.clone(), DOUBLE, param_types)); let mut lowered: Vec = Vec::with_capacity(target_arity); - let mut arg_guard: Option = None; + let arg_guard: Option; if has_rest { - // Fixed (non-rest) params: pass through. + // #7154: the rest twin of the arm below. Fixed params were lowered into + // bare registers and then held across `js_array_alloc` plus a + // `js_array_push_f64` per trailing arg, and the accumulator itself was + // a raw array pointer in a bare register holding the only reference to + // everything pushed so far. See `super::lower_rest_call_args_rooted`. + // + // The rest array is materialized ALWAYS — even with zero trailing args, + // the callee's rest binding must be `[]`. #1816: for a synthetic + // `arguments` param, bundle ALL args (from 0), not just the trailing + // ones, so `arguments.length` is correct. let fixed_count = declared_count.saturating_sub(1); - for a in args.iter().take(fixed_count) { - lowered.push(lower_expr(ctx, a)?); - } - // Pad fixed params if the caller passed too few. - let undefined_lit = double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)); - while lowered.len() < fixed_count { - lowered.push(undefined_lit.clone()); - } - // Materialize the rest array (always — even when zero - // trailing args, the callee's rest binding must be `[]`). - // #1816: for a synthetic `arguments` param, bundle ALL args (from 0), - // not just the trailing ones, so `arguments.length` is correct. let bundle_from = if has_synthetic_args { 0 } else { fixed_count }; - let rest_count = args.len().saturating_sub(bundle_from); - let cap = (rest_count as u32).to_string(); - let mut current = ctx.block().call(I64, "js_array_alloc", &[(I32, &cap)]); - for a in args.iter().skip(bundle_from) { - let v = lower_expr(ctx, a)?; - let blk = ctx.block(); - current = blk.call(I64, "js_array_push_f64", &[(I64, ¤t), (DOUBLE, &v)]); - } - if has_synthetic_args { - current = ctx - .block() - .call(I64, "js_array_mark_arguments_object", &[(I64, ¤t)]); - } - let rest_box = nanbox_pointer_inline(ctx.block(), ¤t); - lowered.push(rest_box); + let (values, guard) = super::lower_rest_call_args_rooted( + ctx, + args, + fixed_count, + &[super::RestBundle { + from: bundle_from, + mark_arguments_object: has_synthetic_args, + }], + )?; + arg_guard = guard; + lowered.extend(values); } else { // #7154: the registry's residual. See `super::lower_call_args_rooted`. let (values, guard) = super::lower_call_args_rooted(ctx, args)?; diff --git a/crates/perry-codegen/src/lower_call/func_ref.rs b/crates/perry-codegen/src/lower_call/func_ref.rs index b8cdd24554..6e5fb653e8 100644 --- a/crates/perry-codegen/src/lower_call/func_ref.rs +++ b/crates/perry-codegen/src/lower_call/func_ref.rs @@ -5,7 +5,7 @@ use anyhow::Result; use perry_hir::Expr; -use crate::expr::{i32_bool_to_nanbox, i32_to_nanbox, lower_expr, nanbox_pointer_inline, FnCtx}; +use crate::expr::{i32_bool_to_nanbox, i32_to_nanbox, lower_expr, FnCtx}; use crate::nanbox::double_literal; use crate::native_value::LoweredValue; use crate::types::{DOUBLE, I1, I32, I64, PTR}; @@ -380,101 +380,79 @@ pub fn try_lower_func_ref_call( let sig = ctx.func_signatures.get(fid).copied(); let (declared_count, has_rest, _, synthetic_is_rest) = sig.unwrap_or((args.len(), false, false, false)); + // #7154: the same-module twin of `extern_func.rs`'s cross-module path. + // + // #7240 fixed the cross-module lowering and needed a two-file fixture to do + // it, precisely because a same-file callee resolves here instead — so the + // identical defect sat one `else` away, unreached by that PR's test. All + // four arms below lowered their arguments into bare SSA registers and then + // held them across work that allocates: the rest arms across + // `js_array_alloc` + a `js_array_push_f64` per element (and the first arm + // across TWO such arrays), the plain arm across the later arguments' own + // lowering. + // + // The guard is released after the call rather than here — see the + // `temp_root_release` below the dispatch chain. That placement is the whole + // reason this was not folded into #7240: `lowered` is consumed by four + // specialized-ABI dispatch paths with block-splitting diamonds, so the + // release has to sit in the merge block that post-dominates all of them, + // not next to the lowering. let mut lowered: Vec = Vec::with_capacity(declared_count); + let arg_guard: Option; if ctx.func_synthetic_arguments.contains(fid) && has_rest && !synthetic_is_rest { - let lowered_args: Vec = args - .iter() - .map(|arg| lower_expr(ctx, arg)) - .collect::>()?; + // #1816: a real `...rest` AND a synthetic `arguments`, over the same + // argument list at two different offsets. let fixed_count = declared_count.saturating_sub(2); - let undef_lit = double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)); - for idx in 0..fixed_count { - if let Some(arg) = lowered_args.get(idx) { - lowered.push(arg.clone()); - } else { - lowered.push(undef_lit.clone()); - } - } - - let rest_count = args.len().saturating_sub(fixed_count); - let cap = (rest_count as u32).to_string(); - let mut current = ctx.block().call(I64, "js_array_alloc", &[(I32, &cap)]); - for v in lowered_args.iter().skip(fixed_count) { - let blk = ctx.block(); - current = blk.call( - I64, - "js_array_push_f64", - &[(I64, ¤t), (DOUBLE, v.as_str())], - ); - } - let rest_box = nanbox_pointer_inline(ctx.block(), ¤t); - lowered.push(rest_box); - - let cap = (args.len() as u32).to_string(); - let mut current = ctx.block().call(I64, "js_array_alloc", &[(I32, &cap)]); - for v in &lowered_args { - let blk = ctx.block(); - current = blk.call( - I64, - "js_array_push_f64", - &[(I64, ¤t), (DOUBLE, v.as_str())], - ); - } - let arguments_box = nanbox_pointer_inline(ctx.block(), ¤t); - lowered.push(arguments_box); + let (values, guard) = super::lower_rest_call_args_rooted( + ctx, + args, + fixed_count, + &[ + super::RestBundle { + from: fixed_count, + mark_arguments_object: false, + }, + super::RestBundle { + from: 0, + mark_arguments_object: false, + }, + ], + )?; + arg_guard = guard; + lowered.extend(values); } else if has_rest && ctx.func_synthetic_arguments.contains(fid) { - let lowered_args: Vec = args - .iter() - .map(|arg| lower_expr(ctx, arg)) - .collect::>()?; let fixed_count = declared_count.saturating_sub(1); - let undef_lit = double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)); - for idx in 0..fixed_count { - if let Some(arg) = lowered_args.get(idx) { - lowered.push(arg.clone()); - } else { - lowered.push(undef_lit.clone()); - } - } - - let cap = (args.len() as u32).to_string(); - let mut current = ctx.block().call(I64, "js_array_alloc", &[(I32, &cap)]); - for v in &lowered_args { - let blk = ctx.block(); - current = blk.call( - I64, - "js_array_push_f64", - &[(I64, ¤t), (DOUBLE, v.as_str())], - ); - } - current = ctx - .block() - .call(I64, "js_array_mark_arguments_object", &[(I64, ¤t)]); - let arguments_box = nanbox_pointer_inline(ctx.block(), ¤t); - lowered.push(arguments_box); + let (values, guard) = super::lower_rest_call_args_rooted( + ctx, + args, + fixed_count, + &[super::RestBundle { + from: 0, + mark_arguments_object: true, + }], + )?; + arg_guard = guard; + lowered.extend(values); } else if has_rest { // Rest is always the LAST declared param. Pass the // first (declared_count - 1) args as-is, then bundle // the rest into an array. let fixed_count = declared_count.saturating_sub(1); - for a in args.iter().take(fixed_count) { - lowered.push(lower_expr(ctx, a)?); - } - // Materialize the rest array. - let rest_count = args.len().saturating_sub(fixed_count); - let cap = (rest_count as u32).to_string(); - let mut current = ctx.block().call(I64, "js_array_alloc", &[(I32, &cap)]); - for a in args.iter().skip(fixed_count) { - let v = lower_expr(ctx, a)?; - let blk = ctx.block(); - current = blk.call(I64, "js_array_push_f64", &[(I64, ¤t), (DOUBLE, &v)]); - } - let rest_box = nanbox_pointer_inline(ctx.block(), ¤t); - lowered.push(rest_box); + let (values, guard) = super::lower_rest_call_args_rooted( + ctx, + args, + fixed_count, + &[super::RestBundle { + from: fixed_count, + mark_arguments_object: false, + }], + )?; + arg_guard = guard; + lowered.extend(values); } else { - for a in args { - lowered.push(lower_expr(ctx, a)?); - } + let (values, guard) = super::lower_call_args_rooted(ctx, args)?; + arg_guard = guard; + lowered.extend(values); } let arg_slices: Vec<(crate::types::LlvmType, &str)> = lowered.iter().map(|s| (DOUBLE, s.as_str())).collect(); @@ -905,9 +883,28 @@ pub fn try_lower_func_ref_call( } else { ctx.block().call(DOUBLE, &fname, &arg_slices) }; + // #7154: release the argument roots HERE and nowhere earlier. + // + // Every arm above either emits one call in the current block or splits into + // a fast/fallback diamond and leaves `ctx.current_block` on the merge, so + // this point post-dominates all five call sites. Below the call, because + // the callee allocates while reading these arguments; after the diamond, + // because releasing on one side of it would leave the other side's call + // reading dropped slots. + // + // AFTER `implicit_this_restore`, and that order is load-bearing rather than + // stylistic. `implicit_this_save` runs BELOW the argument lowering, so its + // slot sits ABOVE this group, and `js_gc_temp_root_truncate` drops `base` + // and everything above it. Releasing first therefore drops the saved + // receiver, and `js_gc_temp_root_get` answers an out-of-range read with + // `0` — so the restore would rebind the enclosing method's `this` to the + // NUMBER 0. `implicit_this_restore` truncates at its own (higher) slot, and + // its doc calls out that a caller holding a lower group may release + // afterwards and drop the slot a second time harmlessly. if let Some(prev) = prev_this { crate::expr::temp_root::implicit_this_restore(ctx, prev); } + crate::expr::temp_root::temp_root_release(ctx, arg_guard); if ctx.local_generator_funcs.contains(fid) { let wrap_ptr = format!("@__perry_wrap_{}", fname); let closure_handle = diff --git a/crates/perry-codegen/src/lower_call/mod.rs b/crates/perry-codegen/src/lower_call/mod.rs index 4880bd1246..18f2f911d9 100644 --- a/crates/perry-codegen/src/lower_call/mod.rs +++ b/crates/perry-codegen/src/lower_call/mod.rs @@ -195,6 +195,137 @@ pub(crate) fn emit_rooted_call( result } +/// One array a rest/`arguments` call has to materialize from its argument +/// list: every argument from `from` onwards, optionally flagged as an +/// `arguments` object. +/// +/// A call needs one of these for a real `...rest` binding, one for a synthetic +/// `arguments` param — and #1816's shape needs BOTH, from different offsets +/// over the same already-lowered arguments. +pub(crate) struct RestBundle { + /// First argument index to bundle. `0` for a synthetic `arguments` + /// (which must reflect ALL passed args); `fixed_count` for a real rest. + pub from: usize, + /// Emit `js_array_mark_arguments_object` over the finished array. + pub mark_arguments_object: bool, +} + +/// #7154: lower a rest/`arguments` call's argument list with every value +/// protected across the array construction that follows it. +/// +/// This is [`lower_call_args_rooted`]'s twin for the rest path, and it is a +/// separate function because the hazard is strictly larger. #7240 fixed only +/// the non-rest arm; the rest arm has **two** unprotected registers, not one: +/// +/// 1. **The fixed parameters**, exactly as in the non-rest arm — except their +/// window does not end when the last argument is lowered. The rest array +/// is materialized *afterwards*, and materializing it runs +/// `js_array_alloc` plus one `js_array_push_f64` per trailing argument, +/// every one of which allocates. So `lower_exprs_rooted` is the wrong tool +/// here: it re-reads immediately, and the collection point it must re-read +/// below is a step it never sees. [`RootedOperands`] exists for precisely +/// that, and the caller picking the re-read point is the whole difference. +/// +/// 2. **The accumulator itself** — and this is the one with no analogue in +/// the non-rest arm. `current` is a RAW `*mut ArrayHeader` in a bare SSA +/// register, threaded through the push loop, holding the ONLY reference to +/// every argument pushed so far while the NEXT argument's expression is +/// lowered — arbitrary user code. Nothing roots it, so a minor in that +/// window does not merely move the array, it is free to sweep it. +/// [`temp_root::rooted_array_begin`]'s doc names this exact shape as "the +/// shape behind every variadic / spread / rest argument list"; the helper +/// has existed since #6951 and this path never adopted it. +/// +/// `collects` is unconditionally true for the fixed parameters, and that is a +/// statement about the code rather than a conservative shrug: the rest array +/// is materialized on every path (a callee's rest binding must be `[]` even +/// when nothing trailing was passed), so `js_array_alloc` is always between a +/// fixed parameter and the call that consumes it. Scalar arguments still cost +/// nothing — [`temp_root::operand_protection`] routes anything +/// `expr_is_known_non_pointer_shadow_value` proves is not a heap reference to +/// `Reuse`, so `f(1, 2, ...rest)` emits the IR it emitted before. +/// +/// Returns the values to pass — fixed parameters first, re-read from their +/// roots, then one boxed array per [`RestBundle`] — and the guard for +/// [`emit_rooted_call`]. +/// +/// [`RootedOperands`]: crate::expr::temp_root::RootedOperands +/// [`temp_root::rooted_array_begin`]: crate::expr::temp_root::rooted_array_begin +/// [`temp_root::operand_protection`]: crate::expr::temp_root::operand_protection +pub(crate) fn lower_rest_call_args_rooted( + ctx: &mut FnCtx<'_>, + args: &[Expr], + fixed_count: usize, + bundles: &[RestBundle], +) -> Result<(Vec, Option)> { + use crate::expr::temp_root; + use crate::types::I64; + + let refs: Vec<&Expr> = args.iter().collect(); + // Incrementally, one argument at a time: root each BEFORE the next is + // lowered. Lowering the whole list and rooting it afterwards is not merely + // late, it is worse than doing nothing — by then an earlier value may + // already have been swept and the push publishes a dangling pointer into a + // slot the collector scans. See `root_operands_begin`. + let mut rooted = temp_root::root_operands_begin(refs.len()); + for expr in &refs { + let value = crate::expr::lower_expr(ctx, expr)?; + rooted.push(ctx, expr, &value, true); + } + + let mut lowered: Vec = Vec::with_capacity(fixed_count + bundles.len()); + + // Build every array FIRST and leave each one in its temp-root slot, then + // read them all back at the end. Building array 2 allocates, so array 1's + // pointer must not be sitting in a bare register while it happens — #1816's + // shape wants both a `...rest` and an `arguments` bundle over the same + // list, and that second `js_array_alloc` is a collection point for the + // first array exactly as the push loop is for its elements. + let mut accs: Vec = Vec::with_capacity(bundles.len()); + for bundle in bundles { + let cap = (args.len().saturating_sub(bundle.from) as u32).to_string(); + let acc = temp_root::rooted_array_begin(ctx, &cap); + for i in bundle.from..refs.len() { + // Re-read per element: the previous push allocated, so the register + // this argument was lowered into is already stale. + let value = rooted.reread_one(ctx, &refs, i)?; + temp_root::temp_rooted_array_push(ctx, &acc, &value); + } + accs.push(acc); + } + + // Below every allocation now. `js_array_mark_arguments_object` only sets a + // flag bit and hands the same pointer back (`array/header.rs:1046`), so it + // is safe between the slot read and the box. + let mut boxed_bundles: Vec = Vec::with_capacity(bundles.len()); + for (bundle, acc) in bundles.iter().zip(accs.iter()) { + let mut current = temp_root::rooted_array_read(ctx, acc); + if bundle.mark_arguments_object { + current = ctx + .block() + .call(I64, "js_array_mark_arguments_object", &[(I64, ¤t)]); + } + boxed_bundles.push(crate::expr::nanbox_pointer_inline(ctx.block(), ¤t)); + } + + let undefined_lit = + crate::nanbox::double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)); + for i in 0..fixed_count { + lowered.push(if i < refs.len() { + rooted.reread_one(ctx, &refs, i)? + } else { + undefined_lit.clone() + }); + } + lowered.extend(boxed_bundles); + + // The operand group was pushed BEFORE the accumulators, so its guard is the + // lower index and one truncate at it drops both. When nothing needed a real + // root the first accumulator is the lowest slot and becomes the guard. + let guard = rooted.guard().or_else(|| accs.first().cloned()); + Ok((lowered, guard)) +} + /// Lower a `Call` expression. Two shapes are supported: /// 1. `FuncRef(id)(args...)` — direct call to a user function by HIR id. /// 2. `console.log(expr)` where `expr` lowers to a double — emits a diff --git a/test-files/fixtures/gc_call_arg_rooting_pkg/rest_callee.ts b/test-files/fixtures/gc_call_arg_rooting_pkg/rest_callee.ts new file mode 100644 index 0000000000..7af9795d1a --- /dev/null +++ b/test-files/fixtures/gc_call_arg_rooting_pkg/rest_callee.ts @@ -0,0 +1,31 @@ +// #7154 fixture: the CROSS-MODULE callee with a trailing `...rest` for +// `test-files/test_gap_gc_rest_argument_rooting.ts`. +// +// It lives in its own module for the same reason +// `gc_call_arg_rooting_pkg/callee.ts` does — the defect under test is in +// `lower_call/extern_func.rs`'s cross-module `perry_fn___` path, and +// a same-file callee compiles through `func_ref.rs` instead. It is a SEPARATE +// file from `callee.ts` because the arm is chosen by the callee's signature: +// `joinArgs` has no rest param and takes the arm #7240 fixed, `joinRest` has one +// and takes the arm this test pins. +// +// The declared signature matters. Two fixed params plus a rest means +// `declared_count == 3` and `fixed_count == 2`, so `url` and `method` are +// lowered into bare registers and then held across the whole rest-array +// construction — `js_array_alloc` plus one `js_array_push_f64` per trailing +// argument, with each trailing argument's own expression lowered in between. +// +// The body reads the fixed params AND the rest contents, so a caller that +// handed over a pre-collection address produces wrong text rather than a latent +// bad pointer. +export function joinRest( + url: string, + method: string, + ...tags: number[] +): string { + let total = 0; + for (let i = 0; i < tags.length; i++) { + total += tags[i]; + } + return url + " " + method + " " + tags.length + " " + total; +} diff --git a/test-files/test_gap_gc_rest_argument_rooting.ts b/test-files/test_gap_gc_rest_argument_rooting.ts new file mode 100644 index 0000000000..4d8ad0f89e --- /dev/null +++ b/test-files/test_gap_gc_rest_argument_rooting.ts @@ -0,0 +1,120 @@ +// parity-env: PERRY_GC_MOVING_LOOP_POLLS=1 PERRY_GC_ZEAL=1 +// +// #7154: a cross-module call to a callee with a trailing `...rest` must root +// its fixed parameters AND its accumulating rest array. +// +// THE `parity-env` LINE IS THE TEST. Measured on the parent (`6aeef5baf`): +// without it the harness compiles and runs in the default configuration, the +// broken compiler prints `bad 0` 10/10, and this file gates nothing. Polls are +// off by default since #7161, so the IR has no back-edge safepoint for a minor +// to land on; and without zeal the only collections are allocation-triggered, +// which take `ManualGcScanGuard::force_full_scan` and make the copying minor +// ineligible — so nothing MOVES and a stale register still names a live +// object. `run_parity_tests.sh` applies `parity-env` to the perry compile AND +// the perry run (lines 955/1001), which is exactly what +// `PERRY_GC_MOVING_LOOP_POLLS` needs: it is read at both. With the line, the +// parent SIGSEGVs 10/10. Node ignores both knobs, so the oracle is unchanged. +// +// #7240 fixed `lower_call/extern_func.rs`'s NON-rest arm and named this one as +// a follow-up it could not ship, because the registry does not exercise it and +// an unmeasured GC edit is exactly what the knob-kill policy exists to stop. +// This is that measurement. +// +// The rest arm has TWO unprotected registers where the non-rest arm had one: +// +// 1. THE FIXED PARAMETERS, exactly as in the non-rest arm — except their +// window does not close when the last argument is lowered. The rest array +// is materialized afterwards, and materializing it allocates: +// +// ; url + method -> bare registers +// bl js_array_alloc ; ALLOCATES +// bl perry_fn_…__churn ; trailing arg 1 -- USER CODE +// bl js_array_push_f64 ; ALLOCATES (grow) +// bl perry_fn_…__churn ; trailing arg 2 -- USER CODE +// bl js_array_push_f64 ; ALLOCATES +// … +// fmov d0, d9 ; STALE url +// fmov d1, d10 ; STALE method +// bl perry_fn_…__joinRest +// +// 2. THE ACCUMULATOR, which has no analogue in the non-rest arm and is the +// more dangerous of the two. `current` is a RAW `*mut ArrayHeader` in a +// bare SSA register, threaded through the push loop, holding the ONLY +// reference to every argument pushed so far while the NEXT argument's +// expression is lowered. Nothing roots it, so a minor landing in that +// window is free to SWEEP the array — not merely move it. +// +// `temp_root::rooted_array_begin`'s doc has named this exact shape as "the +// shape behind every variadic / spread / rest argument list" since #6951, and +// `console_promise.rs` has used it since. This path never adopted it. +// +// Both protections from #7240 are exercised: a STRING LITERAL fixed parameter +// is `OperandProtection::Reload` (its `__perry_init_strings_*` handle global is +// a registered root that evacuation rewrites, so re-emitting the load below the +// collection point is correct and costs no runtime call), and a LOCAL fixed +// parameter is `OperandProtection::Root` (re-deriving it could observe a later +// assignment, so it takes a real temp-root slot). +// +// LIVE BY CONSTRUCTION, the same way #7240's test is: `churn` keeps allocating +// AFTER the back-edge poll that collects, so the retired from-space bytes are +// recycled before the callee reads them. A stale read therefore returns wrong +// text rather than the right answer out of memory nobody has reused yet. +// +// The literal arm needs the collection EARLY: `__perry_init_strings_*` runs at +// startup, so a literal is young for the first couple of minors and tenured +// after that, and only a young object is evacuated. Under `PERRY_GC_ZEAL=1` the +// first back-edge poll inside `churn` already runs an evacuating minor, so +// iteration 0 is where the literal arm bites. + +import { joinRest } from "./fixtures/gc_call_arg_rooting_pkg/rest_callee.ts"; + +// Allocates hard, and keeps allocating after the poll that collects, so the +// retired bytes are reused rather than left intact. +function churn(n: number): number { + const bits: any[] = []; + for (let i = 0; i < 200; i++) { + bits.push({ i: i, s: "y" + i, pad: [i, i + 1, i + 2] }); + } + return bits.length === 200 ? n : -1; +} + +function freshUrl(i: number): string { + return "/v0/orgs/" + i + "/full-scans/[full_scan_id]"; +} + +function run(): number { + let bad = 0; + for (let r = 0; r < 8; r++) { + // Reload arm: both fixed parameters are literals — loads of a + // `__perry_init_strings_*` handle global — and three allocating, + // poll-running trailing arguments are lowered and pushed after them. + const litOut = joinRest( + "/v0/orgs/[org_slug]/full-scans", + "GET", + churn(r), + churn(r), + churn(r), + ); + if (litOut !== "/v0/orgs/[org_slug]/full-scans GET 3 " + 3 * r) { + bad++; + } + // Root arm: fixed parameter 1 is a local holding a freshly-allocated string + // (always young, so it moves on every evacuating minor), fixed parameter 2 + // is a literal. One call, both protections. + const url = freshUrl(r); + const freshOut = joinRest(url, "POST", churn(r), churn(r), churn(r)); + if (freshOut !== url + " POST 3 " + 3 * r) { + bad++; + } + // Zero trailing arguments still materializes the array (a rest binding must + // be `[]`), so `js_array_alloc` still sits between the fixed parameters and + // the call. The cheapest shape that keeps the window open. + const emptyOut = joinRest(freshUrl(r), "HEAD"); + if (emptyOut !== "/v0/orgs/" + r + "/full-scans/[full_scan_id] HEAD 0 0") { + bad++; + } + } + return bad; +} + +console.log("bad", run()); diff --git a/test-files/test_gap_gc_same_module_call_argument_rooting.ts b/test-files/test_gap_gc_same_module_call_argument_rooting.ts new file mode 100644 index 0000000000..fdaf336d48 --- /dev/null +++ b/test-files/test_gap_gc_same_module_call_argument_rooting.ts @@ -0,0 +1,131 @@ +// parity-env: PERRY_GC_MOVING_LOOP_POLLS=1 PERRY_GC_ZEAL=1 +// +// #7154: a call to a top-level function in the SAME module must root its +// arguments, exactly as the cross-module call #7240 fixed does. +// +// THE `parity-env` LINE IS THE TEST — see the same note in +// `test_gap_gc_rest_argument_rooting.ts`. Measured on the parent (`6aeef5baf`): +// in the harness's default configuration the broken compiler prints `bad 0` +// 10/10 and this file gates nothing; with the line it SIGSEGVs 10/10. +// +// This is the follow-up #7240 named and could not fold in. Its own regression +// test needed a two-file fixture precisely because a same-file callee does not +// take `lower_call/extern_func.rs`'s path at all — it resolves through +// `Expr::FuncRef(fid)` into `lower_call/func_ref.rs`, which had the identical +// defect one `else` away, in four arms rather than one: +// +// } else if …synthetic `arguments` && rest… { for a in args { lower } … } +// } else if …synthetic `arguments`… { for a in args { lower } … } +// } else if has_rest { for a in args { lower } … } +// } else { for a in args { lower } } +// +// Every one lowered its arguments into bare SSA registers and then held them +// across work that allocates — the rest arms across `js_array_alloc` plus a +// `js_array_push_f64` per element, the plain arm across the later arguments' +// own lowering. +// +// Why this was not simply copied from #7240: `func_ref.rs` threads `lowered` +// through FOUR specialized-ABI dispatch paths (Tier A static, Tier B guarded, +// and the typed-f64 / i32 / string / i1 clones), each a fast/fallback diamond +// with a phi at the merge. The temp-root release has to sit in the merge block +// that post-dominates all five call sites — releasing on one side of a diamond +// leaves the other side's call reading dropped slots. That is a real change, +// not a one-line copy, which is why #7240 named it instead of guessing at it. +// +// Both protections are exercised, as in #7240: a STRING LITERAL argument is +// `OperandProtection::Reload` (its `__perry_init_strings_*` handle global is a +// registered root that an evacuating cycle REWRITES, so re-emitting the load +// below the collection point is correct and free), and a LOCAL argument is +// `OperandProtection::Root` (re-deriving it could observe an assignment made +// after the call-time value was taken, so it takes a real temp-root slot). +// +// LIVE BY CONSTRUCTION: `churn` keeps allocating AFTER the back-edge poll that +// collects, so the retired from-space bytes are recycled before the callee +// reads them and a stale read returns wrong text rather than the right answer +// out of memory nobody has reused yet. + +// Allocates hard, and keeps allocating after the poll that collects. +function churn(n: number): number { + const bits: any[] = []; + for (let i = 0; i < 200; i++) { + bits.push({ i: i, s: "y" + i, pad: [i, i + 1, i + 2] }); + } + return bits.length === 200 ? n : -1; +} + +function freshUrl(i: number): string { + return "/v0/orgs/" + i + "/full-scans/[full_scan_id]"; +} + +// SAME-MODULE callee, no rest: `func_ref.rs`'s plain `else` arm — the direct +// twin of the cross-module arm #7240 fixed. +function joinSame( + url: string, + method: string, + opts: { n: number }, + schemaTag: number, + parseTag: number, +): string { + return url + " " + method + " " + opts.n + " " + schemaTag + " " + parseTag; +} + +// SAME-MODULE callee WITH rest: `func_ref.rs`'s `has_rest` arm. Two fixed +// params plus a rest means the fixed params are held across the whole +// rest-array construction, and the accumulator holds the only reference to +// everything pushed so far while the next argument is lowered. +function joinSameRest( + url: string, + method: string, + ...tags: number[] +): string { + let total = 0; + for (let i = 0; i < tags.length; i++) { + total += tags[i]; + } + return url + " " + method + " " + tags.length + " " + total; +} + +function run(): number { + let bad = 0; + for (let r = 0; r < 8; r++) { + // --- the plain arm ------------------------------------------------------ + // Reload: both string operands are literals, the registry's exact shape. + const litOut = joinSame( + "/v0/orgs/[org_slug]/full-scans", + "GET", + { n: churn(r) }, + churn(r), + churn(r), + ); + if (litOut !== "/v0/orgs/[org_slug]/full-scans GET " + r + " " + r + " " + r) { + bad++; + } + // Root: argument 1 is a local holding a freshly-allocated string, which is + // always young and therefore moves on every evacuating minor. + const url = freshUrl(r); + const freshOut = joinSame(url, "POST", { n: churn(r) }, churn(r), churn(r)); + if (freshOut !== url + " POST " + r + " " + r + " " + r) { + bad++; + } + + // --- the rest arm ------------------------------------------------------- + const litRest = joinSameRest( + "/v0/orgs/[org_slug]/full-scans", + "GET", + churn(r), + churn(r), + churn(r), + ); + if (litRest !== "/v0/orgs/[org_slug]/full-scans GET 3 " + 3 * r) { + bad++; + } + const url2 = freshUrl(r); + const freshRest = joinSameRest(url2, "POST", churn(r), churn(r), churn(r)); + if (freshRest !== url2 + " POST 3 " + 3 * r) { + bad++; + } + } + return bad; +} + +console.log("bad", run()); From 389544933cfb0c02b0f1dd64630abaae7a5be61c Mon Sep 17 00:00:00 2001 From: jdalton Date: Sun, 2 Aug 2026 14:22:15 -0400 Subject: [PATCH 2/4] feat(gc-checker): model a string-literal handle load as a heap-value source `--stale-registers` classified a heap-value SOURCE as an `ALLOC_RE` call or a shadow-slot load. A `load double, ptr @...str.N.handle` is neither, so the register it defines was never tracked and no stale use could be attributed to it -- which is the blind spot #7240 shipped its fix through, as that PR's own writeup says. The pattern already existed and was defined twice, in effect: `--unrooted- allocas` had `REWRITTEN_LOAD_RE` and used it, while `--stale-registers` had only `GLOBAL_ROOT_RE` and knew about `@perry_global_*` alone. The two modes disagreed about what a collector-rewritten load is, and the narrower one was wrong. There is now one definition and both modes read it. Unlike #7226's `js_implicit_this_set` and #7227's `js_regexp_new`, this could not be closed by adding a name to `ALLOC_RE`: the source is a `load`, not a `call`. Strictly additive by construction -- `GLOBAL_ROOT_RE` is consulted first, so no previously reported source changes kind. Measured over the 116-source / 136-module corpus, emitted twice, once by the parent compiler and once by the commit below, so the checker delta and the codegen delta can be read separately: corpus from mode parent this parent codegen --stale-registers 2914 4805 (+1891 strh) parent codegen --moving-only 110 158 (+48 strh) parent codegen --moving-only --fatal-sinks 32 32 this codegen --stale-registers 2858 4693 (+1835 strh) this codegen --moving-only 62 62 (+0) this codegen --moving-only --fatal-sinks 0 0 Read the two middle rows together, because that is the whole result. On the parent's IR the widening exposes 48 stale uses that reach a moving minor, and ALL 48 are in the two gap tests added in the commit below -- every one a `load double, ptr @...str.N.handle` feeding `joinRest` or `joinSameRest` below the rest-array construction, which is precisely the defect that commit fixes. There are none anywhere else in the corpus. On the fixed IR the same widening adds ZERO `--moving-only` uses. The modelling is therefore not too broad: it found one population, that population was real, and it is now empty. The CI gate is untouched -- `gc-root-dominance.yml` runs the bind-anchored mode, not `--stale-registers`, and exits 0 with 0 violations and 40/40 seeded violations caught on both corpora with both checkers. `--self-test` asserts the new source in both directions and under `--moving-only`, so the widening cannot silently stop working. Recorded rather than hidden: the shared `REWRITTEN_LOAD_RE` also names `@perry_class_keys_*`, which `--unrooted-allocas` has always used. It contributes 0 hits in `--stale-registers` over this corpus, so that arm is currently carried by the shared definition rather than exercised by it. Refs #7154. --- scripts/gc_root_dominance_check.py | 166 +++++++++++++++++++++++++++-- 1 file changed, 156 insertions(+), 10 deletions(-) diff --git a/scripts/gc_root_dominance_check.py b/scripts/gc_root_dominance_check.py index f57ab98701..af13eb6481 100755 --- a/scripts/gc_root_dominance_check.py +++ b/scripts/gc_root_dominance_check.py @@ -1375,6 +1375,56 @@ def seeded_violation_test(paths, moving_only, anchor, want_sites, verbose=False) # `@perry_global_*` and are registered roots that evacuation rewrites. GLOBAL_ROOT_RE = re.compile(r"@perry_global_[\w.$]+") +# Loads whose source is a location the collector REWRITES. A register holding +# one of these is stale below a collection point even though the value survives +# — property (2) without property (3), the module-header distinction. +# +# ## Why this lives here rather than only next to `--unrooted-allocas` +# +# It was defined twice, in effect: `--unrooted-allocas` had this pattern and +# used it, while `--stale-registers` had `GLOBAL_ROOT_RE` and knew about +# `@perry_global_*` alone. **The two modes disagreed about what a +# collector-rewritten load is, and the narrower one was wrong.** +# +# The cost of that disagreement is #7240: a string literal lowers to +# `load double, ptr @…_.str.N.handle`, and the handle global IS a registered +# root — `js_gc_register_global_root`, `codegen/string_pool.rs` — so the string +# is never swept, but an evacuating cycle REWRITES the global while a register +# loaded from it beforehand keeps the pre-move address. Measured at #7240's +# fault: the handle global held the post-move address, the register held the +# retired from-space one. `heap_source_kind` classified that load as nothing at +# all, so the register had no recognised source and no stale use could be +# attributed to it. Over #7240's own gap test the checker reported 24 +# `--moving-only` stale uses at the offending call and named NEITHER of the two +# literals that actually faulted; the registry's `alerts.ts` call passes +# literals in both unprotected positions, so it reported nothing whatsoever. +# +# Unlike `js_implicit_this_set` (#7226) and `js_regexp_new` (#7227), this one +# could NOT be closed by adding a name to `ALLOC_RE` — the source is a `load`, +# not a `call`. It needs the source SET widened, which is why it waited for its +# own before/after rather than riding along with a fix. +# +# One definition, both modes, so the next reader cannot re-derive half of it. +REWRITTEN_LOAD_RE = re.compile( + r"load\s+\S+,\s*ptr\s+@(?:" + r"(?P[\w.$]*_\.str\.\d+\.handle)" # string-literal handles + r"|(?Pperry_global_[\w.$]+)" # module-level variables + r"|(?Pperry_class_keys_[\w.$]+)" # class keys (old-gen, C4b movable) + r")" +) + + +def rewritten_load_kind(text): + """Which collector-rewritten global does this load read, if any? + + Returns the source kind (`strhandle` / `global` / `classkeys`) so the + `--stale-registers` breakdown can be triaged one population at a time — + they have genuinely different verdicts, and lumping them under one label + would hide that. + """ + m = REWRITTEN_LOAD_RE.search(text) + return None if m is None else m.lastgroup + # Calls that READ a collector-rewritten location into a register. ROOT_READ_CALLS = { "js_closure_get_capture_bits", # closure/alloc.rs:463 capture cell read @@ -1431,8 +1481,20 @@ def heap_source_kind(ins, slot_of_alloca): return "capture" if "capture" in ins.callee else "rootread" return None if "= load " in ins.text: + # `GLOBAL_ROOT_RE` first, and deliberately: it is the looser of the two + # (it matches the name anywhere in the line, so it still catches a load + # through a `getelementptr` on the global) and it keeps the `global` + # population reporting under exactly the label it reported under + # before. This widening is then strictly ADDITIVE — no previously + # reported source changes kind, only sources that were invisible start + # appearing — which is the property the gate's baselined counts need. if GLOBAL_ROOT_RE.search(ins.text): return "global" + # #7240's third follow-up: a load of a string-literal handle global is + # a heap-value source. See `REWRITTEN_LOAD_RE`. + kind = rewritten_load_kind(ins.text) + if kind is not None: + return kind m = re.search(r"load\s+(?:i64|double)\s*,\s*ptr %([\w.$]+)", ins.text) if m and m.group(1) in slot_of_alloca: return "slotload" @@ -1645,16 +1707,11 @@ def run_stale(parsed, poll_reaching, verbose, moving_only, fatal_only, # positive and never a missed bug. It is reported separately from the # bind-anchored count because its two populations are disjoint by construction. -# Loads whose source is a location the collector REWRITES. A register holding -# one of these is stale below a collection point even though the value survives -# — property (2) without property (3), the module-header distinction. -REWRITTEN_LOAD_RE = re.compile( - r"load\s+\S+,\s*ptr\s+@(?:" - r"\w*_\.str\.\d+\.handle" # string-literal handle globals - r"|perry_global_\w+" # module-level variables - r"|perry_class_keys_\w+" # class keys arrays -- see EXEMPTIONS - r")" -) +# `REWRITTEN_LOAD_RE` — the loads whose source the collector rewrites — is +# defined once, up with `GLOBAL_ROOT_RE` and the rest of the heap-value source +# vocabulary, because `--stale-registers` needs the same answer this mode does. +# It used to live only here, and the two modes disagreeing about it is exactly +# what made #7240's string-literal argument invisible; see the comment there. # Calls that MATERIALIZE a heap value (a superset of ALLOC_RE: anything that # hands back an object the collector can move). @@ -2241,6 +2298,38 @@ def window_hits(A, B): """ +# #7240's blind spot, both directions. `%lit` is a load of a string-literal +# handle global — a registered root, so the string is never SWEPT, but an +# evacuating cycle rewrites the global and leaves this register naming +# from-space. It is then passed as a call argument below `js_call_function`, +# which is exactly the registry's `defineApiCall(url, method, …)` shape. +# +# Before the widening `heap_source_kind` returned `None` for that load, so the +# register had no source and the mode reported ZERO over this fixture. +_SELFTEST_STR_HANDLE = """\ +define double @perry_fn_selftest__strhandle(double %a) { +entry.0: + %lit = load double, ptr @perry_mod_.str.3.handle + %ret = call double @js_call_function(double %a) + %r = call double @perry_fn_other__callee(double %lit, double %ret) + ret double %r +} +""" + +# The fix's shape: the load is re-emitted BELOW the collection point, so it +# observes the address evacuation wrote back. No runtime call, no temp root — +# `OperandProtection::Reload`. +_SELFTEST_STR_HANDLE_RELOADED = """\ +define double @perry_fn_selftest__reload(double %a) { +entry.0: + %ret = call double @js_call_function(double %a) + %lit = load double, ptr @perry_mod_.str.3.handle + %r = call double @perry_fn_other__callee(double %lit, double %ret) + ret double %r +} +""" + + def _scan_unrooted(paths, moving_only=False, **source_opts): """(violations, n_gc_capable_allocas) over `paths`.""" parsed = [(os.path.basename(p), parse_file(p)) for p in sorted(paths)] @@ -2301,6 +2390,22 @@ def _stale_probe(path, max_stale): return (int(m.group(1)) if m else -1), rc +def _stale_kinds_probe(path, moving_only=False): + """{source kind: count} from --stale-registers over one file. + + The breakdown, not just the total: this widening's whole claim is that a + specific SOURCE became visible, and a total can move for any reason. + """ + parsed = [(os.path.basename(path), parse_file(path))] + poll_reaching, _known = compute_poll_reaching( + [f for _m, fs in parsed for f in fs]) + buf = io.StringIO() + with contextlib.redirect_stdout(buf), contextlib.redirect_stderr(buf): + run_stale(parsed, poll_reaching, False, moving_only, False, None) + return {k: int(n) + for n, k in re.findall(r"(\d+)\s+source=(\w+)", buf.getvalue())} + + def _main_probe(argv): """Exit status of a full `main()` run over `argv` (no `sys.exit`). @@ -2489,6 +2594,47 @@ def self_test(): "violations", file=sys.stderr) ok = False + # --- the string-literal handle source, both directions -------------- + # + # #7240 shipped a codegen fix its own checker could not see. The load + # of a `__perry_init_strings_*` handle global is a heap-value SOURCE — + # the global is a registered root that evacuation REWRITES, so a + # register loaded from it beforehand names from-space. Asserted as a + # named source rather than as a total, because a total moves for any + # reason and the claim here is about one specific population. + strh = os.path.join(td, "strhandle.ll") + strh_fixed = os.path.join(td, "strhandle_reloaded.ll") + for p, text in ((strh, _SELFTEST_STR_HANDLE), + (strh_fixed, _SELFTEST_STR_HANDLE_RELOADED)): + with open(p, "w") as fh: + fh.write(text) + + kinds = _stale_kinds_probe(strh) + if kinds.get("strhandle", 0) != 1: + print("self-test FAIL: --stale-registers must report the load of a " + "string-literal handle global as source=strhandle. Got " + f"{kinds!r}. That load is a registered root the collector " + "REWRITES; a register holding it is stale below a collection " + "point, and missing it is what made #7240 invisible.", + file=sys.stderr) + ok = False + # `--moving-only` is the mode `gc-root-dominance.yml` gates on. A source + # the gate cannot classify as reaching a moving minor is a source the + # gate cannot fail on, so the raw count alone proves nothing. + moving_kinds = _stale_kinds_probe(strh, moving_only=True) + if moving_kinds.get("strhandle", 0) != 1: + print("self-test FAIL: the planted strhandle use reaches a moving " + "minor via js_call_function and must survive --moving-only. " + f"Got {moving_kinds!r}", file=sys.stderr) + ok = False + if _stale_kinds_probe(strh_fixed).get("strhandle", 0) != 0: + print("self-test FAIL: re-loading the handle global BELOW the " + "collection point is the fix (OperandProtection::Reload), so " + "the control fixture must report no strhandle use. The check " + "reports every literal load and cannot tell fixed from " + "broken.", file=sys.stderr) + ok = False + try: _scan([broken], False, "alloc") except MalformedIR: From 50c84a51d56eec1e21cc6bb71ae8d8103e03c92b Mon Sep 17 00:00:00 2001 From: jdalton Date: Sun, 2 Aug 2026 14:22:15 -0400 Subject: [PATCH 3/4] docs(changelog): fragment for the #7154 rest/same-module rooting follow-ups --- ...t-and-same-module-call-argument-rooting.md | 153 ++++++++++++++++++ 1 file changed, 153 insertions(+) create mode 100644 changelog.d/7241-rest-and-same-module-call-argument-rooting.md diff --git a/changelog.d/7241-rest-and-same-module-call-argument-rooting.md b/changelog.d/7241-rest-and-same-module-call-argument-rooting.md new file mode 100644 index 0000000000..6fcb1aece1 --- /dev/null +++ b/changelog.d/7241-rest-and-same-module-call-argument-rooting.md @@ -0,0 +1,153 @@ +### Fixed + +- **`codegen`: the rest-argument and same-module direct-call paths now root + their arguments too** (#7154). #7240 fixed `lower_call/extern_func.rs`'s + cross-module NON-rest arm and named two siblings it would not ship + unmeasured. These are those two, each with a gap test that is a hard fault on + the parent. + + **`extern_func.rs`'s `has_rest` arm** had *two* unprotected registers where + the non-rest arm had one. The fixed parameters, as before — except their + window does not close when the last argument is lowered, because the rest + array is materialized afterwards and materializing it runs `js_array_alloc` + plus one `js_array_push_f64` per trailing argument. And the **accumulator**, + which has no analogue in the non-rest arm: `current` is a raw + `*mut ArrayHeader` in a bare SSA register, threaded through the push loop, + holding the only reference to every argument pushed so far while the next + argument's expression is lowered. Nothing rooted it, so a minor landing in + that window was free to *sweep* the array, not merely move it. + `temp_root::rooted_array_begin` has named this exact shape as "the shape + behind every variadic / spread / rest argument list" since #6951 and + `console_promise.rs` has used it since; this path never adopted it. + + **`func_ref.rs`'s same-module arms** — all four — had the identical defect. + #7240's regression test needed a two-file fixture precisely because a + same-file callee does not reach `extern_func.rs` at all: it resolves through + `Expr::FuncRef(fid)` into `func_ref.rs`, so the bug sat one `else` away, + unreached by that PR's test. This was not folded into #7240 because + `func_ref.rs` threads its lowered arguments through four specialized-ABI + dispatch paths, each a fast/fallback diamond with a phi at the merge; the + temp-root release has to sit in the merge block that post-dominates all five + call sites, since releasing on one side of a diamond leaves the other side's + call reading dropped slots. + + All five arms now share one `lower_call/mod.rs` helper rather than five + copies of the same three mistakes. Each argument is still gated by + `temp_root::operand_protection`, so a list of scalars emits the IR it emitted + before. + + **The `func_ref.rs` release is emitted after `implicit_this_restore`, not + before**, and the order is load-bearing. `implicit_this_save` (#7211) runs + *below* the argument lowering, so its slot sits *above* this group, and + `js_gc_temp_root_truncate` drops `base` and everything above it. Releasing + first therefore drops the saved receiver, and `js_gc_temp_root_get` answers an + out-of-range read with `0` — so the restore would rebind the enclosing + method's `this` to the *number* `0`. That is a miscompile, not a rooting bug, + and it fires whenever a same-module callee reads dynamic `this` and at least + one argument takes a real slot. + + Measured, per gap test, compiled **and** run with + `PERRY_GC_MOVING_LOOP_POLLS=1`: + + | | parent (`6aeef5baf`) | this branch | + |---|---|---| + | polls only | `bad 0` 10/10 | `bad 0` 10/10 | + | polls + `PERRY_GC_ZEAL=1` | **0/10 — SIGSEGV every run** | `bad 0` **10/10** | + | polls + zeal + `PERRY_GEN_GC=0` | `bad 0` 10/10 | `bad 0` 10/10 | + + The first row is why both test files carry a `parity-env:` line. Without it + the harness runs them in the default configuration, the broken compiler prints + `bad 0`, and the files gate nothing: polls are off by default since #7161, so + the IR has no back-edge safepoint to collect on, and without zeal the only + collections are allocation-triggered, which take + `ManualGcScanGuard::force_full_scan` and make the copying minor ineligible — + nothing moves, so a stale register still names a live object. + `run_parity_tests.sh` applies `parity-env` to the perry compile *and* the + perry run, which is what `PERRY_GC_MOVING_LOOP_POLLS` needs, since it is read + at both. The `PERRY_GEN_GC=0` row is the control that proves the tests track + collector mode rather than being flaky. + + `sfw-registry --help`, `PERRY_FORCE_WELL_KNOWN=iovalkey`, compiled and run + with `PERRY_GC_MOVING_LOOP_POLLS=1`, same runtime archives and same routing + decisions on both arms: unchanged at **30/30**. This PR is not measured as a + registry improvement — #7240 already took that workload to 30/30, and the + point here is that these three edits do not give it back. + +### Added + +- `test-files/test_gap_gc_rest_argument_rooting.ts` (+ its cross-module fixture + `test-files/fixtures/gc_call_arg_rooting_pkg/rest_callee.ts`) and + `test-files/test_gap_gc_same_module_call_argument_rooting.ts`. The rest test + needs its own fixture file rather than reusing `callee.ts`, because the arm is + chosen by the *callee's* signature: `joinArgs` has no rest param and takes the + arm #7240 fixed, `joinRest` has one and takes the arm pinned here. The + same-module test needs no fixture at all, for the same reason in reverse — a + same-file callee is what routes the call into `func_ref.rs`. Both exercise + both protections: a string-literal argument is `OperandProtection::Reload`, a + local holding a freshly-allocated string is `OperandProtection::Root`. + +### Changed + +- **`scripts/gc_root_dominance_check.py` models a load of a string-literal + handle global as a heap-value source** (#7154). `--stale-registers` + classified a source as an `ALLOC_RE` call or a shadow-slot load; a + `load double, ptr @…_.str.N.handle` is neither, so the register it defines + was never tracked and no stale use could be attributed to it. That is the + blind spot #7240 shipped its fix through, and #7240's own writeup says so: + "the register it defines is never tracked as a heap value and no stale use + can be attributed to it". Demonstrated below rather than asserted — on the + parent's IR the widening reports 48 `--moving-only` uses the parent checker + reports zero of, and every one is a literal argument at the faulting call. + + The pattern already existed: `--unrooted-allocas` had `REWRITTEN_LOAD_RE` and + used it, while `--stale-registers` had only `GLOBAL_ROOT_RE` and knew about + `@perry_global_*` alone. **The two modes disagreed about what a + collector-rewritten load is, and the narrower one was wrong.** There is now + one definition and both modes read it. Unlike #7226's `js_implicit_this_set` + and #7227's `js_regexp_new`, this could not be closed by adding a name to + `ALLOC_RE` — the source is a `load`, not a `call`. + + Strictly additive by construction: `GLOBAL_ROOT_RE` is still consulted first, + so no previously reported source changes kind. + + Measured over the 116-source / 136-module corpus + (`scripts/gc_root_dominance_corpus.sh`), emitted twice — once by the parent + compiler and once by this branch's — so the checker delta and the codegen + delta can be read separately. Columns are the checker; rows are the compiler + that emitted the IR. + + | corpus emitted by | mode | parent checker | this checker | + |---|---|---|---| + | parent codegen | `--stale-registers` | 2914 | **4805** (+1891 `strhandle`) | + | parent codegen | `--moving-only` | 110 | **158** (+48 `strhandle`) | + | parent codegen | `--moving-only --fatal-sinks` | 32 | 32 | + | this codegen | `--stale-registers` | 2858 | **4693** (+1835 `strhandle`) | + | this codegen | `--moving-only` | 62 | **62** (+0) | + | this codegen | `--moving-only --fatal-sinks` | 0 | 0 | + + Read the two middle rows together, because that is the whole result. On the + parent's IR the widening exposes 48 stale uses that reach a moving minor, and + **all 48 are in the two gap tests added here** — every one is a + `load double, ptr @…_.str.N.handle` feeding `joinRest` or `joinSameRest` + below the rest-array construction, which is precisely the defect the codegen + half of this PR fixes. There are none anywhere else in the corpus. On this + branch's IR the same widening adds **zero** `--moving-only` uses. So the + modelling is not too broad: it found one population, that population was + real, and it is now empty. + + The codegen change reads out of the same table down the parent-checker + column, which is an apples-to-apples measurement of the fix alone: + `--moving-only` 110 → 62, and `--moving-only --fatal-sinks` **32 → 0**. Those + 32 were all `source=alloc sink=js_array_push_f64` — the unrooted rest + accumulator, reported as an allocation held across the next `push`. + + The gate itself is untouched: `gc-root-dominance.yml` runs the bind-anchored + mode, not `--stale-registers`, and exits 0 with 0 violations and 40/40 seeded + violations caught on both corpora with both checkers. `--self-test` asserts + the new source in both directions and under `--moving-only`, so the widening + cannot silently stop working. + + One caveat recorded rather than hidden: the shared `REWRITTEN_LOAD_RE` also + names `@perry_class_keys_*`, which `--unrooted-allocas` has always used. It + contributes **0** hits in `--stale-registers` over this corpus, so that arm is + currently carried by the shared definition rather than exercised by it. From 711c0962179d44eb34cb93c852b91d66b30f6aba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 2 Aug 2026 20:50:53 +0200 Subject: [PATCH 4/4] test(gc): register #7270's two witnesses, and PR-key its changelog fragment --- ...7270-rest-and-same-module-call-argument-rooting.md} | 0 crates/perry-codegen/src/lower_call/mod.rs | 3 +-- test-parity/gc_repsel_corpus.txt | 10 ++++++++++ 3 files changed, 11 insertions(+), 2 deletions(-) rename changelog.d/{7241-rest-and-same-module-call-argument-rooting.md => 7270-rest-and-same-module-call-argument-rooting.md} (100%) diff --git a/changelog.d/7241-rest-and-same-module-call-argument-rooting.md b/changelog.d/7270-rest-and-same-module-call-argument-rooting.md similarity index 100% rename from changelog.d/7241-rest-and-same-module-call-argument-rooting.md rename to changelog.d/7270-rest-and-same-module-call-argument-rooting.md diff --git a/crates/perry-codegen/src/lower_call/mod.rs b/crates/perry-codegen/src/lower_call/mod.rs index 18f2f911d9..aec7d6230b 100644 --- a/crates/perry-codegen/src/lower_call/mod.rs +++ b/crates/perry-codegen/src/lower_call/mod.rs @@ -308,8 +308,7 @@ pub(crate) fn lower_rest_call_args_rooted( boxed_bundles.push(crate::expr::nanbox_pointer_inline(ctx.block(), ¤t)); } - let undefined_lit = - crate::nanbox::double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)); + let undefined_lit = crate::nanbox::double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)); for i in 0..fixed_count { lowered.push(if i < refs.len() { rooted.reread_one(ctx, &refs, i)? diff --git a/test-parity/gc_repsel_corpus.txt b/test-parity/gc_repsel_corpus.txt index 4ea39a54a6..c90c305098 100644 --- a/test-parity/gc_repsel_corpus.txt +++ b/test-parity/gc_repsel_corpus.txt @@ -296,6 +296,16 @@ test_gap_gc_closure_call_argument_rooting # mistake. Registering the file is the whole fix. test_gap_gc_call_argument_rooting +# --- #7270's two witnesses, registered here rather than dark ----------------- +# `rest_argument_rooting` and `same_module_call_argument_rooting` shipped with +# #7270. Registering them at merge rather than after: they are the FOURTH +# instance of the add-a-witness-forget-the-line defect (#7192, #7216, #7252 +# above), and #7252's was caught only by the `test_gap_gc_*` enforcement firing +# on the first main-line `GC Moving Witnesses` run after #7253 gave that matrix +# a trigger at all. Four occurrences is a process problem, not four mistakes. +test_gap_gc_rest_argument_rooting +test_gap_gc_same_module_call_argument_rooting + # --- Two witnesses that were registered nowhere (#7192, #7216) --------------- # Both files exist in test-files/, both say in their own headers that they are # LIVE BY CONSTRUCTION and bite only on the moving arms, and neither was in this