Skip to content

Commit

Permalink
fix(debugger): crash when stepping through locations spanning multipl…
Browse files Browse the repository at this point in the history
…e lines (#3920)

# Description

The debugger REPL was failing under certain scenarios when a stepped
location spanned multiple lines. We tracked down the failure to a
missing bounds check.

This PR fixes the issue, introduces a refactor to make the relevant code
testable, and adds regression tests to prevent reoccurrence.

This PR is best reviewed as a whole instead of commit-by-commit.

## Summary

- Moves source code printing logic from the main debugger REPL module,
to a new source_code_printer module.
- Decouples actual printing from rendering logic to improve testability.
- Adds regression tests

## Documentation

Check one:
- [ ] No documentation needed.
- [x] Documentation included in this PR.
- [ ] **[Exceptional Case]** 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
mverzilli authored Jan 2, 2024
1 parent f9bfed6 commit 223e860
Show file tree
Hide file tree
Showing 6 changed files with 414 additions and 82 deletions.
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion tooling/debugger/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,7 @@ codespan-reporting.workspace = true
dap.workspace = true
easy-repl = "0.2.1"
owo-colors = "3"
serde_json.workspace = true
serde_json.workspace = true

[dev_dependencies]
tempfile.workspace = true
1 change: 1 addition & 0 deletions tooling/debugger/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
mod context;
mod dap;
mod repl;
mod source_code_printer;

use std::io::{Read, Write};

Expand Down
83 changes: 2 additions & 81 deletions tooling/debugger/src/repl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,7 @@ use nargo::{artifacts::debug::DebugArtifact, ops::DefaultForeignCallExecutor, Na
use easy_repl::{command, CommandStatus, Repl};
use std::cell::RefCell;

use codespan_reporting::files::Files;
use noirc_errors::Location;

use owo_colors::OwoColorize;

use std::ops::Range;
use crate::source_code_printer::print_source_code_location;

pub struct ReplDebugger<'a, B: BlackBoxFunctionSolver> {
context: DebugContext<'a, B>,
Expand Down Expand Up @@ -70,73 +65,7 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> {
);
}
}
self.show_source_code_location(&location);
}
}
}

fn print_location_path(&self, loc: Location) {
let line_number = self.debug_artifact.location_line_number(loc).unwrap();
let column_number = self.debug_artifact.location_column_number(loc).unwrap();

println!(
"At {}:{line_number}:{column_number}",
self.debug_artifact.name(loc.file).unwrap()
);
}

fn show_source_code_location(&self, location: &OpcodeLocation) {
let locations = self.debug_artifact.debug_symbols[0].opcode_location(location);
let Some(locations) = locations else { return };
for loc in locations {
self.print_location_path(loc);

let loc_line_index = self.debug_artifact.location_line_index(loc).unwrap();

// How many lines before or after the location's line we
// print
let context_lines = 5;

let first_line_to_print =
if loc_line_index < context_lines { 0 } else { loc_line_index - context_lines };

let last_line_index = self.debug_artifact.last_line_index(loc).unwrap();
let last_line_to_print = std::cmp::min(loc_line_index + context_lines, last_line_index);

let source = self.debug_artifact.location_source_code(loc).unwrap();
for (current_line_index, line) in source.lines().enumerate() {
let current_line_number = current_line_index + 1;

if current_line_index < first_line_to_print {
// Ignore lines before range starts
continue;
} else if current_line_index == first_line_to_print && current_line_index > 0 {
// Denote that there's more lines before but we're not showing them
print_line_of_ellipsis(current_line_index);
}

if current_line_index > last_line_to_print {
// Denote that there's more lines after but we're not showing them,
// and stop printing
print_line_of_ellipsis(current_line_number);
break;
}

if current_line_index == loc_line_index {
// Highlight current location
let Range { start: loc_start, end: loc_end } =
self.debug_artifact.location_in_line(loc).unwrap();
println!(
"{:>3} {:2} {}{}{}",
current_line_number,
"->",
&line[0..loc_start].to_string().dimmed(),
&line[loc_start..loc_end],
&line[loc_end..].to_string().dimmed()
);
} else {
print_dimmed_line(current_line_number, line);
}
print_source_code_location(self.debug_artifact, &location);
}
}
}
Expand Down Expand Up @@ -384,14 +313,6 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> {
}
}

fn print_line_of_ellipsis(line_number: usize) {
println!("{}", format!("{:>3} {}", line_number, "...").dimmed());
}

fn print_dimmed_line(line_number: usize, line: &str) {
println!("{}", format!("{:>3} {:2} {}", line_number, "", line).dimmed());
}

pub fn run<B: BlackBoxFunctionSolver>(
blackbox_solver: &B,
circuit: &Circuit,
Expand Down
Loading

0 comments on commit 223e860

Please sign in to comment.