Skip to content
This repository has been archived by the owner on Oct 28, 2021. It is now read-only.

Distinguish between INVALID opcode and unknown opcodes in aleth-interpreter #5666

Merged
merged 2 commits into from
Jul 15, 2019
Merged

Distinguish between INVALID opcode and unknown opcodes in aleth-interpreter #5666

merged 2 commits into from
Jul 15, 2019

Conversation

gumb0
Copy link
Member

@gumb0 gumb0 commented Jul 11, 2019 •
edited
Loading

This fixes the failure of evm.invalid test, which expects EVMC_INVALID_INSTRUCTION to be returned for INVALID (0xfe) opcode and EVMC_UNKNOWN_INSTRUCTION for all not defined opcodes.

[ RUN      ] evm.invalid
/home/andrei/dev/evmone/test/unittests/evm_test.cpp:668: Failure
Expected equality of these values:
  result.status_code
    Which is: 5
  EVMC_INVALID_INSTRUCTION
    Which is: 4
[  FAILED  ] evm.invalid (0 ms)

@gumb0
Interpreter returns EVMC_INVALID_INSTRUCTION for INVALID opcode,
b4f8978 
and EVMC_UNDEFINED_INSTRUCTION for not defined opcodes
@gumb0 gumb0 mentioned this pull request Jul 11, 2019
Open
chfast
chfast reviewed Jul 12, 2019
View reviewed changes
libaleth-interpreter/VM.cpp
@@ -1416,7 +1420,10 @@ void VM::interpretCases()
CASE(INVALID)
DEFAULT
{
throwBadInstruction();
if (m_OP == Instruction::INVALID)
Copy link
Member

Choose a reason for hiding this comment

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

Shouln't it be just

CASE(INVALID)
  throwInvalidInstruction();
DEFAULT
  throwUndefinedInstruction();

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this doesn't work on gcc/clang, because the jump table is populated with INVALID opcode for all unknown ones

aleth/libaleth-interpreter/VMConfig.h

Line 154 in 089e7dd

static const void* const jumpTable[256] = { \

Copy link
Member

Choose a reason for hiding this comment

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

Can't we change that by having &&UNDEFINED everywhere and &&INVALID in

aleth/libaleth-interpreter/VMConfig.h

Line 409 in 089e7dd

&&INVALID, \
?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's no such thing as UNDEFINED, and this enum is 8 bit-wide

aleth/libevm/Instruction.h

Line 28 in 2b11ce5

enum class Instruction : uint8_t

So we can't really add UNDEFINED unless we change the width

@codecov-io
Copy link

codecov-io commented Jul 12, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@2b11ce5). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #5666   +/-   ##
=========================================
  Coverage          ?   63.01%           
=========================================
  Files             ?      350           
  Lines             ?    29922           
  Branches          ?     3353           
=========================================
  Hits              ?    18856           
  Misses            ?     9851           
  Partials          ?     1215

@gumb0 gumb0 force-pushed the invalid-opcode-is-not-unknown branch from a8b4c0a to e042a52 Compare July 15, 2019 09:25
@gumb0 gumb0 merged commit 39a9e43 into master Jul 15, 2019
@gumb0 gumb0 deleted the invalid-opcode-is-not-unknown branch July 15, 2019 12:59
@gumb0 gumb0 mentioned this pull request Jul 17, 2019
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@chfast chfast chfast approved these changes

@halfalicious halfalicious Awaiting requested review from halfalicious

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gumb0 @codecov-io @chfast