Validate types at branch instructions #408
Conversation
Codecov Report
@@ Coverage Diff @@
## master #408 +/- ##
========================================
Coverage 99.36% 99.37%
========================================
Files 49 49
Lines 14169 14345 +176
========================================
+ Hits 14079 14255 +176
Misses 90 90 |
gumb0
force-pushed
the
validate-types-br
branch
3 times, most recently
from
fc8f134 to
fa95a20
Compare
gumb0
force-pushed
the
validate-types-block
branch
2 times, most recently
from
e8db0dc to
98ae70c
Compare
|
@gumb0 rebase please |
gumb0
force-pushed
the
validate-types-br
branch
5 times, most recently
from
ce7e981 to
294425e
Compare
| // How many stack items to drop when taking the branch. | ||
| // This value can be negative for unreachable instructions. | ||
| const auto stack_drop = stack_height - branch_frame.parent_stack_height - arity; | ||
| const auto stack_drop = stack_height - branch_frame.parent_stack_height; |
There was a problem hiding this comment.
It's not needed to subtract arity now, because now we pop the result from stack before calling this function (in update_branch_stack)
| @@ -557,6 +550,14 @@ parser_result<Code> parse_expr(const uint8_t* pos, const uint8_t* end, FuncIdx f | |||
|
|
|||
| if (instr == Instr::br) | |||
| mark_frame_unreachable(frame, operand_stack); | |||
| else | |||
| { | |||
| // For the case when branch is not taken for br_if, | |||
There was a problem hiding this comment.
Validation Algorithm does something similar:
| @@ -455,7 +455,7 @@ TEST(validation, br_table_invalid_type) | |||
| (func (param $x i32) | |||
| (block $a (result i32) | |||
| (block $b (result i64) | |||
| i32.const 1 | |||
| i64.const 1 | |||
There was a problem hiding this comment.
This fix is needed because now it fist checks the return type on the stack against default label type before iterating over all labels and checking there consistency.
(Without the fix it's "type mismatch" error instead of "inconsistent labels")
|
Rebased. |
lib/fizzy/parser_expr.cpp
Outdated
|
|
||
| // Push frame start location as br immediates - these are final if frame is loop, | ||
| // but for block/if/else these are just placeholders, to be filled at end instruction. | ||
| push(immediates, static_cast<uint32_t>(branch_frame.code_offset)); | ||
| push(immediates, static_cast<uint32_t>(branch_frame.immediates_offset)); | ||
| push(immediates, static_cast<uint32_t>(stack_drop)); | ||
| push(immediates, arity); | ||
| push(immediates, get_branch_arity(branch_frame)); |
There was a problem hiding this comment.
What is the return type of get_branch_arity?
There was a problem hiding this comment.
I know the code prior was not clear too (with using auto), but can you leave a comment here stating it is uint8_t?
We have a separate PR which tries to change the types to unified uint32_t, but that showed slowdown: #376
No description provided.