Merge pull request #12572 from ethereum/fixStackShuffling

Improved stack shuffling in corner cases.

Author: ekpyron

Changelog.md

@@ -25,7 +25,7 @@ Bugfixes:
  * IR Generator: Fix IR syntax error when copying storage arrays of structs containing functions.
  * Natspec: Fix ICE when overriding a struct getter with a Natspec-documented return value and the name in the struct is different.
  * TypeChecker: Fix ICE when a constant variable declaration forward references a struct.
-
+ * Yul EVM Code Transform: Improved stack shuffling in corner cases.
 
 Solc-Js:
  * The wrapper now requires at least nodejs v10.

libyul/backends/evm/StackHelpers.h

@@ -262,6 +262,11 @@ class Shuffler
 							if (ops.sourceMultiplicity(ops.sourceSize() - 1 - swapDepth) < 0)
 							{
 								ops.swap(swapDepth);
+								if (ops.targetIsArbitrary(sourceTop))
+									// Usually we keep a slot that is to-be-removed, if the current top is arbitrary.
+									// However, since we are in a stack-too-deep situation, pop it immediately
+									// to compress the stack (we can always push back junk in the end).
+									ops.pop();
 								return true;
 							}
 						// Otherwise we rely on stack compression or stack-to-memory.
@@ -321,14 +326,44 @@ class Shuffler
 			yulAssert(ops.sourceMultiplicity(i) == 0 && (ops.targetIsArbitrary(i) || ops.targetMultiplicity(i) == 0), "");
 		yulAssert(ops.isCompatible(sourceTop, sourceTop), "");
 
+		auto swappableOffsets = ranges::views::iota(size > 17 ? size - 17 : 0u, size);
+
 		// If we find a lower slot that is out of position, but also compatible with the top, swap that up.
+		for (size_t offset: swappableOffsets)
+			if (!ops.isCompatible(offset, offset) && ops.isCompatible(sourceTop, offset))
+			{
+				ops.swap(size - offset - 1);
+				return true;
+			}
+		// Swap up any reachable slot that is still out of position.
+		for (size_t offset: swappableOffsets)
+			if (!ops.isCompatible(offset, offset) && !ops.sourceIsSame(offset, sourceTop))
+			{
+				ops.swap(size - offset - 1);
+				return true;
+			}
+		// We are in a stack-too-deep situation and try to reduce the stack size.
+		// If the current top is merely kept since the target slot is arbitrary, pop it.
+		if (ops.targetIsArbitrary(sourceTop) && ops.sourceMultiplicity(sourceTop) <= 0)
+		{
+			ops.pop();
+			return true;
+		}
+		// If any reachable slot is merely kept, since the target slot is arbitrary, swap it up and pop it.
+		for (size_t offset: swappableOffsets)
+			if (ops.targetIsArbitrary(offset) && ops.sourceMultiplicity(offset) <= 0)
+			{
+				ops.swap(size - offset - 1);
+				ops.pop();
+				return true;
+			}
+		// We cannot avoid a stack-too-deep error. Repeat the above without restricting to reachable slots.
 		for (size_t offset: ranges::views::iota(0u, size))
 			if (!ops.isCompatible(offset, offset) && ops.isCompatible(sourceTop, offset))
 			{
 				ops.swap(size - offset - 1);
 				return true;
 			}
-		// Swap up any slot that is still out of position.
 		for (size_t offset: ranges::views::iota(0u, size))
 			if (!ops.isCompatible(offset, offset) && !ops.sourceIsSame(offset, sourceTop))
 			{

test/CMakeLists.txt

@@ -147,6 +147,7 @@ set(libyul_sources
     libyul/Parser.cpp
     libyul/StackLayoutGeneratorTest.cpp
     libyul/StackLayoutGeneratorTest.h
+    libyul/StackShufflingTest.cpp
     libyul/SyntaxTest.h
     libyul/SyntaxTest.cpp
     libyul/YulInterpreterTest.cpp

test/libyul/StackShufflingTest.cpp

@@ -0,0 +1,54 @@
+/*
+    This file is part of solidity.
+
+    solidity is free software: you can redistribute it and/or modify
+    it under the terms of the GNU General Public License as published by
+    the Free Software Foundation, either version 3 of the License, or
+    (at your option) any later version.
+
+    solidity is distributed in the hope that it will be useful,
+    but WITHOUT ANY WARRANTY; without even the implied warranty of
+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+    GNU General Public License for more details.
+
+    You should have received a copy of the GNU General Public License
+    along with solidity.  If not, see <http://www.gnu.org/licenses/>.
+*/
+/**
+ * Unit tests for stack shuffling.
+ */
+#include <libyul/backends/evm/StackHelpers.h>
+#include <boost/test/unit_test.hpp>
+
+using namespace std;
+using namespace solidity::langutil;
+
+namespace solidity::yul::test
+{
+
+BOOST_AUTO_TEST_SUITE(YulStackShuffling)
+
+BOOST_AUTO_TEST_CASE(swap_cycle)
+{
+	std::vector<Scope::Variable> scopeVariables;
+	Scope::Function function;
+	std::vector<VariableSlot> v;
+	for (size_t i = 0; i < 17; ++i)
+		scopeVariables.emplace_back(Scope::Variable{""_yulstring, YulString{"v" + to_string(i)}});
+	for (size_t i = 0; i < 17; ++i)
+		v.emplace_back(VariableSlot{scopeVariables[i]});
+
+	Stack sourceStack{
+		v[0], v[1], v[2], v[3], v[4], v[5], v[6], v[7], v[9], v[10], v[11], v[12], v[13], v[14], v[15], v[16],
+		FunctionReturnLabelSlot{function}, FunctionReturnLabelSlot{function}, v[5]};
+	Stack targetStack{
+		v[1], v[0], v[2], v[3], v[4], v[5], v[6], v[7], v[9], v[10], v[11], v[12], v[13], v[14], v[15], v[16],
+		FunctionReturnLabelSlot{function}, JunkSlot{}, JunkSlot{}
+	};
+	// Used to hit a swapping cycle.
+	createStackLayout(sourceStack, targetStack, [](auto){}, [](auto){}, [](){});
+}
+
+BOOST_AUTO_TEST_SUITE_END()
+
+}