fix(gc): root every argument of a cross-module direct call (#7240, by @jdalton)

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_<src>__<name>` 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`).

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<String> = Vec::with_capacity(target_arity);
+    let mut arg_guard: Option<String> = 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(), &current);
         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))
 }

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_<src>__<name>` 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<String>, Option<String>)> {
+    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>,
+) -> 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

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_<src>__<name>` 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;
+}

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_<src>__<name>` 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());