From e0ad0b2b31f6d46be75d23aec6a82850a9c4bd75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Wed, 14 Feb 2024 10:21:11 -0500 Subject: [PATCH] feat: DAP Preflight and debugger compilation options (#4185) # Description ## Problem\* Part of #3015 ## Summary\* This PR adds a preflight mode to DAP in order to make it easier to identify and report back problems to the user when compiling the project for debugging. This preflight mode is invoked from the VS.Code extension before starting the debugging session, and with the same arguments as those that will be used for the session. If the compiler finds any error either loading or compiling the project, the error is reported to stderr which allows the extension to parse the output and present the diagnostic messages to the user. This also changes the default compilation mode to output Brillig code and adds new commands line options to Nargo's `debug` command and launch options to the DAP mode to control the mode and whether to inject instrumentation code to track variables or not. The `debug` options are: - `--acir-mode`, force output of ACIR, which disables instrumentation by default - `--skip-instrumentation={true,false}` to control injection of instrumentation code to track variables values during the debugging session. Similarly, for DAP two launch options can be provided: `generateAcir` and `skipInstrumentation`. ## Additional Context The default is to output in Brillig mode with instrumentation for tracking variables, as this makes it easier to follow along with stepping through the code. If ACIR mode is selected, instrumentation is disabled by default. Instrumentation can be forcefully enabled or disabled by the provided CLI option. ## Documentation\* Check one: - [ ] No documentation needed. - [ ] Documentation included in this PR. - [X] **[Exceptional Case]** Documentation to be submitted in a separate PR. # PR Checklist\* - [X] I have tested the changes locally. - [X] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: Martin Verzilli --- cspell.json | 1 + tooling/debugger/src/dap.rs | 7 +- tooling/debugger/src/errors.rs | 19 ++++ tooling/debugger/src/lib.rs | 1 + tooling/nargo_cli/src/cli/dap_cmd.rs | 148 +++++++++++++++++-------- tooling/nargo_cli/src/cli/debug_cmd.rs | 85 +++++++++----- tooling/nargo_cli/src/errors.rs | 3 +- 7 files changed, 192 insertions(+), 72 deletions(-) create mode 100644 tooling/debugger/src/errors.rs diff --git a/cspell.json b/cspell.json index 2acca0633d3..be6b7c5c7e8 100644 --- a/cspell.json +++ b/cspell.json @@ -90,6 +90,7 @@ "indexmap", "injective", "Inlines", + "instrumenter", "interner", "intrinsics", "jmp", diff --git a/tooling/debugger/src/dap.rs b/tooling/debugger/src/dap.rs index dd9a30d50da..184018e9fcc 100644 --- a/tooling/debugger/src/dap.rs +++ b/tooling/debugger/src/dap.rs @@ -510,7 +510,12 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession<'a, R, W, B> { }; let found_index = match line_to_opcodes.binary_search_by(|x| x.0.cmp(&line)) { Ok(index) => line_to_opcodes[index].1, - Err(index) => line_to_opcodes[index].1, + Err(index) => { + if index >= line_to_opcodes.len() { + return None; + } + line_to_opcodes[index].1 + } }; Some(found_index) } diff --git a/tooling/debugger/src/errors.rs b/tooling/debugger/src/errors.rs new file mode 100644 index 00000000000..4578987d715 --- /dev/null +++ b/tooling/debugger/src/errors.rs @@ -0,0 +1,19 @@ +use thiserror::Error; + +#[derive(Debug, Error)] +pub enum DapError { + #[error("{0}")] + PreFlightGenericError(String), + + #[error(transparent)] + LoadError(#[from] LoadError), + + #[error(transparent)] + ServerError(#[from] dap::errors::ServerError), +} + +#[derive(Debug, Error)] +pub enum LoadError { + #[error("{0}")] + Generic(String), +} diff --git a/tooling/debugger/src/lib.rs b/tooling/debugger/src/lib.rs index 35014f9a8c8..4a25e3417a0 100644 --- a/tooling/debugger/src/lib.rs +++ b/tooling/debugger/src/lib.rs @@ -1,5 +1,6 @@ mod context; mod dap; +pub mod errors; mod foreign_calls; mod repl; mod source_code_printer; diff --git a/tooling/nargo_cli/src/cli/dap_cmd.rs b/tooling/nargo_cli/src/cli/dap_cmd.rs index 7c7e6056901..f4df309f1c9 100644 --- a/tooling/nargo_cli/src/cli/dap_cmd.rs +++ b/tooling/nargo_cli/src/cli/dap_cmd.rs @@ -3,38 +3,52 @@ 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_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}; use noirc_abi::input_parser::Format; -use noirc_driver::{ - file_manager_with_stdlib, CompileOptions, CompiledProgram, NOIR_ARTIFACT_VERSION_STRING, -}; +use noirc_driver::{CompileOptions, CompiledProgram, NOIR_ARTIFACT_VERSION_STRING}; use noirc_frontend::graph::CrateName; use std::io::{BufReader, BufWriter, Read, Write}; use std::path::Path; -use dap::errors::ServerError; use dap::requests::Command; use dap::responses::ResponseBody; use dap::server::Server; use dap::types::Capabilities; use serde_json::Value; -use super::compile_cmd::report_errors; -use super::debug_cmd::instrument_package_files; +use super::debug_cmd::compile_bin_package_for_debugging; use super::fs::inputs::read_inputs_from_file; use crate::errors::CliError; use super::NargoConfig; +use noir_debugger::errors::{DapError, LoadError}; + #[derive(Debug, Clone, Args)] pub(crate) struct DapCommand { /// Override the expression width requested by the backend. #[arg(long, value_parser = parse_expression_width)] expression_width: Option, + + #[clap(long)] + preflight_check: bool, + + #[clap(long)] + preflight_project_folder: Option, + + #[clap(long)] + preflight_package: Option, + + #[clap(long)] + preflight_prover_name: Option, + + #[clap(long)] + preflight_generate_acir: bool, + + #[clap(long)] + preflight_skip_instrumentation: bool, } fn parse_expression_width(input: &str) -> Result { @@ -50,8 +64,6 @@ fn parse_expression_width(input: &str) -> Result) -> Option { let Ok(toml_path) = get_package_manifest(Path::new(project_folder)) else { eprintln!("ERROR: Failed to get package manifest"); @@ -72,55 +84,51 @@ fn find_workspace(project_folder: &str, package: Option<&str>) -> Option) -> String { + match package { + Some(pkg) => format!( + r#"Noir Debugger could not load program from {}, package {}"#, + project_folder, pkg + ), + None => format!(r#"Noir Debugger could not load program from {}"#, project_folder), + } +} + fn load_and_compile_project( project_folder: &str, package: Option<&str>, prover_name: &str, expression_width: ExpressionWidth, + acir_mode: bool, + skip_instrumentation: bool, ) -> Result<(CompiledProgram, WitnessMap), LoadError> { - let workspace = - find_workspace(project_folder, package).ok_or(LoadError("Cannot open workspace"))?; - + let workspace = find_workspace(project_folder, package) + .ok_or(LoadError::Generic(workspace_not_found_error_msg(project_folder, package)))?; let package = workspace .into_iter() .find(|p| p.is_binary()) - .ok_or(LoadError("No matching binary packages found in workspace"))?; - - 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 mut parsed_files = parse_all(&workspace_file_manager); + .ok_or(LoadError::Generic("No matching binary packages found in workspace".into()))?; - 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_instrumenter( - &workspace_file_manager, - &parsed_files, + let compiled_program = compile_bin_package_for_debugging( + &workspace, package, - &compile_options, - None, - debug_state, - ); - - let compiled_program = report_errors( - compilation_result, - &workspace_file_manager, - compile_options.deny_warnings, - compile_options.silence_warnings, + acir_mode, + skip_instrumentation, + CompileOptions::default(), ) - .map_err(|_| LoadError("Failed to compile project"))?; + .map_err(|_| LoadError::Generic("Failed to compile project".into()))?; let compiled_program = nargo::ops::transform_program(compiled_program, expression_width); let (inputs_map, _) = read_inputs_from_file(&package.root_dir, prover_name, Format::Toml, &compiled_program.abi) - .map_err(|_| LoadError("Failed to read program inputs"))?; + .map_err(|_| { + LoadError::Generic(format!("Failed to read program inputs from {}", prover_name)) + })?; let initial_witness = compiled_program .abi .encode(&inputs_map, None) - .map_err(|_| LoadError("Failed to encode inputs"))?; + .map_err(|_| LoadError::Generic("Failed to encode inputs".into()))?; Ok((compiled_program, initial_witness)) } @@ -128,7 +136,7 @@ fn load_and_compile_project( fn loop_uninitialized_dap( mut server: Server, expression_width: ExpressionWidth, -) -> Result<(), ServerError> { +) -> Result<(), DapError> { loop { let req = match server.poll_request()? { Some(req) => req, @@ -163,6 +171,13 @@ fn loop_uninitialized_dap( .and_then(|v| v.as_str()) .unwrap_or(PROVER_INPUT_FILE); + let generate_acir = + additional_data.get("generateAcir").and_then(|v| v.as_bool()).unwrap_or(false); + let skip_instrumentation = additional_data + .get("skipInstrumentation") + .and_then(|v| v.as_bool()) + .unwrap_or(generate_acir); + eprintln!("Project folder: {}", project_folder); eprintln!("Package: {}", package.unwrap_or("(default)")); eprintln!("Prover name: {}", prover_name); @@ -172,6 +187,8 @@ fn loop_uninitialized_dap( package, prover_name, expression_width, + generate_acir, + skip_instrumentation, ) { Ok((compiled_program, initial_witness)) => { server.respond(req.ack()?)?; @@ -186,8 +203,8 @@ fn loop_uninitialized_dap( )?; break; } - Err(LoadError(message)) => { - server.respond(req.error(message))?; + Err(LoadError::Generic(message)) => { + server.respond(req.error(message.as_str()))?; } } } @@ -206,17 +223,58 @@ fn loop_uninitialized_dap( Ok(()) } +fn run_preflight_check( + expression_width: ExpressionWidth, + args: DapCommand, +) -> Result<(), DapError> { + let project_folder = if let Some(project_folder) = args.preflight_project_folder { + project_folder + } else { + return Err(DapError::PreFlightGenericError("Noir Debugger could not initialize because the IDE (for example, VS Code) did not specify a project folder to debug.".into())); + }; + + let package = args.preflight_package.as_deref(); + let prover_name = args.preflight_prover_name.as_deref().unwrap_or(PROVER_INPUT_FILE); + + let _ = load_and_compile_project( + project_folder.as_str(), + package, + prover_name, + expression_width, + args.preflight_generate_acir, + args.preflight_skip_instrumentation, + )?; + + Ok(()) +} + pub(crate) fn run( backend: &Backend, args: DapCommand, _config: NargoConfig, ) -> Result<(), CliError> { + let expression_width = + args.expression_width.unwrap_or_else(|| backend.get_backend_info_or_default()); + + // When the --preflight-check flag is present, we run Noir's DAP server in "pre-flight mode", which test runs + // the DAP initialization code without actually starting the DAP server. + // + // This lets the client IDE present any initialization issues (compiler version mismatches, missing prover files, etc) + // in its own interface. + // + // This was necessary due to the VS Code project being reluctant to let extension authors capture + // stderr output generated by a DAP server wrapped in DebugAdapterExecutable. + // + // Exposing this preflight mode lets us gracefully handle errors that happen *before* + // the DAP loop is established, which otherwise are considered "out of band" by the maintainers of the DAP spec. + // More details here: https://github.com/microsoft/vscode/issues/108138 + if args.preflight_check { + return run_preflight_check(expression_width, args).map_err(CliError::DapError); + } + let output = BufWriter::new(std::io::stdout()); let input = BufReader::new(std::io::stdin()); let server = Server::new(input, output); - let expression_width = - args.expression_width.unwrap_or_else(|| backend.get_backend_info_or_default()); - loop_uninitialized_dap(server, expression_width).map_err(CliError::DapError) } diff --git a/tooling/nargo_cli/src/cli/debug_cmd.rs b/tooling/nargo_cli/src/cli/debug_cmd.rs index b3ee9137530..6fcfee91457 100644 --- a/tooling/nargo_cli/src/cli/debug_cmd.rs +++ b/tooling/nargo_cli/src/cli/debug_cmd.rs @@ -7,8 +7,10 @@ use clap::Args; use fm::FileManager; use nargo::artifacts::debug::DebugArtifact; use nargo::constants::PROVER_INPUT_FILE; -use nargo::ops::compile_program_with_debug_instrumenter; +use nargo::errors::CompileError; +use nargo::ops::{compile_program, compile_program_with_debug_instrumenter}; use nargo::package::Package; +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}; use noirc_abi::input_parser::{Format, InputValue}; @@ -42,6 +44,14 @@ pub(crate) struct DebugCommand { #[clap(flatten)] compile_options: CompileOptions, + + /// Force ACIR output (disabling instrumentation) + #[clap(long)] + acir_mode: bool, + + /// Disable vars debug instrumentation (enabled by default) + #[clap(long)] + skip_instrumentation: Option, } pub(crate) fn run( @@ -49,9 +59,8 @@ pub(crate) fn run( args: DebugCommand, config: NargoConfig, ) -> Result<(), CliError> { - // Override clap default for compiler option flag - let mut args = args.clone(); - args.compile_options.instrument_debug = true; + let acir_mode = args.acir_mode; + let skip_instrumentation = args.skip_instrumentation.unwrap_or(acir_mode); let toml_path = get_package_manifest(&config.program_dir)?; let selection = args.package.map_or(PackageSelection::DefaultOrAll, PackageSelection::Selected); @@ -66,10 +75,6 @@ pub(crate) fn run( .expression_width .unwrap_or_else(|| backend.get_backend_info_or_default()); - 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 mut parsed_files = parse_all(&workspace_file_manager); - let Some(package) = workspace.into_iter().find(|p| p.is_binary()) else { println!( "No matching binary packages found in workspace. Only binary packages can be debugged." @@ -77,23 +82,12 @@ pub(crate) fn run( return Ok(()); }; - let debug_instrumenter = - instrument_package_files(&mut parsed_files, &workspace_file_manager, package); - - let compilation_result = compile_program_with_debug_instrumenter( - &workspace_file_manager, - &parsed_files, + let compiled_program = compile_bin_package_for_debugging( + &workspace, package, - &args.compile_options, - None, - debug_instrumenter, - ); - - let compiled_program = report_errors( - compilation_result, - &workspace_file_manager, - args.compile_options.deny_warnings, - args.compile_options.silence_warnings, + acir_mode, + skip_instrumentation, + args.compile_options.clone(), )?; let compiled_program = nargo::ops::transform_program(compiled_program, expression_width); @@ -101,9 +95,50 @@ pub(crate) fn run( run_async(package, compiled_program, &args.prover_name, &args.witness_name, target_dir) } +pub(crate) fn compile_bin_package_for_debugging( + workspace: &Workspace, + package: &Package, + acir_mode: bool, + skip_instrumentation: bool, + compile_options: CompileOptions, +) -> Result { + 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 mut parsed_files = parse_all(&workspace_file_manager); + + let compile_options = CompileOptions { + instrument_debug: !skip_instrumentation, + force_brillig: !acir_mode, + ..compile_options + }; + + let compilation_result = if !skip_instrumentation { + let debug_state = + instrument_package_files(&mut parsed_files, &workspace_file_manager, package); + + compile_program_with_debug_instrumenter( + &workspace_file_manager, + &parsed_files, + package, + &compile_options, + None, + debug_state, + ) + } else { + compile_program(&workspace_file_manager, &parsed_files, package, &compile_options, None) + }; + + report_errors( + compilation_result, + &workspace_file_manager, + compile_options.deny_warnings, + compile_options.silence_warnings, + ) +} + /// Add debugging instrumentation to all parsed files belonging to the package /// being compiled -pub(crate) fn instrument_package_files( +fn instrument_package_files( parsed_files: &mut ParsedFiles, file_manager: &FileManager, package: &Package, diff --git a/tooling/nargo_cli/src/errors.rs b/tooling/nargo_cli/src/errors.rs index 4636772231b..c2996f53420 100644 --- a/tooling/nargo_cli/src/errors.rs +++ b/tooling/nargo_cli/src/errors.rs @@ -2,6 +2,7 @@ use acvm::acir::native_types::WitnessMapError; use hex::FromHexError; use nargo::{errors::CompileError, NargoError}; use nargo_toml::ManifestError; +use noir_debugger::errors::DapError; use noirc_abi::errors::{AbiError, InputParserError}; use std::path::PathBuf; use thiserror::Error; @@ -54,7 +55,7 @@ pub(crate) enum CliError { LspError(#[from] async_lsp::Error), #[error(transparent)] - DapError(#[from] dap::errors::ServerError), + DapError(#[from] DapError), /// Error from Nargo #[error(transparent)]