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: location tree for debug_info #7034

Open
wants to merge 55 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
55 commits
Select commit Hold shift + click to select a range
cf0eeb9
location tree for debug_info
guipublic Jan 13, 2025
17ff57d
forgot to commit files from tooling
guipublic Jan 13, 2025
eeb1e9f
cargo.lock
guipublic Jan 13, 2025
aa74a7b
Merge branch 'master' into gd/issue_6946
guipublic Jan 13, 2025
0984da7
fix test case
guipublic Jan 13, 2025
a3eb219
set call_stack_id during acir-gen
guipublic Jan 14, 2025
fb962e0
Merge branch 'master' into gd/issue_6946
guipublic Jan 14, 2025
3548030
update ts DebugInfo
guipublic Jan 14, 2025
70f5563
update comment
guipublic Jan 14, 2025
9c06c93
Merge branch 'master' into gd/issue_6946
guipublic Jan 14, 2025
ca52a8e
format
guipublic Jan 14, 2025
4283acf
Merge branch 'master' into gd/issue_6946
TomAFrench Jan 14, 2025
8683746
put call stack in correct order
guipublic Jan 14, 2025
354b075
Merge branch 'master' into gd/issue_6946
guipublic Jan 15, 2025
b3332d5
code review
guipublic Jan 15, 2025
22357be
Merge branch 'master' into gd/issue_6946
TomAFrench Jan 15, 2025
41a9c09
code review
guipublic Jan 16, 2025
60a4fdc
code review
guipublic Jan 16, 2025
6b38614
Merge branch 'master' into gd/issue_6946
guipublic Jan 16, 2025
bed3b77
Separate brillig/acir opcode locations in DebugInfo
guipublic Jan 17, 2025
0ce7133
Merge branch 'master' into gd/issue_6946
guipublic Jan 17, 2025
3db55aa
commit missing file
guipublic Jan 17, 2025
f57145d
Merge branch 'master' into gd/issue_6946
guipublic Jan 17, 2025
201d371
fix unit case
guipublic Jan 17, 2025
0220c80
Merge branch 'master' into gd/issue_6946
guipublic Jan 17, 2025
c8fae47
fix unit test
guipublic Jan 17, 2025
07d8d7f
Merge branch 'master' into gd/issue_6946
guipublic Jan 17, 2025
df0abf3
Fix unit test
guipublic Jan 17, 2025
5e96be1
Merge branch 'master' into gd/issue_6946
guipublic Jan 17, 2025
d2ae968
Merge branch 'master' into gd/issue_6946
guipublic Jan 20, 2025
6f23a8c
fix merge conflict
guipublic Jan 20, 2025
b273687
Merge branch 'master' into gd/issue_6946
guipublic Jan 20, 2025
91496e4
code review: remove string serialisation for AcirOpcodeLocation
guipublic Jan 24, 2025
27ed000
Merge branch 'master' into gd/issue_6946
guipublic Jan 24, 2025
37bfadb
fix merge issue
guipublic Jan 24, 2025
0f43935
Merge branch 'master' into gd/issue_6946
guipublic Jan 28, 2025
5262488
Merge branch 'master' into gd/issue_6946
guipublic Feb 4, 2025
b326ea8
Merge branch 'master' into gd/issue_6946
guipublic Feb 4, 2025
edb58b8
Merge branch 'master' into gd/issue_6946
guipublic Feb 5, 2025
5b0fedb
Merge branch 'master' into gd/issue_6946
guipublic Feb 6, 2025
0e037ec
Merge branch 'master' into gd/issue_6946
vezenovm Feb 6, 2025
5e94272
Merge branch 'master' into gd/issue_6946
guipublic Feb 6, 2025
5a74021
Merge branch 'master' into gd/issue_6946
guipublic Feb 6, 2025
54b6de7
Merge branch 'master' into gd/issue_6946
guipublic Feb 7, 2025
f4c4ec4
Merge branch 'master' into gd/issue_6946
guipublic Feb 7, 2025
7be438e
Update acvm-repo/acir/src/circuit/mod.rs
vezenovm Feb 12, 2025
a549811
Update compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs
vezenovm Feb 12, 2025
06a233f
Update compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs
vezenovm Feb 12, 2025
7cbfca7
Merge branch 'master' into gd/issue_6946
vezenovm Feb 12, 2025
9f98cba
code review
guipublic Feb 7, 2025
f9a3205
Merge branch 'master' into gd/issue_6946
guipublic Feb 25, 2025
6c330a6
code review
guipublic Feb 25, 2025
389f701
code review
guipublic Feb 25, 2025
302d7d2
Merge branch 'master' into gd/issue_6946
guipublic Feb 25, 2025
865da61
Merge branch 'master' into gd/issue_6946
guipublic Feb 26, 2025
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
1 change: 1 addition & 0 deletions Cargo.lock

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

23 changes: 16 additions & 7 deletions compiler/noirc_driver/src/debug.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,14 @@ pub(crate) fn filter_relevant_files(
let mut files_with_debug_symbols: BTreeSet<FileId> = debug_symbols
.iter()
.flat_map(|function_symbols| {
function_symbols
.locations
.values()
.flat_map(|call_stack| call_stack.iter().map(|location| location.file))
function_symbols.location_map.values().flat_map(|call_stack_id| {
function_symbols
.location_tree
.get_call_stack(*call_stack_id)
.iter()
.map(|location| location.file)
.collect::<Vec<_>>()
})
})
.collect();

Expand All @@ -33,9 +37,14 @@ pub(crate) fn filter_relevant_files(
.flat_map(|function_symbols| {
let brillig_location_maps =
function_symbols.brillig_locations.values().flat_map(|brillig_location_map| {
brillig_location_map
.values()
.flat_map(|call_stack| call_stack.iter().map(|location| location.file))
brillig_location_map.values().flat_map(|call_stack_id| {
function_symbols
.location_tree
.get_call_stack(*call_stack_id)
.iter()
.map(|location| location.file)
.collect::<Vec<_>>()
})
});
brillig_location_maps
})
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 @@ -16,6 +16,7 @@ acvm.workspace = true
codespan-reporting.workspace = true
codespan.workspace = true
fm.workspace = true
fxhash.workspace = true
noirc_printable_type.workspace = true
serde.workspace = true
serde_with = "3.2.0"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,48 +1,71 @@
use fxhash::FxHashMap;
use serde::{Deserialize, Serialize};

use noirc_errors::Location;

pub(crate) type CallStack = Vec<Location>;
use crate::Location;

pub type CallStack = Vec<Location>;
#[derive(Debug, Default, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)]
pub(crate) struct CallStackId(u32);
pub struct CallStackId(u32);

impl CallStackId {
pub(crate) fn root() -> Self {
pub fn root() -> Self {
Self::new(0)
}

fn new(id: usize) -> Self {
pub fn new(id: usize) -> Self {
Self(id as u32)
}

pub(crate) fn index(&self) -> usize {
pub fn index(&self) -> usize {
self.0 as usize
}

pub(crate) fn is_root(&self) -> bool {
pub fn is_root(&self) -> bool {
self.0 == 0
}
}

#[derive(Debug, Clone, Serialize, Deserialize, Hash)]
pub struct LocationNodeDebugInfo {
pub parent: Option<CallStackId>,
pub value: Location,
}

#[derive(Debug, Clone, Serialize, Deserialize, Default, Hash)]
pub struct LocationTree {
pub locations: Vec<LocationNodeDebugInfo>,
}

impl LocationTree {
/// Construct a CallStack from a CallStackId
pub fn get_call_stack(&self, mut call_stack: CallStackId) -> CallStack {
let mut result = Vec::new();
while let Some(parent) = self.locations[call_stack.index()].parent {
result.push(self.locations[call_stack.index()].value);
call_stack = parent;
}
result.reverse();
result
}
}

#[derive(Debug, Clone, Serialize, Deserialize)]
pub(crate) struct LocationNode {
pub(crate) parent: Option<CallStackId>,
pub(crate) children: Vec<CallStackId>,
pub(crate) children_hash: FxHashMap<u64, CallStackId>,
pub(crate) value: Location,
pub struct LocationNode {
pub parent: Option<CallStackId>,
pub children: Vec<CallStackId>,
pub children_hash: FxHashMap<u64, CallStackId>,
pub value: Location,
}

impl LocationNode {
pub(crate) fn new(parent: Option<CallStackId>, value: Location) -> Self {
pub fn new(parent: Option<CallStackId>, value: Location) -> Self {
LocationNode { parent, children: Vec::new(), children_hash: FxHashMap::default(), value }
}
}

#[derive(Debug, Clone, Serialize, Deserialize)]
pub(crate) struct CallStackHelper {
locations: Vec<LocationNode>,
pub struct CallStackHelper {
pub locations: Vec<LocationNode>,
}

impl Default for CallStackHelper {
Expand All @@ -56,17 +79,18 @@ impl Default for CallStackHelper {

impl CallStackHelper {
/// Construct a CallStack from a CallStackId
pub(crate) fn get_call_stack(&self, mut call_stack: CallStackId) -> CallStack {
pub fn get_call_stack(&self, mut call_stack: CallStackId) -> CallStack {
let mut result = Vec::new();
while let Some(parent) = self.locations[call_stack.index()].parent {
result.push(self.locations[call_stack.index()].value);
call_stack = parent;
}
result.reverse();
result
}

/// Returns a new CallStackId which extends the call_stack with the provided call_stack.
pub(crate) fn extend_call_stack(
pub fn extend_call_stack(
&mut self,
mut call_stack: CallStackId,
locations: &CallStack,
Expand All @@ -78,7 +102,7 @@ impl CallStackHelper {
}

/// Adds a location to the call stack
pub(crate) fn add_child(&mut self, call_stack: CallStackId, location: Location) -> CallStackId {
pub fn add_child(&mut self, call_stack: CallStackId, location: Location) -> CallStackId {
let key = fxhash::hash64(&location);
if let Some(result) = self.locations[call_stack.index()].children_hash.get(&key) {
if self.locations[result.index()].value == location {
Expand All @@ -96,11 +120,7 @@ impl CallStackHelper {
}

/// Retrieve the CallStackId corresponding to call_stack with the last 'len' locations removed.
pub(crate) fn unwind_call_stack(
&self,
mut call_stack: CallStackId,
mut len: usize,
) -> CallStackId {
pub fn unwind_call_stack(&self, mut call_stack: CallStackId, mut len: usize) -> CallStackId {
while len > 0 {
if let Some(parent) = self.locations[call_stack.index()].parent {
len -= 1;
Expand All @@ -112,7 +132,7 @@ impl CallStackHelper {
call_stack
}

pub(crate) fn add_location_to_root(&mut self, location: Location) -> CallStackId {
pub fn add_location_to_root(&mut self, location: Location) -> CallStackId {
if self.locations.is_empty() {
self.locations.push(LocationNode::new(None, location));
CallStackId::root()
Expand All @@ -122,7 +142,19 @@ impl CallStackHelper {
}

/// Get (or create) a CallStackId corresponding to the given locations
pub(crate) fn get_or_insert_locations(&mut self, locations: CallStack) -> CallStackId {
self.extend_call_stack(CallStackId::root(), &locations)
pub fn get_or_insert_locations(&mut self, locations: &CallStack) -> CallStackId {
self.extend_call_stack(CallStackId::root(), locations)
}

// Clone the locations into a LocationTree
pub fn to_location_tree(&self) -> LocationTree {
LocationTree {
locations: self
.locations
.clone()
.into_iter()
.map(|node| LocationNodeDebugInfo { value: node.value, parent: node.parent })
.collect(),
}
}
}
32 changes: 23 additions & 9 deletions compiler/noirc_errors/src/debug_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ use std::io::Read;
use std::io::Write;
use std::mem;

use crate::call_stack::CallStackId;
use crate::call_stack::LocationTree;
use crate::Location;
use noirc_printable_type::PrintableType;
use serde::{
Expand Down Expand Up @@ -96,13 +98,14 @@ impl ProgramDebugInfo {
#[serde_as]
#[derive(Default, Debug, Clone, Deserialize, Serialize, Hash)]
pub struct DebugInfo {
pub brillig_locations:
BTreeMap<BrilligFunctionId, BTreeMap<BrilligOpcodeLocation, CallStackId>>,
pub location_tree: LocationTree,
/// Map opcode index of an ACIR circuit into the source code location
/// Serde does not support mapping keys being enums for json, so we indicate
/// that they should be serialized to/from strings.
#[serde_as(as = "BTreeMap<DisplayFromStr, _>")]
pub locations: BTreeMap<OpcodeLocation, Vec<Location>>,
pub brillig_locations:
BTreeMap<BrilligFunctionId, BTreeMap<BrilligOpcodeLocation, Vec<Location>>>,
pub location_map: BTreeMap<OpcodeLocation, CallStackId>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give an explainer on how exactly these fit together? I'm thinking that we're storing unnecessary data in location_map now.

My understanding is that:

  • brillig_locations stores for each brillig function a mapping from each opcode index to a callstack id
  • We can then use this and then look up the callstack id in location_tree to get the associate callstack for any brillig opcode.
  • location_map stores a mapping from (acir_opcode_index, brillig_index) to a callstack id which we can use with location_tree.

Why do we need to track (acir_opcode_index, brillig_index) in location_map now? If we fail inside of a brillig call we can tell which brillig function we're executing from the ACIR opcode we've halted on, we then

  1. Get the callstack for the ACIR opcode in which we make the call
  2. Check the brillig opcode within that call we halted on and get the relevant callstack for the unconstrained function.
  3. Smoosh these two together to generate the final callstack.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory this makes sense, in practice I don't see where those brillig locations are inserted in the location_map.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, true we seem to have removed the insertion of any OpcodeLocation::Brillig a while back.

It still stands that we don't need to keep this enum in the build artifact anymore though as it's currently tech debt. We've kept this enum around just to keep the program serialization format the same but as we're changing it now anyway, now's a good time to clear up this tech debt.

#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize, Deserialize)]
/// Opcodes are locatable so that callers can
/// map opcodes to debug information related to their context.
pub enum OpcodeLocation {
Acir(usize),
// TODO(https://github.com/noir-lang/noir/issues/5792): We can not get rid of this enum field entirely just yet as this format is still
// used for resolving assert messages which is a breaking serialization change.
Brillig { acir_index: usize, brillig_index: usize },
}

#5792

Copy link
Contributor Author

@guipublic guipublic Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.
What I did is create a new AcirOpcodeLocation type and use it inside DebugInfo LocationMap, instead of OpcodeLocation, converting between the 2 when necessary.
I did not touch the other usage of OpcodeLocation, which is used also with BrilligFlavor if a crash occur during ACVM execution.

pub variables: DebugVariables,
pub functions: DebugFunctions,
pub types: DebugTypes,
Expand All @@ -113,11 +116,12 @@ pub struct DebugInfo {

impl DebugInfo {
pub fn new(
locations: BTreeMap<OpcodeLocation, Vec<Location>>,
brillig_locations: BTreeMap<
BrilligFunctionId,
BTreeMap<BrilligOpcodeLocation, Vec<Location>>,
BTreeMap<BrilligOpcodeLocation, CallStackId>,
>,
location_map: BTreeMap<OpcodeLocation, CallStackId>,
location_tree: LocationTree,
variables: DebugVariables,
functions: DebugFunctions,
types: DebugTypes,
Expand All @@ -126,7 +130,15 @@ impl DebugInfo {
BTreeMap<ProcedureDebugId, (usize, usize)>,
>,
) -> Self {
Self { locations, brillig_locations, variables, functions, types, brillig_procedure_locs }
Self {
brillig_locations,
location_map,
location_tree,
variables,
functions,
types,
brillig_procedure_locs,
}
}

/// Updates the locations map when the [`Circuit`][acvm::acir::circuit::Circuit] is modified.
Expand All @@ -136,16 +148,18 @@ impl DebugInfo {
/// Note: One old `OpcodeLocation` might have transformed into more than one new `OpcodeLocation`.
#[tracing::instrument(level = "trace", skip(self, update_map))]
pub fn update_acir(&mut self, update_map: AcirTransformationMap) {
let old_locations = mem::take(&mut self.locations);
let old_locations = mem::take(&mut self.location_map);

for (old_opcode_location, source_locations) in old_locations {
update_map.new_locations(old_opcode_location).for_each(|new_opcode_location| {
self.locations.insert(new_opcode_location, source_locations.clone());
self.location_map.insert(new_opcode_location, source_locations);
});
}
}

pub fn opcode_location(&self, loc: &OpcodeLocation) -> Option<Vec<Location>> {
self.locations.get(loc).cloned()
self.location_map
.get(loc)
.map(|call_stack_id| self.location_tree.get_call_stack(*call_stack_id))
}
}
1 change: 1 addition & 0 deletions compiler/noirc_errors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#![warn(unreachable_pub)]
#![warn(clippy::semicolon_if_nothing_returned)]

pub mod call_stack;
pub mod debug_info;
mod position;
pub mod reporter;
Expand Down
12 changes: 8 additions & 4 deletions compiler/noirc_evaluator/src/acir/acir_variable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,14 @@ use acvm::{
};
use fxhash::FxHashMap as HashMap;
use iter_extended::{try_vecmap, vecmap};
use noirc_errors::call_stack::{CallStack, CallStackHelper};
use num_bigint::BigUint;
use std::cmp::Ordering;
use std::{borrow::Cow, hash::Hash};

use crate::brillig::brillig_ir::artifact::GeneratedBrillig;
use crate::errors::{InternalBug, InternalError, RuntimeError, SsaReport};
use crate::ssa::ir::{
call_stack::CallStack, instruction::Endian, types::NumericType, types::Type as SsaType,
};
use crate::ssa::ir::{instruction::Endian, types::NumericType, types::Type as SsaType};

use super::big_int::BigIntContext;
use super::generated_acir::{BrilligStdlibFunc, GeneratedAcir, PLACEHOLDER_BRILLIG_INDEX};
Expand Down Expand Up @@ -240,13 +239,18 @@ impl<F: AcirField, B: BlackBoxFunctionSolver<F>> AcirContext<F, B> {
}

pub(crate) fn get_call_stack(&self) -> CallStack {
self.acir_ir.call_stack.clone()
self.acir_ir.call_stacks.get_call_stack(self.acir_ir.call_stack_id)
}

pub(crate) fn set_call_stack(&mut self, call_stack: CallStack) {
self.acir_ir.call_stack_id = self.acir_ir.call_stacks.get_or_insert_locations(&call_stack);
self.acir_ir.call_stack = call_stack;
}

pub(crate) fn set_call_stack_helper(&mut self, call_stack: CallStackHelper) {
self.acir_ir.call_stacks = call_stack;
}

pub(crate) fn get_or_create_witness_var(
&mut self,
var: AcirVar,
Expand Down
Loading
Loading