Minimum viable type validation by gumb0 · Pull Request #403 · wasmx/fizzy

gumb0 · 2020-07-01T10:59:04Z

No description provided.

codecov · 2020-07-02T09:59:11Z

Codecov Report

Merging #403 into master will decrease coverage by 0.16%.
The diff coverage is 96.05%.

@@            Coverage Diff             @@
##           master     #403      +/-   ##
==========================================
- Coverage   99.35%   99.18%   -0.17%     
==========================================
  Files          49       50       +1     
  Lines       13242    13801     +559     
==========================================
+ Hits        13156    13689     +533     
- Misses         86      112      +26

lib/fizzy/instructions.cpp

gumb0 · 2020-07-02T10:32:32Z

lib/fizzy/parser.cpp


-    const auto [code, pos2] =
-        parse_expr(pos1, end, static_cast<uint32_t>(local_count), func_idx, module);
+    // TODO: Clarify in spec what happens if count of locals and arguments exceed uint32_t::max()


I'm moving this assertion from parse_expr to parse_code, because now parser_expr doesn't get local_count and iterates locals_vec anyway on each local access, to find the type.

lib/fizzy/parser_expr.cpp

gumb0 · 2020-07-02T17:27:39Z

lib/fizzy/instructions.hpp


+struct InstructionType
+{
+    std::initializer_list<ValType> inputs;


I'm still not sure whether it's valid to use initializer_list like this.

Declaring constexpr initalizer_list object works only if it's static local or global, but doesn't compile if it's auto https://godbolt.org/z/iWUg6B

The discussions that I found are not very clear to me, but seem to come to the conclusion that standard doesn't guarantee it to be valid.
https://stackoverflow.com/questions/16063123/is-it-legal-to-declare-a-constexpr-initializer-list-object
https://stackoverflow.com/questions/27496004/why-isnt-stdinitializer-list-defined-as-a-literal-type

Just learned that in C++20 std::vector can be constexpr, that would help a lot.

Found relatively simple solution with a custom struct instead of initializer_list, pushing it as a separate commit for now.

Just learned that in C++20 std::vector can be constexpr, that would help a lot.

I don't think it would help. It is constexpr because you can use new and delete in constexpr evaluation (consteval), but for initialization dynamic memory allocation would be needed, so compilation would probably fail. See also constinit.

lib/fizzy/instructions.hpp

gumb0 · 2020-07-07T09:39:52Z

Rebased and squashed span-related and constexpr_array commits.

lib/fizzy/instructions.hpp

lib/fizzy/module.hpp

chfast

Looks good, but let's keep in mind the performance regression.

gumb0 · 2020-07-10T13:23:36Z

I'm getting somewhat less of regression on my laptop

fizzy/parse/blake2b_mean                                +0.2133         +0.2153            41            50            41            50
fizzy/parse/ecpairing_mean                              +0.3124         +0.3131          2191          2876          2190          2876
fizzy/parse/keccak256_mean                              +0.2555         +0.2565            73            92            73            92
fizzy/parse/memset_mean                                 +0.1513         +0.1517            10            12            10            12
fizzy/parse/mul256_opt0_mean                            +0.2082         +0.2086            13            15            13            15
fizzy/parse/sha1_mean                                   +0.2586         +0.2587            65            82            65            82
fizzy/parse/sha256_mean                                 +0.2389         +0.2391           112           138           112           138
fizzy/parse/micro/eli_interpreter_mean                  +0.1198         +0.1205             7             8             7             8
fizzy/parse/micro/factorial_mean                        +0.1289         +0.1289             2             2             2             2
fizzy/parse/micro/fibonacci_mean                        +0.1435         +0.1435             2             2             2             2
fizzy/parse/micro/host_adler32_mean                     +0.0668         +0.0669             3             3             3             3
fizzy/parse/micro/spinner_mean                          +0.0921         +0.0922             2             2             2             2

axic · 2020-07-14T10:28:11Z

lib/fizzy/instructions.cpp

+    /* block               = 0x02 */ {{}, {}},
+    /* loop                = 0x03 */ {{}, {}},
+    /* if_                 = 0x04 */ {{ValType::i32}, {}},
+    /* else_               = 0x05 */ {{}, {}},


Why have these initialisers {{}, {}}, when empty opcodes can go with {} ?

It can, I just found it somewhat more explicit to show both input and output empty. Would you prefer to make it {}?

It doesn't really make any difference.

axic · 2020-07-14T18:45:23Z

lib/fizzy/parser_expr.cpp

        {
-            std::optional<ValType> type;
-            std::tie(type, pos) = parse_blocktype(pos, end);
+            std::optional<ValType> block_type;


Why do you need this refactoring btw?

type would shadow the new variable in the outer scope (which I called types originally, but @chfast suggested to rename)

axic · 2020-07-16T16:03:57Z

lib/fizzy/parser_expr.cpp

        auto& frame = control_stack.top();
-        const auto& metrics = metrics_table[opcode];
+        const auto metrics = metrics_table[opcode];
+        const auto type = type_table[opcode];


Are these two becoming a copy now and not a reference?

This is a mistake I think, will make them references.

Co-authored-by: Andrei Maiboroda <andrei@ethereum.org>

gumb0 force-pushed the validate-types-no-block-br branch 3 times, most recently from 83be14a to c22d88e Compare July 2, 2020 09:46

gumb0 force-pushed the validate-types-no-block-br branch from c22d88e to e29870a Compare July 2, 2020 10:26

gumb0 commented Jul 2, 2020

View reviewed changes

lib/fizzy/instructions.cpp Outdated Show resolved Hide resolved

gumb0 commented Jul 2, 2020

View reviewed changes

gumb0 force-pushed the validate-types-no-block-br branch from e29870a to 4d3e011 Compare July 2, 2020 11:19

gumb0 commented Jul 2, 2020

View reviewed changes

lib/fizzy/parser_expr.cpp Outdated Show resolved Hide resolved

gumb0 force-pushed the validate-types-no-block-br branch 6 times, most recently from 159edef to 01b99cc Compare July 2, 2020 16:27

gumb0 marked this pull request as ready for review July 2, 2020 16:51

gumb0 commented Jul 2, 2020

View reviewed changes

gumb0 force-pushed the validate-types-no-block-br branch from 48ce911 to aba60a0 Compare July 3, 2020 13:41

gumb0 commented Jul 3, 2020

View reviewed changes

lib/fizzy/instructions.hpp Outdated Show resolved Hide resolved

gumb0 requested review from axic and chfast July 3, 2020 13:50

chfast force-pushed the validate-types-no-block-br branch from aba60a0 to baa2d62 Compare July 6, 2020 08:41

chfast mentioned this pull request Jul 6, 2020

Span utility #404

Merged

chfast force-pushed the validate-types-no-block-br branch from baa2d62 to a6f705c Compare July 6, 2020 09:41

gumb0 force-pushed the validate-types-no-block-br branch 4 times, most recently from f6be8d7 to 1a1b8be Compare July 7, 2020 09:37

chfast reviewed Jul 7, 2020

View reviewed changes

lib/fizzy/instructions.hpp Outdated Show resolved Hide resolved

lib/fizzy/module.hpp Show resolved Hide resolved

gumb0 requested a review from chfast July 10, 2020 11:11

chfast approved these changes Jul 10, 2020

View reviewed changes

gumb0 mentioned this pull request Jul 13, 2020

Remove instruction metrics (except max align) #414

Merged

axic reviewed Jul 14, 2020

View reviewed changes

axic force-pushed the validate-types-no-block-br branch 3 times, most recently from 7d2c6c3 to 3adfb02 Compare July 16, 2020 16:02

axic reviewed Jul 16, 2020

View reviewed changes

gumb0 force-pushed the validate-types-no-block-br branch from 3adfb02 to 0d30b02 Compare July 16, 2020 16:23

gumb0 requested a review from axic July 16, 2020 16:32

axic and others added 15 commits July 17, 2020 12:05

Prefix "type" variables with kind in parse_expr

79f0a41

Co-authored-by: Andrei Maiboroda <andrei@ethereum.org>

Use LocalIdx and GlobalIdx extensively

5efd41d

Co-authored-by: Andrei Maiboroda <andrei@ethereum.org>

Add instruction type table

2dd49a1

Validate types of non-polymorphic instructions and call instructions

bba27ca

Validate stack types for local instructions

73a1c24

Validate stack types for global instructions

37f6540

Remove ControlFrame::stack_height

fdda407

test: fix unit tests with type mismatch

8eba0d9

test: fix unit tests with incorrect local count

5dc5ba9

ci: Update spectest expectations

8bfcf78

test: type validation tests for calls

ef5b852

test: type validation tests for non-polymorphic instructions

85be095

test: type validation for local instructions

925c82c

test: type validation for global instructions

61b58c0

test: validate number of arguments for select

7beb186

gumb0 force-pushed the validate-types-no-block-br branch from 0d30b02 to 7beb186 Compare July 17, 2020 10:06

gumb0 merged commit ee1f21d into master Jul 17, 2020

gumb0 deleted the validate-types-no-block-br branch July 17, 2020 10:13

Conversation

gumb0 commented Jul 1, 2020

Uh oh!

codecov bot commented Jul 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gumb0 Jul 3, 2020 • 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!

Uh oh!

gumb0 commented Jul 7, 2020

Uh oh!

Uh oh!

Uh oh!

chfast left a comment

Choose a reason for hiding this comment

Uh oh!

gumb0 commented Jul 10, 2020

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Jul 2, 2020 •

edited

Loading

gumb0 Jul 3, 2020 •

edited

Loading