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

Simple assembly debugger #693

Merged
merged 2 commits into from
Feb 17, 2023
Merged

Simple assembly debugger #693

merged 2 commits into from
Feb 17, 2023

Conversation

frisitano
Copy link
Contributor

@frisitano frisitano commented Feb 9, 2023

This is a work in progress. This PR introduces a primitive miden assembly debugger. It allows the user to step through code and print out the vm state at each clock cycle. The debugger supports the following commands:

    --------------------------------------------------------
    Miden Assembly Debug CLI
    ---------------------------------------------------------
    !next        steps to the next clock cycle
    !play        executes program until completion or failure
    !play.n      executes n clock cycles
    !prev        steps to the previous clock cycle
    !rewind      rewinds program until beginning
    !rewind.n    rewinds n clock cycles
    !print       displays the complete state of the virtual machine
    !stack       displays the complete state of the stack
    !mem         displays the complete state of memory
    !mem[i]      displays memory at address i
    !help        displays this message

It can be invoked via the CLI using: ./miden debug -a ASSEMBLY_FILE.masm -i INPUT_FILE

Example:
Screenshot from 2023-02-10 00-08-16

@frisitano frisitano force-pushed the frisitano-simple-debug branch 6 times, most recently from 50a98b6 to 4598fac Compare February 13, 2023 08:16
@frisitano frisitano changed the title (DRAFT) Simple assembly debugger Simple assembly debugger Feb 13, 2023
@frisitano frisitano marked this pull request as ready for review February 13, 2023 08:21
Copy link
Contributor

@hackaugusto hackaugusto left a comment

Choose a reason for hiding this comment

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

Tested locally, works as advertised. This is pretty awesome :)

I left one comment, but that is just a detail. I'm not going to merge so other people can review it too.

One thing that I haven't look at, do we have any sort of stack available? A way of figuring out where we are in the program.

miden/src/cli/debug.rs Outdated Show resolved Hide resolved
miden/src/cli/debug.rs Outdated Show resolved Hide resolved
miden/src/cli/debug.rs Outdated Show resolved Hide resolved
miden/src/cli/debug.rs Outdated Show resolved Hide resolved
processor/src/debug.rs Outdated Show resolved Hide resolved
processor/src/debug.rs Outdated Show resolved Hide resolved
@frisitano frisitano force-pushed the frisitano-simple-debug branch 4 times, most recently from ff11bd2 to 8253edf Compare February 15, 2023 06:09
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you for doing this! I left a couple of comments inline, but I also thing we should update the docs to let people know about this. The docs could be updated in this PR or in a different PR (but then we should create an issue for this).

For doc updates, I'd add a bullet point in the "Running Miden VM" section for the debug command, but also I wonder if a separate section (similar to the one we have for REPL) is warranted. If we do add a separate section, we should probably break out REPL and Debug sections into their own files as (still sub-sections under "Usage").

miden/src/repl/mod.rs Outdated Show resolved Hide resolved
miden/src/cli/debug.rs Outdated Show resolved Hide resolved
miden/src/cli/debug.rs Outdated Show resolved Hide resolved
processor/src/debug.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you! I left a couple more comments inline - but to summarize:

  • Let's remove VmState display logic from the processor crate. This should also fix the WASM compilation errors.
  • Could you fix this minor issue: Simple assembly debugger #693 (comment)
  • Let's create an issue for updating documentation (unless it has already been created).

@frisitano frisitano force-pushed the frisitano-simple-debug branch 3 times, most recently from 221f30c to ecb348c Compare February 16, 2023 07:21
@frisitano
Copy link
Contributor Author

frisitano commented Feb 16, 2023

  • Let's remove VmState display logic from the processor crate. This should also fix the WASM compilation errors.

Done.

I've removed all changes to the REPL module such that the debugger implementation is standalone. I've made this issue which states that we should consolidate documentation and logic between the debugger and REPL #699. It would also be cool to allow the REPL to be feature of the debugger. Say you get to a particular clock cycle and you then enter repl mode where you can specify new instructions against the wm.

  • Let's create an issue for updating documentation (unless it has already been created).
    created

#699

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

All looks good! Thank you! I think we just need to resolve the merge conflict and then we can merge.

@frisitano frisitano force-pushed the frisitano-simple-debug branch 2 times, most recently from 5468b4b to 0306691 Compare February 17, 2023 07:46
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