-
Notifications
You must be signed in to change notification settings - Fork 151
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
Tracer #1265
Tracer #1265
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I 100% love this ❤️
Please do check the comments tho. As a general observation, we try to be conservative about exposing internal representations (this helps avoid future breakage) and about panic
s (e.g. due to unwrap
). Could you check those? I saw a few of them.
Thanks! I will work on this and try to push an update in the next few days! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😮 This tracer is a piece of art! Thanks for your work!
I have made the fixes mentioned, let me know if there's anything else you would like me to fix :) |
We will check this tomorrow |
Awesome! I just noticed there were some conflicts, have fixed those! |
docs/tracer/README.md
Outdated
@@ -0,0 +1,39 @@ | |||
# Cairo Tracer | |||
|
|||
Cairo-rs offers a tracer which gives you a visualization of how your memory and registers change line after line as the VM executes the code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should replace cairo-rs with cairo-vm that is the new name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work!
I love this tracer, it will be really useful for debugging!
I just left a couple of comments, you can check them.
Plus we need to run Cairo 1 contracts with the tracer, is this possible? maybe we can add this in another feature? Please tell me what you think
If you have any problem with the CI or want to discuss something please contact me throw telegram, we can arrange a call to
vm/src/vm/vm_core.rs
Outdated
@@ -905,6 +905,10 @@ impl VirtualMachine { | |||
self.trace = None | |||
} | |||
|
|||
pub fn get_relocation_table(&self) -> Result<Vec<usize>, MemoryError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we adding this?
I think it is not necessary, plus the get_ method name can be misunderstood since it is not just a get, it relocates the memory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the tracer needed the relocated segments but segments
was public only inside the crate. I have renamed the function now to relocate_segments
and added a #[cfg(feature = "with_tracer")]
flag as well.
cairo-vm-tracer/src/tracer_data.rs
Outdated
|
||
#[derive(Clone)] | ||
struct TextMark { | ||
_line_start: usize, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we delete the _, or Clippy doesn't allow it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a link to this file in the main README.md so everyone can see it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Thanks @pefontana! This tracer should work for Cairo 1 as well as long as we generate the trace and memory records and also the DebugInfo. I am not very sure how to generate the DebugInfo for Cairo 1 and casm/sierra don't have anything like this as far as I know. Ideally, we should create a way to launch this tracer from the command line, so we can just give the debug info, trace binary and memory binary and the tracer will start. The tracer wouldn't need to know if the program was Cairo 0 or Cairo 1. Let me know what you think about this? I am currently a little busy with some things for StarketCC but I can try to pick this up later when I get time :) |
Great one @apoorvsadana ! |
Sure, it's fixed @pefontana |
Nice @apoorvsadana ! If you have any trouble we can see it together so, we can merge this ASAP. Also, have you checked running a Cairo 1 or Cairo 2 contract with the tracer? |
Hey @pefontana, sorry for the late reply, I was travelling for the Starknet HH. I am in Paris now, I will try to pick this up as soon as I get time! Sorry for the delay |
There seems to be quite a few untested lines according to Codecov. I'll let @pefontana decide whether we require that, but my advice is that we do. There's also a few lints reported by clippy as well. |
e3e7e37
@apoorvsadana Are you able to solve the merge conflicts so we can merge it? |
Hey @pefontana, really sorry for the delay, I have moved this to a new PR - #1643 now as it was easier for me. Closing this one now. |
TITLE
Description
cairo-vm-tracer
which starts a server on the localhost where you can see an web UI. The UI allows you to interact with your Cairo programs and see what's happening inside the VM.with_tracer
andtracer
Checklist