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

fix: Attach Brillig function ids to TS errors #7930

Merged
merged 10 commits into from
Aug 13, 2024
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()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the crux of the AVM changes in this PR @fcarreiro. All Brillig debug locations are going to be contained within a separate per function brillig locations map now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could just fetch at id 0 here as we make the assumption already that we have a single brillig function. Let me know what you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it works I don't mind the solution! The transpiler is not a bottleneck now.

{
// 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
Loading