From 861cfddcd939a0eac5314b5475da242767af77e6 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 20 Oct 2023 14:39:05 -0500 Subject: [PATCH 1/4] Fix #3227 --- compiler/noirc_evaluator/src/errors.rs | 7 +- compiler/noirc_evaluator/src/ssa.rs | 7 +- .../src/ssa/function_builder/mod.rs | 4 + compiler/noirc_evaluator/src/ssa/ir/types.rs | 22 +- .../src/ssa/ssa_gen/context.rs | 82 +++++-- .../noirc_evaluator/src/ssa/ssa_gen/mod.rs | 218 +++++++++--------- .../src/monomorphization/ast.rs | 2 +- .../src/monomorphization/mod.rs | 36 +-- .../src/monomorphization/printer.rs | 2 +- 9 files changed, 234 insertions(+), 146 deletions(-) diff --git a/compiler/noirc_evaluator/src/errors.rs b/compiler/noirc_evaluator/src/errors.rs index 3dc0194c8be..58d13c0affd 100644 --- a/compiler/noirc_evaluator/src/errors.rs +++ b/compiler/noirc_evaluator/src/errors.rs @@ -7,12 +7,12 @@ //! An Error of the former is a user Error //! //! An Error of the latter is an error in the implementation of the compiler -use acvm::acir::native_types::Expression; +use acvm::{acir::native_types::Expression, FieldElement}; use iter_extended::vecmap; use noirc_errors::{CustomDiagnostic as Diagnostic, FileDiagnostic}; use thiserror::Error; -use crate::ssa::ir::dfg::CallStack; +use crate::ssa::ir::{dfg::CallStack, types::NumericType}; #[derive(Debug, PartialEq, Eq, Clone, Error)] pub enum RuntimeError { @@ -29,6 +29,8 @@ pub enum RuntimeError { IndexOutOfBounds { index: usize, array_size: usize, call_stack: CallStack }, #[error("Range constraint of {num_bits} bits is too large for the Field size")] InvalidRangeConstraint { num_bits: u32, call_stack: CallStack }, + #[error("{value} does not fit within the type bounds for {typ}")] + IntegerOutOfBounds { value: FieldElement, typ: NumericType, call_stack: CallStack }, #[error("Expected array index to fit into a u64")] TypeConversion { from: String, into: String, call_stack: CallStack }, #[error("{name:?} is not initialized")] @@ -91,6 +93,7 @@ impl RuntimeError { | RuntimeError::UnInitialized { call_stack, .. } | RuntimeError::UnknownLoopBound { call_stack } | RuntimeError::AssertConstantFailed { call_stack } + | RuntimeError::IntegerOutOfBounds { call_stack, .. } | RuntimeError::UnsupportedIntegerSize { call_stack, .. } => call_stack, } } diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 131cf30a510..7cf51a4f0b3 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -39,7 +39,7 @@ pub(crate) fn optimize_into_acir( print_brillig_trace: bool, ) -> Result { let abi_distinctness = program.return_distinctness; - let ssa = SsaBuilder::new(program, print_ssa_passes) + let ssa = SsaBuilder::new(program, print_ssa_passes)? .run_pass(Ssa::defunctionalize, "After Defunctionalization:") .run_pass(Ssa::inline_functions, "After Inlining:") // Run mem2reg with the CFG separated into blocks @@ -129,8 +129,9 @@ struct SsaBuilder { } impl SsaBuilder { - fn new(program: Program, print_ssa_passes: bool) -> SsaBuilder { - SsaBuilder { print_ssa_passes, ssa: ssa_gen::generate_ssa(program) }.print("Initial SSA:") + fn new(program: Program, print_ssa_passes: bool) -> Result { + let ssa = ssa_gen::generate_ssa(program)?; + Ok(SsaBuilder { print_ssa_passes, ssa }.print("Initial SSA:")) } fn finish(self) -> Ssa { diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index 546a614a27f..155e5a7baba 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -183,6 +183,10 @@ impl FunctionBuilder { self } + pub(crate) fn get_call_stack(&self) -> CallStack { + self.call_stack.clone() + } + /// Insert a Load instruction at the end of the current block, loading from the given offset /// of the given address which should point to a previous Allocate instruction. Note that /// this is limited to loading a single value. Loading multiple values (such as a tuple) diff --git a/compiler/noirc_evaluator/src/ssa/ir/types.rs b/compiler/noirc_evaluator/src/ssa/ir/types.rs index b576ee12d45..b9d44acca4e 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/types.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/types.rs @@ -1,5 +1,6 @@ use std::rc::Rc; +use acvm::FieldElement; use iter_extended::vecmap; /// A numeric type in the Intermediate representation @@ -11,7 +12,7 @@ use iter_extended::vecmap; /// Fields do not have a notion of ordering, so this distinction /// is reasonable. #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash, Ord, PartialOrd)] -pub(crate) enum NumericType { +pub enum NumericType { Signed { bit_size: u32 }, Unsigned { bit_size: u32 }, NativeField, @@ -94,6 +95,25 @@ impl Type { } } +impl NumericType { + /// Returns the numeric limits of this type if it is an integer type. + /// If this is a Field, None is returned. + pub(crate) fn get_limits(self) -> Option<(FieldElement, FieldElement)> { + match self { + NumericType::Signed { bit_size } => { + let min = -(2i128.pow(bit_size - 1)); + let max = 2u128.pow(bit_size - 1) - 1; + Some((min.into(), max.into())) + } + NumericType::Unsigned { bit_size } => { + let max = 2u128.pow(bit_size) - 1; + Some((FieldElement::zero(), max.into())) + } + NumericType::NativeField => None, // Should we check the field size as well? + } + } +} + /// Composite Types are essentially flattened struct or tuple types. /// Array types may have these as elements where each flattened field is /// included in the array sequentially. diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 142e9f81397..b6352879553 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -8,6 +8,7 @@ use noirc_frontend::monomorphization::ast::{self, LocalId, Parameters}; use noirc_frontend::monomorphization::ast::{FuncId, Program}; use noirc_frontend::{BinaryOpKind, Signedness}; +use crate::errors::RuntimeError; use crate::ssa::function_builder::FunctionBuilder; use crate::ssa::ir::dfg::DataFlowGraph; use crate::ssa::ir::function::FunctionId as IrFunctionId; @@ -240,6 +241,32 @@ impl<'a> FunctionContext<'a> { Values::empty() } + /// Insert a numeric constant into the current function + /// + /// Unlike FunctionBuilder::numeric_constant, this version checks the given constant + /// is within the range of the given type. This is needed for user provided values where + /// otherwise values like 2^128 can be assigned to a u8 without error or wrapping. + pub(super) fn checked_numeric_constant( + &mut self, + value: impl Into, + typ: Type, + ) -> Result { + let value = value.into(); + + if let Type::Numeric(typ) = typ { + if let Some((lower_bound, upper_bound)) = typ.get_limits() { + if !(lower_bound <= value && value <= upper_bound) { + let call_stack = self.builder.get_call_stack(); + return Err(RuntimeError::IntegerOutOfBounds { value, typ, call_stack }); + } + } + } else { + panic!("Expected type for numeric constant to be a numeric type, found {typ}"); + } + + Ok(self.builder.numeric_constant(value, typ)) + } + /// Insert ssa instructions which computes lhs << rhs by doing lhs*2^rhs fn insert_shift_left(&mut self, lhs: ValueId, rhs: ValueId) -> ValueId { let base = self.builder.field_constant(FieldElement::from(2_u128)); @@ -544,8 +571,11 @@ impl<'a> FunctionContext<'a> { /// This is operationally equivalent to extract_current_value_recursive, but splitting these /// into two separate functions avoids cloning the outermost `Values` returned by the recursive /// version, as it is only needed for recursion. - pub(super) fn extract_current_value(&mut self, lvalue: &ast::LValue) -> LValue { - match lvalue { + pub(super) fn extract_current_value( + &mut self, + lvalue: &ast::LValue, + ) -> Result { + Ok(match lvalue { ast::LValue::Ident(ident) => { let (reference, should_auto_deref) = self.ident_lvalue(ident); if should_auto_deref { @@ -555,18 +585,18 @@ impl<'a> FunctionContext<'a> { } } ast::LValue::Index { array, index, location, .. } => { - self.index_lvalue(array, index, location).2 + self.index_lvalue(array, index, location)?.2 } ast::LValue::MemberAccess { object, field_index } => { - let (old_object, object_lvalue) = self.extract_current_value_recursive(object); + let (old_object, object_lvalue) = self.extract_current_value_recursive(object)?; let object_lvalue = Box::new(object_lvalue); LValue::MemberAccess { old_object, object_lvalue, index: *field_index } } ast::LValue::Dereference { reference, .. } => { - let (reference, _) = self.extract_current_value_recursive(reference); + let (reference, _) = self.extract_current_value_recursive(reference)?; LValue::Dereference { reference } } - } + }) } fn dereference_lvalue(&mut self, values: &Values, element_type: &ast::Type) -> Values { @@ -596,16 +626,16 @@ impl<'a> FunctionContext<'a> { array: &ast::LValue, index: &ast::Expression, location: &Location, - ) -> (ValueId, ValueId, LValue, Option) { - let (old_array, array_lvalue) = self.extract_current_value_recursive(array); - let index = self.codegen_non_tuple_expression(index); + ) -> Result<(ValueId, ValueId, LValue, Option), RuntimeError> { + let (old_array, array_lvalue) = self.extract_current_value_recursive(array)?; + let index = self.codegen_non_tuple_expression(index)?; let array_lvalue = Box::new(array_lvalue); let array_values = old_array.clone().into_value_list(self); let location = *location; // A slice is represented as a tuple (length, slice contents). // We need to fetch the second value. - if array_values.len() > 1 { + Ok(if array_values.len() > 1 { let slice_lvalue = LValue::SliceIndex { old_slice: old_array, index, @@ -617,37 +647,45 @@ impl<'a> FunctionContext<'a> { let array_lvalue = LValue::Index { old_array: array_values[0], index, array_lvalue, location }; (array_values[0], index, array_lvalue, None) - } + }) } - fn extract_current_value_recursive(&mut self, lvalue: &ast::LValue) -> (Values, LValue) { + fn extract_current_value_recursive( + &mut self, + lvalue: &ast::LValue, + ) -> Result<(Values, LValue), RuntimeError> { match lvalue { ast::LValue::Ident(ident) => { let (variable, should_auto_deref) = self.ident_lvalue(ident); if should_auto_deref { let dereferenced = self.dereference_lvalue(&variable, &ident.typ); - (dereferenced, LValue::Dereference { reference: variable }) + Ok((dereferenced, LValue::Dereference { reference: variable })) } else { - (variable.clone(), LValue::Ident) + Ok((variable.clone(), LValue::Ident)) } } ast::LValue::Index { array, index, element_type, location } => { let (old_array, index, index_lvalue, max_length) = - self.index_lvalue(array, index, location); - let element = - self.codegen_array_index(old_array, index, element_type, *location, max_length); - (element, index_lvalue) + self.index_lvalue(array, index, location)?; + let element = self.codegen_array_index( + old_array, + index, + element_type, + *location, + max_length, + )?; + Ok((element, index_lvalue)) } ast::LValue::MemberAccess { object, field_index: index } => { - let (old_object, object_lvalue) = self.extract_current_value_recursive(object); + let (old_object, object_lvalue) = self.extract_current_value_recursive(object)?; let object_lvalue = Box::new(object_lvalue); let element = Self::get_field_ref(&old_object, *index).clone(); - (element, LValue::MemberAccess { old_object, object_lvalue, index: *index }) + Ok((element, LValue::MemberAccess { old_object, object_lvalue, index: *index })) } ast::LValue::Dereference { reference, element_type } => { - let (reference, _) = self.extract_current_value_recursive(reference); + let (reference, _) = self.extract_current_value_recursive(reference)?; let dereferenced = self.dereference_lvalue(&reference, element_type); - (dereferenced, LValue::Dereference { reference }) + Ok((dereferenced, LValue::Dereference { reference })) } } } diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index d990a95c540..7677f5669cb 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -5,14 +5,17 @@ mod value; pub(crate) use program::Ssa; use context::SharedContext; -use iter_extended::vecmap; +use iter_extended::{try_vecmap, vecmap}; use noirc_errors::Location; use noirc_frontend::{ monomorphization::ast::{self, Binary, Expression, Program}, BinaryOpKind, }; -use crate::ssa::ir::{instruction::Intrinsic, types::NumericType}; +use crate::{ + errors::RuntimeError, + ssa::ir::{instruction::Intrinsic, types::NumericType}, +}; use self::{ context::FunctionContext, @@ -29,7 +32,7 @@ use super::ir::{ /// Generates SSA for the given monomorphized program. /// /// This function will generate the SSA but does not perform any optimizations on it. -pub(crate) fn generate_ssa(program: Program) -> Ssa { +pub(crate) fn generate_ssa(program: Program) -> Result { let return_location = program.return_location; let context = SharedContext::new(program); @@ -45,7 +48,7 @@ pub(crate) fn generate_ssa(program: Program) -> Ssa { if main.unconstrained { RuntimeType::Brillig } else { RuntimeType::Acir }, &context, ); - function_context.codegen_function_body(&main.body); + function_context.codegen_function_body(&main.body)?; if let Some(return_location) = return_location { let block = function_context.builder.current_block(); @@ -69,24 +72,25 @@ pub(crate) fn generate_ssa(program: Program) -> Ssa { while let Some((src_function_id, dest_id)) = context.pop_next_function_in_queue() { let function = &context.program[src_function_id]; function_context.new_function(dest_id, function); - function_context.codegen_function_body(&function.body); + function_context.codegen_function_body(&function.body)?; } - function_context.builder.finish() + Ok(function_context.builder.finish()) } impl<'a> FunctionContext<'a> { /// Codegen a function's body and set its return value to that of its last parameter. /// For functions returning nothing, this will be an empty list. - fn codegen_function_body(&mut self, body: &Expression) { - let return_value = self.codegen_expression(body); + fn codegen_function_body(&mut self, body: &Expression) -> Result<(), RuntimeError> { + let return_value = self.codegen_expression(body)?; let results = return_value.into_value_list(self); self.builder.terminate_with_return(results); + Ok(()) } - fn codegen_expression(&mut self, expr: &Expression) -> Values { + fn codegen_expression(&mut self, expr: &Expression) -> Result { match expr { - Expression::Ident(ident) => self.codegen_ident(ident), + Expression::Ident(ident) => Ok(self.codegen_ident(ident)), Expression::Literal(literal) => self.codegen_literal(literal), Expression::Block(block) => self.codegen_block(block), Expression::Unary(unary) => self.codegen_unary(unary), @@ -111,8 +115,8 @@ impl<'a> FunctionContext<'a> { /// Codegen any non-tuple expression so that we can unwrap the Values /// tree to return a single value for use with most SSA instructions. - fn codegen_non_tuple_expression(&mut self, expr: &Expression) -> ValueId { - self.codegen_expression(expr).into_leaf().eval(self) + fn codegen_non_tuple_expression(&mut self, expr: &Expression) -> Result { + Ok(self.codegen_expression(expr)?.into_leaf().eval(self)) } /// Codegen a reference to an ident. @@ -140,17 +144,19 @@ impl<'a> FunctionContext<'a> { self.codegen_ident_reference(ident).map(|value| value.eval(self).into()) } - fn codegen_literal(&mut self, literal: &ast::Literal) -> Values { + fn codegen_literal(&mut self, literal: &ast::Literal) -> Result { match literal { ast::Literal::Array(array) => { - let elements = vecmap(&array.contents, |element| self.codegen_expression(element)); + let elements = + try_vecmap(&array.contents, |element| self.codegen_expression(element))?; let typ = Self::convert_type(&array.typ).flatten(); - match array.typ { + Ok(match array.typ { ast::Type::Array(_, _) => self.codegen_array(elements, typ[0].clone()), ast::Type::Slice(_) => { let slice_length = self.builder.field_constant(array.contents.len() as u128); + let slice_contents = self.codegen_array(elements, typ[1].clone()); Tree::Branch(vec![slice_length.into(), slice_contents]) } @@ -158,37 +164,37 @@ impl<'a> FunctionContext<'a> { "ICE: array literal type must be an array or a slice, but got {}", array.typ ), - } + }) } - ast::Literal::Integer(value, typ) => { + ast::Literal::Integer(value, typ, location) => { + self.builder.set_location(*location); let typ = Self::convert_non_tuple_type(typ); - self.builder.numeric_constant(*value, typ).into() + self.checked_numeric_constant(*value, typ).map(Into::into) } ast::Literal::Bool(value) => { - self.builder.numeric_constant(*value as u128, Type::bool()).into() - } - ast::Literal::Str(string) => { - let elements = vecmap(string.as_bytes(), |byte| { - self.builder.numeric_constant(*byte as u128, Type::field()).into() - }); - let typ = Self::convert_non_tuple_type(&ast::Type::String(elements.len() as u64)); - self.codegen_array(elements, typ) + // Don't need to call checked_numeric_constant here since `value` can only be true or false + Ok(self.builder.numeric_constant(*value as u128, Type::bool()).into()) } + ast::Literal::Str(string) => Ok(self.codegen_string(string)), ast::Literal::FmtStr(string, number_of_fields, fields) => { // A caller needs multiple pieces of information to make use of a format string // The message string, the number of fields to be formatted, and the fields themselves - let string = Expression::Literal(ast::Literal::Str(string.clone())); - let number_of_fields = Expression::Literal(ast::Literal::Integer( - (*number_of_fields as u128).into(), - ast::Type::Field, - )); - let fields = *fields.clone(); - let fmt_str_tuple = &[string, number_of_fields, fields]; - self.codegen_tuple(fmt_str_tuple) + let string = self.codegen_string(string); + let field_count = self.builder.field_constant(*number_of_fields as u128); + let fields = self.codegen_expression(fields)?; + + Ok(Tree::Branch(vec![string, field_count.into(), fields])) } } } + fn codegen_string(&mut self, string: &str) -> Values { + let elements = + vecmap(string.as_bytes(), |byte| self.builder.field_constant(*byte as u128).into()); + let typ = Self::convert_non_tuple_type(&ast::Type::String(elements.len() as u64)); + self.codegen_array(elements, typ) + } + /// Codegen an array by allocating enough space for each element and inserting separate /// store instructions until each element is stored. The store instructions will be separated /// by add instructions to calculate the new offset address to store to next. @@ -211,35 +217,35 @@ impl<'a> FunctionContext<'a> { self.builder.array_constant(array, typ).into() } - fn codegen_block(&mut self, block: &[Expression]) -> Values { + fn codegen_block(&mut self, block: &[Expression]) -> Result { let mut result = Self::unit_value(); for expr in block { - result = self.codegen_expression(expr); + result = self.codegen_expression(expr)?; } - result + Ok(result) } - fn codegen_unary(&mut self, unary: &ast::Unary) -> Values { + fn codegen_unary(&mut self, unary: &ast::Unary) -> Result { match unary.operator { noirc_frontend::UnaryOp::Not => { - let rhs = self.codegen_expression(&unary.rhs); + let rhs = self.codegen_expression(&unary.rhs)?; let rhs = rhs.into_leaf().eval(self); - self.builder.insert_not(rhs).into() + Ok(self.builder.insert_not(rhs).into()) } noirc_frontend::UnaryOp::Minus => { - let rhs = self.codegen_expression(&unary.rhs); + let rhs = self.codegen_expression(&unary.rhs)?; let rhs = rhs.into_leaf().eval(self); let typ = self.builder.type_of_value(rhs); let zero = self.builder.numeric_constant(0u128, typ); - self.insert_binary( + Ok(self.insert_binary( zero, noirc_frontend::BinaryOpKind::Subtract, rhs, unary.location, - ) + )) } noirc_frontend::UnaryOp::MutableReference => { - self.codegen_reference(&unary.rhs).map(|rhs| { + Ok(self.codegen_reference(&unary.rhs)?.map(|rhs| { match rhs { value::Value::Normal(value) => { let alloc = self.builder.insert_allocate(); @@ -250,11 +256,11 @@ impl<'a> FunctionContext<'a> { // a Value::Normal so it is no longer automatically dereferenced. value::Value::Mutable(reference, _) => reference.into(), } - }) + })) } noirc_frontend::UnaryOp::Dereference { .. } => { - let rhs = self.codegen_expression(&unary.rhs); - self.dereference(&rhs, &unary.result_type) + let rhs = self.codegen_expression(&unary.rhs)?; + Ok(self.dereference(&rhs, &unary.result_type)) } } } @@ -267,26 +273,26 @@ impl<'a> FunctionContext<'a> { }) } - fn codegen_reference(&mut self, expr: &Expression) -> Values { + fn codegen_reference(&mut self, expr: &Expression) -> Result { match expr { - Expression::Ident(ident) => self.codegen_ident_reference(ident), + Expression::Ident(ident) => Ok(self.codegen_ident_reference(ident)), Expression::ExtractTupleField(tuple, index) => { - let tuple = self.codegen_reference(tuple); - Self::get_field(tuple, *index) + let tuple = self.codegen_reference(tuple)?; + Ok(Self::get_field(tuple, *index)) } other => self.codegen_expression(other), } } - fn codegen_binary(&mut self, binary: &ast::Binary) -> Values { - let lhs = self.codegen_non_tuple_expression(&binary.lhs); - let rhs = self.codegen_non_tuple_expression(&binary.rhs); - self.insert_binary(lhs, binary.operator, rhs, binary.location) + fn codegen_binary(&mut self, binary: &ast::Binary) -> Result { + let lhs = self.codegen_non_tuple_expression(&binary.lhs)?; + let rhs = self.codegen_non_tuple_expression(&binary.rhs)?; + Ok(self.insert_binary(lhs, binary.operator, rhs, binary.location)) } - fn codegen_index(&mut self, index: &ast::Index) -> Values { - let array_or_slice = self.codegen_expression(&index.collection).into_value_list(self); - let index_value = self.codegen_non_tuple_expression(&index.index); + fn codegen_index(&mut self, index: &ast::Index) -> Result { + let array_or_slice = self.codegen_expression(&index.collection)?.into_value_list(self); + let index_value = self.codegen_non_tuple_expression(&index.index)?; // Slices are represented as a tuple in the form: (length, slice contents). // Thus, slices require two value ids for their representation. let (array, slice_length) = if array_or_slice.len() > 1 { @@ -316,7 +322,7 @@ impl<'a> FunctionContext<'a> { element_type: &ast::Type, location: Location, length: Option, - ) -> Values { + ) -> Result { // base_index = index * type_size let type_size = Self::convert_type(element_type).size_of_type(); let type_size = self.builder.field_constant(type_size as u128); @@ -324,7 +330,7 @@ impl<'a> FunctionContext<'a> { self.builder.set_location(location).insert_binary(index, BinaryOp::Mul, type_size); let mut field_index = 0u128; - Self::map_type(element_type, |typ| { + Ok(Self::map_type(element_type, |typ| { let offset = self.make_offset(base_index, field_index); field_index += 1; @@ -339,7 +345,7 @@ impl<'a> FunctionContext<'a> { _ => unreachable!("must have array or slice but got {array_type}"), } self.builder.insert_array_get(array, offset, typ).into() - }) + })) } /// Prepare a slice access. @@ -374,11 +380,11 @@ impl<'a> FunctionContext<'a> { ); } - fn codegen_cast(&mut self, cast: &ast::Cast) -> Values { - let lhs = self.codegen_non_tuple_expression(&cast.lhs); + fn codegen_cast(&mut self, cast: &ast::Cast) -> Result { + let lhs = self.codegen_non_tuple_expression(&cast.lhs)?; let typ = Self::convert_non_tuple_type(&cast.r#type); self.builder.set_location(cast.location); - self.builder.insert_cast(lhs, typ).into() + Ok(self.builder.insert_cast(lhs, typ).into()) } /// Codegens a for loop, creating three new blocks in the process. @@ -398,7 +404,7 @@ impl<'a> FunctionContext<'a> { /// br loop_entry(v4) /// loop_end(): /// ... This is the current insert point after codegen_for finishes ... - fn codegen_for(&mut self, for_expr: &ast::For) -> Values { + fn codegen_for(&mut self, for_expr: &ast::For) -> Result { let loop_entry = self.builder.insert_block(); let loop_body = self.builder.insert_block(); let loop_end = self.builder.insert_block(); @@ -408,10 +414,10 @@ impl<'a> FunctionContext<'a> { let loop_index = self.builder.add_block_parameter(loop_entry, index_type); self.builder.set_location(for_expr.start_range_location); - let start_index = self.codegen_non_tuple_expression(&for_expr.start_range); + let start_index = self.codegen_non_tuple_expression(&for_expr.start_range)?; self.builder.set_location(for_expr.end_range_location); - let end_index = self.codegen_non_tuple_expression(&for_expr.end_range); + let end_index = self.codegen_non_tuple_expression(&for_expr.end_range)?; // Set the location of the initial jmp instruction to the start range. This is the location // used to issue an error if the start range cannot be determined at compile-time. @@ -431,13 +437,13 @@ impl<'a> FunctionContext<'a> { // Compile the loop body self.builder.switch_to_block(loop_body); self.define(for_expr.index_variable, loop_index.into()); - self.codegen_expression(&for_expr.block); + self.codegen_expression(&for_expr.block)?; let new_loop_index = self.make_offset(loop_index, 1); self.builder.terminate_with_jmp(loop_entry, vec![new_loop_index]); // Finish by switching back to the end of the loop self.builder.switch_to_block(loop_end); - Self::unit_value() + Ok(Self::unit_value()) } /// Codegens an if expression, handling the case of what to do if there is no 'else'. @@ -464,8 +470,8 @@ impl<'a> FunctionContext<'a> { /// br end_if() /// end_if: // No block parameter is needed. Without an else, the unit value is always returned. /// ... This is the current insert point after codegen_if finishes ... - fn codegen_if(&mut self, if_expr: &ast::If) -> Values { - let condition = self.codegen_non_tuple_expression(&if_expr.condition); + fn codegen_if(&mut self, if_expr: &ast::If) -> Result { + let condition = self.codegen_non_tuple_expression(&if_expr.condition)?; let then_block = self.builder.insert_block(); let else_block = self.builder.insert_block(); @@ -473,7 +479,7 @@ impl<'a> FunctionContext<'a> { self.builder.terminate_with_jmpif(condition, then_block, else_block); self.builder.switch_to_block(then_block); - let then_value = self.codegen_expression(&if_expr.consequence); + let then_value = self.codegen_expression(&if_expr.consequence)?; let mut result = Self::unit_value(); @@ -483,7 +489,7 @@ impl<'a> FunctionContext<'a> { self.builder.terminate_with_jmp(end_block, then_values); self.builder.switch_to_block(else_block); - let else_value = self.codegen_expression(alternative); + let else_value = self.codegen_expression(alternative)?; let else_values = else_value.into_value_list(self); self.builder.terminate_with_jmp(end_block, else_values); @@ -501,31 +507,36 @@ impl<'a> FunctionContext<'a> { self.builder.switch_to_block(else_block); } - result + Ok(result) } - fn codegen_tuple(&mut self, tuple: &[Expression]) -> Values { - Tree::Branch(vecmap(tuple, |expr| self.codegen_expression(expr))) + fn codegen_tuple(&mut self, tuple: &[Expression]) -> Result { + Ok(Tree::Branch(try_vecmap(tuple, |expr| self.codegen_expression(expr))?)) } - fn codegen_extract_tuple_field(&mut self, tuple: &Expression, field_index: usize) -> Values { - let tuple = self.codegen_expression(tuple); - Self::get_field(tuple, field_index) + fn codegen_extract_tuple_field( + &mut self, + tuple: &Expression, + field_index: usize, + ) -> Result { + let tuple = self.codegen_expression(tuple)?; + Ok(Self::get_field(tuple, field_index)) } /// Generate SSA for a function call. Note that calls to built-in functions /// and intrinsics are also represented by the function call instruction. - fn codegen_call(&mut self, call: &ast::Call) -> Values { - let function = self.codegen_non_tuple_expression(&call.func); - let arguments = call - .arguments - .iter() - .flat_map(|argument| self.codegen_expression(argument).into_value_list(self)) - .collect::>(); + fn codegen_call(&mut self, call: &ast::Call) -> Result { + let function = self.codegen_non_tuple_expression(&call.func)?; + let mut arguments = Vec::with_capacity(call.arguments.len()); + + for argument in &call.arguments { + let mut values = self.codegen_expression(argument)?.into_value_list(self); + arguments.append(&mut values); + } self.codegen_intrinsic_call_checks(function, &arguments, call.location); - self.insert_call(function, arguments, &call.return_type, call.location) + Ok(self.insert_call(function, arguments, &call.return_type, call.location)) } fn codegen_intrinsic_call_checks( @@ -539,7 +550,8 @@ impl<'a> FunctionContext<'a> { { match intrinsic { Intrinsic::SliceInsert => { - let one = self.builder.numeric_constant(1u128, Type::field()); + let one = self.builder.field_constant(1u128); + // We add one here in the case of a slice insert as a slice insert at the length of the slice // can be converted to a slice push back let len_plus_one = self.builder.insert_binary(arguments[0], BinaryOp::Add, one); @@ -560,8 +572,8 @@ impl<'a> FunctionContext<'a> { /// If the variable is immutable, no special handling is necessary and we can return the given /// ValueId directly. If it is mutable, we'll need to allocate space for the value and store /// the initial value before returning the allocate instruction. - fn codegen_let(&mut self, let_expr: &ast::Let) -> Values { - let mut values = self.codegen_expression(&let_expr.expression); + fn codegen_let(&mut self, let_expr: &ast::Let) -> Result { + let mut values = self.codegen_expression(&let_expr.expression)?; if let_expr.mutable { values = values.map(|value| { @@ -571,7 +583,7 @@ impl<'a> FunctionContext<'a> { } self.define(let_expr.id, values); - Self::unit_value() + Ok(Self::unit_value()) } fn codegen_constrain( @@ -579,17 +591,17 @@ impl<'a> FunctionContext<'a> { expr: &Expression, location: Location, assert_message: Option, - ) -> Values { + ) -> Result { match expr { // If we're constraining an equality to be true then constrain the two sides directly. Expression::Binary(Binary { lhs, operator: BinaryOpKind::Equal, rhs, .. }) => { - let lhs = self.codegen_non_tuple_expression(lhs); - let rhs = self.codegen_non_tuple_expression(rhs); + let lhs = self.codegen_non_tuple_expression(lhs)?; + let rhs = self.codegen_non_tuple_expression(rhs)?; self.builder.set_location(location).insert_constrain(lhs, rhs, assert_message); } _ => { - let expr = self.codegen_non_tuple_expression(expr); + let expr = self.codegen_non_tuple_expression(expr)?; let true_literal = self.builder.numeric_constant(true, Type::bool()); self.builder.set_location(location).insert_constrain( expr, @@ -598,19 +610,19 @@ impl<'a> FunctionContext<'a> { ); } } - Self::unit_value() + Ok(Self::unit_value()) } - fn codegen_assign(&mut self, assign: &ast::Assign) -> Values { - let lhs = self.extract_current_value(&assign.lvalue); - let rhs = self.codegen_expression(&assign.expression); + fn codegen_assign(&mut self, assign: &ast::Assign) -> Result { + let lhs = self.extract_current_value(&assign.lvalue)?; + let rhs = self.codegen_expression(&assign.expression)?; self.assign_new_value(lhs, rhs); - Self::unit_value() + Ok(Self::unit_value()) } - fn codegen_semi(&mut self, expr: &Expression) -> Values { - self.codegen_expression(expr); - Self::unit_value() + fn codegen_semi(&mut self, expr: &Expression) -> Result { + self.codegen_expression(expr)?; + Ok(Self::unit_value()) } } diff --git a/compiler/noirc_frontend/src/monomorphization/ast.rs b/compiler/noirc_frontend/src/monomorphization/ast.rs index c67b8f8bcec..0a005d766fe 100644 --- a/compiler/noirc_frontend/src/monomorphization/ast.rs +++ b/compiler/noirc_frontend/src/monomorphization/ast.rs @@ -81,7 +81,7 @@ pub struct For { #[derive(Debug, Clone, Hash)] pub enum Literal { Array(ArrayLiteral), - Integer(FieldElement, Type), + Integer(FieldElement, Type, Location), Bool(bool), Str(String), FmtStr(String, u64, Box), diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index 764159a4df3..6891bcf2872 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -312,7 +312,8 @@ impl<'interner> Monomorphizer<'interner> { HirExpression::Literal(HirLiteral::Bool(value)) => Literal(Bool(value)), HirExpression::Literal(HirLiteral::Integer(value)) => { let typ = self.convert_type(&self.interner.id_type(expr)); - Literal(Integer(value, typ)) + let location = self.interner.id_location(expr); + Literal(Integer(value, typ, location)) } HirExpression::Literal(HirLiteral::Array(array)) => match array { HirArrayLiteral::Standard(array) => self.standard_array(expr, array), @@ -672,7 +673,8 @@ impl<'interner> Monomorphizer<'interner> { }; let value = FieldElement::from(value as u128); - ast::Expression::Literal(ast::Literal::Integer(value, ast::Type::Field)) + let location = self.interner.id_location(expr_id); + ast::Expression::Literal(ast::Literal::Integer(value, ast::Type::Field, location)) } } } @@ -990,30 +992,32 @@ impl<'interner> Monomorphizer<'interner> { if let ast::Expression::Ident(ident) = func { if let Definition::Builtin(opcode) = &ident.definition { // TODO(#1736): Move this builtin to the SSA pass + let location = self.interner.expr_location(expr_id); return match opcode.as_str() { - "modulus_num_bits" => Some(ast::Expression::Literal(ast::Literal::Integer( - (FieldElement::max_num_bits() as u128).into(), - ast::Type::Field, - ))), + "modulus_num_bits" => { + let bits = (FieldElement::max_num_bits() as u128).into(); + let typ = ast::Type::Field; + Some(ast::Expression::Literal(ast::Literal::Integer(bits, typ, location))) + } "zeroed" => { let location = self.interner.expr_location(expr_id); Some(self.zeroed_value_of_type(result_type, location)) } "modulus_le_bits" => { let bits = FieldElement::modulus().to_radix_le(2); - Some(self.modulus_array_literal(bits, 1)) + Some(self.modulus_array_literal(bits, 1, location)) } "modulus_be_bits" => { let bits = FieldElement::modulus().to_radix_be(2); - Some(self.modulus_array_literal(bits, 1)) + Some(self.modulus_array_literal(bits, 1, location)) } "modulus_be_bytes" => { let bytes = FieldElement::modulus().to_bytes_be(); - Some(self.modulus_array_literal(bytes, 8)) + Some(self.modulus_array_literal(bytes, 8, location)) } "modulus_le_bytes" => { let bytes = FieldElement::modulus().to_bytes_le(); - Some(self.modulus_array_literal(bytes, 8)) + Some(self.modulus_array_literal(bytes, 8, location)) } _ => None, }; @@ -1022,12 +1026,17 @@ impl<'interner> Monomorphizer<'interner> { None } - fn modulus_array_literal(&self, bytes: Vec, arr_elem_bits: u32) -> ast::Expression { + fn modulus_array_literal( + &self, + bytes: Vec, + arr_elem_bits: u32, + location: Location, + ) -> ast::Expression { use ast::*; let int_type = Type::Integer(crate::Signedness::Unsigned, arr_elem_bits); let bytes_as_expr = vecmap(bytes, |byte| { - Expression::Literal(Literal::Integer((byte as u128).into(), int_type.clone())) + Expression::Literal(Literal::Integer((byte as u128).into(), int_type.clone(), location)) }); let typ = Type::Array(bytes_as_expr.len() as u64, Box::new(int_type)); @@ -1277,7 +1286,8 @@ impl<'interner> Monomorphizer<'interner> { ) -> ast::Expression { match typ { ast::Type::Field | ast::Type::Integer(..) => { - ast::Expression::Literal(ast::Literal::Integer(0_u128.into(), typ.clone())) + let typ = typ.clone(); + ast::Expression::Literal(ast::Literal::Integer(0_u128.into(), typ, location)) } ast::Type::Bool => ast::Expression::Literal(ast::Literal::Bool(false)), // There is no unit literal currently. Replace it with 'false' since it should be ignored diff --git a/compiler/noirc_frontend/src/monomorphization/printer.rs b/compiler/noirc_frontend/src/monomorphization/printer.rs index ff2b7d0d256..e79330de6f8 100644 --- a/compiler/noirc_frontend/src/monomorphization/printer.rs +++ b/compiler/noirc_frontend/src/monomorphization/printer.rs @@ -93,7 +93,7 @@ impl AstPrinter { self.print_comma_separated(&array.contents, f)?; write!(f, "]") } - super::ast::Literal::Integer(x, _) => x.fmt(f), + super::ast::Literal::Integer(x, _, _) => x.fmt(f), super::ast::Literal::Bool(x) => x.fmt(f), super::ast::Literal::Str(s) => s.fmt(f), super::ast::Literal::FmtStr(s, _, _) => { From c7aa4a8dbd9a204de8248edaadffcf57abf7d562 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 20 Oct 2023 14:43:36 -0500 Subject: [PATCH 2/4] Add regression test --- .../compile_failure/integer_literal_overflow/Nargo.toml | 7 +++++++ .../compile_failure/integer_literal_overflow/src/main.nr | 6 ++++++ 2 files changed, 13 insertions(+) create mode 100644 tooling/nargo_cli/tests/compile_failure/integer_literal_overflow/Nargo.toml create mode 100644 tooling/nargo_cli/tests/compile_failure/integer_literal_overflow/src/main.nr diff --git a/tooling/nargo_cli/tests/compile_failure/integer_literal_overflow/Nargo.toml b/tooling/nargo_cli/tests/compile_failure/integer_literal_overflow/Nargo.toml new file mode 100644 index 00000000000..f29ec0408ea --- /dev/null +++ b/tooling/nargo_cli/tests/compile_failure/integer_literal_overflow/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "integer_literal_overflow" +type = "bin" +authors = [""] +compiler_version = "0.16.0" + +[dependencies] \ No newline at end of file diff --git a/tooling/nargo_cli/tests/compile_failure/integer_literal_overflow/src/main.nr b/tooling/nargo_cli/tests/compile_failure/integer_literal_overflow/src/main.nr new file mode 100644 index 00000000000..ab1cb457fee --- /dev/null +++ b/tooling/nargo_cli/tests/compile_failure/integer_literal_overflow/src/main.nr @@ -0,0 +1,6 @@ + +fn main() { + foo(1234) +} + +fn foo(_x: u4) {} From ad63ad559f48ab0d3ec4c9cbd944e4bb9bead4d5 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 24 Oct 2023 10:48:59 -0500 Subject: [PATCH 3/4] Change signed check --- compiler/noirc_evaluator/src/ssa/ir/types.rs | 13 +++++++------ compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs | 8 +++----- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/types.rs b/compiler/noirc_evaluator/src/ssa/ir/types.rs index b9d44acca4e..e69e936372d 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/types.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/types.rs @@ -96,20 +96,21 @@ impl Type { } impl NumericType { - /// Returns the numeric limits of this type if it is an integer type. - /// If this is a Field, None is returned. - pub(crate) fn get_limits(self) -> Option<(FieldElement, FieldElement)> { + /// Returns true if the given Field value is within the numeric limits + /// for the current NumericType. + pub(crate) fn value_is_within_limits(self, field: FieldElement) -> bool { match self { NumericType::Signed { bit_size } => { let min = -(2i128.pow(bit_size - 1)); let max = 2u128.pow(bit_size - 1) - 1; - Some((min.into(), max.into())) + // Signed integers are odd since they will overflow the field value + field <= max.into() || field >= min.into() } NumericType::Unsigned { bit_size } => { let max = 2u128.pow(bit_size) - 1; - Some((FieldElement::zero(), max.into())) + field <= max.into() } - NumericType::NativeField => None, // Should we check the field size as well? + NumericType::NativeField => true, } } } diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index b6352879553..26c818250dc 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -254,11 +254,9 @@ impl<'a> FunctionContext<'a> { let value = value.into(); if let Type::Numeric(typ) = typ { - if let Some((lower_bound, upper_bound)) = typ.get_limits() { - if !(lower_bound <= value && value <= upper_bound) { - let call_stack = self.builder.get_call_stack(); - return Err(RuntimeError::IntegerOutOfBounds { value, typ, call_stack }); - } + if !typ.value_is_within_limits(value) { + let call_stack = self.builder.get_call_stack(); + return Err(RuntimeError::IntegerOutOfBounds { value, typ, call_stack }); } } else { panic!("Expected type for numeric constant to be a numeric type, found {typ}"); From 6ddb57cd38a48dee1de9d5baf7f823aeca4e99eb Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 24 Oct 2023 11:28:57 -0500 Subject: [PATCH 4/4] Comment out failing test with reference to issue --- .../brillig_modulo/src/main.nr | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/tooling/nargo_cli/tests/compile_success_empty/brillig_modulo/src/main.nr b/tooling/nargo_cli/tests/compile_success_empty/brillig_modulo/src/main.nr index 1cab78ecb95..c2c15f88b91 100644 --- a/tooling/nargo_cli/tests/compile_success_empty/brillig_modulo/src/main.nr +++ b/tooling/nargo_cli/tests/compile_success_empty/brillig_modulo/src/main.nr @@ -7,16 +7,18 @@ fn main() { assert(signed_modulo(5, 3) == 2); assert(signed_modulo(2, 3) == 2); - let minus_two: i4 = 14; - let minus_three: i4 = 13; - let minus_five: i4 = 11; + // See #3275. + // Commented out for now since the previous values which would overflow an i4 are now a compiler error. + // let minus_two: i4 = -2; // 14 + // let minus_three: i4 = -3; // 13 + // let minus_five: i4 = -5; // 11 - // (5 / -3) * -3 + 2 = -1 * -3 + 2 = 3 + 2 = 5 - assert(signed_modulo(5, minus_three) == 2); - // (-5 / 3) * 3 - 2 = -1 * 3 - 2 = -3 - 2 = -5 - assert(signed_modulo(minus_five, 3) == minus_two); - // (-5 / -3) * -3 - 2 = 1 * -3 - 2 = -3 - 2 = -5 - assert(signed_modulo(minus_five, minus_three) == minus_two); + // // (5 / -3) * -3 + 2 = -1 * -3 + 2 = 3 + 2 = 5 + // assert(signed_modulo(5, minus_three) == 2); + // // (-5 / 3) * 3 - 2 = -1 * 3 - 2 = -3 - 2 = -5 + // assert(signed_modulo(minus_five, 3) == minus_two); + // // (-5 / -3) * -3 - 2 = 1 * -3 - 2 = -3 - 2 = -5 + // assert(signed_modulo(minus_five, minus_three) == minus_two); } unconstrained fn modulo(x: u32, y: u32) -> u32 {