DataFlowAnalyzer: Fix variable clearing nondeterminism (affecting CSE…

… and bytecode reproducibility) due to set insertions during iteration

Changelog.md

@@ -12,6 +12,7 @@ Compiler Features:
 
 Bugfixes:
  * AST: Fix wrong initial ID for Yul nodes in the AST.
+ * Common Subexpression Eliminator: Fix replacement decisions being affected by Yul variable names generated by the compiler, resulting in different (but equivalent) bytecode in some situations.
  * 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.

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/input1.yul

@@ -0,0 +1,43 @@
+object "C" {
+    code {}
+
+    object "C_deployed" {
+        code {
+            sstore(0, main(sload(0), sload(0), sload(0), sload(0)))
+
+            function main(a, d, b, c) -> v {
+                for {} 1 {}
+                {
+                    if iszero(b) { break }
+
+                    let mid := avg(b, c)
+                    switch gt(1, c)
+                    case 0 {
+                        b := cadd(mid, 0)
+                    }
+                    default {
+                        sstore(0x20, mid)
+                    }
+                }
+            }
+
+            function f(x) -> r {}
+
+            function avg(x, y) -> var {
+                // In the other file `_1` is called `_2`.
+                let _1 := add(x, y)
+                var := add(_1, _1)
+            }
+
+            function cadd(x, y) -> sum {
+                sum := add(x, y)
+
+                if gt(0, sum) {
+                    mstore(0, 0)
+                    mstore(0, 0)
+                    revert(0, 0)
+                }
+            }
+        }
+    }
+}

test/cmdlineTests/~name_dependent_cse_bug/input2.yul

@@ -0,0 +1,43 @@
+object "C" {
+    code {}
+
+    object "C_deployed" {
+        code {
+            sstore(0, main(sload(0), sload(0), sload(0), sload(0)))
+
+            function main(a, d, b, c) -> v {
+                for {} 1 {}
+                {
+                    if iszero(b) { break }
+
+                    let mid := avg(b, c)
+                    switch gt(1, c)
+                    case 0 {
+                        b := cadd(mid, 0)
+                    }
+                    default {
+                        sstore(0x20, mid)
+                    }
+                }
+            }
+
+            function f(x) -> r {}
+
+            function avg(x, y) -> var {
+                // In the other file `_2` is called `_1`.
+                let _2 := add(x, y)
+                var := add(_2, _2)
+            }
+
+            function cadd(x, y) -> sum {
+                sum := add(x, y)
+
+                if gt(0, sum) {
+                    mstore(0, 0)
+                    mstore(0, 0)
+                    revert(0, 0)
+                }
+            }
+        }
+    }
+}

test/cmdlineTests/~name_dependent_cse_bug/test.sh

@@ -0,0 +1,27 @@
+#!/usr/bin/env bash
+set -eo pipefail
+
+# This is a regression test against https://github.com/ethereum/solidity/issues/14494
+# Note that the two input files in this test differ only by the name of a single variable.
+# 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 {
+    local input_file="$1"
+    msg_on_error --no-stderr \
+        "$SOLC" --strict-assembly "$input_file" --optimize --debug-info none |
+            stripCLIDecorations
+}
+
+diff_values \
+    "$(assemble "${SCRIPT_DIR}/input1.yul")" \
+    "$(assemble "${SCRIPT_DIR}/input2.yul")"

test/libsolidity/semanticTests/externalContracts/prbmath_unsigned.sol

@@ -48,7 +48,7 @@ contract test {
 }
 // ----
 // constructor()
-// gas irOptimized: 1722598
+// gas irOptimized: 1722178
 // 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: 406643
 // gas legacy: 631753
 // gas legacyOptimized: 459425
 // prb_pi() -> 3141592656369545286

test/libsolidity/semanticTests/externalContracts/strings.sol

@@ -49,7 +49,7 @@ contract test {
 }
 // ----
 // constructor()
-// gas irOptimized: 634316
+// gas irOptimized: 633668
 // 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