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

Allow decoding while debugging Yul sources in Solidity 0.8.21 (and related changes) #6154

Merged
merged 5 commits into from
Aug 4, 2023

Conversation

haltman-at
Copy link
Contributor

@haltman-at haltman-at commented Jul 26, 2023

This PR allows decoding of variables while decoding Yul sources that were compiled with solc 0.8.21 or greater. Note this PR does not address #3831, I figured that was better saved for later.

It was already possible to decode Yul variables in Solidity assembly blocks, but not in straight Yul files, because Solidity wouldn't give us ASTs for the latter. But now it does!

Well, now it does if we ask for them right. For some reason, Solidity makes us ask for the ast at the contract level when compiling Yul, instead of at the file level like normal; I've filed an issue about that (although for 0.8.21 compatibility we'll have to leave this extra bit in anyway). Also, there's another bug regarding these (which I've also filed an issue about) -- it assigns the singular source an ID of 1 instead of 0. So, I had to add in a workaround for that as well.

Once we have the ASTs, the debugger can do the rest, so not many more changes were needed! But there are two.

First off, it turns out that the top-level node in a Yul AST, the YulObject node type, doesn't have an src field. So the code in Codec that gets the source index from the top node of an AST needed some adjustment in that case. Not only does the YulObject node lack this info, but so does the YulCode node below it; one has to go to the YulBlock node at ast.code.block to get it. Fortunately these nodes are always present -- I tried. (Of course we could also just always return 0, but hey, if the info is there let's use it, right? :P )

Second off, since an AST might now have YulObject rather than SourceUnit as its top-level node, a bunch of code in data that assumed that SourceUnit is the only type of top-level node has been updated to account for YulObject as well. Some of this got factored out into helpers.

Finally, I added a test! One caveat -- the test did not work right once I introduced mutation. O_o I assume this is due to something with the optimizer or something? I decided to just omit that from the test, which is not great, but I didn't see what else I could reasonably do.

Note that adding this test obviously involved increasing the Solidity version used in the debugger tests to 0.8.21, and for some reason that slowed down a few tests, so I increased the timeout on those few.

I also tested this manually using the yul-test project in the solidity-test-cases repo.

@haltman-at
Copy link
Contributor Author

Blech, #6157 is a substantial caveat on this PR it turns out... I still think this should be merged, because it's ridiculous for us to not get the AST, but it does mean things are rarely going to work properly in this case... :-/

@cds-amal
Copy link
Member

I don't think we should present incorrect values. As a compromise, what if we get the AST, but don't use it until #6157 is resolved?

@haltman-at
Copy link
Contributor Author

I mean, I'm not sure there's really anything we can do to fix #6157. Moreover, Solidity is fixing the bug that made us have to do extra work to get the Yul AST, so once that's fixed and released, it won't be a question of whether we'll be getting the Yul AST, because we will, without having to do extra work!

I guess the only question really is whether we deliberately ignore Yul ASTs, or something...

@haltman-at
Copy link
Contributor Author

OK, Nick thinks this is fine to merge as-is, so I'm going to do that! But I will go bug the Solidity team like Amal says...

@haltman-at haltman-at merged commit 1f49f0a into develop Aug 4, 2023
10 checks passed
@haltman-at haltman-at deleted the debug-0821 branch August 4, 2023 20:22
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.

3 participants