Fix validation of typed select with empty result types (#2708) - #2810
Fix validation of typed select with empty result types (#2708)#2810ANAMASGARD wants to merge 4 commits into
Conversation
Reject typed select instructions with an empty result type vector when reading binary modules and validating WAT, while preserving untyped select behavior. Add regression coverage for WebAssembly#2708. Signed-off-by: Gaurav Chaudhary <chaudharygaurav2004@gmail.com>
Signed-off-by: Gaurav Chaudhary <chaudharygaurav2004@gmail.com>
|
Does this mean we are lacking a test in the spec repo for this? |
| SelectExpr(const Location& loc = Location()) | ||
| : ExprMixin<ExprType::Select>(loc) {} | ||
| TypeVector result_type; | ||
| bool has_result_type = false; |
There was a problem hiding this comment.
Can't we use the zero-length result_type of imply that the expr has not result type?
There was a problem hiding this comment.
@sbc100 empty result_type can mean either “untyped select” or “invalid typed select with no types” — has_result_type distinguishes those, so I don’t think we can drop it without losing that check. (Not part of this PR though.)
There was a problem hiding this comment.
Do does has_result_type = false correspond to "untyped select".
Maybe a comment here explaining the distinction and why this member is needed?
There was a problem hiding this comment.
What is the purpose of "untyped select"? This likely rare case could be represented by the void type.
Btw if "supporting untyped select" is not part of the PR, we should not introduce this.
|
@sbc100 the spec repo already has tests for this in the GC proposal files (like struct/array fields with (ref N)). WABT just doesn’t run those GC spec tests yet — they’re marked unimplemented in our test script. So the tests exist upstream, but we weren’t exercising them in WABT, which is why I added regress-2750.txt for this parser bug. |
| i32.const 100 | ||
| i32.const 1 | ||
| 0x1c | ||
| 0x00 |
There was a problem hiding this comment.
What are these two magic numbers?
I think you can document by using the name_of_thing[0x1x] syntax maybe?
There was a problem hiding this comment.
@sbc100 thanks for the feedback I have replaced the raw bytes with select_t[0x1c] and result_count[0x00] to make the test clearer.
But what about the non-GC tests.. if we can write tests for this where in wabt that don't depend on GC types then presumably one could write the same non-GC upstream in the spec repo? |
|
@tlively does this looks like a test case that is missing upstream in the spec maybe? |
| out/test/parse/expr/bad-select-empty-result.txt:8:5: error: invalid arity in select instruction: 0. | ||
| select (result))) | ||
| ^^^^^^ | ||
| out/test/parse/expr/bad-select-empty-result.txt:8:5: error: type mismatch at end of function, expected [] but got [i32, i32, i32] |
There was a problem hiding this comment.
Even if the results are not consumed, the condition i32 should be.
There was a problem hiding this comment.
@zherczeg I have Fixed it . We now report the arity error but still run OnSelect, so the condition and operands are consumed correctly. I also updated the test with drop.
Use Type::Void as the untyped select sentinel instead of has_result_type, fix validator stack recovery, and update regression/spec goldens. Signed-off-by: Gaurav Chaudhary <chaudharygaurav2004@gmail.com>
|
@sbc100 I am sorry I mixed this up with another issue earlier . There is already an $arity-0 test in select.wast, but it uses nop operands, so WABT can fail for a stack mismatch instead of the missing arity check. A test with valid operands would catch this bug directly. Happy to follow up upstream if needed. |
zherczeg
left a comment
zherczeg
left a comment
There was a problem hiding this comment.
I like this code much better.
| auto expr_ptr = std::make_unique<SelectExpr>(); | ||
| expr_ptr->result_type.assign(result_types, result_types + result_count); | ||
| if (result_count == 0) { | ||
| expr_ptr->result_type = {Type::Void}; |
There was a problem hiding this comment.
Do you need {}? expr_ptr->result_type = Type::Void; should work
There was a problem hiding this comment.
@zherczeg since SelectExpr already defaults to {Type::Void}, I simplified it to only assign when result_count != 0.
| result_ |= Result::Error; | ||
| result_count = 0; | ||
| } else if (expr->IsUntyped()) { | ||
| result_count = 0; |
There was a problem hiding this comment.
Index result_count = 0; Could simplify this code.
There was a problem hiding this comment.
Thanks @zherczeg I have simplified this by initializing result_count to 0.
| writer_->WritePutsSpace(Opcode::Select_Opcode.GetName()); | ||
| if (!expr->result_type.empty()) { | ||
| if (expr->IsUntyped()) { | ||
| // no result annotation |
There was a problem hiding this comment.
I usually don't prefer empty statements.
if (expr->result_type.empty()) { ... } else if (!expr->IsUntyped()) { ... }
There was a problem hiding this comment.
Makes sense. Updated it to avoid the empty branch.
| i32.const 100 | ||
| i32.const 1 | ||
| select (result) | ||
| drop)) |
There was a problem hiding this comment.
Should not this be drop drop? Only the i32.const 1 should be consumed if I remember correctly.
There was a problem hiding this comment.
@zherczeg You're right. Only the i32 condition should be consumed here. I updated the recovery logic and changed the test to drop drop.
Simplify select IR handling and consume only the i32 condition during recovery for invalid select (result). Signed-off-by: Gaurav Chaudhary <chaudharygaurav2004@gmail.com>
3 participants
Fixes #2708
Summary
SelectTwith zero result types in the binary reader instead of treating it as untypedselectselect (result)with an empty result list during validationTest plan
python3 test/run-tests.py binary/bad-select-empty-result parse/expr/bad-select-empty-result spec/select./out/wabt-unittestspython3 test/run-tests.py(full suite)SelectT+ empty type vector rejected bywasm-interpandwasm-validate