Merge pull request #14562 from ethereum/fix-viaIR-codegen-import-bug

Fix Via-IR bytecode divergence when compiling multiple files that are already included via imports

Author: cameel

Changelog.md

@@ -12,6 +12,7 @@ Compiler Features:
 
 Bugfixes:
  * AST: Fix wrong initial ID for Yul nodes in the AST.
+ * Code Generator: Fix output from via-IR code generator being dependent on which files were discovered by import callback. In some cases, a different AST ID assignment would alter the order of functions in internal dispatch, resulting in superficially different but semantically equivalent bytecode.
  * 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.

libsolidity/codegen/ir/IRGenerationContext.cpp

@@ -31,6 +31,7 @@
 #include <libsolutil/StringUtils.h>
 
 #include <range/v3/view/map.hpp>
+#include <range/v3/algorithm/find.hpp>
 
 using namespace solidity;
 using namespace solidity::util;
@@ -41,7 +42,7 @@ std::string IRGenerationContext::enqueueFunctionForCodeGeneration(FunctionDefini
 	std::string name = IRNames::function(_function);
 
 	if (!m_functions.contains(name))
-		m_functionGenerationQueue.insert(&_function);
+		m_functionGenerationQueue.push_back(&_function);
 
 	return name;
 }
@@ -50,8 +51,8 @@ FunctionDefinition const* IRGenerationContext::dequeueFunctionForCodeGeneration(
 {
 	solAssert(!m_functionGenerationQueue.empty(), "");
 
-	FunctionDefinition const* result = *m_functionGenerationQueue.begin();
-	m_functionGenerationQueue.erase(m_functionGenerationQueue.begin());
+	FunctionDefinition const* result = m_functionGenerationQueue.front();
+	m_functionGenerationQueue.pop_front();
 	return result;
 }
 
@@ -132,7 +133,7 @@ void IRGenerationContext::initializeInternalDispatch(InternalDispatchMap _intern
 {
 	solAssert(internalDispatchClean(), "");
 
-	for (DispatchSet const& functions: _internalDispatch | ranges::views::values)
+	for (DispatchQueue const& functions: _internalDispatch | ranges::views::values)
 		for (auto function: functions)
 			enqueueFunctionForCodeGeneration(*function);
 
@@ -149,16 +150,15 @@ InternalDispatchMap IRGenerationContext::consumeInternalDispatchMap()
 void IRGenerationContext::addToInternalDispatch(FunctionDefinition const& _function)
 {
 	FunctionType const* functionType = TypeProvider::function(_function, FunctionType::Kind::Internal);
-	solAssert(functionType, "");
+	solAssert(functionType);
 
 	YulArity arity = YulArity::fromType(*functionType);
-
-	if (m_internalDispatchMap.count(arity) != 0 && m_internalDispatchMap[arity].count(&_function) != 0)
-		// Note that m_internalDispatchMap[arity] is a set with a custom comparator, which looks at function IDs not definitions
-		solAssert(*m_internalDispatchMap[arity].find(&_function) == &_function, "Different definitions with the same function ID");
-
-	m_internalDispatchMap[arity].insert(&_function);
-	enqueueFunctionForCodeGeneration(_function);
+	DispatchQueue& dispatchQueue = m_internalDispatchMap[arity];
+	if (ranges::find(dispatchQueue, &_function) == ranges::end(dispatchQueue))
+	{
+		dispatchQueue.push_back(&_function);
+		enqueueFunctionForCodeGeneration(_function);
+	}
 }
 
 

libsolidity/codegen/ir/IRGenerationContext.h

@@ -37,28 +37,15 @@
 #include <set>
 #include <string>
 #include <memory>
-#include <vector>
 
 namespace solidity::frontend
 {
 
 class YulUtilFunctions;
 class ABIFunctions;
 
-struct AscendingFunctionIDCompare
-{
-	bool operator()(FunctionDefinition const* _f1, FunctionDefinition const* _f2) const
-	{
-		// NULLs always first.
-		if (_f1 != nullptr && _f2 != nullptr)
-			return _f1->id() < _f2->id();
-		else
-			return _f1 == nullptr;
-	}
-};
-
-using DispatchSet = std::set<FunctionDefinition const*, AscendingFunctionIDCompare>;
-using InternalDispatchMap = std::map<YulArity, DispatchSet>;
+using DispatchQueue = std::deque<FunctionDefinition const*>;
+using InternalDispatchMap = std::map<YulArity, DispatchQueue>;
 
 /**
  * Class that contains contextual information during IR generation.
@@ -197,10 +184,9 @@ class IRGenerationContext
 	/// were discovered by the IR generator during AST traversal.
 	/// Note that the queue gets filled in a lazy way - new definitions can be added while the
 	/// collected ones get removed and traversed.
-	/// The order and duplicates are irrelevant here (hence std::set rather than std::queue) as
-	/// long as the order of Yul functions in the generated code is deterministic and the same on
-	/// all platforms - which is a property guaranteed by MultiUseYulFunctionCollector.
-	DispatchSet m_functionGenerationQueue;
+	/// The order and duplicates are relevant here
+	/// (see: IRGenerationContext::[enqueue|dequeue]FunctionForCodeGeneration)
+	DispatchQueue m_functionGenerationQueue;
 
 	/// Collection of functions that need to be callable via internal dispatch.
 	/// Note that having a key with an empty set of functions is a valid situation. It means that

libsolidity/codegen/ir/IRGenerator.cpp

@@ -279,6 +279,7 @@ InternalDispatchMap IRGenerator::generateInternalDispatchFunctions(ContractDefin
 			templ("out", suffixedVariableNameList("out_", 0, arity.out));
 
 			std::vector<std::map<std::string, std::string>> cases;
+			std::set<int64_t> caseValues;
 			for (FunctionDefinition const* function: internalDispatchMap.at(arity))
 			{
 				solAssert(function, "");
@@ -289,12 +290,14 @@ InternalDispatchMap IRGenerator::generateInternalDispatchFunctions(ContractDefin
 				solAssert(!function->isConstructor(), "");
 				// 0 is reserved for uninitialized function pointers
 				solAssert(function->id() != 0, "Unexpected function ID: 0");
+				solAssert(caseValues.count(function->id()) == 0, "Duplicate function ID");
 				solAssert(m_context.functionCollector().contains(IRNames::function(*function)), "");
 
 				cases.emplace_back(std::map<std::string, std::string>{
 					{"funID", std::to_string(m_context.mostDerivedContract().annotation().internalFunctionIDs.at(function))},
 					{"name", IRNames::function(*function)}
 				});
+				caseValues.insert(function->id());
 			}
 
 			templ("cases", std::move(cases));

test/libsolidity/semanticTests/abiEncoderV1/abi_encode_calldata_slice.sol

@@ -63,6 +63,6 @@ contract C {
 // gas legacy: 411269
 // gas legacyOptimized: 317754
 // test_uint256() ->
-// gas irOptimized: 509677
+// gas irOptimized: 509479
 // gas legacy: 577469
 // gas legacyOptimized: 441003

test/libsolidity/semanticTests/abiEncoderV2/abi_encode_calldata_slice.sol

@@ -64,6 +64,6 @@ contract C {
 // gas legacy: 411269
 // gas legacyOptimized: 317754
 // test_uint256() ->
-// gas irOptimized: 509677
+// gas irOptimized: 509479
 // gas legacy: 577469
 // gas legacyOptimized: 441003

test/libsolidity/semanticTests/externalContracts/base64.sol

@@ -33,7 +33,7 @@ contract test {
 // EVMVersion: >=constantinople
 // ----
 // constructor()
-// gas irOptimized: 406679
+// gas irOptimized: 406691
 // gas legacy: 737652
 // gas legacyOptimized: 527036
 // encode_inline_asm(bytes): 0x20, 0 -> 0x20, 0

test/libsolidity/semanticTests/externalContracts/prbmath_unsigned.sol

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

test/libsolidity/semanticTests/externalContracts/strings.sol

@@ -49,7 +49,7 @@ contract test {
 }
 // ----
 // constructor()
-// gas irOptimized: 634316
+// gas irOptimized: 634328
 // gas legacy: 1065857
 // gas legacyOptimized: 725423
 // toSlice(string): 0x20, 11, "hello world" -> 11, 0xa0

test/libsolidity/semanticTests/functionTypes/duplicated_function_definition_with_same_id_in_internal_dispatcher.sol

@@ -0,0 +1,9 @@
+contract C {
+    function a() internal {}
+    function f() public {
+        function() ptr1 = a;
+        function() ptr2 = a;
+    }
+}
+// ----
+// f()