Compilation of function modifiers.

libsolidity/AST.cpp

@@ -224,7 +224,7 @@ string FunctionDefinition::getCanonicalSignature() const
 
 Declaration::LValueType VariableDeclaration::getLValueType() const
 {
-	if (dynamic_cast<FunctionDefinition const*>(getScope()))
+	if (dynamic_cast<FunctionDefinition const*>(getScope()) || dynamic_cast<ModifierDefinition const*>(getScope()))
 		return Declaration::LValueType::LOCAL;
 	else
 		return Declaration::LValueType::STORAGE;
@@ -291,7 +291,8 @@ void Return::checkTypeRequirements()
 {
 	if (!m_expression)
 		return;
-	solAssert(m_returnParameters, "Return parameters not assigned.");
+	if (!m_returnParameters)
+		BOOST_THROW_EXCEPTION(createTypeError("Return arguments not allowed."));
 	if (m_returnParameters->getParameters().size() != 1)
 		BOOST_THROW_EXCEPTION(createTypeError("Different number of arguments in return statement "
 											  "than in returns declaration."));

libsolidity/AST.h

@@ -722,12 +722,8 @@ public:
 	virtual void accept(ASTConstVisitor& _visitor) const override;
 	virtual void checkTypeRequirements() override;
 
-	void setFunctionReturnParameters(ParameterList const& _parameters) { m_returnParameters = &_parameters; }
-	ParameterList const& getFunctionReturnParameters() const
-	{
-		solAssert(m_returnParameters, "");
-		return *m_returnParameters;
-	}
+	void setFunctionReturnParameters(ParameterList const* _parameters) { m_returnParameters = _parameters; }
+	ParameterList const* getFunctionReturnParameters() const { return m_returnParameters; }
 	Expression const* getExpression() const { return m_expression.get(); }
 
 private:

libsolidity/Compiler.cpp

@@ -77,6 +77,7 @@ void Compiler::packIntoContractCreator(ContractDefinition const& _contract, Comp
 	std::vector<ContractDefinition const*> const& bases = _contract.getLinearizedBaseContracts();
 	for (ContractDefinition const* contract: bases)
 	{
+		//TODO include modifiers
 		if (FunctionDefinition const* constructor = contract->getConstructor())
 			nodesUsedInConstructors.insert(constructor);
 		for (ASTPointer<InheritanceSpecifier> const& base: contract->getBaseContracts())
@@ -150,11 +151,7 @@ void Compiler::appendBaseConstructorCall(FunctionDefinition const& _constructor,
 	FunctionType constructorType(_constructor);
 	eth::AssemblyItem returnLabel = m_context.pushNewTag();
 	for (unsigned i = 0; i < _arguments.size(); ++i)
-	{
-		compileExpression(*_arguments[i]);
-		ExpressionCompiler::appendTypeConversion(m_context, *_arguments[i]->getType(),
-												 *constructorType.getParameterTypes()[i]);
-	}
+		compileExpression(*_arguments[i], constructorType.getParameterTypes()[i]);
 	m_context.appendJumpTo(m_context.getFunctionEntryLabel(_constructor));
 	m_context << returnLabel;
 }
@@ -280,20 +277,28 @@ bool Compiler::visit(FunctionDefinition const& _function)
 	m_returnTag = m_context.newTag();
 	m_breakTags.clear();
 	m_continueTags.clear();
+	m_stackCleanupForReturn = 0;
+	m_currentFunction = &_function;
+	m_modifierDepth = 0;
 
 	m_context << m_context.getFunctionEntryLabel(_function);
 
 	// stack upon entry: [return address] [arg0] [arg1] ... [argn]
 	// reserve additional slots: [retarg0] ... [retargm] [localvar0] ... [localvarp]
 
+	unsigned parametersSize = CompilerUtils::getSizeOnStack(_function.getParameters());
+	m_context.adjustStackOffset(parametersSize);
 	for (ASTPointer<VariableDeclaration const> const& variable: _function.getParameters())
-		m_context.addVariable(*variable);
+	{
+		m_context.addVariable(*variable, parametersSize);
+		parametersSize -= variable->getType()->getSizeOnStack();
+	}
 	for (ASTPointer<VariableDeclaration const> const& variable: _function.getReturnParameters())
 		m_context.addAndInitializeVariable(*variable);
 	for (VariableDeclaration const* localVariable: _function.getLocalVariables())
 		m_context.addAndInitializeVariable(*localVariable);
 
-	_function.getBody().accept(*this);
+	appendModifierOrFunctionCode();
 
 	m_context << m_returnTag;
 
@@ -420,13 +425,15 @@ bool Compiler::visit(Return const& _return)
 	//@todo modifications are needed to make this work with functions returning multiple values
 	if (Expression const* expression = _return.getExpression())
 	{
-		compileExpression(*expression);
-		VariableDeclaration const& firstVariable = *_return.getFunctionReturnParameters().getParameters().front();
-		ExpressionCompiler::appendTypeConversion(m_context, *expression->getType(), *firstVariable.getType());
-
+		solAssert(_return.getFunctionReturnParameters(), "Invalid return parameters pointer.");
+		VariableDeclaration const& firstVariable = *_return.getFunctionReturnParameters()->getParameters().front();
+		compileExpression(*expression, firstVariable.getType());
 		CompilerUtils(m_context).moveToStackVariable(firstVariable);
 	}
+	for (unsigned i = 0; i < m_stackCleanupForReturn; ++i)
+		m_context << eth::Instruction::POP;
 	m_context.appendJumpTo(m_returnTag);
+	m_context.adjustStackOffset(m_stackCleanupForReturn);
 	return false;
 }
 
@@ -434,10 +441,7 @@ bool Compiler::visit(VariableDefinition const& _variableDefinition)
 {
 	if (Expression const* expression = _variableDefinition.getExpression())
 	{
-		compileExpression(*expression);
-		ExpressionCompiler::appendTypeConversion(m_context,
-												 *expression->getType(),
-												 *_variableDefinition.getDeclaration().getType());
+		compileExpression(*expression, _variableDefinition.getDeclaration().getType());
 		CompilerUtils(m_context).moveToStackVariable(_variableDefinition.getDeclaration());
 	}
 	return false;
@@ -451,9 +455,53 @@ bool Compiler::visit(ExpressionStatement const& _expressionStatement)
 	return false;
 }
 
-void Compiler::compileExpression(Expression const& _expression)
+bool Compiler::visit(PlaceholderStatement const&)
+{
+	++m_modifierDepth;
+	appendModifierOrFunctionCode();
+	--m_modifierDepth;
+}
+
+void Compiler::appendModifierOrFunctionCode()
+{
+	solAssert(m_currentFunction, "");
+	if (m_modifierDepth >= m_currentFunction->getModifiers().size())
+		m_currentFunction->getBody().accept(*this);
+	else
+	{
+		ASTPointer<ModifierInvocation> const& modifierInvocation = m_currentFunction->getModifiers()[m_modifierDepth];
+
+		// TODO get the most derived override of the modifier
+		ModifierDefinition const* modifier = dynamic_cast<ModifierDefinition const*>(
+												 modifierInvocation->getName()->getReferencedDeclaration());
+		solAssert(!!modifier, "Modifier not found.");
+		solAssert(modifier->getParameters().size() == modifierInvocation->getArguments().size(), "");
+		for (unsigned i = 0; i < modifier->getParameters().size(); ++i)
+		{
+			m_context.addVariable(*modifier->getParameters()[i]);
+			compileExpression(*modifierInvocation->getArguments()[i],
+							  modifier->getParameters()[i]->getType());
+		}
+		for (VariableDeclaration const* localVariable: modifier->getLocalVariables())
+			m_context.addAndInitializeVariable(*localVariable);
+
+		unsigned const c_stackSurplus = CompilerUtils::getSizeOnStack(modifier->getParameters()) +
+										CompilerUtils::getSizeOnStack(modifier->getLocalVariables());
+		m_stackCleanupForReturn += c_stackSurplus;
+
+		modifier->getBody().accept(*this);
+
+		for (unsigned i = 0; i < c_stackSurplus; ++i)
+			m_context << eth::Instruction::POP;
+		m_stackCleanupForReturn -= c_stackSurplus;
+	}
+}
+
+void Compiler::compileExpression(Expression const& _expression, TypePointer const& _targetType)
 {
 	ExpressionCompiler::compileExpression(m_context, _expression, m_optimize);
+	if (_targetType)
+		ExpressionCompiler::appendTypeConversion(m_context, *_expression.getType(), *_targetType);
 }
 
 }

libsolidity/Compiler.h

@@ -31,7 +31,8 @@ namespace solidity {
 class Compiler: private ASTConstVisitor
 {
 public:
-	explicit Compiler(bool _optimize = false): m_optimize(_optimize), m_context(), m_returnTag(m_context.newTag()) {}
+	explicit Compiler(bool _optimize = false): m_optimize(_optimize), m_context(),
+		m_returnTag(m_context.newTag()) {}
 
 	void compileContract(ContractDefinition const& _contract,
 						 std::map<ContractDefinition const*, bytes const*> const& _contracts);
@@ -70,8 +71,13 @@ private:
 	virtual bool visit(Return const& _return) override;
 	virtual bool visit(VariableDefinition const& _variableDefinition) override;
 	virtual bool visit(ExpressionStatement const& _expressionStatement) override;
+	virtual bool visit(PlaceholderStatement const&) override;
 
-	void compileExpression(Expression const& _expression);
+	/// Appends one layer of function modifier code of the current function, or the function
+	/// body itself if the last modifier was reached.
+	void appendModifierOrFunctionCode();
+
+	void compileExpression(Expression const& _expression, TypePointer const& _targetType = TypePointer());
 
 	bool const m_optimize;
 	CompilerContext m_context;
@@ -79,6 +85,9 @@ private:
 	std::vector<eth::AssemblyItem> m_breakTags; ///< tag to jump to for a "break" statement
 	std::vector<eth::AssemblyItem> m_continueTags; ///< tag to jump to for a "continue" statement
 	eth::AssemblyItem m_returnTag; ///< tag to jump to for a "return" statement
+	unsigned m_modifierDepth = 0;
+	FunctionDefinition const* m_currentFunction;
+	unsigned m_stackCleanupForReturn; ///< this number of stack elements need to be removed before jump to m_returnTag
 };
 
 }

libsolidity/CompilerContext.cpp

@@ -43,10 +43,11 @@ void CompilerContext::addStateVariable(VariableDeclaration const& _declaration)
 	m_stateVariablesSize += _declaration.getType()->getStorageSize();
 }
 
-void CompilerContext::addVariable(VariableDeclaration const& _declaration)
+void CompilerContext::addVariable(VariableDeclaration const& _declaration,
+								  unsigned _offsetToCurrent)
 {
-	m_localVariables[&_declaration] = m_localVariablesSize;
-	m_localVariablesSize += _declaration.getType()->getSizeOnStack();
+	solAssert(m_asm.deposit() >= 0 && unsigned(m_asm.deposit()) >= _offsetToCurrent, "");
+	m_localVariables[&_declaration] = unsigned(m_asm.deposit()) - _offsetToCurrent;
 }
 
 void CompilerContext::addAndInitializeVariable(VariableDeclaration const& _declaration)
@@ -56,7 +57,6 @@ void CompilerContext::addAndInitializeVariable(VariableDeclaration const& _decla
 	int const size = _declaration.getType()->getSizeOnStack();
 	for (int i = 0; i < size; ++i)
 		*this << u256(0);
-	m_asm.adjustDeposit(-size);
 }
 
 void CompilerContext::addFunction(FunctionDefinition const& _function)
@@ -75,7 +75,7 @@ bytes const& CompilerContext::getCompiledContract(const ContractDefinition& _con
 
 bool CompilerContext::isLocalVariable(Declaration const* _declaration) const
 {
-	return m_localVariables.count(_declaration) > 0;
+	return m_localVariables.count(_declaration);
 }
 
 eth::AssemblyItem CompilerContext::getFunctionEntryLabel(FunctionDefinition const& _function) const
@@ -96,17 +96,17 @@ unsigned CompilerContext::getBaseStackOffsetOfVariable(Declaration const& _decla
 {
 	auto res = m_localVariables.find(&_declaration);
 	solAssert(res != m_localVariables.end(), "Variable not found on stack.");
-	return m_localVariablesSize - res->second - 1;
+	return res->second;
 }
 
 unsigned CompilerContext::baseToCurrentStackOffset(unsigned _baseOffset) const
 {
-	return _baseOffset + m_asm.deposit();
+	return m_asm.deposit() - _baseOffset - 1;
 }
 
 unsigned CompilerContext::currentToBaseStackOffset(unsigned _offset) const
 {
-	return -baseToCurrentStackOffset(-_offset);
+	return m_asm.deposit() - _offset - 1;
 }
 
 u256 CompilerContext::getStorageLocationOfVariable(const Declaration& _declaration) const

libsolidity/CompilerContext.h

@@ -42,7 +42,7 @@ public:
 	void addMagicGlobal(MagicVariableDeclaration const& _declaration);
 	void addStateVariable(VariableDeclaration const& _declaration);
 	void startNewFunction() { m_localVariables.clear(); m_asm.setDeposit(0); }
-	void addVariable(VariableDeclaration const& _declaration);
+	void addVariable(VariableDeclaration const& _declaration, unsigned _offsetToCurrent = 0);
 	void addAndInitializeVariable(VariableDeclaration const& _declaration);
 	void addFunction(FunctionDefinition const& _function);
 
@@ -59,7 +59,7 @@ public:
 	eth::AssemblyItem getFunctionEntryLabel(FunctionDefinition const& _function) const;
 	/// @returns the entry label of the given function and takes overrides into account.
 	eth::AssemblyItem getVirtualFunctionEntryLabel(FunctionDefinition const& _function) const;
-	/// Returns the distance of the given local variable from the top of the local variable stack.
+	/// Returns the distance of the given local variable from the bottom of the stack (of the current function).
 	unsigned getBaseStackOffsetOfVariable(Declaration const& _declaration) const;
 	/// If supplied by a value returned by @ref getBaseStackOffsetOfVariable(variable), returns
 	/// the distance of that variable from the current top of the stack.
@@ -112,10 +112,8 @@ private:
 	u256 m_stateVariablesSize = 0;
 	/// Storage offsets of state variables
 	std::map<Declaration const*, u256> m_stateVariables;
-	/// Offsets of local variables on the stack (relative to stack base).
+	/// Positions of local variables on the stack.
 	std::map<Declaration const*, unsigned> m_localVariables;
-	/// Sum of stack sizes of local variables
-	unsigned m_localVariablesSize;
 	/// Labels pointing to the entry points of funcitons.
 	std::map<Declaration const*, eth::AssemblyItem> m_functionEntryLabels;
 	/// Labels pointing to the entry points of function overrides.

libsolidity/NameAndTypeResolver.cpp

@@ -310,8 +310,7 @@ void ReferencesResolver::endVisit(VariableDeclaration& _variable)
 
 bool ReferencesResolver::visit(Return& _return)
 {
-	solAssert(m_returnParameters, "Return parameters not set.");
-	_return.setFunctionReturnParameters(*m_returnParameters);
+	_return.setFunctionReturnParameters(m_returnParameters);
 	return true;
 }
 

test/SolidityEndToEndTest.cpp

@@ -1653,22 +1653,72 @@ BOOST_AUTO_TEST_CASE(constructor_argument_overriding)
 BOOST_AUTO_TEST_CASE(function_modifier)
 {
 	char const* sourceCode = R"(
-		contract BaseBase {
-			uint m_a;
-			function BaseBase(uint a) {
-				m_a = a;
-			}
+		contract C {
+			function getOne() nonFree returns (uint r) { return 1; }
+			modifier nonFree { if (msg.value > 0) _ }
 		}
-		contract Base is BaseBase(2) { }
-		contract Derived is Base, BaseBase(3) {
-			function getA() returns (uint r) { return m_a; }
+	)";
+	compileAndRun(sourceCode);
+	BOOST_CHECK(callContractFunction("getOne()") == encodeArgs(0));
+	BOOST_CHECK(callContractFunctionWithValue("getOne()", 1) == encodeArgs(1));
+}
+
+BOOST_AUTO_TEST_CASE(function_modifier_local_variables)
+{
+	char const* sourceCode = R"(
+		contract C {
+			modifier mod1 { var a = 1; var b = 2; _ }
+			modifier mod2(bool a) { if (a) return; else _ }
+			function f(bool a) mod1 mod2(a) returns (uint r) { return 3; }
 		}
 	)";
-	compileAndRun(sourceCode, 0, "Derived");
-	BOOST_CHECK(callContractFunction("getA()") == encodeArgs(3));
+	compileAndRun(sourceCode);
+	BOOST_CHECK(callContractFunction("f(bool)", true) == encodeArgs(0));
+	BOOST_CHECK(callContractFunction("f(bool)", false) == encodeArgs(3));
 }
 
+BOOST_AUTO_TEST_CASE(function_modifier_loop)
+{
+	char const* sourceCode = R"(
+		contract C {
+			modifier repeat(uint count) { for (var i = 0; i < count; ++i) _ }
+			function f() repeat(10) returns (uint r) { r += 1; }
+		}
+	)";
+	compileAndRun(sourceCode);
+	BOOST_CHECK(callContractFunction("f()") == encodeArgs(10));
+}
+
+BOOST_AUTO_TEST_CASE(function_modifier_multi_invocation)
+{
+	char const* sourceCode = R"(
+		contract C {
+			modifier repeat(bool twice) { if (twice) _ _ }
+			function f(bool twice) repeat(twice) returns (uint r) { r += 1; }
+		}
+	)";
+	compileAndRun(sourceCode);
+	BOOST_CHECK(callContractFunction("f(bool)", false) == encodeArgs(1));
+	BOOST_CHECK(callContractFunction("f(bool)", true) == encodeArgs(2));
+}
+
+BOOST_AUTO_TEST_CASE(function_modifier_multi_with_return)
+{
+	// Here, the explicit return prevents the second execution
+	char const* sourceCode = R"(
+		contract C {
+			modifier repeat(bool twice) { if (twice) _ _ }
+			function f(bool twice) repeat(twice) returns (uint r) { r += 1; return r; }
+		}
+	)";
+	compileAndRun(sourceCode);
+	BOOST_CHECK(callContractFunction("f(bool)", false) == encodeArgs(1));
+	BOOST_CHECK(callContractFunction("f(bool)", true) == encodeArgs(1));
+}
+
+
 // modifier overriding
+// functions called by modifiers used by constructor need to be pulled into the creation context
 
 BOOST_AUTO_TEST_SUITE_END()
 

test/SolidityExpressionCompiler.cpp

@@ -118,8 +118,11 @@ bytes compileFirstExpression(const string& _sourceCode, vector<vector<string>> _
 			CompilerContext context;
 			for (vector<string> const& function: _functions)
 				context.addFunction(dynamic_cast<FunctionDefinition const&>(resolveDeclaration(function, resolver)));
+			unsigned parametersSize = _localVariables.size(); // assume they are all one slot on the stack
+			context.adjustStackOffset(parametersSize);
 			for (vector<string> const& variable: _localVariables)
-				context.addVariable(dynamic_cast<VariableDeclaration const&>(resolveDeclaration(variable, resolver)));
+				context.addVariable(dynamic_cast<VariableDeclaration const&>(resolveDeclaration(variable, resolver)),
+									parametersSize--);
 
 			ExpressionCompiler::compileExpression(context, *extractor.getExpression());
 

test/SolidityNameAndTypeResolution.cpp

@@ -573,6 +573,17 @@ BOOST_AUTO_TEST_CASE(function_overrides_modifier)
 	BOOST_CHECK_THROW(parseTextAndResolveNames(text), TypeError);
 }
 
+BOOST_AUTO_TEST_CASE(modifier_returns_value)
+{
+	char const* text = R"(
+		contract A {
+			function f(uint a) mod(2) returns (uint r) {}
+			modifier mod(uint a) { return 7; }
+		}
+	)";
+	BOOST_CHECK_THROW(parseTextAndResolveNames(text), TypeError);
+}
+
 BOOST_AUTO_TEST_SUITE_END()
 
 }