fix(codegen): don't root typed-array element reads as GC temporaries (#6996)

changelog.d/6997-temp-root-typed-element-reads.md

@@ -0,0 +1,55 @@
+**fix(codegen): typed-array element reads are not GC temporaries (#6996)**
+
+`compiler-output-regression` — a required check — had been red on `main` since
+`760db2fd8` (#6972, "argument temporaries are precise roots"): the
+`native-abi-proof` workload `native_abi_packet_control` failed
+`hot_loops_no_runtime_calls` on one call, `js_gc_temp_root_push`, inside its
+hot loop.
+
+**Bisected, not guessed.** Over the 28-commit window `95960e0df..760db2fd8`,
+with the compiler rebuilt from source at each endpoint: `95960e0df` pass,
+`1a533a3a8` (#6972's parent) pass, `760db2fd8` fail, current `main` fail. The
+after-opt IR delta between the last two is exactly that one call.
+
+**Root cause.** The fixture's kernel is `(buf[i] + packet.tag + i) & 255`.
+`packet` is `any`, so the add lowers through `js_dynamic_string_or_number_add`
+and #6972 roots the operand pair across the property get — correct in general,
+because a heap operand held only in an SSA register while its sibling allocates
+is precisely the #6951 use-after-free. But the left operand is a *byte*. #6972
+anticipated this and gated emission on
+`expr_is_known_non_pointer_shadow_value`; that predicate had no arm for
+typed-array / Buffer element reads, so the workload's hottest loop
+(4096 × 64 iterations) paid a push + re-read + truncate per iteration to root a
+value that can never be collected.
+
+**Fix.** The contract is unchanged — nothing was relaxed. The predicate learns
+the element-read family, and the proof is about the LOWERING, not the declared
+type (annotations are unenforced, so `buf: Buffer` holding something else must
+not be load-bearing — and it isn't: `js_uint8array_index_get_value` and
+`js_buffer_index_get_value` answer `undefined` for a receiver that is not a
+Uint8Array/Buffer, and `lower_buffer_load`'s inline arm reads a raw byte). The
+three lowerings of `Uint8ArrayGet` that CAN yield a heap value stay rooted and
+have tests holding them from the other side: a symbol key
+(`js_object_get_symbol_property` returns a prototype accessor), and — in
+JS-value and in i32 context respectively — an unproven key
+(`js_typed_array_index_get_dynamic`) or a key that is not numeric-proven
+(`js_object_get_index_polymorphic`), both of which fall through to string-keyed
+property lookup. Those last two are gated on different predicates in the
+lowering, so the skip tests both. `BufferIndexGet` has none of these paths.
+
+Since that argument depends on the gate testing the *same* index proof that
+routes the read to a byte accessor, the verbatim copy of
+`numeric_index_has_integer_array_index_proof` in `expr::arrays_finds` is folded
+into the `expr::index_get` one — identical arms and thresholds, but two copies
+that could drift would make the gate unsound the moment one was edited.
+
+**Cost to #6972: none of its protection.** In this fixture the emitted push
+count goes 5 → 4; the `console.log` accumulator pair and the two
+`js_dynamic_string_or_number_add` results in the return expression (which
+really can be strings) all stay rooted. The hot loop returns to its pre-#6972
+IR.
+
+Verified with the full harness (compile + link + run): `native-abi-proof` and
+`native-region-proof` both `"status": "pass"`, both harness unittest modules
+OK, four new cases in `crates/perry-codegen/tests/temp_root_argument_temporaries.rs`
+(the two "not rooted" cases fail without the change).

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

@@ -6,7 +6,7 @@
 
 use anyhow::{anyhow, bail, Result};
 use perry_hir::types::Type as HirType;
-use perry_hir::{BinaryOp, Expr};
+use perry_hir::Expr;
 
 use crate::nanbox::double_literal;
 use crate::native_value::{
@@ -18,11 +18,19 @@ use crate::types::{DOUBLE, I32, I64, PTR};
 
 use super::{
     buffer_access_materialization_reason, can_lower_expr_as_i32, i32_bool_to_nanbox,
-    int_range_expr, lower_buffer_load, lower_buffer_store, lower_expr, lower_expr_as_i32,
-    materialize_js_value, nanbox_pointer_inline, nanbox_string_inline, unbox_to_i64,
-    BufferAccessSpec, FnCtx,
+    lower_buffer_load, lower_buffer_store, lower_expr, lower_expr_as_i32, materialize_js_value,
+    nanbox_pointer_inline, nanbox_string_inline, unbox_to_i64, BufferAccessSpec, FnCtx,
 };
 
+// #6996: ONE integer-array-index proof, shared with `expr::index_get`. This
+// was a verbatim copy of that function -- same arms, same thresholds, its
+// `BitAnd` arm being the inlined body of `bitand_has_nonnegative_i32_mask`.
+// The copy had to go: `expr::shadow_slot`'s temp-root gate proves a
+// `Uint8ArrayGet` is non-pointer by asserting that THIS proof routes the read
+// to a byte accessor, so two copies that could drift apart would make the gate
+// unsound the moment one of them was edited.
+use super::index_get::numeric_index_has_integer_array_index_proof;
+
 fn lower_index_i32(ctx: &mut FnCtx<'_>, index: &Expr) -> Result<String> {
     if can_lower_expr_as_i32(
         index,
@@ -42,41 +50,6 @@ fn lower_index_i32(ctx: &mut FnCtx<'_>, index: &Expr) -> Result<String> {
     }
 }
 
-fn numeric_index_has_integer_array_index_proof(ctx: &FnCtx<'_>, index: &Expr) -> bool {
-    fn range_is_nonnegative_i32(ctx: &FnCtx<'_>, index: &Expr) -> bool {
-        int_range_expr(ctx, index)
-            .is_some_and(|range| range.min >= 0 && range.max <= i32::MAX as i64)
-    }
-
-    match index {
-        Expr::Integer(i) => (0..=i32::MAX as i64).contains(i),
-        Expr::Number(n) => n.is_finite() && n.fract() == 0.0 && *n >= 0.0 && *n <= i32::MAX as f64,
-        Expr::Binary { op, left, right } if matches!(op, BinaryOp::BitAnd) => {
-            fn mask(expr: &Expr) -> Option<i64> {
-                match expr {
-                    Expr::Integer(i) => Some(*i),
-                    Expr::Number(n) if n.is_finite() && n.fract() == 0.0 => Some(*n as i64),
-                    _ => None,
-                }
-            }
-            mask(left)
-                .or_else(|| mask(right))
-                .is_some_and(|mask| (0..=i32::MAX as i64).contains(&mask))
-        }
-        Expr::LocalGet(id) => {
-            ctx.integer_locals.contains(id)
-                && ctx.i32_counter_slots.contains_key(id)
-                && (ctx.nonnegative_integer_locals.contains(id)
-                    || ctx
-                        .int_range_facts
-                        .iter()
-                        .any(|fact| fact.local_id == *id && fact.range.min >= 0))
-                || range_is_nonnegative_i32(ctx, index)
-        }
-        _ => range_is_nonnegative_i32(ctx, index),
-    }
-}
-
 pub(crate) fn lower_uint8array_get_i32(
     ctx: &mut FnCtx<'_>,
     array: &Expr,

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

@@ -41,6 +41,42 @@ pub(crate) fn expr_is_known_non_pointer_shadow_value(ctx: &FnCtx<'_>, expr: &Exp
             Expr::LocalGet(arr_id)
                 if super::masked_window_fact_for_index(ctx, *arr_id, index).is_some()
         ),
+        // #6996: a typed-array / Buffer element read is a number (or
+        // `undefined` out of range) BY CONSTRUCTION -- `lower_buffer_load`'s
+        // inline byte load, `js_uint8array_index_get_value` and
+        // `js_buffer_index_get_value` can only ever produce one. It is never a
+        // heap reference, so #6951's argument-temporary rooting has nothing to
+        // protect and its push/re-read/truncate trio is pure TLS traffic in
+        // exactly the loops that can least afford it (`buf[i] + packet.tag`
+        // rooted the byte across the property get, once per iteration).
+        //
+        // The proof is about the LOWERING, not the declared type: annotations
+        // are unenforced, so `buf: Buffer` holding something else must not be
+        // load-bearing -- and it isn't, because both runtime accessors answer
+        // `undefined` for a receiver that is not a Uint8Array/Buffer.
+        //
+        // The three lowerings of this node that CAN yield a heap value are
+        // excluded by construction, one condition each:
+        //   * a symbol key (`u8[Symbol.iterator]`) goes to
+        //     `js_object_get_symbol_property`, which returns the accessor;
+        //   * in JS-value context, a key without the integer-array-index proof
+        //     goes to `js_typed_array_index_get_dynamic`, which falls through
+        //     to string-keyed property lookup (an expando holds anything);
+        //   * in i32 context (`lower_uint8array_get_i32`), a key that is not
+        //     numeric-proven goes to `js_object_get_index_polymorphic`, which
+        //     dispatches a string key to that same property path. That arm is
+        //     gated on `is_numeric_expr`, NOT on the index proof, so testing
+        //     the proof alone would not cover it -- both conditions are
+        //     required. `is_numeric_expr` is used here only to NARROW: a wrong
+        //     `true` from it still leaves a byte read on every arm it admits.
+        // `BufferIndexGet` has none of these paths -- every arm coerces the key
+        // to i32 and reads a byte -- so it needs no condition.
+        Expr::Uint8ArrayGet { index, .. } => {
+            !matches!(index.as_ref(), Expr::SymbolFor(_))
+                && crate::type_analysis::is_numeric_expr(ctx, index)
+                && super::index_get::numeric_index_has_integer_array_index_proof(ctx, index)
+        }
+        Expr::BufferIndexGet { .. } => true,
         Expr::Conditional {
             then_expr,
             else_expr,

crates/perry-codegen/tests/temp_root_argument_temporaries.rs

@@ -225,3 +225,181 @@ fn array_literal_roots_elements_before_an_allocating_element() {
          heap elements already evaluated (#6951):\n{ir}"
     );
 }
+
+// ---------------------------------------------------------------------------
+// #6996: the gate must not root values that cannot be heap references.
+//
+// `native_abi_packet_control`'s kernel is `(buf[i] + packet.tag + i) & 255`.
+// The left operand is a byte; the right is a property get on an `any`, so the
+// add lowers through `js_dynamic_string_or_number_add` and the operand pair is
+// rooted across the property get. Rooting the byte protected nothing and cost
+// a push / re-read / truncate on every one of the loop's 262 144 iterations —
+// it turned the `hot_loops_no_runtime_calls` contract red.
+
+/// A local declared with a Buffer annotation, so the receiver of the element
+/// read is a plain local get (the shape the fixture's parameter has).
+fn buffer_local(id: u32) -> Stmt {
+    Stmt::Let {
+        id,
+        name: "buf".to_string(),
+        ty: perry_hir::types::Type::Named("Buffer".to_string()),
+        mutable: false,
+        init: Some(Expr::Undefined),
+    }
+}
+
+/// An `any` local, so a property get on it is neither numeric-proven nor
+/// allocation-free — exactly what forces the dynamic add and its rooting.
+fn any_local(id: u32) -> Stmt {
+    Stmt::Let {
+        id,
+        name: "packet".to_string(),
+        ty: perry_hir::types::Type::Any,
+        mutable: false,
+        init: Some(Expr::Undefined),
+    }
+}
+
+fn packet_tag(id: u32) -> Expr {
+    Expr::PropertyGet {
+        object: Box::new(Expr::LocalGet(id)),
+        property: "tag".to_string(),
+        byte_offset: 0,
+    }
+}
+
+/// `<element read> + packet.tag`, bound to a local so nothing folds it away.
+fn dynamic_add_with_element_read(element: Expr, packet_id: u32) -> Stmt {
+    Stmt::Let {
+        id: 90,
+        name: "next".to_string(),
+        ty: perry_hir::types::Type::Number,
+        mutable: false,
+        init: Some(Expr::Binary {
+            op: perry_hir::BinaryOp::Add,
+            left: Box::new(element),
+            right: Box::new(packet_tag(packet_id)),
+        }),
+    }
+}
+
+/// A proven-index typed-array element read is a byte (or `undefined` out of
+/// range) by construction, so it must not be rooted.
+#[test]
+fn buffer_element_operand_is_not_temp_rooted() {
+    let ir = ir_for(
+        "typed_element_operand.ts",
+        vec![
+            buffer_local(0),
+            any_local(1),
+            dynamic_add_with_element_read(
+                Expr::Uint8ArrayGet {
+                    array: Box::new(Expr::LocalGet(0)),
+                    index: Box::new(Expr::Integer(0)),
+                },
+                1,
+            ),
+        ],
+    );
+
+    assert!(
+        ir.contains("call double @js_dynamic_string_or_number_add"),
+        "the test must actually reach the rooted operand-pair lowering:\n{ir}"
+    );
+    assert!(
+        !ir.contains("call i32 @js_gc_temp_root_push"),
+        "a proven-index typed-array element read is number-or-undefined by \
+         construction — rooting it protects nothing and costs three runtime \
+         calls per iteration (#6996):\n{ir}"
+    );
+}
+
+/// Same for a `Buffer`-node element read, whose every lowering coerces the key
+/// to i32 and reads a byte.
+#[test]
+fn buffer_index_get_operand_is_not_temp_rooted() {
+    let ir = ir_for(
+        "buffer_index_operand.ts",
+        vec![
+            buffer_local(0),
+            any_local(1),
+            dynamic_add_with_element_read(
+                Expr::BufferIndexGet {
+                    buffer: Box::new(Expr::LocalGet(0)),
+                    index: Box::new(Expr::Integer(0)),
+                },
+                1,
+            ),
+        ],
+    );
+
+    assert!(
+        !ir.contains("call i32 @js_gc_temp_root_push"),
+        "a Buffer element read is a byte or `undefined` on every lowering \
+         path (#6996):\n{ir}"
+    );
+}
+
+/// The soundness boundary, held from the other side: a symbol key does NOT
+/// read an element. It resolves through `js_object_get_symbol_property`, which
+/// hands back a `%TypedArray%.prototype` accessor — a heap value that an
+/// allocating sibling can sweep. It must still be rooted.
+#[test]
+fn symbol_keyed_typed_array_read_is_still_temp_rooted() {
+    let ir = ir_for(
+        "typed_symbol_key_operand.ts",
+        vec![
+            buffer_local(0),
+            any_local(1),
+            dynamic_add_with_element_read(
+                Expr::Uint8ArrayGet {
+                    array: Box::new(Expr::LocalGet(0)),
+                    index: Box::new(Expr::SymbolFor(Box::new(Expr::String(
+                        "Symbol.iterator".to_string(),
+                    )))),
+                },
+                1,
+            ),
+        ],
+    );
+
+    assert!(
+        ir.contains("call i32 @js_gc_temp_root_push"),
+        "a symbol-keyed read returns a prototype accessor, not a byte — the \
+         #6996 skip must not reach it:\n{ir}"
+    );
+}
+
+/// The other excluded lowering: without the integer-array-index proof the read
+/// goes to `js_typed_array_index_get_dynamic`, which falls through to
+/// string-keyed property lookup, and an expando can hold anything.
+#[test]
+fn unproven_key_typed_array_read_is_still_temp_rooted() {
+    let ir = ir_for(
+        "typed_unproven_key_operand.ts",
+        vec![
+            buffer_local(0),
+            any_local(1),
+            Stmt::Let {
+                id: 2,
+                name: "k".to_string(),
+                ty: perry_hir::types::Type::Any,
+                mutable: false,
+                init: Some(Expr::Undefined),
+            },
+            dynamic_add_with_element_read(
+                Expr::Uint8ArrayGet {
+                    array: Box::new(Expr::LocalGet(0)),
+                    index: Box::new(Expr::LocalGet(2)),
+                },
+                1,
+            ),
+        ],
+    );
+
+    assert!(
+        ir.contains("call i32 @js_gc_temp_root_push"),
+        "an unproven key reads a property, not an element — the #6996 skip \
+         must not reach it:\n{ir}"
+    );
+}