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.

Sign up for GitHub

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

Jump to bottom

Implement EIP-663 variant C #98

Closed
wants to merge 4 commits into from
Closed

Implement EIP-663 variant C #98

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a33d5f4
Upgrade EVMC for EIP-663/c support
chfast Jul 18, 2019
7bb5916
Implement EIP-663 variant C
chfast Jul 18, 2019
0632f3c
test: Add EIP-663 tests
chfast Jul 18, 2019
282ce35
Rework instructions' immediate value processing
chfast Jul 25, 2019
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
2 changes: 1 addition & 1 deletion evmc
Open in desktop
Submodule evmc updated 10 files
+7 −1 bindings/rust/evmc-declare-tests/src/lib.rs
+18 −11 bindings/rust/evmc-declare/src/lib.rs
+103 −2 bindings/rust/evmc-vm/src/container.rs
+317 −108 bindings/rust/evmc-vm/src/lib.rs
+37 −0 bindings/rust/evmc-vm/src/types.rs
+7 −2 examples/example-rust-vm/src/lib.rs
+9 −7 include/evmc/evmc.h
+5 −0 include/evmc/instructions.h
+268 −0 lib/instructions/instruction_metrics.c
+260 −0 lib/instructions/instruction_names.c
49 changes: 47 additions & 2 deletions lib/evmone/analysis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "analysis.hpp"

#include <evmc/instructions.h>
#include <tuple>

namespace evmone
{
Expand All @@ -15,6 +16,20 @@ bool is_terminator(uint8_t c) noexcept
return c == OP_JUMP || c == OP_JUMPI || c == OP_STOP || c == OP_RETURN || c == OP_REVERT ||
c == OP_SELFDESTRUCT;
}

std::pair<int, size_t> load_1byte_immediate_value(
const uint8_t* code, size_t code_size, size_t index) noexcept
{
return {(index + 1 < code_size) ? code[++index] : 0, index};
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look right, because the order of evaluation of pair constructor arguments is not defined, is it?

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 thought this is now specified in C++17, but it is not. To be fixed.

Copy link
Member

Choose a reason for hiding this comment

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

(re the need to update index conditionally: even if would update it unconditionally on the calling side and go beyond the code size, it will correctly break out of the loop, so updating here doesn't seem necessary to me)

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 though about this, but sounds risky. It would not work if we used != in the loop condition, or used an iterator instead of an index. Quite fragile to have the correctness of the implementation of this helper depend on some other loop implementation outside.

}

std::pair<int, size_t> load_2byte_immediate_value(
const uint8_t* code, size_t code_size, size_t index) noexcept
{
const auto [hi_byte, index1] = load_1byte_immediate_value(code, code_size, index);
const auto [lo_byte, index2] = load_1byte_immediate_value(code, code_size, index1);
return {(hi_byte << 8) | lo_byte, index2};
}
} // namespace

int code_analysis::find_jumpdest(int offset) const noexcept
Expand Down Expand Up @@ -84,8 +99,8 @@ code_analysis analyze(
auto& instr = jumpdest ? analysis.instrs.back() : analysis.instrs.emplace_back(fns[c]);

const auto metrics = instr_table[c];
const auto instr_stack_req = metrics.num_stack_arguments;
const auto instr_stack_change = metrics.num_stack_returned_items - instr_stack_req;
auto instr_stack_req = int{metrics.num_stack_arguments};
auto instr_stack_change = int{metrics.num_stack_returned_items - instr_stack_req};

if (metrics.gas_cost > 0) // can be -1 for undefined instruction
block->gas_cost += metrics.gas_cost;
Expand Down Expand Up @@ -124,6 +139,36 @@ code_analysis analyze(
else if (c >= OP_LOG0 && c <= OP_LOG4)
instr.arg.p.number = c - OP_LOG0;

if (rev >= EVMC_ISTANBUL)
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be better else if, so that it's not even considered if it's some other opcode.

{
if (c == OP_DUPN)
{
std::tie(instr.arg.p.number, i) = load_2byte_immediate_value(code, code_size, i);
instr_stack_req = instr.arg.p.number + 1;
instr_stack_change = 1;
}
else if (c == OP_SWAPN)
{
std::tie(instr.arg.p.number, i) = load_2byte_immediate_value(code, code_size, i);
++instr.arg.p.number;
instr_stack_req = instr.arg.p.number + 1;
instr_stack_change = 0;
}
else if (c == OP_DUPSN)
{
std::tie(instr.arg.p.number, i) = load_1byte_immediate_value(code, code_size, i);
instr_stack_req = instr.arg.p.number + 1;
instr_stack_change = 1;
}
else if (c == OP_SWAPSN)
{
std::tie(instr.arg.p.number, i) = load_1byte_immediate_value(code, code_size, i);
++instr.arg.p.number;
instr_stack_req = instr.arg.p.number + 1;
instr_stack_change = 0;
}
}

block->stack_req = std::max(block->stack_req, instr_stack_req - block->stack_change);
block->stack_change += instr_stack_change;
block->stack_max = std::max(block->stack_max, block->stack_change);
Expand Down
4 changes: 4 additions & 0 deletions lib/evmone/instructions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1284,6 +1284,10 @@ constexpr exec_fn_table create_op_table_constantinople() noexcept
constexpr exec_fn_table create_op_table_istanbul() noexcept
{
auto table = create_op_table_constantinople();
table[OP_DUPN] = op_dup;
table[OP_SWAPN] = op_swap;
table[OP_DUPSN] = op_dup;
table[OP_SWAPSN] = op_swap;
return table;
}
} // namespace
Expand Down
148 changes: 148 additions & 0 deletions test/unittests/evm_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,154 @@ TEST_F(evm, sub_and_swap)
EXPECT_EQ(result.output_data[31], 1);
}

TEST_F(evm, dupn)
{
rev = EVMC_ISTANBUL;
const auto pushes = push(1) + push(2) + push(3) + push(4);

execute(pushes + OP_DUPN + "0000" + ret_top());
EXPECT_STATUS(EVMC_SUCCESS);
EXPECT_OUTPUT_INT(4);

execute(pushes + OP_DUPN + "0002" + ret_top());
EXPECT_STATUS(EVMC_SUCCESS);
EXPECT_OUTPUT_INT(2);

execute(pushes + OP_DUPN + "0003" + ret_top());
EXPECT_STATUS(EVMC_SUCCESS);
EXPECT_OUTPUT_INT(1);

execute(pushes + OP_DUPN + "0004" + ret_top());
EXPECT_STATUS(EVMC_STACK_UNDERFLOW);

execute(pushes + OP_DUPSN + "00" + ret_top());
EXPECT_STATUS(EVMC_SUCCESS);
EXPECT_OUTPUT_INT(4);

execute(pushes + OP_DUPSN + "01" + ret_top());
EXPECT_STATUS(EVMC_SUCCESS);
EXPECT_OUTPUT_INT(3);

execute(pushes + OP_DUPSN + "03" + ret_top());
EXPECT_STATUS(EVMC_SUCCESS);
EXPECT_OUTPUT_INT(1);

execute(pushes + OP_DUPSN + "04" + ret_top());
EXPECT_STATUS(EVMC_STACK_UNDERFLOW);
}

TEST_F(evm, swapn)
{
rev = EVMC_ISTANBUL;
const auto pushes = push(1) + push(2) + push(3) + push(4);

execute(pushes + OP_SWAPN + "0000" + ret_top());
EXPECT_STATUS(EVMC_SUCCESS);
EXPECT_OUTPUT_INT(3);

execute(pushes + OP_SWAPN + "0001" + ret_top());
EXPECT_STATUS(EVMC_SUCCESS);
EXPECT_OUTPUT_INT(2);

execute(pushes + OP_SWAPN + "0002" + ret_top());
EXPECT_STATUS(EVMC_SUCCESS);
EXPECT_OUTPUT_INT(1);

execute(pushes + OP_SWAPN + "0003" + ret_top());
EXPECT_STATUS(EVMC_STACK_UNDERFLOW);

execute(pushes + OP_SWAPSN + "00" + ret_top());
EXPECT_STATUS(EVMC_SUCCESS);
EXPECT_OUTPUT_INT(3);

execute(pushes + OP_SWAPSN + "01" + ret_top());
EXPECT_STATUS(EVMC_SUCCESS);
EXPECT_OUTPUT_INT(2);

execute(pushes + OP_SWAPSN + "02" + ret_top());
EXPECT_STATUS(EVMC_SUCCESS);
EXPECT_OUTPUT_INT(1);

execute(pushes + OP_SWAPSN + "03" + ret_top());
EXPECT_STATUS(EVMC_STACK_UNDERFLOW);
}

TEST_F(evm, dupn_full_stack)
{
rev = EVMC_ISTANBUL;
auto full_stack_code = bytecode{};
for (int i = 1023; i >= 1; --i)
full_stack_code += push(i);

execute(full_stack_code + OP_POP + OP_DUPSN + "00" + ret_top());
EXPECT_STATUS(EVMC_SUCCESS);
EXPECT_OUTPUT_INT(2);

execute(full_stack_code + OP_POP + OP_DUPSN + "ff" + ret_top());
EXPECT_STATUS(EVMC_SUCCESS);
EXPECT_OUTPUT_INT(255 + 2);

execute(full_stack_code + OP_DUPSN + "01" + OP_DUPSN + "02");
EXPECT_STATUS(EVMC_STACK_OVERFLOW);

execute(full_stack_code + OP_POP + OP_DUPN + "0000" + ret_top());
EXPECT_STATUS(EVMC_SUCCESS);
EXPECT_OUTPUT_INT(2);

execute(full_stack_code + OP_POP + OP_DUPN + "0100" + ret_top());
EXPECT_STATUS(EVMC_SUCCESS);
EXPECT_OUTPUT_INT(256 + 2);

execute(full_stack_code + OP_POP + OP_DUPN + "03fd" + ret_top());
EXPECT_STATUS(EVMC_SUCCESS);
EXPECT_OUTPUT_INT(1023);

execute(full_stack_code + OP_DUPN + "03fe" + OP_SWAPSN + "00" + OP_RETURN);
Copy link
Member

Choose a reason for hiding this comment

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

why does it not use ret_top() and how does it return anything without storing in memory?

Copy link
Member Author

Choose a reason for hiding this comment

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

The ret_top() requires additional push therefore more stack space. We only check the output size which matches the last duped item.

EXPECT_STATUS(EVMC_SUCCESS);
EXPECT_EQ(result.output_size, 1023);

execute(full_stack_code + OP_DUPN + "03ff");
EXPECT_STATUS(EVMC_STACK_UNDERFLOW);

execute(full_stack_code + OP_DUPN + "03fe" + OP_DUPN + "03ff");
EXPECT_STATUS(EVMC_STACK_OVERFLOW);
}

TEST_F(evm, swapn_full_stack)
{
rev = EVMC_ISTANBUL;
auto full_stack_code = bytecode{};
for (int i = 1024; i >= 1; --i)
full_stack_code += push(i);

execute(full_stack_code + OP_POP + OP_SWAPSN + "00" + ret_top());
EXPECT_STATUS(EVMC_SUCCESS);
EXPECT_OUTPUT_INT(3);

execute(full_stack_code + OP_POP + OP_SWAPSN + "ff" + ret_top());
EXPECT_STATUS(EVMC_SUCCESS);
EXPECT_OUTPUT_INT(255 + 3);

execute(full_stack_code + OP_POP + OP_SWAPN + "0000" + ret_top());
EXPECT_STATUS(EVMC_SUCCESS);
EXPECT_OUTPUT_INT(3);

execute(full_stack_code + OP_POP + OP_SWAPN + "0100" + ret_top());
EXPECT_STATUS(EVMC_SUCCESS);
EXPECT_OUTPUT_INT(256 + 3);

execute(full_stack_code + OP_POP + OP_SWAPN + "03fd" + ret_top());
EXPECT_STATUS(EVMC_SUCCESS);
EXPECT_OUTPUT_INT(1024);

execute(full_stack_code + OP_SWAPN + "03fe" + OP_SWAPN + "0000" + OP_RETURN);
EXPECT_STATUS(EVMC_SUCCESS);
EXPECT_EQ(result.output_size, 1024);

execute(full_stack_code + OP_SWAPN + "03ff");
EXPECT_STATUS(EVMC_STACK_UNDERFLOW);
}

TEST_F(evm, memory_and_not)
{
execute(42, "600060018019815381518252800190f3");
Expand Down