fix(gc): coercion-capable operators can collect — four rooting gaps from the #6972 review, plus restore the dark gc-stress gate

changelog.d/6975-temp-root-coercion-gate.md

@@ -0,0 +1,43 @@
+**fix(gc): coercion-capable operators can collect — four rooting gaps from the #6972 review**
+
+Follow-up to #6972 (#6951). One soundness hole in the gate predicate plus three
+additional sites of the same bug class, all found by review after #6972 merged.
+
+**The gate was unsound.** `expr/temp_root.rs` documents that
+`expr_may_trigger_gc` is deliberately one-sided — `false` must mean "provably
+allocates nothing" — and then answered `false` for `Compare` / `Unary` /
+non-`Add` `Binary` whenever recursing into the operands found no allocation.
+But `o < x`, `-o` and `o * 2` run ToPrimitive / ToNumber on their operands, and
+a user-defined `Symbol.toPrimitive` / `valueOf` / `toString` is arbitrary JS: it
+allocates and it collects. `a < b` over two plain `LocalGet`s recursed straight
+to `false`, so `f(freshString(), a < b)` skipped rooting its first argument —
+the #6951 use-after-free in a narrower case. These operators are now GC-capable
+unless **every** operand is a proven inert primitive (`expr_is_inert_primitive`:
+literals, plus locals the type analysis proved Number / Int32 / Boolean / Null /
+Void / Never with no reserved shadow slot — a reserved slot means
+pointer-possible whatever the refined type says). `Add` is never inert, since
+concatenation allocates even over two literals. The predicate now takes
+`&FnCtx`; `any_later_arg_may_trigger_gc` had no callers and is deleted rather
+than shipped dead. Cost is unchanged: `i < n` and `x * 2` on proven-numeric
+locals stay inert, and the hot-loop benchmark from #6972 still emits 12 rooting
+calls.
+
+**Three more sites.** (1) `expr/binary.rs`'s BigInt dynamic helper had a second
+copy of the two-`lower_expr` shape in its `!inline_bitwise` branch that #6972's
+pass missed. (2) `lower_canonical_str_self_append`: `s += rhs` must load `s`
+*before* evaluating `rhs` (a `rhs` that reassigns `s` must not be observed), so
+the pre-rhs value crosses both `rhs` and `js_jsvalue_to_string` — re-reading the
+slot would take the wrong value, so it goes into a temp root; the coerced rhs
+handle is rooted too, because the cold arm's `unbox_str_handle` materializes an
+SSO destination onto the heap with that bare handle live. (3)
+`lower_object_literal`'s `this_patches` queue holds method-closure values across
+every remaining property's initializer and then passes them to
+`js_closure_set_capture_bits` as raw pointers; they are now rooted and refreshed
+before the patch loop.
+
+Re-verified against pinned Node 26.5.0: the #6951 repro stays fixed under
+`PERRY_CONSERVATIVE_STACK_SCAN=off`; the 431-file gap corpus is byte-identical
+to the `origin/main` baseline; `scripts/gc_repsel_matrix.sh --arms all` is
+361/361 byte-exact with FAIL=0 and XFAIL=0 and both `cons_scan_off` cells still
+PASS; and a throw through a protected argument list is byte-exact under both
+arms.

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

@@ -541,12 +541,16 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
                         && crate::type_analysis::is_provably_not_bigint(ctx, left)
                         && crate::type_analysis::is_provably_not_bigint(ctx, right);
                     if !inline_bitwise {
+                        // #6951: the dynamic helper runs ToNumeric on both
+                        // operands, so a pointer-bearing left operand must
+                        // survive the right operand's evaluation.
                         let fname = bigint_dynamic_helper(*op);
-                        let l = lower_expr(ctx, left)?;
-                        let r = lower_expr(ctx, right)?;
-                        return Ok(ctx
+                        let (l, r, guard) = lower_operand_pair_rooted(ctx, left, right)?;
+                        let value = ctx
                             .block()
-                            .call(DOUBLE, fname, &[(DOUBLE, &l), (DOUBLE, &r)]));
+                            .call(DOUBLE, fname, &[(DOUBLE, &l), (DOUBLE, &r)]);
+                        temp_root_release(ctx, guard);
+                        return Ok(value);
                     }
                 }
             }

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

@@ -7,6 +7,7 @@ use perry_hir::Expr;
 
 use super::temp_root::{
     any_may_trigger_gc, rooted_handle_begin, rooted_handle_get, rooted_handle_release,
+    temp_root_get_double, temp_root_push_double,
 };
 use super::{lower_expr, nanbox_pointer_inline, FnCtx};
 use crate::nanbox::POINTER_MASK_I64;
@@ -316,7 +317,7 @@ pub(crate) fn lower_object_literal(
     // therefore had its half-built object swept by `f`'s collection, and the
     // remaining field stores landed in recycled memory. Root the handle when any
     // initializer can collect; literals of plain locals emit no extra IR.
-    let protect_handle = any_may_trigger_gc(props.iter().map(|(_, v)| v));
+    let protect_handle = any_may_trigger_gc(ctx, props.iter().map(|(_, v)| v));
     let field_count = props.len() as u32;
     let zero_str = "0".to_string();
     let n_str = field_count.to_string();
@@ -469,10 +470,15 @@ pub(crate) fn lower_object_literal(
         .call(I64, "js_object_alloc", &[(I32, &zero_str), (I32, &n_str)]);
     let rooted = rooted_handle_begin(ctx, &obj_handle, protect_handle);
 
-    // Track `(closure_value_double, reserved_this_slot_idx)` for each
-    // method closure that needs `this` patched after the object is
+    // Track `(temp_root_slot, closure_value_double, reserved_this_slot_idx)`
+    // for each method closure that needs `this` patched after the object is
     // fully built. Enables `calc.add(n) { this.value = ... }`.
-    let mut this_patches: Vec<(String, u32)> = Vec::new();
+    //
+    // #6951: the closure value is *deferred* — it is reused after every
+    // remaining property has been lowered, so it sits in an SSA register
+    // across all of their allocations. Root it whenever any initializer can
+    // collect, and re-read it before the patch loop.
+    let mut this_patches: Vec<(Option<String>, String, u32)> = Vec::new();
 
     for (key, value_expr) in props {
         let key_idx = ctx.strings.intern(key);
@@ -490,7 +496,8 @@ pub(crate) fn lower_object_literal(
             let this_idx = auto_caps.len() as u32;
 
             let v = lower_expr(ctx, value_expr)?;
-            this_patches.push((v.clone(), this_idx));
+            let closure_root = protect_handle.then(|| temp_root_push_double(ctx, &v));
+            this_patches.push((closure_root, v.clone(), this_idx));
 
             let obj_handle = rooted_handle_get(ctx, &rooted);
             let blk = ctx.block();
@@ -519,6 +526,16 @@ pub(crate) fn lower_object_literal(
     // Patch each method closure's reserved `this` slot with the object
     // pointer (NaN-boxed). Done AFTER all fields are set so every
     // method sees the fully-initialized object.
+    // Refresh every deferred closure value from its root BEFORE taking the
+    // block builder — an evacuating cycle during a later property's
+    // initializer rewrote the slot, and the register queued above is stale.
+    let this_patches: Vec<(String, u32)> = this_patches
+        .into_iter()
+        .map(|(root, value, this_idx)| match root {
+            Some(idx) => (temp_root_get_double(ctx, &idx), this_idx),
+            None => (value, this_idx),
+        })
+        .collect();
     let obj_handle = rooted_handle_get(ctx, &rooted);
     if !this_patches.is_empty() {
         let blk = ctx.block();

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

@@ -21,6 +21,7 @@
 //! That is also why this is preferable to widening conservative scanning —
 //! conservative roots have to pin, precise ones can move.
 
+use perry_hir::types::Type as HirType;
 use perry_hir::Expr;
 
 use crate::types::{DOUBLE, I32, I64};
@@ -96,10 +97,12 @@ pub(crate) fn rooted_array_read(ctx: &mut FnCtx<'_>, idx: &str) -> String {
 /// Deliberately one-sided: `false` must mean "provably allocates nothing", and
 /// everything unrecognized answers `true`. A wrong `false` is a
 /// use-after-free; a wrong `true` costs two runtime calls on a cold path.
-pub(crate) fn expr_may_trigger_gc(expr: &Expr) -> bool {
+pub(crate) fn expr_may_trigger_gc(ctx: &FnCtx<'_>, expr: &Expr) -> bool {
     match expr {
         // Immediates and plain slot reads. `LocalGet` reads an alloca,
-        // `GlobalGet` a module global — neither allocates.
+        // `GlobalGet` a module global — neither allocates. (Reading an
+        // object-typed local is still just a load; it is the *operators* below
+        // that can coerce it and run user code.)
         Expr::Undefined
         | Expr::Null
         | Expr::Bool(_)
@@ -111,45 +114,87 @@ pub(crate) fn expr_may_trigger_gc(expr: &Expr) -> bool {
         // `__perry_init_strings_*` and registered as a GC root there; the use
         // site is a load.
         Expr::String(_) => false,
-        Expr::Unary { operand, .. } => expr_may_trigger_gc(operand),
-        Expr::Compare { left, right, .. } => {
-            expr_may_trigger_gc(left) || expr_may_trigger_gc(right)
-        }
-        // `+` on unknown operands can be string concatenation, which allocates;
-        // every other binary operator is numeric or bitwise.
-        Expr::Binary {
-            op, left, right, ..
-        } => {
-            matches!(op, perry_hir::BinaryOp::Add)
-                || expr_may_trigger_gc(left)
-                || expr_may_trigger_gc(right)
+        // Coercing operators. `-o`, `o < x`, `o == x`, `o * 2` all run
+        // ToPrimitive / ToNumber on their operands, and a user-defined
+        // `Symbol.toPrimitive` / `valueOf` / `toString` is arbitrary JS: it
+        // allocates, and it collects. Recursing into the operands is NOT
+        // enough — `a < b` over two plain `LocalGet`s recurses to `false`
+        // while the comparison itself can call into user code. So these are
+        // GC-capable unless every operand is a proven inert primitive.
+        Expr::Unary { .. } | Expr::Compare { .. } | Expr::Binary { .. } => {
+            !expr_is_inert_primitive(ctx, expr)
         }
         Expr::Conditional {
             condition,
             then_expr,
             else_expr,
         } => {
-            expr_may_trigger_gc(condition)
-                || expr_may_trigger_gc(then_expr)
-                || expr_may_trigger_gc(else_expr)
+            expr_may_trigger_gc(ctx, condition)
+                || expr_may_trigger_gc(ctx, then_expr)
+                || expr_may_trigger_gc(ctx, else_expr)
         }
-        Expr::Sequence(exprs) => exprs.iter().any(expr_may_trigger_gc),
+        Expr::Sequence(exprs) => exprs.iter().any(|e| expr_may_trigger_gc(ctx, e)),
         _ => true,
     }
 }
 
-/// Does any expression after index `i` reach a collection point?
+/// Is `expr` a value whose evaluation *and coercion* provably cannot run user
+/// code or allocate?
 ///
-/// This is the gate for protecting argument `i`: a value that nothing
-/// allocating follows cannot be collected before it is consumed, so the
-/// rooting calls would be pure overhead. `"a" + i`, `f(x, y)` on plain locals
-/// and `[1, 2, 3]` therefore emit exactly the IR they emitted before #6951.
-pub(crate) fn any_later_arg_may_trigger_gc(args: &[Expr], i: usize) -> bool {
-    args.iter().skip(i + 1).any(expr_may_trigger_gc)
+/// This is the inner half of [`expr_may_trigger_gc`]'s one-sidedness: only
+/// literals and locals the type analysis proved to be numbers / booleans /
+/// null / undefined qualify, plus operator trees built entirely out of those.
+/// A local carrying an object — or one with a reserved shadow slot, which
+/// means it is pointer-possible regardless of its refined type — is not inert,
+/// because `ToPrimitive` on it dispatches to whatever the object defines.
+fn expr_is_inert_primitive(ctx: &FnCtx<'_>, expr: &Expr) -> bool {
+    match expr {
+        Expr::Undefined | Expr::Null | Expr::Bool(_) | Expr::Number(_) | Expr::Integer(_) => true,
+        // A heap value, but ToPrimitive on a string is the identity: no user
+        // code, no allocation. (`+` is excluded below, since concatenation
+        // does allocate.)
+        Expr::String(_) => true,
+        Expr::LocalGet(id) => {
+            !ctx.shadow_slot_map.contains_key(id)
+                && matches!(
+                    ctx.local_types.get(id),
+                    Some(
+                        HirType::Number
+                            | HirType::Int32
+                            | HirType::Boolean
+                            | HirType::Null
+                            | HirType::Void
+                            | HirType::Never
+                    )
+                )
+        }
+        Expr::Unary { operand, .. } => expr_is_inert_primitive(ctx, operand),
+        Expr::Compare { left, right, .. } => {
+            expr_is_inert_primitive(ctx, left) && expr_is_inert_primitive(ctx, right)
+        }
+        // `+` allocates whenever it is a concatenation, so it is never inert
+        // even over two string literals.
+        Expr::Binary { op, left, right } => {
+            !matches!(op, perry_hir::BinaryOp::Add)
+                && expr_is_inert_primitive(ctx, left)
+                && expr_is_inert_primitive(ctx, right)
+        }
+        _ => false,
+    }
 }
 
-fn any_later_ref_may_trigger_gc(exprs: &[&Expr], i: usize) -> bool {
-    exprs.iter().skip(i + 1).any(|e| expr_may_trigger_gc(e))
+/// Does any expression after index `i` reach a collection point?
+///
+/// This is the gate for protecting value `i`: a value that nothing allocating
+/// follows cannot be collected before it is consumed, so the rooting calls
+/// would be pure overhead. `i < n`, `x * 2` on proven-numeric locals,
+/// `f(x, y)` on plain locals and `[1, 2, 3]` therefore emit exactly the IR
+/// they emitted before #6951.
+fn any_later_ref_may_trigger_gc(ctx: &FnCtx<'_>, exprs: &[&Expr], i: usize) -> bool {
+    exprs
+        .iter()
+        .skip(i + 1)
+        .any(|e| expr_may_trigger_gc(ctx, e))
 }
 
 /// Lower `exprs` left to right, keeping each already-evaluated value precisely
@@ -182,7 +227,7 @@ pub(crate) fn lower_exprs_rooted(
         // 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(exprs, i) {
+        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.
@@ -269,6 +314,9 @@ pub(crate) fn rooted_handle_release(ctx: &mut FnCtx<'_>, handle: RootedHandle) {
 }
 
 /// Do any of an object literal's / call's initializer expressions collect?
-pub(crate) fn any_may_trigger_gc<'a>(exprs: impl IntoIterator<Item = &'a Expr>) -> bool {
-    exprs.into_iter().any(expr_may_trigger_gc)
+pub(crate) fn any_may_trigger_gc<'a>(
+    ctx: &FnCtx<'_>,
+    exprs: impl IntoIterator<Item = &'a Expr>,
+) -> bool {
+    exprs.into_iter().any(|e| expr_may_trigger_gc(ctx, e))
 }

crates/perry-codegen/src/lower_string_method.rs

@@ -8,8 +8,8 @@ use perry_hir::types::Type as HirType;
 use perry_hir::Expr;
 
 use crate::expr::temp_root::{
-    lower_exprs_rooted, lower_operand_pair_rooted, temp_root_get_i64, temp_root_push_i64,
-    temp_root_release, temp_root_truncate,
+    lower_exprs_rooted, lower_operand_pair_rooted, temp_root_get_double, temp_root_get_i64,
+    temp_root_push_double, temp_root_push_i64, temp_root_release, temp_root_truncate,
 };
 use crate::expr::{
     i32_bool_to_nanbox, lower_expr, nanbox_pointer_inline, nanbox_string_inline, unbox_str_handle,
@@ -1421,10 +1421,21 @@ fn lower_canonical_str_self_append(
         // (lhs slot load, then rhs), coerce the rhs once (heap handle
         // guaranteed), then 2-arm on the destination tag only.
         let lhs_box = ctx.block().load(DOUBLE, slot);
+        // #6951: the load must happen before `rhs` per `s += rhs` evaluation
+        // order (a `rhs` that reassigns `s` must not be observed here), so the
+        // pre-rhs value has to be carried across `rhs`'s evaluation and the
+        // `js_jsvalue_to_string` coercion — both of which allocate. Re-reading
+        // the slot would take the wrong value; re-read the temp root instead.
+        let lhs_root = temp_root_push_double(ctx, &lhs_box);
         let rhs_val = lower_expr(ctx, rhs)?;
         let r_handle = ctx
             .block()
             .call(I64, "js_jsvalue_to_string", &[(DOUBLE, &rhs_val)]);
+        // The coerced rhs is a bare string handle that has to survive the cold
+        // arm's `unbox_str_handle`, which materializes an SSO destination onto
+        // the heap — another allocation. Root it too and re-read it per arm.
+        let r_root = temp_root_push_i64(ctx, &r_handle);
+        let lhs_box = temp_root_get_double(ctx, &lhs_root);
         let bits_d = ctx.block().bitcast_double_to_i64(&lhs_box);
         let tag_d = ctx.block().lshr(I64, &bits_d, "48");
         let is_heap = ctx.block().icmp_eq(I64, &tag_d, TAG_HEAP_STR);
@@ -1439,17 +1450,19 @@ fn lower_canonical_str_self_append(
 
         ctx.current_block = heap_idx;
         let h_d = ctx.block().and(I64, &bits_d, POINTER_MASK_I64);
+        let r_heap = temp_root_get_i64(ctx, &r_root);
         let h_heap = ctx
             .block()
-            .call(I64, "js_string_append", &[(I64, &h_d), (I64, &r_handle)]);
+            .call(I64, "js_string_append", &[(I64, &h_d), (I64, &r_heap)]);
         let heap_pred = ctx.block().label.clone();
         ctx.block().br(&merge_label);
 
         ctx.current_block = cold_idx;
         let h_d2 = unbox_str_handle(ctx.block(), &lhs_box);
+        let r_cold = temp_root_get_i64(ctx, &r_root);
         let h_cold = ctx
             .block()
-            .call(I64, "js_string_append", &[(I64, &h_d2), (I64, &r_handle)]);
+            .call(I64, "js_string_append", &[(I64, &h_d2), (I64, &r_cold)]);
         let cold_pred = ctx.block().label.clone();
         ctx.block().br(&merge_label);
 
@@ -1459,6 +1472,9 @@ fn lower_canonical_str_self_append(
             .phi(I64, &[(&h_heap, &heap_pred), (&h_cold, &cold_pred)]);
         let new_box = nanbox_string_inline(ctx.block(), &handle);
         ctx.block().store(DOUBLE, &new_box, slot);
+        // `lhs_root` is the base of the pair, so one truncate drops both. The
+        // index register is defined in the entry block and dominates the merge.
+        temp_root_truncate(ctx, &lhs_root);
         return Ok(new_box);
     }
 

test-parity/gc_repsel_corpus.txt

@@ -41,6 +41,13 @@ test_gap_repsel_p4a3_ptr_numarray
 # --- Phase 4b: class-field store note/addref elision (#6919) ----------------
 test_gap_repsel_p4b_field_store_elision
 
+# --- Phase 5a: Ptr<Shape> proven `this` in methods (#6925) ------------------
+# Registered here after the fact: #6925 added the file without registering it,
+# which is exactly the omission this manifest exists to catch — the matrix
+# script exits 3 on an unregistered `test_gap_repsel_*` file, so `gc-stress`
+# was failing on main for every PR until this line landed.
+test_gap_repsel_proven_this_frozen
+
 # --- The GC-live member ------------------------------------------------------
 # Every file above performs ZERO collections (measured, #6950), which makes the
 # GC arms inert against them. This one holds each representation's local live