-
Notifications
You must be signed in to change notification settings - Fork 236
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(avm-transpiler): patch debug infos with modified PCs #6371
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,14 @@ | ||
use std::collections::BTreeMap; | ||
|
||
use acvm::acir::brillig::Opcode as BrilligOpcode; | ||
|
||
use acvm::acir::circuit::OpcodeLocation; | ||
use acvm::brillig_vm::brillig::{ | ||
BinaryFieldOp, BinaryIntOp, BlackBoxOp, HeapArray, HeapVector, MemoryAddress, ValueOrArray, | ||
}; | ||
use acvm::FieldElement; | ||
use noirc_errors::debug_info::DebugInfo; | ||
use noirc_errors::Location; | ||
|
||
use crate::instructions::{ | ||
AvmInstruction, AvmOperand, AvmTypeTag, ALL_DIRECT, FIRST_OPERAND_INDIRECT, | ||
|
@@ -13,15 +18,14 @@ use crate::opcodes::AvmOpcode; | |
use crate::utils::{dbg_print_avm_program, dbg_print_brillig_program}; | ||
|
||
/// Transpile a Brillig program to AVM bytecode | ||
pub fn brillig_to_avm(brillig_bytecode: &[BrilligOpcode]) -> Vec<u8> { | ||
pub fn brillig_to_avm( | ||
brillig_bytecode: &[BrilligOpcode], | ||
brillig_pcs_to_avm_pcs: &Vec<usize>, | ||
) -> Vec<u8> { | ||
dbg_print_brillig_program(brillig_bytecode); | ||
|
||
let mut avm_instrs: Vec<AvmInstruction> = Vec::new(); | ||
|
||
// Map Brillig pcs to AVM pcs | ||
// (some Brillig instructions map to >1 AVM instruction) | ||
let brillig_pcs_to_avm_pcs = map_brillig_pcs_to_avm_pcs(avm_instrs.len(), brillig_bytecode); | ||
|
||
// Transpile a Brillig instruction to one or more AVM instructions | ||
for brillig_instr in brillig_bytecode { | ||
match brillig_instr { | ||
|
@@ -1116,6 +1120,39 @@ fn handle_storage_read( | |
}) | ||
} | ||
|
||
/// Patch a Noir function's debug info with updated PCs since transpilation injects extra | ||
/// instructions in some cases. | ||
pub fn patch_debug_info_pcs( | ||
debug_infos: &Vec<DebugInfo>, | ||
brillig_pcs_to_avm_pcs: &Vec<usize>, | ||
) -> Vec<DebugInfo> { | ||
let mut patched_debug_infos = debug_infos.clone(); | ||
|
||
for patched_debug_info in patched_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()); | ||
} | ||
OpcodeLocation::Acir(_) => (), | ||
} | ||
} | ||
// patch debug_info entry | ||
patched_debug_info.locations = patched_locations | ||
} | ||
patched_debug_infos | ||
} | ||
Comment on lines
+1123
to
+1154
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the crux of the PR |
||
|
||
/// Compute an array that maps each Brillig pc to an AVM pc. | ||
/// This must be done before transpiling to properly transpile jump destinations. | ||
/// This is necessary for two reasons: | ||
|
@@ -1126,13 +1163,10 @@ fn handle_storage_read( | |
/// brillig: the Brillig program | ||
/// returns: an array where each index is a Brillig pc, | ||
/// and each value is the corresponding AVM pc. | ||
fn map_brillig_pcs_to_avm_pcs( | ||
initial_offset: usize, | ||
brillig_bytecode: &[BrilligOpcode], | ||
) -> Vec<usize> { | ||
pub fn map_brillig_pcs_to_avm_pcs(brillig_bytecode: &[BrilligOpcode]) -> Vec<usize> { | ||
let mut pc_map = vec![0; brillig_bytecode.len()]; | ||
|
||
pc_map[0] = initial_offset; | ||
pc_map[0] = 0; // first PC is always 0 as there are no instructions inserted by AVM at start | ||
for i in 0..brillig_bytecode.len() - 1 { | ||
let num_avm_instrs_for_this_brillig_instr = match &brillig_bytecode[i] { | ||
BrilligOpcode::Const { bit_size: 254, .. } => 2, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,52 +3,56 @@ use log::info; | |
use serde::{Deserialize, Serialize}; | ||
|
||
use acvm::acir::circuit::Program; | ||
use noirc_errors::debug_info::DebugInfo; | ||
use noirc_errors::debug_info::ProgramDebugInfo; | ||
|
||
use crate::transpile::brillig_to_avm; | ||
use crate::transpile::{brillig_to_avm, map_brillig_pcs_to_avm_pcs, patch_debug_info_pcs}; | ||
use crate::utils::extract_brillig_from_acir_program; | ||
|
||
/// Representation of a contract with some transpiled functions | ||
#[derive(Debug, Serialize, Deserialize)] | ||
pub struct TranspiledContract { | ||
pub struct TranspiledContractArtifact { | ||
pub transpiled: bool, | ||
pub noir_version: String, | ||
pub name: String, | ||
// Functions can be ACIR or AVM | ||
pub functions: Vec<AvmOrAcirContractFunction>, | ||
pub functions: Vec<AvmOrAcirContractFunctionArtifact>, | ||
Comment on lines
-12
to
+19
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These were incorrectly named before. They are named after the ContractArtifact and ContractFunctionArtifact structs in Noir. |
||
pub outputs: serde_json::Value, | ||
pub file_map: serde_json::Value, | ||
//pub warnings: serde_json::Value, | ||
} | ||
|
||
/// A regular contract with ACIR+Brillig functions | ||
/// but with fields irrelevant to transpilation | ||
/// represented as catch-all serde Values | ||
#[derive(Debug, Serialize, Deserialize)] | ||
pub struct CompiledAcirContract { | ||
pub struct CompiledAcirContractArtifact { | ||
pub noir_version: String, | ||
pub name: String, | ||
pub functions: Vec<AcirContractFunction>, | ||
pub functions: Vec<AcirContractFunctionArtifact>, | ||
pub outputs: serde_json::Value, | ||
pub file_map: serde_json::Value, | ||
//pub warnings: serde_json::Value, | ||
} | ||
|
||
/// Representation of a contract function | ||
/// with AVM bytecode as a base64 string | ||
#[derive(Debug, Serialize, Deserialize)] | ||
pub struct AvmContractFunction { | ||
pub struct AvmContractFunctionArtifact { | ||
pub name: String, | ||
pub is_unconstrained: bool, | ||
pub custom_attributes: Vec<String>, | ||
pub abi: serde_json::Value, | ||
pub bytecode: String, // base64 | ||
pub debug_symbols: serde_json::Value, | ||
#[serde( | ||
serialize_with = "ProgramDebugInfo::serialize_compressed_base64_json", | ||
deserialize_with = "ProgramDebugInfo::deserialize_compressed_base64_json" | ||
)] | ||
pub debug_symbols: ProgramDebugInfo, | ||
} | ||
|
||
/// Representation of an ACIR contract function but with | ||
/// catch-all serde Values for fields irrelevant to transpilation | ||
#[derive(Debug, Serialize, Deserialize)] | ||
pub struct AcirContractFunction { | ||
pub struct AcirContractFunctionArtifact { | ||
pub name: String, | ||
pub is_unconstrained: bool, | ||
pub custom_attributes: Vec<String>, | ||
|
@@ -58,23 +62,27 @@ pub struct AcirContractFunction { | |
deserialize_with = "Program::deserialize_program_base64" | ||
)] | ||
pub bytecode: Program, | ||
pub debug_symbols: serde_json::Value, | ||
#[serde( | ||
serialize_with = "ProgramDebugInfo::serialize_compressed_base64_json", | ||
deserialize_with = "ProgramDebugInfo::deserialize_compressed_base64_json" | ||
)] | ||
pub debug_symbols: ProgramDebugInfo, | ||
} | ||
|
||
/// An enum that allows the TranspiledContract struct to contain | ||
/// functions with either ACIR or AVM bytecode | ||
#[derive(Debug, Serialize, Deserialize)] | ||
#[serde(untagged)] // omit Acir/Avm tag for these objects in json | ||
pub enum AvmOrAcirContractFunction { | ||
Acir(AcirContractFunction), | ||
Avm(AvmContractFunction), | ||
pub enum AvmOrAcirContractFunctionArtifact { | ||
Acir(AcirContractFunctionArtifact), | ||
Avm(AvmContractFunctionArtifact), | ||
} | ||
|
||
/// Transpilation is performed when a TranspiledContract | ||
/// is constructed from a CompiledAcirContract | ||
impl From<CompiledAcirContract> for TranspiledContract { | ||
fn from(contract: CompiledAcirContract) -> Self { | ||
let mut functions = Vec::new(); | ||
impl From<CompiledAcirContractArtifact> for TranspiledContractArtifact { | ||
fn from(contract: CompiledAcirContractArtifact) -> Self { | ||
let mut functions: Vec<AvmOrAcirContractFunctionArtifact> = Vec::new(); | ||
|
||
for function in contract.functions { | ||
// TODO(4269): once functions are tagged for transpilation to AVM, check tag | ||
|
@@ -90,31 +98,41 @@ impl From<CompiledAcirContract> for TranspiledContract { | |
let acir_program = function.bytecode; | ||
let brillig_bytecode = extract_brillig_from_acir_program(&acir_program); | ||
|
||
// Map Brillig pcs to AVM pcs (index is Brillig PC, value is AVM PC) | ||
let brillig_pcs_to_avm_pcs = map_brillig_pcs_to_avm_pcs(brillig_bytecode); | ||
|
||
// Transpile to AVM | ||
let avm_bytecode = brillig_to_avm(brillig_bytecode); | ||
let avm_bytecode = brillig_to_avm(brillig_bytecode, &brillig_pcs_to_avm_pcs); | ||
|
||
// Patch the debug infos with updated PCs | ||
let debug_infos = patch_debug_info_pcs( | ||
&function.debug_symbols.debug_infos, | ||
&brillig_pcs_to_avm_pcs, | ||
); | ||
|
||
// Push modified function entry to ABI | ||
functions.push(AvmOrAcirContractFunction::Avm(AvmContractFunction { | ||
name: function.name, | ||
is_unconstrained: function.is_unconstrained, | ||
custom_attributes: function.custom_attributes, | ||
abi: function.abi, | ||
bytecode: base64::prelude::BASE64_STANDARD.encode(avm_bytecode), | ||
debug_symbols: function.debug_symbols, | ||
})); | ||
functions.push(AvmOrAcirContractFunctionArtifact::Avm( | ||
AvmContractFunctionArtifact { | ||
name: function.name, | ||
is_unconstrained: function.is_unconstrained, | ||
custom_attributes: function.custom_attributes, | ||
abi: function.abi, | ||
bytecode: base64::prelude::BASE64_STANDARD.encode(avm_bytecode), | ||
debug_symbols: ProgramDebugInfo { debug_infos }, | ||
}, | ||
)); | ||
} else { | ||
// This function is not flagged for transpilation. Push original entry. | ||
functions.push(AvmOrAcirContractFunction::Acir(function)); | ||
functions.push(AvmOrAcirContractFunctionArtifact::Acir(function)); | ||
} | ||
} | ||
TranspiledContract { | ||
TranspiledContractArtifact { | ||
transpiled: true, | ||
noir_version: contract.noir_version, | ||
name: contract.name, | ||
functions, // some acir, some transpiled avm functions | ||
outputs: contract.outputs, | ||
file_map: contract.file_map, | ||
//warnings: contract.warnings, | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you just need a map? Why not HashMap? (I think it exists? I think in general tree-based maps are only used when order matters, and might have worse time complexity). But I'll defer to any Rust expert!
(ahh... is it because it's what DebugInfo uses?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep I'm just overwriting the entries already in DebugInfo! Not sure why it's a BTreeMap