Skip to content

Commit

Permalink
feat(nargo): Handle call stacks for multiple Acir calls (#4711)
Browse files Browse the repository at this point in the history
# Description

## Problem\*

Resolves leftover work from #4428 

We would error out on calls in non-inlined Acir calls, however, we would
only use the debug info from the main function.

## Summary\*

There is now a new `ResolvedOpcodeLocation` that wraps an acir function
index from the `Program` as well as its opcode location. We then build a
call stack during execution that is resolved upon execution failure.
Both `ExecutionError` variants now contain a call stack of
`ResolvedOpcodeLocation`.

For the `fold_basic_nested_call` if I make `assert(x == y)` fail I will
get the following now:
<img width="811" alt="Screenshot 2024-04-03 at 4 53 17 PM"
src="https://github.com/noir-lang/noir/assets/43554004/9c50bec5-4f85-4fe1-93ed-0527f9511f2c">

On master we would get an error in `main` at the first call to
`func_with_nested_foo_call`.

For the new
`test_programs/execution_failure/fold_nested_brillig_assert_fail` test
we have this output:
<img width="859" alt="Screenshot 2024-04-11 at 9 50 34 AM"
src="https://github.com/noir-lang/noir/assets/43554004/58ef8d8f-f67c-477d-9af1-cc1ccd89be5e">

I then also added support for call stacks with dynamic indices in
`test_programs/execution_failure/fold_dyn_index_fail`:
<img width="774" alt="Screenshot 2024-04-11 at 9 51 16 AM"
src="https://github.com/noir-lang/noir/assets/43554004/1066a05a-78db-4c84-83fd-77c17d83cc81">


## Additional Context



## Documentation\*

Check one:
- [] No documentation needed.
- [ ] Documentation included in this PR.
- [X] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [X] I have tested the changes locally.
- [X] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.

---------

Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
  • Loading branch information
vezenovm and TomAFrench committed Apr 15, 2024
1 parent b46d0e3 commit 5b23171
Show file tree
Hide file tree
Showing 27 changed files with 345 additions and 160 deletions.
10 changes: 10 additions & 0 deletions acvm-repo/acir/src/circuit/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,16 @@ impl Circuit {
}
}

#[derive(Debug, Copy, Clone)]
/// The opcode location for a call to a separate ACIR circuit
/// This includes the function index of the caller within a [program][Program]
/// and the index in the callers ACIR to the specific call opcode.
/// This is only resolved and set during circuit execution.
pub struct ResolvedOpcodeLocation {
pub acir_function_index: usize,
pub opcode_location: OpcodeLocation,
}

#[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.
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_driver/src/contract.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ pub struct ContractFunction {
)]
pub bytecode: Program,

pub debug: DebugInfo,
pub debug: Vec<DebugInfo>,

/// Names of the functions in the program. These are used for more informative debugging and benchmarking.
pub names: Vec<String>,
Expand Down
10 changes: 3 additions & 7 deletions compiler/noirc_driver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,8 @@ fn compile_contract_inner(
}

if errors.is_empty() {
let debug_infos: Vec<_> = functions.iter().map(|function| function.debug.clone()).collect();
let debug_infos: Vec<_> =
functions.iter().flat_map(|function| function.debug.clone()).collect();
let file_map = filter_relevant_files(&debug_infos, &context.file_manager);

let out_structs = contract
Expand Down Expand Up @@ -547,13 +548,8 @@ pub fn compile_no_check(

Ok(CompiledProgram {
hash,
// TODO(https://github.com/noir-lang/noir/issues/4428)
program,
// TODO(https://github.com/noir-lang/noir/issues/4428)
// Debug info is only relevant for errors at execution time which is not yet supported
// The CompileProgram `debug` field is used in multiple places and is better
// left to be updated once execution of multiple ACIR functions is enabled
debug: debug[0].clone(),
debug,
abi,
file_map,
noir_version: NOIR_ARTIFACT_VERSION_STRING.to_string(),
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_driver/src/program.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ pub struct CompiledProgram {
)]
pub program: Program,
pub abi: noirc_abi::Abi,
pub debug: DebugInfo,
pub debug: Vec<DebugInfo>,
pub file_map: BTreeMap<FileId, DebugFile>,
pub warnings: Vec<SsaReport>,
/// Names of the functions in the program. These are used for more informative debugging and benchmarking.
Expand Down
79 changes: 43 additions & 36 deletions compiler/noirc_errors/src/debug_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,49 @@ pub type DebugVariables = BTreeMap<DebugVarId, DebugVariable>;
pub type DebugFunctions = BTreeMap<DebugFnId, DebugFunction>;
pub type DebugTypes = BTreeMap<DebugTypeId, PrintableType>;

#[derive(Default, Debug, Clone, Deserialize, Serialize)]
pub struct ProgramDebugInfo {
pub debug_infos: Vec<DebugInfo>,
}

impl ProgramDebugInfo {
pub fn serialize_compressed_base64_json<S>(
debug_info: &ProgramDebugInfo,
s: S,
) -> Result<S::Ok, S::Error>
where
S: Serializer,
{
let json_str = serde_json::to_string(debug_info).map_err(S::Error::custom)?;

let mut encoder = DeflateEncoder::new(Vec::new(), Compression::default());
encoder.write_all(json_str.as_bytes()).map_err(S::Error::custom)?;
let compressed_data = encoder.finish().map_err(S::Error::custom)?;

let encoded_b64 = base64::prelude::BASE64_STANDARD.encode(compressed_data);
s.serialize_str(&encoded_b64)
}

pub fn deserialize_compressed_base64_json<'de, D>(
deserializer: D,
) -> Result<ProgramDebugInfo, D::Error>
where
D: Deserializer<'de>,
{
let encoded_b64: String = Deserialize::deserialize(deserializer)?;

let compressed_data =
base64::prelude::BASE64_STANDARD.decode(encoded_b64).map_err(D::Error::custom)?;

let mut decoder = DeflateDecoder::new(&compressed_data[..]);
let mut decompressed_data = Vec::new();
decoder.read_to_end(&mut decompressed_data).map_err(D::Error::custom)?;

let json_str = String::from_utf8(decompressed_data).map_err(D::Error::custom)?;
serde_json::from_str(&json_str).map_err(D::Error::custom)
}
}

#[serde_as]
#[derive(Default, Debug, Clone, Deserialize, Serialize)]
pub struct DebugInfo {
Expand Down Expand Up @@ -130,40 +173,4 @@ impl DebugInfo {

counted_opcodes
}

pub fn serialize_compressed_base64_json<S>(
debug_info: &DebugInfo,
s: S,
) -> Result<S::Ok, S::Error>
where
S: Serializer,
{
let json_str = serde_json::to_string(debug_info).map_err(S::Error::custom)?;

let mut encoder = DeflateEncoder::new(Vec::new(), Compression::default());
encoder.write_all(json_str.as_bytes()).map_err(S::Error::custom)?;
let compressed_data = encoder.finish().map_err(S::Error::custom)?;

let encoded_b64 = base64::prelude::BASE64_STANDARD.encode(compressed_data);
s.serialize_str(&encoded_b64)
}

pub fn deserialize_compressed_base64_json<'de, D>(
deserializer: D,
) -> Result<DebugInfo, D::Error>
where
D: Deserializer<'de>,
{
let encoded_b64: String = Deserialize::deserialize(deserializer)?;

let compressed_data =
base64::prelude::BASE64_STANDARD.decode(encoded_b64).map_err(D::Error::custom)?;

let mut decoder = DeflateDecoder::new(&compressed_data[..]);
let mut decompressed_data = Vec::new();
decoder.read_to_end(&mut decompressed_data).map_err(D::Error::custom)?;

let json_str = String::from_utf8(decompressed_data).map_err(D::Error::custom)?;
serde_json::from_str(&json_str).map_err(D::Error::custom)
}
}
10 changes: 10 additions & 0 deletions compiler/wasm/src/types/noir_artifact.ts
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,16 @@ export interface DebugInfo {
locations: Record<OpcodeLocation, SourceCodeLocation[]>;
}

/**
* The debug information for a given program.
*/
export interface ProgramDebugInfo {
/**
* An array that maps to each function of a program.
*/
debug_infos: Array<DebugInfo>;
}

/**
* Maps a file ID to its metadata for debugging purposes.
*/
Expand Down
13 changes: 10 additions & 3 deletions compiler/wasm/test/compiler/shared/compile.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
ContractCompilationArtifacts,
DebugFileMap,
DebugInfo,
ProgramDebugInfo,
NoirFunctionEntry,
ProgramArtifact,
ProgramCompilationArtifacts,
Expand All @@ -15,7 +16,7 @@ export function shouldCompileProgramIdentically(
expect: typeof Expect,
timeout = 5000,
) {
it('both nargo and noir_wasm should compile identically', async () => {
it('both nargo and noir_wasm should compile program identically', async () => {
// Compile!
const { nargoArtifact, noirWasmArtifact } = await compileFn();

Expand Down Expand Up @@ -51,7 +52,7 @@ export function shouldCompileContractIdentically(
expect: typeof Expect,
timeout = 5000,
) {
it('both nargo and noir_wasm should compile identically', async () => {
it('both nargo and noir_wasm should compile contract identically', async () => {
// Compile!
const { nargoArtifact, noirWasmArtifact } = await compileFn();

Expand Down Expand Up @@ -90,7 +91,7 @@ function extractDebugInfos(fns: NoirFunctionEntry[]) {
return fns.map((fn) => {
const debugSymbols = inflateDebugSymbols(fn.debug_symbols);
delete (fn as Partial<NoirFunctionEntry>).debug_symbols;
clearFileIdentifiers(debugSymbols);
clearFileIdentifiersProgram(debugSymbols);
return debugSymbols;
});
}
Expand All @@ -113,6 +114,12 @@ function deleteContractDebugMetadata(contract: ContractArtifact) {
return [extractDebugInfos(contract.functions), fileMap];
}

function clearFileIdentifiersProgram(debugSymbols: ProgramDebugInfo) {
debugSymbols.debug_infos.map((debug_info) => {
clearFileIdentifiers(debug_info);
});
}

/** Clears file identifiers from a set of debug symbols. */
function clearFileIdentifiers(debugSymbols: DebugInfo) {
for (const loc of Object.values(debugSymbols.locations)) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "fold_dyn_index_fail"
type = "bin"
authors = [""]
compiler_version = ">=0.26.0"

[dependencies]
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
x = [104, 101, 108, 108, 111]
z = "4"
10 changes: 10 additions & 0 deletions test_programs/execution_failure/fold_dyn_index_fail/src/main.nr
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
fn main(mut x: [u32; 5], z: Field) {
x[z] = 4;
dynamic_index_check(x, z + 10);
}

#[fold]
fn dynamic_index_check(x: [u32; 5], idx: Field) {
// Dynamic index is greater than length of the array
assert(x[idx] != 0);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "fold_nested_brillig_assert_fail"
type = "bin"
authors = [""]
compiler_version = ">=0.26.0"

[dependencies]
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
x = "0"
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// Tests a very simple program.
//
// The features being tested is using assert on brillig that is triggered through nested ACIR calls.
// We want to make sure we get a call stack from the original call in main to the failed assert.
fn main(x: Field) {
assert(1 == fold_conditional_wrapper(x as bool));
}

#[fold]
fn fold_conditional_wrapper(x: bool) -> Field {
fold_conditional(x)
}

#[fold]
fn fold_conditional(x: bool) -> Field {
conditional_wrapper(x)
}

unconstrained fn conditional_wrapper(x: bool) -> Field {
conditional(x)
}

unconstrained fn conditional(x: bool) -> Field {
assert(x);
1
}
6 changes: 4 additions & 2 deletions tooling/debugger/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,8 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> {
self.handle_foreign_call(foreign_call)
}
Err(err) => DebugCommandResult::Error(NargoError::ExecutionError(
ExecutionError::SolvingError(err),
// TODO: debugger does not not handle multiple acir calls
ExecutionError::SolvingError(err, None),
)),
}
}
Expand Down Expand Up @@ -374,7 +375,8 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> {
}
}
ACVMStatus::Failure(error) => DebugCommandResult::Error(NargoError::ExecutionError(
ExecutionError::SolvingError(error),
// TODO: debugger does not not handle multiple acir calls
ExecutionError::SolvingError(error, None),
)),
ACVMStatus::RequiresForeignCall(_) => {
unreachable!("Unexpected pending foreign call resolution");
Expand Down
2 changes: 1 addition & 1 deletion tooling/debugger/src/dap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -603,7 +603,7 @@ pub fn run_session<R: Read, W: Write, B: BlackBoxFunctionSolver>(
initial_witness: WitnessMap,
) -> Result<(), ServerError> {
let debug_artifact = DebugArtifact {
debug_symbols: vec![program.debug],
debug_symbols: program.debug,
file_map: program.file_map,
warnings: program.warnings,
};
Expand Down
19 changes: 12 additions & 7 deletions tooling/lsp/src/requests/profile_run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,24 +84,29 @@ fn on_profile_run_request_inner(
let compiled_program =
nargo::ops::transform_program(compiled_program, expression_width);

let span_opcodes = compiled_program.debug.count_span_opcodes();
let debug_artifact: DebugArtifact = compiled_program.clone().into();
opcodes_counts.extend(span_opcodes);
for function_debug in compiled_program.debug.iter() {
let span_opcodes = function_debug.count_span_opcodes();
opcodes_counts.extend(span_opcodes);
}
let debug_artifact: DebugArtifact = compiled_program.into();
file_map.extend(debug_artifact.file_map);
}

for compiled_contract in compiled_contracts {
let compiled_contract =
nargo::ops::transform_contract(compiled_contract, expression_width);

let function_debug_info: Vec<_> =
compiled_contract.functions.iter().map(|func| &func.debug).cloned().collect();
let debug_artifact: DebugArtifact = compiled_contract.into();
file_map.extend(debug_artifact.file_map);
let function_debug_info = compiled_contract
.functions
.iter()
.flat_map(|func| &func.debug)
.collect::<Vec<_>>();
for contract_function_debug in function_debug_info {
let span_opcodes = contract_function_debug.count_span_opcodes();
opcodes_counts.extend(span_opcodes);
}
let debug_artifact: DebugArtifact = compiled_contract.into();
file_map.extend(debug_artifact.file_map);
}

let result = NargoProfileRunResult { file_map, opcodes_counts };
Expand Down
10 changes: 5 additions & 5 deletions tooling/nargo/src/artifacts/contract.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use noirc_driver::{CompiledContract, CompiledContractOutputs, ContractFunction};
use serde::{Deserialize, Serialize};

use noirc_driver::DebugFile;
use noirc_errors::debug_info::DebugInfo;
use noirc_errors::debug_info::ProgramDebugInfo;
use std::collections::{BTreeMap, HashMap};

use fm::FileId;
Expand Down Expand Up @@ -68,10 +68,10 @@ pub struct ContractFunctionArtifact {
pub bytecode: Program,

#[serde(
serialize_with = "DebugInfo::serialize_compressed_base64_json",
deserialize_with = "DebugInfo::deserialize_compressed_base64_json"
serialize_with = "ProgramDebugInfo::serialize_compressed_base64_json",
deserialize_with = "ProgramDebugInfo::deserialize_compressed_base64_json"
)]
pub debug_symbols: DebugInfo,
pub debug_symbols: ProgramDebugInfo,
}

impl From<ContractFunction> for ContractFunctionArtifact {
Expand All @@ -82,7 +82,7 @@ impl From<ContractFunction> for ContractFunctionArtifact {
custom_attributes: func.custom_attributes,
abi: func.abi,
bytecode: func.bytecode,
debug_symbols: func.debug,
debug_symbols: ProgramDebugInfo { debug_infos: func.debug },
}
}
}
4 changes: 2 additions & 2 deletions tooling/nargo/src/artifacts/debug.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ impl DebugArtifact {
impl From<CompiledProgram> for DebugArtifact {
fn from(compiled_program: CompiledProgram) -> Self {
DebugArtifact {
debug_symbols: vec![compiled_program.debug],
debug_symbols: compiled_program.debug,
file_map: compiled_program.file_map,
warnings: compiled_program.warnings,
}
Expand All @@ -133,7 +133,7 @@ impl From<CompiledContract> for DebugArtifact {
let all_functions_debug: Vec<DebugInfo> = compiled_artifact
.functions
.into_iter()
.map(|contract_function| contract_function.debug)
.flat_map(|contract_function| contract_function.debug)
.collect();

DebugArtifact {
Expand Down

0 comments on commit 5b23171

Please sign in to comment.