From 0419f45f36571804b549bacf6041c5bc6855781b Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 7 Oct 2025 15:19:02 +0100 Subject: [PATCH 1/5] Use unchecked operations for index calculations --- compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 006de36d3a0..a315faf8825 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -495,7 +495,10 @@ impl FunctionContext<'_> { // and we skipped inserting constraints. To stay on the conservative side, start with // a checked operation, and simplify to unchecked if it can be evaluated. // In Brillig we will be protected from overflows by constraints, so we can go unchecked. - let unchecked = runtime.is_brillig(); + // In ACIR the values are represented as Fields, so they don't wrap around, but ACIR has built-in OOB checks, + // so it's okay to use unchecked operations. The SSA interpreter has been updated to have similar semantics. + let unchecked = true; + let base_index = self.builder.set_location(location).insert_binary( index, BinaryOp::Mul { unchecked }, From 79819da638874dab9d64a5e26538a6431eebf5c5 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 7 Oct 2025 19:41:35 +0100 Subject: [PATCH 2/5] Update snapshot --- .../regression_9888/execute__tests__stderr.snap | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/tooling/nargo_cli/tests/snapshots/compile_success_with_bug/regression_9888/execute__tests__stderr.snap b/tooling/nargo_cli/tests/snapshots/compile_success_with_bug/regression_9888/execute__tests__stderr.snap index a2926e6fa0d..9959207eb63 100644 --- a/tooling/nargo_cli/tests/snapshots/compile_success_with_bug/regression_9888/execute__tests__stderr.snap +++ b/tooling/nargo_cli/tests/snapshots/compile_success_with_bug/regression_9888/execute__tests__stderr.snap @@ -51,7 +51,16 @@ warning: Unsafe block must have a safety comment above it │ ------ The comment must start with the "Safety: " word │ -bug: Assertion is always false: attempt to multiply with overflow +warning: Return variable contains a constant value + ┌─ src/main.nr:54:5 + │ +54 │ G_B.4 + │ ----- This variable contains a value which is constrained to be a constant. Consider removing this value as additional return values increase proving/verification time + │ + = Call stack: + 1. src/main.nr:54:5 + +bug: Assertion is always false: Index out of bounds ┌─ src/main.nr:22:60 │ 22 │ if (item_c.2 > unsafe { func_4([]) }[1532016929_u32].3) { From eb5a9f4b5ffe273c960fc8b1f5b51e285c06b81f Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 7 Oct 2025 20:45:14 +0100 Subject: [PATCH 3/5] Handle unfit values in indexing --- .../src/ssa/interpreter/mod.rs | 49 ++++++++++++++----- .../src/ssa/interpreter/value.rs | 15 ++++++ 2 files changed, 53 insertions(+), 11 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs b/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs index f8577b3dd89..b0bd3ec9706 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs @@ -1,4 +1,4 @@ -use std::{cmp::Ordering, collections::BTreeMap, io::Write}; +use std::{cmp::Ordering, collections::BTreeMap, io::Write, u32}; use super::{ Ssa, @@ -384,6 +384,32 @@ impl<'ssa, W: Write> Interpreter<'ssa, W> { self.lookup_helper(value_id, instruction, "array or slice", Value::as_array_or_slice) } + /// Look up an array index. + /// + /// If the value exists but it's `Unfit`, returns `IndexOutOfBounds` with `u32::MAX` as the index, + /// to hint at the likely overflow that happened during index arithmetic. + fn lookup_array_index( + &self, + value_id: ValueId, + instruction: &'static str, + length: u32, + ) -> IResult { + self.lookup_helper(value_id, instruction, "u32", Value::as_u32).map_err(|e| { + if !matches!(e, InterpreterError::Internal(InternalError::TypeError { .. })) { + return e; + } + if !self + .lookup(value_id) + .ok() + .and_then(|v| v.as_numeric()) + .map_or(false, |n| n.is_unfit()) + { + return e; + } + InterpreterError::IndexOutOfBounds { index: u32::MAX, length } + }) + } + fn lookup_bytes(&self, value_id: ValueId, instruction: &'static str) -> IResult> { let array = self.lookup_array_or_slice(value_id, instruction)?; let array = array.elements.borrow(); @@ -945,16 +971,17 @@ impl<'ssa, W: Write> Interpreter<'ssa, W> { ) -> IResult<()> { let offset = self.dfg().array_offset(array, index); let array = self.lookup_array_or_slice(array, "array get")?; - let index = self.lookup_u32(index, "array get index")?; + let length = array.elements.borrow().len() as u32; + let index = self.lookup_array_index(index, "array get index", length)?; let mut index = index - offset.to_u32(); - let element = if array.elements.borrow().is_empty() { + let element = if length == 0 { // Accessing an array of 0-len is replaced by asserting // the branch is not-taken during acir-gen and // a zeroed type is used in case of array get // So we can simply replace it with uninitialized value if side_effects_enabled { - return Err(InterpreterError::IndexOutOfBounds { index, length: 0 }); + return Err(InterpreterError::IndexOutOfBounds { index, length }); } else { let typ = self.dfg().type_of_value(result); Value::uninitialized(&typ, result) @@ -973,9 +1000,9 @@ impl<'ssa, W: Write> Interpreter<'ssa, W> { } } let elements = array.elements.borrow(); - let element = elements.get(index as usize).ok_or_else(|| { - InterpreterError::IndexOutOfBounds { index, length: elements.len() as u32 } - })?; + let element = elements + .get(index as usize) + .ok_or_else(|| InterpreterError::IndexOutOfBounds { index, length })?; // Either return a fresh nested array (in constrained context) or just clone the element. if !self.in_unconstrained_context() { @@ -1014,16 +1041,16 @@ impl<'ssa, W: Write> Interpreter<'ssa, W> { let array = self.lookup_array_or_slice(array, "array set")?; let result_array = if side_effects_enabled { - let index = self.lookup_u32(index, "array set index")?; + let length = array.elements.borrow().len() as u32; + let index = self.lookup_array_index(index, "array set index", length)?; let index = index - offset.to_u32(); let value = self.lookup(value)?; let should_mutate = if self.in_unconstrained_context() { *array.rc.borrow() == 1 } else { mutable }; - let len = array.elements.borrow().len(); - if index as usize >= len { - return Err(InterpreterError::IndexOutOfBounds { index, length: len as u32 }); + if index >= length { + return Err(InterpreterError::IndexOutOfBounds { index, length }); } if should_mutate { diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/value.rs b/compiler/noirc_evaluator/src/ssa/interpreter/value.rs index b7292898afc..8b5b4085b33 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/value.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/value.rs @@ -533,6 +533,21 @@ impl NumericValue { typ => Err(Internal(UnsupportedNumericType { typ })), } } + + pub(crate) fn is_unfit(&self) -> bool { + matches!( + self, + NumericValue::U8(Fitted::Unfit(_)) + | NumericValue::U16(Fitted::Unfit(_)) + | NumericValue::U32(Fitted::Unfit(_)) + | NumericValue::U64(Fitted::Unfit(_)) + | NumericValue::U128(Fitted::Unfit(_)) + | NumericValue::I8(Fitted::Unfit(_)) + | NumericValue::I16(Fitted::Unfit(_)) + | NumericValue::I32(Fitted::Unfit(_)) + | NumericValue::I64(Fitted::Unfit(_)) + ) + } } impl std::fmt::Display for Value { From 641765e560a8e95be7e673b699148caa7b891d98 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 7 Oct 2025 21:48:49 +0100 Subject: [PATCH 4/5] Clippy --- compiler/noirc_evaluator/src/ssa/interpreter/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs b/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs index b0bd3ec9706..032cbdaadfd 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs @@ -1,4 +1,4 @@ -use std::{cmp::Ordering, collections::BTreeMap, io::Write, u32}; +use std::{cmp::Ordering, collections::BTreeMap, io::Write}; use super::{ Ssa, @@ -402,7 +402,7 @@ impl<'ssa, W: Write> Interpreter<'ssa, W> { .lookup(value_id) .ok() .and_then(|v| v.as_numeric()) - .map_or(false, |n| n.is_unfit()) + .is_some_and(|n| n.is_unfit()) { return e; } @@ -1002,7 +1002,7 @@ impl<'ssa, W: Write> Interpreter<'ssa, W> { let elements = array.elements.borrow(); let element = elements .get(index as usize) - .ok_or_else(|| InterpreterError::IndexOutOfBounds { index, length })?; + .ok_or(InterpreterError::IndexOutOfBounds { index, length })?; // Either return a fresh nested array (in constrained context) or just clone the element. if !self.in_unconstrained_context() { From 869e546f522ea516202d4f95aef759e896f1955c Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 7 Oct 2025 23:33:59 +0100 Subject: [PATCH 5/5] Change IndexOutOfBounds::index to be a Field --- .../src/ssa/interpreter/errors.rs | 2 +- .../src/ssa/interpreter/mod.rs | 29 ++++++++----------- .../src/ssa/interpreter/value.rs | 15 ---------- 3 files changed, 13 insertions(+), 33 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/errors.rs b/compiler/noirc_evaluator/src/ssa/interpreter/errors.rs index 13a2e023d1c..640055a2abf 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/errors.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/errors.rs @@ -69,7 +69,7 @@ pub enum InterpreterError { #[error("Reached the unreachable")] ReachedTheUnreachable, #[error("Array index {index} is out of bounds for array of length {length}")] - IndexOutOfBounds { index: u32, length: u32 }, + IndexOutOfBounds { index: FieldElement, length: u32 }, } /// These errors can only result from interpreting malformed SSA diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs b/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs index 032cbdaadfd..62679937e6b 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs @@ -386,8 +386,7 @@ impl<'ssa, W: Write> Interpreter<'ssa, W> { /// Look up an array index. /// - /// If the value exists but it's `Unfit`, returns `IndexOutOfBounds` with `u32::MAX` as the index, - /// to hint at the likely overflow that happened during index arithmetic. + /// If the value exists but it's `Unfit`, returns `IndexOutOfBounds`. fn lookup_array_index( &self, value_id: ValueId, @@ -395,18 +394,14 @@ impl<'ssa, W: Write> Interpreter<'ssa, W> { length: u32, ) -> IResult { self.lookup_helper(value_id, instruction, "u32", Value::as_u32).map_err(|e| { - if !matches!(e, InterpreterError::Internal(InternalError::TypeError { .. })) { - return e; - } - if !self - .lookup(value_id) - .ok() - .and_then(|v| v.as_numeric()) - .is_some_and(|n| n.is_unfit()) - { - return e; - } - InterpreterError::IndexOutOfBounds { index: u32::MAX, length } + if matches!(e, InterpreterError::Internal(InternalError::TypeError { .. })) { + if let Ok(Value::Numeric(NumericValue::U32(Fitted::Unfit(index)))) = + self.lookup(value_id) + { + return InterpreterError::IndexOutOfBounds { index, length }; + } + } + e }) } @@ -981,7 +976,7 @@ impl<'ssa, W: Write> Interpreter<'ssa, W> { // a zeroed type is used in case of array get // So we can simply replace it with uninitialized value if side_effects_enabled { - return Err(InterpreterError::IndexOutOfBounds { index, length }); + return Err(InterpreterError::IndexOutOfBounds { index: index.into(), length }); } else { let typ = self.dfg().type_of_value(result); Value::uninitialized(&typ, result) @@ -1002,7 +997,7 @@ impl<'ssa, W: Write> Interpreter<'ssa, W> { let elements = array.elements.borrow(); let element = elements .get(index as usize) - .ok_or(InterpreterError::IndexOutOfBounds { index, length })?; + .ok_or(InterpreterError::IndexOutOfBounds { index: index.into(), length })?; // Either return a fresh nested array (in constrained context) or just clone the element. if !self.in_unconstrained_context() { @@ -1050,7 +1045,7 @@ impl<'ssa, W: Write> Interpreter<'ssa, W> { if self.in_unconstrained_context() { *array.rc.borrow() == 1 } else { mutable }; if index >= length { - return Err(InterpreterError::IndexOutOfBounds { index, length }); + return Err(InterpreterError::IndexOutOfBounds { index: index.into(), length }); } if should_mutate { diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/value.rs b/compiler/noirc_evaluator/src/ssa/interpreter/value.rs index 8b5b4085b33..b7292898afc 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/value.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/value.rs @@ -533,21 +533,6 @@ impl NumericValue { typ => Err(Internal(UnsupportedNumericType { typ })), } } - - pub(crate) fn is_unfit(&self) -> bool { - matches!( - self, - NumericValue::U8(Fitted::Unfit(_)) - | NumericValue::U16(Fitted::Unfit(_)) - | NumericValue::U32(Fitted::Unfit(_)) - | NumericValue::U64(Fitted::Unfit(_)) - | NumericValue::U128(Fitted::Unfit(_)) - | NumericValue::I8(Fitted::Unfit(_)) - | NumericValue::I16(Fitted::Unfit(_)) - | NumericValue::I32(Fitted::Unfit(_)) - | NumericValue::I64(Fitted::Unfit(_)) - ) - } } impl std::fmt::Display for Value {