Ensure for OpVariable that result type and storage class operand agree (

#2052)

From SPIR-V spec, section 3.32.8 on OpVariable:
  Its Storage Class operand must be the same as the Storage Class
  operand of the result type.

Fixes #941

source/val/validate_memory.cpp

@@ -301,7 +301,7 @@ spv_result_t ValidateVariable(ValidationState_t& _, const Instruction* inst) {
     const auto initializer = _.FindDef(initializer_id);
     const auto is_module_scope_var =
         initializer && (initializer->opcode() == SpvOpVariable) &&
-        (initializer->GetOperandAs<uint32_t>(storage_class_index) !=
+        (initializer->GetOperandAs<SpvStorageClass>(storage_class_index) !=
          SpvStorageClassFunction);
     const auto is_constant =
         initializer && spvOpcodeIsConstant(initializer->opcode());
@@ -312,7 +312,8 @@ spv_result_t ValidateVariable(ValidationState_t& _, const Instruction* inst) {
     }
   }
 
-  const auto storage_class = inst->GetOperandAs<uint32_t>(storage_class_index);
+  const auto storage_class =
+      inst->GetOperandAs<SpvStorageClass>(storage_class_index);
   if (storage_class != SpvStorageClassWorkgroup &&
       storage_class != SpvStorageClassCrossWorkgroup &&
       storage_class != SpvStorageClassPrivate &&
@@ -340,6 +341,18 @@ spv_result_t ValidateVariable(ValidationState_t& _, const Instruction* inst) {
     }
   }
 
+  // SPIR-V 3.32.8: Check that pointer type and variable type have the same
+  // storage class.
+  const auto result_storage_class_index = 1;
+  const auto result_storage_class =
+      result_type->GetOperandAs<uint32_t>(result_storage_class_index);
+  if (storage_class != result_storage_class) {
+    return _.diag(SPV_ERROR_INVALID_ID, inst)
+           << "From SPIR-V spec, section 3.32.8 on OpVariable:\n"
+           << "Its Storage Class operand must be the same as the Storage Class "
+           << "operand of the result type.";
+  }
+
   // Vulkan 14.5.2: Check type of UniformConstant and Uniform variables.
   if (spvIsVulkanEnv(_.context()->target_env)) {
     auto pointee = _.FindDef(result_type->word(3));

test/opt/ir_builder.cpp

@@ -177,7 +177,7 @@ TEST_F(IRBuilderTest, TestCondBranchAddition) {
           %5 = OpTypeVoid
           %6 = OpTypeFunction %5
           %7 = OpTypeBool
-          %8 = OpTypePointer Function %7
+          %8 = OpTypePointer Private %7
           %9 = OpConstantTrue %7
          %10 = OpTypeFloat 32
          %11 = OpTypeVector %10 4

test/val/val_id_test.cpp

@@ -2169,7 +2169,7 @@ TEST_F(ValidateIdWithMessage, OpStoreGood) {
 %3 = OpTypePointer Uniform %2
 %4 = OpTypeFunction %1
 %5 = OpConstant %2 42
-%6 = OpVariable %3 UniformConstant
+%6 = OpVariable %3 Uniform
 %7 = OpFunction %1 None %4
 %8 = OpLabel
      OpStore %6 %5
@@ -2264,7 +2264,7 @@ TEST_F(ValidateIdWithMessage, OpStoreObjectGood) {
 %3 = OpTypePointer Uniform %2
 %4 = OpTypeFunction %1
 %5 = OpConstant %2 42
-%6 = OpVariable %3 UniformConstant
+%6 = OpVariable %3 Uniform
 %7 = OpFunction %1 None %4
 %8 = OpLabel
 %9 = OpUndef %1
@@ -2284,7 +2284,7 @@ TEST_F(ValidateIdWithMessage, OpStoreTypeBad) {
 %3 = OpTypePointer Uniform %2
 %4 = OpTypeFunction %1
 %5 = OpConstant %9 3.14
-%6 = OpVariable %3 UniformConstant
+%6 = OpVariable %3 Uniform
 %7 = OpFunction %1 None %4
 %8 = OpLabel
      OpStore %6 %5
@@ -2549,7 +2549,7 @@ TEST_F(ValidateIdWithMessage, OpStoreVoid) {
 %2 = OpTypeInt 32 0
 %3 = OpTypePointer Uniform %2
 %4 = OpTypeFunction %1
-%6 = OpVariable %3 UniformConstant
+%6 = OpVariable %3 Uniform
 %7 = OpFunction %1 None %4
 %8 = OpLabel
 %9 = OpFunctionCall %1 %7
@@ -2568,7 +2568,7 @@ TEST_F(ValidateIdWithMessage, OpStoreLabel) {
 %2 = OpTypeInt 32 0
 %3 = OpTypePointer Uniform %2
 %4 = OpTypeFunction %1
-%6 = OpVariable %3 UniformConstant
+%6 = OpVariable %3 Uniform
 %7 = OpFunction %1 None %4
 %8 = OpLabel
      OpStore %6 %8
@@ -4647,7 +4647,7 @@ TEST_F(ValidateIdWithMessage, OpReturnValueIsVariableInPhysical) {
      OpMemoryModel Physical32 OpenCL
 %1 = OpTypeVoid
 %2 = OpTypeInt 32 0
-%3 = OpTypePointer Private %2
+%3 = OpTypePointer Function %2
 %4 = OpTypeFunction %3
 %5 = OpFunction %3 None %4
 %6 = OpLabel
@@ -4664,7 +4664,7 @@ TEST_F(ValidateIdWithMessage, OpReturnValueIsVariableInLogical) {
      OpMemoryModel Logical GLSL450
 %1 = OpTypeVoid
 %2 = OpTypeInt 32 0
-%3 = OpTypePointer Private %2
+%3 = OpTypePointer Function %2
 %4 = OpTypeFunction %3
 %5 = OpFunction %3 None %4
 %6 = OpLabel

test/val/val_memory_test.cpp

@@ -303,6 +303,52 @@ TEST_F(ValidateMemory, VulkanUniformOnArrayOfArrayBad) {
                 "must be typed as OpTypeStruct, or an array of this type"));
 }
 
+TEST_F(ValidateMemory, MismatchingStorageClassesBad) {
+  std::string spirv = R"(
+OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint Fragment %func "func"
+OpExecutionMode %func OriginUpperLeft
+%float = OpTypeFloat 32
+%float_ptr = OpTypePointer Uniform %float
+%void = OpTypeVoid
+%functy = OpTypeFunction %void
+%func = OpFunction %void None %functy
+%1 = OpLabel
+%2 = OpVariable %float_ptr Function
+OpReturn
+OpFunctionEnd
+)";
+  CompileSuccessfully(spirv.c_str());
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+  EXPECT_THAT(
+      getDiagnosticString(),
+      HasSubstr(
+          "From SPIR-V spec, section 3.32.8 on OpVariable:\n"
+          "Its Storage Class operand must be the same as the Storage Class "
+          "operand of the result type."));
+}
+
+TEST_F(ValidateMemory, MatchingStorageClassesGood) {
+  std::string spirv = R"(
+OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint Fragment %func "func"
+OpExecutionMode %func OriginUpperLeft
+%float = OpTypeFloat 32
+%float_ptr = OpTypePointer Function %float
+%void = OpTypeVoid
+%functy = OpTypeFunction %void
+%func = OpFunction %void None %functy
+%1 = OpLabel
+%2 = OpVariable %float_ptr Function
+OpReturn
+OpFunctionEnd
+)";
+  CompileSuccessfully(spirv.c_str());
+  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
+}
+
 }  // namespace
 }  // namespace val
 }  // namespace spvtools

test/val/val_ssa_test.cpp

@@ -1132,9 +1132,10 @@ TEST_F(ValidateSSA, IdDoesNotDominateItsUseBad) {
 TEST_F(ValidateSSA, PhiUseDoesntDominateDefinitionGood) {
   std::string str = kHeader + kBasicTypes +
                     R"(
+%funcintptrt = OpTypePointer Function %uintt
 %func        = OpFunction %voidt None %vfunct
 %entry       = OpLabel
-%var_one     = OpVariable %intptrt Function %one
+%var_one     = OpVariable %funcintptrt Function %one
 %one_val     = OpLoad %uintt %var_one
                OpBranch %loop
 %loop        = OpLabel

test/val/val_storage_test.cpp

@@ -73,29 +73,38 @@ TEST_F(ValidateStorage, FunctionStorageOutsideFunction) {
 
 TEST_F(ValidateStorage, OtherStorageOutsideFunction) {
   char str[] = R"(
-          OpCapability Shader
-          OpCapability Kernel
-          OpCapability AtomicStorage
-          OpCapability Linkage
-          OpMemoryModel Logical GLSL450
-%intt   = OpTypeInt 32 0
-%voidt  = OpTypeVoid
-%vfunct = OpTypeFunction %voidt
-%ptrt   = OpTypePointer Function %intt
-%unicon = OpVariable %ptrt UniformConstant
-%input  = OpVariable %ptrt Input
-%unif   = OpVariable %ptrt Uniform
-%output = OpVariable %ptrt Output
-%wgroup = OpVariable %ptrt Workgroup
-%xwgrp  = OpVariable %ptrt CrossWorkgroup
-%priv   = OpVariable %ptrt Private
-%pushco = OpVariable %ptrt PushConstant
-%atomct = OpVariable %ptrt AtomicCounter
-%image  = OpVariable %ptrt Image
-%func   = OpFunction %voidt None %vfunct
-%funcl  = OpLabel
-          OpReturn
-          OpFunctionEnd
+              OpCapability Shader
+              OpCapability Kernel
+              OpCapability AtomicStorage
+              OpCapability Linkage
+              OpMemoryModel Logical GLSL450
+%intt       = OpTypeInt 32 0
+%voidt      = OpTypeVoid
+%vfunct     = OpTypeFunction %voidt
+%uniconptrt = OpTypePointer UniformConstant %intt
+%unicon     = OpVariable %uniconptrt UniformConstant
+%inputptrt  = OpTypePointer Input %intt
+%input      = OpVariable %inputptrt Input
+%unifptrt   = OpTypePointer Uniform %intt
+%unif       = OpVariable %unifptrt Uniform
+%outputptrt = OpTypePointer Output %intt
+%output     = OpVariable %outputptrt Output
+%wgroupptrt = OpTypePointer Workgroup %intt
+%wgroup     = OpVariable %wgroupptrt Workgroup
+%xwgrpptrt  = OpTypePointer CrossWorkgroup %intt
+%xwgrp      = OpVariable %xwgrpptrt CrossWorkgroup
+%privptrt   = OpTypePointer Private %intt
+%priv       = OpVariable %privptrt Private
+%pushcoptrt = OpTypePointer PushConstant %intt
+%pushco     = OpVariable %pushcoptrt PushConstant
+%atomcptrt  = OpTypePointer AtomicCounter %intt
+%atomct     = OpVariable %atomcptrt AtomicCounter
+%imageptrt  = OpTypePointer Image %intt
+%image      = OpVariable %imageptrt Image
+%func       = OpFunction %voidt None %vfunct
+%funcl      = OpLabel
+              OpReturn
+              OpFunctionEnd
 )";
 
   CompileSuccessfully(str);