fix!: Do not duplicate redundant Brillig debug metadata

acvm-repo/acvm/src/pwg/brillig.rs

@@ -29,6 +29,10 @@ pub enum BrilligSolverStatus<F> {
 pub struct BrilligSolver<'b, F, B: BlackBoxFunctionSolver<F>> {
     vm: VM<'b, F, B>,
     acir_index: usize,
+    /// This id references which Brillig function within the main ACIR program we are solving.
+    /// This is used for appropriately resolving errors as the ACIR program artifacts
+    /// set up their Brillig debug metadata by function id.
+    function_id: u32,
 }
 
 impl<'b, B: BlackBoxFunctionSolver<F>, F: AcirField> BrilligSolver<'b, F, B> {
@@ -61,10 +65,11 @@ impl<'b, B: BlackBoxFunctionSolver<F>, F: AcirField> BrilligSolver<'b, F, B> {
         brillig_bytecode: &'b [BrilligOpcode<F>],
         bb_solver: &'b B,
         acir_index: usize,
+        brillig_function_id: u32,
     ) -> Result<Self, OpcodeResolutionError<F>> {
         let vm =
             Self::setup_brillig_vm(initial_witness, memory, inputs, brillig_bytecode, bb_solver)?;
-        Ok(Self { vm, acir_index })
+        Ok(Self { vm, acir_index, function_id: brillig_function_id })
     }
 
     fn setup_brillig_vm(
@@ -182,7 +187,11 @@ impl<'b, B: BlackBoxFunctionSolver<F>, F: AcirField> BrilligSolver<'b, F, B> {
                     }
                 };
 
-                Err(OpcodeResolutionError::BrilligFunctionFailed { payload, call_stack })
+                Err(OpcodeResolutionError::BrilligFunctionFailed {
+                    function_id: self.function_id,
+                    payload,
+                    call_stack,
+                })
             }
             VMStatus::ForeignCallWait { function, inputs } => {
                 Ok(BrilligSolverStatus::ForeignCallWait(ForeignCallWaitInfo { function, inputs }))

acvm-repo/acvm/src/pwg/mod.rs

@@ -56,7 +56,7 @@
     RequiresForeignCall(ForeignCallWaitInfo<F>),
 
     /// The ACVM has encountered a request for an ACIR [call][acir::circuit::Opcode]
     /// to execute a separate ACVM instance. The result of the ACIR call must be passd back to the ACVM.
     ///
     /// Once this is done, the ACVM can be restarted to solve the remaining opcodes.
     RequiresAcirCall(AcirCallWaitInfo<F>),
@@ -132,6 +132,7 @@
     BlackBoxFunctionFailed(BlackBoxFunc, String),
     #[error("Failed to solve brillig function")]
     BrilligFunctionFailed {
+        function_id: u32,
         call_stack: Vec<OpcodeLocation>,
         payload: Option<ResolvedAssertionPayload<F>>,
     },
@@ -478,6 +479,7 @@
                 &self.unconstrained_functions[*id as usize].bytecode,
                 self.backend,
                 self.instruction_pointer,
+                *id,
             )?,
         };
 
@@ -502,7 +504,7 @@
 
     fn map_brillig_error(&self, mut err: OpcodeResolutionError<F>) -> OpcodeResolutionError<F> {
         match &mut err {
-            OpcodeResolutionError::BrilligFunctionFailed { call_stack, payload } => {
+            OpcodeResolutionError::BrilligFunctionFailed { call_stack, payload, .. } => {
                 // Some brillig errors have static strings as payloads, we can resolve them here
                 let last_location =
                     call_stack.last().expect("Call stacks should have at least one item");
@@ -549,6 +551,7 @@
             &self.unconstrained_functions[*id as usize].bytecode,
             self.backend,
             self.instruction_pointer,
+            *id,
         );
         match solver {
             Ok(solver) => StepResult::IntoBrillig(solver),

acvm-repo/acvm/tests/solver.rs

@@ -675,6 +675,7 @@ fn unsatisfied_opcode_resolved_brillig() {
     assert_eq!(
         solver_status,
         ACVMStatus::Failure(OpcodeResolutionError::BrilligFunctionFailed {
+            function_id: 0,
             payload: None,
             call_stack: vec![OpcodeLocation::Brillig { acir_index: 0, brillig_index: 3 }]
         }),

compiler/noirc_errors/src/debug_info.rs

@@ -97,6 +97,8 @@ pub struct DebugInfo {
     /// that they should be serialized to/from strings.
     #[serde_as(as = "BTreeMap<DisplayFromStr, _>")]
     pub locations: BTreeMap<OpcodeLocation, Vec<Location>>,
+    #[serde_as(as = "BTreeMap<_, BTreeMap<DisplayFromStr, _>>")]
+    pub brillig_locations: BTreeMap<u32, BTreeMap<OpcodeLocation, Vec<Location>>>,
     pub variables: DebugVariables,
     pub functions: DebugFunctions,
     pub types: DebugTypes,
@@ -113,11 +115,12 @@ pub struct OpCodesCount {
 impl DebugInfo {
     pub fn new(
         locations: BTreeMap<OpcodeLocation, Vec<Location>>,
+        brillig_locations: BTreeMap<u32, BTreeMap<OpcodeLocation, Vec<Location>>>,
         variables: DebugVariables,
         functions: DebugFunctions,
         types: DebugTypes,
     ) -> Self {
-        Self { locations, variables, functions, types }
+        Self { locations, brillig_locations, variables, functions, types }
     }
 
     /// Updates the locations map when the [`Circuit`][acvm::acir::circuit::Circuit] is modified.

compiler/noirc_evaluator/src/ssa.rs

@@ -117,7 +117,7 @@
     .run_pass(Ssa::array_set_optimization, "After Array Set Optimizations:")
     .finish();
 
     let ssa_level_warnings = ssa.check_for_underconstrained_values();
     let brillig = time("SSA to Brillig", options.print_codegen_timings, || {
         ssa.to_brillig(options.enable_brillig_logging)
     });
@@ -262,6 +262,7 @@
     let GeneratedAcir {
         return_witnesses,
         locations,
+        brillig_locations,
         input_witnesses,
         assertion_payloads: assert_messages,
         warnings,
@@ -292,7 +293,19 @@
         .map(|(index, locations)| (index, locations.into_iter().collect()))
         .collect();
 
-    let mut debug_info = DebugInfo::new(locations, debug_variables, debug_functions, debug_types);
+    let brillig_locations = brillig_locations
+        .into_iter()
+        .map(|(function_index, locations)| {
+            let locations = locations
+                .into_iter()
+                .map(|(index, locations)| (index, locations.into_iter().collect()))
+                .collect();
+            (function_index, locations)
+        })
+        .collect();
+
+    let mut debug_info =
+        DebugInfo::new(locations, brillig_locations, debug_variables, debug_functions, debug_types);
 
     // Perform any ACIR-level optimizations
     let (optimized_circuit, transformation_map) = acvm::compiler::optimize(circuit);

compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs

@@ -49,8 +49,11 @@ pub(crate) struct GeneratedAcir<F: AcirField> {
     /// All witness indices which are inputs to the main function
     pub(crate) input_witnesses: Vec<Witness>,
 
-    /// Correspondence between an opcode index (in opcodes) and the source code call stack which generated it
-    pub(crate) locations: BTreeMap<OpcodeLocation, CallStack>,
+    pub(crate) locations: OpcodeToLocationsMap,
+
+    /// Brillig function id -> Opcodes locations map
+    /// This map is used to prevent redundant locations being stored for the same Brillig entry point.
+    pub(crate) brillig_locations: BTreeMap<u32, OpcodeToLocationsMap>,
 
     /// Source code location of the current instruction being processed
     /// None if we do not know the location
@@ -71,6 +74,9 @@ pub(crate) struct GeneratedAcir<F: AcirField> {
     pub(crate) brillig_stdlib_func_locations: BTreeMap<OpcodeLocation, BrilligStdlibFunc>,
 }
 
+/// Correspondence between an opcode index (in opcodes) and the source code call stack which generated it
+pub(crate) type OpcodeToLocationsMap = BTreeMap<OpcodeLocation, CallStack>;
+
 #[derive(Debug, Clone, Copy, Hash, PartialEq, Eq)]
 pub(crate) enum BrilligStdlibFunc {
     Inverse,
@@ -567,30 +573,44 @@ impl<F: AcirField> GeneratedAcir<F> {
         brillig_function_index: u32,
         stdlib_func: Option<BrilligStdlibFunc>,
     ) {
+        // Check whether we have a call to this Brillig function already exists.
+        // This helps us optimize the Brillig metadata to only be stored once per Brillig entry point.
+        let inserted_func_before = self.brillig_locations.get(&brillig_function_index).is_some();
+
         let opcode =
             AcirOpcode::BrilligCall { id: brillig_function_index, inputs, outputs, predicate };
         self.push_opcode(opcode);
+
         if let Some(stdlib_func) = stdlib_func {
             self.brillig_stdlib_func_locations
                 .insert(self.last_acir_opcode_location(), stdlib_func);
+            // The Brillig stdlib functions are handwritten and do not have any locations or assert messages.
+            // To avoid inserting the `PLACEHOLDER_BRILLIG_INDEX` into `self.brillig_locations` before the first
+            // user-specified Brillig call we can simply return after the Brillig stdlib function call.
+            return;
         }
 
-        for (brillig_index, call_stack) in generated_brillig.locations.iter() {
-            self.locations.insert(
+        for (brillig_index, message) in generated_brillig.assert_messages.iter() {
+            self.assertion_payloads.insert(
                 OpcodeLocation::Brillig {
                     acir_index: self.opcodes.len() - 1,
                     brillig_index: *brillig_index,
                 },
-                call_stack.clone(),
+                AssertionPayload::StaticString(message.clone()),
             );
         }
-        for (brillig_index, message) in generated_brillig.assert_messages.iter() {
-            self.assertion_payloads.insert(
+
+        if inserted_func_before {
+            return;
+        }
+
+        for (brillig_index, call_stack) in generated_brillig.locations.iter() {
+            self.brillig_locations.entry(brillig_function_index).or_default().insert(
                 OpcodeLocation::Brillig {
                     acir_index: self.opcodes.len() - 1,
                     brillig_index: *brillig_index,
                 },
-                AssertionPayload::StaticString(message.clone()),
+                call_stack.clone(),
             );
         }
     }

compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs

@@ -2837,16 +2837,17 @@ fn can_omit_element_sizes_array(array_typ: &Type) -> bool {
 
 #[cfg(test)]
 mod test {
-    use std::collections::BTreeMap;
-
     use acvm::{
         acir::{
             circuit::{ExpressionWidth, Opcode, OpcodeLocation},
             native_types::Witness,
         },
         FieldElement,
     };
+    use im::vector;
+    use noirc_errors::Location;
     use noirc_frontend::monomorphization::ast::InlineType;
+    use std::collections::BTreeMap;
 
     use crate::{
         brillig::Brillig,
@@ -2874,6 +2875,9 @@ mod test {
         } else {
             builder.new_brillig_function("foo".into(), foo_id);
         }
+        // Set a call stack for testing whether `brillig_locations` in the `GeneratedAcir` was accurately set.
+        builder.set_call_stack(vector![Location::dummy(), Location::dummy()]);
+
         let foo_v0 = builder.add_parameter(Type::field());
         let foo_v1 = builder.add_parameter(Type::field());
 
@@ -3257,6 +3261,12 @@ mod test {
                 _ => panic!("Expected only Brillig call opcode"),
             }
         }
+
+        // We have two normal Brillig functions that was called multiple times.
+        // We should have a single locations map for each function's debug metadata.
+        assert_eq!(main_acir.brillig_locations.len(), 2);
+        assert!(main_acir.brillig_locations.get(&0).is_some());
+        assert!(main_acir.brillig_locations.get(&1).is_some());
     }
 
     // Test that given multiple primitive operations that are represented by Brillig directives (e.g. invert/quotient),
@@ -3312,6 +3322,8 @@ mod test {
             4,
             0,
         );
+
+        assert_eq!(main_acir.brillig_locations.len(), 0);
     }
 
     // Test that given both hardcoded Brillig directives and calls to normal Brillig functions,
@@ -3382,13 +3394,12 @@ mod test {
 
         let main_acir = &acir_functions[0];
         let main_opcodes = main_acir.opcodes();
-        check_brillig_calls(
-            &acir_functions[0].brillig_stdlib_func_locations,
-            main_opcodes,
-            1,
-            4,
-            2,
-        );
+        check_brillig_calls(&main_acir.brillig_stdlib_func_locations, main_opcodes, 1, 4, 2);
+
+        // We have one normal Brillig functions that was called twice.
+        // We should have a single locations map for each function's debug metadata.
+        assert_eq!(main_acir.brillig_locations.len(), 1);
+        assert!(main_acir.brillig_locations.get(&0).is_some());
     }
 
     // Test that given both normal Brillig calls, Brillig stdlib calls, and non-inlined ACIR calls, that we accurately generate ACIR.
@@ -3479,9 +3490,14 @@ mod test {
             2,
         );
 
+        assert_eq!(main_acir.brillig_locations.len(), 1);
+        assert!(main_acir.brillig_locations.get(&0).is_some());
+
         let foo_acir = &acir_functions[1];
         let foo_opcodes = foo_acir.opcodes();
         check_brillig_calls(&acir_functions[1].brillig_stdlib_func_locations, foo_opcodes, 1, 1, 0);
+
+        assert_eq!(foo_acir.brillig_locations.len(), 0);
     }
 
     fn check_brillig_calls(

tooling/debugger/src/source_code_printer.rs

@@ -243,7 +243,7 @@
     fn render_multiple_line_location() {
         let source_code = r##"pub fn main(mut state: [Field; 2]) -> [Field; 2] {
     state = permute(
         consts::x5_2_config(),
         state);
 
     state
@@ -259,7 +259,7 @@
         // Location of
         // ```
         // permute(
         //      consts::x5_2_config(),
         //      state)
         // ```
         let loc = Location::new(Span::inclusive(63, 116), file_id);
@@ -274,6 +274,7 @@
             BTreeMap::default(),
             BTreeMap::default(),
             BTreeMap::default(),
+            BTreeMap::default(),
         )];
         let debug_artifact = DebugArtifact::new(debug_symbols, &fm);
 
@@ -297,7 +298,7 @@
                 Content {
                     line_number: 3,
                     cursor: "",
                     content: "        consts::x5_2_config(),",
                     highlight: Some(Range { start: 0, end: 30 }),
                 },
                 Content {

tooling/nargo/src/errors.rs

@@ -149,13 +149,34 @@ fn extract_locations_from_error<F: AcirField>(
         }
     }
 
+    let brillig_function_id = match error {
+        ExecutionError::SolvingError(
+            OpcodeResolutionError::BrilligFunctionFailed { function_id, .. },
+            _,
+        ) => Some(*function_id),
+        _ => None,
+    };
+
     Some(
         opcode_locations
             .iter()
             .flat_map(|resolved_location| {
                 debug[resolved_location.acir_function_index]
                     .opcode_location(&resolved_location.opcode_location)
-                    .unwrap_or_default()
+                    .unwrap_or_else(|| {
+                        if let Some(brillig_function_id) = brillig_function_id {
+                            let brillig_locations = debug[resolved_location.acir_function_index]
+                                .brillig_locations
+                                .get(&brillig_function_id);
+                            brillig_locations
+                                .unwrap()
+                                .get(&resolved_location.opcode_location)
+                                .cloned()
+                                .unwrap_or_default()
+                        } else {
+                            vec![]
+                        }
+                    })
             })
             .collect(),
     )

tooling/noirc_artifacts/src/debug.rs

@@ -208,7 +208,7 @@
     // For example, given the snippet:
     // ```
     // permute(
     //    consts::x5_2_config(),
     //    state);
     // ```
     // We want location_in_line to return the range
@@ -217,7 +217,7 @@
     fn location_in_line_stops_at_end_of_line() {
         let source_code = r##"pub fn main(mut state: [Field; 2]) -> [Field; 2] {
     state = permute(
         consts::x5_2_config(),
         state);
 
     state
@@ -233,7 +233,7 @@
         // Location of
         // ```
         // permute(
         //      consts::x5_2_config(),
         //      state)
         // ```
         let loc = Location::new(Span::inclusive(63, 116), file_id);
@@ -248,6 +248,7 @@
             BTreeMap::default(),
             BTreeMap::default(),
             BTreeMap::default(),
+            BTreeMap::default(),
         )];
         let debug_artifact = DebugArtifact::new(debug_symbols, &fm);
 

tooling/profiler/src/flamegraph.rs

@@ -269,6 +269,7 @@ mod tests {
             BTreeMap::default(),
             BTreeMap::default(),
             BTreeMap::default(),
+            BTreeMap::default(),
         );
 
         let samples_per_opcode = vec![10, 20, 30];