From 67ccaae7389c0cb6a8ea76a8d29600ef4722fcc2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Fri, 19 Jan 2024 15:55:49 -0500 Subject: [PATCH 01/19] feat: Implement debug instrumentation to track variable values Changes partially imported from manastech/dap-with-vars branch. This changes the compiler to inject instrumentation code in order to track variables and their values at runtime for a debugger to consume. This tracking is done via oracles/foreign functions calls which impact a debugger runtime data structure. Co-authored-by: synthia --- Cargo.lock | 2 + acvm-repo/acir_field/src/generic_ark.rs | 10 + compiler/fm/src/lib.rs | 15 + compiler/noirc_driver/src/lib.rs | 40 +- compiler/noirc_errors/Cargo.toml | 1 + compiler/noirc_errors/src/debug_info.rs | 26 +- compiler/noirc_errors/src/position.rs | 4 + .../src/brillig/brillig_gen/brillig_block.rs | 14 +- .../brillig_gen/brillig_block_variables.rs | 8 +- compiler/noirc_evaluator/src/ssa.rs | 23 +- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 10 +- .../noirc_evaluator/src/ssa/ssa_gen/mod.rs | 11 +- compiler/noirc_frontend/src/debug/mod.rs | 610 ++++++++++++++++++ compiler/noirc_frontend/src/hir/mod.rs | 38 +- compiler/noirc_frontend/src/lib.rs | 1 + .../src/monomorphization/ast.rs | 6 +- .../src/monomorphization/debug_types.rs | 70 ++ .../src/monomorphization/mod.rs | 222 ++++++- compiler/noirc_frontend/src/tests.rs | 4 +- compiler/noirc_printable_type/src/lib.rs | 5 +- tooling/debugger/Cargo.toml | 3 +- tooling/debugger/src/context.rs | 62 +- tooling/debugger/src/dap.rs | 4 +- tooling/debugger/src/foreign_calls.rs | 156 +++++ tooling/debugger/src/lib.rs | 1 + tooling/debugger/src/repl.rs | 32 +- tooling/debugger/src/source_code_printer.rs | 2 +- tooling/lsp/src/requests/test_run.rs | 9 +- tooling/nargo/src/artifacts/debug.rs | 30 +- tooling/nargo/src/artifacts/debug_vars.rs | 132 ++++ tooling/nargo/src/artifacts/mod.rs | 1 + tooling/nargo/src/lib.rs | 1 + tooling/nargo/src/ops/foreign_calls.rs | 2 +- tooling/nargo/src/ops/mod.rs | 2 +- tooling/nargo/src/ops/test.rs | 2 +- tooling/nargo_cli/src/cli/debug_cmd.rs | 4 + tooling/nargo_cli/src/cli/export_cmd.rs | 2 +- tooling/nargo_cli/src/cli/test_cmd.rs | 2 +- 38 files changed, 1474 insertions(+), 93 deletions(-) create mode 100644 compiler/noirc_frontend/src/debug/mod.rs create mode 100644 compiler/noirc_frontend/src/monomorphization/debug_types.rs create mode 100644 tooling/debugger/src/foreign_calls.rs create mode 100644 tooling/nargo/src/artifacts/debug_vars.rs diff --git a/Cargo.lock b/Cargo.lock index 93f1d25fc76..8687fe07aea 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2824,6 +2824,7 @@ dependencies = [ "nargo", "noirc_driver", "noirc_errors", + "noirc_frontend", "noirc_printable_type", "owo-colors", "rexpect", @@ -2947,6 +2948,7 @@ dependencies = [ "codespan-reporting", "flate2", "fm", + "noirc_printable_type", "serde", "serde_json", "serde_with", diff --git a/acvm-repo/acir_field/src/generic_ark.rs b/acvm-repo/acir_field/src/generic_ark.rs index 542e291982b..dc54d271beb 100644 --- a/acvm-repo/acir_field/src/generic_ark.rs +++ b/acvm-repo/acir_field/src/generic_ark.rs @@ -175,6 +175,10 @@ impl FieldElement { self == &Self::one() } + pub fn is_negative(&self) -> bool { + self.neg().num_bits() < self.num_bits() + } + pub fn pow(&self, exponent: &Self) -> Self { FieldElement(self.0.pow(exponent.0.into_bigint())) } @@ -240,6 +244,12 @@ impl FieldElement { self.fits_in_u128().then(|| self.to_u128()) } + pub fn to_i128(self) -> i128 { + let is_negative = self.is_negative(); + let bytes = if is_negative { self.neg() } else { self }.to_be_bytes(); + i128::from_be_bytes(bytes[16..32].try_into().unwrap()) * if is_negative { -1 } else { 1 } + } + pub fn try_to_u64(&self) -> Option { (self.num_bits() <= 64).then(|| self.to_u128() as u64) } diff --git a/compiler/fm/src/lib.rs b/compiler/fm/src/lib.rs index bb080c62c0e..e7da3870d9c 100644 --- a/compiler/fm/src/lib.rs +++ b/compiler/fm/src/lib.rs @@ -78,6 +78,21 @@ impl FileManager { Some(file_id) } + pub fn add_file_with_contents(&mut self, file_name: &Path, contents: &str) -> Option { + // Handle both relative file paths and std/lib virtual paths. + let resolved_path: PathBuf = file_name.to_path_buf(); + + // Check that the resolved path already exists in the file map, if it is, we return it. + if let Some(file_id) = self.path_to_id.get(&resolved_path) { + return Some(*file_id); + } + + // Otherwise we add the file + let file_id = self.file_map.add_file(resolved_path.clone().into(), String::from(contents)); + self.register_path(file_id, resolved_path); + Some(file_id) + } + fn register_path(&mut self, file_id: FileId, path: PathBuf) { let old_value = self.id_to_path.insert(file_id, path.clone()); assert!( diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index a2570b26f3e..1d0e4d4ef66 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -10,11 +10,12 @@ use noirc_abi::{AbiParameter, AbiType, ContractEvent}; use noirc_errors::{CustomDiagnostic, FileDiagnostic}; use noirc_evaluator::create_circuit; use noirc_evaluator::errors::RuntimeError; +use noirc_frontend::debug::create_prologue_program; use noirc_frontend::graph::{CrateId, CrateName}; use noirc_frontend::hir::def_map::{Contract, CrateDefMap}; use noirc_frontend::hir::Context; use noirc_frontend::macros_api::MacroProcessor; -use noirc_frontend::monomorphization::monomorphize; +use noirc_frontend::monomorphization::{monomorphize, monomorphize_debug}; use noirc_frontend::node_interner::FuncId; use serde::{Deserialize, Serialize}; use std::path::Path; @@ -33,6 +34,7 @@ pub use debug::DebugFile; pub use program::CompiledProgram; const STD_CRATE_NAME: &str = "std"; +const DEBUG_CRATE_NAME: &str = "__debug"; pub const GIT_COMMIT: &str = env!("GIT_COMMIT"); pub const GIT_DIRTY: &str = env!("GIT_DIRTY"); @@ -79,6 +81,14 @@ pub struct CompileOptions { /// Outputs the monomorphized IR to stdout for debugging #[arg(long, hide = true)] pub show_monomorphized: bool, + + /// Insert debug symbols to inspect variables + #[arg(long)] + pub instrument_debug: bool, + + /// Force Brillig output (for step debugging) + #[arg(long, hide = true)] + pub force_brillig: bool, } /// Helper type used to signify where only warnings are expected in file diagnostics @@ -113,6 +123,10 @@ fn add_stdlib_source_to_file_manager(file_manager: &mut FileManager) { for (path, source) in stdlib_paths_with_source { file_manager.add_file_with_source_canonical_path(Path::new(&path), source); } + + // Adds the synthetic debug module for instrumentation into the file manager + let path_to_debug_lib_file = Path::new(DEBUG_CRATE_NAME).join("lib.nr"); + file_manager.add_file_with_contents(&path_to_debug_lib_file, &create_prologue_program(8)); } /// Adds the file from the file system at `Path` to the crate graph as a root file @@ -127,11 +141,19 @@ pub fn prepare_crate(context: &mut Context, file_name: &Path) -> CrateId { .expect("stdlib file id is expected to be present"); let std_crate_id = context.crate_graph.add_stdlib(std_file_id); + let path_to_debug_lib_file = Path::new(DEBUG_CRATE_NAME).join("lib.nr"); + let debug_file_id = context + .file_manager + .name_to_id(path_to_debug_lib_file) + .expect("debug module is expected to be present"); + let debug_crate_id = context.crate_graph.add_crate(debug_file_id); + let root_file_id = context.file_manager.name_to_id(file_name.to_path_buf()).unwrap_or_else(|| panic!("files are expected to be added to the FileManager before reaching the compiler file_path: {file_name:?}")); let root_crate_id = context.crate_graph.add_crate_root(root_file_id); add_dep(context, root_crate_id, std_crate_id, STD_CRATE_NAME.parse().unwrap()); + add_dep(context, root_crate_id, debug_crate_id, DEBUG_CRATE_NAME.parse().unwrap()); root_crate_id } @@ -309,7 +331,7 @@ fn has_errors(errors: &[FileDiagnostic], deny_warnings: bool) -> bool { /// Compile all of the functions associated with a Noir contract. fn compile_contract_inner( - context: &Context, + context: &mut Context, contract: Contract, options: &CompileOptions, ) -> Result { @@ -385,13 +407,21 @@ fn compile_contract_inner( /// This function assumes [`check_crate`] is called beforehand. #[tracing::instrument(level = "trace", skip_all, fields(function_name = context.function_name(&main_function)))] pub fn compile_no_check( - context: &Context, + context: &mut Context, options: &CompileOptions, main_function: FuncId, cached_program: Option, force_compile: bool, ) -> Result { - let program = monomorphize(main_function, &context.def_interner); + let program = if options.instrument_debug { + monomorphize_debug( + main_function, + &mut context.def_interner, + &context.debug_state.field_names, + ) + } else { + monomorphize(main_function, &mut context.def_interner) + }; let hash = fxhash::hash64(&program); let hashes_match = cached_program.as_ref().map_or(false, |program| program.hash == hash); @@ -410,7 +440,7 @@ pub fn compile_no_check( } let visibility = program.return_visibility; let (circuit, debug, input_witnesses, return_witnesses, warnings) = - create_circuit(program, options.show_ssa, options.show_brillig)?; + create_circuit(program, options.show_ssa, options.show_brillig, options.force_brillig)?; let abi = abi_gen::gen_abi(context, &main_function, input_witnesses, return_witnesses, visibility); diff --git a/compiler/noirc_errors/Cargo.toml b/compiler/noirc_errors/Cargo.toml index 935137ba2fc..da18399971e 100644 --- a/compiler/noirc_errors/Cargo.toml +++ b/compiler/noirc_errors/Cargo.toml @@ -13,6 +13,7 @@ codespan-reporting.workspace = true codespan.workspace = true fm.workspace = true chumsky.workspace = true +noirc_printable_type.workspace = true serde.workspace = true serde_with = "3.2.0" tracing.workspace = true diff --git a/compiler/noirc_errors/src/debug_info.rs b/compiler/noirc_errors/src/debug_info.rs index ffca8fbf2e1..63e0a645dc9 100644 --- a/compiler/noirc_errors/src/debug_info.rs +++ b/compiler/noirc_errors/src/debug_info.rs @@ -1,5 +1,6 @@ use acvm::acir::circuit::OpcodeLocation; use acvm::compiler::AcirTransformationMap; +use fm::FileId; use base64::Engine; use flate2::read::DeflateDecoder; @@ -16,10 +17,15 @@ use std::io::Write; use std::mem; use crate::Location; +use noirc_printable_type::PrintableType; use serde::{ de::Error as DeserializationError, ser::Error as SerializationError, Deserialize, Serialize, }; +pub type Variables = Vec<(u32, (String, u32))>; +pub type Types = Vec<(u32, PrintableType)>; +pub type VariableTypes = (Variables, Types); + #[serde_as] #[derive(Default, Debug, Clone, Deserialize, Serialize)] pub struct DebugInfo { @@ -28,6 +34,8 @@ pub struct DebugInfo { /// that they should be serialized to/from strings. #[serde_as(as = "BTreeMap")] pub locations: BTreeMap>, + pub variables: HashMap, // var_id => (name, type_id) + pub types: HashMap, // type_id => printable type } /// Holds OpCodes Counts for Acir and Brillig Opcodes @@ -39,8 +47,15 @@ pub struct OpCodesCount { } impl DebugInfo { - pub fn new(locations: BTreeMap>) -> Self { - DebugInfo { locations } + pub fn new( + locations: BTreeMap>, + var_types: VariableTypes, + ) -> Self { + Self { + locations, + variables: var_types.0.into_iter().collect(), + types: var_types.1.into_iter().collect(), + } } /// Updates the locations map when the [`Circuit`][acvm::acir::circuit::Circuit] is modified. @@ -97,6 +112,13 @@ impl DebugInfo { counted_opcodes } + pub fn get_file_ids(&self) -> Vec { + self.locations + .values() + .filter_map(|call_stack| call_stack.last().map(|location| location.file)) + .collect() + } + pub fn serialize_compressed_base64_json( debug_info: &DebugInfo, s: S, diff --git a/compiler/noirc_errors/src/position.rs b/compiler/noirc_errors/src/position.rs index 007ec58ca27..c5eacefe9a9 100644 --- a/compiler/noirc_errors/src/position.rs +++ b/compiler/noirc_errors/src/position.rs @@ -86,6 +86,10 @@ impl Span { self.0.end().into() } + pub fn build_from_str(s: &str) -> Span { + Span(ByteSpan::from_str(s)) + } + pub fn contains(&self, other: &Span) -> bool { self.start() <= other.start() && self.end() >= other.end() } diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index db005d9d438..0bb749be1b5 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -1194,7 +1194,19 @@ impl<'block> BrilligBlock<'block> { new_variable } } - Value::Function(_) | Value::Intrinsic(_) | Value::ForeignFunction(_) => { + Value::Function(_) => { + // For the debugger instrumentation we want to allow passing + // around values representing function pointers, even though + // there is no interaction with the function possible given that + // value. + let new_variable = + self.variables.allocate_constant(self.brillig_context, value_id, dfg); + let register_index = new_variable.extract_register(); + + self.brillig_context.const_instruction(register_index, value_id.to_usize().into()); + new_variable + } + Value::Intrinsic(_) | Value::ForeignFunction(_) => { todo!("ICE: Cannot convert value {value:?}") } } diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs index f2e698c0aa9..64e8bb1acf4 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs @@ -173,7 +173,10 @@ pub(crate) fn allocate_value( let typ = dfg.type_of_value(value_id); match typ { - Type::Numeric(_) | Type::Reference(_) => { + Type::Numeric(_) | Type::Reference(_) | Type::Function => { + // NB. function references are converted to a constant when + // translating from SSA to Brillig (to allow for debugger + // instrumentation to work properly) let register = brillig_context.allocate_register(); BrilligVariable::Simple(register) } @@ -199,8 +202,5 @@ pub(crate) fn allocate_value( rc: rc_register, }) } - Type::Function => { - unreachable!("ICE: Function values should have been removed from the SSA") - } } } diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index e2da5652faf..1d746365c51 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -40,12 +40,13 @@ pub(crate) fn optimize_into_acir( program: Program, print_ssa_passes: bool, print_brillig_trace: bool, + force_brillig_output: bool, ) -> Result { let abi_distinctness = program.return_distinctness; let ssa_gen_span = span!(Level::TRACE, "ssa_generation"); let ssa_gen_span_guard = ssa_gen_span.enter(); - let ssa = SsaBuilder::new(program, print_ssa_passes)? + let ssa = SsaBuilder::new(program, print_ssa_passes, force_brillig_output)? .run_pass(Ssa::defunctionalize, "After Defunctionalization:") .run_pass(Ssa::inline_functions, "After Inlining:") // Run mem2reg with the CFG separated into blocks @@ -84,10 +85,16 @@ pub fn create_circuit( program: Program, enable_ssa_logging: bool, enable_brillig_logging: bool, + force_brillig_output: bool, ) -> Result<(Circuit, DebugInfo, Vec, Vec, Vec), RuntimeError> { + let debug_var_types = program.debug_var_types.clone(); let func_sig = program.main_function_signature.clone(); - let mut generated_acir = - optimize_into_acir(program, enable_ssa_logging, enable_brillig_logging)?; + let mut generated_acir = optimize_into_acir( + program, + enable_ssa_logging, + enable_brillig_logging, + force_brillig_output, + )?; let opcodes = generated_acir.take_opcodes(); let current_witness_index = generated_acir.current_witness_index().0; let GeneratedAcir { @@ -120,7 +127,7 @@ pub fn create_circuit( .map(|(index, locations)| (index, locations.into_iter().collect())) .collect(); - let mut debug_info = DebugInfo::new(locations); + let mut debug_info = DebugInfo::new(locations, debug_var_types); // Perform any ACIR-level optimizations let (optimized_circuit, transformation_map) = acvm::compiler::optimize(circuit); @@ -170,8 +177,12 @@ struct SsaBuilder { } impl SsaBuilder { - fn new(program: Program, print_ssa_passes: bool) -> Result { - let ssa = ssa_gen::generate_ssa(program)?; + fn new( + program: Program, + print_ssa_passes: bool, + force_brillig_runtime: bool, + ) -> Result { + let ssa = ssa_gen::generate_ssa(program, force_brillig_runtime)?; Ok(SsaBuilder { print_ssa_passes, ssa }.print("Initial SSA:")) } diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index d832b8d0fbb..eca25905583 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -1328,7 +1328,15 @@ impl Context { AcirValue::Array(elements.collect()) } Value::Intrinsic(..) => todo!(), - Value::Function(..) => unreachable!("ICE: All functions should have been inlined"), + Value::Function(function_id) => { + // This conversion is for debugging support only, to allow the + // debugging instrumentation code to work. Taking the reference + // of a function in ACIR is useless. + AcirValue::Var( + self.acir_context.add_constant(function_id.to_usize()), + AcirType::field(), + ) + } Value::ForeignFunction(_) => unimplemented!( "Oracle calls directly in constrained functions are not yet available." ), diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 9d9635fed34..782a10be036 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -38,7 +38,10 @@ use super::{ /// Generates SSA for the given monomorphized program. /// /// This function will generate the SSA but does not perform any optimizations on it. -pub(crate) fn generate_ssa(program: Program) -> Result { +pub(crate) fn generate_ssa( + program: Program, + force_brillig_runtime: bool, +) -> Result { // see which parameter has call_data/return_data attribute let is_databus = DataBusBuilder::is_databus(&program.main_function_signature); @@ -56,7 +59,11 @@ pub(crate) fn generate_ssa(program: Program) -> Result { let mut function_context = FunctionContext::new( main.name.clone(), &main.parameters, - if main.unconstrained { RuntimeType::Brillig } else { RuntimeType::Acir }, + if force_brillig_runtime || main.unconstrained { + RuntimeType::Brillig + } else { + RuntimeType::Acir + }, &context, ); diff --git a/compiler/noirc_frontend/src/debug/mod.rs b/compiler/noirc_frontend/src/debug/mod.rs new file mode 100644 index 00000000000..940bd4138b4 --- /dev/null +++ b/compiler/noirc_frontend/src/debug/mod.rs @@ -0,0 +1,610 @@ +use crate::parser::{parse_program, ParsedModule}; +use crate::{ + ast, + ast::{Path, PathKind}, + parser::{Item, ItemKind}, +}; +use noirc_errors::{Span, Spanned}; +use std::collections::HashMap; +use std::collections::VecDeque; + +#[derive(Debug, Clone)] +pub struct DebugState { + pub variables: HashMap, // var_id => var_name + pub field_names: HashMap, + next_var_id: u32, + next_field_name_id: u32, + scope: Vec>, // var_name => var_id + pub enabled: bool, +} + +impl Default for DebugState { + fn default() -> Self { + Self { + variables: HashMap::default(), + field_names: HashMap::default(), + scope: vec![], + next_var_id: 0, + next_field_name_id: 1, + enabled: true, // TODO + } + } +} + +impl DebugState { + fn insert_var(&mut self, var_name: &str) -> u32 { + let var_id = self.next_var_id; + self.next_var_id += 1; + self.variables.insert(var_id, var_name.to_string()); + self.scope.last_mut().unwrap().insert(var_name.to_string(), var_id); + var_id + } + + fn lookup_var(&self, var_name: &str) -> Option { + self.scope.iter().rev().find_map(|vars| vars.get(var_name).copied()) + } + + fn insert_field_name(&mut self, field_name: &str) -> u32 { + let field_name_id = self.next_field_name_id; + self.next_field_name_id += 1; + self.field_names.insert(field_name_id, field_name.to_string()); + field_name_id + } + + fn walk_fn(&mut self, f: &mut ast::FunctionDefinition) { + self.scope.push(HashMap::default()); + + let pvars: Vec<(u32, ast::Ident, bool)> = f + .parameters + .iter() + .flat_map(|param| { + pattern_vars(¶m.pattern) + .iter() + .map(|(id, is_mut)| (self.insert_var(&id.0.contents), id.clone(), *is_mut)) + .collect::>() + }) + .collect(); + + let set_fn_params = pvars + .iter() + .map(|(var_id, id, _is_mut)| self.wrap_assign_var(*var_id, id_expr(id))) + .collect(); + + self.walk_scope(&mut f.body.0); + + // prapend fn params: + f.body.0 = vec![set_fn_params, f.body.0.clone()].concat(); + } + + // Modify a vector of statements in-place, adding instrumentation for sets and drops. + // This function will consume a scope level. + fn walk_scope(&mut self, statements: &mut Vec) { + let end_span = statements + .last() + .map_or(none_span(), |statement| Span::empty(statement.span.end() + 1)); + + statements.iter_mut().for_each(|stmt| self.walk_statement(stmt)); + + let (ret_stmt, fn_body) = + statements.split_last().map(|(e, b)| (e.clone(), b.to_vec())).unwrap_or(( + ast::Statement { + kind: ast::StatementKind::Expression(ast::Expression { + kind: ast::ExpressionKind::Literal(ast::Literal::Unit), + span: none_span(), + }), + span: none_span(), + }, + vec![], + )); + + *statements = vec![ + // copy body minus the return expr: + fn_body, + // assign return expr to __debug_expr: + vec![match &ret_stmt.kind { + ast::StatementKind::Expression(ret_expr) => ast::Statement { + kind: ast::StatementKind::Let(ast::LetStatement { + pattern: ast::Pattern::Identifier(ident("__debug_expr", none_span())), + r#type: ast::UnresolvedType::unspecified(), + expression: ret_expr.clone(), + }), + span: none_span(), + }, + _ => ret_stmt.clone(), + }], + // drop fn params: + self.scope + .pop() + .unwrap_or(HashMap::default()) + .values() + .map(|var_id| self.wrap_drop_var(*var_id, end_span)) + .collect(), + // return the __debug_expr value: + vec![match &ret_stmt.kind { + ast::StatementKind::Expression(_ret_expr) => ast::Statement { + kind: ast::StatementKind::Expression(ast::Expression { + kind: ast::ExpressionKind::Variable(ast::Path { + segments: vec![ident("__debug_expr", none_span())], + kind: PathKind::Plain, + span: none_span(), + }), + span: none_span(), + }), + span: none_span(), + }, + _ => ast::Statement { + kind: ast::StatementKind::Expression(ast::Expression { + kind: ast::ExpressionKind::Literal(ast::Literal::Unit), + span: none_span(), + }), + span: none_span(), + }, + }], + ] + .concat(); + } + + pub fn insert_symbols(&mut self, module: &mut ParsedModule) { + if !self.enabled { + return; + } + module.items.iter_mut().for_each(|item| { + if let Item { kind: ItemKind::Function(f), .. } = item { + self.walk_fn(&mut f.def); + } + }); + // this part absolutely must happen after ast traversal above + // so that oracle functions don't get wrapped, resulting in infinite recursion: + self.insert_state_set_oracle(module, 8); + } + + fn wrap_assign_var(&mut self, var_id: u32, expr: ast::Expression) -> ast::Statement { + let span = expr.span; + let kind = ast::ExpressionKind::Call(Box::new(ast::CallExpression { + func: Box::new(ast::Expression { + kind: ast::ExpressionKind::Variable(ast::Path { + segments: vec![ident("__debug_var_assign", span)], + kind: PathKind::Plain, + span, + }), + span, + }), + arguments: vec![uint_expr(var_id as u128, span), expr], + })); + ast::Statement { kind: ast::StatementKind::Semi(ast::Expression { kind, span }), span } + } + + fn wrap_drop_var(&mut self, var_id: u32, span: Span) -> ast::Statement { + let kind = ast::ExpressionKind::Call(Box::new(ast::CallExpression { + func: Box::new(ast::Expression { + kind: ast::ExpressionKind::Variable(ast::Path { + segments: vec![ident("__debug_var_drop", span)], + kind: PathKind::Plain, + span, + }), + span, + }), + arguments: vec![uint_expr(var_id as u128, span)], + })); + ast::Statement { kind: ast::StatementKind::Semi(ast::Expression { kind, span }), span } + } + + fn wrap_assign_member( + &mut self, + var_id: u32, + indexes: &[ast::Expression], + expr: &ast::Expression, + ) -> ast::Statement { + let arity = indexes.len(); + let kind = ast::ExpressionKind::Call(Box::new(ast::CallExpression { + func: Box::new(ast::Expression { + kind: ast::ExpressionKind::Variable(ast::Path { + segments: vec![ident(&format!["__debug_member_assign_{arity}"], none_span())], + kind: PathKind::Plain, + span: none_span(), + }), + span: none_span(), + }), + arguments: [ + vec![uint_expr(var_id as u128, none_span())], + vec![expr.clone()], + indexes.iter().rev().cloned().collect(), + ] + .concat(), + })); + ast::Statement { + kind: ast::StatementKind::Semi(ast::Expression { kind, span: none_span() }), + span: none_span(), + } + } + + fn wrap_let_statement(&mut self, let_stmt: &ast::LetStatement, span: &Span) -> ast::Statement { + // rewrites let statements written like this: + // let (((a,b,c),D { d }),e,f) = x; + // + // into statements like this: + // + // let (a,b,c,d,e,f,g) = { + // let (((a,b,c),D { d }),e,f) = x; + // wrap(1, a); + // wrap(2, b); + // ... + // wrap(6, f); + // (a,b,c,d,e,f,g) + // }; + + // a.b.c[3].x[i*4+1].z + + let vars = pattern_vars(&let_stmt.pattern); + let vars_pattern: Vec = vars + .iter() + .map(|(id, is_mut)| { + if *is_mut { + ast::Pattern::Mutable(Box::new(ast::Pattern::Identifier(id.clone())), id.span()) + } else { + ast::Pattern::Identifier(id.clone()) + } + }) + .collect(); + let vars_exprs: Vec = vars.iter().map(|(id, _)| id_expr(id)).collect(); + + let mut block_stmts = + vec![ast::Statement { kind: ast::StatementKind::Let(let_stmt.clone()), span: *span }]; + block_stmts.extend(vars.iter().map(|(id, _)| { + let var_id = self.insert_var(&id.0.contents); + self.wrap_assign_var(var_id, id_expr(id)) + })); + block_stmts.push(ast::Statement { + kind: ast::StatementKind::Expression(ast::Expression { + kind: ast::ExpressionKind::Tuple(vars_exprs), + span: let_stmt.pattern.span(), + }), + span: let_stmt.pattern.span(), + }); + + ast::Statement { + kind: ast::StatementKind::Let(ast::LetStatement { + pattern: ast::Pattern::Tuple(vars_pattern, let_stmt.pattern.span()), + r#type: ast::UnresolvedType::unspecified(), + expression: ast::Expression { + kind: ast::ExpressionKind::Block(ast::BlockExpression(block_stmts)), + span: let_stmt.expression.span, + }, + }), + span: *span, + } + } + + fn wrap_assign_statement( + &mut self, + assign_stmt: &ast::AssignStatement, + span: &Span, + ) -> ast::Statement { + // X = Y becomes: + // X = { + // let __debug_expr = Y; + // + // __debug_var_assign(17, __debug_expr); + // // or: + // __debug_member_assign_{arity}(17, __debug_expr, _v0, _v1..., _v{arity}); + // + // __debug_expr + // }; + + let let_kind = ast::StatementKind::Let(ast::LetStatement { + pattern: ast::Pattern::Identifier(ident("__debug_expr", assign_stmt.expression.span)), + r#type: ast::UnresolvedType::unspecified(), + expression: assign_stmt.expression.clone(), + }); + let expression_span = assign_stmt.expression.span; + let new_assign_stmt = match &assign_stmt.lvalue { + ast::LValue::Ident(id) => { + let var_id = self + .lookup_var(&id.0.contents) + .unwrap_or_else(|| panic!("var lookup failed for var_name={}", &id.0.contents)); + self.wrap_assign_var(var_id, id_expr(&ident("__debug_expr", id.span()))) + } + ast::LValue::Dereference(_lv) => { + // TODO: this is a dummy statement for now, but we should + // somehow track the derefence and update the pointed to + // variable + ast::Statement { + kind: ast::StatementKind::Expression(uint_expr(0, *span)), + span: *span, + } + } + _ => { + let mut indexes = vec![]; + let mut cursor = &assign_stmt.lvalue; + let var_id; + loop { + match cursor { + ast::LValue::Ident(id) => { + var_id = self.lookup_var(&id.0.contents).unwrap_or_else(|| { + panic!("var lookup failed for var_name={}", &id.0.contents) + }); + break; + } + ast::LValue::MemberAccess { object, field_name } => { + cursor = object; + let field_name_id = self.insert_field_name(&field_name.0.contents); + indexes.push(sint_expr(-(field_name_id as i128))); + } + ast::LValue::Index { index, array } => { + cursor = array; + indexes.push(index.clone()); + } + ast::LValue::Dereference(_ref) => { + unimplemented![] + } + } + } + self.wrap_assign_member( + var_id, + &indexes, + &id_expr(&ident("__debug_expr", expression_span)), + ) + } + }; + let ret_kind = + ast::StatementKind::Expression(id_expr(&ident("__debug_expr", expression_span))); + + ast::Statement { + kind: ast::StatementKind::Assign(ast::AssignStatement { + lvalue: assign_stmt.lvalue.clone(), + expression: ast::Expression { + kind: ast::ExpressionKind::Block(ast::BlockExpression(vec![ + ast::Statement { kind: let_kind, span: expression_span }, + new_assign_stmt, + ast::Statement { kind: ret_kind, span: expression_span }, + ])), + span: expression_span, + }, + }), + span: *span, + } + } + + fn walk_expr(&mut self, expr: &mut ast::Expression) { + match &mut expr.kind { + ast::ExpressionKind::Block(ast::BlockExpression(ref mut statements)) => { + self.scope.push(HashMap::default()); + self.walk_scope(statements); + } + ast::ExpressionKind::Prefix(prefix_expr) => { + self.walk_expr(&mut prefix_expr.rhs); + } + ast::ExpressionKind::Index(index_expr) => { + self.walk_expr(&mut index_expr.collection); + self.walk_expr(&mut index_expr.index); + } + ast::ExpressionKind::Call(call_expr) => { + // TODO: push a stack frame or something here? + self.walk_expr(&mut call_expr.func); + call_expr.arguments.iter_mut().for_each(|ref mut expr| { + self.walk_expr(expr); + }); + } + ast::ExpressionKind::MethodCall(mc_expr) => { + // TODO: also push a stack frame here + self.walk_expr(&mut mc_expr.object); + mc_expr.arguments.iter_mut().for_each(|ref mut expr| { + self.walk_expr(expr); + }); + } + ast::ExpressionKind::Constructor(c_expr) => { + c_expr.fields.iter_mut().for_each(|(_id, ref mut expr)| { + self.walk_expr(expr); + }); + } + ast::ExpressionKind::MemberAccess(ma_expr) => { + self.walk_expr(&mut ma_expr.lhs); + } + ast::ExpressionKind::Cast(cast_expr) => { + self.walk_expr(&mut cast_expr.lhs); + } + ast::ExpressionKind::Infix(infix_expr) => { + self.walk_expr(&mut infix_expr.lhs); + self.walk_expr(&mut infix_expr.rhs); + } + ast::ExpressionKind::If(if_expr) => { + self.walk_expr(&mut if_expr.condition); + self.walk_expr(&mut if_expr.consequence); + if let Some(ref mut alt) = if_expr.alternative { + self.walk_expr(alt); + } + } + ast::ExpressionKind::Tuple(exprs) => { + exprs.iter_mut().for_each(|ref mut expr| { + self.walk_expr(expr); + }); + } + ast::ExpressionKind::Lambda(lambda) => { + self.walk_expr(&mut lambda.body); + } + ast::ExpressionKind::Parenthesized(expr) => { + self.walk_expr(expr); + } + _ => {} + } + } + + fn walk_for(&mut self, for_stmt: &mut ast::ForLoopStatement) { + let var_name = &for_stmt.identifier.0.contents; + let var_id = self.insert_var(var_name); + + let set_stmt = self.wrap_assign_var(var_id, id_expr(&for_stmt.identifier)); + let drop_stmt = self.wrap_drop_var(var_id, Span::empty(for_stmt.span.end() + 1)); + + self.walk_expr(&mut for_stmt.block); + for_stmt.block = ast::Expression { + kind: ast::ExpressionKind::Block(ast::BlockExpression(vec![ + set_stmt, + ast::Statement { + kind: ast::StatementKind::Semi(for_stmt.block.clone()), + span: none_span(), + }, + drop_stmt, + ])), + span: none_span(), + }; + } + + fn walk_statement(&mut self, stmt: &mut ast::Statement) { + match &mut stmt.kind { + ast::StatementKind::Let(let_stmt) => { + *stmt = self.wrap_let_statement(let_stmt, &stmt.span); + } + ast::StatementKind::Assign(assign_stmt) => { + *stmt = self.wrap_assign_statement(assign_stmt, &stmt.span); + } + ast::StatementKind::Expression(expr) => { + self.walk_expr(expr); + } + ast::StatementKind::Semi(expr) => { + self.walk_expr(expr); + } + ast::StatementKind::For(ref mut for_stmt) => { + self.walk_for(for_stmt); + } + _ => {} // Constrain, Error + } + } + + fn insert_state_set_oracle(&self, module: &mut ParsedModule, n: u32) { + let member_assigns = (1..=n) + .map(|i| format!["__debug_member_assign_{i}"]) + .collect::>() + .join(",\n"); + let (program, errors) = parse_program(&format!( + r#" + use dep::__debug::{{ + __debug_var_assign, + __debug_var_drop, + __debug_dereference_assign, + {member_assigns}, + }};"# + )); + if !errors.is_empty() { + panic!("errors parsing internal oracle definitions: {errors:?}") + } + module.items.extend(program.items); + } +} + +pub fn create_prologue_program(n: u32) -> String { + [ + r#" + #[oracle(__debug_var_assign)] + unconstrained fn __debug_var_assign_oracle(_var_id: u32, _value: T) {} + unconstrained fn __debug_var_assign_inner(var_id: u32, value: T) { + __debug_var_assign_oracle(var_id, value); + } + pub fn __debug_var_assign(var_id: u32, value: T) { + __debug_var_assign_inner(var_id, value); + } + + #[oracle(__debug_var_drop)] + unconstrained fn __debug_var_drop_oracle(_var_id: u32) {} + unconstrained fn __debug_var_drop_inner(var_id: u32) { + __debug_var_drop_oracle(var_id); + } + pub fn __debug_var_drop(var_id: u32) { + __debug_var_drop_inner(var_id); + } + + #[oracle(__debug_dereference_assign)] + unconstrained fn __debug_dereference_assign_oracle(_var_id: u32, _value: T) {} + unconstrained fn __debug_dereference_assign_inner(var_id: u32, value: T) { + __debug_dereference_assign_oracle(var_id, value); + } + pub fn __debug_dereference_assign(var_id: u32, value: T) { + __debug_dereference_assign_inner(var_id, value); + } + "# + .to_string(), + (1..=n) + .map(|n| { + let var_sig = + (0..n).map(|i| format!["_v{i}: Field"]).collect::>().join(", "); + let vars = (0..n).map(|i| format!["_v{i}"]).collect::>().join(", "); + format!( + r#" + #[oracle(__debug_member_assign_{n})] + unconstrained fn __debug_member_assign_oracle_{n}( + _var_id: u32, _value: T, {var_sig} + ) {{}} + unconstrained fn __debug_member_assign_inner_{n}( + var_id: u32, value: T, {var_sig} + ) {{ + __debug_member_assign_oracle_{n}(var_id, value, {vars}); + }} + pub fn __debug_member_assign_{n}(var_id: u32, value: T, {var_sig}) {{ + __debug_member_assign_inner_{n}(var_id, value, {vars}); + }} + + "# + ) + }) + .collect::>() + .join("\n"), + ] + .join("\n") +} + +fn pattern_vars(pattern: &ast::Pattern) -> Vec<(ast::Ident, bool)> { + let mut vars = vec![]; + let mut stack = VecDeque::from([(pattern, false)]); + while stack.front().is_some() { + let (pattern, is_mut) = stack.pop_front().unwrap(); + match pattern { + ast::Pattern::Identifier(id) => { + vars.push((id.clone(), is_mut)); + } + ast::Pattern::Mutable(pattern, _) => { + stack.push_back((pattern, true)); + } + ast::Pattern::Tuple(patterns, _) => { + stack.extend(patterns.iter().map(|pattern| (pattern, false))); + } + ast::Pattern::Struct(_, pids, _) => { + stack.extend(pids.iter().map(|(_, pattern)| (pattern, is_mut))); + vars.extend(pids.iter().map(|(id, _)| (id.clone(), false))); + } + } + } + vars +} + +fn ident(s: &str, span: Span) -> ast::Ident { + ast::Ident(Spanned::from(span, s.to_string())) +} + +fn id_expr(id: &ast::Ident) -> ast::Expression { + ast::Expression { + kind: ast::ExpressionKind::Variable(Path { + segments: vec![id.clone()], + kind: PathKind::Plain, + span: id.span(), + }), + span: id.span(), + } +} + +fn uint_expr(x: u128, span: Span) -> ast::Expression { + ast::Expression { + kind: ast::ExpressionKind::Literal(ast::Literal::Integer(x.into(), false)), + span, + } +} + +fn sint_expr(x: i128) -> ast::Expression { + ast::Expression { + kind: ast::ExpressionKind::Literal(ast::Literal::Integer(x.abs().into(), x < 0)), + span: none_span(), + } +} + +fn none_span() -> Span { + Span::build_from_str("") +} diff --git a/compiler/noirc_frontend/src/hir/mod.rs b/compiler/noirc_frontend/src/hir/mod.rs index 2124b5281f4..acd1bf982ed 100644 --- a/compiler/noirc_frontend/src/hir/mod.rs +++ b/compiler/noirc_frontend/src/hir/mod.rs @@ -4,13 +4,14 @@ pub mod resolution; pub mod scope; pub mod type_check; +use crate::debug::DebugState; use crate::graph::{CrateGraph, CrateId}; use crate::hir_def::function::FuncMeta; use crate::node_interner::{FuncId, NodeInterner, StructId}; -use crate::parser::ParserError; +use crate::parser::{ParserError, SortedModule}; use crate::ParsedModule; -use def_map::{Contract, CrateDefMap}; -use fm::FileManager; +use def_map::{parse_file, Contract, CrateDefMap}; +use fm::{FileId, FileManager}; use noirc_errors::Location; use std::borrow::Cow; use std::collections::{BTreeMap, HashMap}; @@ -31,6 +32,10 @@ pub struct Context<'file_manager, 'parsed_files> { // is read-only however, once it has been passed to the Context. pub file_manager: Cow<'file_manager, FileManager>, + pub root_crate_id: CrateId, + pub debug_state: DebugState, + pub instrument_debug: bool, + /// A map of each file that already has been visited from a prior `mod foo;` declaration. /// This is used to issue an error if a second `mod foo;` is declared to the same file. pub visited_files: BTreeMap, @@ -41,6 +46,8 @@ pub struct Context<'file_manager, 'parsed_files> { pub parsed_files: Cow<'parsed_files, ParsedFiles>, } +pub type StorageSlot = u32; + #[derive(Debug, Copy, Clone)] pub enum FunctionNameMatch<'a> { Anything, @@ -56,6 +63,9 @@ impl Context<'_, '_> { visited_files: BTreeMap::new(), crate_graph: CrateGraph::default(), file_manager: Cow::Owned(file_manager), + root_crate_id: CrateId::Dummy, + debug_state: DebugState::default(), + instrument_debug: false, parsed_files: Cow::Owned(parsed_files), } } @@ -70,6 +80,9 @@ impl Context<'_, '_> { visited_files: BTreeMap::new(), crate_graph: CrateGraph::default(), file_manager: Cow::Borrowed(file_manager), + root_crate_id: CrateId::Dummy, + debug_state: DebugState::default(), + instrument_debug: false, parsed_files: Cow::Borrowed(parsed_files), } } @@ -235,4 +248,23 @@ impl Context<'_, '_> { fn module(&self, module_id: def_map::ModuleId) -> &def_map::ModuleData { module_id.module(&self.def_maps) } + + /// Given a FileId, fetch the File, from the FileManager and parse its content, + /// applying sorting and debug transforms if debug mode is enabled. + pub fn parse_file( + &mut self, + file_id: FileId, + crate_id: CrateId, + ) -> (SortedModule, Vec) { + let (mut ast, parsing_errors) = parse_file(&self.file_manager, file_id); + + if self.instrument_debug && crate_id == self.root_crate_id { + self.debug_state.insert_symbols(&mut ast); + } + (ast.into_sorted(), parsing_errors) + } + + pub fn get_root_id(&self, crate_id: CrateId) -> FileId { + self.crate_graph[crate_id].root_file_id + } } diff --git a/compiler/noirc_frontend/src/lib.rs b/compiler/noirc_frontend/src/lib.rs index 9582b80dcba..2cb05184497 100644 --- a/compiler/noirc_frontend/src/lib.rs +++ b/compiler/noirc_frontend/src/lib.rs @@ -11,6 +11,7 @@ #![warn(clippy::semicolon_if_nothing_returned)] pub mod ast; +pub mod debug; pub mod graph; pub mod lexer; pub mod monomorphization; diff --git a/compiler/noirc_frontend/src/monomorphization/ast.rs b/compiler/noirc_frontend/src/monomorphization/ast.rs index 42a618e7d77..71a9e04166c 100644 --- a/compiler/noirc_frontend/src/monomorphization/ast.rs +++ b/compiler/noirc_frontend/src/monomorphization/ast.rs @@ -3,7 +3,8 @@ use iter_extended::vecmap; use noirc_errors::Location; use crate::{ - hir_def::function::FunctionSignature, BinaryOpKind, Distinctness, Signedness, Visibility, + hir_def::function::FunctionSignature, monomorphization::debug_types, BinaryOpKind, + Distinctness, Signedness, Visibility, }; /// The monomorphized AST is expression-based, all statements are also @@ -245,6 +246,7 @@ pub struct Program { /// forwarding to the next phase. pub return_distinctness: Distinctness, pub return_location: Option, + pub debug_var_types: debug_types::VariableTypes, pub return_visibility: Visibility, } @@ -254,6 +256,7 @@ impl Program { main_function_signature: FunctionSignature, return_distinctness: Distinctness, return_location: Option, + debug_var_types: debug_types::VariableTypes, return_visibility: Visibility, ) -> Program { Program { @@ -261,6 +264,7 @@ impl Program { main_function_signature, return_distinctness, return_location, + debug_var_types, return_visibility, } } diff --git a/compiler/noirc_frontend/src/monomorphization/debug_types.rs b/compiler/noirc_frontend/src/monomorphization/debug_types.rs new file mode 100644 index 00000000000..742d15b45dd --- /dev/null +++ b/compiler/noirc_frontend/src/monomorphization/debug_types.rs @@ -0,0 +1,70 @@ +use crate::hir_def::types::Type; +pub use noirc_errors::debug_info::{Types, VariableTypes, Variables}; +use noirc_printable_type::PrintableType; +use std::collections::HashMap; + +/// We keep a collection of the debug variables and their types in this +/// structure. The fe_var_id refers to the ID given when inserting the +/// instrumentation probe. This variable does not have a type yet and hence it +/// can be instantiated multiple types if it's in the context of a generic +/// variable. The var_id refers to the ID of the instantiated variable which +/// will have a valid type. +#[derive(Debug, Clone, Default)] +pub struct DebugTypes { + fe_to_vars: HashMap, // fe_var_id => var_id + variables: HashMap, // var_id => (var_name, type_id) + types: HashMap, + id_to_type: HashMap, + next_type_id: u32, + next_var_id: u32, +} + +impl DebugTypes { + pub fn insert_var(&mut self, fe_var_id: u32, var_name: &str, var_type: Type) -> u32 { + let ptype: PrintableType = var_type.follow_bindings().into(); + let type_id = self.types.get(&ptype).cloned().unwrap_or_else(|| { + let type_id = self.next_type_id; + self.next_type_id += 1; + self.types.insert(ptype.clone(), type_id); + self.id_to_type.insert(type_id, ptype); + type_id + }); + let existing_var_id = self.fe_to_vars.get(&fe_var_id).and_then(|var_id| { + let (_, existing_type_id) = self.variables.get(var_id).unwrap(); + if *existing_type_id == type_id { + Some(var_id) + } else { + None + } + }); + if let Some(var_id) = existing_var_id { + *var_id + } else { + let var_id = self.next_var_id; + self.next_var_id += 1; + self.variables.insert(var_id, (var_name.to_string(), type_id)); + self.fe_to_vars.insert(fe_var_id, var_id); + var_id + } + } + + pub fn get_var_id(&self, fe_var_id: u32) -> Option { + self.fe_to_vars.get(&fe_var_id).copied() + } + + pub fn get_type(&self, fe_var_id: u32) -> Option<&PrintableType> { + self.fe_to_vars + .get(&fe_var_id) + .and_then(|var_id| self.variables.get(var_id)) + .and_then(|(_, type_id)| self.id_to_type.get(type_id)) + } +} + +impl From for VariableTypes { + fn from(val: DebugTypes) -> Self { + ( + val.variables.into_iter().collect(), + val.types.into_iter().map(|(ptype, type_id)| (type_id, ptype)).collect(), + ) + } +} diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index 67b246a02ce..6e0917cc17e 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -20,7 +20,7 @@ use std::{ use crate::{ hir_def::{ expr::*, - function::{FunctionSignature, Parameters}, + function::{FuncMeta, FunctionSignature, Parameters}, stmt::{HirAssignStatement, HirLValue, HirLetStatement, HirPattern, HirStatement}, types, }, @@ -31,8 +31,10 @@ use crate::{ }; use self::ast::{Definition, FuncId, Function, LocalId, Program}; +use self::debug_types::DebugTypes; pub mod ast; +pub mod debug_types; pub mod printer; struct LambdaContext { @@ -40,6 +42,8 @@ struct LambdaContext { captures: Vec, } +const DEBUG_MEMBER_ASSIGN_PREFIX: &str = "__debug_member_assign_"; + /// The context struct for the monomorphization pass. /// /// This struct holds the FIFO queue of functions to monomorphize, which is added to @@ -67,7 +71,7 @@ struct Monomorphizer<'interner> { finished_functions: BTreeMap, /// Used to reference existing definitions in the HIR - interner: &'interner NodeInterner, + interner: &'interner mut NodeInterner, lambda_envs_stack: Vec, @@ -77,6 +81,9 @@ struct Monomorphizer<'interner> { is_range_loop: bool, return_location: Option, + + debug_types: DebugTypes, + debug_field_names: HashMap, } type HirType = crate::Type; @@ -93,9 +100,29 @@ type HirType = crate::Type; /// this function. Typically, this is the function named "main" in the source project, /// but it can also be, for example, an arbitrary test function for running `nargo test`. #[tracing::instrument(level = "trace", skip(main, interner))] -pub fn monomorphize(main: node_interner::FuncId, interner: &NodeInterner) -> Program { +pub fn monomorphize(main: node_interner::FuncId, interner: &mut NodeInterner) -> Program { + monomorphize_option_debug(main, interner, None) +} + +pub fn monomorphize_debug( + main: node_interner::FuncId, + interner: &mut NodeInterner, + field_names: &HashMap, +) -> Program { + monomorphize_option_debug(main, interner, Some(field_names)) +} + +fn monomorphize_option_debug( + main: node_interner::FuncId, + interner: &mut NodeInterner, + option_field_names: Option<&HashMap>, +) -> Program { let mut monomorphizer = Monomorphizer::new(interner); - let function_sig = monomorphizer.compile_main(main); + let function_sig = if let Some(field_names) = option_field_names { + monomorphizer.compile_main_debug(main, field_names) + } else { + monomorphizer.compile_main(main) + }; while !monomorphizer.queue.is_empty() { let (next_fn_id, new_id, bindings, trait_method) = monomorphizer.queue.pop_front().unwrap(); @@ -109,19 +136,21 @@ pub fn monomorphize(main: node_interner::FuncId, interner: &NodeInterner) -> Pro } let functions = vecmap(monomorphizer.finished_functions, |(_, f)| f); - let meta = interner.function_meta(&main); + let FuncMeta { return_distinctness, return_visibility, .. } = + monomorphizer.interner.function_meta(&main); Program::new( functions, function_sig, - meta.return_distinctness, + *return_distinctness, monomorphizer.return_location, - meta.return_visibility, + monomorphizer.debug_types.into(), + *return_visibility, ) } impl<'interner> Monomorphizer<'interner> { - fn new(interner: &'interner NodeInterner) -> Self { + fn new(interner: &'interner mut NodeInterner) -> Self { Monomorphizer { globals: HashMap::new(), locals: HashMap::new(), @@ -133,6 +162,8 @@ impl<'interner> Monomorphizer<'interner> { lambda_envs_stack: Vec::new(), is_range_loop: false, return_location: None, + debug_types: DebugTypes::default(), + debug_field_names: HashMap::default(), } } @@ -224,30 +255,43 @@ impl<'interner> Monomorphizer<'interner> { main_meta.function_signature() } + fn compile_main_debug( + &mut self, + main_id: node_interner::FuncId, + field_names: &HashMap, + ) -> FunctionSignature { + self.debug_field_names = field_names.clone(); + self.compile_main(main_id) + } + fn function(&mut self, f: node_interner::FuncId, id: FuncId) { if let Some((self_type, trait_id)) = self.interner.get_function_trait(&f) { let the_trait = self.interner.get_trait(trait_id); the_trait.self_type_typevar.force_bind(self_type); } - let meta = self.interner.function_meta(&f); - let modifiers = self.interner.function_modifiers(&f); - let name = self.interner.function_name(&f).to_owned(); - - let body_expr_id = *self.interner.function(&f).as_expr(); - let body_return_type = self.interner.id_type(body_expr_id); - let return_type = self.convert_type(match meta.return_type() { - Type::TraitAsType(..) => &body_return_type, - _ => meta.return_type(), - }); + let (meta_parameters, unconstrained, body_expr_id, name, return_type) = { + let meta = self.interner.function_meta(&f).clone(); + let modifiers = self.interner.function_modifiers(&f).clone(); + let name = self.interner.function_name(&f).to_string(); - let parameters = self.parameters(&meta.parameters); + let body_expr_id = *self.interner.function(&f).as_expr(); + let body_return_type = self.interner.id_type(body_expr_id); + let return_type = self.convert_type(match meta.return_type() { + Type::TraitAsType(_, _, _) => &body_return_type, + _ => meta.return_type(), + }); + let unconstrained = modifiers.is_unconstrained + || matches!(modifiers.contract_function_type, Some(ContractFunctionType::Open)); + (meta.parameters, unconstrained, body_expr_id, name, return_type) + }; - let body = self.expr(body_expr_id); - let unconstrained = modifiers.is_unconstrained - || matches!(modifiers.contract_function_type, Some(ContractFunctionType::Open)); + let function = { + let parameters = self.parameters(&meta_parameters).clone(); + let body = self.expr(body_expr_id).clone(); + ast::Function { id, name, parameters, body, return_type, unconstrained } + }; - let function = ast::Function { id, name, parameters, body, return_type, unconstrained }; self.push_function(id, function); } @@ -909,6 +953,15 @@ impl<'interner> Monomorphizer<'interner> { }) } + fn intern_var_id(&mut self, var_id: u32, location: &Location) -> node_interner::ExprId { + let expr_id = self + .interner + .push_expr(HirExpression::Literal(HirLiteral::Integer((var_id as u128).into(), false))); + self.interner.push_expr_type(&expr_id, Type::FieldElement); + self.interner.push_expr_location(expr_id, location.span, location.file); + expr_id + } + fn function_call( &mut self, call: HirCallExpression, @@ -917,6 +970,102 @@ impl<'interner> Monomorphizer<'interner> { let original_func = Box::new(self.expr(call.func)); let mut arguments = vecmap(&call.arguments, |id| self.expr(*id)); let hir_arguments = vecmap(&call.arguments, |id| self.interner.expression(id)); + if let ast::Expression::Ident(ast::Ident { name, .. }) = original_func.as_ref() { + if let ( + Some(HirExpression::Literal(HirLiteral::Integer(fe_var_id, _))), + Some(HirExpression::Ident(HirIdent { id, .. })), + true, + ) = (hir_arguments.get(0), hir_arguments.get(1), name == "__debug_var_assign") + { + // update variable assignments + let var_def = self.interner.definition(*id); + let var_type = self.interner.id_type(call.arguments[1]); + let fe_var_id = fe_var_id.to_u128() as u32; + let var_id = if var_def.name != "__debug_expr" { + self.debug_types.insert_var(fe_var_id, &var_def.name, var_type) + } else { + self.debug_types.get_var_id(fe_var_id).unwrap() + }; + let interned_var_id = self.intern_var_id(var_id, &call.location); + arguments[0] = self.expr(interned_var_id); + } else if let (Some(HirExpression::Literal(HirLiteral::Integer(fe_var_id, _))), true) = + (hir_arguments.get(0), name == "__debug_var_drop") + { + // update variable drops (ie. when the var goes out of scope) + let fe_var_id = fe_var_id.to_u128() as u32; + if let Some(var_id) = self.debug_types.get_var_id(fe_var_id) { + let interned_var_id = self.intern_var_id(var_id, &call.location); + arguments[0] = self.expr(interned_var_id); + } + } else if let ( + Some(HirExpression::Literal(HirLiteral::Integer(fe_var_id, _))), + Some(HirExpression::Ident(HirIdent { id, .. })), + true, + ) = ( + hir_arguments.get(0), + hir_arguments.get(1), + name.starts_with(DEBUG_MEMBER_ASSIGN_PREFIX), + ) { + // update variable member assignments + let var_def_name = self.interner.definition(*id).name.clone(); + let var_type = self.interner.id_type(call.arguments[2]); + let fe_var_id = fe_var_id.to_u128() as u32; + let arity = name[DEBUG_MEMBER_ASSIGN_PREFIX.len()..] + .parse::() + .expect("failed to parse member assign arity"); + + let mut cursor_type = self + .debug_types + .get_type(fe_var_id) + .unwrap_or_else(|| panic!("type not found for fe_var_id={fe_var_id}")) + .clone(); + for i in 0..arity { + if let Some(HirExpression::Literal(HirLiteral::Integer(fe_i, i_neg))) = + hir_arguments.get(2 + i) + { + let mut index = fe_i.to_i128(); + if *i_neg { + index = -index; + } + if index < 0 { + let index = index.unsigned_abs(); + let field_name = self + .debug_field_names + .get(&(index as u32)) + .unwrap_or_else(|| panic!("field name not available for {i:?}")); + let field_i = + (get_field(&cursor_type, field_name).unwrap_or_else(|| { + panic!("failed to find field_name: {field_name}") + }) as i128) + .unsigned_abs(); + cursor_type = next_type(&cursor_type, field_i as usize); + let index_id = self.interner.push_expr(HirExpression::Literal( + HirLiteral::Integer(field_i.into(), false), + )); + self.interner.push_expr_type(&index_id, Type::FieldElement); + self.interner.push_expr_location( + index_id, + call.location.span, + call.location.file, + ); + arguments[2 + i] = self.expr(index_id); + } else { + cursor_type = next_type(&cursor_type, 0); + } + } else { + cursor_type = next_type(&cursor_type, 0); + } + } + + let var_id = if &var_def_name != "__debug_expr" { + self.debug_types.insert_var(fe_var_id, &var_def_name, var_type) + } else { + self.debug_types.get_var_id(fe_var_id).unwrap() + }; + let interned_var_id = self.intern_var_id(var_id, &call.location); + arguments[0] = self.expr(interned_var_id); + } + } let return_type = self.interner.id_type(id); let return_type = self.convert_type(&return_type); @@ -1590,3 +1739,30 @@ fn undo_instantiation_bindings(bindings: TypeBindings) { var.unbind(id); } } + +fn get_field(ptype: &PrintableType, field_name: &str) -> Option { + match ptype { + PrintableType::Struct { fields, .. } => { + fields.iter().position(|(name, _)| name == field_name) + } + PrintableType::Tuple { .. } | PrintableType::Array { .. } => { + field_name.parse::().ok() + } + _ => None, + } +} + +fn next_type(ptype: &PrintableType, i: usize) -> PrintableType { + match ptype { + PrintableType::Array { length: _length, typ } => (**typ).clone(), + PrintableType::Tuple { types } => types[i].clone(), + PrintableType::Struct { name: _name, fields } => fields[i].1.clone(), + PrintableType::String { length: _length } => { + // TODO: check bounds + PrintableType::UnsignedInteger { width: 8 } + } + _ => { + panic!["expected type with sub-fields, found terminal type"] + } + } +} diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 9ccbddab9ec..58eba9fd8e7 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -1126,9 +1126,9 @@ mod test { } fn check_rewrite(src: &str, expected: &str) { - let (_program, context, _errors) = get_program(src); + let (_program, mut context, _errors) = get_program(src); let main_func_id = context.def_interner.find_function("main").unwrap(); - let program = monomorphize(main_func_id, &context.def_interner); + let program = monomorphize(main_func_id, &mut context.def_interner); assert!(format!("{}", program) == expected); } diff --git a/compiler/noirc_printable_type/src/lib.rs b/compiler/noirc_printable_type/src/lib.rs index 18f2fe0a873..bae3b1e43a3 100644 --- a/compiler/noirc_printable_type/src/lib.rs +++ b/compiler/noirc_printable_type/src/lib.rs @@ -6,7 +6,7 @@ use regex::{Captures, Regex}; use serde::{Deserialize, Serialize}; use thiserror::Error; -#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Clone, Debug, PartialEq, Eq, Hash, Serialize, Deserialize)] #[serde(tag = "kind", rename_all = "lowercase")] pub enum PrintableType { Field, @@ -50,6 +50,7 @@ pub enum PrintableValue { String(String), Vec(Vec), Struct(BTreeMap), + Other, } /// In order to display a `PrintableValue` we need a `PrintableType` to accurately @@ -293,7 +294,7 @@ fn format_field_string(field: FieldElement) -> String { } /// Assumes that `field_iterator` contains enough [FieldElement] in order to decode the [PrintableType] -fn decode_value( +pub fn decode_value( field_iterator: &mut impl Iterator, typ: &PrintableType, ) -> PrintableValue { diff --git a/tooling/debugger/Cargo.toml b/tooling/debugger/Cargo.toml index 4d240c61f90..785eacf9463 100644 --- a/tooling/debugger/Cargo.toml +++ b/tooling/debugger/Cargo.toml @@ -11,11 +11,12 @@ build-data.workspace = true [dependencies] acvm.workspace = true +fm.workspace = true nargo.workspace = true +noirc_frontend.workspace = true noirc_printable_type.workspace = true noirc_errors.workspace = true noirc_driver.workspace = true -fm.workspace = true thiserror.workspace = true codespan-reporting.workspace = true dap.workspace = true diff --git a/tooling/debugger/src/context.rs b/tooling/debugger/src/context.rs index 12b55708b15..ddc8c870305 100644 --- a/tooling/debugger/src/context.rs +++ b/tooling/debugger/src/context.rs @@ -1,3 +1,4 @@ +use crate::foreign_calls::DebugForeignCallExecutor; use acvm::acir::circuit::{Circuit, Opcode, OpcodeLocation}; use acvm::acir::native_types::{Witness, WitnessMap}; use acvm::brillig_vm::{brillig::Value, Registers}; @@ -8,8 +9,8 @@ use acvm::{BlackBoxFunctionSolver, FieldElement}; use nargo::artifacts::debug::DebugArtifact; use nargo::errors::{ExecutionError, Location}; -use nargo::ops::ForeignCallExecutor; use nargo::NargoError; +use noirc_printable_type::{PrintableType, PrintableValue}; use std::collections::{hash_set::Iter, HashSet}; @@ -24,7 +25,7 @@ pub(super) enum DebugCommandResult { pub(super) struct DebugContext<'a, B: BlackBoxFunctionSolver> { acvm: ACVM<'a, B>, brillig_solver: Option>, - foreign_call_executor: Box, + foreign_call_executor: Box, debug_artifact: &'a DebugArtifact, breakpoints: HashSet, } @@ -35,7 +36,7 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { circuit: &'a Circuit, debug_artifact: &'a DebugArtifact, initial_witness: WitnessMap, - foreign_call_executor: Box, + foreign_call_executor: Box, ) -> Self { Self { acvm: ACVM::new(blackbox_solver, &circuit.opcodes, initial_witness), @@ -76,15 +77,49 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { } } + pub(super) fn is_source_location_in_debug_module(&self, location: &Location) -> bool { + self.debug_artifact + .file_map + .get(&location.file) + .map(|file| file.path.starts_with("__debug/")) + .unwrap_or(false) + } + /// Returns the callstack in source code locations for the currently /// executing opcode. This can be `None` if the execution finished (and /// `get_current_opcode_location()` returns `None`) or if the opcode is not /// mapped to a specific source location in the debug artifact (which can - /// happen for certain opcodes inserted synthetically by the compiler) + /// happen for certain opcodes inserted synthetically by the compiler). + /// 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() .as_ref() - .and_then(|location| self.debug_artifact.debug_symbols[0].opcode_location(location)) + .map(|opcode_location| self.get_source_location_for_opcode_location(opcode_location)) + .filter(|v: &Vec| !v.is_empty()) + } + + /// Returns the (possible) stack of source locations corresponding to the + /// given opcode location. Due to compiler inlining it's possible for this + /// function to return multiple source locations. An empty vector means that + /// 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( + &self, + opcode_location: &OpcodeLocation, + ) -> Vec { + self.debug_artifact.debug_symbols[0] + .opcode_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(vec![]) } fn get_opcodes_sizes(&self) -> Vec { @@ -372,6 +407,10 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { } } + pub(super) fn get_variables(&self) -> Vec<(&str, &PrintableValue, &PrintableType)> { + return self.foreign_call_executor.get_variables(); + } + fn breakpoint_reached(&self) -> bool { if let Some(location) = self.get_current_opcode_location() { self.breakpoints.contains(&location) @@ -432,6 +471,7 @@ mod tests { use super::*; use crate::context::{DebugCommandResult, DebugContext}; + use crate::foreign_calls::DefaultDebugForeignCallExecutor; use acvm::{ acir::{ circuit::{ @@ -445,7 +485,7 @@ mod tests { BinaryFieldOp, Opcode as BrilligOpcode, RegisterIndex, RegisterOrMemory, }, }; - use nargo::{artifacts::debug::DebugArtifact, ops::DefaultForeignCallExecutor}; + use nargo::artifacts::debug::DebugArtifact; use std::collections::BTreeMap; #[test] @@ -485,12 +525,14 @@ mod tests { let initial_witness = BTreeMap::from([(Witness(1), fe_1)]).into(); + let foreign_call_executor = + Box::new(DefaultDebugForeignCallExecutor::from_artifact(true, &debug_artifact)); let mut context = DebugContext::new( &StubbedBlackBoxSolver, circuit, debug_artifact, initial_witness, - Box::new(DefaultForeignCallExecutor::new(true, None)), + foreign_call_executor, ); assert_eq!(context.get_current_opcode_location(), Some(OpcodeLocation::Acir(0))); @@ -577,12 +619,14 @@ mod tests { let initial_witness = BTreeMap::from([(Witness(1), fe_1), (Witness(2), fe_1)]).into(); + let foreign_call_executor = + Box::new(DefaultDebugForeignCallExecutor::from_artifact(true, &debug_artifact)); let mut context = DebugContext::new( &StubbedBlackBoxSolver, circuit, debug_artifact, initial_witness, - Box::new(DefaultForeignCallExecutor::new(true, None)), + foreign_call_executor, ); // set breakpoint @@ -631,7 +675,7 @@ mod tests { &circuit, &debug_artifact, WitnessMap::new(), - Box::new(DefaultForeignCallExecutor::new(true, None)), + Box::new(DefaultDebugForeignCallExecutor::new(true)), ); assert_eq!(context.offset_opcode_location(&None, 0), (None, 0)); diff --git a/tooling/debugger/src/dap.rs b/tooling/debugger/src/dap.rs index 803f9f108db..2de7dac77d8 100644 --- a/tooling/debugger/src/dap.rs +++ b/tooling/debugger/src/dap.rs @@ -9,6 +9,7 @@ use codespan_reporting::files::{Files, SimpleFile}; use crate::context::DebugCommandResult; use crate::context::DebugContext; +use crate::foreign_calls::DefaultDebugForeignCallExecutor; use dap::errors::ServerError; use dap::events::StoppedEventBody; @@ -25,7 +26,6 @@ use dap::types::{ StoppedEventReason, Thread, }; use nargo::artifacts::debug::DebugArtifact; -use nargo::ops::DefaultForeignCallExecutor; use fm::FileId; use noirc_driver::CompiledProgram; @@ -57,7 +57,7 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession<'a, R, W, B> { circuit, debug_artifact, initial_witness, - Box::new(DefaultForeignCallExecutor::new(true, None)), + Box::new(DefaultDebugForeignCallExecutor::from_artifact(true, debug_artifact)), ); Self { server, diff --git a/tooling/debugger/src/foreign_calls.rs b/tooling/debugger/src/foreign_calls.rs new file mode 100644 index 00000000000..4091f73fb69 --- /dev/null +++ b/tooling/debugger/src/foreign_calls.rs @@ -0,0 +1,156 @@ +use acvm::{ + acir::brillig::{ForeignCallParam, ForeignCallResult, Value}, + pwg::ForeignCallWaitInfo, +}; +use nargo::{ + artifacts::debug::{DebugArtifact, DebugVars}, + ops::{DefaultForeignCallExecutor, ForeignCallExecutor}, +}; +use noirc_printable_type::{ForeignCallError, PrintableType, PrintableValue}; + +pub(crate) enum DebugForeignCall { + VarAssign, + VarDrop, + MemberAssign(u32), + DerefAssign, +} + +impl std::fmt::Display for DebugForeignCall { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.name()) + } +} + +impl DebugForeignCall { + pub(crate) fn name(&self) -> &'static str { + match self { + DebugForeignCall::VarAssign => "__debug_var_assign", + DebugForeignCall::VarDrop => "__debug_var_drop", + DebugForeignCall::MemberAssign(1) => "__debug_member_assign_1", + DebugForeignCall::MemberAssign(2) => "__debug_member_assign_2", + DebugForeignCall::MemberAssign(3) => "__debug_member_assign_3", + DebugForeignCall::MemberAssign(4) => "__debug_member_assign_4", + DebugForeignCall::MemberAssign(5) => "__debug_member_assign_5", + DebugForeignCall::MemberAssign(6) => "__debug_member_assign_6", + DebugForeignCall::MemberAssign(7) => "__debug_member_assign_7", + DebugForeignCall::MemberAssign(8) => "__debug_member_assign_8", + DebugForeignCall::MemberAssign(_) => panic!("unsupported member assignment arity"), + DebugForeignCall::DerefAssign => "__debug_deref_assign", + } + } + + pub(crate) fn lookup(op_name: &str) -> Option { + let member_pre = "__debug_member_assign_"; + if let Some(op_suffix) = op_name.strip_prefix(member_pre) { + let arity = + op_suffix.parse::().expect("failed to parse debug_member_assign arity"); + return Some(DebugForeignCall::MemberAssign(arity)); + } + match op_name { + "__debug_var_assign" => Some(DebugForeignCall::VarAssign), + "__debug_var_drop" => Some(DebugForeignCall::VarDrop), + "__debug_deref_assign" => Some(DebugForeignCall::DerefAssign), + _ => None, + } + } +} + +pub trait DebugForeignCallExecutor: ForeignCallExecutor { + fn get_variables(&self) -> Vec<(&str, &PrintableValue, &PrintableType)>; +} + +pub struct DefaultDebugForeignCallExecutor { + executor: DefaultForeignCallExecutor, + pub debug_vars: DebugVars, +} + +impl DefaultDebugForeignCallExecutor { + pub fn new(show_output: bool) -> Self { + Self { + executor: DefaultForeignCallExecutor::new(show_output, None), + debug_vars: DebugVars::default(), + } + } + + pub fn from_artifact(show_output: bool, artifact: &DebugArtifact) -> Self { + let mut ex = Self::new(show_output); + ex.load_artifact(artifact); + ex + } + + pub fn load_artifact(&mut self, artifact: &DebugArtifact) { + artifact.debug_symbols.iter().for_each(|info| { + self.debug_vars.insert_variables(&info.variables); + self.debug_vars.insert_types(&info.types); + }); + } +} + +impl DebugForeignCallExecutor for DefaultDebugForeignCallExecutor { + fn get_variables(&self) -> Vec<(&str, &PrintableValue, &PrintableType)> { + self.debug_vars.get_variables() + } +} + +impl ForeignCallExecutor for DefaultDebugForeignCallExecutor { + fn execute( + &mut self, + foreign_call: &ForeignCallWaitInfo, + ) -> Result { + let foreign_call_name = foreign_call.function.as_str(); + match DebugForeignCall::lookup(foreign_call_name) { + Some(DebugForeignCall::VarAssign) => { + let fcp_var_id = &foreign_call.inputs[0]; + if let ForeignCallParam::Single(var_id_value) = fcp_var_id { + let var_id = var_id_value.to_u128() as u32; + let values: Vec = + foreign_call.inputs[1..].iter().flat_map(|x| x.values()).collect(); + self.debug_vars.assign(var_id, &values); + } + Ok(ForeignCallResult { values: vec![] }) + } + Some(DebugForeignCall::VarDrop) => { + let fcp_var_id = &foreign_call.inputs[0]; + if let ForeignCallParam::Single(var_id_value) = fcp_var_id { + let var_id = var_id_value.to_u128() as u32; + self.debug_vars.drop(var_id); + } + Ok(ForeignCallResult { values: vec![] }) + } + Some(DebugForeignCall::MemberAssign(arity)) => { + if let Some(ForeignCallParam::Single(var_id_value)) = foreign_call.inputs.get(0) { + let arity = arity as usize; + let var_id = var_id_value.to_u128() as u32; + let n = foreign_call.inputs.len(); + let indexes: Vec = foreign_call.inputs[(n - arity)..n] + .iter() + .map(|fcp_v| { + if let ForeignCallParam::Single(v) = fcp_v { + v.to_u128() as u32 + } else { + panic!("expected ForeignCallParam::Single(v)"); + } + }) + .collect(); + let values: Vec = (0..n - 1 - arity) + .flat_map(|i| { + foreign_call.inputs.get(1 + i).map(|fci| fci.values()).unwrap_or(vec![]) + }) + .collect(); + self.debug_vars.assign_field(var_id, indexes, &values); + } + Ok(ForeignCallResult { values: vec![] }) + } + Some(DebugForeignCall::DerefAssign) => { + let fcp_var_id = &foreign_call.inputs[0]; + let fcp_value = &foreign_call.inputs[1]; + if let ForeignCallParam::Single(var_id_value) = fcp_var_id { + let var_id = var_id_value.to_u128() as u32; + self.debug_vars.assign_deref(var_id, &fcp_value.values()); + } + Ok(ForeignCallResult { values: vec![] }) + } + None => self.executor.execute(foreign_call), + } + } +} diff --git a/tooling/debugger/src/lib.rs b/tooling/debugger/src/lib.rs index 21834e44f93..35014f9a8c8 100644 --- a/tooling/debugger/src/lib.rs +++ b/tooling/debugger/src/lib.rs @@ -1,5 +1,6 @@ mod context; mod dap; +mod foreign_calls; mod repl; mod source_code_printer; diff --git a/tooling/debugger/src/repl.rs b/tooling/debugger/src/repl.rs index b1af2bc2686..2112f7414fa 100644 --- a/tooling/debugger/src/repl.rs +++ b/tooling/debugger/src/repl.rs @@ -4,9 +4,11 @@ use acvm::acir::circuit::{Circuit, Opcode, OpcodeLocation}; use acvm::acir::native_types::{Witness, WitnessMap}; use acvm::{BlackBoxFunctionSolver, FieldElement}; -use nargo::{artifacts::debug::DebugArtifact, ops::DefaultForeignCallExecutor, NargoError}; +use crate::foreign_calls::DefaultDebugForeignCallExecutor; +use nargo::{artifacts::debug::DebugArtifact, NargoError}; use easy_repl::{command, CommandStatus, Repl}; +use noirc_printable_type::PrintableValueDisplay; use std::cell::RefCell; use crate::source_code_printer::print_source_code_location; @@ -27,12 +29,14 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { debug_artifact: &'a DebugArtifact, initial_witness: WitnessMap, ) -> Self { + let foreign_call_executor = + Box::new(DefaultDebugForeignCallExecutor::from_artifact(true, debug_artifact)); let context = DebugContext::new( blackbox_solver, circuit, debug_artifact, initial_witness.clone(), - Box::new(DefaultForeignCallExecutor::new(true, None)), + foreign_call_executor, ); Self { context, @@ -65,7 +69,6 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { ); } } - print_source_code_location(self.debug_artifact, &location); } } @@ -203,12 +206,14 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { fn restart_session(&mut self) { 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.debug_artifact, self.initial_witness.clone(), - Box::new(DefaultForeignCallExecutor::new(true, None)), + foreign_call_executor, ); for opcode_location in breakpoints { self.context.add_breakpoint(opcode_location); @@ -305,6 +310,15 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { self.context.write_brillig_memory(index, field_value); } + pub fn show_vars(&self) { + let vars = self.context.get_variables(); + for (var_name, value, var_type) in vars.iter() { + let printable_value = + PrintableValueDisplay::Plain((*value).clone(), (*var_type).clone()); + println!("{var_name}:{var_type:?} = {}", printable_value); + } + } + fn is_solved(&self) -> bool { self.context.is_solved() } @@ -477,6 +491,16 @@ pub fn run( } }, ) + .add( + "vars", + command! { + "show variable values available at this point in execution", + () => || { + ref_context.borrow_mut().show_vars(); + Ok(CommandStatus::Done) + } + }, + ) .build() .expect("Failed to initialize debugger repl"); diff --git a/tooling/debugger/src/source_code_printer.rs b/tooling/debugger/src/source_code_printer.rs index 1707f9066b7..35345037b5c 100644 --- a/tooling/debugger/src/source_code_printer.rs +++ b/tooling/debugger/src/source_code_printer.rs @@ -276,7 +276,7 @@ mod tests { let mut opcode_locations = BTreeMap::>::new(); opcode_locations.insert(OpcodeLocation::Acir(42), vec![loc]); - let debug_symbols = vec![DebugInfo::new(opcode_locations)]; + let debug_symbols = vec![DebugInfo::new(opcode_locations, (vec![], vec![]))]; let debug_artifact = DebugArtifact::new(debug_symbols, &fm); let location_rendered: Vec<_> = render_location(&debug_artifact, &loc).collect(); diff --git a/tooling/lsp/src/requests/test_run.rs b/tooling/lsp/src/requests/test_run.rs index 135090d7ed9..259b5c5779e 100644 --- a/tooling/lsp/src/requests/test_run.rs +++ b/tooling/lsp/src/requests/test_run.rs @@ -4,7 +4,7 @@ use async_lsp::{ErrorCode, ResponseError}; use nargo::{ insert_all_files_for_workspace_into_file_manager, ops::{run_test, TestStatus}, - prepare_package, + parse_all, prepare_package, }; use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection}; use noirc_driver::{ @@ -13,7 +13,6 @@ use noirc_driver::{ use noirc_frontend::hir::FunctionNameMatch; use crate::{ - parse_diff, types::{NargoTestRunParams, NargoTestRunResult}, LspState, }; @@ -26,7 +25,7 @@ pub(crate) fn on_test_run_request( } fn on_test_run_request_inner( - state: &mut LspState, + state: &LspState, params: NargoTestRunParams, ) -> Result { let root_path = state.root_path.as_deref().ok_or_else(|| { @@ -53,7 +52,7 @@ fn on_test_run_request_inner( let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir); insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager); - let parsed_files = parse_diff(&workspace_file_manager, state); + let parsed_files = parse_all(&workspace_file_manager); // Since we filtered on crate name, this should be the only item in the iterator match workspace.into_iter().next() { @@ -83,7 +82,7 @@ fn on_test_run_request_inner( let test_result = run_test( &state.solver, - &context, + &mut context, test_function, false, None, diff --git a/tooling/nargo/src/artifacts/debug.rs b/tooling/nargo/src/artifacts/debug.rs index 3f5df801b66..ba93e30ff37 100644 --- a/tooling/nargo/src/artifacts/debug.rs +++ b/tooling/nargo/src/artifacts/debug.rs @@ -8,6 +8,7 @@ use std::{ ops::Range, }; +pub use super::debug_vars::DebugVars; use fm::{FileId, FileManager, PathString}; /// A Debug Artifact stores, for a given program, the debug info for every function @@ -23,22 +24,15 @@ 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 - .iter() - .flat_map(|function_symbols| { - function_symbols - .locations - .values() - .flat_map(|call_stack| call_stack.iter().map(|location| location.file)) - }) - .collect(); + let file_ids: BTreeSet = + debug_symbols.iter().flat_map(|debug_info| debug_info.get_file_ids()).collect(); - 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"); + for file_id in file_ids.iter() { + 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"); file_map.insert( - file_id, + *file_id, DebugFile { source: file_source.to_string(), path: file_path.to_path_buf() }, ); } @@ -114,6 +108,14 @@ impl DebugArtifact { let source = self.source(location.file)?; self.line_index(location.file, source.len()) } + + pub fn from_program(program: &CompiledProgram) -> Self { + Self { + debug_symbols: vec![program.debug.clone()], + file_map: program.file_map.clone(), + warnings: program.warnings.clone(), + } + } } impl From for DebugArtifact { @@ -229,7 +231,7 @@ mod tests { let mut opcode_locations = BTreeMap::>::new(); opcode_locations.insert(OpcodeLocation::Acir(42), vec![loc]); - let debug_symbols = vec![DebugInfo::new(opcode_locations)]; + let debug_symbols = vec![DebugInfo::new(opcode_locations, (vec![], vec![]))]; let debug_artifact = DebugArtifact::new(debug_symbols, &fm); let location_in_line = debug_artifact.location_in_line(loc).expect("Expected a range"); diff --git a/tooling/nargo/src/artifacts/debug_vars.rs b/tooling/nargo/src/artifacts/debug_vars.rs new file mode 100644 index 00000000000..b4acfc5ced2 --- /dev/null +++ b/tooling/nargo/src/artifacts/debug_vars.rs @@ -0,0 +1,132 @@ +use acvm::brillig_vm::brillig::Value; +use noirc_printable_type::{decode_value, PrintableType, PrintableValue}; +use std::collections::{HashMap, HashSet}; + +#[derive(Debug, Default, Clone)] +pub struct DebugVars { + id_to_name: HashMap, + active: HashSet, + id_to_value: HashMap, // TODO: something more sophisticated for lexical levels + id_to_type: HashMap, + types: HashMap, +} + +impl DebugVars { + pub fn new(vars: &HashMap) -> Self { + Self { id_to_name: vars.clone(), ..Self::default() } + } + + pub fn get_variables(&self) -> Vec<(&str, &PrintableValue, &PrintableType)> { + self.active + .iter() + .filter_map(|var_id| { + self.id_to_name + .get(var_id) + .and_then(|name| self.id_to_value.get(var_id).map(|value| (name, value))) + .and_then(|(name, value)| { + self.id_to_type.get(var_id).map(|type_id| (name, value, type_id)) + }) + .and_then(|(name, value, type_id)| { + self.types.get(type_id).map(|ptype| (name.as_str(), value, ptype)) + }) + }) + .collect() + } + + pub fn insert_variables(&mut self, vars: &HashMap) { + vars.iter().for_each(|(var_id, (var_name, var_type))| { + self.id_to_name.insert(*var_id, var_name.clone()); + self.id_to_type.insert(*var_id, *var_type); + }); + } + + pub fn insert_types(&mut self, types: &HashMap) { + types.iter().for_each(|(type_id, ptype)| { + self.types.insert(*type_id, ptype.clone()); + }); + } + + pub fn assign(&mut self, var_id: u32, values: &[Value]) { + self.active.insert(var_id); + // TODO: assign values as PrintableValue + let type_id = self.id_to_type.get(&var_id).unwrap(); + let ptype = self.types.get(type_id).unwrap(); + self.id_to_value + .insert(var_id, decode_value(&mut values.iter().map(|v| v.to_field()), ptype)); + } + + pub fn assign_field(&mut self, var_id: u32, indexes: Vec, values: &[Value]) { + let mut cursor: &mut PrintableValue = self + .id_to_value + .get_mut(&var_id) + .unwrap_or_else(|| panic!("value unavailable for var_id {var_id}")); + let cursor_type_id = self + .id_to_type + .get(&var_id) + .unwrap_or_else(|| panic!("type id unavailable for var_id {var_id}")); + let mut cursor_type = self + .types + .get(cursor_type_id) + .unwrap_or_else(|| panic!("type unavailable for type id {cursor_type_id}")); + for index in indexes.iter() { + (cursor, cursor_type) = match (cursor, cursor_type) { + (PrintableValue::Vec(array), PrintableType::Array { length, typ }) => { + if let Some(len) = length { + if *index as u64 >= *len { + panic!("unexpected field index past array length") + } + if *len != array.len() as u64 { + panic!("type/array length mismatch") + } + } + (array.get_mut(*index as usize).unwrap(), &*Box::leak(typ.clone())) + } + ( + PrintableValue::Struct(field_map), + PrintableType::Struct { name: _name, fields }, + ) => { + if *index as usize >= fields.len() { + panic!("unexpected field index past struct field length") + } + let (key, typ) = fields.get(*index as usize).unwrap(); + (field_map.get_mut(key).unwrap(), typ) + } + (PrintableValue::Vec(array), PrintableType::Tuple { types }) => { + if *index >= types.len() as u32 { + panic!( + "unexpected field index ({index}) past tuple length ({})", + types.len() + ); + } + if types.len() != array.len() { + panic!("type/array length mismatch") + } + let typ = types.get(*index as usize).unwrap(); + (array.get_mut(*index as usize).unwrap(), typ) + } + _ => { + panic!("unexpected assign field of {cursor_type:?} type"); + } + }; + } + *cursor = decode_value(&mut values.iter().map(|v| v.to_field()), cursor_type); + self.active.insert(var_id); + } + + pub fn assign_deref(&mut self, _var_id: u32, _values: &[Value]) { + // TODO + unimplemented![] + } + + pub fn get(&mut self, var_id: u32) -> Option<&PrintableValue> { + self.id_to_value.get(&var_id) + } + + pub fn get_type(&self, var_id: u32) -> Option<&PrintableType> { + self.id_to_type.get(&var_id).and_then(|type_id| self.types.get(type_id)) + } + + pub fn drop(&mut self, var_id: u32) { + self.active.remove(&var_id); + } +} diff --git a/tooling/nargo/src/artifacts/mod.rs b/tooling/nargo/src/artifacts/mod.rs index 180a900fd81..c7b3736f90b 100644 --- a/tooling/nargo/src/artifacts/mod.rs +++ b/tooling/nargo/src/artifacts/mod.rs @@ -5,4 +5,5 @@ //! to generate them using these artifacts as a starting point. pub mod contract; pub mod debug; +mod debug_vars; pub mod program; diff --git a/tooling/nargo/src/lib.rs b/tooling/nargo/src/lib.rs index 0fdff8b202f..056e92e8034 100644 --- a/tooling/nargo/src/lib.rs +++ b/tooling/nargo/src/lib.rs @@ -119,6 +119,7 @@ pub fn prepare_package<'file_manager, 'parsed_files>( let mut context = Context::from_ref_file_manager(file_manager, parsed_files); let crate_id = prepare_crate(&mut context, &package.entry_path); + context.root_crate_id = crate_id; prepare_dependencies(&mut context, crate_id, &package.dependencies); diff --git a/tooling/nargo/src/ops/foreign_calls.rs b/tooling/nargo/src/ops/foreign_calls.rs index cbe40c92b4e..056df976264 100644 --- a/tooling/nargo/src/ops/foreign_calls.rs +++ b/tooling/nargo/src/ops/foreign_calls.rs @@ -14,7 +14,7 @@ pub trait ForeignCallExecutor { /// This enumeration represents the Brillig foreign calls that are natively supported by nargo. /// After resolution of a foreign call, nargo will restart execution of the ACVM -pub(crate) enum ForeignCall { +pub enum ForeignCall { Print, CreateMock, SetMockParams, diff --git a/tooling/nargo/src/ops/mod.rs b/tooling/nargo/src/ops/mod.rs index 4912c84839e..f19a4057ecd 100644 --- a/tooling/nargo/src/ops/mod.rs +++ b/tooling/nargo/src/ops/mod.rs @@ -1,6 +1,6 @@ pub use self::compile::{compile_contract, compile_program, compile_workspace}; pub use self::execute::execute_circuit; -pub use self::foreign_calls::{DefaultForeignCallExecutor, ForeignCallExecutor}; +pub use self::foreign_calls::{DefaultForeignCallExecutor, ForeignCall, ForeignCallExecutor}; pub use self::optimize::{optimize_contract, optimize_program}; pub use self::test::{run_test, TestStatus}; diff --git a/tooling/nargo/src/ops/test.rs b/tooling/nargo/src/ops/test.rs index f38dcad0c2f..0929739a6ab 100644 --- a/tooling/nargo/src/ops/test.rs +++ b/tooling/nargo/src/ops/test.rs @@ -16,7 +16,7 @@ pub enum TestStatus { pub fn run_test( blackbox_solver: &B, - context: &Context, + context: &mut Context, test_function: TestFunction, show_output: bool, foreign_call_resolver_url: Option<&str>, diff --git a/tooling/nargo_cli/src/cli/debug_cmd.rs b/tooling/nargo_cli/src/cli/debug_cmd.rs index ce89324a3b9..661996aeb4d 100644 --- a/tooling/nargo_cli/src/cli/debug_cmd.rs +++ b/tooling/nargo_cli/src/cli/debug_cmd.rs @@ -46,6 +46,10 @@ pub(crate) fn run( args: DebugCommand, config: NargoConfig, ) -> Result<(), CliError> { + // TODO: set clap default for flattened flag + let mut args = args.clone(); + args.compile_options.instrument_debug = true; + let toml_path = get_package_manifest(&config.program_dir)?; let selection = args.package.map_or(PackageSelection::DefaultOrAll, PackageSelection::Selected); let workspace = resolve_workspace_from_toml( diff --git a/tooling/nargo_cli/src/cli/export_cmd.rs b/tooling/nargo_cli/src/cli/export_cmd.rs index feaa55857e5..96b24796a2b 100644 --- a/tooling/nargo_cli/src/cli/export_cmd.rs +++ b/tooling/nargo_cli/src/cli/export_cmd.rs @@ -102,7 +102,7 @@ fn compile_exported_functions( exported_functions, |(function_name, function_id)| -> Result<(String, CompiledProgram), CompileError> { // TODO: We should to refactor how to deal with compilation errors to avoid this. - let program = compile_no_check(&context, compile_options, function_id, None, false) + let program = compile_no_check(&mut context, compile_options, function_id, None, false) .map_err(|error| vec![FileDiagnostic::from(error)]); let program = report_errors( diff --git a/tooling/nargo_cli/src/cli/test_cmd.rs b/tooling/nargo_cli/src/cli/test_cmd.rs index 9fee27b9172..503fd5afdd4 100644 --- a/tooling/nargo_cli/src/cli/test_cmd.rs +++ b/tooling/nargo_cli/src/cli/test_cmd.rs @@ -160,7 +160,7 @@ fn run_tests( let test_status = run_test( blackbox_solver, - &context, + &mut context, test_function, show_output, foreign_call_resolver_url, From 5170d2fa1efb99fa4989bd86e75756371d8bc40f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Mon, 22 Jan 2024 12:37:13 -0500 Subject: [PATCH 02/19] feat: Add debugging instrumentation to files in the package debugged --- compiler/noirc_frontend/src/hir/mod.rs | 31 ++----------------- tooling/nargo/src/lib.rs | 1 - tooling/nargo/src/ops/compile.rs | 22 +++++++++++++ tooling/nargo/src/ops/mod.rs | 4 ++- tooling/nargo_cli/src/cli/dap_cmd.rs | 11 +++++-- tooling/nargo_cli/src/cli/debug_cmd.rs | 43 ++++++++++++++++++++++++-- 6 files changed, 76 insertions(+), 36 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/mod.rs b/compiler/noirc_frontend/src/hir/mod.rs index acd1bf982ed..d63d2cbe4a6 100644 --- a/compiler/noirc_frontend/src/hir/mod.rs +++ b/compiler/noirc_frontend/src/hir/mod.rs @@ -8,10 +8,10 @@ use crate::debug::DebugState; use crate::graph::{CrateGraph, CrateId}; use crate::hir_def::function::FuncMeta; use crate::node_interner::{FuncId, NodeInterner, StructId}; -use crate::parser::{ParserError, SortedModule}; +use crate::parser::ParserError; use crate::ParsedModule; -use def_map::{parse_file, Contract, CrateDefMap}; -use fm::{FileId, FileManager}; +use def_map::{Contract, CrateDefMap}; +use fm::FileManager; use noirc_errors::Location; use std::borrow::Cow; use std::collections::{BTreeMap, HashMap}; @@ -32,9 +32,7 @@ pub struct Context<'file_manager, 'parsed_files> { // is read-only however, once it has been passed to the Context. pub file_manager: Cow<'file_manager, FileManager>, - pub root_crate_id: CrateId, pub debug_state: DebugState, - pub instrument_debug: bool, /// A map of each file that already has been visited from a prior `mod foo;` declaration. /// This is used to issue an error if a second `mod foo;` is declared to the same file. @@ -63,9 +61,7 @@ impl Context<'_, '_> { visited_files: BTreeMap::new(), crate_graph: CrateGraph::default(), file_manager: Cow::Owned(file_manager), - root_crate_id: CrateId::Dummy, debug_state: DebugState::default(), - instrument_debug: false, parsed_files: Cow::Owned(parsed_files), } } @@ -80,9 +76,7 @@ impl Context<'_, '_> { visited_files: BTreeMap::new(), crate_graph: CrateGraph::default(), file_manager: Cow::Borrowed(file_manager), - root_crate_id: CrateId::Dummy, debug_state: DebugState::default(), - instrument_debug: false, parsed_files: Cow::Borrowed(parsed_files), } } @@ -248,23 +242,4 @@ impl Context<'_, '_> { fn module(&self, module_id: def_map::ModuleId) -> &def_map::ModuleData { module_id.module(&self.def_maps) } - - /// Given a FileId, fetch the File, from the FileManager and parse its content, - /// applying sorting and debug transforms if debug mode is enabled. - pub fn parse_file( - &mut self, - file_id: FileId, - crate_id: CrateId, - ) -> (SortedModule, Vec) { - let (mut ast, parsing_errors) = parse_file(&self.file_manager, file_id); - - if self.instrument_debug && crate_id == self.root_crate_id { - self.debug_state.insert_symbols(&mut ast); - } - (ast.into_sorted(), parsing_errors) - } - - pub fn get_root_id(&self, crate_id: CrateId) -> FileId { - self.crate_graph[crate_id].root_file_id - } } diff --git a/tooling/nargo/src/lib.rs b/tooling/nargo/src/lib.rs index 056e92e8034..0fdff8b202f 100644 --- a/tooling/nargo/src/lib.rs +++ b/tooling/nargo/src/lib.rs @@ -119,7 +119,6 @@ pub fn prepare_package<'file_manager, 'parsed_files>( let mut context = Context::from_ref_file_manager(file_manager, parsed_files); let crate_id = prepare_crate(&mut context, &package.entry_path); - context.root_crate_id = crate_id; prepare_dependencies(&mut context, crate_id, &package.dependencies); diff --git a/tooling/nargo/src/ops/compile.rs b/tooling/nargo/src/ops/compile.rs index 866bfe39d7b..247e731bb1a 100644 --- a/tooling/nargo/src/ops/compile.rs +++ b/tooling/nargo/src/ops/compile.rs @@ -1,6 +1,7 @@ use acvm::ExpressionWidth; use fm::FileManager; use noirc_driver::{CompilationResult, CompileOptions, CompiledContract, CompiledProgram}; +use noirc_frontend::debug::DebugState; use noirc_frontend::hir::ParsedFiles; use crate::errors::CompileError; @@ -82,8 +83,29 @@ pub fn compile_program( compile_options: &CompileOptions, expression_width: ExpressionWidth, cached_program: Option, +) -> CompilationResult { + compile_program_with_debug_state( + file_manager, + parsed_files, + package, + compile_options, + expression_width, + cached_program, + DebugState::default(), + ) +} + +pub fn compile_program_with_debug_state( + file_manager: &FileManager, + parsed_files: &ParsedFiles, + package: &Package, + compile_options: &CompileOptions, + expression_width: ExpressionWidth, + cached_program: Option, + debug_state: DebugState, ) -> CompilationResult { let (mut context, crate_id) = prepare_package(file_manager, parsed_files, package); + context.debug_state = debug_state; let (program, warnings) = noirc_driver::compile_main(&mut context, crate_id, compile_options, cached_program)?; diff --git a/tooling/nargo/src/ops/mod.rs b/tooling/nargo/src/ops/mod.rs index f19a4057ecd..d960a7a9f45 100644 --- a/tooling/nargo/src/ops/mod.rs +++ b/tooling/nargo/src/ops/mod.rs @@ -1,4 +1,6 @@ -pub use self::compile::{compile_contract, compile_program, compile_workspace}; +pub use self::compile::{ + compile_contract, compile_program, compile_program_with_debug_state, compile_workspace, +}; pub use self::execute::execute_circuit; pub use self::foreign_calls::{DefaultForeignCallExecutor, ForeignCall, ForeignCallExecutor}; pub use self::optimize::{optimize_contract, optimize_program}; diff --git a/tooling/nargo_cli/src/cli/dap_cmd.rs b/tooling/nargo_cli/src/cli/dap_cmd.rs index c028545c11c..ea89183584b 100644 --- a/tooling/nargo_cli/src/cli/dap_cmd.rs +++ b/tooling/nargo_cli/src/cli/dap_cmd.rs @@ -2,7 +2,7 @@ use acvm::acir::native_types::WitnessMap; use backend_interface::Backend; use clap::Args; use nargo::constants::PROVER_INPUT_FILE; -use nargo::ops::compile_program; +use nargo::ops::compile_program_with_debug_state; use nargo::workspace::Workspace; use nargo::{insert_all_files_for_workspace_into_file_manager, parse_all}; use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection}; @@ -23,6 +23,7 @@ use dap::types::Capabilities; use serde_json::Value; use super::compile_cmd::report_errors; +use super::debug_cmd::instrument_package_files; use super::fs::inputs::read_inputs_from_file; use crate::errors::CliError; @@ -71,16 +72,20 @@ fn load_and_compile_project( let mut workspace_file_manager = file_manager_with_stdlib(std::path::Path::new("")); insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager); - let parsed_files = parse_all(&workspace_file_manager); + let mut parsed_files = parse_all(&workspace_file_manager); let compile_options = CompileOptions::default(); - let compilation_result = compile_program( + let debug_state = + instrument_package_files(&mut parsed_files, &workspace_file_manager, package); + + let compilation_result = compile_program_with_debug_state( &workspace_file_manager, &parsed_files, package, &compile_options, expression_width, None, + debug_state, ); let compiled_program = report_errors( diff --git a/tooling/nargo_cli/src/cli/debug_cmd.rs b/tooling/nargo_cli/src/cli/debug_cmd.rs index 661996aeb4d..4031ca11409 100644 --- a/tooling/nargo_cli/src/cli/debug_cmd.rs +++ b/tooling/nargo_cli/src/cli/debug_cmd.rs @@ -4,9 +4,10 @@ use acvm::acir::native_types::WitnessMap; use bn254_blackbox_solver::Bn254BlackBoxSolver; use clap::Args; +use fm::FileManager; use nargo::artifacts::debug::DebugArtifact; use nargo::constants::PROVER_INPUT_FILE; -use nargo::ops::compile_program; +use nargo::ops::compile_program_with_debug_state; use nargo::package::Package; use nargo::{insert_all_files_for_workspace_into_file_manager, parse_all}; use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection}; @@ -15,7 +16,9 @@ use noirc_abi::InputMap; use noirc_driver::{ file_manager_with_stdlib, CompileOptions, CompiledProgram, NOIR_ARTIFACT_VERSION_STRING, }; +use noirc_frontend::debug::DebugState; use noirc_frontend::graph::CrateName; +use noirc_frontend::hir::ParsedFiles; use super::compile_cmd::report_errors; use super::fs::{inputs::read_inputs_from_file, witness::save_witness_to_dir}; @@ -62,7 +65,7 @@ pub(crate) fn run( let mut workspace_file_manager = file_manager_with_stdlib(std::path::Path::new("")); insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager); - let parsed_files = parse_all(&workspace_file_manager); + let mut parsed_files = parse_all(&workspace_file_manager); let Some(package) = workspace.into_iter().find(|p| p.is_binary()) else { println!( @@ -71,13 +74,17 @@ pub(crate) fn run( return Ok(()); }; - let compilation_result = compile_program( + let debug_state = + instrument_package_files(&mut parsed_files, &workspace_file_manager, package); + + let compilation_result = compile_program_with_debug_state( &workspace_file_manager, &parsed_files, package, &args.compile_options, expression_width, None, + debug_state, ); let compiled_program = report_errors( @@ -90,6 +97,36 @@ pub(crate) fn run( run_async(package, compiled_program, &args.prover_name, &args.witness_name, target_dir) } +/// Add debugging instrumentation to all parsed files belonging to the package +/// being compiled +pub(crate) fn instrument_package_files( + parsed_files: &mut ParsedFiles, + file_manager: &FileManager, + package: &Package, +) -> DebugState { + // Start off at the entry path and read all files in the parent directory. + let entry_path_parent = package + .entry_path + .parent() + .unwrap_or_else(|| panic!("The entry path is expected to be a single file within a directory and so should have a parent {:?}", package.entry_path)) + .clone(); + + let mut debug_state = DebugState::default(); + + for (file_id, parsed_file) in parsed_files.iter_mut() { + let file_path = + file_manager.path(*file_id).expect("Parsed file ID not found in file manager"); + for ancestor in file_path.ancestors() { + if ancestor == entry_path_parent { + // file is in package + debug_state.insert_symbols(&mut parsed_file.0); + } + } + } + + debug_state +} + fn run_async( package: &Package, program: CompiledProgram, From 93d4c80fbfd96f02c3825cb45a658dfdad56b5f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Mon, 22 Jan 2024 12:37:46 -0500 Subject: [PATCH 03/19] feat: Hide debug module stack frames in the debugger --- tooling/debugger/src/repl.rs | 3 ++- tooling/debugger/src/source_code_printer.rs | 6 +----- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/tooling/debugger/src/repl.rs b/tooling/debugger/src/repl.rs index 2112f7414fa..53c8a51c148 100644 --- a/tooling/debugger/src/repl.rs +++ b/tooling/debugger/src/repl.rs @@ -69,7 +69,8 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { ); } } - print_source_code_location(self.debug_artifact, &location); + let locations = self.context.get_source_location_for_opcode_location(&location); + print_source_code_location(self.debug_artifact, &locations); } } } diff --git a/tooling/debugger/src/source_code_printer.rs b/tooling/debugger/src/source_code_printer.rs index 35345037b5c..2c8d037dcaf 100644 --- a/tooling/debugger/src/source_code_printer.rs +++ b/tooling/debugger/src/source_code_printer.rs @@ -1,4 +1,3 @@ -use acvm::acir::circuit::OpcodeLocation; use codespan_reporting::files::Files; use nargo::artifacts::debug::DebugArtifact; use noirc_errors::Location; @@ -33,11 +32,8 @@ struct LocationPrintContext { // visual aids to highlight the location itself. pub(crate) fn print_source_code_location( debug_artifact: &DebugArtifact, - location: &OpcodeLocation, + locations: &[Location], ) { - let locations = debug_artifact.debug_symbols[0].opcode_location(location); - let Some(locations) = locations else { return; }; - let locations = locations.iter(); for loc in locations { From de3a78e2e5a27476c1459ef23d186defe966b266 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Mon, 22 Jan 2024 13:43:04 -0500 Subject: [PATCH 04/19] chore: Ignore debugger failing tests and use Brillig mode --- tooling/debugger/build.rs | 14 +++++++++++--- tooling/debugger/ignored-tests.txt | 17 +++++++++++++++++ tooling/debugger/src/source_code_printer.rs | 5 +---- tooling/debugger/tests/debug.rs | 2 +- tooling/nargo_cli/src/cli/dap_cmd.rs | 7 ++++--- tooling/nargo_cli/src/cli/debug_cmd.rs | 3 +-- 6 files changed, 35 insertions(+), 13 deletions(-) create mode 100644 tooling/debugger/ignored-tests.txt diff --git a/tooling/debugger/build.rs b/tooling/debugger/build.rs index cedeebcae86..26a8bc64b0e 100644 --- a/tooling/debugger/build.rs +++ b/tooling/debugger/build.rs @@ -1,3 +1,4 @@ +use std::collections::HashSet; use std::fs::File; use std::io::Write; use std::path::{Path, PathBuf}; @@ -6,9 +7,6 @@ use std::{env, fs}; const GIT_COMMIT: &&str = &"GIT_COMMIT"; fn main() { - // Rebuild if the tests have changed - println!("cargo:rerun-if-changed=tests"); - // Only use build_data if the environment variable isn't set // The environment variable is always set when working via Nix if std::env::var(GIT_COMMIT).is_err() { @@ -29,6 +27,11 @@ fn main() { }; let test_dir = root_dir.join("test_programs"); + // Rebuild if the tests have changed + println!("cargo:rerun-if-changed=tests"); + println!("cargo:rerun-if-changed=ignored-tests.txt"); + println!("cargo:rerun-if-changed={}", test_dir.as_os_str().to_str().unwrap()); + generate_debugger_tests(&mut test_file, &test_dir); } @@ -38,10 +41,13 @@ fn generate_debugger_tests(test_file: &mut File, test_data_dir: &Path) { let test_case_dirs = fs::read_dir(test_data_dir).unwrap().flatten().filter(|c| c.path().is_dir()); + let ignored_tests_contents = fs::read_to_string("ignored-tests.txt").unwrap(); + let ignored_tests = ignored_tests_contents.lines().collect::>(); for test_dir in test_case_dirs { let test_name = test_dir.file_name().into_string().expect("Directory can't be converted to string"); + let ignored = ignored_tests.contains(test_name.as_str()); if test_name.contains('-') { panic!( "Invalid test directory: {test_name}. Cannot include `-`, please convert to `_`" @@ -53,11 +59,13 @@ fn generate_debugger_tests(test_file: &mut File, test_data_dir: &Path) { test_file, r#" #[test] +{ignored} fn debug_{test_name}() {{ debugger_execution_success("{test_dir}"); }} "#, test_dir = test_dir.display(), + ignored = if ignored { "#[ignore]" } else { "" }, ) .expect("Could not write templated test file."); } diff --git a/tooling/debugger/ignored-tests.txt b/tooling/debugger/ignored-tests.txt new file mode 100644 index 00000000000..a9aeb44051f --- /dev/null +++ b/tooling/debugger/ignored-tests.txt @@ -0,0 +1,17 @@ +array_sort +assign_ex +bit_shifts_comptime +brillig_cow +brillig_nested_arrays +brillig_references +brillig_to_bytes_integration +debug_logs +double_verify_proof +modulus +nested_array_dynamic +nested_array_in_slice +nested_arrays_from_brillig +references +signed_comparison +simple_2d_array +to_bytes_integration diff --git a/tooling/debugger/src/source_code_printer.rs b/tooling/debugger/src/source_code_printer.rs index 2c8d037dcaf..e430a8f0330 100644 --- a/tooling/debugger/src/source_code_printer.rs +++ b/tooling/debugger/src/source_code_printer.rs @@ -30,10 +30,7 @@ struct LocationPrintContext { // Given a DebugArtifact and an OpcodeLocation, prints all the source code // locations the OpcodeLocation maps to, with some surrounding context and // visual aids to highlight the location itself. -pub(crate) fn print_source_code_location( - debug_artifact: &DebugArtifact, - locations: &[Location], -) { +pub(crate) fn print_source_code_location(debug_artifact: &DebugArtifact, locations: &[Location]) { let locations = locations.iter(); for loc in locations { diff --git a/tooling/debugger/tests/debug.rs b/tooling/debugger/tests/debug.rs index e8b17b8a7af..82872ce2739 100644 --- a/tooling/debugger/tests/debug.rs +++ b/tooling/debugger/tests/debug.rs @@ -27,7 +27,7 @@ mod tests { // Start debugger and test that it loads for the given program. dbg_session .execute( - &format!("{} debug --program-dir {}", nargo_bin, test_program_dir), + &format!("{} debug --program-dir {} --force-brillig", nargo_bin, test_program_dir), ".*\\Starting debugger.*", ) .expect("Could not start debugger"); diff --git a/tooling/nargo_cli/src/cli/dap_cmd.rs b/tooling/nargo_cli/src/cli/dap_cmd.rs index ea89183584b..008612a16a1 100644 --- a/tooling/nargo_cli/src/cli/dap_cmd.rs +++ b/tooling/nargo_cli/src/cli/dap_cmd.rs @@ -74,9 +74,10 @@ fn load_and_compile_project( insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager); let mut parsed_files = parse_all(&workspace_file_manager); - let compile_options = CompileOptions::default(); - let debug_state = - instrument_package_files(&mut parsed_files, &workspace_file_manager, package); + let compile_options = + CompileOptions { instrument_debug: true, force_brillig: true, ..CompileOptions::default() }; + + let debug_state = instrument_package_files(&mut parsed_files, &workspace_file_manager, package); let compilation_result = compile_program_with_debug_state( &workspace_file_manager, diff --git a/tooling/nargo_cli/src/cli/debug_cmd.rs b/tooling/nargo_cli/src/cli/debug_cmd.rs index 4031ca11409..e1fe9b673da 100644 --- a/tooling/nargo_cli/src/cli/debug_cmd.rs +++ b/tooling/nargo_cli/src/cli/debug_cmd.rs @@ -74,8 +74,7 @@ pub(crate) fn run( return Ok(()); }; - let debug_state = - instrument_package_files(&mut parsed_files, &workspace_file_manager, package); + let debug_state = instrument_package_files(&mut parsed_files, &workspace_file_manager, package); let compilation_result = compile_program_with_debug_state( &workspace_file_manager, From ca45375d897291a6d08b7b49e47537f4adc72f23 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Mon, 22 Jan 2024 14:51:34 -0500 Subject: [PATCH 05/19] chore: Refactor instrumentation patching code during monomorphization --- compiler/noirc_frontend/src/debug/mod.rs | 8 +- .../src/monomorphization/mod.rs | 230 +++++++++++------- 2 files changed, 141 insertions(+), 97 deletions(-) diff --git a/compiler/noirc_frontend/src/debug/mod.rs b/compiler/noirc_frontend/src/debug/mod.rs index 940bd4138b4..0484b6cf8b2 100644 --- a/compiler/noirc_frontend/src/debug/mod.rs +++ b/compiler/noirc_frontend/src/debug/mod.rs @@ -531,16 +531,16 @@ pub fn create_prologue_program(n: u32) -> String { format!( r#" #[oracle(__debug_member_assign_{n})] - unconstrained fn __debug_member_assign_oracle_{n}( + unconstrained fn __debug_oracle_member_assign_{n}( _var_id: u32, _value: T, {var_sig} ) {{}} - unconstrained fn __debug_member_assign_inner_{n}( + unconstrained fn __debug_inner_member_assign_{n}( var_id: u32, value: T, {var_sig} ) {{ - __debug_member_assign_oracle_{n}(var_id, value, {vars}); + __debug_oracle_member_assign_{n}(var_id, value, {vars}); }} pub fn __debug_member_assign_{n}(var_id: u32, value: T, {var_sig}) {{ - __debug_member_assign_inner_{n}(var_id, value, {vars}); + __debug_inner_member_assign_{n}(var_id, value, {vars}); }} "# diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index 6e0917cc17e..308483d405d 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -962,6 +962,133 @@ impl<'interner> Monomorphizer<'interner> { expr_id } + /// Update instrumentation code inserted on variable assignment. We need to + /// register the variable instance, its type and replace the temporary ID + /// (fe_var_id) with the ID of the registration. Multiple registrations of + /// the same variable are possible if using generic functions, hence the + /// temporary ID created when injecting the instrumentation code can map to + /// multiple IDs at runtime. + fn patch_debug_var_assign( + &mut self, + call: &HirCallExpression, + arguments: &mut [ast::Expression], + ) { + let hir_arguments = vecmap(&call.arguments, |id| self.interner.expression(id)); + let Some(HirExpression::Literal(HirLiteral::Integer(fe_var_id, _))) = hir_arguments.get(0) else { + unreachable!("Missing FE var ID in __debug_var_assign call"); + }; + let Some(HirExpression::Ident(HirIdent { id, .. })) = hir_arguments.get(1) else { + unreachable!("Missing value identifier in __debug_var_assign call"); + }; + + // update variable assignments + let var_def = self.interner.definition(*id); + let var_type = self.interner.id_type(call.arguments[1]); + let fe_var_id = fe_var_id.to_u128() as u32; + let var_id = if var_def.name != "__debug_expr" { + self.debug_types.insert_var(fe_var_id, &var_def.name, var_type) + } else { + self.debug_types.get_var_id(fe_var_id).unwrap() + }; + let interned_var_id = self.intern_var_id(var_id, &call.location); + arguments[0] = self.expr(interned_var_id); + } + + /// Update instrumentation code for a variable being dropped out of scope. + /// Given the fe_var_id we search for the last assigned runtime variable ID + /// and replace it instead. + fn patch_debug_var_drop( + &mut self, + call: &HirCallExpression, + arguments: &mut [ast::Expression], + ) { + let hir_arguments = vecmap(&call.arguments, |id| self.interner.expression(id)); + let Some(HirExpression::Literal(HirLiteral::Integer(fe_var_id, _))) = hir_arguments.get(0) else { + unreachable!("Missing FE var ID in __debug_var_drop call"); + }; + // update variable drops (ie. when the var goes out of scope) + let fe_var_id = fe_var_id.to_u128() as u32; + if let Some(var_id) = self.debug_types.get_var_id(fe_var_id) { + let interned_var_id = self.intern_var_id(var_id, &call.location); + arguments[0] = self.expr(interned_var_id); + } + } + + /// Update instrumentation code inserted when assigning to a member of an + /// existing variable. Same as above for replacing the fe_var_id, but also + /// we need to resolve the path and the type of the member being assigned. + /// For this last part, we need to resolve the mapping from field names in + /// structs to positions in the runtime tuple, since all structs are + /// replaced by tuples during compilation. + fn patch_debug_member_assign( + &mut self, + call: &HirCallExpression, + arguments: &mut [ast::Expression], + arity: usize, + ) { + let hir_arguments = vecmap(&call.arguments, |id| self.interner.expression(id)); + let Some(HirExpression::Literal(HirLiteral::Integer(fe_var_id, _))) = hir_arguments.get(0) else { + unreachable!("Missing FE var ID in __debug_member_assign call"); + }; + let Some(HirExpression::Ident(HirIdent { id, .. })) = hir_arguments.get(1) else { + unreachable!("Missing value identifier in __debug_member_assign call"); + }; + // update variable member assignments + let var_def_name = self.interner.definition(*id).name.clone(); + let var_type = self.interner.id_type(call.arguments[2]); + let fe_var_id = fe_var_id.to_u128() as u32; + + let mut cursor_type = self + .debug_types + .get_type(fe_var_id) + .unwrap_or_else(|| panic!("type not found for fe_var_id={fe_var_id}")) + .clone(); + for i in 0..arity { + if let Some(HirExpression::Literal(HirLiteral::Integer(fe_i, i_neg))) = + hir_arguments.get(2 + i) + { + let mut index = fe_i.to_i128(); + if *i_neg { + index = -index; + } + if index < 0 { + let index = index.unsigned_abs(); + let field_name = self + .debug_field_names + .get(&(index as u32)) + .unwrap_or_else(|| panic!("field name not available for {i:?}")); + let field_i = (get_field(&cursor_type, field_name) + .unwrap_or_else(|| panic!("failed to find field_name: {field_name}")) + as i128) + .unsigned_abs(); + cursor_type = next_type(&cursor_type, field_i as usize); + let index_id = self.interner.push_expr(HirExpression::Literal( + HirLiteral::Integer(field_i.into(), false), + )); + self.interner.push_expr_type(&index_id, Type::FieldElement); + self.interner.push_expr_location( + index_id, + call.location.span, + call.location.file, + ); + arguments[2 + i] = self.expr(index_id); + } else { + cursor_type = next_type(&cursor_type, 0); + } + } else { + cursor_type = next_type(&cursor_type, 0); + } + } + + let var_id = if &var_def_name != "__debug_expr" { + self.debug_types.insert_var(fe_var_id, &var_def_name, var_type) + } else { + self.debug_types.get_var_id(fe_var_id).unwrap() + }; + let interned_var_id = self.intern_var_id(var_id, &call.location); + arguments[0] = self.expr(interned_var_id); + } + fn function_call( &mut self, call: HirCallExpression, @@ -970,102 +1097,19 @@ impl<'interner> Monomorphizer<'interner> { let original_func = Box::new(self.expr(call.func)); let mut arguments = vecmap(&call.arguments, |id| self.expr(*id)); let hir_arguments = vecmap(&call.arguments, |id| self.interner.expression(id)); - if let ast::Expression::Ident(ast::Ident { name, .. }) = original_func.as_ref() { - if let ( - Some(HirExpression::Literal(HirLiteral::Integer(fe_var_id, _))), - Some(HirExpression::Ident(HirIdent { id, .. })), - true, - ) = (hir_arguments.get(0), hir_arguments.get(1), name == "__debug_var_assign") - { - // update variable assignments - let var_def = self.interner.definition(*id); - let var_type = self.interner.id_type(call.arguments[1]); - let fe_var_id = fe_var_id.to_u128() as u32; - let var_id = if var_def.name != "__debug_expr" { - self.debug_types.insert_var(fe_var_id, &var_def.name, var_type) - } else { - self.debug_types.get_var_id(fe_var_id).unwrap() - }; - let interned_var_id = self.intern_var_id(var_id, &call.location); - arguments[0] = self.expr(interned_var_id); - } else if let (Some(HirExpression::Literal(HirLiteral::Integer(fe_var_id, _))), true) = - (hir_arguments.get(0), name == "__debug_var_drop") - { - // update variable drops (ie. when the var goes out of scope) - let fe_var_id = fe_var_id.to_u128() as u32; - if let Some(var_id) = self.debug_types.get_var_id(fe_var_id) { - let interned_var_id = self.intern_var_id(var_id, &call.location); - arguments[0] = self.expr(interned_var_id); - } - } else if let ( - Some(HirExpression::Literal(HirLiteral::Integer(fe_var_id, _))), - Some(HirExpression::Ident(HirIdent { id, .. })), - true, - ) = ( - hir_arguments.get(0), - hir_arguments.get(1), - name.starts_with(DEBUG_MEMBER_ASSIGN_PREFIX), - ) { - // update variable member assignments - let var_def_name = self.interner.definition(*id).name.clone(); - let var_type = self.interner.id_type(call.arguments[2]); - let fe_var_id = fe_var_id.to_u128() as u32; - let arity = name[DEBUG_MEMBER_ASSIGN_PREFIX.len()..] - .parse::() - .expect("failed to parse member assign arity"); - - let mut cursor_type = self - .debug_types - .get_type(fe_var_id) - .unwrap_or_else(|| panic!("type not found for fe_var_id={fe_var_id}")) - .clone(); - for i in 0..arity { - if let Some(HirExpression::Literal(HirLiteral::Integer(fe_i, i_neg))) = - hir_arguments.get(2 + i) - { - let mut index = fe_i.to_i128(); - if *i_neg { - index = -index; - } - if index < 0 { - let index = index.unsigned_abs(); - let field_name = self - .debug_field_names - .get(&(index as u32)) - .unwrap_or_else(|| panic!("field name not available for {i:?}")); - let field_i = - (get_field(&cursor_type, field_name).unwrap_or_else(|| { - panic!("failed to find field_name: {field_name}") - }) as i128) - .unsigned_abs(); - cursor_type = next_type(&cursor_type, field_i as usize); - let index_id = self.interner.push_expr(HirExpression::Literal( - HirLiteral::Integer(field_i.into(), false), - )); - self.interner.push_expr_type(&index_id, Type::FieldElement); - self.interner.push_expr_location( - index_id, - call.location.span, - call.location.file, - ); - arguments[2 + i] = self.expr(index_id); - } else { - cursor_type = next_type(&cursor_type, 0); - } - } else { - cursor_type = next_type(&cursor_type, 0); - } - } - let var_id = if &var_def_name != "__debug_expr" { - self.debug_types.insert_var(fe_var_id, &var_def_name, var_type) - } else { - self.debug_types.get_var_id(fe_var_id).unwrap() - }; - let interned_var_id = self.intern_var_id(var_id, &call.location); - arguments[0] = self.expr(interned_var_id); + // patch instrumentation code inserted for debugging + if let ast::Expression::Ident(ast::Ident { name, .. }) = original_func.as_ref() { + if name == "__debug_var_assign" { + self.patch_debug_var_assign(&call, &mut arguments); + } else if name == "__debug_var_drop" { + self.patch_debug_var_drop(&call, &mut arguments); + } else if let Some(arity) = name.strip_prefix(DEBUG_MEMBER_ASSIGN_PREFIX) { + let arity = arity.parse::().expect("failed to parse member assign arity"); + self.patch_debug_member_assign(&call, &mut arguments, arity); } } + let return_type = self.interner.id_type(id); let return_type = self.convert_type(&return_type); From 6744c1d97d9fe391cd82d3f3fa180bfbfed1397f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Tue, 23 Jan 2024 01:55:10 -0500 Subject: [PATCH 06/19] feat: Only link the debug crate when instrumenting code for the debugger --- compiler/noirc_driver/src/lib.rs | 24 ++++++++++++++++-------- tooling/debugger/ignored-tests.txt | 2 -- tooling/nargo/src/ops/compile.rs | 5 ++++- 3 files changed, 20 insertions(+), 11 deletions(-) diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index 1d0e4d4ef66..c9c03089b84 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -109,6 +109,7 @@ pub fn file_manager_with_stdlib(root: &Path) -> FileManager { let mut file_manager = FileManager::new(root); add_stdlib_source_to_file_manager(&mut file_manager); + add_debug_source_to_file_manager(&mut file_manager); file_manager } @@ -123,7 +124,11 @@ fn add_stdlib_source_to_file_manager(file_manager: &mut FileManager) { for (path, source) in stdlib_paths_with_source { file_manager.add_file_with_source_canonical_path(Path::new(&path), source); } +} +/// Adds the source code of the debug crate needed to support instrumentation to +/// track variables values +fn add_debug_source_to_file_manager(file_manager: &mut FileManager) { // Adds the synthetic debug module for instrumentation into the file manager let path_to_debug_lib_file = Path::new(DEBUG_CRATE_NAME).join("lib.nr"); file_manager.add_file_with_contents(&path_to_debug_lib_file, &create_prologue_program(8)); @@ -141,23 +146,26 @@ pub fn prepare_crate(context: &mut Context, file_name: &Path) -> CrateId { .expect("stdlib file id is expected to be present"); let std_crate_id = context.crate_graph.add_stdlib(std_file_id); - let path_to_debug_lib_file = Path::new(DEBUG_CRATE_NAME).join("lib.nr"); - let debug_file_id = context - .file_manager - .name_to_id(path_to_debug_lib_file) - .expect("debug module is expected to be present"); - let debug_crate_id = context.crate_graph.add_crate(debug_file_id); - let root_file_id = context.file_manager.name_to_id(file_name.to_path_buf()).unwrap_or_else(|| panic!("files are expected to be added to the FileManager before reaching the compiler file_path: {file_name:?}")); let root_crate_id = context.crate_graph.add_crate_root(root_file_id); add_dep(context, root_crate_id, std_crate_id, STD_CRATE_NAME.parse().unwrap()); - add_dep(context, root_crate_id, debug_crate_id, DEBUG_CRATE_NAME.parse().unwrap()); root_crate_id } +pub fn link_to_debug_crate(context: &mut Context, root_crate_id: CrateId) { + let path_to_debug_lib_file = Path::new(DEBUG_CRATE_NAME).join("lib.nr"); + let debug_file_id = context + .file_manager + .name_to_id(path_to_debug_lib_file) + .expect("debug source is expected to be present in file manager"); + let debug_crate_id = context.crate_graph.add_crate(debug_file_id); + + add_dep(context, root_crate_id, debug_crate_id, DEBUG_CRATE_NAME.parse().unwrap()); +} + // Adds the file from the file system at `Path` to the crate graph pub fn prepare_dependency(context: &mut Context, file_name: &Path) -> CrateId { let root_file_id = context diff --git a/tooling/debugger/ignored-tests.txt b/tooling/debugger/ignored-tests.txt index a9aeb44051f..f84712e20e0 100644 --- a/tooling/debugger/ignored-tests.txt +++ b/tooling/debugger/ignored-tests.txt @@ -1,6 +1,5 @@ array_sort assign_ex -bit_shifts_comptime brillig_cow brillig_nested_arrays brillig_references @@ -12,6 +11,5 @@ nested_array_dynamic nested_array_in_slice nested_arrays_from_brillig references -signed_comparison simple_2d_array to_bytes_integration diff --git a/tooling/nargo/src/ops/compile.rs b/tooling/nargo/src/ops/compile.rs index 247e731bb1a..06f26906f97 100644 --- a/tooling/nargo/src/ops/compile.rs +++ b/tooling/nargo/src/ops/compile.rs @@ -1,6 +1,8 @@ use acvm::ExpressionWidth; use fm::FileManager; -use noirc_driver::{CompilationResult, CompileOptions, CompiledContract, CompiledProgram}; +use noirc_driver::{ + link_to_debug_crate, CompilationResult, CompileOptions, CompiledContract, CompiledProgram, +}; use noirc_frontend::debug::DebugState; use noirc_frontend::hir::ParsedFiles; @@ -105,6 +107,7 @@ pub fn compile_program_with_debug_state( debug_state: DebugState, ) -> CompilationResult { let (mut context, crate_id) = prepare_package(file_manager, parsed_files, package); + link_to_debug_crate(&mut context, crate_id); context.debug_state = debug_state; let (program, warnings) = From 260add6a0c7444bff934816e92ec9131a2d0622c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Tue, 23 Jan 2024 02:16:32 -0500 Subject: [PATCH 07/19] chore: Ignore two more tests for debugging which fail with assertions Both tests pass the debugger ok in ACIR mode, but not in Brillig mode. --- tooling/debugger/ignored-tests.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tooling/debugger/ignored-tests.txt b/tooling/debugger/ignored-tests.txt index f84712e20e0..a9aeb44051f 100644 --- a/tooling/debugger/ignored-tests.txt +++ b/tooling/debugger/ignored-tests.txt @@ -1,5 +1,6 @@ array_sort assign_ex +bit_shifts_comptime brillig_cow brillig_nested_arrays brillig_references @@ -11,5 +12,6 @@ nested_array_dynamic nested_array_in_slice nested_arrays_from_brillig references +signed_comparison simple_2d_array to_bytes_integration From 0c0848a6af3f59e3bafc0a86936ef9c5c2766098 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Mon, 29 Jan 2024 11:53:30 -0500 Subject: [PATCH 08/19] fix: Address code review comments and remove unneeded code --- compiler/fm/src/lib.rs | 15 ----- compiler/noirc_driver/src/lib.rs | 12 ++-- compiler/noirc_errors/src/debug_info.rs | 8 --- compiler/noirc_errors/src/position.rs | 4 -- compiler/noirc_frontend/src/debug/mod.rs | 76 +++++++++++------------- tooling/nargo/src/artifacts/debug.rs | 22 ++++--- tooling/nargo_cli/src/cli/debug_cmd.rs | 2 +- 7 files changed, 54 insertions(+), 85 deletions(-) diff --git a/compiler/fm/src/lib.rs b/compiler/fm/src/lib.rs index e7da3870d9c..bb080c62c0e 100644 --- a/compiler/fm/src/lib.rs +++ b/compiler/fm/src/lib.rs @@ -78,21 +78,6 @@ impl FileManager { Some(file_id) } - pub fn add_file_with_contents(&mut self, file_name: &Path, contents: &str) -> Option { - // Handle both relative file paths and std/lib virtual paths. - let resolved_path: PathBuf = file_name.to_path_buf(); - - // Check that the resolved path already exists in the file map, if it is, we return it. - if let Some(file_id) = self.path_to_id.get(&resolved_path) { - return Some(*file_id); - } - - // Otherwise we add the file - let file_id = self.file_map.add_file(resolved_path.clone().into(), String::from(contents)); - self.register_path(file_id, resolved_path); - Some(file_id) - } - fn register_path(&mut self, file_id: FileId, path: PathBuf) { let old_value = self.id_to_path.insert(file_id, path.clone()); assert!( diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index c9c03089b84..afda2a9a8f6 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -10,7 +10,7 @@ use noirc_abi::{AbiParameter, AbiType, ContractEvent}; use noirc_errors::{CustomDiagnostic, FileDiagnostic}; use noirc_evaluator::create_circuit; use noirc_evaluator::errors::RuntimeError; -use noirc_frontend::debug::create_prologue_program; +use noirc_frontend::debug::build_debug_crate_file; use noirc_frontend::graph::{CrateId, CrateName}; use noirc_frontend::hir::def_map::{Contract, CrateDefMap}; use noirc_frontend::hir::Context; @@ -131,7 +131,8 @@ fn add_stdlib_source_to_file_manager(file_manager: &mut FileManager) { fn add_debug_source_to_file_manager(file_manager: &mut FileManager) { // Adds the synthetic debug module for instrumentation into the file manager let path_to_debug_lib_file = Path::new(DEBUG_CRATE_NAME).join("lib.nr"); - file_manager.add_file_with_contents(&path_to_debug_lib_file, &create_prologue_program(8)); + file_manager + .add_file_with_source_canonical_path(&path_to_debug_lib_file, build_debug_crate_file()); } /// Adds the file from the file system at `Path` to the crate graph as a root file @@ -157,12 +158,7 @@ pub fn prepare_crate(context: &mut Context, file_name: &Path) -> CrateId { pub fn link_to_debug_crate(context: &mut Context, root_crate_id: CrateId) { let path_to_debug_lib_file = Path::new(DEBUG_CRATE_NAME).join("lib.nr"); - let debug_file_id = context - .file_manager - .name_to_id(path_to_debug_lib_file) - .expect("debug source is expected to be present in file manager"); - let debug_crate_id = context.crate_graph.add_crate(debug_file_id); - + let debug_crate_id = prepare_dependency(context, &path_to_debug_lib_file); add_dep(context, root_crate_id, debug_crate_id, DEBUG_CRATE_NAME.parse().unwrap()); } diff --git a/compiler/noirc_errors/src/debug_info.rs b/compiler/noirc_errors/src/debug_info.rs index 63e0a645dc9..842fcab75af 100644 --- a/compiler/noirc_errors/src/debug_info.rs +++ b/compiler/noirc_errors/src/debug_info.rs @@ -1,6 +1,5 @@ use acvm::acir::circuit::OpcodeLocation; use acvm::compiler::AcirTransformationMap; -use fm::FileId; use base64::Engine; use flate2::read::DeflateDecoder; @@ -112,13 +111,6 @@ impl DebugInfo { counted_opcodes } - pub fn get_file_ids(&self) -> Vec { - self.locations - .values() - .filter_map(|call_stack| call_stack.last().map(|location| location.file)) - .collect() - } - pub fn serialize_compressed_base64_json( debug_info: &DebugInfo, s: S, diff --git a/compiler/noirc_errors/src/position.rs b/compiler/noirc_errors/src/position.rs index c5eacefe9a9..007ec58ca27 100644 --- a/compiler/noirc_errors/src/position.rs +++ b/compiler/noirc_errors/src/position.rs @@ -86,10 +86,6 @@ impl Span { self.0.end().into() } - pub fn build_from_str(s: &str) -> Span { - Span(ByteSpan::from_str(s)) - } - pub fn contains(&self, other: &Span) -> bool { self.start() <= other.start() && self.end() >= other.end() } diff --git a/compiler/noirc_frontend/src/debug/mod.rs b/compiler/noirc_frontend/src/debug/mod.rs index 0484b6cf8b2..7ce64d84228 100644 --- a/compiler/noirc_frontend/src/debug/mod.rs +++ b/compiler/noirc_frontend/src/debug/mod.rs @@ -8,6 +8,8 @@ use noirc_errors::{Span, Spanned}; use std::collections::HashMap; use std::collections::VecDeque; +const MAX_MEMBER_ASSIGN_DEPTH: usize = 8; + #[derive(Debug, Clone)] pub struct DebugState { pub variables: HashMap, // var_id => var_name @@ -15,7 +17,6 @@ pub struct DebugState { next_var_id: u32, next_field_name_id: u32, scope: Vec>, // var_name => var_id - pub enabled: bool, } impl Default for DebugState { @@ -26,7 +27,6 @@ impl Default for DebugState { scope: vec![], next_var_id: 0, next_field_name_id: 1, - enabled: true, // TODO } } } @@ -70,18 +70,16 @@ impl DebugState { .map(|(var_id, id, _is_mut)| self.wrap_assign_var(*var_id, id_expr(id))) .collect(); - self.walk_scope(&mut f.body.0); + self.walk_scope(&mut f.body.0, f.span); - // prapend fn params: + // prepend fn params: f.body.0 = vec![set_fn_params, f.body.0.clone()].concat(); } // Modify a vector of statements in-place, adding instrumentation for sets and drops. // This function will consume a scope level. - fn walk_scope(&mut self, statements: &mut Vec) { - let end_span = statements - .last() - .map_or(none_span(), |statement| Span::empty(statement.span.end() + 1)); + fn walk_scope(&mut self, statements: &mut Vec, span: Span) { + let end_span = Span::empty(span.end()); statements.iter_mut().for_each(|stmt| self.walk_statement(stmt)); @@ -90,9 +88,9 @@ impl DebugState { ast::Statement { kind: ast::StatementKind::Expression(ast::Expression { kind: ast::ExpressionKind::Literal(ast::Literal::Unit), - span: none_span(), + span: end_span, }), - span: none_span(), + span: end_span, }, vec![], )); @@ -104,11 +102,11 @@ impl DebugState { vec![match &ret_stmt.kind { ast::StatementKind::Expression(ret_expr) => ast::Statement { kind: ast::StatementKind::Let(ast::LetStatement { - pattern: ast::Pattern::Identifier(ident("__debug_expr", none_span())), + pattern: ast::Pattern::Identifier(ident("__debug_expr", ret_expr.span)), r#type: ast::UnresolvedType::unspecified(), expression: ret_expr.clone(), }), - span: none_span(), + span: ret_expr.span, }, _ => ret_stmt.clone(), }], @@ -124,20 +122,20 @@ impl DebugState { ast::StatementKind::Expression(_ret_expr) => ast::Statement { kind: ast::StatementKind::Expression(ast::Expression { kind: ast::ExpressionKind::Variable(ast::Path { - segments: vec![ident("__debug_expr", none_span())], + segments: vec![ident("__debug_expr", ret_stmt.span)], kind: PathKind::Plain, - span: none_span(), + span: ret_stmt.span, }), - span: none_span(), + span: ret_stmt.span, }), - span: none_span(), + span: ret_stmt.span, }, _ => ast::Statement { kind: ast::StatementKind::Expression(ast::Expression { kind: ast::ExpressionKind::Literal(ast::Literal::Unit), - span: none_span(), + span: ret_stmt.span, }), - span: none_span(), + span: ret_stmt.span, }, }], ] @@ -145,9 +143,6 @@ impl DebugState { } pub fn insert_symbols(&mut self, module: &mut ParsedModule) { - if !self.enabled { - return; - } module.items.iter_mut().for_each(|item| { if let Item { kind: ItemKind::Function(f), .. } = item { self.walk_fn(&mut f.def); @@ -196,26 +191,27 @@ impl DebugState { expr: &ast::Expression, ) -> ast::Statement { let arity = indexes.len(); + if arity > MAX_MEMBER_ASSIGN_DEPTH { + unreachable!("Assignment to member exceeds maximum depth for debugging"); + } + let span = expr.span; let kind = ast::ExpressionKind::Call(Box::new(ast::CallExpression { func: Box::new(ast::Expression { kind: ast::ExpressionKind::Variable(ast::Path { - segments: vec![ident(&format!["__debug_member_assign_{arity}"], none_span())], + segments: vec![ident(&format!["__debug_member_assign_{arity}"], span)], kind: PathKind::Plain, - span: none_span(), + span, }), - span: none_span(), + span, }), arguments: [ - vec![uint_expr(var_id as u128, none_span())], + vec![uint_expr(var_id as u128, span)], vec![expr.clone()], indexes.iter().rev().cloned().collect(), ] .concat(), })); - ast::Statement { - kind: ast::StatementKind::Semi(ast::Expression { kind, span: none_span() }), - span: none_span(), - } + ast::Statement { kind: ast::StatementKind::Semi(ast::Expression { kind, span }), span } } fn wrap_let_statement(&mut self, let_stmt: &ast::LetStatement, span: &Span) -> ast::Statement { @@ -328,7 +324,7 @@ impl DebugState { ast::LValue::MemberAccess { object, field_name } => { cursor = object; let field_name_id = self.insert_field_name(&field_name.0.contents); - indexes.push(sint_expr(-(field_name_id as i128))); + indexes.push(sint_expr(-(field_name_id as i128), expression_span)); } ast::LValue::Index { index, array } => { cursor = array; @@ -369,7 +365,7 @@ impl DebugState { match &mut expr.kind { ast::ExpressionKind::Block(ast::BlockExpression(ref mut statements)) => { self.scope.push(HashMap::default()); - self.walk_scope(statements); + self.walk_scope(statements, expr.span); } ast::ExpressionKind::Prefix(prefix_expr) => { self.walk_expr(&mut prefix_expr.rhs); @@ -434,7 +430,7 @@ impl DebugState { let var_id = self.insert_var(var_name); let set_stmt = self.wrap_assign_var(var_id, id_expr(&for_stmt.identifier)); - let drop_stmt = self.wrap_drop_var(var_id, Span::empty(for_stmt.span.end() + 1)); + let drop_stmt = self.wrap_drop_var(var_id, Span::empty(for_stmt.span.end())); self.walk_expr(&mut for_stmt.block); for_stmt.block = ast::Expression { @@ -442,11 +438,11 @@ impl DebugState { set_stmt, ast::Statement { kind: ast::StatementKind::Semi(for_stmt.block.clone()), - span: none_span(), + span: for_stmt.block.span, }, drop_stmt, ])), - span: none_span(), + span: for_stmt.span, }; } @@ -492,7 +488,7 @@ impl DebugState { } } -pub fn create_prologue_program(n: u32) -> String { +pub fn build_debug_crate_file() -> String { [ r#" #[oracle(__debug_var_assign)] @@ -523,7 +519,7 @@ pub fn create_prologue_program(n: u32) -> String { } "# .to_string(), - (1..=n) + (1..=MAX_MEMBER_ASSIGN_DEPTH) .map(|n| { let var_sig = (0..n).map(|i| format!["_v{i}: Field"]).collect::>().join(", "); @@ -598,13 +594,9 @@ fn uint_expr(x: u128, span: Span) -> ast::Expression { } } -fn sint_expr(x: i128) -> ast::Expression { +fn sint_expr(x: i128, span: Span) -> ast::Expression { ast::Expression { kind: ast::ExpressionKind::Literal(ast::Literal::Integer(x.abs().into(), x < 0)), - span: none_span(), + span, } } - -fn none_span() -> Span { - Span::build_from_str("") -} diff --git a/tooling/nargo/src/artifacts/debug.rs b/tooling/nargo/src/artifacts/debug.rs index d246307ff73..f0cf84bdfc8 100644 --- a/tooling/nargo/src/artifacts/debug.rs +++ b/tooling/nargo/src/artifacts/debug.rs @@ -24,15 +24,22 @@ impl DebugArtifact { pub fn new(debug_symbols: Vec, file_manager: &FileManager) -> Self { let mut file_map = BTreeMap::new(); - let file_ids: BTreeSet = - debug_symbols.iter().flat_map(|debug_info| debug_info.get_file_ids()).collect(); + let files_with_debug_symbols: BTreeSet = debug_symbols + .iter() + .flat_map(|function_symbols| { + function_symbols + .locations + .values() + .flat_map(|call_stack| call_stack.iter().map(|location| location.file)) + }) + .collect(); - for file_id in file_ids.iter() { - 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"); + 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"); file_map.insert( - *file_id, + file_id, DebugFile { source: file_source.to_string(), path: file_path.to_path_buf() }, ); } @@ -80,7 +87,8 @@ impl DebugArtifact { let line_index = self.line_index(location.file, location_start)?; let line_span = self.line_range(location.file, line_index)?; - let line_length = line_span.end - (line_span.start + 1); + let line_length = + if line_span.end > line_span.start { line_span.end - (line_span.start + 1) } else { 0 }; let start_in_line = location_start - line_span.start; // The location might continue beyond the line, diff --git a/tooling/nargo_cli/src/cli/debug_cmd.rs b/tooling/nargo_cli/src/cli/debug_cmd.rs index fa5fa7c70d4..d21b3c54b10 100644 --- a/tooling/nargo_cli/src/cli/debug_cmd.rs +++ b/tooling/nargo_cli/src/cli/debug_cmd.rs @@ -49,7 +49,7 @@ pub(crate) fn run( args: DebugCommand, config: NargoConfig, ) -> Result<(), CliError> { - // TODO: set clap default for flattened flag + // Override clap default for compiler option flag let mut args = args.clone(); args.compile_options.instrument_debug = true; From 8ebf01eb625ba67cacad469ae4809344b9b16c48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Tue, 30 Jan 2024 19:21:14 -0500 Subject: [PATCH 09/19] fix: First pass addressing code review comments --- compiler/noirc_driver/src/lib.rs | 7 +- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 6 +- compiler/noirc_frontend/src/debug/mod.rs | 257 +++++++++--------- compiler/noirc_frontend/src/hir/mod.rs | 2 - .../src/monomorphization/ast.rs | 6 +- .../src/monomorphization/debug_types.rs | 6 +- .../src/monomorphization/mod.rs | 68 ++--- 7 files changed, 153 insertions(+), 199 deletions(-) diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index afda2a9a8f6..8e76b7185cb 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -418,11 +418,8 @@ pub fn compile_no_check( force_compile: bool, ) -> Result { let program = if options.instrument_debug { - monomorphize_debug( - main_function, - &mut context.def_interner, - &context.debug_state.field_names, - ) + let field_names = context.debug_state.field_names.clone(); + monomorphize_debug(main_function, &mut context.def_interner, field_names) } else { monomorphize(main_function, &mut context.def_interner) }; diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index eca25905583..c10ee42a74d 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -1332,10 +1332,8 @@ impl Context { // This conversion is for debugging support only, to allow the // debugging instrumentation code to work. Taking the reference // of a function in ACIR is useless. - AcirValue::Var( - self.acir_context.add_constant(function_id.to_usize()), - AcirType::field(), - ) + let id = self.acir_context.add_constant(function_id.to_usize()); + AcirValue::Var(id, AcirType::field()) } Value::ForeignFunction(_) => unimplemented!( "Oracle calls directly in constrained functions are not yet available." diff --git a/compiler/noirc_frontend/src/debug/mod.rs b/compiler/noirc_frontend/src/debug/mod.rs index 7ce64d84228..9b0b131869e 100644 --- a/compiler/noirc_frontend/src/debug/mod.rs +++ b/compiler/noirc_frontend/src/debug/mod.rs @@ -51,95 +51,87 @@ impl DebugState { field_name_id } - fn walk_fn(&mut self, f: &mut ast::FunctionDefinition) { + fn walk_fn(&mut self, func: &mut ast::FunctionDefinition) { self.scope.push(HashMap::default()); - let pvars: Vec<(u32, ast::Ident, bool)> = f + let set_fn_params = func .parameters .iter() .flat_map(|param| { pattern_vars(¶m.pattern) .iter() - .map(|(id, is_mut)| (self.insert_var(&id.0.contents), id.clone(), *is_mut)) - .collect::>() + .map(|(id, _is_mut)| { + let var_id = self.insert_var(&id.0.contents); + build_assign_var_stmt(var_id, id_expr(id)) + }) + .collect::>() }) .collect(); - let set_fn_params = pvars - .iter() - .map(|(var_id, id, _is_mut)| self.wrap_assign_var(*var_id, id_expr(id))) - .collect(); - - self.walk_scope(&mut f.body.0, f.span); + self.walk_scope(&mut func.body.0, func.span); // prepend fn params: - f.body.0 = vec![set_fn_params, f.body.0.clone()].concat(); + func.body.0 = vec![set_fn_params, func.body.0.clone()].concat(); } // Modify a vector of statements in-place, adding instrumentation for sets and drops. // This function will consume a scope level. fn walk_scope(&mut self, statements: &mut Vec, span: Span) { - let end_span = Span::empty(span.end()); - statements.iter_mut().for_each(|stmt| self.walk_statement(stmt)); - let (ret_stmt, fn_body) = - statements.split_last().map(|(e, b)| (e.clone(), b.to_vec())).unwrap_or(( - ast::Statement { - kind: ast::StatementKind::Expression(ast::Expression { - kind: ast::ExpressionKind::Literal(ast::Literal::Unit), - span: end_span, - }), - span: end_span, - }, - vec![], - )); - - *statements = vec![ - // copy body minus the return expr: - fn_body, - // assign return expr to __debug_expr: - vec![match &ret_stmt.kind { - ast::StatementKind::Expression(ret_expr) => ast::Statement { + // extract and save the return value from the scope if there is one + let ret_stmt = statements.pop(); + let has_ret_expr = match ret_stmt { + None => false, + Some(ast::Statement { kind: ast::StatementKind::Expression(ret_expr), .. }) => { + let save_ret_expr = ast::Statement { kind: ast::StatementKind::Let(ast::LetStatement { pattern: ast::Pattern::Identifier(ident("__debug_expr", ret_expr.span)), r#type: ast::UnresolvedType::unspecified(), expression: ret_expr.clone(), }), span: ret_expr.span, - }, - _ => ret_stmt.clone(), - }], - // drop fn params: - self.scope - .pop() - .unwrap_or(HashMap::default()) - .values() - .map(|var_id| self.wrap_drop_var(*var_id, end_span)) - .collect(), - // return the __debug_expr value: - vec![match &ret_stmt.kind { - ast::StatementKind::Expression(_ret_expr) => ast::Statement { - kind: ast::StatementKind::Expression(ast::Expression { - kind: ast::ExpressionKind::Variable(ast::Path { - segments: vec![ident("__debug_expr", ret_stmt.span)], - kind: PathKind::Plain, - span: ret_stmt.span, - }), - span: ret_stmt.span, - }), - span: ret_stmt.span, - }, - _ => ast::Statement { - kind: ast::StatementKind::Expression(ast::Expression { - kind: ast::ExpressionKind::Literal(ast::Literal::Unit), - span: ret_stmt.span, + }; + statements.push(save_ret_expr); + true + } + Some(ret_stmt) => { + // not an expression, so leave it untouched + statements.push(ret_stmt); + false + } + }; + + let span = Span::empty(span.end()); + + // drop scope variables + let scope_vars = self.scope.pop().unwrap_or(HashMap::default()); + let drop_vars_stmts = scope_vars.values().map(|var_id| build_drop_var_stmt(*var_id, span)); + statements.extend(drop_vars_stmts); + + // return the saved value in __debug_expr, or unit otherwise + let last_stmt = if has_ret_expr { + ast::Statement { + kind: ast::StatementKind::Expression(ast::Expression { + kind: ast::ExpressionKind::Variable(ast::Path { + segments: vec![ident("__debug_expr", span)], + kind: PathKind::Plain, + span, }), - span: ret_stmt.span, - }, - }], - ] - .concat(); + span, + }), + span, + } + } else { + ast::Statement { + kind: ast::StatementKind::Expression(ast::Expression { + kind: ast::ExpressionKind::Literal(ast::Literal::Unit), + span, + }), + span, + } + }; + statements.push(last_stmt); } pub fn insert_symbols(&mut self, module: &mut ParsedModule) { @@ -153,68 +145,7 @@ impl DebugState { self.insert_state_set_oracle(module, 8); } - fn wrap_assign_var(&mut self, var_id: u32, expr: ast::Expression) -> ast::Statement { - let span = expr.span; - let kind = ast::ExpressionKind::Call(Box::new(ast::CallExpression { - func: Box::new(ast::Expression { - kind: ast::ExpressionKind::Variable(ast::Path { - segments: vec![ident("__debug_var_assign", span)], - kind: PathKind::Plain, - span, - }), - span, - }), - arguments: vec![uint_expr(var_id as u128, span), expr], - })); - ast::Statement { kind: ast::StatementKind::Semi(ast::Expression { kind, span }), span } - } - - fn wrap_drop_var(&mut self, var_id: u32, span: Span) -> ast::Statement { - let kind = ast::ExpressionKind::Call(Box::new(ast::CallExpression { - func: Box::new(ast::Expression { - kind: ast::ExpressionKind::Variable(ast::Path { - segments: vec![ident("__debug_var_drop", span)], - kind: PathKind::Plain, - span, - }), - span, - }), - arguments: vec![uint_expr(var_id as u128, span)], - })); - ast::Statement { kind: ast::StatementKind::Semi(ast::Expression { kind, span }), span } - } - - fn wrap_assign_member( - &mut self, - var_id: u32, - indexes: &[ast::Expression], - expr: &ast::Expression, - ) -> ast::Statement { - let arity = indexes.len(); - if arity > MAX_MEMBER_ASSIGN_DEPTH { - unreachable!("Assignment to member exceeds maximum depth for debugging"); - } - let span = expr.span; - let kind = ast::ExpressionKind::Call(Box::new(ast::CallExpression { - func: Box::new(ast::Expression { - kind: ast::ExpressionKind::Variable(ast::Path { - segments: vec![ident(&format!["__debug_member_assign_{arity}"], span)], - kind: PathKind::Plain, - span, - }), - span, - }), - arguments: [ - vec![uint_expr(var_id as u128, span)], - vec![expr.clone()], - indexes.iter().rev().cloned().collect(), - ] - .concat(), - })); - ast::Statement { kind: ast::StatementKind::Semi(ast::Expression { kind, span }), span } - } - - fn wrap_let_statement(&mut self, let_stmt: &ast::LetStatement, span: &Span) -> ast::Statement { + fn walk_let_statement(&mut self, let_stmt: &ast::LetStatement, span: &Span) -> ast::Statement { // rewrites let statements written like this: // let (((a,b,c),D { d }),e,f) = x; // @@ -248,7 +179,7 @@ impl DebugState { vec![ast::Statement { kind: ast::StatementKind::Let(let_stmt.clone()), span: *span }]; block_stmts.extend(vars.iter().map(|(id, _)| { let var_id = self.insert_var(&id.0.contents); - self.wrap_assign_var(var_id, id_expr(id)) + build_assign_var_stmt(var_id, id_expr(id)) })); block_stmts.push(ast::Statement { kind: ast::StatementKind::Expression(ast::Expression { @@ -271,7 +202,7 @@ impl DebugState { } } - fn wrap_assign_statement( + fn walk_assign_statement( &mut self, assign_stmt: &ast::AssignStatement, span: &Span, @@ -298,7 +229,7 @@ impl DebugState { let var_id = self .lookup_var(&id.0.contents) .unwrap_or_else(|| panic!("var lookup failed for var_name={}", &id.0.contents)); - self.wrap_assign_var(var_id, id_expr(&ident("__debug_expr", id.span()))) + build_assign_var_stmt(var_id, id_expr(&ident("__debug_expr", id.span()))) } ast::LValue::Dereference(_lv) => { // TODO: this is a dummy statement for now, but we should @@ -335,7 +266,7 @@ impl DebugState { } } } - self.wrap_assign_member( + build_assign_member_stmt( var_id, &indexes, &id_expr(&ident("__debug_expr", expression_span)), @@ -429,8 +360,8 @@ impl DebugState { let var_name = &for_stmt.identifier.0.contents; let var_id = self.insert_var(var_name); - let set_stmt = self.wrap_assign_var(var_id, id_expr(&for_stmt.identifier)); - let drop_stmt = self.wrap_drop_var(var_id, Span::empty(for_stmt.span.end())); + let set_stmt = build_assign_var_stmt(var_id, id_expr(&for_stmt.identifier)); + let drop_stmt = build_drop_var_stmt(var_id, Span::empty(for_stmt.span.end())); self.walk_expr(&mut for_stmt.block); for_stmt.block = ast::Expression { @@ -449,10 +380,10 @@ impl DebugState { fn walk_statement(&mut self, stmt: &mut ast::Statement) { match &mut stmt.kind { ast::StatementKind::Let(let_stmt) => { - *stmt = self.wrap_let_statement(let_stmt, &stmt.span); + *stmt = self.walk_let_statement(let_stmt, &stmt.span); } ast::StatementKind::Assign(assign_stmt) => { - *stmt = self.wrap_assign_statement(assign_stmt, &stmt.span); + *stmt = self.walk_assign_statement(assign_stmt, &stmt.span); } ast::StatementKind::Expression(expr) => { self.walk_expr(expr); @@ -548,6 +479,66 @@ pub fn build_debug_crate_file() -> String { .join("\n") } +fn build_assign_var_stmt(var_id: u32, expr: ast::Expression) -> ast::Statement { + let span = expr.span; + let kind = ast::ExpressionKind::Call(Box::new(ast::CallExpression { + func: Box::new(ast::Expression { + kind: ast::ExpressionKind::Variable(ast::Path { + segments: vec![ident("__debug_var_assign", span)], + kind: PathKind::Plain, + span, + }), + span, + }), + arguments: vec![uint_expr(var_id as u128, span), expr], + })); + ast::Statement { kind: ast::StatementKind::Semi(ast::Expression { kind, span }), span } +} + +fn build_drop_var_stmt(var_id: u32, span: Span) -> ast::Statement { + let kind = ast::ExpressionKind::Call(Box::new(ast::CallExpression { + func: Box::new(ast::Expression { + kind: ast::ExpressionKind::Variable(ast::Path { + segments: vec![ident("__debug_var_drop", span)], + kind: PathKind::Plain, + span, + }), + span, + }), + arguments: vec![uint_expr(var_id as u128, span)], + })); + ast::Statement { kind: ast::StatementKind::Semi(ast::Expression { kind, span }), span } +} + +fn build_assign_member_stmt( + var_id: u32, + indexes: &[ast::Expression], + expr: &ast::Expression, +) -> ast::Statement { + let arity = indexes.len(); + if arity > MAX_MEMBER_ASSIGN_DEPTH { + unreachable!("Assignment to member exceeds maximum depth for debugging"); + } + let span = expr.span; + let kind = ast::ExpressionKind::Call(Box::new(ast::CallExpression { + func: Box::new(ast::Expression { + kind: ast::ExpressionKind::Variable(ast::Path { + segments: vec![ident(&format!["__debug_member_assign_{arity}"], span)], + kind: PathKind::Plain, + span, + }), + span, + }), + arguments: [ + vec![uint_expr(var_id as u128, span)], + vec![expr.clone()], + indexes.iter().rev().cloned().collect(), + ] + .concat(), + })); + ast::Statement { kind: ast::StatementKind::Semi(ast::Expression { kind, span }), span } +} + fn pattern_vars(pattern: &ast::Pattern) -> Vec<(ast::Ident, bool)> { let mut vars = vec![]; let mut stack = VecDeque::from([(pattern, false)]); diff --git a/compiler/noirc_frontend/src/hir/mod.rs b/compiler/noirc_frontend/src/hir/mod.rs index d63d2cbe4a6..f3d01ee7ccd 100644 --- a/compiler/noirc_frontend/src/hir/mod.rs +++ b/compiler/noirc_frontend/src/hir/mod.rs @@ -44,8 +44,6 @@ pub struct Context<'file_manager, 'parsed_files> { pub parsed_files: Cow<'parsed_files, ParsedFiles>, } -pub type StorageSlot = u32; - #[derive(Debug, Copy, Clone)] pub enum FunctionNameMatch<'a> { Anything, diff --git a/compiler/noirc_frontend/src/monomorphization/ast.rs b/compiler/noirc_frontend/src/monomorphization/ast.rs index 71a9e04166c..40829f7b1e2 100644 --- a/compiler/noirc_frontend/src/monomorphization/ast.rs +++ b/compiler/noirc_frontend/src/monomorphization/ast.rs @@ -246,8 +246,8 @@ pub struct Program { /// forwarding to the next phase. pub return_distinctness: Distinctness, pub return_location: Option, - pub debug_var_types: debug_types::VariableTypes, pub return_visibility: Visibility, + pub debug_var_types: debug_types::VariableTypes, } impl Program { @@ -256,16 +256,16 @@ impl Program { main_function_signature: FunctionSignature, return_distinctness: Distinctness, return_location: Option, - debug_var_types: debug_types::VariableTypes, return_visibility: Visibility, + debug_var_types: debug_types::VariableTypes, ) -> Program { Program { functions, main_function_signature, return_distinctness, return_location, - debug_var_types, return_visibility, + debug_var_types, } } diff --git a/compiler/noirc_frontend/src/monomorphization/debug_types.rs b/compiler/noirc_frontend/src/monomorphization/debug_types.rs index 742d15b45dd..910df46ffba 100644 --- a/compiler/noirc_frontend/src/monomorphization/debug_types.rs +++ b/compiler/noirc_frontend/src/monomorphization/debug_types.rs @@ -31,11 +31,7 @@ impl DebugTypes { }); let existing_var_id = self.fe_to_vars.get(&fe_var_id).and_then(|var_id| { let (_, existing_type_id) = self.variables.get(var_id).unwrap(); - if *existing_type_id == type_id { - Some(var_id) - } else { - None - } + (*existing_type_id == type_id).then_some(var_id) }); if let Some(var_id) = existing_var_id { *var_id diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index 308483d405d..4d4f3aae595 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -101,28 +101,16 @@ type HirType = crate::Type; /// but it can also be, for example, an arbitrary test function for running `nargo test`. #[tracing::instrument(level = "trace", skip(main, interner))] pub fn monomorphize(main: node_interner::FuncId, interner: &mut NodeInterner) -> Program { - monomorphize_option_debug(main, interner, None) + monomorphize_debug(main, interner, HashMap::default()) } pub fn monomorphize_debug( main: node_interner::FuncId, interner: &mut NodeInterner, - field_names: &HashMap, + field_names: HashMap, ) -> Program { - monomorphize_option_debug(main, interner, Some(field_names)) -} - -fn monomorphize_option_debug( - main: node_interner::FuncId, - interner: &mut NodeInterner, - option_field_names: Option<&HashMap>, -) -> Program { - let mut monomorphizer = Monomorphizer::new(interner); - let function_sig = if let Some(field_names) = option_field_names { - monomorphizer.compile_main_debug(main, field_names) - } else { - monomorphizer.compile_main(main) - }; + let mut monomorphizer = Monomorphizer::new(interner, field_names); + let function_sig = monomorphizer.compile_main(main); while !monomorphizer.queue.is_empty() { let (next_fn_id, new_id, bindings, trait_method) = monomorphizer.queue.pop_front().unwrap(); @@ -144,13 +132,13 @@ fn monomorphize_option_debug( function_sig, *return_distinctness, monomorphizer.return_location, - monomorphizer.debug_types.into(), *return_visibility, + monomorphizer.debug_types.into(), ) } impl<'interner> Monomorphizer<'interner> { - fn new(interner: &'interner mut NodeInterner) -> Self { + fn new(interner: &'interner mut NodeInterner, debug_field_names: HashMap) -> Self { Monomorphizer { globals: HashMap::new(), locals: HashMap::new(), @@ -163,7 +151,7 @@ impl<'interner> Monomorphizer<'interner> { is_range_loop: false, return_location: None, debug_types: DebugTypes::default(), - debug_field_names: HashMap::default(), + debug_field_names, } } @@ -255,42 +243,28 @@ impl<'interner> Monomorphizer<'interner> { main_meta.function_signature() } - fn compile_main_debug( - &mut self, - main_id: node_interner::FuncId, - field_names: &HashMap, - ) -> FunctionSignature { - self.debug_field_names = field_names.clone(); - self.compile_main(main_id) - } - fn function(&mut self, f: node_interner::FuncId, id: FuncId) { if let Some((self_type, trait_id)) = self.interner.get_function_trait(&f) { let the_trait = self.interner.get_trait(trait_id); the_trait.self_type_typevar.force_bind(self_type); } - let (meta_parameters, unconstrained, body_expr_id, name, return_type) = { - let meta = self.interner.function_meta(&f).clone(); - let modifiers = self.interner.function_modifiers(&f).clone(); - let name = self.interner.function_name(&f).to_string(); + let meta = self.interner.function_meta(&f).clone(); + let modifiers = self.interner.function_modifiers(&f); + let name = self.interner.function_name(&f).to_owned(); - let body_expr_id = *self.interner.function(&f).as_expr(); - let body_return_type = self.interner.id_type(body_expr_id); - let return_type = self.convert_type(match meta.return_type() { - Type::TraitAsType(_, _, _) => &body_return_type, - _ => meta.return_type(), - }); - let unconstrained = modifiers.is_unconstrained - || matches!(modifiers.contract_function_type, Some(ContractFunctionType::Open)); - (meta.parameters, unconstrained, body_expr_id, name, return_type) - }; + let body_expr_id = *self.interner.function(&f).as_expr(); + let body_return_type = self.interner.id_type(body_expr_id); + let return_type = self.convert_type(match meta.return_type() { + Type::TraitAsType(_, _, _) => &body_return_type, + _ => meta.return_type(), + }); + let unconstrained = modifiers.is_unconstrained + || matches!(modifiers.contract_function_type, Some(ContractFunctionType::Open)); - let function = { - let parameters = self.parameters(&meta_parameters).clone(); - let body = self.expr(body_expr_id).clone(); - ast::Function { id, name, parameters, body, return_type, unconstrained } - }; + let parameters = self.parameters(&meta.parameters); + let body = self.expr(body_expr_id); + let function = ast::Function { id, name, parameters, body, return_type, unconstrained }; self.push_function(id, function); } From e0a4c8809b05adc9411f837ad1be614b26f84ec0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Wed, 31 Jan 2024 17:12:17 -0500 Subject: [PATCH 10/19] chore: Extract debug related code in monomorphization And apply more code review feedback --- .../src/monomorphization/debug.rs | 178 ++++++++++++++++++ .../src/monomorphization/mod.rs | 169 +---------------- tooling/debugger/src/foreign_calls.rs | 4 +- tooling/nargo/src/artifacts/debug_vars.rs | 9 +- 4 files changed, 184 insertions(+), 176 deletions(-) create mode 100644 compiler/noirc_frontend/src/monomorphization/debug.rs diff --git a/compiler/noirc_frontend/src/monomorphization/debug.rs b/compiler/noirc_frontend/src/monomorphization/debug.rs new file mode 100644 index 00000000000..2eecc277688 --- /dev/null +++ b/compiler/noirc_frontend/src/monomorphization/debug.rs @@ -0,0 +1,178 @@ +use iter_extended::vecmap; +use noirc_printable_type::PrintableType; + +use crate::hir_def::expr::*; + +use super::ast::{Expression, Ident}; +use super::Monomorphizer; + +const DEBUG_MEMBER_ASSIGN_PREFIX: &str = "__debug_member_assign_"; +const DEBUG_VAR_ID_ARG_SLOT: usize = 0; +const DEBUG_VALUE_ARG_SLOT: usize = 1; +const DEBUG_MEMBER_FIELD_INDEX_ARG_SLOT: usize = 2; + +impl<'interner> Monomorphizer<'interner> { + /// Try to patch instrumentation code inserted for debugging. This will + /// record tracked variables and their types, and assign them an ID to use + /// at runtime. + pub(super) fn patch_debug_instrumentation_call( + &mut self, + call: &HirCallExpression, + arguments: &mut [Expression], + ) { + let original_func = Box::new(self.expr(call.func)); + if let Expression::Ident(Ident { name, .. }) = original_func.as_ref() { + if name == "__debug_var_assign" { + self.patch_debug_var_assign(call, arguments); + } else if name == "__debug_var_drop" { + self.patch_debug_var_drop(call, arguments); + } else if let Some(arity) = name.strip_prefix(DEBUG_MEMBER_ASSIGN_PREFIX) { + let arity = arity.parse::().expect("failed to parse member assign arity"); + self.patch_debug_member_assign(call, arguments, arity); + } + } + } + + /// Update instrumentation code inserted on variable assignment. We need to + /// register the variable instance, its type and replace the temporary ID + /// (fe_var_id) with the ID of the registration. Multiple registrations of + /// the same variable are possible if using generic functions, hence the + /// temporary ID created when injecting the instrumentation code can map to + /// multiple IDs at runtime. + fn patch_debug_var_assign(&mut self, call: &HirCallExpression, arguments: &mut [Expression]) { + let hir_arguments = vecmap(&call.arguments, |id| self.interner.expression(id)); + let Some(HirExpression::Literal(HirLiteral::Integer(fe_var_id, _))) = hir_arguments.get(DEBUG_VAR_ID_ARG_SLOT) else { + unreachable!("Missing FE var ID in __debug_var_assign call"); + }; + let Some(HirExpression::Ident(HirIdent { id, .. })) = hir_arguments.get(DEBUG_VALUE_ARG_SLOT) else { + unreachable!("Missing value identifier in __debug_var_assign call"); + }; + + // update variable assignments + let var_def = self.interner.definition(*id); + let var_type = self.interner.id_type(call.arguments[DEBUG_VALUE_ARG_SLOT]); + let fe_var_id = fe_var_id.to_u128() as u32; + let var_id = if var_def.name != "__debug_expr" { + self.debug_types.insert_var(fe_var_id, &var_def.name, var_type) + } else { + self.debug_types.get_var_id(fe_var_id).unwrap() + }; + let interned_var_id = self.intern_var_id(var_id, &call.location); + arguments[DEBUG_VAR_ID_ARG_SLOT] = self.expr(interned_var_id); + } + + /// Update instrumentation code for a variable being dropped out of scope. + /// Given the fe_var_id we search for the last assigned runtime variable ID + /// and replace it instead. + fn patch_debug_var_drop(&mut self, call: &HirCallExpression, arguments: &mut [Expression]) { + let hir_arguments = vecmap(&call.arguments, |id| self.interner.expression(id)); + let Some(HirExpression::Literal(HirLiteral::Integer(fe_var_id, _))) = hir_arguments.get(DEBUG_VAR_ID_ARG_SLOT) else { + unreachable!("Missing FE var ID in __debug_var_drop call"); + }; + // update variable drops (ie. when the var goes out of scope) + let fe_var_id = fe_var_id.to_u128() as u32; + if let Some(var_id) = self.debug_types.get_var_id(fe_var_id) { + let interned_var_id = self.intern_var_id(var_id, &call.location); + arguments[DEBUG_VAR_ID_ARG_SLOT] = self.expr(interned_var_id); + } + } + + /// Update instrumentation code inserted when assigning to a member of an + /// existing variable. Same as above for replacing the fe_var_id, but also + /// we need to resolve the path and the type of the member being assigned. + /// For this last part, we need to resolve the mapping from field names in + /// structs to positions in the runtime tuple, since all structs are + /// replaced by tuples during compilation. + fn patch_debug_member_assign( + &mut self, + call: &HirCallExpression, + arguments: &mut [Expression], + arity: usize, + ) { + let hir_arguments = vecmap(&call.arguments, |id| self.interner.expression(id)); + let Some(HirExpression::Literal(HirLiteral::Integer(fe_var_id, _))) = hir_arguments.get(DEBUG_VAR_ID_ARG_SLOT) else { + unreachable!("Missing FE var ID in __debug_member_assign call"); + }; + let Some(HirExpression::Ident(HirIdent { id, .. })) = hir_arguments.get(DEBUG_VALUE_ARG_SLOT) else { + unreachable!("Missing value identifier in __debug_member_assign call"); + }; + // update variable member assignments + let var_def_name = self.interner.definition(*id).name.clone(); + let var_type = self.interner.id_type(call.arguments[DEBUG_VALUE_ARG_SLOT]); + let fe_var_id = fe_var_id.to_u128() as u32; + + let mut cursor_type = self + .debug_types + .get_type(fe_var_id) + .unwrap_or_else(|| panic!("type not found for fe_var_id={fe_var_id}")) + .clone(); + for i in 0..arity { + if let Some(HirExpression::Literal(HirLiteral::Integer(fe_i, i_neg))) = + hir_arguments.get(DEBUG_MEMBER_FIELD_INDEX_ARG_SLOT + i) + { + let mut index = fe_i.to_i128(); + if *i_neg { + index = -index; + } + if index < 0 { + let index = index.unsigned_abs(); + let field_name = self + .debug_field_names + .get(&(index as u32)) + .unwrap_or_else(|| panic!("field name not available for {i:?}")); + let field_i = (get_field(&cursor_type, field_name) + .unwrap_or_else(|| panic!("failed to find field_name: {field_name}")) + as i128) + .unsigned_abs(); + cursor_type = element_type_at_index(&cursor_type, field_i as usize); + let index_id = self.interner.push_expr(HirExpression::Literal( + HirLiteral::Integer(field_i.into(), false), + )); + self.interner.push_expr_type(&index_id, crate::Type::FieldElement); + self.interner.push_expr_location( + index_id, + call.location.span, + call.location.file, + ); + arguments[DEBUG_MEMBER_FIELD_INDEX_ARG_SLOT + i] = self.expr(index_id); + } else { + cursor_type = element_type_at_index(&cursor_type, 0); + } + } else { + cursor_type = element_type_at_index(&cursor_type, 0); + } + } + + let var_id = if &var_def_name != "__debug_expr" { + self.debug_types.insert_var(fe_var_id, &var_def_name, var_type) + } else { + self.debug_types.get_var_id(fe_var_id).unwrap() + }; + let interned_var_id = self.intern_var_id(var_id, &call.location); + arguments[DEBUG_VAR_ID_ARG_SLOT] = self.expr(interned_var_id); + } +} + +fn get_field(ptype: &PrintableType, field_name: &str) -> Option { + match ptype { + PrintableType::Struct { fields, .. } => { + fields.iter().position(|(name, _)| name == field_name) + } + PrintableType::Tuple { .. } | PrintableType::Array { .. } => { + field_name.parse::().ok() + } + _ => None, + } +} + +fn element_type_at_index(ptype: &PrintableType, i: usize) -> PrintableType { + match ptype { + PrintableType::Array { length: _length, typ } => (**typ).clone(), + PrintableType::Tuple { types } => types[i].clone(), + PrintableType::Struct { name: _name, fields } => fields[i].1.clone(), + PrintableType::String { length: _length } => PrintableType::UnsignedInteger { width: 8 }, + _ => { + panic!["expected type with sub-fields, found terminal type"] + } + } +} diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index 4d4f3aae595..308e664829d 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -34,6 +34,7 @@ use self::ast::{Definition, FuncId, Function, LocalId, Program}; use self::debug_types::DebugTypes; pub mod ast; +mod debug; pub mod debug_types; pub mod printer; @@ -42,8 +43,6 @@ struct LambdaContext { captures: Vec, } -const DEBUG_MEMBER_ASSIGN_PREFIX: &str = "__debug_member_assign_"; - /// The context struct for the monomorphization pass. /// /// This struct holds the FIFO queue of functions to monomorphize, which is added to @@ -936,133 +935,6 @@ impl<'interner> Monomorphizer<'interner> { expr_id } - /// Update instrumentation code inserted on variable assignment. We need to - /// register the variable instance, its type and replace the temporary ID - /// (fe_var_id) with the ID of the registration. Multiple registrations of - /// the same variable are possible if using generic functions, hence the - /// temporary ID created when injecting the instrumentation code can map to - /// multiple IDs at runtime. - fn patch_debug_var_assign( - &mut self, - call: &HirCallExpression, - arguments: &mut [ast::Expression], - ) { - let hir_arguments = vecmap(&call.arguments, |id| self.interner.expression(id)); - let Some(HirExpression::Literal(HirLiteral::Integer(fe_var_id, _))) = hir_arguments.get(0) else { - unreachable!("Missing FE var ID in __debug_var_assign call"); - }; - let Some(HirExpression::Ident(HirIdent { id, .. })) = hir_arguments.get(1) else { - unreachable!("Missing value identifier in __debug_var_assign call"); - }; - - // update variable assignments - let var_def = self.interner.definition(*id); - let var_type = self.interner.id_type(call.arguments[1]); - let fe_var_id = fe_var_id.to_u128() as u32; - let var_id = if var_def.name != "__debug_expr" { - self.debug_types.insert_var(fe_var_id, &var_def.name, var_type) - } else { - self.debug_types.get_var_id(fe_var_id).unwrap() - }; - let interned_var_id = self.intern_var_id(var_id, &call.location); - arguments[0] = self.expr(interned_var_id); - } - - /// Update instrumentation code for a variable being dropped out of scope. - /// Given the fe_var_id we search for the last assigned runtime variable ID - /// and replace it instead. - fn patch_debug_var_drop( - &mut self, - call: &HirCallExpression, - arguments: &mut [ast::Expression], - ) { - let hir_arguments = vecmap(&call.arguments, |id| self.interner.expression(id)); - let Some(HirExpression::Literal(HirLiteral::Integer(fe_var_id, _))) = hir_arguments.get(0) else { - unreachable!("Missing FE var ID in __debug_var_drop call"); - }; - // update variable drops (ie. when the var goes out of scope) - let fe_var_id = fe_var_id.to_u128() as u32; - if let Some(var_id) = self.debug_types.get_var_id(fe_var_id) { - let interned_var_id = self.intern_var_id(var_id, &call.location); - arguments[0] = self.expr(interned_var_id); - } - } - - /// Update instrumentation code inserted when assigning to a member of an - /// existing variable. Same as above for replacing the fe_var_id, but also - /// we need to resolve the path and the type of the member being assigned. - /// For this last part, we need to resolve the mapping from field names in - /// structs to positions in the runtime tuple, since all structs are - /// replaced by tuples during compilation. - fn patch_debug_member_assign( - &mut self, - call: &HirCallExpression, - arguments: &mut [ast::Expression], - arity: usize, - ) { - let hir_arguments = vecmap(&call.arguments, |id| self.interner.expression(id)); - let Some(HirExpression::Literal(HirLiteral::Integer(fe_var_id, _))) = hir_arguments.get(0) else { - unreachable!("Missing FE var ID in __debug_member_assign call"); - }; - let Some(HirExpression::Ident(HirIdent { id, .. })) = hir_arguments.get(1) else { - unreachable!("Missing value identifier in __debug_member_assign call"); - }; - // update variable member assignments - let var_def_name = self.interner.definition(*id).name.clone(); - let var_type = self.interner.id_type(call.arguments[2]); - let fe_var_id = fe_var_id.to_u128() as u32; - - let mut cursor_type = self - .debug_types - .get_type(fe_var_id) - .unwrap_or_else(|| panic!("type not found for fe_var_id={fe_var_id}")) - .clone(); - for i in 0..arity { - if let Some(HirExpression::Literal(HirLiteral::Integer(fe_i, i_neg))) = - hir_arguments.get(2 + i) - { - let mut index = fe_i.to_i128(); - if *i_neg { - index = -index; - } - if index < 0 { - let index = index.unsigned_abs(); - let field_name = self - .debug_field_names - .get(&(index as u32)) - .unwrap_or_else(|| panic!("field name not available for {i:?}")); - let field_i = (get_field(&cursor_type, field_name) - .unwrap_or_else(|| panic!("failed to find field_name: {field_name}")) - as i128) - .unsigned_abs(); - cursor_type = next_type(&cursor_type, field_i as usize); - let index_id = self.interner.push_expr(HirExpression::Literal( - HirLiteral::Integer(field_i.into(), false), - )); - self.interner.push_expr_type(&index_id, Type::FieldElement); - self.interner.push_expr_location( - index_id, - call.location.span, - call.location.file, - ); - arguments[2 + i] = self.expr(index_id); - } else { - cursor_type = next_type(&cursor_type, 0); - } - } else { - cursor_type = next_type(&cursor_type, 0); - } - } - - let var_id = if &var_def_name != "__debug_expr" { - self.debug_types.insert_var(fe_var_id, &var_def_name, var_type) - } else { - self.debug_types.get_var_id(fe_var_id).unwrap() - }; - let interned_var_id = self.intern_var_id(var_id, &call.location); - arguments[0] = self.expr(interned_var_id); - } - fn function_call( &mut self, call: HirCallExpression, @@ -1072,17 +944,7 @@ impl<'interner> Monomorphizer<'interner> { let mut arguments = vecmap(&call.arguments, |id| self.expr(*id)); let hir_arguments = vecmap(&call.arguments, |id| self.interner.expression(id)); - // patch instrumentation code inserted for debugging - if let ast::Expression::Ident(ast::Ident { name, .. }) = original_func.as_ref() { - if name == "__debug_var_assign" { - self.patch_debug_var_assign(&call, &mut arguments); - } else if name == "__debug_var_drop" { - self.patch_debug_var_drop(&call, &mut arguments); - } else if let Some(arity) = name.strip_prefix(DEBUG_MEMBER_ASSIGN_PREFIX) { - let arity = arity.parse::().expect("failed to parse member assign arity"); - self.patch_debug_member_assign(&call, &mut arguments, arity); - } - } + self.patch_debug_instrumentation_call(&call, &mut arguments); let return_type = self.interner.id_type(id); let return_type = self.convert_type(&return_type); @@ -1757,30 +1619,3 @@ fn undo_instantiation_bindings(bindings: TypeBindings) { var.unbind(id); } } - -fn get_field(ptype: &PrintableType, field_name: &str) -> Option { - match ptype { - PrintableType::Struct { fields, .. } => { - fields.iter().position(|(name, _)| name == field_name) - } - PrintableType::Tuple { .. } | PrintableType::Array { .. } => { - field_name.parse::().ok() - } - _ => None, - } -} - -fn next_type(ptype: &PrintableType, i: usize) -> PrintableType { - match ptype { - PrintableType::Array { length: _length, typ } => (**typ).clone(), - PrintableType::Tuple { types } => types[i].clone(), - PrintableType::Struct { name: _name, fields } => fields[i].1.clone(), - PrintableType::String { length: _length } => { - // TODO: check bounds - PrintableType::UnsignedInteger { width: 8 } - } - _ => { - panic!["expected type with sub-fields, found terminal type"] - } - } -} diff --git a/tooling/debugger/src/foreign_calls.rs b/tooling/debugger/src/foreign_calls.rs index 4091f73fb69..71f45f5b7e1 100644 --- a/tooling/debugger/src/foreign_calls.rs +++ b/tooling/debugger/src/foreign_calls.rs @@ -105,7 +105,7 @@ impl ForeignCallExecutor for DefaultDebugForeignCallExecutor { let var_id = var_id_value.to_u128() as u32; let values: Vec = foreign_call.inputs[1..].iter().flat_map(|x| x.values()).collect(); - self.debug_vars.assign(var_id, &values); + self.debug_vars.assign_var(var_id, &values); } Ok(ForeignCallResult { values: vec![] }) } @@ -113,7 +113,7 @@ impl ForeignCallExecutor for DefaultDebugForeignCallExecutor { let fcp_var_id = &foreign_call.inputs[0]; if let ForeignCallParam::Single(var_id_value) = fcp_var_id { let var_id = var_id_value.to_u128() as u32; - self.debug_vars.drop(var_id); + self.debug_vars.drop_var(var_id); } Ok(ForeignCallResult { values: vec![] }) } diff --git a/tooling/nargo/src/artifacts/debug_vars.rs b/tooling/nargo/src/artifacts/debug_vars.rs index b4acfc5ced2..9a93143bc13 100644 --- a/tooling/nargo/src/artifacts/debug_vars.rs +++ b/tooling/nargo/src/artifacts/debug_vars.rs @@ -46,9 +46,8 @@ impl DebugVars { }); } - pub fn assign(&mut self, var_id: u32, values: &[Value]) { + pub fn assign_var(&mut self, var_id: u32, values: &[Value]) { self.active.insert(var_id); - // TODO: assign values as PrintableValue let type_id = self.id_to_type.get(&var_id).unwrap(); let ptype = self.types.get(type_id).unwrap(); self.id_to_value @@ -118,15 +117,11 @@ impl DebugVars { unimplemented![] } - pub fn get(&mut self, var_id: u32) -> Option<&PrintableValue> { - self.id_to_value.get(&var_id) - } - pub fn get_type(&self, var_id: u32) -> Option<&PrintableType> { self.id_to_type.get(&var_id).and_then(|type_id| self.types.get(type_id)) } - pub fn drop(&mut self, var_id: u32) { + pub fn drop_var(&mut self, var_id: u32) { self.active.remove(&var_id); } } From 937793d0b538579bdbcad03f648e22f729857e02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Thu, 1 Feb 2024 13:47:35 -0500 Subject: [PATCH 11/19] feat: Use var names collected during injection in monomorphization phase Also, move some debug related internal state from Monomorphizer into DebugTypes. --- compiler/noirc_driver/src/lib.rs | 3 +- .../src/monomorphization/debug.rs | 119 ++++++++---------- .../src/monomorphization/debug_types.rs | 37 +++++- .../src/monomorphization/mod.rs | 14 +-- 4 files changed, 96 insertions(+), 77 deletions(-) diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index 8e76b7185cb..3b6a0c9e8a9 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -418,8 +418,7 @@ pub fn compile_no_check( force_compile: bool, ) -> Result { let program = if options.instrument_debug { - let field_names = context.debug_state.field_names.clone(); - monomorphize_debug(main_function, &mut context.def_interner, field_names) + monomorphize_debug(main_function, &mut context.def_interner, &context.debug_state) } else { monomorphize(main_function, &mut context.def_interner) }; diff --git a/compiler/noirc_frontend/src/monomorphization/debug.rs b/compiler/noirc_frontend/src/monomorphization/debug.rs index 2eecc277688..e38982eeab2 100644 --- a/compiler/noirc_frontend/src/monomorphization/debug.rs +++ b/compiler/noirc_frontend/src/monomorphization/debug.rs @@ -12,9 +12,13 @@ const DEBUG_VALUE_ARG_SLOT: usize = 1; const DEBUG_MEMBER_FIELD_INDEX_ARG_SLOT: usize = 2; impl<'interner> Monomorphizer<'interner> { - /// Try to patch instrumentation code inserted for debugging. This will - /// record tracked variables and their types, and assign them an ID to use - /// at runtime. + /// Patch instrumentation calls inserted for debugging. This will record + /// tracked variables and their types, and assign them an ID to use at + /// runtime. This ID is different from the ID assigned at instrumentation + /// time because at that point we haven't fully resolved the types for + /// generic functions. So a single generic function may be instantiated + /// multiple times with its tracked variables being of different types for + /// each instance at runtime. pub(super) fn patch_debug_instrumentation_call( &mut self, call: &HirCallExpression, @@ -41,22 +45,17 @@ impl<'interner> Monomorphizer<'interner> { /// multiple IDs at runtime. fn patch_debug_var_assign(&mut self, call: &HirCallExpression, arguments: &mut [Expression]) { let hir_arguments = vecmap(&call.arguments, |id| self.interner.expression(id)); - let Some(HirExpression::Literal(HirLiteral::Integer(fe_var_id, _))) = hir_arguments.get(DEBUG_VAR_ID_ARG_SLOT) else { + let var_id_arg = hir_arguments.get(DEBUG_VAR_ID_ARG_SLOT); + let Some(HirExpression::Literal(HirLiteral::Integer(fe_var_id, _))) = var_id_arg else { unreachable!("Missing FE var ID in __debug_var_assign call"); }; - let Some(HirExpression::Ident(HirIdent { id, .. })) = hir_arguments.get(DEBUG_VALUE_ARG_SLOT) else { - unreachable!("Missing value identifier in __debug_var_assign call"); - }; - // update variable assignments - let var_def = self.interner.definition(*id); + // instantiate tracked variable for the value type and associate it with + // the ID used by the injected instrumentation code let var_type = self.interner.id_type(call.arguments[DEBUG_VALUE_ARG_SLOT]); let fe_var_id = fe_var_id.to_u128() as u32; - let var_id = if var_def.name != "__debug_expr" { - self.debug_types.insert_var(fe_var_id, &var_def.name, var_type) - } else { - self.debug_types.get_var_id(fe_var_id).unwrap() - }; + // then update the ID used for tracking at runtime + let var_id = self.debug_types.insert_var(fe_var_id, var_type); let interned_var_id = self.intern_var_id(var_id, &call.location); arguments[DEBUG_VAR_ID_ARG_SLOT] = self.expr(interned_var_id); } @@ -66,15 +65,18 @@ impl<'interner> Monomorphizer<'interner> { /// and replace it instead. fn patch_debug_var_drop(&mut self, call: &HirCallExpression, arguments: &mut [Expression]) { let hir_arguments = vecmap(&call.arguments, |id| self.interner.expression(id)); - let Some(HirExpression::Literal(HirLiteral::Integer(fe_var_id, _))) = hir_arguments.get(DEBUG_VAR_ID_ARG_SLOT) else { + let var_id_arg = hir_arguments.get(DEBUG_VAR_ID_ARG_SLOT); + let Some(HirExpression::Literal(HirLiteral::Integer(fe_var_id, _))) = var_id_arg else { unreachable!("Missing FE var ID in __debug_var_drop call"); }; - // update variable drops (ie. when the var goes out of scope) + // update variable ID for tracked drops (ie. when the var goes out of scope) let fe_var_id = fe_var_id.to_u128() as u32; - if let Some(var_id) = self.debug_types.get_var_id(fe_var_id) { - let interned_var_id = self.intern_var_id(var_id, &call.location); - arguments[DEBUG_VAR_ID_ARG_SLOT] = self.expr(interned_var_id); - } + let var_id = self + .debug_types + .get_var_id(fe_var_id) + .unwrap_or_else(|| unreachable!("failed to find debug variable")); + let interned_var_id = self.intern_var_id(var_id, &call.location); + arguments[DEBUG_VAR_ID_ARG_SLOT] = self.expr(interned_var_id); } /// Update instrumentation code inserted when assigning to a member of an @@ -90,43 +92,39 @@ impl<'interner> Monomorphizer<'interner> { arity: usize, ) { let hir_arguments = vecmap(&call.arguments, |id| self.interner.expression(id)); - let Some(HirExpression::Literal(HirLiteral::Integer(fe_var_id, _))) = hir_arguments.get(DEBUG_VAR_ID_ARG_SLOT) else { + let var_id_arg = hir_arguments.get(DEBUG_VAR_ID_ARG_SLOT); + let Some(HirExpression::Literal(HirLiteral::Integer(fe_var_id, _))) = var_id_arg else { unreachable!("Missing FE var ID in __debug_member_assign call"); }; - let Some(HirExpression::Ident(HirIdent { id, .. })) = hir_arguments.get(DEBUG_VALUE_ARG_SLOT) else { - unreachable!("Missing value identifier in __debug_member_assign call"); - }; // update variable member assignments - let var_def_name = self.interner.definition(*id).name.clone(); - let var_type = self.interner.id_type(call.arguments[DEBUG_VALUE_ARG_SLOT]); let fe_var_id = fe_var_id.to_u128() as u32; - let mut cursor_type = self + let var_type = self .debug_types .get_type(fe_var_id) .unwrap_or_else(|| panic!("type not found for fe_var_id={fe_var_id}")) .clone(); + let mut cursor_type = &var_type; for i in 0..arity { if let Some(HirExpression::Literal(HirLiteral::Integer(fe_i, i_neg))) = hir_arguments.get(DEBUG_MEMBER_FIELD_INDEX_ARG_SLOT + i) { - let mut index = fe_i.to_i128(); + let index = fe_i.to_i128().unsigned_abs(); if *i_neg { - index = -index; - } - if index < 0 { - let index = index.unsigned_abs(); - let field_name = self - .debug_field_names - .get(&(index as u32)) - .unwrap_or_else(|| panic!("field name not available for {i:?}")); - let field_i = (get_field(&cursor_type, field_name) - .unwrap_or_else(|| panic!("failed to find field_name: {field_name}")) - as i128) - .unsigned_abs(); - cursor_type = element_type_at_index(&cursor_type, field_i as usize); + // We use negative indices at instrumentation time to indicate + // and reference member accesses by name which cannot be + // resolved until we have a type. This code path is also used + // for tuple member access. + let field_index = self + .debug_types + .resolve_field_index(index as u32, cursor_type) + .unwrap_or_else(|| { + unreachable!("failed to resolve {i}-th member indirection on type {cursor_type:?}") + }); + + cursor_type = element_type_at_index(cursor_type, field_index); let index_id = self.interner.push_expr(HirExpression::Literal( - HirLiteral::Integer(field_i.into(), false), + HirLiteral::Integer(field_index.into(), false), )); self.interner.push_expr_type(&index_id, crate::Type::FieldElement); self.interner.push_expr_location( @@ -136,41 +134,30 @@ impl<'interner> Monomorphizer<'interner> { ); arguments[DEBUG_MEMBER_FIELD_INDEX_ARG_SLOT + i] = self.expr(index_id); } else { - cursor_type = element_type_at_index(&cursor_type, 0); + // array/string element using constant index + cursor_type = element_type_at_index(cursor_type, index as usize); } } else { - cursor_type = element_type_at_index(&cursor_type, 0); + // array element using non-constant index + cursor_type = element_type_at_index(cursor_type, 0); } } - let var_id = if &var_def_name != "__debug_expr" { - self.debug_types.insert_var(fe_var_id, &var_def_name, var_type) - } else { - self.debug_types.get_var_id(fe_var_id).unwrap() - }; + let var_id = self + .debug_types + .get_var_id(fe_var_id) + .unwrap_or_else(|| unreachable!("failed to find debug variable")); let interned_var_id = self.intern_var_id(var_id, &call.location); arguments[DEBUG_VAR_ID_ARG_SLOT] = self.expr(interned_var_id); } } -fn get_field(ptype: &PrintableType, field_name: &str) -> Option { - match ptype { - PrintableType::Struct { fields, .. } => { - fields.iter().position(|(name, _)| name == field_name) - } - PrintableType::Tuple { .. } | PrintableType::Array { .. } => { - field_name.parse::().ok() - } - _ => None, - } -} - -fn element_type_at_index(ptype: &PrintableType, i: usize) -> PrintableType { +fn element_type_at_index(ptype: &PrintableType, i: usize) -> &PrintableType { match ptype { - PrintableType::Array { length: _length, typ } => (**typ).clone(), - PrintableType::Tuple { types } => types[i].clone(), - PrintableType::Struct { name: _name, fields } => fields[i].1.clone(), - PrintableType::String { length: _length } => PrintableType::UnsignedInteger { width: 8 }, + PrintableType::Array { length: _length, typ } => typ.as_ref(), + PrintableType::Tuple { types } => &types[i], + PrintableType::Struct { name: _name, fields } => &fields[i].1, + PrintableType::String { length: _length } => &PrintableType::UnsignedInteger { width: 8 }, _ => { panic!["expected type with sub-fields, found terminal type"] } diff --git a/compiler/noirc_frontend/src/monomorphization/debug_types.rs b/compiler/noirc_frontend/src/monomorphization/debug_types.rs index 910df46ffba..fd0298b33e5 100644 --- a/compiler/noirc_frontend/src/monomorphization/debug_types.rs +++ b/compiler/noirc_frontend/src/monomorphization/debug_types.rs @@ -1,4 +1,4 @@ -use crate::hir_def::types::Type; +use crate::{debug::DebugState, hir_def::types::Type}; pub use noirc_errors::debug_info::{Types, VariableTypes, Variables}; use noirc_printable_type::PrintableType; use std::collections::HashMap; @@ -11,6 +11,8 @@ use std::collections::HashMap; /// will have a valid type. #[derive(Debug, Clone, Default)] pub struct DebugTypes { + debug_variables: HashMap, + debug_field_names: HashMap, fe_to_vars: HashMap, // fe_var_id => var_id variables: HashMap, // var_id => (var_name, type_id) types: HashMap, @@ -20,7 +22,26 @@ pub struct DebugTypes { } impl DebugTypes { - pub fn insert_var(&mut self, fe_var_id: u32, var_name: &str, var_type: Type) -> u32 { + pub fn build_from_debug_state(debug_state: &DebugState) -> Self { + DebugTypes { + debug_variables: debug_state.variables.clone(), + debug_field_names: debug_state.field_names.clone(), + ..DebugTypes::default() + } + } + + pub fn resolve_field_index(&self, field_id: u32, cursor_type: &PrintableType) -> Option { + self.debug_field_names + .get(&field_id) + .and_then(|field_name| get_field(cursor_type, field_name)) + } + + pub fn insert_var(&mut self, fe_var_id: u32, var_type: Type) -> u32 { + let var_name = self + .debug_variables + .get(&fe_var_id) + .unwrap_or_else(|| unreachable!("cannot find name for debug variable {fe_var_id}")); + let ptype: PrintableType = var_type.follow_bindings().into(); let type_id = self.types.get(&ptype).cloned().unwrap_or_else(|| { let type_id = self.next_type_id; @@ -56,6 +77,18 @@ impl DebugTypes { } } +fn get_field(ptype: &PrintableType, field_name: &str) -> Option { + match ptype { + PrintableType::Struct { fields, .. } => { + fields.iter().position(|(name, _)| name == field_name) + } + PrintableType::Tuple { .. } | PrintableType::Array { .. } => { + field_name.parse::().ok() + } + _ => None, + } +} + impl From for VariableTypes { fn from(val: DebugTypes) -> Self { ( diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index 308e664829d..59b7b9418e7 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -18,6 +18,7 @@ use std::{ }; use crate::{ + debug::DebugState, hir_def::{ expr::*, function::{FuncMeta, FunctionSignature, Parameters}, @@ -82,7 +83,6 @@ struct Monomorphizer<'interner> { return_location: Option, debug_types: DebugTypes, - debug_field_names: HashMap, } type HirType = crate::Type; @@ -100,15 +100,16 @@ type HirType = crate::Type; /// but it can also be, for example, an arbitrary test function for running `nargo test`. #[tracing::instrument(level = "trace", skip(main, interner))] pub fn monomorphize(main: node_interner::FuncId, interner: &mut NodeInterner) -> Program { - monomorphize_debug(main, interner, HashMap::default()) + monomorphize_debug(main, interner, &DebugState::default()) } pub fn monomorphize_debug( main: node_interner::FuncId, interner: &mut NodeInterner, - field_names: HashMap, + debug_state: &DebugState, ) -> Program { - let mut monomorphizer = Monomorphizer::new(interner, field_names); + let debug_types = DebugTypes::build_from_debug_state(debug_state); + let mut monomorphizer = Monomorphizer::new(interner, debug_types); let function_sig = monomorphizer.compile_main(main); while !monomorphizer.queue.is_empty() { @@ -137,7 +138,7 @@ pub fn monomorphize_debug( } impl<'interner> Monomorphizer<'interner> { - fn new(interner: &'interner mut NodeInterner, debug_field_names: HashMap) -> Self { + fn new(interner: &'interner mut NodeInterner, debug_types: DebugTypes) -> Self { Monomorphizer { globals: HashMap::new(), locals: HashMap::new(), @@ -149,8 +150,7 @@ impl<'interner> Monomorphizer<'interner> { lambda_envs_stack: Vec::new(), is_range_loop: false, return_location: None, - debug_types: DebugTypes::default(), - debug_field_names, + debug_types, } } From bd68ee8d1c5e645cdcc78d8a5a54b85d1c0b9f42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Thu, 1 Feb 2024 21:02:33 -0500 Subject: [PATCH 12/19] feat: Refactor debug instrumentation code in the compiler - Rename DebugState to DebugInstrumenter - Rename DebugTypes to DebugTypeTracker - Replace all u32 identifier types by more specific types: - SourceVarId: variable instrumented in the source code - SourceFieldId: identifier for fields used for member accesses - DebugVarId: variable being tracked at runtime (this is an instance of a SourceVar with a fully reified type) - DebugTypeId: identifier for types being tracked at runtime --- compiler/noirc_errors/src/debug_info.rs | 30 ++-- compiler/noirc_evaluator/src/ssa.rs | 5 +- compiler/noirc_frontend/src/debug/mod.rs | 72 ++++++---- compiler/noirc_frontend/src/hir/mod.rs | 8 +- .../src/monomorphization/ast.rs | 17 ++- .../src/monomorphization/debug.rs | 93 ++++++++----- .../src/monomorphization/debug_types.rs | 130 +++++++++++------- .../src/monomorphization/mod.rs | 31 ++--- tooling/debugger/src/foreign_calls.rs | 13 +- tooling/debugger/src/source_code_printer.rs | 3 +- tooling/nargo/src/artifacts/debug.rs | 3 +- tooling/nargo/src/artifacts/debug_vars.rs | 72 +++++----- tooling/nargo/src/ops/compile.rs | 6 +- tooling/nargo_cli/src/cli/debug_cmd.rs | 10 +- 14 files changed, 287 insertions(+), 206 deletions(-) diff --git a/compiler/noirc_errors/src/debug_info.rs b/compiler/noirc_errors/src/debug_info.rs index 842fcab75af..25722aac57f 100644 --- a/compiler/noirc_errors/src/debug_info.rs +++ b/compiler/noirc_errors/src/debug_info.rs @@ -21,9 +21,20 @@ use serde::{ de::Error as DeserializationError, ser::Error as SerializationError, Deserialize, Serialize, }; -pub type Variables = Vec<(u32, (String, u32))>; -pub type Types = Vec<(u32, PrintableType)>; -pub type VariableTypes = (Variables, Types); +#[derive(Debug, Clone, Copy, Eq, PartialEq, Hash, PartialOrd, Ord, Deserialize, Serialize)] +pub struct DebugVarId(pub u32); + +#[derive(Debug, Clone, Copy, Eq, PartialEq, Hash, PartialOrd, Ord, Deserialize, Serialize)] +pub struct DebugTypeId(pub u32); + +#[derive(Debug, Clone, Hash, Deserialize, Serialize)] +pub struct DebugVariable { + pub name: String, + pub debug_type_id: DebugTypeId, +} + +pub type DebugVariables = BTreeMap; +pub type DebugTypes = BTreeMap; #[serde_as] #[derive(Default, Debug, Clone, Deserialize, Serialize)] @@ -33,8 +44,8 @@ pub struct DebugInfo { /// that they should be serialized to/from strings. #[serde_as(as = "BTreeMap")] pub locations: BTreeMap>, - pub variables: HashMap, // var_id => (name, type_id) - pub types: HashMap, // type_id => printable type + pub variables: DebugVariables, + pub types: DebugTypes, } /// Holds OpCodes Counts for Acir and Brillig Opcodes @@ -48,13 +59,10 @@ pub struct OpCodesCount { impl DebugInfo { pub fn new( locations: BTreeMap>, - var_types: VariableTypes, + variables: DebugVariables, + types: DebugTypes, ) -> Self { - Self { - locations, - variables: var_types.0.into_iter().collect(), - types: var_types.1.into_iter().collect(), - } + Self { locations, variables, types } } /// Updates the locations map when the [`Circuit`][acvm::acir::circuit::Circuit] is modified. diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index f59a65024c2..694351006c5 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -86,7 +86,8 @@ pub fn create_circuit( enable_brillig_logging: bool, force_brillig_output: bool, ) -> Result<(Circuit, DebugInfo, Vec, Vec, Vec), RuntimeError> { - let debug_var_types = program.debug_var_types.clone(); + let debug_variables = program.debug_variables.clone(); + let debug_types = program.debug_types.clone(); let func_sig = program.main_function_signature.clone(); let mut generated_acir = optimize_into_acir( program, @@ -126,7 +127,7 @@ pub fn create_circuit( .map(|(index, locations)| (index, locations.into_iter().collect())) .collect(); - let mut debug_info = DebugInfo::new(locations, debug_var_types); + let mut debug_info = DebugInfo::new(locations, debug_variables, debug_types); // Perform any ACIR-level optimizations let (optimized_circuit, transformation_map) = acvm::compiler::optimize(circuit); diff --git a/compiler/noirc_frontend/src/debug/mod.rs b/compiler/noirc_frontend/src/debug/mod.rs index 9b0b131869e..9f182d2baa2 100644 --- a/compiler/noirc_frontend/src/debug/mod.rs +++ b/compiler/noirc_frontend/src/debug/mod.rs @@ -10,16 +10,30 @@ use std::collections::VecDeque; const MAX_MEMBER_ASSIGN_DEPTH: usize = 8; +#[derive(Debug, Clone, Copy, Eq, PartialEq, Hash)] +pub struct SourceVarId(pub u32); + +#[derive(Debug, Clone, Copy, Eq, PartialEq, Hash)] +pub struct SourceFieldId(pub u32); + +/// This structure is used to collect information about variables to track +/// for debugging during the instrumentation injection phase. #[derive(Debug, Clone)] -pub struct DebugState { - pub variables: HashMap, // var_id => var_name - pub field_names: HashMap, +pub struct DebugInstrumenter { + // all collected variable names while instrumenting the source for variable tracking + pub variables: HashMap, + + // all field names referenced when assigning to a member of a variable + pub field_names: HashMap, + next_var_id: u32, next_field_name_id: u32, - scope: Vec>, // var_name => var_id + + // last seen variable names and their IDs grouped by scope + scope: Vec>, } -impl Default for DebugState { +impl Default for DebugInstrumenter { fn default() -> Self { Self { variables: HashMap::default(), @@ -31,21 +45,32 @@ impl Default for DebugState { } } -impl DebugState { - fn insert_var(&mut self, var_name: &str) -> u32 { - let var_id = self.next_var_id; +impl DebugInstrumenter { + pub fn instrument_module(&mut self, module: &mut ParsedModule) { + module.items.iter_mut().for_each(|item| { + if let Item { kind: ItemKind::Function(f), .. } = item { + self.walk_fn(&mut f.def); + } + }); + // this part absolutely must happen after ast traversal above + // so that oracle functions don't get wrapped, resulting in infinite recursion: + self.insert_state_set_oracle(module, 8); + } + + fn insert_var(&mut self, var_name: &str) -> SourceVarId { + let var_id = SourceVarId(self.next_var_id); self.next_var_id += 1; self.variables.insert(var_id, var_name.to_string()); self.scope.last_mut().unwrap().insert(var_name.to_string(), var_id); var_id } - fn lookup_var(&self, var_name: &str) -> Option { + fn lookup_var(&self, var_name: &str) -> Option { self.scope.iter().rev().find_map(|vars| vars.get(var_name).copied()) } - fn insert_field_name(&mut self, field_name: &str) -> u32 { - let field_name_id = self.next_field_name_id; + fn insert_field_name(&mut self, field_name: &str) -> SourceFieldId { + let field_name_id = SourceFieldId(self.next_field_name_id); self.next_field_name_id += 1; self.field_names.insert(field_name_id, field_name.to_string()); field_name_id @@ -134,17 +159,6 @@ impl DebugState { statements.push(last_stmt); } - pub fn insert_symbols(&mut self, module: &mut ParsedModule) { - module.items.iter_mut().for_each(|item| { - if let Item { kind: ItemKind::Function(f), .. } = item { - self.walk_fn(&mut f.def); - } - }); - // this part absolutely must happen after ast traversal above - // so that oracle functions don't get wrapped, resulting in infinite recursion: - self.insert_state_set_oracle(module, 8); - } - fn walk_let_statement(&mut self, let_stmt: &ast::LetStatement, span: &Span) -> ast::Statement { // rewrites let statements written like this: // let (((a,b,c),D { d }),e,f) = x; @@ -255,7 +269,7 @@ impl DebugState { ast::LValue::MemberAccess { object, field_name } => { cursor = object; let field_name_id = self.insert_field_name(&field_name.0.contents); - indexes.push(sint_expr(-(field_name_id as i128), expression_span)); + indexes.push(sint_expr(-(field_name_id.0 as i128), expression_span)); } ast::LValue::Index { index, array } => { cursor = array; @@ -479,7 +493,7 @@ pub fn build_debug_crate_file() -> String { .join("\n") } -fn build_assign_var_stmt(var_id: u32, expr: ast::Expression) -> ast::Statement { +fn build_assign_var_stmt(var_id: SourceVarId, expr: ast::Expression) -> ast::Statement { let span = expr.span; let kind = ast::ExpressionKind::Call(Box::new(ast::CallExpression { func: Box::new(ast::Expression { @@ -490,12 +504,12 @@ fn build_assign_var_stmt(var_id: u32, expr: ast::Expression) -> ast::Statement { }), span, }), - arguments: vec![uint_expr(var_id as u128, span), expr], + arguments: vec![uint_expr(var_id.0 as u128, span), expr], })); ast::Statement { kind: ast::StatementKind::Semi(ast::Expression { kind, span }), span } } -fn build_drop_var_stmt(var_id: u32, span: Span) -> ast::Statement { +fn build_drop_var_stmt(var_id: SourceVarId, span: Span) -> ast::Statement { let kind = ast::ExpressionKind::Call(Box::new(ast::CallExpression { func: Box::new(ast::Expression { kind: ast::ExpressionKind::Variable(ast::Path { @@ -505,13 +519,13 @@ fn build_drop_var_stmt(var_id: u32, span: Span) -> ast::Statement { }), span, }), - arguments: vec![uint_expr(var_id as u128, span)], + arguments: vec![uint_expr(var_id.0 as u128, span)], })); ast::Statement { kind: ast::StatementKind::Semi(ast::Expression { kind, span }), span } } fn build_assign_member_stmt( - var_id: u32, + var_id: SourceVarId, indexes: &[ast::Expression], expr: &ast::Expression, ) -> ast::Statement { @@ -530,7 +544,7 @@ fn build_assign_member_stmt( span, }), arguments: [ - vec![uint_expr(var_id as u128, span)], + vec![uint_expr(var_id.0 as u128, span)], vec![expr.clone()], indexes.iter().rev().cloned().collect(), ] diff --git a/compiler/noirc_frontend/src/hir/mod.rs b/compiler/noirc_frontend/src/hir/mod.rs index f3d01ee7ccd..1a758a37c3a 100644 --- a/compiler/noirc_frontend/src/hir/mod.rs +++ b/compiler/noirc_frontend/src/hir/mod.rs @@ -4,7 +4,7 @@ pub mod resolution; pub mod scope; pub mod type_check; -use crate::debug::DebugState; +use crate::debug::DebugInstrumenter; use crate::graph::{CrateGraph, CrateId}; use crate::hir_def::function::FuncMeta; use crate::node_interner::{FuncId, NodeInterner, StructId}; @@ -32,7 +32,7 @@ pub struct Context<'file_manager, 'parsed_files> { // is read-only however, once it has been passed to the Context. pub file_manager: Cow<'file_manager, FileManager>, - pub debug_state: DebugState, + pub debug_state: DebugInstrumenter, /// A map of each file that already has been visited from a prior `mod foo;` declaration. /// This is used to issue an error if a second `mod foo;` is declared to the same file. @@ -59,7 +59,7 @@ impl Context<'_, '_> { visited_files: BTreeMap::new(), crate_graph: CrateGraph::default(), file_manager: Cow::Owned(file_manager), - debug_state: DebugState::default(), + debug_state: DebugInstrumenter::default(), parsed_files: Cow::Owned(parsed_files), } } @@ -74,7 +74,7 @@ impl Context<'_, '_> { visited_files: BTreeMap::new(), crate_graph: CrateGraph::default(), file_manager: Cow::Borrowed(file_manager), - debug_state: DebugState::default(), + debug_state: DebugInstrumenter::default(), parsed_files: Cow::Borrowed(parsed_files), } } diff --git a/compiler/noirc_frontend/src/monomorphization/ast.rs b/compiler/noirc_frontend/src/monomorphization/ast.rs index 40829f7b1e2..2af4c6c90a8 100644 --- a/compiler/noirc_frontend/src/monomorphization/ast.rs +++ b/compiler/noirc_frontend/src/monomorphization/ast.rs @@ -1,10 +1,12 @@ use acvm::FieldElement; use iter_extended::vecmap; -use noirc_errors::Location; +use noirc_errors::{ + debug_info::{DebugTypes, DebugVariables}, + Location, +}; use crate::{ - hir_def::function::FunctionSignature, monomorphization::debug_types, BinaryOpKind, - Distinctness, Signedness, Visibility, + hir_def::function::FunctionSignature, BinaryOpKind, Distinctness, Signedness, Visibility, }; /// The monomorphized AST is expression-based, all statements are also @@ -247,7 +249,8 @@ pub struct Program { pub return_distinctness: Distinctness, pub return_location: Option, pub return_visibility: Visibility, - pub debug_var_types: debug_types::VariableTypes, + pub debug_variables: DebugVariables, + pub debug_types: DebugTypes, } impl Program { @@ -257,7 +260,8 @@ impl Program { return_distinctness: Distinctness, return_location: Option, return_visibility: Visibility, - debug_var_types: debug_types::VariableTypes, + debug_variables: DebugVariables, + debug_types: DebugTypes, ) -> Program { Program { functions, @@ -265,7 +269,8 @@ impl Program { return_distinctness, return_location, return_visibility, - debug_var_types, + debug_variables, + debug_types, } } diff --git a/compiler/noirc_frontend/src/monomorphization/debug.rs b/compiler/noirc_frontend/src/monomorphization/debug.rs index e38982eeab2..d36816e3d37 100644 --- a/compiler/noirc_frontend/src/monomorphization/debug.rs +++ b/compiler/noirc_frontend/src/monomorphization/debug.rs @@ -1,7 +1,11 @@ use iter_extended::vecmap; +use noirc_errors::debug_info::DebugVarId; +use noirc_errors::Location; use noirc_printable_type::PrintableType; +use crate::debug::{SourceFieldId, SourceVarId}; use crate::hir_def::expr::*; +use crate::node_interner::ExprId; use super::ast::{Expression, Ident}; use super::Monomorphizer; @@ -11,14 +15,26 @@ const DEBUG_VAR_ID_ARG_SLOT: usize = 0; const DEBUG_VALUE_ARG_SLOT: usize = 1; const DEBUG_MEMBER_FIELD_INDEX_ARG_SLOT: usize = 2; +impl From for SourceVarId { + fn from(var_id: u128) -> Self { + Self(var_id as u32) + } +} + +impl From for SourceFieldId { + fn from(field_id: u128) -> Self { + Self(field_id as u32) + } +} + impl<'interner> Monomorphizer<'interner> { /// Patch instrumentation calls inserted for debugging. This will record /// tracked variables and their types, and assign them an ID to use at - /// runtime. This ID is different from the ID assigned at instrumentation - /// time because at that point we haven't fully resolved the types for - /// generic functions. So a single generic function may be instantiated - /// multiple times with its tracked variables being of different types for - /// each instance at runtime. + /// runtime. This ID is different from the source ID assigned at + /// instrumentation time because at that point we haven't fully resolved the + /// types for generic functions. So a single generic function may be + /// instantiated multiple times with its tracked variables being of + /// different types for each instance at runtime. pub(super) fn patch_debug_instrumentation_call( &mut self, call: &HirCallExpression, @@ -38,49 +54,49 @@ impl<'interner> Monomorphizer<'interner> { } /// Update instrumentation code inserted on variable assignment. We need to - /// register the variable instance, its type and replace the temporary ID - /// (fe_var_id) with the ID of the registration. Multiple registrations of - /// the same variable are possible if using generic functions, hence the - /// temporary ID created when injecting the instrumentation code can map to - /// multiple IDs at runtime. + /// register the variable instance, its type and replace the source_var_id + /// with the ID of the registration. Multiple registrations of the same + /// variable are possible if using generic functions, hence the temporary ID + /// created when injecting the instrumentation code can map to multiple IDs + /// at runtime. fn patch_debug_var_assign(&mut self, call: &HirCallExpression, arguments: &mut [Expression]) { let hir_arguments = vecmap(&call.arguments, |id| self.interner.expression(id)); let var_id_arg = hir_arguments.get(DEBUG_VAR_ID_ARG_SLOT); - let Some(HirExpression::Literal(HirLiteral::Integer(fe_var_id, _))) = var_id_arg else { - unreachable!("Missing FE var ID in __debug_var_assign call"); + let Some(HirExpression::Literal(HirLiteral::Integer(source_var_id, _))) = var_id_arg else { + unreachable!("Missing source_var_id in __debug_var_assign call"); }; // instantiate tracked variable for the value type and associate it with // the ID used by the injected instrumentation code let var_type = self.interner.id_type(call.arguments[DEBUG_VALUE_ARG_SLOT]); - let fe_var_id = fe_var_id.to_u128() as u32; + let source_var_id = source_var_id.to_u128().into(); // then update the ID used for tracking at runtime - let var_id = self.debug_types.insert_var(fe_var_id, var_type); + let var_id = self.debug_type_tracker.insert_var(source_var_id, var_type); let interned_var_id = self.intern_var_id(var_id, &call.location); arguments[DEBUG_VAR_ID_ARG_SLOT] = self.expr(interned_var_id); } /// Update instrumentation code for a variable being dropped out of scope. - /// Given the fe_var_id we search for the last assigned runtime variable ID - /// and replace it instead. + /// Given the source_var_id we search for the last assigned debug var_id and + /// replace it instead. fn patch_debug_var_drop(&mut self, call: &HirCallExpression, arguments: &mut [Expression]) { let hir_arguments = vecmap(&call.arguments, |id| self.interner.expression(id)); let var_id_arg = hir_arguments.get(DEBUG_VAR_ID_ARG_SLOT); - let Some(HirExpression::Literal(HirLiteral::Integer(fe_var_id, _))) = var_id_arg else { - unreachable!("Missing FE var ID in __debug_var_drop call"); + let Some(HirExpression::Literal(HirLiteral::Integer(source_var_id, _))) = var_id_arg else { + unreachable!("Missing source_var_id in __debug_var_drop call"); }; // update variable ID for tracked drops (ie. when the var goes out of scope) - let fe_var_id = fe_var_id.to_u128() as u32; + let source_var_id = source_var_id.to_u128().into(); let var_id = self - .debug_types - .get_var_id(fe_var_id) + .debug_type_tracker + .get_var_id(source_var_id) .unwrap_or_else(|| unreachable!("failed to find debug variable")); let interned_var_id = self.intern_var_id(var_id, &call.location); arguments[DEBUG_VAR_ID_ARG_SLOT] = self.expr(interned_var_id); } /// Update instrumentation code inserted when assigning to a member of an - /// existing variable. Same as above for replacing the fe_var_id, but also + /// existing variable. Same as above for replacing the source_var_id, but also /// we need to resolve the path and the type of the member being assigned. /// For this last part, we need to resolve the mapping from field names in /// structs to positions in the runtime tuple, since all structs are @@ -93,16 +109,16 @@ impl<'interner> Monomorphizer<'interner> { ) { let hir_arguments = vecmap(&call.arguments, |id| self.interner.expression(id)); let var_id_arg = hir_arguments.get(DEBUG_VAR_ID_ARG_SLOT); - let Some(HirExpression::Literal(HirLiteral::Integer(fe_var_id, _))) = var_id_arg else { - unreachable!("Missing FE var ID in __debug_member_assign call"); + let Some(HirExpression::Literal(HirLiteral::Integer(source_var_id, _))) = var_id_arg else { + unreachable!("Missing source_var_id in __debug_member_assign call"); }; // update variable member assignments - let fe_var_id = fe_var_id.to_u128() as u32; + let source_var_id = source_var_id.to_u128().into(); let var_type = self - .debug_types - .get_type(fe_var_id) - .unwrap_or_else(|| panic!("type not found for fe_var_id={fe_var_id}")) + .debug_type_tracker + .get_type(source_var_id) + .unwrap_or_else(|| panic!("type not found for {source_var_id:?}")) .clone(); let mut cursor_type = &var_type; for i in 0..arity { @@ -113,11 +129,12 @@ impl<'interner> Monomorphizer<'interner> { if *i_neg { // We use negative indices at instrumentation time to indicate // and reference member accesses by name which cannot be - // resolved until we have a type. This code path is also used - // for tuple member access. + // resolved until we have a type. This strategy is also used + // for tuple member access because syntactically they are + // the same as named field accesses. let field_index = self - .debug_types - .resolve_field_index(index as u32, cursor_type) + .debug_type_tracker + .resolve_field_index(index.into(), cursor_type) .unwrap_or_else(|| { unreachable!("failed to resolve {i}-th member indirection on type {cursor_type:?}") }); @@ -144,12 +161,20 @@ impl<'interner> Monomorphizer<'interner> { } let var_id = self - .debug_types - .get_var_id(fe_var_id) + .debug_type_tracker + .get_var_id(source_var_id) .unwrap_or_else(|| unreachable!("failed to find debug variable")); let interned_var_id = self.intern_var_id(var_id, &call.location); arguments[DEBUG_VAR_ID_ARG_SLOT] = self.expr(interned_var_id); } + + fn intern_var_id(&mut self, var_id: DebugVarId, location: &Location) -> ExprId { + let var_id_literal = HirLiteral::Integer((var_id.0 as u128).into(), false); + let expr_id = self.interner.push_expr(HirExpression::Literal(var_id_literal)); + self.interner.push_expr_type(&expr_id, crate::Type::FieldElement); + self.interner.push_expr_location(expr_id, location.span, location.file); + expr_id + } } fn element_type_at_index(ptype: &PrintableType, i: usize) -> &PrintableType { diff --git a/compiler/noirc_frontend/src/monomorphization/debug_types.rs b/compiler/noirc_frontend/src/monomorphization/debug_types.rs index fd0298b33e5..dddaa349399 100644 --- a/compiler/noirc_frontend/src/monomorphization/debug_types.rs +++ b/compiler/noirc_frontend/src/monomorphization/debug_types.rs @@ -1,5 +1,10 @@ -use crate::{debug::DebugState, hir_def::types::Type}; -pub use noirc_errors::debug_info::{Types, VariableTypes, Variables}; +use crate::{ + debug::{DebugInstrumenter, SourceFieldId, SourceVarId}, + hir_def::types::Type, +}; +use noirc_errors::debug_info::{ + DebugTypeId, DebugTypes, DebugVarId, DebugVariable, DebugVariables, +}; use noirc_printable_type::PrintableType; use std::collections::HashMap; @@ -10,70 +15,112 @@ use std::collections::HashMap; /// variable. The var_id refers to the ID of the instantiated variable which /// will have a valid type. #[derive(Debug, Clone, Default)] -pub struct DebugTypes { - debug_variables: HashMap, - debug_field_names: HashMap, - fe_to_vars: HashMap, // fe_var_id => var_id - variables: HashMap, // var_id => (var_name, type_id) - types: HashMap, - id_to_type: HashMap, - next_type_id: u32, +pub struct DebugTypeTracker { + // Variable names collected during instrumentation injection + source_variables: HashMap, + + // Field names used for member access collected during instrumentation injection + source_field_names: HashMap, + + // Current instances of tracked variables from the ID given during + // instrumentation. The tracked var_id will change for each source_var_id + // when compiling generic functions. + source_to_debug_vars: HashMap, + + // All instances of tracked variables + variables: HashMap, + + // Types of tracked variables + types: HashMap, + types_reverse: HashMap, + next_var_id: u32, + next_type_id: u32, } -impl DebugTypes { - pub fn build_from_debug_state(debug_state: &DebugState) -> Self { - DebugTypes { - debug_variables: debug_state.variables.clone(), - debug_field_names: debug_state.field_names.clone(), - ..DebugTypes::default() +impl DebugTypeTracker { + pub fn build_from_debug_instrumenter(instrumenter: &DebugInstrumenter) -> Self { + DebugTypeTracker { + source_variables: instrumenter.variables.clone(), + source_field_names: instrumenter.field_names.clone(), + ..DebugTypeTracker::default() } } - pub fn resolve_field_index(&self, field_id: u32, cursor_type: &PrintableType) -> Option { - self.debug_field_names + pub fn extract_vars_and_types(&self) -> (DebugVariables, DebugTypes) { + let debug_variables = self + .variables + .clone() + .into_iter() + .map(|(var_id, (source_var_id, type_id))| { + ( + var_id, + DebugVariable { + name: self.source_variables.get(&source_var_id).cloned().unwrap_or_else( + || { + unreachable!( + "failed to retrieve variable name for {source_var_id:?}" + ); + }, + ), + debug_type_id: type_id, + }, + ) + }) + .collect(); + let debug_types = self.types.clone().into_iter().collect(); + + (debug_variables, debug_types) + } + + pub fn resolve_field_index( + &self, + field_id: SourceFieldId, + cursor_type: &PrintableType, + ) -> Option { + self.source_field_names .get(&field_id) .and_then(|field_name| get_field(cursor_type, field_name)) } - pub fn insert_var(&mut self, fe_var_id: u32, var_type: Type) -> u32 { - let var_name = self - .debug_variables - .get(&fe_var_id) - .unwrap_or_else(|| unreachable!("cannot find name for debug variable {fe_var_id}")); + pub fn insert_var(&mut self, source_var_id: SourceVarId, var_type: Type) -> DebugVarId { + if !self.source_variables.contains_key(&source_var_id) { + unreachable!("cannot find source debug variable {source_var_id:?}"); + } let ptype: PrintableType = var_type.follow_bindings().into(); - let type_id = self.types.get(&ptype).cloned().unwrap_or_else(|| { - let type_id = self.next_type_id; + let type_id = self.types_reverse.get(&ptype).copied().unwrap_or_else(|| { + let type_id = DebugTypeId(self.next_type_id); self.next_type_id += 1; - self.types.insert(ptype.clone(), type_id); - self.id_to_type.insert(type_id, ptype); + self.types_reverse.insert(ptype.clone(), type_id); + self.types.insert(type_id, ptype); type_id }); - let existing_var_id = self.fe_to_vars.get(&fe_var_id).and_then(|var_id| { + // check if we need to instantiate the var with a new type + let existing_var_id = self.source_to_debug_vars.get(&source_var_id).and_then(|var_id| { let (_, existing_type_id) = self.variables.get(var_id).unwrap(); (*existing_type_id == type_id).then_some(var_id) }); if let Some(var_id) = existing_var_id { *var_id } else { - let var_id = self.next_var_id; + let var_id = DebugVarId(self.next_var_id); self.next_var_id += 1; - self.variables.insert(var_id, (var_name.to_string(), type_id)); - self.fe_to_vars.insert(fe_var_id, var_id); + self.variables.insert(var_id, (source_var_id, type_id)); + self.source_to_debug_vars.insert(source_var_id, var_id); var_id } } - pub fn get_var_id(&self, fe_var_id: u32) -> Option { - self.fe_to_vars.get(&fe_var_id).copied() + pub fn get_var_id(&self, source_var_id: SourceVarId) -> Option { + self.source_to_debug_vars.get(&source_var_id).copied() } - pub fn get_type(&self, fe_var_id: u32) -> Option<&PrintableType> { - self.fe_to_vars - .get(&fe_var_id) + pub fn get_type(&self, source_var_id: SourceVarId) -> Option<&PrintableType> { + self.source_to_debug_vars + .get(&source_var_id) .and_then(|var_id| self.variables.get(var_id)) - .and_then(|(_, type_id)| self.id_to_type.get(type_id)) + .and_then(|(_, type_id)| self.types.get(type_id)) } } @@ -88,12 +135,3 @@ fn get_field(ptype: &PrintableType, field_name: &str) -> Option { _ => None, } } - -impl From for VariableTypes { - fn from(val: DebugTypes) -> Self { - ( - val.variables.into_iter().collect(), - val.types.into_iter().map(|(ptype, type_id)| (type_id, ptype)).collect(), - ) - } -} diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index 59b7b9418e7..e5b0555d93e 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -18,7 +18,7 @@ use std::{ }; use crate::{ - debug::DebugState, + debug::DebugInstrumenter, hir_def::{ expr::*, function::{FuncMeta, FunctionSignature, Parameters}, @@ -32,7 +32,7 @@ use crate::{ }; use self::ast::{Definition, FuncId, Function, LocalId, Program}; -use self::debug_types::DebugTypes; +use self::debug_types::DebugTypeTracker; pub mod ast; mod debug; @@ -82,7 +82,7 @@ struct Monomorphizer<'interner> { return_location: Option, - debug_types: DebugTypes, + debug_type_tracker: DebugTypeTracker, } type HirType = crate::Type; @@ -100,16 +100,16 @@ type HirType = crate::Type; /// but it can also be, for example, an arbitrary test function for running `nargo test`. #[tracing::instrument(level = "trace", skip(main, interner))] pub fn monomorphize(main: node_interner::FuncId, interner: &mut NodeInterner) -> Program { - monomorphize_debug(main, interner, &DebugState::default()) + monomorphize_debug(main, interner, &DebugInstrumenter::default()) } pub fn monomorphize_debug( main: node_interner::FuncId, interner: &mut NodeInterner, - debug_state: &DebugState, + debug_instrumenter: &DebugInstrumenter, ) -> Program { - let debug_types = DebugTypes::build_from_debug_state(debug_state); - let mut monomorphizer = Monomorphizer::new(interner, debug_types); + let debug_type_tracker = DebugTypeTracker::build_from_debug_instrumenter(debug_instrumenter); + let mut monomorphizer = Monomorphizer::new(interner, debug_type_tracker); let function_sig = monomorphizer.compile_main(main); while !monomorphizer.queue.is_empty() { @@ -127,18 +127,20 @@ pub fn monomorphize_debug( let FuncMeta { return_distinctness, return_visibility, .. } = monomorphizer.interner.function_meta(&main); + let (debug_variables, debug_types) = monomorphizer.debug_type_tracker.extract_vars_and_types(); Program::new( functions, function_sig, *return_distinctness, monomorphizer.return_location, *return_visibility, - monomorphizer.debug_types.into(), + debug_variables, + debug_types, ) } impl<'interner> Monomorphizer<'interner> { - fn new(interner: &'interner mut NodeInterner, debug_types: DebugTypes) -> Self { + fn new(interner: &'interner mut NodeInterner, debug_type_tracker: DebugTypeTracker) -> Self { Monomorphizer { globals: HashMap::new(), locals: HashMap::new(), @@ -150,7 +152,7 @@ impl<'interner> Monomorphizer<'interner> { lambda_envs_stack: Vec::new(), is_range_loop: false, return_location: None, - debug_types, + debug_type_tracker, } } @@ -926,15 +928,6 @@ impl<'interner> Monomorphizer<'interner> { }) } - fn intern_var_id(&mut self, var_id: u32, location: &Location) -> node_interner::ExprId { - let expr_id = self - .interner - .push_expr(HirExpression::Literal(HirLiteral::Integer((var_id as u128).into(), false))); - self.interner.push_expr_type(&expr_id, Type::FieldElement); - self.interner.push_expr_location(expr_id, location.span, location.file); - expr_id - } - fn function_call( &mut self, call: HirCallExpression, diff --git a/tooling/debugger/src/foreign_calls.rs b/tooling/debugger/src/foreign_calls.rs index 71f45f5b7e1..db52c39b437 100644 --- a/tooling/debugger/src/foreign_calls.rs +++ b/tooling/debugger/src/foreign_calls.rs @@ -6,6 +6,7 @@ use nargo::{ artifacts::debug::{DebugArtifact, DebugVars}, ops::{DefaultForeignCallExecutor, ForeignCallExecutor}, }; +use noirc_errors::debug_info::DebugVarId; use noirc_printable_type::{ForeignCallError, PrintableType, PrintableValue}; pub(crate) enum DebugForeignCall { @@ -92,6 +93,10 @@ impl DebugForeignCallExecutor for DefaultDebugForeignCallExecutor { } } +fn debug_var_id(value: &Value) -> DebugVarId { + DebugVarId(value.to_u128() as u32) +} + impl ForeignCallExecutor for DefaultDebugForeignCallExecutor { fn execute( &mut self, @@ -102,7 +107,7 @@ impl ForeignCallExecutor for DefaultDebugForeignCallExecutor { Some(DebugForeignCall::VarAssign) => { let fcp_var_id = &foreign_call.inputs[0]; if let ForeignCallParam::Single(var_id_value) = fcp_var_id { - let var_id = var_id_value.to_u128() as u32; + let var_id = debug_var_id(var_id_value); let values: Vec = foreign_call.inputs[1..].iter().flat_map(|x| x.values()).collect(); self.debug_vars.assign_var(var_id, &values); @@ -112,7 +117,7 @@ impl ForeignCallExecutor for DefaultDebugForeignCallExecutor { Some(DebugForeignCall::VarDrop) => { let fcp_var_id = &foreign_call.inputs[0]; if let ForeignCallParam::Single(var_id_value) = fcp_var_id { - let var_id = var_id_value.to_u128() as u32; + let var_id = debug_var_id(var_id_value); self.debug_vars.drop_var(var_id); } Ok(ForeignCallResult { values: vec![] }) @@ -120,7 +125,7 @@ impl ForeignCallExecutor for DefaultDebugForeignCallExecutor { Some(DebugForeignCall::MemberAssign(arity)) => { if let Some(ForeignCallParam::Single(var_id_value)) = foreign_call.inputs.get(0) { let arity = arity as usize; - let var_id = var_id_value.to_u128() as u32; + let var_id = debug_var_id(var_id_value); let n = foreign_call.inputs.len(); let indexes: Vec = foreign_call.inputs[(n - arity)..n] .iter() @@ -145,7 +150,7 @@ impl ForeignCallExecutor for DefaultDebugForeignCallExecutor { let fcp_var_id = &foreign_call.inputs[0]; let fcp_value = &foreign_call.inputs[1]; if let ForeignCallParam::Single(var_id_value) = fcp_var_id { - let var_id = var_id_value.to_u128() as u32; + let var_id = debug_var_id(var_id_value); self.debug_vars.assign_deref(var_id, &fcp_value.values()); } Ok(ForeignCallResult { values: vec![] }) diff --git a/tooling/debugger/src/source_code_printer.rs b/tooling/debugger/src/source_code_printer.rs index e430a8f0330..b5ffdb12d01 100644 --- a/tooling/debugger/src/source_code_printer.rs +++ b/tooling/debugger/src/source_code_printer.rs @@ -269,7 +269,8 @@ mod tests { let mut opcode_locations = BTreeMap::>::new(); opcode_locations.insert(OpcodeLocation::Acir(42), vec![loc]); - let debug_symbols = vec![DebugInfo::new(opcode_locations, (vec![], vec![]))]; + let debug_symbols = + vec![DebugInfo::new(opcode_locations, BTreeMap::default(), BTreeMap::default())]; let debug_artifact = DebugArtifact::new(debug_symbols, &fm); let location_rendered: Vec<_> = render_location(&debug_artifact, &loc).collect(); diff --git a/tooling/nargo/src/artifacts/debug.rs b/tooling/nargo/src/artifacts/debug.rs index f0cf84bdfc8..c4c25081325 100644 --- a/tooling/nargo/src/artifacts/debug.rs +++ b/tooling/nargo/src/artifacts/debug.rs @@ -239,7 +239,8 @@ mod tests { let mut opcode_locations = BTreeMap::>::new(); opcode_locations.insert(OpcodeLocation::Acir(42), vec![loc]); - let debug_symbols = vec![DebugInfo::new(opcode_locations, (vec![], vec![]))]; + let debug_symbols = + vec![DebugInfo::new(opcode_locations, BTreeMap::default(), BTreeMap::default())]; let debug_artifact = DebugArtifact::new(debug_symbols, &fm); let location_in_line = debug_artifact.location_in_line(loc).expect("Expected a range"); diff --git a/tooling/nargo/src/artifacts/debug_vars.rs b/tooling/nargo/src/artifacts/debug_vars.rs index 9a93143bc13..b5559ca53c8 100644 --- a/tooling/nargo/src/artifacts/debug_vars.rs +++ b/tooling/nargo/src/artifacts/debug_vars.rs @@ -1,72 +1,63 @@ use acvm::brillig_vm::brillig::Value; +use noirc_errors::debug_info::{ + DebugTypeId, DebugTypes, DebugVarId, DebugVariable, DebugVariables, +}; use noirc_printable_type::{decode_value, PrintableType, PrintableValue}; use std::collections::{HashMap, HashSet}; #[derive(Debug, Default, Clone)] pub struct DebugVars { - id_to_name: HashMap, - active: HashSet, - id_to_value: HashMap, // TODO: something more sophisticated for lexical levels - id_to_type: HashMap, - types: HashMap, + variables: HashMap, + types: HashMap, + active: HashSet, + values: HashMap, } impl DebugVars { - pub fn new(vars: &HashMap) -> Self { - Self { id_to_name: vars.clone(), ..Self::default() } - } - pub fn get_variables(&self) -> Vec<(&str, &PrintableValue, &PrintableType)> { self.active .iter() .filter_map(|var_id| { - self.id_to_name + self.variables .get(var_id) - .and_then(|name| self.id_to_value.get(var_id).map(|value| (name, value))) - .and_then(|(name, value)| { - self.id_to_type.get(var_id).map(|type_id| (name, value, type_id)) - }) - .and_then(|(name, value, type_id)| { - self.types.get(type_id).map(|ptype| (name.as_str(), value, ptype)) + .and_then(|debug_var| { + let Some(value) = self.values.get(var_id) else { return None; }; + let Some(ptype) = self.types.get(&debug_var.debug_type_id) else { return None; }; + Some((debug_var.name.as_str(), value, ptype)) }) }) .collect() } - pub fn insert_variables(&mut self, vars: &HashMap) { - vars.iter().for_each(|(var_id, (var_name, var_type))| { - self.id_to_name.insert(*var_id, var_name.clone()); - self.id_to_type.insert(*var_id, *var_type); - }); + pub fn insert_variables(&mut self, vars: &DebugVariables) { + self.variables.extend(vars.clone().into_iter()); } - pub fn insert_types(&mut self, types: &HashMap) { - types.iter().for_each(|(type_id, ptype)| { - self.types.insert(*type_id, ptype.clone()); - }); + pub fn insert_types(&mut self, types: &DebugTypes) { + self.types.extend(types.clone().into_iter()); } - pub fn assign_var(&mut self, var_id: u32, values: &[Value]) { + pub fn assign_var(&mut self, var_id: DebugVarId, values: &[Value]) { self.active.insert(var_id); - let type_id = self.id_to_type.get(&var_id).unwrap(); + let type_id = &self.variables.get(&var_id).unwrap().debug_type_id; let ptype = self.types.get(type_id).unwrap(); - self.id_to_value - .insert(var_id, decode_value(&mut values.iter().map(|v| v.to_field()), ptype)); + self.values.insert(var_id, decode_value(&mut values.iter().map(|v| v.to_field()), ptype)); } - pub fn assign_field(&mut self, var_id: u32, indexes: Vec, values: &[Value]) { + pub fn assign_field(&mut self, var_id: DebugVarId, indexes: Vec, values: &[Value]) { let mut cursor: &mut PrintableValue = self - .id_to_value + .values .get_mut(&var_id) - .unwrap_or_else(|| panic!("value unavailable for var_id {var_id}")); - let cursor_type_id = self - .id_to_type + .unwrap_or_else(|| panic!("value unavailable for var_id {var_id:?}")); + let cursor_type_id = &self + .variables .get(&var_id) - .unwrap_or_else(|| panic!("type id unavailable for var_id {var_id}")); + .unwrap_or_else(|| panic!("variable {var_id:?} not found")) + .debug_type_id; let mut cursor_type = self .types .get(cursor_type_id) - .unwrap_or_else(|| panic!("type unavailable for type id {cursor_type_id}")); + .unwrap_or_else(|| panic!("type unavailable for type id {cursor_type_id:?}")); for index in indexes.iter() { (cursor, cursor_type) = match (cursor, cursor_type) { (PrintableValue::Vec(array), PrintableType::Array { length, typ }) => { @@ -112,16 +103,15 @@ impl DebugVars { self.active.insert(var_id); } - pub fn assign_deref(&mut self, _var_id: u32, _values: &[Value]) { - // TODO + pub fn assign_deref(&mut self, _var_id: DebugVarId, _values: &[Value]) { unimplemented![] } - pub fn get_type(&self, var_id: u32) -> Option<&PrintableType> { - self.id_to_type.get(&var_id).and_then(|type_id| self.types.get(type_id)) + pub fn get_type(&self, var_id: DebugVarId) -> Option<&PrintableType> { + self.variables.get(&var_id).and_then(|debug_var| self.types.get(&debug_var.debug_type_id)) } - pub fn drop_var(&mut self, var_id: u32) { + pub fn drop_var(&mut self, var_id: DebugVarId) { self.active.remove(&var_id); } } diff --git a/tooling/nargo/src/ops/compile.rs b/tooling/nargo/src/ops/compile.rs index a4b39911097..58f72034ad0 100644 --- a/tooling/nargo/src/ops/compile.rs +++ b/tooling/nargo/src/ops/compile.rs @@ -2,7 +2,7 @@ use fm::FileManager; use noirc_driver::{ link_to_debug_crate, CompilationResult, CompileOptions, CompiledContract, CompiledProgram, }; -use noirc_frontend::debug::DebugState; +use noirc_frontend::debug::DebugInstrumenter; use noirc_frontend::hir::ParsedFiles; use crate::errors::CompileError; @@ -78,7 +78,7 @@ pub fn compile_program( package, compile_options, cached_program, - DebugState::default(), + DebugInstrumenter::default(), ) } @@ -88,7 +88,7 @@ pub fn compile_program_with_debug_state( package: &Package, compile_options: &CompileOptions, cached_program: Option, - debug_state: DebugState, + debug_state: DebugInstrumenter, ) -> CompilationResult { let (mut context, crate_id) = prepare_package(file_manager, parsed_files, package); link_to_debug_crate(&mut context, crate_id); diff --git a/tooling/nargo_cli/src/cli/debug_cmd.rs b/tooling/nargo_cli/src/cli/debug_cmd.rs index d21b3c54b10..adfb7fdc9e4 100644 --- a/tooling/nargo_cli/src/cli/debug_cmd.rs +++ b/tooling/nargo_cli/src/cli/debug_cmd.rs @@ -16,7 +16,7 @@ use noirc_abi::InputMap; use noirc_driver::{ file_manager_with_stdlib, CompileOptions, CompiledProgram, NOIR_ARTIFACT_VERSION_STRING, }; -use noirc_frontend::debug::DebugState; +use noirc_frontend::debug::DebugInstrumenter; use noirc_frontend::graph::CrateName; use noirc_frontend::hir::ParsedFiles; @@ -103,7 +103,7 @@ pub(crate) fn instrument_package_files( parsed_files: &mut ParsedFiles, file_manager: &FileManager, package: &Package, -) -> DebugState { +) -> DebugInstrumenter { // Start off at the entry path and read all files in the parent directory. let entry_path_parent = package .entry_path @@ -111,7 +111,7 @@ pub(crate) fn instrument_package_files( .unwrap_or_else(|| panic!("The entry path is expected to be a single file within a directory and so should have a parent {:?}", package.entry_path)) .clone(); - let mut debug_state = DebugState::default(); + let mut debug_instrumenter = DebugInstrumenter::default(); for (file_id, parsed_file) in parsed_files.iter_mut() { let file_path = @@ -119,12 +119,12 @@ pub(crate) fn instrument_package_files( for ancestor in file_path.ancestors() { if ancestor == entry_path_parent { // file is in package - debug_state.insert_symbols(&mut parsed_file.0); + debug_instrumenter.instrument_module(&mut parsed_file.0); } } } - debug_state + debug_instrumenter } fn run_async( From 10ae07a9ed5ba90c9b4418fe46c5c32f35d571d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Thu, 1 Feb 2024 21:23:54 -0500 Subject: [PATCH 13/19] chore: Missing renames from last refactor --- compiler/noirc_driver/src/lib.rs | 2 +- compiler/noirc_frontend/src/hir/mod.rs | 6 +++--- compiler/noirc_frontend/src/monomorphization/mod.rs | 2 +- tooling/nargo/src/ops/compile.rs | 8 ++++---- tooling/nargo/src/ops/mod.rs | 2 +- tooling/nargo_cli/src/cli/dap_cmd.rs | 4 ++-- tooling/nargo_cli/src/cli/debug_cmd.rs | 4 ++-- 7 files changed, 14 insertions(+), 14 deletions(-) diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index 3b6a0c9e8a9..c04b1bf4d21 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -418,7 +418,7 @@ pub fn compile_no_check( force_compile: bool, ) -> Result { let program = if options.instrument_debug { - monomorphize_debug(main_function, &mut context.def_interner, &context.debug_state) + monomorphize_debug(main_function, &mut context.def_interner, &context.debug_instrumenter) } else { monomorphize(main_function, &mut context.def_interner) }; diff --git a/compiler/noirc_frontend/src/hir/mod.rs b/compiler/noirc_frontend/src/hir/mod.rs index 1a758a37c3a..4d3800f1a50 100644 --- a/compiler/noirc_frontend/src/hir/mod.rs +++ b/compiler/noirc_frontend/src/hir/mod.rs @@ -32,7 +32,7 @@ pub struct Context<'file_manager, 'parsed_files> { // is read-only however, once it has been passed to the Context. pub file_manager: Cow<'file_manager, FileManager>, - pub debug_state: DebugInstrumenter, + pub debug_instrumenter: DebugInstrumenter, /// A map of each file that already has been visited from a prior `mod foo;` declaration. /// This is used to issue an error if a second `mod foo;` is declared to the same file. @@ -59,7 +59,7 @@ impl Context<'_, '_> { visited_files: BTreeMap::new(), crate_graph: CrateGraph::default(), file_manager: Cow::Owned(file_manager), - debug_state: DebugInstrumenter::default(), + debug_instrumenter: DebugInstrumenter::default(), parsed_files: Cow::Owned(parsed_files), } } @@ -74,7 +74,7 @@ impl Context<'_, '_> { visited_files: BTreeMap::new(), crate_graph: CrateGraph::default(), file_manager: Cow::Borrowed(file_manager), - debug_state: DebugInstrumenter::default(), + debug_instrumenter: DebugInstrumenter::default(), parsed_files: Cow::Borrowed(parsed_files), } } diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index e5b0555d93e..767fa83bf4a 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -257,7 +257,7 @@ impl<'interner> Monomorphizer<'interner> { let body_expr_id = *self.interner.function(&f).as_expr(); let body_return_type = self.interner.id_type(body_expr_id); let return_type = self.convert_type(match meta.return_type() { - Type::TraitAsType(_, _, _) => &body_return_type, + Type::TraitAsType(..) => &body_return_type, _ => meta.return_type(), }); let unconstrained = modifiers.is_unconstrained diff --git a/tooling/nargo/src/ops/compile.rs b/tooling/nargo/src/ops/compile.rs index 58f72034ad0..bd1850649c4 100644 --- a/tooling/nargo/src/ops/compile.rs +++ b/tooling/nargo/src/ops/compile.rs @@ -72,7 +72,7 @@ pub fn compile_program( compile_options: &CompileOptions, cached_program: Option, ) -> CompilationResult { - compile_program_with_debug_state( + compile_program_with_debug_instrumenter( file_manager, parsed_files, package, @@ -82,17 +82,17 @@ pub fn compile_program( ) } -pub fn compile_program_with_debug_state( +pub fn compile_program_with_debug_instrumenter( file_manager: &FileManager, parsed_files: &ParsedFiles, package: &Package, compile_options: &CompileOptions, cached_program: Option, - debug_state: DebugInstrumenter, + debug_instrumenter: DebugInstrumenter, ) -> CompilationResult { let (mut context, crate_id) = prepare_package(file_manager, parsed_files, package); link_to_debug_crate(&mut context, crate_id); - context.debug_state = debug_state; + context.debug_instrumenter = debug_instrumenter; noirc_driver::compile_main(&mut context, crate_id, compile_options, cached_program) } diff --git a/tooling/nargo/src/ops/mod.rs b/tooling/nargo/src/ops/mod.rs index 8717c06aad5..215efe7e4cb 100644 --- a/tooling/nargo/src/ops/mod.rs +++ b/tooling/nargo/src/ops/mod.rs @@ -1,5 +1,5 @@ pub use self::compile::{ - compile_contract, compile_program, compile_program_with_debug_state, compile_workspace, + compile_contract, compile_program, compile_program_with_debug_instrumenter, compile_workspace, }; pub use self::execute::execute_circuit; pub use self::foreign_calls::{DefaultForeignCallExecutor, ForeignCall, ForeignCallExecutor}; diff --git a/tooling/nargo_cli/src/cli/dap_cmd.rs b/tooling/nargo_cli/src/cli/dap_cmd.rs index 44204092427..cdc624a60f9 100644 --- a/tooling/nargo_cli/src/cli/dap_cmd.rs +++ b/tooling/nargo_cli/src/cli/dap_cmd.rs @@ -2,7 +2,7 @@ use acvm::acir::native_types::WitnessMap; use backend_interface::Backend; use clap::Args; use nargo::constants::PROVER_INPUT_FILE; -use nargo::ops::compile_program_with_debug_state; +use nargo::ops::compile_program_with_debug_instrumenter; use nargo::workspace::Workspace; use nargo::{insert_all_files_for_workspace_into_file_manager, parse_all}; use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection}; @@ -79,7 +79,7 @@ fn load_and_compile_project( let debug_state = instrument_package_files(&mut parsed_files, &workspace_file_manager, package); - let compilation_result = compile_program_with_debug_state( + let compilation_result = compile_program_with_debug_instrumenter( &workspace_file_manager, &parsed_files, package, diff --git a/tooling/nargo_cli/src/cli/debug_cmd.rs b/tooling/nargo_cli/src/cli/debug_cmd.rs index adfb7fdc9e4..57ceddafc6c 100644 --- a/tooling/nargo_cli/src/cli/debug_cmd.rs +++ b/tooling/nargo_cli/src/cli/debug_cmd.rs @@ -7,7 +7,7 @@ use clap::Args; use fm::FileManager; use nargo::artifacts::debug::DebugArtifact; use nargo::constants::PROVER_INPUT_FILE; -use nargo::ops::compile_program_with_debug_state; +use nargo::ops::compile_program_with_debug_instrumenter; use nargo::package::Package; use nargo::{insert_all_files_for_workspace_into_file_manager, parse_all}; use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection}; @@ -76,7 +76,7 @@ pub(crate) fn run( let debug_state = instrument_package_files(&mut parsed_files, &workspace_file_manager, package); - let compilation_result = compile_program_with_debug_state( + let compilation_result = compile_program_with_debug_instrumenter( &workspace_file_manager, &parsed_files, package, From 6e26f608361a304d50698e93e8017757cb50749f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Thu, 1 Feb 2024 21:49:08 -0500 Subject: [PATCH 14/19] chore: Remove unneeded code and undo unnecessary changes --- tooling/debugger/src/foreign_calls.rs | 23 ----------------------- tooling/lsp/src/requests/test_run.rs | 7 ++++--- tooling/nargo/src/artifacts/debug.rs | 8 -------- 3 files changed, 4 insertions(+), 34 deletions(-) diff --git a/tooling/debugger/src/foreign_calls.rs b/tooling/debugger/src/foreign_calls.rs index db52c39b437..14124fb336d 100644 --- a/tooling/debugger/src/foreign_calls.rs +++ b/tooling/debugger/src/foreign_calls.rs @@ -16,30 +16,7 @@ pub(crate) enum DebugForeignCall { DerefAssign, } -impl std::fmt::Display for DebugForeignCall { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "{}", self.name()) - } -} - impl DebugForeignCall { - pub(crate) fn name(&self) -> &'static str { - match self { - DebugForeignCall::VarAssign => "__debug_var_assign", - DebugForeignCall::VarDrop => "__debug_var_drop", - DebugForeignCall::MemberAssign(1) => "__debug_member_assign_1", - DebugForeignCall::MemberAssign(2) => "__debug_member_assign_2", - DebugForeignCall::MemberAssign(3) => "__debug_member_assign_3", - DebugForeignCall::MemberAssign(4) => "__debug_member_assign_4", - DebugForeignCall::MemberAssign(5) => "__debug_member_assign_5", - DebugForeignCall::MemberAssign(6) => "__debug_member_assign_6", - DebugForeignCall::MemberAssign(7) => "__debug_member_assign_7", - DebugForeignCall::MemberAssign(8) => "__debug_member_assign_8", - DebugForeignCall::MemberAssign(_) => panic!("unsupported member assignment arity"), - DebugForeignCall::DerefAssign => "__debug_deref_assign", - } - } - pub(crate) fn lookup(op_name: &str) -> Option { let member_pre = "__debug_member_assign_"; if let Some(op_suffix) = op_name.strip_prefix(member_pre) { diff --git a/tooling/lsp/src/requests/test_run.rs b/tooling/lsp/src/requests/test_run.rs index 259b5c5779e..0b88d814265 100644 --- a/tooling/lsp/src/requests/test_run.rs +++ b/tooling/lsp/src/requests/test_run.rs @@ -4,7 +4,7 @@ use async_lsp::{ErrorCode, ResponseError}; use nargo::{ insert_all_files_for_workspace_into_file_manager, ops::{run_test, TestStatus}, - parse_all, prepare_package, + prepare_package, }; use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection}; use noirc_driver::{ @@ -13,6 +13,7 @@ use noirc_driver::{ use noirc_frontend::hir::FunctionNameMatch; use crate::{ + parse_diff, types::{NargoTestRunParams, NargoTestRunResult}, LspState, }; @@ -25,7 +26,7 @@ pub(crate) fn on_test_run_request( } fn on_test_run_request_inner( - state: &LspState, + state: &mut LspState, params: NargoTestRunParams, ) -> Result { let root_path = state.root_path.as_deref().ok_or_else(|| { @@ -52,7 +53,7 @@ fn on_test_run_request_inner( let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir); insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager); - let parsed_files = parse_all(&workspace_file_manager); + let parsed_files = parse_diff(&workspace_file_manager, state); // Since we filtered on crate name, this should be the only item in the iterator match workspace.into_iter().next() { diff --git a/tooling/nargo/src/artifacts/debug.rs b/tooling/nargo/src/artifacts/debug.rs index c4c25081325..a249ecb03ad 100644 --- a/tooling/nargo/src/artifacts/debug.rs +++ b/tooling/nargo/src/artifacts/debug.rs @@ -116,14 +116,6 @@ impl DebugArtifact { let source = self.source(location.file)?; self.line_index(location.file, source.len()) } - - pub fn from_program(program: &CompiledProgram) -> Self { - Self { - debug_symbols: vec![program.debug.clone()], - file_map: program.file_map.clone(), - warnings: program.warnings.clone(), - } - } } impl From for DebugArtifact { From 8d4142baa64da201c32e2abbb4b4a6b453c938aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Thu, 1 Feb 2024 22:10:51 -0500 Subject: [PATCH 15/19] chore: Small documentation update --- compiler/noirc_frontend/src/monomorphization/debug_types.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_frontend/src/monomorphization/debug_types.rs b/compiler/noirc_frontend/src/monomorphization/debug_types.rs index dddaa349399..fea073d394f 100644 --- a/compiler/noirc_frontend/src/monomorphization/debug_types.rs +++ b/compiler/noirc_frontend/src/monomorphization/debug_types.rs @@ -9,9 +9,9 @@ use noirc_printable_type::PrintableType; use std::collections::HashMap; /// We keep a collection of the debug variables and their types in this -/// structure. The fe_var_id refers to the ID given when inserting the -/// instrumentation probe. This variable does not have a type yet and hence it -/// can be instantiated multiple types if it's in the context of a generic +/// structure. The source_var_id refers to the ID given by the debug +/// instrumenter. This variable does not have a type yet and hence it +/// can be instantiated for multiple types if it's in the context of a generic /// variable. The var_id refers to the ID of the instantiated variable which /// will have a valid type. #[derive(Debug, Clone, Default)] From c4745ba4a8d3e23eb5f2a7830bb85533bbba7cfc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Thu, 1 Feb 2024 22:13:33 -0500 Subject: [PATCH 16/19] chore: And one more missing rename --- tooling/nargo_cli/src/cli/debug_cmd.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tooling/nargo_cli/src/cli/debug_cmd.rs b/tooling/nargo_cli/src/cli/debug_cmd.rs index 57ceddafc6c..7224756ec6f 100644 --- a/tooling/nargo_cli/src/cli/debug_cmd.rs +++ b/tooling/nargo_cli/src/cli/debug_cmd.rs @@ -74,7 +74,8 @@ pub(crate) fn run( return Ok(()); }; - let debug_state = instrument_package_files(&mut parsed_files, &workspace_file_manager, package); + let debug_instrumenter = + instrument_package_files(&mut parsed_files, &workspace_file_manager, package); let compilation_result = compile_program_with_debug_instrumenter( &workspace_file_manager, @@ -82,7 +83,7 @@ pub(crate) fn run( package, &args.compile_options, None, - debug_state, + debug_instrumenter, ); let compiled_program = report_errors( From c01a993cc3acb93ca416fd4e812517da58df575f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Thu, 1 Feb 2024 23:20:06 -0500 Subject: [PATCH 17/19] fix: Compiler errors after merge --- tooling/debugger/ignored-tests.txt | 1 + tooling/debugger/src/foreign_calls.rs | 12 ++++++------ tooling/nargo/src/ops/mod.rs | 2 +- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/tooling/debugger/ignored-tests.txt b/tooling/debugger/ignored-tests.txt index a9aeb44051f..1aa886866f2 100644 --- a/tooling/debugger/ignored-tests.txt +++ b/tooling/debugger/ignored-tests.txt @@ -12,6 +12,7 @@ nested_array_dynamic nested_array_in_slice nested_arrays_from_brillig references +scalar_mul signed_comparison simple_2d_array to_bytes_integration diff --git a/tooling/debugger/src/foreign_calls.rs b/tooling/debugger/src/foreign_calls.rs index 14124fb336d..01676adfef3 100644 --- a/tooling/debugger/src/foreign_calls.rs +++ b/tooling/debugger/src/foreign_calls.rs @@ -4,7 +4,7 @@ use acvm::{ }; use nargo::{ artifacts::debug::{DebugArtifact, DebugVars}, - ops::{DefaultForeignCallExecutor, ForeignCallExecutor}, + ops::{DefaultForeignCallExecutor, ForeignCallExecutor, NargoForeignCallResult}, }; use noirc_errors::debug_info::DebugVarId; use noirc_printable_type::{ForeignCallError, PrintableType, PrintableValue}; @@ -78,7 +78,7 @@ impl ForeignCallExecutor for DefaultDebugForeignCallExecutor { fn execute( &mut self, foreign_call: &ForeignCallWaitInfo, - ) -> Result { + ) -> Result { let foreign_call_name = foreign_call.function.as_str(); match DebugForeignCall::lookup(foreign_call_name) { Some(DebugForeignCall::VarAssign) => { @@ -89,7 +89,7 @@ impl ForeignCallExecutor for DefaultDebugForeignCallExecutor { foreign_call.inputs[1..].iter().flat_map(|x| x.values()).collect(); self.debug_vars.assign_var(var_id, &values); } - Ok(ForeignCallResult { values: vec![] }) + Ok(ForeignCallResult::default().into()) } Some(DebugForeignCall::VarDrop) => { let fcp_var_id = &foreign_call.inputs[0]; @@ -97,7 +97,7 @@ impl ForeignCallExecutor for DefaultDebugForeignCallExecutor { let var_id = debug_var_id(var_id_value); self.debug_vars.drop_var(var_id); } - Ok(ForeignCallResult { values: vec![] }) + Ok(ForeignCallResult::default().into()) } Some(DebugForeignCall::MemberAssign(arity)) => { if let Some(ForeignCallParam::Single(var_id_value)) = foreign_call.inputs.get(0) { @@ -121,7 +121,7 @@ impl ForeignCallExecutor for DefaultDebugForeignCallExecutor { .collect(); self.debug_vars.assign_field(var_id, indexes, &values); } - Ok(ForeignCallResult { values: vec![] }) + Ok(ForeignCallResult::default().into()) } Some(DebugForeignCall::DerefAssign) => { let fcp_var_id = &foreign_call.inputs[0]; @@ -130,7 +130,7 @@ impl ForeignCallExecutor for DefaultDebugForeignCallExecutor { let var_id = debug_var_id(var_id_value); self.debug_vars.assign_deref(var_id, &fcp_value.values()); } - Ok(ForeignCallResult { values: vec![] }) + Ok(ForeignCallResult::default().into()) } None => self.executor.execute(foreign_call), } diff --git a/tooling/nargo/src/ops/mod.rs b/tooling/nargo/src/ops/mod.rs index 215efe7e4cb..3200e5bf046 100644 --- a/tooling/nargo/src/ops/mod.rs +++ b/tooling/nargo/src/ops/mod.rs @@ -2,7 +2,7 @@ pub use self::compile::{ compile_contract, compile_program, compile_program_with_debug_instrumenter, compile_workspace, }; pub use self::execute::execute_circuit; -pub use self::foreign_calls::{DefaultForeignCallExecutor, ForeignCall, ForeignCallExecutor}; +pub use self::foreign_calls::{DefaultForeignCallExecutor, ForeignCall, ForeignCallExecutor, NargoForeignCallResult}; pub use self::optimize::{optimize_contract, optimize_program}; pub use self::transform::{transform_contract, transform_program}; From 9a54467ae1270757ae90a1c7718f97db74f2be9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Thu, 1 Feb 2024 23:22:16 -0500 Subject: [PATCH 18/19] chore: cargo fmt --- tooling/nargo/src/ops/mod.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tooling/nargo/src/ops/mod.rs b/tooling/nargo/src/ops/mod.rs index 3200e5bf046..23dd0db15b9 100644 --- a/tooling/nargo/src/ops/mod.rs +++ b/tooling/nargo/src/ops/mod.rs @@ -2,7 +2,9 @@ pub use self::compile::{ compile_contract, compile_program, compile_program_with_debug_instrumenter, compile_workspace, }; pub use self::execute::execute_circuit; -pub use self::foreign_calls::{DefaultForeignCallExecutor, ForeignCall, ForeignCallExecutor, NargoForeignCallResult}; +pub use self::foreign_calls::{ + DefaultForeignCallExecutor, ForeignCall, ForeignCallExecutor, NargoForeignCallResult, +}; pub use self::optimize::{optimize_contract, optimize_program}; pub use self::transform::{transform_contract, transform_program}; From 82318b3841cd599713cf54ecc55f0a65aca4e921 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Mon, 5 Feb 2024 09:55:20 -0500 Subject: [PATCH 19/19] feat: Hide instrumentation compile option by default Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- compiler/noirc_driver/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index 4d55e8a2396..d782330a677 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -87,7 +87,7 @@ pub struct CompileOptions { pub show_monomorphized: bool, /// Insert debug symbols to inspect variables - #[arg(long)] + #[arg(long, hide = true)] pub instrument_debug: bool, /// Force Brillig output (for step debugging)