From d39adddc5812c634a4f7b0af41baf8ffdf19b19b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Thu, 12 Oct 2023 22:56:39 -0400 Subject: [PATCH 1/2] feat: When debugging, step into Brillig blocks and execute opcode by opcode --- acvm-repo/acvm/src/pwg/brillig.rs | 9 ++++ acvm-repo/acvm/src/pwg/mod.rs | 71 +++++++++++++++++++++++++++++++ tooling/debugger/src/lib.rs | 29 +++++++++---- 3 files changed, 101 insertions(+), 8 deletions(-) diff --git a/acvm-repo/acvm/src/pwg/brillig.rs b/acvm-repo/acvm/src/pwg/brillig.rs index 6fc54d42eab..d96cf6ba076 100644 --- a/acvm-repo/acvm/src/pwg/brillig.rs +++ b/acvm-repo/acvm/src/pwg/brillig.rs @@ -111,11 +111,20 @@ impl<'b, B: BlackBoxFunctionSolver> BrilligSolver<'b, B> { Ok(Self { vm, acir_index }) } + pub(super) fn program_counter(&self) -> usize { + self.vm.program_counter() + } + pub(super) fn solve(&mut self) -> Result { let status = self.vm.process_opcodes(); self.handle_vm_status(status) } + pub(super) fn step(&mut self) -> Result { + let status = self.vm.process_opcode(); + self.handle_vm_status(status) + } + fn handle_vm_status( &self, vm_status: VMStatus, diff --git a/acvm-repo/acvm/src/pwg/mod.rs b/acvm-repo/acvm/src/pwg/mod.rs index 057597e6392..5b42981f0f6 100644 --- a/acvm-repo/acvm/src/pwg/mod.rs +++ b/acvm-repo/acvm/src/pwg/mod.rs @@ -177,6 +177,20 @@ impl<'a, B: BlackBoxFunctionSolver> ACVM<'a, B> { self.instruction_pointer } + pub fn location(&self) -> Option { + if self.instruction_pointer >= self.opcodes.len() { + // evaluation finished + None + } else if let Some(solver) = &self.brillig_solver { + Some(OpcodeLocation::Brillig { + acir_index: self.instruction_pointer, + brillig_index: solver.program_counter(), + }) + } else { + Some(OpcodeLocation::Acir(self.instruction_pointer)) + } + } + /// Finalize the ACVM execution, returning the resulting [`WitnessMap`]. pub fn finalize(self) -> WitnessMap { if self.status != ACVMStatus::Solved { @@ -263,6 +277,28 @@ impl<'a, B: BlackBoxFunctionSolver> ACVM<'a, B> { res => res.map(|_| ()), }, }; + self.handle_resolution(resolution) + } + + pub fn step_opcode(&mut self) -> ACVMStatus { + match &self.opcodes[self.instruction_pointer] { + Opcode::Brillig(_) => { + let resolution = match self.step_brillig_opcode() { + Ok(BrilligSolverStatus::ForeignCallWait(foreign_call)) => { + return self.wait_for_foreign_call(foreign_call) + } + Ok(BrilligSolverStatus::InProgress) => { + return self.status(ACVMStatus::InProgress) + } + res => res.map(|_| ()), + }; + self.handle_resolution(resolution) + } + _ => self.solve_opcode(), + } + } + + fn handle_resolution(&mut self, resolution: Result<(), OpcodeResolutionError>) -> ACVMStatus { match resolution { Ok(()) => { self.instruction_pointer += 1; @@ -331,6 +367,41 @@ impl<'a, B: BlackBoxFunctionSolver> ACVM<'a, B> { } } } + + fn step_brillig_opcode(&mut self) -> Result { + let Opcode::Brillig(brillig) = &self.opcodes[self.instruction_pointer] else { + unreachable!("Not executing a Brillig opcode"); + }; + let witness = &mut self.witness_map; + + // Try to use the cached `BrilligSolver` when stepping through opcodes + let mut solver: BrilligSolver<'_, B> = match self.brillig_solver.take() { + Some(solver) => solver, + None => { + if BrilligSolver::::should_skip(witness, brillig)? { + return BrilligSolver::::zero_out_brillig_outputs(witness, brillig) + .map(|_| BrilligSolverStatus::Finished); + } + BrilligSolver::new(witness, brillig, self.backend, self.instruction_pointer)? + } + }; + let status = solver.step()?; + match status { + BrilligSolverStatus::ForeignCallWait(_) => { + // Cache the current state of the solver + self.brillig_solver = Some(solver); + } + BrilligSolverStatus::InProgress => { + // Cache the current state of the solver + self.brillig_solver = Some(solver); + } + BrilligSolverStatus::Finished => { + // Write execution outputs + solver.finalize(witness, brillig)?; + } + }; + Ok(status) + } } // Returns the concrete value for a particular witness diff --git a/tooling/debugger/src/lib.rs b/tooling/debugger/src/lib.rs index 6fde85e35f1..9d46450780c 100644 --- a/tooling/debugger/src/lib.rs +++ b/tooling/debugger/src/lib.rs @@ -27,9 +27,13 @@ struct DebugContext<'backend, B: BlackBoxFunctionSolver> { impl<'backend, B: BlackBoxFunctionSolver> DebugContext<'backend, B> { fn step_opcode(&mut self) -> Result { - let solver_status = self.acvm.as_mut().unwrap().solve_opcode(); + let solver_status = self.acvm.as_mut().unwrap().step_opcode(); - match solver_status { + self.handle_acvm_status(solver_status) + } + + fn handle_acvm_status(&mut self, status: ACVMStatus) -> Result { + match status { ACVMStatus::Solved => Ok(SolveResult::Done), ACVMStatus::InProgress => Ok(SolveResult::Ok), ACVMStatus::Failure(error) => { @@ -67,13 +71,22 @@ impl<'backend, B: BlackBoxFunctionSolver> DebugContext<'backend, B> { fn show_current_vm_status(&self) { let acvm = self.acvm.as_ref().unwrap(); - let ip = acvm.instruction_pointer(); + let location = acvm.location(); let opcodes = acvm.opcodes(); - if ip >= opcodes.len() { - println!("Finished execution"); - } else { - println!("Stopped at opcode {}: {}", ip, opcodes[ip]); - Self::show_source_code_location(&OpcodeLocation::Acir(ip), &self.debug_artifact); + match location { + None => println!("Finished execution"), + Some(location) => { + match location { + OpcodeLocation::Acir(ip) => { + println!("Stopped at opcode {}: {}", ip, opcodes[ip]) + } + OpcodeLocation::Brillig { acir_index: ip, brillig_index } => println!( + "Stopped at opcode {} in Brillig block {}: {}", + brillig_index, ip, opcodes[ip] + ), + } + Self::show_source_code_location(&location, &self.debug_artifact); + } } } From ea4c4cd33b92f22afc75d3d6cfc306d4b857df2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Fri, 13 Oct 2023 12:03:19 -0400 Subject: [PATCH 2/2] chore: deduplicate code --- acvm-repo/acvm/src/pwg/mod.rs | 83 ++++++++++------------------------- tooling/debugger/src/lib.rs | 2 +- 2 files changed, 23 insertions(+), 62 deletions(-) diff --git a/acvm-repo/acvm/src/pwg/mod.rs b/acvm-repo/acvm/src/pwg/mod.rs index 5b42981f0f6..08b2463c9ce 100644 --- a/acvm-repo/acvm/src/pwg/mod.rs +++ b/acvm-repo/acvm/src/pwg/mod.rs @@ -177,6 +177,7 @@ impl<'a, B: BlackBoxFunctionSolver> ACVM<'a, B> { self.instruction_pointer } + /// Returns the location for the next opcode to execute, or None if execution is finished pub fn location(&self) -> Option { if self.instruction_pointer >= self.opcodes.len() { // evaluation finished @@ -256,6 +257,10 @@ impl<'a, B: BlackBoxFunctionSolver> ACVM<'a, B> { } pub fn solve_opcode(&mut self) -> ACVMStatus { + self.step_opcode(false) + } + + pub fn step_opcode(&mut self, step_into_brillig: bool) -> ACVMStatus { let opcode = &self.opcodes[self.instruction_pointer]; let resolution = match opcode { @@ -272,33 +277,15 @@ impl<'a, B: BlackBoxFunctionSolver> ACVM<'a, B> { let solver = self.block_solvers.entry(*block_id).or_default(); solver.solve_memory_op(op, &mut self.witness_map, predicate) } - Opcode::Brillig(_) => match self.solve_brillig_opcode() { - Ok(Some(foreign_call)) => return self.wait_for_foreign_call(foreign_call), + Opcode::Brillig(_) => match self.step_brillig_opcode(step_into_brillig) { + Ok(BrilligSolverStatus::ForeignCallWait(foreign_call)) => { + return self.wait_for_foreign_call(foreign_call) + } + Ok(BrilligSolverStatus::InProgress) => return self.status(ACVMStatus::InProgress), res => res.map(|_| ()), }, }; - self.handle_resolution(resolution) - } - - pub fn step_opcode(&mut self) -> ACVMStatus { - match &self.opcodes[self.instruction_pointer] { - Opcode::Brillig(_) => { - let resolution = match self.step_brillig_opcode() { - Ok(BrilligSolverStatus::ForeignCallWait(foreign_call)) => { - return self.wait_for_foreign_call(foreign_call) - } - Ok(BrilligSolverStatus::InProgress) => { - return self.status(ACVMStatus::InProgress) - } - res => res.map(|_| ()), - }; - self.handle_resolution(resolution) - } - _ => self.solve_opcode(), - } - } - fn handle_resolution(&mut self, resolution: Result<(), OpcodeResolutionError>) -> ACVMStatus { match resolution { Ok(()) => { self.instruction_pointer += 1; @@ -332,66 +319,40 @@ impl<'a, B: BlackBoxFunctionSolver> ACVM<'a, B> { } } - fn solve_brillig_opcode( + fn step_brillig_opcode( &mut self, - ) -> Result, OpcodeResolutionError> { - let Opcode::Brillig(brillig) = &self.opcodes[self.instruction_pointer] else { - unreachable!("Not executing a Brillig opcode"); - }; - let witness = &mut self.witness_map; - if BrilligSolver::::should_skip(witness, brillig)? { - BrilligSolver::::zero_out_brillig_outputs(witness, brillig).map(|_| None) - } else { - // If we're resuming execution after resolving a foreign call then - // there will be a cached `BrilligSolver` to avoid recomputation. - let mut solver: BrilligSolver<'_, B> = match self.brillig_solver.take() { - Some(solver) => solver, - None => { - BrilligSolver::new(witness, brillig, self.backend, self.instruction_pointer)? - } - }; - match solver.solve()? { - BrilligSolverStatus::ForeignCallWait(foreign_call) => { - // Cache the current state of the solver - self.brillig_solver = Some(solver); - Ok(Some(foreign_call)) - } - BrilligSolverStatus::InProgress => { - unreachable!("Brillig solver still in progress") - } - BrilligSolverStatus::Finished => { - // Write execution outputs - solver.finalize(witness, brillig)?; - Ok(None) - } - } - } - } - - fn step_brillig_opcode(&mut self) -> Result { + step_into: bool, + ) -> Result { let Opcode::Brillig(brillig) = &self.opcodes[self.instruction_pointer] else { unreachable!("Not executing a Brillig opcode"); }; let witness = &mut self.witness_map; - // Try to use the cached `BrilligSolver` when stepping through opcodes + // Try to use the cached `BrilligSolver` which we will have if: + // - stepping into a Brillig block + // - resuming execution from a foreign call let mut solver: BrilligSolver<'_, B> = match self.brillig_solver.take() { Some(solver) => solver, None => { if BrilligSolver::::should_skip(witness, brillig)? { + // Exit early if the block doesn't need to be executed (false predicate) return BrilligSolver::::zero_out_brillig_outputs(witness, brillig) .map(|_| BrilligSolverStatus::Finished); } BrilligSolver::new(witness, brillig, self.backend, self.instruction_pointer)? } }; - let status = solver.step()?; + + let status = if step_into { solver.step()? } else { solver.solve()? }; match status { BrilligSolverStatus::ForeignCallWait(_) => { // Cache the current state of the solver self.brillig_solver = Some(solver); } BrilligSolverStatus::InProgress => { + if !step_into { + unreachable!("Brillig solver still in progress") + } // Cache the current state of the solver self.brillig_solver = Some(solver); } diff --git a/tooling/debugger/src/lib.rs b/tooling/debugger/src/lib.rs index 9d46450780c..5e7325334ed 100644 --- a/tooling/debugger/src/lib.rs +++ b/tooling/debugger/src/lib.rs @@ -27,7 +27,7 @@ struct DebugContext<'backend, B: BlackBoxFunctionSolver> { impl<'backend, B: BlackBoxFunctionSolver> DebugContext<'backend, B> { fn step_opcode(&mut self) -> Result { - let solver_status = self.acvm.as_mut().unwrap().step_opcode(); + let solver_status = self.acvm.as_mut().unwrap().step_opcode(true); self.handle_acvm_status(solver_status) }