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>
diff --git a/src/dawn_native/BUILD.gn b/src/dawn_native/BUILD.gn
index d44de36..ee59d23 100644
--- a/src/dawn_native/BUILD.gn
+++ b/src/dawn_native/BUILD.gn
@@ -474,6 +474,13 @@
]
}
+ if (dawn_enable_opengl || dawn_enable_vulkan) {
+ sources += [
+ "SpirvValidation.cpp",
+ "SpirvValidation.h",
+ ]
+ }
+
if (dawn_enable_opengl) {
public_deps += [
":dawn_native_opengl_loader_gen",
diff --git a/src/dawn_native/CMakeLists.txt b/src/dawn_native/CMakeLists.txt
index 84417d4..3445757 100644
--- a/src/dawn_native/CMakeLists.txt
+++ b/src/dawn_native/CMakeLists.txt
@@ -194,7 +194,7 @@
endif()
# Only win32 app needs to link with user32.lib
-# In UWP, all availiable APIs are defined in WindowsApp.lib
+# In UWP, all availiable APIs are defined in WindowsApp.lib
# and is automatically linked when WINDOWS_STORE set
if (WIN32 AND NOT WINDOWS_STORE)
target_link_libraries(dawn_native PRIVATE user32.lib)
@@ -354,6 +354,13 @@
)
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"
diff --git a/src/dawn_native/SpirvValidation.cpp b/src/dawn_native/SpirvValidation.cpp
new file mode 100644
index 0000000..bec3a23
--- /dev/null
+++ b/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
diff --git a/src/dawn_native/SpirvValidation.h b/src/dawn_native/SpirvValidation.h
new file mode 100644
index 0000000..b22fd06
--- /dev/null
+++ b/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
diff --git a/src/dawn_native/opengl/ShaderModuleGL.cpp b/src/dawn_native/opengl/ShaderModuleGL.cpp
index 5d9b957..1ea18a3 100644
--- a/src/dawn_native/opengl/ShaderModuleGL.cpp
+++ b/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 @@
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 @@
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)) {
- spvtools::SpirvTools spirvTools(SPV_ENV_VULKAN_1_1);
- 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 */";
- }
- GetDevice()->EmitLog(WGPULoggingType_Info, dumpedMsg.str().c_str());
+ // 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;
spirv_cross::CompilerGLSL compiler(std::move(spirv));
compiler.set_common_options(options);
diff --git a/src/dawn_native/vulkan/ShaderModuleVk.cpp b/src/dawn_native/vulkan/ShaderModuleVk.cpp
index 5142951..0282b77 100644
--- a/src/dawn_native/vulkan/ShaderModuleVk.cpp
+++ b/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 @@
}
MaybeError ShaderModule::Initialize(ShaderModuleParseResult* parseResult) {
- std::vector<uint32_t> spirv;
- const std::vector<uint32_t>* spirvPtr;
-
- ScopedTintICEHandler scopedICEHandler(GetDevice());
-
- std::ostringstream errorStream;
- errorStream << "Tint SPIR-V writer failure:" << std::endl;
-
- tint::transform::Manager transformManager;
if (GetDevice()->IsRobustnessEnabled()) {
- transformManager.Add<tint::transform::BoundArrayAccessors>();
+ ScopedTintICEHandler scopedICEHandler(GetDevice());
+
+ tint::transform::BoundArrayAccessors boundArrayAccessors;
+ tint::transform::DataMap transformInputs;
+
+ tint::Program program;
+ DAWN_TRY_ASSIGN(program,
+ RunTransforms(&boundArrayAccessors, parseResult->tintProgram.get(),
+ transformInputs, nullptr, nullptr));
+ // Rather than use a new ParseResult object, we just reuse the original parseResult
+ parseResult->tintProgram = std::make_unique<tint::Program>(std::move(program));
}
- tint::transform::DataMap transformInputs;
-
- tint::Program program;
- DAWN_TRY_ASSIGN(program, RunTransforms(&transformManager, 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");
+ return InitializeBase(parseResult);
}
- ShaderModule::~ShaderModule() {
- Device* device = ToBackend(GetDevice());
-
- if (mHandle != VK_NULL_HANDLE) {
- device->GetFencedDeleter()->DeleteWhenUnused(mHandle);
- mHandle = VK_NULL_HANDLE;
- }
- }
+ ShaderModule::~ShaderModule() = default;
ResultOrError<VkShaderModule> ShaderModule::GetTransformedModuleHandle(
const char* entryPointName,
@@ -204,28 +165,14 @@
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();
diff --git a/src/dawn_native/vulkan/ShaderModuleVk.h b/src/dawn_native/vulkan/ShaderModuleVk.h
index bbb7300..b0a7bc3 100644
--- a/src/dawn_native/vulkan/ShaderModuleVk.h
+++ b/src/dawn_native/vulkan/ShaderModuleVk.h
@@ -41,8 +41,6 @@
~ShaderModule() override;
MaybeError Initialize(ShaderModuleParseResult* parseResult);
- VkShaderModule mHandle = VK_NULL_HANDLE;
-
// New handles created by GetTransformedModuleHandle at pipeline creation time
class ConcurrentTransformedShaderModuleCache {
public:
