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.

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

Modifiers add to stack, discourages modifier usage #3060

Closed
redsquirrel opened this issue Oct 10, 2017 · 10 comments
Closed

Modifiers add to stack, discourages modifier usage #3060

redsquirrel opened this issue Oct 10, 2017 · 10 comments
Labels
closed due inactivity The issue/PR was automatically closed due to inactivity. stale The issue/PR was marked as stale because it has been open for too long.

Comments

@redsquirrel
Copy link
Contributor

I assume there's nothing that can be done about this, but I was surprised to see that the following contract doesn't compile because each modifier is added to the stack:

pragma solidity ^0.4.17;

contract Testing {
    modifier a(uint256 x) {
        require(true);
        _;
    }

    function foo(uint256 x)
        public
        a(x)
        a(x)
        a(x)
        a(x)
        a(x)
        a(x)
        a(x)
        a(x)
        a(x)
        a(x)
        a(x)
        a(x)
        a(x)
        a(x)
        a(x)
    {
        uint256 aa = 22 + x;
    }
}

Error:

browser/Testing.sol:27:27: CompilerError: Stack too deep, try removing local variables.
        uint256 aa = 22 + x;
                          ^

For methods with many modifiers and a lot of variables, it means that people will need to write functions to do their checking instead of modifiers in order to work around stack depth issues. Like this:

pragma solidity ^0.4.17;

contract Testing {
    function a(uint256 x) {
        require(true);
    }

    function foo(uint256 x) public {
        a(x);
        a(x);
        a(x);
        a(x);
        a(x);
        a(x);
        a(x);
        a(x);
        a(x);
        a(x);
        a(x);
        a(x);
        a(x);
        a(x);
        a(x);
        uint256 aa = 22 + x;
    }
}

Is this expected behavior?

@federicobond
Copy link
Contributor

Yes, that is expected behavior. Since modifier wraps the function like a decorator, the variable x which is pushed into the stack at the beginning of the call to foo must remain available until the _; returns.

You showcase an interesting case though, and I think there is a simple optimization to be made here, which is to pop modifier local variables when the _ statement is in tail position.

@federicobond
Copy link
Contributor

There is a more general case to be made for incorporating liveness analysis into the compiler, as long as it can intelligently balance stack/gas tradeoffs.

@chriseth
Copy link
Contributor

We are not planning any such optimizations at the high level language layer, since the benefit / danger - ratio is just too low.

@axic
Copy link
Member

axic commented Oct 11, 2017

I think it is good to keep this open as a reminder. With the new IR it should be possible to move some of the arguments into memory to accommodate this (or evm1.5/ewasm would actually allow way more arguments).

@axic axic reopened this Oct 11, 2017
@axic
Copy link
Member

axic commented Jul 30, 2018

@leonardoalt @chriseth @ekpyron I think with the new scoping rules this could be improved in certain cases.

@chriseth
Copy link
Contributor

chriseth commented Aug 3, 2018

Not really, because modifiers generate a block that surrounds the function.

@hshar7
Copy link

hshar7 commented Aug 19, 2020

My code is now a lot uglier because of this

@wminshew
Copy link

wminshew commented Dec 28, 2021

I know this is low priority but feel like it should be re-considered. At least in the case where _; comes at the tail-end..

@github-actions
Copy link

This issue has been marked as stale due to inactivity for the last 90 days.
It will be automatically closed in 7 days.

@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Feb 13, 2023
@github-actions
Copy link

Hi everyone! This issue has been automatically closed due to inactivity.
If you think this issue is still relevant in the latest Solidity version and you have something to contribute, feel free to reopen.
However, unless the issue is a concrete proposal that can be implemented, we recommend starting a language discussion on the forum instead.

@github-actions github-actions bot added the closed due inactivity The issue/PR was automatically closed due to inactivity. label Feb 21, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed due inactivity The issue/PR was automatically closed due to inactivity. stale The issue/PR was marked as stale because it has been open for too long.
Projects
No open projects
Development

No branches or pull requests

7 participants