Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Add size checks to integer literals #3236

Merged
merged 5 commits into from
Oct 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions compiler/noirc_evaluator/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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")]
Expand Down Expand Up @@ -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,
}
}
Expand Down
7 changes: 4 additions & 3 deletions compiler/noirc_evaluator/src/ssa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ pub(crate) fn optimize_into_acir(
print_brillig_trace: bool,
) -> Result<GeneratedAcir, RuntimeError> {
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
Expand Down Expand Up @@ -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<SsaBuilder, RuntimeError> {
let ssa = ssa_gen::generate_ssa(program)?;
Ok(SsaBuilder { print_ssa_passes, ssa }.print("Initial SSA:"))
}

fn finish(self) -> Ssa {
Expand Down
4 changes: 4 additions & 0 deletions compiler/noirc_evaluator/src/ssa/function_builder/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
23 changes: 22 additions & 1 deletion compiler/noirc_evaluator/src/ssa/ir/types.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::rc::Rc;

use acvm::FieldElement;
use iter_extended::vecmap;

/// A numeric type in the Intermediate representation
Expand All @@ -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,
Expand Down Expand Up @@ -94,6 +95,26 @@ impl Type {
}
}

impl NumericType {
/// 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;
// 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;
field <= max.into()
}
NumericType::NativeField => true,
}
}
}

/// 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.
Expand Down
80 changes: 58 additions & 22 deletions compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -240,6 +241,30 @@ 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<FieldElement>,
typ: Type,
) -> Result<ValueId, RuntimeError> {
let value = value.into();

if let Type::Numeric(typ) = typ {
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}");
}

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));
Expand Down Expand Up @@ -544,8 +569,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(
guipublic marked this conversation as resolved.
Show resolved Hide resolved
&mut self,
lvalue: &ast::LValue,
) -> Result<LValue, RuntimeError> {
Ok(match lvalue {
ast::LValue::Ident(ident) => {
let (reference, should_auto_deref) = self.ident_lvalue(ident);
if should_auto_deref {
Expand All @@ -555,18 +583,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 {
Expand Down Expand Up @@ -596,16 +624,16 @@ impl<'a> FunctionContext<'a> {
array: &ast::LValue,
index: &ast::Expression,
location: &Location,
) -> (ValueId, ValueId, LValue, Option<ValueId>) {
let (old_array, array_lvalue) = self.extract_current_value_recursive(array);
let index = self.codegen_non_tuple_expression(index);
) -> Result<(ValueId, ValueId, LValue, Option<ValueId>), 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,
Expand All @@ -617,37 +645,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 }))
}
}
}
Expand Down
Loading
Loading