Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(debugger): Allow debugger to inspect variable state #3302

Closed
wants to merge 11 commits into from

Conversation

mverzilli
Copy link
Contributor

@mverzilli mverzilli commented Oct 26, 2023

Description

Problem

Part of #3015. This is an experimental implementation of a mechanism to instrument variable state inspection for consumption from the debugger.

Summary

We want to use this draft PR as an RFC on an approach to inspect variable state at debugging time. Read Additional Context for a detailed overview.

Additional Context

So far, maybe the trickiest piece of the debugger puzzle that we have been trying to figure out is how to map variables defined in a Noir program to their runtime values, so you can inspect them upon debugging.

The first naïve idea was to attempt extending the current DebugArtifact, and somehow relating opcode offsets to variable names. We found some weeks ago that this approach wasn't very promising. As far as we can tell from tinkering with the compiler internals, there's just too much information lost throughout the different compiler phases, mainly because of optimization passes. There's also some challenges arising from the fact that some info is stored in contexts and discarded at different phases of the compiler.

We then pivoted to another direction, which is the one we want to propose. I'll try to explain and provide some references to specific snippets of code in this PR. The code is super raw as we're still dealing with "plumbing" issues, but we believe the approach is sound and would add a level of instrumentation to the runtime (only when built in debug mode) that would set us up to do pretty powerful stuff, like not only inspecting but also modifying variable values at runtime, or instrumenting particularly important statements such as comparisons in a specific way.

The gist of the idea is to add a new foreign call __debug_state_set that logs debug data to a new DebugState data structure. We then introduce a new debug phase (or maybe more aptly called "aspect"?) that walks the AST and injects calls to that function. Example here of how it looks when capturing variable values right after a let expression, which is the initial use case we're using as an excuse to lay out the plumbing of the solution.

Note that this same kind of AST manipulation could be used to route variable access from the program itself to these DebugState data structures, thus eventually enabling us to let users override variable values from the debugger REPL, for example. It's also worth noting too that this machinery would only be applied when compiling with debug symbols and using the debugger.

The debugger then can simply consume this DebugState data structure, as shown here in a very crude way, which ends up printing things like the code below for the arithmetic_binary_operations test example:

> vars
ITER
  var_id_to_value={1: "Single(Value { inner: 2 })", 0: "Single(Value { inner: 6 })"}
  var_id_to_name={2: "c", 0: "a", 1: "b", 3: "d"}
  var_name_to_id={"b": 1, "c": 2, "a": 0, "d": 3}
{"a": "Single(Value { inner: 6 })", "b": "Single(Value { inner: 2 })"}

The main caveat of this approach, is that it could be considered a bit invasive, kind of proportionally to the power of the features that it enables. Please do share your comments 🙂

Documentation

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [Exceptional Case] Documentation to be submitted in a separate PR.

PR Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've given my main thoughts on Slack, adding a review here to talk about constrained __debug_state_set

@@ -21,30 +22,32 @@ impl DebugArtifact {
pub fn new(debug_symbols: Vec<DebugInfo>, file_manager: &FileManager) -> Self {
let mut file_map = BTreeMap::new();

let files_with_debug_symbols: BTreeSet<FileId> = debug_symbols
let file_ids: Vec<FileId> = debug_symbols
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is removing our deduplication behaviour. We'd like to keep this as otherwise we can get duplicated source files in the debug artifact. Why was this change made?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was made as part of a different approach that keyed the variable mappings by FileId, but I dropped that because it was much more cumbersome and I found a cleaner way to get the result I was going for. But it won't affect anything else now to put this back to a BTreeSet.

#[oracle(__debug_state_set)]
unconstrained fn __debug_state_set_oracle<T>(_var_id: u32, _input: T) {}

unconstrained pub fn __debug_state_set<T>(var_id: u32, value: T) -> T {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
unconstrained pub fn __debug_state_set<T>(var_id: u32, value: T) -> T {
pub fn __debug_state_set<T>(var_id: u32, value: T) -> T {

My understanding of this approach is that any statements of the form

let x = 2 + 3;

get mapped to

let x = __debug_state_set(<value set by compiler>, 2+3);

If __debug_state_set is unconstrained then we break a lot of our compiler optimisations (this is due to the fact that by its very nature, we can't reason about what an unconstrained function will return), if we wanted to maintain these then we can make __debug_state_set constrained instead. This would still emit the value of the variable but we wouldn't be able to modify it in the debugger nicely.

This may need some ugly double nesting here (constrained __debug_state_set calling an unconstrained __debug_state_set calling the _debug_state_set_oracle) unfortunately.

@mverzilli
Copy link
Contributor Author

Closing in favor of #3523, which supersedes this. Both code review comments by @TomAFrench were addressed by @nthiad there.

@mverzilli mverzilli closed this Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants