Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add instrumentation for tracking variables in debugging #4122

Merged
merged 21 commits into from
Feb 5, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
67ccaae
feat: Implement debug instrumentation to track variable values
ggiraldez Jan 19, 2024
5170d2f
feat: Add debugging instrumentation to files in the package debugged
ggiraldez Jan 22, 2024
93d4c80
feat: Hide debug module stack frames in the debugger
ggiraldez Jan 22, 2024
de3a78e
chore: Ignore debugger failing tests and use Brillig mode
ggiraldez Jan 22, 2024
ca45375
chore: Refactor instrumentation patching code during monomorphization
ggiraldez Jan 22, 2024
6744c1d
feat: Only link the debug crate when instrumenting code for the debugger
ggiraldez Jan 23, 2024
260add6
chore: Ignore two more tests for debugging which fail with assertions
ggiraldez Jan 23, 2024
eee6724
Merge remote-tracking branch 'upstream/master' into debug-vars-instru…
ggiraldez Jan 24, 2024
0c0848a
fix: Address code review comments and remove unneeded code
ggiraldez Jan 29, 2024
8ebf01e
fix: First pass addressing code review comments
ggiraldez Jan 31, 2024
e0a4c88
chore: Extract debug related code in monomorphization
ggiraldez Jan 31, 2024
937793d
feat: Use var names collected during injection in monomorphization phase
ggiraldez Feb 1, 2024
bd68ee8
feat: Refactor debug instrumentation code in the compiler
ggiraldez Feb 2, 2024
10ae07a
chore: Missing renames from last refactor
ggiraldez Feb 2, 2024
6e26f60
chore: Remove unneeded code and undo unnecessary changes
ggiraldez Feb 2, 2024
8d4142b
chore: Small documentation update
ggiraldez Feb 2, 2024
c4745ba
chore: And one more missing rename
ggiraldez Feb 2, 2024
236239b
Merge remote-tracking branch 'upstream/master' into debug-vars-instru…
ggiraldez Feb 2, 2024
c01a993
fix: Compiler errors after merge
ggiraldez Feb 2, 2024
9a54467
chore: cargo fmt
ggiraldez Feb 2, 2024
82318b3
feat: Hide instrumentation compile option by default
ggiraldez Feb 5, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions acvm-repo/acir_field/src/generic_ark.rs
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,10 @@ impl<F: PrimeField> FieldElement<F> {
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()))
}
Expand Down Expand Up @@ -240,6 +244,12 @@ impl<F: PrimeField> FieldElement<F> {
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<u64> {
(self.num_bits() <= 64).then(|| self.to_u128() as u64)
}
Expand Down
15 changes: 15 additions & 0 deletions compiler/fm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,21 @@ impl FileManager {
Some(file_id)
}

pub fn add_file_with_contents(&mut self, file_name: &Path, contents: &str) -> Option<FileId> {
// 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)
}
TomAFrench marked this conversation as resolved.
Show resolved Hide resolved

fn register_path(&mut self, file_id: FileId, path: PathBuf) {
let old_value = self.id_to_path.insert(file_id, path.clone());
assert!(
Expand Down
48 changes: 43 additions & 5 deletions compiler/noirc_driver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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");
Expand Down Expand Up @@ -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)]
ggiraldez marked this conversation as resolved.
Show resolved Hide resolved
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
Expand All @@ -99,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
}
Expand All @@ -115,6 +126,14 @@ fn add_stdlib_source_to_file_manager(file_manager: &mut FileManager) {
}
}

/// 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));
}

/// Adds the file from the file system at `Path` to the crate graph as a root file
///
/// Note: This methods adds the stdlib as a dependency to the crate.
Expand All @@ -136,6 +155,17 @@ pub fn prepare_crate(context: &mut Context, file_name: &Path) -> CrateId {
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());
ggiraldez marked this conversation as resolved.
Show resolved Hide resolved
}

// 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
Expand Down Expand Up @@ -309,7 +339,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<CompiledContract, ErrorsAndWarnings> {
Expand Down Expand Up @@ -385,13 +415,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<CompiledProgram>,
force_compile: bool,
) -> Result<CompiledProgram, RuntimeError> {
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,
)
ggiraldez marked this conversation as resolved.
Show resolved Hide resolved
} 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);
Expand All @@ -410,7 +448,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);
Expand Down
1 change: 1 addition & 0 deletions compiler/noirc_errors/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
26 changes: 24 additions & 2 deletions compiler/noirc_errors/src/debug_info.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use acvm::acir::circuit::OpcodeLocation;
use acvm::compiler::AcirTransformationMap;
use fm::FileId;

use base64::Engine;
use flate2::read::DeflateDecoder;
Expand All @@ -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 {
Expand All @@ -28,6 +34,8 @@ pub struct DebugInfo {
/// that they should be serialized to/from strings.
#[serde_as(as = "BTreeMap<DisplayFromStr, _>")]
pub locations: BTreeMap<OpcodeLocation, Vec<Location>>,
pub variables: HashMap<u32, (String, u32)>, // var_id => (name, type_id)
pub types: HashMap<u32, PrintableType>, // type_id => printable type
jfecher marked this conversation as resolved.
Show resolved Hide resolved
}

/// Holds OpCodes Counts for Acir and Brillig Opcodes
Expand All @@ -39,8 +47,15 @@ pub struct OpCodesCount {
}

impl DebugInfo {
pub fn new(locations: BTreeMap<OpcodeLocation, Vec<Location>>) -> Self {
DebugInfo { locations }
pub fn new(
locations: BTreeMap<OpcodeLocation, Vec<Location>>,
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.
Expand Down Expand Up @@ -97,6 +112,13 @@ impl DebugInfo {
counted_opcodes
}

pub fn get_file_ids(&self) -> Vec<FileId> {
self.locations
.values()
.filter_map(|call_stack| call_stack.last().map(|location| location.file))
.collect()
}

pub fn serialize_compressed_base64_json<S>(
debug_info: &DebugInfo,
s: S,
Expand Down
4 changes: 4 additions & 0 deletions compiler/noirc_errors/src/position.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,10 @@ impl Span {
self.0.end().into()
}

pub fn build_from_str(s: &str) -> Span {
Span(ByteSpan::from_str(s))
}

ggiraldez marked this conversation as resolved.
Show resolved Hide resolved
pub fn contains(&self, other: &Span) -> bool {
self.start() <= other.start() && self.end() >= other.end()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1227,7 +1227,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:?}")
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand All @@ -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")
}
}
}
23 changes: 17 additions & 6 deletions compiler/noirc_evaluator/src/ssa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<GeneratedAcir, RuntimeError> {
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
Expand Down Expand Up @@ -83,10 +84,16 @@ pub fn create_circuit(
program: Program,
enable_ssa_logging: bool,
enable_brillig_logging: bool,
force_brillig_output: bool,
) -> Result<(Circuit, DebugInfo, Vec<Witness>, Vec<Witness>, Vec<SsaReport>), 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 {
Expand Down Expand Up @@ -119,7 +126,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);
Expand Down Expand Up @@ -169,8 +176,12 @@ struct SsaBuilder {
}

impl SsaBuilder {
fn new(program: Program, print_ssa_passes: bool) -> Result<SsaBuilder, RuntimeError> {
let ssa = ssa_gen::generate_ssa(program)?;
fn new(
program: Program,
print_ssa_passes: bool,
force_brillig_runtime: bool,
) -> Result<SsaBuilder, RuntimeError> {
let ssa = ssa_gen::generate_ssa(program, force_brillig_runtime)?;
Ok(SsaBuilder { print_ssa_passes, ssa }.print("Initial SSA:"))
}

Expand Down
10 changes: 9 additions & 1 deletion compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
)
ggiraldez marked this conversation as resolved.
Show resolved Hide resolved
}
Value::ForeignFunction(_) => unimplemented!(
"Oracle calls directly in constrained functions are not yet available."
),
Expand Down
Loading
Loading