From 84ef13c84fb38d37793d7aef2025be38dd9265f5 Mon Sep 17 00:00:00 2001
From: David Neto <dneto@google.com>
Date: Tue, 23 Mar 2021 17:06:29 +0000
Subject: [PATCH] spirv-reader: disallow OpSwitch without preceding
 OpSelectionMerge

This was updated/clarified by the SPIR WG.

Bug: tint:3
Change-Id: Ie4c503f0e5f80ffeabada9c526375588e81a5ceb
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/45740
Auto-Submit: David Neto <dneto@google.com>
Commit-Queue: Alan Baker <alanbaker@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Baker <alanbaker@google.com>
---
 src/reader/spirv/function.cc          | 10 ++++--
 src/reader/spirv/function_cfg_test.cc | 49 ++++++++++++++++++++-------
 2 files changed, 44 insertions(+), 15 deletions(-)

diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc
index 0b03991cb9..09eaa5bee1 100644
--- a/src/reader/spirv/function.cc
+++ b/src/reader/spirv/function.cc
@@ -2621,8 +2621,14 @@ bool FunctionEmitter::EmitNormalTerminator(const BlockInfo& block_info) {
       return true;
     }
     case SpvOpSwitch:
-      // TODO(dneto)
-      break;
+      // An OpSelectionMerge must precede an OpSwitch.  That is clarified
+      // in the resolution to Khronos-internal SPIR-V issue 115.
+      // A new enough version of the SPIR-V validator checks this case.
+      // But issue an error in this case, as a defensive measure.
+      return Fail() << "invalid structured control flow: found an OpSwitch "
+                       "that is not preceded by an "
+                       "OpSelectionMerge: "
+                    << terminator.PrettyPrint();
     default:
       break;
   }
diff --git a/src/reader/spirv/function_cfg_test.cc b/src/reader/spirv/function_cfg_test.cc
index d5a5756751..62ef82129b 100644
--- a/src/reader/spirv/function_cfg_test.cc
+++ b/src/reader/spirv/function_cfg_test.cc
@@ -13542,9 +13542,31 @@ TEST_F(SpvParserTest,
                  "a structured header (it has no merge instruction)"));
 }
 
+TEST_F(SpvParserTest, Switch_NotAsSelectionHeader_Simple) {
+  auto assembly = CommonTypes() + R"(
+     %100 = OpFunction %void None %voidfn
+
+     %10 = OpLabel
+     OpSwitch %uint_0 %99
+
+     %99 = OpLabel
+     OpReturn
+
+     OpFunctionEnd
+)";
+  auto p = parser(test::Assemble(assembly));
+  ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error();
+  FunctionEmitter fe(p.get(), *spirv_function(p.get(), 100));
+  EXPECT_FALSE(fe.EmitBody());
+  EXPECT_THAT(
+      p->error(),
+      HasSubstr("invalid structured control flow: found an OpSwitch that "
+                "is not preceded by an OpSelectionMerge:"));
+}
+
 TEST_F(SpvParserTest,
-       DISABLED_Switch_NotAsSelectionHeader_NonDefaultBranchesAreContinue) {
-  // Adapted from SPIRV-Tools test MissingMergeOneUnseenTargetSwitchGood
+       Switch_NotAsSelectionHeader_NonDefaultBranchesAreContinue) {
+  // Adapted from SPIRV-Tools test MissingMergeOneUnseenTargetSwitchBad
   auto p = parser(test::Assemble(CommonTypes() + R"(
  %100 = OpFunction %void None %voidfn
  %entry = OpLabel
@@ -13571,15 +13593,15 @@ TEST_F(SpvParserTest,
    )"));
   ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error();
   FunctionEmitter fe(p.get(), *spirv_function(p.get(), 100));
-  EXPECT_TRUE(fe.EmitBody()) << p->error();
-  auto got = ToString(p->builder(), fe.ast_body());
-  auto* expect = "unhandled case";
-  ASSERT_EQ(expect, got);
+  EXPECT_FALSE(fe.EmitBody());
+  EXPECT_THAT(
+      p->error(),
+      HasSubstr("invalid structured control flow: found an OpSwitch that "
+                "is not preceded by an OpSelectionMerge:"));
 }
 
-TEST_F(SpvParserTest,
-       DISABLED_Switch_NotAsSelectionHeader_DefaultBranchIsContinue) {
-  // Adapted from SPIRV-Tools test MissingMergeOneUnseenTargetSwitchGood
+TEST_F(SpvParserTest, Switch_NotAsSelectionHeader_DefaultBranchIsContinue) {
+  // Adapted from SPIRV-Tools test MissingMergeOneUnseenTargetSwitchBad
   auto p = parser(test::Assemble(CommonTypes() + R"(
  %100 = OpFunction %void None %voidfn
  %entry = OpLabel
@@ -13606,10 +13628,11 @@ TEST_F(SpvParserTest,
    )"));
   ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error();
   FunctionEmitter fe(p.get(), *spirv_function(p.get(), 100));
-  EXPECT_TRUE(fe.EmitBody()) << p->error();
-  auto got = ToString(p->builder(), fe.ast_body());
-  auto* expect = "unhandled case";
-  ASSERT_EQ(expect, got);
+  EXPECT_FALSE(fe.EmitBody());
+  EXPECT_THAT(
+      p->error(),
+      HasSubstr("invalid structured control flow: found an OpSwitch that "
+                "is not preceded by an OpSelectionMerge:"));
 }
 
 TEST_F(SpvParserTest, SiblingLoopConstruct_Null) {