[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 <[email protected]> Auto-Submit: Ryan Harrison <[email protected]> Commit-Queue: Ryan Harrison <[email protected]> Reviewed-by: James Price <[email protected]>
google · Aug 15, 2024 · 8570a64 · 8570a64
1 parent 305938c
commit 8570a64
Show file tree

Hide file tree

Showing 4 changed files with 95 additions and 12 deletions.
diff --git a/src/tint/lang/core/ir/return.h b/src/tint/lang/core/ir/return.h
@@ -49,6 +49,15 @@ class Return final : public Castable<Return, Terminator> {
     /// 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
@@ -46,7 +46,7 @@ class Swizzle final : public Castable<Swizzle, OperandInstruction<1, 1>> {
     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
@@ -263,10 +263,14 @@ class Validator {
     /// @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 @@ void Validator::AddDeclarationNote(const InstructionResult* res) {
 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::CheckExitIf(const ExitIf* e) {
 }
 
 void Validator::CheckReturn(const Return* ret) {
+    if (!CheckResultsAndOperandRange(ret, Return::kNumResults, Return::kMinOperands,
+                                     Return::kMaxOperands)) {
+        return;
+    }
+
     auto* func = ret->Func();
     if (func == nullptr) {
-        AddError(ret) << "undefined function";
+        // 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 @@ void Validator::CheckReturn(const Return* ret) {
         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
@@ -5993,15 +5993,71 @@ TEST_F(IR_ValidatorTest, Return_WithValue) {
     EXPECT_EQ(ir::Validate(mod), Success);
 }
 
-TEST_F(IR_ValidatorTest, Return_NullFunction) {
+TEST_F(IR_ValidatorTest, Return_UnexpectedResult) {
+    auto* f = b.Function("my_func", ty.i32());
+    b.Append(f->Block(), [&] {  //
+        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: 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(), [&] {  //
-        b.Return(nullptr);
+        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: undefined function
+    EXPECT_EQ(res.Failure().reason.Str(),
+              R"(:3:5 error: return: expected between 1 and 2 operands, got 0
     ret
     ^^^