Merge pull request #5716 from ethereum/fixDataSizeOffset

Fix data size offset and properly provide builtins.

Changelog.md

@@ -7,6 +7,7 @@ Compiler Features:
 
 
 Bugfixes:
+ * Yul: Check that arguments to ``dataoffset`` and ``datasize`` are literals at parse time and properly take this into account in the optimizer.
 
 
 Build System:

libyul/AsmAnalysis.cpp

@@ -299,11 +299,14 @@ bool AsmAnalyzer::operator()(FunctionCall const& _funCall)
 	bool success = true;
 	size_t parameters = 0;
 	size_t returns = 0;
+	bool needsLiteralArguments = false;
 	if (BuiltinFunction const* f = m_dialect->builtin(_funCall.functionName.name))
 	{
 		// TODO: compare types, too
 		parameters = f->parameters.size();
 		returns = f->returns.size();
+		if (f->literalArguments)
+			needsLiteralArguments = true;
 	}
 	else if (!m_currentScope->lookup(_funCall.functionName.name, Scope::Visitor(
 		[&](Scope::Variable const&)
@@ -347,8 +350,15 @@ bool AsmAnalyzer::operator()(FunctionCall const& _funCall)
 		}
 
 	for (auto const& arg: _funCall.arguments | boost::adaptors::reversed)
+	{
 		if (!expectExpression(arg))
 			success = false;
+		else if (needsLiteralArguments && arg.type() != typeid(Literal))
+			m_errorReporter.typeError(
+				_funCall.functionName.location,
+				"Function expects direct literals as arguments."
+			);
+	}
 	// Use argument size instead of parameter count to avoid misleading errors.
 	m_stackHeight += int(returns) - int(_funCall.arguments.size());
 	m_info.stackHeightInfo[&_funCall] = m_stackHeight;

libyul/Dialect.h

@@ -44,7 +44,13 @@ struct BuiltinFunction
 	YulString name;
 	std::vector<Type> parameters;
 	std::vector<Type> returns;
-	bool movable;
+	/// If true, calls to this function can be freely moved and copied (as long as their
+	/// arguments are either variables or also movable) without altering the semantics.
+	/// This means the function cannot depend on storage or memory, cannot have any side-effects,
+	/// but it can depend on state that is constant across an EVM-call.
+	bool movable = false;
+	/// If true, can only accept literals as arguments and they cannot be moved to voriables.
+	bool literalArguments = false;
 };
 
 struct Dialect: boost::noncopyable

libyul/backends/evm/EVMDialect.cpp

@@ -44,7 +44,7 @@ EVMDialect::EVMDialect(AsmFlavour _flavour, bool _objectAccess):
 	if (!m_objectAccess)
 		return;
 
-	addFunction("datasize", 1, 1, true, [this](
+	addFunction("datasize", 1, 1, true, true, [this](
 		FunctionCall const& _call,
 		AbstractAssembly& _assembly,
 		std::function<void()>
@@ -58,7 +58,7 @@ EVMDialect::EVMDialect(AsmFlavour _flavour, bool _objectAccess):
 		else
 			_assembly.appendDataSize(m_subIDs.at(dataName));
 	});
-	addFunction("dataoffset", 1, 1, true, [this](
+	addFunction("dataoffset", 1, 1, true, true, [this](
 		FunctionCall const& _call,
 		AbstractAssembly& _assembly,
 		std::function<void()>
@@ -72,7 +72,7 @@ EVMDialect::EVMDialect(AsmFlavour _flavour, bool _objectAccess):
 		else
 			_assembly.appendDataOffset(m_subIDs.at(dataName));
 	});
-	addFunction("datacopy", 3, 0, false, [](
+	addFunction("datacopy", 3, 0, false, false, [](
 		FunctionCall const&,
 		AbstractAssembly& _assembly,
 		std::function<void()> _visitArguments
@@ -128,6 +128,7 @@ void EVMDialect::addFunction(
 	size_t _params,
 	size_t _returns,
 	bool _movable,
+	bool _literalArguments,
 	std::function<void(FunctionCall const&, AbstractAssembly&, std::function<void()>)> _generateCode
 )
 {
@@ -137,5 +138,6 @@ void EVMDialect::addFunction(
 	f.parameters.resize(_params);
 	f.returns.resize(_returns);
 	f.movable = _movable;
+	f.literalArguments = _literalArguments;
 	f.generateCode = std::move(_generateCode);
 }

libyul/backends/evm/EVMDialect.h

@@ -72,6 +72,7 @@ private:
 		size_t _params,
 		size_t _returns,
 		bool _movable,
+		bool _literalArguments,
 		std::function<void(FunctionCall const&, AbstractAssembly&, std::function<void()>)> _generateCode
 	);
 

libyul/optimiser/CommonSubexpressionEliminator.cpp

@@ -25,6 +25,7 @@
 #include <libyul/optimiser/SyntacticalEquality.h>
 #include <libyul/Exceptions.h>
 #include <libyul/AsmData.h>
+#include <libyul/Dialect.h>
 
 using namespace std;
 using namespace dev;
@@ -32,12 +33,24 @@ using namespace yul;
 
 void CommonSubexpressionEliminator::visit(Expression& _e)
 {
+	bool descend = true;
+	// If this is a function call to a function that requires literal arguments,
+	// do not try to simplify there.
+	if (_e.type() == typeid(FunctionCall))
+		if (BuiltinFunction const* builtin = m_dialect.builtin(boost::get<FunctionCall>(_e).functionName.name))
+			if (builtin->literalArguments)
+				// We should not modify function arguments that have to be literals
+				// Note that replacing the function call entirely is fine,
+				// if the function call is movable.
+				descend = false;
+
 	// We visit the inner expression first to first simplify inner expressions,
 	// which hopefully allows more matches.
 	// Note that the DataFlowAnalyzer itself only has code for visiting Statements,
 	// so this basically invokes the AST walker directly and thus post-visiting
 	// is also fine with regards to data flow analysis.
-	DataFlowAnalyzer::visit(_e);
+	if (descend)
+		DataFlowAnalyzer::visit(_e);
 
 	if (_e.type() == typeid(Identifier))
 	{

libyul/optimiser/CommonSubexpressionEliminator.h

@@ -26,6 +26,8 @@
 namespace yul
 {
 
+struct Dialect;
+
 /**
  * Optimisation stage that replaces expressions known to be the current value of a variable
  * in scope by a reference to that variable.

libyul/optimiser/ExpressionSplitter.cpp

@@ -24,6 +24,7 @@
 #include <libyul/optimiser/ASTWalker.h>
 
 #include <libyul/AsmData.h>
+#include <libyul/Dialect.h>
 
 #include <libdevcore/CommonData.h>
 
@@ -43,6 +44,11 @@ void ExpressionSplitter::operator()(FunctionalInstruction& _instruction)
 
 void ExpressionSplitter::operator()(FunctionCall& _funCall)
 {
+	if (BuiltinFunction const* builtin = m_dialect.builtin(_funCall.functionName.name))
+		if (builtin->literalArguments)
+			// We cannot outline function arguments that have to be literals
+			return;
+
 	for (auto& arg: _funCall.arguments | boost::adaptors::reversed)
 		outlineExpression(arg);
 }

libyul/optimiser/ExpressionSplitter.h

@@ -31,6 +31,7 @@ namespace yul
 {
 
 class NameCollector;
+struct Dialect;
 
 
 /**
@@ -57,8 +58,8 @@ class NameCollector;
 class ExpressionSplitter: public ASTModifier
 {
 public:
-	explicit ExpressionSplitter(NameDispenser& _nameDispenser):
-		m_nameDispenser(_nameDispenser)
+	explicit ExpressionSplitter(Dialect const& _dialect, NameDispenser& _nameDispenser):
+		m_dialect(_dialect), m_nameDispenser(_nameDispenser)
 	{ }
 
 	void operator()(FunctionalInstruction&) override;
@@ -77,6 +78,7 @@ private:
 	/// List of statements that should go in front of the currently visited AST element,
 	/// at the statement level.
 	std::vector<Statement> m_statementsToPrefix;
+	Dialect const& m_dialect;
 	NameDispenser& m_nameDispenser;
 };
 

libyul/optimiser/Suite.cpp

@@ -67,7 +67,7 @@ void OptimiserSuite::run(
 
 	for (size_t i = 0; i < 4; i++)
 	{
-		ExpressionSplitter{dispenser}(ast);
+		ExpressionSplitter{_dialect, dispenser}(ast);
 		SSATransform::run(ast, dispenser);
 		RedundantAssignEliminator::run(_dialect, ast);
 		RedundantAssignEliminator::run(_dialect, ast);
@@ -90,7 +90,7 @@ void OptimiserSuite::run(
 		ExpressionInliner(_dialect, ast).run();
 		UnusedPruner::runUntilStabilised(_dialect, ast);
 
-		ExpressionSplitter{dispenser}(ast);
+		ExpressionSplitter{_dialect, dispenser}(ast);
 		SSATransform::run(ast, dispenser);
 		RedundantAssignEliminator::run(_dialect, ast);
 		RedundantAssignEliminator::run(_dialect, ast);

libyul/optimiser/Suite.h

@@ -41,7 +41,6 @@ public:
 		Dialect const& _dialect,
 		Block& _ast,
 		AsmAnalysisInfo const& _analysisInfo,
-
 		std::set<YulString> const& _externallyUsedIdentifiers = {}
 	);
 };

test/libyul/ObjectParser.cpp

@@ -250,6 +250,36 @@ BOOST_AUTO_TEST_CASE(to_string)
 	BOOST_CHECK_EQUAL(asmStack.print(), expectation);
 }
 
+BOOST_AUTO_TEST_CASE(arg_to_dataoffset_must_be_literal)
+{
+	string code = R"(
+		object "outer" {
+			code { let x := "outer" let y := dataoffset(x) }
+		}
+	)";
+	CHECK_ERROR(code, TypeError, "Function expects direct literals as arguments.");
+}
+
+BOOST_AUTO_TEST_CASE(arg_to_datasize_must_be_literal)
+{
+	string code = R"(
+		object "outer" {
+			code { let x := "outer" let y := datasize(x) }
+		}
+	)";
+	CHECK_ERROR(code, TypeError, "Function expects direct literals as arguments.");
+}
+
+BOOST_AUTO_TEST_CASE(args_to_datacopy_are_arbitrary)
+{
+	string code = R"(
+		object "outer" {
+			code { let x := 0 let y := 2 let s := 3 datacopy(x, y, s) }
+		}
+	)";
+	BOOST_CHECK(successParse(code));
+}
+
 BOOST_AUTO_TEST_SUITE_END()
 
 }

test/libyul/Parser.cpp

@@ -331,7 +331,7 @@ BOOST_AUTO_TEST_CASE(builtins_analysis)
 		{
 			return _name == "builtin"_yulstring ? &f : nullptr;
 		}
-		BuiltinFunction f{"builtin"_yulstring, vector<Type>(2), vector<Type>(3), false};
+		BuiltinFunction f{"builtin"_yulstring, vector<Type>(2), vector<Type>(3), false, false};
 	};
 
 	shared_ptr<Dialect> dialect = make_shared<SimpleDialect>();

test/libyul/YulOptimizerTest.cpp

@@ -122,7 +122,7 @@ bool YulOptimizerTest::run(ostream& _stream, string const& _linePrefix, bool con
 	else if (m_optimizerStep == "expressionSplitter")
 	{
 		NameDispenser nameDispenser{*m_dialect, *m_ast};
-		ExpressionSplitter{nameDispenser}(*m_ast);
+		ExpressionSplitter{*m_dialect, nameDispenser}(*m_ast);
 	}
 	else if (m_optimizerStep == "expressionJoiner")
 	{
@@ -133,7 +133,7 @@ bool YulOptimizerTest::run(ostream& _stream, string const& _linePrefix, bool con
 	{
 		disambiguate();
 		NameDispenser nameDispenser{*m_dialect, *m_ast};
-		ExpressionSplitter{nameDispenser}(*m_ast);
+		ExpressionSplitter{*m_dialect, nameDispenser}(*m_ast);
 		ExpressionJoiner::run(*m_ast);
 		ExpressionJoiner::run(*m_ast);
 	}
@@ -158,7 +158,7 @@ bool YulOptimizerTest::run(ostream& _stream, string const& _linePrefix, bool con
 		(FunctionHoister{})(*m_ast);
 		(FunctionGrouper{})(*m_ast);
 		NameDispenser nameDispenser{*m_dialect, *m_ast};
-		ExpressionSplitter{nameDispenser}(*m_ast);
+		ExpressionSplitter{*m_dialect, nameDispenser}(*m_ast);
 		FullInliner(*m_ast, nameDispenser).run();
 		ExpressionJoiner::run(*m_ast);
 	}
@@ -182,7 +182,7 @@ bool YulOptimizerTest::run(ostream& _stream, string const& _linePrefix, bool con
 	{
 		disambiguate();
 		NameDispenser nameDispenser{*m_dialect, *m_ast};
-		ExpressionSplitter{nameDispenser}(*m_ast);
+		ExpressionSplitter{*m_dialect, nameDispenser}(*m_ast);
 		CommonSubexpressionEliminator{*m_dialect}(*m_ast);
 		ExpressionSimplifier::run(*m_dialect, *m_ast);
 		UnusedPruner::runUntilStabilised(*m_dialect, *m_ast);
@@ -260,7 +260,7 @@ void YulOptimizerTest::printIndented(ostream& _stream, string const& _output, st
 
 bool YulOptimizerTest::parse(ostream& _stream, string const& _linePrefix, bool const _formatted)
 {
-	m_dialect = m_yul ? yul::Dialect::yul() : yul::EVMDialect::strictAssemblyForEVM();
+	m_dialect = m_yul ? yul::Dialect::yul() : yul::EVMDialect::strictAssemblyForEVMObjects();
 	ErrorList errors;
 	ErrorReporter errorReporter(errors);
 	shared_ptr<Scanner> scanner = make_shared<Scanner>(CharStream(m_source, ""));

test/libyul/yulOptimizerTests/commonSubexpressionEliminator/object_access.yul

@@ -0,0 +1,23 @@
+{
+    // Arguments to ``datasize`` and ``dataoffset`` need to be
+    // literals. We cannot simplify their arguments, but we can
+    // simplify them as a full expression.
+    // ``datacopy`` does not have this restriction.
+    let r := "abc"
+    let a := datasize("abc")
+    let x := dataoffset("abc")
+    // should be replaced by a
+    let y := datasize("abc")
+    datacopy("abc", x, y)
+    mstore(a, x)
+}
+// ----
+// commonSubexpressionEliminator
+// {
+//     let r := "abc"
+//     let a := datasize("abc")
+//     let x := dataoffset("abc")
+//     let y := a
+//     datacopy(r, x, a)
+//     mstore(a, x)
+// }

test/libyul/yulOptimizerTests/expressionSplitter/object_access.yul

@@ -0,0 +1,21 @@
+{
+    // We should never split arguments to ``dataoffset``
+    // or ``datasize`` because they need to be literals
+    let x := dataoffset("abc")
+    let y := datasize("abc")
+    // datacopy is fine, though
+    datacopy(mload(0), mload(1), mload(2))
+}
+// ----
+// expressionSplitter
+// {
+//     let x := dataoffset("abc")
+//     let y := datasize("abc")
+//     let _1 := 2
+//     let _2 := mload(_1)
+//     let _3 := 1
+//     let _4 := mload(_3)
+//     let _5 := 0
+//     let _6 := mload(_5)
+//     datacopy(_6, _4, _2)
+// }

test/tools/yulopti.cpp

@@ -149,7 +149,7 @@ public:
 				(VarDeclInitializer{})(*m_ast);
 				break;
 			case 'x':
-				ExpressionSplitter{*m_nameDispenser}(*m_ast);
+				ExpressionSplitter{*m_dialect, *m_nameDispenser}(*m_ast);
 				break;
 			case 'j':
 				ExpressionJoiner::run(*m_ast);