From 900418192216dc2657d6ffe48f85ac82411fb054 Mon Sep 17 00:00:00 2001 From: jfecher Date: Tue, 15 Aug 2023 15:04:02 -0500 Subject: [PATCH] feat: Add full call stacks to runtime errors (#2310) --- crates/lsp/src/lib.rs | 2 +- crates/nargo/src/artifacts/debug.rs | 7 +- crates/nargo_cli/src/cli/execute_cmd.rs | 22 ++-- crates/noirc_driver/src/lib.rs | 20 ++-- crates/noirc_errors/src/debug_info.rs | 10 +- crates/noirc_errors/src/lib.rs | 15 +++ crates/noirc_errors/src/reporter.rs | 64 +++++++++-- .../src/brillig/brillig_gen/brillig_block.rs | 2 +- crates/noirc_evaluator/src/errors.rs | 102 ++++++++++-------- crates/noirc_evaluator/src/ssa.rs | 7 ++ .../src/ssa/acir_gen/acir_ir/acir_variable.rs | 24 +++-- .../ssa/acir_gen/acir_ir/generated_acir.rs | 16 +-- .../src/ssa/acir_gen/acir_ir/sort.rs | 10 +- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 29 ++--- crates/noirc_evaluator/src/ssa/ir/cfg.rs | 2 +- crates/noirc_evaluator/src/ssa/ir/dfg.rs | 32 ++++-- .../src/ssa/ir/function_inserter.rs | 11 +- .../noirc_evaluator/src/ssa/ir/instruction.rs | 11 +- crates/noirc_evaluator/src/ssa/ir/printer.rs | 2 +- .../src/ssa/opt/assert_constant.rs | 4 +- .../src/ssa/opt/constant_folding.rs | 4 +- .../src/ssa/opt/flatten_cfg.rs | 52 ++++----- .../noirc_evaluator/src/ssa/opt/inlining.rs | 50 ++++++--- .../src/ssa/opt/simplify_cfg.rs | 5 +- .../noirc_evaluator/src/ssa/opt/unrolling.rs | 33 +++--- .../src/ssa/ssa_builder/mod.rs | 34 +++--- crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs | 4 +- crates/noirc_frontend/src/graph/mod.rs | 2 +- 28 files changed, 349 insertions(+), 227 deletions(-) diff --git a/crates/lsp/src/lib.rs b/crates/lsp/src/lib.rs index 7af00fa15a8..1fe0565da40 100644 --- a/crates/lsp/src/lib.rs +++ b/crates/lsp/src/lib.rs @@ -332,7 +332,7 @@ fn on_did_save_text_document( let fm = &context.file_manager; let files = fm.as_simple_files(); - for FileDiagnostic { file_id, diagnostic } in file_diagnostics { + for FileDiagnostic { file_id, diagnostic, call_stack: _ } in file_diagnostics { // Ignore diagnostics for any file that wasn't the file we saved // TODO: In the future, we could create "related" diagnostics for these files // TODO: This currently just appends the `.nr` file extension that we store as a constant, diff --git a/crates/nargo/src/artifacts/debug.rs b/crates/nargo/src/artifacts/debug.rs index 31c15311f06..b6eefd17189 100644 --- a/crates/nargo/src/artifacts/debug.rs +++ b/crates/nargo/src/artifacts/debug.rs @@ -30,7 +30,12 @@ impl DebugArtifact { let files_with_debug_symbols: BTreeSet = debug_symbols .iter() - .flat_map(|function_symbols| function_symbols.locations.values().map(|loc| loc.file)) + .flat_map(|function_symbols| { + function_symbols + .locations + .values() + .filter_map(|call_stack| call_stack.last().map(|location| location.file)) + }) .collect(); for file_id in files_with_debug_symbols { diff --git a/crates/nargo_cli/src/cli/execute_cmd.rs b/crates/nargo_cli/src/cli/execute_cmd.rs index f2fa6426a7e..169ddb3d937 100644 --- a/crates/nargo_cli/src/cli/execute_cmd.rs +++ b/crates/nargo_cli/src/cli/execute_cmd.rs @@ -108,23 +108,23 @@ fn extract_unsatisfied_constraint_from_nargo_error(nargo_err: &NargoError) -> Op _ => None, } } + fn report_unsatisfied_constraint_error( opcode_idx: Option, debug: &DebugInfo, context: &Context, ) { if let Some(opcode_index) = opcode_idx { - if let Some(loc) = debug.opcode_location(opcode_index) { - noirc_errors::reporter::report( - &context.file_manager, - &CustomDiagnostic::simple_error( - "Unsatisfied constraint".to_string(), - "Constraint failed".to_string(), - loc.span, - ), - Some(loc.file), - false, - ); + if let Some(locations) = debug.opcode_location(opcode_index) { + // The location of the error itself will be the location at the top + // of the call stack (the last item in the Vec). + if let Some(location) = locations.last() { + let message = "Failed constraint".into(); + CustomDiagnostic::simple_error(message, String::new(), location.span) + .in_file(location.file) + .with_call_stack(locations) + .report(&context.file_manager, false); + } } } } diff --git a/crates/noirc_driver/src/lib.rs b/crates/noirc_driver/src/lib.rs index 0e6a3c519b5..a4341e3a37b 100644 --- a/crates/noirc_driver/src/lib.rs +++ b/crates/noirc_driver/src/lib.rs @@ -151,12 +151,10 @@ pub fn compile_main( Some(m) => m, None => { // TODO(#2155): This error might be a better to exist in Nargo - let err = FileDiagnostic { - file_id: FileId::default(), - diagnostic: CustomDiagnostic::from_message( - "cannot compile crate into a program as it does not contain a `main` function", - ), - }; + let err = CustomDiagnostic::from_message( + "cannot compile crate into a program as it does not contain a `main` function", + ) + .in_file(FileId::default()); return Err(vec![err]); } }; @@ -226,13 +224,13 @@ fn compile_contract( options: &CompileOptions, ) -> Result> { let mut functions = Vec::new(); - let mut errs = Vec::new(); + let mut errors = Vec::new(); for function_id in &contract.functions { let name = context.function_name(function_id).to_owned(); let function = match compile_no_check(context, options, *function_id) { Ok(function) => function, - Err(err) => { - errs.push(err); + Err(new_error) => { + errors.push(new_error); continue; } }; @@ -253,10 +251,10 @@ fn compile_contract( }); } - if errs.is_empty() { + if errors.is_empty() { Ok(CompiledContract { name: contract.name, functions }) } else { - Err(errs) + Err(errors) } } diff --git a/crates/noirc_errors/src/debug_info.rs b/crates/noirc_errors/src/debug_info.rs index 0c05bee61d5..56c9699042e 100644 --- a/crates/noirc_errors/src/debug_info.rs +++ b/crates/noirc_errors/src/debug_info.rs @@ -6,11 +6,11 @@ use serde::{Deserialize, Serialize}; #[derive(Default, Debug, Clone, Deserialize, Serialize)] pub struct DebugInfo { /// Map opcode index of an ACIR circuit into the source code location - pub locations: BTreeMap, + pub locations: BTreeMap>, } impl DebugInfo { - pub fn new(locations: BTreeMap) -> Self { + pub fn new(locations: BTreeMap>) -> Self { DebugInfo { locations } } @@ -27,13 +27,13 @@ impl DebugInfo { let mut new_locations = BTreeMap::new(); for (i, idx) in opcode_indices.iter().enumerate() { if self.locations.contains_key(idx) { - new_locations.insert(i, self.locations[idx]); + new_locations.insert(i, self.locations[idx].clone()); } } self.locations = new_locations; } - pub fn opcode_location(&self, idx: usize) -> Option<&Location> { - self.locations.get(&idx) + pub fn opcode_location(&self, idx: usize) -> Option> { + self.locations.get(&idx).cloned() } } diff --git a/crates/noirc_errors/src/lib.rs b/crates/noirc_errors/src/lib.rs index 43d5715fc54..6eaa96e0cb1 100644 --- a/crates/noirc_errors/src/lib.rs +++ b/crates/noirc_errors/src/lib.rs @@ -13,6 +13,21 @@ pub use reporter::{CustomDiagnostic, DiagnosticKind}; pub struct FileDiagnostic { pub file_id: fm::FileId, pub diagnostic: CustomDiagnostic, + + /// An optional call stack to display the full runtime call stack + /// leading up to a runtime error. If this is empty it will not be displayed. + pub call_stack: Vec, +} + +impl FileDiagnostic { + pub fn new(file_id: fm::FileId, diagnostic: CustomDiagnostic) -> FileDiagnostic { + FileDiagnostic { file_id, diagnostic, call_stack: Vec::new() } + } + + pub fn with_call_stack(mut self, call_stack: Vec) -> Self { + self.call_stack = call_stack; + self + } } impl From for Vec { diff --git a/crates/noirc_errors/src/reporter.rs b/crates/noirc_errors/src/reporter.rs index 6a595e5d4f6..8f4cdd9b551 100644 --- a/crates/noirc_errors/src/reporter.rs +++ b/crates/noirc_errors/src/reporter.rs @@ -1,4 +1,4 @@ -use crate::{FileDiagnostic, Span}; +use crate::{FileDiagnostic, Location, Span}; use codespan_reporting::diagnostic::{Diagnostic, Label}; use codespan_reporting::term; use codespan_reporting::term::termcolor::{ColorChoice, StandardStream}; @@ -60,7 +60,7 @@ impl CustomDiagnostic { } pub fn in_file(self, file_id: fm::FileId) -> FileDiagnostic { - FileDiagnostic { file_id, diagnostic: self } + FileDiagnostic::new(file_id, self) } pub fn add_note(&mut self, message: String) { @@ -115,25 +115,31 @@ pub fn report_all( diagnostics: &[FileDiagnostic], deny_warnings: bool, ) -> ReportedErrors { - let error_count = diagnostics - .iter() - .map(|error| report(files, &error.diagnostic, Some(error.file_id), deny_warnings) as u32) - .sum(); + let error_count = + diagnostics.iter().map(|error| error.report(files, deny_warnings) as u32).sum(); ReportedErrors { error_count } } +impl FileDiagnostic { + pub fn report(&self, files: &fm::FileManager, deny_warnings: bool) -> bool { + report(files, &self.diagnostic, Some(self.file_id), &self.call_stack, deny_warnings) + } +} + /// Report the given diagnostic, and return true if it was an error pub fn report( files: &fm::FileManager, custom_diagnostic: &CustomDiagnostic, file: Option, + call_stack: &[Location], deny_warnings: bool, ) -> bool { let writer = StandardStream::stderr(ColorChoice::Always); let config = codespan_reporting::term::Config::default(); - let diagnostic = convert_diagnostic(custom_diagnostic, file, deny_warnings); + let stack_trace = stack_trace(files, call_stack); + let diagnostic = convert_diagnostic(custom_diagnostic, file, stack_trace, deny_warnings); term::emit(&mut writer.lock(), &config, files.as_simple_files(), &diagnostic).unwrap(); deny_warnings || custom_diagnostic.is_error() @@ -142,6 +148,7 @@ pub fn report( fn convert_diagnostic( cd: &CustomDiagnostic, file: Option, + stack_trace: String, deny_warnings: bool, ) -> Diagnostic { let diagnostic = match (cd.kind, deny_warnings) { @@ -162,5 +169,46 @@ fn convert_diagnostic( vec![] }; - diagnostic.with_message(&cd.message).with_labels(secondary_labels).with_notes(cd.notes.clone()) + let mut notes = cd.notes.clone(); + notes.push(stack_trace); + + diagnostic.with_message(&cd.message).with_labels(secondary_labels).with_notes(notes) +} + +fn stack_trace(files: &fm::FileManager, call_stack: &[Location]) -> String { + if call_stack.is_empty() { + return String::new(); + } + + let mut result = "Call stack:\n".to_string(); + + for (i, call_item) in call_stack.iter().enumerate() { + let path = files.path(call_item.file); + let source = files.fetch_file(call_item.file).source(); + + let (line, column) = location(source, call_item.span.start()); + result += &format!("{}. {}.nr:{}:{}\n", i + 1, path.display(), line, column); + } + + result +} + +fn location(source: &str, span_start: u32) -> (u32, u32) { + let mut line = 1; + let mut column = 0; + + for (i, char) in source.chars().enumerate() { + column += 1; + + if char == '\n' { + line += 1; + column = 0; + } + + if span_start <= i as u32 { + break; + } + } + + (line, column) } diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 943826201d2..99af0b4eed0 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -109,7 +109,7 @@ impl<'block> BrilligBlock<'block> { self.create_block_label_for_current_function(*else_destination), ); } - TerminatorInstruction::Jmp { destination, arguments, location: _ } => { + TerminatorInstruction::Jmp { destination, arguments, call_stack: _ } => { let target = &dfg[*destination]; for (src, dest) in arguments.iter().zip(target.parameters()) { // Destination variable might have already been created by another block that jumps to this target diff --git a/crates/noirc_evaluator/src/errors.rs b/crates/noirc_evaluator/src/errors.rs index aebadfcd0ae..847bff7f46c 100644 --- a/crates/noirc_evaluator/src/errors.rs +++ b/crates/noirc_evaluator/src/errors.rs @@ -8,95 +8,105 @@ //! //! An Error of the latter is an error in the implementation of the compiler use acvm::FieldElement; -use noirc_errors::{CustomDiagnostic as Diagnostic, FileDiagnostic, Location}; +use iter_extended::vecmap; +use noirc_errors::{CustomDiagnostic as Diagnostic, FileDiagnostic}; use thiserror::Error; +use crate::ssa::ir::dfg::CallStack; + #[derive(Debug, PartialEq, Eq, Clone, Error)] pub enum RuntimeError { // We avoid showing the actual lhs and rhs since most of the time they are just 0 // and 1 respectively. This would confuse users if a constraint such as // assert(foo < bar) fails with "failed constraint: 0 = 1." #[error("Failed constraint")] - FailedConstraint { lhs: FieldElement, rhs: FieldElement, location: Option }, + FailedConstraint { lhs: FieldElement, rhs: FieldElement, call_stack: CallStack }, #[error(transparent)] InternalError(#[from] InternalError), #[error("Index out of bounds, array has size {index:?}, but index was {array_size:?}")] - IndexOutOfBounds { index: usize, array_size: usize, location: Option }, + IndexOutOfBounds { index: usize, array_size: usize, call_stack: CallStack }, #[error("Range constraint of {num_bits} bits is too large for the Field size")] - InvalidRangeConstraint { num_bits: u32, location: Option }, + InvalidRangeConstraint { num_bits: u32, call_stack: CallStack }, #[error("Expected array index to fit into a u64")] - TypeConversion { from: String, into: String, location: Option }, + TypeConversion { from: String, into: String, call_stack: CallStack }, #[error("{name:?} is not initialized")] - UnInitialized { name: String, location: Option }, + UnInitialized { name: String, call_stack: CallStack }, #[error("Integer sized {num_bits:?} is over the max supported size of {max_num_bits:?}")] - UnsupportedIntegerSize { num_bits: u32, max_num_bits: u32, location: Option }, + UnsupportedIntegerSize { num_bits: u32, max_num_bits: u32, call_stack: CallStack }, #[error("Could not determine loop bound at compile-time")] - UnknownLoopBound { location: Option }, + UnknownLoopBound { call_stack: CallStack }, #[error("Argument is not constant")] - AssertConstantFailed { location: Option }, + AssertConstantFailed { call_stack: CallStack }, } #[derive(Debug, PartialEq, Eq, Clone, Error)] pub enum InternalError { #[error("ICE: Both expressions should have degree<=1")] - DegreeNotReduced { location: Option }, + DegreeNotReduced { call_stack: CallStack }, #[error("Try to get element from empty array")] - EmptyArray { location: Option }, + EmptyArray { call_stack: CallStack }, #[error("ICE: {message:?}")] - General { message: String, location: Option }, + General { message: String, call_stack: CallStack }, #[error("ICE: {name:?} missing {arg:?} arg")] - MissingArg { name: String, arg: String, location: Option }, + MissingArg { name: String, arg: String, call_stack: CallStack }, #[error("ICE: {name:?} should be a constant")] - NotAConstant { name: String, location: Option }, + NotAConstant { name: String, call_stack: CallStack }, #[error("ICE: Undeclared AcirVar")] - UndeclaredAcirVar { location: Option }, + UndeclaredAcirVar { call_stack: CallStack }, #[error("ICE: Expected {expected:?}, found {found:?}")] - UnExpected { expected: String, found: String, location: Option }, + UnExpected { expected: String, found: String, call_stack: CallStack }, } impl RuntimeError { - fn location(&self) -> Option { + fn call_stack(&self) -> &CallStack { match self { RuntimeError::InternalError( - InternalError::DegreeNotReduced { location } - | InternalError::EmptyArray { location } - | InternalError::General { location, .. } - | InternalError::MissingArg { location, .. } - | InternalError::NotAConstant { location, .. } - | InternalError::UndeclaredAcirVar { location } - | InternalError::UnExpected { location, .. }, + InternalError::DegreeNotReduced { call_stack } + | InternalError::EmptyArray { call_stack } + | InternalError::General { call_stack, .. } + | InternalError::MissingArg { call_stack, .. } + | InternalError::NotAConstant { call_stack, .. } + | InternalError::UndeclaredAcirVar { call_stack } + | InternalError::UnExpected { call_stack, .. }, ) - | RuntimeError::FailedConstraint { location, .. } - | RuntimeError::IndexOutOfBounds { location, .. } - | RuntimeError::InvalidRangeConstraint { location, .. } - | RuntimeError::TypeConversion { location, .. } - | RuntimeError::UnInitialized { location, .. } - | RuntimeError::UnknownLoopBound { location } - | RuntimeError::AssertConstantFailed { location } - | RuntimeError::UnsupportedIntegerSize { location, .. } => *location, + | RuntimeError::FailedConstraint { call_stack, .. } + | RuntimeError::IndexOutOfBounds { call_stack, .. } + | RuntimeError::InvalidRangeConstraint { call_stack, .. } + | RuntimeError::TypeConversion { call_stack, .. } + | RuntimeError::UnInitialized { call_stack, .. } + | RuntimeError::UnknownLoopBound { call_stack } + | RuntimeError::AssertConstantFailed { call_stack } + | RuntimeError::UnsupportedIntegerSize { call_stack, .. } => call_stack, } } } impl From for FileDiagnostic { - fn from(error: RuntimeError) -> Self { - let file_id = error.location().map(|loc| loc.file).unwrap(); - FileDiagnostic { file_id, diagnostic: error.into() } + fn from(error: RuntimeError) -> FileDiagnostic { + let call_stack = vecmap(error.call_stack(), |location| *location); + let diagnostic = error.into_diagnostic(); + let file_id = call_stack.last().map(|location| location.file).unwrap_or_default(); + + diagnostic.in_file(file_id).with_call_stack(call_stack) } } -impl From for Diagnostic { - fn from(error: RuntimeError) -> Diagnostic { - match error { - RuntimeError::InternalError(_) => Diagnostic::simple_error( - "Internal Consistency Evaluators Errors: \n - This is likely a bug. Consider Opening an issue at https://github.com/noir-lang/noir/issues".to_owned(), - "".to_string(), - noirc_errors::Span::new(0..0) - ), +impl RuntimeError { + fn into_diagnostic(self) -> Diagnostic { + match self { + RuntimeError::InternalError(_) => { + Diagnostic::simple_error( + "Internal Consistency Evaluators Errors: \n + This is likely a bug. Consider Opening an issue at https://github.com/noir-lang/noir/issues".to_owned(), + "".to_string(), + noirc_errors::Span::new(0..0) + ) + } _ => { - let span = if let Some(loc) = error.location() { loc.span } else { noirc_errors::Span::new(0..0) }; - Diagnostic::simple_error("".to_owned(), error.to_string(), span) + let message = self.to_string(); + let location = self.call_stack().back().expect("Expected RuntimeError to have a location"); + + Diagnostic::simple_error(message, String::new(), location.span) } } } diff --git a/crates/noirc_evaluator/src/ssa.rs b/crates/noirc_evaluator/src/ssa.rs index 10099037dfe..d6cc309907f 100644 --- a/crates/noirc_evaluator/src/ssa.rs +++ b/crates/noirc_evaluator/src/ssa.rs @@ -104,6 +104,13 @@ pub fn create_circuit( public_parameters, return_values, }; + + // This converts each im::Vector in the BTreeMap to a Vec + let locations = locations + .into_iter() + .map(|(index, locations)| (index, locations.into_iter().collect())) + .collect(); + let debug_info = DebugInfo::new(locations); Ok((circuit, debug_info, abi)) diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs b/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs index cf4c4160e9b..382d12e7c74 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs @@ -2,6 +2,7 @@ use super::generated_acir::GeneratedAcir; use crate::brillig::brillig_gen::brillig_directive; use crate::errors::{InternalError, RuntimeError}; use crate::ssa::acir_gen::{AcirDynamicArray, AcirValue}; +use crate::ssa::ir::dfg::CallStack; use crate::ssa::ir::types::Type as SsaType; use crate::ssa::ir::{instruction::Endian, types::NumericType}; use acvm::acir::circuit::opcodes::{BlockId, MemOp}; @@ -21,7 +22,6 @@ use acvm::{ }; use acvm::{BlackBoxFunctionSolver, BlackBoxResolutionError}; use iter_extended::{try_vecmap, vecmap}; -use noirc_errors::Location; use std::collections::HashMap; use std::{borrow::Cow, hash::Hash}; @@ -153,12 +153,12 @@ impl AcirContext { self.add_data(var_data) } - pub(crate) fn get_location(&self) -> Option { - self.acir_ir.current_location + pub(crate) fn get_call_stack(&self) -> CallStack { + self.acir_ir.call_stack.clone() } - pub(crate) fn set_location(&mut self, location: Option) { - self.acir_ir.current_location = location; + pub(crate) fn set_call_stack(&mut self, call_stack: CallStack) { + self.acir_ir.call_stack = call_stack; } /// Converts an [`AcirVar`] to a [`Witness`] @@ -171,7 +171,9 @@ impl AcirContext { fn var_to_expression(&self, var: AcirVar) -> Result { let var_data = match self.vars.get(&var) { Some(var_data) => var_data, - None => return Err(InternalError::UndeclaredAcirVar { location: self.get_location() }), + None => { + return Err(InternalError::UndeclaredAcirVar { call_stack: self.get_call_stack() }) + } }; Ok(var_data.to_expression().into_owned()) } @@ -338,7 +340,7 @@ impl AcirContext { Err(RuntimeError::FailedConstraint { lhs: *lhs_const, rhs: *rhs_const, - location: self.get_location(), + call_stack: self.get_call_stack(), }) } } else { @@ -673,7 +675,7 @@ impl AcirContext { return Err(RuntimeError::InternalError(InternalError::MissingArg { name: "pedersen call".to_string(), arg: "domain separator".to_string(), - location: self.get_location(), + call_stack: self.get_call_stack(), })) } }; @@ -683,7 +685,7 @@ impl AcirContext { None => { return Err(RuntimeError::InternalError(InternalError::NotAConstant { name: "domain separator".to_string(), - location: self.get_location(), + call_stack: self.get_call_stack(), })) } }; @@ -749,7 +751,7 @@ impl AcirContext { None => { return Err(RuntimeError::InternalError(InternalError::NotAConstant { name: "radix".to_string(), - location: self.get_location(), + call_stack: self.get_call_stack(), })); } }; @@ -759,7 +761,7 @@ impl AcirContext { None => { return Err(RuntimeError::InternalError(InternalError::NotAConstant { name: "limb_size".to_string(), - location: self.get_location(), + call_stack: self.get_call_stack(), })); } }; diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs b/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs index fd88921bb3d..8fc46db2b1a 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs @@ -5,6 +5,7 @@ use std::collections::BTreeMap; use crate::{ brillig::brillig_gen::brillig_directive, errors::{InternalError, RuntimeError}, + ssa::ir::dfg::CallStack, }; use acvm::acir::{ @@ -22,7 +23,6 @@ use acvm::{ FieldElement, }; use iter_extended::vecmap; -use noirc_errors::Location; use num_bigint::BigUint; #[derive(Debug, Default)] @@ -45,12 +45,12 @@ pub(crate) struct GeneratedAcir { /// All witness indices which are inputs to the main function pub(crate) input_witnesses: Vec, - /// Correspondance between an opcode index (in opcodes) and the source code location which generated it - pub(crate) locations: BTreeMap, + /// Correspondance between an opcode index (in opcodes) and the source code call stack which generated it + pub(crate) locations: BTreeMap, /// Source code location of the current instruction being processed /// None if we do not know the location - pub(crate) current_location: Option, + pub(crate) call_stack: CallStack, } impl GeneratedAcir { @@ -62,8 +62,8 @@ impl GeneratedAcir { /// Adds a new opcode into ACIR. fn push_opcode(&mut self, opcode: AcirOpcode) { self.opcodes.push(opcode); - if let Some(location) = self.current_location { - self.locations.insert(self.opcodes.len() - 1, location); + if !self.call_stack.is_empty() { + self.locations.insert(self.opcodes.len() - 1, self.call_stack.clone()); } } @@ -195,7 +195,7 @@ impl GeneratedAcir { return Err(InternalError::MissingArg { name: "".to_string(), arg: "message_size".to_string(), - location: self.current_location, + call_stack: self.call_stack.clone(), }); } }; @@ -706,7 +706,7 @@ impl GeneratedAcir { if num_bits >= FieldElement::max_num_bits() { return Err(RuntimeError::InvalidRangeConstraint { num_bits: FieldElement::max_num_bits(), - location: self.current_location, + call_stack: self.call_stack.clone(), }); }; diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/sort.rs b/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/sort.rs index 42a6a5f1a4a..52640d32337 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/sort.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/sort.rs @@ -50,7 +50,9 @@ impl GeneratedAcir { if n % 2 == 1 { in_sub2.push(match in_expr.last() { Some(in_expr) => in_expr.clone(), - None => return Err(InternalError::EmptyArray { location: self.current_location }), + None => { + return Err(InternalError::EmptyArray { call_stack: self.call_stack.clone() }) + } }); } let mut out_expr = Vec::new(); @@ -70,12 +72,14 @@ impl GeneratedAcir { if n % 2 == 0 { out_expr.push(match b1.last() { Some(b1) => b1.clone(), - None => return Err(InternalError::EmptyArray { location: self.current_location }), + None => { + return Err(InternalError::EmptyArray { call_stack: self.call_stack.clone() }) + } }); } out_expr.push(match b2.last() { Some(b2) => b2.clone(), - None => return Err(InternalError::EmptyArray { location: self.current_location }), + None => return Err(InternalError::EmptyArray { call_stack: self.call_stack.clone() }), }); conf.extend(w1); conf.extend(w2); diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs index 7409a199641..96ff3acc426 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -6,6 +6,7 @@ use std::fmt::Debug; use std::ops::RangeInclusive; use self::acir_ir::acir_variable::{AcirContext, AcirType, AcirVar}; +use super::ir::dfg::CallStack; use super::{ ir::{ dfg::DataFlowGraph, @@ -93,7 +94,7 @@ impl AcirValue { AcirValue::Var(var, _) => Ok(var), AcirValue::DynamicArray(_) | AcirValue::Array(_) => Err(InternalError::General { message: "Called AcirValue::into_var on an array".to_string(), - location: None, + call_stack: CallStack::new(), }), } } @@ -320,7 +321,7 @@ impl Context { last_array_uses: &HashMap, ) -> Result<(), RuntimeError> { let instruction = &dfg[instruction_id]; - self.acir_context.set_location(dfg.get_location(&instruction_id)); + self.acir_context.set_call_stack(dfg.get_call_stack(instruction_id)); match instruction { Instruction::Binary(binary) => { let result_acir_var = self.convert_ssa_binary(binary, dfg)?; @@ -426,7 +427,7 @@ impl Context { unreachable!("Expected all load instructions to be removed before acir_gen") } } - self.acir_context.set_location(None); + self.acir_context.set_call_stack(CallStack::new()); Ok(()) } @@ -449,7 +450,7 @@ impl Context { None => { return Err(InternalError::General { message: format!("Cannot find linked fn {unresolved_fn_label}"), - location: None, + call_stack: CallStack::new(), }) } }; @@ -478,7 +479,7 @@ impl Context { return Err(RuntimeError::InternalError(InternalError::UnExpected { expected: "an array value".to_string(), found: format!("{acir_var:?}"), - location: self.acir_context.get_location(), + call_stack: self.acir_context.get_call_stack(), })) } AcirValue::Array(array) => { @@ -487,11 +488,11 @@ impl Context { let index = match index_const.try_to_u64() { Some(index_const) => index_const as usize, None => { - let location = self.acir_context.get_location(); + let call_stack = self.acir_context.get_call_stack(); return Err(RuntimeError::TypeConversion { from: "array index".to_string(), into: "u64".to_string(), - location, + call_stack, }); } }; @@ -499,11 +500,11 @@ impl Context { // Ignore the error if side effects are disabled. if self.acir_context.is_constant_one(&self.current_side_effects_enabled_var) { - let location = self.acir_context.get_location(); + let call_stack = self.acir_context.get_call_stack(); return Err(RuntimeError::IndexOutOfBounds { index, array_size, - location, + call_stack, }); } let result_type = @@ -558,7 +559,7 @@ impl Context { _ => { return Err(RuntimeError::UnInitialized { name: "array".to_string(), - location: self.acir_context.get_location(), + call_stack: self.acir_context.get_call_stack(), }) } } @@ -620,7 +621,7 @@ impl Context { _ => { return Err(InternalError::General { message: format!("Array {array} should be initialized"), - location: self.acir_context.get_location(), + call_stack: self.acir_context.get_call_stack(), }) } } @@ -770,12 +771,12 @@ impl Context { AcirValue::Array(array) => Err(InternalError::UnExpected { expected: "a numeric value".to_string(), found: format!("{array:?}"), - location: self.acir_context.get_location(), + call_stack: self.acir_context.get_call_stack(), }), AcirValue::DynamicArray(_) => Err(InternalError::UnExpected { expected: "a numeric value".to_string(), found: "an array".to_string(), - location: self.acir_context.get_location(), + call_stack: self.acir_context.get_call_stack(), }), } } @@ -801,7 +802,7 @@ impl Context { return Err(RuntimeError::UnsupportedIntegerSize { num_bits: *bit_size, max_num_bits: max_integer_bit_size, - location: self.acir_context.get_location(), + call_stack: self.acir_context.get_call_stack(), }); } } diff --git a/crates/noirc_evaluator/src/ssa/ir/cfg.rs b/crates/noirc_evaluator/src/ssa/ir/cfg.rs index 8faa0fa8565..8d58511eb40 100644 --- a/crates/noirc_evaluator/src/ssa/ir/cfg.rs +++ b/crates/noirc_evaluator/src/ssa/ir/cfg.rs @@ -220,7 +220,7 @@ mod tests { func.dfg[block2_id].set_terminator(TerminatorInstruction::Jmp { destination: ret_block_id, arguments: vec![], - location: None, + call_stack: im::Vector::new(), }); func.dfg[block0_id].set_terminator(TerminatorInstruction::JmpIf { condition: cond, diff --git a/crates/noirc_evaluator/src/ssa/ir/dfg.rs b/crates/noirc_evaluator/src/ssa/ir/dfg.rs index 28879e99701..7dcf652c6e6 100644 --- a/crates/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/crates/noirc_evaluator/src/ssa/ir/dfg.rs @@ -73,11 +73,19 @@ pub(crate) struct DataFlowGraph { /// Source location of each instruction for debugging and issuing errors. /// + /// The `CallStack` here corresponds to the entire callstack of locations. Initially this + /// only contains the actual location of the instruction. During inlining, a new location + /// will be pushed to each instruction for the location of the function call of the function + /// the instruction was originally located in. Once inlining is complete, the locations Vec + /// here should contain the entire callstack for each instruction. + /// /// Instructions inserted by internal SSA passes that don't correspond to user code /// may not have a corresponding location. - locations: HashMap, + locations: HashMap, } +pub(crate) type CallStack = im::Vector; + impl DataFlowGraph { /// Creates a new basic block with no parameters. /// After being created, the block is unreachable in the current function @@ -149,7 +157,7 @@ impl DataFlowGraph { instruction: Instruction, block: BasicBlockId, ctrl_typevars: Option>, - location: Option, + call_stack: CallStack, ) -> InsertInstructionResult { use InsertInstructionResult::*; match instruction.simplify(self, block) { @@ -162,9 +170,7 @@ impl DataFlowGraph { let instruction = result.instruction().unwrap_or(instruction); let id = self.make_instruction(instruction, ctrl_typevars); self.blocks[block].insert_instruction(id); - if let Some(location) = location { - self.locations.insert(id, location); - } + self.locations.insert(id, call_stack); InsertInstructionResult::Results(self.instruction_results(id)) } } @@ -407,14 +413,18 @@ impl DataFlowGraph { destination.set_terminator(terminator); } - pub(crate) fn get_location(&self, id: &InstructionId) -> Option { - self.locations.get(id).copied() + pub(crate) fn get_call_stack(&self, instruction: InstructionId) -> CallStack { + self.locations.get(&instruction).cloned().unwrap_or_default() } - pub(crate) fn get_value_location(&self, id: &ValueId) -> Option { - match &self.values[self.resolve(*id)] { - Value::Instruction { instruction, .. } => self.get_location(instruction), - _ => None, + pub(crate) fn add_location(&mut self, instruction: InstructionId, location: Location) { + self.locations.entry(instruction).or_default().push_back(location); + } + + pub(crate) fn get_value_call_stack(&self, value: ValueId) -> CallStack { + match &self.values[self.resolve(value)] { + Value::Instruction { instruction, .. } => self.get_call_stack(*instruction), + _ => im::Vector::new(), } } diff --git a/crates/noirc_evaluator/src/ssa/ir/function_inserter.rs b/crates/noirc_evaluator/src/ssa/ir/function_inserter.rs index 15c755f40c2..bc0084b6d4e 100644 --- a/crates/noirc_evaluator/src/ssa/ir/function_inserter.rs +++ b/crates/noirc_evaluator/src/ssa/ir/function_inserter.rs @@ -1,11 +1,10 @@ use std::collections::HashMap; use iter_extended::vecmap; -use noirc_errors::Location; use super::{ basic_block::BasicBlockId, - dfg::InsertInstructionResult, + dfg::{CallStack, InsertInstructionResult}, function::Function, instruction::{Instruction, InstructionId}, value::ValueId, @@ -56,10 +55,10 @@ impl<'f> FunctionInserter<'f> { self.values.insert(key, value); } - pub(crate) fn map_instruction(&mut self, id: InstructionId) -> (Instruction, Option) { + pub(crate) fn map_instruction(&mut self, id: InstructionId) -> (Instruction, CallStack) { ( self.function.dfg[id].clone().map_values(|id| self.resolve(id)), - self.function.dfg.get_location(&id), + self.function.dfg.get_call_stack(id), ) } @@ -73,7 +72,7 @@ impl<'f> FunctionInserter<'f> { instruction: Instruction, id: InstructionId, block: BasicBlockId, - location: Option, + call_stack: CallStack, ) -> InsertInstructionResult { let results = self.function.dfg.instruction_results(id); let results = vecmap(results, |id| self.function.dfg.resolve(*id)); @@ -86,7 +85,7 @@ impl<'f> FunctionInserter<'f> { instruction, block, ctrl_typevars, - location, + call_stack, ); Self::insert_new_instruction_results(&mut self.values, &results, &new_results); diff --git a/crates/noirc_evaluator/src/ssa/ir/instruction.rs b/crates/noirc_evaluator/src/ssa/ir/instruction.rs index e09e21f158f..b1bde908b84 100644 --- a/crates/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/crates/noirc_evaluator/src/ssa/ir/instruction.rs @@ -1,11 +1,10 @@ use acvm::{acir::BlackBoxFunc, FieldElement}; use iter_extended::vecmap; -use noirc_errors::Location; use num_bigint::BigUint; use super::{ basic_block::BasicBlockId, - dfg::DataFlowGraph, + dfg::{CallStack, DataFlowGraph}, map::Id, types::{NumericType, Type}, value::{Value, ValueId}, @@ -427,9 +426,9 @@ pub(crate) enum TerminatorInstruction { /// Unconditional Jump /// /// Jumps to specified `destination` with `arguments`. - /// The optional Location here is expected to be used to issue an error when the start range of + /// The CallStack here is expected to be used to issue an error when the start range of /// a for loop cannot be deduced at compile-time. - Jmp { destination: BasicBlockId, arguments: Vec, location: Option }, + Jmp { destination: BasicBlockId, arguments: Vec, call_stack: CallStack }, /// Return from the current function with the given return values. /// @@ -454,10 +453,10 @@ impl TerminatorInstruction { then_destination: *then_destination, else_destination: *else_destination, }, - Jmp { destination, arguments, location } => Jmp { + Jmp { destination, arguments, call_stack } => Jmp { destination: *destination, arguments: vecmap(arguments, |value| f(*value)), - location: *location, + call_stack: call_stack.clone(), }, Return { return_values } => { Return { return_values: vecmap(return_values, |value| f(*value)) } diff --git a/crates/noirc_evaluator/src/ssa/ir/printer.rs b/crates/noirc_evaluator/src/ssa/ir/printer.rs index e6faf4570c5..e8cea151ad1 100644 --- a/crates/noirc_evaluator/src/ssa/ir/printer.rs +++ b/crates/noirc_evaluator/src/ssa/ir/printer.rs @@ -100,7 +100,7 @@ pub(crate) fn display_terminator( f: &mut Formatter, ) -> Result { match terminator { - Some(TerminatorInstruction::Jmp { destination, arguments, location: _ }) => { + Some(TerminatorInstruction::Jmp { destination, arguments, call_stack: _ }) => { writeln!(f, " jmp {}({})", destination, value_list(function, arguments)) } Some(TerminatorInstruction::JmpIf { condition, then_destination, else_destination }) => { diff --git a/crates/noirc_evaluator/src/ssa/opt/assert_constant.rs b/crates/noirc_evaluator/src/ssa/opt/assert_constant.rs index 81cee2ffde4..cd3a509a62e 100644 --- a/crates/noirc_evaluator/src/ssa/opt/assert_constant.rs +++ b/crates/noirc_evaluator/src/ssa/opt/assert_constant.rs @@ -77,7 +77,7 @@ fn evaluate_assert_constant( if arguments.iter().all(|arg| function.dfg.is_constant(*arg)) { Ok(false) } else { - let location = function.dfg.get_location(&instruction); - Err(RuntimeError::AssertConstantFailed { location }) + let call_stack = function.dfg.get_call_stack(instruction); + Err(RuntimeError::AssertConstantFailed { call_stack }) } } diff --git a/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs b/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs index c4f3184f794..75ff92ebbf1 100644 --- a/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -71,12 +71,12 @@ impl Context { .requires_ctrl_typevars() .then(|| vecmap(&old_results, |result| function.dfg.type_of_value(*result))); - let location = function.dfg.get_location(&id); + let call_stack = function.dfg.get_call_stack(id); let new_results = match function.dfg.insert_instruction_and_results( instruction, block, ctrl_typevars, - location, + call_stack, ) { InsertInstructionResult::SimplifiedTo(new_result) => vec![new_result], InsertInstructionResult::SimplifiedToMultiple(new_results) => new_results, diff --git a/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 8481f5610bb..3df27868737 100644 --- a/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -135,13 +135,12 @@ use std::collections::{BTreeMap, HashMap, HashSet}; use acvm::FieldElement; use iter_extended::vecmap; -use noirc_errors::Location; use crate::ssa::{ ir::{ basic_block::BasicBlockId, cfg::ControlFlowGraph, - dfg::InsertInstructionResult, + dfg::{CallStack, InsertInstructionResult}, function::Function, function_inserter::FunctionInserter, instruction::{BinaryOp, Instruction, InstructionId, TerminatorInstruction}, @@ -260,7 +259,7 @@ impl<'f> Context<'f> { self.inline_branch(block, then_block, old_condition, then_condition, one); let else_condition = - self.insert_instruction(Instruction::Not(then_condition), None); + self.insert_instruction(Instruction::Not(then_condition), CallStack::new()); let zero = FieldElement::zero(); // Make sure the else branch sees the previous values of each store @@ -285,7 +284,7 @@ impl<'f> Context<'f> { let end = self.branch_ends[&block]; self.inline_branch_end(end, then_branch, else_branch) } - TerminatorInstruction::Jmp { destination, arguments, location: _ } => { + TerminatorInstruction::Jmp { destination, arguments, call_stack: _ } => { if let Some((end_block, _)) = self.conditions.last() { if destination == end_block { return block; @@ -317,7 +316,7 @@ impl<'f> Context<'f> { if let Some((_, previous_condition)) = self.conditions.last() { let and = Instruction::binary(BinaryOp::And, *previous_condition, condition); - let new_condition = self.insert_instruction(and, None); + let new_condition = self.insert_instruction(and, CallStack::new()); self.conditions.push((end_block, new_condition)); } else { self.conditions.push((end_block, condition)); @@ -327,16 +326,12 @@ impl<'f> Context<'f> { /// Insert a new instruction into the function's entry block. /// Unlike push_instruction, this function will not map any ValueIds. /// within the given instruction, nor will it modify self.values in any way. - fn insert_instruction( - &mut self, - instruction: Instruction, - location: Option, - ) -> ValueId { + fn insert_instruction(&mut self, instruction: Instruction, call_stack: CallStack) -> ValueId { let block = self.inserter.function.entry_block(); self.inserter .function .dfg - .insert_instruction_and_results(instruction, block, None, location) + .insert_instruction_and_results(instruction, block, None, call_stack) .first() } @@ -354,7 +349,7 @@ impl<'f> Context<'f> { instruction, block, ctrl_typevars, - None, + CallStack::new(), ) } @@ -465,22 +460,27 @@ impl<'f> Context<'f> { "Expected values merged to be of the same type but found {then_type} and {else_type}" ); - let then_location = self.inserter.function.dfg.get_value_location(&then_value); - let else_location = self.inserter.function.dfg.get_value_location(&else_value); - let merge_location = then_location.or(else_location); + let then_call_stack = self.inserter.function.dfg.get_value_call_stack(then_value); + let else_call_stack = self.inserter.function.dfg.get_value_call_stack(else_value); + + let call_stack = if then_call_stack.is_empty() { + else_call_stack.clone() + } else { + then_call_stack.clone() + }; // We must cast the bool conditions to the actual numeric type used by each value. let then_condition = - self.insert_instruction(Instruction::Cast(then_condition, then_type), then_location); + self.insert_instruction(Instruction::Cast(then_condition, then_type), then_call_stack); let else_condition = - self.insert_instruction(Instruction::Cast(else_condition, else_type), else_location); + self.insert_instruction(Instruction::Cast(else_condition, else_type), else_call_stack); let mul = Instruction::binary(BinaryOp::Mul, then_condition, then_value); let then_value = self .inserter .function .dfg - .insert_instruction_and_results(mul, block, None, merge_location) + .insert_instruction_and_results(mul, block, None, call_stack.clone()) .first(); let mul = Instruction::binary(BinaryOp::Mul, else_condition, else_value); @@ -488,14 +488,14 @@ impl<'f> Context<'f> { .inserter .function .dfg - .insert_instruction_and_results(mul, block, None, merge_location) + .insert_instruction_and_results(mul, block, None, call_stack.clone()) .first(); let add = Instruction::binary(BinaryOp::Add, then_value, else_value); self.inserter .function .dfg - .insert_instruction_and_results(add, block, None, merge_location) + .insert_instruction_and_results(add, block, None, call_stack) .first() } @@ -681,12 +681,12 @@ impl<'f> Context<'f> { /// with a different InstructionId from the original. The results of the given instruction /// will also be mapped to the results of the new instruction. fn push_instruction(&mut self, id: InstructionId) { - let (instruction, location) = self.inserter.map_instruction(id); - let instruction = self.handle_instruction_side_effects(instruction, location); + let (instruction, call_stack) = self.inserter.map_instruction(id); + let instruction = self.handle_instruction_side_effects(instruction, call_stack.clone()); let is_allocate = matches!(instruction, Instruction::Allocate); let entry = self.inserter.function.entry_block(); - let results = self.inserter.push_instruction_value(instruction, id, entry, location); + let results = self.inserter.push_instruction_value(instruction, id, entry, call_stack); // Remember an allocate was created local to this branch so that we do not try to merge store // values across branches for it later. @@ -700,18 +700,18 @@ impl<'f> Context<'f> { fn handle_instruction_side_effects( &mut self, instruction: Instruction, - location: Option, + call_stack: CallStack, ) -> Instruction { if let Some((_, condition)) = self.conditions.last().copied() { match instruction { Instruction::Constrain(value) => { let mul = self.insert_instruction( Instruction::binary(BinaryOp::Mul, value, condition), - location, + call_stack.clone(), ); let eq = self.insert_instruction( Instruction::binary(BinaryOp::Eq, mul, condition), - location, + call_stack, ); Instruction::Constrain(eq) } diff --git a/crates/noirc_evaluator/src/ssa/opt/inlining.rs b/crates/noirc_evaluator/src/ssa/opt/inlining.rs index 36398bd98fe..cf7b5860ea6 100644 --- a/crates/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/crates/noirc_evaluator/src/ssa/opt/inlining.rs @@ -9,7 +9,7 @@ use iter_extended::vecmap; use crate::ssa::{ ir::{ basic_block::BasicBlockId, - dfg::InsertInstructionResult, + dfg::{CallStack, InsertInstructionResult}, function::{Function, FunctionId, RuntimeType}, instruction::{Instruction, InstructionId, TerminatorInstruction}, value::{Value, ValueId}, @@ -49,6 +49,8 @@ struct InlineContext { recursion_level: u32, builder: FunctionBuilder, + call_stack: CallStack, + /// True if we failed to inline at least one call. If this is still false when finishing /// inlining we can remove all other functions from the resulting Ssa struct and keep only /// the function that was inlined into. @@ -98,7 +100,12 @@ impl InlineContext { fn new(ssa: &Ssa) -> InlineContext { let main_name = ssa.main().name().to_owned(); let builder = FunctionBuilder::new(main_name, ssa.next_id.next(), RuntimeType::Acir); - Self { builder, recursion_level: 0, failed_to_inline_a_call: false } + Self { + builder, + recursion_level: 0, + call_stack: CallStack::new(), + failed_to_inline_a_call: false, + } } /// Start inlining the main function and all functions reachable from it. @@ -370,7 +377,21 @@ impl<'function> PerFunctionContext<'function> { ) { let old_results = self.source_function.dfg.instruction_results(call_id); let arguments = vecmap(arguments, |arg| self.translate_value(*arg)); + + let mut call_stack = self.source_function.dfg.get_call_stack(call_id); + let has_location = !call_stack.is_empty(); + + // Function calls created by the defunctionalization pass will not have source locations + if let Some(location) = call_stack.pop_back() { + self.context.call_stack.push_back(location); + } + let new_results = self.context.inline_function(ssa, function, &arguments); + + if has_location { + self.context.call_stack.pop_back(); + } + let new_results = InsertInstructionResult::Results(&new_results); Self::insert_new_instruction_results(&mut self.values, old_results, new_results); } @@ -379,7 +400,10 @@ impl<'function> PerFunctionContext<'function> { /// function being inlined into. fn push_instruction(&mut self, id: InstructionId) { let instruction = self.source_function.dfg[id].map_values(|id| self.translate_value(id)); - let location = self.source_function.dfg.get_location(&id); + + let mut call_stack = self.context.call_stack.clone(); + call_stack.append(self.source_function.dfg.get_call_stack(id)); + let results = self.source_function.dfg.instruction_results(id); let results = vecmap(results, |id| self.source_function.dfg.resolve(*id)); @@ -387,9 +411,8 @@ impl<'function> PerFunctionContext<'function> { .requires_ctrl_typevars() .then(|| vecmap(&results, |result| self.source_function.dfg.type_of_value(*result))); - if let Some(location) = location { - self.context.builder.set_location(location); - } + self.context.builder.set_call_stack(call_stack); + let new_results = self.context.builder.insert_instruction(instruction, ctrl_typevars); Self::insert_new_instruction_results(&mut self.values, &results, new_results); } @@ -433,14 +456,17 @@ impl<'function> PerFunctionContext<'function> { block_queue: &mut Vec, ) -> Option<(BasicBlockId, Vec)> { match self.source_function.dfg[block_id].unwrap_terminator() { - TerminatorInstruction::Jmp { destination, arguments, location } => { + TerminatorInstruction::Jmp { destination, arguments, call_stack } => { let destination = self.translate_block(*destination, block_queue); let arguments = vecmap(arguments, |arg| self.translate_value(*arg)); - self.context.builder.terminate_with_jmp_with_location( - destination, - arguments, - *location, - ); + + let mut new_call_stack = self.context.call_stack.clone(); + new_call_stack.append(call_stack.clone()); + + self.context + .builder + .set_call_stack(new_call_stack) + .terminate_with_jmp(destination, arguments); None } TerminatorInstruction::JmpIf { condition, then_destination, else_destination } => { diff --git a/crates/noirc_evaluator/src/ssa/opt/simplify_cfg.rs b/crates/noirc_evaluator/src/ssa/opt/simplify_cfg.rs index 26c7b161550..ad257362da5 100644 --- a/crates/noirc_evaluator/src/ssa/opt/simplify_cfg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/simplify_cfg.rs @@ -13,7 +13,7 @@ use std::collections::HashSet; use crate::ssa::{ ir::{ - basic_block::BasicBlockId, cfg::ControlFlowGraph, function::Function, + basic_block::BasicBlockId, cfg::ControlFlowGraph, dfg::CallStack, function::Function, instruction::TerminatorInstruction, }, ssa_gen::Ssa, @@ -87,7 +87,8 @@ fn check_for_constant_jmpif( if constant.is_zero() { *else_destination } else { *then_destination }; let arguments = Vec::new(); - let jmp = TerminatorInstruction::Jmp { destination, arguments, location: None }; + let jmp = + TerminatorInstruction::Jmp { destination, arguments, call_stack: CallStack::new() }; function.dfg[block].set_terminator(jmp); cfg.recompute_block(function, block); } diff --git a/crates/noirc_evaluator/src/ssa/opt/unrolling.rs b/crates/noirc_evaluator/src/ssa/opt/unrolling.rs index ac38f102712..603fbc6170a 100644 --- a/crates/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/crates/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -14,15 +14,13 @@ //! program that will need to be removed by a later simplify cfg pass. use std::collections::{HashMap, HashSet}; -use noirc_errors::Location; - use crate::{ errors::RuntimeError, ssa::{ ir::{ basic_block::BasicBlockId, cfg::ControlFlowGraph, - dfg::DataFlowGraph, + dfg::{CallStack, DataFlowGraph}, dom::DominatorTree, function::{Function, RuntimeType}, function_inserter::FunctionInserter, @@ -126,8 +124,8 @@ impl Loops { if !self.failed_to_unroll.contains(&next_loop.header) { match unroll_loop(function, &self.cfg, &next_loop) { Ok(_) => self.modified_blocks.extend(next_loop.blocks), - Err(location) if abort_on_error => { - return Err(RuntimeError::UnknownLoopBound { location }); + Err(call_stack) if abort_on_error => { + return Err(RuntimeError::UnknownLoopBound { call_stack }); } Err(_) => { self.failed_to_unroll.insert(next_loop.header); @@ -176,7 +174,7 @@ fn unroll_loop( function: &mut Function, cfg: &ControlFlowGraph, loop_: &Loop, -) -> Result<(), Option> { +) -> Result<(), CallStack> { let mut unroll_into = get_pre_header(cfg, loop_); let mut jump_value = get_induction_variable(function, unroll_into)?; @@ -206,12 +204,9 @@ fn get_pre_header(cfg: &ControlFlowGraph, loop_: &Loop) -> BasicBlockId { /// /// Expects the current block to terminate in `jmp h(N)` where h is the loop header and N is /// a Field value. -fn get_induction_variable( - function: &Function, - block: BasicBlockId, -) -> Result> { +fn get_induction_variable(function: &Function, block: BasicBlockId) -> Result { match function.dfg[block].terminator() { - Some(TerminatorInstruction::Jmp { arguments, location, .. }) => { + Some(TerminatorInstruction::Jmp { arguments, call_stack: location, .. }) => { // This assumption will no longer be valid if e.g. mutable variables are represented as // block parameters. If that becomes the case we'll need to figure out which variable // is generally constant and increasing to guess which parameter is the induction @@ -221,10 +216,10 @@ fn get_induction_variable( if function.dfg.get_numeric_constant(value).is_some() { Ok(value) } else { - Err(*location) + Err(location.clone()) } } - _ => Err(None), + _ => Err(CallStack::new()), } } @@ -236,7 +231,7 @@ fn unroll_loop_header<'a>( loop_: &'a Loop, unroll_into: BasicBlockId, induction_value: ValueId, -) -> Result>, Option> { +) -> Result>, CallStack> { // We insert into a fresh block first and move instructions into the unroll_into block later // only once we verify the jmpif instruction has a constant condition. If it does not, we can // just discard this fresh block and leave the loop unmodified. @@ -269,7 +264,7 @@ fn unroll_loop_header<'a>( } else { // If this case is reached the loop either uses non-constant indices or we need // another pass, such as mem2reg to resolve them to constants. - Err(context.inserter.function.dfg.get_value_location(&condition)) + Err(context.inserter.function.dfg.get_value_call_stack(condition)) } } other => unreachable!("Expected loop header to terminate in a JmpIf to the loop body, but found {other:?} instead"), @@ -361,7 +356,7 @@ impl<'f> LoopIteration<'f> { TerminatorInstruction::JmpIf { condition, then_destination, else_destination } => { self.handle_jmpif(*condition, *then_destination, *else_destination) } - TerminatorInstruction::Jmp { destination, arguments, location: _ } => { + TerminatorInstruction::Jmp { destination, arguments, call_stack: _ } => { if self.get_original_block(*destination) == self.loop_.header { assert_eq!(arguments.len(), 1); self.induction_value = Some((self.insert_block, arguments[0])); @@ -391,7 +386,11 @@ impl<'f> LoopIteration<'f> { self.source_block = self.get_original_block(destination); let arguments = Vec::new(); - let jmp = TerminatorInstruction::Jmp { destination, arguments, location: None }; + let jmp = TerminatorInstruction::Jmp { + destination, + arguments, + call_stack: CallStack::new(), + }; self.inserter.function.dfg.set_block_terminator(self.insert_block, jmp); vec![destination] } diff --git a/crates/noirc_evaluator/src/ssa/ssa_builder/mod.rs b/crates/noirc_evaluator/src/ssa/ssa_builder/mod.rs index 61daa06e37b..0958b0d7a67 100644 --- a/crates/noirc_evaluator/src/ssa/ssa_builder/mod.rs +++ b/crates/noirc_evaluator/src/ssa/ssa_builder/mod.rs @@ -14,7 +14,7 @@ use crate::ssa::ir::{ use super::{ ir::{ basic_block::BasicBlock, - dfg::InsertInstructionResult, + dfg::{CallStack, InsertInstructionResult}, function::RuntimeType, instruction::{InstructionId, Intrinsic}, }, @@ -32,7 +32,7 @@ pub(crate) struct FunctionBuilder { pub(super) current_function: Function, current_block: BasicBlockId, finished_functions: Vec, - current_location: Option, + call_stack: CallStack, } impl FunctionBuilder { @@ -53,7 +53,7 @@ impl FunctionBuilder { current_function: new_function, current_block, finished_functions: Vec::new(), - current_location: None, + call_stack: CallStack::new(), } } @@ -150,7 +150,7 @@ impl FunctionBuilder { instruction, self.current_block, ctrl_typevars, - self.current_location, + self.call_stack.clone(), ) } @@ -174,7 +174,12 @@ impl FunctionBuilder { } pub(crate) fn set_location(&mut self, location: Location) -> &mut FunctionBuilder { - self.current_location = Some(location); + self.call_stack = im::Vector::unit(location); + self + } + + pub(crate) fn set_call_stack(&mut self, call_stack: CallStack) -> &mut FunctionBuilder { + self.call_stack = call_stack; self } @@ -281,19 +286,12 @@ impl FunctionBuilder { destination: BasicBlockId, arguments: Vec, ) { - self.terminate_with_jmp_with_location(destination, arguments, None); - } - - /// Terminate the current block with a jmp instruction to jmp to the given - /// block with the given arguments. This version also remembers the Location of the jmp - /// for issuing error messages. - pub(crate) fn terminate_with_jmp_with_location( - &mut self, - destination: BasicBlockId, - arguments: Vec, - location: Option, - ) { - self.terminate_block_with(TerminatorInstruction::Jmp { destination, arguments, location }); + let call_stack = self.call_stack.clone(); + self.terminate_block_with(TerminatorInstruction::Jmp { + destination, + arguments, + call_stack, + }); } /// Terminate the current block with a jmpif instruction to jmp with the given arguments diff --git a/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 4cf9e83e21e..790c99fee05 100644 --- a/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -314,8 +314,8 @@ impl<'a> FunctionContext<'a> { // Set the location of the initial jmp instruction to the start range. This is the location // used to issue an error if the start range cannot be determined at compile-time. - let jmp_location = Some(for_expr.start_range_location); - self.builder.terminate_with_jmp_with_location(loop_entry, vec![start_index], jmp_location); + self.builder.set_location(for_expr.start_range_location); + self.builder.terminate_with_jmp(loop_entry, vec![start_index]); // Compile the loop entry block self.builder.switch_to_block(loop_entry); diff --git a/crates/noirc_frontend/src/graph/mod.rs b/crates/noirc_frontend/src/graph/mod.rs index 7a1daef45fd..90490c47c36 100644 --- a/crates/noirc_frontend/src/graph/mod.rs +++ b/crates/noirc_frontend/src/graph/mod.rs @@ -30,7 +30,7 @@ impl CrateId { pub struct CrateName(SmolStr); impl CrateName { - fn is_valid_name(name: &str) -> bool { + fn is_valid_name(name: &str) -> bool { !name.is_empty() && name.chars().all(|n| !CHARACTER_BLACK_LIST.contains(&n)) } }