diff --git a/changelog.d/7240-cross-module-call-argument-rooting.md b/changelog.d/7240-cross-module-call-argument-rooting.md new file mode 100644 index 0000000000..ee106c37ff --- /dev/null +++ b/changelog.d/7240-cross-module-call-argument-rooting.md @@ -0,0 +1,63 @@ +### Fixed + +- **`codegen`: every argument of a cross-module direct call is now protected + across the lowering of the arguments that follow it** (#7154). An argument + list is evaluated left to right and each finished value sits in a bare SSA + register while the later ones are lowered. `lower_call/extern_func.rs`'s + generic `perry_fn___` path lowered the whole list in a plain + `for a in args` loop with no protection at all, so + `f(A, B, {…}, Schema.array(), body => …)` leaves `A` and `B` naming + pre-collection addresses the moment an evacuating minor lands in argument 3, + 4 or 5 — and it does: argument 3 allocates an object, argument 4 runs user + code with its own loop back-edge polls, argument 5 allocates a closure. + + This is the residual #7227 measured and named rather than fixed. In the + `sfw-registry` reproducer it is `src/lib/api/alerts.ts`'s module init calling + `defineApiCall(url, method, {…}, SocketAlert.array(), body => …)` across the + module boundary. The fault surfaces one frame down, inside `js_regexp_test` + called from `defineApiCall + 428`, because the stale `url` argument is what + `/\[[a-zA-Z]+\]/.test(url)` hands to it: + + ```asm + ldp d9, d10, [x24, #0x18] ; url + method, loaded from their + ; __perry_init_strings_* handle globals + bl js_object_alloc_class_inline_keys ; argument 3 -- ALLOCATES + bl perry_fn_…zod… ; argument 4 -- USER CODE + bl js_closure_alloc_singleton ; argument 5 -- ALLOCATES + fmov d0, d9 ; STALE + fmov d1, d10 ; STALE + bl perry_fn_src_lib_api_shared_ts__defineApiCall + ``` + + The diagnosis is a measurement rather than a reading of the disassembly. At + the fault the `__perry_init_strings_*` handle global held `0x…76561xxx` — the + post-move address evacuation wrote back — while `defineApiCall`'s shadow slot + (and the register it was stored from) held `0x…74eb5d58`, inside the + quarantined from-space block the reporter named. Root rewritten, register not. + + A string-literal argument therefore takes `OperandProtection::Reload`: its + handle global is a registered root, so the string is never *swept*, and the + fix is to emit the load again below the collection point — no runtime call at + all. Non-literal arguments take a real temp root, as + `temp_root::lower_exprs_rooted` already does for the `new C(…)` argument list + (#6969). Each argument is gated on `any_later_ref_may_trigger_gc`, so an + argument list with nothing allocating after it emits exactly the IR it did + before. + + **Why `scripts/gc_root_dominance_check.py` reports nothing here**, which is + the part worth carrying forward: the checker classifies a heap-value SOURCE + as an `ALLOC_RE` call or a shadow-slot load. A load of a string-literal + handle global is neither, so the register it defines is never tracked as a + heap value and no stale use can be attributed to it. That is a third shape of + the same blind spot `js_implicit_this_set` (#7226) and `js_regexp_new` (#7227) + each cost a round for — and unlike those two it is not fixed by adding a name + to a pattern, because the source is a `load`, not a `call`. + +### Added + +- `test-files/test_gap_gc_call_argument_rooting.ts` (+ its cross-module fixture + `test-files/fixtures/gc_call_arg_rooting_pkg/callee.ts`). It has to be two + files: the defect is in the cross-module lowering, and a same-file callee + compiles through `func_ref.rs` instead. Both protections are exercised — one + call passes two string literals (`Reload`), the other passes a local holding + a freshly-allocated string plus a literal (`Root` + `Reload`). diff --git a/crates/perry-codegen/src/lower_call/extern_func.rs b/crates/perry-codegen/src/lower_call/extern_func.rs index 7a0edd2d1b..94dfcaa18b 100644 --- a/crates/perry-codegen/src/lower_call/extern_func.rs +++ b/crates/perry-codegen/src/lower_call/extern_func.rs @@ -1757,6 +1757,7 @@ 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; if has_rest { // Fixed (non-rest) params: pass through. let fixed_count = declared_count.saturating_sub(1); @@ -1789,16 +1790,16 @@ pub fn try_lower_extern_func_call( let rest_box = nanbox_pointer_inline(ctx.block(), ¤t); lowered.push(rest_box); } else { - for a in args { - lowered.push(lower_expr(ctx, a)?); - } + // #7154: the registry's residual. See `super::lower_call_args_rooted`. + let (values, guard) = super::lower_call_args_rooted(ctx, args)?; + arg_guard = guard; + lowered.extend(values); // Pad with TAG_UNDEFINED for the missing trailing args. let undefined_lit = double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)); while lowered.len() < target_arity { lowered.push(undefined_lit.clone()); } } - let arg_slices: Vec<(crate::types::LlvmType, &str)> = - lowered.iter().map(|s| (DOUBLE, s.as_str())).collect(); - Ok(Some(ctx.block().call(DOUBLE, &fname, &arg_slices))) + let call = super::emit_rooted_call(ctx, &fname, &lowered, arg_guard); + Ok(Some(call)) } diff --git a/crates/perry-codegen/src/lower_call/mod.rs b/crates/perry-codegen/src/lower_call/mod.rs index 7913ca5e04..4880bd1246 100644 --- a/crates/perry-codegen/src/lower_call/mod.rs +++ b/crates/perry-codegen/src/lower_call/mod.rs @@ -136,6 +136,65 @@ pub(crate) use options::extract_options_fields; // API as `lower_call::iter_native_module_table` — keep that path stable. pub(crate) use native_table::iter_native_module_table; +/// #7154: lower a direct call's argument list with each already-evaluated +/// argument protected across the evaluation of the ones that follow it. +/// +/// An argument list is evaluated left to right, and every value produced so +/// far lives in a bare SSA register while the later ones are lowered. The +/// cross-module `perry_fn___` path lowered the whole list in a +/// plain `for a in args` loop with no protection at all, so +/// `f(URL, "GET", {…}, Schema.array(), body => …)` — the `sfw-registry` +/// reproducer's `defineApiCall(…)` shape — leaves arguments 1 and 2 naming +/// pre-collection addresses the moment an evacuating minor lands in argument +/// 3, 4 or 5. It does: argument 3 is an object literal (allocates), argument 4 +/// runs user code with its own loop back-edge polls, argument 5 allocates a +/// closure. +/// +/// The two string literals are the case that faulted, and they are the *cheap* +/// case to fix. A literal lowers to a load of a `__perry_init_strings_*` handle +/// global, which IS a registered root — so the string is never swept — but an +/// evacuating cycle REWRITES that global while the register keeps the pre-move +/// address. [`OperandProtection::Reload`] re-emits the load below the collection +/// point and costs no runtime call at all. Measured at the fault: the handle +/// global held the post-move address, the register held the retired from-space +/// one. +/// +/// [`temp_root::lower_exprs_rooted`] gates each argument on +/// `any_later_ref_may_trigger_gc`, so an argument list nothing allocating +/// follows emits exactly the IR it emitted before. +/// +/// Returns the values to pass and the guard for [`emit_rooted_call`]. +/// +/// [`OperandProtection::Reload`]: crate::expr::temp_root +/// [`temp_root::lower_exprs_rooted`]: crate::expr::temp_root::lower_exprs_rooted +pub(crate) fn lower_call_args_rooted( + ctx: &mut FnCtx<'_>, + args: &[Expr], +) -> Result<(Vec, Option)> { + let refs: Vec<&Expr> = args.iter().collect(); + crate::expr::temp_root::lower_exprs_rooted(ctx, &refs) +} + +/// Emit a direct call over an already-lowered argument list, then release the +/// [`lower_call_args_rooted`] guard. +/// +/// The release has to sit BELOW the call, not above it: the callee allocates +/// while reading these arguments, so the slots have to outlive the call itself. +pub(crate) fn emit_rooted_call( + ctx: &mut FnCtx<'_>, + fname: &str, + lowered: &[String], + guard: Option, +) -> String { + let arg_slices: Vec<(crate::types::LlvmType, &str)> = lowered + .iter() + .map(|s| (crate::types::DOUBLE, s.as_str())) + .collect(); + let result = ctx.block().call(crate::types::DOUBLE, fname, &arg_slices); + crate::expr::temp_root::temp_root_release(ctx, guard); + result +} + /// 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/callee.ts b/test-files/fixtures/gc_call_arg_rooting_pkg/callee.ts new file mode 100644 index 0000000000..b87657a052 --- /dev/null +++ b/test-files/fixtures/gc_call_arg_rooting_pkg/callee.ts @@ -0,0 +1,20 @@ +// #7154 fixture: the CROSS-MODULE callee for +// `test-files/test_gap_gc_call_argument_rooting.ts`. +// +// It has to live in its own module because the defect under test is in the +// cross-module direct-call lowering (`lower_call/extern_func.rs`'s +// `perry_fn___` path), which is a different code path from the +// same-module one. A same-file callee would compile through `func_ref.rs` and +// never touch the arm this test pins. +// +// The body reads every string argument, so a caller that handed over a +// pre-collection address produces wrong text rather than a latent bad pointer. +export function joinArgs( + url: string, + method: string, + opts: { n: number }, + schemaTag: number, + parseTag: number, +): string { + return url + " " + method + " " + opts.n + " " + schemaTag + " " + parseTag; +} diff --git a/test-files/test_gap_gc_call_argument_rooting.ts b/test-files/test_gap_gc_call_argument_rooting.ts new file mode 100644 index 0000000000..d37e6ad10c --- /dev/null +++ b/test-files/test_gap_gc_call_argument_rooting.ts @@ -0,0 +1,106 @@ +// #7154: every argument of a cross-module direct call must survive the +// lowering of the arguments that follow it. +// +// An argument list is evaluated left to right and each finished value sits in +// a bare SSA register while the later ones are lowered. `lower_call/ +// extern_func.rs`'s `perry_fn___` path lowered the whole list in a +// plain loop with no protection at all, so `f(A, B, alloc(), userCode(), …)` +// leaves A and B naming pre-collection addresses the moment an evacuating +// minor lands in argument 3, 4 or 5. +// +// This is the residual #7227 measured and named. In the `sfw-registry` +// reproducer it is `src/lib/api/alerts.ts`'s module init calling +// `defineApiCall(url, method, {…}, Schema.array(), body => JSON.parse(body))` +// across the module boundary, faulting one frame down at +// `perry_fn_src_lib_api_shared_ts__defineApiCall + 428` inside `js_regexp_test` +// with `obj_type=3` (a string). The compiled shape: +// +// ldp d9, d10, [x24, #0x18] ; url + method, loaded from their +// ; `__perry_init_strings_*` handle globals +// bl js_object_alloc_class_inline_keys ; argument 3 -- ALLOCATES +// bl perry_fn_…__SocketAlert / zod ; argument 4 -- USER CODE +// bl js_closure_alloc_singleton ; argument 5 -- ALLOCATES +// fmov d0, d9 ; STALE +// fmov d1, d10 ; STALE +// bl perry_fn_src_lib_api_shared_ts__defineApiCall +// +// Measured at the fault, which is what makes the diagnosis a fact rather than +// a reading of the disassembly: the handle global held `0x…76561xxx` — the +// post-move address evacuation wrote back — while the register (and therefore +// the callee's shadow slot) held `0x…74eb5d58`, inside the quarantined +// from-space block the reporter named. +// +// Two protections, both exercised below: +// +// * a STRING LITERAL argument is `OperandProtection::Reload`. Its handle +// global is a registered root, so the string is never swept — but an +// evacuating cycle REWRITES that global, so the fix is to emit the load +// again below the collection point. No runtime call at all. +// * a LOCAL argument is `OperandProtection::Root`. Re-deriving it would +// observe an assignment made after the call-time value was taken, so it +// takes a real temp-root slot instead. +// +// Why the static checker reports nothing here: `gc_root_dominance_check.py` +// classifies a heap-value SOURCE as an `ALLOC_RE` call or a shadow-slot load. +// A load of a string-literal handle global is neither, so the register it +// defines is never tracked as a heap value and no stale use is attributed to +// it. That is the same shape of blind spot `js_implicit_this_set` (#7226) and +// `js_regexp_new` (#7227) each cost a round for. +// +// LIVE BY CONSTRUCTION. `churn` keeps allocating AFTER the back-edge poll that +// collects, so the abandoned from-space bytes are recycled before the callee +// reads them — a stale read returns wrong text instead of the right answer out +// of memory nobody has reused yet. Both arms compare against strings built +// from values re-read after the call, so a stale argument is observable. +// +// The literal arm needs the collection EARLY: a string literal is allocated by +// `__perry_init_strings_*` at startup, so it 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. The loop is +// short on purpose — zeal collects at every safepoint. + +import { joinArgs } from "./fixtures/gc_call_arg_rooting_pkg/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 string operands are literals, so both are loads of a + // `__perry_init_strings_*` handle global — the registry's exact shape. + const litOut = joinArgs( + "/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 arm: argument 1 is a local holding a freshly-allocated string + // (always young, so it moves on every evacuating minor), argument 2 is a + // literal. One call, both protections. + const url = freshUrl(r); + const freshOut = joinArgs(url, "POST", { n: churn(r) }, churn(r), churn(r)); + if (freshOut !== url + " POST " + r + " " + r + " " + r) { + bad++; + } + } + return bad; +} + +console.log("bad", run());