From 1f77deada1abc9ca1e635ab13d45707e852a5628 Mon Sep 17 00:00:00 2001 From: Leonardo Alt Date: Thu, 3 May 2018 16:40:33 +0200 Subject: [PATCH 1/5] [050] Reserving and popping local vars in their scope --- Changelog.md | 1 + libsolidity/codegen/CompilerContext.cpp | 15 +++- libsolidity/codegen/CompilerContext.h | 4 +- libsolidity/codegen/ContractCompiler.cpp | 104 +++++++++++++++-------- libsolidity/codegen/ContractCompiler.h | 28 ++++-- 5 files changed, 111 insertions(+), 41 deletions(-) diff --git a/Changelog.md b/Changelog.md index 1e92863be..b349a5e12 100644 --- a/Changelog.md +++ b/Changelog.md @@ -50,6 +50,7 @@ Compiler Features: * C API (``libsolc``): Export the ``solidity_license``, ``solidity_version`` and ``solidity_compile`` methods. * Type Checker: Show named argument in case of error. * Tests: Determine transaction status during IPC calls. + * Code Generator: Allocate and free local variables according to their scope. Bugfixes: * Tests: Fix chain parameters to make ipc tests work with newer versions of cpp-ethereum. diff --git a/libsolidity/codegen/CompilerContext.cpp b/libsolidity/codegen/CompilerContext.cpp index a35eea738..9c5dc89cd 100644 --- a/libsolidity/codegen/CompilerContext.cpp +++ b/libsolidity/codegen/CompilerContext.cpp @@ -130,7 +130,7 @@ void CompilerContext::addVariable(VariableDeclaration const& _declaration, m_localVariables[&_declaration].push_back(unsigned(m_asm->deposit()) - _offsetToCurrent); } -void CompilerContext::removeVariable(VariableDeclaration const& _declaration) +void CompilerContext::removeVariable(Declaration const& _declaration) { solAssert(m_localVariables.count(&_declaration) && !m_localVariables[&_declaration].empty(), ""); m_localVariables[&_declaration].pop_back(); @@ -138,6 +138,19 @@ void CompilerContext::removeVariable(VariableDeclaration const& _declaration) m_localVariables.erase(&_declaration); } +void CompilerContext::removeVariablesAboveStackHeight(unsigned _stackHeight) +{ + vector toRemove; + for (auto _var: m_localVariables) + { + solAssert(!_var.second.empty(), ""); + if (_var.second.back() >= _stackHeight) + toRemove.push_back(_var.first); + } + for (auto _var: toRemove) + removeVariable(*_var); +} + eth::Assembly const& CompilerContext::compiledContract(const ContractDefinition& _contract) const { auto ret = m_compiledContracts.find(&_contract); diff --git a/libsolidity/codegen/CompilerContext.h b/libsolidity/codegen/CompilerContext.h index 5776b5d1b..f3934615e 100644 --- a/libsolidity/codegen/CompilerContext.h +++ b/libsolidity/codegen/CompilerContext.h @@ -71,7 +71,9 @@ public: void addStateVariable(VariableDeclaration const& _declaration, u256 const& _storageOffset, unsigned _byteOffset); void addVariable(VariableDeclaration const& _declaration, unsigned _offsetToCurrent = 0); - void removeVariable(VariableDeclaration const& _declaration); + void removeVariable(Declaration const& _declaration); + /// Removes all local variables currently allocated above _stackHeight. + void removeVariablesAboveStackHeight(unsigned _stackHeight); void setCompiledContracts(std::map const& _contracts) { m_compiledContracts = _contracts; } eth::Assembly const& compiledContract(ContractDefinition const& _contract) const; diff --git a/libsolidity/codegen/ContractCompiler.cpp b/libsolidity/codegen/ContractCompiler.cpp index 81aba21ea..def4a878c 100644 --- a/libsolidity/codegen/ContractCompiler.cpp +++ b/libsolidity/codegen/ContractCompiler.cpp @@ -427,7 +427,7 @@ bool ContractCompiler::visit(FunctionDefinition const& _function) m_context.startFunction(_function); // stack upon entry: [return address] [arg0] [arg1] ... [argn] - // reserve additional slots: [retarg0] ... [retargm] [localvar0] ... [localvarp] + // reserve additional slots: [retarg0] ... [retargm] unsigned parametersSize = CompilerUtils::sizeOnStack(_function.parameters()); if (!_function.isConstructor()) @@ -441,22 +441,18 @@ bool ContractCompiler::visit(FunctionDefinition const& _function) for (ASTPointer const& variable: _function.returnParameters()) appendStackVariableInitialisation(*variable); - for (VariableDeclaration const* localVariable: _function.localVariables()) - appendStackVariableInitialisation(*localVariable); if (_function.isConstructor()) if (auto c = m_context.nextConstructor(dynamic_cast(*_function.scope()))) appendBaseConstructor(*c); - solAssert(m_returnTags.empty(), ""); m_breakTags.clear(); m_continueTags.clear(); - m_stackCleanupForReturn = 0; m_currentFunction = &_function; m_modifierDepth = -1; + m_scopeStackHeight.clear(); appendModifierOrFunctionCode(); - solAssert(m_returnTags.empty(), ""); // Now we need to re-shuffle the stack. For this we keep a record of the stack layout @@ -467,14 +463,12 @@ bool ContractCompiler::visit(FunctionDefinition const& _function) unsigned const c_argumentsSize = CompilerUtils::sizeOnStack(_function.parameters()); unsigned const c_returnValuesSize = CompilerUtils::sizeOnStack(_function.returnParameters()); - unsigned const c_localVariablesSize = CompilerUtils::sizeOnStack(_function.localVariables()); vector stackLayout; stackLayout.push_back(c_returnValuesSize); // target of return address stackLayout += vector(c_argumentsSize, -1); // discard all arguments for (unsigned i = 0; i < c_returnValuesSize; ++i) stackLayout.push_back(i); - stackLayout += vector(c_localVariablesSize, -1); if (stackLayout.size() > 17) BOOST_THROW_EXCEPTION( @@ -493,18 +487,19 @@ bool ContractCompiler::visit(FunctionDefinition const& _function) m_context << swapInstruction(stackLayout.size() - stackLayout.back() - 1); swap(stackLayout[stackLayout.back()], stackLayout.back()); } - //@todo assert that everything is in place now + for (int i = 0; i < int(stackLayout.size()); ++i) + if (stackLayout[i] != i) + solAssert(false, "Invalid stack layout on cleanup."); for (ASTPointer const& variable: _function.parameters() + _function.returnParameters()) m_context.removeVariable(*variable); - for (VariableDeclaration const* localVariable: _function.localVariables()) - m_context.removeVariable(*localVariable); m_context.adjustStackOffset(-(int)c_returnValuesSize); /// The constructor and the fallback function doesn't to jump out. if (!_function.isConstructor() && !_function.isFallback()) m_context.appendJump(eth::AssemblyItem::JumpType::OutOfFunction); + return false; } @@ -669,14 +664,16 @@ bool ContractCompiler::visit(WhileStatement const& _whileStatement) eth::AssemblyItem loopStart = m_context.newTag(); eth::AssemblyItem loopEnd = m_context.newTag(); - m_breakTags.push_back(loopEnd); + m_breakTags.push_back({loopEnd, m_context.stackHeight()}); + + visitLoop(&_whileStatement); m_context << loopStart; if (_whileStatement.isDoWhile()) { eth::AssemblyItem condition = m_context.newTag(); - m_continueTags.push_back(condition); + m_continueTags.push_back({condition, m_context.stackHeight()}); _whileStatement.body().accept(*this); @@ -687,7 +684,7 @@ bool ContractCompiler::visit(WhileStatement const& _whileStatement) } else { - m_continueTags.push_back(loopStart); + m_continueTags.push_back({loopStart, m_context.stackHeight()}); compileExpression(_whileStatement.condition()); m_context << Instruction::ISZERO; m_context.appendConditionalJumpTo(loopEnd); @@ -712,12 +709,14 @@ bool ContractCompiler::visit(ForStatement const& _forStatement) eth::AssemblyItem loopStart = m_context.newTag(); eth::AssemblyItem loopEnd = m_context.newTag(); eth::AssemblyItem loopNext = m_context.newTag(); - m_continueTags.push_back(loopNext); - m_breakTags.push_back(loopEnd); + + visitLoop(&_forStatement); if (_forStatement.initializationExpression()) _forStatement.initializationExpression()->accept(*this); + m_breakTags.push_back({loopEnd, m_context.stackHeight()}); + m_continueTags.push_back({loopNext, m_context.stackHeight()}); m_context << loopStart; // if there is no terminating condition in for, default is to always be true @@ -737,11 +736,16 @@ bool ContractCompiler::visit(ForStatement const& _forStatement) _forStatement.loopExpression()->accept(*this); m_context.appendJumpTo(loopStart); + m_context << loopEnd; m_continueTags.pop_back(); m_breakTags.pop_back(); + // For the case where no break/return is executed: + // loop initialization variables have to be freed + popScopedVariables(&_forStatement); + checker.check(); return false; } @@ -750,7 +754,7 @@ bool ContractCompiler::visit(Continue const& _continueStatement) { CompilerContext::LocationSetter locationSetter(m_context, _continueStatement); solAssert(!m_continueTags.empty(), ""); - m_context.appendJumpTo(m_continueTags.back()); + popAndJump(m_context.stackHeight() - m_continueTags.back().second, m_continueTags.back().first); return false; } @@ -758,7 +762,7 @@ bool ContractCompiler::visit(Break const& _breakStatement) { CompilerContext::LocationSetter locationSetter(m_context, _breakStatement); solAssert(!m_breakTags.empty(), ""); - m_context.appendJumpTo(m_breakTags.back()); + popAndJump(m_context.stackHeight() - m_breakTags.back().second, m_breakTags.back().first); return false; } @@ -784,10 +788,8 @@ bool ContractCompiler::visit(Return const& _return) for (auto const& retVariable: boost::adaptors::reverse(returnParameters)) CompilerUtils(m_context).moveToStackVariable(*retVariable); } - for (unsigned i = 0; i < m_stackCleanupForReturn; ++i) - m_context << Instruction::POP; - m_context.appendJumpTo(m_returnTags.back()); - m_context.adjustStackOffset(m_stackCleanupForReturn); + + popAndJump(m_context.stackHeight() - m_returnTags.back().second, m_returnTags.back().first); return false; } @@ -810,8 +812,15 @@ bool ContractCompiler::visit(EmitStatement const& _emit) bool ContractCompiler::visit(VariableDeclarationStatement const& _variableDeclarationStatement) { - StackHeightChecker checker(m_context); CompilerContext::LocationSetter locationSetter(m_context, _variableDeclarationStatement); + + // Local variable slots are reserved when their declaration is visited, + // and freed in the end of their scope. + for (auto _decl: _variableDeclarationStatement.declarations()) + if (_decl) + appendStackVariableInitialisation(*_decl); + + StackHeightChecker checker(m_context); if (Expression const* expression = _variableDeclarationStatement.initialValue()) { CompilerUtils utils(m_context); @@ -861,6 +870,18 @@ bool ContractCompiler::visit(PlaceholderStatement const& _placeholderStatement) return true; } +bool ContractCompiler::visit(Block const& _block) +{ + m_scopeStackHeight[m_modifierDepth][&_block] = m_context.stackHeight(); + return true; +} + +void ContractCompiler::endVisit(Block const& _block) +{ + // Frees local variables declared in the scope of this block. + popScopedVariables(&_block); +} + void ContractCompiler::appendMissingFunctions() { while (Declaration const* function = m_context.nextFunctionToCompile()) @@ -916,27 +937,19 @@ void ContractCompiler::appendModifierOrFunctionCode() modifier.parameters()[i]->annotation().type ); } - for (VariableDeclaration const* localVariable: modifier.localVariables()) - { - addedVariables.push_back(localVariable); - appendStackVariableInitialisation(*localVariable); - } - stackSurplus = - CompilerUtils::sizeOnStack(modifier.parameters()) + - CompilerUtils::sizeOnStack(modifier.localVariables()); + stackSurplus = CompilerUtils::sizeOnStack(modifier.parameters()); codeBlock = &modifier.body(); } } if (codeBlock) { - m_returnTags.push_back(m_context.newTag()); - + m_returnTags.push_back({m_context.newTag(), m_context.stackHeight()}); codeBlock->accept(*this); solAssert(!m_returnTags.empty(), ""); - m_context << m_returnTags.back(); + m_context << m_returnTags.back().first; m_returnTags.pop_back(); CompilerUtils(m_context).popStackSlots(stackSurplus); @@ -983,3 +996,26 @@ eth::AssemblyPointer ContractCompiler::cloneRuntime() const a << u256(0x20) << u256(0) << Instruction::RETURN; return make_shared(a); } + +void ContractCompiler::popScopedVariables(ASTNode const* _node) +{ + unsigned blockHeight = m_scopeStackHeight[m_modifierDepth][_node]; + unsigned stackDiff = m_context.stackHeight() - blockHeight; + CompilerUtils(m_context).popStackSlots(stackDiff); + m_context.removeVariablesAboveStackHeight(blockHeight); + m_scopeStackHeight[m_modifierDepth].erase(_node); + if (m_scopeStackHeight[m_modifierDepth].size() == 0) + m_scopeStackHeight.erase(m_modifierDepth); +} + +void ContractCompiler::visitLoop(BreakableStatement const* _loop) +{ + m_scopeStackHeight[m_modifierDepth][_loop] = m_context.stackHeight(); +} + +void ContractCompiler::popAndJump(unsigned _amount, eth::AssemblyItem const& _jumpTo) +{ + CompilerUtils(m_context).popStackSlots(_amount); + m_context.appendJumpTo(_jumpTo); + m_context.adjustStackOffset(_amount); +} diff --git a/libsolidity/codegen/ContractCompiler.h b/libsolidity/codegen/ContractCompiler.h index 02a3452fd..72f46495a 100644 --- a/libsolidity/codegen/ContractCompiler.h +++ b/libsolidity/codegen/ContractCompiler.h @@ -109,6 +109,8 @@ private: virtual bool visit(VariableDeclarationStatement const& _variableDeclarationStatement) override; virtual bool visit(ExpressionStatement const& _expressionStatement) override; virtual bool visit(PlaceholderStatement const&) override; + virtual bool visit(Block const& _block) override; + virtual void endVisit(Block const& _block) override; /// Repeatedly visits all function which are referenced but which are not compiled yet. void appendMissingFunctions(); @@ -123,19 +125,35 @@ private: /// @returns the runtime assembly for clone contracts. eth::AssemblyPointer cloneRuntime() const; + /// Frees the variables of a certain scope (to be used when leaving). + void popScopedVariables(ASTNode const* _node); + + /// Pops _amount slots from the stack and jumps to _jumpTo. + /// Also readjusts the stack offset to the original value. + void popAndJump(unsigned _amount, eth::AssemblyItem const& _jumpTo); + + /// Sets the stack height for the visited loop. + void visitLoop(BreakableStatement const* _loop); + bool const m_optimise; /// Pointer to the runtime compiler in case this is a creation compiler. ContractCompiler* m_runtimeCompiler = nullptr; CompilerContext& m_context; - std::vector m_breakTags; ///< tag to jump to for a "break" statement - std::vector m_continueTags; ///< tag to jump to for a "continue" statement - /// Tag to jump to for a "return" statement, needs to be stacked because of modifiers. - std::vector m_returnTags; + /// Tag to jump to for a "break" statement and the stack height after freeing the local loop variables. + std::vector> m_breakTags; + /// Tag to jump to for a "continue" statement and the stack height after freeing the local loop variables. + std::vector> m_continueTags; + /// Tag to jump to for a "return" statement and the stack height after freeing the local function or modifier variables. + /// Needs to be stacked because of modifiers. + std::vector> m_returnTags; unsigned m_modifierDepth = 0; FunctionDefinition const* m_currentFunction = nullptr; - unsigned m_stackCleanupForReturn = 0; ///< this number of stack elements need to be removed before jump to m_returnTag + // arguments for base constructors, filled in derived-to-base order std::map const* m_baseArguments; + + /// Stores the variables that were declared inside a specific scope, for each modifier depth. + std::map> m_scopeStackHeight; }; } From 9d895e002d0e5e1e3435d03ac82277caa397bbec Mon Sep 17 00:00:00 2001 From: Leonardo Alt Date: Wed, 4 Jul 2018 14:14:41 +0200 Subject: [PATCH 2/5] Added tests and review suggestions --- libsolidity/codegen/CompilerUtils.cpp | 8 ++ libsolidity/codegen/CompilerUtils.h | 4 + libsolidity/codegen/ContractCompiler.cpp | 26 ++-- libsolidity/codegen/ContractCompiler.h | 6 +- test/libsolidity/SolidityEndToEndTest.cpp | 154 ++++++++++++++++++++++ 5 files changed, 176 insertions(+), 22 deletions(-) diff --git a/libsolidity/codegen/CompilerUtils.cpp b/libsolidity/codegen/CompilerUtils.cpp index 2f45765a8..92c452271 100644 --- a/libsolidity/codegen/CompilerUtils.cpp +++ b/libsolidity/codegen/CompilerUtils.cpp @@ -1170,6 +1170,14 @@ void CompilerUtils::popStackSlots(size_t _amount) m_context << Instruction::POP; } +void CompilerUtils::popAndJump(unsigned _toHeight, eth::AssemblyItem const& _jumpTo) +{ + unsigned amount = m_context.stackHeight() - _toHeight; + popStackSlots(amount); + m_context.appendJumpTo(_jumpTo); + m_context.adjustStackOffset(amount); +} + unsigned CompilerUtils::sizeOnStack(vector> const& _variableTypes) { unsigned size = 0; diff --git a/libsolidity/codegen/CompilerUtils.h b/libsolidity/codegen/CompilerUtils.h index 0ff3ad7c7..26df47652 100644 --- a/libsolidity/codegen/CompilerUtils.h +++ b/libsolidity/codegen/CompilerUtils.h @@ -241,6 +241,10 @@ public: void popStackElement(Type const& _type); /// Removes element from the top of the stack _amount times. void popStackSlots(size_t _amount); + /// Pops slots from the stack such that its height is _toHeight. + /// Adds jump to _jumpTo. + /// Readjusts the stack offset to the original value. + void popAndJump(unsigned _toHeight, eth::AssemblyItem const& _jumpTo); template static unsigned sizeOnStack(std::vector const& _variables); diff --git a/libsolidity/codegen/ContractCompiler.cpp b/libsolidity/codegen/ContractCompiler.cpp index def4a878c..474d2b687 100644 --- a/libsolidity/codegen/ContractCompiler.cpp +++ b/libsolidity/codegen/ContractCompiler.cpp @@ -446,6 +446,7 @@ bool ContractCompiler::visit(FunctionDefinition const& _function) if (auto c = m_context.nextConstructor(dynamic_cast(*_function.scope()))) appendBaseConstructor(*c); + solAssert(m_returnTags.empty(), ""); m_breakTags.clear(); m_continueTags.clear(); m_currentFunction = &_function; @@ -666,8 +667,6 @@ bool ContractCompiler::visit(WhileStatement const& _whileStatement) eth::AssemblyItem loopEnd = m_context.newTag(); m_breakTags.push_back({loopEnd, m_context.stackHeight()}); - visitLoop(&_whileStatement); - m_context << loopStart; if (_whileStatement.isDoWhile()) @@ -710,7 +709,7 @@ bool ContractCompiler::visit(ForStatement const& _forStatement) eth::AssemblyItem loopEnd = m_context.newTag(); eth::AssemblyItem loopNext = m_context.newTag(); - visitLoop(&_forStatement); + storeStackHeight(&_forStatement); if (_forStatement.initializationExpression()) _forStatement.initializationExpression()->accept(*this); @@ -754,7 +753,7 @@ bool ContractCompiler::visit(Continue const& _continueStatement) { CompilerContext::LocationSetter locationSetter(m_context, _continueStatement); solAssert(!m_continueTags.empty(), ""); - popAndJump(m_context.stackHeight() - m_continueTags.back().second, m_continueTags.back().first); + CompilerUtils(m_context).popAndJump(m_continueTags.back().second, m_continueTags.back().first); return false; } @@ -762,7 +761,7 @@ bool ContractCompiler::visit(Break const& _breakStatement) { CompilerContext::LocationSetter locationSetter(m_context, _breakStatement); solAssert(!m_breakTags.empty(), ""); - popAndJump(m_context.stackHeight() - m_breakTags.back().second, m_breakTags.back().first); + CompilerUtils(m_context).popAndJump(m_breakTags.back().second, m_breakTags.back().first); return false; } @@ -789,7 +788,7 @@ bool ContractCompiler::visit(Return const& _return) CompilerUtils(m_context).moveToStackVariable(*retVariable); } - popAndJump(m_context.stackHeight() - m_returnTags.back().second, m_returnTags.back().first); + CompilerUtils(m_context).popAndJump(m_returnTags.back().second, m_returnTags.back().first); return false; } @@ -872,7 +871,7 @@ bool ContractCompiler::visit(PlaceholderStatement const& _placeholderStatement) bool ContractCompiler::visit(Block const& _block) { - m_scopeStackHeight[m_modifierDepth][&_block] = m_context.stackHeight(); + storeStackHeight(&_block); return true; } @@ -999,7 +998,7 @@ eth::AssemblyPointer ContractCompiler::cloneRuntime() const void ContractCompiler::popScopedVariables(ASTNode const* _node) { - unsigned blockHeight = m_scopeStackHeight[m_modifierDepth][_node]; + unsigned blockHeight = m_scopeStackHeight.at(m_modifierDepth).at(_node); unsigned stackDiff = m_context.stackHeight() - blockHeight; CompilerUtils(m_context).popStackSlots(stackDiff); m_context.removeVariablesAboveStackHeight(blockHeight); @@ -1008,14 +1007,7 @@ void ContractCompiler::popScopedVariables(ASTNode const* _node) m_scopeStackHeight.erase(m_modifierDepth); } -void ContractCompiler::visitLoop(BreakableStatement const* _loop) +void ContractCompiler::storeStackHeight(ASTNode const* _node) { - m_scopeStackHeight[m_modifierDepth][_loop] = m_context.stackHeight(); -} - -void ContractCompiler::popAndJump(unsigned _amount, eth::AssemblyItem const& _jumpTo) -{ - CompilerUtils(m_context).popStackSlots(_amount); - m_context.appendJumpTo(_jumpTo); - m_context.adjustStackOffset(_amount); + m_scopeStackHeight[m_modifierDepth][_node] = m_context.stackHeight(); } diff --git a/libsolidity/codegen/ContractCompiler.h b/libsolidity/codegen/ContractCompiler.h index 72f46495a..8516ec2c0 100644 --- a/libsolidity/codegen/ContractCompiler.h +++ b/libsolidity/codegen/ContractCompiler.h @@ -128,12 +128,8 @@ private: /// Frees the variables of a certain scope (to be used when leaving). void popScopedVariables(ASTNode const* _node); - /// Pops _amount slots from the stack and jumps to _jumpTo. - /// Also readjusts the stack offset to the original value. - void popAndJump(unsigned _amount, eth::AssemblyItem const& _jumpTo); - /// Sets the stack height for the visited loop. - void visitLoop(BreakableStatement const* _loop); + void storeStackHeight(ASTNode const* _node); bool const m_optimise; /// Pointer to the runtime compiler in case this is a creation compiler. diff --git a/test/libsolidity/SolidityEndToEndTest.cpp b/test/libsolidity/SolidityEndToEndTest.cpp index fbc6a9c6e..52fdfed70 100644 --- a/test/libsolidity/SolidityEndToEndTest.cpp +++ b/test/libsolidity/SolidityEndToEndTest.cpp @@ -523,6 +523,57 @@ BOOST_AUTO_TEST_CASE(do_while_loop_continue) ABI_CHECK(callContractFunction("f()"), encodeArgs(42)); } +BOOST_AUTO_TEST_CASE(do_while_loop_multiple_local_vars) +{ + char const* sourceCode = R"( + contract test { + function f(uint x) public pure returns(uint r) { + uint i = 0; + do + { + uint z = x * 2; + if (z < 4) break; + else { + uint k = z + 1; + if (k < 8) { + x++; + continue; + } + } + if (z > 12) return 0; + x++; + i++; + } while (true); + return 42; + } + } + )"; + compileAndRun(sourceCode); + + auto do_while = [](u256 n) -> u256 + { + u256 i = 0; + do + { + u256 z = n * 2; + if (z < 4) break; + else { + u256 k = z + 1; + if (k < 8) { + n++; + continue; + } + } + if (z > 12) return 0; + n++; + i++; + } while (true); + return 42; + }; + + testContractAgainstCppOnRange("f(uint256)", do_while, 0, 12); +} + BOOST_AUTO_TEST_CASE(nested_loops) { // tests that break and continue statements in nested loops jump to the correct place @@ -574,6 +625,109 @@ BOOST_AUTO_TEST_CASE(nested_loops) testContractAgainstCppOnRange("f(uint256)", nested_loops_cpp, 0, 12); } +BOOST_AUTO_TEST_CASE(nested_loops_multiple_local_vars) +{ + // tests that break and continue statements in nested loops jump to the correct place + char const* sourceCode = R"( + contract test { + function f(uint x) returns(uint y) { + while (x > 0) { + uint z = x + 10; + uint k = z + 1; + if (k > 20) break; + if (k > 15) { + x--; + continue; + } + while (k > 10) { + uint m = k - 1; + if (m == 10) return x; + return k; + } + x--; + break; + } + return x; + } + } + )"; + compileAndRun(sourceCode); + + auto nested_loops_cpp = [](u256 n) -> u256 + { + while (n > 0) + { + u256 z = n + 10; + u256 k = z + 1; + if (k > 20) break; + if (k > 15) { + n--; + continue; + } + while (k > 10) + { + u256 m = k - 1; + if (m == 10) return n; + return k; + } + n--; + break; + } + + return n; + }; + + testContractAgainstCppOnRange("f(uint256)", nested_loops_cpp, 0, 12); +} + +BOOST_AUTO_TEST_CASE(for_loop_multiple_local_vars) +{ + char const* sourceCode = R"( + contract test { + function f(uint x) public pure returns(uint r) { + for (uint i = 0; i < 12; i++) + { + uint z = x + 1; + if (z < 4) break; + else { + uint k = z * 2; + if (i + k < 10) { + x++; + continue; + } + } + if (z > 8) return 0; + x++; + } + return 42; + } + } + )"; + compileAndRun(sourceCode); + + auto for_loop = [](u256 n) -> u256 + { + for (u256 i = 0; i < 12; i++) + { + u256 z = n + 1; + if (z < 4) break; + else { + u256 k = z * 2; + if (i + k < 10) { + n++; + continue; + } + } + if (z > 8) return 0; + n++; + } + return 42; + }; + + testContractAgainstCppOnRange("f(uint256)", for_loop, 0, 12); +} + + BOOST_AUTO_TEST_CASE(for_loop) { char const* sourceCode = R"( From b750ca9741aba47cc8ba650a04dd620725ed4610 Mon Sep 17 00:00:00 2001 From: Leonardo Alt Date: Fri, 6 Jul 2018 09:56:27 +0200 Subject: [PATCH 3/5] Add more tests and assertions --- libsolidity/codegen/CompilerContext.cpp | 2 + libsolidity/codegen/CompilerUtils.cpp | 1 + libsolidity/codegen/ContractCompiler.cpp | 1 + test/libsolidity/SolidityEndToEndTest.cpp | 154 +++++++++++++++++++++- 4 files changed, 157 insertions(+), 1 deletion(-) diff --git a/libsolidity/codegen/CompilerContext.cpp b/libsolidity/codegen/CompilerContext.cpp index 9c5dc89cd..51f763991 100644 --- a/libsolidity/codegen/CompilerContext.cpp +++ b/libsolidity/codegen/CompilerContext.cpp @@ -127,6 +127,8 @@ void CompilerContext::addVariable(VariableDeclaration const& _declaration, unsigned _offsetToCurrent) { solAssert(m_asm->deposit() >= 0 && unsigned(m_asm->deposit()) >= _offsetToCurrent, ""); + unsigned sizeOnStack = _declaration.annotation().type->sizeOnStack(); + solAssert(sizeOnStack == 1 || sizeOnStack == 2, ""); m_localVariables[&_declaration].push_back(unsigned(m_asm->deposit()) - _offsetToCurrent); } diff --git a/libsolidity/codegen/CompilerUtils.cpp b/libsolidity/codegen/CompilerUtils.cpp index 92c452271..5adce4a03 100644 --- a/libsolidity/codegen/CompilerUtils.cpp +++ b/libsolidity/codegen/CompilerUtils.cpp @@ -1172,6 +1172,7 @@ void CompilerUtils::popStackSlots(size_t _amount) void CompilerUtils::popAndJump(unsigned _toHeight, eth::AssemblyItem const& _jumpTo) { + solAssert(m_context.stackHeight() >= _toHeight, ""); unsigned amount = m_context.stackHeight() - _toHeight; popStackSlots(amount); m_context.appendJumpTo(_jumpTo); diff --git a/libsolidity/codegen/ContractCompiler.cpp b/libsolidity/codegen/ContractCompiler.cpp index 474d2b687..207e0af72 100644 --- a/libsolidity/codegen/ContractCompiler.cpp +++ b/libsolidity/codegen/ContractCompiler.cpp @@ -999,6 +999,7 @@ eth::AssemblyPointer ContractCompiler::cloneRuntime() const void ContractCompiler::popScopedVariables(ASTNode const* _node) { unsigned blockHeight = m_scopeStackHeight.at(m_modifierDepth).at(_node); + solAssert(m_context.stackHeight() >= blockHeight, ""); unsigned stackDiff = m_context.stackHeight() - blockHeight; CompilerUtils(m_context).popStackSlots(stackDiff); m_context.removeVariablesAboveStackHeight(blockHeight); diff --git a/test/libsolidity/SolidityEndToEndTest.cpp b/test/libsolidity/SolidityEndToEndTest.cpp index 52fdfed70..ae92810dd 100644 --- a/test/libsolidity/SolidityEndToEndTest.cpp +++ b/test/libsolidity/SolidityEndToEndTest.cpp @@ -523,6 +523,45 @@ BOOST_AUTO_TEST_CASE(do_while_loop_continue) ABI_CHECK(callContractFunction("f()"), encodeArgs(42)); } +BOOST_AUTO_TEST_CASE(array_multiple_local_vars) +{ + char const* sourceCode = R"( + contract test { + function f(uint256[] seq) public pure returns (uint256) { + uint i = 0; + uint sum = 0; + while (i < seq.length) + { + uint idx = i; + if (idx >= 10) break; + uint x = seq[idx]; + if (x >= 1000) { + uint n = i + 1; + i = n; + continue; + } + else { + uint y = sum + x; + sum = y; + } + if (sum >= 500) return sum; + i++; + } + return sum; + } + } + )"; + compileAndRun(sourceCode); + + ABI_CHECK(callContractFunction("f(uint256[])", 32, 3, u256(1000), u256(1), u256(2)), encodeArgs(3)); + ABI_CHECK(callContractFunction("f(uint256[])", 32, 3, u256(100), u256(500), u256(300)), encodeArgs(600)); + ABI_CHECK(callContractFunction( + "f(uint256[])", 32, 11, + u256(1), u256(2), u256(3), u256(4), u256(5), u256(6), u256(7), u256(8), u256(9), u256(10), u256(111) + ), encodeArgs(55)); +} + + BOOST_AUTO_TEST_CASE(do_while_loop_multiple_local_vars) { char const* sourceCode = R"( @@ -628,21 +667,30 @@ BOOST_AUTO_TEST_CASE(nested_loops) BOOST_AUTO_TEST_CASE(nested_loops_multiple_local_vars) { // tests that break and continue statements in nested loops jump to the correct place + // and free local variables properly char const* sourceCode = R"( contract test { function f(uint x) returns(uint y) { while (x > 0) { uint z = x + 10; uint k = z + 1; - if (k > 20) break; + if (k > 20) { + break; + uint p = 100; + k += p; + } if (k > 15) { x--; continue; + uint t = 1000; + x += t; } while (k > 10) { uint m = k - 1; if (m == 10) return x; return k; + uint h = 10000; + z += h; } x--; break; @@ -727,6 +775,66 @@ BOOST_AUTO_TEST_CASE(for_loop_multiple_local_vars) testContractAgainstCppOnRange("f(uint256)", for_loop, 0, 12); } +BOOST_AUTO_TEST_CASE(nested_for_loop_multiple_local_vars) +{ + char const* sourceCode = R"( + contract test { + function f(uint x) public pure returns(uint r) { + for (uint i = 0; i < 5; i++) + { + uint z = x + 1; + if (z < 3) { + break; + uint p = z + 2; + } + for (uint j = 0; j < 5; j++) + { + uint k = z * 2; + if (j + k < 8) { + x++; + continue; + uint t = z * 3; + } + x++; + if (x > 20) { + return 84; + uint h = x + 42; + } + } + if (x > 30) { + return 42; + uint b = 0xcafe; + } + } + return 42; + } + } + )"; + compileAndRun(sourceCode); + + auto for_loop = [](u256 n) -> u256 + { + for (u256 i = 0; i < 5; i++) + { + u256 z = n + 1; + if (z < 3) break; + for (u256 j = 0; j < 5; j++) + { + u256 k = z * 2; + if (j + k < 8) { + n++; + continue; + } + n++; + if (n > 20) return 84; + } + if (n > 30) return 42; + } + return 42; + }; + + testContractAgainstCppOnRange("f(uint256)", for_loop, 0, 12); +} BOOST_AUTO_TEST_CASE(for_loop) { @@ -9396,6 +9504,50 @@ BOOST_AUTO_TEST_CASE(break_in_modifier) ABI_CHECK(callContractFunction("x()"), encodeArgs(u256(1))); } +BOOST_AUTO_TEST_CASE(continue_in_modifier) +{ + char const* sourceCode = R"( + contract C { + uint public x; + modifier run() { + for (uint i = 0; i < 10; i++) { + if (i % 2 == 1) continue; + _; + } + } + function f() run { + x++; + } + } + )"; + compileAndRun(sourceCode, 0, "C"); + ABI_CHECK(callContractFunction("x()"), encodeArgs(u256(0))); + ABI_CHECK(callContractFunction("f()"), encodeArgs()); + ABI_CHECK(callContractFunction("x()"), encodeArgs(u256(5))); +} + +BOOST_AUTO_TEST_CASE(return_in_modifier) +{ + char const* sourceCode = R"( + contract C { + uint public x; + modifier run() { + for (uint i = 1; i < 10; i++) { + if (i == 5) return; + _; + } + } + function f() run { + x++; + } + } + )"; + compileAndRun(sourceCode, 0, "C"); + ABI_CHECK(callContractFunction("x()"), encodeArgs(u256(0))); + ABI_CHECK(callContractFunction("f()"), encodeArgs()); + ABI_CHECK(callContractFunction("x()"), encodeArgs(u256(4))); +} + BOOST_AUTO_TEST_CASE(stacked_return_with_modifiers) { char const* sourceCode = R"( From 0c5e0e0d59dc55fcfe5b95a8c649a7a769ad3400 Mon Sep 17 00:00:00 2001 From: Leonardo Alt Date: Tue, 10 Jul 2018 18:39:26 +0200 Subject: [PATCH 4/5] Added assertion and tests suggestions --- libsolidity/codegen/CompilerContext.cpp | 8 ++++++++ libsolidity/codegen/CompilerContext.h | 2 ++ libsolidity/codegen/ContractCompiler.cpp | 10 +++++++--- test/libsolidity/SolidityEndToEndTest.cpp | 18 +++++++++++++----- 4 files changed, 30 insertions(+), 8 deletions(-) diff --git a/libsolidity/codegen/CompilerContext.cpp b/libsolidity/codegen/CompilerContext.cpp index 51f763991..3b1b4ec06 100644 --- a/libsolidity/codegen/CompilerContext.cpp +++ b/libsolidity/codegen/CompilerContext.cpp @@ -128,6 +128,8 @@ void CompilerContext::addVariable(VariableDeclaration const& _declaration, { solAssert(m_asm->deposit() >= 0 && unsigned(m_asm->deposit()) >= _offsetToCurrent, ""); unsigned sizeOnStack = _declaration.annotation().type->sizeOnStack(); + // Variables should not have stack size other than [1, 2], + // but that might change when new types are introduced. solAssert(sizeOnStack == 1 || sizeOnStack == 2, ""); m_localVariables[&_declaration].push_back(unsigned(m_asm->deposit()) - _offsetToCurrent); } @@ -146,6 +148,7 @@ void CompilerContext::removeVariablesAboveStackHeight(unsigned _stackHeight) for (auto _var: m_localVariables) { solAssert(!_var.second.empty(), ""); + solAssert(_var.second.back() <= stackHeight(), ""); if (_var.second.back() >= _stackHeight) toRemove.push_back(_var.first); } @@ -153,6 +156,11 @@ void CompilerContext::removeVariablesAboveStackHeight(unsigned _stackHeight) removeVariable(*_var); } +unsigned CompilerContext::numberOfLocalVariables() const +{ + return m_localVariables.size(); +} + eth::Assembly const& CompilerContext::compiledContract(const ContractDefinition& _contract) const { auto ret = m_compiledContracts.find(&_contract); diff --git a/libsolidity/codegen/CompilerContext.h b/libsolidity/codegen/CompilerContext.h index f3934615e..3f357821c 100644 --- a/libsolidity/codegen/CompilerContext.h +++ b/libsolidity/codegen/CompilerContext.h @@ -74,6 +74,8 @@ public: void removeVariable(Declaration const& _declaration); /// Removes all local variables currently allocated above _stackHeight. void removeVariablesAboveStackHeight(unsigned _stackHeight); + /// Returns the number of currently allocated local variables. + unsigned numberOfLocalVariables() const; void setCompiledContracts(std::map const& _contracts) { m_compiledContracts = _contracts; } eth::Assembly const& compiledContract(ContractDefinition const& _contract) const; diff --git a/libsolidity/codegen/ContractCompiler.cpp b/libsolidity/codegen/ContractCompiler.cpp index 207e0af72..93f698bc6 100644 --- a/libsolidity/codegen/ContractCompiler.cpp +++ b/libsolidity/codegen/ContractCompiler.cpp @@ -498,8 +498,12 @@ bool ContractCompiler::visit(FunctionDefinition const& _function) m_context.adjustStackOffset(-(int)c_returnValuesSize); /// The constructor and the fallback function doesn't to jump out. - if (!_function.isConstructor() && !_function.isFallback()) - m_context.appendJump(eth::AssemblyItem::JumpType::OutOfFunction); + if (!_function.isConstructor()) + { + solAssert(m_context.numberOfLocalVariables() == 0, ""); + if (!_function.isFallback()) + m_context.appendJump(eth::AssemblyItem::JumpType::OutOfFunction); + } return false; } @@ -999,10 +1003,10 @@ eth::AssemblyPointer ContractCompiler::cloneRuntime() const void ContractCompiler::popScopedVariables(ASTNode const* _node) { unsigned blockHeight = m_scopeStackHeight.at(m_modifierDepth).at(_node); + m_context.removeVariablesAboveStackHeight(blockHeight); solAssert(m_context.stackHeight() >= blockHeight, ""); unsigned stackDiff = m_context.stackHeight() - blockHeight; CompilerUtils(m_context).popStackSlots(stackDiff); - m_context.removeVariablesAboveStackHeight(blockHeight); m_scopeStackHeight[m_modifierDepth].erase(_node); if (m_scopeStackHeight[m_modifierDepth].size() == 0) m_scopeStackHeight.erase(m_modifierDepth); diff --git a/test/libsolidity/SolidityEndToEndTest.cpp b/test/libsolidity/SolidityEndToEndTest.cpp index ae92810dd..d1466ea8a 100644 --- a/test/libsolidity/SolidityEndToEndTest.cpp +++ b/test/libsolidity/SolidityEndToEndTest.cpp @@ -802,7 +802,7 @@ BOOST_AUTO_TEST_CASE(nested_for_loop_multiple_local_vars) } } if (x > 30) { - return 42; + return 42; uint b = 0xcafe; } } @@ -9494,7 +9494,9 @@ BOOST_AUTO_TEST_CASE(break_in_modifier) } } function f() run { - x++; + uint k = x; + uint t = k + 1; + x = t; } } )"; @@ -9516,7 +9518,9 @@ BOOST_AUTO_TEST_CASE(continue_in_modifier) } } function f() run { - x++; + uint k = x; + uint t = k + 1; + x = t; } } )"; @@ -9538,7 +9542,9 @@ BOOST_AUTO_TEST_CASE(return_in_modifier) } } function f() run { - x++; + uint k = x; + uint t = k + 1; + x = t; } } )"; @@ -9560,7 +9566,9 @@ BOOST_AUTO_TEST_CASE(stacked_return_with_modifiers) } } function f() run { - x++; + uint k = x; + uint t = k + 1; + x = t; } } )"; From a18a475b1a6ba1ebcf235f322dec0e7ff3d3dcb0 Mon Sep 17 00:00:00 2001 From: Leonardo Alt Date: Tue, 10 Jul 2018 19:08:05 +0200 Subject: [PATCH 5/5] Change test from public to external --- test/libsolidity/SolidityEndToEndTest.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/libsolidity/SolidityEndToEndTest.cpp b/test/libsolidity/SolidityEndToEndTest.cpp index d1466ea8a..29fc808b4 100644 --- a/test/libsolidity/SolidityEndToEndTest.cpp +++ b/test/libsolidity/SolidityEndToEndTest.cpp @@ -527,7 +527,7 @@ BOOST_AUTO_TEST_CASE(array_multiple_local_vars) { char const* sourceCode = R"( contract test { - function f(uint256[] seq) public pure returns (uint256) { + function f(uint256[] seq) external pure returns (uint256) { uint i = 0; uint sum = 0; while (i < seq.length)