From 8a828ec90fd3c681c429604ee8611e894b16366e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Tue, 11 Aug 2026 09:45:34 +0200 Subject: [PATCH 1/2] perf(codegen): put the numeric array push's GC bookkeeping behind one live test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The inline array-append tier emitted `js_string_addref_if_heap_string`, `js_gc_note_slot_layout` and a seq_cst load of `PERRY_INCREMENTAL_MARK_BARRIER_ACTIVE_COUNT` on EVERY element. On `bench/push_num.ts` — 20,000,000 pushes of a double into a `number[]` — all three are dead on all 20M of them. The static proof that retires them cannot be made for the shape that matters: `keep.push(base + j)` is an `Expr::Binary { Add }`, and `expr_produces_non_pointer_bits_by_construction` answers `false` there unconditionally, because `+` is string concatenation for non-numeric operands. This is #7511's answer to the identical problem on class-field stores, applied to the array append: ask the question ONCE inline, on the live bits, and branch over all three calls. The array's half of the proof rides the header test the `nofwd` block already performs — the integrity mask widens from 0x0407 to 0x3C07, so reaching the inline store additionally proves ELEMENT_SHAPE, TYPED_LAYOUT_INTACT and ALL_POINTERS clear, the three states in which `js_gc_note_slot_layout` does real work for a non-pointer value. A guard, not an elision: Perry does not validate declared types, so a `number`-annotated value that is a heap string at runtime takes the guarded arm and records the slot exactly as it always did. --- .../7839-numeric-push-pointer-tested.md | 43 +++ crates/perry-codegen/src/expr/array_push.rs | 170 +++++++++- .../src/expr/array_push_guard_tests.rs | 309 ++++++++++++++++++ crates/perry-codegen/src/expr/mod.rs | 3 + 4 files changed, 523 insertions(+), 2 deletions(-) create mode 100644 changelog.d/7839-numeric-push-pointer-tested.md create mode 100644 crates/perry-codegen/src/expr/array_push_guard_tests.rs diff --git a/changelog.d/7839-numeric-push-pointer-tested.md b/changelog.d/7839-numeric-push-pointer-tested.md new file mode 100644 index 0000000000..1a13940b27 --- /dev/null +++ b/changelog.d/7839-numeric-push-pointer-tested.md @@ -0,0 +1,43 @@ +### Fixed / Performance + +**`arr.push()` no longer pays three GC-bookkeeping obligations per element.** + +The inline array-append tier (`apush.inbounds`) emitted, on *every* element: +`js_string_addref_if_heap_string`, `js_gc_note_slot_layout`, and a seq_cst load +of `PERRY_INCREMENTAL_MARK_BARRIER_ACTIVE_COUNT` to gate `js_write_barrier_slot`. +On `gc-handoff/bench/push_num.ts` — 20,000,000 pushes of a double into a +`number[]` — all three are dead on all 20M of them. + +The static proof that retires them (`array_store_needs_layout_note` → +`expr_produces_non_pointer_bits_by_construction`) cannot be made for the shape +that matters. `keep.push(base + j)` is an `Expr::Binary { Add }`, and that arm +answers `false` unconditionally, because `+` is string concatenation for +non-numeric operands. It fires only for a bare canonical-i32 local, which is why +`keep.push(j)` compiles to a materially different loop than `keep.push(base + j)` +does. + +This is #7511's answer to the identical problem on class-field stores, applied to +the array append: ask the question ONCE inline, on the live bits, and branch over +all three calls. `emit_may_carry_heap_pointer_check` — already the codegen mirror +of `layout_pointer_bearing_bits` and `decode_heap_addr`, already contract-tested +over the whole 16-bit tag space — is the predicate. The store itself stays +unconditional and outside the branch; only the bookkeeping moves. + +The array's own half of the proof rides the header test the `nofwd` block already +performs: the integrity mask widens from `0x0407` to `0x0407 | 0x3800` for a +numeric push, so reaching the inline store additionally proves +`GC_ARRAY_ELEMENT_SHAPE`, `GC_OBJ_TYPED_LAYOUT_INTACT` and +`GC_LAYOUT_ALL_POINTERS` all clear — the three states in which +`js_gc_note_slot_layout` does real work for a non-pointer value. An array in any +of them takes `js_array_push_f64`, which notes the slot exactly as before. That +costs those arrays the inline store and can never cost correctness. + +**This is a guard, not an elision.** Perry does not validate declared types, so a +`number`-annotated value that is a heap string at runtime takes the guarded arm +and records the slot exactly as it always did. `the_guarded_arm_still_reaches_ +every_call_it_moved` asserts the calls are still emitted, precisely so a future +"simplification" to an outright elision fails here rather than as heap corruption. + +Gated on `is_numeric_expr`, so a pointer-pushing loop (`churn`, `tree`, +`push_cls`) emits byte-identical IR and pays nothing for a test it would always +fail. diff --git a/crates/perry-codegen/src/expr/array_push.rs b/crates/perry-codegen/src/expr/array_push.rs index d7eecab64d..d428ca0b13 100644 --- a/crates/perry-codegen/src/expr/array_push.rs +++ b/crates/perry-codegen/src/expr/array_push.rs @@ -66,7 +66,8 @@ use crate::types::{DOUBLE, I1, I16, I32, I64, I8}; use super::{ array_store_needs_layout_note, array_store_needs_write_barrier, emit_array_numeric_write_note_on_block, emit_jsvalue_slot_store_with_flags_on_block, - emit_jsvalue_slot_store_with_value_bits_on_block, emit_root_nanbox_store_on_block, + emit_jsvalue_slot_store_with_value_bits_on_block, emit_layout_note_slot_on_block, + emit_may_carry_heap_pointer_check, emit_root_nanbox_store_on_block, emit_typed_feedback_register_site, emit_write_barrier, emit_write_barrier_slot_generation_tested, expr_has_numeric_pointer_free_array_layout, lower_expr, lower_expr_native, nanbox_pointer_inline, raw_f64_layout_fact, unbox_to_i64, FnCtx, @@ -84,6 +85,142 @@ use super::{ /// expression, never an operand — a consumed `n = arr.push(x)` always /// computes the real length). When set, the placeholder constant is returned /// without emitting the call. +/// The `nofwd` admission test for a #7839 numeric push: the historical +/// integrity mask `0x0407` PLUS the three `_reserved` states in which +/// `js_gc_note_slot_layout` does real work for a **non-pointer** value stored +/// into a `GC_TYPE_ARRAY`. Every other state that function can be in is a +/// provable no-op for such a value (see +/// [`emit_numeric_push_store_pointer_tested`]). +/// +/// * `0x0407` `FROZEN|SEALED|NO_EXTEND|ARRAY_DESCRIPTORS` — the historical +/// integrity bits, unchanged in meaning and in destination. +/// * `0x0800` `GC_ARRAY_ELEMENT_SHAPE` — a live element-shape proof (#7480). +/// `note_element_store` must CLEAR it when a non-object lands in the array, +/// and that call sits ahead of every early return in `layout_note_slot`. +/// * `0x1000` — `GC_OBJ_TYPED_LAYOUT_INTACT` as `layout_note_slot` reads it +/// (`GC_ARRAY_RAW_F64_HOLES` as `gc::types` writes it for an array; the two +/// share the bit and are disjoint by `obj_type`). Set, it routes into the +/// typed-descriptor probe, whose `slot_index >= slot_count` arm downgrades. +/// * `0x2000` `GC_LAYOUT_ALL_POINTERS` — a non-pointer store into an +/// all-pointer array calls `layout_mark_unknown`, which is a real state +/// change, not a no-op. +/// +/// `GC_LAYOUT_SIDE_MASK` is deliberately absent. Skipping the note there leaves +/// a stale set bit over a non-pointer, and `mark_field_into_worklist` +/// re-validates every slot word, so the cost is one rejected visit and never a +/// stranded child — the identical argument `class_field_store_needs_layout_note` +/// already ships. +/// +/// Failing this test costs the push its inline store: it takes +/// `js_array_push_f64`, which notes the slot exactly as it always did. So a +/// widening here can only ever be slower, never wrong — the same direction of +/// approximation `emit_may_carry_heap_pointer_check` documents. +/// +/// `0x0407 | 0x3800` == `0x3C07` == 15367. +const ARRAY_PUSH_NUMERIC_CLEAN_I16: &str = "15367"; + +/// #7839 — the inline array append's GC bookkeeping behind ONE live test. +/// +/// The `apush.inbounds` store used to pay `js_string_addref_if_heap_string` + +/// `js_gc_note_slot_layout` unconditionally and then an `ldar` on +/// `PERRY_INCREMENTAL_MARK_BARRIER_ACTIVE_COUNT` for the barrier gate — three +/// cross-crate obligations on *every* element of a `number[]` push loop, where +/// all three are dead. `bench/push_num.ts` is 20M such pushes. +/// +/// The static proof that would retire them (`array_store_needs_layout_note` → +/// `expr_produces_non_pointer_bits_by_construction`) cannot be made for the +/// shape that matters: `keep.push(base + j)` is an `Expr::Binary { Add }`, and +/// that arm answers `false` unconditionally because `+` is string concatenation +/// for non-numeric operands. It fires only for a bare canonical-i32 local +/// (`keep.push(j)`), which is why the same loop is ~1.7x faster written that +/// way. This is #7511's answer to the identical problem on class fields: ask +/// the question ONCE inline, on the live bits, and branch over all three. +/// +/// Why each obligation is dead when the test says no: +/// +/// * `js_string_addref_if_heap_string` is tag-checked and a no-op for every +/// non-`STRING_TAG` value — `emit_may_carry_heap_pointer_check` admits +/// `STRING_TAG`, so a string always takes the guarded arm. +/// * `js_write_barrier_slot` opens with `barrier_child_prologue`, which returns +/// immediately when `decode_heap_addr(child) == 0`. The predicate is a +/// superset of every address that decoder resolves. +/// * `js_gc_note_slot_layout` for a non-pointer value is a no-op in every +/// layout state EXCEPT three, and reaching this block already PROVES all +/// three clear: [`ARRAY_PUSH_NUMERIC_CLEAN_I16`] widens the `nofwd` +/// integrity mask to cover them, so the array's half of the proof costs a +/// wider constant on an `and` that was being emitted anyway, and this block +/// has only the value left to test. +/// +/// Gated on `value_is_numeric` at the call site, so a pointer-pushing loop +/// (`churn`, `tree`, `push_cls`) emits byte-identical IR to before rather than +/// paying the predicate for a test it always fails. That also keeps +/// `js_array_note_numeric_write` out of the picture: it is already statically +/// elided for exactly this class of value. +#[allow(clippy::too_many_arguments)] +fn emit_numeric_push_store_pointer_tested( + ctx: &mut FnCtx<'_>, + arr_handle: &str, + value_double: &str, + value_bits_override: Option<&str>, + string_addref_needed: bool, + layout_note_needed: bool, + write_barrier_needed: bool, +) -> (String, String, Option) { + let (length, element_addr, value_bits) = { + let blk = ctx.block(); + let length = blk.safe_load_i32_from_ptr(arr_handle); + let length_i64 = blk.zext(I32, &length, I64); + let byte_offset = blk.shl(I64, &length_i64, "3"); + let with_header = blk.add(I64, &byte_offset, "8"); + let element_addr = blk.add(I64, arr_handle, &with_header); + let element_ptr = blk.inttoptr(I64, &element_addr); + // GC_STORE_AUDIT(BARRIERED): the slot write itself is unconditional; + // only the bookkeeping moves behind the live test below, and the + // barrier's own first test is a subset of that predicate. + blk.store(DOUBLE, value_double, &element_ptr); + let value_bits = value_bits_override + .map(ToOwned::to_owned) + .unwrap_or_else(|| blk.bitcast_double_to_i64(value_double)); + (length, element_addr, value_bits) + }; + let bookkeeping_idx = ctx.new_block("apush.gc_bookkeeping"); + let done_idx = ctx.new_block("apush.gc_bookkeeping.done"); + let bookkeeping_label = ctx.block_label(bookkeeping_idx); + let done_label = ctx.block_label(done_idx); + { + let blk = ctx.block(); + let may_carry_pointer = emit_may_carry_heap_pointer_check(blk, &value_bits); + blk.cond_br(&may_carry_pointer, &bookkeeping_label, &done_label); + } + ctx.current_block = bookkeeping_idx; + { + let blk = ctx.block(); + if string_addref_needed { + blk.call_void("js_string_addref_if_heap_string", &[(DOUBLE, value_double)]); + } + if layout_note_needed { + emit_layout_note_slot_on_block(blk, arr_handle, &length, &value_bits); + } + } + if write_barrier_needed { + // `arr_handle` reached here through the `nofwd` header test, so it is a + // live, non-forwarded GC array user pointer — the precondition for + // reading its header byte. The generation test stays: this arm is + // reached for real pointer children too. + emit_write_barrier_slot_generation_tested( + ctx, + arr_handle, + arr_handle, + &element_addr, + &value_bits, + "apush", + ); + } + ctx.block().br(&done_label); + ctx.current_block = done_idx; + (length, element_addr, None) +} + fn emit_array_handle_length( ctx: &mut FnCtx<'_>, array_handle: &str, @@ -423,6 +560,14 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr, value_discarded: bool) -> let value_is_numeric = is_numeric_expr(ctx, value); let require_numeric_layout = value_is_numeric && expr_has_numeric_pointer_free_array_layout(ctx, &array_expr); + // #7839 — the inline append's three GC-bookkeeping calls behind ONE + // live test of the stored bits, exactly #7511's class-field shape. + // See `emit_numeric_push_store_pointer_tested` for why each call is + // dead when the test says "no pointer, no watched layout state", + // and why the gate is `value_is_numeric` rather than unconditional. + let guarded_numeric_bookkeeping = value_is_numeric + && !declared_all_pointer + && (layout_note_needed || string_addref_needed || write_barrier_needed); // #7634: spec order (receiver Reference, then argument) is only // observable when the argument can rebind the receiver. When it // can, take the rooted spec-ordered arm; when it cannot — the hot @@ -764,6 +909,17 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr, value_discarded: bool) -> // notes the slot); it can never cost correctness. let admitted_bits = blk.and(I16, &obj_flags, "62599"); blk.icmp_eq(I16, &admitted_bits, "40960") + } else if guarded_numeric_bookkeeping { + // #7839 — the array's half of the guard, folded into the + // integrity test rather than emitted as a second one: + // same `and`, same `icmp`, a wider constant. Reaching + // the inline store now additionally proves the three + // `_reserved` states in which `js_gc_note_slot_layout` + // does real work for a NON-pointer value, so the store's + // guard has only the value left to test. See + // `emit_numeric_push_store_pointer_tested`. + let admitted_bits = blk.and(I16, &obj_flags, ARRAY_PUSH_NUMERIC_CLEAN_I16); + blk.icmp_eq(I16, &admitted_bits, "0") } else { // FROZEN(0x1)|SEALED(0x2)|NO_EXTEND(0x4)|ARRAY_DESCRIPTORS(0x400). let integrity_bits = blk.and(I16, &obj_flags, "1031"); @@ -796,7 +952,17 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr, value_discarded: bool) -> // between the store and the barrier would run with the // old→young edge unrecorded. The block is split here rather // than the call being sunk to the end of the block. - let (length, element_addr, barrier_value_bits) = { + let (length, element_addr, barrier_value_bits) = if guarded_numeric_bookkeeping { + emit_numeric_push_store_pointer_tested( + ctx, + &arr_handle, + &v, + v_bits.as_deref(), + string_addref_needed, + layout_note_needed, + write_barrier_needed, + ) + } else { let blk = ctx.block(); let length = blk.safe_load_i32_from_ptr(&arr_handle); let length_i64 = blk.zext(I32, &length, I64); diff --git a/crates/perry-codegen/src/expr/array_push_guard_tests.rs b/crates/perry-codegen/src/expr/array_push_guard_tests.rs new file mode 100644 index 0000000000..8e12fdec28 --- /dev/null +++ b/crates/perry-codegen/src/expr/array_push_guard_tests.rs @@ -0,0 +1,309 @@ +//! #7839: the inline array append's GC bookkeeping behind ONE live test of the +//! stored bits. +//! +//! These are IR-census tests, and both directions matter. +//! +//! The positive one asserts the subject is LIVE. A guard predicate that +//! silently never fires still compiles, still prints the right answer, and +//! shows up in no other test — `push_num.ts` would simply stay slow. Only the +//! emitted block label separates "implemented" from "reached" (CLAUDE.md, "a +//! gate must assert its subject was live"), so the block name is asserted +//! present AND the `apush.inbounds` fast path is asserted free of the two calls +//! the guard exists to move out of it. +//! +//! The negative is the safety half, twice over. A pointer-valued push must keep +//! the historical unguarded shape — widening the guard to it would pay a +//! predicate for a test that always says "yes" — and, more importantly, the +//! bookkeeping calls must still be REACHABLE from the guarded arm. The change +//! is "skip the calls when the live bits prove them dead", never "elide them +//! outright": a `number`-annotated parameter that actually holds a string at +//! runtime (Perry does not validate declared types) takes the guarded arm and +//! records the slot exactly as it always did. A test that asserted the calls +//! ABSENT would be pinning silent heap corruption. + +use crate::{compile_module, AppMetadata, CompileOptions}; +use perry_hir::types::Type; +use perry_hir::{ + BinaryOp, Class, ClassField, CompareOp, Expr, Function, Module, ModuleInitKind, Param, Stmt, + UpdateOp, +}; + +/// The block that exists only when the #7839 guard was emitted. +const GUARD_BLOCK: &str = "apush.gc_bookkeeping"; +const NOTE_CALL: &str = "call void @js_gc_note_slot_layout("; +const ADDREF_CALL: &str = "call void @js_string_addref_if_heap_string("; +/// `ARRAY_PUSH_NUMERIC_CLEAN_I16` as it appears in the `nofwd` admission test. +const WIDENED_ADMISSION_MASK: &str = "15367"; +/// The historical integrity mask, which the numeric push must NOT still use. +const NARROW_INTEGRITY_MASK: &str = ", 1031"; + +fn ir_opts() -> CompileOptions { + CompileOptions { + target: None, + is_entry_module: true, + non_entry_module_prefixes: Vec::new(), + nextjs_path_init_modules: Vec::new(), + import_function_prefixes: std::collections::HashMap::new(), + import_function_ffi_aliases: std::collections::HashMap::new(), + import_function_origin_names: std::collections::HashMap::new(), + import_function_v8_specifiers: std::collections::HashMap::new(), + import_function_node_submodule: std::collections::HashMap::new(), + namespace_node_submodules: std::collections::HashMap::new(), + namespace_v8_specifiers: std::collections::HashMap::new(), + namespace_member_prefixes: std::collections::HashMap::new(), + namespace_member_origin_names: std::collections::HashMap::new(), + emit_ir_only: true, + verify_native_regions: false, + disable_buffer_fast_path: false, + namespace_imports: Vec::new(), + namespace_member_nested: Vec::new(), + imported_classes: Vec::new(), + imported_enums: Vec::new(), + imported_async_funcs: std::collections::HashSet::new(), + type_aliases: std::collections::HashMap::new(), + imported_func_param_counts: std::collections::HashMap::new(), + imported_func_has_rest: std::collections::HashSet::new(), + imported_func_synthetic_arguments: std::collections::HashSet::new(), + imported_func_return_types: std::collections::HashMap::new(), + imported_vars: std::collections::HashSet::new(), + output_type: "executable".to_string(), + needs_stdlib: false, + needs_ui: false, + needs_geisterhand: false, + geisterhand_port: 7676, + enabled_features: Vec::new(), + native_module_init_names: Vec::new(), + js_module_specifiers: Vec::new(), + bundled_extensions: Vec::new(), + native_library_functions: Vec::new(), + i18n_table: None, + fast_math: false, + fp_contract_mode: crate::FpContractMode::Off, + app_metadata: AppMetadata::default(), + namespace_entries: Vec::new(), + dynamic_import_path_to_prefix: std::collections::HashMap::new(), + deferred_module_prefixes: std::collections::HashSet::new(), + module_init_deps: Vec::new(), + is_dynamic_import_target: false, + debug_locations: false, + module_source: None, + debug_source_line_offset: 0, + } +} + +const ARRAY_ID: u32 = 1; +const COUNTER_ID: u32 = 2; +const BASE_ID: u32 = 3; + +fn node_class() -> Class { + Class { + id: 404, + name: "Node".to_string(), + type_params: Vec::new(), + extends: None, + extends_name: None, + native_extends: None, + extends_expr: None, + heritage_lexically_shadowed: false, + fields: vec![ClassField { + name: "v".to_string(), + key_expr: None, + ty: Type::Number, + init: None, + is_private: false, + is_readonly: false, + decorators: Vec::new(), + }], + constructor: None, + methods: Vec::new(), + getters: Vec::new(), + setters: Vec::new(), + static_accessor_names: Vec::new(), + static_accessor_fn_ids: Vec::new(), + computed_members: Vec::new(), + static_fields: Vec::new(), + static_methods: Vec::new(), + decorators: Vec::new(), + is_exported: false, + aliases: Vec::new(), + is_nested: false, + alloc_width_hint: 0, + specialized_from: None, + } +} + +/// `function chunk(base: number) { const keep: [] = []; for (let j = 0; +/// j < 1000; j++) keep.push() }` — `bench/push_num.ts`'s kernel, in the +/// position that matters: the array is a plain function LOCAL, which is what +/// puts the push on the inline `apush` tier at all. +fn push_module(elem: Type, value: Expr, classes: Vec) -> Module { + let mut m = Module::new("array_push_guard.ts"); + m.classes = classes; + m.functions = vec![Function { + id: 700, + name: "chunk".to_string(), + type_params: Vec::new(), + params: vec![Param { + id: BASE_ID, + name: "base".to_string(), + ty: Type::Number, + default: None, + decorators: Vec::new(), + is_rest: false, + arguments_object: None, + }], + return_type: Type::Void, + body: vec![ + Stmt::Let { + id: ARRAY_ID, + name: "keep".to_string(), + ty: Type::Array(Box::new(elem)), + mutable: false, + init: Some(Expr::Array(Vec::new())), + }, + Stmt::For { + init: Some(Box::new(Stmt::Let { + id: COUNTER_ID, + name: "j".to_string(), + ty: Type::Number, + mutable: true, + init: Some(Expr::Integer(0)), + })), + condition: Some(Expr::Compare { + op: CompareOp::Lt, + left: Box::new(Expr::LocalGet(COUNTER_ID)), + right: Box::new(Expr::Integer(1000)), + }), + update: Some(Expr::Update { + id: COUNTER_ID, + op: UpdateOp::Increment, + prefix: false, + }), + body: vec![Stmt::Expr(Expr::ArrayPush { + array_id: ARRAY_ID, + value: Box::new(value), + })], + }, + ], + is_async: false, + is_generator: false, + is_strict: false, + is_exported: false, + captures: Vec::new(), + decorators: Vec::new(), + was_plain_async: false, + was_unrolled: false, + }]; + // Called once from module init so the function is not dead-stripped before + // the census can see it. + m.init = vec![Stmt::Expr(Expr::Call { + callee: Box::new(Expr::FuncRef(700)), + args: vec![Expr::Number(1.0)], + type_args: Vec::new(), + byte_offset: 0, + })]; + m.init_kind = ModuleInitKind::Eager; + m +} + +fn ir_for(m: Module) -> String { + String::from_utf8(compile_module(&m, ir_opts()).expect("module compiles")) + .expect("LLVM IR should be UTF-8") +} + +/// The one block between `apush.inbounds` and the next label, i.e. the fast +/// path the guard exists to empty. Asserting over the WHOLE function would pass +/// while the calls sat in the fast path, because the guarded arm contains them +/// too. +fn inbounds_block(ir: &str) -> String { + let start = ir + .find("\napush.inbounds") + .unwrap_or_else(|| panic!("no apush.inbounds block in:\n{ir}")); + let rest = &ir[start + 1..]; + let body_start = rest.find('\n').expect("label line") + 1; + let end = rest[body_start..] + .find("\n\n") + .map(|e| body_start + e) + .unwrap_or(rest.len()); + rest[..end].to_string() +} + +/// `keep.push(base + j)` — `push_num.ts` verbatim. `Expr::Binary { Add }` is +/// the shape no static non-pointer proof can admit (`+` is string +/// concatenation for non-numeric operands), which is exactly why the live test +/// is what retires the calls here. +fn numeric_add_push() -> Expr { + Expr::Binary { + op: BinaryOp::Add, + left: Box::new(Expr::LocalGet(BASE_ID)), + right: Box::new(Expr::LocalGet(COUNTER_ID)), + } +} + +#[test] +fn a_numeric_push_moves_its_gc_bookkeeping_behind_one_live_test() { + let ir = ir_for(push_module(Type::Number, numeric_add_push(), Vec::new())); + assert!( + ir.contains(GUARD_BLOCK), + "the #7839 guard was never emitted for `keep.push(base + j)` on a \ + `number[]`; without it every element of push_num.ts pays two \ + cross-crate calls:\n{ir}" + ); + let inbounds = inbounds_block(&ir); + assert!( + !inbounds.contains(NOTE_CALL), + "js_gc_note_slot_layout is still on the inline fast path:\n{inbounds}" + ); + assert!( + !inbounds.contains(ADDREF_CALL), + "js_string_addref_if_heap_string is still on the inline fast path:\n{inbounds}" + ); + // The array's half of the proof: the `nofwd` admission test must have + // widened, or an element-shape-proven / all-pointer / typed-descriptor + // array would reach the inline store and silently skip the note it needs. + assert!( + ir.contains(WIDENED_ADMISSION_MASK), + "the nofwd admission mask did not widen to 0x3C07 for a numeric push:\n{ir}" + ); + assert!( + !ir.contains(NARROW_INTEGRITY_MASK), + "a numeric push still admits on the narrow 0x0407 integrity mask, so \ + the guard rests on nothing about the array:\n{ir}" + ); +} + +#[test] +fn the_guarded_arm_still_reaches_every_call_it_moved() { + let ir = ir_for(push_module(Type::Number, numeric_add_push(), Vec::new())); + // Not an elision. A `number`-annotated value that is a heap string at + // runtime (Perry does not validate declared types) takes this arm. + assert!( + ir.contains(NOTE_CALL), + "the layout note was ELIDED rather than guarded — a pointer reaching \ + this push would strand a live child:\n{ir}" + ); + assert!( + ir.contains(ADDREF_CALL), + "the string addref was ELIDED rather than guarded:\n{ir}" + ); +} + +#[test] +fn a_pointer_push_keeps_the_historical_unguarded_shape() { + let ir = ir_for(push_module( + Type::Named("Node".to_string()), + Expr::New { + class_name: "Node".to_string(), + args: vec![Expr::LocalGet(COUNTER_ID)], + type_args: Vec::new(), + byte_offset: 0, + cap_args_appended: 0, + }, + vec![node_class()], + )); + assert!( + !ir.contains(GUARD_BLOCK), + "a `new Node()` push took the numeric guard: it would pay the \ + predicate for a test whose answer is always yes:\n{ir}" + ); +} diff --git a/crates/perry-codegen/src/expr/mod.rs b/crates/perry-codegen/src/expr/mod.rs index b3d19b9cb0..a68d3f00f8 100644 --- a/crates/perry-codegen/src/expr/mod.rs +++ b/crates/perry-codegen/src/expr/mod.rs @@ -123,6 +123,7 @@ pub(crate) use write_barrier::{ emit_array_numeric_write_note_on_block, emit_jsvalue_slot_store_on_block, emit_jsvalue_slot_store_pointer_tested, emit_jsvalue_slot_store_scalar_aware_on_block, emit_jsvalue_slot_store_with_flags_on_block, emit_jsvalue_slot_store_with_value_bits_on_block, + emit_layout_note_slot_on_block, emit_may_carry_heap_pointer_check, emit_root_heap_word_store_on_block, emit_root_nanbox_store_on_block, emit_write_barrier, emit_write_barrier_slot_generation_tested, emit_write_barrier_slot_on_block, lower_array_super_init, lower_event_emitter_subclass_init, lower_node_stream_super_init, @@ -133,6 +134,8 @@ pub(crate) use write_barrier::{ // bulky `record_lowered_value*` method family, the shadow-slot free helpers, // and the `lower_expr` dispatch table moved into siblings to keep this file // under 2000 lines. Inherent methods (`record_value`) need no re-export. +#[cfg(test)] +mod array_push_guard_tests; mod dispatch; mod record_value; mod repsel_gates; From 30f4dc9c76063a676ba004e31f77021ec3b4a5b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Tue, 11 Aug 2026 13:34:31 +0200 Subject: [PATCH 2/2] test(codegen): pin that a declared-type lie cannot reach the numeric push guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two sabotage-verified IR gates, prompted by review of the #7831/#7837 family against #7839's guard. `a_declared_type_lie_is_routed_to_the_runtime_tier_not_the_guard` — a `number[]` really can hold heap strings at runtime, and `is_numeric_expr` admits an element read off one (#7810). What keeps that value off the inline guard is `expr_produces_canonical_raw_f64` excluding every READ, which routes it to the pre-existing runtime numeric tier instead. Widening that predicate to admit a read fails this test. `the_guard_branches_on_the_live_bits_not_on_a_constant` — pins the guard's condition to a computed register and its predicate to the full heap-tag set. Hard-wiring the branch to `false` fails this test; it is invisible to every output-equality probe, because the elided bookkeeping is a GC-liveness fact rather than an arithmetic one. --- .../7839-numeric-push-pointer-tested.md | 23 +++++ .../src/expr/array_push_guard_tests.rs | 96 +++++++++++++++++++ 2 files changed, 119 insertions(+) diff --git a/changelog.d/7839-numeric-push-pointer-tested.md b/changelog.d/7839-numeric-push-pointer-tested.md index 1a13940b27..a181700ca5 100644 --- a/changelog.d/7839-numeric-push-pointer-tested.md +++ b/changelog.d/7839-numeric-push-pointer-tested.md @@ -41,3 +41,26 @@ every_call_it_moved` asserts the calls are still emitted, precisely so a future Gated on `is_numeric_expr`, so a pointer-pushing loop (`churn`, `tree`, `push_cls`) emits byte-identical IR and pays nothing for a test it would always fail. + +**Why an erased annotation cannot reach this guard (#7831/#7837 collision).** +A `number[]` really can hold heap strings at runtime, so it matters exactly +which values arrive at the live-bits test. Two independent predicates decide, +and only the second is load-bearing here. `is_numeric_expr` DOES admit a read +off a `number[]` (#7810), so an annotation alone would put a heap string on a +numeric push path — but `expr_produces_canonical_raw_f64` excludes every READ +("cold fallbacks return boxed bits"), which keeps `keep_guarded_numeric_push` +true and routes those pushes to the pre-existing RUNTIME numeric tier +(`js_array_numeric_push_f64_unboxed` behind its feedback guard), never to this +inline guard. The inline guard is reached only for values that are canonical +raw f64 BY CONSTRUCTION — a machine FP op, which cannot produce a pointer +except by ARM NaN-payload propagation from a NaN-boxed operand, and that is +precisely the case the live-bits test catches. + +Verified rather than asserted: `gc-handoff/m0810/numarr_lie.ts` and four more +declared-type-lie shapes (a `number` parameter, a `number` object field, a +module-level `number` global, an element read off a `number[]`) emit **zero** +guard blocks. `a_declared_type_lie_is_routed_to_the_runtime_tier_not_the_guard` +pins that routing, and `the_guard_branches_on_the_live_bits_not_on_a_constant` +pins the guard's condition to a computed register. Both are sabotage-verified: +hard-wiring the branch to `false` fails the second, and widening +`expr_produces_canonical_raw_f64` to admit a read fails the first. diff --git a/crates/perry-codegen/src/expr/array_push_guard_tests.rs b/crates/perry-codegen/src/expr/array_push_guard_tests.rs index 8e12fdec28..89d3241509 100644 --- a/crates/perry-codegen/src/expr/array_push_guard_tests.rs +++ b/crates/perry-codegen/src/expr/array_push_guard_tests.rs @@ -307,3 +307,99 @@ fn a_pointer_push_keeps_the_historical_unguarded_shape() { predicate for a test whose answer is always yes:\n{ir}" ); } + +// --------------------------------------------------------------------------- +// #7831/#7837 collision: an erased annotation is a hint, not a runtime proof. +// --------------------------------------------------------------------------- + +/// `arr.push()` — a declared-type LIE vehicle. +/// +/// A `number[]` can hold heap strings at runtime; Perry does not validate +/// declared types, and `gc-handoff/m0810/numarr_lie.ts` builds exactly such an +/// array. `is_numeric_expr` DOES admit this read (it consults the declared +/// element type, #7810), so the annotation alone would put a heap string on a +/// numeric push path. +/// +/// What keeps it off #7839's guard is a second, independent test: +/// `expr_produces_canonical_raw_f64` excludes every READ ("cold fallbacks +/// return boxed bits"), so `keep_guarded_numeric_push` stays true and the push +/// takes the pre-existing RUNTIME numeric tier — `js_array_numeric_push_f64_ +/// unboxed` behind its feedback guard — which validates the value at runtime. +/// #7839's inline guard is reached only when the value is canonical raw f64 BY +/// CONSTRUCTION, i.e. produced by a machine FP op that cannot yield a pointer +/// except through NaN-payload propagation, which is precisely what its +/// live-bits test catches. +/// +/// This test pins that routing. If `expr_produces_canonical_raw_f64` ever +/// widened to admit a read, a declared-type lie would start arriving at the +/// inline guard, and this fails instead of the guard silently resting on an +/// erased annotation. +fn element_read_push_module() -> Module { + let mut m = push_module(Type::Number, Expr::Number(0.0), Vec::new()); + let f = &mut m.functions[0]; + let Some(Stmt::For { body, .. }) = f.body.get_mut(1) else { + panic!("push_module's second statement should be the `for`"); + }; + body[0] = Stmt::Expr(Expr::ArrayPush { + array_id: ARRAY_ID, + value: Box::new(Expr::IndexGet { + object: Box::new(Expr::LocalGet(ARRAY_ID)), + index: Box::new(Expr::LocalGet(COUNTER_ID)), + }), + }); + m +} + +#[test] +fn a_declared_type_lie_is_routed_to_the_runtime_tier_not_the_guard() { + let ir = ir_for(element_read_push_module()); + // Non-vacuity: the push must actually have been lowered on the tier this + // test names. Without this the assertion below would also pass for a push + // that was not lowered at all. + assert!( + ir.contains("js_array_numeric_push_f64_unboxed"), + "expected the runtime numeric tier for an element-read value; this test is not observing the tier it claims to:\n{ir}" + ); + assert!( + !ir.contains(GUARD_BLOCK), + "an element read off a `number[]` reached the #7839 inline guard. That array can hold heap strings at runtime (numarr_lie.ts), so the guard would be resting on an erased annotation instead of on the value's construction:\n{ir}" + ); +} + +#[test] +fn the_guard_branches_on_the_live_bits_not_on_a_constant() { + let ir = ir_for(push_module(Type::Number, numeric_add_push(), Vec::new())); + let inbounds = inbounds_block(&ir); + let branch = inbounds + .lines() + .find(|l| l.contains("br i1") && l.contains(GUARD_BLOCK)) + .unwrap_or_else(|| panic!("no branch into the guarded arm:\n{inbounds}")); + let cond = branch + .trim() + .strip_prefix("br i1 ") + .and_then(|r| r.split(',').next()) + .expect("br i1 , ..."); + // A constant condition is the exact shape a "simplification" of the guard + // collapses to, and it is invisible to every output-equality test: a + // sabotaged build with `br i1 false` still prints the right answer on every + // probe, because the elided bookkeeping is a GC-liveness fact, not an + // arithmetic one. Pin the condition to a computed register. + assert!( + cond.starts_with('%'), + "the guard branches on the constant `{cond}` — the bookkeeping arm is \ + unreachable and the guard proves nothing:\n{inbounds}" + ); + assert!( + inbounds.contains(&format!("{cond} = or i1 ")), + "the guard's condition {cond} is not the `or` of the live-bits tests:\n{inbounds}" + ); + // ...and the live-bits tests themselves: POINTER_TAG / STRING_TAG / + // BIGINT_TAG top-16 comparands, plus the bare-heap-address floor. + for needle in ["lshr i64", "32765", "32767", "32762", "4096"] { + assert!( + inbounds.contains(needle), + "the live-bits predicate lost `{needle}`, so it no longer covers \ + every heap tag `layout_pointer_bearing_bits` accepts:\n{inbounds}" + ); + } +}