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(debugger): Highlight memory region for any instruction #5940

Merged
merged 2 commits into from
Oct 4, 2023

Conversation

soyccan
Copy link
Contributor

@soyccan soyccan commented Sep 28, 2023

Highlight memory region for any instruction in debugger

Motivation

Currently, debugger highlights the 32-byte memory word that is being read/write in cyan/red when an
MLOAD/MSTORE is encountered, respectively.

But there are quite a few instructions that access memory, e.g. CALL, CALLDATACOPY, SHA3...
This update is to colorize the memory region (which may be of any size) for all of them.

This is an enhancement relevant to #4440. A related work #4681 was to highlight unaligned memory.

Solution

All instructions that access memory are considered. Memory regions are highlighted in cyan/red for
read/write, consistent with MLOAD/MSTORE. Also, memory regions that are just written by the previous
instruction is highlighted in green. Memory regions may be any length.

image

@soyccan soyccan changed the title Highlight memory region for any instruction in debugger feat(debugger) Highlight memory region for any instruction in debugger Sep 28, 2023
@soyccan soyccan changed the title feat(debugger) Highlight memory region for any instruction in debugger feat(debugger): Highlight memory region for any instruction in debugger Sep 28, 2023
@soyccan soyccan changed the title feat(debugger): Highlight memory region for any instruction in debugger feat(debugger): Highlight memory region for any instruction Sep 28, 2023
Copy link
Member

@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.

cool, I like this.

didn't review all the offsets in detail, but I'm willing to try this. changes lgtm and should be easy to fix if something's wrong.

smol doc nit

crates/debugger/src/lib.rs Outdated Show resolved Hide resolved
crates/debugger/src/lib.rs Show resolved Hide resolved
crates/debugger/src/lib.rs Outdated Show resolved Hide resolved
Move some in-function comments to the be function docs.

Since now not only a memory word but a region of memory is highlighted,
we changed some var name to be clear.
@soyccan soyccan requested review from Evalir and mattsse October 4, 2023 13:38
Copy link
Member

@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, tysm

@mattsse mattsse merged commit 39eee9e into foundry-rs:master Oct 4, 2023
16 checks passed
@soyccan soyccan deleted the memhl branch December 3, 2023 16:53
soyccan added a commit to soyccan/foundry that referenced this pull request Dec 3, 2023
The debugger colors memory region for a variety of instructions that
access the memory, as described in foundry-rs#5940. But there is a potential
underflow if the size is 0 (where offset + size - 1 underflows).
Change to a simpler and more robust way to index the memory region.
DaniPopes added a commit that referenced this pull request Dec 4, 2023
The debugger colors memory region for a variety of instructions that
access the memory, as described in #5940. But there is a potential
underflow if the size is 0 (where offset + size - 1 underflows).
Change to a simpler and more robust way to index the memory region.

Co-authored-by: DaniPopes <57450786+DaniPopes@users.noreply.github.com>
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