Interpreter refactoring #5800

chfast · 2019-10-28T09:35:46Z

No description provided.

codecov-io · 2019-10-28T10:20:41Z

Codecov Report

Merging #5800 into master will decrease coverage by <.01%.
The diff coverage is 26.53%.

@@            Coverage Diff             @@
##           master    #5800      +/-   ##
==========================================
- Coverage   64.02%   64.02%   -0.01%     
==========================================
  Files         359      359              
  Lines       30796    30772      -24     
  Branches     3417     3416       -1     
==========================================
- Hits        19718    19702      -16     
+ Misses       9855     9845      -10     
- Partials     1223     1225       +2

halfalicious · 2019-10-29T16:05:30Z

libaleth-interpreter/VM.cpp

-            uint64_t s = (uint64_t)m_SP[1];
-            owning_bytes_ref output{std::move(m_mem), b, s};
-            throwRevertInstruction(std::move(output));
+            throwRevertInstruction((uint64_t)m_SP[0], (uint64_t)m_SP[1]);


Should we static_cast here?

Would be better, yes. But I wanted to minimize number of changes.

halfalicious · 2019-10-29T16:10:37Z

libaleth-interpreter/VMCalls.cpp

-    if (size < _required)
-        BOOST_THROW_EXCEPTION(StackUnderflow() << RequirementError((bigint)_required, size));
+    if (m_stackEnd - m_SPP < _required)
+        throw EVMC_STACK_UNDERFLOW;


Why don't we include the stack information in the exception anymore?

Everything is converted into a single evmc_status_code anyway.

libaleth-interpreter/VMCalls.cpp

gumb0 · 2019-10-31T19:23:11Z

libaleth-interpreter/VM.h

-    void throwRevertInstruction(owning_bytes_ref&& _output);
-    void throwDisallowedStateChange();
-    void throwBufferOverrun(intx::uint512 const& _enfOfAccess);
+    static void throwOutOfGas();


Maybe better to make them inline or/and free functions

Or maybe there's not much sense in these helpers at all, and we can just put throws inline

This was done previously for performance reasons. I plan to rework this in separate PR with benchmarks.

I did inlining. From quick benchmarks, the inlining commit gives ~3% improvement. In total the PR gives up to 13% improvement.

gumb0 · 2019-10-31T19:24:07Z

libaleth-interpreter/VM.h

+    static void throwBadInstruction();
+    static void throwBadJumpDestination();
+    void throwBadStack(int _removed);
+    void throwRevertInstruction(uint64_t _offset, uint64_t _size);


I'd fill m_output outside of the function and make it static as others

m_output can then stay private, too

This is accessed also in EVMC's execute() method. More refactoring is possible to clean up how EVMC result is created and reported.

libaleth-interpreter/VM.cpp

Instead of using complex exception types from Aleth, throw EVMC status code in simple cases.

gumb0

many unnecessary whitespace changes

gumb0 · 2019-11-04T18:47:40Z

libaleth-interpreter/VM.cpp


    uint64_t dest = decodeJumpDest(_code, pc);

-    _pc += 1 + n * 2;               // adust inout _pc to opcode after table
+    _pc += 1 + n * 2;  // adust inout _pc to opcode after table


Suggested change

_pc += 1 + n * 2; // adust inout _pc to opcode after table

_pc += 1 + n * 2; // adjust inout _pc to opcode after table

gumb0 · 2019-11-04T18:49:10Z

libaleth-interpreter/VM.cpp

@@ -271,9 +239,9 @@ void VM::interpretCases()
        {
            ON_OP();
            if (m_rev < EVMC_CONSTANTINOPLE)
-                throwBadInstruction();
+                throw EVMC_BAD_JUMP_DESTINATION;


Suggested change

throw EVMC_BAD_JUMP_DESTINATION;

throw EVMC_UNDEFINED_INSTRUCTION;

gumb0 · 2019-11-04T18:55:03Z

libaleth-interpreter/VM.cpp

@@ -387,7 +354,7 @@ void VM::interpretCases()
        }
        NEXT

-        CASE(MSTORE)
+            CASE(MSTORE)


Indentation here... Maybe let's put semicolon after NEXT to work around this?

gumb0 · 2019-11-04T18:57:24Z

libaleth-interpreter/VM.cpp

-        CASE(BEGINDATA)
-        CASE(GETLOCAL)
-        CASE(PUTLOCAL)
+            CASE(JUMPTO) CASE(JUMPIF) CASE(JUMPV) CASE(JUMPSUB) CASE(JUMPSUBV) CASE(RETURNSUB)


😱 here semicolon won't help, so I don't know what to do other that manually overriding it

I accidentally formatted whole file. I will have to revert the formatting changes somehow, but I didn't have time to do so so far.

chfast marked this pull request as ready for review October 28, 2019 09:59

chfast requested review from gumb0 and halfalicious October 28, 2019 10:00

halfalicious reviewed Oct 29, 2019

View reviewed changes

libaleth-interpreter/VMCalls.cpp Show resolved Hide resolved

chfast force-pushed the interpreter branch from cc169fe to db6b18a Compare October 30, 2019 14:31

gumb0 reviewed Oct 31, 2019

View reviewed changes

libaleth-interpreter/VM.cpp Outdated Show resolved Hide resolved

interpreter: Simplify exception usage

266370d

Instead of using complex exception types from Aleth, throw EVMC status code in simple cases.

chfast force-pushed the interpreter branch 2 times, most recently from 6d81224 to 32b30d7 Compare November 3, 2019 18:02

chfast added 3 commits November 3, 2019 19:12

interpreter: Simplify REVERT output handling

569d610

interpreter: Simplify reporting stack violation

6848f24

interpreter: Inline throws

5b0accd

chfast force-pushed the interpreter branch from 32b30d7 to 5b0accd Compare November 3, 2019 21:02

chfast requested review from gumb0 and halfalicious November 3, 2019 21:06

gumb0 suggested changes Nov 4, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Interpreter refactoring #5800

Interpreter refactoring #5800

chfast commented Oct 28, 2019

codecov-io commented Oct 28, 2019 •

edited

Loading

halfalicious Oct 29, 2019

chfast Nov 3, 2019

halfalicious Oct 29, 2019

chfast Nov 3, 2019

gumb0 Oct 31, 2019

gumb0 Oct 31, 2019

chfast Nov 3, 2019

chfast Nov 3, 2019

gumb0 Oct 31, 2019

gumb0 Oct 31, 2019

chfast Nov 3, 2019

gumb0 left a comment

gumb0 Nov 4, 2019

gumb0 Nov 4, 2019

gumb0 Nov 4, 2019

gumb0 Nov 4, 2019

chfast Nov 12, 2019

	_pc += 1 + n * 2; // adust inout _pc to opcode after table
	_pc += 1 + n * 2; // adjust inout _pc to opcode after table

	throw EVMC_BAD_JUMP_DESTINATION;
	throw EVMC_UNDEFINED_INSTRUCTION;

Interpreter refactoring #5800

Are you sure you want to change the base?

Interpreter refactoring #5800

Conversation

chfast commented Oct 28, 2019

codecov-io commented Oct 28, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 28, 2019 •

edited

Loading