diff --git a/changelog.d/affine-index-magnitude-bound.md b/changelog.d/affine-index-magnitude-bound.md new file mode 100644 index 0000000000..a48f7013db --- /dev/null +++ b/changelog.d/affine-index-magnitude-bound.md @@ -0,0 +1,29 @@ +**The affine index materialization can no longer wrap i64** (#9294 +follow-up, from a review flag on its sibling PR). + +#9294 computed `a[]` indices in i64 on the claim that proven-i32 +leaves cannot overflow it. That is true for one multiply — `|i32 * i32|` is +at most 2^62 — and false beyond it: three chained near-2^31 factors reach +2^93, wrap i64, and a wrapped value that happens to land inside `[0, len)` +passes the unsigned bounds check and reads a DIFFERENT element than the +generic path, silently — JS computes the index in doubles, goes out of +bounds, and yields `undefined`. + +Measured honestly: the wrap is LATENT today, not live. Neither a +const-folded spelling nor parameter leaves of a triple-multiply chain +currently reach the affine lowering — admission happens to be blocked by +which locals carry i32 shadow slots, an accident of unrelated analyses +rather than a guarantee. Widening shadow coverage is a plausible future +change, and it would have turned this into a silent wrong-read with no +failing test anywhere. + +The fix is a static magnitude bound, `affine_index_magnitude_bound`: +interval arithmetic in i128 at match time with every leaf at its i32 +extreme, admitting a tree only when its worst case fits i63. Admission +therefore costs nothing at run time; `i * size + k` (2^62 + 2^31) stays +admitted and matmul's numbers are unchanged, while any tree that could wrap +declines to the generic path. One shared predicate gates both the matcher +and the lowering, so the two cannot drift. A tripwire test pins the exact +2^64 tree (`2^21 * 2^22 * (2^21 + k)`) to node's `NaN` under both collector +modes — it passes today on both sides and exists to fail the moment +admission widens past the bound. diff --git a/crates/perry-codegen/src/expr/index_get.rs b/crates/perry-codegen/src/expr/index_get.rs index b7c694ea3e..21a4b00ba1 100644 --- a/crates/perry-codegen/src/expr/index_get.rs +++ b/crates/perry-codegen/src/expr/index_get.rs @@ -42,7 +42,7 @@ use super::{ mod foreign_counter; mod guarded_array; -pub(crate) use foreign_counter::packed_f64_loop_index_parts; +pub(crate) use foreign_counter::{affine_index_fits_i64, packed_f64_loop_index_parts}; use foreign_counter::{ affine_packed_loop_read, emit_affine_index_i64, foreign_packed_loop_read, packed_f64_loop_offset_read, diff --git a/crates/perry-codegen/src/expr/index_get/foreign_counter.rs b/crates/perry-codegen/src/expr/index_get/foreign_counter.rs index 679c11c740..c42f52e089 100644 --- a/crates/perry-codegen/src/expr/index_get/foreign_counter.rs +++ b/crates/perry-codegen/src/expr/index_get/foreign_counter.rs @@ -103,6 +103,45 @@ pub(crate) fn packed_f64_loop_offset_read( /// /// The bare-local cases belong to `foreign_packed_loop_read` and the clone's /// own counter path; this is `a[i * size + k]`. + +/// Conservative magnitude bound of an affine index tree, with every leaf at +/// its i32 extreme. `None` means a node outside the affine grammar. +/// +/// #9294 shipped the i64 materialization with the claim that proven-i32 +/// leaves cannot overflow it. That is true for one multiply — |i32 * i32| +/// <= 2^62 — and FALSE beyond it: three chained near-2^31 factors reach +/// 2^93, wrap i64, and a wrapped value that happens to land in [0, len) +/// passes the unsigned bounds check and reads a DIFFERENT element than the +/// generic path (JS computes the index in doubles, goes out of bounds, and +/// yields `undefined`). Flagged by review on the follow-up PR. The bound is +/// computed in i128 at match time, so admission costs nothing at run time: +/// a tree whose worst case fits i63 cannot wrap, and anything else declines +/// to the generic path. +pub(crate) fn affine_index_magnitude_bound(index: &Expr) -> Option { + match index { + Expr::Integer(v) => Some((*v as i128).unsigned_abs().min(1 << 31) as i128), + Expr::Number(n) if n.fract() == 0.0 && n.abs() <= f64::from(i32::MAX) => { + Some(n.abs() as i128) + } + Expr::LocalGet(_) => Some(1_i128 << 31), + Expr::Binary { op, left, right } => { + let l = affine_index_magnitude_bound(left)?; + let r = affine_index_magnitude_bound(right)?; + match op { + perry_hir::BinaryOp::Add | perry_hir::BinaryOp::Sub => l.checked_add(r), + perry_hir::BinaryOp::Mul => l.checked_mul(r), + _ => None, + } + } + _ => None, + } +} + +/// The i64 materialization is wrap-free for exactly the trees this accepts. +pub(crate) fn affine_index_fits_i64(index: &Expr) -> bool { + affine_index_magnitude_bound(index).is_some_and(|b| b < (1_i128 << 63)) +} + pub(crate) fn affine_packed_loop_read( ctx: &FnCtx<'_>, arr_id: u32, @@ -117,7 +156,9 @@ pub(crate) fn affine_packed_loop_read( .rev() .find(|fact| fact.array_local_id == arr_id && fact.affine_indices)? .clone(); - affine_index_leaves_materializable(ctx, index, fact.index_local_id).then_some(fact) + (affine_index_fits_i64(index) + && affine_index_leaves_materializable(ctx, index, fact.index_local_id)) + .then_some(fact) } /// Every leaf must be readable as an i32 here, matching exactly what the diff --git a/crates/perry-codegen/src/expr/mod.rs b/crates/perry-codegen/src/expr/mod.rs index 749ee678a1..e4f65bee03 100644 --- a/crates/perry-codegen/src/expr/mod.rs +++ b/crates/perry-codegen/src/expr/mod.rs @@ -2746,7 +2746,7 @@ mod unary_bigint_tests; #[cfg(test)] mod unary_bitnot_tests; pub(crate) use index_get::{ - numeric_index_has_integer_array_index_proof, packed_f64_loop_index_parts, + affine_index_fits_i64, numeric_index_has_integer_array_index_proof, packed_f64_loop_index_parts, }; pub(crate) use masked_window::masked_window_fact_for_index; /// Rooting coverage for the computed-store arms the TS corpora cannot reach diff --git a/crates/perry-codegen/src/stmt/loops.rs b/crates/perry-codegen/src/stmt/loops.rs index b31746711d..256563393e 100644 --- a/crates/perry-codegen/src/stmt/loops.rs +++ b/crates/perry-codegen/src/stmt/loops.rs @@ -2277,6 +2277,10 @@ pub(super) fn packed_f64_range_loop_pure_expr_collect( // loop from that tier, which is a regression, not a win. if packed_f64_range_loop_index_is_affine_with(index, counter_id, leaf_ok) && expr_mentions_local(index, counter_id) + // #9294 follow-up: the i64 materialization is wrap-free + // only when the tree's worst case fits i63. Shared with + // the lowering so the two cannot disagree. + && crate::expr::affine_index_fits_i64(index) { record_packed_f64_range_affine_access(accesses, *arr_id); return true; diff --git a/crates/perry/tests/affine_index_overflow_bound.rs b/crates/perry/tests/affine_index_overflow_bound.rs new file mode 100644 index 0000000000..0ab04f981d --- /dev/null +++ b/crates/perry/tests/affine_index_overflow_bound.rs @@ -0,0 +1,81 @@ +//! The affine index materialization cannot wrap i64 (#9294 follow-up). +//! +//! #9294 computed `a[]` indices in i64 on the claim that proven-i32 +//! leaves cannot overflow it. True for one multiply (|i32 * i32| <= 2^62), +//! false beyond: `2^21 * 2^22 * (2^21 + k)` at `k = 0` is exactly 2^64, the +//! i64 computation wraps to 0, the wrapped index passes the unsigned bounds +//! check, and the fast path reads `a[0]` — the WRONG element, silently — +//! where JS computes the index in doubles, goes out of bounds, and yields +//! `undefined` (NaN after the add). Flagged by review on the follow-up PR. +//! +//! The fix is a static magnitude bound (`affine_index_magnitude_bound`): +//! interval arithmetic in i128 at match time with every leaf at its i32 +//! extreme, admitting the tree only when its worst case fits i63 — so +//! admission costs nothing at run time and the matcher and the lowering +//! share one predicate. `i * size + k` (2^62 + 2^31) stays admitted; this +//! tree (~2^74) declines to the generic path. + +use std::path::PathBuf; +use std::process::Command; + +fn perry_bin() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_perry")) +} + +const SOURCE: &str = r#" +function run(a: number[]): number { + const x = 2097152; + const y = 4194304; + const z = 2097152; + let s = 0.0; + for (let k = 0; k < 1; k++) { + s = s * 1.0 + a[x * y * (z + k)]; + } + return s; +} +const a: number[] = []; +for (let i = 0; i < 64; i++) a.push(7.5 + i); +console.log("s:" + run(a)); +"#; + +/// The wrapped read must not happen: node's answer is NaN (the index is far +/// out of bounds in double arithmetic), and the wrap would print `s:7.5` — +/// element 0, in bounds, wrong. +#[test] +fn a_multiply_chain_that_wraps_i64_declines_to_the_generic_path() { + let dir = tempfile::tempdir().expect("tempdir"); + let entry = dir.path().join("main.ts"); + let output = dir.path().join("main_bin"); + std::fs::write(&entry, SOURCE).expect("write entry"); + let compile = Command::new(perry_bin()) + .current_dir(dir.path()) + .arg("compile") + .arg(&entry) + .arg("-o") + .arg(&output) + .env("PERRY_NO_CACHE", "1") + .output() + .expect("run perry compile"); + assert!( + compile.status.success(), + "perry compile failed\nstderr:\n{}", + String::from_utf8_lossy(&compile.stderr) + ); + for moving_gc in [false, true] { + let mut command = Command::new(&output); + command.current_dir(dir.path()); + if moving_gc { + command + .env("PERRY_GC_FORCE_EVACUATE", "1") + .env("PERRY_GC_VERIFY_EVACUATION", "1"); + } + let run = command.output().expect("run binary"); + assert!(run.status.success()); + assert_eq!( + String::from_utf8_lossy(&run.stdout), + "s:NaN\n", + "a wrapped affine index read an in-bounds element the generic path \ + never touches (moving_gc={moving_gc})" + ); + } +}