Merge pull request #14574 from ethereum/fix-data-flow-analyzer-inconsistent-variable-clearing

Fix variable clearing in DataFlowAnalyzer being affected by variable names

Author: ekpyron

Changelog.md

@@ -16,6 +16,7 @@ Bugfixes:
  * NatSpec: Fix internal error when requesting userdoc or devdoc for a contract that emits an event defined in a foreign contract or interface.
  * SMTChecker: Fix encoding error that causes loops to unroll after completion.
  * SMTChecker: Fix inconsistency on constant condition checks when ``while`` or ``for`` loops are unrolled before the condition check.
+ * Yul Optimizer: Fix replacement decisions during CSE being affected by Yul variable names generated by the compiler, resulting in different (but equivalent) bytecode in some situations.
 
 
 ### 0.8.21 (2023-07-19)

libyul/optimiser/DataFlowAnalyzer.cpp

@@ -351,13 +351,14 @@ void DataFlowAnalyzer::clearValues(std::set<YulString> _variables)
 	});
 
 	// Also clear variables that reference variables to be cleared.
+	std::set<YulString> referencingVariables;
 	for (auto const& variableToClear: _variables)
 		for (auto const& [ref, names]: m_state.references)
 			if (names.count(variableToClear))
-				_variables.emplace(ref);
+				referencingVariables.emplace(ref);
 
 	// Clear the value and update the reference relation.
-	for (auto const& name: _variables)
+	for (auto const& name: _variables + referencingVariables)
 	{
 		m_state.value.erase(name);
 		m_state.references.erase(name);

test/cmdlineTests/~name_dependent_cse_bug/cse_bug.yul

@@ -0,0 +1,30 @@
+object "C" {
+    code {}
+
+    object "C_deployed" {
+        code {
+            main(0, 0)
+
+            function main(a, b) {
+                for {} 1 {}
+                {
+                    if iszero(a) { break }
+
+                    let mid := avg(a, a)
+                    switch a
+                    case 0 {
+                        a := mid
+                    }
+                    default {
+                        sstore(0, mid)
+                    }
+                }
+            }
+
+            function avg(x, y) -> var {
+                let __placeholder__ := add(x, y)
+                var := add(__placeholder__, __placeholder__)
+            }
+        }
+    }
+}

test/cmdlineTests/~name_dependent_cse_bug/test.sh

@@ -0,0 +1,28 @@
+#!/usr/bin/env bash
+set -eo pipefail
+
+# This is a regression test against https://github.com/ethereum/solidity/issues/14494
+# Due to the bug, a decision about which variable to use to replace a subexpression in CSE would
+# depend on sorting order of variable names. A variable not being used as a replacement could lead
+# to it being unused in general and removed by Unused Pruner. This would show up as a difference
+# in the bytecode.
+
+# shellcheck source=scripts/common.sh
+source "${REPO_ROOT}/scripts/common.sh"
+# shellcheck source=scripts/common_cmdline.sh
+source "${REPO_ROOT}/scripts/common_cmdline.sh"
+
+SCRIPT_DIR=$(cd "$(dirname "$0")" && pwd)
+
+function assemble_with_variable_name {
+    local input_file="$1"
+    local variable_name="$2"
+
+    sed -e "s|__placeholder__|${variable_name}|g" "$input_file" | msg_on_error --no-stderr \
+        "$SOLC" --strict-assembly - --optimize --debug-info none |
+            stripCLIDecorations
+}
+
+diff_values \
+    "$(assemble_with_variable_name "${SCRIPT_DIR}/cse_bug.yul" _1)" \
+    "$(assemble_with_variable_name "${SCRIPT_DIR}/cse_bug.yul" _2)"

test/libsolidity/semanticTests/externalContracts/prbmath_unsigned.sol

@@ -48,7 +48,7 @@ contract test {
 }
 // ----
 // constructor()
-// gas irOptimized: 1722610
+// gas irOptimized: 1722166
 // gas legacy: 2210160
 // gas legacyOptimized: 1734152
 // div(uint256,uint256): 3141592653589793238, 88714123 -> 35412542528203691288251815328

test/libsolidity/semanticTests/externalContracts/ramanujan_pi.sol

@@ -33,7 +33,7 @@ contract test {
 }
 // ----
 // constructor()
-// gas irOptimized: 407075
+// gas irOptimized: 406595
 // gas legacy: 631753
 // gas legacyOptimized: 459425
 // prb_pi() -> 3141592656369545286

test/libsolidity/semanticTests/externalContracts/strings.sol

@@ -49,7 +49,7 @@ contract test {
 }
 // ----
 // constructor()
-// gas irOptimized: 634328
+// gas irOptimized: 633680
 // gas legacy: 1065857
 // gas legacyOptimized: 725423
 // toSlice(string): 0x20, 11, "hello world" -> 11, 0xa0
@@ -69,6 +69,6 @@ contract test {
 // gas legacy: 31621
 // gas legacyOptimized: 27914
 // benchmark(string,bytes32): 0x40, 0x0842021, 8, "solidity" -> 0x2020
-// gas irOptimized: 1981677
+// gas irOptimized: 1981664
 // gas legacy: 4235651
 // gas legacyOptimized: 2319622

test/libyul/yulOptimizerTests/fullSuite/name_dependent_cse_bug_part_1.yul

@@ -0,0 +1,45 @@
+{
+    main(0, 0)
+
+    function main(a, b) {
+        for {} 1 {}
+        {
+            if iszero(a) { break }
+
+            let mid := avg(a, a)
+            switch a
+            case 0 {
+                a := mid
+            }
+            default {
+                sstore(0, mid)
+            }
+        }
+    }
+
+    function avg(x, y) -> var {
+        // NOTE: Variable names should not affect CSE.
+        // This should not be optimized differently than name_dependent_cse_bug_part_2.yul.
+        // `let mid := var` should be present in both or missing in both.
+        let _1 := add(x, y)
+        var := add(_1, _1)
+    }
+}
+// ====
+// EVMVersion: >=shanghai
+// ----
+// step: fullSuite
+//
+// {
+//     {
+//         let a := 0
+//         for { } a { }
+//         {
+//             let _1 := add(a, a)
+//             let var := add(_1, _1)
+//             switch a
+//             case 0 { a := var }
+//             default { sstore(0, var) }
+//         }
+//     }
+// }

test/libyul/yulOptimizerTests/fullSuite/name_dependent_cse_bug_part_1_pre_shanghai.yul

@@ -0,0 +1,46 @@
+{
+    main(0, 0)
+
+    function main(a, b) {
+        for {} 1 {}
+        {
+            if iszero(a) { break }
+
+            let mid := avg(a, a)
+            switch a
+            case 0 {
+                a := mid
+            }
+            default {
+                sstore(0, mid)
+            }
+        }
+    }
+
+    function avg(x, y) -> var {
+        // NOTE: Variable names should not affect CSE.
+        // This should not be optimized differently than name_dependent_cse_bug_part_2_pre_shanghai.yul.
+        // `let mid := var` should be present in both or missing in both.
+        let _1 := add(x, y)
+        var := add(_1, _1)
+    }
+}
+// ====
+// EVMVersion: <shanghai
+// ----
+// step: fullSuite
+//
+// {
+//     {
+//         let a := 0
+//         let a_1 := 0
+//         for { } a_1 { }
+//         {
+//             let _1 := add(a_1, a_1)
+//             let var := add(_1, _1)
+//             switch a_1
+//             case 0 { a_1 := var }
+//             default { sstore(a, var) }
+//         }
+//     }
+// }

test/libyul/yulOptimizerTests/fullSuite/name_dependent_cse_bug_part_2.yul

@@ -0,0 +1,45 @@
+{
+    main(0, 0)
+
+    function main(a, b) {
+        for {} 1 {}
+        {
+            if iszero(a) { break }
+
+            let mid := avg(a, a)
+            switch a
+            case 0 {
+                a := mid
+            }
+            default {
+                sstore(0, mid)
+            }
+        }
+    }
+
+    function avg(x, y) -> var {
+        // NOTE: Variable names should not affect CSE.
+        // This should not be optimized differently than name_dependent_cse_bug_part_1.yul.
+        // `let mid := var` should be present in both or missing in both.
+        let _2 := add(x, y)
+        var := add(_2, _2)
+    }
+}
+// ====
+// EVMVersion: >=shanghai
+// ----
+// step: fullSuite
+//
+// {
+//     {
+//         let a := 0
+//         for { } a { }
+//         {
+//             let _1 := add(a, a)
+//             let var := add(_1, _1)
+//             switch a
+//             case 0 { a := var }
+//             default { sstore(0, var) }
+//         }
+//     }
+// }