feat(debugger): CodeTracer time-travelling debugger support#7758
Open
nickysn wants to merge 229 commits intonoir-lang:masterfrom
Open
feat(debugger): CodeTracer time-travelling debugger support#7758nickysn wants to merge 229 commits intonoir-lang:masterfrom
nickysn wants to merge 229 commits intonoir-lang:masterfrom
Conversation
This commit introduces `nargo trace`. When invoked, it behaves as if `nargo debug` is invoked, and then the `next` command is used in the REPL until the end of the program is reached. At the moment, just the number of `next` commands is counted and reported. The next commit will generate and write a `trace.json` file that will contain a record of the execution trace. To test: ``` cd test_programs/plonky2_prove_success/zk_dungeon/ nargo trace --trace-dir /tmp ``` It should output the following: ``` Total tracing steps: 3644 ``` Note that the trace-dir is not used at the moment, so it doesn't matter what you pass as an argument. (cherry picked from commit 96f3b29)
This scaffolding allows the additions of tests for `nargo trace` in the same vein as tests are added for other nargo commands. To check locally what this PR does, checkout the repo and run `cargo test trace` It should result in a single test running successfully. To convince yourself that the test is real, try changing the info message printed in `tooling/tracer/src/lib.rs:117` and running the test again. Note: there is no actual tracing logic yet, but this will be added in following commits. (cherry picked from commit 818d433)
Some of the copy-crud related to witnesses is deleted from `trace_cmd.rs:run`, `trace_program_and_decode`, `trace_program`, and `trace_circuit`. `trace_circuit` now returns a `TraceArtifact`, which is saved to a file in `trace_program` and any errors are propagated back. The test scaffold is updated to look for an `expected_trace.json` in each test directory and compares it with the file generated by running `nargo trace`. Also, the test verifies that `nargo trace` prints an info message that the file has been saved. (cherry picked from commit 3829579)
This commit has no functional changes. To verify, run `cargo test trace`, which should still pass (the test is not touched by this commit). Changes: - `next_into` is renamed to `step_debugger`; a doccomment is added - the function is simplified: it now performs less matching - `validate_in_progress`, a remnant from the debugger REPL is no longer needed, so it's removed - `handle_debug_command_result`: ditto - the field `context` is renamed to `debug_context`, to reduce potential confusion - the complexity (matching of the debugger result) is lifted into the `trace_circuit` function Next commit to start adding more logic to `step_debugger`. (cherry picked from commit d4d4813)
Before this commit, a tracing step was a jump over one opcode. After this commit, a tracing step is a jump over one line in the source code. The test relevant to tracing is updated, so now the expected number of tracing steps is less (7, instead of 14). For some reason, the test does not print "warning: no call stack" anymore. I'm not completely sure why, but this is a wanted improvement, so the expectation for that message is removed. There is a potential low-priority bug, involving function calls. A TODO for it is added in the code. **Details** The following functions are no longer private in `DebugContext`: * `get_call_stack` * `get_source_location_for_opcode_location` Although this is not ideal, it is an acceptable middle-term solution, until common logic of the debugger and the tracer is extracted out of the debugger. The following functions are added to `DebugContext`, in order to surface functionality of the contained `debug_artifact`: * `get_filepath_for_location` * `get_line_for_location` In `tracer/src/lib.rs`: * the initializing logic that checks that the circuit contains opcodes is now moved into the `trace_circuit` function, rather than in the constructor for `TracingContext`; * `debug_context` is renamed to `tracing_context`, which matches the type; * the error for when there is a breakpoint during tracing is promoted to `panic!`, as it should never happen (indicates a serious bug if it happens); * `last_result` is no longer stored in TracingContext, to keep things purer. **Next steps** More tests need to be added to explore the error modes. When is there no call stack? When is there no mapping from opcodes to source locations? This is not a priority, however, so the first follow-up that is coming is actually recording information on each step and saving it in the tracing artifact. (cherry picked from commit dde0fcd)
This commit imports `metacraft-labs/runtime_tracing` as a dependency and uses it instead of `TraceArtifact` to construct the tracing record. For this reason, `tooling/nargo_cli/src/cli/fs/trace.rs` and `tooling/noirc_artifacts/src/trace.rs` are no longer needed. `update_record` actually propagates some information to `Tracer`, which is now written to the output. This can be seen from the fact that `expected_trace.json` is now non-empty. To test, run the following: ``` cargo test trace ``` The test still passes, exactly because of the change in `expected_trace.json`. **Next steps** The next steps include recording function calls and variables. (cherry picked from commit ba7ab37)
This commit just outlines some logic into the `get_current_line_and_filepath` function, to keep the body of `step_debugger` fitting on a single screen. Also, a long TODO comment is replaced with a reference to a filed bug. There is no functional change: the test still passes: ``` cargo test trace ``` (cherry picked from commit adb4904)
Before this commit, the `expected_trace.json` files had to have the exact same format as the output of the serializer, which for size considerations is not pretty printed. Now, the files are not diff'ed exactly, but read into serde Value's, which allows the comparison to ignore formatting. This is illustrated by formatting the one test that currently exists. The test still passes, even though the actual generated .json looks like how `expected_trace.json` looked before this commit. (cherry picked from commit 2fcf540)
The generated trace from this trace illustrates a problem: lines containing only function calls (e.g. lines 6, 10, and 11) are skipped in the currently generated trace. A follow-up PR will address this problem. This PR also updated 1_mul/expected_trace.json, because otherwise the test does not pass (I forgot to update it previously). (cherry picked from commit 19f72ee)
This commit simplifies the testing scaffold (in `tooling/nargo_cli/build.rs`) because instead of comparing a message about the number of steps, we now compare the full traces. It also updates `expected_trace.json` in the test `2_function_calls`, but the change is not related to code change in this commit. Rather, the test was broken in the `plonky2` (default) branch, and this PR fixes that as a drive-by. (cherry picked from commit 59f6c69)
Changes: * a `SourceLocation` struct is introduced to handle a file-line pair in a more hygienic way * a `DebugStepResult` is introduced, to simplify the interface between `trace_circuit` and `step_debugger` Run the tracing tests to see that they still pass: ``` cargo test trace ``` There is no change in the tests, which demonstrates that there's no change in the business logic. This is just a structural change. (cherry picked from commit 94d8f20)
If the next source line for `nargo debug` is a function call, one debugger step includes it, together with the first line of the called function. This is just how `nargo debug` works and a fact of life we choose not to change. Instead, this commit takes this into account in order to produce a correct trace that includes as steps both the invocation site, as well as the first line of the function. The tests are updated to demonstrate the effect of the bugfix. Note the new lines that show up in the expected traces now. As a drive-by, the initial location that is registered now depends on the implementation of create_unknown, rather than an ad-hoc invalid location. (See change in expected_trace.json for 1_mul and call to `tracer.start`.) Run tests with: ``` cargo test trace ``` Note: the change in `get_current_source_locations` is actually simpler than it looks: new scopes and indentation are introduced, however, which trips up the change diff algorithm. (cherry picked from commit db866a6)
The new test `3_two_files` contains multiple source files, where `main.nr` calls into `foo.nr` which calls into `bar.nr`. The `expected_trace.json` demonstrates that the tracer handles this correctly. Note the `path_id` fields in the JSON, which say which file the step is in. To run and see that the test passes: ``` cargo test trace ``` (cherry picked from commit 3ab7113)
This commit also records function calls and returns from functions, without passing the actual arguments and values in either direction. That would be a follow-up. Details: * `get_variables` is now public in `tooling/debugger/src/context.rs`; it is needed to extract the stack frame from the debugger; * drive-by: `trace_metadata.json` is also stored, because the frontend expects that too; * `tail_diff_vecs` is a generic utility function to extract the difference between two vectors; it has good documentation, so details within; * a local structure `StackFrame` is introduced, because the `StackFrame` in the debugger does not own its data; here, we need to remember the previous stack trace (a vector of `StackFrame`'s) in order to discover function calls or returns, so we need to own the data; * a `stack_trace` field is added to `TracingContext`; see previous point; * the new `fn get_stack_trace` performs the conversion mentioned; * the old contents of `update_record` are the basis for `tail_diff_vecs`; the new contents use that function to perform the same operation as before; in addition, the core logic of the PR is here: return events are registered for dropped stack frames and call events are registered for new stack frames; * all tracing tests are updated, to reflect the new traces that are generated. To test: ``` cargo test trace ``` (cherry picked from commit 38bfd20)
(cherry picked from commit 35aee57)
(cherry picked from commit 4cff853)
(cherry picked from commit 1978e83)
(cherry picked from commit 3db2137)
(cherry picked from commit 8b40415)
This prepares the function for containing more than just extracting the function name: the next change will add to it the function call parameters and their values. (cherry picked from commit edfc04d)
This commit is a first part of a two-part change; the foundation is laid here to register variables during execution (debugging) of a Noir program. The second part change will convert all types, not just Field, as can be seen in the code from this first part. The tests deleted in this commit will be restored then. (cherry picked from commit ed050b2)
The previous commit supports only the conversion of values of type Field to the appropriate trace type. This commit also adds support for UnsignedInteger, which allows the two tests deleted in the previous commit to be restored, because now they pass. (cherry picked from commit e28d762)
Usage instruction at the top of the script. (cherry picked from commit 305ecb5)
This newer version makes some changes to the format of the trace, but in parts that we are not using yet. (cherry picked from commit f4d642e)
The `variables` in `StackFrame` are now sorted before construction of the frame. This is an unenforced invariant of the type. The sorting is thus not needed anymore after the field is accessed (previously, done in `fn register_variables`). The new field of `StackFrame` -- `function_param_indexes` -- is a vector of indexes in this sorted vector (`variables`). It lists which variables are function parameters. `stack_trace` is renamed to `stack_frames` for increased symmetry of the code (it's of type `Vec<StackFrame>`) Finally, instead of an empty vec[], actual args are passed to `register_call`, which are built on top of the other changes mentioned. One question that remains open is whether `register_value` needs to be called in `convert_params_to_args_vec` or not. This is noted in a comment and will be addressed at a later point. (cherry picked from commit 8b93b2d)
Also, this commit moves the logic of what files need to be generated from the trace to the `tracer_glue` module which keeps the use-site sparkling clean. (cherry picked from commit 887e2f2)
Rewrote the tool in Rust instead of bash so it integrates better with the rest of our system. Also, it now prints arrays on multiple lines. More information in README.md. (cherry picked from commit 314a6e3)
(cherry picked from commit 517168d)
(cherry picked from commit f55e0a8)
When the `format_trace` package was added, the effect on Cargo.lock was not added to the commit, so this commit fixes that. Also, it contains some version bumps as a drive-by. (cherry picked from commit 55980fd)
…lues **Problem:** A recent change in the Noir compiler (noir-lang#9579) modified its output for reference types. It no longer dereferences them to provide the underlying value in the trace. This caused the tracer to panic because it was designed to always receive a `PrintableValue` for a `PrintableType::Reference`. **Solution:** This commit adapts the tracer to this new debugger behavior. When processing a reference, it now acknowledges that the `PrintableValue` may not be available. It constructs a `ValueRecord::Reference` with the correct type information but uses `ValueRecord::None` as a placeholder for the dereferenced content. This prevents the panic and correctly represents a reference to an unknown value within the trace.
…acer_trace_writer 0.16.0
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This adds support for the CodeTracer time-travelling debugger:
https://github.com/metacraft-labs/codetracer
It also includes several fixes for the nargo debugger.
Problem*
Adds support for CodeTracer (
nargo tracecommand).Adds the
nargo format-tracecommand, which pretty-prints files, produced bynargo trace.Resolves:
Debugger enters both the "then" and the "else" statement of a single "if" #5922
Resolves other bugs in the debugger (
nargo debug)Summary*
This adds a new command
nargo trace, which produces an execution trace, compatible with CodeTracer. Also adds another commandnargo format-trace, which pretty-prints thenargo tracefiles.It also includes several debugger fixes.
Additional Context
This is extracted from the Blocksense Noir branch at:
https://github.com/blocksense-network/noir
By cherry picking only the commits, related to
nargo traceand the debugger.Documentation*
Check one:
PR Checklist*
cargo fmton default settings.