From cda4e1d07860e12aa1721d8ca6899f1709df0aa3 Mon Sep 17 00:00:00 2001
From: Ryan Harrison <rharrison@chromium.org>
Date: Thu, 5 Dec 2024 15:04:58 +0000
Subject: [PATCH] [tint][ir][val] Reject discards outside of functions

While working on root causing the issue that led to this change I
found some other related issues that should be addressed. They have
been bundled up into this change:

- Run() is modified so that later validation passes don't run if
  CheckStructureSoundness fails, since they assume that it passed.
- ContainingFunction returns nullptr if the input is in the root
  block, which helps prevent ContainingEntryPoint from crashing in a
  very confusing way.

Fixes: 382298307
Fixes: 382291447
Fixes: 382294238
Change-Id: Ibce8c5c52d6df960a54b0438cff7a07fe740c961
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/218174
Commit-Queue: dan sinclair <dsinclair@chromium.org>
Auto-Submit: Ryan Harrison <rharrison@chromium.org>
Reviewed-by: dan sinclair <dsinclair@chromium.org>
---
 src/tint/lang/core/ir/validator.cc      | 24 ++++++++++++++++++++++--
 src/tint/lang/core/ir/validator_test.cc | 23 +++++++++++++++++++++++
 2 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/src/tint/lang/core/ir/validator.cc b/src/tint/lang/core/ir/validator.cc
index c96bda2b8c..7b9e25a986 100644
--- a/src/tint/lang/core/ir/validator.cc
+++ b/src/tint/lang/core/ir/validator.cc
@@ -1250,6 +1250,10 @@ class Validator {
     /// @param inst the instruction
     /// @returns the function
     const ir::Function* ContainingFunction(const ir::Instruction* inst) {
+        if (inst->Block() == mod_.root_block) {
+            return nullptr;
+        }
+
         return block_to_function_.GetOrAdd(inst->Block(), [&] {  //
             return ContainingFunction(inst->Block()->Parent());
         });
@@ -1259,6 +1263,10 @@ class Validator {
     /// @param f the function
     /// @returns all end points that call the function
     Hashset<const ir::Function*, 4> ContainingEndPoints(const ir::Function* f) {
+        if (!f) {
+            return {};
+        }
+
         Hashset<const ir::Function*, 4> result{};
         Hashset<const ir::Function*, 4> visited{f};
 
@@ -1266,6 +1274,10 @@ class Validator {
         while (!call_sites.IsEmpty()) {
             auto call_site = call_sites.Pop();
             auto calling_function = ContainingFunction(call_site);
+            if (!calling_function) {
+                continue;
+            }
+
             if (visited.Contains(calling_function)) {
                 continue;
             }
@@ -1336,8 +1348,13 @@ Disassembler& Validator::Disassemble() {
 Result<SuccessType> Validator::Run() {
     RunStructuralSoundnessChecks();
 
-    CheckForOrphanedInstructions();
-    CheckForNonFragmentDiscards();
+    if (!diagnostics_.ContainsErrors()) {
+        CheckForOrphanedInstructions();
+    }
+
+    if (!diagnostics_.ContainsErrors()) {
+        CheckForNonFragmentDiscards();
+    }
 
     if (diagnostics_.ContainsErrors()) {
         diagnostics_.AddNote(Source{}) << "# Disassembly\n" << Disassemble().Text();
@@ -1861,6 +1878,9 @@ void Validator::CheckRootBlock(const Block* blk) {
                     AddError(inst) << "root block: invalid instruction: " << inst->TypeInfo().name;
                 }
             },
+            [&](const core::ir::Discard* d) {
+                AddError(d) << "root block: invalid instruction: " << d->TypeInfo().name;
+            },
             [&](Default) {
                 // Note, this validation is looser then it could be. There are only certain
                 // expressions and builtins which can be used in an override. We can tighten this up
diff --git a/src/tint/lang/core/ir/validator_test.cc b/src/tint/lang/core/ir/validator_test.cc
index 5e4dc7b8f2..1381505392 100644
--- a/src/tint/lang/core/ir/validator_test.cc
+++ b/src/tint/lang/core/ir/validator_test.cc
@@ -3849,6 +3849,29 @@ note: # Disassembly
 )");
 }
 
+TEST_F(IR_ValidatorTest, Discard_OutsideFunction) {
+    auto* d = b.Discard();
+    mod.root_block->Append(d);
+
+    auto res = ir::Validate(mod);
+    ASSERT_NE(res, Success);
+    EXPECT_EQ(res.Failure().reason.Str(),
+              R"(:2:3 error: discard: root block: invalid instruction: tint::core::ir::Discard
+  discard
+  ^^^^^^^
+
+:1:1 note: in block
+$B1: {  # root
+^^^
+
+note: # Disassembly
+$B1: {  # root
+  discard
+}
+
+)");
+}
+
 TEST_F(IR_ValidatorTest, Terminator_HasResult) {
     auto* ret_func = b.Function("ret_func", ty.void_());
     b.Append(ret_func->Block(), [&] {