Merge pull request #3527 from ethereum/warnLooseAsm

Warn about using loose assembly.

Changelog.md

@@ -5,6 +5,7 @@ Features:
  * Code Generator: Assert that ``k != 0`` for ``mulmod(a, b, k)`` and ``addmod(a, b, k)`` as experimental 0.5.0 feature.
  * Code Generator: Do not retain any gas in calls (except if EVM version is set to homestead).
  * Code Generator: Use ``STATICCALL`` opcode for calling ``view`` and ``pure`` functions as experimenal 0.5.0 feature.
+ * Inline Assembly: Enforce strict mode as experimental 0.5.0 feature.
  * Interface: Provide ability to select target EVM version (homestead or byzantium, with byzantium being the default).
  * Standard JSON: Reject badly formatted invalid JSON inputs.
  * Type Checker: Disallow uninitialized storage pointers as experimental 0.5.0 feature.

libsolidity/analysis/ReferencesResolver.cpp

@@ -280,7 +280,8 @@ bool ReferencesResolver::visit(InlineAssembly const& _inlineAssembly)
 	// Will be re-generated later with correct information
 	// We use the latest EVM version because we will re-run it anyway.
 	assembly::AsmAnalysisInfo analysisInfo;
-	assembly::AsmAnalyzer(analysisInfo, errorsIgnored, EVMVersion(), assembly::AsmFlavour::Loose, resolver).analyze(_inlineAssembly.operations());
+	boost::optional<Error::Type> errorTypeForLoose = m_experimental050Mode ? Error::Type::SyntaxError : Error::Type::Warning;
+	assembly::AsmAnalyzer(analysisInfo, errorsIgnored, EVMVersion(), errorTypeForLoose, assembly::AsmFlavour::Loose, resolver).analyze(_inlineAssembly.operations());
 	return false;
 }
 

libsolidity/analysis/TypeChecker.cpp

@@ -894,10 +894,15 @@ bool TypeChecker::visit(InlineAssembly const& _inlineAssembly)
 	};
 	solAssert(!_inlineAssembly.annotation().analysisInfo, "");
 	_inlineAssembly.annotation().analysisInfo = make_shared<assembly::AsmAnalysisInfo>();
+	boost::optional<Error::Type> errorTypeForLoose =
+		m_scope->sourceUnit().annotation().experimentalFeatures.count(ExperimentalFeature::V050) ?
+		Error::Type::SyntaxError :
+		Error::Type::Warning;
 	assembly::AsmAnalyzer analyzer(
 		*_inlineAssembly.annotation().analysisInfo,
 		m_errorReporter,
 		m_evmVersion,
+		errorTypeForLoose,
 		assembly::AsmFlavour::Loose,
 		identifierAccess
 	);

libsolidity/codegen/CompilerContext.cpp

@@ -330,6 +330,7 @@ void CompilerContext::appendInlineAssembly(
 			analysisInfo,
 			errorReporter,
 			m_evmVersion,
+			boost::none,
 			assembly::AsmFlavour::Strict,
 			identifierAccess.resolve
 		).analyze(*parserResult);

libsolidity/inlineasm/AsmAnalysis.cpp

@@ -54,7 +54,10 @@ bool AsmAnalyzer::analyze(Block const& _block)
 
 bool AsmAnalyzer::operator()(Label const& _label)
 {
-	solAssert(m_flavour == AsmFlavour::Loose, "");
+	checkLooseFeature(
+		_label.location,
+		"The use of labels is deprecated. Please use \"if\", \"switch\", \"for\" or function calls instead."
+	);
 	m_info.stackHeightInfo[&_label] = m_stackHeight;
 	warnOnInstructions(solidity::Instruction::JUMPDEST, _label.location);
 	return true;
@@ -62,7 +65,10 @@ bool AsmAnalyzer::operator()(Label const& _label)
 
 bool AsmAnalyzer::operator()(assembly::Instruction const& _instruction)
 {
-	solAssert(m_flavour == AsmFlavour::Loose, "");
+	checkLooseFeature(
+		_instruction.location,
+		"The use of non-functional instructions is deprecated. Please use functional notation instead."
+	);
 	auto const& info = instructionInfo(_instruction.instruction);
 	m_stackHeight += info.ret - info.args;
 	m_info.stackHeightInfo[&_instruction] = m_stackHeight;
@@ -170,18 +176,31 @@ bool AsmAnalyzer::operator()(FunctionalInstruction const& _instr)
 
 bool AsmAnalyzer::operator()(assembly::ExpressionStatement const& _statement)
 {
-	size_t initialStackHeight = m_stackHeight;
+	int initialStackHeight = m_stackHeight;
 	bool success = boost::apply_visitor(*this, _statement.expression);
-	if (m_flavour != AsmFlavour::Loose)
+	if (m_stackHeight != initialStackHeight && (m_flavour != AsmFlavour::Loose || m_errorTypeForLoose))
-		if (!expectDeposit(0, initialStackHeight, _statement.location))
+	{
+		Error::Type errorType = m_flavour == AsmFlavour::Loose ? *m_errorTypeForLoose : Error::Type::TypeError;
+		string msg =
+			"Top-level expressions are not supposed to return values (this expression returns " +
+			boost::lexical_cast<string>(m_stackHeight - initialStackHeight) +
+			" value" +
+			(m_stackHeight - initialStackHeight == 1 ? "" : "s") +
+			"). Use ``pop()`` or assign them.";
+		m_errorReporter.error(errorType, _statement.location, msg);
+		if (errorType != Error::Type::Warning)
 			success = false;
+	}
 	m_info.stackHeightInfo[&_statement] = m_stackHeight;
 	return success;
 }
 
 bool AsmAnalyzer::operator()(assembly::StackAssignment const& _assignment)
 {
-	solAssert(m_flavour == AsmFlavour::Loose, "");
+	checkLooseFeature(
+		_assignment.location,
+		"The use of stack assignment is deprecated. Please use assignment in functional notation instead."
+	);
 	bool success = checkAssignment(_assignment.variableName, size_t(-1));
 	m_info.stackHeightInfo[&_assignment] = m_stackHeight;
 	return success;
@@ -577,10 +596,22 @@ void AsmAnalyzer::warnOnInstructions(solidity::Instruction _instr, SourceLocatio
 		);
 
 	if (_instr == solidity::Instruction::JUMP || _instr == solidity::Instruction::JUMPI || _instr == solidity::Instruction::JUMPDEST)
-		m_errorReporter.warning(
+	{
+		solAssert(m_flavour == AsmFlavour::Loose, "");
+		m_errorReporter.error(
+			m_errorTypeForLoose ? *m_errorTypeForLoose : Error::Type::Warning,
 			_location,
 			"Jump instructions and labels are low-level EVM features that can lead to "
 			"incorrect stack access. Because of that they are discouraged. "
 			"Please consider using \"switch\", \"if\" or \"for\" statements instead."
 		);
+	}
+}
+
+void AsmAnalyzer::checkLooseFeature(SourceLocation const& _location, string const& _description)
+{
+	if (m_flavour != AsmFlavour::Loose)
+		solAssert(false, _description);
+	else if (m_errorTypeForLoose)
+		m_errorReporter.error(*m_errorTypeForLoose, _location, _description);
 }

libsolidity/inlineasm/AsmAnalysis.h

@@ -30,6 +30,7 @@
 #include <libsolidity/inlineasm/AsmDataForward.h>
 
 #include <boost/variant.hpp>
+#include <boost/optional.hpp>
 
 #include <functional>
 #include <memory>
@@ -56,9 +57,17 @@ public:
 		AsmAnalysisInfo& _analysisInfo,
 		ErrorReporter& _errorReporter,
 		EVMVersion _evmVersion,
+		boost::optional<Error::Type> _errorTypeForLoose,
 		AsmFlavour _flavour = AsmFlavour::Loose,
 		julia::ExternalIdentifierAccess::Resolver const& _resolver = julia::ExternalIdentifierAccess::Resolver()
-	): m_resolver(_resolver), m_info(_analysisInfo), m_errorReporter(_errorReporter), m_evmVersion(_evmVersion), m_flavour(_flavour) {}
+	):
+		m_resolver(_resolver),
+		m_info(_analysisInfo),
+		m_errorReporter(_errorReporter),
+		m_evmVersion(_evmVersion),
+		m_flavour(_flavour),
+		m_errorTypeForLoose(_errorTypeForLoose)
+	{}
 
 	bool analyze(assembly::Block const& _block);
 
@@ -91,6 +100,11 @@ private:
 	void expectValidType(std::string const& type, SourceLocation const& _location);
 	void warnOnInstructions(solidity::Instruction _instr, SourceLocation const& _location);
 
+	/// Depending on @a m_flavour and @a m_errorTypeForLoose, throws an internal compiler
+	/// exception (if the flavour is not Loose), reports an error/warning
+	/// (if m_errorTypeForLoose is set) or does nothing.
+	void checkLooseFeature(SourceLocation const& _location, std::string const& _description);
+
 	int m_stackHeight = 0;
 	julia::ExternalIdentifierAccess::Resolver m_resolver;
 	Scope* m_currentScope = nullptr;
@@ -101,6 +115,7 @@ private:
 	ErrorReporter& m_errorReporter;
 	EVMVersion m_evmVersion;
 	AsmFlavour m_flavour = AsmFlavour::Loose;
+	boost::optional<Error::Type> m_errorTypeForLoose;
 };
 
 }

libsolidity/interface/AssemblyStack.cpp

@@ -91,7 +91,7 @@ bool AssemblyStack::analyze(assembly::Block const& _block, Scanner const* _scann
 bool AssemblyStack::analyzeParsed()
 {
 	m_analysisInfo = make_shared<assembly::AsmAnalysisInfo>();
-	assembly::AsmAnalyzer analyzer(*m_analysisInfo, m_errorReporter, m_evmVersion, languageToAsmFlavour(m_language));
+	assembly::AsmAnalyzer analyzer(*m_analysisInfo, m_errorReporter, m_evmVersion, boost::none, languageToAsmFlavour(m_language));
 	m_analysisSuccessful = analyzer.analyze(*m_parserResult);
 	return m_analysisSuccessful;
 }

test/libjulia/Common.cpp

@@ -67,6 +67,7 @@ pair<shared_ptr<Block>, shared_ptr<assembly::AsmAnalysisInfo>> dev::julia::test:
 			*analysisInfo,
 			errorReporter,
 			dev::test::Options::get().evmVersion(),
+			boost::none,
 			flavour
 		);
 		if (analyzer.analyze(*parserResult))

test/libjulia/Parser.cpp

@@ -60,6 +60,7 @@ bool parse(string const& _source, ErrorReporter& errorReporter)
 				analysisInfo,
 				errorReporter,
 				dev::test::Options::get().evmVersion(),
+				boost::none,
 				assembly::AsmFlavour::IULIA
 			)).analyze(*parserResult);
 		}

test/libsolidity/SolidityNameAndTypeResolution.cpp

@@ -5946,10 +5946,11 @@ BOOST_AUTO_TEST_CASE(inline_assembly_unbalanced_negative_stack)
 BOOST_AUTO_TEST_CASE(inline_assembly_unbalanced_two_stack_load)
 {
 	char const* text = R"(
+		pragma experimental "v0.5.0";
 		contract c {
 			uint8 x;
 			function f() public {
-				assembly { x pop }
+				assembly { pop(x) }
 			}
 		}
 	)";
@@ -5959,6 +5960,7 @@ BOOST_AUTO_TEST_CASE(inline_assembly_unbalanced_two_stack_load)
 BOOST_AUTO_TEST_CASE(inline_assembly_in_modifier)
 {
 	char const* text = R"(
+		pragma experimental "v0.5.0";
 		contract test {
 			modifier m {
 				uint a = 1;
@@ -5967,7 +5969,7 @@ BOOST_AUTO_TEST_CASE(inline_assembly_in_modifier)
 				}
 				_;
 			}
-			function f() m {
+			function f() public m {
 			}
 		}
 	)";
@@ -5977,6 +5979,7 @@ BOOST_AUTO_TEST_CASE(inline_assembly_in_modifier)
 BOOST_AUTO_TEST_CASE(inline_assembly_storage)
 {
 	char const* text = R"(
+		pragma experimental "v0.5.0";
 		contract test {
 			uint x = 1;
 			function f() public {
@@ -5992,6 +5995,7 @@ BOOST_AUTO_TEST_CASE(inline_assembly_storage)
 BOOST_AUTO_TEST_CASE(inline_assembly_storage_in_modifiers)
 {
 	char const* text = R"(
+		pragma experimental "v0.5.0";
 		contract test {
 			uint x = 1;
 			modifier m {
@@ -6000,7 +6004,7 @@ BOOST_AUTO_TEST_CASE(inline_assembly_storage_in_modifiers)
 				}
 				_;
 			}
-			function f() m {
+			function f() public m {
 			}
 		}
 	)";
@@ -6010,6 +6014,7 @@ BOOST_AUTO_TEST_CASE(inline_assembly_storage_in_modifiers)
 BOOST_AUTO_TEST_CASE(inline_assembly_constant_assign)
 {
 	char const* text = R"(
+		pragma experimental "v0.5.0";
 		contract test {
 			uint constant x = 1;
 			function f() public {
@@ -6025,6 +6030,7 @@ BOOST_AUTO_TEST_CASE(inline_assembly_constant_assign)
 BOOST_AUTO_TEST_CASE(inline_assembly_constant_access)
 {
 	char const* text = R"(
+		pragma experimental "v0.5.0";
 		contract test {
 			uint constant x = 1;
 			function f() public {
@@ -6040,6 +6046,7 @@ BOOST_AUTO_TEST_CASE(inline_assembly_constant_access)
 BOOST_AUTO_TEST_CASE(inline_assembly_local_variable_access_out_of_functions)
 {
 	char const* text = R"(
+		pragma experimental "v0.5.0";
 		contract test {
 			function f() public {
 				uint a;
@@ -6055,6 +6062,7 @@ BOOST_AUTO_TEST_CASE(inline_assembly_local_variable_access_out_of_functions)
 BOOST_AUTO_TEST_CASE(inline_assembly_local_variable_access_out_of_functions_storage_ptr)
 {
 	char const* text = R"(
+		pragma experimental "v0.5.0";
 		contract test {
 			uint[] r;
 			function f() public {
@@ -6071,6 +6079,7 @@ BOOST_AUTO_TEST_CASE(inline_assembly_local_variable_access_out_of_functions_stor
 BOOST_AUTO_TEST_CASE(inline_assembly_storage_variable_access_out_of_functions)
 {
 	char const* text = R"(
+		pragma experimental "v0.5.0";
 		contract test {
 			uint a;
 			function f() pure public {
@@ -6101,6 +6110,7 @@ BOOST_AUTO_TEST_CASE(inline_assembly_constant_variable_via_offset)
 BOOST_AUTO_TEST_CASE(inline_assembly_calldata_variables)
 {
 	char const* text = R"(
+		pragma experimental "v0.5.0";
 		contract C {
 			function f(bytes bytesAsCalldata) external {
 				assembly {
@@ -6112,6 +6122,182 @@ BOOST_AUTO_TEST_CASE(inline_assembly_calldata_variables)
 	CHECK_ERROR(text, TypeError, "Call data elements cannot be accessed directly.");
 }
 
+BOOST_AUTO_TEST_CASE(inline_assembly_050_literals_on_stack)
+{
+	char const* text = R"(
+		pragma experimental "v0.5.0";
+		contract C {
+			function f() pure public {
+				assembly {
+					1
+				}
+			}
+		}
+	)";
+	CHECK_ALLOW_MULTI(text, (std::vector<std::pair<Error::Type, std::string>>{
+		{Error::Type::SyntaxError, "are not supposed to return"},
+		{Error::Type::DeclarationError, "Unbalanced stack"},
+	}));
+}
+
+BOOST_AUTO_TEST_CASE(inline_assembly_literals_on_stack)
+{
+	char const* text = R"(
+		contract C {
+			function f() pure public {
+				assembly {
+					1
+				}
+			}
+		}
+	)";
+	CHECK_ALLOW_MULTI(text, (std::vector<std::pair<Error::Type, std::string>>{
+		{Error::Type::Warning, "are not supposed to return"},
+		{Error::Type::DeclarationError, "Unbalanced stack"},
+	}));
+}
+
+BOOST_AUTO_TEST_CASE(inline_assembly_050_bare_instructions)
+{
+	char const* text = R"(
+		pragma experimental "v0.5.0";
+		contract C {
+			function f() view public {
+				assembly {
+					address
+					pop
+				}
+			}
+		}
+	)";
+	CHECK_ALLOW_MULTI(text, (std::vector<std::pair<Error::Type, std::string>>{
+		{Error::Type::SyntaxError, "The use of non-functional"},
+		{Error::Type::SyntaxError, "The use of non-functional"}
+	}));
+}
+
+BOOST_AUTO_TEST_CASE(inline_assembly_bare_instructions)
+{
+	char const* text = R"(
+		contract C {
+			function f() view public {
+				assembly {
+					address
+					pop
+				}
+			}
+		}
+	)";
+	CHECK_ALLOW_MULTI(text, (std::vector<std::pair<Error::Type, std::string>>{
+		{Error::Type::Warning, "The use of non-functional"},
+		{Error::Type::Warning, "The use of non-functional"}
+	}));
+}
+
+BOOST_AUTO_TEST_CASE(inline_assembly_050_labels)
+{
+	char const* text = R"(
+		pragma experimental "v0.5.0";
+		contract C {
+			function f() pure public {
+				assembly {
+					label:
+				}
+			}
+		}
+	)";
+	CHECK_ALLOW_MULTI(text, (std::vector<std::pair<Error::Type, std::string>>{
+		{Error::Type::SyntaxError, "Jump instructions and labels are low-level"},
+		{Error::Type::SyntaxError, "The use of labels is deprecated"}
+	}));
+}
+
+BOOST_AUTO_TEST_CASE(inline_assembly_labels)
+{
+	char const* text = R"(
+		contract C {
+			function f() pure public {
+				assembly {
+					label:
+				}
+			}
+		}
+	)";
+	CHECK_ALLOW_MULTI(text, (std::vector<std::pair<Error::Type, std::string>>{
+		{Error::Type::Warning, "Jump instructions and labels are low-level"},
+		{Error::Type::Warning, "The use of labels is deprecated"}
+	}));
+}
+
+BOOST_AUTO_TEST_CASE(inline_assembly_050_jump)
+{
+	char const* text = R"(
+		pragma experimental "v0.5.0";
+		contract C {
+			function f() pure public {
+				assembly {
+					jump(2)
+				}
+			}
+		}
+	)";
+	CHECK_ALLOW_MULTI(text, (std::vector<std::pair<Error::Type, std::string>>{
+		{Error::Type::SyntaxError, "Jump instructions and labels are low-level"}
+	}));
+}
+
+BOOST_AUTO_TEST_CASE(inline_assembly_jump)
+{
+	char const* text = R"(
+		contract C {
+			function f() pure public {
+				assembly {
+					jump(2)
+				}
+			}
+		}
+	)";
+	CHECK_ALLOW_MULTI(text, (std::vector<std::pair<Error::Type, std::string>>{
+		{Error::Type::TypeError, "Function declared as pure"},
+		{Error::Type::Warning, "Jump instructions and labels are low-level"}
+	}));
+}
+
+BOOST_AUTO_TEST_CASE(inline_assembly_050_leave_items_on_stack)
+{
+	char const* text = R"(
+		pragma experimental "v0.5.0";
+		contract C {
+			function f() pure public {
+				assembly {
+					mload(0)
+				}
+			}
+		}
+	)";
+	CHECK_ALLOW_MULTI(text, (std::vector<std::pair<Error::Type, std::string>>{
+		{Error::Type::SyntaxError, "are not supposed to return"},
+		{Error::Type::DeclarationError, "Unbalanced stack"},
+	}));
+}
+
+BOOST_AUTO_TEST_CASE(inline_assembly_leave_items_on_stack)
+{
+	char const* text = R"(
+		contract C {
+			function f() pure public {
+				assembly {
+					mload(0)
+				}
+			}
+		}
+	)";
+	CHECK_ALLOW_MULTI(text, (std::vector<std::pair<Error::Type, std::string>>{
+		{Error::Type::Warning, "are not supposed to return"},
+		{Error::Type::DeclarationError, "Unbalanced stack"},
+	}));
+}
+
 BOOST_AUTO_TEST_CASE(invalid_mobile_type)
 {
 	char const* text = R"(
@@ -7157,6 +7343,7 @@ BOOST_AUTO_TEST_CASE(returndatacopy_as_variable)
 	)";
 	vector<pair<Error::Type, std::string>> expectations(vector<pair<Error::Type, std::string>>{
 		{Error::Type::Warning, "Variable is shadowed in inline assembly by an instruction of the same name"},
+		{Error::Type::Warning, "The use of non-functional instructions is deprecated."},
 		{Error::Type::DeclarationError, "Unbalanced stack"}
 	});
 	if (!dev::test::Options::get().evmVersion().supportsReturndata())
@@ -7172,7 +7359,8 @@ BOOST_AUTO_TEST_CASE(create2_as_variable)
 	CHECK_ALLOW_MULTI(text, (std::vector<std::pair<Error::Type, std::string>>{
 		{Error::Type::Warning, "Variable is shadowed in inline assembly by an instruction of the same name"},
 		{Error::Type::Warning, "The \"create2\" instruction is not supported by the VM version"},
-		{Error::Type::DeclarationError, "Unbalanced stack"}
+		{Error::Type::DeclarationError, "Unbalanced stack"},
+		{Error::Type::Warning, "not supposed to return values"}
 	}));
 }