From 8570a64b9acb24c3a43c4d7001f3c548de6313cd Mon Sep 17 00:00:00 2001 From: Ryan Harrison Date: Thu, 15 Aug 2024 21:33:38 +0000 Subject: [PATCH] [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 Auto-Submit: Ryan Harrison Commit-Queue: Ryan Harrison Reviewed-by: James Price --- src/tint/lang/core/ir/return.h | 9 ++++ src/tint/lang/core/ir/swizzle.h | 2 +- src/tint/lang/core/ir/validator.cc | 34 ++++++++++---- src/tint/lang/core/ir/validator_test.cc | 62 +++++++++++++++++++++++-- 4 files changed, 95 insertions(+), 12 deletions(-) diff --git a/src/tint/lang/core/ir/return.h b/src/tint/lang/core/ir/return.h index 9faa020dc78..fa474bf1e93 100644 --- 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 { /// 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 0b32ffd2511..b29db4518dc 100644 --- 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> { 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 c6cfa174498..f7b5af11eab 100644 --- 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()) { 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 index 678291c51d4..4115311a585 100644 --- 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()); + 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 = 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 ^^^