From fd1662d1c49776232b491f1871391ad1cd90309a Mon Sep 17 00:00:00 2001
From: chriseth <chris@ethereum.org>
Date: Thu, 15 Feb 2018 15:18:09 +0100
Subject: [PATCH 1/3] Warn about using loose inline assembly features as
experimental 0.5.0 feature.
---
libsolidity/analysis/ReferencesResolver.cpp | 3 +-
libsolidity/analysis/TypeChecker.cpp | 5 +++
libsolidity/codegen/CompilerContext.cpp | 1 +
libsolidity/inlineasm/AsmAnalysis.cpp | 45 +++++++++++++++++----
libsolidity/inlineasm/AsmAnalysis.h | 17 +++++++-
libsolidity/interface/AssemblyStack.cpp | 2 +-
test/libjulia/Common.cpp | 1 +
test/libjulia/Parser.cpp | 1 +
8 files changed, 65 insertions(+), 10 deletions(-)
diff --git a/libsolidity/analysis/ReferencesResolver.cpp b/libsolidity/analysis/ReferencesResolver.cpp
index 296a39c23..f91eaf6eb 100644
--- a/libsolidity/analysis/ReferencesResolver.cpp
+++ b/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;
}
diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp
index 1748b518c..5d28e776e 100644
--- a/libsolidity/analysis/TypeChecker.cpp
+++ b/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
);
diff --git a/libsolidity/codegen/CompilerContext.cpp b/libsolidity/codegen/CompilerContext.cpp
index ebf0213ad..0bf88267f 100644
--- a/libsolidity/codegen/CompilerContext.cpp
+++ b/libsolidity/codegen/CompilerContext.cpp
@@ -330,6 +330,7 @@ void CompilerContext::appendInlineAssembly(
analysisInfo,
errorReporter,
m_evmVersion,
+ boost::none,
assembly::AsmFlavour::Strict,
identifierAccess.resolve
).analyze(*parserResult);
diff --git a/libsolidity/inlineasm/AsmAnalysis.cpp b/libsolidity/inlineasm/AsmAnalysis.cpp
index a7f764a5b..abf7ddf26 100644
--- a/libsolidity/inlineasm/AsmAnalysis.cpp
+++ b/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 (!expectDeposit(0, initialStackHeight, _statement.location))
+ if (m_stackHeight != initialStackHeight && (m_flavour != AsmFlavour::Loose || m_errorTypeForLoose))
+ {
+ 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);
}
diff --git a/libsolidity/inlineasm/AsmAnalysis.h b/libsolidity/inlineasm/AsmAnalysis.h
index 867711c71..8d2a71f06 100644
--- a/libsolidity/inlineasm/AsmAnalysis.h
+++ b/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;
};
}
diff --git a/libsolidity/interface/AssemblyStack.cpp b/libsolidity/interface/AssemblyStack.cpp
index 7a9fffbf2..7f97336b0 100644
--- a/libsolidity/interface/AssemblyStack.cpp
+++ b/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;
}
diff --git a/test/libjulia/Common.cpp b/test/libjulia/Common.cpp
index d8cd20b61..41f5d3206 100644
--- a/test/libjulia/Common.cpp
+++ b/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))
diff --git a/test/libjulia/Parser.cpp b/test/libjulia/Parser.cpp
index 6476c4d45..df905dd68 100644
--- a/test/libjulia/Parser.cpp
+++ b/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);
}
From 84d711fd1d38d36eb6ef2d3c118694a9ad06938e Mon Sep 17 00:00:00 2001
From: chriseth <chris@ethereum.org>
Date: Thu, 15 Feb 2018 17:25:54 +0100
Subject: [PATCH 2/3] Tests for warnings/errors for loose assembly.
---
.../SolidityNameAndTypeResolution.cpp | 196 +++++++++++++++++-
1 file changed, 192 insertions(+), 4 deletions(-)
diff --git a/test/libsolidity/SolidityNameAndTypeResolution.cpp b/test/libsolidity/SolidityNameAndTypeResolution.cpp
index 87d629735..71d3e93eb 100644
--- a/test/libsolidity/SolidityNameAndTypeResolution.cpp
+++ b/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"}
}));
}
From af7c64c1fc56221736320582dd9644470f157fa3 Mon Sep 17 00:00:00 2001
From: chriseth <chris@ethereum.org>
Date: Thu, 15 Feb 2018 15:47:50 +0100
Subject: [PATCH 3/3] Changelog entry.
---
Changelog.md | 1 +
1 file changed, 1 insertion(+)
diff --git a/Changelog.md b/Changelog.md
index c643c943d..b4ba572f7 100644
--- a/Changelog.md
+++ b/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.