Skip to content

Commit

Permalink
fix: Use plain integer addresses for opcodes in DAP disassembly view (#…
Browse files Browse the repository at this point in the history 
…4941)

# Description

## Problem\*

Resolves #4904

The disassembly view from VS.Code didn't work properly. Sometimes it
would only show the first opcode only and once open it wouldn't update
the current opcode pointer when stepping through the code. This was
because we were using opcode locations (eg. `3.17`) as instruction
references (typed as string in the API), but VS.Code expects integers
either in decimal or hexadecimal format.

## Summary\*

This PR updates the disassembly request DAP command handler to return
integer addresses only, and changes the instruction set breakpoint
command to parse and map the given address to the appropriate opcode
location.

## Additional Context



## 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.
  • Loading branch information
@ggiraldez
ggiraldez authored May 22, 2024
1 parent 20c4b81 commit d43ba1b
Show file tree
Hide file tree
Showing 2 changed files with 145 additions and 207 deletions.
259 changes: 97 additions & 162 deletions tooling/debugger/src/context.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ pub(super) struct DebugContext<'a, B: BlackBoxFunctionSolver> {
breakpoints: HashSet<OpcodeLocation>,
source_to_opcodes: BTreeMap<FileId, Vec<(usize, OpcodeLocation)>>,
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<usize>,
}

impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> {
Expand All @@ -46,6 +50,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(
Expand All @@ -61,6 +66,7 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> {
breakpoints: HashSet::new(),
source_to_opcodes,
unconstrained_functions,
acir_opcode_addresses,
}
}

Expand Down Expand Up @@ -213,111 +219,55 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> {
.collect()
}

fn get_opcodes_sizes(&self) -> Vec<usize> {
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<OpcodeLocation>,
mut offset: i64,
) -> (Option<OpcodeLocation>, i64) {
if offset == 0 {
return (*location, 0);
pub fn address_to_opcode_location(&self, address: usize) -> Option<OpcodeLocation> {
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);
}
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 }
}
} 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;
}
}
}
}
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<OpcodeLocation>) -> 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 }) => {
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"),
},
}
}

Expand Down Expand Up @@ -640,6 +590,28 @@ fn build_source_to_opcode_debug_mappings(
result
}

fn build_acir_opcode_offsets(
circuit: &Circuit,
unconstrained_functions: &[BrilligBytecode],
) -> Vec<usize> {
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 {
Expand Down Expand Up @@ -851,7 +823,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 },
Expand Down Expand Up @@ -883,85 +855,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::<Vec<_>>();

// 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::<Vec<_>>();

// and vice-versa
assert_eq!(addresses, (0..=7).collect::<Vec<_>>());

// 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
})
);
}
}
Loading

0 comments on commit d43ba1b

Please sign in to comment.