From 9345f424953921027435f9a13927482da7900530 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 29 Jul 2026 05:01:51 -0700 Subject: [PATCH 1/3] fix(codegen): don't root typed-array element reads as GC temporaries (#6996) `compiler-output-regression` has been red on main since 760db2fd8 (#6972, "argument temporaries are precise roots"): the `native-abi-proof` workload `native_abi_packet_control` fails `hot_loops_no_runtime_calls` with one unexpected call in its hot loop, `js_gc_temp_root_push`. Bisected, not guessed: 1a533a3a8 (#6972's parent) is green, 760db2fd8 is red, and the after-opt IR delta is exactly that one call in the hot-loop blocks. It is still red at aa1c15028. Root cause. The fixture's kernel is `(buf[i] + packet.tag + i) & 255`. The right operand is a property get on an `any`, so the add lowers through `js_dynamic_string_or_number_add` and #6972 roots the operand pair across the property get -- correctly, in general: a heap operand sitting in an SSA register while its sibling allocates is exactly the #6951 use-after-free. But the left operand here is `buf[i]`, a byte. #6972 anticipated this and gated on `expr_is_known_non_pointer_shadow_value`; that predicate has no arm for typed-array / Buffer element reads, so every iteration of the workload's hottest loop (4096 x 64) paid a push + re-read + truncate to root a value that can never be collected. Fix. Teach the predicate the element-read family. 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 two lowerings of `Uint8ArrayGet` that CAN yield a heap value are excluded by construction: a symbol key (`js_object_get_symbol_property` hands back a prototype accessor) and a key without the integer-array-index proof (`js_typed_array_index_get_dynamic` falls through to string-keyed property lookup, and an expando can hold anything). `BufferIndexGet` has neither path. Because the gate's soundness argument is "the index proof that routes this read to a byte accessor is the same proof the gate tests", 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; the copy's `BitAnd` arm was the inlined body of `bitand_has_nonnegative_i32_mask`). Two copies that could drift would make the gate unsound the moment one was edited. This is a skip, not a relaxation: #6972's rooting is untouched everywhere it protects something. 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 -- and the hot loop returns to its pre-#6972 IR. Tests. Four cases in the #6951 suite, both directions: the proven-index `Uint8ArrayGet` and `BufferIndexGet` operands emit no rooting call (both fail without this change), and the symbol-keyed and unproven-key reads still emit one. Verified: `native-abi-proof` and `native-region-proof` both report `"status": "pass"` with every workload green (full harness, link + run, not just the IR probe), both harness unittest modules pass, `cargo fmt` is clean, and `cargo test -p perry-codegen` is unchanged against main (its 3 `native_proof_regressions` failures reproduce identically on a pristine main checkout). --- crates/perry-codegen/src/expr/arrays_finds.rs | 51 ++--- crates/perry-codegen/src/expr/shadow_slot.rs | 28 +++ .../tests/temp_root_argument_temporaries.rs | 178 ++++++++++++++++++ 3 files changed, 218 insertions(+), 39 deletions(-) diff --git a/crates/perry-codegen/src/expr/arrays_finds.rs b/crates/perry-codegen/src/expr/arrays_finds.rs index a2bb3780f4..d4c9a691be 100644 --- a/crates/perry-codegen/src/expr/arrays_finds.rs +++ b/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 { if can_lower_expr_as_i32( index, @@ -42,41 +50,6 @@ fn lower_index_i32(ctx: &mut FnCtx<'_>, index: &Expr) -> Result { } } -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 { - 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, diff --git a/crates/perry-codegen/src/expr/shadow_slot.rs b/crates/perry-codegen/src/expr/shadow_slot.rs index 31cc27a714..60c50c634a 100644 --- a/crates/perry-codegen/src/expr/shadow_slot.rs +++ b/crates/perry-codegen/src/expr/shadow_slot.rs @@ -41,6 +41,34 @@ 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 two lowerings of this node that CAN yield a heap value are + // excluded by construction: + // * a symbol key (`u8[Symbol.iterator]`) goes to + // `js_object_get_symbol_property`, which returns the accessor; + // * 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 can hold anything). + // `BufferIndexGet` has neither path -- 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(_)) + && super::index_get::numeric_index_has_integer_array_index_proof(ctx, index) + } + Expr::BufferIndexGet { .. } => true, Expr::Conditional { then_expr, else_expr, diff --git a/crates/perry-codegen/tests/temp_root_argument_temporaries.rs b/crates/perry-codegen/tests/temp_root_argument_temporaries.rs index 14ea0f5c4b..5b016cf20e 100644 --- a/crates/perry-codegen/tests/temp_root_argument_temporaries.rs +++ b/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, + } +} + +/// ` + 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}" + ); +} From eba0ef1c169d9edfe2e5d5f4219c7b9ef236a05b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 29 Jul 2026 05:03:58 -0700 Subject: [PATCH 2/3] docs: changelog fragment for #6997 --- .../6997-temp-root-typed-element-reads.md | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 changelog.d/6997-temp-root-typed-element-reads.md diff --git a/changelog.d/6997-temp-root-typed-element-reads.md b/changelog.d/6997-temp-root-typed-element-reads.md new file mode 100644 index 0000000000..1d35900466 --- /dev/null +++ b/changelog.d/6997-temp-root-typed-element-reads.md @@ -0,0 +1,53 @@ +**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 +two 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 a key +without the integer-array-index proof (`js_typed_array_index_get_dynamic` +falls through to string-keyed property lookup). `BufferIndexGet` has neither +path. + +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). From 0a68c9e33d6d4240d17b8df043cf05dfca7adc46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 29 Jul 2026 05:21:28 -0700 Subject: [PATCH 3/3] review: exclude the i32-context polymorphic index arm from the temp-root skip CodeRabbit on #6997: the unproven-key regression actually emits `js_object_get_index_polymorphic`, not `js_typed_array_index_get_dynamic`. That is a THIRD heap-capable lowering of `Uint8ArrayGet` -- the i32-context one in `lower_uint8array_get_i32` -- and it is gated on `is_numeric_expr`, not on the integer-array-index proof, so testing the proof alone would not have covered it. Require both predicates (`is_numeric_expr` only ever narrows the skip: every arm it admits still reads a byte). Changelog fragment names all three arms. --- .../6997-temp-root-typed-element-reads.md | 12 +++++----- crates/perry-codegen/src/expr/shadow_slot.rs | 22 +++++++++++++------ 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/changelog.d/6997-temp-root-typed-element-reads.md b/changelog.d/6997-temp-root-typed-element-reads.md index 1d35900466..4ded5add50 100644 --- a/changelog.d/6997-temp-root-typed-element-reads.md +++ b/changelog.d/6997-temp-root-typed-element-reads.md @@ -28,12 +28,14 @@ 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 -two lowerings of `Uint8ArrayGet` that CAN yield a heap value stay rooted and +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 a key -without the integer-array-index proof (`js_typed_array_index_get_dynamic` -falls through to string-keyed property lookup). `BufferIndexGet` has neither -path. +(`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 diff --git a/crates/perry-codegen/src/expr/shadow_slot.rs b/crates/perry-codegen/src/expr/shadow_slot.rs index 60c50c634a..1c4d9b0398 100644 --- a/crates/perry-codegen/src/expr/shadow_slot.rs +++ b/crates/perry-codegen/src/expr/shadow_slot.rs @@ -55,17 +55,25 @@ pub(crate) fn expr_is_known_non_pointer_shadow_value(ctx: &FnCtx<'_>, expr: &Exp // load-bearing -- and it isn't, because both runtime accessors answer // `undefined` for a receiver that is not a Uint8Array/Buffer. // - // The two lowerings of this node that CAN yield a heap value are - // excluded by construction: + // 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; - // * 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 can hold anything). - // `BufferIndexGet` has neither path -- every arm coerces the key to - // i32 and reads a byte -- so it needs no condition. + // * 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,