Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
8 changes: 7 additions & 1 deletion include/wabt/ir.h
Original file line number Diff line number Diff line change
Expand Up @@ -701,7 +701,13 @@ class SelectExpr : public ExprMixin<ExprType::Select> {
public:
SelectExpr(const Location& loc = Location())
: ExprMixin<ExprType::Select>(loc) {}
TypeVector result_type;
// Untyped select is represented by {Type::Void}. An empty result_type
// represents an explicit typed select with zero result types and is invalid.
TypeVector result_type{Type::Void};

bool IsUntyped() const {
return result_type.size() == 1 && result_type[0] == Type::Void;
}
};

class TableInitExpr : public ExprMixin<ExprType::TableInit> {
Expand Down
6 changes: 5 additions & 1 deletion src/binary-reader-ir.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1212,7 +1212,11 @@ Result BinaryReaderIR::OnReturnExpr() {

Result BinaryReaderIR::OnSelectExpr(Index result_count, Type* result_types) {
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};

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.

Do you need {}? expr_ptr->result_type = Type::Void; should work

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 since SelectExpr already defaults to {Type::Void}, I simplified it to only assign when result_count != 0.

} else {
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
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
2 changes: 1 addition & 1 deletion src/binary-writer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1081,7 +1081,7 @@ void BinaryWriter::WriteExpr(const Func* func, const Expr* expr) {
break;
case ExprType::Select: {
auto* select_expr = cast<SelectExpr>(expr);
if (select_expr->result_type.empty()) {
if (select_expr->IsUntyped()) {
WriteOpcode(stream_, Opcode::Select);
} else {
WriteOpcode(stream_, Opcode::SelectT);
Expand Down
17 changes: 15 additions & 2 deletions src/validator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -589,8 +589,21 @@ Result Validator::OnReturnCallRefExpr(ReturnCallRefExpr* expr) {
}

Result Validator::OnSelectExpr(SelectExpr* expr) {
result_ |= validator_.OnSelect(expr->loc, expr->result_type.size(),
expr->result_type.data());
Index result_count;
Type* result_types = nullptr;

if (expr->result_type.empty()) {
validator_.PrintError(expr->loc, "invalid arity in select instruction: 0.");
result_ |= Result::Error;
result_count = 0;
} else if (expr->IsUntyped()) {
result_count = 0;

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.

Index result_count = 0; Could simplify this code.

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.

Thanks @zherczeg I have simplified this by initializing result_count to 0.

} else {
result_count = expr->result_type.size();
result_types = expr->result_type.data();
}

result_ |= validator_.OnSelect(expr->loc, result_count, result_types);
return Result::Ok;
}

Expand Down
1 change: 1 addition & 0 deletions src/wast-parser.cc
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->result_type.clear();
CHECK_RESULT(ParseResultList(&expr->result_type, &result_type.vars));
}
*out_expr = std::move(expr);
Expand Down
7 changes: 6 additions & 1 deletion src/wat-writer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -977,7 +977,12 @@ Result WatWriter::ExprVisitorDelegate::OnReturnCallRefExpr(

Result WatWriter::ExprVisitorDelegate::OnSelectExpr(SelectExpr* expr) {
writer_->WritePutsSpace(Opcode::Select_Opcode.GetName());
if (!expr->result_type.empty()) {
if (expr->IsUntyped()) {
// no result annotation

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.

I usually don't prefer empty statements.

if (expr->result_type.empty()) { ... } else if (!expr->IsUntyped()) { ... }

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.

Makes sense. Updated it to avoid the empty branch.

} else if (expr->result_type.empty()) {
writer_->WriteOpenSpace("result");
writer_->WriteCloseSpace();
} else {
writer_->WriteTypes(expr->result_type, "result");
}
writer_->WriteNewline(NO_FORCE_NEWLINE);
Expand Down
26 changes: 26 additions & 0 deletions test/binary/bad-select-empty-result.txt
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
select_t[0x1c]
result_count[0x00]
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 ;;)
14 changes: 14 additions & 0 deletions test/parse/expr/bad-select-empty-result.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
;;; TOOL: wat2wasm
;;; ERROR: 1
(module
(func
i32.const 42
i32.const 100
i32.const 1
select (result)
drop))

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.

Should not this be drop drop? Only the i32.const 1 should be consumed if I remember correctly.

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 You're right. Only the i32 condition should be consumed here. I updated the recovery logic and changed the test to drop drop.

(;; STDERR ;;;
out/test/parse/expr/bad-select-empty-result.txt:8:5: error: invalid arity in select instruction: 0.
select (result)
^^^^^^
;;; STDERR ;;)
3 changes: 1 addition & 2 deletions test/spec/function-references/select.txt
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,7 @@ out/test/spec/function-references/select.wast:364: assert_invalid passed:
out/test/spec/function-references/select/select.1.wasm:000001c: error: type mismatch in select, expected [any, any, i32] but got [i32]
000001c: error: OnSelectExpr callback failed
out/test/spec/function-references/select.wast:368: assert_invalid passed:
out/test/spec/function-references/select/select.2.wasm:000001c: error: type mismatch in select, expected [any, any, i32] but got [i32]
000001c: error: OnSelectExpr callback failed
000001d: error: invalid arity in select instruction: 0.
out/test/spec/function-references/select.wast:372: assert_invalid passed:
out/test/spec/function-references/select/select.3.wasm:0000027: error: invalid arity in select instruction: 2.
0000027: error: OnSelectExpr callback failed
Expand Down
3 changes: 1 addition & 2 deletions test/spec/select.txt
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@ out/test/spec/select.wast:320: assert_invalid passed:
out/test/spec/select/select.1.wasm:000001c: error: type mismatch in select, expected [any, any, i32] but got [i32]
000001c: error: OnSelectExpr callback failed
out/test/spec/select.wast:324: assert_invalid passed:
out/test/spec/select/select.2.wasm:000001c: error: type mismatch in select, expected [any, any, i32] but got [i32]
000001c: error: OnSelectExpr callback failed
000001d: error: invalid arity in select instruction: 0.
out/test/spec/select.wast:328: assert_invalid passed:
out/test/spec/select/select.3.wasm:0000027: error: invalid arity in select instruction: 2.
0000027: error: OnSelectExpr callback failed
Expand Down
Loading