From 02b9f9c54008c4de236f1094d2884f1039b7c868 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Mon, 21 Aug 2023 18:35:24 +0000 Subject: [PATCH 01/10] attach locations to acir MemoryOps and restrict how we push opcodes when generating acir --- crates/nargo_cli/src/cli/execute_cmd.rs | 52 +++++++++++++++---- .../dynamic_index_failure/Nargo.toml | 7 +++ .../dynamic_index_failure/Prover.toml | 2 + .../dynamic_index_failure/src/main.nr | 21 ++++++++ crates/noirc_evaluator/src/ssa.rs | 11 ++-- .../src/ssa/acir_gen/acir_ir/acir_variable.rs | 7 +-- .../ssa/acir_gen/acir_ir/generated_acir.rs | 10 ++-- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 2 +- crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs | 5 +- 9 files changed, 91 insertions(+), 26 deletions(-) create mode 100644 crates/nargo_cli/tests/compile_failure/dynamic_index_failure/Nargo.toml create mode 100644 crates/nargo_cli/tests/compile_failure/dynamic_index_failure/Prover.toml create mode 100644 crates/nargo_cli/tests/compile_failure/dynamic_index_failure/src/main.nr diff --git a/crates/nargo_cli/src/cli/execute_cmd.rs b/crates/nargo_cli/src/cli/execute_cmd.rs index 537af6da48e..df1acd6739b 100644 --- a/crates/nargo_cli/src/cli/execute_cmd.rs +++ b/crates/nargo_cli/src/cli/execute_cmd.rs @@ -82,7 +82,6 @@ fn execute_package( // Parse the initial witness values from Prover.toml let (inputs_map, _) = read_inputs_from_file(&package.root_dir, prover_name, Format::Toml, &abi)?; - let solved_witness = execute_program(backend, circuit, &abi, &inputs_map, Some((debug, context)))?; let public_abi = abi.public_abi(); @@ -91,38 +90,68 @@ fn execute_package( Ok((return_value, solved_witness)) } -fn extract_unsatisfied_constraint_from_nargo_error( +fn report_unsatisfied_constraint_error( + opcode_err_info: Option<(OpcodeLocation, &acvm::pwg::OpcodeResolutionError)>, + debug: &DebugInfo, + context: &Context, +) { + if let Some((opcode_location, _)) = opcode_err_info { + if let Some(locations) = debug.opcode_location(&opcode_location) { + // The location of the error itself will be the location at the top + // of the call stack (the last item in the Vec). + if let Some(location) = locations.last() { + let message = "Failed constraint".into(); + CustomDiagnostic::simple_error(message, String::new(), location.span) + .in_file(location.file) + .with_call_stack(locations) + .report(&context.file_manager, false); + } + } + } +} + +fn extract_opcode_error_from_nargo_error( nargo_err: &NargoError, -) -> Option { +) -> Option<(OpcodeLocation, &acvm::pwg::OpcodeResolutionError)> { let solving_err = match nargo_err { nargo::NargoError::SolvingError(err) => err, _ => return None, }; match solving_err { - acvm::pwg::OpcodeResolutionError::UnsatisfiedConstrain { + acvm::pwg::OpcodeResolutionError::IndexOutOfBounds { + opcode_location: error_location, + .. + } + | acvm::pwg::OpcodeResolutionError::UnsatisfiedConstrain { opcode_location: error_location, } => match error_location { ErrorLocation::Unresolved => { unreachable!("Cannot resolve index for unsatisfied constraint") } - ErrorLocation::Resolved(opcode_location) => Some(*opcode_location), + ErrorLocation::Resolved(opcode_location) => Some((*opcode_location, solving_err)), }, _ => None, } } -fn report_unsatisfied_constraint_error( - opcode_location: Option, +fn report_index_out_of_bounds_error( + opcode_err_info: Option<(OpcodeLocation, &acvm::pwg::OpcodeResolutionError)>, debug: &DebugInfo, context: &Context, ) { - if let Some(opcode_location) = opcode_location { + if let Some(( + opcode_location, + acvm::pwg::OpcodeResolutionError::IndexOutOfBounds { index, array_size, .. }, + )) = opcode_err_info + { if let Some(locations) = debug.opcode_location(&opcode_location) { // The location of the error itself will be the location at the top // of the call stack (the last item in the Vec). if let Some(location) = locations.last() { - let message = "Failed constraint".into(); + let message = format!( + "Index out of bounds, array has size {array_size:?}, but index was {index:?}" + ); CustomDiagnostic::simple_error(message, String::new(), location.span) .in_file(location.file) .with_call_stack(locations) @@ -145,8 +174,9 @@ pub(crate) fn execute_program( Ok(solved_witness) => Ok(solved_witness), Err(err) => { if let Some((debug, context)) = debug_data { - let opcode_location = extract_unsatisfied_constraint_from_nargo_error(&err); - report_unsatisfied_constraint_error(opcode_location, &debug, &context); + let opcode_err_info = extract_opcode_error_from_nargo_error(&err); + report_unsatisfied_constraint_error(opcode_err_info, &debug, &context); + report_index_out_of_bounds_error(opcode_err_info, &debug, &context); } Err(crate::errors::CliError::NargoError(err)) diff --git a/crates/nargo_cli/tests/compile_failure/dynamic_index_failure/Nargo.toml b/crates/nargo_cli/tests/compile_failure/dynamic_index_failure/Nargo.toml new file mode 100644 index 00000000000..c68615052c1 --- /dev/null +++ b/crates/nargo_cli/tests/compile_failure/dynamic_index_failure/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "dynamic_index_failure" +type = "bin" +authors = [""] +compiler_version = "0.10.3" + +[dependencies] \ No newline at end of file diff --git a/crates/nargo_cli/tests/compile_failure/dynamic_index_failure/Prover.toml b/crates/nargo_cli/tests/compile_failure/dynamic_index_failure/Prover.toml new file mode 100644 index 00000000000..caf3448c56f --- /dev/null +++ b/crates/nargo_cli/tests/compile_failure/dynamic_index_failure/Prover.toml @@ -0,0 +1,2 @@ +x = [104, 101, 108, 108, 111] +z = "4" diff --git a/crates/nargo_cli/tests/compile_failure/dynamic_index_failure/src/main.nr b/crates/nargo_cli/tests/compile_failure/dynamic_index_failure/src/main.nr new file mode 100644 index 00000000000..387032e3687 --- /dev/null +++ b/crates/nargo_cli/tests/compile_failure/dynamic_index_failure/src/main.nr @@ -0,0 +1,21 @@ +fn main(mut x: [u32; 5], z: Field) { + let idx = z + 10; + + x[z] = 4; + + // Dynamic index is greater than length of the array + assert(x[idx] != 0); + + // TODO(#2133): Provide more accurate call stacks for flattening arrays + // if z == 20 { + // x[0] = x[4]; + // } else { + // x[idx] = 10; + // for i in 0..5 { + // x[idx] = x[i]; + // } + // } + // TODO(#2133): This line should be where we show the error + // but instead it will show x[0] == x[4] as the line where there is an index out of bounds + // assert(x[idx] != 0) +} \ No newline at end of file diff --git a/crates/noirc_evaluator/src/ssa.rs b/crates/noirc_evaluator/src/ssa.rs index 77399994e83..841f89260e4 100644 --- a/crates/noirc_evaluator/src/ssa.rs +++ b/crates/noirc_evaluator/src/ssa.rs @@ -87,14 +87,11 @@ pub fn create_circuit( enable_brillig_logging: bool, ) -> Result<(Circuit, DebugInfo, Abi), RuntimeError> { let func_sig = program.main_function_signature.clone(); + let generated_acir = optimize_into_acir(program, enable_ssa_logging, enable_brillig_logging)?; + let opcodes = generated_acir.opcodes().to_vec(); let GeneratedAcir { - current_witness_index, - opcodes, - return_witnesses, - locations, - input_witnesses, - .. - } = optimize_into_acir(program, enable_ssa_logging, enable_brillig_logging)?; + current_witness_index, return_witnesses, locations, input_witnesses, .. + } = generated_acir; let abi = gen_abi(&context.def_interner, func_sig, &input_witnesses, return_witnesses.clone()); let public_abi = abi.clone().public_abi(); diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs b/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs index d682c30d5ab..0c4da4ea530 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs @@ -1081,7 +1081,7 @@ impl AcirContext { // Add the memory read operation to the list of opcodes let op = MemOp::read_at_mem_index(index_witness.into(), value_read_witness); - self.acir_ir.opcodes.push(Opcode::MemoryOp { block_id, op }); + self.acir_ir.push_opcode(Opcode::MemoryOp { block_id, op }); Ok(value_read_var) } @@ -1102,7 +1102,8 @@ impl AcirContext { // Add the memory write operation to the list of opcodes let op = MemOp::write_to_mem_index(index_witness.into(), value_write_witness.into()); - self.acir_ir.opcodes.push(Opcode::MemoryOp { block_id, op }); + self.acir_ir.push_opcode(Opcode::MemoryOp { block_id, op }); + Ok(()) } @@ -1128,7 +1129,7 @@ impl AcirContext { })?, }; - self.acir_ir.opcodes.push(Opcode::MemoryInit { block_id, init: initialized_values }); + self.acir_ir.push_opcode(Opcode::MemoryInit { block_id, init: initialized_values }); Ok(()) } } diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs b/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs index 37fac411e26..162ef7eb77f 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs @@ -34,7 +34,7 @@ pub(crate) struct GeneratedAcir { pub(crate) current_witness_index: u32, /// The opcodes of which the compiled ACIR will comprise. - pub(crate) opcodes: Vec, + opcodes: Vec, /// All witness indices that comprise the final return value of the program /// @@ -60,7 +60,7 @@ impl GeneratedAcir { } /// Adds a new opcode into ACIR. - fn push_opcode(&mut self, opcode: AcirOpcode) { + pub(crate) fn push_opcode(&mut self, opcode: AcirOpcode) { self.opcodes.push(opcode); if !self.call_stack.is_empty() { self.locations @@ -68,6 +68,10 @@ impl GeneratedAcir { } } + pub(crate) fn opcodes(&self) -> &[AcirOpcode] { + &self.opcodes + } + /// Updates the witness index counter and returns /// the next witness index. pub(crate) fn next_witness_index(&mut self) -> Witness { @@ -228,7 +232,7 @@ impl GeneratedAcir { } }; - self.opcodes.push(AcirOpcode::BlackBoxFuncCall(black_box_func_call)); + self.push_opcode(AcirOpcode::BlackBoxFuncCall(black_box_func_call)); Ok(outputs_clone) } diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs index f1c865a4b8f..463b5dc42c9 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -1175,7 +1175,7 @@ mod tests { let expected_opcodes = vec![Opcode::Arithmetic(&Expression::one() - &Expression::from(Witness(1)))]; - assert_eq!(acir.opcodes, expected_opcodes); + assert_eq!(acir.opcodes(), expected_opcodes); assert_eq!(acir.return_witnesses, vec![Witness(1)]); } } diff --git a/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs index cc3a7c02a75..1367b1753af 100644 --- a/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -130,7 +130,10 @@ impl<'a> FunctionContext<'a> { let slice_contents = self.codegen_array(elements, typ[1].clone()); Tree::Branch(vec![slice_length.into(), slice_contents]) } - _ => unreachable!("ICE: array literal type must be an array or a slice, but got {}", array.typ), + _ => unreachable!( + "ICE: array literal type must be an array or a slice, but got {}", + array.typ + ), } } ast::Literal::Integer(value, typ) => { From 5f7a72ba5c2ce06eff088690ab71e9b80b6c640f Mon Sep 17 00:00:00 2001 From: vezenovm Date: Mon, 21 Aug 2023 18:51:25 +0000 Subject: [PATCH 02/10] better reporting for unconstrained and index out of bounds from acvm --- crates/nargo_cli/src/cli/execute_cmd.rs | 61 +++++++++++-------------- 1 file changed, 27 insertions(+), 34 deletions(-) diff --git a/crates/nargo_cli/src/cli/execute_cmd.rs b/crates/nargo_cli/src/cli/execute_cmd.rs index df1acd6739b..880870e8a9f 100644 --- a/crates/nargo_cli/src/cli/execute_cmd.rs +++ b/crates/nargo_cli/src/cli/execute_cmd.rs @@ -90,26 +90,6 @@ fn execute_package( Ok((return_value, solved_witness)) } -fn report_unsatisfied_constraint_error( - opcode_err_info: Option<(OpcodeLocation, &acvm::pwg::OpcodeResolutionError)>, - debug: &DebugInfo, - context: &Context, -) { - if let Some((opcode_location, _)) = opcode_err_info { - if let Some(locations) = debug.opcode_location(&opcode_location) { - // The location of the error itself will be the location at the top - // of the call stack (the last item in the Vec). - if let Some(location) = locations.last() { - let message = "Failed constraint".into(); - CustomDiagnostic::simple_error(message, String::new(), location.span) - .in_file(location.file) - .with_call_stack(locations) - .report(&context.file_manager, false); - } - } - } -} - fn extract_opcode_error_from_nargo_error( nargo_err: &NargoError, ) -> Option<(OpcodeLocation, &acvm::pwg::OpcodeResolutionError)> { @@ -135,27 +115,41 @@ fn extract_opcode_error_from_nargo_error( } } -fn report_index_out_of_bounds_error( +fn report_opcode_error( opcode_err_info: Option<(OpcodeLocation, &acvm::pwg::OpcodeResolutionError)>, debug: &DebugInfo, context: &Context, ) { if let Some(( opcode_location, - acvm::pwg::OpcodeResolutionError::IndexOutOfBounds { index, array_size, .. }, + opcode_err, )) = opcode_err_info { if let Some(locations) = debug.opcode_location(&opcode_location) { - // The location of the error itself will be the location at the top - // of the call stack (the last item in the Vec). - if let Some(location) = locations.last() { - let message = format!( - "Index out of bounds, array has size {array_size:?}, but index was {index:?}" - ); - CustomDiagnostic::simple_error(message, String::new(), location.span) - .in_file(location.file) - .with_call_stack(locations) - .report(&context.file_manager, false); + match opcode_err { + // The location of the error itself will be the location at the top + // of the call stack (the last item in the Vec). + acvm::pwg::OpcodeResolutionError::IndexOutOfBounds { index, array_size, .. } => { + if let Some(location) = locations.last() { + let message = format!( + "Index out of bounds, array has size {array_size:?}, but index was {index:?}" + ); + CustomDiagnostic::simple_error(message, String::new(), location.span) + .in_file(location.file) + .with_call_stack(locations) + .report(&context.file_manager, false); + } + } + acvm::pwg::OpcodeResolutionError::UnsatisfiedConstrain { .. } => { + if let Some(location) = locations.last() { + let message = "Failed constraint".into(); + CustomDiagnostic::simple_error(message, String::new(), location.span) + .in_file(location.file) + .with_call_stack(locations) + .report(&context.file_manager, false); + } + } + _ => (), } } } @@ -175,8 +169,7 @@ pub(crate) fn execute_program( Err(err) => { if let Some((debug, context)) = debug_data { let opcode_err_info = extract_opcode_error_from_nargo_error(&err); - report_unsatisfied_constraint_error(opcode_err_info, &debug, &context); - report_index_out_of_bounds_error(opcode_err_info, &debug, &context); + report_opcode_error(opcode_err_info, &debug, &context); } Err(crate::errors::CliError::NargoError(err)) From 0ac1c33c74879d476b51c27950d3591ac3764a4b Mon Sep 17 00:00:00 2001 From: vezenovm Date: Mon, 21 Aug 2023 19:00:28 +0000 Subject: [PATCH 03/10] check locations.last() once --- crates/nargo_cli/src/cli/execute_cmd.rs | 26 ++++++++++++------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/crates/nargo_cli/src/cli/execute_cmd.rs b/crates/nargo_cli/src/cli/execute_cmd.rs index 880870e8a9f..8e5e55c3bb6 100644 --- a/crates/nargo_cli/src/cli/execute_cmd.rs +++ b/crates/nargo_cli/src/cli/execute_cmd.rs @@ -120,17 +120,17 @@ fn report_opcode_error( debug: &DebugInfo, context: &Context, ) { - if let Some(( - opcode_location, - opcode_err, - )) = opcode_err_info - { + if let Some((opcode_location, opcode_err)) = opcode_err_info { if let Some(locations) = debug.opcode_location(&opcode_location) { - match opcode_err { - // The location of the error itself will be the location at the top - // of the call stack (the last item in the Vec). - acvm::pwg::OpcodeResolutionError::IndexOutOfBounds { index, array_size, .. } => { - if let Some(location) = locations.last() { + // The location of the error itself will be the location at the top + // of the call stack (the last item in the Vec). + if let Some(location) = locations.last() { + match opcode_err { + acvm::pwg::OpcodeResolutionError::IndexOutOfBounds { + index, + array_size, + .. + } => { let message = format!( "Index out of bounds, array has size {array_size:?}, but index was {index:?}" ); @@ -139,17 +139,15 @@ fn report_opcode_error( .with_call_stack(locations) .report(&context.file_manager, false); } - } - acvm::pwg::OpcodeResolutionError::UnsatisfiedConstrain { .. } => { - if let Some(location) = locations.last() { + acvm::pwg::OpcodeResolutionError::UnsatisfiedConstrain { .. } => { let message = "Failed constraint".into(); CustomDiagnostic::simple_error(message, String::new(), location.span) .in_file(location.file) .with_call_stack(locations) .report(&context.file_manager, false); } + _ => (), } - _ => (), } } } From b3ae58916924d13d152f08ef7119336f089311e5 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Mon, 21 Aug 2023 23:07:20 +0000 Subject: [PATCH 04/10] add location data to array_set --- .../dynamic_index_failure/src/main.nr | 24 ++++++++++------- .../src/ssa/ssa_gen/context.rs | 26 +++++++++++-------- 2 files changed, 29 insertions(+), 21 deletions(-) diff --git a/crates/nargo_cli/tests/compile_failure/dynamic_index_failure/src/main.nr b/crates/nargo_cli/tests/compile_failure/dynamic_index_failure/src/main.nr index 387032e3687..6a559d08ffb 100644 --- a/crates/nargo_cli/tests/compile_failure/dynamic_index_failure/src/main.nr +++ b/crates/nargo_cli/tests/compile_failure/dynamic_index_failure/src/main.nr @@ -4,18 +4,22 @@ fn main(mut x: [u32; 5], z: Field) { x[z] = 4; // Dynamic index is greater than length of the array - assert(x[idx] != 0); + // assert(x[idx] != 0); // TODO(#2133): Provide more accurate call stacks for flattening arrays - // if z == 20 { - // x[0] = x[4]; - // } else { - // x[idx] = 10; - // for i in 0..5 { - // x[idx] = x[i]; - // } - // } + if z != 20 { + x[0] = x[4]; + } else { + // TODO: Dynamic predicate still gives index out of bounds error + if idx as u32 < 3 { + x[idx] = 10; + } + x[idx] = 10; + for i in 0..5 { + x[idx] = x[i]; + } + } // TODO(#2133): This line should be where we show the error // but instead it will show x[0] == x[4] as the line where there is an index out of bounds - // assert(x[idx] != 0) + assert(x[idx] != 0); } \ No newline at end of file diff --git a/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs b/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs index b04e4263f07..ed62b858713 100644 --- a/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -551,7 +551,7 @@ impl<'a> FunctionContext<'a> { LValue::Ident } } - ast::LValue::Index { array, index, .. } => self.index_lvalue(array, index).2, + ast::LValue::Index { array, index, location, .. } => 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 object_lvalue = Box::new(object_lvalue); @@ -590,20 +590,22 @@ impl<'a> FunctionContext<'a> { &mut self, 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); 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 { let slice_lvalue = - LValue::SliceIndex { old_slice: old_array, index, slice_lvalue: array_lvalue }; + LValue::SliceIndex { old_slice: old_array, index, slice_lvalue: array_lvalue, location }; (array_values[1], index, slice_lvalue, Some(array_values[0])) } else { - let array_lvalue = LValue::Index { old_array: array_values[0], index, array_lvalue }; + let array_lvalue = LValue::Index { old_array: array_values[0], index, array_lvalue, location }; (array_values[0], index, array_lvalue, None) } } @@ -620,7 +622,7 @@ impl<'a> FunctionContext<'a> { } } ast::LValue::Index { array, index, element_type, location } => { - let (old_array, index, index_lvalue, max_length) = self.index_lvalue(array, index); + 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) @@ -647,14 +649,14 @@ impl<'a> FunctionContext<'a> { pub(super) fn assign_new_value(&mut self, lvalue: LValue, new_value: Values) { match lvalue { LValue::Ident => unreachable!("Cannot assign to a variable without a reference"), - LValue::Index { old_array: mut array, index, array_lvalue } => { - array = self.assign_lvalue_index(new_value, array, index); + LValue::Index { old_array: mut array, index, array_lvalue, location } => { + array = self.assign_lvalue_index(new_value, array, index, location); self.assign_new_value(*array_lvalue, array.into()); } - LValue::SliceIndex { old_slice: slice, index, slice_lvalue } => { + LValue::SliceIndex { old_slice: slice, index, slice_lvalue, location } => { let mut slice_values = slice.into_value_list(self); - slice_values[1] = self.assign_lvalue_index(new_value, slice_values[1], index); + slice_values[1] = self.assign_lvalue_index(new_value, slice_values[1], index, location); // The size of the slice does not change in a slice index assignment so we can reuse the same length value let new_slice = Tree::Branch(vec![slice_values[0].into(), slice_values[1].into()]); @@ -675,11 +677,13 @@ impl<'a> FunctionContext<'a> { new_value: Values, mut array: ValueId, index: ValueId, + location: Location, ) -> ValueId { let element_size = self.builder.field_constant(self.element_size(array)); // The actual base index is the user's index * the array element type's size - let mut index = self.builder.insert_binary(index, BinaryOp::Mul, element_size); + let mut index = self.builder.set_location(location).insert_binary(index, BinaryOp::Mul, element_size); + // let mut index = self.builder.insert_binary(index, BinaryOp::Mul, element_size); let one = self.builder.field_constant(FieldElement::one()); new_value.for_each(|value| { @@ -862,8 +866,8 @@ impl SharedContext { #[derive(Debug)] pub(super) enum LValue { Ident, - Index { old_array: ValueId, index: ValueId, array_lvalue: Box }, - SliceIndex { old_slice: Values, index: ValueId, slice_lvalue: Box }, + Index { old_array: ValueId, index: ValueId, array_lvalue: Box, location: Location }, + SliceIndex { old_slice: Values, index: ValueId, slice_lvalue: Box, location: Location }, MemberAccess { old_object: Values, index: usize, object_lvalue: Box }, Dereference { reference: Values }, } From 260220931aad2b41c41b1cc23cda7b9f6deaf226 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Mon, 21 Aug 2023 23:07:44 +0000 Subject: [PATCH 05/10] fix up compile failure test --- .../dynamic_index_failure/src/main.nr | 32 +++++++++---------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/crates/nargo_cli/tests/compile_failure/dynamic_index_failure/src/main.nr b/crates/nargo_cli/tests/compile_failure/dynamic_index_failure/src/main.nr index 6a559d08ffb..2665356ccd6 100644 --- a/crates/nargo_cli/tests/compile_failure/dynamic_index_failure/src/main.nr +++ b/crates/nargo_cli/tests/compile_failure/dynamic_index_failure/src/main.nr @@ -4,22 +4,20 @@ fn main(mut x: [u32; 5], z: Field) { x[z] = 4; // Dynamic index is greater than length of the array - // assert(x[idx] != 0); - - // TODO(#2133): Provide more accurate call stacks for flattening arrays - if z != 20 { - x[0] = x[4]; - } else { - // TODO: Dynamic predicate still gives index out of bounds error - if idx as u32 < 3 { - x[idx] = 10; - } - x[idx] = 10; - for i in 0..5 { - x[idx] = x[i]; - } - } - // TODO(#2133): This line should be where we show the error - // but instead it will show x[0] == x[4] as the line where there is an index out of bounds assert(x[idx] != 0); + + // TODO(#2133): Provide more accurate call stacks for arrays merged in if statements + // if z != 20 { + // x[0] = x[4]; + // } else { + // // TODO: Dynamic predicate still gives index out of bounds error + // if idx as u32 < 3 { + // x[idx] = 10; + // } + // x[idx] = 10; + // for i in 0..5 { + // x[idx] = x[i]; + // } + // } + // assert(x[idx] != 0); } \ No newline at end of file From f508f33037d25565dc41e2be63d29ea93336545f Mon Sep 17 00:00:00 2001 From: vezenovm Date: Tue, 22 Aug 2023 00:33:25 +0000 Subject: [PATCH 06/10] initial work adding predicates to memops --- Cargo.lock | 16 ---------- Cargo.toml | 4 +++ .../dynamic_index_failure/src/main.nr | 30 ++++++++++--------- .../src/ssa/acir_gen/acir_ir/acir_variable.rs | 23 +++++++++----- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 8 ++--- 5 files changed, 40 insertions(+), 41 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index be1270f4fa2..b6411e2049b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5,8 +5,6 @@ version = 3 [[package]] name = "acir" version = "0.22.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "86577747c44f23e2e8e6d972287d01341c0eea42a78ce15c5efd212a39d0fc27" dependencies = [ "acir_field", "bincode", @@ -19,8 +17,6 @@ dependencies = [ [[package]] name = "acir_field" version = "0.22.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4239156a8eddd55b2ae8bd25aa169d012bae70e0fd7c635f08f68ada54a8cb6c" dependencies = [ "ark-bn254", "ark-ff", @@ -33,8 +29,6 @@ dependencies = [ [[package]] name = "acvm" version = "0.22.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "74351bab6e0fd2ec1bd631abc73260f374cc28d2baf85c0e11300c0c989d5e53" dependencies = [ "acir", "acvm_blackbox_solver", @@ -50,8 +44,6 @@ dependencies = [ [[package]] name = "acvm-backend-barretenberg" version = "0.11.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "438eb3837cfc37e0798e18f4a0ebae595e4cbe32a3f4cecfb47ccc1354180dc8" dependencies = [ "acvm", "barretenberg-sys", @@ -70,8 +62,6 @@ dependencies = [ [[package]] name = "acvm_blackbox_solver" version = "0.22.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a362499180c6498acc0ebf77bd919be8ccd9adabc84a695d4af44ca180ba0709" dependencies = [ "acir", "blake2", @@ -85,8 +75,6 @@ dependencies = [ [[package]] name = "acvm_stdlib" version = "0.22.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e485b3bc3331eaa10bc92fb092ca14275936c8935c3ae7ec89fb0bd48246ab42" dependencies = [ "acir", ] @@ -537,8 +525,6 @@ dependencies = [ [[package]] name = "brillig" version = "0.22.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d64df3df7d2d96fc2519e4dd64bc6bc23eee2949ee86725d9041ef7703c283ab" dependencies = [ "acir_field", "serde", @@ -547,8 +533,6 @@ dependencies = [ [[package]] name = "brillig_vm" version = "0.22.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b306b3d79b6da192fd2ed68b94ab07712496f39bb5d50fedce44dac3f4953065" dependencies = [ "acir", "acvm_blackbox_solver", diff --git a/Cargo.toml b/Cargo.toml index 76ec9edfa0d..66ceb5b53d8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -57,3 +57,7 @@ url = "2.2.0" wasm-bindgen = { version = "=0.2.86", features = ["serde-serialize"] } wasm-bindgen-test = "0.3.33" base64 = "0.21.2" + +[patch.crates-io] +acvm = { path = "/mnt/user-data/maxim/acvm/acvm" } +acvm-backend-barretenberg = { path = "/mnt/user-data/maxim/noir-lang/acvm-backend-barretenberg" } \ No newline at end of file diff --git a/crates/nargo_cli/tests/compile_failure/dynamic_index_failure/src/main.nr b/crates/nargo_cli/tests/compile_failure/dynamic_index_failure/src/main.nr index 2665356ccd6..78f6130cace 100644 --- a/crates/nargo_cli/tests/compile_failure/dynamic_index_failure/src/main.nr +++ b/crates/nargo_cli/tests/compile_failure/dynamic_index_failure/src/main.nr @@ -4,20 +4,22 @@ fn main(mut x: [u32; 5], z: Field) { x[z] = 4; // Dynamic index is greater than length of the array - assert(x[idx] != 0); + // assert(x[idx] != 0); // TODO(#2133): Provide more accurate call stacks for arrays merged in if statements - // if z != 20 { - // x[0] = x[4]; - // } else { - // // TODO: Dynamic predicate still gives index out of bounds error - // if idx as u32 < 3 { - // x[idx] = 10; - // } - // x[idx] = 10; - // for i in 0..5 { - // x[idx] = x[i]; - // } - // } - // assert(x[idx] != 0); + if z == 20 { + x[0] = x[4]; + } else { + // TODO: Dynamic predicate still gives index out of bounds error + if idx as u32 < 3 { + x[idx] = 10; + } + // assert(x[idx] != 0); + // x[idx] = 10; + // for i in 0..5 { + // x[idx] = x[i]; + // } + } + // Error should occur here as the index out of bounds write occurs + assert(x[idx] != 0); } \ No newline at end of file diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs b/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs index 0c4da4ea530..03526b04b0b 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs @@ -856,13 +856,13 @@ impl AcirContext { AcirValue::Array(vars) => { let mut var_expressions: Vec = Vec::new(); for var in vars { - self.brillig_array_input(&mut var_expressions, var)?; + self.brillig_array_input(&mut var_expressions, var, predicate)?; } Ok(BrilligInputs::Array(var_expressions)) } AcirValue::DynamicArray(_) => { let mut var_expressions = Vec::new(); - self.brillig_array_input(&mut var_expressions, i)?; + self.brillig_array_input(&mut var_expressions, i, predicate)?; Ok(BrilligInputs::Array(var_expressions)) } })?; @@ -899,6 +899,7 @@ impl AcirContext { &mut self, var_expressions: &mut Vec, input: AcirValue, + predicate: AcirVar ) -> Result<(), InternalError> { match input { AcirValue::Var(var, _) => { @@ -906,7 +907,7 @@ impl AcirContext { } AcirValue::Array(vars) => { for var in vars { - self.brillig_array_input(var_expressions, var)?; + self.brillig_array_input(var_expressions, var, predicate)?; } } AcirValue::DynamicArray(AcirDynamicArray { block_id, len }) => { @@ -918,13 +919,13 @@ impl AcirContext { ); let index_var = index.into_var()?; - let value_read_var = self.read_from_memory(block_id, &index_var)?; + let value_read_var = self.read_from_memory(block_id, &index_var, &predicate)?; let value_read = AcirValue::Var( value_read_var, AcirType::NumericType(NumericType::NativeField), ); - self.brillig_array_input(var_expressions, value_read)?; + self.brillig_array_input(var_expressions, value_read, predicate)?; } } } @@ -1071,6 +1072,7 @@ impl AcirContext { &mut self, block_id: BlockId, index: &AcirVar, + predicate: &AcirVar, ) -> Result { // Fetch the witness corresponding to the index let index_witness = self.var_to_witness(*index)?; @@ -1079,9 +1081,12 @@ impl AcirContext { let value_read_var = self.add_variable(); let value_read_witness = self.var_to_witness(value_read_var)?; + // Fetch the witness corresponding to the predicate + let predicate_witness = self.var_to_witness(*predicate)?; + // Add the memory read operation to the list of opcodes let op = MemOp::read_at_mem_index(index_witness.into(), value_read_witness); - self.acir_ir.push_opcode(Opcode::MemoryOp { block_id, op }); + self.acir_ir.push_opcode(Opcode::MemoryOp { block_id, op, predicate: Some(predicate_witness.into()) }); Ok(value_read_var) } @@ -1092,6 +1097,7 @@ impl AcirContext { block_id: BlockId, index: &AcirVar, value: &AcirVar, + predicate: &AcirVar, ) -> Result<(), InternalError> { // Fetch the witness corresponding to the index // @@ -1100,9 +1106,12 @@ impl AcirContext { // Fetch the witness corresponding to the value to be written let value_write_witness = self.var_to_witness(*value)?; + // Fetch the witness corresponding to the predicate + let predicate_witness = self.var_to_witness(*predicate)?; + // Add the memory write operation to the list of opcodes let op = MemOp::write_to_mem_index(index_witness.into(), value_write_witness.into()); - self.acir_ir.push_opcode(Opcode::MemoryOp { block_id, op }); + self.acir_ir.push_opcode(Opcode::MemoryOp { block_id, op, predicate: Some(predicate_witness.into()) }); Ok(()) } diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs index 463b5dc42c9..100c5518350 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -566,7 +566,7 @@ impl Context { } let index_var = self.convert_value(index, dfg).into_var()?; - let read = self.acir_context.read_from_memory(block_id, &index_var)?; + let read = self.acir_context.read_from_memory(block_id, &index_var, &self.current_side_effects_enabled_var)?; let typ = match dfg.type_of_value(array) { Type::Array(typ, _) => { if typ.len() != 1 { @@ -647,7 +647,7 @@ impl Context { AcirType::NumericType(NumericType::NativeField), ); let var = index.into_var()?; - let read = self.acir_context.read_from_memory(block_id, &var)?; + let read = self.acir_context.read_from_memory(block_id, &var, &self.current_side_effects_enabled_var)?; Ok(AcirValue::Var(read, AcirType::NumericType(NumericType::NativeField))) })?; self.initialize_array(result_block_id, len, Some(&init_values))?; @@ -656,7 +656,7 @@ impl Context { // Write the new value into the new array at the specified index let index_var = self.convert_value(index, dfg).into_var()?; let value_var = self.convert_value(store_value, dfg).into_var()?; - self.acir_context.write_to_memory(result_block_id, &index_var, &value_var)?; + self.acir_context.write_to_memory(result_block_id, &index_var, &value_var, &self.current_side_effects_enabled_var)?; let result_value = AcirValue::DynamicArray(AcirDynamicArray { block_id: result_block_id, len }); @@ -739,7 +739,7 @@ impl Context { if let Some(acir_value) = self.ssa_values.get(&value_id) { return acir_value.clone(); } - + let acir_value = match value { Value::NumericConstant { constant, typ } => { AcirValue::Var(self.acir_context.add_constant(*constant), typ.into()) From 7f930606124634c250c0b802402eb131a619137f Mon Sep 17 00:00:00 2001 From: vezenovm Date: Tue, 22 Aug 2023 00:51:01 +0000 Subject: [PATCH 07/10] switch to take_opcodes --- crates/noirc_evaluator/src/ssa.rs | 5 ++-- .../ssa/acir_gen/acir_ir/generated_acir.rs | 4 ++-- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 4 ++-- .../src/ssa/ssa_gen/context.rs | 24 +++++++++++++------ 4 files changed, 24 insertions(+), 13 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa.rs b/crates/noirc_evaluator/src/ssa.rs index 841f89260e4..7dbd627a949 100644 --- a/crates/noirc_evaluator/src/ssa.rs +++ b/crates/noirc_evaluator/src/ssa.rs @@ -87,8 +87,9 @@ pub fn create_circuit( enable_brillig_logging: bool, ) -> Result<(Circuit, DebugInfo, Abi), RuntimeError> { let func_sig = program.main_function_signature.clone(); - let generated_acir = optimize_into_acir(program, enable_ssa_logging, enable_brillig_logging)?; - let opcodes = generated_acir.opcodes().to_vec(); + let mut generated_acir = + optimize_into_acir(program, enable_ssa_logging, enable_brillig_logging)?; + let opcodes = generated_acir.take_opcodes(); let GeneratedAcir { current_witness_index, return_witnesses, locations, input_witnesses, .. } = generated_acir; diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs b/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs index 162ef7eb77f..9201179803b 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs @@ -68,8 +68,8 @@ impl GeneratedAcir { } } - pub(crate) fn opcodes(&self) -> &[AcirOpcode] { - &self.opcodes + pub(crate) fn take_opcodes(&mut self) -> Vec { + std::mem::take(&mut self.opcodes) } /// Updates the witness index counter and returns diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs index 463b5dc42c9..4fd828f05d0 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -1171,11 +1171,11 @@ mod tests { let ssa = builder.finish(); let context = Context::new(); - let acir = context.convert_ssa(ssa, Brillig::default(), &HashMap::new()).unwrap(); + let mut acir = context.convert_ssa(ssa, Brillig::default(), &HashMap::new()).unwrap(); let expected_opcodes = vec![Opcode::Arithmetic(&Expression::one() - &Expression::from(Witness(1)))]; - assert_eq!(acir.opcodes(), expected_opcodes); + assert_eq!(acir.take_opcodes(), expected_opcodes); assert_eq!(acir.return_witnesses, vec![Witness(1)]); } } diff --git a/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs b/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs index ed62b858713..85c3b0fd1cb 100644 --- a/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -551,7 +551,9 @@ impl<'a> FunctionContext<'a> { LValue::Ident } } - ast::LValue::Index { array, index, location, .. } => self.index_lvalue(array, index, location).2, + ast::LValue::Index { array, index, location, .. } => { + 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 object_lvalue = Box::new(object_lvalue); @@ -601,11 +603,16 @@ impl<'a> FunctionContext<'a> { // A slice is represented as a tuple (length, slice contents). // We need to fetch the second value. if array_values.len() > 1 { - let slice_lvalue = - LValue::SliceIndex { old_slice: old_array, index, slice_lvalue: array_lvalue, location }; + let slice_lvalue = LValue::SliceIndex { + old_slice: old_array, + index, + slice_lvalue: array_lvalue, + location, + }; (array_values[1], index, slice_lvalue, Some(array_values[0])) } else { - let array_lvalue = LValue::Index { old_array: array_values[0], index, array_lvalue, location }; + let array_lvalue = + LValue::Index { old_array: array_values[0], index, array_lvalue, location }; (array_values[0], index, array_lvalue, None) } } @@ -622,7 +629,8 @@ impl<'a> FunctionContext<'a> { } } ast::LValue::Index { array, index, element_type, location } => { - let (old_array, index, index_lvalue, max_length) = self.index_lvalue(array, index, 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) @@ -656,7 +664,8 @@ impl<'a> FunctionContext<'a> { LValue::SliceIndex { old_slice: slice, index, slice_lvalue, location } => { let mut slice_values = slice.into_value_list(self); - slice_values[1] = self.assign_lvalue_index(new_value, slice_values[1], index, location); + slice_values[1] = + self.assign_lvalue_index(new_value, slice_values[1], index, location); // The size of the slice does not change in a slice index assignment so we can reuse the same length value let new_slice = Tree::Branch(vec![slice_values[0].into(), slice_values[1].into()]); @@ -682,7 +691,8 @@ impl<'a> FunctionContext<'a> { let element_size = self.builder.field_constant(self.element_size(array)); // The actual base index is the user's index * the array element type's size - let mut index = self.builder.set_location(location).insert_binary(index, BinaryOp::Mul, element_size); + let mut index = + self.builder.set_location(location).insert_binary(index, BinaryOp::Mul, element_size); // let mut index = self.builder.insert_binary(index, BinaryOp::Mul, element_size); let one = self.builder.field_constant(FieldElement::one()); From 77fdc23758fd81727d1ebfe21f81532a37c2567e Mon Sep 17 00:00:00 2001 From: vezenovm Date: Tue, 22 Aug 2023 00:54:49 +0000 Subject: [PATCH 08/10] remove old comment --- crates/noirc_evaluator/src/ssa/ssa_gen/context.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs b/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs index 85c3b0fd1cb..0b3ee5b9acf 100644 --- a/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -693,7 +693,6 @@ impl<'a> FunctionContext<'a> { // The actual base index is the user's index * the array element type's size let mut index = self.builder.set_location(location).insert_binary(index, BinaryOp::Mul, element_size); - // let mut index = self.builder.insert_binary(index, BinaryOp::Mul, element_size); let one = self.builder.field_constant(FieldElement::one()); new_value.for_each(|value| { From 476d25f26dfbf1489377170136eb9f8d7054ddcc Mon Sep 17 00:00:00 2001 From: vezenovm Date: Tue, 22 Aug 2023 15:11:13 +0000 Subject: [PATCH 09/10] references to git patched acvm and backend --- Cargo.lock | 8 +++++++ Cargo.toml | 4 ++-- crates/nargo/src/ops/execute.rs | 3 ++- crates/nargo_cli/src/cli/execute_cmd.rs | 6 ++--- crates/nargo_cli/src/cli/prove_cmd.rs | 4 ++-- crates/nargo_cli/src/cli/test_cmd.rs | 6 ++--- .../dynamic_index_failure/src/main.nr | 16 +++---------- .../src/ssa/acir_gen/acir_ir/acir_variable.rs | 14 ++++++++--- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 21 ++++++++++++---- .../src/ssa/ssa_gen/context.rs | 24 +++++++++++++------ 10 files changed, 68 insertions(+), 38 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b6411e2049b..a651e2fdc90 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5,6 +5,7 @@ version = 3 [[package]] name = "acir" version = "0.22.0" +source = "git+https://github.com/noir-lang/acvm.git?rev=4b4e8a6a25b27e82bddbfe58caa3d421285b4a82#4b4e8a6a25b27e82bddbfe58caa3d421285b4a82" dependencies = [ "acir_field", "bincode", @@ -17,6 +18,7 @@ dependencies = [ [[package]] name = "acir_field" version = "0.22.0" +source = "git+https://github.com/noir-lang/acvm.git?rev=4b4e8a6a25b27e82bddbfe58caa3d421285b4a82#4b4e8a6a25b27e82bddbfe58caa3d421285b4a82" dependencies = [ "ark-bn254", "ark-ff", @@ -29,6 +31,7 @@ dependencies = [ [[package]] name = "acvm" version = "0.22.0" +source = "git+https://github.com/noir-lang/acvm.git?rev=4b4e8a6a25b27e82bddbfe58caa3d421285b4a82#4b4e8a6a25b27e82bddbfe58caa3d421285b4a82" dependencies = [ "acir", "acvm_blackbox_solver", @@ -44,6 +47,7 @@ dependencies = [ [[package]] name = "acvm-backend-barretenberg" version = "0.11.0" +source = "git+https://github.com/noir-lang/acvm-backend-barretenberg.git?rev=c80aee4434a2db502ed8b17ddd0b555f8f3e7c4b#c80aee4434a2db502ed8b17ddd0b555f8f3e7c4b" dependencies = [ "acvm", "barretenberg-sys", @@ -62,6 +66,7 @@ dependencies = [ [[package]] name = "acvm_blackbox_solver" version = "0.22.0" +source = "git+https://github.com/noir-lang/acvm.git?rev=4b4e8a6a25b27e82bddbfe58caa3d421285b4a82#4b4e8a6a25b27e82bddbfe58caa3d421285b4a82" dependencies = [ "acir", "blake2", @@ -75,6 +80,7 @@ dependencies = [ [[package]] name = "acvm_stdlib" version = "0.22.0" +source = "git+https://github.com/noir-lang/acvm.git?rev=4b4e8a6a25b27e82bddbfe58caa3d421285b4a82#4b4e8a6a25b27e82bddbfe58caa3d421285b4a82" dependencies = [ "acir", ] @@ -525,6 +531,7 @@ dependencies = [ [[package]] name = "brillig" version = "0.22.0" +source = "git+https://github.com/noir-lang/acvm.git?rev=4b4e8a6a25b27e82bddbfe58caa3d421285b4a82#4b4e8a6a25b27e82bddbfe58caa3d421285b4a82" dependencies = [ "acir_field", "serde", @@ -533,6 +540,7 @@ dependencies = [ [[package]] name = "brillig_vm" version = "0.22.0" +source = "git+https://github.com/noir-lang/acvm.git?rev=4b4e8a6a25b27e82bddbfe58caa3d421285b4a82#4b4e8a6a25b27e82bddbfe58caa3d421285b4a82" dependencies = [ "acir", "acvm_blackbox_solver", diff --git a/Cargo.toml b/Cargo.toml index 66ceb5b53d8..671288ea58d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -59,5 +59,5 @@ wasm-bindgen-test = "0.3.33" base64 = "0.21.2" [patch.crates-io] -acvm = { path = "/mnt/user-data/maxim/acvm/acvm" } -acvm-backend-barretenberg = { path = "/mnt/user-data/maxim/noir-lang/acvm-backend-barretenberg" } \ No newline at end of file +acvm = { git = "https://github.com/noir-lang/acvm.git", rev = "4b4e8a6a25b27e82bddbfe58caa3d421285b4a82" } +acvm-backend-barretenberg = { git = "https://github.com/noir-lang/acvm-backend-barretenberg.git", rev = "c80aee4434a2db502ed8b17ddd0b555f8f3e7c4b" } \ No newline at end of file diff --git a/crates/nargo/src/ops/execute.rs b/crates/nargo/src/ops/execute.rs index 2a126443468..ea1fe04baaf 100644 --- a/crates/nargo/src/ops/execute.rs +++ b/crates/nargo/src/ops/execute.rs @@ -12,7 +12,8 @@ pub fn execute_circuit( initial_witness: WitnessMap, show_output: bool, ) -> Result { - let mut acvm = ACVM::new(B::default(), circuit.opcodes, initial_witness); + let backend = B::default(); + let mut acvm = ACVM::new(&backend, circuit.opcodes, initial_witness); loop { let solver_status = acvm.solve(); diff --git a/crates/nargo_cli/src/cli/execute_cmd.rs b/crates/nargo_cli/src/cli/execute_cmd.rs index 8e5e55c3bb6..540c0ca3d51 100644 --- a/crates/nargo_cli/src/cli/execute_cmd.rs +++ b/crates/nargo_cli/src/cli/execute_cmd.rs @@ -41,7 +41,7 @@ pub(crate) struct ExecuteCommand { compile_options: CompileOptions, } -pub(crate) fn run( +pub(crate) fn run( backend: &B, args: ExecuteCommand, config: NargoConfig, @@ -70,7 +70,7 @@ pub(crate) fn run( Ok(()) } -fn execute_package( +fn execute_package( backend: &B, package: &Package, prover_name: &str, @@ -153,7 +153,7 @@ fn report_opcode_error( } } -pub(crate) fn execute_program( +pub(crate) fn execute_program( backend: &B, circuit: Circuit, abi: &Abi, diff --git a/crates/nargo_cli/src/cli/prove_cmd.rs b/crates/nargo_cli/src/cli/prove_cmd.rs index e4766828a5b..ae36b930ea4 100644 --- a/crates/nargo_cli/src/cli/prove_cmd.rs +++ b/crates/nargo_cli/src/cli/prove_cmd.rs @@ -51,7 +51,7 @@ pub(crate) struct ProveCommand { compile_options: CompileOptions, } -pub(crate) fn run( +pub(crate) fn run( backend: &B, args: ProveCommand, config: NargoConfig, @@ -82,7 +82,7 @@ pub(crate) fn run( } #[allow(clippy::too_many_arguments)] -pub(crate) fn prove_package( +pub(crate) fn prove_package( backend: &B, package: &Package, prover_name: &str, diff --git a/crates/nargo_cli/src/cli/test_cmd.rs b/crates/nargo_cli/src/cli/test_cmd.rs index 94c8ff86dcd..0ea83c36219 100644 --- a/crates/nargo_cli/src/cli/test_cmd.rs +++ b/crates/nargo_cli/src/cli/test_cmd.rs @@ -42,7 +42,7 @@ pub(crate) struct TestCommand { compile_options: CompileOptions, } -pub(crate) fn run( +pub(crate) fn run( backend: &B, args: TestCommand, config: NargoConfig, @@ -71,7 +71,7 @@ pub(crate) fn run( Ok(()) } -fn run_tests( +fn run_tests( backend: &B, package: &Package, test_name: FunctionNameMatch, @@ -120,7 +120,7 @@ fn run_tests( Ok(()) } -fn run_test( +fn run_test( backend: &B, test_name: &str, main: FuncId, diff --git a/crates/nargo_cli/tests/compile_failure/dynamic_index_failure/src/main.nr b/crates/nargo_cli/tests/compile_failure/dynamic_index_failure/src/main.nr index 78f6130cace..96700105bcd 100644 --- a/crates/nargo_cli/tests/compile_failure/dynamic_index_failure/src/main.nr +++ b/crates/nargo_cli/tests/compile_failure/dynamic_index_failure/src/main.nr @@ -1,25 +1,15 @@ fn main(mut x: [u32; 5], z: Field) { let idx = z + 10; - x[z] = 4; - - // Dynamic index is greater than length of the array - // assert(x[idx] != 0); - - // TODO(#2133): Provide more accurate call stacks for arrays merged in if statements if z == 20 { x[0] = x[4]; } else { - // TODO: Dynamic predicate still gives index out of bounds error + // We should not hit out of bounds here as we have a predicate + // that should not be hit if idx as u32 < 3 { x[idx] = 10; } - // assert(x[idx] != 0); - // x[idx] = 10; - // for i in 0..5 { - // x[idx] = x[i]; - // } } - // Error should occur here as the index out of bounds write occurs + // Error should occur here as an index out of bounds read occurs assert(x[idx] != 0); } \ No newline at end of file diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs b/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs index 03526b04b0b..582dc04eb23 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs @@ -899,7 +899,7 @@ impl AcirContext { &mut self, var_expressions: &mut Vec, input: AcirValue, - predicate: AcirVar + predicate: AcirVar, ) -> Result<(), InternalError> { match input { AcirValue::Var(var, _) => { @@ -1086,7 +1086,11 @@ impl AcirContext { // Add the memory read operation to the list of opcodes let op = MemOp::read_at_mem_index(index_witness.into(), value_read_witness); - self.acir_ir.push_opcode(Opcode::MemoryOp { block_id, op, predicate: Some(predicate_witness.into()) }); + self.acir_ir.push_opcode(Opcode::MemoryOp { + block_id, + op, + predicate: Some(predicate_witness.into()), + }); Ok(value_read_var) } @@ -1111,7 +1115,11 @@ impl AcirContext { // Add the memory write operation to the list of opcodes let op = MemOp::write_to_mem_index(index_witness.into(), value_write_witness.into()); - self.acir_ir.push_opcode(Opcode::MemoryOp { block_id, op, predicate: Some(predicate_witness.into()) }); + self.acir_ir.push_opcode(Opcode::MemoryOp { + block_id, + op, + predicate: Some(predicate_witness.into()), + }); Ok(()) } diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs index 100c5518350..e7112b8cd16 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -566,7 +566,11 @@ impl Context { } let index_var = self.convert_value(index, dfg).into_var()?; - let read = self.acir_context.read_from_memory(block_id, &index_var, &self.current_side_effects_enabled_var)?; + let read = self.acir_context.read_from_memory( + block_id, + &index_var, + &self.current_side_effects_enabled_var, + )?; let typ = match dfg.type_of_value(array) { Type::Array(typ, _) => { if typ.len() != 1 { @@ -647,7 +651,11 @@ impl Context { AcirType::NumericType(NumericType::NativeField), ); let var = index.into_var()?; - let read = self.acir_context.read_from_memory(block_id, &var, &self.current_side_effects_enabled_var)?; + let read = self.acir_context.read_from_memory( + block_id, + &var, + &self.current_side_effects_enabled_var, + )?; Ok(AcirValue::Var(read, AcirType::NumericType(NumericType::NativeField))) })?; self.initialize_array(result_block_id, len, Some(&init_values))?; @@ -656,7 +664,12 @@ impl Context { // Write the new value into the new array at the specified index let index_var = self.convert_value(index, dfg).into_var()?; let value_var = self.convert_value(store_value, dfg).into_var()?; - self.acir_context.write_to_memory(result_block_id, &index_var, &value_var, &self.current_side_effects_enabled_var)?; + self.acir_context.write_to_memory( + result_block_id, + &index_var, + &value_var, + &self.current_side_effects_enabled_var, + )?; let result_value = AcirValue::DynamicArray(AcirDynamicArray { block_id: result_block_id, len }); @@ -739,7 +752,7 @@ impl Context { if let Some(acir_value) = self.ssa_values.get(&value_id) { return acir_value.clone(); } - + let acir_value = match value { Value::NumericConstant { constant, typ } => { AcirValue::Var(self.acir_context.add_constant(*constant), typ.into()) diff --git a/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs b/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs index ed62b858713..85c3b0fd1cb 100644 --- a/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -551,7 +551,9 @@ impl<'a> FunctionContext<'a> { LValue::Ident } } - ast::LValue::Index { array, index, location, .. } => self.index_lvalue(array, index, location).2, + ast::LValue::Index { array, index, location, .. } => { + 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 object_lvalue = Box::new(object_lvalue); @@ -601,11 +603,16 @@ impl<'a> FunctionContext<'a> { // A slice is represented as a tuple (length, slice contents). // We need to fetch the second value. if array_values.len() > 1 { - let slice_lvalue = - LValue::SliceIndex { old_slice: old_array, index, slice_lvalue: array_lvalue, location }; + let slice_lvalue = LValue::SliceIndex { + old_slice: old_array, + index, + slice_lvalue: array_lvalue, + location, + }; (array_values[1], index, slice_lvalue, Some(array_values[0])) } else { - let array_lvalue = LValue::Index { old_array: array_values[0], index, array_lvalue, location }; + let array_lvalue = + LValue::Index { old_array: array_values[0], index, array_lvalue, location }; (array_values[0], index, array_lvalue, None) } } @@ -622,7 +629,8 @@ impl<'a> FunctionContext<'a> { } } ast::LValue::Index { array, index, element_type, location } => { - let (old_array, index, index_lvalue, max_length) = self.index_lvalue(array, index, 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) @@ -656,7 +664,8 @@ impl<'a> FunctionContext<'a> { LValue::SliceIndex { old_slice: slice, index, slice_lvalue, location } => { let mut slice_values = slice.into_value_list(self); - slice_values[1] = self.assign_lvalue_index(new_value, slice_values[1], index, location); + slice_values[1] = + self.assign_lvalue_index(new_value, slice_values[1], index, location); // The size of the slice does not change in a slice index assignment so we can reuse the same length value let new_slice = Tree::Branch(vec![slice_values[0].into(), slice_values[1].into()]); @@ -682,7 +691,8 @@ impl<'a> FunctionContext<'a> { let element_size = self.builder.field_constant(self.element_size(array)); // The actual base index is the user's index * the array element type's size - let mut index = self.builder.set_location(location).insert_binary(index, BinaryOp::Mul, element_size); + let mut index = + self.builder.set_location(location).insert_binary(index, BinaryOp::Mul, element_size); // let mut index = self.builder.insert_binary(index, BinaryOp::Mul, element_size); let one = self.builder.field_constant(FieldElement::one()); From c235e0a06cf82f8d0df6173652e1f5fb76114dde Mon Sep 17 00:00:00 2001 From: vezenovm Date: Wed, 30 Aug 2023 15:27:20 +0000 Subject: [PATCH 10/10] cargo fmt and mem_op_predicate regression inside of execution success --- .../tests/execution_success/array_dynamic/src/main.nr | 11 +++++++++++ crates/noirc_evaluator/src/ssa/acir_gen/mod.rs | 6 +++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/crates/nargo_cli/tests/execution_success/array_dynamic/src/main.nr b/crates/nargo_cli/tests/execution_success/array_dynamic/src/main.nr index db8f10b27e9..bc797f848df 100644 --- a/crates/nargo_cli/tests/execution_success/array_dynamic/src/main.nr +++ b/crates/nargo_cli/tests/execution_success/array_dynamic/src/main.nr @@ -15,6 +15,8 @@ fn main(x: [u32; 5], mut z: u32, t: u32, index: [Field;5], index2: [Field;5], of if 3 < (sublen as u32) { assert(index[offset + 3] == index2[3]); } + + regression_mem_op_predicate(x, idx + 10); } fn dyn_array(mut x: [u32; 5], y: Field, z: Field) { @@ -30,3 +32,12 @@ fn dyn_array(mut x: [u32; 5], y: Field, z: Field) { } assert(x[4] == 109); } + +fn regression_mem_op_predicate(mut x: [u32; 5], idx: Field) { + // We should not hit out of bounds here as we have a predicate + // that should not be hit + if idx as u32 < 3 { + x[idx] = 10; + } + assert(x[4] == 111); +} diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs index 9d7187d8ff7..762b7c8b607 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -378,7 +378,11 @@ impl Context { ); let index_var = index.into_var()?; - self.acir_context.read_from_memory(block_id, &index_var, &self.current_side_effects_enabled_var) + self.acir_context.read_from_memory( + block_id, + &index_var, + &self.current_side_effects_enabled_var, + ) }; for (lhs, rhs) in