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..4ded5add50 --- /dev/null +++ b/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). 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..1c4d9b0398 100644 --- a/crates/perry-codegen/src/expr/shadow_slot.rs +++ b/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, 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}" + ); +}