Accept empty optimizer sequence with Yul optimizer disabled #14657

nikola-matic · 2023-10-30T14:19:29Z

Fixes: #14621

cameel

Seems correct in terms of logic. Still needs a changelog entry and docs.

And can be simplified in many ways - see my comments below.

solc/CommandLineParser.h

solc/CommandLineParser.cpp

test/cmdlineTests/standard_optimizer_yulDetails_optimiserSteps_no_yul/input.json

libsolidity/interface/StandardCompiler.cpp

libyul/YulStack.cpp

.../cmdlineTests/standard_optimizer_yulDetails_optimiserSteps_with_empty_sequence_no_yul/in.sol

libsolidity/interface/StandardCompiler.cpp

libyul/YulStack.cpp

docs/internals/optimizer.rst

test/cmdlineTests/yul_optimizer_steps_without_optimize_empty_sequence/input.sol

cameel

There are still things we could improve but nothing so horrible that it would prevent us from shipping this :)

libyul/optimiser/Suite.cpp

.../cmdlineTests/standard_optimizer_yulDetails_optimiserSteps_with_empty_sequence_no_yul/in.sol

test/cmdlineTests/yul_optimizer_steps_without_optimize_empty_sequence/input.sol

ekpyron · 2023-11-06T12:08:51Z

libyul/optimiser/Suite.cpp

+{
+	size_t delimiterCount{0};
+	for (char const step: _sequence)
+		switch (step)
+		{
+		case ':':
+			if (++delimiterCount > 1)
+				return false;
+			break;
+		case ' ':
+		case '\n':
+			break;
+		default:
+			return false;
+		}
+	return true;
+}


I mean, I'm very much fine with leaving this as is - but given that I think I even already saw some discussion in the channel around this on the side, I can't help but wonder: why the special handling of different counts of colons in here and all the fuzz (the sequence will separately be validated and arguably it even makes more sense to treat "::" as empty but malformed rather than as non-empty...)?

This could just be (inline):

Suggested change

{

size_t delimiterCount{0};

for (char const step: _sequence)

switch (step)

{

case ':':

if (++delimiterCount > 1)

return false;

break;

case ' ':

case '\n':

break;

default:

return false;

}

return true;

}

{

return ranges::any_of(_sequence, [](auto _step) { return _step != ':' && _step != ' ' && _step != '\n'; });

}

in one case - and in the second case for standard json even simpler...

ekpyron · 2023-11-06T12:20:18Z

Without looking through this fully: how exactly does this bypass the fact that we don't enter the sequence to metadata if runYulOptimiser is false? I'd expect to see some change accounting for that (i.e. in CompilerStack::createMetadata), but I don't on a quick glance.

r0qs · 2023-11-06T12:21:40Z

libsolidity/interface/StandardCompiler.cpp

 {
 	if (_details.isMember(_name))
 	{
 		if (_details[_name].isString())
 		{
+			std::string const fullSequence = _details[_name].asString();
+			if (!_runYulOptimizer && !OptimiserSuite::isEmptyOptimizerSequence(fullSequence))
+				return formatFatalError(Error::Type::JSONError, "If Yul optimizer is disabled, only an empty optimizerSteps sequence is accepted.");


Suggested change

return formatFatalError(Error::Type::JSONError, "If Yul optimizer is disabled, only an empty optimizerSteps sequence is accepted.");

return formatFatalError(Error::Type::JSONError, "When Yul optimizer is disabled, only an empty optimizerSteps sequence is accepted.");

libsolidity/interface/StandardCompiler.cpp

libyul/optimiser/Suite.cpp

libyul/YulStack.cpp

cameel

Without looking through this fully: how exactly does this bypass the fact that we don't enter the sequence to metadata if runYulOptimiser is false? I'd expect to see some change accounting for that (i.e. in CompilerStack::createMetadata), but I don't on a quick glance.

Right, good catch, it doesn't. createMetadata() will skip the sequence when runYulOptimiser is false and this will result in metadata not recreating the bytecode. This needs to be fixed before we can merge the PR.

libsolidity/interface/StandardCompiler.cpp

libyul/YulStack.cpp

nikola-matic · 2023-11-06T21:16:01Z

Without looking through this fully: how exactly does this bypass the fact that we don't enter the sequence to metadata if runYulOptimiser is false? I'd expect to see some change accounting for that (i.e. in CompilerStack::createMetadata), but I don't on a quick glance.

Right, good catch, it doesn't. createMetadata() will skip the sequence when runYulOptimiser is false and this will result in metadata not recreating the bytecode. This needs to be fixed before we can merge the PR.

This was implemented this morning though.

cameel

Needs some asserts. Other than that seems good.

This was implemented this morning though.

An, right. Somehow I missed that when I skimmed though the PR and thought the changes were not applied yet.

cameel · 2023-11-07T12:18:19Z

libsolidity/interface/CompilerStack.cpp

+		else if
+		(


Suggested change

else if

(

else if(

libsolidity/interface/CompilerStack.cpp

Whitespace and newline validation for empty sequence Changelog and docs Kamil Revert "fixup! Accept empty optimizer sequence with Yul optimizer disabled" This reverts commit 1cbcc8e.

nikola-matic added this to the 0.8.23 milestone Oct 30, 2023

nikola-matic self-assigned this Oct 30, 2023

nikola-matic force-pushed the accept-empty-optimizer-sequence-with-yul-optimizer-disabled branch 3 times, most recently from e183552 to 100474a Compare October 31, 2023 10:56

cameel reviewed Nov 1, 2023

View reviewed changes

nikola-matic force-pushed the accept-empty-optimizer-sequence-with-yul-optimizer-disabled branch 3 times, most recently from d778c59 to b44e88d Compare November 2, 2023 14:47

cameel mentioned this pull request Nov 2, 2023

Update optimizer docs to include new unchecked loop increment #14650

Merged

cameel reviewed Nov 2, 2023

View reviewed changes

libsolidity/interface/StandardCompiler.cpp Outdated Show resolved Hide resolved

libyul/YulStack.cpp Show resolved Hide resolved

libyul/YulStack.cpp Show resolved Hide resolved

docs/internals/optimizer.rst Outdated Show resolved Hide resolved

nikola-matic force-pushed the accept-empty-optimizer-sequence-with-yul-optimizer-disabled branch 2 times, most recently from 0a35d7f to 4b646b3 Compare November 2, 2023 19:46

nikola-matic commented Nov 3, 2023

View reviewed changes

test/cmdlineTests/yul_optimizer_steps_without_optimize_empty_sequence/input.sol Show resolved Hide resolved

nikola-matic marked this pull request as ready for review November 3, 2023 08:49

cameel previously approved these changes Nov 3, 2023

View reviewed changes

libyul/optimiser/Suite.cpp Show resolved Hide resolved

.../cmdlineTests/standard_optimizer_yulDetails_optimiserSteps_with_empty_sequence_no_yul/in.sol Show resolved Hide resolved

test/cmdlineTests/yul_optimizer_steps_without_optimize_empty_sequence/input.sol Show resolved Hide resolved

nikola-matic dismissed cameel’s stale review via 69eecc1 November 5, 2023 09:00

nikola-matic force-pushed the accept-empty-optimizer-sequence-with-yul-optimizer-disabled branch 2 times, most recently from 69eecc1 to 5855731 Compare November 5, 2023 11:58

nikola-matic requested a review from cameel November 5, 2023 12:35

ekpyron reviewed Nov 6, 2023

View reviewed changes

r0qs reviewed Nov 6, 2023

View reviewed changes

nikola-matic force-pushed the accept-empty-optimizer-sequence-with-yul-optimizer-disabled branch from 10136ff to 1cbcc8e Compare November 6, 2023 16:19

cameel requested changes Nov 6, 2023

View reviewed changes

libsolidity/interface/StandardCompiler.cpp Show resolved Hide resolved

libyul/YulStack.cpp Show resolved Hide resolved

cameel reviewed Nov 7, 2023

View reviewed changes

Accept empty optimizer sequence with Yul optimizer disabled

d899d9c

Whitespace and newline validation for empty sequence Changelog and docs Kamil Revert "fixup! Accept empty optimizer sequence with Yul optimizer disabled" This reverts commit 1cbcc8e.

nikola-matic force-pushed the accept-empty-optimizer-sequence-with-yul-optimizer-disabled branch from f0618dc to d899d9c Compare November 7, 2023 14:19

cameel approved these changes Nov 7, 2023

View reviewed changes

nikola-matic merged commit cb93e6e into develop Nov 7, 2023
1 check passed

nikola-matic deleted the accept-empty-optimizer-sequence-with-yul-optimizer-disabled branch November 7, 2023 15:46

nikola-matic removed this from the 0.8.24 milestone Jan 25, 2024

danceratopz mentioned this pull request Mar 15, 2024

Providing an empty string to --yul-optimizations without enabling yul optimization triggers uncaught exception #14946

Closed

nikola-matic mentioned this pull request Mar 27, 2024

Fix uncaught exception when using empty string for yul optimization flags #14967

Merged

This was referenced Dec 16, 2024

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

Open

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accept empty optimizer sequence with Yul optimizer disabled #14657

Accept empty optimizer sequence with Yul optimizer disabled #14657

nikola-matic commented Oct 30, 2023 •

edited

Loading

cameel left a comment

cameel left a comment •

edited

Loading

ekpyron Nov 6, 2023 •

edited

Loading

ekpyron commented Nov 6, 2023 •

edited

Loading

r0qs Nov 6, 2023

cameel left a comment

nikola-matic commented Nov 6, 2023

cameel left a comment •

edited

Loading

cameel Nov 7, 2023

	return formatFatalError(Error::Type::JSONError, "If Yul optimizer is disabled, only an empty optimizerSteps sequence is accepted.");
	return formatFatalError(Error::Type::JSONError, "When Yul optimizer is disabled, only an empty optimizerSteps sequence is accepted.");

Accept empty optimizer sequence with Yul optimizer disabled #14657

Accept empty optimizer sequence with Yul optimizer disabled #14657

Conversation

nikola-matic commented Oct 30, 2023 • edited Loading

cameel left a comment

Choose a reason for hiding this comment

cameel left a comment • edited Loading

Choose a reason for hiding this comment

ekpyron Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

ekpyron commented Nov 6, 2023 • edited Loading

r0qs Nov 6, 2023

Choose a reason for hiding this comment

cameel left a comment

Choose a reason for hiding this comment

nikola-matic commented Nov 6, 2023

cameel left a comment • edited Loading

Choose a reason for hiding this comment

cameel Nov 7, 2023

Choose a reason for hiding this comment

nikola-matic commented Oct 30, 2023 •

edited

Loading

cameel left a comment •

edited

Loading

ekpyron Nov 6, 2023 •

edited

Loading

ekpyron commented Nov 6, 2023 •

edited

Loading

cameel left a comment •

edited

Loading