fix(gc): re-derive a string-literal operand below the collection point (#7114)

changelog.d/7116-string-literal-operand-stale-after-evacuation.md

@@ -0,0 +1,56 @@
+### Fixed
+
+- **A string-literal operand was reused from a register across an allocating
+  sibling, so an evacuating collection silently truncated the result (#7114).**
+  `console.log("acc:" + run(10_000_000))` printed an **empty line and exited 0**
+  — no crash, no diagnostic. The corruption was allocation-count dependent
+  (correct at 100 000 iterations, prefix replaced by a garbage byte or gone
+  entirely at 10⁷), which is the signature of a stale heap address rather than a
+  logic error. Hoisting the call into its own statement made it correct, which
+  localised it to operand evaluation order rather than to `run`, to
+  `js_string_concat_value`, or to the arithmetic.
+
+  In the emitted IR the literal's `__perry_init_strings_*` handle was loaded
+  *above* the call and masked to a pointer *below* it:
+
+  ```llvm
+  %r1 = load double, ptr @m_ts_.str.1.handle          ; read BEFORE
+  %r2 = call double @perry_fn_m_ts__run__spec_i32(i32 10000000)
+  %r3 = bitcast double %r1 to i64                     ; STALE
+  %r4 = and  i64 %r3, 281474976710655
+  %r5 = call i64 @js_string_concat_value(i64 %r4, double %r2)
+  ```
+
+  The handle global *is* a registered GC root, so the string was never swept and
+  the global was rewritten when the copying minor relocated it. The register
+  taken beforehand was not.
+
+  **Root cause: two implementations of one contract, drifted.**
+  `crates/perry-codegen/src/expr/temp_root.rs` suppresses a string literal from
+  temp rooting — correctly; it is already a root and cannot be swept — and
+  compensates by re-deriving it below the collection point. `RootedOperands`
+  (`new C(a, b)`, native collection methods) did both halves. `lower_exprs_rooted`
+  — behind `lower_operand_pair_rooted`, the array-literal element list and the
+  string-concat chain — did only the suppression. Its own comment said the
+  staleness "is not the hazard #6951 is about"; it was #7114.
+
+  **The invariant this establishes**, now stated in the module header: *no
+  operand register may outlive a collection point — after the last thing that
+  can collect, every operand is either re-read from a root the collector rewrote
+  or re-derived from immutable storage, never reused.* A root buys three things
+  and they are not the same: liveness, a rewritten location, and **the value the
+  consuming call actually observes**. The third is the one #7114 dropped.
+
+  The fix routes both helper families through one `operand_protection()`
+  decision (`Root` / `Reload` / `Reuse`) so the pair cannot drift again. This is
+  the codegen shadow-stack/temp-root mechanism, not `RuntimeHandleScope`: the
+  stale value lived in an LLVM SSA register in generated code, so no runtime
+  helper's handle scope could have seen it.
+
+  Cost is zero runtime calls — `Reload` re-emits the `load` that was going to be
+  emitted anyway, and only when a later operand can collect, so `"user_" + i`
+  and every other non-collecting concat keep their previous IR byte for byte.
+
+  Verified on an M1 at `--release` against Node 26.5.1: the new gap probe prints
+  `!119999700000` on `ff85fd483` and `acc:119999700000` after, with 60 GC cycles
+  and 2 255 476 objects relocated by the copying minor in the same run.

crates/perry-codegen/src/expr/temp_root.rs

@@ -20,6 +20,30 @@
 //! evacuating cycle rewrites it and the register pushed beforehand is stale.
 //! That is also why this is preferable to widening conservative scanning —
 //! conservative roots have to pin, precise ones can move.
+//!
+//! # The invariant (#7114)
+//!
+//! **No operand register may outlive a collection point. After the last thing
+//! that can collect, every operand is either re-read from a root the collector
+//! rewrote or re-derived from immutable storage — never reused.**
+//!
+//! A root buys three things, and they are not the same thing:
+//!
+//!  1. **liveness** — the object is marked instead of swept;
+//!  2. **a rewritten location** — a slot evacuation updates to the new address;
+//!  3. **the value the consuming call observes** — which is (2) only if the
+//!     code that resumes after the safepoint *reads that location again*.
+//!
+//! #7114 is what dropping (3) on its own looks like. A string literal is a load
+//! from a `__perry_init_strings_*` handle global that
+//! `js_gc_register_global_root` registered, so it has (1) and (2) for free —
+//! and `console.log("acc:" + run(1e7))` still printed an empty line, because
+//! the register loaded *before* `run` held the pre-move address. Exit code 0,
+//! no diagnostic, no crash.
+//!
+//! [`operand_protection`] is the single place that decides which of the three
+//! strategies an operand needs. Both helper families in this module route
+//! through it; before #7114 they answered it separately and disagreed.
 
 use perry_hir::types::Type as HirType;
 use perry_hir::Expr;
@@ -262,49 +286,56 @@ fn any_later_ref_may_trigger_gc(ctx: &FnCtx<'_>, exprs: &[&Expr], i: usize) -> b
 /// Lower `exprs` left to right, keeping each already-evaluated value precisely
 /// rooted across the evaluation of the ones that follow (#6951).
 ///
-/// Returns the lowered values — **re-read from their roots**, so they are
-/// valid after an evacuating cycle — and the guard index the caller must pass
-/// to [`temp_root_release`] once the consuming call has run. `None` means
-/// nothing needed protecting and no runtime calls were emitted.
+/// Returns the lowered values — **re-read from their roots, or re-derived from
+/// their immutable storage** ([`OperandProtection`]), so they are valid after
+/// an evacuating cycle — and the guard index the caller must pass to
+/// [`temp_root_release`] once the consuming call has run. `None` means nothing
+/// needed a temp-root slot; it does NOT mean nothing was re-read, because the
+/// [`OperandProtection::Reload`] half emits no runtime call at all.
 pub(crate) fn lower_exprs_rooted(
     ctx: &mut FnCtx<'_>,
     exprs: &[&Expr],
 ) -> anyhow::Result<(Vec<String>, Option<String>)> {
     let mut values = Vec::with_capacity(exprs.len());
     let mut slots: Vec<Option<String>> = Vec::with_capacity(exprs.len());
     let mut guard: Option<String> = None;
+    let mut reload: Vec<bool> = Vec::with_capacity(exprs.len());
     for (i, expr) in exprs.iter().enumerate() {
         let value = super::lower_expr(ctx, expr)?;
-        // A value that provably cannot be a heap reference roots nothing, so a
-        // slot for it is pure TLS traffic. This is the gate that keeps
-        // `total + s.length` and other numeric operand pairs at their old IR.
-        //
-        // A string literal is skipped for the opposite reason: it is a load
-        // from a module global that `__perry_init_strings_*` registered with
-        // `js_gc_register_global_root`, so it already has a precise root and
-        // the sweep can never take it. (A register loaded from that global is
-        // still stale after an *evacuating* cycle — but that is true of every
-        // `Expr::String` use in the compiler, not something this site
-        // introduces, and it is not the hazard #6951 is about.) Template
-        // literals are mostly literal parts, so this matters.
-        let needs_root = !super::expr_is_known_non_pointer_shadow_value(ctx, expr)
-            && !matches!(expr, Expr::String(_));
-        if needs_root && any_later_ref_may_trigger_gc(ctx, exprs, i) {
-            let idx = temp_root_push_double(ctx, &value);
-            // The FIRST slot pushed is the guard: truncating it drops every
-            // slot above it too, so one call releases the whole group.
-            if guard.is_none() {
-                guard = Some(idx.clone());
+        // `any_later_ref_may_trigger_gc` is the *window*: can anything between
+        // this operand and the consuming call collect? [`operand_protection`]
+        // turns that window into the one strategy this operand needs.
+        let collects = any_later_ref_may_trigger_gc(ctx, exprs, i);
+        match operand_protection(ctx, expr, collects) {
+            OperandProtection::Root => {
+                let idx = temp_root_push_double(ctx, &value);
+                // The FIRST slot pushed is the guard: truncating it drops every
+                // slot above it too, so one call releases the whole group.
+                if guard.is_none() {
+                    guard = Some(idx.clone());
+                }
+                slots.push(Some(idx));
+                reload.push(false);
+            }
+            OperandProtection::Reload => {
+                slots.push(None);
+                reload.push(true);
+            }
+            OperandProtection::Reuse => {
+                slots.push(None);
+                reload.push(false);
             }
-            slots.push(Some(idx));
-        } else {
-            slots.push(None);
         }
         values.push(value);
     }
-    for (value, slot) in values.iter_mut().zip(slots.iter()) {
-        if let Some(idx) = slot {
-            *value = temp_root_get_double(ctx, idx);
+    for (i, value) in values.iter_mut().enumerate() {
+        if let Some(idx) = slots[i].clone() {
+            *value = temp_root_get_double(ctx, &idx);
+        } else if reload[i] {
+            // #7114: no runtime call — just the load that was already emitted,
+            // emitted again below the collection point so it observes the
+            // address evacuation wrote back into the handle global.
+            *value = super::lower_expr(ctx, exprs[i])?;
         }
     }
     Ok((values, guard))
@@ -362,6 +393,25 @@ pub(crate) struct RootedOperands {
 ///
 /// This is the same staleness #6981 reports one layer in (a raw typed-array
 /// pointer passed under the specialized ABI).
+///
+/// # Why the sibling literal forms are deliberately absent
+///
+/// `Expr::WtfString` (a lone-surrogate literal) and `Expr::I18nString` lower to
+/// exactly the same thing as `Expr::String` — one load of a
+/// `__perry_init_strings_*` handle global, registered with
+/// `js_gc_register_global_root` by the same loop, `is_wtf8` or not
+/// (`codegen/string_pool.rs`). They would be sound here. They are not listed
+/// because [`operand_needs_root`] does not suppress them either, so they take a
+/// real temp root — and **`Root` is strictly stronger than `Reload`**: it
+/// supplies liveness, a rewritten location and the call-time value on its own,
+/// where `Reload` borrows the first from the handle global.
+///
+/// The failure mode to guard against is not the asymmetry, it is *half*-closing
+/// it: adding a literal form to [`operand_needs_root`]'s suppression list
+/// without adding it here leaves it on `Reuse`, which is #7114 for that form.
+/// `wtf8_literal_operand_is_rooted_not_merely_reused` in
+/// `tests/temp_root_operand_temporaries.rs` pins the current answer so that edit
+/// goes red instead of shipping another silent wrong answer.
 pub(crate) fn operand_is_reloadable(expr: &Expr) -> bool {
     // ONLY provably immutable sources. A string literal always re-lowers to a
     // load of the same `__perry_init_strings_*` handle, so re-reading it can
@@ -426,8 +476,8 @@ impl RootedOperands {
         value: &str,
         collects: bool,
     ) {
-        let needs_root = collects && operand_needs_root(ctx, operand);
-        if needs_root {
+        let protection = operand_protection(ctx, operand, collects);
+        if protection == OperandProtection::Root {
             let idx = temp_root_push_double(ctx, value);
             // The FIRST slot pushed is the guard: truncating it drops every
             // slot above it too, so one call releases the whole group.
@@ -439,7 +489,7 @@ impl RootedOperands {
             self.slots.push(None);
         }
         self.reloadable
-            .push(!needs_root && collects && operand_is_reloadable(operand));
+            .push(protection == OperandProtection::Reload);
         self.values.push(value.to_string());
     }
 
@@ -602,6 +652,69 @@ pub(crate) fn operand_needs_root(ctx: &FnCtx<'_>, expr: &Expr) -> bool {
     !matches!(expr, Expr::String(_))
 }
 
+/// What an already-lowered operand needs so that the consuming call observes a
+/// valid, current address across a following collection point.
+///
+/// See the module header for the three properties a root buys. Each variant is
+/// the cheapest strategy that supplies all three for its class of operand:
+#[derive(Clone, Copy, PartialEq, Eq, Debug)]
+pub(crate) enum OperandProtection {
+    /// Push a temp-root slot and re-read it. The only strategy that gives
+    /// liveness *and* a rewritten location *and* the call-time value, so it is
+    /// what every operand with no other root gets — and also what a local or a
+    /// module global gets, because those are mutable and re-deriving them would
+    /// observe a later assignment instead of the value the call was given.
+    Root,
+    /// Emit no runtime call; re-derive the operand below the collection point.
+    /// For operands whose storage is a registered root the collector rewrites
+    /// **and** which are immutable, so re-lowering provably yields the same
+    /// value at the corrected address. Only [`operand_is_reloadable`] answers
+    /// yes here, and only for a string literal.
+    Reload,
+    /// Reuse the register. Correct in exactly two cases: nothing between this
+    /// operand and its consumer can collect, or the value provably is not a
+    /// heap reference and relocation cannot invalidate it.
+    Reuse,
+}
+
+/// THE decision. Every operand-protection helper in this module routes through
+/// it, so "root, re-derive, or reuse?" is answered in exactly one place.
+///
+/// It used to be answered in two, and they disagreed. [`RootedOperands`] paired
+/// its suppression of string literals with the compensating re-load;
+/// [`lower_exprs_rooted`] suppressed them and reused the register. That is
+/// #7114: `"acc:" + run(1e7)` lowers through `lower_string_coerce_concat` →
+/// `lower_operand_pair_rooted` → `lower_exprs_rooted`, the literal's handle was
+/// loaded before the call and masked to a pointer after it, and once `run` drove
+/// an evacuating minor the concat read the string's *old* address — printing an
+/// empty line and exiting 0.
+///
+/// Keeping the two predicates but calling them from two places is what let the
+/// pair drift, so the fix is the single call site, not a second copy of the
+/// re-load.
+pub(crate) fn operand_protection(
+    ctx: &FnCtx<'_>,
+    expr: &Expr,
+    collects: bool,
+) -> OperandProtection {
+    if !collects {
+        // Nothing can be swept and nothing can move before the consumer runs,
+        // so the register still holds the value the call observes. This is the
+        // gate that keeps `total + s.length`, `f(x, y)` and `[1, 2, 3]` at
+        // exactly the IR they emitted before #6951.
+        return OperandProtection::Reuse;
+    }
+    if operand_needs_root(ctx, expr) {
+        return OperandProtection::Root;
+    }
+    if operand_is_reloadable(expr) {
+        return OperandProtection::Reload;
+    }
+    // Suppressed by `expr_is_known_non_pointer_shadow_value`: not a heap
+    // reference, so there is nothing for the collector to move.
+    OperandProtection::Reuse
+}
+
 /// Open an expression-scope temp-root barrier for a call/constructor whose
 /// operands are `args`.
 ///

crates/perry-codegen/src/lower_call/new.rs

@@ -138,11 +138,14 @@ pub(crate) fn lower_new_member_captured(
 /// - a **rooted** argument is re-read from its slot, because the slot is a
 ///   *mutable* root that an evacuating cycle rewrites in place, leaving the
 ///   register pushed beforehand stale;
-/// - an argument that was NOT rooted because it reads a registered root (a
-///   shadow-slotted local, a module global, a string literal) is **re-loaded**.
-///   Those are never swept, but evacuation rewrote their storage too, so the
-///   cached register points at where the value used to be. Re-lowering emits
-///   the load again and costs no runtime call.
+/// - an argument that was NOT rooted because it reads an *immutable* registered
+///   root — a string literal, the only `temp_root::operand_is_reloadable` case
+///   — is **re-loaded**. It is never swept, but evacuation rewrote its handle
+///   global too, so the cached register points at where the string used to be.
+///   Re-lowering emits the load again and costs no runtime call. (A
+///   shadow-slotted local or a module global is a registered root as well, but
+///   a *mutable* one, so it takes a temp-root slot instead: re-deriving it
+///   would observe an assignment made after the call-time value was taken.)
 ///
 /// Called after the instance allocation and again before the late consumers
 /// that sit behind further arbitrary lowering (field initializers, an inlined