From 7fe3c3637689032bac5e8f71b02e4f356d27b571 Mon Sep 17 00:00:00 2001 From: David Neto Date: Fri, 26 Jun 2020 15:47:03 +0000 Subject: [PATCH] [spirv-reader] Weaken input validation to Vulkan 1.0 The process of passing the module through this reader, the WGSL semantics, and the SPIR-V writer will sanitize the module such that the end result should satisfy SPV_ENV_WEBGPU_0 requirements. Being more forgiving about the input SPIR-V will be a quality-of-life improvement. Bug: tint:3 Change-Id: Ib54cbf729b9e078d797a1ef31422bad497daa5a0 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/23942 Reviewed-by: dan sinclair Commit-Queue: dan sinclair --- src/reader/spirv/parser_impl.cc | 16 +++--- src/reader/spirv/parser_impl.h | 1 - src/reader/spirv/parser_impl_test.cc | 74 +++++++++++++++++++++++++++- 3 files changed, 82 insertions(+), 9 deletions(-) diff --git a/src/reader/spirv/parser_impl.cc b/src/reader/spirv/parser_impl.cc index 4ad72eeb19..e4c13278fa 100644 --- a/src/reader/spirv/parser_impl.cc +++ b/src/reader/spirv/parser_impl.cc @@ -73,7 +73,11 @@ namespace spirv { namespace { -const spv_target_env kTargetEnv = SPV_ENV_WEBGPU_0; +// Input SPIR-V needs only to conform to Vulkan 1.0 requirements. +// The combination of the SPIR-V reader and the semantics of WGSL +// tighten up the code so that the output of the SPIR-V *writer* +// will satisfy SPV_ENV_WEBGPU_0 validation. +const spv_target_env kInputEnv = SPV_ENV_VULKAN_1_0; // A FunctionTraverser is used to compute an ordering of functions in the // module such that callees precede callers. @@ -197,8 +201,7 @@ ParserImpl::ParserImpl(Context* ctx, const std::vector& spv_binary) bool_type_(ctx->type_mgr().Get(std::make_unique())), namer_(fail_stream_), enum_converter_(fail_stream_), - tools_context_(kTargetEnv), - tools_(kTargetEnv) { + tools_context_(kInputEnv) { // Create a message consumer to propagate error messages from SPIRV-Tools // out as our own failures. message_consumer_ = [this](spv_message_level_t level, const char* /*source*/, @@ -222,7 +225,7 @@ ParserImpl::~ParserImpl() = default; bool ParserImpl::Parse() { // Set up use of SPIRV-Tools utilities. - spvtools::SpirvTools spv_tools(kTargetEnv); + spvtools::SpirvTools spv_tools(kInputEnv); // Error messages from SPIRV-Tools are forwarded as failures, including // setting |success_| to false. @@ -232,8 +235,8 @@ bool ParserImpl::Parse() { return false; } - // Only consider valid modules. On failure, the message consumer - // will set the error status. + // Only consider modules valid for Vulkan 1.0. On failure, the message + // consumer will set the error status. if (!spv_tools.Validate(spv_binary_)) { return false; } @@ -380,7 +383,6 @@ bool ParserImpl::BuildInternalModule() { if (!success_) { return false; } - tools_.SetMessageConsumer(message_consumer_); const spv_context& context = tools_context_.CContext(); ir_context_ = spvtools::BuildModule(context->target_env, context->consumer, diff --git a/src/reader/spirv/parser_impl.h b/src/reader/spirv/parser_impl.h index b235b1b847..c4b717fbb7 100644 --- a/src/reader/spirv/parser_impl.h +++ b/src/reader/spirv/parser_impl.h @@ -364,7 +364,6 @@ class ParserImpl : Reader { // The internal representation of the SPIR-V module and its context. spvtools::Context tools_context_; - spvtools::SpirvTools tools_; // All the state is owned by ir_context_. std::unique_ptr ir_context_; // The following are borrowed pointers to the internal state of ir_context_. diff --git a/src/reader/spirv/parser_impl_test.cc b/src/reader/spirv/parser_impl_test.cc index fe8114ac37..bc80675e07 100644 --- a/src/reader/spirv/parser_impl_test.cc +++ b/src/reader/spirv/parser_impl_test.cc @@ -45,7 +45,79 @@ TEST_F(SpvParserTest, Impl_InvalidModuleFails) { EXPECT_THAT(p->error(), HasSubstr("OpTypeInt 3 0")); } -// TODO(dneto): uint32 vec, valid SPIR-V +TEST_F(SpvParserTest, Impl_GenericVulkanShader_SimpleMemoryModel) { + auto spv = test::Assemble(R"( + OpCapability Shader + OpMemoryModel Logical Simple + OpEntryPoint GLCompute %main "main" + OpExecutionMode %main LocalSize 1 1 1 + %void = OpTypeVoid + %voidfn = OpTypeFunction %void + %main = OpFunction %void None %voidfn + %entry = OpLabel + OpReturn + OpFunctionEnd +)"); + auto* p = parser(spv); + EXPECT_TRUE(p->Parse()); + EXPECT_TRUE(p->error().empty()); +} + +TEST_F(SpvParserTest, Impl_GenericVulkanShader_GLSL450MemoryModel) { + auto spv = test::Assemble(R"( + OpCapability Shader + OpMemoryModel Logical GLSL450 + OpEntryPoint GLCompute %main "main" + OpExecutionMode %main LocalSize 1 1 1 + %void = OpTypeVoid + %voidfn = OpTypeFunction %void + %main = OpFunction %void None %voidfn + %entry = OpLabel + OpReturn + OpFunctionEnd +)"); + auto* p = parser(spv); + EXPECT_TRUE(p->Parse()); + EXPECT_TRUE(p->error().empty()); +} + +TEST_F(SpvParserTest, Impl_GenericVulkanShader_VulkanMemoryModel) { + auto spv = test::Assemble(R"( + OpCapability Shader + OpCapability VulkanMemoryModelKHR + OpExtension "SPV_KHR_vulkan_memory_model" + OpMemoryModel Logical VulkanKHR + OpEntryPoint GLCompute %main "main" + OpExecutionMode %main LocalSize 1 1 1 + %void = OpTypeVoid + %voidfn = OpTypeFunction %void + %main = OpFunction %void None %voidfn + %entry = OpLabel + OpReturn + OpFunctionEnd +)"); + auto* p = parser(spv); + EXPECT_TRUE(p->Parse()); + EXPECT_TRUE(p->error().empty()); +} + +TEST_F(SpvParserTest, Impl_OpenCLKernel_Fails) { + auto spv = test::Assemble(R"( + OpCapability Kernel + OpCapability Addresses + OpMemoryModel Physical32 OpenCL + OpEntryPoint Kernel %main "main" + %void = OpTypeVoid + %voidfn = OpTypeFunction %void + %main = OpFunction %void None %voidfn + %entry = OpLabel + OpReturn + OpFunctionEnd +)"); + auto* p = parser(spv); + EXPECT_FALSE(p->Parse()); + EXPECT_THAT(p->error(), HasSubstr("Capability Kernel is not allowed")); +} } // namespace } // namespace spirv