Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add RETURNDATACOPY and RETURNDATASIZE to assembly (and LLL) #2275

Merged
merged 8 commits into from
Jun 13, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions Changelog.md
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
### 0.4.12 (unreleased)

Features:
* Assembler: renamed ``SHA3`` to `KECCAK256``.
* AST: export all attributes to Json format
* Assembly: renamed ``SHA3`` to `KECCAK256``.
* Assembly: Add ``RETURNDATASIZE`` and ``RETURNDATACOPY`` (EIP211) instructions.
* AST: export all attributes to JSON format.
* Inline Assembly: Present proper error message when not supplying enough arguments to a functional
instruction.
* Inline Assembly: introduce ``keccak256`` as an opcode. ``sha3`` is still a valid alias.

Bugfixes:
* Unused variable warnings no longer issued for variables used inside inline assembly
* Unused variable warnings no longer issued for variables used inside inline assembly.

### 0.4.11 (2017-05-03)

Expand Down
4 changes: 4 additions & 0 deletions docs/assembly.rst
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,10 @@ In the grammar, opcodes are represented as pre-defined identifiers.
+-------------------------+------+-----------------------------------------------------------------+
| extcodecopy(a, t, f, s) | `-` | like codecopy(t, f, s) but take code at address a |
+-------------------------+------+-----------------------------------------------------------------+
| returndatasize | | size of the last returndata |
+-------------------------+------+-----------------------------------------------------------------+
| returndatacopy(t, f, s) | `*` | copy s bytes from returndata at position f to mem at position t |
+-------------------------+------+-----------------------------------------------------------------+
| create(v, p, s) | | create new contract with code mem[p..(p+s)) and send v wei |
| | | and return the new address |
+-------------------------+------+-----------------------------------------------------------------+
Expand Down
1 change: 1 addition & 0 deletions libevmasm/GasMeter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ GasMeter::GasConsumption GasMeter::estimateMax(AssemblyItem const& _item, bool _
break;
case Instruction::CALLDATACOPY:
case Instruction::CODECOPY:
case Instruction::RETURNDATACOPY:
gas += memoryGas(0, -2);
gas += wordGas(GasCosts::copyGas, m_state->relativeStackElement(-2));
break;
Expand Down
4 changes: 4 additions & 0 deletions libevmasm/Instruction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ const std::map<std::string, Instruction> dev::solidity::c_instructions =
{ "GASPRICE", Instruction::GASPRICE },
{ "EXTCODESIZE", Instruction::EXTCODESIZE },
{ "EXTCODECOPY", Instruction::EXTCODECOPY },
{ "RETURNDATASIZE", Instruction::RETURNDATASIZE },
{ "RETURNDATACOPY", Instruction::RETURNDATACOPY },
{ "BLOCKHASH", Instruction::BLOCKHASH },
{ "COINBASE", Instruction::COINBASE },
{ "TIMESTAMP", Instruction::TIMESTAMP },
Expand Down Expand Up @@ -203,6 +205,8 @@ static const std::map<Instruction, InstructionInfo> c_instructionInfo =
{ Instruction::GASPRICE, { "GASPRICE", 0, 0, 1, false, Tier::Base } },
{ Instruction::EXTCODESIZE, { "EXTCODESIZE", 0, 1, 1, false, Tier::ExtCode } },
{ Instruction::EXTCODECOPY, { "EXTCODECOPY", 0, 4, 0, true, Tier::ExtCode } },
{ Instruction::RETURNDATASIZE, {"RETURNDATASIZE", 0, 0, 1, false, Tier::Base } },
{ Instruction::RETURNDATACOPY, {"RETURNDATACOPY", 0, 3, 0, true, Tier::VeryLow } },
{ Instruction::BLOCKHASH, { "BLOCKHASH", 0, 1, 1, false, Tier::Ext } },
{ Instruction::COINBASE, { "COINBASE", 0, 0, 1, false, Tier::Base } },
{ Instruction::TIMESTAMP, { "TIMESTAMP", 0, 0, 1, false, Tier::Base } },
Expand Down
2 changes: 2 additions & 0 deletions libevmasm/Instruction.h
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ enum class Instruction: uint8_t
GASPRICE, ///< get price of gas in current environment
EXTCODESIZE, ///< get external code size (from another contract)
EXTCODECOPY, ///< copy external code (from another contract)
RETURNDATASIZE, ///< get size of the last return data
RETURNDATACOPY, ///< copy last return data to memory

BLOCKHASH = 0x40, ///< get hash of most recent complete block
COINBASE, ///< get the block's coinbase address
Expand Down
3 changes: 3 additions & 0 deletions libevmasm/SemanticInformation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,8 @@ bool SemanticInformation::isDeterministic(AssemblyItem const& _item)
case Instruction::MSIZE: // depends on previous writes and reads, not only on content
case Instruction::BALANCE: // depends on previous calls
case Instruction::EXTCODESIZE:
case Instruction::RETURNDATACOPY: // depends on previous calls
case Instruction::RETURNDATASIZE:
return false;
default:
return true;
Expand All @@ -156,6 +158,7 @@ bool SemanticInformation::invalidatesMemory(Instruction _instruction)
case Instruction::CALLDATACOPY:
case Instruction::CODECOPY:
case Instruction::EXTCODECOPY:
case Instruction::RETURNDATACOPY:
case Instruction::MSTORE:
case Instruction::MSTORE8:
case Instruction::CALL:
Expand Down
22 changes: 22 additions & 0 deletions libsolidity/analysis/NameAndTypeResolver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
#include <libsolidity/analysis/TypeChecker.h>
#include <libsolidity/interface/ErrorReporter.h>

#include <boost/algorithm/string.hpp>

using namespace std;

namespace dev
Expand Down Expand Up @@ -232,6 +234,26 @@ vector<Declaration const*> NameAndTypeResolver::cleanedDeclarations(
return uniqueFunctions;
}

void NameAndTypeResolver::warnVariablesNamedLikeInstructions()
{
for (auto const& instruction: c_instructions)
{
string const instructionName{boost::algorithm::to_lower_copy(instruction.first)};
auto declarations = nameFromCurrentScope(instructionName);
for (Declaration const* const declaration: declarations)
{
solAssert(!!declaration, "");
if (dynamic_cast<MagicVariableDeclaration const* const>(declaration))
// Don't warn the user for what the user did not.
continue;
m_errorReporter.warning(
declaration->location(),
"Variable is shadowed in inline assembly by an instruction of the same name"
);
}
}
}

bool NameAndTypeResolver::resolveNamesAndTypesInternal(ASTNode& _node, bool _resolveInsideCode)
{
if (ContractDefinition* contract = dynamic_cast<ContractDefinition*>(&_node))
Expand Down
3 changes: 3 additions & 0 deletions libsolidity/analysis/NameAndTypeResolver.h
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,9 @@ class NameAndTypeResolver: private boost::noncopyable
std::vector<Declaration const*> const& _declarations
);

/// Generate and store warnings about variables that are named like instructions.
void warnVariablesNamedLikeInstructions();

private:
/// Internal version of @a resolveNamesAndTypes (called from there) throws exceptions on fatal errors.
bool resolveNamesAndTypesInternal(ASTNode& _node, bool _resolveInsideCode = true);
Expand Down
2 changes: 2 additions & 0 deletions libsolidity/analysis/ReferencesResolver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,8 @@ void ReferencesResolver::endVisit(ArrayTypeName const& _typeName)

bool ReferencesResolver::visit(InlineAssembly const& _inlineAssembly)
{
m_resolver.warnVariablesNamedLikeInstructions();

// Errors created in this stage are completely ignored because we do not yet know
// the type and size of external identifiers, which would result in false errors.
// The only purpose of this step is to fill the inline assembly annotation with
Expand Down
20 changes: 19 additions & 1 deletion libsolidity/inlineasm/AsmAnalysis.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +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);
return true;
}

Expand Down Expand Up @@ -149,6 +150,7 @@ 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;
}

Expand Down Expand Up @@ -431,7 +433,6 @@ Scope& AsmAnalyzer::scope(Block const* _block)
solAssert(scopePtr, "Scope requested but not present.");
return *scopePtr;
}

void AsmAnalyzer::expectValidType(string const& type, SourceLocation const& _location)
{
if (!m_julia)
Expand All @@ -443,3 +444,20 @@ void AsmAnalyzer::expectValidType(string const& type, SourceLocation const& _loc
"\"" + type + "\" is not a valid type (user defined types are not yet supported)."
);
}

void AsmAnalyzer::warnOnFutureInstruction(solidity::Instruction _instr, SourceLocation const& _location)
{
switch (_instr)
{
case solidity::Instruction::RETURNDATASIZE:
case solidity::Instruction::RETURNDATACOPY:
m_errorReporter.warning(
_location,
"The RETURNDATASIZE/RETURNDATACOPY instructions are only available after "
"the Metropolis hard fork. Before that they act as an invalid instruction."
);
break;
default:
break;
}
}
1 change: 1 addition & 0 deletions libsolidity/inlineasm/AsmAnalysis.h
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ class AsmAnalyzer: public boost::static_visitor<bool>

Scope& scope(assembly::Block const* _block);
void expectValidType(std::string const& type, SourceLocation const& _location);
void warnOnFutureInstruction(solidity::Instruction _instr, SourceLocation const& _location);

int m_stackHeight = 0;
julia::ExternalIdentifierAccess::Resolver m_resolver;
Expand Down
20 changes: 20 additions & 0 deletions test/libsolidity/InlineAssembly.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -542,6 +542,26 @@ BOOST_AUTO_TEST_CASE(keccak256)
BOOST_CHECK(successAssemble("{ pop(sha3(0, 0)) }"));
}

BOOST_AUTO_TEST_CASE(returndatasize)
{
BOOST_CHECK(successAssemble("{ let r := returndatasize }"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also potentially this should be in "functional notation" to be in line with returndatacopy / revert.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an exception that for an opcode x, if x() is valid, you can omit the parentheses. We could make that more strict (it is more strict in julia already).

Copy link
Member

@axic axic May 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both are valid, it is only about consistency:

  • do not test for these
  • test with one notation only (but use the same for these "opcode tests")
  • test with both notations (this has its own merits, given the code paths in the parser/analyzer are different)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should test random styles on random instructions to catch different kinds of errors. I don't think we get coverage efficiently if we tested the same set of styles for all instructions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to catch as many bugs as possible we should test for both notations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But, if no other instruction is tested for "both styles", maybe now is a good time to do that for at least one instruction.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the opposite, practically no "functional instructions" are tested in the assembler section.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add is already tested for both styles in functional, but it has two arguments: not zero or three.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have to look at the SolidityInlineAssembler/Analysis suite. The others don't assemble.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, and at this point, it's easier to add both styles.

}

BOOST_AUTO_TEST_CASE(returndatasize_functional)
{
BOOST_CHECK(successAssemble("{ let r := returndatasize() }"));
}

BOOST_AUTO_TEST_CASE(returndatacopy)
{
BOOST_CHECK(successAssemble("{ 64 32 0 returndatacopy }"));
}

BOOST_AUTO_TEST_CASE(returndatacopy_functional)
{
BOOST_CHECK(successAssemble("{ returndatacopy(0, 32, 64) }"));
}

BOOST_AUTO_TEST_SUITE_END()

BOOST_AUTO_TEST_SUITE_END()
Expand Down
25 changes: 23 additions & 2 deletions test/libsolidity/SolidityNameAndTypeResolution.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -193,11 +193,16 @@ CHECK_ERROR_OR_WARNING(text, type, substring, false, false)
#define CHECK_ERROR_ALLOW_MULTI(text, type, substring) \
CHECK_ERROR_OR_WARNING(text, type, substring, false, true)

// [checkWarning(text, type, substring)] asserts that the compilation down to typechecking
// emits a warning of type [type] and with a message containing [substring].
// [checkWarning(text, substring)] asserts that the compilation down to typechecking
// emits a warning and with a message containing [substring].
#define CHECK_WARNING(text, substring) \
CHECK_ERROR_OR_WARNING(text, Warning, substring, true, false)

// [checkWarningAllowMulti(text, substring)] aserts that the compilation down to typechecking
// emits a warning and with a message containing [substring].
#define CHECK_WARNING_ALLOW_MULTI(text, substring) \
CHECK_ERROR_OR_WARNING(text, Warning, substring, true, true)

// [checkSuccess(text)] asserts that the compilation down to typechecking succeeds.
#define CHECK_SUCCESS(text) do { BOOST_CHECK(success((text))); } while(0)

Expand Down Expand Up @@ -5780,6 +5785,22 @@ BOOST_AUTO_TEST_CASE(no_unused_inline_asm)
CHECK_SUCCESS_NO_WARNINGS(text);
}

BOOST_AUTO_TEST_CASE(returndatacopy_as_variable)
{
char const* text = R"(
contract c { function f() { uint returndatasize; assembly { returndatasize }}}
)";
CHECK_WARNING_ALLOW_MULTI(text, "Variable is shadowed in inline assembly by an instruction of the same name");
}

BOOST_AUTO_TEST_CASE(shadowing_warning_can_be_removed)
{
char const* text = R"(
contract C {function f() {assembly {}}}
)";
CHECK_SUCCESS_NO_WARNINGS(text);
}



BOOST_AUTO_TEST_SUITE_END()
Expand Down