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

Tracer #1265

Closed
wants to merge 22 commits into from
Closed

Tracer #1265

wants to merge 22 commits into from

Conversation

apoorvsadana
Copy link
Contributor

TITLE

Description

  • Track the ap, fp, pc and the state of the memory in each step of the execution.
  • A new crate 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.
  • Most functionality in existing crates has been put behind feature flags with_tracer and tracer

Checklist

  • Linked to Github Issue
  • Unit tests added
  • Integration tests added.
  • This change requires new documentation.
    • Documentation has been added/updated.
    • CHANGELOG has been updated.

Copy link
Member

@Oppen Oppen left a 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 panics (e.g. due to unwrap). Could you check those? I saw a few of them.

cairo-vm-cli/Cargo.toml Outdated Show resolved Hide resolved
cairo-vm-cli/src/main.rs Outdated Show resolved Hide resolved
cairo-vm-cli/src/main.rs Outdated Show resolved Hide resolved
cairo-vm-tracer/Cargo.toml Outdated Show resolved Hide resolved
vm/src/serde/deserialize_program.rs Outdated Show resolved Hide resolved
vm/src/serde/deserialize_program.rs Outdated Show resolved Hide resolved
vm/src/types/program.rs Outdated Show resolved Hide resolved
vm/src/vm/context/run_context.rs Outdated Show resolved Hide resolved
vm/src/vm/vm_core.rs Outdated Show resolved Hide resolved
@apoorvsadana
Copy link
Contributor Author

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 panics (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!

Copy link
Contributor

@MegaRedHand MegaRedHand left a 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!

cairo-vm-tracer/static/tracer.js Outdated Show resolved Hide resolved
Oppen
Oppen previously approved these changes Jun 30, 2023
.gitignore Outdated Show resolved Hide resolved
@apoorvsadana
Copy link
Contributor Author

I have made the fixes mentioned, let me know if there's anything else you would like me to fix :)

@unbalancedparentheses
Copy link
Member

We will check this tomorrow

@apoorvsadana
Copy link
Contributor Author

Awesome! I just noticed there were some conflicts, have fixed those!

@@ -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.
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

juanbono
juanbono previously approved these changes Jul 11, 2023
pefontana
pefontana previously approved these changes Jul 11, 2023
Copy link
Member

@pefontana pefontana left a 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

@@ -905,6 +905,10 @@ impl VirtualMachine {
self.trace = None
}

pub fn get_relocation_table(&self) -> Result<Vec<usize>, MemoryError> {
Copy link
Member

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

Copy link
Contributor Author

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.


#[derive(Clone)]
struct TextMark {
_line_start: usize,
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Member

@pefontana pefontana Jul 11, 2023

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@apoorvsadana
Copy link
Contributor Author

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 :)

@pefontana
Copy link
Member

Great one @apoorvsadana !
Can you solve the cargo.toml conflicts and we will merge it!

@apoorvsadana
Copy link
Contributor Author

Sure, it's fixed @pefontana

@pefontana
Copy link
Member

Sure, it's fixed @pefontana

Nice @apoorvsadana !
The PR is having some problems with the CI, can you please check them.
The make clippy command is failing, no-std and wasm tests too, you can check all the CI errors here.

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?
If it is possible it will be good to add an example so you can complete the bounty! As we talk earlier, you can take this on other PR, but tell us if you need any help!

@apoorvsadana
Copy link
Contributor Author

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

@Oppen
Copy link
Member

Oppen commented Aug 10, 2023

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.

@apoorvsadana apoorvsadana dismissed stale reviews from pefontana, juanbono, and Oppen via e3e7e37 December 18, 2023 22:47
@pefontana
Copy link
Member

pefontana commented Mar 1, 2024

@apoorvsadana Are you able to solve the merge conflicts so we can merge it?

@apoorvsadana
Copy link
Contributor Author

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.

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.

None yet

6 participants