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

IR-based compilation w/o optimization uses way too many stack slots, easily hits stack too deep errors #12533

Closed
Tracked by #13720
haltman-at opened this issue Jan 14, 2022 · 18 comments
Labels
high effort A lot to implement but still doable by a single person. The task is large or difficult. high impact Changes are very prominent and affect users or the project in a major way. must have Something we consider an essential part of Solidity 1.0. needs design The proposal is too vague to be implemented right away optimizer viair

Comments

@haltman-at
Copy link
Contributor

haltman-at commented Jan 14, 2022

Description

Compiling Solidity with viaIR: true but the optimizer turned off results in bytecode that uses an enormous number of stack slots and runs into "stack too deep" errors with high frequency, in situations where the Solidity compiler would normally have plenty of stack space left (see repro example below).

Basically what's going on here seems to be that, whenviaIR is set to true, the Solidity compiler generates code that doesn't discard intermediate results after it's done with them. Ordinarily, Solidity divides each stackframe into a lower area for local variables, and a higher area for working state, and the latter only sticks around as necessary. But with viaIR: true, it's all mixed together, and none of it is discarded promptly.

For instance, with the code below (but with the lines about w removed so that it actually compiles with viaIR: true), the line uint x = 1, which would normally result in a single 1 being placed on the stack, in the slot representing x, instead results in two 1s being placed on the stack; presumably, one for the literal and one for x, and the one for the literal then doesn't go away. All sorts of other intermediate results just remain on the stack as well, even after there's no more need for them, such as the value of x+y, the signature of Num (multiple times), and the literal 88.

And, of course, one result of using so many stack slots and not discarding anything is that it's really to hit stack too deep errors, in situations where the Solidity compiler wouldn't normally be anywhere near such an error.

Thanks!

Environment

  • Compiler version: 0.8.11
  • Target EVM version (as per compiler settings): default
  • Framework/IDE (e.g. Truffle or Remix): Truffle

Steps to Reproduce

Here's an exampe of a contract that won't compile with viaIR on due to stack-too-deep errors:

//SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

contract StackTest {

  event Num(uint);

  function doStuff() public {
    uint x = 1;
    uint y = 2;
    uint z = 3;
    uint w = 4;
    emit Num(w);
    emit Num(z);
    emit Num(x);
    emit Num(y);
    emit Num(x + y);
    emit Num(88);
  }
}

Compiling with viaIR: true yields the error message:

YulException: Variable var_x_42 is 3 slot(s) too deep inside the stack.

Meanwhile, with viaIR: false, this compiles just fine, because the compiler isn't wasting huge numbers of stack slots.

I'll skip including repro steps for the other stuff, but if you just take the above contract and delete the lines mentioning w, you'll get the contract I observed them with.

@ekpyron
Copy link
Member

ekpyron commented Jan 14, 2022

The IR/Yul code generation from solidity is intentionally "safe and stupid", i.e. it indeed produces a local Yul-variable for each intermediate expression (you quickly see it in compiling the contract with solc --ir).
The unoptimized code transform from Yul code to EVM code is similarly "safe and stupid" and, as you say, doesn't do any cleanup after the last use of a Yul variable.

In general, the viaIR pipeline is not intended to be run without at least some degree of optimization.

So this is not exactly unexpected or unintentional, but I agree that it's a bad situation in which the default compiler runs fail even on simple contracts - we should probably enable a minimal set of optimizations by default - however, that's something we'd want to coordinate with you, s.t. we can retain debugging.

The currently minimal amount of optimization already makes the contract compilable:

    "viaIR": true,
    "optimizer": {
      "enabled": true,
      "details": {
        "peephole": false,
        "inliner": false,
        "jumpdestRemover": false,
        "orderLiterals": false,
        "deduplicate": false,
        "cse": false,
        "constantOptimizer": false,
        "yul": true,
        "yulDetails": {
          "stackAllocation": false,
          "optimizerSteps": ""
        }
      }
    }

(although right now this will run some hard-coded Yul optimizer steps, we'll allow to disable those in the near future as well #9627)

In particular, the yulDetails option stackAllocation will have significant effects on the stack-effectiveness of the yul-to-evm transformation.

Long story short:

  • At the moment it's expected that fully unoptimized compilation via IR fails quickly due to stack-too-deep.
  • Eventually we may enable some sane minimal optimizations by default, s.t. this stack-too-deep doesn't happen by default all the time.
  • We have to figure out how we can help e.g. the Truffle debugger to be able to deal with the compilation result with and without optimization.

@chriseth
Copy link
Contributor

Would it be an option to disable any code modifications but enable the optimized evm code transforrm?

@cameel cameel added high effort A lot to implement but still doable by a single person. The task is large or difficult. high impact Changes are very prominent and affect users or the project in a major way. must have Something we consider an essential part of Solidity 1.0. needs design The proposal is too vague to be implemented right away labels Oct 3, 2022
@cameel
Copy link
Member

cameel commented Oct 6, 2022

@haltman-at @fvictorio With #9627 implemented it's now possible to disable pretty much all optimizations without disabling stack optimization. You can do this by setting --yul-optimizations (on the CLI) or
settings.optimizer.details.yulDetails.optimizerSteps (in Standard JSON) to :. An empty string for compatibility still means that the cleanup sequence is there but if you include a colon, you now change both the base sequence and the cleanup sequence. See Optimizer > Selecting Optimizations for details.

Can you check if this would be enough for you to properly support viaIR: true?

@fvictorio
Copy link
Contributor

I get this error if I do that:

Invalid optimizer step sequence in "settings.optimizer.details.optimizerSteps": ':' is not a valid step abbreviation

@cameel
Copy link
Member

cameel commented Oct 11, 2022

Ah, wait. My bad. I was convinced that this feature already went into 0.8.17 but I now realized that we only merged it shortly after the release. Sorry for the confusion.

In that case maybe you could try one of the recent builds from our CI? E.g. this one from develop.

@leonardoalt
Copy link
Member

I'm sure optimizerSteps is already there, I've been using "optimizerSteps: '' " for a while. I think he's missing yulDetails in the field chain.

@cameel
Copy link
Member

cameel commented Oct 11, 2022

Yeah, optimizerSteps is there but in 0.8.17 it's only the part of the sequence that happens before the stack compressor. The new feature lets you put : in it and replace the cleanup sequence that happens after the stack compression.

Actually, it could be that clearing the main sequence is already good enough for Hardhat - might be worth a try - but still, it's not clearing the whole sequence.

@fvictorio Also, one thing I forgot to mention is that you'll probably want to disable the EVM-based optimizer - clearing the sequence does not automatically do that. You obviously can't just set settings.optimizer.enabled to false so you need to disable the individual components under settings.optimizer.details instead.

@alcuadrado
Copy link
Member

I mentioned this at our meeting in Bogota, but I want to bring it up again: can viaIR: true "optimizer": {"enabled": false } be an alias to "only do variable spilling"? I think that's the behavior most users will expect.

@cameel
Copy link
Member

cameel commented Oct 27, 2022

Not sure if "optimizer": {"enabled": false} is the best way to achieve that but I do think that an easier way to enable this mode is a good idea.

But before we do that, I think we should first hear from you whether this mode is actually enough to work around the root issue here.

@ekpyron
Copy link
Member

ekpyron commented Oct 27, 2022

I actually misunderstood you in Bogota on this then and thought you wanted something more along the lines of what @cameel proposes, i.e. an independent simple way to enable "only stack-to-memory, no other optimizations".
But you do have a point in that it would actually make sense for viaIR: true and "optimizer": {"enabled": false } itself to behave like that - since it's a prerequisite for compiling most non-trivial code bases in general.
So I'm actually in favour of exploring this - however, we still need some minimal optimization preprocessing like disambiguating and function hoisting for stack-to-memory - so we'd need to check, if we can either avoid those or want to accept this level of messing with the plain unoptimized yul code. It's worth a thought in any case!

@cameel
Copy link
Member

cameel commented Oct 27, 2022

We could also introduce explicit optimization levels. Internally enabled: false does not even mean completely disabling the optimizer. It's just the "minimal" level. Which has almost everything disabled but is still a notch above "none".

@alcuadrado
Copy link
Member

I actually misunderstood you in Bogota on this then and thought you wanted something more along the lines of what @cameel proposes, i.e. an independent simple way to enable "only stack-to-memory, no other optimizations".

No misunderstanding, that's what I want. But then it came to my mind that it could be viaIR: true with enabled: false, precisely because of what you mentioned "since it's a prerequisite for compiling most non-trivial code bases in general.".

@fvictorio
Copy link
Contributor

I found a very weird bug when using optimizerSteps: "" (it thinks that a contract needs a linked library, when it shouldn't). Is this worth reporting? Or is the empty string for optimizerSteps something that shouldn't be used?

For the record, this does not happen with the nightly version and optimizerSteps: ":"

@ekpyron
Copy link
Member

ekpyron commented Dec 7, 2022

@fvictorio Best to report it - I'd not consider an empty optimizer step sequence common, but it should still work.
If ":" works, it may be an issue in the cleanup optimizer sequence without running the full suite.

@ekpyron
Copy link
Member

ekpyron commented Dec 7, 2022

@fvictorio What probably happens is that without optimizer, any mention of a library as an expression (including expressions that will merely result in an internal call) will result in a variable being assigned the address of the library - that variable will be unused and eliminated by the optimizer, but without the optimizer it will remain, so you'll end up with an unresolved link reference, even though it's effectively unused. However, out of my head I'd actually expect this to only happen with ":", while "" may actually still remove this, but I can look into it based on a concrete case.

@fvictorio
Copy link
Contributor

fvictorio commented Dec 7, 2022

I reported it here: #13786

@fvictorio
Copy link
Contributor

More information on this:

Using a nightly version and optimizerSteps: ":", we still get some test failures. As far as I can tell, they are all related to having less descriptive sourcemaps.

Particularly important for us is that the jump types are not as useful with the IR codegen. For example, given this contract:

contract C {

  uint i = 0;

  function test() public {


    revert();
    i += 1;
  }

}

If we call the test function, we expect one of the executed opcodes to have a corresponding sourcemap with an "into function" j value. But all the jumps in that execution are marked as internal jumps.

@NunoFilipeSantos
Copy link
Contributor

This was solved by #13972 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high effort A lot to implement but still doable by a single person. The task is large or difficult. high impact Changes are very prominent and affect users or the project in a major way. must have Something we consider an essential part of Solidity 1.0. needs design The proposal is too vague to be implemented right away optimizer viair
Projects
None yet
Development

No branches or pull requests

9 participants