Merge pull request #14967 from ethereum/fixEmptyStringYulOptimization…

…Flags Fix uncaught exception when using empty string for yul optimization flags
ethereum · Apr 19, 2024 · 2e8cd3f · 2e8cd3f
2 parents 00b2e54 + a0e1a3e
commit 2e8cd3f
Show file tree

Hide file tree

Showing 21 changed files with 233 additions and 30 deletions.
diff --git a/Changelog.md b/Changelog.md
@@ -7,9 +7,11 @@ Compiler Features:
 
 
 Bugfixes:
+ * Commandline Interface: Fix ICE when the optimizer is disabled and an empty/blank string is used for ``--yul-optimizations`` sequence.
  * SMTChecker: Fix internal error on mapping access caused by too strong requirements on sort compatibility of the index and mapping domain.
  * SMTChecker: Fix internal error when using an empty tuple in a conditional operator.
  * SMTChecker: Fix internal error when using bitwise operators with an array element as argument.
+ * Standard JSON Interface: Fix ICE when the optimizer is disabled and an empty/blank string is used for ``optimizerSteps`` sequence.
  * Yul Optimizer: Fix the order of assignments generated by ``SSATransform`` being dependent on AST IDs, sometimes resulting in different (but equivalent) bytecode when unrelated files were added to the compilation pipeline.
 
 

diff --git a/docs/internals/optimizer.rst b/docs/internals/optimizer.rst
@@ -46,7 +46,7 @@ for a stand-alone Yul mode.
     enabled by default and can only be turned off via the :ref:`Standard JSON <compiler-api>`.
 
 .. note::
-    An empty optimizer sequence is accepted even without ``--optimize`` in order to fully disable
+    An empty optimizer sequence, i.e ``:``, is accepted even without ``--optimize`` in order to fully disable
     the user-supplied portion of the Yul :ref:`optimizer sequence <selecting-optimizations>`, as by default,
     even when the optimizer is not turned on, the :ref:`unused pruner <unused-pruner>` step will be run.
 

diff --git a/libsolidity/interface/CompilerStack.cpp b/libsolidity/interface/CompilerStack.cpp
@@ -1722,10 +1722,7 @@ std::string CompilerStack::createMetadata(Contract const& _contract, bool _forIR
 			details["yulDetails"]["stackAllocation"] = m_optimiserSettings.optimizeStackAllocation;
 			details["yulDetails"]["optimizerSteps"] = m_optimiserSettings.yulOptimiserSteps + ":" + m_optimiserSettings.yulOptimiserCleanupSteps;
 		}
-		else if (
-			OptimiserSuite::isEmptyOptimizerSequence(m_optimiserSettings.yulOptimiserSteps) &&
-			OptimiserSuite::isEmptyOptimizerSequence(m_optimiserSettings.yulOptimiserCleanupSteps)
-		)
+		else if (OptimiserSuite::isEmptyOptimizerSequence(m_optimiserSettings.yulOptimiserSteps + ":" + m_optimiserSettings.yulOptimiserCleanupSteps))
 		{
 			solAssert(m_optimiserSettings.optimizeStackAllocation == false);
 			details["yulDetails"] = Json::objectValue;

diff --git a/libsolidity/interface/StandardCompiler.cpp b/libsolidity/interface/StandardCompiler.cpp
@@ -465,7 +465,12 @@ std::optional<Json::Value> checkOptimizerDetailSteps(Json::Value const& _details
 		{
 			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.");
+			{
+				std::string errorMessage =
+					"If Yul optimizer is disabled, only an empty optimizerSteps sequence is accepted."
+					" Note that the empty optimizer sequence is properly denoted by \":\".";
+				return formatFatalError(Error::Type::JSONError, errorMessage);
+			}
 
 			try
 			{

diff --git a/libyul/optimiser/Suite.cpp b/libyul/optimiser/Suite.cpp
@@ -77,6 +77,8 @@
 
 #include <range/v3/view/map.hpp>
 #include <range/v3/action/remove.hpp>
+#include <range/v3/algorithm/count.hpp>
+#include <range/v3/algorithm/none_of.hpp>
 
 #include <limits>
 #include <tuple>
@@ -385,21 +387,9 @@ void OptimiserSuite::validateSequence(std::string_view _stepAbbreviations)
 
 bool OptimiserSuite::isEmptyOptimizerSequence(std::string const& _sequence)
 {
-	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::count(_sequence, ':') == 1 &&
+		ranges::none_of(_sequence, [](auto _step) { return _step != ':' && _step != ' ' && _step != '\n'; });
 }
 
 void OptimiserSuite::runSequence(std::string_view _stepAbbreviations, Block& _ast, bool _repeatUntilStable)

diff --git a/solc/CommandLineParser.cpp b/solc/CommandLineParser.cpp
@@ -1229,8 +1229,15 @@ void CommandLineParser::processArgs()
 	if (m_args.count(g_strYulOptimizations))
 	{
 		OptimiserSettings optimiserSettings = m_options.optimiserSettings();
-		if (!optimiserSettings.runYulOptimiser && !OptimiserSuite::isEmptyOptimizerSequence(m_args[g_strYulOptimizations].as<std::string>()))
-			solThrow(CommandLineValidationError, "--" + g_strYulOptimizations + " is invalid with a non-empty sequence if Yul optimizer is disabled.");
+		if (
+			!optimiserSettings.runYulOptimiser &&
+			!OptimiserSuite::isEmptyOptimizerSequence(m_args[g_strYulOptimizations].as<std::string>())
+		)
+			solThrow(
+				CommandLineValidationError,
+				"--" + g_strYulOptimizations + " is invalid with a non-empty sequence if Yul optimizer is disabled."
+				" Note that the empty optimizer sequence is properly denoted by \":\"."
+			);
 
 		try
 		{

diff --git a/...lineTests/standard_optimizer_yulDetails_optimiserSteps_empty_sequence_metadata/input.json b/...lineTests/standard_optimizer_yulDetails_optimiserSteps_empty_sequence_metadata/input.json
@@ -0,0 +1,25 @@
+{
+    "language": "Solidity",
+    "sources": {
+        "A": {"content": "
+            // SPDX-License-Identifier: GPL-3.0
+            pragma solidity >=0.0;
+            contract C { }
+        "}
+    },
+    "settings": {
+        "optimizer": {
+            "details": {
+                "yul": false,
+                "yulDetails": {
+                    "optimizerSteps": ":"
+                }
+            }
+        },
+        "outputSelection": {
+            "*": {
+              "*": ["metadata"]
+            }
+        }
+    }
+}
diff --git a/...ineTests/standard_optimizer_yulDetails_optimiserSteps_empty_sequence_metadata/output.json b/...ineTests/standard_optimizer_yulDetails_optimiserSteps_empty_sequence_metadata/output.json
@@ -0,0 +1,19 @@
+{
+    "contracts":
+    {
+        "A":
+        {
+            "C":
+            {
+                "metadata": "{\"compiler\":{\"version\":\"<VERSION REMOVED>\"},\"language\":\"Solidity\",\"output\":{\"abi\":[],\"devdoc\":{\"kind\":\"dev\",\"methods\":{},\"version\":1},\"userdoc\":{\"kind\":\"user\",\"methods\":{},\"version\":1}},\"settings\":{\"compilationTarget\":{\"A\":\"C\"},\"evmVersion\":\"cancun\",\"libraries\":{},\"metadata\":{\"bytecodeHash\":\"ipfs\"},\"optimizer\":{\"details\":{\"constantOptimizer\":false,\"cse\":false,\"deduplicate\":false,\"inliner\":false,\"jumpdestRemover\":true,\"orderLiterals\":false,\"peephole\":true,\"simpleCounterForLoopUncheckedIncrement\":true,\"yul\":false,\"yulDetails\":{\"optimizerSteps\":\":\"}},\"runs\":200},\"remappings\":[]},\"sources\":{\"A\":{\"keccak256\":\"0xb284c39999cb85b80be315a6e9e322adf67a783c66e91ba4439168694580a66d\",\"license\":\"GPL-3.0\",\"urls\":[\"bzz-raw://098cee915fad095b8a996813768bd7d5e8c9e40c405e8c43d0572bb7bbc17334\",\"dweb:/ipfs/QmZmUzvSryrrD7pJ9S32iQnEWn4QBL4J1NdbQqL2Xc3yTr\"]}},\"version\":1}"
+            }
+        }
+    },
+    "sources":
+    {
+        "A":
+        {
+            "id": 0
+        }
+    }
+}
diff --git a/...andard_optimizer_yulDetails_optimiserSteps_empty_sequence_whitespaces_metadata/input.json b/...andard_optimizer_yulDetails_optimiserSteps_empty_sequence_whitespaces_metadata/input.json
@@ -0,0 +1,25 @@
+{
+    "language": "Solidity",
+    "sources": {
+        "A": {"content": "
+            // SPDX-License-Identifier: GPL-3.0
+            pragma solidity >=0.0;
+            contract C { }
+        "}
+    },
+    "settings": {
+        "optimizer": {
+            "details": {
+                "yul": false,
+                "yulDetails": {
+                    "optimizerSteps": " :  "
+                }
+            }
+        },
+        "outputSelection": {
+            "*": {
+              "*": ["metadata"]
+            }
+        }
+    }
+}
diff --git a/...ndard_optimizer_yulDetails_optimiserSteps_empty_sequence_whitespaces_metadata/output.json b/...ndard_optimizer_yulDetails_optimiserSteps_empty_sequence_whitespaces_metadata/output.json
@@ -0,0 +1,19 @@
+{
+    "contracts":
+    {
+        "A":
+        {
+            "C":
+            {
+                "metadata": "{\"compiler\":{\"version\":\"<VERSION REMOVED>\"},\"language\":\"Solidity\",\"output\":{\"abi\":[],\"devdoc\":{\"kind\":\"dev\",\"methods\":{},\"version\":1},\"userdoc\":{\"kind\":\"user\",\"methods\":{},\"version\":1}},\"settings\":{\"compilationTarget\":{\"A\":\"C\"},\"evmVersion\":\"cancun\",\"libraries\":{},\"metadata\":{\"bytecodeHash\":\"ipfs\"},\"optimizer\":{\"details\":{\"constantOptimizer\":false,\"cse\":false,\"deduplicate\":false,\"inliner\":false,\"jumpdestRemover\":true,\"orderLiterals\":false,\"peephole\":true,\"simpleCounterForLoopUncheckedIncrement\":true,\"yul\":false,\"yulDetails\":{\"optimizerSteps\":\":\"}},\"runs\":200},\"remappings\":[]},\"sources\":{\"A\":{\"keccak256\":\"0xb284c39999cb85b80be315a6e9e322adf67a783c66e91ba4439168694580a66d\",\"license\":\"GPL-3.0\",\"urls\":[\"bzz-raw://098cee915fad095b8a996813768bd7d5e8c9e40c405e8c43d0572bb7bbc17334\",\"dweb:/ipfs/QmZmUzvSryrrD7pJ9S32iQnEWn4QBL4J1NdbQqL2Xc3yTr\"]}},\"version\":1}"
+            }
+        }
+    },
+    "sources":
+    {
+        "A":
+        {
+            "id": 0
+        }
+    }
+}
diff --git a/test/cmdlineTests/standard_optimizer_yulDetails_optimiserSteps_no_yul/output.json b/test/cmdlineTests/standard_optimizer_yulDetails_optimiserSteps_no_yul/output.json
@@ -3,8 +3,8 @@
     [
         {
             "component": "general",
-            "formattedMessage": "If Yul optimizer is disabled, only an empty optimizerSteps sequence is accepted.",
-            "message": "If Yul optimizer is disabled, only an empty optimizerSteps sequence is accepted.",
+            "formattedMessage": "If Yul optimizer is disabled, only an empty optimizerSteps sequence is accepted. Note that the empty optimizer sequence is properly denoted by \":\".",
+            "message": "If Yul optimizer is disabled, only an empty optimizerSteps sequence is accepted. Note that the empty optimizer sequence is properly denoted by \":\".",
             "severity": "error",
             "type": "JSONError"
         }

diff --git a/.../cmdlineTests/standard_optimizer_yulDetails_optimiserSteps_no_yul_empty_string/input.json b/.../cmdlineTests/standard_optimizer_yulDetails_optimiserSteps_no_yul_empty_string/input.json
@@ -0,0 +1,19 @@
+{
+    "language": "Solidity",
+    "sources": {
+        "A": {"content": "
+            // SPDX-License-Identifier: GPL-3.0
+            pragma solidity >=0.0;
+        "}
+    },
+    "settings": {
+        "optimizer": {
+            "details": {
+                "yul": false,
+                "yulDetails": {
+                    "optimizerSteps": ""
+                }
+            }
+        }
+    }
+}
diff --git a/...cmdlineTests/standard_optimizer_yulDetails_optimiserSteps_no_yul_empty_string/output.json b/...cmdlineTests/standard_optimizer_yulDetails_optimiserSteps_no_yul_empty_string/output.json
@@ -0,0 +1,12 @@
+{
+    "errors":
+    [
+        {
+            "component": "general",
+            "formattedMessage": "If Yul optimizer is disabled, only an empty optimizerSteps sequence is accepted. Note that the empty optimizer sequence is properly denoted by \":\".",
+            "message": "If Yul optimizer is disabled, only an empty optimizerSteps sequence is accepted. Note that the empty optimizer sequence is properly denoted by \":\".",
+            "severity": "error",
+            "type": "JSONError"
+        }
+    ]
+}
diff --git a/test/cmdlineTests/standard_optimizer_yulDetails_optimiserSteps_no_yul_whitespaces/input.json b/test/cmdlineTests/standard_optimizer_yulDetails_optimiserSteps_no_yul_whitespaces/input.json
@@ -0,0 +1,19 @@
+{
+    "language": "Solidity",
+    "sources": {
+        "A": {"content": "
+            // SPDX-License-Identifier: GPL-3.0
+            pragma solidity >=0.0;
+        "}
+    },
+    "settings": {
+        "optimizer": {
+            "details": {
+                "yul": false,
+                "yulDetails": {
+                    "optimizerSteps": " "
+                }
+            }
+        }
+    }
+}
diff --git a/.../cmdlineTests/standard_optimizer_yulDetails_optimiserSteps_no_yul_whitespaces/output.json b/.../cmdlineTests/standard_optimizer_yulDetails_optimiserSteps_no_yul_whitespaces/output.json
@@ -0,0 +1,12 @@
+{
+    "errors":
+    [
+        {
+            "component": "general",
+            "formattedMessage": "If Yul optimizer is disabled, only an empty optimizerSteps sequence is accepted. Note that the empty optimizer sequence is properly denoted by \":\".",
+            "message": "If Yul optimizer is disabled, only an empty optimizerSteps sequence is accepted. Note that the empty optimizer sequence is properly denoted by \":\".",
+            "severity": "error",
+            "type": "JSONError"
+        }
+    ]
+}
diff --git a/test/cmdlineTests/yul_optimizer_disabled_sequence_empty/args b/test/cmdlineTests/yul_optimizer_disabled_sequence_empty/args
@@ -0,0 +1 @@
+--metadata --yul-optimizations :
diff --git a/test/cmdlineTests/yul_optimizer_disabled_sequence_empty/input.sol b/test/cmdlineTests/yul_optimizer_disabled_sequence_empty/input.sol
@@ -0,0 +1,3 @@
+// SPDX-License-Identifier: GPL-3.0
+pragma solidity >=0.0;
+contract C { }
diff --git a/test/cmdlineTests/yul_optimizer_disabled_sequence_empty/output b/test/cmdlineTests/yul_optimizer_disabled_sequence_empty/output
@@ -0,0 +1,4 @@
+
+======= yul_optimizer_disabled_sequence_empty/input.sol:C =======
+Metadata:
+{"compiler":{"version": "<VERSION REMOVED>"},"language":"Solidity","output":{"abi":[],"devdoc":{"kind":"dev","methods":{},"version":1},"userdoc":{"kind":"user","methods":{},"version":1}},"settings":{"compilationTarget":{"yul_optimizer_disabled_sequence_empty/input.sol":"C"},"evmVersion":"cancun","libraries":{},"metadata":{"bytecodeHash":"ipfs"},"optimizer":{"details":{"constantOptimizer":false,"cse":false,"deduplicate":false,"inliner":false,"jumpdestRemover":true,"orderLiterals":false,"peephole":true,"simpleCounterForLoopUncheckedIncrement":true,"yul":false,"yulDetails":{"optimizerSteps":":"}},"runs":200},"remappings":[]},"sources":{"yul_optimizer_disabled_sequence_empty/input.sol":{"keccak256":"0xc2db3500808896ce1e69de2fe20cecab7ae2ffbb47cdf6ba8321296d95f49fc5","license":"GPL-3.0","urls":["bzz-raw://fde21393c068cd9f2d2b10ba4782db54f6f1c9a725074b17fa742531076be8a4","dweb:/ipfs/QmeTD6mR7YrWNyRowKRS7xs6cJNeMF3T49GAHzGM1bquyM"]}},"version":1}
diff --git a/test/cmdlineTests/yul_optimizer_steps_disabled/err b/test/cmdlineTests/yul_optimizer_steps_disabled/err
@@ -1 +1 @@
-Error: --yul-optimizations is invalid with a non-empty sequence if Yul optimizer is disabled.
+Error: --yul-optimizations is invalid with a non-empty sequence if Yul optimizer is disabled. Note that the empty optimizer sequence is properly denoted by ":".
diff --git a/test/solc/CommandLineInterface.cpp b/test/solc/CommandLineInterface.cpp
@@ -241,6 +241,29 @@ BOOST_AUTO_TEST_CASE(cli_input)
 	BOOST_CHECK_EQUAL(result.reader.allowedDirectories(), expectedAllowedPaths);
 }
 
+BOOST_AUTO_TEST_CASE(cli_optimizer_disabled_yul_optimization_input_whitespaces_or_empty)
+{
+	TemporaryDirectory tempDir(TEST_CASE_NAME);
+	createFilesWithParentDirs({tempDir.path() / "input.sol"});
+	createFilesWithParentDirs({tempDir.path() / "input.yul"});
+
+	std::string const expectedMessage =
+		"--yul-optimizations is invalid with a non-empty sequence if Yul optimizer is disabled."
+		" Note that the empty optimizer sequence is properly denoted by \":\".";
+	std::vector<std::vector<std::string>> const commandVariations = {
+		{"solc", "--strict-assembly", "--yul-optimizations", "", (tempDir.path() / "input.yul").string()},
+		{"solc", "--strict-assembly", "--yul-optimizations", "   ", (tempDir.path() / "input.yul").string()},
+		{"solc", "--yul-optimizations", "", (tempDir.path() / "input.sol").string()},
+		{"solc", "--yul-optimizations", "   ", (tempDir.path() / "input.sol").string()},
+	};
+	for (auto const& command: commandVariations)
+		BOOST_CHECK_EXCEPTION(
+			parseCommandLineAndReadInputFiles(command),
+			CommandLineValidationError,
+			[&](auto const& _exception) { BOOST_TEST(_exception.what() == expectedMessage); return true; }
+		);
+}
+
 BOOST_AUTO_TEST_CASE(cli_ignore_missing_some_files_exist)
 {
 	TemporaryDirectory tempDir1(TEST_CASE_NAME);

diff --git a/test/solc/CommandLineParser.cpp b/test/solc/CommandLineParser.cpp
@@ -498,6 +498,9 @@ BOOST_AUTO_TEST_CASE(valid_optimiser_sequences)
 {
 	std::vector<std::string> validSequenceInputs {
 		":",                         // Empty optimization sequence and empty cleanup sequence
+		" : ",                       // whitespaces only optimization sequence and whitespaces only cleanup sequence
+		": ",                        // Empty optimization sequence and whitespaces only cleanup sequence
+		" :",                        // whitespaces only optimization sequence and empty cleanup sequence
 		":fDn",                      // Empty optimization sequence and specified cleanup sequence
 		"dhfoDgvulfnTUtnIf:",        // Specified optimization sequence and empty cleanup sequence
 		"dhfoDgvulfnTUtnIf:fDn",     // Specified optimization sequence and cleanup sequence
@@ -509,6 +512,9 @@ BOOST_AUTO_TEST_CASE(valid_optimiser_sequences)
 
 	std::vector<std::tuple<std::string, std::string>> const expectedParsedSequences {
 		{"", ""},
+		{" ", " "},
+		{"", " "},
+		{" ", ""},
 		{"", "fDn"},
 		{"dhfoDgvulfnTUtnIf", ""},
 		{"dhfoDgvulfnTUtnIf", "fDn"},
@@ -600,11 +606,26 @@ BOOST_AUTO_TEST_CASE(valid_empty_optimizer_sequences_without_optimize)
 
 BOOST_AUTO_TEST_CASE(invalid_optimizer_sequence_without_optimize)
 {
-	std::string const invalidSequence{"u: "};
-	std::string const expectedErrorMessage{"--yul-optimizations is invalid with a non-empty sequence if Yul optimizer is disabled."};
-	std::vector<std::string> commandLineOptions{"solc", "contract.sol", "--yul-optimizations=" + invalidSequence};
+	std::vector<std::string> const invalidSequenceInputs {
+		{" "},
+		{"u: "},
+		{"u:"},
+		{":f"},
+		{" :f"}
+	};
+
+	std::string const expectedErrorMessage{
+		"--yul-optimizations is invalid with a non-empty sequence if Yul optimizer is disabled."
+		" Note that the empty optimizer sequence is properly denoted by \":\"."
+	};
+
 	auto hasCorrectMessage = [&](CommandLineValidationError const& _exception) { return _exception.what() == expectedErrorMessage; };
-	BOOST_CHECK_EXCEPTION(parseCommandLine(commandLineOptions), CommandLineValidationError, hasCorrectMessage);
+
+	for (auto const& invalidSequence: invalidSequenceInputs)
+	{
+		std::vector<std::string> commandLineOptions{"solc", "contract.sol", "--yul-optimizations=" + invalidSequence};
+		BOOST_CHECK_EXCEPTION(parseCommandLine(commandLineOptions), CommandLineValidationError, hasCorrectMessage);
+	}
 }
 
 BOOST_AUTO_TEST_SUITE_END()