From 93bb7a7b5b9799e13dac4edffdaa17813c2e0694 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 21 Aug 2024 21:32:45 +0000 Subject: [PATCH 01/12] add optional acir index for brillig opcode location and appropriately handle multiple brillig entry points in call stacks --- acvm-repo/acir/src/circuit/mod.rs | 27 +++++++- acvm-repo/acvm/src/compiler/mod.rs | 11 ++- acvm-repo/acvm/src/pwg/brillig.rs | 2 +- acvm-repo/acvm/tests/solver.rs | 2 +- compiler/noirc_evaluator/src/ssa.rs | 1 + .../ssa/acir_gen/acir_ir/generated_acir.rs | 17 +++-- .../brillig_calls/src/main.nr | 3 + tooling/debugger/src/context.rs | 68 +++++++++++++------ tooling/debugger/src/repl.rs | 16 +++-- tooling/nargo/src/errors.rs | 6 +- .../profiler/src/cli/gates_flamegraph_cmd.rs | 3 +- .../src/cli/opcodes_flamegraph_cmd.rs | 7 +- tooling/profiler/src/flamegraph.rs | 27 +++++++- tooling/profiler/src/gates_provider.rs | 1 - 14 files changed, 144 insertions(+), 47 deletions(-) diff --git a/acvm-repo/acir/src/circuit/mod.rs b/acvm-repo/acir/src/circuit/mod.rs index 43984e4a922..c26fc7f2494 100644 --- a/acvm-repo/acir/src/circuit/mod.rs +++ b/acvm-repo/acir/src/circuit/mod.rs @@ -153,7 +153,19 @@ pub struct ResolvedOpcodeLocation { /// map opcodes to debug information related to their context. pub enum OpcodeLocation { Acir(usize), - Brillig { acir_index: usize, brillig_index: usize }, + Brillig { acir_index: Option, brillig_index: usize }, + // BrilligUnresolved { brillig_index: usize }, +} + +impl OpcodeLocation { + pub fn reset_brillig_acir_index(self) -> Self { + match self { + OpcodeLocation::Brillig { brillig_index, .. } => { + OpcodeLocation::Brillig { acir_index: None, brillig_index } + } + _ => self, + } + } } impl std::fmt::Display for OpcodeLocation { @@ -161,7 +173,11 @@ impl std::fmt::Display for OpcodeLocation { match self { OpcodeLocation::Acir(index) => write!(f, "{index}"), OpcodeLocation::Brillig { acir_index, brillig_index } => { - write!(f, "{acir_index}.{brillig_index}") + if let Some(acir_index) = acir_index { + write!(f, "{acir_index}.{brillig_index}") + } else { + write!(f, "unknown_acir_index.{brillig_index}") + } } } } @@ -191,7 +207,12 @@ impl FromStr for OpcodeLocation { Ok(OpcodeLocation::Acir(index)) } 2 => { - let acir_index = parts[0].parse()?; + // let acir_index = parts[0].parse()?; + let acir_index = if let Ok(acir_index) = parts[0].parse::() { + Some(acir_index) + } else { + None + }; let brillig_index = parts[1].parse()?; Ok(OpcodeLocation::Brillig { acir_index, brillig_index }) } diff --git a/acvm-repo/acvm/src/compiler/mod.rs b/acvm-repo/acvm/src/compiler/mod.rs index 5ece3d19a6e..a2531524c33 100644 --- a/acvm-repo/acvm/src/compiler/mod.rs +++ b/acvm-repo/acvm/src/compiler/mod.rs @@ -40,7 +40,14 @@ impl AcirTransformationMap { ) -> impl Iterator + '_ { let old_acir_index = match old_location { OpcodeLocation::Acir(index) => index, - OpcodeLocation::Brillig { acir_index, .. } => acir_index, + OpcodeLocation::Brillig { acir_index, .. } => { + if let Some(acir_index) = acir_index { + acir_index + } else { + panic!("ICE: Should only be transforming ACIR locations") + } + } // _ => panic!("ICE: Should only be transforming ACIR locations"), + // _ => return }; self.old_indices_to_new_indices.get(&old_acir_index).into_iter().flat_map( @@ -48,7 +55,7 @@ impl AcirTransformationMap { new_indices.iter().map(move |new_index| match old_location { OpcodeLocation::Acir(_) => OpcodeLocation::Acir(*new_index), OpcodeLocation::Brillig { brillig_index, .. } => { - OpcodeLocation::Brillig { acir_index: *new_index, brillig_index } + OpcodeLocation::Brillig { acir_index: Some(*new_index), brillig_index } } }) }, diff --git a/acvm-repo/acvm/src/pwg/brillig.rs b/acvm-repo/acvm/src/pwg/brillig.rs index 5ec3224dbaa..ab5a43ba2a9 100644 --- a/acvm-repo/acvm/src/pwg/brillig.rs +++ b/acvm-repo/acvm/src/pwg/brillig.rs @@ -170,7 +170,7 @@ impl<'b, B: BlackBoxFunctionSolver, F: AcirField> BrilligSolver<'b, F, B> { let call_stack = call_stack .iter() .map(|brillig_index| OpcodeLocation::Brillig { - acir_index: self.acir_index, + acir_index: Some(self.acir_index), brillig_index: *brillig_index, }) .collect(); diff --git a/acvm-repo/acvm/tests/solver.rs b/acvm-repo/acvm/tests/solver.rs index a1b8b62f8bf..ef786972966 100644 --- a/acvm-repo/acvm/tests/solver.rs +++ b/acvm-repo/acvm/tests/solver.rs @@ -677,7 +677,7 @@ fn unsatisfied_opcode_resolved_brillig() { ACVMStatus::Failure(OpcodeResolutionError::BrilligFunctionFailed { function_id: BrilligFunctionId(0), payload: None, - call_stack: vec![OpcodeLocation::Brillig { acir_index: 0, brillig_index: 3 }] + call_stack: vec![OpcodeLocation::Brillig { acir_index: Some(0), brillig_index: 3 }] }), "The first opcode is not satisfiable, expected an error indicating this" ); diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 9daf98e606b..4852416a329 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -275,6 +275,7 @@ fn convert_generated_acir_into_circuit( .. } = generated_acir; + dbg!(brillig_locations.keys().collect::>()); let (public_parameter_witnesses, private_parameters) = split_public_and_private_inputs(&func_sig, &input_witnesses); diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs index 661371c5de6..4d9bfc502bc 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs @@ -1,6 +1,6 @@ //! `GeneratedAcir` is constructed as part of the `acir_gen` pass to accumulate all of the ACIR //! program as it is being converted from SSA form. -use std::collections::BTreeMap; +use std::{collections::BTreeMap, u32}; use crate::{ brillig::{brillig_gen::brillig_directive, brillig_ir::artifact::GeneratedBrillig}, @@ -27,7 +27,7 @@ use num_bigint::BigUint; /// This index should be used when adding a Brillig call during code generation. /// Code generation should then keep track of that unresolved call opcode which will be resolved with the /// correct function index after code generation. -pub(crate) const PLACEHOLDER_BRILLIG_INDEX: BrilligFunctionId = BrilligFunctionId(0); +pub(crate) const PLACEHOLDER_BRILLIG_INDEX: BrilligFunctionId = BrilligFunctionId(u32::MAX); #[derive(Debug, Default)] /// The output of the Acir-gen pass, which should only be produced for entry point Acir functions @@ -593,7 +593,8 @@ impl GeneratedAcir { for (brillig_index, message) in generated_brillig.assert_messages.iter() { self.assertion_payloads.insert( OpcodeLocation::Brillig { - acir_index: self.opcodes.len() - 1, + acir_index: Some(self.opcodes.len() - 1), + // acir_index: None, brillig_index: *brillig_index, }, AssertionPayload::StaticString(message.clone()), @@ -607,12 +608,17 @@ impl GeneratedAcir { for (brillig_index, call_stack) in generated_brillig.locations.iter() { self.brillig_locations.entry(brillig_function_index).or_default().insert( OpcodeLocation::Brillig { - acir_index: self.opcodes.len() - 1, + // acir_index: self.opcodes.len() - 1, + acir_index: None, brillig_index: *brillig_index, }, call_stack.clone(), ); } + + // if generated_brillig.locations.is_empty() { + // self.brillig_locations.insert(brillig_function_index, Default::default()); + // } } // We can only resolve the Brillig stdlib after having processed the entire ACIR @@ -625,6 +631,9 @@ impl GeneratedAcir { OpcodeLocation::Acir(index) => index, _ => panic!("should not have brillig index"), }; + + self.brillig_locations.insert(brillig_function_index, Default::default()); + match &mut self.opcodes[acir_index] { AcirOpcode::BrilligCall { id, .. } => *id = brillig_function_index, _ => panic!("expected brillig call opcode"), diff --git a/test_programs/execution_success/brillig_calls/src/main.nr b/test_programs/execution_success/brillig_calls/src/main.nr index 3e23da53b18..e83dfff542b 100644 --- a/test_programs/execution_success/brillig_calls/src/main.nr +++ b/test_programs/execution_success/brillig_calls/src/main.nr @@ -7,6 +7,9 @@ fn main(x: u32) { swap_entry_point(x, x + 1); assert(deep_entry_point(x) == 4); multiple_values_entry_point(x); + + assert(entry_point(x) != 10); + assert(entry_point(x) != 11); } } diff --git a/tooling/debugger/src/context.rs b/tooling/debugger/src/context.rs index 890732b579c..2edd6b0bd0f 100644 --- a/tooling/debugger/src/context.rs +++ b/tooling/debugger/src/context.rs @@ -124,7 +124,9 @@ impl AddressMap { match &location.opcode_location { OpcodeLocation::Acir(acir_index) => circuit_addresses[*acir_index], OpcodeLocation::Brillig { acir_index, brillig_index } => { - circuit_addresses[*acir_index] + *brillig_index + let acir_index = + acir_index.expect("Debug locations should have a resolved acir index"); + circuit_addresses[acir_index] + *brillig_index } } } @@ -162,7 +164,10 @@ impl AddressMap { && address <= brillig_address_space.end_address }) .map(|brillig_address_space| brillig_address_space.brillig_function_id); - (OpcodeLocation::Brillig { acir_index, brillig_index }, brillig_function_id) + ( + OpcodeLocation::Brillig { acir_index: Some(acir_index), brillig_index }, + brillig_function_id, + ) } }; @@ -183,6 +188,8 @@ impl std::fmt::Display for DebugLocation { match self.opcode_location { OpcodeLocation::Acir(index) => write!(f, "{circuit_id}:{index}"), OpcodeLocation::Brillig { acir_index, brillig_index } => { + let acir_index = + acir_index.expect("Debug locations should have a resolved acir index"); write!(f, "{circuit_id}:{acir_index}.{brillig_index}") } } @@ -319,7 +326,7 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { let function_id = solver.function_id; ( OpcodeLocation::Brillig { - acir_index: ip, + acir_index: Some(ip), brillig_index: solver.program_counter(), }, Some(function_id), @@ -354,7 +361,7 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { frames.extend(solver.get_call_stack().iter().map(|program_counter| DebugLocation { circuit_id, opcode_location: OpcodeLocation::Brillig { - acir_index: instruction_pointer, + acir_index: Some(instruction_pointer), brillig_index: *program_counter, }, brillig_function_id: Some(solver.function_id), @@ -499,14 +506,18 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { _ => format!("{opcode:?}"), } } - OpcodeLocation::Brillig { acir_index, brillig_index } => match &opcodes[*acir_index] { - Opcode::BrilligCall { id, .. } => { - let bytecode = &self.unconstrained_functions[id.as_usize()].bytecode; - let opcode = &bytecode[*brillig_index]; - format!(" | {opcode:?}") + OpcodeLocation::Brillig { acir_index, brillig_index } => { + let acir_index = + acir_index.expect("Debug locations should have a resolved acir index"); + match &opcodes[acir_index] { + Opcode::BrilligCall { id, .. } => { + let bytecode = &self.unconstrained_functions[id.as_usize()].bytecode; + let opcode = &bytecode[*brillig_index]; + format!(" | {opcode:?}") + } + _ => String::from(" | invalid"), } - _ => String::from(" | invalid"), - }, + } } } @@ -661,7 +672,9 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { self.get_current_debug_location().map(|debug_location| { match debug_location.opcode_location { OpcodeLocation::Acir(acir_index) => acir_index, - OpcodeLocation::Brillig { acir_index, .. } => acir_index, + OpcodeLocation::Brillig { acir_index, .. } => { + acir_index.expect("Debug locations should have a resolved acir index") + } } }) } @@ -811,6 +824,8 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { match location.opcode_location { OpcodeLocation::Acir(acir_index) => acir_index < opcodes.len(), OpcodeLocation::Brillig { acir_index, brillig_index } => { + let acir_index = + acir_index.expect("Debug locations should have a resolved acir index"); if acir_index < opcodes.len() { match &opcodes[acir_index] { Opcode::BrilligCall { id, .. } => { @@ -1032,7 +1047,7 @@ mod tests { context.get_current_debug_location(), Some(DebugLocation { circuit_id: 0, - opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 1 }, + opcode_location: OpcodeLocation::Brillig { acir_index: Some(0), brillig_index: 1 }, brillig_function_id: Some(BrilligFunctionId(0)), }) ); @@ -1044,7 +1059,7 @@ mod tests { context.get_current_debug_location(), Some(DebugLocation { circuit_id: 0, - opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 2 }, + opcode_location: OpcodeLocation::Brillig { acir_index: Some(0), brillig_index: 2 }, brillig_function_id: Some(BrilligFunctionId(0)), }) ); @@ -1056,7 +1071,7 @@ mod tests { context.get_current_debug_location(), Some(DebugLocation { circuit_id: 0, - opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 2 }, + opcode_location: OpcodeLocation::Brillig { acir_index: Some(0), brillig_index: 2 }, brillig_function_id: Some(BrilligFunctionId(0)), }) ); @@ -1068,7 +1083,7 @@ mod tests { context.get_current_debug_location(), Some(DebugLocation { circuit_id: 0, - opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 3 }, + opcode_location: OpcodeLocation::Brillig { acir_index: Some(0), brillig_index: 3 }, brillig_function_id: Some(BrilligFunctionId(0)), }) ); @@ -1153,7 +1168,7 @@ mod tests { // set breakpoint let breakpoint_location = DebugLocation { circuit_id: 0, - opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 1 }, + opcode_location: OpcodeLocation::Brillig { acir_index: Some(0), brillig_index: 1 }, brillig_function_id: Some(BrilligFunctionId(0)), }; assert!(context.add_breakpoint(breakpoint_location)); @@ -1264,7 +1279,10 @@ mod tests { }), Some(DebugLocation { circuit_id: 0, - opcode_location: OpcodeLocation::Brillig { acir_index: 1, brillig_index: 1 }, + opcode_location: OpcodeLocation::Brillig { + acir_index: Some(1), + brillig_index: 1 + }, brillig_function_id: Some(BrilligFunctionId(0)), }), Some(DebugLocation { @@ -1284,12 +1302,18 @@ mod tests { }), Some(DebugLocation { circuit_id: 1, - opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 1 }, + opcode_location: OpcodeLocation::Brillig { + acir_index: Some(0), + brillig_index: 1 + }, brillig_function_id: Some(BrilligFunctionId(1)), }), Some(DebugLocation { circuit_id: 1, - opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 2 }, + opcode_location: OpcodeLocation::Brillig { + acir_index: Some(0), + brillig_index: 2 + }, brillig_function_id: Some(BrilligFunctionId(1)), }), Some(DebugLocation { @@ -1315,7 +1339,7 @@ mod tests { 1, context.debug_location_to_address(&DebugLocation { circuit_id: 0, - opcode_location: OpcodeLocation::Brillig { acir_index: 1, brillig_index: 0 }, + opcode_location: OpcodeLocation::Brillig { acir_index: Some(1), brillig_index: 0 }, brillig_function_id: Some(BrilligFunctionId(0)), }) ); @@ -1323,7 +1347,7 @@ mod tests { 5, context.debug_location_to_address(&DebugLocation { circuit_id: 1, - opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 0 }, + opcode_location: OpcodeLocation::Brillig { acir_index: Some(0), brillig_index: 0 }, brillig_function_id: Some(BrilligFunctionId(1)), }) ); diff --git a/tooling/debugger/src/repl.rs b/tooling/debugger/src/repl.rs index 1a7c2d6c7a8..73258ddde65 100644 --- a/tooling/debugger/src/repl.rs +++ b/tooling/debugger/src/repl.rs @@ -81,8 +81,10 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { println!("At opcode {} :: {}", location, opcodes[*ip]); } OpcodeLocation::Brillig { acir_index, brillig_index } => { + let acir_index = + acir_index.expect("Debug locations should have a resolved acir index"); let brillig_bytecode = - if let Opcode::BrilligCall { id, .. } = opcodes[*acir_index] { + if let Opcode::BrilligCall { id, .. } = opcodes[acir_index] { &self.unconstrained_functions[id.as_usize()].bytecode } else { unreachable!("Brillig location does not contain Brillig opcodes"); @@ -109,8 +111,9 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { ) } OpcodeLocation::Brillig { acir_index, brillig_index } => { - let brillig_bytecode = if let Opcode::BrilligCall { id, .. } = opcodes[*acir_index] - { + let acir_index = + acir_index.expect("Debug locations should have a resolved acir index"); + let brillig_bytecode = if let Opcode::BrilligCall { id, .. } = opcodes[acir_index] { &self.unconstrained_functions[id.as_usize()].bytecode } else { unreachable!("Brillig location does not contain Brillig opcodes"); @@ -155,7 +158,7 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { let opcodes = self.context.get_opcodes_of_circuit(circuit_id); let current_acir_index = match current_opcode_location { Some(OpcodeLocation::Acir(ip)) => Some(ip), - Some(OpcodeLocation::Brillig { acir_index, .. }) => Some(acir_index), + Some(OpcodeLocation::Brillig { acir_index, .. }) => acir_index, None => None, }; let current_brillig_index = match current_opcode_location { @@ -180,7 +183,10 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { "->" } else if self.context.is_breakpoint_set(&DebugLocation { circuit_id, - opcode_location: OpcodeLocation::Brillig { acir_index, brillig_index }, + opcode_location: OpcodeLocation::Brillig { + acir_index: Some(acir_index), + brillig_index, + }, brillig_function_id: Some(brillig_function_id), }) { " *" diff --git a/tooling/nargo/src/errors.rs b/tooling/nargo/src/errors.rs index f9668653d0b..9a7e949b765 100644 --- a/tooling/nargo/src/errors.rs +++ b/tooling/nargo/src/errors.rs @@ -130,9 +130,11 @@ fn extract_locations_from_error( opcode_location: OpcodeLocation::Brillig { acir_index, .. }, } = resolved_location { + let acir_index = acir_index + .expect("Resolved Brillig opcode locations are expected to have an ACIR index"); let acir_location = ResolvedOpcodeLocation { acir_function_index: *acir_function_index, - opcode_location: OpcodeLocation::Acir(*acir_index), + opcode_location: OpcodeLocation::Acir(acir_index), }; opcode_locations.insert(i, acir_location); @@ -164,7 +166,7 @@ fn extract_locations_from_error( .get(&brillig_function_id); brillig_locations .unwrap() - .get(&resolved_location.opcode_location) + .get(&resolved_location.opcode_location.reset_brillig_acir_index()) .cloned() .unwrap_or_default() } else { diff --git a/tooling/profiler/src/cli/gates_flamegraph_cmd.rs b/tooling/profiler/src/cli/gates_flamegraph_cmd.rs index 0fa12239d05..d5fefc4ecda 100644 --- a/tooling/profiler/src/cli/gates_flamegraph_cmd.rs +++ b/tooling/profiler/src/cli/gates_flamegraph_cmd.rs @@ -22,7 +22,7 @@ pub(crate) struct GatesFlamegraphCommand { backend_path: String, /// Command to get a gates report from the backend. Defaults to "gates" - #[clap(long, short, default_value = "gates")] + #[clap(long, short = 'g', default_value = "gates")] backend_gates_command: String, #[arg(trailing_var_arg = true, allow_hyphen_values = true)] @@ -87,6 +87,7 @@ fn run_with_provider( opcode: AcirOrBrilligOpcode::Acir(opcode), call_stack: vec![OpcodeLocation::Acir(index)], count: gates, + brillig_function_id: None, }) .collect(); diff --git a/tooling/profiler/src/cli/opcodes_flamegraph_cmd.rs b/tooling/profiler/src/cli/opcodes_flamegraph_cmd.rs index d7f3cbb9b85..faba5ff3892 100644 --- a/tooling/profiler/src/cli/opcodes_flamegraph_cmd.rs +++ b/tooling/profiler/src/cli/opcodes_flamegraph_cmd.rs @@ -1,5 +1,6 @@ use std::path::{Path, PathBuf}; +use acir::circuit::brillig::BrilligFunctionId; use acir::circuit::{Circuit, Opcode, OpcodeLocation}; use clap::Args; use color_eyre::eyre::{self, Context}; @@ -20,7 +21,7 @@ pub(crate) struct OpcodesFlamegraphCommand { #[clap(long, short)] output: String, - /// Wether to skip brillig functions + /// Whether to skip brillig functions #[clap(long, short, action)] skip_brillig: bool, } @@ -62,6 +63,7 @@ fn run_with_generator( opcode: AcirOrBrilligOpcode::Acir(opcode.clone()), call_stack: vec![OpcodeLocation::Acir(index)], count: 1, + brillig_function_id: None, }) .collect(); @@ -97,10 +99,11 @@ fn run_with_generator( .map(|(brillig_index, opcode)| Sample { opcode: AcirOrBrilligOpcode::Brillig(opcode), call_stack: vec![OpcodeLocation::Brillig { - acir_index: acir_opcode_index, + acir_index: Some(acir_opcode_index), brillig_index, }], count: 1, + brillig_function_id: Some(BrilligFunctionId(brillig_fn_index as u32)), }) .collect(); diff --git a/tooling/profiler/src/flamegraph.rs b/tooling/profiler/src/flamegraph.rs index da76f9b9938..a20af479e70 100644 --- a/tooling/profiler/src/flamegraph.rs +++ b/tooling/profiler/src/flamegraph.rs @@ -1,6 +1,7 @@ use std::path::Path; use std::{collections::BTreeMap, io::BufWriter}; +use acir::circuit::brillig::BrilligFunctionId; use acir::circuit::OpcodeLocation; use acir::AcirField; use color_eyre::eyre::{self}; @@ -19,6 +20,7 @@ pub(crate) struct Sample { pub(crate) opcode: AcirOrBrilligOpcode, pub(crate) call_stack: Vec, pub(crate) count: usize, + pub(crate) brillig_function_id: Option, } #[derive(Debug, Default)] @@ -90,9 +92,25 @@ fn generate_folded_sorted_lines<'files, F: AcirField>( let mut location_names: Vec = sample .call_stack .into_iter() - .flat_map(|opcode_location| debug_symbols.locations.get(&opcode_location)) - .flatten() - .map(|location| location_to_callsite_label(*location, files)) + .flat_map(|opcode_location| { + debug_symbols.opcode_location(&opcode_location).unwrap_or_else(|| { + if let Some(brillig_function_id) = sample.brillig_function_id { + let brillig_locations = + debug_symbols.brillig_locations.get(&brillig_function_id); + if let Some(brillig_locations) = brillig_locations { + brillig_locations + .get(&opcode_location.reset_brillig_acir_index()) + .cloned() + .unwrap_or_default() + } else { + vec![] + } + } else { + vec![] + } + }) + }) + .map(|location| location_to_callsite_label(location, files)) .collect(); if location_names.is_empty() { @@ -286,11 +304,13 @@ mod tests { opcode: AcirOrBrilligOpcode::Acir(AcirOpcode::AssertZero(Expression::default())), call_stack: vec![OpcodeLocation::Acir(0)], count: 10, + brillig_function_id: None, }, Sample { opcode: AcirOrBrilligOpcode::Acir(AcirOpcode::AssertZero(Expression::default())), call_stack: vec![OpcodeLocation::Acir(1)], count: 20, + brillig_function_id: None, }, Sample { opcode: AcirOrBrilligOpcode::Acir(AcirOpcode::MemoryInit { @@ -300,6 +320,7 @@ mod tests { }), call_stack: vec![OpcodeLocation::Acir(2)], count: 30, + brillig_function_id: None, }, ]; diff --git a/tooling/profiler/src/gates_provider.rs b/tooling/profiler/src/gates_provider.rs index 3f07f3e4be6..5758205b5d6 100644 --- a/tooling/profiler/src/gates_provider.rs +++ b/tooling/profiler/src/gates_provider.rs @@ -17,7 +17,6 @@ pub(crate) struct BackendGatesProvider { impl GatesProvider for BackendGatesProvider { fn get_gates(&self, artifact_path: &Path) -> eyre::Result { let mut backend_gates_cmd = Command::new(&self.backend_path); - backend_gates_cmd.arg(self.gates_command.clone()).arg("-b").arg(artifact_path); for arg in &self.extra_args { From 92e8b64d619f925ad44cf1e25d5b2c40e4e2059b Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 22 Aug 2024 02:00:46 +0000 Subject: [PATCH 02/12] don't insert locations for brillig stdlib --- .../src/ssa/acir_gen/acir_ir/generated_acir.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs index 4d9bfc502bc..e4b0435103d 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs @@ -594,7 +594,6 @@ impl GeneratedAcir { self.assertion_payloads.insert( OpcodeLocation::Brillig { acir_index: Some(self.opcodes.len() - 1), - // acir_index: None, brillig_index: *brillig_index, }, AssertionPayload::StaticString(message.clone()), @@ -608,17 +607,12 @@ impl GeneratedAcir { for (brillig_index, call_stack) in generated_brillig.locations.iter() { self.brillig_locations.entry(brillig_function_index).or_default().insert( OpcodeLocation::Brillig { - // acir_index: self.opcodes.len() - 1, acir_index: None, brillig_index: *brillig_index, }, call_stack.clone(), ); } - - // if generated_brillig.locations.is_empty() { - // self.brillig_locations.insert(brillig_function_index, Default::default()); - // } } // We can only resolve the Brillig stdlib after having processed the entire ACIR @@ -632,7 +626,7 @@ impl GeneratedAcir { _ => panic!("should not have brillig index"), }; - self.brillig_locations.insert(brillig_function_index, Default::default()); + // self.brillig_locations.insert(brillig_function_index, Default::default()); match &mut self.opcodes[acir_index] { AcirOpcode::BrilligCall { id, .. } => *id = brillig_function_index, From 4712deb1d2778af9fc4a2afe97e2f24891edf23a Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 22 Aug 2024 02:02:43 +0000 Subject: [PATCH 03/12] cargo fmt --- .../src/ssa/acir_gen/acir_ir/generated_acir.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs index e4b0435103d..8bd53d11734 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs @@ -606,10 +606,7 @@ impl GeneratedAcir { for (brillig_index, call_stack) in generated_brillig.locations.iter() { self.brillig_locations.entry(brillig_function_index).or_default().insert( - OpcodeLocation::Brillig { - acir_index: None, - brillig_index: *brillig_index, - }, + OpcodeLocation::Brillig { acir_index: None, brillig_index: *brillig_index }, call_stack.clone(), ); } From 03116387ff2dfdf011040dc88bc076179a9a4818 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 22 Aug 2024 02:14:59 +0000 Subject: [PATCH 04/12] cleanup old comments and dbg --- acvm-repo/acir/src/circuit/mod.rs | 5 ++++- acvm-repo/acvm/src/compiler/mod.rs | 3 +-- compiler/noirc_evaluator/src/ssa.rs | 1 - .../src/ssa/acir_gen/acir_ir/generated_acir.rs | 4 +--- test_programs/execution_success/brillig_calls/src/main.nr | 3 --- tooling/profiler/src/gates_provider.rs | 1 + 6 files changed, 7 insertions(+), 10 deletions(-) diff --git a/acvm-repo/acir/src/circuit/mod.rs b/acvm-repo/acir/src/circuit/mod.rs index c26fc7f2494..ab7dfca4df4 100644 --- a/acvm-repo/acir/src/circuit/mod.rs +++ b/acvm-repo/acir/src/circuit/mod.rs @@ -153,8 +153,11 @@ pub struct ResolvedOpcodeLocation { /// map opcodes to debug information related to their context. pub enum OpcodeLocation { Acir(usize), + // A Brillig opcode location can have either a resolved or unresolved acir location. + // Resolved locations are generally specified when returning an error during execution. + // The acir index is not actually needed to determine a Brillig location + // as it can be resolved separately. Brillig { acir_index: Option, brillig_index: usize }, - // BrilligUnresolved { brillig_index: usize }, } impl OpcodeLocation { diff --git a/acvm-repo/acvm/src/compiler/mod.rs b/acvm-repo/acvm/src/compiler/mod.rs index a2531524c33..1d191276df1 100644 --- a/acvm-repo/acvm/src/compiler/mod.rs +++ b/acvm-repo/acvm/src/compiler/mod.rs @@ -46,8 +46,7 @@ impl AcirTransformationMap { } else { panic!("ICE: Should only be transforming ACIR locations") } - } // _ => panic!("ICE: Should only be transforming ACIR locations"), - // _ => return + } }; self.old_indices_to_new_indices.get(&old_acir_index).into_iter().flat_map( diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 4852416a329..9daf98e606b 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -275,7 +275,6 @@ fn convert_generated_acir_into_circuit( .. } = generated_acir; - dbg!(brillig_locations.keys().collect::>()); let (public_parameter_witnesses, private_parameters) = split_public_and_private_inputs(&func_sig, &input_witnesses); diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs index 8bd53d11734..fb0d1d9439a 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs @@ -27,7 +27,7 @@ use num_bigint::BigUint; /// This index should be used when adding a Brillig call during code generation. /// Code generation should then keep track of that unresolved call opcode which will be resolved with the /// correct function index after code generation. -pub(crate) const PLACEHOLDER_BRILLIG_INDEX: BrilligFunctionId = BrilligFunctionId(u32::MAX); +pub(crate) const PLACEHOLDER_BRILLIG_INDEX: BrilligFunctionId = BrilligFunctionId(0); #[derive(Debug, Default)] /// The output of the Acir-gen pass, which should only be produced for entry point Acir functions @@ -623,8 +623,6 @@ impl GeneratedAcir { _ => panic!("should not have brillig index"), }; - // self.brillig_locations.insert(brillig_function_index, Default::default()); - match &mut self.opcodes[acir_index] { AcirOpcode::BrilligCall { id, .. } => *id = brillig_function_index, _ => panic!("expected brillig call opcode"), diff --git a/test_programs/execution_success/brillig_calls/src/main.nr b/test_programs/execution_success/brillig_calls/src/main.nr index e83dfff542b..3e23da53b18 100644 --- a/test_programs/execution_success/brillig_calls/src/main.nr +++ b/test_programs/execution_success/brillig_calls/src/main.nr @@ -7,9 +7,6 @@ fn main(x: u32) { swap_entry_point(x, x + 1); assert(deep_entry_point(x) == 4); multiple_values_entry_point(x); - - assert(entry_point(x) != 10); - assert(entry_point(x) != 11); } } diff --git a/tooling/profiler/src/gates_provider.rs b/tooling/profiler/src/gates_provider.rs index 5758205b5d6..3f07f3e4be6 100644 --- a/tooling/profiler/src/gates_provider.rs +++ b/tooling/profiler/src/gates_provider.rs @@ -17,6 +17,7 @@ pub(crate) struct BackendGatesProvider { impl GatesProvider for BackendGatesProvider { fn get_gates(&self, artifact_path: &Path) -> eyre::Result { let mut backend_gates_cmd = Command::new(&self.backend_path); + backend_gates_cmd.arg(self.gates_command.clone()).arg("-b").arg(artifact_path); for arg in &self.extra_args { From 27a41c73dc2505b07208f843fe31ac32cbe07ab9 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 22 Aug 2024 02:15:20 +0000 Subject: [PATCH 05/12] another comment --- acvm-repo/acir/src/circuit/mod.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/acvm-repo/acir/src/circuit/mod.rs b/acvm-repo/acir/src/circuit/mod.rs index ab7dfca4df4..3ca69fd9cd8 100644 --- a/acvm-repo/acir/src/circuit/mod.rs +++ b/acvm-repo/acir/src/circuit/mod.rs @@ -210,7 +210,6 @@ impl FromStr for OpcodeLocation { Ok(OpcodeLocation::Acir(index)) } 2 => { - // let acir_index = parts[0].parse()?; let acir_index = if let Ok(acir_index) = parts[0].parse::() { Some(acir_index) } else { From 88506e2c46b086488ab9ca3152dd89ebb5160d96 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 22 Aug 2024 02:16:09 +0000 Subject: [PATCH 06/12] rename to reset_acir_index --- acvm-repo/acir/src/circuit/mod.rs | 2 +- tooling/nargo/src/errors.rs | 2 +- tooling/profiler/src/flamegraph.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/acvm-repo/acir/src/circuit/mod.rs b/acvm-repo/acir/src/circuit/mod.rs index 3ca69fd9cd8..1f6f61b9383 100644 --- a/acvm-repo/acir/src/circuit/mod.rs +++ b/acvm-repo/acir/src/circuit/mod.rs @@ -161,7 +161,7 @@ pub enum OpcodeLocation { } impl OpcodeLocation { - pub fn reset_brillig_acir_index(self) -> Self { + pub fn reset_acir_index(self) -> Self { match self { OpcodeLocation::Brillig { brillig_index, .. } => { OpcodeLocation::Brillig { acir_index: None, brillig_index } diff --git a/tooling/nargo/src/errors.rs b/tooling/nargo/src/errors.rs index 9a7e949b765..fd99b35cd32 100644 --- a/tooling/nargo/src/errors.rs +++ b/tooling/nargo/src/errors.rs @@ -166,7 +166,7 @@ fn extract_locations_from_error( .get(&brillig_function_id); brillig_locations .unwrap() - .get(&resolved_location.opcode_location.reset_brillig_acir_index()) + .get(&resolved_location.opcode_location.reset_acir_index()) .cloned() .unwrap_or_default() } else { diff --git a/tooling/profiler/src/flamegraph.rs b/tooling/profiler/src/flamegraph.rs index a20af479e70..7810d51929a 100644 --- a/tooling/profiler/src/flamegraph.rs +++ b/tooling/profiler/src/flamegraph.rs @@ -99,7 +99,7 @@ fn generate_folded_sorted_lines<'files, F: AcirField>( debug_symbols.brillig_locations.get(&brillig_function_id); if let Some(brillig_locations) = brillig_locations { brillig_locations - .get(&opcode_location.reset_brillig_acir_index()) + .get(&opcode_location.reset_acir_index()) .cloned() .unwrap_or_default() } else { From dfc0825d35e80ddd7fd30306f3ac6ae25076742c Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 22 Aug 2024 02:30:28 +0000 Subject: [PATCH 07/12] comments about resolution of acir index --- acvm-repo/acir/src/circuit/mod.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/acvm-repo/acir/src/circuit/mod.rs b/acvm-repo/acir/src/circuit/mod.rs index 1f6f61b9383..31979ff5112 100644 --- a/acvm-repo/acir/src/circuit/mod.rs +++ b/acvm-repo/acir/src/circuit/mod.rs @@ -157,10 +157,16 @@ pub enum OpcodeLocation { // Resolved locations are generally specified when returning an error during execution. // The acir index is not actually needed to determine a Brillig location // as it can be resolved separately. + // + // We can not get rid of the acir index entirely just yet as this format is still + // used for resolving assert messages. Brillig { acir_index: Option, brillig_index: usize }, } impl OpcodeLocation { + // Utility method to allow easily comparing a resolved Brillig location and unresolved Brillig location. + // This method is useful when fetching Brillig debug locations as this does not need an ACIR index, + // and just need the Brillig index. pub fn reset_acir_index(self) -> Self { match self { OpcodeLocation::Brillig { brillig_index, .. } => { From 83b3073e1153db68d5e7d8959196d82f9b22d8b1 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 22 Aug 2024 03:27:38 +0000 Subject: [PATCH 08/12] switch to separate BrilligOpcodeLocation to avoid serialization changes --- acvm-repo/acir/src/circuit/mod.rs | 51 ++++++----- acvm-repo/acvm/src/compiler/mod.rs | 10 +-- acvm-repo/acvm/src/pwg/brillig.rs | 2 +- acvm-repo/acvm/tests/solver.rs | 2 +- compiler/noirc_errors/src/debug_info.rs | 9 +- .../ssa/acir_gen/acir_ir/generated_acir.rs | 16 ++-- tooling/debugger/src/context.rs | 90 +++++++++---------- tooling/debugger/src/repl.rs | 16 ++-- tooling/nargo/src/errors.rs | 11 +-- .../src/cli/opcodes_flamegraph_cmd.rs | 2 +- tooling/profiler/src/flamegraph.rs | 9 +- 11 files changed, 105 insertions(+), 113 deletions(-) diff --git a/acvm-repo/acir/src/circuit/mod.rs b/acvm-repo/acir/src/circuit/mod.rs index 31979ff5112..c2efc573a90 100644 --- a/acvm-repo/acir/src/circuit/mod.rs +++ b/acvm-repo/acir/src/circuit/mod.rs @@ -153,26 +153,24 @@ pub struct ResolvedOpcodeLocation { /// map opcodes to debug information related to their context. pub enum OpcodeLocation { Acir(usize), - // A Brillig opcode location can have either a resolved or unresolved acir location. - // Resolved locations are generally specified when returning an error during execution. - // The acir index is not actually needed to determine a Brillig location - // as it can be resolved separately. - // - // We can not get rid of the acir index entirely just yet as this format is still - // used for resolving assert messages. - Brillig { acir_index: Option, brillig_index: usize }, + // We can not get rid of this enum field entirely just yet as this format is still + // used for resolving assert messages which is breaking serialization change. + Brillig { acir_index: usize, brillig_index: usize }, } +#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize, Deserialize)] +pub struct BrilligOpcodeLocation(pub usize); + impl OpcodeLocation { - // Utility method to allow easily comparing a resolved Brillig location and unresolved Brillig location. + // Utility method to allow easily comparing a resolved Brillig location and a debug Brillig location. // This method is useful when fetching Brillig debug locations as this does not need an ACIR index, // and just need the Brillig index. - pub fn reset_acir_index(self) -> Self { + pub fn to_brillig_location(self) -> Option { match self { OpcodeLocation::Brillig { brillig_index, .. } => { - OpcodeLocation::Brillig { acir_index: None, brillig_index } + Some(BrilligOpcodeLocation(brillig_index)) } - _ => self, + _ => None, } } } @@ -182,11 +180,7 @@ impl std::fmt::Display for OpcodeLocation { match self { OpcodeLocation::Acir(index) => write!(f, "{index}"), OpcodeLocation::Brillig { acir_index, brillig_index } => { - if let Some(acir_index) = acir_index { - write!(f, "{acir_index}.{brillig_index}") - } else { - write!(f, "unknown_acir_index.{brillig_index}") - } + write!(f, "{acir_index}.{brillig_index}") } } } @@ -216,11 +210,7 @@ impl FromStr for OpcodeLocation { Ok(OpcodeLocation::Acir(index)) } 2 => { - let acir_index = if let Ok(acir_index) = parts[0].parse::() { - Some(acir_index) - } else { - None - }; + let acir_index = parts[0].parse()?; let brillig_index = parts[1].parse()?; Ok(OpcodeLocation::Brillig { acir_index, brillig_index }) } @@ -233,6 +223,23 @@ impl FromStr for OpcodeLocation { } } +impl FromStr for BrilligOpcodeLocation { + type Err = OpcodeLocationFromStrError; + fn from_str(s: &str) -> Result { + let index = s + .parse() + .map_err(|_| OpcodeLocationFromStrError::InvalidOpcodeLocationString(s.to_string()))?; + Ok(BrilligOpcodeLocation(index)) + } +} + +impl std::fmt::Display for BrilligOpcodeLocation { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let index = self.0; + write!(f, "{index}") + } +} + impl Circuit { pub fn num_vars(&self) -> u32 { self.current_witness_index + 1 diff --git a/acvm-repo/acvm/src/compiler/mod.rs b/acvm-repo/acvm/src/compiler/mod.rs index 1d191276df1..5ece3d19a6e 100644 --- a/acvm-repo/acvm/src/compiler/mod.rs +++ b/acvm-repo/acvm/src/compiler/mod.rs @@ -40,13 +40,7 @@ impl AcirTransformationMap { ) -> impl Iterator + '_ { let old_acir_index = match old_location { OpcodeLocation::Acir(index) => index, - OpcodeLocation::Brillig { acir_index, .. } => { - if let Some(acir_index) = acir_index { - acir_index - } else { - panic!("ICE: Should only be transforming ACIR locations") - } - } + OpcodeLocation::Brillig { acir_index, .. } => acir_index, }; self.old_indices_to_new_indices.get(&old_acir_index).into_iter().flat_map( @@ -54,7 +48,7 @@ impl AcirTransformationMap { new_indices.iter().map(move |new_index| match old_location { OpcodeLocation::Acir(_) => OpcodeLocation::Acir(*new_index), OpcodeLocation::Brillig { brillig_index, .. } => { - OpcodeLocation::Brillig { acir_index: Some(*new_index), brillig_index } + OpcodeLocation::Brillig { acir_index: *new_index, brillig_index } } }) }, diff --git a/acvm-repo/acvm/src/pwg/brillig.rs b/acvm-repo/acvm/src/pwg/brillig.rs index ab5a43ba2a9..5ec3224dbaa 100644 --- a/acvm-repo/acvm/src/pwg/brillig.rs +++ b/acvm-repo/acvm/src/pwg/brillig.rs @@ -170,7 +170,7 @@ impl<'b, B: BlackBoxFunctionSolver, F: AcirField> BrilligSolver<'b, F, B> { let call_stack = call_stack .iter() .map(|brillig_index| OpcodeLocation::Brillig { - acir_index: Some(self.acir_index), + acir_index: self.acir_index, brillig_index: *brillig_index, }) .collect(); diff --git a/acvm-repo/acvm/tests/solver.rs b/acvm-repo/acvm/tests/solver.rs index ef786972966..a1b8b62f8bf 100644 --- a/acvm-repo/acvm/tests/solver.rs +++ b/acvm-repo/acvm/tests/solver.rs @@ -677,7 +677,7 @@ fn unsatisfied_opcode_resolved_brillig() { ACVMStatus::Failure(OpcodeResolutionError::BrilligFunctionFailed { function_id: BrilligFunctionId(0), payload: None, - call_stack: vec![OpcodeLocation::Brillig { acir_index: Some(0), brillig_index: 3 }] + call_stack: vec![OpcodeLocation::Brillig { acir_index: 0, brillig_index: 3 }] }), "The first opcode is not satisfiable, expected an error indicating this" ); diff --git a/compiler/noirc_errors/src/debug_info.rs b/compiler/noirc_errors/src/debug_info.rs index 1a254175c0a..f347e75fd64 100644 --- a/compiler/noirc_errors/src/debug_info.rs +++ b/compiler/noirc_errors/src/debug_info.rs @@ -1,4 +1,5 @@ use acvm::acir::circuit::brillig::BrilligFunctionId; +use acvm::acir::circuit::BrilligOpcodeLocation; use acvm::acir::circuit::OpcodeLocation; use acvm::compiler::AcirTransformationMap; @@ -99,7 +100,8 @@ pub struct DebugInfo { #[serde_as(as = "BTreeMap")] pub locations: BTreeMap>, #[serde_as(as = "BTreeMap<_, BTreeMap>")] - pub brillig_locations: BTreeMap>>, + pub brillig_locations: + BTreeMap>>, pub variables: DebugVariables, pub functions: DebugFunctions, pub types: DebugTypes, @@ -116,7 +118,10 @@ pub struct OpCodesCount { impl DebugInfo { pub fn new( locations: BTreeMap>, - brillig_locations: BTreeMap>>, + brillig_locations: BTreeMap< + BrilligFunctionId, + BTreeMap>, + >, variables: DebugVariables, functions: DebugFunctions, types: DebugTypes, diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs index fb0d1d9439a..4250a7f0cff 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs @@ -11,7 +11,7 @@ use acvm::acir::{ circuit::{ brillig::{BrilligFunctionId, BrilligInputs, BrilligOutputs}, opcodes::{BlackBoxFuncCall, FunctionInput, Opcode as AcirOpcode}, - AssertionPayload, OpcodeLocation, + AssertionPayload, BrilligOpcodeLocation, OpcodeLocation, }, native_types::Witness, BlackBoxFunc, @@ -53,7 +53,7 @@ pub(crate) struct GeneratedAcir { /// Brillig function id -> Opcodes locations map /// This map is used to prevent redundant locations being stored for the same Brillig entry point. - pub(crate) brillig_locations: BTreeMap, + pub(crate) brillig_locations: BTreeMap, /// Source code location of the current instruction being processed /// None if we do not know the location @@ -77,6 +77,8 @@ pub(crate) struct GeneratedAcir { /// Correspondence between an opcode index (in opcodes) and the source code call stack which generated it pub(crate) type OpcodeToLocationsMap = BTreeMap; +pub(crate) type BrilligOpcodeToLocationsMap = BTreeMap; + #[derive(Debug, Clone, Copy, Hash, PartialEq, Eq)] pub(crate) enum BrilligStdlibFunc { Inverse, @@ -593,7 +595,7 @@ impl GeneratedAcir { for (brillig_index, message) in generated_brillig.assert_messages.iter() { self.assertion_payloads.insert( OpcodeLocation::Brillig { - acir_index: Some(self.opcodes.len() - 1), + acir_index: self.opcodes.len() - 1, brillig_index: *brillig_index, }, AssertionPayload::StaticString(message.clone()), @@ -605,10 +607,10 @@ impl GeneratedAcir { } for (brillig_index, call_stack) in generated_brillig.locations.iter() { - self.brillig_locations.entry(brillig_function_index).or_default().insert( - OpcodeLocation::Brillig { acir_index: None, brillig_index: *brillig_index }, - call_stack.clone(), - ); + self.brillig_locations + .entry(brillig_function_index) + .or_default() + .insert(BrilligOpcodeLocation(*brillig_index), call_stack.clone()); } } diff --git a/tooling/debugger/src/context.rs b/tooling/debugger/src/context.rs index 2edd6b0bd0f..db5c316c3f2 100644 --- a/tooling/debugger/src/context.rs +++ b/tooling/debugger/src/context.rs @@ -124,9 +124,7 @@ impl AddressMap { match &location.opcode_location { OpcodeLocation::Acir(acir_index) => circuit_addresses[*acir_index], OpcodeLocation::Brillig { acir_index, brillig_index } => { - let acir_index = - acir_index.expect("Debug locations should have a resolved acir index"); - circuit_addresses[acir_index] + *brillig_index + circuit_addresses[*acir_index] + *brillig_index } } } @@ -165,7 +163,7 @@ impl AddressMap { }) .map(|brillig_address_space| brillig_address_space.brillig_function_id); ( - OpcodeLocation::Brillig { acir_index: Some(acir_index), brillig_index }, + OpcodeLocation::Brillig { acir_index: acir_index, brillig_index }, brillig_function_id, ) } @@ -188,8 +186,6 @@ impl std::fmt::Display for DebugLocation { match self.opcode_location { OpcodeLocation::Acir(index) => write!(f, "{circuit_id}:{index}"), OpcodeLocation::Brillig { acir_index, brillig_index } => { - let acir_index = - acir_index.expect("Debug locations should have a resolved acir index"); write!(f, "{circuit_id}:{acir_index}.{brillig_index}") } } @@ -326,7 +322,7 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { let function_id = solver.function_id; ( OpcodeLocation::Brillig { - acir_index: Some(ip), + acir_index: ip, brillig_index: solver.program_counter(), }, Some(function_id), @@ -361,7 +357,7 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { frames.extend(solver.get_call_stack().iter().map(|program_counter| DebugLocation { circuit_id, opcode_location: OpcodeLocation::Brillig { - acir_index: Some(instruction_pointer), + acir_index: instruction_pointer, brillig_index: *program_counter, }, brillig_function_id: Some(solver.function_id), @@ -449,16 +445,15 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { self.debug_artifact.debug_symbols[debug_location.circuit_id as usize] .opcode_location(&debug_location.opcode_location) .unwrap_or_else(|| { - if let Some(brillig_function_id) = debug_location.brillig_function_id { + if let (Some(brillig_function_id), Some(brillig_location)) = ( + debug_location.brillig_function_id, + debug_location.opcode_location.to_brillig_location(), + ) { let brillig_locations = self.debug_artifact.debug_symbols [debug_location.circuit_id as usize] .brillig_locations .get(&brillig_function_id); - brillig_locations - .unwrap() - .get(&debug_location.opcode_location) - .cloned() - .unwrap_or_default() + brillig_locations.unwrap().get(&brillig_location).cloned().unwrap_or_default() } else { vec![] } @@ -506,18 +501,14 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { _ => format!("{opcode:?}"), } } - OpcodeLocation::Brillig { acir_index, brillig_index } => { - let acir_index = - acir_index.expect("Debug locations should have a resolved acir index"); - match &opcodes[acir_index] { - Opcode::BrilligCall { id, .. } => { - let bytecode = &self.unconstrained_functions[id.as_usize()].bytecode; - let opcode = &bytecode[*brillig_index]; - format!(" | {opcode:?}") - } - _ => String::from(" | invalid"), + OpcodeLocation::Brillig { acir_index, brillig_index } => match &opcodes[*acir_index] { + Opcode::BrilligCall { id, .. } => { + let bytecode = &self.unconstrained_functions[id.as_usize()].bytecode; + let opcode = &bytecode[*brillig_index]; + format!(" | {opcode:?}") } - } + _ => String::from(" | invalid"), + }, } } @@ -671,9 +662,8 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { fn get_current_acir_index(&self) -> Option { self.get_current_debug_location().map(|debug_location| { match debug_location.opcode_location { - OpcodeLocation::Acir(acir_index) => acir_index, - OpcodeLocation::Brillig { acir_index, .. } => { - acir_index.expect("Debug locations should have a resolved acir index") + OpcodeLocation::Acir(acir_index) | OpcodeLocation::Brillig { acir_index, .. } => { + acir_index } } }) @@ -824,8 +814,6 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { match location.opcode_location { OpcodeLocation::Acir(acir_index) => acir_index < opcodes.len(), OpcodeLocation::Brillig { acir_index, brillig_index } => { - let acir_index = - acir_index.expect("Debug locations should have a resolved acir index"); if acir_index < opcodes.len() { match &opcodes[acir_index] { Opcode::BrilligCall { id, .. } => { @@ -908,8 +896,19 @@ fn build_source_to_opcode_debug_mappings( ); for (brillig_function_id, brillig_locations_map) in &debug_symbols.brillig_locations { + let brillig_locations_map = brillig_locations_map + .into_iter() + .map(|(key, val)| { + ( + // TODO: this is a temporary placeholder until the debugger is updated to handle the new brillig debug locations. + OpcodeLocation::Brillig { acir_index: 0, brillig_index: key.0 }, + val.clone(), + ) + }) + .collect(); + add_opcode_locations_map( - brillig_locations_map, + &brillig_locations_map, &mut result, &simple_files, circuit_id, @@ -1047,7 +1046,7 @@ mod tests { context.get_current_debug_location(), Some(DebugLocation { circuit_id: 0, - opcode_location: OpcodeLocation::Brillig { acir_index: Some(0), brillig_index: 1 }, + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 1 }, brillig_function_id: Some(BrilligFunctionId(0)), }) ); @@ -1059,7 +1058,7 @@ mod tests { context.get_current_debug_location(), Some(DebugLocation { circuit_id: 0, - opcode_location: OpcodeLocation::Brillig { acir_index: Some(0), brillig_index: 2 }, + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 2 }, brillig_function_id: Some(BrilligFunctionId(0)), }) ); @@ -1071,7 +1070,7 @@ mod tests { context.get_current_debug_location(), Some(DebugLocation { circuit_id: 0, - opcode_location: OpcodeLocation::Brillig { acir_index: Some(0), brillig_index: 2 }, + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 2 }, brillig_function_id: Some(BrilligFunctionId(0)), }) ); @@ -1083,7 +1082,7 @@ mod tests { context.get_current_debug_location(), Some(DebugLocation { circuit_id: 0, - opcode_location: OpcodeLocation::Brillig { acir_index: Some(0), brillig_index: 3 }, + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 3 }, brillig_function_id: Some(BrilligFunctionId(0)), }) ); @@ -1168,7 +1167,7 @@ mod tests { // set breakpoint let breakpoint_location = DebugLocation { circuit_id: 0, - opcode_location: OpcodeLocation::Brillig { acir_index: Some(0), brillig_index: 1 }, + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 1 }, brillig_function_id: Some(BrilligFunctionId(0)), }; assert!(context.add_breakpoint(breakpoint_location)); @@ -1279,10 +1278,7 @@ mod tests { }), Some(DebugLocation { circuit_id: 0, - opcode_location: OpcodeLocation::Brillig { - acir_index: Some(1), - brillig_index: 1 - }, + opcode_location: OpcodeLocation::Brillig { acir_index: 1, brillig_index: 1 }, brillig_function_id: Some(BrilligFunctionId(0)), }), Some(DebugLocation { @@ -1302,18 +1298,12 @@ mod tests { }), Some(DebugLocation { circuit_id: 1, - opcode_location: OpcodeLocation::Brillig { - acir_index: Some(0), - brillig_index: 1 - }, + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 1 }, brillig_function_id: Some(BrilligFunctionId(1)), }), Some(DebugLocation { circuit_id: 1, - opcode_location: OpcodeLocation::Brillig { - acir_index: Some(0), - brillig_index: 2 - }, + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 2 }, brillig_function_id: Some(BrilligFunctionId(1)), }), Some(DebugLocation { @@ -1339,7 +1329,7 @@ mod tests { 1, context.debug_location_to_address(&DebugLocation { circuit_id: 0, - opcode_location: OpcodeLocation::Brillig { acir_index: Some(1), brillig_index: 0 }, + opcode_location: OpcodeLocation::Brillig { acir_index: 1, brillig_index: 0 }, brillig_function_id: Some(BrilligFunctionId(0)), }) ); @@ -1347,7 +1337,7 @@ mod tests { 5, context.debug_location_to_address(&DebugLocation { circuit_id: 1, - opcode_location: OpcodeLocation::Brillig { acir_index: Some(0), brillig_index: 0 }, + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 0 }, brillig_function_id: Some(BrilligFunctionId(1)), }) ); diff --git a/tooling/debugger/src/repl.rs b/tooling/debugger/src/repl.rs index 73258ddde65..2c953747456 100644 --- a/tooling/debugger/src/repl.rs +++ b/tooling/debugger/src/repl.rs @@ -81,10 +81,8 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { println!("At opcode {} :: {}", location, opcodes[*ip]); } OpcodeLocation::Brillig { acir_index, brillig_index } => { - let acir_index = - acir_index.expect("Debug locations should have a resolved acir index"); let brillig_bytecode = - if let Opcode::BrilligCall { id, .. } = opcodes[acir_index] { + if let Opcode::BrilligCall { id, .. } = opcodes[*acir_index] { &self.unconstrained_functions[id.as_usize()].bytecode } else { unreachable!("Brillig location does not contain Brillig opcodes"); @@ -111,9 +109,8 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { ) } OpcodeLocation::Brillig { acir_index, brillig_index } => { - let acir_index = - acir_index.expect("Debug locations should have a resolved acir index"); - let brillig_bytecode = if let Opcode::BrilligCall { id, .. } = opcodes[acir_index] { + let brillig_bytecode = if let Opcode::BrilligCall { id, .. } = opcodes[*acir_index] + { &self.unconstrained_functions[id.as_usize()].bytecode } else { unreachable!("Brillig location does not contain Brillig opcodes"); @@ -158,7 +155,7 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { let opcodes = self.context.get_opcodes_of_circuit(circuit_id); let current_acir_index = match current_opcode_location { Some(OpcodeLocation::Acir(ip)) => Some(ip), - Some(OpcodeLocation::Brillig { acir_index, .. }) => acir_index, + Some(OpcodeLocation::Brillig { acir_index, .. }) => Some(acir_index), None => None, }; let current_brillig_index = match current_opcode_location { @@ -183,10 +180,7 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { "->" } else if self.context.is_breakpoint_set(&DebugLocation { circuit_id, - opcode_location: OpcodeLocation::Brillig { - acir_index: Some(acir_index), - brillig_index, - }, + opcode_location: OpcodeLocation::Brillig { acir_index: acir_index, brillig_index }, brillig_function_id: Some(brillig_function_id), }) { " *" diff --git a/tooling/nargo/src/errors.rs b/tooling/nargo/src/errors.rs index fd99b35cd32..b5571ff7758 100644 --- a/tooling/nargo/src/errors.rs +++ b/tooling/nargo/src/errors.rs @@ -130,11 +130,9 @@ fn extract_locations_from_error( opcode_location: OpcodeLocation::Brillig { acir_index, .. }, } = resolved_location { - let acir_index = acir_index - .expect("Resolved Brillig opcode locations are expected to have an ACIR index"); let acir_location = ResolvedOpcodeLocation { acir_function_index: *acir_function_index, - opcode_location: OpcodeLocation::Acir(acir_index), + opcode_location: OpcodeLocation::Acir(*acir_index), }; opcode_locations.insert(i, acir_location); @@ -160,13 +158,16 @@ fn extract_locations_from_error( debug[resolved_location.acir_function_index] .opcode_location(&resolved_location.opcode_location) .unwrap_or_else(|| { - if let Some(brillig_function_id) = brillig_function_id { + if let (Some(brillig_function_id), Some(brillig_location)) = ( + brillig_function_id, + &resolved_location.opcode_location.to_brillig_location(), + ) { let brillig_locations = debug[resolved_location.acir_function_index] .brillig_locations .get(&brillig_function_id); brillig_locations .unwrap() - .get(&resolved_location.opcode_location.reset_acir_index()) + .get(brillig_location) .cloned() .unwrap_or_default() } else { diff --git a/tooling/profiler/src/cli/opcodes_flamegraph_cmd.rs b/tooling/profiler/src/cli/opcodes_flamegraph_cmd.rs index faba5ff3892..863d45b96d1 100644 --- a/tooling/profiler/src/cli/opcodes_flamegraph_cmd.rs +++ b/tooling/profiler/src/cli/opcodes_flamegraph_cmd.rs @@ -99,7 +99,7 @@ fn run_with_generator( .map(|(brillig_index, opcode)| Sample { opcode: AcirOrBrilligOpcode::Brillig(opcode), call_stack: vec![OpcodeLocation::Brillig { - acir_index: Some(acir_opcode_index), + acir_index: acir_opcode_index, brillig_index, }], count: 1, diff --git a/tooling/profiler/src/flamegraph.rs b/tooling/profiler/src/flamegraph.rs index 7810d51929a..488079de50a 100644 --- a/tooling/profiler/src/flamegraph.rs +++ b/tooling/profiler/src/flamegraph.rs @@ -94,14 +94,13 @@ fn generate_folded_sorted_lines<'files, F: AcirField>( .into_iter() .flat_map(|opcode_location| { debug_symbols.opcode_location(&opcode_location).unwrap_or_else(|| { - if let Some(brillig_function_id) = sample.brillig_function_id { + if let (Some(brillig_function_id), Some(brillig_location)) = + (sample.brillig_function_id, opcode_location.to_brillig_location()) + { let brillig_locations = debug_symbols.brillig_locations.get(&brillig_function_id); if let Some(brillig_locations) = brillig_locations { - brillig_locations - .get(&opcode_location.reset_acir_index()) - .cloned() - .unwrap_or_default() + brillig_locations.get(&brillig_location).cloned().unwrap_or_default() } else { vec![] } From 83f18404a7a8f75edb48e4570f72d0bd6c86a722 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 22 Aug 2024 03:28:39 +0000 Subject: [PATCH 09/12] fix clippy --- tooling/debugger/src/context.rs | 7 ++----- tooling/debugger/src/repl.rs | 2 +- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/tooling/debugger/src/context.rs b/tooling/debugger/src/context.rs index db5c316c3f2..0d348cf172d 100644 --- a/tooling/debugger/src/context.rs +++ b/tooling/debugger/src/context.rs @@ -162,10 +162,7 @@ impl AddressMap { && address <= brillig_address_space.end_address }) .map(|brillig_address_space| brillig_address_space.brillig_function_id); - ( - OpcodeLocation::Brillig { acir_index: acir_index, brillig_index }, - brillig_function_id, - ) + (OpcodeLocation::Brillig { acir_index, brillig_index }, brillig_function_id) } }; @@ -897,7 +894,7 @@ fn build_source_to_opcode_debug_mappings( for (brillig_function_id, brillig_locations_map) in &debug_symbols.brillig_locations { let brillig_locations_map = brillig_locations_map - .into_iter() + .iter() .map(|(key, val)| { ( // TODO: this is a temporary placeholder until the debugger is updated to handle the new brillig debug locations. diff --git a/tooling/debugger/src/repl.rs b/tooling/debugger/src/repl.rs index 2c953747456..1a7c2d6c7a8 100644 --- a/tooling/debugger/src/repl.rs +++ b/tooling/debugger/src/repl.rs @@ -180,7 +180,7 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { "->" } else if self.context.is_breakpoint_set(&DebugLocation { circuit_id, - opcode_location: OpcodeLocation::Brillig { acir_index: acir_index, brillig_index }, + opcode_location: OpcodeLocation::Brillig { acir_index, brillig_index }, brillig_function_id: Some(brillig_function_id), }) { " *" From 6dcbb7479a09feaa93b0e03ca534b0878b928052 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 22 Aug 2024 03:42:23 +0000 Subject: [PATCH 10/12] slight comment update --- acvm-repo/acir/src/circuit/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acvm-repo/acir/src/circuit/mod.rs b/acvm-repo/acir/src/circuit/mod.rs index c2efc573a90..bf7da9b8f8e 100644 --- a/acvm-repo/acir/src/circuit/mod.rs +++ b/acvm-repo/acir/src/circuit/mod.rs @@ -154,7 +154,7 @@ pub struct ResolvedOpcodeLocation { pub enum OpcodeLocation { Acir(usize), // We can not get rid of this enum field entirely just yet as this format is still - // used for resolving assert messages which is breaking serialization change. + // used for resolving assert messages which is a breaking serialization change. Brillig { acir_index: usize, brillig_index: usize }, } From 83e8af164db4acd3f41bf7f15a3624f334b4caf0 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 22 Aug 2024 13:49:37 +0000 Subject: [PATCH 11/12] add todos --- acvm-repo/acir/src/circuit/mod.rs | 2 +- .../noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/acvm-repo/acir/src/circuit/mod.rs b/acvm-repo/acir/src/circuit/mod.rs index bf7da9b8f8e..0f1fea99f39 100644 --- a/acvm-repo/acir/src/circuit/mod.rs +++ b/acvm-repo/acir/src/circuit/mod.rs @@ -153,7 +153,7 @@ pub struct ResolvedOpcodeLocation { /// map opcodes to debug information related to their context. pub enum OpcodeLocation { Acir(usize), - // We can not get rid of this enum field entirely just yet as this format is still + // TODO(https://github.com/noir-lang/noir/issues/5792): We can not get rid of this enum field entirely just yet as this format is still // used for resolving assert messages which is a breaking serialization change. Brillig { acir_index: usize, brillig_index: usize }, } diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs index 4250a7f0cff..004979ce84e 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs @@ -592,6 +592,7 @@ impl GeneratedAcir { return; } + // TODO(https://github.com/noir-lang/noir/issues/5792) for (brillig_index, message) in generated_brillig.assert_messages.iter() { self.assertion_payloads.insert( OpcodeLocation::Brillig { From 8e50032b135e8a3261f57ac9f4a1c47cc288ce32 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 23 Aug 2024 15:49:34 +0000 Subject: [PATCH 12/12] remove serde_as on brillig_locations --- acvm-repo/acir/src/circuit/mod.rs | 10 ---------- compiler/noirc_errors/src/debug_info.rs | 1 - 2 files changed, 11 deletions(-) diff --git a/acvm-repo/acir/src/circuit/mod.rs b/acvm-repo/acir/src/circuit/mod.rs index 0f1fea99f39..f700fefe0cd 100644 --- a/acvm-repo/acir/src/circuit/mod.rs +++ b/acvm-repo/acir/src/circuit/mod.rs @@ -223,16 +223,6 @@ impl FromStr for OpcodeLocation { } } -impl FromStr for BrilligOpcodeLocation { - type Err = OpcodeLocationFromStrError; - fn from_str(s: &str) -> Result { - let index = s - .parse() - .map_err(|_| OpcodeLocationFromStrError::InvalidOpcodeLocationString(s.to_string()))?; - Ok(BrilligOpcodeLocation(index)) - } -} - impl std::fmt::Display for BrilligOpcodeLocation { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { let index = self.0; diff --git a/compiler/noirc_errors/src/debug_info.rs b/compiler/noirc_errors/src/debug_info.rs index f347e75fd64..b480d20fde4 100644 --- a/compiler/noirc_errors/src/debug_info.rs +++ b/compiler/noirc_errors/src/debug_info.rs @@ -99,7 +99,6 @@ pub struct DebugInfo { /// that they should be serialized to/from strings. #[serde_as(as = "BTreeMap")] pub locations: BTreeMap>, - #[serde_as(as = "BTreeMap<_, BTreeMap>")] pub brillig_locations: BTreeMap>>, pub variables: DebugVariables,