[tint][ir][val] Improve checks on Returns
- Adds testing for operands and results expected for the instruction
- Refactors error logging to use NameOf to avoid accidental segfaults
- Fixes a couple of potential bad patterns in other implementations of
NameOf
- Fixes a typo in swizzle.h
Fixes: 360058656
Change-Id: I87dbbb02e7dbf2a6203662b1ecc2c7014e460963
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/202500
Reviewed-by: dan sinclair <dsinclair@chromium.org>
Auto-Submit: Ryan Harrison <rharrison@chromium.org>
Commit-Queue: Ryan Harrison <rharrison@chromium.org>
Reviewed-by: James Price <jrprice@google.com>
diff --git a/src/tint/lang/core/ir/return.h b/src/tint/lang/core/ir/return.h
index 9faa020..fa474bf 100644
--- a/src/tint/lang/core/ir/return.h
+++ b/src/tint/lang/core/ir/return.h
@@ -49,6 +49,15 @@
/// The offset in Operands() for the return argument
static constexpr size_t kArgsOperandOffset = 1;
+ /// The fixed number of results returned by return instructions
+ static constexpr size_t kNumResults = 0;
+
+ /// The minimum number of operands accepted by return instructions
+ static constexpr size_t kMinOperands = 1;
+
+ /// The maximum number of operands accepted by return instructions
+ static constexpr size_t kMaxOperands = 2;
+
/// Constructor (no operands)
/// @param id the instruction id
explicit Return(Id id);
diff --git a/src/tint/lang/core/ir/swizzle.h b/src/tint/lang/core/ir/swizzle.h
index 0b32ffd..b29db45 100644
--- a/src/tint/lang/core/ir/swizzle.h
+++ b/src/tint/lang/core/ir/swizzle.h
@@ -46,7 +46,7 @@
static constexpr size_t kNumResults = 1;
/// The fixed number of operands expected for swizzle instructions
- /// @note indices for swizzle are handled seperately from the operands, so not included here
+ /// @note indices for swizzle are handled separately from the operands, so not included here
static constexpr size_t kNumOperands = 1;
/// Minimum number of indices expected for swizzle instructions
diff --git a/src/tint/lang/core/ir/validator.cc b/src/tint/lang/core/ir/validator.cc
index c6cfa17..f7b5af1 100644
--- a/src/tint/lang/core/ir/validator.cc
+++ b/src/tint/lang/core/ir/validator.cc
@@ -263,10 +263,14 @@
/// @param res the res
void AddDeclarationNote(const InstructionResult* res);
- /// @param decl the value, instruction or block to get the name for
+ /// @param decl the type, value, instruction or block to get the name for
/// @returns the styled name for the given value, instruction or block
StyledText NameOf(const CastableBase* decl);
+ // @param ty the type to get the name for
+ /// @returns the styled name for the given type
+ StyledText NameOf(const type::Type* ty);
+
/// @param v the value to get the name for
/// @returns the styled name for the given value
StyledText NameOf(const Value* v);
@@ -841,22 +845,30 @@
StyledText Validator::NameOf(const CastableBase* decl) {
return tint::Switch(
decl, //
+ [&](const type::Type* ty) { return NameOf(ty); },
[&](const Value* value) { return NameOf(value); },
[&](const Instruction* inst) { return NameOf(inst); },
[&](const Block* block) { return NameOf(block); }, //
TINT_ICE_ON_NO_MATCH);
}
+StyledText Validator::NameOf(const type::Type* ty) {
+ auto name = ty ? ty->FriendlyName() : "undef";
+ return StyledText{} << style::Type(name);
+}
+
StyledText Validator::NameOf(const Value* value) {
return Disassemble().NameOf(value);
}
StyledText Validator::NameOf(const Instruction* inst) {
- return StyledText{} << style::Instruction(inst->FriendlyName());
+ auto name = inst ? inst->FriendlyName() : "undef";
+ return StyledText{} << style::Instruction(name);
}
StyledText Validator::NameOf(const Block* block) {
- return StyledText{} << style::Instruction(block->Parent()->FriendlyName()) << " block "
+ auto parent_name = block->Parent() ? block->Parent()->FriendlyName() : "undef";
+ return StyledText{} << style::Instruction(parent_name) << " block "
<< Disassemble().NameOf(block);
}
@@ -1967,11 +1979,19 @@
}
void Validator::CheckReturn(const Return* ret) {
- auto* func = ret->Func();
- if (func == nullptr) {
- AddError(ret) << "undefined function";
+ if (!CheckResultsAndOperandRange(ret, Return::kNumResults, Return::kMinOperands,
+ Return::kMaxOperands)) {
return;
}
+
+ auto* func = ret->Func();
+ if (func == nullptr) {
+ // Func() returning nullptr after CheckResultsAndOperandRange is due to the first operand
+ // being not a function
+ AddError(ret) << "expected function for first operand";
+ return;
+ }
+
if (func->ReturnType()->Is<core::type::Void>()) {
if (ret->Value()) {
AddError(ret) << "unexpected return value";
@@ -1980,10 +2000,8 @@
if (!ret->Value()) {
AddError(ret) << "expected return value";
} else if (ret->Value()->Type() != func->ReturnType()) {
- AddError(ret) << "return value type "
- << style::Type(ret->Value()->Type()->FriendlyName())
- << " does not match function return type "
- << style::Type(func->ReturnType()->FriendlyName());
+ AddError(ret) << "return value type " << NameOf(ret->Value()->Type())
+ << " does not match function return type " << NameOf(func->ReturnType());
}
}
}
diff --git a/src/tint/lang/core/ir/validator_test.cc b/src/tint/lang/core/ir/validator_test.cc
index 678291c..4115311 100644
--- a/src/tint/lang/core/ir/validator_test.cc
+++ b/src/tint/lang/core/ir/validator_test.cc
@@ -5993,15 +5993,71 @@
EXPECT_EQ(ir::Validate(mod), Success);
}
-TEST_F(IR_ValidatorTest, Return_NullFunction) {
- auto* f = b.Function("my_func", ty.void_());
+TEST_F(IR_ValidatorTest, Return_UnexpectedResult) {
+ auto* f = b.Function("my_func", ty.i32());
b.Append(f->Block(), [&] { //
- b.Return(nullptr);
+ auto* r = b.Return(f, 42_i);
+ r->SetResults(Vector{b.InstructionResult(ty.i32())});
});
auto res = ir::Validate(mod);
ASSERT_NE(res, Success);
- EXPECT_EQ(res.Failure().reason.Str(), R"(:3:5 error: return: undefined function
+ EXPECT_EQ(res.Failure().reason.Str(), R"(:3:5 error: return: expected exactly 0 results, got 1
+ ret 42i
+ ^^^^^^^
+
+:2:3 note: in block
+ $B1: {
+ ^^^
+
+note: # Disassembly
+%my_func = func():i32 {
+ $B1: {
+ ret 42i
+ }
+}
+)");
+}
+
+TEST_F(IR_ValidatorTest, Return_NotFunction) {
+ auto* f = b.Function("my_func", ty.void_());
+ b.Append(f->Block(), [&] { //
+ auto* var = b.Var(ty.ptr<function, f32>());
+ auto* r = b.Return(nullptr);
+ r->SetOperand(0, var->Result(0));
+ });
+
+ auto res = ir::Validate(mod);
+ ASSERT_NE(res, Success);
+ EXPECT_EQ(res.Failure().reason.Str(), R"(:4:5 error: return: expected function for first operand
+ ret
+ ^^^
+
+:2:3 note: in block
+ $B1: {
+ ^^^
+
+note: # Disassembly
+%my_func = func():void {
+ $B1: {
+ %2:ptr<function, f32, read_write> = var
+ ret
+ }
+}
+)");
+}
+
+TEST_F(IR_ValidatorTest, Return_MissingFunction) {
+ auto* f = b.Function("my_func", ty.void_());
+ b.Append(f->Block(), [&] {
+ auto* r = b.Return(f);
+ r->ClearOperands();
+ });
+
+ auto res = ir::Validate(mod);
+ ASSERT_NE(res, Success);
+ EXPECT_EQ(res.Failure().reason.Str(),
+ R"(:3:5 error: return: expected between 1 and 2 operands, got 0
ret
^^^
