From ccaa81fbe7218298c72b8dcb6928c33d72268ac1 Mon Sep 17 00:00:00 2001 From: chriseth Date: Mon, 30 Nov 2020 18:59:49 +0100 Subject: [PATCH 1/4] Implement function modifiers. --- libsolidity/codegen/ir/Common.cpp | 18 ++- libsolidity/codegen/ir/Common.h | 2 + .../codegen/ir/IRGenerationContext.cpp | 5 + libsolidity/codegen/ir/IRGenerationContext.h | 1 + libsolidity/codegen/ir/IRGenerator.cpp | 136 +++++++++++++++++- libsolidity/codegen/ir/IRGenerator.h | 6 + .../codegen/ir/IRGeneratorForStatements.cpp | 6 + .../codegen/ir/IRGeneratorForStatements.h | 15 +- ...ode_v2_in_modifier_used_in_v1_contract.sol | 2 + .../inline_assembly_in_modifiers.sol | 2 + .../modifiers/break_in_modifier.sol | 2 + .../modifiers/continue_in_modifier.sol | 2 + .../modifiers/function_modifier.sol | 2 + .../modifiers/function_modifier_empty.sol | 2 + .../modifiers/function_modifier_library.sol | 2 + .../function_modifier_library_inheritance.sol | 2 + .../function_modifier_overriding.sol | 2 + .../modifiers/modifier_init_return.sol | 13 ++ .../return_does_not_skip_modifier.sol | 2 + .../modifiers/return_in_modifier.sol | 2 + ...o_nonpayable_circumvention_by_modifier.sol | 2 + .../semanticTests/various/multi_modifiers.sol | 2 + 22 files changed, 220 insertions(+), 8 deletions(-) create mode 100644 test/libsolidity/semanticTests/modifiers/modifier_init_return.sol diff --git a/libsolidity/codegen/ir/Common.cpp b/libsolidity/codegen/ir/Common.cpp index 41c711c99..b7b8d092d 100644 --- a/libsolidity/codegen/ir/Common.cpp +++ b/libsolidity/codegen/ir/Common.cpp @@ -32,10 +32,9 @@ YulArity YulArity::fromType(FunctionType const& _functionType) TupleType(_functionType.returnParameterTypes()).sizeOnStack() }; } + string IRNames::function(FunctionDefinition const& _function) { - // @TODO previously, we had to distinguish creation context and runtime context, - // but since we do not work with jump positions anymore, this should not be a problem, right? return "fun_" + _function.name() + "_" + to_string(_function.id()); } @@ -44,6 +43,21 @@ string IRNames::function(VariableDeclaration const& _varDecl) return "getter_fun_" + _varDecl.name() + "_" + to_string(_varDecl.id()); } +string IRNames::modifierInvocation(ModifierInvocation const& _modifierInvocation) +{ + // This uses the ID of the modifier invocation because it has to be unique + // for each invocation. + solAssert(!_modifierInvocation.name().path().empty(), ""); + string const& modifierName = _modifierInvocation.name().path().back(); + solAssert(!modifierName.empty(), ""); + return "modifier_" + modifierName + "_" + to_string(_modifierInvocation.id()); +} + +string IRNames::functionWithModifierInner(FunctionDefinition const& _function) +{ + return "fun_" + _function.name() + "_" + to_string(_function.id()) + "_inner"; +} + string IRNames::creationObject(ContractDefinition const& _contract) { return _contract.name() + "_" + toString(_contract.id()); diff --git a/libsolidity/codegen/ir/Common.h b/libsolidity/codegen/ir/Common.h index da6c61a7f..345eed784 100644 --- a/libsolidity/codegen/ir/Common.h +++ b/libsolidity/codegen/ir/Common.h @@ -49,6 +49,8 @@ struct IRNames { static std::string function(FunctionDefinition const& _function); static std::string function(VariableDeclaration const& _varDecl); + static std::string modifierInvocation(ModifierInvocation const& _modifierInvocation); + static std::string functionWithModifierInner(FunctionDefinition const& _function); static std::string creationObject(ContractDefinition const& _contract); static std::string runtimeObject(ContractDefinition const& _contract); static std::string internalDispatch(YulArity const& _arity); diff --git a/libsolidity/codegen/ir/IRGenerationContext.cpp b/libsolidity/codegen/ir/IRGenerationContext.cpp index 99e5fb887..618b816ec 100644 --- a/libsolidity/codegen/ir/IRGenerationContext.cpp +++ b/libsolidity/codegen/ir/IRGenerationContext.cpp @@ -80,6 +80,11 @@ IRVariable const& IRGenerationContext::localVariable(VariableDeclaration const& return m_localVariables.at(&_varDecl); } +void IRGenerationContext::resetLocalVariables() +{ + m_localVariables.clear(); +} + void IRGenerationContext::registerImmutableVariable(VariableDeclaration const& _variable) { solAssert(_variable.immutable(), "Attempted to register a non-immutable variable as immutable."); diff --git a/libsolidity/codegen/ir/IRGenerationContext.h b/libsolidity/codegen/ir/IRGenerationContext.h index 5a36041be..9976367d9 100644 --- a/libsolidity/codegen/ir/IRGenerationContext.h +++ b/libsolidity/codegen/ir/IRGenerationContext.h @@ -97,6 +97,7 @@ public: IRVariable const& addLocalVariable(VariableDeclaration const& _varDecl); bool isLocalVariable(VariableDeclaration const& _varDecl) const { return m_localVariables.count(&_varDecl); } IRVariable const& localVariable(VariableDeclaration const& _varDecl); + void resetLocalVariables(); /// Registers an immutable variable of the contract. /// Should only be called at construction time. diff --git a/libsolidity/codegen/ir/IRGenerator.cpp b/libsolidity/codegen/ir/IRGenerator.cpp index 56df101f8..fef88aeb7 100644 --- a/libsolidity/codegen/ir/IRGenerator.cpp +++ b/libsolidity/codegen/ir/IRGenerator.cpp @@ -249,10 +249,10 @@ string IRGenerator::generateFunction(FunctionDefinition const& _function) { string functionName = IRNames::function(_function); return m_context.functionCollector().createFunction(functionName, [&]() { - solUnimplementedAssert(_function.modifiers().empty(), "Modifiers not implemented yet."); + m_context.resetLocalVariables(); Whiskers t(R"( function () -> { - + } )"); @@ -268,8 +268,137 @@ string IRGenerator::generateFunction(FunctionDefinition const& _function) retParams += m_context.addLocalVariable(*varDecl).stackSlots(); retInit += generateInitialAssignment(*varDecl); } + t("retParams", joinHumanReadable(retParams)); - t("initReturnVariables", retInit); + t("retInit", retInit); + + if (_function.modifiers().empty()) + t("body", generate(_function.body())); + else + { + for (size_t i = 0; i < _function.modifiers().size(); ++i) + { + ModifierInvocation const& modifier = *_function.modifiers().at(i); + string next = + i + 1 < _function.modifiers().size() ? + IRNames::modifierInvocation(*_function.modifiers().at(i + 1)) : + IRNames::functionWithModifierInner(_function); + generateModifier(modifier, _function, next); + } + t("body", + (retParams.empty() ? string{} : joinHumanReadable(retParams) + " := ") + + IRNames::modifierInvocation(*_function.modifiers().at(0)) + + "(" + + joinHumanReadable(retParams + params) + + ")" + ); + // Now generate the actual inner function. + generateFunctionWithModifierInner(_function); + } + return t.render(); + }); +} + +string IRGenerator::generateModifier( + ModifierInvocation const& _modifierInvocation, + FunctionDefinition const& _function, + string const& _nextFunction +) +{ + string functionName = IRNames::modifierInvocation(_modifierInvocation); + return m_context.functionCollector().createFunction(functionName, [&]() { + m_context.resetLocalVariables(); + Whiskers t(R"( + function () -> { + + + + } + )"); + t("functionName", functionName); + vector retParamsIn; + for (auto const& varDecl: _function.returnParameters()) + retParamsIn += IRVariable(*varDecl).stackSlots(); + vector params = retParamsIn; + for (auto const& varDecl: _function.parameters()) + params += m_context.addLocalVariable(*varDecl).stackSlots(); + t("params", joinHumanReadable(params)); + vector retParams; + string assignRetParams; + for (size_t i = 0; i < retParamsIn.size(); ++i) + { + retParams.emplace_back(m_context.newYulVariable()); + assignRetParams += retParams.back() + " := " + retParamsIn[i] + "\n"; + } + t("retParams", joinHumanReadable(retParams)); + t("assignRetParams", assignRetParams); + + solAssert(*_modifierInvocation.name().annotation().requiredLookup == VirtualLookup::Virtual, ""); + + ModifierDefinition const& modifier = dynamic_cast( + *_modifierInvocation.name().annotation().referencedDeclaration + ).resolveVirtual(m_context.mostDerivedContract()); + + solAssert( + modifier.parameters().empty() == + (!_modifierInvocation.arguments() || _modifierInvocation.arguments()->empty()), + "" + ); + IRGeneratorForStatements expressionEvaluator(m_context, m_utils); + if (_modifierInvocation.arguments()) + for (size_t i = 0; i < _modifierInvocation.arguments()->size(); i++) + { + IRVariable argument = expressionEvaluator.evaluateExpression( + *_modifierInvocation.arguments()->at(i), + *modifier.parameters()[i]->annotation().type + ); + expressionEvaluator.define( + m_context.addLocalVariable(*modifier.parameters()[i]), + argument + ); + } + + t("evalArgs", expressionEvaluator.code()); + IRGeneratorForStatements generator(m_context, m_utils, [&]() { + string ret = joinHumanReadable(retParams); + return + (ret.empty() ? "" : ret + " := ") + + _nextFunction + "(" + joinHumanReadable(params) + ")\n"; + }); + generator.generate(modifier.body()); + t("body", generator.code()); + return t.render(); + }); +} + +string IRGenerator::generateFunctionWithModifierInner(FunctionDefinition const& _function) +{ + string functionName = IRNames::functionWithModifierInner(_function); + return m_context.functionCollector().createFunction(functionName, [&]() { + m_context.resetLocalVariables(); + Whiskers t(R"( + function () -> { + + + } + )"); + t("functionName", functionName); + vector retParams; + vector retParamsIn; + for (auto const& varDecl: _function.returnParameters()) + retParams += m_context.addLocalVariable(*varDecl).stackSlots(); + string assignRetParams; + for (size_t i = 0; i < retParams.size(); ++i) + { + retParamsIn.emplace_back(m_context.newYulVariable()); + assignRetParams += retParams.back() + " := " + retParamsIn[i] + "\n"; + } + vector params = retParamsIn; + for (auto const& varDecl: _function.parameters()) + params += m_context.addLocalVariable(*varDecl).stackSlots(); + t("params", joinHumanReadable(params)); + t("retParams", joinHumanReadable(retParams)); + t("assignRetParams", assignRetParams); t("body", generate(_function.body())); return t.render(); }); @@ -510,6 +639,7 @@ string IRGenerator::initStateVariables(ContractDefinition const& _contract) void IRGenerator::generateImplicitConstructors(ContractDefinition const& _contract) { + // TODO reset local variables somewhere here. auto listAllParams = [&]( map> const& baseParams) -> vector { diff --git a/libsolidity/codegen/ir/IRGenerator.h b/libsolidity/codegen/ir/IRGenerator.h index cdb431e83..7b7c58bd4 100644 --- a/libsolidity/codegen/ir/IRGenerator.h +++ b/libsolidity/codegen/ir/IRGenerator.h @@ -73,6 +73,12 @@ private: InternalDispatchMap generateInternalDispatchFunctions(); /// Generates code for and returns the name of the function. std::string generateFunction(FunctionDefinition const& _function); + std::string generateModifier( + ModifierInvocation const& _modifierInvocation, + FunctionDefinition const& _function, + std::string const& _nextFunction + ); + std::string generateFunctionWithModifierInner(FunctionDefinition const& _function); /// Generates a getter for the given declaration and returns its name std::string generateGetter(VariableDeclaration const& _varDecl); diff --git a/libsolidity/codegen/ir/IRGeneratorForStatements.cpp b/libsolidity/codegen/ir/IRGeneratorForStatements.cpp index 121da89c3..ee1a24bb3 100644 --- a/libsolidity/codegen/ir/IRGeneratorForStatements.cpp +++ b/libsolidity/codegen/ir/IRGeneratorForStatements.cpp @@ -581,6 +581,12 @@ bool IRGeneratorForStatements::visit(IfStatement const& _ifStatement) return false; } +void IRGeneratorForStatements::endVisit(PlaceholderStatement const&) +{ + solAssert(m_placeholderCallback, ""); + m_code << m_placeholderCallback(); +} + bool IRGeneratorForStatements::visit(ForStatement const& _forStatement) { setLocation(_forStatement); diff --git a/libsolidity/codegen/ir/IRGeneratorForStatements.h b/libsolidity/codegen/ir/IRGeneratorForStatements.h index 24ac8a61e..2af463132 100644 --- a/libsolidity/codegen/ir/IRGeneratorForStatements.h +++ b/libsolidity/codegen/ir/IRGeneratorForStatements.h @@ -40,8 +40,13 @@ class YulUtilFunctions; class IRGeneratorForStatements: public ASTConstVisitor { public: - IRGeneratorForStatements(IRGenerationContext& _context, YulUtilFunctions& _utils): + IRGeneratorForStatements( + IRGenerationContext& _context, + YulUtilFunctions& _utils, + std::function _placeholderCallback = {} + ): m_context(_context), + m_placeholderCallback(std::move(_placeholderCallback)), m_utils(_utils) {} @@ -58,6 +63,9 @@ public: /// Calculates expression's value and returns variable where it was stored IRVariable evaluateExpression(Expression const& _expression, Type const& _to); + /// Defines @a _var using the value of @a _value while performing type conversions, if required. + void define(IRVariable const& _var, IRVariable const& _value) { declareAssign(_var, _value, true); } + /// @returns the name of a function that computes the value of the given constant /// and also generates the function. std::string constantValueFunction(VariableDeclaration const& _constant); @@ -66,6 +74,7 @@ public: bool visit(Conditional const& _conditional) override; bool visit(Assignment const& _assignment) override; bool visit(TupleExpression const& _tuple) override; + void endVisit(PlaceholderStatement const& _placeholder) override; bool visit(Block const& _block) override; void endVisit(Block const& _block) override; bool visit(IfStatement const& _ifStatement) override; @@ -135,8 +144,7 @@ private: /// @returns an output stream that can be used to define @a _var using a function call or /// single stack slot expression. std::ostream& define(IRVariable const& _var); - /// Defines @a _var using the value of @a _value while performing type conversions, if required. - void define(IRVariable const& _var, IRVariable const& _value) { declareAssign(_var, _value, true); } + /// Assigns @a _var to the value of @a _value while performing type conversions, if required. void assign(IRVariable const& _var, IRVariable const& _value) { declareAssign(_var, _value, false); } /// Declares variable @a _var. @@ -189,6 +197,7 @@ private: std::ostringstream m_code; IRGenerationContext& m_context; + std::function m_placeholderCallback; YulUtilFunctions& m_utils; std::optional m_currentLValue; langutil::SourceLocation m_currentLocation; diff --git a/test/libsolidity/semanticTests/abiEncoderV2/abi_encode_v2_in_modifier_used_in_v1_contract.sol b/test/libsolidity/semanticTests/abiEncoderV2/abi_encode_v2_in_modifier_used_in_v1_contract.sol index 81095f83c..60d20a781 100644 --- a/test/libsolidity/semanticTests/abiEncoderV2/abi_encode_v2_in_modifier_used_in_v1_contract.sol +++ b/test/libsolidity/semanticTests/abiEncoderV2/abi_encode_v2_in_modifier_used_in_v1_contract.sol @@ -34,5 +34,7 @@ contract C is B { return (x, y); } } +// ==== +// compileViaYul: also // ---- // test() -> 5, 10 diff --git a/test/libsolidity/semanticTests/inlineAssembly/inline_assembly_in_modifiers.sol b/test/libsolidity/semanticTests/inlineAssembly/inline_assembly_in_modifiers.sol index 70f19479a..80f7b134f 100644 --- a/test/libsolidity/semanticTests/inlineAssembly/inline_assembly_in_modifiers.sol +++ b/test/libsolidity/semanticTests/inlineAssembly/inline_assembly_in_modifiers.sol @@ -28,6 +28,8 @@ contract C { } } +// ==== +// compileViaYul: also // ---- // f() -> true // g() -> FAILURE diff --git a/test/libsolidity/semanticTests/modifiers/break_in_modifier.sol b/test/libsolidity/semanticTests/modifiers/break_in_modifier.sol index 67fc0e6be..977c03ea4 100644 --- a/test/libsolidity/semanticTests/modifiers/break_in_modifier.sol +++ b/test/libsolidity/semanticTests/modifiers/break_in_modifier.sol @@ -14,6 +14,8 @@ contract C { x = t; } } +// ==== +// compileViaYul: also // ---- // x() -> 0 // f() -> diff --git a/test/libsolidity/semanticTests/modifiers/continue_in_modifier.sol b/test/libsolidity/semanticTests/modifiers/continue_in_modifier.sol index 1db0f2e9f..e3875a69c 100644 --- a/test/libsolidity/semanticTests/modifiers/continue_in_modifier.sol +++ b/test/libsolidity/semanticTests/modifiers/continue_in_modifier.sol @@ -14,6 +14,8 @@ contract C { } } +// ==== +// compileViaYul: also // ---- // x() -> 0 // f() -> diff --git a/test/libsolidity/semanticTests/modifiers/function_modifier.sol b/test/libsolidity/semanticTests/modifiers/function_modifier.sol index fcb8f64a2..94fef9bb3 100644 --- a/test/libsolidity/semanticTests/modifiers/function_modifier.sol +++ b/test/libsolidity/semanticTests/modifiers/function_modifier.sol @@ -8,6 +8,8 @@ contract C { } } +// ==== +// compileViaYul: also // ---- // getOne() -> 0 // getOne(), 1 wei -> 1 diff --git a/test/libsolidity/semanticTests/modifiers/function_modifier_empty.sol b/test/libsolidity/semanticTests/modifiers/function_modifier_empty.sol index 94a352587..c11d37c38 100644 --- a/test/libsolidity/semanticTests/modifiers/function_modifier_empty.sol +++ b/test/libsolidity/semanticTests/modifiers/function_modifier_empty.sol @@ -13,5 +13,7 @@ contract C is A { } } +// ==== +// compileViaYul: also // ---- // f() -> false diff --git a/test/libsolidity/semanticTests/modifiers/function_modifier_library.sol b/test/libsolidity/semanticTests/modifiers/function_modifier_library.sol index f10ebb0e7..a47b4dea5 100644 --- a/test/libsolidity/semanticTests/modifiers/function_modifier_library.sol +++ b/test/libsolidity/semanticTests/modifiers/function_modifier_library.sol @@ -24,5 +24,7 @@ contract Test { } } +// ==== +// compileViaYul: also // ---- // f() -> 0x202 diff --git a/test/libsolidity/semanticTests/modifiers/function_modifier_library_inheritance.sol b/test/libsolidity/semanticTests/modifiers/function_modifier_library_inheritance.sol index 3d5e97de0..00779ef9b 100644 --- a/test/libsolidity/semanticTests/modifiers/function_modifier_library_inheritance.sol +++ b/test/libsolidity/semanticTests/modifiers/function_modifier_library_inheritance.sol @@ -30,5 +30,7 @@ contract Test { } } +// ==== +// compileViaYul: also // ---- // f() -> 0x202 diff --git a/test/libsolidity/semanticTests/modifiers/function_modifier_overriding.sol b/test/libsolidity/semanticTests/modifiers/function_modifier_overriding.sol index c91869615..8363db3b4 100644 --- a/test/libsolidity/semanticTests/modifiers/function_modifier_overriding.sol +++ b/test/libsolidity/semanticTests/modifiers/function_modifier_overriding.sol @@ -15,5 +15,7 @@ contract C is A { } } +// ==== +// compileViaYul: also // ---- // f() -> false diff --git a/test/libsolidity/semanticTests/modifiers/modifier_init_return.sol b/test/libsolidity/semanticTests/modifiers/modifier_init_return.sol new file mode 100644 index 000000000..097fd274c --- /dev/null +++ b/test/libsolidity/semanticTests/modifiers/modifier_init_return.sol @@ -0,0 +1,13 @@ +contract C { + modifier m(bool condition) { + if (condition) _; + } + + function f(uint x) public m(x >= 10) returns (uint[5] memory r) { + r[2] = 3; + } +} + +// ---- +// f(uint256): 9 -> 0x00, 0x00, 0x00, 0x00, 0x00 +// f(uint256): 10 -> 0x00, 0x00, 3, 0x00, 0x00 diff --git a/test/libsolidity/semanticTests/modifiers/return_does_not_skip_modifier.sol b/test/libsolidity/semanticTests/modifiers/return_does_not_skip_modifier.sol index c437922e4..6bfefdcca 100644 --- a/test/libsolidity/semanticTests/modifiers/return_does_not_skip_modifier.sol +++ b/test/libsolidity/semanticTests/modifiers/return_does_not_skip_modifier.sol @@ -10,6 +10,8 @@ contract C { } } +// ==== +// compileViaYul: also // ---- // x() -> 0 // f() -> 2 diff --git a/test/libsolidity/semanticTests/modifiers/return_in_modifier.sol b/test/libsolidity/semanticTests/modifiers/return_in_modifier.sol index 81fbd794e..22ec595c5 100644 --- a/test/libsolidity/semanticTests/modifiers/return_in_modifier.sol +++ b/test/libsolidity/semanticTests/modifiers/return_in_modifier.sol @@ -14,6 +14,8 @@ contract C { } } +// ==== +// compileViaYul: also // ---- // x() -> 0 // f() -> diff --git a/test/libsolidity/semanticTests/payable/no_nonpayable_circumvention_by_modifier.sol b/test/libsolidity/semanticTests/payable/no_nonpayable_circumvention_by_modifier.sol index cefb8f3b7..e0cc97057 100644 --- a/test/libsolidity/semanticTests/payable/no_nonpayable_circumvention_by_modifier.sol +++ b/test/libsolidity/semanticTests/payable/no_nonpayable_circumvention_by_modifier.sol @@ -13,6 +13,8 @@ contract C { return address(this).balance; } } +// ==== +// compileViaYul: also // ---- // f(), 27 wei -> FAILURE // balance() -> 0 diff --git a/test/libsolidity/semanticTests/various/multi_modifiers.sol b/test/libsolidity/semanticTests/various/multi_modifiers.sol index 07f6c038d..37bfe270c 100644 --- a/test/libsolidity/semanticTests/various/multi_modifiers.sol +++ b/test/libsolidity/semanticTests/various/multi_modifiers.sol @@ -16,6 +16,8 @@ contract C { x += 3; } } +// ==== +// compileViaYul: also // ---- // f1() -> // x() -> 0x08 From 35908c602b775863a198bcb09e069e1bb291b5f8 Mon Sep 17 00:00:00 2001 From: chriseth Date: Wed, 2 Dec 2020 18:00:04 +0100 Subject: [PATCH 2/4] Modifiers for constructors. --- libsolidity/codegen/ir/IRGenerator.cpp | 42 ++++++++++++++----- ..._calling_functions_in_creation_context.sol | 2 + .../function_modifier_for_constructor.sol | 3 +- 3 files changed, 36 insertions(+), 11 deletions(-) diff --git a/libsolidity/codegen/ir/IRGenerator.cpp b/libsolidity/codegen/ir/IRGenerator.cpp index fef88aeb7..ac7475a7a 100644 --- a/libsolidity/codegen/ir/IRGenerator.cpp +++ b/libsolidity/codegen/ir/IRGenerator.cpp @@ -639,7 +639,6 @@ string IRGenerator::initStateVariables(ContractDefinition const& _contract) void IRGenerator::generateImplicitConstructors(ContractDefinition const& _contract) { - // TODO reset local variables somewhere here. auto listAllParams = [&]( map> const& baseParams) -> vector { @@ -656,6 +655,7 @@ void IRGenerator::generateImplicitConstructors(ContractDefinition const& _contra ContractDefinition const* contract = _contract.annotation().linearizedBaseContracts[i]; baseConstructorParams.erase(contract); + m_context.resetLocalVariables(); m_context.functionCollector().createFunction(IRNames::implicitConstructor(*contract), [&]() { Whiskers t(R"( function () { @@ -667,16 +667,8 @@ void IRGenerator::generateImplicitConstructors(ContractDefinition const& _contra )"); vector params; if (contract->constructor()) - { - for (auto const& modifierInvocation: contract->constructor()->modifiers()) - // This can be ContractDefinition too for super arguments. That is supported. - solUnimplementedAssert( - !dynamic_cast(modifierInvocation->name().annotation().referencedDeclaration), - "Modifiers not implemented yet." - ); for (ASTPointer const& varDecl: contract->constructor()->parameters()) params += m_context.addLocalVariable(*varDecl).stackSlots(); - } t("params", joinHumanReadable(params)); vector baseParams = listAllParams(baseConstructorParams); t("baseParams", joinHumanReadable(baseParams)); @@ -695,7 +687,37 @@ void IRGenerator::generateImplicitConstructors(ContractDefinition const& _contra else t("hasNextConstructor", false); t("initStateVariables", initStateVariables(*contract)); - t("userDefinedConstructorBody", contract->constructor() ? generate(contract->constructor()->body()) : ""); + string body; + if (FunctionDefinition const* constructor = contract->constructor()) + { + vector realModifiers; + for (auto const& modifierInvocation: constructor->modifiers()) + // Filter out the base constructor calls + if (dynamic_cast(modifierInvocation->name().annotation().referencedDeclaration)) + realModifiers.emplace_back(modifierInvocation.get()); + if (realModifiers.empty()) + body = generate(constructor->body()); + else + { + for (size_t i = 0; i < realModifiers.size(); ++i) + { + ModifierInvocation const& modifier = *realModifiers.at(i); + string next = + i + 1 < realModifiers.size() ? + IRNames::modifierInvocation(*realModifiers.at(i + 1)) : + IRNames::functionWithModifierInner(*constructor); + generateModifier(modifier, *constructor, next); + } + body = + IRNames::modifierInvocation(*constructor->modifiers().at(0)) + + "(" + + joinHumanReadable(params) + + ")"; + // Now generate the actual inner function. + generateFunctionWithModifierInner(*constructor); + } + } + t("userDefinedConstructorBody", move(body)); return t.render(); }); diff --git a/test/libsolidity/semanticTests/modifiers/function_modifier_calling_functions_in_creation_context.sol b/test/libsolidity/semanticTests/modifiers/function_modifier_calling_functions_in_creation_context.sol index ebe8d6086..86eb9ffc6 100644 --- a/test/libsolidity/semanticTests/modifiers/function_modifier_calling_functions_in_creation_context.sol +++ b/test/libsolidity/semanticTests/modifiers/function_modifier_calling_functions_in_creation_context.sol @@ -45,5 +45,7 @@ contract C is A { } } +// ==== +// compileViaYul: also // ---- // getData() -> 0x4300 diff --git a/test/libsolidity/semanticTests/modifiers/function_modifier_for_constructor.sol b/test/libsolidity/semanticTests/modifiers/function_modifier_for_constructor.sol index c05f7e202..005d8cafa 100644 --- a/test/libsolidity/semanticTests/modifiers/function_modifier_for_constructor.sol +++ b/test/libsolidity/semanticTests/modifiers/function_modifier_for_constructor.sol @@ -22,6 +22,7 @@ contract C is A { _; } } - +// ==== +// compileViaYul: also // ---- // getData() -> 6 From 1fa371d426be24bc24aaa0ea2137cb05615f2d00 Mon Sep 17 00:00:00 2001 From: chriseth Date: Thu, 3 Dec 2020 16:08:38 +0100 Subject: [PATCH 3/4] Tests --- .../checked_modifier_called_by_unchecked.sol | 2 ++ .../modifiers/evaluation_order.sol | 22 +++++++++++++++++++ .../function_modifier_local_variables.sol | 2 ++ .../modifiers/function_modifier_loop.sol | 3 ++- .../function_modifier_loop_viair.sol | 14 ++++++++++++ .../function_modifier_multi_invocation.sol | 3 ++- ...nction_modifier_multi_invocation_viair.sol | 15 +++++++++++++ .../function_modifier_multi_with_return.sol | 3 ++- .../function_modifier_multiple_times.sol | 2 ++ ...ion_modifier_multiple_times_local_vars.sol | 2 ++ .../modifiers/modifier_init_return.sol | 2 ++ .../stacked_return_with_modifiers.sol | 2 ++ 12 files changed, 69 insertions(+), 3 deletions(-) create mode 100644 test/libsolidity/semanticTests/modifiers/evaluation_order.sol create mode 100644 test/libsolidity/semanticTests/modifiers/function_modifier_loop_viair.sol create mode 100644 test/libsolidity/semanticTests/modifiers/function_modifier_multi_invocation_viair.sol diff --git a/test/libsolidity/semanticTests/arithmetics/checked_modifier_called_by_unchecked.sol b/test/libsolidity/semanticTests/arithmetics/checked_modifier_called_by_unchecked.sol index 42658f83a..322ba77b9 100644 --- a/test/libsolidity/semanticTests/arithmetics/checked_modifier_called_by_unchecked.sol +++ b/test/libsolidity/semanticTests/arithmetics/checked_modifier_called_by_unchecked.sol @@ -8,6 +8,8 @@ contract C { return b + c; } } +// ==== +// compileViaYul: also // ---- // f(uint16,uint16,uint16): 0xe000, 0xe500, 2 -> 58626 // f(uint16,uint16,uint16): 0x1000, 0xe500, 0xe000 -> FAILURE, hex"4e487b71", 0x11 diff --git a/test/libsolidity/semanticTests/modifiers/evaluation_order.sol b/test/libsolidity/semanticTests/modifiers/evaluation_order.sol new file mode 100644 index 000000000..b8b383e88 --- /dev/null +++ b/test/libsolidity/semanticTests/modifiers/evaluation_order.sol @@ -0,0 +1,22 @@ +contract A { constructor(uint) {} } +contract B { constructor(uint) {} } +contract C { constructor(uint) {} } + +contract D is A, B, C { + uint[] x; + constructor() m2(f(1)) B(f(2)) m1(f(3)) C(f(4)) m3(f(5)) A(f(6)) { + f(7); + } + + function query() public view returns (uint[] memory) { return x; } + + modifier m1(uint) { _; } + modifier m2(uint) { _; } + modifier m3(uint) { _; } + + function f(uint y) internal returns (uint) { x.push(y); return 0; } +} +// ==== +// compileViaYul: also +// ---- +// query() -> 0x20, 7, 4, 2, 6, 1, 3, 5, 7 diff --git a/test/libsolidity/semanticTests/modifiers/function_modifier_local_variables.sol b/test/libsolidity/semanticTests/modifiers/function_modifier_local_variables.sol index 1df8b0874..d92976044 100644 --- a/test/libsolidity/semanticTests/modifiers/function_modifier_local_variables.sol +++ b/test/libsolidity/semanticTests/modifiers/function_modifier_local_variables.sol @@ -14,6 +14,8 @@ contract C { } } +// ==== +// compileViaYul: also // ---- // f(bool): true -> 0 // f(bool): false -> 3 diff --git a/test/libsolidity/semanticTests/modifiers/function_modifier_loop.sol b/test/libsolidity/semanticTests/modifiers/function_modifier_loop.sol index 0c4c07828..452ad0dba 100644 --- a/test/libsolidity/semanticTests/modifiers/function_modifier_loop.sol +++ b/test/libsolidity/semanticTests/modifiers/function_modifier_loop.sol @@ -8,6 +8,7 @@ contract C { r += 1; } } - +// ==== +// compileViaYul: false // ---- // f() -> 10 diff --git a/test/libsolidity/semanticTests/modifiers/function_modifier_loop_viair.sol b/test/libsolidity/semanticTests/modifiers/function_modifier_loop_viair.sol new file mode 100644 index 000000000..81caba767 --- /dev/null +++ b/test/libsolidity/semanticTests/modifiers/function_modifier_loop_viair.sol @@ -0,0 +1,14 @@ +contract C { + modifier repeat(uint256 count) { + uint256 i; + for (i = 0; i < count; ++i) _; + } + + function f() public repeat(10) returns (uint256 r) { + r += 1; + } +} +// ==== +// compileViaYul: true +// ---- +// f() -> 1 diff --git a/test/libsolidity/semanticTests/modifiers/function_modifier_multi_invocation.sol b/test/libsolidity/semanticTests/modifiers/function_modifier_multi_invocation.sol index 124e15970..30b67de87 100644 --- a/test/libsolidity/semanticTests/modifiers/function_modifier_multi_invocation.sol +++ b/test/libsolidity/semanticTests/modifiers/function_modifier_multi_invocation.sol @@ -8,7 +8,8 @@ contract C { r += 1; } } - +// ==== +// compileViaYul: false // ---- // f(bool): false -> 1 // f(bool): true -> 2 diff --git a/test/libsolidity/semanticTests/modifiers/function_modifier_multi_invocation_viair.sol b/test/libsolidity/semanticTests/modifiers/function_modifier_multi_invocation_viair.sol new file mode 100644 index 000000000..929b006e3 --- /dev/null +++ b/test/libsolidity/semanticTests/modifiers/function_modifier_multi_invocation_viair.sol @@ -0,0 +1,15 @@ +contract C { + modifier repeat(bool twice) { + if (twice) _; + _; + } + + function f(bool twice) public repeat(twice) returns (uint256 r) { + r += 1; + } +} +// ==== +// compileViaYul: true +// ---- +// f(bool): false -> 1 +// f(bool): true -> 1 diff --git a/test/libsolidity/semanticTests/modifiers/function_modifier_multi_with_return.sol b/test/libsolidity/semanticTests/modifiers/function_modifier_multi_with_return.sol index 286476820..0b27ad8b2 100644 --- a/test/libsolidity/semanticTests/modifiers/function_modifier_multi_with_return.sol +++ b/test/libsolidity/semanticTests/modifiers/function_modifier_multi_with_return.sol @@ -11,7 +11,8 @@ contract C { return r; } } - +// ==== +// compileViaYul: false // ---- // f(bool): false -> 1 // f(bool): true -> 2 diff --git a/test/libsolidity/semanticTests/modifiers/function_modifier_multiple_times.sol b/test/libsolidity/semanticTests/modifiers/function_modifier_multiple_times.sol index 722334ec9..71b572d71 100644 --- a/test/libsolidity/semanticTests/modifiers/function_modifier_multiple_times.sol +++ b/test/libsolidity/semanticTests/modifiers/function_modifier_multiple_times.sol @@ -10,6 +10,8 @@ contract C { } } +// ==== +// compileViaYul: also // ---- // f(uint256): 3 -> 10 // a() -> 10 diff --git a/test/libsolidity/semanticTests/modifiers/function_modifier_multiple_times_local_vars.sol b/test/libsolidity/semanticTests/modifiers/function_modifier_multiple_times_local_vars.sol index 64354ba5e..596fd361e 100644 --- a/test/libsolidity/semanticTests/modifiers/function_modifier_multiple_times_local_vars.sol +++ b/test/libsolidity/semanticTests/modifiers/function_modifier_multiple_times_local_vars.sol @@ -13,6 +13,8 @@ contract C { } } +// ==== +// compileViaYul: also // ---- // f(uint256): 3 -> 10 // a() -> 0 diff --git a/test/libsolidity/semanticTests/modifiers/modifier_init_return.sol b/test/libsolidity/semanticTests/modifiers/modifier_init_return.sol index 097fd274c..36252bdb4 100644 --- a/test/libsolidity/semanticTests/modifiers/modifier_init_return.sol +++ b/test/libsolidity/semanticTests/modifiers/modifier_init_return.sol @@ -8,6 +8,8 @@ contract C { } } +// ==== +// compileViaYul: also // ---- // f(uint256): 9 -> 0x00, 0x00, 0x00, 0x00, 0x00 // f(uint256): 10 -> 0x00, 0x00, 3, 0x00, 0x00 diff --git a/test/libsolidity/semanticTests/modifiers/stacked_return_with_modifiers.sol b/test/libsolidity/semanticTests/modifiers/stacked_return_with_modifiers.sol index 616a2183c..6eb15494e 100644 --- a/test/libsolidity/semanticTests/modifiers/stacked_return_with_modifiers.sol +++ b/test/libsolidity/semanticTests/modifiers/stacked_return_with_modifiers.sol @@ -15,6 +15,8 @@ contract C { } } } +// ==== +// compileViaYul: also // ---- // x() -> 0 // f() -> 42 From 242bf9b6dce5e35ab45a7d098dabe59c7044f3ae Mon Sep 17 00:00:00 2001 From: chriseth Date: Thu, 3 Dec 2020 16:21:21 +0100 Subject: [PATCH 4/4] Document modifier changes. --- docs/ir/ir-breaking-changes.rst | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/docs/ir/ir-breaking-changes.rst b/docs/ir/ir-breaking-changes.rst index 13a3e0759..ea0823be2 100644 --- a/docs/ir/ir-breaking-changes.rst +++ b/docs/ir/ir-breaking-changes.rst @@ -34,6 +34,27 @@ Consequently, if the padding space within a struct is used to store data (e.g. i We have the same behavior for implicit delete, for example when array of structs is shortened. + * Function modifiers are implemented in a slightly different way regarding function parameters. + This especially has an effect if the placeholder ``_;`` is evaluated multiple times in a modifier. + In the old code generator, each function parameter has a fixed slot on the stack. If the function + is run multiple times because ``_;`` is used multiple times or used in a loop, then a change to the + function parameter's value is visible in the next execution of the function. + The new code generator implements modifiers using actual functions and passes function parameters on. + This means that multiple executions of a function will get the same values for the parameters. + +:: + // SPDX-License-Identifier: GPL-3.0 + pragma solidity >=0.7.0; + contract C { + function f(uint a) public pure mod() returns (uint r) { + r = a++; + } + modifier mod() { _; _; } + } + +If you execute ``f(0)`` in the old code generator, it will return ``2``, while +it will return ``1`` when using the new code generator. + * The order of contract initialization has changed in case of inheritance. The order used to be: