Test to make sure that LLLC is compiling all valid opcodes correctly by jwasinger · Pull Request #3002 · argotorg/solidity

jwasinger · 2017-10-02T03:50:04Z

AFAIK SHA3 is the only opcode not currently implemented in LLLC.

axic · 2017-10-02T07:56:51Z

test/liblll/Parser.cpp

+  };
+
+  std::vector<std::string> opcodes_lll {
+    "{ (STOP) (RETURN 0x0 0x1) }",


return is dead code here after stop

axic · 2017-10-02T07:57:10Z

test/liblll/Parser.cpp

+    "{ (XOR 0x00 0x00) (RETURN 0x0 0x0) }",
+    "{ (NOT 0x00) (RETURN 0x0 0x0) }",
+    "{ (BYTE 0 0x8050201008040201) (RETURN 0x0 0x0) }",
+    //"{ (SHA3 0x00 0x00) (RETURN 0x0 0x0) }", //SHA3 opcode not implemented


It is called keccak256 and not sha3.

axic · 2017-10-02T07:57:41Z

test/liblll/Parser.cpp

+    "{ 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff (return 0x0 0x0)}",
+    "{ 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff (return 0x0 0x0)}",
+    "{ 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff (return 0x0 0x0)}",
+    "{ (DUP1 0xff) (return 0x0 0x0)}",


What is the point of the 0xff parameter for DUPs and SWAPs? They do not take parameters and this case (for some reason) gets compiled as pushes.

Well to be honest, DUP and SWAP make no sense for the instructional form in LLL.

All these are also available as inline assembly:

(asm 1 2 3 DUP2 SWAP3)

Fixed this in #3005.

axic · 2017-10-02T08:01:31Z

test/liblll/Parser.cpp

+    "{ (EXTCODECOPY 0x1000000000000000000000000000000000000010 0 0 20) (RETURN 0x0 0x0) }",
+    "{ (RETURNDATASIZE) (RETURN 0x0 0x0) }",
+    "{ (RETURNDATACOPY 0x0 0x0 0x0) (RETURN 0x0 0x0) }",
+    "{ 0x00 (POP 0x00) (RETURN 0x0 0x0) }",


This seems to be a a push 0, push 0, pop, but not as an instruction as the rest of them.

I'd argue having POP as a functional instruction is useless given every stray stack argument is popped anyway.

e.g.

(ORIGIN)

and

(POP (ORIGIN))

has the same stack result.

axic · 2017-10-02T08:04:20Z

test/liblll/Parser.cpp

+    "{ (DELEGATECALL 100000 0x0f572e5295c57f15886f9b263e2f6d2d6c7b5ec6 0 0 0 0) (return 0x0 0x0) }",
+    "{ (STATICCALL 10000 0x0f572e5295c57f15886f9b263e2f6d2d6c7b5ec6 0 0 0 0) (return 0x0 0x0) }",
+    "{ (REVERT 0x0 0x0) (return 0x0 0x0)}",
+    "{ (SELFDESTRUCT (ORIGIN)) }"


If all the others used a PUSH for the argument, perhaps do the same here?

axic · 2017-10-02T08:04:59Z

Some of the instructions are lowercase while others are uppercase. Also having return after every one of them seems not very useful. Any reason for these?

axic · 2017-10-02T08:15:58Z

test/liblll/Parser.cpp

+
+    if (!errors.empty()) {
+      BOOST_FAIL(opcodes_lll[i]);
+    }


For this part above you could copy the code from test/liblll/ExecutionFramework.h.

axic · 2017-10-02T08:17:23Z

test/liblll/Parser.cpp

+    std::stringstream ss;
+
+    for (size_t i = 0; i < code.size(); i++) {
+      ss << boost::format("%02x") % (int)code[i];


Could just use toHex(code) to achieve the same.

axic · 2017-10-02T08:35:08Z

test/liblll/Parser.cpp

+    "{ (PC) (RETURN 0x0 0x0) }",
+    "{ (MSIZE) (RETURN 0x0 0x0) }",
+    "{ (GAS) (RETURN 0x0 0x0) }",
+    "{ (JUMPDEST) (RETURN 0x0 0x0) }",


This seems pointless too, should disable it in the compiler.

Fixed this in #3005.

axic · 2017-10-02T08:36:15Z

test/liblll/Parser.cpp

+    "{ (RETURN 0x0 0x0) }",
+    "{ (DELEGATECALL 100000 0x0f572e5295c57f15886f9b263e2f6d2d6c7b5ec6 0 0 0 0) (return 0x0 0x0) }",
+    "{ (STATICCALL 10000 0x0f572e5295c57f15886f9b263e2f6d2d6c7b5ec6 0 0 0 0) (return 0x0 0x0) }",
+    "{ (REVERT 0x0 0x0) (return 0x0 0x0)}",


INVALID is not tested for.

axic · 2017-10-02T09:57:24Z

test/liblll/Parser.cpp

+    "{ (CALLDATASIZE) (RETURN 0x0 0x0) }",
+    "{ (CALLDATACOPY 0x0 0x0 0x0) (RETURN 0x0 0x0) }",
+    "{ (CODESIZE) (RETURN 0x0 0x0) }",
+    "{ (CODECOPY 0x1 0x0 (CODESIZE)) [[ 0 ]] (MLOAD 0x1) (RETURN 0x0 0x0) }",


Why mix the built in notation of sstore with the rest? This code right now reads as

(CODECOPY 0x1 0x0 (CODESIZE)) (SSTORE 0 (MLOAD 0x1)) (RETURN 0x0 0x0)

is that intended?

jwasinger · 2017-10-02T19:08:09Z

Thanks for the review. Have a long day of flights today. I will address these points tomorrow.

axic

I think you should add a second set of tests for using inline assembly, where all the opcodes are available: (asm 1 2 ADD BALANCE SUB)

jwasinger · 2017-10-02T20:53:43Z

These were pulled directly from a state test I wrote: https://github.com/ethereum/tests/blob/develop/GeneralStateTests/stTransactionTest/Opcodes_TransactionInit.json .

Makes sense to re-write that as well.

Having two tests (one for good opcodes, one for unused opcodes) is sensible here.

axic · 2017-10-03T10:17:15Z

Needs to be rebased for latest LLL fixes.

jwasinger · 2017-10-03T20:00:12Z

@axic I revised the test simplifying the code, added the use of toHex, rebased off develop.

axic · 2017-10-04T10:08:28Z

Thanks @jwasinger. I think the functional tests were useful too. Not sure which one is used in test generation though.

Also all these should move to test/liblll/Compiler from Parser.

axic · 2017-10-05T08:37:52Z

I am not sure which one is used in ethereum/tests, (asm) blocks or functional instructions? If the latter, having tests for them cannot hurt.

jwasinger · 2017-10-12T05:50:01Z

I added a test for functional instructions. I think it should be ready to merge @axic

axic · 2017-10-12T10:20:47Z

test/liblll/Compiler.cpp

+    "{ (asm MSIZE) }",
+    "{ (asm GAS) }",
+    "{ (asm JUMPDEST) }",
+    "{ (asm PUSH1) }",


Hm, these actually seem to show another bug. They do insert the correct opcode, but there is no possible way to push an immediate, because having a number there will include the PUSH opcode too:

(asm PUSH1) -> 0x60

(asm PUSH1 0x42) -> 0x60 0x60 0x42

(asm 0x42) -> 0x60 0x42

I'll push a patch which removes that from assembly, but these tests should be changed to not include PUSHnn, but rather have a literal each for all widths (0x42, 0x4242, ..)

Fixed in #3070.

axic · 2017-10-13T00:51:15Z

test/liblll/Compiler.cpp

+  };
+
+  for (size_t i = 0; i < opcodes_bytecode.size(); i++) {
+    std::vector<std::string> errors;


No need for the std prefix as the namespace is imported, vector<string> should be enough.

axic · 2017-10-13T00:51:54Z

test/liblll/Compiler.cpp

+
+    if (!errors.empty()) {
+      BOOST_FAIL(opcodes_lll[i]);
+    }


I think this could be replaced with BOOST_REQUIRE_MESSAGE(errors.empty(), opcodes_lll[i]); though I'm not 100% sure.

done. and yeah, it works.

jwasinger · 2017-10-13T20:45:19Z

I also added opcodes for environmental ops like (BLOCKHASH, TIMESTAMP, etc.)

axic · 2017-10-18T10:53:19Z

Rebased, indented, sorted instructions by opcode number and changed all parameters to be 0 to aid comparing the opcode itself.

jwasinger mentioned this pull request Oct 2, 2017

add a test to travis that checks LLLC filling is correct ethereum/aleth#4519

Open

axic reviewed Oct 2, 2017

View reviewed changes

axic suggested changes Oct 2, 2017

View reviewed changes

jwasinger force-pushed the lll-opcode-test branch from 84f3065 to 8b9fbc9 Compare October 3, 2017 19:50

jwasinger force-pushed the lll-opcode-test branch from 886a98b to 040a561 Compare October 5, 2017 07:24

jwasinger force-pushed the lll-opcode-test branch 2 times, most recently from 949bbab to dcb651a Compare October 12, 2017 04:36

axic reviewed Oct 12, 2017

View reviewed changes

axic reviewed Oct 13, 2017

View reviewed changes

axic mentioned this pull request Oct 13, 2017

lll: disallow useless PUSHn in assembly #3070

Merged

jwasinger force-pushed the lll-opcode-test branch from dcb651a to a9cbef7 Compare October 13, 2017 20:44

axic force-pushed the lll-opcode-test branch from a9cbef7 to dc9cde0 Compare October 18, 2017 10:52

axic force-pushed the lll-opcode-test branch from dc9cde0 to e18dca6 Compare October 18, 2017 10:54

axic approved these changes Oct 18, 2017

View reviewed changes

Add test for each assembly opcode/instruction in LLL

9acc2ea

axic force-pushed the lll-opcode-test branch from e18dca6 to 9acc2ea Compare October 18, 2017 10:58

axic merged commit 7454a76 into argotorg:develop Oct 18, 2017

Conversation

jwasinger commented Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic commented Oct 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jwasinger commented Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

axic left a comment

Choose a reason for hiding this comment

Uh oh!

jwasinger commented Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

axic commented Oct 3, 2017

Uh oh!

jwasinger commented Oct 3, 2017

Uh oh!

axic commented Oct 4, 2017

Uh oh!

axic commented Oct 5, 2017

Uh oh!

jwasinger commented Oct 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jwasinger commented Oct 13, 2017

Uh oh!

axic commented Oct 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

jwasinger commented Oct 2, 2017 •

edited

Loading

axic Oct 2, 2017 •

edited

Loading

jwasinger commented Oct 2, 2017 •

edited

Loading

jwasinger commented Oct 2, 2017 •

edited

Loading