-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Allow decoding of internal function pointers in viaIR
compilations (in Solidity 0.8.20)
#6050
Conversation
Note: This PR needs to be rebased against |
OK, I've updated this PR with the new components. I haven't currently tested the new components, they should probably be tested at some point before merging this. @cliffoo I've added you as reviewer on this for obvious reasons! |
...mponents/src/react/components/codec/format.errors.deployed-function-in-constructor-error.tsx
Show resolved
Hide resolved
...ec-components/src/react/components/codec/format.errors.malformed-internal-function-error.tsx
Outdated
Show resolved
Hide resolved
...odec-components/src/react/components/codec/format.errors.no-such-internal-function-error.tsx
Show resolved
Hide resolved
...components/src/react/components/codec/format.values.function-internal-value-info-unknown.tsx
Show resolved
Hide resolved
...dec-components/src/react/components/codec/format.values.function-internal-raw-info-index.tsx
Outdated
Show resolved
Hide resolved
...c-components/src/react/components/codec/format.values.function-internal-raw-info-pc-pair.tsx
Outdated
Show resolved
Hide resolved
…rors.malformed-internal-function-error.tsx Co-authored-by: cliffoo <[email protected]>
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.
components lgtm! (sorry didn't see the re-review request)
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.
Looks reasonable... I think we can get this in :)
(Not a full thorough review, but in discussing this with @haltman-at, I think we should have a separate test so that we don't increase timeouts for |
OK, I redid the tests. I didn't want to go changing the structure of how the decoder tests work, so since these tests need special compilation settings, I just put them in a new directory, (I did also update the other tests to 0.8.20 while I was at it, though. Although, hm, not the ENS ones -- let me go do that real quick.) |
That's right, this branch is back from the grave! I rebased it against develop and made the necessary changes to make it work. (Also: This PR addresses issue #5466.)
This PR is a follow-up to #5596. Originally that PR was going to do what this one did, but, oops, that turned out to be impossible (OK, not impossible necessarily, but too difficult to be worth it). But now Solidity 0.8.20 is out, and it provides us with the extra debugging information we need to decode
viaIR
-style internal function pointers!(That means that yes substantial parts of this PR I wrote back in October, I'm going to have to remember them... >_> )
Now, the existing output format for internal function pointers was... really not built to handle this. So, uh, I had to make a breaking change to the codec output format there. Hence why this is a breaking change to three packages. But, it only affects the part of the format regarding internal function pointers; everything else remains unaffected. (Obviously I updated all inspectors, etc, appropriately.)
Anyway, what's going on here?
This PR builds on the earlier #5596. That PR allowed us to detect, in the decoder and debugger, whether
viaIR
was set, and turn off internal function pointer decoding if so. This PR turns it back on in that case, but of course it works differently than with it off. Instead of being based on using source maps to decode program counter values, it's based on looking up an index in a table Solidity gives us.Ignoring the changes to the format and inspectors (and tests), we can break this down into three parts:
codec/lib/basic/decode.ts
,Let's start with (2). Now, when
viaIR
is set, forinternalFunctionsTable
, instead of usingfunctionsByProgramCounter
, we usefunctionsByIndex
. This latter is fairly straightforward; it sets up a table based on the information in theinternalFunctionIDs
node. Note that the information is given to us in the form{ [nodeId]: functionIndex }
, but that's fine. Remember, index 0 is reserved for the designated invalid function! (This isn't included ininternalFunctionIDs
, we just put that in ourselves.)However we now also pass to Codec something new, which is
internalFunctionsTableKind
. We now have to tell Codec whether we're decoding based on PC pairs or based on indices. Ideally this would not be separate from the table itself, as the two pieces of information go together, but, well, I didn't want to change too much.What goes on in (3) is similar, except that we don't have access to all the nice information that Debugger does, so the code is a bit messier as it has to do searches instead of just looking things up in a giant object. Oh well. Also I omitted the pointer information because it was too much trouble to determine and we don't use it anyway, that was just some future-proofing. :P
Finally let's take a look at (1). (Ugh... this is the part I wrote back in October.) You know what? I don't have a lot to say about this actually, but I did have to move some of the cases around... like, I had to move earlier the check to see if both constructor and deployed PCs are zero (when we're in the
"pcpair"
case). Anyway yeah the new code handles both cases, but y'know ultimately we're basically looking up stuff in a table.Um, I hope that's a good enough description... if it's not, I'm sure I'll get questions in review. :P
Testing instructions
As with #6049, because I was trying to get this up quickly, I tested it less than I otherwise might have. Basically my tests consisted of the following:
For Decoder, I relied on the automated tests; I set the version to 0.8.20 and turned on
viaIR
. One test had to be updated as a result. Note that doing this substantially increased compilation times, so I had to turn up the timeouts on thebefore
s; hopefully that's OK. If not, um, we can figure something out I'm sure.For Debugger, I relied purely on manual testing. I used the
internals-ir-test
in thesolidity-test-cases
repo. I calledrun
, and inspected the value ofpointer
after each line. It worked!