Skip to content

Track dbg! calls #293

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

Merged
merged 1 commit into from
May 6, 2025
Merged

Conversation

m-kus
Copy link
Contributor

@m-kus m-kus commented May 1, 2025

This PR extends the ExecTracker trait to support tracking debug calls (identity nodes generated by dbg! macro in Simfony). It is enabled by feature debug to avoid the decoding overhead by default.

Ported from https://github.com/uncomputable/simfony-webide/blob/adb60d4e60c0d6b019ea13f64b1a166da3467d8d/src/function.rs#L144

Example of usage: https://github.com/keep-starknet-strange/stark-symphony/blob/37b0a1892c69ea3b0f2d1c8088563e7fae7c3825/simfony-cli/src/tracker.rs#L65

Sample output
image

@m-kus m-kus force-pushed the feat/track-dbg-nodes branch 4 times, most recently from d466ab0 to 7c43d07 Compare May 1, 2025 12:02
@apoelstra
Copy link
Collaborator

In 7c43d07:

Rather than feature-gating it, can you add a new trait method enable_track_debug(&self) -> bool which has a default impl of false, and gate the expensive code on that?

Also sorry for the late review -- I assumed from the name that this PR was much bigger than it was!

@m-kus m-kus force-pushed the feat/track-dbg-nodes branch from 7c43d07 to 2806157 Compare May 5, 2025 17:55
@m-kus
Copy link
Contributor Author

m-kus commented May 5, 2025

Done!

Haha no worries, minimised footprint was the goal :)


fn is_track_debug_enabled(&self) -> bool {
// Set flag to test frame decoding in unit tests
cfg!(test)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not ideal, but I don't have a better idea

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hehe, I think it's ok. Agree that it looks pretty weird, but the nice thing is that cfg(test) is only ever set within this project, so we don't need to worry about this weirdness somehow being exposed to our dependencies.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess the "right" way to do it is to add a whole nother Tracker type for use in tests. But I think this is fine for now.

Copy link
Collaborator

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 2806157; successfully ran local tests

@apoelstra apoelstra merged commit 567d88e into BlockstreamResearch:master May 6, 2025
24 checks passed
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