Skip to content

Conversation

@ekpyron
Copy link
Collaborator

@ekpyron ekpyron commented Feb 10, 2025

I didn't think through the actual limit cases properly, so we'd need to double-check. Annoying to test SWAP257, though...
I'm marking it as ready for a first review regardless.
Depends on #15845 (merged)

FYI: I'd be happy to remove the swapn{256} and dupn{256} tests here, shrinking the PR by a few thousand lines :-).

@ekpyron ekpyron changed the title Make use of EOF's SWAPN/DUPN eof: Make use of EOF's SWAPN/DUPN Feb 10, 2025
@ekpyron ekpyron force-pushed the eofSwapnDupn branch 2 times, most recently from 3661921 to 07b1faf Compare February 10, 2025 18:43
@argotorg argotorg deleted a comment from stackenbotten Feb 10, 2025
@ekpyron ekpyron force-pushed the eofSwapnDupn branch 3 times, most recently from 716e3bb to 4ab0b35 Compare February 10, 2025 18:57
@argotorg argotorg deleted a comment from stackenbotten Feb 10, 2025
@argotorg argotorg deleted a comment from stackenbotten Feb 10, 2025
@argotorg argotorg deleted a comment from stackenbotten Feb 10, 2025
@ekpyron ekpyron changed the base branch from develop to eofAddSwapnDupn February 10, 2025 18:58
@ekpyron ekpyron force-pushed the eofSwapnDupn branch 2 times, most recently from ecc8080 to 6e06e46 Compare February 10, 2025 19:09
@ekpyron ekpyron force-pushed the eofSwapnDupn branch 4 times, most recently from 5677f51 to 47a927a Compare February 10, 2025 19:56
@cameel cameel added the has dependencies The PR depends on other PRs that must be merged first label Feb 12, 2025
@ekpyron ekpyron force-pushed the eofAddSwapnDupn branch 4 times, most recently from feb0459 to 9c42ce1 Compare February 12, 2025 17:01
// SWAP17
// [ deep v00 v01 v02 v03 v04 v05 v06 v07 v08 v09 v10 v11 v12 v13 v14 junk junk ]
// POP
// [ deep v00 v01 v02 v03 v04 v05 v06 v07 v08 v09 v10 v11 v12 v13 v14 junk ]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that starting from here this is identical to pop_early_to_avoid_swap17.stack, but, using SWAP17 above, we spent only 2 instead of 3 operations to get here.

Copy link
Collaborator Author

@ekpyron ekpyron Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, that means, I can also just use this as a target directly, which simplifies the tests quite a bit :-).
EDIT: I did that now - but kept the commit with the original larger test for now to make it clear where this is coming from. We can squash it away later.

@ekpyron ekpyron force-pushed the eofSwapnDupn branch 3 times, most recently from 5fb04a3 to cffa009 Compare February 17, 2025 21:03
// /* "":9555:9572 */
// calldataload
// /* "":9581:9594 */
// swapn{256}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how valuable this test case really is... but here we have swapn{256}... generating one more variable in the test case, correctly triggers a swap-too-deep.
Also note that we don't run any optimizer here. If we rematerialized here, the code would even become entirely swap-free (and we'd need to prevent reordering by introducing side-effects - luckily without the optimizer this remains rather straight-forward).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO better to have it than not. Sometimes unexpected things happen at the limits.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, we could make these at least a little less verbose by adding a test setting for disabling the source locations. I.e. the equivalent of --debug-info none. I'd keep them on by default, but often they don't add much and it would be nice to be able to silence them.

Maybe then we'd also get the functional formatting (i.e. calldatalad(0x0100)) to make it even more compact - not sure when that kicks in, but I suspect it may be the comments preventing it here.

Copy link
Collaborator

@cameel cameel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not yet done looking at the tests, but here's what I found so far.

The most relevant things are 3 places that still have hard-coded reachable slots. Two of them look just like heuristics that are now out of sync, but one seems to me like it might result in StackTooDeep in some cases that would be avoided otherwise.

@cameel
Copy link
Collaborator

cameel commented Feb 26, 2025

Not necessarily something to fix in this PR, but note that we may not get much coverage for this from some parts of the fuzzer, because it's hard-coded to max 15 variables:

https://github.com/ethereum/solidity/blob/eb216716830f45681df0da7641b4216a255b8946/test/tools/ossfuzz/protoToSol.h#L182-L184

@cameel
Copy link
Collaborator

cameel commented Feb 26, 2025

Looks like all the important things are fixed. I'll do one more pass to make sure I did not miss anything and to respond to comments, but I don't expect to find much to change at this point.

I'll also check if I can suggest some tests, but so far I could not come up with any over what you have already. Most changes seem like they indeed should be proven correct by the fact that the existing tests still pass. I'm not entirely sure if we have enough coverage for that, but it's hard to point out any specific cases that might be necessary.

Apart from that, two things we're not really testing are: cases where StackTooDeep does happen (but that would probably require bigger adjustments to test) and gas meter. For the latter the changes in the PR are not complicated, so we should be fine, but we should really have at least some basic coverage for this component. Maybe we should have a custom test case type that lets us import assembly items and shows the calculated cost?

@cameel cameel added the EOF label Feb 26, 2025
cameel
cameel previously approved these changes Feb 26, 2025
@ekpyron
Copy link
Collaborator Author

ekpyron commented Feb 26, 2025

Rebased and largely squashed.

@ekpyron ekpyron merged commit 5879562 into develop Feb 27, 2025
74 checks passed
@ekpyron ekpyron deleted the eofSwapnDupn branch February 27, 2025 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants