val: interface struct with builtins must be Block (#4665)

For a shader input/output interface variable of structure type:
If the structure has BuiltIn members, then the structure type
must be Block decorated.

Otherwise, the variable, or the struct members must have locations,
but nothing can have both a location be a BuiltIn.

Implements validation needed to reject the example in
KhronosGroup/SPIRV-Registry#134

source/val/validate_decorations.cpp

@@ -98,6 +98,14 @@ bool isBuiltInStruct(uint32_t struct_id, ValidationState_t& vstate) {
       });
 }
 
+// Returns true if the given structure type has a Block decoration.
+bool isBlock(uint32_t struct_id, ValidationState_t& vstate) {
+  const auto& decorations = vstate.id_decorations(struct_id);
+  return std::any_of(
+      decorations.begin(), decorations.end(),
+      [](const Decoration& d) { return SpvDecorationBlock == d.dec_type(); });
+}
+
 // Returns true if the given ID has the Import LinkageAttributes decoration.
 bool hasImportLinkageAttribute(uint32_t id, ValidationState_t& vstate) {
   const auto& decorations = vstate.id_decorations(id);
@@ -716,8 +724,8 @@ spv_result_t CheckBuiltInVariable(uint32_t var_id, ValidationState_t& vstate) {
 spv_result_t CheckDecorationsOfEntryPoints(ValidationState_t& vstate) {
   for (uint32_t entry_point : vstate.entry_points()) {
     const auto& descs = vstate.entry_point_descriptions(entry_point);
-    int num_builtin_inputs = 0;
-    int num_builtin_outputs = 0;
+    int num_builtin_block_inputs = 0;
+    int num_builtin_block_outputs = 0;
     int num_workgroup_variables = 0;
     int num_workgroup_variables_with_block = 0;
     int num_workgroup_variables_with_aliased = 0;
@@ -768,9 +776,20 @@ spv_result_t CheckDecorationsOfEntryPoints(ValidationState_t& vstate) {
         Instruction* type_instr = vstate.FindDef(type_id);
         if (type_instr && SpvOpTypeStruct == type_instr->opcode() &&
             isBuiltInStruct(type_id, vstate)) {
-          if (storage_class == SpvStorageClassInput) ++num_builtin_inputs;
-          if (storage_class == SpvStorageClassOutput) ++num_builtin_outputs;
-          if (num_builtin_inputs > 1 || num_builtin_outputs > 1) break;
+          if (!isBlock(type_id, vstate)) {
+            return vstate.diag(SPV_ERROR_INVALID_DATA, vstate.FindDef(type_id))
+                   << vstate.VkErrorID(4919)
+                   << "Interface struct has no Block decoration but has "
+                      "BuiltIn members. "
+                      "Location decorations must be used on each member of "
+                      "OpVariable with a structure type that is a block not "
+                      "decorated with Location.";
+          }
+          if (storage_class == SpvStorageClassInput) ++num_builtin_block_inputs;
+          if (storage_class == SpvStorageClassOutput)
+            ++num_builtin_block_outputs;
+          if (num_builtin_block_inputs > 1 || num_builtin_block_outputs > 1)
+            break;
           if (auto error = CheckBuiltInVariable(interface, vstate))
             return error;
         } else if (isBuiltInVar(interface, vstate)) {
@@ -788,7 +807,7 @@ spv_result_t CheckDecorationsOfEntryPoints(ValidationState_t& vstate) {
           }
         }
       }
-      if (num_builtin_inputs > 1 || num_builtin_outputs > 1) {
+      if (num_builtin_block_inputs > 1 || num_builtin_block_outputs > 1) {
         return vstate.diag(SPV_ERROR_INVALID_BINARY,
                            vstate.FindDef(entry_point))
                << "There must be at most one object per Storage Class that can "

test/val/val_builtins_test.cpp

@@ -93,7 +93,8 @@ CodeGenerator GetInMainCodeGenerator(const char* const built_in,
     generator.extensions_ += extensions;
   }
 
-  generator.before_types_ = "OpMemberDecorate %built_in_type 0 BuiltIn ";
+  generator.before_types_ = R"(OpDecorate %built_in_type Block
+                               OpMemberDecorate %built_in_type 0 BuiltIn )";
   generator.before_types_ += built_in;
   generator.before_types_ += "\n";
 
@@ -251,7 +252,8 @@ CodeGenerator GetInFunctionCodeGenerator(const char* const built_in,
     generator.extensions_ += extensions;
   }
 
-  generator.before_types_ = "OpMemberDecorate %built_in_type 0 BuiltIn ";
+  generator.before_types_ = R"(OpDecorate %built_in_type Block
+                              OpMemberDecorate %built_in_type 0 BuiltIn )";
   generator.before_types_ += built_in;
   generator.before_types_ += "\n";
 
@@ -3098,6 +3100,8 @@ TEST_F(ValidateBuiltIns, TwoBuiltInsFirstFails) {
   CodeGenerator generator = CodeGenerator::GetDefaultShaderCodeGenerator();
 
   generator.before_types_ = R"(
+OpDecorate %input_type Block
+OpDecorate %output_type Block
 OpMemberDecorate %input_type 0 BuiltIn FragCoord
 OpMemberDecorate %output_type 0 BuiltIn Position
 )";
@@ -3138,6 +3142,8 @@ TEST_F(ValidateBuiltIns, TwoBuiltInsSecondFails) {
   CodeGenerator generator = CodeGenerator::GetDefaultShaderCodeGenerator();
 
   generator.before_types_ = R"(
+OpDecorate %input_type Block
+OpDecorate %output_type Block
 OpMemberDecorate %input_type 0 BuiltIn Position
 OpMemberDecorate %output_type 0 BuiltIn FragCoord
 )";
@@ -3201,6 +3207,7 @@ OpStore %position %f32vec4_0123
 TEST_F(ValidateBuiltIns, FragmentPositionTwoEntryPoints) {
   CodeGenerator generator = CodeGenerator::GetDefaultShaderCodeGenerator();
   generator.before_types_ = R"(
+OpDecorate %output_type Block
 OpMemberDecorate %output_type 0 BuiltIn Position
 )";
 
@@ -3252,6 +3259,7 @@ CodeGenerator GetNoDepthReplacingGenerator() {
   CodeGenerator generator = CodeGenerator::GetDefaultShaderCodeGenerator();
 
   generator.before_types_ = R"(
+OpDecorate %output_type Block
 OpMemberDecorate %output_type 0 BuiltIn FragDepth
 )";
 
@@ -3303,6 +3311,7 @@ CodeGenerator GetOneMainHasDepthReplacingOtherHasntGenerator() {
   CodeGenerator generator = CodeGenerator::GetDefaultShaderCodeGenerator();
 
   generator.before_types_ = R"(
+OpDecorate %output_type Block
 OpMemberDecorate %output_type 0 BuiltIn FragDepth
 )";
 
@@ -3374,6 +3383,7 @@ OpExtension "SPV_NV_ray_tracing"
 )";
 
   generator.before_types_ = R"(
+OpDecorate %input_type Block
 OpMemberDecorate %input_type 0 BuiltIn InstanceId
 )";
 
@@ -3609,6 +3619,7 @@ OpCapability GroupNonUniformBallot
 OpMemoryModel Logical GLSL450
 OpEntryPoint GLCompute %foo "foo"
 OpExecutionMode %foo LocalSize 1 1 1
+OpDecorate %struct Block
 OpMemberDecorate %struct 0 BuiltIn SubgroupEqMask
 %void = OpTypeVoid
 %int = OpTypeInt 32 0
@@ -3663,6 +3674,7 @@ OpCapability GroupNonUniform
 OpMemoryModel Logical GLSL450
 OpEntryPoint GLCompute %foo "foo"
 OpExecutionMode %foo LocalSize 1 1 1
+OpDecorate %struct Block
 OpMemberDecorate %struct 0 BuiltIn SubgroupSize
 %void = OpTypeVoid
 %int = OpTypeInt 32 0
@@ -3723,6 +3735,7 @@ OpCapability GroupNonUniform
 OpMemoryModel Logical GLSL450
 OpEntryPoint GLCompute %foo "foo"
 OpExecutionMode %foo LocalSize 1 1 1
+OpDecorate %struct Block
 OpMemberDecorate %struct 0 BuiltIn SubgroupId
 %void = OpTypeVoid
 %int = OpTypeInt 32 0

test/val/val_decoration_test.cpp

@@ -226,6 +226,7 @@ TEST_F(ValidateDecorations, StructAllMembersHaveBuiltInDecorationsGood) {
                OpCapability Shader
                OpCapability Linkage
                OpMemoryModel Logical GLSL450
+               OpDecorate %_struct_1 Block
                OpMemberDecorate %_struct_1 0 BuiltIn Position
                OpMemberDecorate %_struct_1 1 BuiltIn Position
                OpMemberDecorate %_struct_1 2 BuiltIn Position
@@ -243,6 +244,7 @@ TEST_F(ValidateDecorations, MixedBuiltInDecorationsBad) {
                OpCapability Shader
                OpCapability Linkage
                OpMemoryModel Logical GLSL450
+               OpDecorate %_struct_1 Block
                OpMemberDecorate %_struct_1 0 BuiltIn Position
                OpMemberDecorate %_struct_1 1 BuiltIn Position
       %float = OpTypeFloat 32
@@ -265,6 +267,7 @@ TEST_F(ValidateDecorations, StructContainsBuiltInStructBad) {
                OpCapability Shader
                OpCapability Linkage
                OpMemoryModel Logical GLSL450
+               OpDecorate %_struct_1 Block
                OpMemberDecorate %_struct_1 0 BuiltIn Position
                OpMemberDecorate %_struct_1 1 BuiltIn Position
                OpMemberDecorate %_struct_1 2 BuiltIn Position
@@ -305,6 +308,8 @@ TEST_F(ValidateDecorations, MultipleBuiltInObjectsConsumedByOpEntryPointBad) {
                OpEntryPoint Geometry %main "main" %in_1 %in_2
                OpExecutionMode %main InputPoints
                OpExecutionMode %main OutputPoints
+               OpDecorate %struct_1 Block
+               OpDecorate %struct_2 Block
                OpMemberDecorate %struct_1 0 BuiltIn InvocationId
                OpMemberDecorate %struct_2 0 BuiltIn Position
       %int = OpTypeInt 32 1
@@ -339,6 +344,8 @@ TEST_F(ValidateDecorations,
                OpEntryPoint Geometry %main "main" %in_1 %out_1
                OpExecutionMode %main InputPoints
                OpExecutionMode %main OutputPoints
+               OpDecorate %struct_1 Block
+               OpDecorate %struct_2 Block
                OpMemberDecorate %struct_1 0 BuiltIn InvocationId
                OpMemberDecorate %struct_2 0 BuiltIn Position
       %int = OpTypeInt 32 1

test/val/val_interfaces_test.cpp

@@ -1500,6 +1500,42 @@ OpFunctionEnd
   EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_VULKAN_1_0));
 }
 
+TEST_F(ValidateInterfacesTest, StructWithBuiltinsMissingBlock_Bad) {
+  // See https://github.com/KhronosGroup/SPIRV-Registry/issues/134
+  //
+  // When a shader input or output is a struct that does not have Block,
+  // then it must have a Location.
+  // But BuiltIns must not have locations.
+  const std::string text = R"(
+OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint Fragment %main "main" %in
+OpExecutionMode %main OriginUpperLeft
+; %struct needs a Block decoration
+OpMemberDecorate %struct 0 BuiltIn Position
+%void = OpTypeVoid
+%float = OpTypeFloat 32
+%v4float = OpTypeVector %float 4
+%struct = OpTypeStruct %v4float
+%in_ptr = OpTypePointer Input %struct
+%in = OpVariable %in_ptr Input
+%void_fn = OpTypeFunction %void
+%main = OpFunction %void None %void_fn
+%entry = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(text, SPV_ENV_VULKAN_1_0);
+  EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_0));
+  EXPECT_THAT(getDiagnosticString(),
+              AnyVUID("VUID-StandaloneSpirv-Location-04919"));
+  EXPECT_THAT(
+      getDiagnosticString(),
+      HasSubstr(
+          "Interface struct has no Block decoration but has BuiltIn members."));
+}
+
 }  // namespace
 }  // namespace val
 }  // namespace spvtools