From 0541568b4c209927a70778b895e8f1e50d9b6543 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Thu, 11 Jul 2024 20:28:34 -0400 Subject: [PATCH] feat: Handle ACIR calls in the debugger (#5051) # Description ## Problem\* Resolves #4824 With the introduction of ACIR calls to allow non-inlined ACIR functions, the debugger did not have proper support for programs that had them, neither when compiling in ACIR mode, nor in the default mode which compiles the full program to Brillig. ## Summary\* This PR depends on #4941 and is built on top of it. The changes allow compiling a program with `#[fold]` (and other non-inline) annotated functions in Brillig mode (the default when debugging) by forcing the selection of the Brillig runtime when compiling them. For inline functions, during SSA generation the runtime is _not changed_ in order to allow some optimizations in the stdlib to still work. For example, [some constant assertions](https://github.com/noir-lang/noir/blob/9c6de4b25d318c6d211361dd62a112a9d2432c56/noir_stdlib/src/field.nr#L6) don't work unless the function is inlined. When debugging in ACIR mode (with the `--acir-mode` CLI or `generateAcir: true` DAP options), the debugger now properly handles programs with multiple circuits and ACIR calls by: 1. keeping a stack of ACVMs and a record of the circuit being executed by each one, and 2. extending the debug location to also indicate in which circuit is the current opcode being executed ## Additional Context ## Documentation\* Check one: - [X] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [X] I have tested the changes locally. - [X] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: Martin Verzilli Co-authored-by: Maxim Vezenov Co-authored-by: Ana Perez Ghiglia --- compiler/noirc_evaluator/src/ssa.rs | 19 +- .../src/ssa/ssa_gen/context.rs | 11 +- .../noirc_evaluator/src/ssa/ssa_gen/mod.rs | 2 +- tooling/debugger/Cargo.toml | 2 +- tooling/debugger/ignored-tests.txt | 8 - tooling/debugger/src/context.rs | 680 ++++++++++++------ tooling/debugger/src/dap.rs | 48 +- tooling/debugger/src/lib.rs | 17 +- tooling/debugger/src/repl.rs | 138 ++-- tooling/debugger/tests/debug.rs | 2 +- tooling/nargo_cli/src/cli/debug_cmd.rs | 52 +- 11 files changed, 633 insertions(+), 346 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 820374df9c1..81327cec013 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -190,12 +190,19 @@ pub fn create_program( let recursive = program.recursive; let ArtifactsAndWarnings((generated_acirs, generated_brillig, error_types), ssa_level_warnings) = optimize_into_acir(program, options)?; - assert_eq!( - generated_acirs.len(), - func_sigs.len(), - "The generated ACIRs should match the supplied function signatures" - ); - + if options.force_brillig_output { + assert_eq!( + generated_acirs.len(), + 1, + "Only the main ACIR is expected when forcing Brillig output" + ); + } else { + assert_eq!( + generated_acirs.len(), + func_sigs.len(), + "The generated ACIRs should match the supplied function signatures" + ); + } let mut program_artifact = SsaProgramArtifact::new(generated_brillig, error_types); // Add warnings collected at the Ssa stage diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index e013546f14a..8e55debec1d 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -5,7 +5,7 @@ use acvm::{acir::AcirField, FieldElement}; use iter_extended::vecmap; use noirc_errors::Location; use noirc_frontend::ast::{BinaryOpKind, Signedness}; -use noirc_frontend::monomorphization::ast::{self, LocalId, Parameters}; +use noirc_frontend::monomorphization::ast::{self, InlineType, LocalId, Parameters}; use noirc_frontend::monomorphization::ast::{FuncId, Program}; use crate::errors::RuntimeError; @@ -121,9 +121,14 @@ impl<'a> FunctionContext<'a> { /// /// Note that the previous function cannot be resumed after calling this. Developers should /// avoid calling new_function until the previous function is completely finished with ssa-gen. - pub(super) fn new_function(&mut self, id: IrFunctionId, func: &ast::Function) { + pub(super) fn new_function( + &mut self, + id: IrFunctionId, + func: &ast::Function, + force_brillig_runtime: bool, + ) { self.definitions.clear(); - if func.unconstrained { + if func.unconstrained || (force_brillig_runtime && func.inline_type != InlineType::Inline) { self.builder.new_brillig_function(func.name.clone(), id); } else { self.builder.new_function(func.name.clone(), id, func.inline_type); diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index afe44881830..abd251b008f 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -111,7 +111,7 @@ pub(crate) fn generate_ssa( // to generate SSA for each function used within the program. while let Some((src_function_id, dest_id)) = context.pop_next_function_in_queue() { let function = &context.program[src_function_id]; - function_context.new_function(dest_id, function); + function_context.new_function(dest_id, function, force_brillig_runtime); function_context.codegen_function_body(&function.body)?; } diff --git a/tooling/debugger/Cargo.toml b/tooling/debugger/Cargo.toml index 05b28f9d95a..540d6d11bc0 100644 --- a/tooling/debugger/Cargo.toml +++ b/tooling/debugger/Cargo.toml @@ -14,7 +14,7 @@ build-data.workspace = true acvm.workspace = true fm.workspace = true nargo.workspace = true -noirc_frontend.workspace = true +noirc_frontend = { workspace = true, features = ["bn254"] } noirc_printable_type.workspace = true noirc_errors.workspace = true noirc_driver.workspace = true diff --git a/tooling/debugger/ignored-tests.txt b/tooling/debugger/ignored-tests.txt index 2a6648b094b..745971d9b28 100644 --- a/tooling/debugger/ignored-tests.txt +++ b/tooling/debugger/ignored-tests.txt @@ -1,13 +1,5 @@ brillig_references debug_logs -fold_after_inlined_calls -fold_basic -fold_basic_nested_call -fold_call_witness_condition -fold_complex_outputs -fold_distinct_return -fold_fibonacci -fold_numeric_generic_poseidon is_unconstrained macros references diff --git a/tooling/debugger/src/context.rs b/tooling/debugger/src/context.rs index cb36988bf0b..d18ec5f0786 100644 --- a/tooling/debugger/src/context.rs +++ b/tooling/debugger/src/context.rs @@ -1,10 +1,11 @@ use crate::foreign_calls::DebugForeignCallExecutor; use acvm::acir::circuit::brillig::BrilligBytecode; use acvm::acir::circuit::{Circuit, Opcode, OpcodeLocation}; -use acvm::acir::native_types::{Witness, WitnessMap}; +use acvm::acir::native_types::{Witness, WitnessMap, WitnessStack}; use acvm::brillig_vm::MemoryValue; use acvm::pwg::{ - ACVMStatus, BrilligSolver, BrilligSolverStatus, ForeignCallWaitInfo, StepResult, ACVM, + ACVMStatus, AcirCallWaitInfo, BrilligSolver, BrilligSolverStatus, ForeignCallWaitInfo, + OpcodeNotSolvable, StepResult, ACVM, }; use acvm::{BlackBoxFunctionSolver, FieldElement}; @@ -15,56 +16,235 @@ use nargo::NargoError; use noirc_artifacts::debug::{DebugArtifact, StackFrame}; use noirc_driver::DebugFile; +use thiserror::Error; + use std::collections::BTreeMap; use std::collections::{hash_set::Iter, HashSet}; +/// A Noir program is composed by +/// `n` ACIR circuits +/// |_ `m` ACIR opcodes +/// |_ Acir call +/// |_ Acir Brillig function invocation +/// |_ `p` Brillig opcodes +/// +/// The purpose of this structure is to map the opcode locations in ACIR circuits into +/// a flat contiguous address space to be able to expose them to the DAP interface. +/// In this address space, the ACIR circuits are laid out one after the other, and +/// Brillig functions called from such circuits are expanded inline, replacing +/// the `BrilligCall` ACIR opcode. +/// +/// `addresses: Vec>` +/// * The outer vec is `n` sized - one element per ACIR circuit +/// * Each nested vec is `m` sized - one element per ACIR opcode in circuit +/// * Each element is the "virtual address" of such opcode +/// +/// For flattening we map each ACIR circuit and ACIR opcode with a sequential address number +/// We start by assigning 0 to the very first ACIR opcode and then start accumulating by +/// traversing by depth-first +/// +/// Even if the address space is continuous, the `addresses` tree only +/// keeps track of the ACIR opcodes, since the Brillig opcode addresses can be +/// calculated from the initial opcode address. +/// As a result the flattened indexed addresses list may have "holes". +/// +/// If between two consequent `addresses` nodes there is a "hole" (an address jump), +/// this means that the first one is actually a ACIR Brillig call +/// which has as many brillig opcodes as `second_address - first_address` +/// +#[derive(Clone, Debug, Eq, Hash, PartialEq, PartialOrd, Ord)] +pub struct AddressMap { + addresses: Vec>, + + // virtual address of the last opcode of the program + last_valid_address: usize, +} + +impl AddressMap { + pub(super) fn new( + circuits: &[Circuit], + unconstrained_functions: &[BrilligBytecode], + ) -> Self { + let opcode_address_size = |opcode: &Opcode| { + if let Opcode::BrilligCall { id, .. } = opcode { + unconstrained_functions[*id as usize].bytecode.len() + } else { + 1 + } + }; + + let mut addresses = Vec::with_capacity(circuits.len()); + let mut next_address = 0usize; + + for circuit in circuits { + let mut circuit_addresses = Vec::with_capacity(circuit.opcodes.len()); + for opcode in &circuit.opcodes { + circuit_addresses.push(next_address); + next_address += opcode_address_size(opcode); + } + addresses.push(circuit_addresses); + } + + Self { addresses, last_valid_address: next_address - 1 } + } + + /// Returns the absolute address of the opcode at the given location. + /// Absolute here means accounting for nested Brillig opcodes in BrilligCall + /// opcodes. + pub fn debug_location_to_address(&self, location: &DebugLocation) -> usize { + let circuit_addresses = &self.addresses[location.circuit_id as usize]; + match &location.opcode_location { + OpcodeLocation::Acir(acir_index) => circuit_addresses[*acir_index], + OpcodeLocation::Brillig { acir_index, brillig_index } => { + circuit_addresses[*acir_index] + *brillig_index + } + } + } + + pub fn address_to_debug_location(&self, address: usize) -> Option { + if address > self.last_valid_address { + return None; + } + // We binary search if the given address is the first opcode address of each circuit id + // if is not, this means that the address itself is "contained" in the previous + // circuit indicated by `Err(insert_index)` + let circuit_id = + match self.addresses.binary_search_by(|addresses| addresses[0].cmp(&address)) { + Ok(found_index) => found_index, + // This means that the address is not in `insert_index` circuit + // because is an `Err`, so it must be included in previous circuit vec of opcodes + Err(insert_index) => insert_index - 1, + }; + + // We binary search among the selected `circuit_id`` list of opcodes + // If Err(insert_index) this means that the given address + // is a Brillig addresses that's contained in previous index ACIR opcode index + let opcode_location = match self.addresses[circuit_id].binary_search(&address) { + Ok(found_index) => OpcodeLocation::Acir(found_index), + Err(insert_index) => { + let acir_index = insert_index - 1; + let base_offset = self.addresses[circuit_id][acir_index]; + let brillig_index = address - base_offset; + OpcodeLocation::Brillig { acir_index, brillig_index } + } + }; + Some(DebugLocation { circuit_id: circuit_id as u32, opcode_location }) + } +} + +#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq, PartialOrd, Ord)] +pub struct DebugLocation { + pub circuit_id: u32, + pub opcode_location: OpcodeLocation, +} + +impl std::fmt::Display for DebugLocation { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let circuit_id = self.circuit_id; + match self.opcode_location { + OpcodeLocation::Acir(index) => write!(f, "{circuit_id}:{index}"), + OpcodeLocation::Brillig { acir_index, brillig_index } => { + write!(f, "{circuit_id}:{acir_index}.{brillig_index}") + } + } + } +} + +#[derive(Error, Debug)] +pub enum DebugLocationFromStrError { + #[error("Invalid debug location string: {0}")] + InvalidDebugLocationString(String), +} + +impl std::str::FromStr for DebugLocation { + type Err = DebugLocationFromStrError; + fn from_str(s: &str) -> Result { + let parts: Vec<_> = s.split(':').collect(); + let error = Err(DebugLocationFromStrError::InvalidDebugLocationString(s.to_string())); + + match parts.len() { + 1 => OpcodeLocation::from_str(parts[0]).map_or(error, |opcode_location| { + Ok(DebugLocation { circuit_id: 0, opcode_location }) + }), + 2 => { + let first_part = parts[0].parse().ok(); + let second_part = OpcodeLocation::from_str(parts[1]).ok(); + if let (Some(circuit_id), Some(opcode_location)) = (first_part, second_part) { + Ok(DebugLocation { circuit_id, opcode_location }) + } else { + error + } + } + _ => error, + } + } +} + #[derive(Debug)] pub(super) enum DebugCommandResult { Done, Ok, - BreakpointReached(OpcodeLocation), + BreakpointReached(DebugLocation), Error(NargoError), } +pub struct ExecutionFrame<'a, B: BlackBoxFunctionSolver> { + circuit_id: u32, + acvm: ACVM<'a, FieldElement, B>, +} + pub(super) struct DebugContext<'a, B: BlackBoxFunctionSolver> { acvm: ACVM<'a, FieldElement, B>, + current_circuit_id: u32, brillig_solver: Option>, + + witness_stack: WitnessStack, + acvm_stack: Vec>, + + backend: &'a B, foreign_call_executor: Box, + debug_artifact: &'a DebugArtifact, - breakpoints: HashSet, - source_to_opcodes: BTreeMap>, + breakpoints: HashSet, + source_to_locations: BTreeMap>, + + circuits: &'a [Circuit], unconstrained_functions: &'a [BrilligBytecode], - // Absolute (in terms of all the opcodes ACIR+Brillig) addresses of the ACIR - // opcodes with one additional entry for to indicate the last valid address. - acir_opcode_addresses: Vec, + acir_opcode_addresses: AddressMap, } impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { pub(super) fn new( blackbox_solver: &'a B, - circuit: &'a Circuit, + circuits: &'a [Circuit], debug_artifact: &'a DebugArtifact, initial_witness: WitnessMap, foreign_call_executor: Box, unconstrained_functions: &'a [BrilligBytecode], ) -> Self { let source_to_opcodes = build_source_to_opcode_debug_mappings(debug_artifact); - let acir_opcode_addresses = build_acir_opcode_offsets(circuit, unconstrained_functions); + let current_circuit_id: u32 = 0; + let initial_circuit = &circuits[current_circuit_id as usize]; + let acir_opcode_addresses = AddressMap::new(circuits, unconstrained_functions); Self { - // TODO: need to handle brillig pointer in the debugger acvm: ACVM::new( blackbox_solver, - &circuit.opcodes, + &initial_circuit.opcodes, initial_witness, unconstrained_functions, - &circuit.assert_messages, + &initial_circuit.assert_messages, ), + current_circuit_id, brillig_solver: None, + witness_stack: WitnessStack::default(), + acvm_stack: vec![], + backend: blackbox_solver, foreign_call_executor, debug_artifact, breakpoints: HashSet::new(), - source_to_opcodes, + source_to_locations: source_to_opcodes, + circuits, unconstrained_functions, acir_opcode_addresses, } @@ -74,6 +254,10 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { self.acvm.opcodes() } + pub(super) fn get_opcodes_of_circuit(&self, circuit_id: u32) -> &[Opcode] { + &self.circuits[circuit_id as usize].opcodes + } + pub(super) fn get_witness_map(&self) -> &WitnessMap { self.acvm.witness_map() } @@ -86,36 +270,49 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { self.acvm.overwrite_witness(witness, value) } - pub(super) fn get_current_opcode_location(&self) -> Option { + pub(super) fn get_current_debug_location(&self) -> Option { let ip = self.acvm.instruction_pointer(); if ip >= self.get_opcodes().len() { None - } else if let Some(ref solver) = self.brillig_solver { - Some(OpcodeLocation::Brillig { - acir_index: ip, - brillig_index: solver.program_counter(), - }) } else { - Some(OpcodeLocation::Acir(ip)) + let opcode_location = if let Some(ref solver) = self.brillig_solver { + OpcodeLocation::Brillig { acir_index: ip, brillig_index: solver.program_counter() } + } else { + OpcodeLocation::Acir(ip) + }; + Some(DebugLocation { circuit_id: self.current_circuit_id, opcode_location }) } } - pub(super) fn get_call_stack(&self) -> Vec { + pub(super) fn get_call_stack(&self) -> Vec { + // Build the frames from parent ACIR calls + let mut frames: Vec<_> = self + .acvm_stack + .iter() + .map(|ExecutionFrame { circuit_id, acvm }| DebugLocation { + circuit_id: *circuit_id, + opcode_location: OpcodeLocation::Acir(acvm.instruction_pointer()), + }) + .collect(); + + // Now add the frame(s) for the currently executing ACVM let instruction_pointer = self.acvm.instruction_pointer(); - if instruction_pointer >= self.get_opcodes().len() { - vec![] - } else if let Some(ref solver) = self.brillig_solver { - solver - .get_call_stack() - .iter() - .map(|program_counter| OpcodeLocation::Brillig { + let circuit_id = self.current_circuit_id; + if let Some(ref solver) = self.brillig_solver { + frames.extend(solver.get_call_stack().iter().map(|program_counter| DebugLocation { + circuit_id, + opcode_location: OpcodeLocation::Brillig { acir_index: instruction_pointer, brillig_index: *program_counter, - }) - .collect() - } else { - vec![OpcodeLocation::Acir(instruction_pointer)] + }, + })); + } else if instruction_pointer < self.get_opcodes().len() { + frames.push(DebugLocation { + circuit_id, + opcode_location: OpcodeLocation::Acir(instruction_pointer), + }); } + frames } pub(super) fn is_source_location_in_debug_module(&self, location: &Location) -> bool { @@ -142,10 +339,10 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { &self, file_id: &FileId, line: i64, - ) -> Option { + ) -> Option { let line = line as usize; - let line_to_opcodes = self.source_to_opcodes.get(file_id)?; - let found_index = match line_to_opcodes.binary_search_by(|x| x.0.cmp(&line)) { + let line_to_opcodes = self.source_to_locations.get(file_id)?; + let found_location = match line_to_opcodes.binary_search_by(|x| x.0.cmp(&line)) { Ok(index) => { // move backwards to find the first opcode which matches the line let mut index = index; @@ -161,7 +358,7 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { line_to_opcodes[index].1 } }; - Some(found_index) + Some(found_location) } /// Returns the callstack in source code locations for the currently @@ -172,9 +369,9 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { /// This function also filters source locations that are determined to be in /// the internal debug module. pub(super) fn get_current_source_location(&self) -> Option> { - self.get_current_opcode_location() + self.get_current_debug_location() .as_ref() - .map(|opcode_location| self.get_source_location_for_opcode_location(opcode_location)) + .map(|debug_location| self.get_source_location_for_debug_location(debug_location)) .filter(|v: &Vec| !v.is_empty()) } @@ -184,15 +381,12 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { /// the given opcode location cannot be mapped back to a source location /// (eg. it may be pure debug instrumentation code or other synthetically /// produced opcode by the compiler) - pub(super) fn get_source_location_for_opcode_location( + pub(super) fn get_source_location_for_debug_location( &self, - opcode_location: &OpcodeLocation, + debug_location: &DebugLocation, ) -> Vec { - // TODO: this assumes we're debugging a program (ie. the DebugArtifact - // will contain a single DebugInfo), but this assumption doesn't hold - // for contracts - self.debug_artifact.debug_symbols[0] - .opcode_location(opcode_location) + self.debug_artifact.debug_symbols[debug_location.circuit_id as usize] + .opcode_location(&debug_location.opcode_location) .map(|source_locations| { source_locations .into_iter() @@ -208,48 +402,30 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { /// general, the matching between opcode location and source location is 1 /// to 1, but due to the compiler inlining functions a single opcode /// location may expand to multiple source locations. - pub(super) fn get_source_call_stack(&self) -> Vec<(OpcodeLocation, Location)> { + pub(super) fn get_source_call_stack(&self) -> Vec<(DebugLocation, Location)> { self.get_call_stack() .iter() - .flat_map(|opcode_location| { - self.get_source_location_for_opcode_location(opcode_location) + .flat_map(|debug_location| { + self.get_source_location_for_debug_location(debug_location) .into_iter() - .map(|source_location| (*opcode_location, source_location)) + .map(|source_location| (*debug_location, source_location)) }) .collect() } /// Returns the absolute address of the opcode at the given location. - /// Absolute here means accounting for nested Brillig opcodes in BrilligCall - /// opcodes. - pub fn opcode_location_to_address(&self, location: &OpcodeLocation) -> usize { - match location { - OpcodeLocation::Acir(acir_index) => self.acir_opcode_addresses[*acir_index], - OpcodeLocation::Brillig { acir_index, brillig_index } => { - self.acir_opcode_addresses[*acir_index] + *brillig_index - } - } + pub fn debug_location_to_address(&self, location: &DebugLocation) -> usize { + self.acir_opcode_addresses.debug_location_to_address(location) } - pub fn address_to_opcode_location(&self, address: usize) -> Option { - if address >= *self.acir_opcode_addresses.last().unwrap_or(&0) { - return None; - } - let location = match self.acir_opcode_addresses.binary_search(&address) { - Ok(found_index) => OpcodeLocation::Acir(found_index), - Err(insert_index) => { - let acir_index = insert_index - 1; - let base_offset = self.acir_opcode_addresses[acir_index]; - let brillig_index = address - base_offset; - OpcodeLocation::Brillig { acir_index, brillig_index } - } - }; - Some(location) + // Returns the DebugLocation associated to the given address + pub fn address_to_debug_location(&self, address: usize) -> Option { + self.acir_opcode_addresses.address_to_debug_location(address) } - pub(super) fn render_opcode_at_location(&self, location: &OpcodeLocation) -> String { - let opcodes = self.get_opcodes(); - match location { + pub(super) fn render_opcode_at_location(&self, location: &DebugLocation) -> String { + let opcodes = self.get_opcodes_of_circuit(location.circuit_id); + match &location.opcode_location { OpcodeLocation::Acir(acir_index) => { let opcode = &opcodes[*acir_index]; match opcode { @@ -280,7 +456,7 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { self.brillig_solver = Some(solver); if self.breakpoint_reached() { DebugCommandResult::BreakpointReached( - self.get_current_opcode_location() + self.get_current_debug_location() .expect("Breakpoint reached but we have no location"), ) } else { @@ -296,7 +472,6 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { self.handle_foreign_call(foreign_call) } Err(err) => DebugCommandResult::Error(NargoError::ExecutionError( - // TODO: debugger does not handle multiple acir calls ExecutionError::SolvingError(err, None), )), } @@ -315,24 +490,82 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { } else { self.acvm.resolve_pending_foreign_call(foreign_call_result); } - // TODO: should we retry executing the opcode somehow in this case? + // TODO: should we retry executing the opcode somehow in this + // case? Otherwise, executing a foreign call takes two debugging + // steps. DebugCommandResult::Ok } Err(error) => DebugCommandResult::Error(error.into()), } } - fn handle_acvm_status(&mut self, status: ACVMStatus) -> DebugCommandResult { - if let ACVMStatus::RequiresForeignCall(foreign_call) = status { - return self.handle_foreign_call(foreign_call); + fn handle_acir_call( + &mut self, + call_info: AcirCallWaitInfo, + ) -> DebugCommandResult { + let callee_circuit = &self.circuits[call_info.id as usize]; + let callee_witness_map = call_info.initial_witness; + let callee_acvm = ACVM::new( + self.backend, + &callee_circuit.opcodes, + callee_witness_map, + self.unconstrained_functions, + &callee_circuit.assert_messages, + ); + let caller_acvm = std::mem::replace(&mut self.acvm, callee_acvm); + self.acvm_stack + .push(ExecutionFrame { circuit_id: self.current_circuit_id, acvm: caller_acvm }); + self.current_circuit_id = call_info.id; + + // Explicitly handling the new ACVM status here handles two edge cases: + // 1. there is a breakpoint set at the beginning of a circuit + // 2. the called circuit has no opcodes + self.handle_acvm_status(self.acvm.get_status().clone()) + } + + fn handle_acir_call_finished(&mut self) -> DebugCommandResult { + let caller_frame = self.acvm_stack.pop().expect("Execution stack should not be empty"); + let caller_acvm = caller_frame.acvm; + let callee_acvm = std::mem::replace(&mut self.acvm, caller_acvm); + self.current_circuit_id = caller_frame.circuit_id; + let call_solved_witness = callee_acvm.finalize(); + + let ACVMStatus::RequiresAcirCall(call_info) = self.acvm.get_status() else { + unreachable!("Resolving an ACIR call, the caller is in an invalid state"); + }; + let acir_to_call = &self.circuits[call_info.id as usize]; + + let mut call_resolved_outputs = Vec::new(); + for return_witness_index in acir_to_call.return_values.indices() { + if let Some(return_value) = call_solved_witness.get_index(return_witness_index) { + call_resolved_outputs.push(*return_value); + } else { + return DebugCommandResult::Error( + ExecutionError::SolvingError( + OpcodeNotSolvable::MissingAssignment(return_witness_index).into(), + None, // Missing assignment errors do not supply user-facing diagnostics so we do not need to attach a call stack + ) + .into(), + ); + } } + self.acvm.resolve_pending_acir_call(call_resolved_outputs); + + DebugCommandResult::Ok + } + fn handle_acvm_status(&mut self, status: ACVMStatus) -> DebugCommandResult { match status { - ACVMStatus::Solved => DebugCommandResult::Done, + ACVMStatus::Solved => { + if self.acvm_stack.is_empty() { + return DebugCommandResult::Done; + } + self.handle_acir_call_finished() + } ACVMStatus::InProgress => { if self.breakpoint_reached() { DebugCommandResult::BreakpointReached( - self.get_current_opcode_location() + self.get_current_debug_location() .expect("Breakpoint reached but we have no location"), ) } else { @@ -340,15 +573,10 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { } } ACVMStatus::Failure(error) => DebugCommandResult::Error(NargoError::ExecutionError( - // TODO: debugger does not handle multiple acir calls ExecutionError::SolvingError(error, None), )), - ACVMStatus::RequiresForeignCall(_) => { - unreachable!("Unexpected pending foreign call resolution"); - } - ACVMStatus::RequiresAcirCall(_) => { - todo!("Multiple ACIR calls are not supported"); - } + ACVMStatus::RequiresForeignCall(foreign_call) => self.handle_foreign_call(foreign_call), + ACVMStatus::RequiresAcirCall(call_info) => self.handle_acir_call(call_info), } } @@ -367,9 +595,11 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { } fn get_current_acir_index(&self) -> Option { - self.get_current_opcode_location().map(|opcode_location| match opcode_location { - OpcodeLocation::Acir(acir_index) => acir_index, - OpcodeLocation::Brillig { acir_index, .. } => acir_index, + 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, + } }) } @@ -394,10 +624,16 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { return true; } - match self.get_current_opcode_location() { - Some(OpcodeLocation::Brillig { .. }) => true, - Some(OpcodeLocation::Acir(acir_index)) => { - matches!(self.get_opcodes()[acir_index], Opcode::BrilligCall { .. }) + match self.get_current_debug_location() { + Some(DebugLocation { opcode_location: OpcodeLocation::Brillig { .. }, .. }) => true, + Some(DebugLocation { + circuit_id, + opcode_location: OpcodeLocation::Acir(acir_index), + }) => { + matches!( + self.get_opcodes_of_circuit(circuit_id)[acir_index], + Opcode::BrilligCall { .. } + ) } _ => false, } @@ -491,16 +727,19 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { } fn breakpoint_reached(&self) -> bool { - if let Some(location) = self.get_current_opcode_location() { + if let Some(location) = self.get_current_debug_location() { self.breakpoints.contains(&location) } else { false } } - pub(super) fn is_valid_opcode_location(&self, location: &OpcodeLocation) -> bool { - let opcodes = self.get_opcodes(); - match *location { + pub(super) fn is_valid_debug_location(&self, location: &DebugLocation) -> bool { + if location.circuit_id as usize >= self.circuits.len() { + return false; + } + let opcodes = self.get_opcodes_of_circuit(location.circuit_id); + match location.opcode_location { OpcodeLocation::Acir(acir_index) => acir_index < opcodes.len(), OpcodeLocation::Brillig { acir_index, brillig_index } => { if acir_index < opcodes.len() { @@ -518,19 +757,19 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { } } - pub(super) fn is_breakpoint_set(&self, location: &OpcodeLocation) -> bool { + pub(super) fn is_breakpoint_set(&self, location: &DebugLocation) -> bool { self.breakpoints.contains(location) } - pub(super) fn add_breakpoint(&mut self, location: OpcodeLocation) -> bool { + pub(super) fn add_breakpoint(&mut self, location: DebugLocation) -> bool { self.breakpoints.insert(location) } - pub(super) fn delete_breakpoint(&mut self, location: &OpcodeLocation) -> bool { + pub(super) fn delete_breakpoint(&mut self, location: &DebugLocation) -> bool { self.breakpoints.remove(location) } - pub(super) fn iterate_breakpoints(&self) -> Iter<'_, OpcodeLocation> { + pub(super) fn iterate_breakpoints(&self) -> Iter<'_, DebugLocation> { self.breakpoints.iter() } @@ -542,8 +781,10 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { matches!(self.acvm.get_status(), ACVMStatus::Solved) } - pub fn finalize(self) -> WitnessMap { - self.acvm.finalize() + pub fn finalize(mut self) -> WitnessStack { + let last_witness_map = self.acvm.finalize(); + self.witness_stack.push(0, last_witness_map); + self.witness_stack } } @@ -555,11 +796,10 @@ fn is_debug_file_in_debug_crate(debug_file: &DebugFile) -> bool { /// numbers and opcode locations corresponding to those line numbers fn build_source_to_opcode_debug_mappings( debug_artifact: &DebugArtifact, -) -> BTreeMap> { +) -> BTreeMap> { if debug_artifact.debug_symbols.is_empty() { return BTreeMap::new(); } - let locations = &debug_artifact.debug_symbols[0].locations; let simple_files: BTreeMap<_, _> = debug_artifact .file_map .iter() @@ -572,50 +812,34 @@ fn build_source_to_opcode_debug_mappings( }) .collect(); - let mut result: BTreeMap> = BTreeMap::new(); - locations.iter().for_each(|(opcode_location, source_locations)| { - source_locations.iter().for_each(|source_location| { - let span = source_location.span; - let file_id = source_location.file; - let Some(file) = simple_files.get(&file_id) else { - return; - }; - let Ok(line_index) = file.line_index((), span.start() as usize) else { - return; - }; - let line_number = line_index + 1; - - result.entry(file_id).or_default().push((line_number, *opcode_location)); - }); - }); + let mut result: BTreeMap> = BTreeMap::new(); + + for (circuit_id, debug_symbols) in debug_artifact.debug_symbols.iter().enumerate() { + for (opcode_location, source_locations) in &debug_symbols.locations { + source_locations.iter().for_each(|source_location| { + let span = source_location.span; + let file_id = source_location.file; + let Some(file) = simple_files.get(&file_id) else { + return; + }; + let Ok(line_index) = file.line_index((), span.start() as usize) else { + return; + }; + let line_number = line_index + 1; + + let debug_location = DebugLocation { + circuit_id: circuit_id as u32, + opcode_location: *opcode_location, + }; + result.entry(file_id).or_default().push((line_number, debug_location)); + }); + } + } result.iter_mut().for_each(|(_, file_locations)| file_locations.sort_by_key(|x| (x.0, x.1))); result } -fn build_acir_opcode_offsets( - circuit: &Circuit, - unconstrained_functions: &[BrilligBytecode], -) -> Vec { - let mut result = Vec::with_capacity(circuit.opcodes.len() + 1); - // address of the first opcode is always 0 - result.push(0); - circuit.opcodes.iter().fold(0, |acc, opcode| { - let acc = acc - + match opcode { - Opcode::BrilligCall { id, .. } => { - unconstrained_functions[*id as usize].bytecode.len() - } - _ => 1, - }; - // push the starting address of the next opcode - result.push(acc); - acc - }); - result -} - -// TODO: update all debugger tests to use unconstrained brillig pointers #[cfg(test)] mod tests { use super::*; @@ -625,7 +849,7 @@ mod tests { acir::{ circuit::{ brillig::{BrilligInputs, BrilligOutputs}, - opcodes::BlockId, + opcodes::{BlockId, BlockType}, }, native_types::Expression, AcirField, @@ -675,7 +899,8 @@ mod tests { }]; let brillig_funcs = &vec![brillig_bytecode]; let current_witness_index = 2; - let circuit = &Circuit { current_witness_index, opcodes, ..Circuit::default() }; + let circuit = Circuit { current_witness_index, opcodes, ..Circuit::default() }; + let circuits = &vec![circuit]; let debug_symbols = vec![]; let file_map = BTreeMap::new(); @@ -687,51 +912,66 @@ mod tests { Box::new(DefaultDebugForeignCallExecutor::from_artifact(true, debug_artifact)); let mut context = DebugContext::new( &StubbedBlackBoxSolver, - circuit, + circuits, debug_artifact, initial_witness, foreign_call_executor, brillig_funcs, ); - assert_eq!(context.get_current_opcode_location(), Some(OpcodeLocation::Acir(0))); + assert_eq!( + context.get_current_debug_location(), + Some(DebugLocation { circuit_id: 0, opcode_location: OpcodeLocation::Acir(0) }) + ); // Execute the first Brillig opcode (calldata copy) let result = context.step_into_opcode(); assert!(matches!(result, DebugCommandResult::Ok)); assert_eq!( - context.get_current_opcode_location(), - Some(OpcodeLocation::Brillig { acir_index: 0, brillig_index: 1 }) + context.get_current_debug_location(), + Some(DebugLocation { + circuit_id: 0, + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 1 } + }) ); // execute the second Brillig opcode (const) let result = context.step_into_opcode(); assert!(matches!(result, DebugCommandResult::Ok)); assert_eq!( - context.get_current_opcode_location(), - Some(OpcodeLocation::Brillig { acir_index: 0, brillig_index: 2 }) + context.get_current_debug_location(), + Some(DebugLocation { + circuit_id: 0, + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 2 } + }) ); // try to execute the third Brillig opcode (and resolve the foreign call) let result = context.step_into_opcode(); assert!(matches!(result, DebugCommandResult::Ok)); assert_eq!( - context.get_current_opcode_location(), - Some(OpcodeLocation::Brillig { acir_index: 0, brillig_index: 2 }) + context.get_current_debug_location(), + Some(DebugLocation { + circuit_id: 0, + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 2 } + }) ); // retry the third Brillig opcode (foreign call should be finished) let result = context.step_into_opcode(); assert!(matches!(result, DebugCommandResult::Ok)); assert_eq!( - context.get_current_opcode_location(), - Some(OpcodeLocation::Brillig { acir_index: 0, brillig_index: 3 }) + context.get_current_debug_location(), + Some(DebugLocation { + circuit_id: 0, + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 3 } + }) ); // last Brillig opcode let result = context.step_into_opcode(); assert!(matches!(result, DebugCommandResult::Done)); - assert_eq!(context.get_current_opcode_location(), None); + assert_eq!(context.get_current_debug_location(), None); } #[test] @@ -784,7 +1024,8 @@ mod tests { }), ]; let current_witness_index = 3; - let circuit = &Circuit { current_witness_index, opcodes, ..Circuit::default() }; + let circuit = Circuit { current_witness_index, opcodes, ..Circuit::default() }; + let circuits = &vec![circuit]; let debug_symbols = vec![]; let file_map = BTreeMap::new(); @@ -797,7 +1038,7 @@ mod tests { let brillig_funcs = &vec![brillig_bytecode]; let mut context = DebugContext::new( &StubbedBlackBoxSolver, - circuit, + circuits, debug_artifact, initial_witness, foreign_call_executor, @@ -805,28 +1046,40 @@ mod tests { ); // set breakpoint - let breakpoint_location = OpcodeLocation::Brillig { acir_index: 0, brillig_index: 1 }; + let breakpoint_location = DebugLocation { + circuit_id: 0, + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 1 }, + }; assert!(context.add_breakpoint(breakpoint_location)); // execute the first ACIR opcode (Brillig block) -> should reach the breakpoint instead let result = context.step_acir_opcode(); assert!(matches!(result, DebugCommandResult::BreakpointReached(_))); - assert_eq!(context.get_current_opcode_location(), Some(breakpoint_location)); + assert_eq!(context.get_current_debug_location(), Some(breakpoint_location)); // continue execution to the next ACIR opcode let result = context.step_acir_opcode(); assert!(matches!(result, DebugCommandResult::Ok)); - assert_eq!(context.get_current_opcode_location(), Some(OpcodeLocation::Acir(1))); + assert_eq!( + context.get_current_debug_location(), + Some(DebugLocation { circuit_id: 0, opcode_location: OpcodeLocation::Acir(1) }) + ); // last ACIR opcode let result = context.step_acir_opcode(); assert!(matches!(result, DebugCommandResult::Done)); - assert_eq!(context.get_current_opcode_location(), None); + assert_eq!(context.get_current_debug_location(), None); } #[test] - fn test_address_opcode_location_mapping() { - let brillig_bytecode = BrilligBytecode { + fn test_address_debug_location_mapping() { + let brillig_one = BrilligBytecode { + bytecode: vec![ + BrilligOpcode::Stop { return_data_offset: 0, return_data_size: 0 }, + BrilligOpcode::Stop { return_data_offset: 0, return_data_size: 0 }, + ], + }; + let brillig_two = BrilligBytecode { bytecode: vec![ BrilligOpcode::Stop { return_data_offset: 0, return_data_size: 0 }, BrilligOpcode::Stop { return_data_offset: 0, return_data_size: 0 }, @@ -834,22 +1087,33 @@ mod tests { ], }; - let opcodes = vec![ - Opcode::BrilligCall { id: 0, inputs: vec![], outputs: vec![], predicate: None }, - Opcode::MemoryInit { - block_id: BlockId(0), - init: vec![], - block_type: acvm::acir::circuit::opcodes::BlockType::Memory, - }, - Opcode::BrilligCall { id: 0, inputs: vec![], outputs: vec![], predicate: None }, - Opcode::AssertZero(Expression::default()), - ]; - let circuit = Circuit { opcodes, ..Circuit::default() }; + let circuit_one = Circuit { + opcodes: vec![ + Opcode::MemoryInit { + block_id: BlockId(0), + init: vec![], + block_type: BlockType::Memory, + }, + Opcode::BrilligCall { id: 0, inputs: vec![], outputs: vec![], predicate: None }, + Opcode::Call { id: 1, inputs: vec![], outputs: vec![], predicate: None }, + Opcode::AssertZero(Expression::default()), + ], + ..Circuit::default() + }; + let circuit_two = Circuit { + opcodes: vec![ + Opcode::BrilligCall { id: 1, inputs: vec![], outputs: vec![], predicate: None }, + Opcode::AssertZero(Expression::default()), + ], + ..Circuit::default() + }; + let circuits = vec![circuit_one, circuit_two]; let debug_artifact = DebugArtifact { debug_symbols: vec![], file_map: BTreeMap::new() }; - let brillig_funcs = &vec![brillig_bytecode]; + let brillig_funcs = &vec![brillig_one, brillig_two]; + let context = DebugContext::new( &StubbedBlackBoxSolver, - &circuit, + &circuits, &debug_artifact, WitnessMap::new(), Box::new(DefaultDebugForeignCallExecutor::new(true)), @@ -857,46 +1121,56 @@ mod tests { ); let locations = - (0..=7).map(|address| context.address_to_opcode_location(address)).collect::>(); + (0..=8).map(|address| context.address_to_debug_location(address)).collect::>(); // mapping from addresses to opcode locations assert_eq!( locations, vec![ - Some(OpcodeLocation::Acir(0)), - Some(OpcodeLocation::Brillig { acir_index: 0, brillig_index: 1 }), - Some(OpcodeLocation::Brillig { acir_index: 0, brillig_index: 2 }), - Some(OpcodeLocation::Acir(1)), - Some(OpcodeLocation::Acir(2)), - Some(OpcodeLocation::Brillig { acir_index: 2, brillig_index: 1 }), - Some(OpcodeLocation::Brillig { acir_index: 2, brillig_index: 2 }), - Some(OpcodeLocation::Acir(3)), + Some(DebugLocation { circuit_id: 0, opcode_location: OpcodeLocation::Acir(0) }), + Some(DebugLocation { circuit_id: 0, opcode_location: OpcodeLocation::Acir(1) }), + Some(DebugLocation { + circuit_id: 0, + opcode_location: OpcodeLocation::Brillig { acir_index: 1, brillig_index: 1 } + }), + Some(DebugLocation { circuit_id: 0, opcode_location: OpcodeLocation::Acir(2) }), + Some(DebugLocation { circuit_id: 0, opcode_location: OpcodeLocation::Acir(3) }), + Some(DebugLocation { circuit_id: 1, opcode_location: OpcodeLocation::Acir(0) }), + Some(DebugLocation { + circuit_id: 1, + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 1 } + }), + Some(DebugLocation { + circuit_id: 1, + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 2 } + }), + Some(DebugLocation { circuit_id: 1, opcode_location: OpcodeLocation::Acir(1) }), ] ); let addresses = locations .iter() .flatten() - .map(|location| context.opcode_location_to_address(location)) + .map(|location| context.debug_location_to_address(location)) .collect::>(); // and vice-versa - assert_eq!(addresses, (0..=7).collect::>()); + assert_eq!(addresses, (0..=8).collect::>()); // check edge cases - assert_eq!(None, context.address_to_opcode_location(8)); + assert_eq!(None, context.address_to_debug_location(9)); assert_eq!( - 0, - context.opcode_location_to_address(&OpcodeLocation::Brillig { - acir_index: 0, - brillig_index: 0 + 1, + context.debug_location_to_address(&DebugLocation { + circuit_id: 0, + opcode_location: OpcodeLocation::Brillig { acir_index: 1, brillig_index: 0 } }) ); assert_eq!( - 4, - context.opcode_location_to_address(&OpcodeLocation::Brillig { - acir_index: 2, - brillig_index: 0 + 5, + context.debug_location_to_address(&DebugLocation { + circuit_id: 1, + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 0 } }) ); } diff --git a/tooling/debugger/src/dap.rs b/tooling/debugger/src/dap.rs index 77abf3093cd..cfe33a61cb5 100644 --- a/tooling/debugger/src/dap.rs +++ b/tooling/debugger/src/dap.rs @@ -2,12 +2,12 @@ use std::collections::BTreeMap; use std::io::{Read, Write}; use acvm::acir::circuit::brillig::BrilligBytecode; -use acvm::acir::circuit::{Circuit, OpcodeLocation}; +use acvm::acir::circuit::Circuit; use acvm::acir::native_types::WitnessMap; use acvm::{BlackBoxFunctionSolver, FieldElement}; -use crate::context::DebugCommandResult; use crate::context::DebugContext; +use crate::context::{DebugCommandResult, DebugLocation}; use crate::foreign_calls::DefaultDebugForeignCallExecutor; use dap::errors::ServerError; @@ -37,8 +37,8 @@ pub struct DapSession<'a, R: Read, W: Write, B: BlackBoxFunctionSolver, - source_breakpoints: BTreeMap>, + instruction_breakpoints: Vec<(DebugLocation, BreakpointId)>, + source_breakpoints: BTreeMap>, } enum ScopeReferences { @@ -61,14 +61,14 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession< pub fn new( server: Server, solver: &'a B, - circuit: &'a Circuit, + circuits: &'a [Circuit], debug_artifact: &'a DebugArtifact, initial_witness: WitnessMap, unconstrained_functions: &'a [BrilligBytecode], ) -> Self { let context = DebugContext::new( solver, - circuit, + circuits, debug_artifact, initial_witness, Box::new(DefaultDebugForeignCallExecutor::from_artifact(true, debug_artifact)), @@ -100,7 +100,7 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession< } pub fn run_loop(&mut self) -> Result<(), ServerError> { - self.running = self.context.get_current_opcode_location().is_some(); + self.running = self.context.get_current_debug_location().is_some(); if self.running && self.context.get_current_source_location().is_none() { // TODO: remove this? This is to ensure that the tool has a proper @@ -194,7 +194,7 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession< .get_source_call_stack() .iter() .enumerate() - .map(|(index, (opcode_location, source_location))| { + .map(|(index, (debug_location, source_location))| { let line_number = self.debug_artifact.location_line_number(*source_location).unwrap(); let column_number = @@ -204,7 +204,7 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession< Some(frame) => format!("{} {}", frame.function_name, index), None => format!("frame #{index}"), }; - let address = self.context.opcode_location_to_address(opcode_location); + let address = self.context.debug_location_to_address(debug_location); StackFrame { id: index as i64, @@ -251,18 +251,18 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession< let mut instructions: Vec = vec![]; while count > 0 { - let opcode_location = if address >= 0 { - self.context.address_to_opcode_location(address as usize) + let debug_location = if address >= 0 { + self.context.address_to_debug_location(address as usize) } else { None }; - if let Some(opcode_location) = opcode_location { + if let Some(debug_location) = debug_location { instructions.push(DisassembledInstruction { address: address.to_string(), // we'll use the instruction_bytes field to render the OpcodeLocation - instruction_bytes: Some(opcode_location.to_string()), - instruction: self.context.render_opcode_at_location(&opcode_location), + instruction_bytes: Some(debug_location.to_string()), + instruction: self.context.render_opcode_at_location(&debug_location), ..DisassembledInstruction::default() }); } else { @@ -320,16 +320,16 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession< self.handle_execution_result(result) } - fn find_breakpoints_at_location(&self, opcode_location: &OpcodeLocation) -> Vec { + fn find_breakpoints_at_location(&self, debug_location: &DebugLocation) -> Vec { let mut result = vec![]; for (location, id) in &self.instruction_breakpoints { - if opcode_location == location { + if debug_location == location { result.push(*id); } } for breakpoints in self.source_breakpoints.values() { for (location, id) in breakpoints { - if opcode_location == location { + if debug_location == location { result.push(*id); } } @@ -404,7 +404,7 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession< }; // compute breakpoints to set and return - let mut breakpoints_to_set: Vec<(OpcodeLocation, i64)> = vec![]; + let mut breakpoints_to_set: Vec<(DebugLocation, i64)> = vec![]; let breakpoints: Vec = args .breakpoints .iter() @@ -420,8 +420,8 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession< }; let Some(location) = self .context - .address_to_opcode_location(address) - .filter(|location| self.context.is_valid_opcode_location(location)) + .address_to_debug_location(address) + .filter(|location| self.context.is_valid_debug_location(location)) else { return Breakpoint { verified: false, @@ -472,7 +472,7 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession< let Some(ref breakpoints) = &args.breakpoints else { return vec![]; }; - let mut breakpoints_to_set: Vec<(OpcodeLocation, i64)> = vec![]; + let mut breakpoints_to_set: Vec<(DebugLocation, i64)> = vec![]; let breakpoints = breakpoints .iter() .map(|breakpoint| { @@ -490,14 +490,14 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession< // TODO: line will not necessarily be the one requested; we // should do the reverse mapping and retrieve the actual source // code line number - if !self.context.is_valid_opcode_location(&location) { + if !self.context.is_valid_debug_location(&location) { return Breakpoint { verified: false, message: Some(String::from("Invalid opcode location")), ..Breakpoint::default() }; } - let breakpoint_address = self.context.opcode_location_to_address(&location); + let breakpoint_address = self.context.debug_location_to_address(&location); let instruction_reference = format!("{}", breakpoint_address); let breakpoint_id = self.get_next_breakpoint_id(); breakpoints_to_set.push((location, breakpoint_id)); @@ -612,7 +612,7 @@ pub fn run_session>( let mut session = DapSession::new( server, solver, - &program.program.functions[0], + &program.program.functions, &debug_artifact, initial_witness, &program.program.unconstrained_functions, diff --git a/tooling/debugger/src/lib.rs b/tooling/debugger/src/lib.rs index 9d0059ee495..37ac088ca35 100644 --- a/tooling/debugger/src/lib.rs +++ b/tooling/debugger/src/lib.rs @@ -9,23 +9,18 @@ use std::io::{Read, Write}; use ::dap::errors::ServerError; use ::dap::server::Server; -use acvm::acir::circuit::brillig::BrilligBytecode; -use acvm::{acir::circuit::Circuit, acir::native_types::WitnessMap}; +use acvm::acir::native_types::{WitnessMap, WitnessStack}; use acvm::{BlackBoxFunctionSolver, FieldElement}; -use noirc_artifacts::debug::DebugArtifact; - use nargo::NargoError; use noirc_driver::CompiledProgram; -pub fn debug_circuit>( - blackbox_solver: &B, - circuit: &Circuit, - debug_artifact: DebugArtifact, +pub fn run_repl_session>( + solver: &B, + program: CompiledProgram, initial_witness: WitnessMap, - unconstrained_functions: &[BrilligBytecode], -) -> Result>, NargoError> { - repl::run(blackbox_solver, circuit, &debug_artifact, initial_witness, unconstrained_functions) +) -> Result>, NargoError> { + repl::run(solver, program, initial_witness) } pub fn run_dap_loop>( diff --git a/tooling/debugger/src/repl.rs b/tooling/debugger/src/repl.rs index 7d8c6e0947d..d3462985642 100644 --- a/tooling/debugger/src/repl.rs +++ b/tooling/debugger/src/repl.rs @@ -1,11 +1,12 @@ -use crate::context::{DebugCommandResult, DebugContext}; +use crate::context::{DebugCommandResult, DebugContext, DebugLocation}; use acvm::acir::circuit::brillig::BrilligBytecode; use acvm::acir::circuit::{Circuit, Opcode, OpcodeLocation}; -use acvm::acir::native_types::{Witness, WitnessMap}; +use acvm::acir::native_types::{Witness, WitnessMap, WitnessStack}; use acvm::brillig_vm::brillig::Opcode as BrilligOpcode; use acvm::{BlackBoxFunctionSolver, FieldElement}; use nargo::NargoError; +use noirc_driver::CompiledProgram; use crate::foreign_calls::DefaultDebugForeignCallExecutor; use noirc_artifacts::debug::DebugArtifact; @@ -19,17 +20,21 @@ use crate::source_code_printer::print_source_code_location; pub struct ReplDebugger<'a, B: BlackBoxFunctionSolver> { context: DebugContext<'a, B>, blackbox_solver: &'a B, - circuit: &'a Circuit, debug_artifact: &'a DebugArtifact, initial_witness: WitnessMap, last_result: DebugCommandResult, + + // ACIR functions to debug + circuits: &'a [Circuit], + + // Brillig functions referenced from the ACIR circuits above unconstrained_functions: &'a [BrilligBytecode], } impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { pub fn new( blackbox_solver: &'a B, - circuit: &'a Circuit, + circuits: &'a [Circuit], debug_artifact: &'a DebugArtifact, initial_witness: WitnessMap, unconstrained_functions: &'a [BrilligBytecode], @@ -38,13 +43,13 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { Box::new(DefaultDebugForeignCallExecutor::from_artifact(true, debug_artifact)); let context = DebugContext::new( blackbox_solver, - circuit, + circuits, debug_artifact, initial_witness.clone(), foreign_call_executor, unconstrained_functions, ); - let last_result = if context.get_current_opcode_location().is_none() { + let last_result = if context.get_current_debug_location().is_none() { // handle circuit with no opcodes DebugCommandResult::Done } else { @@ -53,7 +58,7 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { Self { context, blackbox_solver, - circuit, + circuits, debug_artifact, initial_witness, last_result, @@ -62,42 +67,43 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { } pub fn show_current_vm_status(&self) { - let location = self.context.get_current_opcode_location(); - let opcodes = self.context.get_opcodes(); + let location = self.context.get_current_debug_location(); match location { None => println!("Finished execution"), Some(location) => { - match location { + let circuit_id = location.circuit_id; + let opcodes = self.context.get_opcodes_of_circuit(circuit_id); + match &location.opcode_location { OpcodeLocation::Acir(ip) => { - println!("At opcode {}: {}", ip, opcodes[ip]); + println!("At opcode {} :: {}", location, opcodes[*ip]); } OpcodeLocation::Brillig { acir_index, brillig_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"); }; println!( - "At opcode {}.{}: {:?}", - acir_index, brillig_index, brillig_bytecode[brillig_index] + "At opcode {} :: {:?}", + location, brillig_bytecode[*brillig_index] ); } } - let locations = self.context.get_source_location_for_opcode_location(&location); + let locations = self.context.get_source_location_for_debug_location(&location); print_source_code_location(self.debug_artifact, &locations); } } } - fn show_stack_frame(&self, index: usize, location: &OpcodeLocation) { + fn show_stack_frame(&self, index: usize, debug_location: &DebugLocation) { let opcodes = self.context.get_opcodes(); - match location { + match &debug_location.opcode_location { OpcodeLocation::Acir(instruction_pointer) => { println!( - "Frame #{index}, opcode {}: {}", - instruction_pointer, opcodes[*instruction_pointer] + "Frame #{index}, opcode {} :: {}", + debug_location, opcodes[*instruction_pointer] ) } OpcodeLocation::Brillig { acir_index, brillig_index } => { @@ -108,12 +114,12 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { unreachable!("Brillig location does not contain Brillig opcodes"); }; println!( - "Frame #{index}, opcode {}.{}: {:?}", - acir_index, brillig_index, brillig_bytecode[*brillig_index] + "Frame #{index}, opcode {} :: {:?}", + debug_location, brillig_bytecode[*brillig_index] ); } } - let locations = self.context.get_source_location_for_opcode_location(location); + let locations = self.context.get_source_location_for_debug_location(debug_location); print_source_code_location(self.debug_artifact, &locations); } @@ -130,8 +136,21 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { } fn display_opcodes(&self) { - let opcodes = self.context.get_opcodes(); - let current_opcode_location = self.context.get_current_opcode_location(); + for i in 0..self.circuits.len() { + self.display_opcodes_of_circuit(i as u32); + } + } + + fn display_opcodes_of_circuit(&self, circuit_id: u32) { + let current_opcode_location = + self.context.get_current_debug_location().and_then(|debug_location| { + if debug_location.circuit_id == circuit_id { + Some(debug_location.opcode_location) + } else { + None + } + }); + 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), @@ -144,7 +163,10 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { let outer_marker = |acir_index| { if current_acir_index == Some(acir_index) { "->" - } else if self.context.is_breakpoint_set(&OpcodeLocation::Acir(acir_index)) { + } else if self.context.is_breakpoint_set(&DebugLocation { + circuit_id, + opcode_location: OpcodeLocation::Acir(acir_index), + }) { " *" } else { "" @@ -153,10 +175,10 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { let brillig_marker = |acir_index, brillig_index| { if current_acir_index == Some(acir_index) && brillig_index == current_brillig_index { "->" - } else if self - .context - .is_breakpoint_set(&OpcodeLocation::Brillig { acir_index, brillig_index }) - { + } else if self.context.is_breakpoint_set(&DebugLocation { + circuit_id, + opcode_location: OpcodeLocation::Brillig { acir_index, brillig_index }, + }) { " *" } else { "" @@ -165,7 +187,8 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { let print_brillig_bytecode = |acir_index, bytecode: &[BrilligOpcode]| { for (brillig_index, brillig_opcode) in bytecode.iter().enumerate() { println!( - "{:>3}.{:<2} |{:2} {:?}", + "{:>2}:{:>3}.{:<2} |{:2} {:?}", + circuit_id, acir_index, brillig_index, brillig_marker(acir_index, brillig_index), @@ -178,33 +201,33 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { match &opcode { Opcode::BrilligCall { id, inputs, outputs, .. } => { println!( - "{:>3} {:2} BRILLIG CALL id={} inputs={:?}", - acir_index, marker, id, inputs + "{:>2}:{:>3} {:2} BRILLIG CALL id={} inputs={:?}", + circuit_id, acir_index, marker, id, inputs ); - println!(" | outputs={:?}", outputs); + println!(" | outputs={:?}", outputs); let bytecode = &self.unconstrained_functions[*id as usize].bytecode; print_brillig_bytecode(acir_index, bytecode); } - _ => println!("{:>3} {:2} {:?}", acir_index, marker, opcode), + _ => println!("{:>2}:{:>3} {:2} {:?}", circuit_id, acir_index, marker, opcode), } } } - fn add_breakpoint_at(&mut self, location: OpcodeLocation) { - if !self.context.is_valid_opcode_location(&location) { - println!("Invalid opcode location {location}"); + fn add_breakpoint_at(&mut self, location: DebugLocation) { + if !self.context.is_valid_debug_location(&location) { + println!("Invalid location {location}"); } else if self.context.add_breakpoint(location) { - println!("Added breakpoint at opcode {location}"); + println!("Added breakpoint at {location}"); } else { - println!("Breakpoint at opcode {location} already set"); + println!("Breakpoint at {location} already set"); } } - fn delete_breakpoint_at(&mut self, location: OpcodeLocation) { + fn delete_breakpoint_at(&mut self, location: DebugLocation) { if self.context.delete_breakpoint(&location) { - println!("Breakpoint at opcode {location} deleted"); + println!("Breakpoint at {location} deleted"); } else { - println!("Breakpoint at opcode {location} not set"); + println!("Breakpoint at {location} not set"); } } @@ -281,20 +304,19 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { } fn restart_session(&mut self) { - let breakpoints: Vec = - self.context.iterate_breakpoints().copied().collect(); + let breakpoints: Vec = self.context.iterate_breakpoints().copied().collect(); let foreign_call_executor = Box::new(DefaultDebugForeignCallExecutor::from_artifact(true, self.debug_artifact)); self.context = DebugContext::new( self.blackbox_solver, - self.circuit, + self.circuits, self.debug_artifact, self.initial_witness.clone(), foreign_call_executor, self.unconstrained_functions, ); - for opcode_location in breakpoints { - self.context.add_breakpoint(opcode_location); + for debug_location in breakpoints { + self.context.add_breakpoint(debug_location); } self.last_result = DebugCommandResult::Ok; println!("Restarted debugging session."); @@ -372,21 +394,23 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { self.context.is_solved() } - fn finalize(self) -> WitnessMap { + fn finalize(self) -> WitnessStack { self.context.finalize() } } pub fn run>( blackbox_solver: &B, - circuit: &Circuit, - debug_artifact: &DebugArtifact, + program: CompiledProgram, initial_witness: WitnessMap, - unconstrained_functions: &[BrilligBytecode], -) -> Result>, NargoError> { +) -> Result>, NargoError> { + let circuits = &program.program.functions; + let debug_artifact = + &DebugArtifact { debug_symbols: program.debug, file_map: program.file_map }; + let unconstrained_functions = &program.program.unconstrained_functions; let context = RefCell::new(ReplDebugger::new( blackbox_solver, - circuit, + circuits, debug_artifact, initial_witness, unconstrained_functions, @@ -480,7 +504,7 @@ pub fn run>( "break", command! { "add a breakpoint at an opcode location", - (LOCATION:OpcodeLocation) => |location| { + (LOCATION:DebugLocation) => |location| { ref_context.borrow_mut().add_breakpoint_at(location); Ok(CommandStatus::Done) } @@ -490,7 +514,7 @@ pub fn run>( "delete", command! { "delete breakpoint at an opcode location", - (LOCATION:OpcodeLocation) => |location| { + (LOCATION:DebugLocation) => |location| { ref_context.borrow_mut().delete_breakpoint_at(location); Ok(CommandStatus::Done) } @@ -576,8 +600,8 @@ pub fn run>( drop(repl); if context.borrow().is_solved() { - let solved_witness = context.into_inner().finalize(); - Ok(Some(solved_witness)) + let solved_witness_stack = context.into_inner().finalize(); + Ok(Some(solved_witness_stack)) } else { Ok(None) } diff --git a/tooling/debugger/tests/debug.rs b/tooling/debugger/tests/debug.rs index 313b6b30591..2dca6b95f0e 100644 --- a/tooling/debugger/tests/debug.rs +++ b/tooling/debugger/tests/debug.rs @@ -12,7 +12,7 @@ mod tests { let nargo_bin = cargo_bin("nargo").into_os_string().into_string().expect("Cannot parse nargo path"); - let timeout_seconds = 20; + let timeout_seconds = 25; let mut dbg_session = spawn_bash(Some(timeout_seconds * 1000)).expect("Could not start bash session"); diff --git a/tooling/nargo_cli/src/cli/debug_cmd.rs b/tooling/nargo_cli/src/cli/debug_cmd.rs index 778009bf791..2014a3acaa4 100644 --- a/tooling/nargo_cli/src/cli/debug_cmd.rs +++ b/tooling/nargo_cli/src/cli/debug_cmd.rs @@ -1,6 +1,6 @@ use std::path::PathBuf; -use acvm::acir::native_types::{WitnessMap, WitnessStack}; +use acvm::acir::native_types::WitnessStack; use acvm::FieldElement; use bn254_blackbox_solver::Bn254BlackBoxSolver; use clap::Args; @@ -15,7 +15,6 @@ use nargo::{insert_all_files_for_workspace_into_file_manager, parse_all}; use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection}; use noirc_abi::input_parser::{Format, InputValue}; use noirc_abi::InputMap; -use noirc_artifacts::debug::DebugArtifact; use noirc_driver::{ file_manager_with_stdlib, CompileOptions, CompiledProgram, NOIR_ARTIFACT_VERSION_STRING, }; @@ -169,10 +168,10 @@ fn run_async( runtime.block_on(async { println!("[{}] Starting debugger", package.name); - let (return_value, solved_witness) = + let (return_value, witness_stack) = debug_program_and_decode(program, package, prover_name)?; - if let Some(solved_witness) = solved_witness { + if let Some(solved_witness_stack) = witness_stack { println!("[{}] Circuit witness successfully solved", package.name); if let Some(return_value) = return_value { @@ -180,11 +179,8 @@ fn run_async( } if let Some(witness_name) = witness_name { - let witness_path = save_witness_to_dir( - WitnessStack::from(solved_witness), - witness_name, - target_dir, - )?; + let witness_path = + save_witness_to_dir(solved_witness_stack, witness_name, target_dir)?; println!("[{}] Witness saved to {}", package.name, witness_path.display()); } @@ -200,38 +196,32 @@ fn debug_program_and_decode( program: CompiledProgram, package: &Package, prover_name: &str, -) -> Result<(Option, Option>), CliError> { +) -> Result<(Option, Option>), CliError> { // Parse the initial witness values from Prover.toml let (inputs_map, _) = read_inputs_from_file(&package.root_dir, prover_name, Format::Toml, &program.abi)?; - let solved_witness = debug_program(&program, &inputs_map)?; - - match solved_witness { - Some(witness) => { - let (_, return_value) = program.abi.decode(&witness)?; - Ok((return_value, Some(witness))) + let program_abi = program.abi.clone(); + let witness_stack = debug_program(program, &inputs_map)?; + + match witness_stack { + Some(witness_stack) => { + let main_witness = &witness_stack + .peek() + .expect("Should have at least one witness on the stack") + .witness; + let (_, return_value) = program_abi.decode(main_witness)?; + Ok((return_value, Some(witness_stack))) } None => Ok((None, None)), } } pub(crate) fn debug_program( - compiled_program: &CompiledProgram, + compiled_program: CompiledProgram, inputs_map: &InputMap, -) -> Result>, CliError> { +) -> Result>, CliError> { let initial_witness = compiled_program.abi.encode(inputs_map, None)?; - let debug_artifact = DebugArtifact { - debug_symbols: compiled_program.debug.clone(), - file_map: compiled_program.file_map.clone(), - }; - - noir_debugger::debug_circuit( - &Bn254BlackBoxSolver, - &compiled_program.program.functions[0], - debug_artifact, - initial_witness, - &compiled_program.program.unconstrained_functions, - ) - .map_err(CliError::from) + noir_debugger::run_repl_session(&Bn254BlackBoxSolver, compiled_program, initial_witness) + .map_err(CliError::from) }