Validate pointer variables (#2111)

Fixes #2104

* Checks the rules for logical addressing and variable pointers
 * Has an out for relaxed logical pointers
* Updated PassFixture to expose validator options
 * enabled relaxed logical pointers for some tests
* New validator tests

source/val/validate_memory.cpp

@@ -353,10 +353,28 @@ spv_result_t ValidateVariable(ValidationState_t& _, const Instruction* inst) {
            << "operand of the result type.";
   }
 
+  // Variable pointer related restrictions.
+  auto pointee = _.FindDef(result_type->word(3));
+  if (_.addressing_model() == SpvAddressingModelLogical &&
+      !_.options()->relax_logical_pointer) {
+    // VariablePointersStorageBuffer is implied by VariablePointers.
+    if (pointee->opcode() == SpvOpTypePointer) {
+      if (!_.HasCapability(SpvCapabilityVariablePointersStorageBuffer)) {
+        return _.diag(SPV_ERROR_INVALID_ID, inst) << "In Logical addressing, "
+                                                     "variables may not "
+                                                     "allocate a pointer type";
+      } else if (storage_class != SpvStorageClassFunction &&
+                 storage_class != SpvStorageClassPrivate) {
+        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";
+      }
+    }
+  }
+
   // Vulkan 14.5.2: Check type of UniformConstant and Uniform variables.
   if (spvIsVulkanEnv(_.context()->target_env)) {
-    auto pointee = _.FindDef(result_type->word(3));
-
     if (storage_class == SpvStorageClassUniformConstant) {
       if (!IsAllowedTypeOrArrayOfSame(
               _, pointee,

test/opt/aggressive_dead_code_elim_test.cpp

@@ -3597,6 +3597,8 @@ OpFunctionEnd
   SinglePassRunAndCheck<AggressiveDCEPass>(assembly, assembly, true, true);
 }
 
+// This is not valid input and ADCE does not support variable pointers and only
+// supports shaders.
 TEST_F(AggressiveDCETest, PointerVariable) {
   // ADCE is able to handle code that contains a load whose base address
   // comes from a load and not an OpVariable.  I want to see an instruction
@@ -3693,6 +3695,10 @@ OpReturn
 OpFunctionEnd
 )";
 
+  // The input is not valid and ADCE only supports shaders, but not variable
+  // pointers. Workaround this by enabling relaxed logical pointers in the
+  // validator.
+  ValidatorOptions()->relax_logical_pointer = true;
   SinglePassRunAndCheck<AggressiveDCEPass>(before, after, true, true);
 }
 

test/opt/local_single_block_elim.cpp

@@ -865,7 +865,9 @@ OpReturn
 OpFunctionEnd
 )";
 
+  // Relax logical pointers to allow pointer allocations.
   SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
+  ValidatorOptions()->relax_logical_pointer = true;
   SinglePassRunAndCheck<LocalSingleBlockLoadStoreElimPass>(before, after, true,
                                                            true);
 }

test/opt/local_single_store_elim_test.cpp

@@ -767,7 +767,9 @@ OpReturn
 OpFunctionEnd
 )";
 
+  // Relax logical pointers to allow pointer allocations.
   SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
+  ValidatorOptions()->relax_logical_pointer = true;
   SinglePassRunAndCheck<LocalSingleStoreElimPass>(before, after, true, true);
 }
 

test/opt/local_ssa_elim_test.cpp

@@ -1528,7 +1528,9 @@ OpReturn
 OpFunctionEnd
 )";
 
+  // Relax logical pointers to allow pointer allocations.
   SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
+  ValidatorOptions()->relax_logical_pointer = true;
   SinglePassRunAndCheck<LocalMultiStoreElimPass>(before, after, true, true);
 }
 

test/opt/pass_fixture.h

@@ -27,6 +27,7 @@
 #include "source/opt/build_module.h"
 #include "source/opt/pass_manager.h"
 #include "source/opt/passes.h"
+#include "source/spirv_validator_options.h"
 #include "source/util/make_unique.h"
 #include "spirv-tools/libspirv.hpp"
 
@@ -98,7 +99,8 @@ class PassTest : public TestT {
       spv_context spvContext = spvContextCreate(target_env);
       spv_diagnostic diagnostic = nullptr;
       spv_const_binary_t binary = {optimized_bin.data(), optimized_bin.size()};
-      spv_result_t error = spvValidate(spvContext, &binary, &diagnostic);
+      spv_result_t error = spvValidateWithOptions(
+          spvContext, ValidatorOptions(), &binary, &diagnostic);
       EXPECT_EQ(error, 0);
       if (error != 0) spvDiagnosticPrint(diagnostic);
       spvDiagnosticDestroy(diagnostic);
@@ -134,7 +136,8 @@ class PassTest : public TestT {
       spv_context spvContext = spvContextCreate(target_env);
       spv_diagnostic diagnostic = nullptr;
       spv_const_binary_t binary = {optimized_bin.data(), optimized_bin.size()};
-      spv_result_t error = spvValidate(spvContext, &binary, &diagnostic);
+      spv_result_t error = spvValidateWithOptions(
+          spvContext, ValidatorOptions(), &binary, &diagnostic);
       EXPECT_EQ(error, 0);
       if (error != 0) spvDiagnosticPrint(diagnostic);
       spvDiagnosticDestroy(diagnostic);
@@ -226,6 +229,8 @@ class PassTest : public TestT {
     consumer_ = msg_consumer;
   }
 
+  spv_validator_options ValidatorOptions() { return &validator_options_; }
+
  private:
   MessageConsumer consumer_;            // Message consumer.
   std::unique_ptr<IRContext> context_;  // IR context
@@ -233,6 +238,7 @@ class PassTest : public TestT {
   std::unique_ptr<PassManager> manager_;  // The pass manager.
   uint32_t assemble_options_;
   uint32_t disassemble_options_;
+  spv_validator_options_t validator_options_;
 };
 
 }  // namespace opt

test/val/val_id_test.cpp

@@ -1931,8 +1931,9 @@ TEST_F(ValidateIdWithMessage, OpVariableInitializerGlobalVariableGood) {
 %1 = OpTypeInt 32 0
 %2 = OpTypePointer Uniform %1
 %3 = OpVariable %2 Uniform
-%4 = OpTypePointer Uniform %2 ; pointer to pointer
-%5 = OpVariable %4 Uniform %3)";
+%4 = OpTypePointer Private %2 ; pointer to pointer
+%5 = OpVariable %4 Private %3
+)";
   CompileSuccessfully(spirv.c_str());
   EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
 }
@@ -2061,6 +2062,123 @@ OpFunctionEnd
   EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
 }
 
+TEST_F(ValidateIdWithMessage, OpVariablePointerNoVariablePointersBad) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability Linkage
+OpMemoryModel Logical GLSL450
+%void = OpTypeVoid
+%int = OpTypeInt 32 0
+%_ptr_workgroup_int = OpTypePointer Workgroup %int
+%_ptr_function_ptr = OpTypePointer Function %_ptr_workgroup_int
+%voidfn = OpTypeFunction %void
+%func = OpFunction %void None %voidfn
+%entry = OpLabel
+%var = OpVariable %_ptr_function_ptr Function
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv);
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+  EXPECT_THAT(
+      getDiagnosticString(),
+      HasSubstr(
+          "In Logical addressing, variables may not allocate a pointer type"));
+}
+
+TEST_F(ValidateIdWithMessage,
+       OpVariablePointerNoVariablePointersRelaxedLogicalGood) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability Linkage
+OpMemoryModel Logical GLSL450
+%void = OpTypeVoid
+%int = OpTypeInt 32 0
+%_ptr_workgroup_int = OpTypePointer Workgroup %int
+%_ptr_function_ptr = OpTypePointer Function %_ptr_workgroup_int
+%voidfn = OpTypeFunction %void
+%func = OpFunction %void None %voidfn
+%entry = OpLabel
+%var = OpVariable %_ptr_function_ptr Function
+OpReturn
+OpFunctionEnd
+)";
+
+  auto options = getValidatorOptions();
+  options->relax_logical_pointer = true;
+  CompileSuccessfully(spirv);
+  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
+}
+
+TEST_F(ValidateIdWithMessage,
+       OpVariablePointerVariablePointersStorageBufferGood) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability Linkage
+OpCapability VariablePointersStorageBuffer
+OpExtension "SPV_KHR_variable_pointers"
+OpMemoryModel Logical GLSL450
+%void = OpTypeVoid
+%int = OpTypeInt 32 0
+%_ptr_workgroup_int = OpTypePointer Workgroup %int
+%_ptr_function_ptr = OpTypePointer Function %_ptr_workgroup_int
+%voidfn = OpTypeFunction %void
+%func = OpFunction %void None %voidfn
+%entry = OpLabel
+%var = OpVariable %_ptr_function_ptr Function
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv);
+  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
+}
+
+TEST_F(ValidateIdWithMessage, OpVariablePointerVariablePointersGood) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability Linkage
+OpCapability VariablePointers
+OpExtension "SPV_KHR_variable_pointers"
+OpMemoryModel Logical GLSL450
+%void = OpTypeVoid
+%int = OpTypeInt 32 0
+%_ptr_workgroup_int = OpTypePointer Workgroup %int
+%_ptr_function_ptr = OpTypePointer Function %_ptr_workgroup_int
+%voidfn = OpTypeFunction %void
+%func = OpFunction %void None %voidfn
+%entry = OpLabel
+%var = OpVariable %_ptr_function_ptr Function
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv);
+  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
+}
+
+TEST_F(ValidateIdWithMessage, OpVariablePointerVariablePointersBad) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability VariablePointers
+OpExtension "SPV_KHR_variable_pointers"
+OpMemoryModel Logical GLSL450
+%void = OpTypeVoid
+%int = OpTypeInt 32 0
+%_ptr_workgroup_int = OpTypePointer Workgroup %int
+%_ptr_uniform_ptr = OpTypePointer Uniform %_ptr_workgroup_int
+%var = OpVariable %_ptr_uniform_ptr Uniform
+)";
+
+  CompileSuccessfully(spirv);
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("In Logical addressing with variable pointers, "
+                        "variables that allocate pointers must be in Function "
+                        "or Private storage classes"));
+}
+
 TEST_F(ValidateIdWithMessage, OpLoadGood) {
   std::string spirv = kGLSL450MemoryModel + R"(
  %1 = OpTypeVoid