Skip to content

Make small push value parsing endianness independent #178

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

Merged
chfast merged 3 commits into from
Sep 17, 2019
Merged

Make small push value parsing endianness independent #178

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2142392
Fix cast to broad in scope
chfast Sep 16, 2019
408ad24
Make small push value parsing endianness independent
chfast Sep 16, 2019
95b9b04
Find single push_end value
chfast Sep 17, 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
30 changes: 12 additions & 18 deletions lib/evmone/analysis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,6 @@ struct block_analysis
explicit block_analysis(size_t index) noexcept : begin_block_index{index} {}
};

inline constexpr uint64_t load64be(const unsigned char* data) noexcept
{
return uint64_t{data[7]} | (uint64_t{data[6]} << 8) | (uint64_t{data[5]} << 16) |
(uint64_t{data[4]} << 24) | (uint64_t{data[3]} << 32) | (uint64_t{data[2]} << 40) |
(uint64_t{data[1]} << 48) | (uint64_t{data[0]} << 56);
}

code_analysis analyze(evmc_revision rev, const uint8_t* code, size_t code_size) noexcept
{
static constexpr auto stack_req_max = std::numeric_limits<int16_t>::max();
Expand Down Expand Up @@ -91,22 +84,23 @@ code_analysis analyze(evmc_revision rev, const uint8_t* code, size_t code_size)

case ANY_SMALL_PUSH:
{
const auto push_size = size_t(opcode - OP_PUSH1 + 1);
const auto push_end = code_pos + push_size;

uint8_t value_bytes[8]{};
auto insert_pos = &value_bytes[sizeof(value_bytes) - push_size];

// TODO: Consier the same endianness-specific loop as in ANY_LARGE_PUSH case.
while (code_pos < push_end && code_pos < code_end)
*insert_pos++ = *code_pos++;
instr.arg.small_push_value = load64be(value_bytes);
const auto push_size = static_cast<size_t>(opcode - OP_PUSH1) + 1;
const auto push_end = std::min(code_pos + push_size, code_end);

uint64_t value = 0;
auto insert_bit_pos = (push_size - 1) * 8;
while (code_pos < push_end)
{
value |= uint64_t{*code_pos++} << insert_bit_pos;
Copy link
Member

@gumb0 gumb0 Sep 17, 2019

Choose a reason for hiding this comment

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

Could it still be in a separate helper function maybe?

Copy link
Member Author

@chfast chfast Sep 17, 2019

Choose a reason for hiding this comment

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

The load64be was only doing bswap. I would need to extract whole case:.

Copy link
Member Author

@chfast chfast Sep 17, 2019

Choose a reason for hiding this comment

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

I pushed an example how it could look like, but it is up to 30% slower.

Copy link
Member

@gumb0 gumb0 Sep 17, 2019

Choose a reason for hiding this comment

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

Wow then let's leave it, if it's so much slower (why, can't be inlined sometimes?)

push_end change looks fine, though

Copy link
Member Author

@chfast chfast Sep 17, 2019

Choose a reason for hiding this comment

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

I think returning multiple values and using tuple and std::tie messes it up. But I'm only guessing. I'd need much more time to investigate.

insert_bit_pos -= 8;
}
instr.arg.small_push_value = value;
break;
}

case ANY_LARGE_PUSH:
{
const auto push_size = size_t(opcode - OP_PUSH1 + 1);
const auto push_size = static_cast<size_t>(opcode - OP_PUSH1) + 1;
const auto push_end = code_pos + push_size;

auto& push_value = analysis.push_values.emplace_back();
Expand Down