Validate types for select instruction #413

gumb0 · 2020-07-07T15:05:58Z

Depends on #408

Some of related spec tests were already passing by accident, because the tests are not great:

https://github.com/WebAssembly/spec/blob/636b862b9c8a25ad65fb240fefd673e7f23bcdd0/test/core/select.wast#L296-L307

(assert_invalid
  (module (func $type-num-vs-num (select (i32.const 1) (i64.const 1) (i32.const 1))))
  "type mismatch"
)
(assert_invalid
  (module (func $type-num-vs-num (select (i32.const 1) (f32.const 1.0) (i32.const 1))))
  "type mismatch"
)
(assert_invalid
  (module (func $type-num-vs-num (select (i32.const 1) (f64.const 1.0) (i32.const 1))))
  "type mismatch"
)

^ These were invalid but for the wrong reason - functions are void but have result of select on stack in the end.

https://github.com/WebAssembly/spec/blob/636b862b9c8a25ad65fb240fefd673e7f23bcdd0/test/core/select.wast#L326-L333

(assert_invalid
  (module
    (func $type-3rd-operand-empty
      (i32.const 0) (i32.const 0) (select) (drop)
    )
  )
  "type mismatch"
)

^ This was also invalid for the wrong reason - select popped two values and pushed 0, so it was type mismatch for drop instruction.

(added to "To Upstream")

codecov · 2020-07-07T15:12:15Z

Codecov Report

Merging #413 into master will increase coverage by 0.18%.
The diff coverage is 99.48%.

@@            Coverage Diff             @@
##           master     #413      +/-   ##
==========================================
+ Coverage   99.37%   99.55%   +0.18%     
==========================================
  Files          49       49              
  Lines       14345    14529     +184     
==========================================
+ Hits        14255    14464     +209     
+ Misses         90       65      -25

lib/fizzy/parser_expr.cpp

chfast · 2020-07-28T09:11:00Z

lib/fizzy/parser_expr.cpp

+    return static_cast<OperandStackType>(valType);
+}
+
+inline bool type_matches(OperandStackType actual_type, ValType expected_type) noexcept


Suggested change

inline bool type_matches(OperandStackType actual_type, ValType expected_type) noexcept

inline bool operator==(OperandStackType actual_type, ValType expected_type) noexcept

?

chfast · 2020-07-28T09:11:39Z

lib/fizzy/parser_expr.cpp

+    return static_cast<ValType>(actual_type) == expected_type;
+}
+
+inline bool type_matches(OperandStackType actual_type, OperandStackType expected_type) noexcept


Suggested change

inline bool type_matches(OperandStackType actual_type, OperandStackType expected_type) noexcept

inline bool operator==(OperandStackType actual_type, OperandStackType expected_type) noexcept

?

I didn't go with operators, because then we should probably also have operator!= for completeness, and for all pairs OperansStackType-OperandStackType, OperandStackType-ValType, ValType-OperandStackType, and unit tests for all of them...

Also it doesn't seem exacly like equivalence-kind of relation.

I have similar doubts.

chfast · 2020-07-28T09:15:21Z

lib/fizzy/parser_expr.cpp

 {
    frame.unreachable = true;
    operand_stack.shrink(static_cast<size_t>(frame.parent_stack_height));
 }

-inline void push_operand(Stack<ValType>& operand_stack, ValType type)
+inline void push_operand(Stack<OperandStackType>& operand_stack, ValType type)


Because of this overload, it may be better to just allow implicit conversion ValType -> OperandStackType.

How would you do that exactly? I can imagine only making OperandStackType a class, containing another enum type, and implicit conversion constructor.

Yes, it looks like. Not sure this is worth it then.

Yeah, it looks that it would be quite verbose then.

axic · 2020-07-28T11:51:54Z

lib/fizzy/parser_expr.cpp

+enum class OperandStackType : uint8_t
+{
+    Unknown = 0,
+    i32 = static_cast<uint8_t>(ValType::i32),


enum class ValType : uint8_t so why is there a need for typecasting here?

Couldn't this just extend ValType?

Because ValType is still a different type from uint8_t, enum classes are strongly typed unlike plain enums.

Couldn't this just extend ValType?

No, not in C++

axic · 2020-07-28T11:56:38Z

lib/fizzy/parser_expr.cpp

-inline void drop_operand(
-    const ControlFrame& frame, Stack<ValType>& operand_stack, std::optional<ValType> expected_type)
+inline void drop_operand(const ControlFrame& frame, Stack<OperandStackType>& operand_stack,
+    OperandStackType expected_type)


Wouldn't it be actually easier codewise to keep using optional<ValType>, not introducing OperandStackType, and comparing that way?

I did it, too, at some point. These two variants are quite close, but enum one is perhaps a bit more concise and closer to Validation Algorithm.

Also enum way guarantees stack operands to be 1 byte each, without "has_value" flag overhead in optional.

(Also "Unknown" maybe better expresses the notion of "value with unknown/opaque type" than empty optional, which is rather supposed to mean no value/no type)

We still use a mixture of optional<ValType> and OperandStackType. Perhaps we should aim to only use OperandStackType where possible?

I don't think so, the remaining uses of optional<ValType> are all for block types, where empty one really means "no result value" (and so for example at jumps this means nothing is kept on stack; keeping Unknown would be incorrect)

axic · 2020-07-28T13:05:18Z

lib/fizzy/parser_expr.cpp

+
+            const auto operand_type = frame_stack_height > frame.parent_stack_height ?
+                                          operand_stack[0] :
+                                          OperandStackType::Unknown;


So we get unknown in case of unreachable?

Yes, if unreachable and nothing is pushed before, then select doesn't know types and will push Unknown in the end.

This comes from "polymorphic stack" concept in wasm. Even if unreachable, some information about instructions is validated.

gumb0 force-pushed the validate-types-select branch from cb886fb to 58d9a60 Compare July 7, 2020 16:40

gumb0 mentioned this pull request Jul 9, 2020

Minimum viable type validation #403

Merged

gumb0 force-pushed the validate-types-select branch 2 times, most recently from 7be40fc to abcc344 Compare July 14, 2020 16:26

gumb0 changed the base branch from master to validate-types-br July 14, 2020 16:26

gumb0 force-pushed the validate-types-select branch 6 times, most recently from 0e63773 to e719374 Compare July 15, 2020 13:41

gumb0 force-pushed the validate-types-br branch 6 times, most recently from 012d40a to f0962f4 Compare July 20, 2020 18:34

gumb0 force-pushed the validate-types-select branch 3 times, most recently from 2f4487d to 8fd62a3 Compare July 21, 2020 10:10

gumb0 marked this pull request as ready for review July 21, 2020 10:37

gumb0 requested review from chfast and axic July 21, 2020 14:02

gumb0 force-pushed the validate-types-br branch from f0962f4 to b977028 Compare July 27, 2020 12:40

gumb0 force-pushed the validate-types-select branch from 325254b to 2e7fe50 Compare July 27, 2020 12:44

chfast reviewed Jul 28, 2020

View reviewed changes

gumb0 force-pushed the validate-types-select branch from 2e7fe50 to e1e91fd Compare July 28, 2020 09:39

gumb0 force-pushed the validate-types-br branch from b977028 to 053b6fe Compare July 28, 2020 09:48

gumb0 force-pushed the validate-types-select branch from e1e91fd to 3bca082 Compare July 28, 2020 09:59

gumb0 force-pushed the validate-types-select branch from 3bca082 to b14c4e0 Compare July 28, 2020 10:15

gumb0 force-pushed the validate-types-br branch from 053b6fe to 935c588 Compare July 28, 2020 10:16

gumb0 force-pushed the validate-types-select branch from b14c4e0 to 3ccacae Compare July 28, 2020 10:18

chfast approved these changes Jul 28, 2020

View reviewed changes

gumb0 force-pushed the validate-types-br branch 2 times, most recently from a16aa56 to 33eede1 Compare July 28, 2020 11:35

axic reviewed Jul 28, 2020

View reviewed changes

Base automatically changed from validate-types-br to master July 28, 2020 12:07

gumb0 added 5 commits July 28, 2020 14:13

Allow Unknown value to be pushed to validation operand stack

10c6a06

Validate select instruction operand types

7f6ed8c

test: select instruction type validation

795b0d7

ci: update spectest expectations

9ea518c

test: enable select stack height validation tests

f648368

gumb0 force-pushed the validate-types-select branch from 3ccacae to f648368 Compare July 28, 2020 12:15

axic reviewed Jul 28, 2020

View reviewed changes

axic approved these changes Jul 28, 2020

View reviewed changes

gumb0 merged commit 9111419 into master Jul 28, 2020

gumb0 deleted the validate-types-select branch July 28, 2020 14:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate types for select instruction #413

Validate types for select instruction #413

gumb0 commented Jul 7, 2020 •

edited

Loading

codecov bot commented Jul 7, 2020 •

edited

Loading

chfast Jul 28, 2020

chfast Jul 28, 2020

gumb0 Jul 28, 2020

chfast Jul 28, 2020

chfast Jul 28, 2020

gumb0 Jul 28, 2020

chfast Jul 28, 2020

gumb0 Jul 28, 2020

axic Jul 28, 2020

axic Jul 28, 2020

gumb0 Jul 28, 2020

gumb0 Jul 28, 2020

axic Jul 28, 2020

gumb0 Jul 28, 2020

axic Jul 28, 2020

gumb0 Jul 28, 2020

axic Jul 28, 2020

gumb0 Jul 28, 2020

chfast Jul 28, 2020

	inline bool type_matches(OperandStackType actual_type, ValType expected_type) noexcept
	inline bool operator==(OperandStackType actual_type, ValType expected_type) noexcept

Validate types for select instruction #413

Validate types for select instruction #413

Conversation

gumb0 commented Jul 7, 2020 • edited Loading

codecov bot commented Jul 7, 2020 • 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

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 commented Jul 7, 2020 •

edited

Loading

codecov bot commented Jul 7, 2020 •

edited

Loading