Fix Inconsistencies in Stack Allocation Optimizer Settings for CLI and Standard JSON Interface

[r0qs]

Fixes #15641

The PR addresses two issues detailed below:

1. The first commit resolves an inconsistency between the CLI and the Standard JSON interface, where optimizeStackAllocation depended solely on the Yul optimizer option in the latter, while in the CLI it can be enabled by either the general optimizer settings or the Yul optimizer. This inconsistency also caused an ICE in the CLI at the line below when generating metadata, as the assertion is not always true (e.g., when the user specifies --optimize and --no-optimize-yul).

solidity/libsolidity/interface/CompilerStack.cpp

Line 1745 in 8508a1c

solAssert(m_optimiserSettings.optimizeStackAllocation == false);

See also:

• solidity/solc/CommandLineParser.cpp

  Line 276 in 8508a1c

  settings.optimizeStackAllocation = true;

• solidity/solc/CommandLineParser.cpp

  Line 1243 in 8508a1c

  m_options.optimizer.optimizeYul = (

I believe that the optimizeStackAllocation setting should also be enabled in the Standard JSON interface whenever the optimizer (or Yul optimizer) is enabled (this behavior is consistent with the CLI as far as I can tell), unless explicitly disabled by the user through the optimizer.details.yulDetails.stackAllocation setting.

After fixing the inconsistency, I was able to reproduce the ICE in the Standard JSON interface using optimizer.enabled: true and optimizer.details.yul: false.

2. The second commit addresses the ICE in both the CLI (when used with the --optimize and --no-optimize-yul options) and the Standard JSON interface by correcting our assumptions about optimizeStackAllocation.

[r0qs]

Maybe we should add the optimizeStackAllocation setting to the metadata here as well. I didn’t include it because yulDetails is omitted when the optimizer is disabled. However, now it is also being included when an empty optimizer sequence is provided, which wasn’t the case previously.

Since optimizeStackAllocation can be either true or false in this else block, I think we should include it to the metadata here. That said, this would essentially mean always including yulDetails in the metadata, and I’m not sure if that’s acceptable.

[cameel]

The only case where the flag must be included is when it has a non-default value. I.e. if you'd have to set it explicitly in the input to reproduce the same bytecode.

In other situations it's optional, and either choice has pros and cons. The advantage of always including it is that if you get into a weird situation, where you get two inconsistent defaults depending on how you invoke the compiler, the options reconstructed from metadata will still let you reproduce the bytecode. The advantage of omitting it is that if you make wrong assumptions about the default, you won't get wrong metadata, because you did not store the value there - reconstructed options will rely on the actual default.

Hard to say which is better, because here we got bitten by both of those problems at the same time so neither choice would really have saved us :) So the choice is a bit arbitrary.

[cameel]

In any case, in this situation the flag will always have the default value, because we don't allow changing it. You can choose whether to store it or not, both will work.

Though the funny thing is that even though we know it must have the default value, we don't know what the default was. That's because it depends on the state of the enabled flag and we don't store it in OptimiserSettings. We instead store the state of individual components. We deal with it by just not writing it into metadata (which means it will default to false regardless of what the original value was), which is fine because the other flags are enough to determine the behavior.

I mean, we can see the current value so we "know" it but we must trust that it's actually the default; we can't assert anything about it to check if we're wrong. Which is not great because the assert is what allowed us to detect this bug (even if it wasn't the right assert).

[r0qs]

This was removed because it is no longer true.

[cameel]

Our optimizer options are just madness. They seem not that bad on the surface, but actually figuring out behavior across differences between CLI/Standard JSON/OptimiserSettings/metadata, defaults that depend on defaults that depend on defaults (and are ever so slightly different in each case) and changes across versions of the compiler just drives me crazy every time I try to do it accurately. No wonder we have so many bugs in it.

[cameel]

This is wrong, because it means that enabled: true + yul: false combination enables the flag (on the CLI the flag stays false in that case). It should be:

Suggested change

	if (optimizerEnabled || settings.runYulOptimiser)
	settings.optimizeStackAllocation = true;
	if (settings.runYulOptimiser)
	settings.optimizeStackAllocation = true;

[cameel]

Now that I think of it, this means that both on the command line and in Standard JSON we do disable the flag when the optimizer as a whole is disabled. That's not a part of the bug, but I wonder if I should have changed that behavior in #14149. After all, the way it is now means that the state of the flag depends on whether we run evmasm optimizer, which is weird.

We should probably change that in a separate PR.

[r0qs]

This is wrong, because it means that enabled: true + yul: false combination enables the flag (on the CLI the flag stays false in that case). It should be:

However, on the CLI, the optimizeStackAllocation setting depends on the value of optimizer.optimizeYul:

solidity/solc/CommandLineParser.cpp

Lines 272 to 276 in b6486db

	settings.runYulOptimiser = optimizer.optimizeYul;
	if (optimizer.optimizeYul)
	// NOTE: Standard JSON disables optimizeStackAllocation by default when yul optimizer is disabled.
	// --optimize --no-optimize-yul on the CLI does not have that effect.
	settings.optimizeStackAllocation = true;

This value is determined either by using --optimize without --no-optimize-yul or by explicitly setting --optimize-yul:

solidity/solc/CommandLineParser.cpp

Lines 1242 to 1246 in b6486db

	m_options.optimizer.optimizeEvmasm = (m_args.count(g_strOptimize) > 0);
	m_options.optimizer.optimizeYul = (
	(m_args.count(g_strOptimize) > 0 && m_args.count(g_strNoOptimizeYul) == 0) ||
	m_args.count(g_strOptimizeYul) > 0
	);

In contrast, in the Standard JSON, settings.runYulOptimiser depends solely on the details.yul option:

solidity/libsolidity/interface/StandardCompiler.cpp

Lines 587 to 591 in b6486db

	if (auto error = checkOptimizerDetail(details, "yul", settings.runYulOptimiser))
	return *error;
	if (auto error = checkOptimizerDetail(details, "simpleCounterForLoopUncheckedIncrement", settings.simpleCounterForLoopUncheckedIncrement))
	return *error;
	settings.optimizeStackAllocation = settings.runYulOptimiser;

And it does not depend on optimizer.enabled:

solidity/libsolidity/interface/StandardCompiler.cpp

Line 556 in b6486db

if (_jsonInput["enabled"].get<bool>())

Therefore, on the CLI, the flag does not remain false. This mismatch was the root cause of the ICE, where we assumed the flag was false, but it is actually true in createMetadata. This is why I attempted to align the behavior in the Standard JSON with the CLI. However, now that I think about it, the CLI behavior itself is incorrect and should be fixed instead.

Since the optimizer enables the Yul optimizer by default, the optimizeStackAllocation should only be disabled if the user explicitly use --no-optimize-yul or details.yul: false, right?

Also from our docs:

   "optimizer": {
        // Disabled by default.
        // NOTE: enabled=false still leaves some optimizations on. See comments below.
        // WARNING: Before version 0.8.6 omitting the 'enabled' key was not equivalent to setting
        // it to false and would actually disable all the optimizations.
        "enabled": true,
        ...
        // The new Yul optimizer. Mostly operates on the code of ABI coder v2
        // and inline assembly.
        // It is activated together with the global optimizer setting
        // and can be deactivated here.
        // Before Solidity 0.6.0 it had to be activated through this switch.
        "yul": false,
        // Tuning options for the Yul optimizer.
        "yulDetails": {
          // Improve allocation of stack slots for variables, can free up stack slots early.
          // Activated by default if the Yul optimizer is activated.
          "stackAllocation": true,
          ...
        }
   }

[cameel]

Since the optimizer enables the Yul optimizer by default, the optimizeStackAllocation should only be disabled if the user explicitly use --no-optimize-yul or details.yul: false, right?

No. I am assuming it should be tied to the internal state of the Yul optimizer. I.e. if the sequence runs, optimized stack allocation should happen unless the user explicitly disables it with optimizeStackAllocation: false. And after #14149 the sequence always runs.

Currently it's also disabled when you disable the optimizer as a whole (#15655 (comment)), which is inconsistent with that assumption and IMO should also be changed.

At least this is the assumption I've been making in my description in the issue and in what I reviewed here so far. IIRC Daniel said that it's just an internal detail and we should always have it enabled in optimized compilation eventually and deprecate the setting. But I'm actually not entirely sure how indispensable this flag is for unoptimized compilation (or if it matters at all). On one hand we still want to avoid doing anything unnecessary that would be considered an optimization in that mode. On the other hand the reason for always running UnusedPruner in the first place was to help with stack issues and this flag is aimed at that too. I think kept flipping between those two views in the original issue and it's part of the reason why it ended up being this inconsistent. We should discuss it with Daniel and get this straight.

[cameel]

In contrast, in the Standard JSON, settings.runYulOptimiser depends solely on the details.yul option:

And it does not depend on optimizer.enabled:

It does depend on optimizer.enabled, because that setting determines its default value. Note the settings = OptimiserSettings::standard(); on the line below :)

BTW, this is exactly what I meant when I complained about defaults depending on defaults depending on defaults. WTF is this thing this tricky to reason about? It's just a bunch of simple boolean flags :)

[cameel]

The only case where the flag must be included is when it has a non-default value. I.e. if you'd have to set it explicitly in the input to reproduce the same bytecode.

In other situations it's optional, and either choice has pros and cons. The advantage of always including it is that if you get into a weird situation, where you get two inconsistent defaults depending on how you invoke the compiler, the options reconstructed from metadata will still let you reproduce the bytecode. The advantage of omitting it is that if you make wrong assumptions about the default, you won't get wrong metadata, because you did not store the value there - reconstructed options will rely on the actual default.

Hard to say which is better, because here we got bitten by both of those problems at the same time so neither choice would really have saved us :) So the choice is a bit arbitrary.

[cameel]

Oh, I guess this is yet another bug, this time in #14657. It means that my mention of wrong metadata being produced was not entirely incorrect, it just happens not with Yul optimizer disabled but when using an empty sequence.

So yul: true + optimizerStep: ":" + stackAllocation: false would pass this assert but not store the flag, assuming that its default value it's false. But in that case it's actually true so options reconstructed from metadata will use the wrong value.

And another consequence is probably that yul: true + optimizerStep: ":" + stackAllocation: false (or with the flag omitted) will produce an ICE.

[cameel]

In any case, in this situation the flag will always have the default value, because we don't allow changing it. You can choose whether to store it or not, both will work.

Though the funny thing is that even though we know it must have the default value, we don't know what the default was. That's because it depends on the state of the enabled flag and we don't store it in OptimiserSettings. We instead store the state of individual components. We deal with it by just not writing it into metadata (which means it will default to false regardless of what the original value was), which is fine because the other flags are enough to determine the behavior.

I mean, we can see the current value so we "know" it but we must trust that it's actually the default; we can't assert anything about it to check if we're wrong. Which is not great because the assert is what allowed us to detect this bug (even if it wasn't the right assert).

[cameel]

Please remember that we need changelog entries for it all. I think this one will even require multiple entries because we're fixing multiple bugs.