From 14518f80f9f44b93c29be233cbb6db3dfffd4546 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Fri, 26 Apr 2024 18:23:54 -0400 Subject: [PATCH 1/3] For disassembly view in DAP mode, return integer addresses Returning serialized opcode locations is invalid according to the DAP specification and confuses VS.Code. --- tooling/debugger/src/context.rs | 131 +++++++++++++------------------- tooling/debugger/src/dap.rs | 59 ++++++-------- 2 files changed, 75 insertions(+), 115 deletions(-) diff --git a/tooling/debugger/src/context.rs b/tooling/debugger/src/context.rs index a423016eac..7bfead29b4 100644 --- a/tooling/debugger/src/context.rs +++ b/tooling/debugger/src/context.rs @@ -35,6 +35,10 @@ pub(super) struct DebugContext<'a, B: BlackBoxFunctionSolver> { breakpoints: HashSet, source_to_opcodes: BTreeMap>, unconstrained_functions: &'a [BrilligBytecode], + + // Absolute (in terms of all the opcodes ACIR+Brillig) addresses of the ACIR + // opcodes with one additional entry for to indicate the last valid address. + acir_opcode_addresses: Vec, } impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { @@ -47,6 +51,7 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { unconstrained_functions: &'a [BrilligBytecode], ) -> Self { let source_to_opcodes = build_source_to_opcode_debug_mappings(debug_artifact); + let acir_opcode_addresses = build_acir_opcode_offsets(circuit, unconstrained_functions); Self { // TODO: need to handle brillig pointer in the debugger acvm: ACVM::new( @@ -61,6 +66,7 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { breakpoints: HashSet::new(), source_to_opcodes, unconstrained_functions, + acir_opcode_addresses, } } @@ -213,102 +219,48 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { .collect() } - fn get_opcodes_sizes(&self) -> Vec { - self.get_opcodes() - .iter() - .map(|opcode| match opcode { - Opcode::BrilligCall { id, .. } => { - self.unconstrained_functions[*id as usize].bytecode.len() - } - _ => 1, - }) - .collect() + /// Returns the absolute address of the opcode at the given location. + /// Absolute here means accounting for nested Brillig opcodes in BrilligCall + /// opcodes. + pub fn opcode_location_to_address(&self, location: &OpcodeLocation) -> usize { + match location { + OpcodeLocation::Acir(acir_index) => self.acir_opcode_addresses[*acir_index], + OpcodeLocation::Brillig { acir_index, brillig_index } => { + self.acir_opcode_addresses[*acir_index] + *brillig_index + } + } } - /// Offsets the given location by the given number of opcodes (including - /// Brillig opcodes). If the offset would move the location outside of a - /// valid circuit location, returns None and the number of remaining - /// opcodes/instructions left which span outside the valid range in the - /// second element of the returned tuple. - pub(super) fn offset_opcode_location( - &self, - location: &Option, - mut offset: i64, - ) -> (Option, i64) { - if offset == 0 { - return (*location, 0); + pub fn address_to_opcode_location(&self, address: usize) -> Option { + if address >= *self.acir_opcode_addresses.last().unwrap_or(&0) { + return None; } - let Some(location) = location else { - return (None, offset); - }; - - let (mut acir_index, mut brillig_index) = match location { - OpcodeLocation::Acir(acir_index) => (*acir_index, 0), - OpcodeLocation::Brillig { acir_index, brillig_index } => (*acir_index, *brillig_index), - }; - let opcode_sizes = self.get_opcodes_sizes(); - if offset > 0 { - while offset > 0 { - let opcode_size = opcode_sizes[acir_index] as i64 - brillig_index as i64; - if offset >= opcode_size { - acir_index += 1; - offset -= opcode_size; - brillig_index = 0; - } else { - brillig_index += offset as usize; - offset = 0; - } - if acir_index >= opcode_sizes.len() { - return (None, offset); - } - } - } else { - while offset < 0 { - if brillig_index > 0 { - if brillig_index > (-offset) as usize { - brillig_index -= (-offset) as usize; - offset = 0; - } else { - offset += brillig_index as i64; - brillig_index = 0; - } - } else { - if acir_index == 0 { - return (None, offset); - } - acir_index -= 1; - let opcode_size = opcode_sizes[acir_index] as i64; - if opcode_size <= -offset { - offset += opcode_size; - } else { - brillig_index = (opcode_size + offset) as usize; - offset = 0; - } - } + let location = match self.acir_opcode_addresses.binary_search(&address) { + Ok(found_index) => OpcodeLocation::Acir(found_index), + Err(insert_index) => { + let acir_index = insert_index - 1; + let base_offset = self.acir_opcode_addresses[acir_index]; + let brillig_index = address - base_offset; + OpcodeLocation::Brillig { acir_index, brillig_index } } - } - if brillig_index > 0 { - (Some(OpcodeLocation::Brillig { acir_index, brillig_index }), 0) - } else { - (Some(OpcodeLocation::Acir(acir_index)), 0) - } + }; + Some(location) } - pub(super) fn render_opcode_at_location(&self, location: &Option) -> String { + pub(super) fn render_opcode_at_location(&self, location: &OpcodeLocation) -> String { let opcodes = self.get_opcodes(); match location { - None => String::from("invalid"), - Some(OpcodeLocation::Acir(acir_index)) => { + OpcodeLocation::Acir(acir_index) => { let opcode = &opcodes[*acir_index]; match opcode { Opcode::BrilligCall { id, .. } => { let first_opcode = &self.unconstrained_functions[*id as usize].bytecode[0]; - format!("BRILLIG CALL {first_opcode:?}") + format!("BRILLIG {first_opcode:?}") } _ => format!("{opcode:?}"), } } - Some(OpcodeLocation::Brillig { acir_index, brillig_index }) => { + OpcodeLocation::Brillig { acir_index, brillig_index } => { match &opcodes[*acir_index] { Opcode::BrilligCall { id, .. } => { let bytecode = &self.unconstrained_functions[*id as usize].bytecode; @@ -643,6 +595,25 @@ fn build_source_to_opcode_debug_mappings( result } +fn build_acir_opcode_offsets( + circuit: &Circuit, + unconstrained_functions: &[BrilligBytecode], +) -> Vec { + let mut result = Vec::with_capacity(circuit.opcodes.len() + 1); + // address of the first opcode is always 0 + result.push(0); + circuit.opcodes.iter().fold(0, |acc, opcode| { + let acc = acc + match opcode { + Opcode::BrilligCall { id, .. } => unconstrained_functions[*id as usize].bytecode.len(), + _ => 1, + }; + // push the starting address of the next opcode + result.push(acc); + acc + }); + result +} + // TODO: update all debugger tests to use unconstrained brillig pointers #[cfg(test)] mod tests { diff --git a/tooling/debugger/src/dap.rs b/tooling/debugger/src/dap.rs index 060945132f..5253fa400b 100644 --- a/tooling/debugger/src/dap.rs +++ b/tooling/debugger/src/dap.rs @@ -205,6 +205,7 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession<'a, R, W, B> { Some(frame) => format!("{} {}", frame.function_name, index), None => format!("frame #{index}"), }; + let address = self.context.opcode_location_to_address(opcode_location); StackFrame { id: index as i64, @@ -218,7 +219,7 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession<'a, R, W, B> { }), line: line_number as i64, column: column_number as i64, - instruction_pointer_reference: Some(opcode_location.to_string()), + instruction_pointer_reference: Some(address.to_string()), ..StackFrame::default() } }) @@ -240,50 +241,38 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession<'a, R, W, B> { let Command::Disassemble(ref args) = req.command else { unreachable!("handle_disassemble called on a non disassemble request"); }; - let starting_ip = OpcodeLocation::from_str(args.memory_reference.as_str()).ok(); + + // we assume memory references are unsigned integers + let starting_address = args.memory_reference.parse::().unwrap_or(0); let instruction_offset = args.instruction_offset.unwrap_or(0); - let (mut opcode_location, mut invalid_count) = - self.context.offset_opcode_location(&starting_ip, instruction_offset); + + let mut address = starting_address + instruction_offset; let mut count = args.instruction_count; let mut instructions: Vec = vec![]; - // leading invalid locations (when the request goes back - // beyond the start of the program) - if invalid_count < 0 { - while invalid_count < 0 { + while count > 0 { + let opcode_location = + if address >= 0 { self.context.address_to_opcode_location(address as usize) } else { None }; + + if let Some(opcode_location) = opcode_location { instructions.push(DisassembledInstruction { - address: String::from("---"), - instruction: String::from("---"), + address: address.to_string(), + // we'll use the instruction_bytes field to render the OpcodeLocation + instruction_bytes: Some(opcode_location.to_string()), + instruction: self.context.render_opcode_at_location(&opcode_location), + ..DisassembledInstruction::default() + }); + } else { + // entry for invalid location to fill up the request + instructions.push(DisassembledInstruction { + address: "---".to_owned(), + instruction: "---".to_owned(), ..DisassembledInstruction::default() }); - invalid_count += 1; - count -= 1; - } - if count > 0 { - opcode_location = Some(OpcodeLocation::Acir(0)); } - } - // the actual opcodes - while count > 0 && opcode_location.is_some() { - instructions.push(DisassembledInstruction { - address: format!("{}", opcode_location.unwrap()), - instruction: self.context.render_opcode_at_location(&opcode_location), - ..DisassembledInstruction::default() - }); - (opcode_location, _) = self.context.offset_opcode_location(&opcode_location, 1); - count -= 1; - } - // any remaining instruction count is beyond the valid opcode - // vector so return invalid placeholders - while count > 0 { - instructions.push(DisassembledInstruction { - address: String::from("---"), - instruction: String::from("---"), - ..DisassembledInstruction::default() - }); - invalid_count -= 1; count -= 1; + address += 1; } self.server.respond( From c0b713de6922075bae76cdcdee45f9a68291f17e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Fri, 26 Apr 2024 18:46:19 -0400 Subject: [PATCH 2/3] Fix unit tests --- tooling/debugger/src/context.rs | 138 ++++++++++++-------------------- 1 file changed, 51 insertions(+), 87 deletions(-) diff --git a/tooling/debugger/src/context.rs b/tooling/debugger/src/context.rs index 7bfead29b4..6124caad01 100644 --- a/tooling/debugger/src/context.rs +++ b/tooling/debugger/src/context.rs @@ -260,16 +260,14 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { _ => format!("{opcode:?}"), } } - OpcodeLocation::Brillig { acir_index, brillig_index } => { - 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"), + OpcodeLocation::Brillig { acir_index, brillig_index } => 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"), + }, } } @@ -603,10 +601,13 @@ fn build_acir_opcode_offsets( // address of the first opcode is always 0 result.push(0); circuit.opcodes.iter().fold(0, |acc, opcode| { - let acc = acc + match opcode { - Opcode::BrilligCall { id, .. } => unconstrained_functions[*id as usize].bytecode.len(), - _ => 1, - }; + let acc = acc + + match opcode { + Opcode::BrilligCall { id, .. } => { + unconstrained_functions[*id as usize].bytecode.len() + } + _ => 1, + }; // push the starting address of the next opcode result.push(acc); acc @@ -825,7 +826,7 @@ mod tests { } #[test] - fn test_offset_opcode_location() { + fn test_address_opcode_location_mapping() { let brillig_bytecode = BrilligBytecode { bytecode: vec![ BrilligOpcode::Stop { return_data_offset: 0, return_data_size: 0 }, @@ -853,85 +854,48 @@ mod tests { brillig_funcs, ); - assert_eq!(context.offset_opcode_location(&None, 0), (None, 0)); - assert_eq!(context.offset_opcode_location(&None, 2), (None, 2)); - assert_eq!(context.offset_opcode_location(&None, -2), (None, -2)); - assert_eq!( - context.offset_opcode_location(&Some(OpcodeLocation::Acir(0)), 0), - (Some(OpcodeLocation::Acir(0)), 0) - ); - assert_eq!( - context.offset_opcode_location(&Some(OpcodeLocation::Acir(0)), 1), - (Some(OpcodeLocation::Brillig { acir_index: 0, brillig_index: 1 }), 0) - ); - assert_eq!( - context.offset_opcode_location(&Some(OpcodeLocation::Acir(0)), 2), - (Some(OpcodeLocation::Brillig { acir_index: 0, brillig_index: 2 }), 0) - ); - assert_eq!( - context.offset_opcode_location(&Some(OpcodeLocation::Acir(0)), 3), - (Some(OpcodeLocation::Acir(1)), 0) - ); - assert_eq!( - context.offset_opcode_location(&Some(OpcodeLocation::Acir(0)), 4), - (Some(OpcodeLocation::Acir(2)), 0) - ); - assert_eq!( - context.offset_opcode_location(&Some(OpcodeLocation::Acir(0)), 5), - (Some(OpcodeLocation::Brillig { acir_index: 2, brillig_index: 1 }), 0) - ); - assert_eq!( - context.offset_opcode_location(&Some(OpcodeLocation::Acir(0)), 7), - (Some(OpcodeLocation::Acir(3)), 0) - ); - assert_eq!(context.offset_opcode_location(&Some(OpcodeLocation::Acir(0)), 8), (None, 0)); - assert_eq!(context.offset_opcode_location(&Some(OpcodeLocation::Acir(0)), 20), (None, 12)); - assert_eq!( - context.offset_opcode_location(&Some(OpcodeLocation::Acir(1)), 2), - (Some(OpcodeLocation::Brillig { acir_index: 2, brillig_index: 1 }), 0) - ); - assert_eq!(context.offset_opcode_location(&Some(OpcodeLocation::Acir(0)), -1), (None, -1)); - assert_eq!( - context.offset_opcode_location(&Some(OpcodeLocation::Acir(0)), -10), - (None, -10) - ); + let locations = + (0..=7).map(|address| context.address_to_opcode_location(address)).collect::>(); + // mapping from addresses to opcode locations assert_eq!( - context.offset_opcode_location( - &Some(OpcodeLocation::Brillig { acir_index: 0, brillig_index: 1 }), - -1 - ), - (Some(OpcodeLocation::Acir(0)), 0) - ); - assert_eq!( - context.offset_opcode_location( - &Some(OpcodeLocation::Brillig { acir_index: 0, brillig_index: 2 }), - -2 - ), - (Some(OpcodeLocation::Acir(0)), 0) - ); - assert_eq!( - context.offset_opcode_location(&Some(OpcodeLocation::Acir(1)), -3), - (Some(OpcodeLocation::Acir(0)), 0) - ); - assert_eq!( - context.offset_opcode_location(&Some(OpcodeLocation::Acir(2)), -4), - (Some(OpcodeLocation::Acir(0)), 0) - ); - assert_eq!( - context.offset_opcode_location( - &Some(OpcodeLocation::Brillig { acir_index: 2, brillig_index: 1 }), - -5 - ), - (Some(OpcodeLocation::Acir(0)), 0) + locations, + vec![ + Some(OpcodeLocation::Acir(0)), + Some(OpcodeLocation::Brillig { acir_index: 0, brillig_index: 1 }), + Some(OpcodeLocation::Brillig { acir_index: 0, brillig_index: 2 }), + Some(OpcodeLocation::Acir(1)), + Some(OpcodeLocation::Acir(2)), + Some(OpcodeLocation::Brillig { acir_index: 2, brillig_index: 1 }), + Some(OpcodeLocation::Brillig { acir_index: 2, brillig_index: 2 }), + Some(OpcodeLocation::Acir(3)), + ] ); + + let addresses = locations + .iter() + .flatten() + .map(|location| context.opcode_location_to_address(location)) + .collect::>(); + + // and vice-versa + assert_eq!(addresses, (0..=7).collect::>()); + + // check edge cases + assert_eq!(None, context.address_to_opcode_location(8)); assert_eq!( - context.offset_opcode_location(&Some(OpcodeLocation::Acir(3)), -7), - (Some(OpcodeLocation::Acir(0)), 0) + 0, + context.opcode_location_to_address(&OpcodeLocation::Brillig { + acir_index: 0, + brillig_index: 0 + }) ); assert_eq!( - context.offset_opcode_location(&Some(OpcodeLocation::Acir(2)), -2), - (Some(OpcodeLocation::Brillig { acir_index: 0, brillig_index: 2 }), 0) + 4, + context.opcode_location_to_address(&OpcodeLocation::Brillig { + acir_index: 2, + brillig_index: 0 + }) ); } } From 8e1427cf7e270ae00dda02d0c8c15160996ca05d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Mon, 29 Apr 2024 13:31:58 -0400 Subject: [PATCH 3/3] Update breakpoint set commands to handle new opcode addressing scheme --- tooling/debugger/src/dap.rs | 38 +++++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/tooling/debugger/src/dap.rs b/tooling/debugger/src/dap.rs index 5253fa400b..c9b6b816a7 100644 --- a/tooling/debugger/src/dap.rs +++ b/tooling/debugger/src/dap.rs @@ -1,6 +1,5 @@ use std::collections::BTreeMap; use std::io::{Read, Write}; -use std::str::FromStr; use acvm::acir::circuit::brillig::BrilligBytecode; use acvm::acir::circuit::{Circuit, OpcodeLocation}; @@ -252,8 +251,11 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession<'a, R, W, B> { let mut instructions: Vec = vec![]; while count > 0 { - let opcode_location = - if address >= 0 { self.context.address_to_opcode_location(address as usize) } else { None }; + let opcode_location = if address >= 0 { + self.context.address_to_opcode_location(address as usize) + } else { + None + }; if let Some(opcode_location) = opcode_location { instructions.push(DisassembledInstruction { @@ -407,25 +409,35 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession<'a, R, W, B> { .breakpoints .iter() .map(|breakpoint| { - let Ok(location) = - OpcodeLocation::from_str(breakpoint.instruction_reference.as_str()) - else { + let offset = breakpoint.offset.unwrap_or(0); + let address = breakpoint.instruction_reference.parse::().unwrap_or(0) + offset; + let Ok(address): Result = address.try_into() else { return Breakpoint { verified: false, - message: Some(String::from("Missing instruction reference")), + message: Some(String::from("Invalid instruction reference/offset")), ..Breakpoint::default() }; }; - if !self.context.is_valid_opcode_location(&location) { + let Some(location) = self + .context + .address_to_opcode_location(address) + .filter(|location| self.context.is_valid_opcode_location(location)) + else { return Breakpoint { verified: false, message: Some(String::from("Invalid opcode location")), ..Breakpoint::default() }; - } + }; let id = self.get_next_breakpoint_id(); breakpoints_to_set.push((location, id)); - Breakpoint { id: Some(id), verified: true, ..Breakpoint::default() } + Breakpoint { + id: Some(id), + verified: true, + offset: Some(0), + instruction_reference: Some(address.to_string()), + ..Breakpoint::default() + } }) .collect(); @@ -485,15 +497,17 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession<'a, R, W, B> { ..Breakpoint::default() }; } - let instruction_reference = format!("{}", location); + let breakpoint_address = self.context.opcode_location_to_address(&location); + let instruction_reference = format!("{}", breakpoint_address); let breakpoint_id = self.get_next_breakpoint_id(); breakpoints_to_set.push((location, breakpoint_id)); Breakpoint { id: Some(breakpoint_id), verified: true, source: Some(args.source.clone()), - instruction_reference: Some(instruction_reference), line: Some(line), + instruction_reference: Some(instruction_reference), + offset: Some(0), ..Breakpoint::default() } })