Merge pull request #8082 from ethereum/fixRedundantContinue

Fix redundant assignment removal in combination with break / continue.

Changelog.md

@@ -7,6 +7,7 @@ Compiler Features:
 
 
 Bugfixes:
+ * Yul Optimizer: Fix bug in redundant assignment remover in combination with break and continue statements.
 
 
 

docs/bugs.json

@@ -1,4 +1,15 @@
 [
+    {
+        "name": "YulOptimizerRedundantAssignmentBreakContinue",
+        "summary": "The Yul optimizer can remove essential assignments to variables declared inside for loops when Yul's continue or break statement is used. You are unlikely to be affected if you do not use inline assembly with for loops and continue and break statements.",
+        "description": "The Yul optimizer has a stage that removes assignments to variables that are overwritten again or are not used in all following control-flow branches. This logic incorrectly removes such assignments to variables declared inside a for loop if they can be removed in a control-flow branch that ends with ``break`` or ``continue`` even though they cannot be removed in other control-flow branches. Variables declared outside of the respective for loop are not affected.",
+        "introduced": "0.6.0",
+        "fixed": "0.6.1",
+        "severity": "medium",
+        "conditions": {
+            "yulOptimizer": true
+        }
+    },
     {
         "name": "ABIEncoderV2LoopYulOptimizer",
         "summary": "If both the experimental ABIEncoderV2 and the experimental Yul optimizer are activated, one component of the Yul optimizer may reuse data in memory that has been changed in the meantime.",

docs/bugs_by_version.json

@@ -856,7 +856,9 @@
         "released": "2019-05-28"
     },
     "0.6.0": {
-        "bugs": [],
+        "bugs": [
+            "YulOptimizerRedundantAssignmentBreakContinue"
+        ],
         "released": "2019-12-17"
     }
 }

libyul/optimiser/RedundantAssignEliminator.cpp

@@ -280,29 +280,28 @@ void RedundantAssignEliminator::changeUndecidedTo(YulString _variable, Redundant
 
 void RedundantAssignEliminator::finalize(YulString _variable, RedundantAssignEliminator::State _finalState)
 {
-	finalize(m_assignments, _variable, _finalState);
-	for (auto& assignments: m_forLoopInfo.pendingBreakStmts)
-		finalize(assignments, _variable, _finalState);
-	for (auto& assignments: m_forLoopInfo.pendingContinueStmts)
-		finalize(assignments, _variable, _finalState);
-}
+	std::map<Assignment const*, State> assignments;
+	joinMap(assignments, std::move(m_assignments[_variable]), State::join);
+	m_assignments.erase(_variable);
 
-void RedundantAssignEliminator::finalize(
-	TrackedAssignments& _assignments,
-	YulString _variable,
-	RedundantAssignEliminator::State _finalState
-)
-{
-	for (auto const& assignment: _assignments[_variable])
+	for (auto& breakAssignments: m_forLoopInfo.pendingBreakStmts)
+	{
+		joinMap(assignments, std::move(breakAssignments[_variable]), State::join);
+		breakAssignments.erase(_variable);
+	}
+	for (auto& continueAssignments: m_forLoopInfo.pendingContinueStmts)
+	{
+		joinMap(assignments, std::move(continueAssignments[_variable]), State::join);
+		continueAssignments.erase(_variable);
+	}
+
+	for (auto const& assignment: assignments)
 	{
 		State const state = assignment.second == State::Undecided ? _finalState : assignment.second;
 
 		if (state == State::Unused && SideEffectsCollector{*m_dialect, *assignment.first->value}.movable())
-			// TODO the only point where we actually need this
-			// to be a set is for the for loop
 			m_pendingRemovals.insert(assignment.first);
 	}
-	_assignments.erase(_variable);
 }
 
 void AssignmentRemover::operator()(Block& _block)

libyul/optimiser/RedundantAssignEliminator.h

@@ -161,8 +161,6 @@ private:
 	/// assignments to the final state. In this case, this also applies to pending
 	/// break and continue TrackedAssignments.
 	void finalize(YulString _variable, State _finalState);
-	/// Helper function for the above.
-	void finalize(TrackedAssignments& _assignments, YulString _variable, State _finalState);
 
 	Dialect const* m_dialect;
 	std::set<YulString> m_declaredVariables;

test/libsolidity/SolidityEndToEndTest.cpp

@@ -14319,8 +14319,6 @@ BOOST_AUTO_TEST_CASE(event_wrong_abi_name)
 	)";
 	compileAndRun(sourceCode, 0, "ClientReceipt", bytes());
 	compileAndRun(sourceCode, 0, "Test", bytes(), map<string, Address>{{"ClientReceipt", m_contractAddress}});
-	u256 value(18);
-	u256 id(0x1234);
 
 	callContractFunction("f()");
 	BOOST_REQUIRE_EQUAL(numLogs(), 1);

test/libyul/yulOptimizerTests/redundantAssignEliminator/remove_break.yul

@@ -0,0 +1,26 @@
+{
+    let i := 0
+    for {} lt(i, 2) { i := add(i, 1) }
+    {
+        let x
+        x := 1337
+        if lt(i,1) {
+            x := 42
+            break
+        }
+        mstore(0, x)
+    }
+}
+// ====
+// step: redundantAssignEliminator
+// ----
+// {
+//     let i := 0
+//     for { } lt(i, 2) { i := add(i, 1) }
+//     {
+//         let x
+//         x := 1337
+//         if lt(i, 1) { break }
+//         mstore(0, x)
+//     }
+// }

test/libyul/yulOptimizerTests/redundantAssignEliminator/remove_continue.yul

@@ -0,0 +1,27 @@
+{
+    let i := 0
+    for {} lt(i, 2) { i := add(i, 1) }
+    {
+        let x
+        x := 1337
+        if lt(i,1) {
+            x := 42
+            continue
+        }
+        mstore(0, x)
+    }
+}
+
+// ====
+// step: redundantAssignEliminator
+// ----
+// {
+//     let i := 0
+//     for { } lt(i, 2) { i := add(i, 1) }
+//     {
+//         let x
+//         x := 1337
+//         if lt(i, 1) { continue }
+//         mstore(0, x)
+//     }
+// }