Extra resource interface validation (#2864)

* Vulkan specific checks * storage buffer variables must be structs or arrays of structs * storage buffer struct must be Block decorated * uniform struct must be Block or BufferBlock decorated * new tests
2024-10-18 19:20:05 +00:00 · 2019-09-16 10:46:31 -04:00 · 2019-09-16 10:46:31 -04:00 · 9325619353
commit 9325619353
parent 1e146e8a34
4 changed files with 371 additions and 5 deletions
--- a/source/val/validate_decorations.cpp
+++ b/source/val/validate_decorations.cpp
@ -942,6 +942,7 @@ spv_result_t CheckDecorationsOfBuffers(ValidationState_t& vstate) {
          id = id_inst->GetOperandAs<uint32_t>(1u);
          id_inst = vstate.FindDef(id);
        }
+        // Struct requirement is checked on variables so just move on here.
        if (SpvOpTypeStruct != id_inst->opcode()) continue;
        MemberConstraints constraints;
        ComputeMemberConstraintsForStruct(&constraints, id, LayoutConstraints(),
@ -952,15 +953,18 @@ spv_result_t CheckDecorationsOfBuffers(ValidationState_t& vstate) {
                    : (push_constant ? "PushConstant" : "StorageBuffer");

        if (spvIsVulkanEnv(vstate.context()->target_env)) {
-          if (storage_buffer &&
-              hasDecoration(id, SpvDecorationBufferBlock, vstate)) {
+          const bool block = hasDecoration(id, SpvDecorationBlock, vstate);
+          const bool buffer_block =
+              hasDecoration(id, SpvDecorationBufferBlock, vstate);
+          if (storage_buffer && buffer_block) {
            return vstate.diag(SPV_ERROR_INVALID_ID, vstate.FindDef(var_id))
                   << "Storage buffer id '" << var_id
                   << " In Vulkan, BufferBlock is disallowed on variables in "
                      "the StorageBuffer storage class";
          }
-          // Vulkan 14.5.1: Check Block decoration for PushConstant variables.
-          if (push_constant && !hasDecoration(id, SpvDecorationBlock, vstate)) {
+          // Vulkan 14.5.1/2: Check Block decoration for PushConstant, Uniform
+          // and StorageBuffer variables. Uniform can also use BufferBlock.
+          if (push_constant && !block) {
            return vstate.diag(SPV_ERROR_INVALID_ID, vstate.FindDef(id))
                   << "PushConstant id '" << id
                   << "' is missing Block decoration.\n"
@ -968,6 +972,22 @@ spv_result_t CheckDecorationsOfBuffers(ValidationState_t& vstate) {
                   << "Such variables must be identified with a Block "
                      "decoration";
          }
+          if (storage_buffer && !block) {
+            return vstate.diag(SPV_ERROR_INVALID_ID, vstate.FindDef(id))
+                   << "StorageBuffer id '" << id
+                   << "' is missing Block decoration.\n"
+                   << "From Vulkan spec, section 14.5.2:\n"
+                   << "Such variables must be identified with a Block "
+                      "decoration";
+          }
+          if (uniform && !block && !buffer_block) {
+            return vstate.diag(SPV_ERROR_INVALID_ID, vstate.FindDef(id))
+                   << "Uniform id '" << id
+                   << "' is missing Block or BufferBlock decoration.\n"
+                   << "From Vulkan spec, section 14.5.2:\n"
+                   << "Such variables must be identified with a Block or "
+                      "BufferBlock decoration";
+          }
          // Vulkan 14.5.2: Check DescriptorSet and Binding decoration for
          // Uniform and StorageBuffer variables.
          if (uniform || storage_buffer) {
--- a/source/val/validate_memory.cpp
+++ b/source/val/validate_memory.cpp
@ -536,6 +536,19 @@ spv_result_t ValidateVariable(ValidationState_t& _, const Instruction* inst) {
               << "this type";
      }
    }
+
+    if (storage_class == SpvStorageClassStorageBuffer) {
+      if (!IsAllowedTypeOrArrayOfSame(_, pointee, {SpvOpTypeStruct})) {
+        return _.diag(SPV_ERROR_INVALID_ID, inst)
+               << "StorageBuffer OpVariable <id> '" << _.getIdName(inst->id())
+               << "' has illegal type.\n"
+               << "From Vulkan spec, section 14.5.2:\n"
+               << "Variables identified with the StorageBuffer storage class "
+                  "are used to access transparent buffer backed resources. "
+                  "Such variables must be typed as OpTypeStruct, or an array "
+                  "of this type";
+      }
+    }
  }

  // WebGPU & Vulkan Appendix A: Check that if contains initializer, then
--- a/test/val/val_decoration_test.cpp
+++ b/test/val/val_decoration_test.cpp
@ -6691,6 +6691,245 @@ TEST_F(ValidateDecorations, ComponentDecorationFunctionParameter) {
  EXPECT_THAT(getDiagnosticString(), Eq(""));
 }

+TEST_F(ValidateDecorations, VulkanStorageBufferBlock) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpExtension "SPV_KHR_storage_buffer_storage_class"
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %main "main"
+OpExecutionMode %main LocalSize 1 1 1
+OpDecorate %struct Block
+OpMemberDecorate %struct 0 Offset 0
+%void = OpTypeVoid
+%uint = OpTypeInt 32 0
+%struct = OpTypeStruct %uint
+%ptr_ssbo = OpTypePointer StorageBuffer %struct
+%var = OpVariable %ptr_ssbo StorageBuffer
+%void_fn = OpTypeFunction %void
+%main = OpFunction %void None %void_fn
+%entry = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_0);
+  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_VULKAN_1_0));
+}
+
+TEST_F(ValidateDecorations, VulkanStorageBufferMissingBlock) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpExtension "SPV_KHR_storage_buffer_storage_class"
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %main "main"
+OpExecutionMode %main LocalSize 1 1 1
+%void = OpTypeVoid
+%uint = OpTypeInt 32 0
+%struct = OpTypeStruct %uint
+%ptr_ssbo = OpTypePointer StorageBuffer %struct
+%var = OpVariable %ptr_ssbo StorageBuffer
+%void_fn = OpTypeFunction %void
+%main = OpFunction %void None %void_fn
+%entry = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_0);
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_0));
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("From Vulkan spec, section 14.5.2:\nSuch variables "
+                        "must be identified with a Block decoration"));
+}
+
+TEST_F(ValidateDecorations, VulkanStorageBufferArrayMissingBlock) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpExtension "SPV_KHR_storage_buffer_storage_class"
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %main "main"
+OpExecutionMode %main LocalSize 1 1 1
+%void = OpTypeVoid
+%uint = OpTypeInt 32 0
+%uint_4 = OpConstant %uint 4
+%struct = OpTypeStruct %uint
+%array = OpTypeArray %struct %uint_4
+%ptr_ssbo = OpTypePointer StorageBuffer %array
+%var = OpVariable %ptr_ssbo StorageBuffer
+%void_fn = OpTypeFunction %void
+%main = OpFunction %void None %void_fn
+%entry = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_0);
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_0));
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("From Vulkan spec, section 14.5.2:\nSuch variables "
+                        "must be identified with a Block decoration"));
+}
+
+TEST_F(ValidateDecorations, VulkanStorageBufferRuntimeArrayMissingBlock) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability RuntimeDescriptorArrayEXT
+OpExtension "SPV_EXT_descriptor_indexing"
+OpExtension "SPV_KHR_storage_buffer_storage_class"
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %main "main"
+OpExecutionMode %main LocalSize 1 1 1
+%void = OpTypeVoid
+%uint = OpTypeInt 32 0
+%struct = OpTypeStruct %uint
+%array = OpTypeRuntimeArray %struct
+%ptr_ssbo = OpTypePointer StorageBuffer %array
+%var = OpVariable %ptr_ssbo StorageBuffer
+%void_fn = OpTypeFunction %void
+%main = OpFunction %void None %void_fn
+%entry = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_0);
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_0));
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("From Vulkan spec, section 14.5.2:\nSuch variables "
+                        "must be identified with a Block decoration"));
+}
+
+TEST_F(ValidateDecorations, VulkanUniformBlock) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %main "main"
+OpExecutionMode %main LocalSize 1 1 1
+OpDecorate %struct Block
+OpMemberDecorate %struct 0 Offset 0
+%void = OpTypeVoid
+%uint = OpTypeInt 32 0
+%struct = OpTypeStruct %uint
+%ptr_ubo = OpTypePointer Uniform %struct
+%var = OpVariable %ptr_ubo Uniform
+%void_fn = OpTypeFunction %void
+%main = OpFunction %void None %void_fn
+%entry = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_0);
+  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_VULKAN_1_0));
+}
+
+TEST_F(ValidateDecorations, VulkanUniformBufferBlock) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %main "main"
+OpExecutionMode %main LocalSize 1 1 1
+OpDecorate %struct BufferBlock
+OpMemberDecorate %struct 0 Offset 0
+%void = OpTypeVoid
+%uint = OpTypeInt 32 0
+%struct = OpTypeStruct %uint
+%ptr_ubo = OpTypePointer Uniform %struct
+%var = OpVariable %ptr_ubo Uniform
+%void_fn = OpTypeFunction %void
+%main = OpFunction %void None %void_fn
+%entry = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_0);
+  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_VULKAN_1_0));
+}
+
+TEST_F(ValidateDecorations, VulkanUniformMissingBlock) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %main "main"
+OpExecutionMode %main LocalSize 1 1 1
+%void = OpTypeVoid
+%uint = OpTypeInt 32 0
+%struct = OpTypeStruct %uint
+%ptr_ubo = OpTypePointer Uniform %struct
+%var = OpVariable %ptr_ubo Uniform
+%void_fn = OpTypeFunction %void
+%main = OpFunction %void None %void_fn
+%entry = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_0);
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_0));
+  EXPECT_THAT(
+      getDiagnosticString(),
+      HasSubstr("From Vulkan spec, section 14.5.2:\nSuch variables must be "
+                "identified with a Block or BufferBlock decoration"));
+}
+
+TEST_F(ValidateDecorations, VulkanUniformArrayMissingBlock) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %main "main"
+OpExecutionMode %main LocalSize 1 1 1
+%void = OpTypeVoid
+%uint = OpTypeInt 32 0
+%uint_4 = OpConstant %uint 4
+%struct = OpTypeStruct %uint
+%array = OpTypeArray %struct %uint_4
+%ptr_ubo = OpTypePointer Uniform %array
+%var = OpVariable %ptr_ubo Uniform
+%void_fn = OpTypeFunction %void
+%main = OpFunction %void None %void_fn
+%entry = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_0);
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_0));
+  EXPECT_THAT(
+      getDiagnosticString(),
+      HasSubstr("From Vulkan spec, section 14.5.2:\nSuch variables must be "
+                "identified with a Block or BufferBlock decoration"));
+}
+
+TEST_F(ValidateDecorations, VulkanUniformRuntimeArrayMissingBlock) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability RuntimeDescriptorArrayEXT
+OpExtension "SPV_EXT_descriptor_indexing"
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %main "main"
+OpExecutionMode %main LocalSize 1 1 1
+%void = OpTypeVoid
+%uint = OpTypeInt 32 0
+%struct = OpTypeStruct %uint
+%array = OpTypeRuntimeArray %struct
+%ptr_ubo = OpTypePointer Uniform %array
+%var = OpVariable %ptr_ubo Uniform
+%void_fn = OpTypeFunction %void
+%main = OpFunction %void None %void_fn
+%entry = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_0);
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_0));
+  EXPECT_THAT(
+      getDiagnosticString(),
+      HasSubstr("From Vulkan spec, section 14.5.2:\nSuch variables must be "
+                "identified with a Block or BufferBlock decoration"));
+}
+
 }  // namespace
 }  // namespace val
 }  // namespace spvtools
--- a/test/val/val_memory_test.cpp
+++ b/test/val/val_memory_test.cpp
@ -2318,9 +2318,14 @@ OpExtension "SPV_EXT_descriptor_indexing"
 OpMemoryModel Logical GLSL450
 OpEntryPoint Fragment %func "func"
 OpExecutionMode %func OriginUpperLeft
+OpDecorate %struct Block
+OpMemberDecorate %struct 0 Offset 0
 %sampler_t = OpTypeSampler
+%uint = OpTypeInt 32 0
 %array_t = OpTypeRuntimeArray %sampler_t
-%array_sb_ptr = OpTypePointer StorageBuffer %array_t
+%struct = OpTypeStruct %uint
+%sb_array_t = OpTypeRuntimeArray %struct
+%array_sb_ptr = OpTypePointer StorageBuffer %sb_array_t
 %2 = OpVariable %array_sb_ptr StorageBuffer
 %array_uc_ptr = OpTypePointer UniformConstant %array_t
 %3 = OpVariable %array_uc_ptr UniformConstant
@ -4292,6 +4297,95 @@ OpMemberDecorate %block 0 Offset 0
  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_UNIVERSAL_1_3));
 }

+TEST_F(ValidateMemory, VulkanStorageBufferNotAStruct) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpExtension "SPV_KHR_storage_buffer_storage_class"
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %main "main"
+OpExecutionMode %main LocalSize 1 1 1
+%void = OpTypeVoid
+%uint = OpTypeInt 32 0
+%ptr_ssbo = OpTypePointer StorageBuffer %uint
+%var = OpVariable %ptr_ssbo StorageBuffer
+%void_fn = OpTypeFunction %void
+%main = OpFunction %void None %void_fn
+%entry = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_0);
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_0));
+  EXPECT_THAT(
+      getDiagnosticString(),
+      HasSubstr("From Vulkan spec, section 14.5.2:\nVariables identified with "
+                "the StorageBuffer storage class are used to access "
+                "transparent buffer backed resources. Such variables must be "
+                "typed as OpTypeStruct, or an array of this type"));
+}
+
+TEST_F(ValidateMemory, VulkanStorageBufferRuntimeArrayNotAStruct) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability RuntimeDescriptorArrayEXT
+OpExtension "SPV_KHR_storage_buffer_storage_class"
+OpExtension "SPV_EXT_descriptor_indexing"
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %main "main"
+OpExecutionMode %main LocalSize 1 1 1
+%void = OpTypeVoid
+%uint = OpTypeInt 32 0
+%array = OpTypeRuntimeArray %uint
+%ptr_ssbo = OpTypePointer StorageBuffer %array
+%var = OpVariable %ptr_ssbo StorageBuffer
+%void_fn = OpTypeFunction %void
+%main = OpFunction %void None %void_fn
+%entry = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_0);
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_0));
+  EXPECT_THAT(
+      getDiagnosticString(),
+      HasSubstr("From Vulkan spec, section 14.5.2:\nVariables identified with "
+                "the StorageBuffer storage class are used to access "
+                "transparent buffer backed resources. Such variables must be "
+                "typed as OpTypeStruct, or an array of this type"));
+}
+
+TEST_F(ValidateMemory, VulkanStorageBufferArrayNotAStruct) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpExtension "SPV_KHR_storage_buffer_storage_class"
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %main "main"
+OpExecutionMode %main LocalSize 1 1 1
+%void = OpTypeVoid
+%uint = OpTypeInt 32 0
+%uint_4 = OpConstant %uint 4
+%array = OpTypeArray %uint %uint_4
+%ptr_ssbo = OpTypePointer StorageBuffer %array
+%var = OpVariable %ptr_ssbo StorageBuffer
+%void_fn = OpTypeFunction %void
+%main = OpFunction %void None %void_fn
+%entry = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_0);
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_0));
+  EXPECT_THAT(
+      getDiagnosticString(),
+      HasSubstr("From Vulkan spec, section 14.5.2:\nVariables identified with "
+                "the StorageBuffer storage class are used to access "
+                "transparent buffer backed resources. Such variables must be "
+                "typed as OpTypeStruct, or an array of this type"));
+}
+
 }  // namespace
 }  // namespace val
 }  // namespace spvtools