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

Expand capabilities of the debug instruction #1103

Merged
merged 1 commit into from
Oct 19, 2023
Merged

Conversation

Fumuran
Copy link
Contributor

@Fumuran Fumuran commented Oct 11, 2023

This PR expands the capabilities of the debug instruction by printing information about memory and procedure locals, namely:

  • debug.mem prints out the entire contents of RAM.
  • debug.mem.<n> prints out contents of memory at address n.
  • debug.mem.<n>.<m> prints out the contents of memory starting at address n and ending at address m (both inclusive). m must be greater than n.
  • debug.local prints out all locals of the currently executing procedure.
  • debug.local.<n> prints out contents of the local at index n for the currently executing procedure.
  • debug.local.<n>.<m> prints out contents of the local starting at index n and ending at index m (both inclusive). m must be greater than n.

TODO:

  • Update docs and changelog

@Fumuran Fumuran changed the title WiP Expand capabilities of the debug instruction Oct 13, 2023
@Fumuran Fumuran requested a review from bobbinth October 13, 2023 14:28
@Fumuran Fumuran marked this pull request as ready for review October 13, 2023 14:28
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.

Thank you! Looks good! This is not a full review yet, but I wanted to leave a few comments and I think it will change things a bit.

Also, could you paste a couple of screen shots of how the results look like?

processor/src/lib.rs Outdated Show resolved Hide resolved
processor/src/host/debug.rs Show resolved Hide resolved
processor/src/host/debug.rs Outdated Show resolved Hide resolved
processor/src/host/debug.rs Outdated Show resolved Hide resolved
processor/src/host/debug.rs Outdated Show resolved Hide resolved
core/src/operations/decorators/debug.rs Outdated Show resolved Hide resolved
@Fumuran Fumuran force-pushed the andrew-expand-debug branch 2 times, most recently from e580c9e to ab600ee Compare October 16, 2023 20:35
@Fumuran
Copy link
Contributor Author

Fumuran commented Oct 16, 2023

There is a problem with formatting the final print of the memory address and value (as you mentioned it here).

I didn't find the way to parametrize the output: in case we print in decimal we need to have something like println!("{address:<10}: ...") — so we want to add space for potentially long u32 value and move it to the left side of the output. I also added dots to make it look a little bit more pretty (println!("{address:.<10}: ...")), but it is not important now. In case we want to print address in hex we use something like println!("{address:#010x}: ...")#x for hex form and 010 for making hex to be length 10 which is spaced with 0's. The problem is that I don't know how to use some kind of parameter which would specify the output appearance (.<10 in the first example and #010x in the second).

Without that parametrization the code which outputs the values would be almost identical except the parts mentioned above, so there will be a lot of copy-paste. For now I left only the decimal form for both global and local memory. I am not sure how these addresses would be used in debugging, so I don't now how crucial or convenient will be using hexes instead of decimals.

Now output looks like that:

Screenshot 2023-10-16 at 10 03 54 PM

@Fumuran Fumuran requested a review from bobbinth October 16, 2023 20:51
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.

Thank you! I left some more comments inline. Regarding formatting, I suggested how we can format addresses/indexes for mem vs. locals. Let me know if that doesn't work for some reason.

processor/src/lib.rs Outdated Show resolved Hide resolved
miden/examples/merkle_store/merkle_store.masm Outdated Show resolved Hide resolved
core/src/operations/decorators/debug.rs Outdated Show resolved Hide resolved
core/src/operations/decorators/debug.rs Outdated Show resolved Hide resolved
processor/src/host/debug.rs Outdated Show resolved Hide resolved
processor/src/host/debug.rs Outdated Show resolved Hide resolved
processor/src/host/debug.rs Outdated Show resolved Hide resolved
@Fumuran Fumuran force-pushed the andrew-expand-debug branch 2 times, most recently from e872242 to 51b3f39 Compare October 17, 2023 17:19
@Fumuran
Copy link
Contributor Author

Fumuran commented Oct 17, 2023

I thought that it's strange to restrict user to request the locals in any interval he wants (as we do it in memory), so I get rid of the check if the end of the interval is smaller than number of locals. Instead I added a check that the end of the interval is smaller than 2^16 (since it's forbidden to have more than 2^16 locals for each procedure). Using it I also implemented the "print whole local memory" variant without boolean flag, using 2^16 as an end of the interval in that case.

What do you think about this decision? Should I return everything as it was?

Now output looks like this:

Screenshot 2023-10-17 at 7 18 53 PM

@Fumuran Fumuran requested a review from bobbinth October 17, 2023 17:32
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 good! Thank you! I left a few more comments inline.

core/src/operations/decorators/debug.rs Outdated Show resolved Hide resolved
assembly/src/ast/parsers/debug.rs Outdated Show resolved Hide resolved
processor/src/host/debug.rs Outdated Show resolved Hide resolved
processor/src/host/debug.rs Outdated Show resolved Hide resolved
processor/src/host/debug.rs Outdated Show resolved Hide resolved
@bobbinth
Copy link
Contributor

What do you think about this decision?

I think it looks good! Let's keep it as you have it.

@Fumuran Fumuran requested a review from bobbinth October 18, 2023 17:51
@Fumuran Fumuran force-pushed the andrew-expand-debug branch 3 times, most recently from bb1264f to 2f1bf3c Compare October 18, 2023 23:52
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 good! Thank you! I left a few nit comments inline. After these are addressed, we can merge.

assembly/src/assembler/instruction/mod.rs Outdated Show resolved Hide resolved
assembly/src/ast/parsers/context.rs Outdated Show resolved Hide resolved
assembly/src/ast/parsers/debug.rs Outdated Show resolved Hide resolved
processor/src/host/debug.rs Outdated Show resolved Hide resolved
processor/src/host/debug.rs Outdated Show resolved Hide resolved
processor/src/host/debug.rs Outdated Show resolved Hide resolved
@Fumuran Fumuran merged commit 51e238e into next Oct 19, 2023
15 checks passed
@Fumuran Fumuran deleted the andrew-expand-debug branch October 19, 2023 13:50
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.

2 participants