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: Instrument AST in debug mode to track variable state #3523

Closed
wants to merge 11 commits into from

Conversation

nthiad
Copy link
Contributor

@nthiad nthiad commented Nov 21, 2023

Description

This patch builds on existing debugger work to add support for a vars command to inspect variable values as the debugger steps through instructions. This is accomplished by instrumenting the AST after parsing with foreign calls to debug methods for tracking assignments and drops. There is an additional pass during monomorphization that collects type info.

The instrumentation approach ensures that variables aren't optimized away so that users can step through program logic with variable names as they appear in the original source code, not only with witness maps and register values.

This patch isn't yet ready to be merged in but is available for feedback.

Here's what it looks like to use, demonstrating a shadowed "a" variable. (Ignore the source pointer bug)

At /home/nthia/dev/manas/noir/tooling/nargo_cli/tests/execution_success/arithmetic_binary_operations/src/main.nr:18:9
 12 ...
 13        let b = a - y; // 6 - 4 = 2
 14        let c = b * z; // 2 * 5 = 10
 15        {
 16            let mut a = 3;
 17            a *= 111;
 18 ->         println(f"a={a}");
 19        }
 20        let d = c / a; // 10 / 6 (This uses field inversion, so we test it by multiplying by `a`)
 21        d * a
 22    }
> vars
z:Field=5, x:Field=3, b:Field=2, a:Field=3, a:Field=6, y:Field=4, c:Field=10

Problem*

This feature is part of #3015 and other related work on the debugger. The instrumentation is also useful for integrating with DAP (sending type and callstack info) and tracking callstack parameters.

Earlier into this feature, I explored mapping variables onto witness map entries and register values, but that approach quickly proved to be much more difficult because optimizations happen at multiple stages of the compiler process that destroy information required to establish mappings, and adding more info would be much more difficult than adding instrumentation instructions. Instrumentation also makes other features easier to add beyond only variable tracking such as tracking call stack parameters.

Summary*

  • Adds a vars command to the debugger
  • Instruments code with foreign call wrappers in debug mode to track variable state
  • Works with shadowing but does not yet work with callstacks.

Additional Context

The double-wrapping in the injected oracles for the debug foreign calls is intentional and without this the compiler complains about All `oracle` methods should be wrapped in an unconstrained fn.

This patch is mostly ready but has a few known issues to work on. I will update this list as they are completed:

  • instrumented AST nodes break source offset tracking
  • ensure debugging instrumentation is only turned on in the debugger and can be turned on/off manually
  • member and index assignment
  • display non-scalar data types (structs, tuples, arrays), reading the appropriate number of fields
  • DebugState can probably be removed from the context instance now, if not removed altogether due to the extra pass added to the monomorphize stage

Future work will add tests for various instrumentation cases and will track variables against the callstack (likely with an additional foreign method). Right now the assignment is flat but it does work properly with shadowing.

Documentation*

Check one:

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

The documentation for this feature will be added along with the rest of the debugger documentation, as mentioned in #2995.

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'm going to hold off from digging too deep into this as it seems like there's additional changes to be made so just a few small notes.

@@ -49,6 +49,7 @@ pub fn prepare_package(package: &Package, file_reader: Box<FileReader>) -> (Cont
let mut context = Context::new(fm, graph);

let crate_id = prepare_crate(&mut context, &package.entry_path);
context.root_crate_id = crate_id.clone();
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
context.root_crate_id = crate_id.clone();
context.root_crate_id = crate_id;

This is Copy so a clone is unnecessary.

Comment on lines +16 to +18
let mut debug_vars = Self::default();
debug_vars.id_to_name = vars.clone();
debug_vars
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
let mut debug_vars = Self::default();
debug_vars.id_to_name = vars.clone();
debug_vars
Self {
id_to_name: vars.clone()
..Self::default()
}

debug_vars
}

pub fn get_variables<'a>(&'a self) -> Vec<(&'a str, &'a Value, &'a PrintableType)> {
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
pub fn get_variables<'a>(&'a self) -> Vec<(&'a str, &'a Value, &'a PrintableType)> {
pub fn get_variables(&self) -> Vec<(&str, &Value, &PrintableType)> {

Lifetimes aren't necessary here.

Comment on lines +25 to +34
self.id_to_name
.get(var_id)
.and_then(|name| self.id_to_value.get(var_id).map(|value| (name, value)))
.and_then(|(name, value)| {
self.id_to_type.get(var_id).map(|type_id| (name, value, type_id))
})
.and_then(|(name, value, type_id)| {
self.types.get(type_id).map(|ptype| (name.as_str(), value, ptype))
})
})
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
self.id_to_name
.get(var_id)
.and_then(|name| self.id_to_value.get(var_id).map(|value| (name, value)))
.and_then(|(name, value)| {
self.id_to_type.get(var_id).map(|type_id| (name, value, type_id))
})
.and_then(|(name, value, type_id)| {
self.types.get(type_id).map(|ptype| (name.as_str(), value, ptype))
})
})
let name = self.id_to_name.get(var_id)?;
let value = self.id_to_value.get(var_id)?;
let type_id = self.id_to_type.get(var_id)?;
let printable_type = self.types.get(type_id)?;
Some((name.as_str(), value, printable_type))

We can avoid the usage of and_then with ? which greatly helps readability imo.

// TODO
}

pub fn get<'a>(&'a mut self, var_id: u32) -> Option<&'a Value> {
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
pub fn get<'a>(&'a mut self, var_id: u32) -> Option<&'a Value> {
pub fn get(&mut self, var_id: u32) -> Option<&Value> {

Comment on lines +26 to +27
pub variables: HashMap<u32, (String, u32)>, // var_id => (name, type_id)
pub types: HashMap<u32, PrintableType>, // type_id => printable type
Copy link
Member

Choose a reason for hiding this comment

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

Note to self to look at how this acts when we're not in a debug context. Should we make this optional?

Copy link
Member

Choose a reason for hiding this comment

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

I'm considering whether we want to have these foreign calls defined the the debugger crate instead. Ideally the DefaultForeignCallExecutor would be as thin as possible.

pub fn get_file_ids(&self) -> Vec<FileId> {
self.locations
.values()
.filter_map(|call_stack| call_stack.last().map(|location| location.file))
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure we can't have empty callstacks so this filtering may be unnecessary.


self.walk_scope(&mut f.body.0);

// prapend fn params:
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
// prapend fn params:
// prepend fn params:

nit

Copy link
Member

@TomAFrench TomAFrench Nov 22, 2023

Choose a reason for hiding this comment

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

We need to update Cargo.lock to reflect this.

@TomAFrench TomAFrench marked this pull request as draft November 22, 2023 10:40
@TomAFrench
Copy link
Member

Marking this as draft until the remaining TODOs are addressed.

@mverzilli
Copy link
Contributor

@TomAFrench this has now been superceded by #4122, which should have also addressed all your comments here.

@ggiraldez mentioning you here in case you weren't aware of this PR.

@nthiad if you see this notification, would you close this PR? :).

@mverzilli mverzilli deleted the debug-introspection-vars branch March 15, 2024 14:27
@TomAFrench TomAFrench closed this Jul 15, 2024
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.

3 participants