Skip to content

Commit f3f1169

Browse files
alan-bakeralan-baker
authored
[val] Fix ValidateMemory with descriptor heap cap (KhronosGroup#6729)
Fixes KhronosGroup#6728 * FindUntypedBaseVariable didn't handle loading pointers from pointers correctly and had an infinite loop * fix those cases * Additionally add a filter to the check so it is only checked in potentially problematic cases
1 parent 21605d2 commit f3f1169

3 files changed

Lines changed: 100 additions & 44 deletions

File tree

‎source/val/validate_memory.cpp‎

Lines changed: 59 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -1384,50 +1384,65 @@ spv_result_t ValidateLoad(ValidationState_t& _, const Instruction* inst) {
13841384

13851385
// EXT_descriptor_heap
13861386
if (spvIsVulkanEnv(_.context()->target_env) &&
1387-
_.IsDescriptorHeapBaseVariable(_.FindDef(pointer_id))) {
1388-
auto descBaseVariable = _.FindUntypedBaseVariable(_.FindDef(pointer_id));
1389-
auto descBaseVariableId = descBaseVariable->id();
1390-
if (!_.HasDecoration(descBaseVariableId, spv::Decoration::DescriptorSet) &&
1391-
!_.HasDecoration(descBaseVariableId, spv::Decoration::Binding)) {
1392-
switch (result_type->opcode()) {
1393-
case spv::Op::OpTypeSampler:
1394-
if (!_.IsBuiltin(descBaseVariableId, spv::BuiltIn::SamplerHeapEXT)) {
1395-
return _.diag(SPV_ERROR_INVALID_ID, inst)
1396-
<< _.VkErrorID(11336)
1397-
<< "OpTypeSampler pointer instruction has no descriptor set "
1398-
<< "or binding and is not derived from a variable decorated "
1399-
"with "
1400-
"SamplerHeapEXT";
1401-
}
1402-
break;
1403-
case spv::Op::OpTypeImage:
1404-
if (!_.IsBuiltin(descBaseVariableId, spv::BuiltIn::ResourceHeapEXT)) {
1405-
return _.diag(SPV_ERROR_INVALID_ID, inst)
1406-
<< _.VkErrorID(11337)
1407-
<< "OpTypeImage pointer instruction has no descriptor set "
1408-
<< "or binding and is not derived from a variable decorated "
1409-
"with "
1410-
"ResourceHeapEXT";
1411-
}
1412-
break;
1413-
case spv::Op::OpTypeAccelerationStructureKHR:
1414-
uint32_t data_type;
1415-
spv::StorageClass sc;
1416-
if (_.GetPointerTypeInfo(descBaseVariable->type_id(), &data_type,
1417-
&sc) &&
1418-
sc != spv::StorageClass::Private &&
1419-
sc != spv::StorageClass::Function &&
1420-
!_.IsBuiltin(descBaseVariableId, spv::BuiltIn::ResourceHeapEXT)) {
1421-
return _.diag(SPV_ERROR_INVALID_ID, inst)
1422-
<< _.VkErrorID(11339)
1423-
<< "OpTypeAccelerationStructureKHR pointer instruction has "
1424-
"no "
1425-
<< "descriptor set or binding and is not derived from a "
1426-
"variable decorated with ResourceHeapEXT";
1387+
(result_type->opcode() == spv::Op::OpTypeSampler ||
1388+
result_type->opcode() == spv::Op::OpTypeImage ||
1389+
result_type->opcode() == spv::Op::OpTypeAccelerationStructureKHR)) {
1390+
if (_.IsDescriptorHeapBaseVariable(_.FindDef(pointer_id))) {
1391+
if (auto descBaseVariable =
1392+
_.FindUntypedBaseVariable(_.FindDef(pointer_id))) {
1393+
auto descBaseVariableId = descBaseVariable->id();
1394+
if (!_.HasDecoration(descBaseVariableId,
1395+
spv::Decoration::DescriptorSet) &&
1396+
!_.HasDecoration(descBaseVariableId, spv::Decoration::Binding)) {
1397+
switch (result_type->opcode()) {
1398+
case spv::Op::OpTypeSampler:
1399+
if (!_.IsBuiltin(descBaseVariableId,
1400+
spv::BuiltIn::SamplerHeapEXT)) {
1401+
return _.diag(SPV_ERROR_INVALID_ID, inst)
1402+
<< _.VkErrorID(11336)
1403+
<< "OpTypeSampler pointer instruction has no descriptor "
1404+
"set "
1405+
<< "or binding and is not derived from a variable "
1406+
"decorated "
1407+
"with "
1408+
"SamplerHeapEXT";
1409+
}
1410+
break;
1411+
case spv::Op::OpTypeImage:
1412+
if (!_.IsBuiltin(descBaseVariableId,
1413+
spv::BuiltIn::ResourceHeapEXT)) {
1414+
return _.diag(SPV_ERROR_INVALID_ID, inst)
1415+
<< _.VkErrorID(11337)
1416+
<< "OpTypeImage pointer instruction has no descriptor "
1417+
"set "
1418+
<< "or binding and is not derived from a variable "
1419+
"decorated "
1420+
"with "
1421+
"ResourceHeapEXT";
1422+
}
1423+
break;
1424+
case spv::Op::OpTypeAccelerationStructureKHR:
1425+
uint32_t data_type;
1426+
spv::StorageClass sc;
1427+
if (_.GetPointerTypeInfo(descBaseVariable->type_id(), &data_type,
1428+
&sc) &&
1429+
sc != spv::StorageClass::Private &&
1430+
sc != spv::StorageClass::Function &&
1431+
!_.IsBuiltin(descBaseVariableId,
1432+
spv::BuiltIn::ResourceHeapEXT)) {
1433+
return _.diag(SPV_ERROR_INVALID_ID, inst)
1434+
<< _.VkErrorID(11339)
1435+
<< "OpTypeAccelerationStructureKHR pointer instruction "
1436+
"has "
1437+
"no "
1438+
<< "descriptor set or binding and is not derived from a "
1439+
"variable decorated with ResourceHeapEXT";
1440+
}
1441+
break;
1442+
default:
1443+
break;
14271444
}
1428-
break;
1429-
default:
1430-
break;
1445+
}
14311446
}
14321447
}
14331448
}
@@ -2792,7 +2807,7 @@ spv_result_t ValidateBufferPointerEXT(ValidationState_t& _,
27922807
// Buffer operand
27932808
auto buffer =
27942809
_.FindUntypedBaseVariable(_.FindDef(inst->GetOperandAs<uint32_t>(2)));
2795-
if (!_.IsBuiltin(buffer->id(), spv::BuiltIn::ResourceHeapEXT)) {
2810+
if (!buffer || !_.IsBuiltin(buffer->id(), spv::BuiltIn::ResourceHeapEXT)) {
27962811
return _.diag(SPV_ERROR_INVALID_ID, inst)
27972812
<< "OpBufferPointerEXT's buffer must be an untyped pointer"
27982813
<< " into a variable declared with the ResourceHeapEXT built-in";

‎source/val/validation_state.cpp‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1617,6 +1617,8 @@ const Instruction* ValidationState_t::FindUntypedBaseVariable(
16171617
if (GetIdOpcode(GetOperandTypeId(base_inst, 2)) ==
16181618
spv::Op::OpTypeUntypedPointerKHR) {
16191619
base_inst = FindDef(base_inst->GetOperandAs<uint32_t>(2));
1620+
} else {
1621+
return nullptr;
16201622
}
16211623
break;
16221624
case spv::Op::OpAtomicExchange:
@@ -1640,6 +1642,8 @@ const Instruction* ValidationState_t::FindUntypedBaseVariable(
16401642
if (GetIdOpcode(GetOperandTypeId(base_inst, 0)) ==
16411643
spv::Op::OpTypeUntypedPointerKHR) {
16421644
base_inst = FindDef(base_inst->GetOperandAs<uint32_t>(0));
1645+
} else {
1646+
return nullptr;
16431647
}
16441648
break;
16451649
default:
@@ -1660,6 +1664,9 @@ bool ValidationState_t::IsDescriptorHeapBaseVariable(const Instruction* inst) {
16601664
return false;
16611665
}
16621666
const Instruction* base_inst = FindUntypedBaseVariable(inst);
1667+
if (!base_inst) {
1668+
return false;
1669+
}
16631670
const bool is_heap_base =
16641671
IsBuiltin(base_inst->id(), spv::BuiltIn::SamplerHeapEXT) ||
16651672
IsBuiltin(base_inst->id(), spv::BuiltIn::ResourceHeapEXT);

‎test/val/val_memory_test.cpp‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11028,6 +11028,40 @@ OpFunctionEnd
1102811028
AnyVUID("VUID-StandaloneSpirv-None-12295"));
1102911029
}
1103011030

11031+
TEST_F(ValidateMemory, LoadPointerFromPointer_DescriptorHeapCap) {
11032+
std::string spirv = R"(
11033+
OpCapability Shader
11034+
OpCapability PhysicalStorageBufferAddresses
11035+
OpCapability DescriptorHeapEXT
11036+
OpExtension "SPV_EXT_descriptor_heap"
11037+
OpMemoryModel PhysicalStorageBuffer64 GLSL450
11038+
OpEntryPoint GLCompute %main "main" %pc
11039+
OpExecutionMode %main LocalSize 1 1 1
11040+
OpDecorate %block Block
11041+
OpMemberDecorate %block 0 Offset 0
11042+
%void = OpTypeVoid
11043+
%void_fn = OpTypeFunction %void
11044+
%uint = OpTypeInt 32 0
11045+
%uint_0 = OpConstant %uint 0
11046+
%ptr_pssbo_uint = OpTypePointer PhysicalStorageBuffer %uint
11047+
%ptr_pssbo_ptr = OpTypePointer PhysicalStorageBuffer %ptr_pssbo_uint
11048+
%block = OpTypeStruct %ptr_pssbo_ptr
11049+
%ptr_pc_block = OpTypePointer PushConstant %block
11050+
%ptr_pc_ptr = OpTypePointer PushConstant %ptr_pssbo_ptr
11051+
%pc = OpVariable %ptr_pc_block PushConstant
11052+
%main = OpFunction %void None %void_fn
11053+
%entry = OpLabel
11054+
%gep = OpAccessChain %ptr_pc_ptr %pc %uint_0
11055+
%ld1 = OpLoad %ptr_pssbo_ptr %gep Aligned 8
11056+
%ld2 = OpLoad %ptr_pssbo_uint %ld1 Aligned 8
11057+
OpReturn
11058+
OpFunctionEnd
11059+
)";
11060+
11061+
CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_4);
11062+
EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_VULKAN_1_4));
11063+
}
11064+
1103111065
} // namespace
1103211066
} // namespace val
1103311067
} // namespace spvtools

0 commit comments

Comments
 (0)