mirror of
https://github.com/KhronosGroup/SPIRV-Tools
synced 2024-10-18 11:10:05 +00:00
Instrument: Fix bindless checking for BufferDeviceAddress (#5049)
Avoid using OpConstantNull with types that do not allow it. Update existing tests for slight changes in code generation. Add new tests based on the Vulkan Validation layer test case that exposed this problem.
This commit is contained in:
parent
0e6fbba776
commit
ba4c9fe534
@ -536,6 +536,8 @@ void InstBindlessCheckPass::GenCheckCode(
|
|||||||
new BasicBlock(std::move(valid_label)));
|
new BasicBlock(std::move(valid_label)));
|
||||||
builder.SetInsertPoint(&*new_blk_ptr);
|
builder.SetInsertPoint(&*new_blk_ptr);
|
||||||
uint32_t new_ref_id = CloneOriginalReference(ref, &builder);
|
uint32_t new_ref_id = CloneOriginalReference(ref, &builder);
|
||||||
|
uint32_t null_id = 0;
|
||||||
|
uint32_t ref_type_id = ref->ref_inst->type_id();
|
||||||
(void)builder.AddBranch(merge_blk_id);
|
(void)builder.AddBranch(merge_blk_id);
|
||||||
new_blocks->push_back(std::move(new_blk_ptr));
|
new_blocks->push_back(std::move(new_blk_ptr));
|
||||||
// Gen invalid block
|
// Gen invalid block
|
||||||
@ -563,10 +565,23 @@ void InstBindlessCheckPass::GenCheckCode(
|
|||||||
GenDebugStreamWrite(uid2offset_[ref->ref_inst->unique_id()], stage_idx,
|
GenDebugStreamWrite(uid2offset_[ref->ref_inst->unique_id()], stage_idx,
|
||||||
{error_id, u_index_id, u_length_id}, &builder);
|
{error_id, u_index_id, u_length_id}, &builder);
|
||||||
}
|
}
|
||||||
|
// Generate a ConstantNull, converting to uint64 if the type cannot be a null.
|
||||||
|
if (new_ref_id != 0) {
|
||||||
|
analysis::TypeManager* type_mgr = context()->get_type_mgr();
|
||||||
|
analysis::Type* ref_type = type_mgr->GetType(ref_type_id);
|
||||||
|
if (ref_type->AsPointer() != nullptr) {
|
||||||
|
context()->AddCapability(spv::Capability::Int64);
|
||||||
|
uint32_t null_u64_id = GetNullId(GetUint64Id());
|
||||||
|
Instruction* null_ptr_inst = builder.AddUnaryOp(
|
||||||
|
ref_type_id, spv::Op::OpConvertUToPtr, null_u64_id);
|
||||||
|
null_id = null_ptr_inst->result_id();
|
||||||
|
} else {
|
||||||
|
null_id = GetNullId(ref_type_id);
|
||||||
|
}
|
||||||
|
}
|
||||||
// Remember last invalid block id
|
// Remember last invalid block id
|
||||||
uint32_t last_invalid_blk_id = new_blk_ptr->GetLabelInst()->result_id();
|
uint32_t last_invalid_blk_id = new_blk_ptr->GetLabelInst()->result_id();
|
||||||
// Gen zero for invalid reference
|
// Gen zero for invalid reference
|
||||||
uint32_t ref_type_id = ref->ref_inst->type_id();
|
|
||||||
(void)builder.AddBranch(merge_blk_id);
|
(void)builder.AddBranch(merge_blk_id);
|
||||||
new_blocks->push_back(std::move(new_blk_ptr));
|
new_blocks->push_back(std::move(new_blk_ptr));
|
||||||
// Gen merge block
|
// Gen merge block
|
||||||
@ -577,8 +592,7 @@ void InstBindlessCheckPass::GenCheckCode(
|
|||||||
// reference.
|
// reference.
|
||||||
if (new_ref_id != 0) {
|
if (new_ref_id != 0) {
|
||||||
Instruction* phi_inst = builder.AddPhi(
|
Instruction* phi_inst = builder.AddPhi(
|
||||||
ref_type_id, {new_ref_id, valid_blk_id, GetNullId(ref_type_id),
|
ref_type_id, {new_ref_id, valid_blk_id, null_id, last_invalid_blk_id});
|
||||||
last_invalid_blk_id});
|
|
||||||
context()->ReplaceAllUsesWith(ref->ref_inst->result_id(),
|
context()->ReplaceAllUsesWith(ref->ref_inst->result_id(),
|
||||||
phi_inst->result_id());
|
phi_inst->result_id());
|
||||||
}
|
}
|
||||||
@ -734,15 +748,7 @@ void InstBindlessCheckPass::GenTexBuffCheckCode(
|
|||||||
if (image_ty_inst->GetSingleWordInOperand(kSpvTypeImageArrayed) != 0) return;
|
if (image_ty_inst->GetSingleWordInOperand(kSpvTypeImageArrayed) != 0) return;
|
||||||
if (image_ty_inst->GetSingleWordInOperand(kSpvTypeImageMS) != 0) return;
|
if (image_ty_inst->GetSingleWordInOperand(kSpvTypeImageMS) != 0) return;
|
||||||
// Enable ImageQuery Capability if not yet enabled
|
// Enable ImageQuery Capability if not yet enabled
|
||||||
if (!get_feature_mgr()->HasCapability(spv::Capability::ImageQuery)) {
|
context()->AddCapability(spv::Capability::ImageQuery);
|
||||||
std::unique_ptr<Instruction> cap_image_query_inst(
|
|
||||||
new Instruction(context(), spv::Op::OpCapability, 0, 0,
|
|
||||||
std::initializer_list<Operand>{
|
|
||||||
{SPV_OPERAND_TYPE_CAPABILITY,
|
|
||||||
{uint32_t(spv::Capability::ImageQuery)}}}));
|
|
||||||
get_def_use_mgr()->AnalyzeInstDefUse(&*cap_image_query_inst);
|
|
||||||
context()->AddCapability(std::move(cap_image_query_inst));
|
|
||||||
}
|
|
||||||
// Move original block's preceding instructions into first new block
|
// Move original block's preceding instructions into first new block
|
||||||
std::unique_ptr<BasicBlock> new_blk_ptr;
|
std::unique_ptr<BasicBlock> new_blk_ptr;
|
||||||
MovePreludeCode(ref_inst_itr, ref_block_itr, &new_blk_ptr);
|
MovePreludeCode(ref_inst_itr, ref_block_itr, &new_blk_ptr);
|
||||||
|
@ -408,14 +408,7 @@ uint32_t InstBuffAddrCheckPass::GenSearchAndTest(Instruction* ref_inst,
|
|||||||
InstructionBuilder* builder,
|
InstructionBuilder* builder,
|
||||||
uint32_t* ref_uptr_id) {
|
uint32_t* ref_uptr_id) {
|
||||||
// Enable Int64 if necessary
|
// Enable Int64 if necessary
|
||||||
if (!get_feature_mgr()->HasCapability(spv::Capability::Int64)) {
|
context()->AddCapability(spv::Capability::Int64);
|
||||||
std::unique_ptr<Instruction> cap_int64_inst(new Instruction(
|
|
||||||
context(), spv::Op::OpCapability, 0, 0,
|
|
||||||
std::initializer_list<Operand>{{SPV_OPERAND_TYPE_CAPABILITY,
|
|
||||||
{uint32_t(spv::Capability::Int64)}}}));
|
|
||||||
get_def_use_mgr()->AnalyzeInstDefUse(&*cap_int64_inst);
|
|
||||||
context()->AddCapability(std::move(cap_int64_inst));
|
|
||||||
}
|
|
||||||
// Convert reference pointer to uint64
|
// Convert reference pointer to uint64
|
||||||
uint32_t ref_ptr_id = ref_inst->GetSingleWordInOperand(0);
|
uint32_t ref_ptr_id = ref_inst->GetSingleWordInOperand(0);
|
||||||
Instruction* ref_uptr_inst =
|
Instruction* ref_uptr_inst =
|
||||||
|
File diff suppressed because it is too large
Load Diff
@ -120,6 +120,22 @@ static const std::string kStreamWrite4Compute = kStreamWrite4Begin + R"(
|
|||||||
)" + kStreamWrite4End;
|
)" + kStreamWrite4End;
|
||||||
// clang-format on
|
// clang-format on
|
||||||
|
|
||||||
|
// clang-format off
|
||||||
|
static const std::string kStreamWrite4Vert = kStreamWrite4Begin + R"(
|
||||||
|
; CHECK: {{%\w+}} = OpIAdd %uint {{%\w+}} %uint_3
|
||||||
|
; CHECK: {{%\w+}} = OpAccessChain %_ptr_StorageBuffer_uint [[output_buffer_var]] %uint_2 {{%\w+}}
|
||||||
|
; CHECK: OpStore {{%\w+}} %uint_0
|
||||||
|
; CHECK: {{%\w+}} = OpLoad %uint %gl_VertexIndex
|
||||||
|
; CHECK: {{%\w+}} = OpIAdd %uint {{%\w+}} %uint_4
|
||||||
|
; CHECK: {{%\w+}} = OpAccessChain %_ptr_StorageBuffer_uint [[output_buffer_var]] %uint_2 {{%\w+}}
|
||||||
|
; CHECK: OpStore {{%\w+}} {{%\w+}}
|
||||||
|
; CHECK: {{%\w+}} = OpLoad %uint %gl_InstanceIndex
|
||||||
|
; CHECK: {{%\w+}} = OpIAdd %uint {{%\w+}} %uint_5
|
||||||
|
; CHECK: {{%\w+}} = OpAccessChain %_ptr_StorageBuffer_uint [[output_buffer_var]] %uint_2 {{%\w+}}
|
||||||
|
; CHECK: OpStore {{%\w+}} {{%\w+}}
|
||||||
|
)" + kStreamWrite4End;
|
||||||
|
// clang-format on
|
||||||
|
|
||||||
static const std::string kInputDecorations = R"(
|
static const std::string kInputDecorations = R"(
|
||||||
; CHECK: OpDecorate [[input_buffer_type:%inst_buff_addr_InputBuffer]] Block
|
; CHECK: OpDecorate [[input_buffer_type:%inst_buff_addr_InputBuffer]] Block
|
||||||
; CHECK: OpMemberDecorate [[input_buffer_type]] 0 Offset 0
|
; CHECK: OpMemberDecorate [[input_buffer_type]] 0 Offset 0
|
||||||
@ -552,6 +568,118 @@ OpFunctionEnd
|
|||||||
defs + decorates + globals + main_func + output_funcs, true);
|
defs + decorates + globals + main_func + output_funcs, true);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
TEST_F(InstBuffAddrTest, DeviceBufferAddressOOB) {
|
||||||
|
// #version 450
|
||||||
|
// #extension GL_EXT_buffer_reference : enable
|
||||||
|
// layout(buffer_reference, buffer_reference_align = 16) buffer bufStruct;
|
||||||
|
// layout(set = 0, binding = 0) uniform ufoo {
|
||||||
|
// bufStruct data;
|
||||||
|
// int nWrites;
|
||||||
|
// } u_info;
|
||||||
|
// layout(buffer_reference, std140) buffer bufStruct {
|
||||||
|
// int a[4];
|
||||||
|
// };
|
||||||
|
// void main() {
|
||||||
|
// for (int i=0; i < u_info.nWrites; ++i) {
|
||||||
|
// u_info.data.a[i] = 0xdeadca71;
|
||||||
|
// }
|
||||||
|
// }
|
||||||
|
|
||||||
|
// clang-format off
|
||||||
|
const std::string text = R"(
|
||||||
|
OpCapability Shader
|
||||||
|
OpCapability PhysicalStorageBufferAddresses
|
||||||
|
%1 = OpExtInstImport "GLSL.std.450"
|
||||||
|
OpMemoryModel PhysicalStorageBuffer64 GLSL450
|
||||||
|
OpEntryPoint Vertex %main "main" %u_info
|
||||||
|
;CHECK: OpEntryPoint Vertex %main "main" %u_info %inst_buff_addr_input_buffer %inst_buff_addr_output_buffer %gl_VertexIndex %gl_InstanceIndex
|
||||||
|
OpSource GLSL 450
|
||||||
|
OpSourceExtension "GL_EXT_buffer_reference"
|
||||||
|
OpName %main "main"
|
||||||
|
OpName %i "i"
|
||||||
|
OpName %ufoo "ufoo"
|
||||||
|
OpMemberName %ufoo 0 "data"
|
||||||
|
OpMemberName %ufoo 1 "nWrites"
|
||||||
|
OpName %bufStruct "bufStruct"
|
||||||
|
OpMemberName %bufStruct 0 "a"
|
||||||
|
OpName %u_info "u_info"
|
||||||
|
OpMemberDecorate %ufoo 0 Offset 0
|
||||||
|
OpMemberDecorate %ufoo 1 Offset 8
|
||||||
|
OpDecorate %ufoo Block
|
||||||
|
OpDecorate %_arr_int_uint_4 ArrayStride 16
|
||||||
|
OpMemberDecorate %bufStruct 0 Offset 0
|
||||||
|
OpDecorate %bufStruct Block
|
||||||
|
OpDecorate %u_info DescriptorSet 0
|
||||||
|
OpDecorate %u_info Binding 0)" + kInputDecorations + kOutputDecorations + R"(
|
||||||
|
%void = OpTypeVoid
|
||||||
|
%3 = OpTypeFunction %void
|
||||||
|
%int = OpTypeInt 32 1
|
||||||
|
%_ptr_Function_int = OpTypePointer Function %int
|
||||||
|
%int_0 = OpConstant %int 0
|
||||||
|
OpTypeForwardPointer %_ptr_PhysicalStorageBuffer_bufStruct PhysicalStorageBuffer
|
||||||
|
%ufoo = OpTypeStruct %_ptr_PhysicalStorageBuffer_bufStruct %int
|
||||||
|
%uint = OpTypeInt 32 0
|
||||||
|
%uint_4 = OpConstant %uint 4
|
||||||
|
%_arr_int_uint_4 = OpTypeArray %int %uint_4
|
||||||
|
%bufStruct = OpTypeStruct %_arr_int_uint_4
|
||||||
|
%_ptr_PhysicalStorageBuffer_bufStruct = OpTypePointer PhysicalStorageBuffer %bufStruct
|
||||||
|
%_ptr_Uniform_ufoo = OpTypePointer Uniform %ufoo
|
||||||
|
%u_info = OpVariable %_ptr_Uniform_ufoo Uniform
|
||||||
|
%int_1 = OpConstant %int 1
|
||||||
|
%_ptr_Uniform_int = OpTypePointer Uniform %int
|
||||||
|
%bool = OpTypeBool
|
||||||
|
%_ptr_Uniform__ptr_PhysicalStorageBuffer_bufStruct = OpTypePointer Uniform %_ptr_PhysicalStorageBuffer_bufStruct
|
||||||
|
%int_n559035791 = OpConstant %int -559035791
|
||||||
|
%_ptr_PhysicalStorageBuffer_int = OpTypePointer PhysicalStorageBuffer %int)" + kInputGlobals + kOutputGlobals + R"(
|
||||||
|
%main = OpFunction %void None %3
|
||||||
|
%5 = OpLabel
|
||||||
|
%i = OpVariable %_ptr_Function_int Function
|
||||||
|
OpStore %i %int_0
|
||||||
|
OpBranch %10
|
||||||
|
%10 = OpLabel
|
||||||
|
OpLoopMerge %12 %13 None
|
||||||
|
OpBranch %14
|
||||||
|
%14 = OpLabel
|
||||||
|
%15 = OpLoad %int %i
|
||||||
|
%26 = OpAccessChain %_ptr_Uniform_int %u_info %int_1
|
||||||
|
%27 = OpLoad %int %26
|
||||||
|
%29 = OpSLessThan %bool %15 %27
|
||||||
|
OpBranchConditional %29 %11 %12
|
||||||
|
%11 = OpLabel
|
||||||
|
%31 = OpAccessChain %_ptr_Uniform__ptr_PhysicalStorageBuffer_bufStruct %u_info %int_0
|
||||||
|
%32 = OpLoad %_ptr_PhysicalStorageBuffer_bufStruct %31
|
||||||
|
%33 = OpLoad %int %i
|
||||||
|
%36 = OpAccessChain %_ptr_PhysicalStorageBuffer_int %32 %int_0 %33
|
||||||
|
;CHECK: %41 = OpConvertPtrToU %ulong %36
|
||||||
|
;CHECK: %76 = OpFunctionCall %bool %inst_buff_addr_search_and_test %41 %uint_4
|
||||||
|
;CHECK: OpSelectionMerge %77 None
|
||||||
|
;CHECK: OpBranchConditional %76 %78 %79
|
||||||
|
;CHECK: %78 = OpLabel
|
||||||
|
OpStore %36 %int_n559035791 Aligned 16
|
||||||
|
;CHECK: OpBranch %77
|
||||||
|
;CHECK: 79 = OpLabel
|
||||||
|
;CHECK: 80 = OpUConvert %uint %41
|
||||||
|
;CHECK: 82 = OpShiftRightLogical %ulong %41 %uint_32
|
||||||
|
;CHECK: 83 = OpUConvert %uint %82
|
||||||
|
;CHECK: 134 = OpFunctionCall %void %inst_buff_addr_stream_write_4 %uint_62 %uint_2 %80 %83
|
||||||
|
;CHECK: OpBranch %77
|
||||||
|
;CHECK: 77 = OpLabel
|
||||||
|
OpBranch %13
|
||||||
|
%13 = OpLabel
|
||||||
|
%37 = OpLoad %int %i
|
||||||
|
%38 = OpIAdd %int %37 %int_1
|
||||||
|
OpStore %i %38
|
||||||
|
OpBranch %10
|
||||||
|
%12 = OpLabel
|
||||||
|
OpReturn
|
||||||
|
OpFunctionEnd)" + kSearchAndTest + kStreamWrite4Vert;
|
||||||
|
// clang-format on
|
||||||
|
|
||||||
|
SetTargetEnv(SPV_ENV_VULKAN_1_2);
|
||||||
|
SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
|
||||||
|
SinglePassRunAndMatch<InstBuffAddrCheckPass>(text, true, 7, 23);
|
||||||
|
}
|
||||||
|
|
||||||
} // namespace
|
} // namespace
|
||||||
} // namespace opt
|
} // namespace opt
|
||||||
} // namespace spvtools
|
} // namespace spvtools
|
||||||
|
Loading…
Reference in New Issue
Block a user