fix(gc): operand temporaries are precise roots — collection methods, constructor args, string-method receiver (#6970, #6969, #6971)

changelog.d/6983-operand-temporaries-precise-roots.md

@@ -0,0 +1,83 @@
+### Fixed
+
+- **GC: operand temporaries in three more lowering paths are precise roots (#6969, #6970, #6971).**
+  #6951 (via #6972) rooted variadic argument accumulators, concat operand pairs
+  and literal element lists; #6975 closed the coercion hole in the gate. Three sibling paths
+  still kept an evaluated operand in a bare LLVM SSA register across a
+  collection point, which under precise-roots-only
+  (`PERRY_CONSERVATIVE_STACK_SCAN=off`) is a live use-after-free:
+
+  - **#6970 — collection-method operands.** `m.set(fresh(0), churn(N))`
+    **aborted** (exit 134, `grown Map must retain its side-allocation owner
+    record`): the key was finished and live only in a register across the
+    value's lowering, so `js_map_set` ran against a header the sweep had freed
+    and `churn` had reused. Fixed in the `Expr::MapSet` / `MapGet` / `MapHas`
+    lowering and in the `PropertyGet` dispatch that handles non-`Ident`
+    receivers (`this.field.set(…)`), including `Map`/`Set`/`URLSearchParams`
+    `forEach`, whose callback closure is itself an allocation the receiver has
+    to survive.
+  - **#6969 — constructor arguments.** `new Pair(fresh(0), churn(N))` held
+    argument 0 across argument 1's lowering *and* across the instance
+    allocation, which always collects.
+  - **#6971 — string-method receiver and the `concat` accumulator.**
+    `fresh(0).concat("|" + churn(N))` dropped its receiver. `concat` is the
+    dangerous form: its accumulator is a bare `StringHeader*`, the word form
+    only `gc::root_words`' bare case covers, and every `js_string_concat`
+    returns a *new* address — so the slot is written back with
+    `js_gc_temp_root_set`, not merely re-read.
+
+  Two mechanism additions, both built from #6972's primitives:
+  `RootedOperands` (root already-lowered operands when the *caller* knows what
+  follows collects — a per-branch operand representation for `MapSet`, an
+  allocation for `new`), and `temp_root_scope_begin`/`_end`, an
+  expression-scope barrier. The barrier is what makes `lower_new` tractable:
+  truncation is a stack *cut*, so one marker slot releases the whole group on
+  whichever of that function's ~20 return paths ran, instead of a
+  `temp_root_release` at each that future edits must keep balanced.
+
+  A new suppression, `operand_needs_root`, keeps this free where it was already
+  safe: literals, module globals, provable non-pointers, and locals that
+  **have a reserved shadow slot**. The shadow-slot check is load-bearing — a
+  blanket `LocalGet` suppression regressed #6970 straight back to an abort,
+  because a local can be pointer-valued with no shadow slot and therefore no
+  precise root at all (that is #6968).
+
+  A temp root buys **three** things, and an operand needs all of them: liveness
+  (not swept), a location the collector rewrites (survives relocation), and the
+  value the call actually observed (not a later one). A registered root — a
+  local, a module global, a string literal — supplies the first for free, which
+  tempts you to skip the slot. Skipping it is only safe when the source is also
+  *immutable*: re-loading a local or global recovers the right address after
+  evacuation but reads its value **now**, after later arguments, field
+  initializers and possibly an inlined constructor body have run, any of which
+  may have reassigned it. `new C(g, bump())` where `bump()` sets `g` then
+  captured the post-`bump()` value — a miscompile, not a rooting bug, caught in
+  review and covered by `test_gap_ctor_arg_capture_order.ts`. So only string
+  literals are re-loaded; locals and globals get a real slot, which preserves
+  the call-time value and is rewritten on evacuation.
+
+  Cost: on a probe of already-safe shapes (`"user_" + i`, `[1,2,3]`,
+  `{a:i,b:total}`, all-local argument lists, `m.set(k, 1)`, `m.get(k)`,
+  `label.slice(1,3)`, `new Pair(label, i)`) the emitted LLVM IR is
+  **byte-identical** to `main`, md5 included. The three protected shapes add 11
+  runtime calls and 14 IR lines in total. Where a real allocation does intervene
+  over a registered-root operand, the cost is one extra `load` per operand and
+  no runtime call.
+
+  Verification: each issue's reproducer is byte-exact over 4 runs under
+  `PERRY_CONSERVATIVE_STACK_SCAN=off PERRY_GC_HEAP_LIMIT=8` with the arm
+  measurably live (`PERRY_GC_TRACE=1`: 22 completed cycles), against exit-134 /
+  silent-DIFF before. Rooting a constructor's argument list *after* the lowering
+  loop rather than interleaved turned #6969's silent DIFF into a SIGSEGV — it
+  publishes an already-dangling pointer to the scanner — so the interleaving is
+  pinned by a test.
+
+### Notes
+
+- `gc::tests::temp_roots::rewriting_a_slot_roots_the_new_value_and_releases_the_replaced_one`
+  pins `ConservativeStackScanMode::Disabled`, as every test in that module must:
+  with the unit-test default (`Full`) the native-stack scan finds the raw
+  pointers in the test's own Rust locals and the test passes without proving
+  anything about precise roots.
+- Six codegen IR tests pin the emission contract and, just as importantly, the
+  *absence* of rooting on the shapes that were never broken.

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

@@ -8,6 +8,7 @@ use anyhow::Result;
 use perry_hir::types::Type as HirType;
 use perry_hir::{BinaryOp, Expr};
 
+use crate::expr::temp_root;
 use crate::type_analysis::{is_definitely_string_expr, is_numeric_expr, map_static_type_args};
 use crate::types::{DOUBLE, F32, I1, I32, I64};
 
@@ -264,6 +265,34 @@ fn guarded_map_number_key_set(
     )
 }
 
+/// Re-read the `MapSet` receiver + key after `value` has been lowered (#6970).
+///
+/// Every `Expr::MapSet` branch lowers `value` before it touches the receiver
+/// handle or the key, and that lowering is the collection point. On the
+/// protected path this hands back values read out of their temp-root slots —
+/// mandatory, since an evacuating cycle rewrites the slot in place — and
+/// derives the receiver handle from the re-read box. On the unprotected path
+/// `RootedOperands::reread` returns the original registers and
+/// `m_handle_unrooted` is the eagerly computed handle, so nothing is emitted.
+fn reread_map_set_receiver_and_key(
+    ctx: &mut FnCtx<'_>,
+    roots: &temp_root::RootedOperands,
+    operands: &[&Expr; 2],
+    m_handle_unrooted: &Option<String>,
+) -> Result<(String, String)> {
+    let values = roots.reread(ctx, operands)?;
+    let k_box = values[1].clone();
+    let m_handle = match m_handle_unrooted {
+        Some(handle) => handle.clone(),
+        None => {
+            let m_box = values[0].clone();
+            let blk = ctx.block();
+            unbox_to_i64(blk, &m_box)
+        }
+    };
+    Ok((m_handle, k_box))
+}
+
 fn guarded_map_number_key_get(ctx: &mut FnCtx<'_>, map_handle: &str, key_box: &str) -> String {
     let guard_raw = ctx
         .block()
@@ -521,15 +550,41 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
             let use_string_string_map = is_static_string_string_map(ctx, map)
                 && is_definitely_string_expr(ctx, key)
                 && is_definitely_string_expr(ctx, value);
+            // #6970: each operand is finished before the next is lowered, and
+            // both are live in nothing but SSA registers until the runtime
+            // call. `m.set(fresh(k), churn(N))` aborted inside `js_map_set` on
+            // a key whose header had been recycled.
+            //
+            // Root each one BEFORE lowering the next, not after the whole list:
+            // the receiver's exposure starts at `key`'s lowering, not `value`'s,
+            // and rooting a list that is already lowered can publish an
+            // already-dangling pointer into a scanned slot — strictly worse
+            // than not rooting at all.
+            let key_collects = temp_root::expr_may_trigger_gc(ctx, key);
+            let value_collects = temp_root::expr_may_trigger_gc(ctx, value);
+            let map_key_operands: [&Expr; 2] = [map, key];
+            let mut roots = temp_root::root_operands_begin(2);
             let m_box = lower_expr(ctx, map)?;
+            roots.push(ctx, map, &m_box, key_collects || value_collects);
             let k_box = lower_expr(ctx, key)?;
-            let m_handle = {
+            roots.push(ctx, key, &k_box, value_collects);
+            // Unbox eagerly only on the unprotected path, so its IR — including
+            // register numbering — is exactly what it was before this change.
+            // On the protected path the handle has to come from the *re-read*
+            // box, so it is derived after `value` is lowered instead.
+            let m_handle_unrooted = (!roots.is_rooted()).then(|| {
                 let blk = ctx.block();
                 unbox_to_i64(blk, &m_box)
-            };
+            });
             let new_handle = if use_string_i32_map {
                 let value_i32 =
                     lower_expr_native(ctx, value, crate::native_value::ExpectedNativeRep::I32)?;
+                let (m_handle, k_box) = reread_map_set_receiver_and_key(
+                    ctx,
+                    &roots,
+                    &map_key_operands,
+                    &m_handle_unrooted,
+                )?;
                 let (k_handle, new_handle) = {
                     let blk = ctx.block();
                     let k_handle = unbox_str_handle(blk, &k_box);
@@ -561,6 +616,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
             } else if use_string_u32_map {
                 let value_u32 =
                     lower_expr_native(ctx, value, crate::native_value::ExpectedNativeRep::U32)?;
+                let (m_handle, k_box) = reread_map_set_receiver_and_key(
+                    ctx,
+                    &roots,
+                    &map_key_operands,
+                    &m_handle_unrooted,
+                )?;
                 let (k_handle, new_handle) = {
                     let blk = ctx.block();
                     let k_handle = unbox_str_handle(blk, &k_box);
@@ -592,6 +653,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
             } else if use_string_f32_map {
                 let value_f32 =
                     lower_expr_native(ctx, value, crate::native_value::ExpectedNativeRep::F32)?;
+                let (m_handle, k_box) = reread_map_set_receiver_and_key(
+                    ctx,
+                    &roots,
+                    &map_key_operands,
+                    &m_handle_unrooted,
+                )?;
                 let (k_handle, new_handle) = {
                     let blk = ctx.block();
                     let k_handle = unbox_str_handle(blk, &k_box);
@@ -622,6 +689,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
                 new_handle
             } else if use_string_number_map {
                 let v_box = lower_expr(ctx, value)?;
+                let (m_handle, k_box) = reread_map_set_receiver_and_key(
+                    ctx,
+                    &roots,
+                    &map_key_operands,
+                    &m_handle_unrooted,
+                )?;
                 let (k_handle, new_handle) = {
                     let blk = ctx.block();
                     let k_handle = unbox_str_handle(blk, &k_box);
@@ -644,6 +717,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
             } else if use_string_boolean_map {
                 let value_i1 =
                     lower_expr_native(ctx, value, crate::native_value::ExpectedNativeRep::I1)?;
+                let (m_handle, k_box) = reread_map_set_receiver_and_key(
+                    ctx,
+                    &roots,
+                    &map_key_operands,
+                    &m_handle_unrooted,
+                )?;
                 let (k_handle, new_handle) = {
                     let blk = ctx.block();
                     let k_handle = unbox_str_handle(blk, &k_box);
@@ -675,6 +754,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
                 new_handle
             } else if use_string_string_map {
                 let v_box = lower_expr(ctx, value)?;
+                let (m_handle, k_box) = reread_map_set_receiver_and_key(
+                    ctx,
+                    &roots,
+                    &map_key_operands,
+                    &m_handle_unrooted,
+                )?;
                 let (k_handle, v_handle, new_handle) = {
                     let blk = ctx.block();
                     let k_handle = unbox_str_handle(blk, &k_box);
@@ -707,6 +792,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
                 new_handle
             } else if has_string_key_map {
                 let v_box = lower_expr(ctx, value)?;
+                let (m_handle, k_box) = reread_map_set_receiver_and_key(
+                    ctx,
+                    &roots,
+                    &map_key_operands,
+                    &m_handle_unrooted,
+                )?;
                 let (k_handle, new_handle) = {
                     let blk = ctx.block();
                     let k_handle = unbox_str_handle(blk, &k_box);
@@ -740,6 +831,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
                 new_handle
             } else if use_number_string_map {
                 let v_box = lower_expr(ctx, value)?;
+                let (m_handle, k_box) = reread_map_set_receiver_and_key(
+                    ctx,
+                    &roots,
+                    &map_key_operands,
+                    &m_handle_unrooted,
+                )?;
                 let (v_handle, v_slot_box) = {
                     let blk = ctx.block();
                     let v_handle = unbox_str_handle(blk, &v_box);
@@ -760,6 +857,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
                 guarded_map_number_key_set(ctx, &m_handle, &k_box, &v_slot_box)
             } else if use_number_key_map {
                 let v_box = lower_expr(ctx, value)?;
+                let (m_handle, k_box) = reread_map_set_receiver_and_key(
+                    ctx,
+                    &roots,
+                    &map_key_operands,
+                    &m_handle_unrooted,
+                )?;
                 if static_number_string_map {
                     record_collection_typed_value_fallback(
                         ctx,
@@ -775,6 +878,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
                 guarded_map_number_key_set(ctx, &m_handle, &k_box, &v_box)
             } else {
                 let v_box = lower_expr(ctx, value)?;
+                let (m_handle, k_box) = reread_map_set_receiver_and_key(
+                    ctx,
+                    &roots,
+                    &map_key_operands,
+                    &m_handle_unrooted,
+                )?;
                 let new_handle = {
                     let blk = ctx.block();
                     blk.call(
@@ -794,6 +903,9 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
                 );
                 new_handle
             };
+            // Released only now: the runtime call above allocates while it
+            // reads the key, so the group has to stay rooted across it.
+            roots.release(ctx);
             // map.set returns the (possibly-realloc'd) map. Re-NaN-box
             // and return. The caller may need to write this back to a
             // local; that's the caller's problem if Map is held in a
@@ -807,13 +919,14 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
             let use_number_key_map = !use_string_key_map
                 && is_static_number_key_map(ctx, map)
                 && is_numeric_expr(ctx, key);
-            let m_box = lower_expr(ctx, map)?;
-            let k_box = lower_expr(ctx, key)?;
+            // #6970: `key` is lowered after the receiver and can collect, so the
+            // receiver would otherwise sit unrooted in an SSA register across it.
+            let (m_box, k_box, guard) = temp_root::lower_operand_pair_rooted(ctx, map, key)?;
             let m_handle = {
                 let blk = ctx.block();
                 unbox_to_i64(blk, &m_box)
             };
-            if use_string_key_map {
+            let value = if use_string_key_map {
                 let (k_handle, value) = {
                     let blk = ctx.block();
                     let k_handle = unbox_str_handle(blk, &k_box);
@@ -832,9 +945,9 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
                     "map",
                     "js_map_get_string_key",
                 );
-                Ok(value)
+                value
             } else if use_number_key_map {
-                Ok(guarded_map_number_key_get(ctx, &m_handle, &k_box))
+                guarded_map_number_key_get(ctx, &m_handle, &k_box)
             } else {
                 let value = {
                     let blk = ctx.block();
@@ -849,17 +962,20 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
                     "js_map_get",
                     "receiver_or_key_not_static_string",
                 );
-                Ok(value)
-            }
+                value
+            };
+            temp_root::temp_root_release(ctx, guard);
+            Ok(value)
         }
         Expr::MapHas { map, key } => {
             let use_string_key_map =
                 is_static_string_key_map(ctx, map) && is_definitely_string_expr(ctx, key);
             let use_number_key_map = !use_string_key_map
                 && is_static_number_key_map(ctx, map)
                 && is_numeric_expr(ctx, key);
-            let m_box = lower_expr(ctx, map)?;
-            let k_box = lower_expr(ctx, key)?;
+            // #6970: same hazard as `MapGet` — the key's lowering can collect
+            // while the receiver is live only in an SSA register.
+            let (m_box, k_box, guard) = temp_root::lower_operand_pair_rooted(ctx, map, key)?;
             let m_handle = {
                 let blk = ctx.block();
                 unbox_to_i64(blk, &m_box)
@@ -902,6 +1018,7 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
                 );
                 i32_v
             };
+            temp_root::temp_root_release(ctx, guard);
             // NaN-tagged boolean for "true"/"false" printing.
             let blk = ctx.block();
             let bit = blk.icmp_ne(I32, &i32_v, "0");