Skip to content

Fix validation of typed select with empty result types (#2708) #2810

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sbc100 merged 4 commits into from
Aug 10, 2026
Merged

Fix validation of typed select with empty result types (#2708) #2810

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ba20b0e
Fix typed select validation for empty result types
ANAMASGARD Aug 7, 2026
435c6ad
Applyed the clang-format formatting fixes.
ANAMASGARD Aug 7, 2026
73d0f3b
Address review feedback for typed select empty result validation.
ANAMASGARD Aug 8, 2026
e6d3387
Address follow-up review feedback for typed select
ANAMASGARD Aug 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions include/wabt/ir.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -702,6 +702,7 @@ class SelectExpr : public ExprMixin<ExprType::Select> {
SelectExpr(const Location& loc = Location())
: ExprMixin<ExprType::Select>(loc) {}
TypeVector result_type;
bool has_result_type = false;

@sbc100 sbc100 Aug 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we use the zero-length result_type of imply that the expr has not result type?

@ANAMASGARD ANAMASGARD Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.)

@sbc100 sbc100 Aug 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do does has_result_type = false correspond to "untyped select".

Maybe a comment here explaining the distinction and why this member is needed?

@zherczeg zherczeg Aug 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ANAMASGARD ANAMASGARD Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbc100 & @zherczeg Thanks for the feedback .
I removed has_result_type and now use {Type::Void} to represent the existing untyped select. An empty result_type now represents invalid typed select (result), and I added a comment documenting the distinction.

};

class TableInitExpr : public ExprMixin<ExprType::TableInit> {
Expand Down
1 change: 1 addition & 0 deletions src/binary-reader-ir.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1212,6 +1212,7 @@ Result BinaryReaderIR::OnReturnExpr() {

Result BinaryReaderIR::OnSelectExpr(Index result_count, Type* result_types) {
auto expr_ptr = std::make_unique<SelectExpr>();
expr_ptr->has_result_type = result_count > 0;
expr_ptr->result_type.assign(result_types, result_types + result_count);
return AppendExpr(std::move(expr_ptr));
}
Expand Down
10 changes: 3 additions & 7 deletions src/binary-reader.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -813,6 +813,7 @@ Result BinaryReader::ReadInstructions(Offset end_offset, const char* context) {
case Opcode::SelectT: {
Index num_results;
CHECK_RESULT(ReadCount(&num_results, "num result types"));
ERROR_IF(num_results == 0, "invalid arity in select instruction: 0.");

result_types_.resize(num_results);
for (Index i = 0; i < num_results; ++i) {
Expand All @@ -825,13 +826,8 @@ Result BinaryReader::ReadInstructions(Offset end_offset, const char* context) {
result_types_[i] = result_type;
}

if (num_results) {
CALLBACK(OnSelectExpr, num_results, result_types_.data());
CALLBACK(OnOpcodeType, result_types_[0]);
} else {
CALLBACK(OnSelectExpr, 0, NULL);
CALLBACK0(OnOpcodeBare);
}
CALLBACK(OnSelectExpr, num_results, result_types_.data());
CALLBACK(OnOpcodeType, result_types_[0]);
break;
}

Expand Down
5 changes: 5 additions & 0 deletions src/validator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,6 +589,11 @@ Result Validator::OnReturnCallRefExpr(ReturnCallRefExpr* expr) {
}

Result Validator::OnSelectExpr(SelectExpr* expr) {
if (expr->has_result_type && expr->result_type.empty()) {
validator_.PrintError(expr->loc, "invalid arity in select instruction: 0.");
result_ |= Result::Error;
return Result::Ok;
}
result_ |= validator_.OnSelect(expr->loc, expr->result_type.size(),
expr->result_type.data());
return Result::Ok;
Expand Down
1 change: 1 addition & 0 deletions src/wast-parser.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2538,6 +2538,7 @@ Result WastParser::ParsePlainInstr(std::unique_ptr<Expr>* out_expr) {
ResolveTypeVector result_type(&expr->result_type);
if (options_->features.reference_types_enabled() &&
PeekMatchLpar(TokenType::Result)) {
expr->has_result_type = true;
CHECK_RESULT(ParseResultList(&expr->result_type, &result_type.vars));
}
*out_expr = std::move(expr);
Expand Down
26 changes: 26 additions & 0 deletions test/binary/bad-select-empty-result.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
;;; TOOL: run-gen-wasm-bad
;;; ERROR: 1
;;; RUN: %(wasm-interp)s --enable-all %(temp_file)s.wasm --run-export=run
;;; ERROR3: 1
magic
version
section(TYPE) { count[1] function params[0] results[1] i32 }
section(FUNCTION) { count[1] type[0] }
section(EXPORT) { count[1] str("run") func_kind 0 }
section(CODE) {
count[1]
func {
locals[0]
i32.const 42
i32.const 100
i32.const 1
0x1c
0x00

@sbc100 sbc100 Aug 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are these two magic numbers?

I think you can document by using the name_of_thing[0x1x] syntax maybe?

@ANAMASGARD ANAMASGARD Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbc100 thanks for the feedback I have replaced the raw bytes with select_t[0x1c] and result_count[0x00] to make the test clearer.

end
}
}
(;; STDERR ;;;
0000029: error: invalid arity in select instruction: 0.
0000029: error: invalid arity in select instruction: 0.
0000029: error: invalid arity in select instruction: 0.
;;; STDERR ;;)
16 changes: 16 additions & 0 deletions test/parse/expr/bad-select-empty-result.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
;;; TOOL: wat2wasm
;;; ERROR: 1
(module
(func
i32.const 42
i32.const 100
i32.const 1
select (result)))
(;; STDERR ;;;
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]

@zherczeg zherczeg Aug 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if the results are not consumed, the condition i32 should be.

@ANAMASGARD ANAMASGARD Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

select (result)))
^^^^^^
;;; STDERR ;;)
Loading