Merge pull request #2445 from ethereum/inlineasm-warn-jump

Warn on JUMP/JUMPI in inline assembly

Changelog.md

@@ -12,6 +12,7 @@ Features:
  * Inline Assembly: introduce ``keccak256`` as an opcode. ``sha3`` is still a valid alias.
  * Inline Assembly: ``for`` and ``switch`` statements.
  * Inline Assembly: function definitions and function calls.
+ * Inline Assembly: Warn when using ``jump``s.
  * Type Checker: Warn about copies in storage that might overwrite unexpectedly.
  * Code Generator: Added the Whiskers template system.
  * Remove obsolete Why3 output.

libsolidity/inlineasm/AsmAnalysis.cpp

@@ -65,7 +65,7 @@ bool AsmAnalyzer::operator()(assembly::Instruction const& _instruction)
 	auto const& info = instructionInfo(_instruction.instruction);
 	m_stackHeight += info.ret - info.args;
 	m_info.stackHeightInfo[&_instruction] = m_stackHeight;
-	warnOnFutureInstruction(_instruction.instruction, _instruction.location);
+	warnOnInstructions(_instruction.instruction, _instruction.location);
 	return true;
 }
 
@@ -150,7 +150,6 @@ bool AsmAnalyzer::operator()(FunctionalInstruction const& _instr)
 	if (!(*this)(_instr.instruction))
 		success = false;
 	m_info.stackHeightInfo[&_instr] = m_stackHeight;
-	warnOnFutureInstruction(_instr.instruction.instruction, _instr.location);
 	return success;
 }
 
@@ -470,7 +469,7 @@ void AsmAnalyzer::expectValidType(string const& type, SourceLocation const& _loc
 		);
 }
 
-void AsmAnalyzer::warnOnFutureInstruction(solidity::Instruction _instr, SourceLocation const& _location)
+void AsmAnalyzer::warnOnInstructions(solidity::Instruction _instr, SourceLocation const& _location)
 {
 	static set<solidity::Instruction> futureInstructions{
 		solidity::Instruction::CREATE2,
@@ -486,4 +485,12 @@ void AsmAnalyzer::warnOnFutureInstruction(solidity::Instruction _instr, SourceLo
 			+ "\" instruction is only available after " +
 			"the Metropolis hard fork. Before that it acts as an invalid instruction."
 		);
+
+	if (_instr == solidity::Instruction::JUMP || _instr == solidity::Instruction::JUMPI)
+		m_errorReporter.warning(
+			_location,
+			"Jump instructions are low-level EVM features that can lead to "
+			"incorrect stack access. Because of that they are discouraged. "
+			"Please consider using \"switch\" or \"for\" statements instead."
+		);
 }

libsolidity/inlineasm/AsmAnalysis.h

@@ -85,7 +85,7 @@ private:
 
 	Scope& scope(assembly::Block const* _block);
 	void expectValidType(std::string const& type, SourceLocation const& _location);
-	void warnOnFutureInstruction(solidity::Instruction _instr, SourceLocation const& _location);
+	void warnOnInstructions(solidity::Instruction _instr, SourceLocation const& _location);
 
 	int m_stackHeight = 0;
 	julia::ExternalIdentifierAccess::Resolver m_resolver;

test/libsolidity/InlineAssembly.cpp

@@ -67,23 +67,19 @@ boost::optional<Error> parseAndReturnFirstError(
 		BOOST_FAIL("Fatal error leaked.");
 		success = false;
 	}
+	shared_ptr<Error const> error;
+	for (auto const& e: stack.errors())
+	{
+		if (_allowWarnings && e->type() == Error::Type::Warning)
+			continue;
+		if (error)
+			BOOST_FAIL("Found more than one error.");
+		error = e;
+	}
 	if (!success)
-	{
-		BOOST_REQUIRE_EQUAL(stack.errors().size(), 1);
-		return *stack.errors().front();
-	}
-	else
-	{
-		// If success is true, there might still be an error in the assembly stage.
-		if (_allowWarnings && Error::containsOnlyWarnings(stack.errors()))
-			return {};
-		else if (!stack.errors().empty())
-		{
-			if (!_allowWarnings)
-				BOOST_CHECK_EQUAL(stack.errors().size(), 1);
-			return *stack.errors().front();
-		}
-	}
+		BOOST_REQUIRE(error);
+	if (error)
+		return *error;
 	return {};
 }
 
@@ -111,29 +107,35 @@ Error expectError(std::string const& _source, bool _assemble, bool _allowWarning
 	return *error;
 }
 
-void parsePrintCompare(string const& _source)
+void parsePrintCompare(string const& _source, bool _canWarn = false)
 {
 	AssemblyStack stack;
 	BOOST_REQUIRE(stack.parseAndAnalyze("", _source));
-	BOOST_REQUIRE(stack.errors().empty());
+	if (_canWarn)
+		BOOST_REQUIRE(Error::containsOnlyWarnings(stack.errors()));
+	else
+		BOOST_REQUIRE(stack.errors().empty());
 	BOOST_CHECK_EQUAL(stack.print(), _source);
 }
 
 }
 
-#define CHECK_ERROR(text, assemble, typ, substring) \
+#define CHECK_ERROR(text, assemble, typ, substring, warnings) \
 do \
 { \
-	Error err = expectError((text), (assemble), false); \
+	Error err = expectError((text), (assemble), warnings); \
 	BOOST_CHECK(err.type() == (Error::Type::typ)); \
 	BOOST_CHECK(searchErrorMessage(err, (substring))); \
 } while(0)
 
 #define CHECK_PARSE_ERROR(text, type, substring) \
-CHECK_ERROR(text, false, type, substring)
+CHECK_ERROR(text, false, type, substring, false)
+
+#define CHECK_PARSE_WARNING(text, type, substring) \
+CHECK_ERROR(text, false, type, substring, false)
 
 #define CHECK_ASSEMBLE_ERROR(text, type, substring) \
-CHECK_ERROR(text, true, type, substring)
+CHECK_ERROR(text, true, type, substring, false)
 
 
 
@@ -411,7 +413,7 @@ BOOST_AUTO_TEST_CASE(print_functional)
 
 BOOST_AUTO_TEST_CASE(print_label)
 {
-	parsePrintCompare("{\n    loop:\n    jump(loop)\n}");
+	parsePrintCompare("{\n    loop:\n    jump(loop)\n}", true);
 }
 
 BOOST_AUTO_TEST_CASE(print_assignments)
@@ -515,7 +517,7 @@ BOOST_AUTO_TEST_CASE(imbalanced_stack)
 
 BOOST_AUTO_TEST_CASE(error_tag)
 {
-	CHECK_ASSEMBLE_ERROR("{ jump(invalidJumpLabel) }", DeclarationError, "Identifier not found");
+	CHECK_ERROR("{ jump(invalidJumpLabel) }", true, DeclarationError, "Identifier not found", true);
 }
 
 BOOST_AUTO_TEST_CASE(designated_invalid_instruction)
@@ -630,6 +632,14 @@ BOOST_AUTO_TEST_CASE(create2)
 	BOOST_CHECK(successAssemble("{ pop(create2(10, 0x123, 32, 64)) }"));
 }
 
+BOOST_AUTO_TEST_CASE(jump_warning)
+{
+	CHECK_PARSE_WARNING("{ 1 jump }", Warning, "Jump instructions");
+	CHECK_PARSE_WARNING("{ 1 2 jumpi }", Warning, "Jump instructions");
+	CHECK_PARSE_WARNING("{ a: jump(a) }", Warning, "Jump instructions");
+	CHECK_PARSE_WARNING("{ a: jumpi(a, 2) }", Warning, "Jump instructions");
+}
+
 BOOST_AUTO_TEST_SUITE_END()
 
 BOOST_AUTO_TEST_SUITE_END()