Merge pull request #1224 from ethereum/inline-assembly-stack-warning

Issue inline assembly stack warning if not balanced

Changelog.md

@@ -4,6 +4,7 @@ Features:
 
  * Inline assembly: support both ``suicide`` and ``selfdestruct`` opcodes
    (note: ``suicide`` is deprecated).
+ * Inline assembly: issue warning if stack is not balanced after block.
  * Include ``keccak256()`` as an alias to ``sha3()``.
  * Support shifting constant numbers.
 

libsolidity/analysis/TypeChecker.cpp

@@ -718,11 +718,10 @@ bool TypeChecker::visit(VariableDeclarationStatement const& _statement)
 		{
 			if (ref->dataStoredIn(DataLocation::Storage))
 			{
-				auto err = make_shared<Error>(Error::Type::Warning);
-				*err <<
-					errinfo_sourceLocation(varDecl.location()) <<
-					errinfo_comment("Uninitialized storage pointer. Did you mean '<type> memory " + varDecl.name() + "'?");
-				m_errors.push_back(err);
+				warning(
+					varDecl.location(),
+					"Uninitialized storage pointer. Did you mean '<type> memory " + varDecl.name() + "'?"
+				);
 			}
 		}
 		varDecl.accept(*this);

libsolidity/codegen/ContractCompiler.cpp

@@ -575,7 +575,7 @@ bool ContractCompiler::visit(InlineAssembly const& _inlineAssembly)
 			return true;
 		}
 	);
-	solAssert(errors.empty(), "Code generation for inline assembly with errors requested.");
+	solAssert(Error::containsOnlyWarnings(errors), "Code generation for inline assembly with errors requested.");
 	m_context.setStackOffset(startStackHeight);
 	return false;
 }

libsolidity/inlineasm/AsmCodeGen.cpp

@@ -23,6 +23,7 @@
 #include <libsolidity/inlineasm/AsmCodeGen.h>
 #include <memory>
 #include <functional>
+#include <libdevcore/CommonIO.h>
 #include <libevmasm/Assembly.h>
 #include <libevmasm/SourceLocation.h>
 #include <libevmasm/Instruction.h>
@@ -213,10 +214,31 @@ public:
 	void operator()(assembly::Block const& _block)
 	{
 		size_t numVariables = m_state.variables.size();
+		int deposit = m_state.assembly.deposit();
 		std::for_each(_block.statements.begin(), _block.statements.end(), boost::apply_visitor(*this));
-		// pop variables
-		// we deliberately do not check stack height
+		deposit = m_state.assembly.deposit() - deposit;
+
 		m_state.assembly.setSourceLocation(_block.location);
+
+		// issue warnings for stack height discrepancies
+		if (deposit < 0)
+		{
+			m_state.addError(
+				Error::Type::Warning,
+				"Inline assembly block is not balanced. It takes " + toString(-deposit) + " item(s) from the stack.",
+				_block.location
+			);
+		}
+		else if (deposit > 0)
+		{
+			m_state.addError(
+				Error::Type::Warning,
+				"Inline assembly block is not balanced. It leaves " + toString(deposit) + " item(s) on the stack.",
+				_block.location
+			);
+		}
+
+		// pop variables
 		while (m_state.variables.size() > numVariables)
 		{
 			m_state.assembly.append(solidity::Instruction::POP);

test/libsolidity/InlineAssembly.cpp

@@ -51,7 +51,7 @@ bool successParse(std::string const& _source, bool _assemble = false)
 		if (_assemble)
 		{
 			stack.assemble();
-			if (!stack.errors().empty())
+			if (!stack.errors().empty() && !Error::containsOnlyWarnings(stack.errors()))
 				return false;
 		}
 	}

test/libsolidity/SolidityNameAndTypeResolution.cpp

@@ -4079,6 +4079,34 @@ BOOST_AUTO_TEST_CASE(shift_constant_right_excessive_rvalue)
 	BOOST_CHECK(expectError(text, false) == Error::Type::TypeError);
 }
 
+BOOST_AUTO_TEST_CASE(inline_assembly_unbalanced_positive_stack)
+{
+	char const* text = R"(
+		contract test {
+			function f() {
+				assembly {
+					1
+				}
+			}
+		}
+	)";
+	BOOST_CHECK(expectError(text, true) == Error::Type::Warning);
+}
+
+BOOST_AUTO_TEST_CASE(inline_assembly_unbalanced_negative_stack)
+{
+	char const* text = R"(
+		contract test {
+			function f() {
+				assembly {
+					pop
+				}
+			}
+		}
+	)";
+	BOOST_CHECK(expectError(text, true) == Error::Type::Warning);
+}
+
 BOOST_AUTO_TEST_SUITE_END()
 
 }