-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Add support for PUSH0 instruction to debugger & disassembler #5898
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.
I left a nit and an observation, but nothing to prevent me from hitting approval.
@@ -75,6 +75,7 @@ const codes = { | |||
0x5b: "JUMPDEST", | |||
|
|||
// 0x60 & 0x70 range - pushes | |||
0x5f: "PUSH0", |
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.
The comment above is confusing.
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.
Yeah, didn't really know what to change it to though... (thoughts?)
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.
Beginning of push op codes
maybe?
const pushAddressInstruction = (0x60 + Evm.Utils.ADDRESS_SIZE - 1).toString( | ||
16 | ||
); //"73" | ||
const pushAddressInstruction = (0x5f + Evm.Utils.ADDRESS_SIZE).toString(16); //"73" |
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.
Note, this calculation is used in three places across two modules, adding error-prone maintenance complexity.
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.
There's not really any maintenance complexity here? Like there's nothing here that needs to be updated. I just changed the base of the calculation from PUSH1
to PUSH0
now that the latter exists... what would change it further in the future?
Note of course I could have just used a literal "73"
but I figured that doing the calculation made the intent clearer.
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.
I meant that if a new opcode is updated to be base, future maintainers would have three places to update. However, what can come before PUSH0, as you pointed out? :)
Just going to go ahead and merge this to make sure it makes it into the release... @cds-amal, if you want to change the comments to be more accurate, well, uh, you can do that? :P |
This PR adds support for the upcoming PUSH0 instruction to Truffle's debugger and disassembler. I'm doing this now because I'm going to be gone in March and this sort of thing usually falls to me, so, why not get it done before I leave.
This PR has 3 commits. The first commit adds support for the opcode. Note I grouped it with the other PUSHes rather than with the rest of the 0x50's.
The second commit makes it so that, when printing out PUSH0 in the debugger or disassembler, we just print out
PUSH0
, notPUSH0 0x
, because the latter would look ugly.The third commit is purely a code style matter -- I rewrote some calculations that previously used 0x60 (
PUSH1
) as a base to now use 0x5f (PUSH0
) as a base.NOTE: I didn't really test this (beyond existing CI) because Shanghai's not out yet...