Refactor hardcoded reachable stack depth#16467
Conversation
|
Thank you for your contribution to the Solidity compiler! A team member will follow up shortly. If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother. If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix. |
|
@r0qs How should this PR deal with this part you mentioned in #16424 (review): solidity/libyul/backends/evm/ssa/Stack.h Line 164 in e98ec36 |
You can ignore that in this PR. Thanks for bringing it up! |
r0qs
left a comment
r0qs
left a comment
There was a problem hiding this comment.
Thanks a lot for the PR @frangio. I’ve taken a look and left a few comments.
I think we may also need to update a couple of additional files that are currently missing:
-
solidity/libevmasm/GasMeter.cpp
Line 314 in 9dc6b2e
Here we could usereachableStackDepth()from_evmVersion. -
For this one, I’m a bit on the fence. It would likely require a larger refactor, sinceCSECodeGeneratordoesn’t currently have access toEVMVersion, we would need to make it available there.
| } | ||
|
|
||
| template<StackManipulationCallbackConcept Callback, std::size_t ReachableStackDepth=16> | ||
| template<StackManipulationCallbackConcept Callback, std::size_t ReachableStackDepth> |
There was a problem hiding this comment.
As mentioned in #16467 (comment), I think it would be fine to ignore these changes in the ssa-cfg pipeline for now.
Although, on second thought, this refactor, unlike the one in #16424 could probably be applied to the ssa-cfg pipeline as well, no @clonker? I’m just not sure how much effort that would take, but it seems to be only the StackShuffler and its tests.
There was a problem hiding this comment.
It seems a little safer and relatively harmless to remove this constant in this PR. What do you think?
There was a problem hiding this comment.
Ok, it was not harmless because it was causing a bunch of compilation errors. 🙂 I've reverted this because I noticed it requires a larger refactor once the reachable stack depth is no longer a static value that can be passed by template parameter.
There was a problem hiding this comment.
since it's all experimental in that part of the code (the whole ssa subdirectory) i suggest we leave it as-is right now. i want to rethink bits of the shuffler, too, provided we have these new opcodes. should open up interesting moves.
clonker
left a comment
clonker
left a comment
There was a problem hiding this comment.
some more comments, beyond that it looks good to me:)
| AssemblyItem invalidTag(PushTag, u256(-0x10)); | ||
| state->feedItem(invalidTag, true); | ||
| if (parametersSize > 0) | ||
| state->feedItem(swapInstruction(parametersSize)); |
There was a problem hiding this comment.
My guess is we'll comb through every occurence of dupInstruction, swapInstruction, and reachableStackDepth.
clonker
left a comment
clonker
left a comment
There was a problem hiding this comment.
Looks good to me now, what's from my point of view is left to do for this PR is cleaning up the commit history a bit (squash, rebase against develop so we get rid of merge commits).
frangio
force-pushed
the
refactor-stack-limit
branch
from
5c21af3 to
fbc707e
Compare
|
Squashed and rebased! |
clonker
force-pushed
the
refactor-stack-limit
branch
from
fbc707e to
90eb849
Compare
4 participants
Preparation for #16424