Skip to content

Commit

Permalink
fix: Attach Brillig function ids to TS errors (#7930)
Browse files Browse the repository at this point in the history
After noir-lang/noir#5696 Brillig locations have
been split out from ACIR locations in the DebugMetadata. As to maintain
appropriate call stacks in the simulator we need to attach a possible
brillig function id (if we error'd out in a Brillig function) to our TS
errors. The resolution of debug metadata locations was also updated to
account for the additional optional brillig function id.
  • Loading branch information
vezenovm authored Aug 13, 2024
1 parent 346fdd5 commit 29d4e66
Show file tree
Hide file tree
Showing 7 changed files with 84 additions and 27 deletions.
36 changes: 22 additions & 14 deletions avm-transpiler/src/transpile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1060,23 +1060,31 @@ pub fn patch_debug_info_pcs(
brillig_pcs_to_avm_pcs: &[usize],
) -> Vec<DebugInfo> {
for patched_debug_info in debug_infos.iter_mut() {
// create a new map with all of its keys (OpcodeLocations) patched
let mut patched_locations: BTreeMap<OpcodeLocation, Vec<Location>> = BTreeMap::new();
for (original_opcode_location, source_locations) in patched_debug_info.locations.iter() {
match original_opcode_location {
OpcodeLocation::Brillig { acir_index, brillig_index } => {
let avm_opcode_location = OpcodeLocation::Brillig {
acir_index: *acir_index,
// patch the PC
brillig_index: brillig_pcs_to_avm_pcs[*brillig_index],
};
patched_locations.insert(avm_opcode_location, source_locations.clone());
let mut patched_brillig_locations = BTreeMap::new();
for (brillig_function_id, opcode_locations_map) in
patched_debug_info.brillig_locations.iter()
{
// create a new map with all of its keys (OpcodeLocations) patched
let mut patched_locations: BTreeMap<OpcodeLocation, Vec<Location>> = BTreeMap::new();
for (original_opcode_location, source_locations) in opcode_locations_map.iter() {
match original_opcode_location {
OpcodeLocation::Brillig { acir_index, brillig_index } => {
let avm_opcode_location = OpcodeLocation::Brillig {
acir_index: *acir_index,
// patch the PC
brillig_index: brillig_pcs_to_avm_pcs[*brillig_index],
};
patched_locations.insert(avm_opcode_location, source_locations.clone());
}
OpcodeLocation::Acir(_) => (),
}
OpcodeLocation::Acir(_) => (),
}
// insert the new map as a brillig locations map for the current function id
patched_brillig_locations.insert(*brillig_function_id, patched_locations);
}
// patch debug_info entry
patched_debug_info.locations = patched_locations;

// patch the `DebugInfo` entry
patched_debug_info.brillig_locations = patched_brillig_locations;
}
debug_infos
}
Expand Down
18 changes: 14 additions & 4 deletions noir/noir-repo/acvm-repo/acvm_js/src/execute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ pub async fn execute_circuit_with_return_witness(
console_error_panic_hook::set_once();

let program: Program<FieldElement> = Program::deserialize_program(&program)
.map_err(|_| JsExecutionError::new("Failed to deserialize circuit. This is likely due to differing serialization formats between ACVM_JS and your compiler".to_string(), None, None))?;
.map_err(|_| JsExecutionError::new("Failed to deserialize circuit. This is likely due to differing serialization formats between ACVM_JS and your compiler".to_string(), None, None, None))?;

let mut witness_stack = execute_program_with_native_program_and_return(
&program,
Expand All @@ -71,7 +71,7 @@ pub async fn execute_circuit_with_return_witness(
let main_circuit = &program.functions[0];
let return_witness =
extract_indices(&solved_witness, main_circuit.return_values.0.iter().copied().collect())
.map_err(|err| JsExecutionError::new(err, None, None))?;
.map_err(|err| JsExecutionError::new(err, None, None, None))?;

Ok((solved_witness, return_witness).into())
}
Expand Down Expand Up @@ -106,7 +106,8 @@ async fn execute_program_with_native_type_return(
.map_err(|_| JsExecutionError::new(
"Failed to deserialize circuit. This is likely due to differing serialization formats between ACVM_JS and your compiler".to_string(),
None,
None))?;
None,
None))?;

execute_program_with_native_program_and_return(&program, initial_witness, foreign_call_executor)
.await
Expand Down Expand Up @@ -205,6 +206,14 @@ impl<'a, B: BlackBoxFunctionSolver<FieldElement>> ProgramExecutor<'a, B> {
}
_ => None,
};

let brillig_function_id = match &error {
OpcodeResolutionError::BrilligFunctionFailed {
function_id, ..
} => Some(*function_id),
_ => None,
};

// If the failed opcode has an assertion message, integrate it into the error message for backwards compatibility.
// Otherwise, pass the raw assertion payload as is.
let (message, raw_assertion_payload) = match error {
Expand All @@ -230,6 +239,7 @@ impl<'a, B: BlackBoxFunctionSolver<FieldElement>> ProgramExecutor<'a, B> {
message,
call_stack,
raw_assertion_payload,
brillig_function_id,
)
.into());
}
Expand All @@ -253,7 +263,7 @@ impl<'a, B: BlackBoxFunctionSolver<FieldElement>> ProgramExecutor<'a, B> {
call_resolved_outputs.push(*return_value);
} else {
// TODO: look at changing this call stack from None
return Err(JsExecutionError::new(format!("Failed to read from solved witness of ACIR call at witness {}", return_witness_index), None, None).into());
return Err(JsExecutionError::new(format!("Failed to read from solved witness of ACIR call at witness {}", return_witness_index), None, None, None).into());
}
}
acvm.resolve_pending_acir_call(call_resolved_outputs);
Expand Down
14 changes: 13 additions & 1 deletion noir/noir-repo/acvm-repo/acvm_js/src/js_execution_error.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use acvm::{
acir::circuit::{OpcodeLocation, RawAssertionPayload},
acir::circuit::{brillig::BrilligFunctionId, OpcodeLocation, RawAssertionPayload},
FieldElement,
};
use gloo_utils::format::JsValueSerdeExt;
Expand All @@ -12,9 +12,11 @@ export type RawAssertionPayload = {
selector: string;
data: string[];
};
export type ExecutionError = Error & {
callStack?: string[];
rawAssertionPayload?: RawAssertionPayload;
brilligFunctionId?: number;
};
"#;

Expand All @@ -38,6 +40,7 @@ impl JsExecutionError {
message: String,
call_stack: Option<Vec<OpcodeLocation>>,
assertion_payload: Option<RawAssertionPayload<FieldElement>>,
brillig_function_id: Option<BrilligFunctionId>,
) -> Self {
let mut error = JsExecutionError::constructor(JsString::from(message));
let js_call_stack = match call_stack {
Expand All @@ -56,8 +59,17 @@ impl JsExecutionError {
None => JsValue::UNDEFINED,
};

let brillig_function_id = match brillig_function_id {
Some(function_id) => {
<wasm_bindgen::JsValue as JsValueSerdeExt>::from_serde(&function_id)
.expect("Cannot serialize Brillig function id")
}
None => JsValue::UNDEFINED,
};

error.set_property("callStack", js_call_stack);
error.set_property("rawAssertionPayload", assertion_payload);
error.set_property("brilligFunctionId", brillig_function_id);

error
}
Expand Down
10 changes: 9 additions & 1 deletion noir/noir-repo/compiler/wasm/src/types/noir_artifact.ts
Original file line number Diff line number Diff line change
Expand Up @@ -141,14 +141,22 @@ export interface SourceCodeLocation {
*/
export type OpcodeLocation = string;

export type BrilligFunctionId = number;

export type OpcodeToLocationsMap = Record<OpcodeLocation, SourceCodeLocation[]>;

/**
* The debug information for a given function.
*/
export interface DebugInfo {
/**
* A map of the opcode location to the source code location.
*/
locations: Record<OpcodeLocation, SourceCodeLocation[]>;
locations: OpcodeToLocationsMap;
/**
* For each Brillig function, we have a map of the opcode location to the source code location.
*/
brillig_locations: Record<BrilligFunctionId, OpcodeToLocationsMap>;
}

/**
Expand Down
10 changes: 9 additions & 1 deletion yarn-project/foundation/src/abi/abi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -231,14 +231,22 @@ export interface SourceCodeLocation {
*/
export type OpcodeLocation = string;

export type BrilligFunctionId = number;

export type OpcodeToLocationsMap = Record<OpcodeLocation, SourceCodeLocation[]>;

/**
* The debug information for a given function.
*/
export interface DebugInfo {
/**
* A map of the opcode location to the source code location.
*/
locations: Record<OpcodeLocation, SourceCodeLocation[]>;
locations: OpcodeToLocationsMap;
/**
* For each Brillig function, we have a map of the opcode location to the source code location.
*/
brillig_locations: Record<BrilligFunctionId, OpcodeToLocationsMap>;
}

/**
Expand Down
4 changes: 3 additions & 1 deletion yarn-project/pxe/src/pxe_service/pxe_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -718,7 +718,9 @@ export class PXEService implements PXE {
const noirCallStack = err.getNoirCallStack();
if (debugInfo && isNoirCallStackUnresolved(noirCallStack)) {
try {
const parsedCallStack = resolveOpcodeLocations(noirCallStack, debugInfo);
// Public functions are simulated as a single Brillig entry point.
// Thus, we can safely assume here that the Brillig function id is `0`.
const parsedCallStack = resolveOpcodeLocations(noirCallStack, debugInfo, 0);
err.setNoirCallStack(parsedCallStack);
} catch (err) {
this.log.warn(
Expand Down
19 changes: 14 additions & 5 deletions yarn-project/simulator/src/acvm/acvm.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { type NoirCallStack, type SourceCodeLocation } from '@aztec/circuit-types';
import { type FunctionDebugMetadata, type OpcodeLocation } from '@aztec/foundation/abi';
import type { BrilligFunctionId, FunctionDebugMetadata, OpcodeLocation } from '@aztec/foundation/abi';
import { createDebugLogger } from '@aztec/foundation/log';

import {
Expand Down Expand Up @@ -41,10 +41,16 @@ export interface ACIRExecutionResult {
function getSourceCodeLocationsFromOpcodeLocation(
opcodeLocation: string,
debug: FunctionDebugMetadata,
brilligFunctionId?: BrilligFunctionId,
): SourceCodeLocation[] {
const { debugSymbols, files } = debug;

const callStack = debugSymbols.locations[opcodeLocation] || [];
let callStack = debugSymbols.locations[opcodeLocation] || [];
if (callStack.length === 0) {
if (brilligFunctionId !== undefined) {
callStack = debugSymbols.brillig_locations[brilligFunctionId][opcodeLocation] || [];
}
}
return callStack.map(call => {
const { file: fileId, span } = call;

Expand Down Expand Up @@ -76,8 +82,11 @@ function getSourceCodeLocationsFromOpcodeLocation(
export function resolveOpcodeLocations(
opcodeLocations: OpcodeLocation[],
debug: FunctionDebugMetadata,
brilligFunctionId?: BrilligFunctionId,
): SourceCodeLocation[] {
return opcodeLocations.flatMap(opcodeLocation => getSourceCodeLocationsFromOpcodeLocation(opcodeLocation, debug));
return opcodeLocations.flatMap(opcodeLocation =>
getSourceCodeLocationsFromOpcodeLocation(opcodeLocation, debug, brilligFunctionId),
);
}

/**
Expand Down Expand Up @@ -143,13 +152,13 @@ export function extractCallStack(
if (!('callStack' in error) || !error.callStack) {
return undefined;
}
const { callStack } = error;
const { callStack, brilligFunctionId } = error;
if (!debug) {
return callStack;
}

try {
return resolveOpcodeLocations(callStack, debug);
return resolveOpcodeLocations(callStack, debug, brilligFunctionId);
} catch (err) {
return callStack;
}
Expand Down

0 comments on commit 29d4e66

Please sign in to comment.