From a31f82e598def60d00c65b79b8c5411f8aa832aa Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Mon, 12 Aug 2024 13:01:02 -0400 Subject: [PATCH] fix(debugger): Update the debugger to handle the new Brillig debug metadata format (#5706) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description ## Problem\* Resolves #5703 ## Summary\* After https://github.com/noir-lang/noir/pull/5696 debug metadata from Brillig functions was de-duplicated to avoid a blow-up in artifact size. The debugger uses this debug metadata format to accurately display information in the REPL and DAP interface. This PR contains the updates needed for the debugger to handle the new debug metadata format. The debugger on `inline_never_basic` (the example used in the linked issue) gives the following again: Screenshot 2024-08-09 at 2 28 56 PM The output matches what was on master before PR #5696. ## 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. --- acvm-repo/acvm/src/pwg/brillig.rs | 2 +- compiler/noirc_driver/src/debug.rs | 17 +- tooling/debugger/src/context.rs | 264 ++++++++++++++++++++------- tooling/debugger/src/repl.rs | 35 ++-- tooling/noirc_artifacts/src/debug.rs | 17 +- 5 files changed, 251 insertions(+), 84 deletions(-) diff --git a/acvm-repo/acvm/src/pwg/brillig.rs b/acvm-repo/acvm/src/pwg/brillig.rs index c12629b0543..5ec3224dbaa 100644 --- a/acvm-repo/acvm/src/pwg/brillig.rs +++ b/acvm-repo/acvm/src/pwg/brillig.rs @@ -32,7 +32,7 @@ pub struct BrilligSolver<'b, F, B: BlackBoxFunctionSolver> { /// This id references which Brillig function within the main ACIR program we are solving. /// This is used for appropriately resolving errors as the ACIR program artifacts /// set up their Brillig debug metadata by function id. - function_id: BrilligFunctionId, + pub function_id: BrilligFunctionId, } impl<'b, B: BlackBoxFunctionSolver, F: AcirField> BrilligSolver<'b, F, B> { diff --git a/compiler/noirc_driver/src/debug.rs b/compiler/noirc_driver/src/debug.rs index 5e309398cc5..f5eaede89b2 100644 --- a/compiler/noirc_driver/src/debug.rs +++ b/compiler/noirc_driver/src/debug.rs @@ -18,7 +18,7 @@ pub(crate) fn filter_relevant_files( debug_symbols: &[DebugInfo], file_manager: &FileManager, ) -> BTreeMap { - let files_with_debug_symbols: BTreeSet = debug_symbols + let mut files_with_debug_symbols: BTreeSet = debug_symbols .iter() .flat_map(|function_symbols| { function_symbols @@ -28,6 +28,21 @@ pub(crate) fn filter_relevant_files( }) .collect(); + let files_with_brillig_debug_symbols: BTreeSet = debug_symbols + .iter() + .flat_map(|function_symbols| { + let brillig_location_maps = + function_symbols.brillig_locations.values().flat_map(|brillig_location_map| { + brillig_location_map + .values() + .flat_map(|call_stack| call_stack.iter().map(|location| location.file)) + }); + brillig_location_maps + }) + .collect(); + + files_with_debug_symbols.extend(files_with_brillig_debug_symbols); + let mut file_map = BTreeMap::new(); for file_id in files_with_debug_symbols { diff --git a/tooling/debugger/src/context.rs b/tooling/debugger/src/context.rs index 2a550b84020..6ec1aff8325 100644 --- a/tooling/debugger/src/context.rs +++ b/tooling/debugger/src/context.rs @@ -1,6 +1,6 @@ use crate::foreign_calls::DebugForeignCallExecutor; use acvm::acir::brillig::BitSize; -use acvm::acir::circuit::brillig::BrilligBytecode; +use acvm::acir::circuit::brillig::{BrilligBytecode, BrilligFunctionId}; use acvm::acir::circuit::{Circuit, Opcode, OpcodeLocation}; use acvm::acir::native_types::{Witness, WitnessMap, WitnessStack}; use acvm::brillig_vm::MemoryValue; @@ -57,8 +57,25 @@ use std::collections::{hash_set::Iter, HashSet}; pub struct AddressMap { addresses: Vec>, - // virtual address of the last opcode of the program + /// Virtual address of the last opcode of the program last_valid_address: usize, + + /// Maps the "holes" in the `addresses` nodes to the Brillig function ID + /// associated with that address space. + brillig_addresses: Vec, +} + +/// Associates a BrilligFunctionId with the address space. +/// A BrilligFunctionId is found by checking whether an address is between +/// the `start_address` and `end_address` +#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq, PartialOrd, Ord)] +struct BrilligAddressSpace { + /// The start of the Brillig call address space + start_address: usize, + /// The end of the Brillig address space + end_address: usize, + /// The Brillig function id associated with this address space + brillig_function_id: BrilligFunctionId, } impl AddressMap { @@ -68,25 +85,35 @@ impl AddressMap { ) -> Self { let opcode_address_size = |opcode: &Opcode| { if let Opcode::BrilligCall { id, .. } = opcode { - unconstrained_functions[id.as_usize()].bytecode.len() + (unconstrained_functions[id.as_usize()].bytecode.len(), Some(*id)) } else { - 1 + (1, None) } }; let mut addresses = Vec::with_capacity(circuits.len()); let mut next_address = 0usize; + let mut brillig_addresses = Vec::new(); 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); + let (address_size, brillig_function_id) = opcode_address_size(opcode); + if let Some(brillig_function_id) = brillig_function_id { + let brillig_address_space = BrilligAddressSpace { + start_address: next_address, + end_address: next_address + address_size, + brillig_function_id, + }; + brillig_addresses.push(brillig_address_space); + } + next_address += address_size; } addresses.push(circuit_addresses); } - Self { addresses, last_valid_address: next_address - 1 } + Self { addresses, last_valid_address: next_address - 1, brillig_addresses } } /// Returns the absolute address of the opcode at the given location. @@ -120,16 +147,26 @@ impl AddressMap { // 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 }) + let (opcode_location, brillig_function_id) = + match self.addresses[circuit_id].binary_search(&address) { + Ok(found_index) => (OpcodeLocation::Acir(found_index), None), + Err(insert_index) => { + let acir_index = insert_index - 1; + let base_offset = self.addresses[circuit_id][acir_index]; + let brillig_index = address - base_offset; + let brillig_function_id = self + .brillig_addresses + .iter() + .find(|brillig_address_space| { + address >= brillig_address_space.start_address + && address <= brillig_address_space.end_address + }) + .map(|brillig_address_space| brillig_address_space.brillig_function_id); + (OpcodeLocation::Brillig { acir_index, brillig_index }, brillig_function_id) + } + }; + + Some(DebugLocation { circuit_id: circuit_id as u32, opcode_location, brillig_function_id }) } } @@ -137,6 +174,7 @@ impl AddressMap { pub struct DebugLocation { pub circuit_id: u32, pub opcode_location: OpcodeLocation, + pub brillig_function_id: Option, } impl std::fmt::Display for DebugLocation { @@ -165,13 +203,13 @@ impl std::str::FromStr for DebugLocation { match parts.len() { 1 => OpcodeLocation::from_str(parts[0]).map_or(error, |opcode_location| { - Ok(DebugLocation { circuit_id: 0, opcode_location }) + Ok(DebugLocation { circuit_id: 0, opcode_location, brillig_function_id: None }) }), 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 }) + Ok(DebugLocation { circuit_id, opcode_location, brillig_function_id: None }) } else { error } @@ -276,12 +314,24 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { if ip >= self.get_opcodes().len() { None } else { - 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 }) + let (opcode_location, brillig_function_id) = + if let Some(ref solver) = self.brillig_solver { + let function_id = solver.function_id; + ( + OpcodeLocation::Brillig { + acir_index: ip, + brillig_index: solver.program_counter(), + }, + Some(function_id), + ) + } else { + (OpcodeLocation::Acir(ip), None) + }; + Some(DebugLocation { + circuit_id: self.current_circuit_id, + brillig_function_id, + opcode_location, + }) } } @@ -293,6 +343,7 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { .map(|ExecutionFrame { circuit_id, acvm }| DebugLocation { circuit_id: *circuit_id, opcode_location: OpcodeLocation::Acir(acvm.instruction_pointer()), + brillig_function_id: None, }) .collect(); @@ -306,11 +357,13 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { acir_index: instruction_pointer, brillig_index: *program_counter, }, + brillig_function_id: Some(solver.function_id), })); } else if instruction_pointer < self.get_opcodes().len() { frames.push(DebugLocation { circuit_id, opcode_location: OpcodeLocation::Acir(instruction_pointer), + brillig_function_id: None, }); } frames @@ -388,15 +441,24 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { ) -> Vec { self.debug_artifact.debug_symbols[debug_location.circuit_id as usize] .opcode_location(&debug_location.opcode_location) - .map(|source_locations| { - source_locations - .into_iter() - .filter(|source_location| { - !self.is_source_location_in_debug_module(source_location) - }) - .collect() + .unwrap_or_else(|| { + if let Some(brillig_function_id) = debug_location.brillig_function_id { + let brillig_locations = self.debug_artifact.debug_symbols + [debug_location.circuit_id as usize] + .brillig_locations + .get(&brillig_function_id); + brillig_locations + .unwrap() + .get(&debug_location.opcode_location) + .cloned() + .unwrap_or_default() + } else { + vec![] + } }) - .unwrap_or_default() + .into_iter() + .filter(|source_location| !self.is_source_location_in_debug_module(source_location)) + .collect() } /// Returns the current call stack with expanded source locations. In @@ -630,6 +692,7 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { Some(DebugLocation { circuit_id, opcode_location: OpcodeLocation::Acir(acir_index), + .. }) => { matches!( self.get_opcodes_of_circuit(circuit_id)[acir_index], @@ -821,24 +884,22 @@ fn build_source_to_opcode_debug_mappings( 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; + add_opcode_locations_map( + &debug_symbols.locations, + &mut result, + &simple_files, + circuit_id, + None, + ); - 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)); - }); + for (brillig_function_id, brillig_locations_map) in &debug_symbols.brillig_locations { + add_opcode_locations_map( + brillig_locations_map, + &mut result, + &simple_files, + circuit_id, + Some(*brillig_function_id), + ); } } result.iter_mut().for_each(|(_, file_locations)| file_locations.sort_by_key(|x| (x.0, x.1))); @@ -846,6 +907,35 @@ fn build_source_to_opcode_debug_mappings( result } +fn add_opcode_locations_map( + opcode_to_locations: &BTreeMap>, + source_to_locations: &mut BTreeMap>, + simple_files: &BTreeMap<&FileId, SimpleFile<&str, &str>>, + circuit_id: usize, + brillig_function_id: Option, +) { + for (opcode_location, source_locations) in opcode_to_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, + brillig_function_id, + }; + source_to_locations.entry(file_id).or_default().push((line_number, debug_location)); + }); + } +} + #[cfg(test)] mod tests { use super::*; @@ -928,7 +1018,11 @@ mod tests { assert_eq!( context.get_current_debug_location(), - Some(DebugLocation { circuit_id: 0, opcode_location: OpcodeLocation::Acir(0) }) + Some(DebugLocation { + circuit_id: 0, + opcode_location: OpcodeLocation::Acir(0), + brillig_function_id: None, + }) ); // Execute the first Brillig opcode (calldata copy) @@ -938,7 +1032,8 @@ mod tests { context.get_current_debug_location(), Some(DebugLocation { circuit_id: 0, - opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 1 } + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 1 }, + brillig_function_id: Some(BrilligFunctionId(0)), }) ); @@ -949,7 +1044,8 @@ mod tests { context.get_current_debug_location(), Some(DebugLocation { circuit_id: 0, - opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 2 } + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 2 }, + brillig_function_id: Some(BrilligFunctionId(0)), }) ); @@ -960,7 +1056,8 @@ mod tests { context.get_current_debug_location(), Some(DebugLocation { circuit_id: 0, - opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 2 } + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 2 }, + brillig_function_id: Some(BrilligFunctionId(0)), }) ); @@ -971,7 +1068,8 @@ mod tests { context.get_current_debug_location(), Some(DebugLocation { circuit_id: 0, - opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 3 } + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 3 }, + brillig_function_id: Some(BrilligFunctionId(0)), }) ); @@ -1056,6 +1154,7 @@ mod tests { let breakpoint_location = DebugLocation { circuit_id: 0, opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 1 }, + brillig_function_id: Some(BrilligFunctionId(0)), }; assert!(context.add_breakpoint(breakpoint_location)); @@ -1069,7 +1168,11 @@ mod tests { assert!(matches!(result, DebugCommandResult::Ok)); assert_eq!( context.get_current_debug_location(), - Some(DebugLocation { circuit_id: 0, opcode_location: OpcodeLocation::Acir(1) }) + Some(DebugLocation { + circuit_id: 0, + opcode_location: OpcodeLocation::Acir(1), + brillig_function_id: None + }) ); // last ACIR opcode @@ -1144,24 +1247,51 @@ mod tests { assert_eq!( locations, vec![ - 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 } + opcode_location: OpcodeLocation::Acir(0), + brillig_function_id: None + }), + Some(DebugLocation { + circuit_id: 0, + opcode_location: OpcodeLocation::Acir(1), + brillig_function_id: None + }), + Some(DebugLocation { + circuit_id: 0, + opcode_location: OpcodeLocation::Brillig { acir_index: 1, brillig_index: 1 }, + brillig_function_id: Some(BrilligFunctionId(0)), + }), + Some(DebugLocation { + circuit_id: 0, + opcode_location: OpcodeLocation::Acir(2), + brillig_function_id: None + }), + Some(DebugLocation { + circuit_id: 0, + opcode_location: OpcodeLocation::Acir(3), + brillig_function_id: None + }), + Some(DebugLocation { + circuit_id: 1, + opcode_location: OpcodeLocation::Acir(0), + brillig_function_id: None + }), + Some(DebugLocation { + circuit_id: 1, + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 1 }, + brillig_function_id: Some(BrilligFunctionId(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 } + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 2 }, + brillig_function_id: Some(BrilligFunctionId(1)), }), Some(DebugLocation { circuit_id: 1, - opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 2 } + opcode_location: OpcodeLocation::Acir(1), + brillig_function_id: None }), - Some(DebugLocation { circuit_id: 1, opcode_location: OpcodeLocation::Acir(1) }), ] ); @@ -1180,14 +1310,16 @@ mod tests { 1, context.debug_location_to_address(&DebugLocation { circuit_id: 0, - opcode_location: OpcodeLocation::Brillig { acir_index: 1, brillig_index: 0 } + opcode_location: OpcodeLocation::Brillig { acir_index: 1, brillig_index: 0 }, + brillig_function_id: Some(BrilligFunctionId(0)), }) ); assert_eq!( 5, context.debug_location_to_address(&DebugLocation { circuit_id: 1, - opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 0 } + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 0 }, + brillig_function_id: Some(BrilligFunctionId(1)), }) ); } diff --git a/tooling/debugger/src/repl.rs b/tooling/debugger/src/repl.rs index b4b2bff53be..1a7c2d6c7a8 100644 --- a/tooling/debugger/src/repl.rs +++ b/tooling/debugger/src/repl.rs @@ -1,7 +1,7 @@ use crate::context::{DebugCommandResult, DebugContext, DebugLocation}; use acvm::acir::brillig::{BitSize, IntegerBitSize}; -use acvm::acir::circuit::brillig::BrilligBytecode; +use acvm::acir::circuit::brillig::{BrilligBytecode, BrilligFunctionId}; use acvm::acir::circuit::{Circuit, Opcode, OpcodeLocation}; use acvm::acir::native_types::{Witness, WitnessMap, WitnessStack}; use acvm::brillig_vm::brillig::Opcode as BrilligOpcode; @@ -168,36 +168,41 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { } else if self.context.is_breakpoint_set(&DebugLocation { circuit_id, opcode_location: OpcodeLocation::Acir(acir_index), + brillig_function_id: None, }) { " *" } else { "" } }; - let brillig_marker = |acir_index, brillig_index| { + let brillig_marker = |acir_index, brillig_index, brillig_function_id| { if current_acir_index == Some(acir_index) && brillig_index == current_brillig_index { "->" } else if self.context.is_breakpoint_set(&DebugLocation { circuit_id, opcode_location: OpcodeLocation::Brillig { acir_index, brillig_index }, + brillig_function_id: Some(brillig_function_id), }) { " *" } else { "" } }; - let print_brillig_bytecode = |acir_index, bytecode: &[BrilligOpcode]| { - for (brillig_index, brillig_opcode) in bytecode.iter().enumerate() { - println!( - "{:>2}:{:>3}.{:<2} |{:2} {:?}", - circuit_id, - acir_index, - brillig_index, - brillig_marker(acir_index, brillig_index), - brillig_opcode - ); - } - }; + let print_brillig_bytecode = + |acir_index, + bytecode: &[BrilligOpcode], + brillig_function_id: BrilligFunctionId| { + for (brillig_index, brillig_opcode) in bytecode.iter().enumerate() { + println!( + "{:>2}:{:>3}.{:<2} |{:2} {:?}", + circuit_id, + acir_index, + brillig_index, + brillig_marker(acir_index, brillig_index, brillig_function_id), + brillig_opcode + ); + } + }; for (acir_index, opcode) in opcodes.iter().enumerate() { let marker = outer_marker(acir_index); match &opcode { @@ -208,7 +213,7 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { ); println!(" | outputs={:?}", outputs); let bytecode = &self.unconstrained_functions[id.as_usize()].bytecode; - print_brillig_bytecode(acir_index, bytecode); + print_brillig_bytecode(acir_index, bytecode, *id); } _ => println!("{:>2}:{:>3} {:2} {:?}", circuit_id, acir_index, marker, opcode), } diff --git a/tooling/noirc_artifacts/src/debug.rs b/tooling/noirc_artifacts/src/debug.rs index 8ae4156a5f6..8e2add70ae7 100644 --- a/tooling/noirc_artifacts/src/debug.rs +++ b/tooling/noirc_artifacts/src/debug.rs @@ -23,7 +23,7 @@ impl DebugArtifact { pub fn new(debug_symbols: Vec, file_manager: &FileManager) -> Self { let mut file_map = BTreeMap::new(); - let files_with_debug_symbols: BTreeSet = debug_symbols + let mut files_with_debug_symbols: BTreeSet = debug_symbols .iter() .flat_map(|function_symbols| { function_symbols @@ -33,6 +33,21 @@ impl DebugArtifact { }) .collect(); + let files_with_brillig_debug_symbols: BTreeSet = debug_symbols + .iter() + .flat_map(|function_symbols| { + let brillig_location_maps = + function_symbols.brillig_locations.values().flat_map(|brillig_location_map| { + brillig_location_map + .values() + .flat_map(|call_stack| call_stack.iter().map(|location| location.file)) + }); + brillig_location_maps + }) + .collect(); + + files_with_debug_symbols.extend(files_with_brillig_debug_symbols); + for file_id in files_with_debug_symbols { let file_path = file_manager.path(file_id).expect("file should exist"); let file_source = file_manager.fetch_file(file_id).expect("file should exist");