Inconsistent defaults of `optimizeStackAllocation` flag cause ICEs and metadata issues in optimized compilation with Yul optimizer disabled

[P1umH0]

Environment

• Compiler version: 0.8.28
• Target EVM version (as per compiler settings): None
• Framework/IDE (e.g. Truffle or Remix): None
• EVM execution environment / backend / blockchain client: None
• Operating system: Ubuntu

Steps to Reproduce

A fairly simple code named a.sol

contract SimpleContract {
    uint256 public value;
    constructor(uint256 _initialValue) {
        value = _initialValue;
    }
    function setValue(uint256 _newValue) public {
        value = _newValue;
    }
    function getValue() public view returns (uint256) {
        return value;
    }
}

The help message shows two points:

1. options "--optimize-yul" and "--no-optimize-yul" are contradictory
2. if "--optimize" is enabled, "--optimize-yul" will be enabled automatically

Optimizer Options:
  --optimize           Enable optimizer.
  --optimize-runs n (=200)
                       The number of runs specifies roughly how often each 
                       opcode of the deployed code will be executed across the 
                       lifetime of the contract. Lower values will optimize 
                       more for initial deployment cost, higher values will 
                       optimize more for high-frequency usage.
  --optimize-yul       Enable Yul optimizer (independently of the EVM assembly 
                       optimizer). The general --optimize option automatically 
                       enables this unless --no-optimize-yul is specified.
  --no-optimize-yul    Disable Yul optimizer (independently of the EVM assembly
                       optimizer).
  --yul-optimizations steps
                       Forces Yul optimizer to use the specified sequence of 
                       optimization steps instead of the built-in one.

I try command

solc --bin-runtime --optimize-yul --no-optimize-yul ./a.sol

and i get

Error: Options --optimize-yul and --no-optimize-yul cannot be used together.

this is consistent with the 1st point
and then i try command

solc --bin-runtime --optimize --no-optimize-yul ./a.sol

the compilation crashes at this time

Internal compiler error:
/solidity/libsolidity/interface/CompilerStack.cpp(1719): Throw in function std::string solidity::frontend::CompilerStack::createMetadata(const solidity::frontend::CompilerStack::Contract&, bool) const
Dynamic exception type: boost::wrapexcept<solidity::langutil::InternalCompilerError>
std::exception::what: Solidity assertion failed
[solidity::util::tag_comment*] = Solidity assertion failed

According to the 2nd point, I should get the same or similar error as before, but here it crashes for some reason

[cameel]

I investigated this today and... it's complicated. The short of it is that we do have a bug here, even more than one, but it's not what it may seem. The ICE is only a symptom.

Optimized compilation with Yul optimizer disabled both via CLI and via Standard JSON has apparently been slightly broken since 0.8.8 with regard to stack optimizations. The flag controlling whether they are applied has wrong value in some circumstances. The resulting code is not incorrect, just not necessarily optimized exactly the way that was requested. There are also potential issues with reproducing that exact code by exactly following metadata (CC: @kuzdogan).

The full explanation is long so I'll post it in a separate comment below.

[cameel]

The situation is complicated because there are multiple interdependent bugs here and in the meantime we had a PR that turned buggy behavior into the expected behavior. I think it will be the easiest to understand what happened if describe how this functionality was modified over time:

• As initially implemented, the default value of optimizeStackAllocation always matched the state of Yul optimizer, both on the CLI and in Standard JSON.
• --optimize-yul was introduced with Yul optimizer. --optimize back then used to enable only the evmasm optimizer and Yul optimizer had to be enabled separately.
• In 0.8.0 we made --optimize enable Yul optimizer automatically, deprecated --optimize-yul (it became a no-op) and introduced --no-optimize-yul.
• In 0.8.8 I did some CLI refactors. One of them (#11730) inadvertently changed optimizeStackAllocation behavior on the CLI. Disabling Yul optimizer (which was only possible with --no-optimize-yul) no longer disabled the flag. This went unnoticed because of our almost non-existent test coverage for CLI options (in fact that series of refactors was what made introducing CLI parser tests possible in the first place).
• In 0.8.21 I noticed this and submitted a fix (#14265), but it was never merged because in the same release we changed how --optimize works (#14149), which meant that the buggy behavior became the right behavior. The new behavior is to always run some steps of Yul optimizer, even with --no-optimize-yul and the flag should stay always on.
  • Before the PR in Standard JSON the flag could be set explicitly, but only when Yul optimizer was enabled. The PR extended this behavior to make it possible to set it when Yul optimizer is nominally disabled. I decided there was not much harm in letting users still explicitly set it to false, because the default still follows Yul optimizer state. This actually came in handy as a workaround at least once.
    • EDIT: @r0qs points out that this is not the case. I think I considered that ended up not implementing it. optimizer.details.yulDetails.stackAllocation can be toggled only when optimizer.details.yul is true.
  • Looking at this today, I realized that there is a bug in that PR: we don't include the flag in metadata when the optimizer is disabled. Instead we assume that it must have the default value in that case.
  • This is not the only bug: in Standard JSON I did not change the default value of the flag to depend on whether we internally run Yul optimizer. Instead I left it dependent on the nominal value of the optimizer.details.yul flag. This means that Standard JSON behavior remained different from CLI behavior (different defaults). What changed was just which can be considered correct.
  • EDIT2: Another quirk of this PR is that when Yul optimizer is not enabled, the state of the flag depends on the state of the rest of the optimizer (i.e. on --optimize/settings.optimizer.enabled). Not sure if I'd call it a bug, but I think this is undesirable and should be changed. It's weird to have it depend on anything other than the state of Yul optimizer.
• In 0.8.21 I restored --optimize-yul (#14268) to make it possible to run only the Yul optimizer, without any evmasm optimizations (which is something that Standard JSON always allowed). But this did not affect the buggy behavior.
• In 0.8.23 an unrelated PR (#14657) introduced an assert that verifies the assumption about the flag before creating metadata. This assumption is not always satisfied, so the assert rightly started failing.
  • However, the assert was not entirely correct. It was based on Standard JSON behavior (flag false by default), and was instead failing for the (correct) CLI behavior (flag true by default)
  • EDIT2: @r0qs discovered that PR also introduced another bug. The state of the flag is not stored in metadata when an empty optimizer sequence is used - on the assumption that it's always false and that false is the default. This is not the case with optimizer.details.yul: true, because then the flag defaults to true. Explicitly setting the flag to true is not possible (will result in an ICE), however, setting it to false will override the default and pass the check but the overridden value will not end up in metadata.

How serious is the problem?

• Only optimized compilation with Yul optimizer explicitly disabled is affected.
• Compilation produces different results with regard to stack optimization between CLI and Standard JSON:
  • On 0.8.8..0.8.20 --no-optimize-yul applies stack optimization even though it shouldn't. Standard JSON behaves correctly.
  • On 0.8.21..0.8.22 optimizer.details.yul: false does not apply stack optimization by default even though it should. CLI behaves correctly.
  • Since 0.8.23 optimizer.details.yul: false still does not apply stack optimization by default, while CLI is broken.
• Since 0.8.8 metadata produced in optimized compilation with Yul optimizer disabled will only allow recreating the same compilation if used via the same interface (Standard JSON vs CLI).
• Since 0.8.21 metadata is wrong when compiling via Standard JSON with both optimizer.details.yul: false and optimizer.details.yulDetails.stackAllocation: true - the non-default state of the stackAllocation flag is not recorded.
  • EDIT: Not true after all. See edit above.
• EDIT2: Since 0.8.23 metadata is wrong when compiling via Standard JSON with optimizer.details.yul: true, an empty optimizer sequence and optimizer.details.yulDetails.stackAllocation: false - the non-default state of the stackAllocation flag is not recorded.
  • If the flag is explicitly set to true instead, we get an ICE.

What do we need to do to fix it?

• Standard JSON behavior must be changed: do not disable optimizeStackAllocation when optimizer.details.yul is false.
  • EDIT2: (Optional) Change both Standard JSON and CLI behavior to enable optimizeStackAllocation flag even when --optimize is not used or optimizer.enabled is false/unset.
• Metadata: when Yul optimizer is disabled store the flag if it does not match the default value. With that the assert is no longer necessary.
  • EDIT: Not strictly necessary after all. We could do this, but it's also enough to fix the assert to assume the correct default.
• EDIT2: Metadata: when Yul optimizer is enabled and the optimizer sequence is empty store the flag if it does not match the default value.

[cameel]

@r0qs Pointed out that optimizer.details.yulDetails.stackAllocation: true is only allowed with yul: true.

i.e. This is a invalid configuration:

		"optimizer": {
			"details": {
				"yul": false,
				"yulDetails": {
					"stackAllocation": true
				}
			}
		},

It's still possible to toggle the flag with optimizer enabled and sequence set to u, which is equivalent to disabling the Yul optimizer, but fortunately does not result in the flag being omitted.

Good, one less harmful consequence of this. I updated my assessment above accordingly.

[cameel]

After reviewing @r0qs' fix (#15655) I realized that there are more problems here. There actually is one case where the produced metadata is wrong. See parts marked as "EDIT2" in the updated comment above.

[kuzdogan]

If it helps we can provide some example contracts e.g. that have yul: false etc.

SELECT 
   cd.chain_id,
   cd.address,
   cc.compiler_settings,
   cc.compiler,
   cc.version
FROM verified_contracts vc
JOIN contract_deployments cd ON vc.deployment_id = cd.id
JOIN compiled_contracts cc ON vc.compilation_id = cc.id
WHERE cc.compiler_settings @> '{"optimizer": {"details": { "yul": false}}}'::jsonb
ORDER BY cd.chain_id, cd.created_at
limit 100;

[cameel]

On today's call we talked about what the proper defaults for optimizeStackAllocation should be and @ekpyron's opinion is that the flag itself is actually rather useless and should be deprecated. For context:

Regarding the optimizeStackAllocation business: it's rather messy. setting it to false will actually use the very old yul-evm code transform, but then simultaneously dumb it down to use naive stack allocation (which is a combination that's basically useless) - I don't think it's a valuable thing to do at this point anyways, I'd say we can remove it (as in: remove the flag internally, assuming it being true everywhere, ignoring it in input and not outputting it in metadata)

We'll get better code transforms that much more easily and reliably can avoid stack too deeps in the not too far future anyways - it's theoretically possible in very specific cases that the flag being false would actually work while it being true wouldn't, but it's not a good solution for those anyways, so yeah, I'd still just remove it.

So we're going to go with that rather than spend time untangling it. TBH this is not much different from what a fix would entail anyway - Yul optimizer always runs internally so after a fix the flag would end up being enabled almost always. What we really take away is just the ability to explicitly disable it. The upside is that we'll be able to drop some legacy code (though only once we drop support for homestead EVM as well - but that will happen pretty soon).