fix: #5051 PR comments

tooling/debugger/src/context.rs

@@ -21,6 +21,117 @@
 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
+    //
+    // 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<Vec<usize>>``
+    //  * 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<Vec<usize>>,
+
+    // virtual address of the last opcode of the program
+    last_valid_address: usize,
+}
+
+impl AddressMap {
+    pub(super) fn new(
+        circuits: &[Circuit<FieldElement>],
+        unconstrained_functions: &[BrilligBytecode<FieldElement>],
+    ) -> Self {
+        let opcode_address_size = |opcode: &Opcode<FieldElement>| {
+            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<DebugLocation> {
+        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,
@@ -49,28 +160,23 @@
     type Err = DebugLocationFromStrError;
     fn from_str(s: &str) -> Result<Self, Self::Err> {
         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<DebugLocation> {
-            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()))
     }
 }
 
@@ -114,7 +220,7 @@
     // 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<Vec<usize>>,
+    acir_opcode_addresses: AddressMap,
 }
 
 impl<'a, B: BlackBoxFunctionSolver<FieldElement>> DebugContext<'a, B> {
@@ -129,7 +235,7 @@
         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,
@@ -317,39 +423,13 @@
     }
 
     /// 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<DebugLocation> {
-        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 {
@@ -769,36 +849,6 @@
     result
 }
 
-fn build_acir_opcode_addresses(
-    circuits: &[Circuit<FieldElement>],
-    unconstrained_functions: &[BrilligBytecode<FieldElement>],
-) -> Vec<Vec<usize>> {
-    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::*;
@@ -856,7 +906,7 @@
             outputs: vec![],
             predicate: None,
         }];
         let brillig_funcs = &vec![brillig_bytecode];
         let current_witness_index = 2;
         let circuit = Circuit { current_witness_index, opcodes, ..Circuit::default() };
         let circuits = &vec![circuit];
@@ -875,7 +925,7 @@
             debug_artifact,
             initial_witness,
             foreign_call_executor,
             brillig_funcs,
         );
 
         assert_eq!(
@@ -994,14 +1044,14 @@
 
         let foreign_call_executor =
             Box::new(DefaultDebugForeignCallExecutor::from_artifact(true, debug_artifact));
         let brillig_funcs = &vec![brillig_bytecode];
         let mut context = DebugContext::new(
             &StubbedBlackBoxSolver,
             circuits,
             debug_artifact,
             initial_witness,
             foreign_call_executor,
             brillig_funcs,
         );
 
         // set breakpoint
@@ -1068,7 +1118,7 @@
         };
         let circuits = vec![circuit_one, circuit_two];
         let debug_artifact = DebugArtifact { debug_symbols: vec![], file_map: BTreeMap::new() };
         let brillig_funcs = &vec![brillig_one, brillig_two];
 
         let context = DebugContext::new(
             &StubbedBlackBoxSolver,

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;