Skip to content

Commit

Permalink
feat: Handle BrilligCall opcodes in the debugger (#4897)
Browse files Browse the repository at this point in the history 
# Description

## Problem\*

Resolves #4823 

With the introduction of the `BrilligCall` opcode to deduplicate the
Brillig bytecode instead of inlining it into `Brillig` opcodes, the
debugger was not being able to correctly step into the execution of
unconstrained functions.

## Summary\*

This PR adds support to the Noir debugger to handle the new
`BrilligCall` opcodes and allows stepping into unconstrained functions
by retrieving the opcodes from the deduplicated Brillig functions. The
changes impact both the REPL and the DAP debuggers.

## Additional Context

In DAP mode, the disassembly request is still not working properly. It
seems that we are not responding to the command in an appropriate
manner. We will tackle that problem separately.

## Documentation\*

Check one:
- [X] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[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: Maxim Vezenov <mvezenov@gmail.com>
  • Loading branch information
@ggiraldez @vezenovm
ggiraldez and vezenovm authored Apr 24, 2024
1 parent 7dfa151 commit b380dc4
Show file tree
Hide file tree
Showing 4 changed files with 129 additions and 137 deletions.
13 changes: 0 additions & 13 deletions acvm-repo/acvm/src/pwg/brillig.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,19 +30,6 @@ pub struct BrilligSolver<'b, B: BlackBoxFunctionSolver> {
}

impl<'b, B: BlackBoxFunctionSolver> BrilligSolver<'b, B> {
/// Evaluates if the Brillig block should be skipped entirely
pub(super) fn should_skip(
witness: &WitnessMap,
brillig: &Brillig,
) -> Result<bool, OpcodeResolutionError> {
// If the predicate is `None`, the block should never be skipped
// If the predicate is `Some` but we cannot find a value, then we return stalled
match &brillig.predicate {
Some(pred) => Ok(get_value(pred, witness)?.is_zero()),
None => Ok(false),
}
}

/// Assigns the zero value to all outputs of the given [`Brillig`] bytecode.
pub(super) fn zero_out_brillig_outputs(
initial_witness: &mut WitnessMap,
Expand Down
23 changes: 12 additions & 11 deletions acvm-repo/acvm/src/pwg/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,7 @@ impl<'a, B: BlackBoxFunctionSolver> ACVM<'a, B> {
let Opcode::BrilligCall { id, inputs, outputs, predicate } =
&self.opcodes[self.instruction_pointer]
else {
unreachable!("Not executing a Brillig opcode");
unreachable!("Not executing a BrilligCall opcode");
};

let witness = &mut self.witness_map;
Expand Down Expand Up @@ -468,27 +468,28 @@ impl<'a, B: BlackBoxFunctionSolver> ACVM<'a, B> {
}
}

pub fn step_into_brillig_opcode(&mut self) -> StepResult<'a, B> {
let Opcode::Brillig(brillig) = &self.opcodes[self.instruction_pointer] else {
pub fn step_into_brillig(&mut self) -> StepResult<'a, B> {
let Opcode::BrilligCall { id, inputs, outputs, predicate } =
&self.opcodes[self.instruction_pointer]
else {
return StepResult::Status(self.solve_opcode());
};

let witness = &mut self.witness_map;
let should_skip = match BrilligSolver::<B>::should_skip(witness, brillig) {
let should_skip = match is_predicate_false(witness, predicate) {
Ok(result) => result,
Err(err) => return StepResult::Status(self.handle_opcode_resolution(Err(err))),
};

if should_skip {
let resolution =
BrilligSolver::<B>::zero_out_brillig_outputs(witness, &brillig.outputs);
let resolution = BrilligSolver::<B>::zero_out_brillig_outputs(witness, outputs);
return StepResult::Status(self.handle_opcode_resolution(resolution));
}

let solver = BrilligSolver::new(
let solver = BrilligSolver::new_call(
witness,
&self.block_solvers,
brillig,
inputs,
&self.unconstrained_functions[*id as usize].bytecode,
self.backend,
self.instruction_pointer,
);
Expand All @@ -499,8 +500,8 @@ impl<'a, B: BlackBoxFunctionSolver> ACVM<'a, B> {
}

pub fn finish_brillig_with_solver(&mut self, solver: BrilligSolver<'a, B>) -> ACVMStatus {
if !matches!(&self.opcodes[self.instruction_pointer], Opcode::Brillig(..)) {
unreachable!("Not executing a Brillig opcode");
if !matches!(self.opcodes[self.instruction_pointer], Opcode::BrilligCall { .. }) {
unreachable!("Not executing a Brillig/BrilligCall opcode");
}
self.brillig_solver = Some(solver);
self.solve_opcode()
Expand Down
169 changes: 83 additions & 86 deletions tooling/debugger/src/context.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ pub(super) struct DebugContext<'a, B: BlackBoxFunctionSolver> {
debug_artifact: &'a DebugArtifact,
breakpoints: HashSet<OpcodeLocation>,
source_to_opcodes: BTreeMap<FileId, Vec<(usize, OpcodeLocation)>>,
unconstrained_functions: &'a [BrilligBytecode],
}

impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> {
Expand All @@ -59,6 +60,7 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> {
debug_artifact,
breakpoints: HashSet::new(),
source_to_opcodes,
unconstrained_functions,
}
}

Expand Down Expand Up @@ -215,7 +217,9 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> {
self.get_opcodes()
.iter()
.map(|opcode| match opcode {
Opcode::Brillig(brillig_block) => brillig_block.bytecode.len(),
Opcode::BrilligCall { id, .. } => {
self.unconstrained_functions[*id as usize].bytecode.len()
}
_ => 1,
})
.collect()
Expand Down Expand Up @@ -296,19 +300,22 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> {
None => String::from("invalid"),
Some(OpcodeLocation::Acir(acir_index)) => {
let opcode = &opcodes[*acir_index];
if let Opcode::Brillig(ref brillig) = opcode {
let first_opcode = &brillig.bytecode[0];
format!("BRILLIG {first_opcode:?}")
} else {
format!("{opcode:?}")
match opcode {
Opcode::BrilligCall { id, .. } => {
let first_opcode = &self.unconstrained_functions[*id as usize].bytecode[0];
format!("BRILLIG CALL {first_opcode:?}")
}
_ => format!("{opcode:?}"),
}
}
Some(OpcodeLocation::Brillig { acir_index, brillig_index }) => {
if let Opcode::Brillig(ref brillig) = opcodes[*acir_index] {
let opcode = &brillig.bytecode[*brillig_index];
format!(" | {opcode:?}")
} else {
String::from(" | invalid")
match &opcodes[*acir_index] {
Opcode::BrilligCall { id, .. } => {
let bytecode = &self.unconstrained_functions[*id as usize].bytecode;
let opcode = &bytecode[*brillig_index];
format!(" | {opcode:?}")
}
_ => String::from(" | invalid"),
}
}
}
Expand Down Expand Up @@ -400,7 +407,7 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> {
return self.step_brillig_opcode();
}

match self.acvm.step_into_brillig_opcode() {
match self.acvm.step_into_brillig() {
StepResult::IntoBrillig(solver) => {
self.brillig_solver = Some(solver);
self.step_brillig_opcode()
Expand All @@ -409,20 +416,6 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> {
}
}

fn currently_executing_brillig(&self) -> bool {
if self.brillig_solver.is_some() {
return true;
}

match self.get_current_opcode_location() {
Some(OpcodeLocation::Brillig { .. }) => true,
Some(OpcodeLocation::Acir(acir_index)) => {
matches!(self.get_opcodes()[acir_index], Opcode::Brillig(_))
}
_ => false,
}
}

fn get_current_acir_index(&self) -> Option<usize> {
self.get_current_opcode_location().map(|opcode_location| match opcode_location {
OpcodeLocation::Acir(acir_index) => acir_index,
Expand All @@ -446,8 +439,22 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> {
}
}

pub(super) fn is_executing_brillig(&self) -> bool {
if self.brillig_solver.is_some() {
return true;
}

match self.get_current_opcode_location() {
Some(OpcodeLocation::Brillig { .. }) => true,
Some(OpcodeLocation::Acir(acir_index)) => {
matches!(self.get_opcodes()[acir_index], Opcode::BrilligCall { .. })
}
_ => false,
}
}

pub(super) fn step_acir_opcode(&mut self) -> DebugCommandResult {
if self.currently_executing_brillig() {
if self.is_executing_brillig() {
self.step_out_of_brillig_opcode()
} else {
let status = self.acvm.solve_opcode();
Expand Down Expand Up @@ -511,12 +518,6 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> {
}
}

pub(super) fn is_executing_brillig(&self) -> bool {
let opcodes = self.get_opcodes();
let acir_index = self.acvm.instruction_pointer();
acir_index < opcodes.len() && matches!(opcodes[acir_index], Opcode::Brillig(..))
}

pub(super) fn get_brillig_memory(&self) -> Option<&[MemoryValue]> {
self.brillig_solver.as_ref().map(|solver| solver.get_memory())
}
Expand Down Expand Up @@ -552,15 +553,17 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> {
match *location {
OpcodeLocation::Acir(acir_index) => acir_index < opcodes.len(),
OpcodeLocation::Brillig { acir_index, brillig_index } => {
acir_index < opcodes.len()
&& matches!(opcodes[acir_index], Opcode::Brillig(..))
&& {
if let Opcode::Brillig(ref brillig) = opcodes[acir_index] {
brillig_index < brillig.bytecode.len()
} else {
false
if acir_index < opcodes.len() {
match &opcodes[acir_index] {
Opcode::BrilligCall { id, .. } => {
let bytecode = &self.unconstrained_functions[*id as usize].bytecode;
brillig_index < bytecode.len()
}
_ => false,
}
} else {
false
}
}
}
}
Expand Down Expand Up @@ -649,7 +652,7 @@ mod tests {
use acvm::{
acir::{
circuit::{
brillig::{Brillig, BrilligInputs, BrilligOutputs},
brillig::{BrilligInputs, BrilligOutputs},
opcodes::BlockId,
},
native_types::Expression,
Expand All @@ -666,12 +669,7 @@ mod tests {
let fe_1 = FieldElement::one();
let w_x = Witness(1);

let brillig_opcodes = Brillig {
inputs: vec![BrilligInputs::Single(Expression {
linear_combinations: vec![(fe_1, w_x)],
..Expression::default()
})],
outputs: vec![],
let brillig_bytecode = BrilligBytecode {
bytecode: vec![
BrilligOpcode::CalldataCopy {
destination_address: MemoryAddress(0),
Expand All @@ -692,9 +690,17 @@ mod tests {
},
BrilligOpcode::Stop { return_data_offset: 0, return_data_size: 0 },
],
predicate: None,
};
let opcodes = vec![Opcode::Brillig(brillig_opcodes)];
let opcodes = vec![Opcode::BrilligCall {
id: 0,
inputs: vec![BrilligInputs::Single(Expression {
linear_combinations: vec![(fe_1, w_x)],
..Expression::default()
})],
outputs: vec![],
predicate: None,
}];
let brillig_funcs = &vec![brillig_bytecode];
let current_witness_index = 2;
let circuit = &Circuit { current_witness_index, opcodes, ..Circuit::default() };

Expand All @@ -707,7 +713,6 @@ mod tests {

let foreign_call_executor =
Box::new(DefaultDebugForeignCallExecutor::from_artifact(true, debug_artifact));
let brillig_funcs = &vec![];
let mut context = DebugContext::new(
&StubbedBlackBoxSolver,
circuit,
Expand Down Expand Up @@ -766,18 +771,7 @@ mod tests {
let w_z = Witness(3);

// This Brillig block is equivalent to: z = x + y
let brillig_opcodes = Brillig {
inputs: vec![
BrilligInputs::Single(Expression {
linear_combinations: vec![(fe_1, w_x)],
..Expression::default()
}),
BrilligInputs::Single(Expression {
linear_combinations: vec![(fe_1, w_y)],
..Expression::default()
}),
],
outputs: vec![BrilligOutputs::Simple(w_z)],
let brillig_bytecode = BrilligBytecode {
bytecode: vec![
BrilligOpcode::CalldataCopy {
destination_address: MemoryAddress(0),
Expand All @@ -792,11 +786,24 @@ mod tests {
},
BrilligOpcode::Stop { return_data_offset: 0, return_data_size: 1 },
],
predicate: None,
};
let opcodes = vec![
// z = x + y
Opcode::Brillig(brillig_opcodes),
Opcode::BrilligCall {
id: 0,
inputs: vec![
BrilligInputs::Single(Expression {
linear_combinations: vec![(fe_1, w_x)],
..Expression::default()
}),
BrilligInputs::Single(Expression {
linear_combinations: vec![(fe_1, w_y)],
..Expression::default()
}),
],
outputs: vec![BrilligOutputs::Simple(w_z)],
predicate: None,
},
// x + y - z = 0
Opcode::AssertZero(Expression {
mul_terms: vec![],
Expand All @@ -816,7 +823,7 @@ mod tests {

let foreign_call_executor =
Box::new(DefaultDebugForeignCallExecutor::from_artifact(true, debug_artifact));
let brillig_funcs = &vec![];
let brillig_funcs = &vec![brillig_bytecode];
let mut context = DebugContext::new(
&StubbedBlackBoxSolver,
circuit,
Expand Down Expand Up @@ -848,34 +855,24 @@ mod tests {

#[test]
fn test_offset_opcode_location() {
let brillig_bytecode = BrilligBytecode {
bytecode: vec![
BrilligOpcode::Stop { return_data_offset: 0, return_data_size: 0 },
BrilligOpcode::Stop { return_data_offset: 0, return_data_size: 0 },
BrilligOpcode::Stop { return_data_offset: 0, return_data_size: 0 },
],
};

let opcodes = vec![
Opcode::Brillig(Brillig {
inputs: vec![],
outputs: vec![],
bytecode: vec![
BrilligOpcode::Stop { return_data_offset: 0, return_data_size: 0 },
BrilligOpcode::Stop { return_data_offset: 0, return_data_size: 0 },
BrilligOpcode::Stop { return_data_offset: 0, return_data_size: 0 },
],
predicate: None,
}),
Opcode::BrilligCall { id: 0, inputs: vec![], outputs: vec![], predicate: None },
Opcode::MemoryInit { block_id: BlockId(0), init: vec![] },
Opcode::Brillig(Brillig {
inputs: vec![],
outputs: vec![],
bytecode: vec![
BrilligOpcode::Stop { return_data_offset: 0, return_data_size: 0 },
BrilligOpcode::Stop { return_data_offset: 0, return_data_size: 0 },
BrilligOpcode::Stop { return_data_offset: 0, return_data_size: 0 },
],
predicate: None,
}),
Opcode::BrilligCall { id: 0, inputs: vec![], outputs: vec![], predicate: None },
Opcode::AssertZero(Expression::default()),
];
let circuit = Circuit { opcodes, ..Circuit::default() };
let debug_artifact =
DebugArtifact { debug_symbols: vec![], file_map: BTreeMap::new(), warnings: vec![] };
let brillig_funcs = &vec![];
let brillig_funcs = &vec![brillig_bytecode];
let context = DebugContext::new(
&StubbedBlackBoxSolver,
&circuit,
Expand Down
Loading

0 comments on commit b380dc4

Please sign in to comment.