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(acir): Attach locations to MemoryOps in ACIR #2389

Merged
merged 8 commits into from
Aug 22, 2023
Merged
Show file tree
Hide file tree
Changes from 3 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
51 changes: 36 additions & 15 deletions crates/nargo_cli/src/cli/execute_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ fn execute_package<B: Backend>(
// 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();
Expand All @@ -91,42 +90,64 @@ fn execute_package<B: Backend>(
Ok((return_value, solved_witness))
}

fn extract_unsatisfied_constraint_from_nargo_error(
fn extract_opcode_error_from_nargo_error(
nargo_err: &NargoError,
) -> Option<OpcodeLocation> {
) -> 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<OpcodeLocation>,
fn report_opcode_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, 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 = "Failed constraint".into();
CustomDiagnostic::simple_error(message, String::new(), location.span)
.in_file(location.file)
.with_call_stack(locations)
.report(&context.file_manager, false);
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:?}"
);
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 { .. } => {
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);
}
_ => (),
}
jfecher marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Expand All @@ -145,8 +166,8 @@ pub(crate) fn execute_program<B: Backend>(
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_opcode_error(opcode_err_info, &debug, &context);
}

Err(crate::errors::CliError::NargoError(err))
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "dynamic_index_failure"
type = "bin"
authors = [""]
compiler_version = "0.10.3"

[dependencies]
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
x = [104, 101, 108, 108, 111]
z = "4"
Original file line number Diff line number Diff line change
@@ -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)
}
11 changes: 4 additions & 7 deletions crates/noirc_evaluator/src/ssa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
jfecher marked this conversation as resolved.
Show resolved Hide resolved
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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -514,7 +514,7 @@
/// Note that this is not the euclidian division, where we have instead remainder < |rhs|
fn signed_division_var(
&mut self,
lhs: AcirVar,

Check warning on line 517 in crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (euclidian)
rhs: AcirVar,
bit_size: u32,
) -> Result<(AcirVar, AcirVar), RuntimeError> {
Expand Down Expand Up @@ -599,7 +599,7 @@

/// Returns an `AcirVar` which will be constrained to be lhs mod 2^{rhs}
/// In order to do this, we simply perform euclidian division of lhs by 2^{rhs}
/// The remainder of the division is then lhs mod 2^{rhs}

Check warning on line 602 in crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (euclidian)
pub(crate) fn truncate_var(
&mut self,
lhs: AcirVar,
Expand Down Expand Up @@ -933,7 +933,7 @@

/// Recursively create acir values for returned arrays. This is necessary because a brillig returned array can have nested arrays as elements.
/// A singular array of witnesses is collected for a top level array, by deflattening the assigned witnesses at each level.
fn brillig_array_output(

Check warning on line 936 in crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (deflattening)
&mut self,
element_types: &[AcirType],
size: usize,
Expand Down Expand Up @@ -1081,7 +1081,7 @@

// 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)
}
Expand All @@ -1102,7 +1102,8 @@

// 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(())
}

Expand All @@ -1128,7 +1129,7 @@
})?,
};

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(())
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
pub(crate) current_witness_index: u32,

/// The opcodes of which the compiled ACIR will comprise.
pub(crate) opcodes: Vec<AcirOpcode>,
opcodes: Vec<AcirOpcode>,

/// All witness indices that comprise the final return value of the program
///
Expand All @@ -45,7 +45,7 @@
/// All witness indices which are inputs to the main function
pub(crate) input_witnesses: Vec<Witness>,

/// Correspondance between an opcode index (in opcodes) and the source code call stack which generated it

Check warning on line 48 in crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (Correspondance)
pub(crate) locations: BTreeMap<OpcodeLocation, CallStack>,

/// Source code location of the current instruction being processed
Expand All @@ -60,14 +60,18 @@
}

/// 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
.insert(OpcodeLocation::Acir(self.opcodes.len() - 1), self.call_stack.clone());
}
}

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 {
Expand Down Expand Up @@ -228,7 +232,7 @@
}
};

self.opcodes.push(AcirOpcode::BlackBoxFuncCall(black_box_func_call));
self.push_opcode(AcirOpcode::BlackBoxFuncCall(black_box_func_call));

Ok(outputs_clone)
}
Expand Down Expand Up @@ -345,8 +349,8 @@
}

/// Signed division lhs / rhs
/// We derive the signed division from the unsigned euclidian division.

Check warning on line 352 in crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (euclidian)
/// note that this is not euclidian division!

Check warning on line 353 in crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (euclidian)
// if x is a signed integer, then sign(x)x >= 0
// so if a and b are signed integers, we can do the unsigned division:
// sign(a)a = q1*sign(b)b + r1
Expand Down Expand Up @@ -747,7 +751,7 @@
let two_max_bits: FieldElement = two.pow(&FieldElement::from(max_bits as i128));
let comparison_evaluation = (a - b) + two_max_bits;

// Euclidian division by 2^{max_bits} : 2^{max_bits} + a - b = q * 2^{max_bits} + r

Check warning on line 754 in crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (Euclidian)
//
// 2^{max_bits} is of max_bits+1 bit size
// If a>b, then a-b is less than 2^{max_bits} - 1, so 2^{max_bits} + a - b is less than 2^{max_bits} + 2^{max_bits} - 1 = 2^{max_bits+1} - 1
Expand Down
2 changes: 1 addition & 1 deletion crates/noirc_evaluator/src/ssa/acir_gen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]);
}
}
5 changes: 4 additions & 1 deletion crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,10 @@
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) => {
Expand Down Expand Up @@ -344,7 +347,7 @@
self.builder.insert_cast(lhs, typ).into()
}

/// Codegens a for loop, creating three new blocks in the process.

Check warning on line 350 in crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (Codegens)
/// The return value of a for loop is always a unit literal.
///
/// For example, the loop `for i in start .. end { body }` is codegen'd as:
Expand All @@ -354,7 +357,7 @@
/// br loop_entry(v0)
/// loop_entry(i: Field):
/// v2 = lt i v1
/// brif v2, then: loop_body, else: loop_end

Check warning on line 360 in crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (brif)
/// loop_body():
/// v3 = ... codegen body ...
/// v4 = add 1, i
Expand Down Expand Up @@ -403,7 +406,7 @@
Self::unit_value()
}

/// Codegens an if expression, handling the case of what to do if there is no 'else'.

Check warning on line 409 in crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (Codegens)
///
/// For example, the expression `if cond { a } else { b }` is codegen'd as:
///
Expand Down
Loading