Improve variable pointer validation (#6595)

Fixes #6532

* Check through types to ensure only valid pointers are allocated in
logical variables
* Fix up some invalid tests

Author: alan-baker

source/val/validate_memory.cpp

@@ -619,35 +619,54 @@ spv_result_t ValidateVariablePointer(ValidationState_t& _,
   if ((_.addressing_model() == spv::AddressingModel::Logical ||
        _.addressing_model() == spv::AddressingModel::PhysicalStorageBuffer64) &&
       !_.options()->relax_logical_pointer) {
-    if ((pointee.opcode() == spv::Op::OpTypePointer ||
-         pointee.opcode() == spv::Op::OpTypeUntypedPointerKHR)) {
-      const auto sc = pointee.GetOperandAs<spv::StorageClass>(1u);
-      if (sc != spv::StorageClass::PhysicalStorageBuffer) {
-        if (sc != spv::StorageClass::StorageBuffer &&
-            sc != spv::StorageClass::Workgroup) {
-          return _.diag(SPV_ERROR_INVALID_ID, inst)
-                 << "In Logical addressing, variables can only allocate a "
-                    "pointer to the StorageBuffer or Workgroup storage classes";
-        } else if (!_.HasCapability(
-                       spv::Capability::VariablePointersStorageBuffer) &&
-                   sc == spv::StorageClass::StorageBuffer) {
-          return _.diag(SPV_ERROR_INVALID_ID, inst)
-                 << "In Logical addressing, variables can only allocate a "
-                    "storage buffer pointer if the "
-                    "VariablePointersStorageBuffer capability is declared";
-        } else if (!_.HasCapability(spv::Capability::VariablePointers) &&
-                   sc == spv::StorageClass::Workgroup) {
-          return _.diag(SPV_ERROR_INVALID_ID, inst)
-                 << "In Logical addressing, variables can only allocate a "
-                    "workgroup pointer if the VariablePointers capability is "
-                    "declared";
-        } else if (storage_class != spv::StorageClass::Function &&
-                   storage_class != spv::StorageClass::Private) {
-          return _.diag(SPV_ERROR_INVALID_ID, inst)
-                 << "In Logical addressing with variable pointers, variables "
-                 << "that allocate pointers must be in Function or Private "
-                 << "storage classes";
-        }
+    spv_result_t error = SPV_SUCCESS;
+    bool contains_logical_pointer = _.ContainsType(
+        pointee.id(),
+        [&_, inst, &error](const Instruction* type) {
+          if (type->opcode() == spv::Op::OpTypePointer ||
+              type->opcode() == spv::Op::OpTypeUntypedPointerKHR) {
+            const auto sc = type->GetOperandAs<spv::StorageClass>(1u);
+            if (sc != spv::StorageClass::PhysicalStorageBuffer) {
+              if (sc != spv::StorageClass::StorageBuffer &&
+                  sc != spv::StorageClass::Workgroup) {
+                error =
+                    _.diag(SPV_ERROR_INVALID_ID, inst)
+                    << "In Logical addressing, variables can only allocate a "
+                       "pointer to the StorageBuffer or Workgroup storage "
+                       "classes";
+              } else if (!_.HasCapability(
+                             spv::Capability::VariablePointersStorageBuffer) &&
+                         sc == spv::StorageClass::StorageBuffer) {
+                error =
+                    _.diag(SPV_ERROR_INVALID_ID, inst)
+                    << "In Logical addressing, variables can only allocate a "
+                       "storage buffer pointer if the "
+                       "VariablePointersStorageBuffer capability is declared";
+              } else if (!_.HasCapability(spv::Capability::VariablePointers) &&
+                         sc == spv::StorageClass::Workgroup) {
+                error =
+                    _.diag(SPV_ERROR_INVALID_ID, inst)
+                    << "In Logical addressing, variables can only allocate a "
+                       "workgroup pointer if the VariablePointers capability "
+                       "is "
+                       "declared";
+              }
+              return true;
+            }
+          }
+          return false;
+        },
+        /* traverse_all_types = */ false);
+
+    if (error != SPV_SUCCESS) return error;
+
+    if (contains_logical_pointer) {
+      if (storage_class != spv::StorageClass::Function &&
+          storage_class != spv::StorageClass::Private) {
+        return _.diag(SPV_ERROR_INVALID_ID, inst)
+               << "In Logical addressing with variable pointers, variables "
+               << "that allocate pointers must be in Function or Private "
+               << "storage classes";
       }
     }
   }

test/val/val_id_test.cpp

@@ -2541,11 +2541,11 @@ OpFunctionEnd
 TEST_P(ValidateIdWithMessage, OpVariableContainsBoolPointerGood) {
   std::string spirv = kGLSL450MemoryModel + R"(
 %bool = OpTypeBool
-%boolptr = OpTypePointer Uniform %bool
+%boolptr = OpTypePointer Workgroup %bool
 %int = OpTypeInt 32 0
 %block = OpTypeStruct %boolptr %int
-%_ptr_Uniform_block = OpTypePointer Uniform %block
-%var = OpVariable %_ptr_Uniform_block Uniform
+%_ptr_Private_block = OpTypePointer Private %block
+%var = OpVariable %_ptr_Private_block Private
 %void = OpTypeVoid
 %fnty = OpTypeFunction %void
 %main = OpFunction %void None %fnty
@@ -2801,6 +2801,11 @@ OpMemoryModel Logical GLSL450
 %_ptr_workgroup_int = OpTypePointer Workgroup %int
 %_ptr_uniform_ptr = OpTypePointer Uniform %_ptr_workgroup_int
 %var = OpVariable %_ptr_uniform_ptr Uniform
+%voidfn = OpTypeFunction %void
+%func = OpFunction %void None %voidfn
+%entry = OpLabel
+OpReturn
+OpFunctionEnd
 )";
 
   CompileSuccessfully(spirv);

test/val/val_interfaces_test.cpp

@@ -1640,7 +1640,7 @@ OpFunctionEnd
           "Interface struct has no Block decoration but has BuiltIn members."));
 }
 
-TEST_F(ValidateInterfacesTest, InvalidLocationTypePointer) {
+TEST_F(ValidateInterfacesTest, InvalidLocationTypeSampler) {
   const std::string text = R"(
                OpCapability Shader
                OpMemoryModel Logical Simple
@@ -1649,14 +1649,13 @@ TEST_F(ValidateInterfacesTest, InvalidLocationTypePointer) {
        %void = OpTypeVoid
           %5 = OpTypeFunction %void
       %float = OpTypeFloat 32
-%_ptr_Private_void = OpTypePointer Private %void
+      %sampler = OpTypeSampler
        %uint = OpTypeInt 32 0
 %uint_4278132784 = OpConstant %uint 4278132784
-%_arr__ptr_Private_void_uint_4278132784 = OpTypeArray %_ptr_Private_void %uint_4278132784
-%_ptr_Output__arr__ptr_Private_void_uint_4278132784 = OpTypePointer Output %_arr__ptr_Private_void_uint_4278132784
-          %2 = OpVariable %_ptr_Output__arr__ptr_Private_void_uint_4278132784 Output
-%_ptr_Output__ptr_Private_void = OpTypePointer Output %_ptr_Private_void
-          %3 = OpVariable %_ptr_Output__arr__ptr_Private_void_uint_4278132784 Output
+%_arr__sampler_uint_4278132784 = OpTypeArray %sampler %uint_4278132784
+%_ptr_Output__arr__sampler_uint_4278132784 = OpTypePointer Output %_arr__sampler_uint_4278132784
+          %2 = OpVariable %_ptr_Output__arr__sampler_uint_4278132784 Output
+          %3 = OpVariable %_ptr_Output__arr__sampler_uint_4278132784 Output
           %1 = OpFunction %void None %5
          %15 = OpLabel
                OpReturn

test/val/val_memory_test.cpp

@@ -8931,6 +8931,84 @@ OpFunctionEnd
           "allocate pointers must be in Function or Private storage classes"));
 }
 
+TEST_F(ValidateMemory, VariablePointerInStruct) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability VariablePointers
+OpExtension "SPV_KHR_variable_pointers"
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %main "main"
+OpExecutionMode %main LocalSize 1 1 1
+%void = OpTypeVoid
+%void_fn = OpTypeFunction %void
+%int = OpTypeInt 32 0
+%ptr_s = OpTypePointer Workgroup %int
+%struct_t = OpTypeStruct %ptr_s
+%ptr_struct = OpTypePointer Function %struct_t
+%main = OpFunction %void None %void_fn
+%entry = OpLabel
+%var = OpVariable %ptr_struct Function
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv, SPV_ENV_UNIVERSAL_1_3);
+  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_UNIVERSAL_1_3));
+}
+
+TEST_F(ValidateMemory, VariablePointerInArray) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability VariablePointers
+OpExtension "SPV_KHR_variable_pointers"
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %main "main"
+OpExecutionMode %main LocalSize 1 1 1
+%void = OpTypeVoid
+%void_fn = OpTypeFunction %void
+%int = OpTypeInt 32 0
+%int_4 = OpConstant %int 4
+%ptr_s = OpTypePointer Workgroup %int
+%array_t = OpTypeArray %ptr_s %int_4
+%ptr_array = OpTypePointer Function %array_t
+%main = OpFunction %void None %void_fn
+%entry = OpLabel
+%var = OpVariable %ptr_array Function
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv, SPV_ENV_UNIVERSAL_1_3);
+  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_UNIVERSAL_1_3));
+}
+
+TEST_F(ValidateMemory, NoVariablePointerInStruct) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %main "main"
+OpExecutionMode %main LocalSize 1 1 1
+%void = OpTypeVoid
+%void_fn = OpTypeFunction %void
+%int = OpTypeInt 32 0
+%ptr_f = OpTypePointer Function %int
+%struct_t = OpTypeStruct %ptr_f
+%ptr_struct = OpTypePointer Function %struct_t
+%main = OpFunction %void None %void_fn
+%entry = OpLabel
+%var = OpVariable %ptr_struct Function
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv, SPV_ENV_UNIVERSAL_1_3);
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_UNIVERSAL_1_3));
+  EXPECT_THAT(
+      getDiagnosticString(),
+      HasSubstr("In Logical addressing, variables can only allocate a pointer "
+                "to the StorageBuffer or Workgroup storage classes"));
+}
+
 TEST_F(ValidateMemory, VariablePointerBadStorageClassUntyped) {
   const std::string spirv = R"(
 OpCapability Shader