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

feat: small updates for steps tracing #152

Merged
merged 6 commits into from
Jun 25, 2024

Conversation

klkvr
Copy link
Contributor

@klkvr klkvr commented Jun 24, 2024

This should be enough to migrate foundry's debugger to using TracingInspector from here, I will open PR for this later today.

src/tracing/config.rs Outdated Show resolved Hide resolved
src/tracing/config.rs Outdated Show resolved Hide resolved
@klkvr klkvr requested a review from DaniPopes June 24, 2024 20:40
@klkvr
Copy link
Contributor Author

klkvr commented Jun 24, 2024

This PR conflicts with approved #84 as it relies on previous recorded step always containing memory during previous instruction. This wouldn't be true if record_opcodes_filter is configured

@DaniPopes
Copy link
Member

We could just override the filter to say that we're tracing memory opcodes if we also want memory in that case

@mattsse
Copy link
Contributor

mattsse commented Jun 25, 2024

We could just override the filter to say that we're tracing memory opcodes if we also want memory in that case

that makes sense

@klkvr
Copy link
Contributor Author

klkvr commented Jun 25, 2024

We could just override the filter to say that we're tracing memory opcodes if we also want memory in that case

hmm, perhaps we should just fallback to old behavior in that case? if user provided filter to only collect JUMPs for example it might be unexpected that we'd collect 11 other opcodes as well potentially increasing memory usage

Copy link
Contributor

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

if I'm reading this correctly, then this does not change the existing recording logic and only adds returndata recording based on opcode filter?

the overhead is negligible here

we can do @DaniPopes suggestion separately I believe

src/tracing/mod.rs Outdated Show resolved Hide resolved
src/tracing/mod.rs Outdated Show resolved Hide resolved
Comment on lines +509 to +510
/// Returndata before step execution
pub returndata: Bytes,
Copy link
Contributor

Choose a reason for hiding this comment

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

I do remember we had this at some point

Copy link
Contributor

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm, pending @DaniPopes

@klkvr
Copy link
Contributor Author

klkvr commented Jun 25, 2024

if I'm reading this correctly, then this does not change the existing recording logic and only adds returndata recording based on opcode filter?

opcode filter and returndata are separate configs

returndata is recorded in the same way as memory: if record_returndata_snapshots is true, we always save snapshot

and opcode filter is just a way to make tracer record only specific steps to optimize memory usage if we're only interested in a small subset of steps. e.g. this is the case for internal fns tracking in foundry where we only want JUMPs and JUMPDESTs along with their stack and memory snapshots

@mattsse mattsse merged commit 3c6c0a3 into paradigmxyz:main Jun 25, 2024
11 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.

3 participants