Skip to content

Commit 5d6745b

Browse files
alan-bakeralan-baker
authored
Improve variable pointer validation (KhronosGroup#6595)
Fixes KhronosGroup#6532 * Check through types to ensure only valid pointers are allocated in logical variables * Fix up some invalid tests
1 parent 5a1eea1 commit 5d6745b

4 files changed

Lines changed: 140 additions & 39 deletions

File tree

‎source/val/validate_memory.cpp‎

Lines changed: 48 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -619,35 +619,54 @@ spv_result_t ValidateVariablePointer(ValidationState_t& _,
619619
if ((_.addressing_model() == spv::AddressingModel::Logical ||
620620
_.addressing_model() == spv::AddressingModel::PhysicalStorageBuffer64) &&
621621
!_.options()->relax_logical_pointer) {
622-
if ((pointee.opcode() == spv::Op::OpTypePointer ||
623-
pointee.opcode() == spv::Op::OpTypeUntypedPointerKHR)) {
624-
const auto sc = pointee.GetOperandAs<spv::StorageClass>(1u);
625-
if (sc != spv::StorageClass::PhysicalStorageBuffer) {
626-
if (sc != spv::StorageClass::StorageBuffer &&
627-
sc != spv::StorageClass::Workgroup) {
628-
return _.diag(SPV_ERROR_INVALID_ID, inst)
629-
<< "In Logical addressing, variables can only allocate a "
630-
"pointer to the StorageBuffer or Workgroup storage classes";
631-
} else if (!_.HasCapability(
632-
spv::Capability::VariablePointersStorageBuffer) &&
633-
sc == spv::StorageClass::StorageBuffer) {
634-
return _.diag(SPV_ERROR_INVALID_ID, inst)
635-
<< "In Logical addressing, variables can only allocate a "
636-
"storage buffer pointer if the "
637-
"VariablePointersStorageBuffer capability is declared";
638-
} else if (!_.HasCapability(spv::Capability::VariablePointers) &&
639-
sc == spv::StorageClass::Workgroup) {
640-
return _.diag(SPV_ERROR_INVALID_ID, inst)
641-
<< "In Logical addressing, variables can only allocate a "
642-
"workgroup pointer if the VariablePointers capability is "
643-
"declared";
644-
} else if (storage_class != spv::StorageClass::Function &&
645-
storage_class != spv::StorageClass::Private) {
646-
return _.diag(SPV_ERROR_INVALID_ID, inst)
647-
<< "In Logical addressing with variable pointers, variables "
648-
<< "that allocate pointers must be in Function or Private "
649-
<< "storage classes";
650-
}
622+
spv_result_t error = SPV_SUCCESS;
623+
bool contains_logical_pointer = _.ContainsType(
624+
pointee.id(),
625+
[&_, inst, &error](const Instruction* type) {
626+
if (type->opcode() == spv::Op::OpTypePointer ||
627+
type->opcode() == spv::Op::OpTypeUntypedPointerKHR) {
628+
const auto sc = type->GetOperandAs<spv::StorageClass>(1u);
629+
if (sc != spv::StorageClass::PhysicalStorageBuffer) {
630+
if (sc != spv::StorageClass::StorageBuffer &&
631+
sc != spv::StorageClass::Workgroup) {
632+
error =
633+
_.diag(SPV_ERROR_INVALID_ID, inst)
634+
<< "In Logical addressing, variables can only allocate a "
635+
"pointer to the StorageBuffer or Workgroup storage "
636+
"classes";
637+
} else if (!_.HasCapability(
638+
spv::Capability::VariablePointersStorageBuffer) &&
639+
sc == spv::StorageClass::StorageBuffer) {
640+
error =
641+
_.diag(SPV_ERROR_INVALID_ID, inst)
642+
<< "In Logical addressing, variables can only allocate a "
643+
"storage buffer pointer if the "
644+
"VariablePointersStorageBuffer capability is declared";
645+
} else if (!_.HasCapability(spv::Capability::VariablePointers) &&
646+
sc == spv::StorageClass::Workgroup) {
647+
error =
648+
_.diag(SPV_ERROR_INVALID_ID, inst)
649+
<< "In Logical addressing, variables can only allocate a "
650+
"workgroup pointer if the VariablePointers capability "
651+
"is "
652+
"declared";
653+
}
654+
return true;
655+
}
656+
}
657+
return false;
658+
},
659+
/* traverse_all_types = */ false);
660+
661+
if (error != SPV_SUCCESS) return error;
662+
663+
if (contains_logical_pointer) {
664+
if (storage_class != spv::StorageClass::Function &&
665+
storage_class != spv::StorageClass::Private) {
666+
return _.diag(SPV_ERROR_INVALID_ID, inst)
667+
<< "In Logical addressing with variable pointers, variables "
668+
<< "that allocate pointers must be in Function or Private "
669+
<< "storage classes";
651670
}
652671
}
653672
}

‎test/val/val_id_test.cpp‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2541,11 +2541,11 @@ OpFunctionEnd
25412541
TEST_P(ValidateIdWithMessage, OpVariableContainsBoolPointerGood) {
25422542
std::string spirv = kGLSL450MemoryModel + R"(
25432543
%bool = OpTypeBool
2544-
%boolptr = OpTypePointer Uniform %bool
2544+
%boolptr = OpTypePointer Workgroup %bool
25452545
%int = OpTypeInt 32 0
25462546
%block = OpTypeStruct %boolptr %int
2547-
%_ptr_Uniform_block = OpTypePointer Uniform %block
2548-
%var = OpVariable %_ptr_Uniform_block Uniform
2547+
%_ptr_Private_block = OpTypePointer Private %block
2548+
%var = OpVariable %_ptr_Private_block Private
25492549
%void = OpTypeVoid
25502550
%fnty = OpTypeFunction %void
25512551
%main = OpFunction %void None %fnty
@@ -2801,6 +2801,11 @@ OpMemoryModel Logical GLSL450
28012801
%_ptr_workgroup_int = OpTypePointer Workgroup %int
28022802
%_ptr_uniform_ptr = OpTypePointer Uniform %_ptr_workgroup_int
28032803
%var = OpVariable %_ptr_uniform_ptr Uniform
2804+
%voidfn = OpTypeFunction %void
2805+
%func = OpFunction %void None %voidfn
2806+
%entry = OpLabel
2807+
OpReturn
2808+
OpFunctionEnd
28042809
)";
28052810

28062811
CompileSuccessfully(spirv);

‎test/val/val_interfaces_test.cpp‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1640,7 +1640,7 @@ OpFunctionEnd
16401640
"Interface struct has no Block decoration but has BuiltIn members."));
16411641
}
16421642

1643-
TEST_F(ValidateInterfacesTest, InvalidLocationTypePointer) {
1643+
TEST_F(ValidateInterfacesTest, InvalidLocationTypeSampler) {
16441644
const std::string text = R"(
16451645
OpCapability Shader
16461646
OpMemoryModel Logical Simple
@@ -1649,14 +1649,13 @@ TEST_F(ValidateInterfacesTest, InvalidLocationTypePointer) {
16491649
%void = OpTypeVoid
16501650
%5 = OpTypeFunction %void
16511651
%float = OpTypeFloat 32
1652-
%_ptr_Private_void = OpTypePointer Private %void
1652+
%sampler = OpTypeSampler
16531653
%uint = OpTypeInt 32 0
16541654
%uint_4278132784 = OpConstant %uint 4278132784
1655-
%_arr__ptr_Private_void_uint_4278132784 = OpTypeArray %_ptr_Private_void %uint_4278132784
1656-
%_ptr_Output__arr__ptr_Private_void_uint_4278132784 = OpTypePointer Output %_arr__ptr_Private_void_uint_4278132784
1657-
%2 = OpVariable %_ptr_Output__arr__ptr_Private_void_uint_4278132784 Output
1658-
%_ptr_Output__ptr_Private_void = OpTypePointer Output %_ptr_Private_void
1659-
%3 = OpVariable %_ptr_Output__arr__ptr_Private_void_uint_4278132784 Output
1655+
%_arr__sampler_uint_4278132784 = OpTypeArray %sampler %uint_4278132784
1656+
%_ptr_Output__arr__sampler_uint_4278132784 = OpTypePointer Output %_arr__sampler_uint_4278132784
1657+
%2 = OpVariable %_ptr_Output__arr__sampler_uint_4278132784 Output
1658+
%3 = OpVariable %_ptr_Output__arr__sampler_uint_4278132784 Output
16601659
%1 = OpFunction %void None %5
16611660
%15 = OpLabel
16621661
OpReturn

‎test/val/val_memory_test.cpp‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8931,6 +8931,84 @@ OpFunctionEnd
89318931
"allocate pointers must be in Function or Private storage classes"));
89328932
}
89338933

8934+
TEST_F(ValidateMemory, VariablePointerInStruct) {
8935+
const std::string spirv = R"(
8936+
OpCapability Shader
8937+
OpCapability VariablePointers
8938+
OpExtension "SPV_KHR_variable_pointers"
8939+
OpMemoryModel Logical GLSL450
8940+
OpEntryPoint GLCompute %main "main"
8941+
OpExecutionMode %main LocalSize 1 1 1
8942+
%void = OpTypeVoid
8943+
%void_fn = OpTypeFunction %void
8944+
%int = OpTypeInt 32 0
8945+
%ptr_s = OpTypePointer Workgroup %int
8946+
%struct_t = OpTypeStruct %ptr_s
8947+
%ptr_struct = OpTypePointer Function %struct_t
8948+
%main = OpFunction %void None %void_fn
8949+
%entry = OpLabel
8950+
%var = OpVariable %ptr_struct Function
8951+
OpReturn
8952+
OpFunctionEnd
8953+
)";
8954+
8955+
CompileSuccessfully(spirv, SPV_ENV_UNIVERSAL_1_3);
8956+
EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_UNIVERSAL_1_3));
8957+
}
8958+
8959+
TEST_F(ValidateMemory, VariablePointerInArray) {
8960+
const std::string spirv = R"(
8961+
OpCapability Shader
8962+
OpCapability VariablePointers
8963+
OpExtension "SPV_KHR_variable_pointers"
8964+
OpMemoryModel Logical GLSL450
8965+
OpEntryPoint GLCompute %main "main"
8966+
OpExecutionMode %main LocalSize 1 1 1
8967+
%void = OpTypeVoid
8968+
%void_fn = OpTypeFunction %void
8969+
%int = OpTypeInt 32 0
8970+
%int_4 = OpConstant %int 4
8971+
%ptr_s = OpTypePointer Workgroup %int
8972+
%array_t = OpTypeArray %ptr_s %int_4
8973+
%ptr_array = OpTypePointer Function %array_t
8974+
%main = OpFunction %void None %void_fn
8975+
%entry = OpLabel
8976+
%var = OpVariable %ptr_array Function
8977+
OpReturn
8978+
OpFunctionEnd
8979+
)";
8980+
8981+
CompileSuccessfully(spirv, SPV_ENV_UNIVERSAL_1_3);
8982+
EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_UNIVERSAL_1_3));
8983+
}
8984+
8985+
TEST_F(ValidateMemory, NoVariablePointerInStruct) {
8986+
const std::string spirv = R"(
8987+
OpCapability Shader
8988+
OpMemoryModel Logical GLSL450
8989+
OpEntryPoint GLCompute %main "main"
8990+
OpExecutionMode %main LocalSize 1 1 1
8991+
%void = OpTypeVoid
8992+
%void_fn = OpTypeFunction %void
8993+
%int = OpTypeInt 32 0
8994+
%ptr_f = OpTypePointer Function %int
8995+
%struct_t = OpTypeStruct %ptr_f
8996+
%ptr_struct = OpTypePointer Function %struct_t
8997+
%main = OpFunction %void None %void_fn
8998+
%entry = OpLabel
8999+
%var = OpVariable %ptr_struct Function
9000+
OpReturn
9001+
OpFunctionEnd
9002+
)";
9003+
9004+
CompileSuccessfully(spirv, SPV_ENV_UNIVERSAL_1_3);
9005+
EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_UNIVERSAL_1_3));
9006+
EXPECT_THAT(
9007+
getDiagnosticString(),
9008+
HasSubstr("In Logical addressing, variables can only allocate a pointer "
9009+
"to the StorageBuffer or Workgroup storage classes"));
9010+
}
9011+
89349012
TEST_F(ValidateMemory, VariablePointerBadStorageClassUntyped) {
89359013
const std::string spirv = R"(
89369014
OpCapability Shader

0 commit comments

Comments
 (0)