Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use side-effects of user defined functions in evm code transform. #12132

Merged
merged 2 commits into from
Nov 22, 2022
Merged

Use side-effects of user defined functions in evm code transform. #12132

merged 2 commits into from
Nov 22, 2022

Conversation

chriseth
Copy link
Contributor

@chriseth chriseth commented Oct 13, 2021 •
edited by ekpyron
Loading

I tried but failed. Assigning to @ekpyron
Depends on #12759 and #12795

@chriseth chriseth requested a review from ekpyron October 13, 2021 15:52
@chriseth chriseth changed the base branch from develop to applyControlFlowSideEffectsUserDefined October 13, 2021 15:53
@chriseth chriseth changed the title User defined side effects code transform Use side-effects of user defined functions in evm code transform. Oct 13, 2021
@chriseth chriseth force-pushed the applyControlFlowSideEffectsUserDefined branch 2 times, most recently from 3149036 to 785e16f Compare October 25, 2021 14:36
@chriseth chriseth force-pushed the applyControlFlowSideEffectsUserDefined branch 3 times, most recently from 245c277 to be6fb5f Compare November 2, 2021 10:59
Base automatically changed from applyControlFlowSideEffectsUserDefined to develop November 2, 2021 14:50
@chriseth chriseth force-pushed the userDefinedSideEffectsCodeTransform branch from 21386df to 89dbc9b Compare November 2, 2021 17:09
@ekpyron

This comment was marked as outdated.

Sign in to view
@chriseth chriseth force-pushed the userDefinedSideEffectsCodeTransform branch from 4be8f36 to 1a56588 Compare November 4, 2021 12:40
@chriseth chriseth changed the base branch from develop to nondisambiguatedsideeffects November 4, 2021 15:40
@chriseth chriseth force-pushed the userDefinedSideEffectsCodeTransform branch from 1a56588 to 7d2fc5a Compare November 4, 2021 15:49
Base automatically changed from nondisambiguatedsideeffects to develop November 4, 2021 17:01
ekpyron
ekpyron reviewed Nov 9, 2021
View reviewed changes
libyul/backends/evm/OptimizedEVMCodeTransform.cpp
@@ -490,14 +490,7 @@ void OptimizedEVMCodeTransform::operator()(CFG::BasicBlock const& _block)
createStackLayout(debugDataOf(_functionReturn), exitStack);
m_assembly.appendJump(0, AbstractAssembly::JumpType::OutOfFunction);
},
[&](CFG::BasicBlock::Terminated const&)
{
// Assert that the last builtin call was in fact terminating.
Copy link
Member

Choose a reason for hiding this comment

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

Yeah - if we wanted to, we could of course drag along the entire logic into the code transform as well and keep validating things here - but I guess it's fine to trust that it's ok.

Copy link
Member

Choose a reason for hiding this comment

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

I added at least some validation back, even though it relies on functionCall->canContinue to be correct. But conceptually that's fine - the canContinue flag is part of the CFG and the code transform only guarantees correctness relative to the CFG...

ekpyron
ekpyron previously approved these changes Nov 9, 2021
View reviewed changes
Copy link
Member

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

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

This just needs simple test updates, but is good to go otherwise, isn't it? So I'm approving modulo the tests.

No gas cost updates on this, though? Makes me wonder if this really just doesn't happen or if the gas expectations are just not properly updated and checked...

@chriseth chriseth force-pushed the userDefinedSideEffectsCodeTransform branch 2 times, most recently from 5d64479 to cf443ea Compare November 10, 2021 10:29
@chriseth
Copy link
Contributor Author

Ok this is weird - now there are gas changes...

@chriseth

This comment was marked as outdated.

Sign in to view
@chriseth
Copy link
Contributor Author

Another optimization here could be that we should not push a return tag for a call to a terminating function.

@ekpyron ekpyron force-pushed the userDefinedSideEffectsCodeTransform branch from 2e34906 to 8a85e7d Compare March 15, 2022 15:45
@ekpyron ekpyron changed the base branch from develop to evmCodeTransformAvoidPops March 15, 2022 15:46
@ekpyron ekpyron force-pushed the userDefinedSideEffectsCodeTransform branch from 8a85e7d to fcdc339 Compare March 15, 2022 15:49
@ekpyron
Copy link
Member

ekpyron commented Aug 12, 2022

@ekpyron @cameel how about this PR?

Sigh, I thought this was also already merged ages ago :-).
I'll rebase it and we can hope for another review :-).

@ekpyron ekpyron added the priority review We want this get the review of this PR to a conclusion ASAP. label Aug 12, 2022
@ekpyron ekpyron requested a review from cameel August 12, 2022 10:06
@ekpyron ekpyron dismissed a stale review via 2324a15 August 12, 2022 10:09
@ekpyron ekpyron force-pushed the userDefinedSideEffectsCodeTransform branch from 908e93a to 2324a15 Compare August 12, 2022 10:09
@Marenz Marenz force-pushed the userDefinedSideEffectsCodeTransform branch from 2324a15 to 5e63b62 Compare August 25, 2022 11:06
@Marenz
Copy link
Contributor

Marenz commented Aug 25, 2022

I rebased this and updated the gas costs

@github-actions
Copy link

This pull request is stale because it has been open for 14 days with no activity.
It will be closed in 7 days unless the stale label is removed.

@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Oct 25, 2022
@ekpyron ekpyron removed the stale The issue/PR was marked as stale because it has been open for too long. label Nov 1, 2022
@ekpyron ekpyron force-pushed the userDefinedSideEffectsCodeTransform branch 3 times, most recently from b63d4a4 to 6651394 Compare November 1, 2022 10:22
@ethereum ethereum deleted a comment from stackenbotten Nov 1, 2022
@ethereum ethereum deleted a comment from stackenbotten Nov 1, 2022
@ethereum ethereum deleted a comment from stackenbotten Nov 1, 2022
@ethereum ethereum deleted a comment from stackenbotten Nov 1, 2022
@ekpyron ekpyron force-pushed the userDefinedSideEffectsCodeTransform branch from 6651394 to 4c23d00 Compare November 1, 2022 10:37
@ekpyron
Copy link
Member

ekpyron commented Nov 1, 2022

Unstaled and rebased this once more - this is mainly in dire need of reviews.

If anyone is up for this, ping me and I can walk you trough the PR to help in reviewing.

cameel
cameel previously approved these changes Nov 10, 2022
View reviewed changes
Copy link
Member

@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.

Needs another rebase.

@chriseth
Copy link
Contributor Author

Code looks good!

@chriseth chriseth force-pushed the userDefinedSideEffectsCodeTransform branch from 4c23d00 to dd8b3f5 Compare November 14, 2022 18:00
@chriseth chriseth force-pushed the userDefinedSideEffectsCodeTransform branch from dd8b3f5 to 1b6063e Compare November 14, 2022 20:26
ekpyron
ekpyron approved these changes Nov 22, 2022
View reviewed changes
Copy link
Member

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

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

Approving based on @chriseth's approval above.

@ekpyron ekpyron merged commit 3109ce2 into develop Nov 22, 2022
@ekpyron ekpyron deleted the userDefinedSideEffectsCodeTransform branch November 22, 2022 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aarlt aarlt aarlt left review comments

@leonardoalt leonardoalt leonardoalt left review comments

@ekpyron ekpyron ekpyron approved these changes

@cameel cameel cameel left review comments

@hrkrshnn hrkrshnn Awaiting requested review from hrkrshnn

Assignees

@ekpyron ekpyron

Labels
priority review We want this get the review of this PR to a conclusion ASAP.
Projects
Solidity Focus Board
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@chriseth @ekpyron @leonardoalt @cameel @Marenz @aarlt