fix!: Do not duplicate redundant Brillig debug metadata

acvm-repo/acir/src/circuit/brillig.rs

@@ -27,3 +27,6 @@ pub enum BrilligOutputs {
 pub struct BrilligBytecode<F> {
     pub bytecode: Vec<BrilligOpcode<F>>,
 }
+
+/// Id for the function being called.
+pub type BrilligFunctionId = u32;

acvm-repo/acir/src/circuit/opcodes.rs

@@ -1,5 +1,5 @@
 use super::{
-    brillig::{BrilligInputs, BrilligOutputs},
+    brillig::{BrilligFunctionId, BrilligInputs, BrilligOutputs},
     directives::Directive,
 };
 use crate::native_types::{Expression, Witness};
@@ -111,7 +111,7 @@
     BrilligCall {
         /// Id for the function being called. It is the responsibility of the executor
         /// to fetch the appropriate Brillig bytecode from this id.
-        id: u32,
+        id: BrilligFunctionId,
         /// Inputs to the function call
         inputs: Vec<BrilligInputs<F>>,
         /// Outputs to the function call
@@ -153,7 +153,7 @@
 
             Opcode::BlackBoxFuncCall(g) => write!(f, "{g}"),
             Opcode::Directive(Directive::ToLeRadix { a, b, radix: _ }) => {
                 write!(f, "DIR::TORADIX ")?;
                 write!(
                     f,
                     // TODO (Note): this assumes that the decomposed bits have contiguous witness indices
@@ -184,7 +184,7 @@
                 match databus {
                     BlockType::Memory => write!(f, "INIT ")?,
                     BlockType::CallData => write!(f, "INIT CALLDATA ")?,
                     BlockType::ReturnData => write!(f, "INIT RETURNDATA ")?,
                 }
                 write!(f, "(id: {}, len: {}) ", block_id.0, init.len())
             }

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

@@ -3,7 +3,7 @@ use std::collections::HashMap;
 use acir::{
     brillig::{ForeignCallParam, ForeignCallResult, Opcode as BrilligOpcode},
     circuit::{
-        brillig::{BrilligInputs, BrilligOutputs},
+        brillig::{BrilligFunctionId, BrilligInputs, BrilligOutputs},
         opcodes::BlockId,
         ErrorSelector, OpcodeLocation, RawAssertionPayload, ResolvedAssertionPayload,
         STRING_ERROR_SELECTOR,
@@ -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: BrilligFunctionId,
 }
 
 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: BrilligFunctionId,
     ) -> 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

@@ -5,7 +5,7 @@
 use acir::{
     brillig::ForeignCallResult,
     circuit::{
-        brillig::BrilligBytecode,
+        brillig::{BrilligBytecode, BrilligFunctionId},
         opcodes::{BlockId, ConstantOrWitnessEnum, FunctionInput},
         AssertionPayload, ErrorSelector, ExpressionOrMemory, Opcode, OpcodeLocation,
         RawAssertionPayload, ResolvedAssertionPayload, STRING_ERROR_SELECTOR,
@@ -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: BrilligFunctionId,
         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

@@ -1,3 +1,4 @@
+use acvm::acir::circuit::brillig::BrilligFunctionId;
 use acvm::acir::circuit::OpcodeLocation;
 use acvm::compiler::AcirTransformationMap;
 
@@ -97,6 +98,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<BrilligFunctionId, BTreeMap<OpcodeLocation, Vec<Location>>>,
     pub variables: DebugVariables,
     pub functions: DebugFunctions,
     pub types: DebugTypes,
@@ -113,11 +116,12 @@ pub struct OpCodesCount {
 impl DebugInfo {
     pub fn new(
         locations: BTreeMap<OpcodeLocation, Vec<Location>>,
+        brillig_locations: BTreeMap<BrilligFunctionId, 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

@@ -262,6 +262,7 @@ fn convert_generated_acir_into_circuit(
     let GeneratedAcir {
         return_witnesses,
         locations,
+        brillig_locations,
         input_witnesses,
         assertion_payloads: assert_messages,
         warnings,
@@ -292,7 +293,19 @@ fn convert_generated_acir_into_circuit(
         .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

@@ -9,7 +9,7 @@ use crate::{
 };
 use acvm::acir::{
     circuit::{
-        brillig::{BrilligInputs, BrilligOutputs},
+        brillig::{BrilligFunctionId, BrilligInputs, BrilligOutputs},
         opcodes::{BlackBoxFuncCall, FunctionInput, Opcode as AcirOpcode},
         AssertionPayload, OpcodeLocation,
     },
@@ -27,7 +27,7 @@ use num_bigint::BigUint;
 /// This index should be used when adding a Brillig call during code generation.
 /// Code generation should then keep track of that unresolved call opcode which will be resolved with the
 /// correct function index after code generation.
-pub(crate) const PLACEHOLDER_BRILLIG_INDEX: u32 = 0;
+pub(crate) const PLACEHOLDER_BRILLIG_INDEX: BrilligFunctionId = 0;
 
 #[derive(Debug, Default)]
 /// The output of the Acir-gen pass, which should only be produced for entry point Acir functions
@@ -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<BrilligFunctionId, 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,
@@ -564,33 +570,47 @@ impl<F: AcirField> GeneratedAcir<F> {
         generated_brillig: &GeneratedBrillig<F>,
         inputs: Vec<BrilligInputs<F>>,
         outputs: Vec<BrilligOutputs>,
-        brillig_function_index: u32,
+        brillig_function_index: BrilligFunctionId,
         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(),
             );
         }
     }