Validate SPIRV produced by Tint

The now-removed SPIRV-Cross path used to always do this, and the
pure Tint-only path never actually validated the SPIRV. Tint
does not run SPIRV-Tools validation on its output, so add in
validation to ensure we don't pass invalid SPIRV to the driver.
The validation can probably eventually be removed when we're more
confident that Tint's SPIRV output is always correct.

Also include various cleanups for old / unused code.

Bug: dawn:1036
Change-Id: Iaab037518965e52edbd1829f6ab6ba2af0e70143
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/61589
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Reviewed-by: Jiawei Shao <jiawei.shao@intel.com>
Commit-Queue: Jiawei Shao <jiawei.shao@intel.com>
Auto-Submit: Austin Eng <enga@chromium.org>

src/dawn_native/BUILD.gn

@@ -474,6 +474,13 @@ source_set("dawn_native_sources") {
     ]
   }
 
+  if (dawn_enable_opengl || dawn_enable_vulkan) {
+    sources += [
+      "SpirvValidation.cpp",
+      "SpirvValidation.h",
+    ]
+  }
+
   if (dawn_enable_opengl) {
     public_deps += [
       ":dawn_native_opengl_loader_gen",

src/dawn_native/CMakeLists.txt

@@ -354,6 +354,13 @@ if (DAWN_ENABLE_NULL)
     )
 endif()
 
+if (DAWN_ENABLE_OPENGL OR DAWN_ENABLE_VULKAN)
+    target_sources(dawn_native PRIVATE
+        "SpirvValidation.cpp"
+        "SpirvValidation.h"
+    )
+endif()
+
 if (DAWN_ENABLE_OPENGL)
     DawnGenerator(
         SCRIPT "${Dawn_SOURCE_DIR}/generator/opengl_loader_generator.py"

src/dawn_native/SpirvValidation.cpp

@@ -0,0 +1,76 @@
+// Copyright 2021 The Dawn Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "dawn_native/SpirvValidation.h"
+
+#include "dawn_native/Device.h"
+
+#include <spirv-tools/libspirv.hpp>
+#include <sstream>
+
+namespace dawn_native {
+
+    MaybeError ValidateSpirv(DeviceBase* device,
+                             const std::vector<uint32_t>& spirv,
+                             bool dumpSpirv) {
+        spvtools::SpirvTools spirvTools(SPV_ENV_VULKAN_1_1);
+        spirvTools.SetMessageConsumer([device](spv_message_level_t level, const char*,
+                                               const spv_position_t& position,
+                                               const char* message) {
+            WGPULoggingType wgpuLogLevel;
+            switch (level) {
+                case SPV_MSG_FATAL:
+                case SPV_MSG_INTERNAL_ERROR:
+                case SPV_MSG_ERROR:
+                    wgpuLogLevel = WGPULoggingType_Error;
+                    break;
+                case SPV_MSG_WARNING:
+                    wgpuLogLevel = WGPULoggingType_Warning;
+                    break;
+                case SPV_MSG_INFO:
+                    wgpuLogLevel = WGPULoggingType_Info;
+                    break;
+                default:
+                    wgpuLogLevel = WGPULoggingType_Error;
+                    break;
+            }
+
+            std::ostringstream ss;
+            ss << "SPIRV line " << position.index << ": " << message << std::endl;
+            device->EmitLog(wgpuLogLevel, ss.str().c_str());
+        });
+
+        const bool valid = spirvTools.Validate(spirv);
+        if (dumpSpirv || !valid) {
+            std::ostringstream dumpedMsg;
+            std::string disassembly;
+            if (spirvTools.Disassemble(
+                    spirv, &disassembly,
+                    SPV_BINARY_TO_TEXT_OPTION_FRIENDLY_NAMES | SPV_BINARY_TO_TEXT_OPTION_INDENT)) {
+                dumpedMsg << "/* Dumped generated SPIRV disassembly */" << std::endl << disassembly;
+            } else {
+                dumpedMsg << "/* Failed to disassemble generated SPIRV */";
+            }
+            device->EmitLog(WGPULoggingType_Info, dumpedMsg.str().c_str());
+        }
+
+        if (!valid) {
+            return DAWN_VALIDATION_ERROR(
+                "Produced invalid SPIRV. Please file a bug at https://crbug.com/tint.");
+        }
+
+        return {};
+    }
+
+}  // namespace dawn_native

src/dawn_native/SpirvValidation.h

@@ -0,0 +1,27 @@
+// Copyright 2021 The Dawn Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "dawn_native/Error.h"
+
+#include <vector>
+
+namespace dawn_native {
+
+    class DeviceBase;
+
+    MaybeError ValidateSpirv(DeviceBase* device,
+                             const std::vector<uint32_t>& spirv,
+                             bool dumpSpirv);
+
+}  // namespace dawn_native

src/dawn_native/opengl/ShaderModuleGL.cpp

@@ -17,6 +17,7 @@
 #include "common/Assert.h"
 #include "common/Platform.h"
 #include "dawn_native/BindGroupLayout.h"
+#include "dawn_native/SpirvValidation.h"
 #include "dawn_native/TintUtils.h"
 #include "dawn_native/opengl/DeviceGL.h"
 #include "dawn_native/opengl/PipelineLayoutGL.h"
@@ -364,26 +365,6 @@ namespace dawn_native { namespace opengl {
                                                              CombinedSamplerInfo* combinedSamplers,
                                                              const PipelineLayout* layout,
                                                              bool* needsDummySampler) const {
-        // If these options are changed, the values in DawnSPIRVCrossGLSLFastFuzzer.cpp need to
-        // be updated.
-        spirv_cross::CompilerGLSL::Options options;
-
-        // The range of Z-coordinate in the clipping volume of OpenGL is [-w, w], while it is
-        // [0, w] in D3D12, Metal and Vulkan, so we should normalize it in shaders in all
-        // backends. See the documentation of
-        // spirv_cross::CompilerGLSL::Options::vertex::fixup_clipspace for more details.
-        options.vertex.flip_vert_y = true;
-        options.vertex.fixup_clipspace = true;
-
-        const OpenGLVersion& version = ToBackend(GetDevice())->gl.GetVersion();
-        if (version.IsDesktop()) {
-            // The computation of GLSL version below only works for 3.3 and above.
-            ASSERT(version.IsAtLeast(3, 3));
-        }
-        options.es = version.IsES();
-        options.version = version.GetMajor() * 100 + version.GetMinor() * 10;
-
-        std::vector<uint32_t> spirv;
         tint::transform::SingleEntryPoint singleEntryPointTransform;
 
         tint::transform::DataMap transformInputs;
@@ -403,21 +384,28 @@ namespace dawn_native { namespace opengl {
             return DAWN_VALIDATION_ERROR(errorStream.str().c_str());
         }
 
-        spirv = std::move(result.spirv);
+        std::vector<uint32_t> spirv = std::move(result.spirv);
+        DAWN_TRY(
+            ValidateSpirv(GetDevice(), spirv, GetDevice()->IsToggleEnabled(Toggle::DumpShaders)));
 
-        if (GetDevice()->IsToggleEnabled(Toggle::DumpShaders)) {
+        // If these options are changed, the values in DawnSPIRVCrossGLSLFastFuzzer.cpp need to
-            spvtools::SpirvTools spirvTools(SPV_ENV_VULKAN_1_1);
+        // be updated.
-            std::ostringstream dumpedMsg;
+        spirv_cross::CompilerGLSL::Options options;
-            std::string disassembly;
+
-            if (spirvTools.Disassemble(
+        // The range of Z-coordinate in the clipping volume of OpenGL is [-w, w], while it is
-                    spirv, &disassembly,
+        // [0, w] in D3D12, Metal and Vulkan, so we should normalize it in shaders in all
-                    SPV_BINARY_TO_TEXT_OPTION_FRIENDLY_NAMES | SPV_BINARY_TO_TEXT_OPTION_INDENT)) {
+        // backends. See the documentation of
-                dumpedMsg << "/* Dumped generated SPIRV disassembly */" << std::endl << disassembly;
+        // spirv_cross::CompilerGLSL::Options::vertex::fixup_clipspace for more details.
-            } else {
+        options.vertex.flip_vert_y = true;
-                dumpedMsg << "/* Failed to disassemble generated SPIRV */";
+        options.vertex.fixup_clipspace = true;
-            }
+
-            GetDevice()->EmitLog(WGPULoggingType_Info, dumpedMsg.str().c_str());
+        const OpenGLVersion& version = ToBackend(GetDevice())->gl.GetVersion();
+        if (version.IsDesktop()) {
+            // The computation of GLSL version below only works for 3.3 and above.
+            ASSERT(version.IsAtLeast(3, 3));
         }
+        options.es = version.IsES();
+        options.version = version.GetMajor() * 100 + version.GetMinor() * 10;
 
         spirv_cross::CompilerGLSL compiler(std::move(spirv));
         compiler.set_common_options(options);

src/dawn_native/vulkan/ShaderModuleVk.cpp

@@ -14,6 +14,7 @@
 
 #include "dawn_native/vulkan/ShaderModuleVk.h"
 
+#include "dawn_native/SpirvValidation.h"
 #include "dawn_native/TintUtils.h"
 #include "dawn_native/vulkan/BindGroupLayoutVk.h"
 #include "dawn_native/vulkan/DeviceVk.h"
@@ -79,64 +80,24 @@ namespace dawn_native { namespace vulkan {
     }
 
     MaybeError ShaderModule::Initialize(ShaderModuleParseResult* parseResult) {
-        std::vector<uint32_t> spirv;
+        if (GetDevice()->IsRobustnessEnabled()) {
-        const std::vector<uint32_t>* spirvPtr;
-
             ScopedTintICEHandler scopedICEHandler(GetDevice());
 
-        std::ostringstream errorStream;
+            tint::transform::BoundArrayAccessors boundArrayAccessors;
-        errorStream << "Tint SPIR-V writer failure:" << std::endl;
-
-        tint::transform::Manager transformManager;
-        if (GetDevice()->IsRobustnessEnabled()) {
-            transformManager.Add<tint::transform::BoundArrayAccessors>();
-        }
-
             tint::transform::DataMap transformInputs;
 
             tint::Program program;
-        DAWN_TRY_ASSIGN(program, RunTransforms(&transformManager, parseResult->tintProgram.get(),
+            DAWN_TRY_ASSIGN(program,
+                            RunTransforms(&boundArrayAccessors, parseResult->tintProgram.get(),
                                           transformInputs, nullptr, nullptr));
-
-        tint::writer::spirv::Options options;
-        options.emit_vertex_point_size = true;
-        options.disable_workgroup_init = GetDevice()->IsToggleEnabled(Toggle::DisableWorkgroupInit);
-        auto result = tint::writer::spirv::Generate(&program, options);
-        if (!result.success) {
-            errorStream << "Generator: " << result.error << std::endl;
-            return DAWN_VALIDATION_ERROR(errorStream.str().c_str());
-        }
-
-        spirv = std::move(result.spirv);
-        spirvPtr = &spirv;
-
             // Rather than use a new ParseResult object, we just reuse the original parseResult
             parseResult->tintProgram = std::make_unique<tint::Program>(std::move(program));
-
-        DAWN_TRY(InitializeBase(parseResult));
-
-        VkShaderModuleCreateInfo createInfo;
-        createInfo.sType = VK_STRUCTURE_TYPE_SHADER_MODULE_CREATE_INFO;
-        createInfo.pNext = nullptr;
-        createInfo.flags = 0;
-        std::vector<uint32_t> vulkanSource;
-        createInfo.codeSize = spirvPtr->size() * sizeof(uint32_t);
-        createInfo.pCode = spirvPtr->data();
-
-        Device* device = ToBackend(GetDevice());
-        return CheckVkSuccess(
-            device->fn.CreateShaderModule(device->GetVkDevice(), &createInfo, nullptr, &*mHandle),
-            "CreateShaderModule");
         }
 
-    ShaderModule::~ShaderModule() {
+        return InitializeBase(parseResult);
-        Device* device = ToBackend(GetDevice());
+    }
 
-        if (mHandle != VK_NULL_HANDLE) {
+    ShaderModule::~ShaderModule() = default;
-            device->GetFencedDeleter()->DeleteWhenUnused(mHandle);
-            mHandle = VK_NULL_HANDLE;
-        }
-    }
 
     ResultOrError<VkShaderModule> ShaderModule::GetTransformedModuleHandle(
         const char* entryPointName,
@@ -204,28 +165,14 @@ namespace dawn_native { namespace vulkan {
             return DAWN_VALIDATION_ERROR(errorStream.str().c_str());
         }
 
-        std::vector<uint32_t> spirv = result.spirv;
+        std::vector<uint32_t> spirv = std::move(result.spirv);
+        DAWN_TRY(
+            ValidateSpirv(GetDevice(), spirv, GetDevice()->IsToggleEnabled(Toggle::DumpShaders)));
 
-        if (GetDevice()->IsToggleEnabled(Toggle::DumpShaders)) {
-            spvtools::SpirvTools spirvTools(SPV_ENV_VULKAN_1_1);
-            std::ostringstream dumpedMsg;
-            std::string disassembly;
-            if (spirvTools.Disassemble(
-                    result.spirv, &disassembly,
-                    SPV_BINARY_TO_TEXT_OPTION_FRIENDLY_NAMES | SPV_BINARY_TO_TEXT_OPTION_INDENT)) {
-                dumpedMsg << "/* Dumped generated SPIRV disassembly */" << std::endl << disassembly;
-            } else {
-                dumpedMsg << "/* Failed to disassemble generated SPIRV */";
-            }
-            GetDevice()->EmitLog(WGPULoggingType_Info, dumpedMsg.str().c_str());
-        }
-
-        // Don't save the transformedParseResult but just create a VkShaderModule
         VkShaderModuleCreateInfo createInfo;
         createInfo.sType = VK_STRUCTURE_TYPE_SHADER_MODULE_CREATE_INFO;
         createInfo.pNext = nullptr;
         createInfo.flags = 0;
-        std::vector<uint32_t> vulkanSource;
         createInfo.codeSize = spirv.size() * sizeof(uint32_t);
         createInfo.pCode = spirv.data();
 

src/dawn_native/vulkan/ShaderModuleVk.h

@@ -41,8 +41,6 @@ namespace dawn_native { namespace vulkan {
         ~ShaderModule() override;
         MaybeError Initialize(ShaderModuleParseResult* parseResult);
 
-        VkShaderModule mHandle = VK_NULL_HANDLE;
-
         // New handles created by GetTransformedModuleHandle at pipeline creation time
         class ConcurrentTransformedShaderModuleCache {
           public: