From f28ada2bd4f6d4d4ab0167f46e6abe266796b6f8 Mon Sep 17 00:00:00 2001 From: Ana Perez Ghiglia Date: Mon, 17 Jun 2024 16:31:53 -0600 Subject: [PATCH 1/4] clean up DebugLocation::from_str --- tooling/debugger/src/context.rs | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/tooling/debugger/src/context.rs b/tooling/debugger/src/context.rs index aaf3f1d01eb..1bcdb8a0a11 100644 --- a/tooling/debugger/src/context.rs +++ b/tooling/debugger/src/context.rs @@ -49,28 +49,23 @@ 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())); - if parts.is_empty() || parts.len() > 2 { - return Err(DebugLocationFromStrError::InvalidDebugLocationString(s.to_string())); - } - - fn parse_components(parts: &Vec<&str>) -> Option { - match parts.len() { - 1 => { - let opcode_location = OpcodeLocation::from_str(parts[0]).ok()?; - Some(DebugLocation { circuit_id: 0, opcode_location }) - } - 2 => { - let circuit_id = parts[0].parse().ok()?; - let opcode_location = OpcodeLocation::from_str(parts[1]).ok()?; - Some(DebugLocation { circuit_id, opcode_location }) + 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 } - _ => unreachable!("`DebugLocation` has too many components"), } + _ => error, } - - parse_components(&parts) - .ok_or(DebugLocationFromStrError::InvalidDebugLocationString(s.to_string())) } } From 613a4f3a9251981d9577df5fe7aed5f5687dd3ae Mon Sep 17 00:00:00 2001 From: Ana Perez Ghiglia Date: Tue, 18 Jun 2024 14:46:26 -0600 Subject: [PATCH 2/4] Create new AddressMap struct for abstracting acir_opcode_addresses type --- tooling/debugger/src/context.rs | 188 +++++++++++++++++++++----------- tooling/debugger/src/repl.rs | 2 +- 2 files changed, 128 insertions(+), 62 deletions(-) diff --git a/tooling/debugger/src/context.rs b/tooling/debugger/src/context.rs index 1bcdb8a0a11..19d97a62a8e 100644 --- a/tooling/debugger/src/context.rs +++ b/tooling/debugger/src/context.rs @@ -21,6 +21,128 @@ use thiserror::Error; use std::collections::BTreeMap; use std::collections::{hash_set::Iter, HashSet}; +#[derive(Clone, Debug, Eq, Hash, PartialEq, PartialOrd, Ord)] +pub struct AddressMap { + // A Noir program is composed by + // `n` ACIR circuits + // |_ `m` ACIR opcodes + // |_ Acir call + // |_ Acir Brillig function invocation + // |_ `p` Brillig opcodes + // + // We need to inline such structure to a one-dimension "virtual address" space + // We define the address space as a contiguous space where all ACIR and + // Brillig opcodes from all circuits are laid out + // + // `addresses: Vec>`` + // * The first 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 + // + // * an ACIR call accumulates 1 (since it's a single opcode) + // * an ACIR Brillig call accumulates as many opcodes the brillig function has (`p`) + // + // As a result the flattened addresses list may have "holes". + // This holes are a side effect of the brillig function calls + // + 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 mut result = Vec::with_capacity(circuits.len()); + let mut circuit_address_start = 0usize; + + for circuit in circuits { + let mut circuit_addresses = Vec::with_capacity(circuit.opcodes.len()); + // push the starting address of the first opcode + circuit_addresses.push(circuit_address_start); + + // iterate opcodes _init_ (all but last) + let last_opcode_address = circuit.opcodes.iter().take(circuit.opcodes.len() - 1).fold( + circuit_address_start, + |acc, opcode| { + let acc = acc + Self::calculate_opcode_size(opcode, unconstrained_functions); + // push the starting address of the next opcode + circuit_addresses.push(acc); + acc + }, + ); + // last opcode size should be next circuit address start + let last_opcode = circuit.opcodes.last().expect("There is a last opcode"); + circuit_address_start = last_opcode_address + + Self::calculate_opcode_size(last_opcode, unconstrained_functions); + + result.push(circuit_addresses); + } + let last_valid_address = circuit_address_start - 1; + Self { addresses: result, last_valid_address } + } + + fn calculate_opcode_size( + opcode: &Opcode, + unconstrained_functions: &[BrilligBytecode], + ) -> usize { + match opcode { + Opcode::BrilligCall { id, .. } => unconstrained_functions[*id as usize].bytecode.len(), + _ => 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, @@ -109,7 +231,7 @@ pub(super) struct DebugContext<'a, B: BlackBoxFunctionSolver> { // At the end of each vector (both outer and inner) there will be an extra // sentinel element with the address of the next opcode. This is only to // make bounds checking and working with binary search easier. - acir_opcode_addresses: Vec>, + acir_opcode_addresses: AddressMap, } impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { @@ -124,7 +246,7 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { let source_to_opcodes = build_source_to_opcode_debug_mappings(debug_artifact); let current_circuit_id: u32 = 0; let initial_circuit = &circuits[current_circuit_id as usize]; - let acir_opcode_addresses = build_acir_opcode_addresses(circuits, unconstrained_functions); + let acir_opcode_addresses = AddressMap::new(circuits, unconstrained_functions); Self { acvm: ACVM::new( blackbox_solver, @@ -312,39 +434,13 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { } /// 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.acir_opcode_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 - } - } + self.acir_opcode_addresses.debug_location_to_address(location) } + // Returns the DebugLocation associated to the given address pub fn address_to_debug_location(&self, address: usize) -> Option { - if address >= *self.acir_opcode_addresses.last()?.first()? { - return None; - } - let circuit_id = match self - .acir_opcode_addresses - .binary_search_by(|addresses| addresses[0].cmp(&address)) - { - Ok(found_index) => found_index, - Err(insert_index) => insert_index - 1, - }; - let opcode_location = match self.acir_opcode_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.acir_opcode_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 }) + self.acir_opcode_addresses.address_to_debug_location(address) } pub(super) fn render_opcode_at_location(&self, location: &DebugLocation) -> String { @@ -764,36 +860,6 @@ fn build_source_to_opcode_debug_mappings( result } -fn build_acir_opcode_addresses( - circuits: &[Circuit], - unconstrained_functions: &[BrilligBytecode], -) -> Vec> { - let mut result = Vec::with_capacity(circuits.len() + 1); - let mut circuit_address_start = 0usize; - for circuit in circuits { - let mut circuit_addresses = Vec::with_capacity(circuit.opcodes.len() + 1); - // push the starting address of the first opcode - circuit_addresses.push(circuit_address_start); - circuit_address_start = - circuit.opcodes.iter().fold(circuit_address_start, |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 - circuit_addresses.push(acc); - acc - }); - result.push(circuit_addresses); - } - result.push(vec![circuit_address_start]); - - result -} - #[cfg(test)] mod tests { use super::*; diff --git a/tooling/debugger/src/repl.rs b/tooling/debugger/src/repl.rs index e08ebf1c0aa..d3462985642 100644 --- a/tooling/debugger/src/repl.rs +++ b/tooling/debugger/src/repl.rs @@ -5,8 +5,8 @@ use acvm::acir::circuit::{Circuit, Opcode, OpcodeLocation}; use acvm::acir::native_types::{Witness, WitnessMap, WitnessStack}; use acvm::brillig_vm::brillig::Opcode as BrilligOpcode; use acvm::{BlackBoxFunctionSolver, FieldElement}; -use noirc_driver::CompiledProgram; use nargo::NargoError; +use noirc_driver::CompiledProgram; use crate::foreign_calls::DefaultDebugForeignCallExecutor; use noirc_artifacts::debug::DebugArtifact; From fa68400c13d589c9e74096793f1453b2da2ea317 Mon Sep 17 00:00:00 2001 From: Ana Perez Ghiglia Date: Fri, 21 Jun 2024 14:18:12 -0600 Subject: [PATCH 3/4] Improve AddressMap::new function * improve descriptive comments Co-authored-by: ggiraldez --- tooling/debugger/src/context.rs | 67 ++++++++++++++------------------- 1 file changed, 28 insertions(+), 39 deletions(-) diff --git a/tooling/debugger/src/context.rs b/tooling/debugger/src/context.rs index 19d97a62a8e..bd47a483653 100644 --- a/tooling/debugger/src/context.rs +++ b/tooling/debugger/src/context.rs @@ -30,9 +30,11 @@ pub struct AddressMap { // |_ Acir Brillig function invocation // |_ `p` Brillig opcodes // - // We need to inline such structure to a one-dimension "virtual address" space - // We define the address space as a contiguous space where all ACIR and - // Brillig opcodes from all circuits are laid out + // 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 first vec is `n` sized - one element per ACIR circuit @@ -43,11 +45,14 @@ pub struct AddressMap { // We start by assigning 0 to the very first ACIR opcode and then start accumulating by // traversing by depth-first // - // * an ACIR call accumulates 1 (since it's a single opcode) - // * an ACIR Brillig call accumulates as many opcodes the brillig function has (`p`) + // 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". // - // As a result the flattened addresses list may have "holes". - // This holes are a side effect of the brillig function calls + // 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` // addresses: Vec>, @@ -60,43 +65,27 @@ impl AddressMap { circuits: &[Circuit], unconstrained_functions: &[BrilligBytecode], ) -> Self { - let mut result = Vec::with_capacity(circuits.len()); - let mut circuit_address_start = 0usize; + 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()); - // push the starting address of the first opcode - circuit_addresses.push(circuit_address_start); - - // iterate opcodes _init_ (all but last) - let last_opcode_address = circuit.opcodes.iter().take(circuit.opcodes.len() - 1).fold( - circuit_address_start, - |acc, opcode| { - let acc = acc + Self::calculate_opcode_size(opcode, unconstrained_functions); - // push the starting address of the next opcode - circuit_addresses.push(acc); - acc - }, - ); - // last opcode size should be next circuit address start - let last_opcode = circuit.opcodes.last().expect("There is a last opcode"); - circuit_address_start = last_opcode_address - + Self::calculate_opcode_size(last_opcode, unconstrained_functions); - - result.push(circuit_addresses); + for opcode in &circuit.opcodes { + circuit_addresses.push(next_address); + next_address += opcode_address_size(opcode); + } + addresses.push(circuit_addresses); } - let last_valid_address = circuit_address_start - 1; - Self { addresses: result, last_valid_address } - } - fn calculate_opcode_size( - opcode: &Opcode, - unconstrained_functions: &[BrilligBytecode], - ) -> usize { - match opcode { - Opcode::BrilligCall { id, .. } => unconstrained_functions[*id as usize].bytecode.len(), - _ => 1, - } + Self { addresses, last_valid_address: next_address - 1 } } /// Returns the absolute address of the opcode at the given location. From ca628d4c5b1df24300a5a7e0bf1dbeea9dcbe48d Mon Sep 17 00:00:00 2001 From: Ana Perez Ghiglia Date: Fri, 21 Jun 2024 14:45:15 -0600 Subject: [PATCH 4/4] final tweaks --- tooling/debugger/src/context.rs | 71 ++++++++++++++------------------- 1 file changed, 31 insertions(+), 40 deletions(-) diff --git a/tooling/debugger/src/context.rs b/tooling/debugger/src/context.rs index bd47a483653..d18ec5f0786 100644 --- a/tooling/debugger/src/context.rs +++ b/tooling/debugger/src/context.rs @@ -21,39 +21,39 @@ 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 { - // 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 first 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` - // addresses: Vec>, // virtual address of the last opcode of the program @@ -211,15 +211,6 @@ pub(super) struct DebugContext<'a, B: BlackBoxFunctionSolver> { circuits: &'a [Circuit], unconstrained_functions: &'a [BrilligBytecode], - // We define the address space as a contiguous space where all ACIR and - // Brillig opcodes from all circuits are laid out. - // The first element of the outer vector will contain the addresses at which - // all ACIR opcodes of the first circuit are. The second element will - // contain the second circuit and so on. Brillig functions should are - // expanded on each ACIR Brillig call opcode. - // At the end of each vector (both outer and inner) there will be an extra - // sentinel element with the address of the next opcode. This is only to - // make bounds checking and working with binary search easier. acir_opcode_addresses: AddressMap, }