Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Allow decoding of internal function pointers in viaIR compilations (in Solidity 0.8.20) #6050

Merged
merged 20 commits into from Jun 23, 2023

Conversation

haltman-at
Copy link
Contributor

@haltman-at haltman-at commented May 17, 2023

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:

  1. The changes to internal function pointer decoding in codec/lib/basic/decode.ts,
  2. The changes to how the internal function pointer table is set up in the debugger, and
  3. The changes to how the internal function pointer table is set up in the decoder.

Let's start with (2). Now, when viaIR is set, for internalFunctionsTable, instead of using functionsByProgramCounter, we use functionsByIndex. This latter is fairly straightforward; it sets up a table based on the information in the internalFunctionIDs 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 in internalFunctionIDs, 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:

  1. 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 the befores; hopefully that's OK. If not, um, we can figure something out I'm sure.

  2. For Debugger, I relied purely on manual testing. I used the internals-ir-test in the solidity-test-cases repo. I called run, and inspected the value of pointer after each line. It worked!

@haltman-at
Copy link
Contributor Author

Note: This PR needs to be rebased against develop and updated to account for codec-components introduced in #6076.

@haltman-at
Copy link
Contributor Author

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!

haltman-at and others added 2 commits June 20, 2023 21:33
…rors.malformed-internal-function-error.tsx

Co-authored-by: cliffoo <41348973+cliffoo@users.noreply.github.com>
@haltman-at haltman-at requested a review from cliffoo June 21, 2023 01:40
Copy link
Contributor

@cliffoo cliffoo left a 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)

gnidan
gnidan previously approved these changes Jun 22, 2023
Copy link
Contributor

@gnidan gnidan left a 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 :)

@gnidan gnidan dismissed their stale review June 22, 2023 19:58

Nevermind I must be more thorough

@gnidan
Copy link
Contributor

gnidan commented Jun 22, 2023

(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 before() blocks... i.e., we should optimize for faster CI where possible nowadays)

@haltman-at
Copy link
Contributor Author

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, viair, because that's currently how decoder tests are set up, different compilation settings get different directories. (Notice this required a change to the package.json.) We should probably revisit this structure at some point, though, because it's pretty clunky.

(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.)

@haltman-at haltman-at requested a review from gnidan June 22, 2023 21:37
@haltman-at haltman-at merged commit 5483e17 into develop Jun 23, 2023
10 checks passed
@haltman-at haltman-at deleted the newtable branch June 23, 2023 17:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants