Skip to content

Generate differentiated error codes for buffer oob checking #4097

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dnovillo merged 1 commit into from
Jan 11, 2021
Merged

Generate differentiated error codes for buffer oob checking #4097

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion include/spirv-tools/instrument.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,10 @@ static const int kInstMaxOutCnt = kInstStageOutCnt + 4;
static const int kInstErrorBindlessBounds = 0;
static const int kInstErrorBindlessUninit = 1;
static const int kInstErrorBuffAddrUnallocRef = 2;
static const int kInstErrorBindlessBuffOOB = 3;
// Deleted: static const int kInstErrorBindlessBuffOOB = 3;
Copy link

@dnovillo dnovillo Jan 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest to just delete the line. It can be recovered from revision control if needed later.

Copy link
Contributor Author

@greg-lunarg greg-lunarg Jan 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the careful review! :)

Since this is a public header and I have deleted a previously used code, I did this so if there are problems between the instrumentation and the tool using this instrumentation around this change, it is a little more clear what the problem is ie. that the constant was purposely removed.

Rather than delete this line, I am going to add another comment line explaining things better. Let me know if you still have a problem with this.

Copy link

@dnovillo dnovillo Jan 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, so the value 3 is now reserved in the sense that you need backwards compatibility?

Copy link

@dnovillo dnovillo Jan 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, is there a way for us to at some point remove this line? Or do we have to have this value reserved for ever?

Copy link

@dnovillo dnovillo Jan 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incidentally, I still don't see the changes you are referring to :)

Copy link
Contributor Author

@greg-lunarg greg-lunarg Jan 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you see the new line at 174?

The comment will eventually go away. It is just there for the transitional period.

Copy link

@dnovillo dnovillo Jan 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was expecting this rationale in the comment. Otherwise, looking at this code without the context of our conversation will tempt anyone to just get rid of the line.

Copy link
Contributor Author

@greg-lunarg greg-lunarg Jan 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I have added the rationale. Let me know if this is good.

Copy link
Collaborator

@dj2 dj2 Jan 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is breaking the validation layers which use the old value. Can we please leave the old value in place, add the new values, update downstream and then remove the old value?

// This comment will will remain for 2 releases to allow
// for the transition of all builds. Buffer OOB is
// generating the following four differentiated codes instead:
static const int kInstErrorBuffOOBUniform = 4;
static const int kInstErrorBuffOOBStorage = 5;
static const int kInstErrorBuffOOBUniformTexel = 6;
Expand Down
44 changes: 40 additions & 4 deletions source/opt/inst_bindless_check_pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,16 @@ static const int kSpvCopyObjectOperandIdInIdx = 0;
static const int kSpvLoadPtrIdInIdx = 0;
static const int kSpvAccessChainBaseIdInIdx = 0;
static const int kSpvAccessChainIndex0IdInIdx = 1;
static const int kSpvTypeArrayTypeIdInIdx = 0;
static const int kSpvTypeArrayLengthIdInIdx = 1;
static const int kSpvConstantValueInIdx = 0;
static const int kSpvVariableStorageClassInIdx = 0;
static const int kSpvTypePtrTypeIdInIdx = 1;
static const int kSpvTypeImageDim = 1;
static const int kSpvTypeImageDepth = 2;
static const int kSpvTypeImageArrayed = 3;
static const int kSpvTypeImageMS = 4;
static const int kSpvTypeImageSampled = 5;
} // anonymous namespace

// Avoid unused variable warning/error on Linux
Expand Down Expand Up @@ -206,13 +209,40 @@ bool InstBindlessCheckPass::AnalyzeDescriptorReference(Instruction* ref_inst,
var_inst->GetSingleWordInOperand(kSpvVariableStorageClassInIdx);
switch (storage_class) {
case SpvStorageClassUniform:
case SpvStorageClassUniformConstant:
case SpvStorageClassStorageBuffer:
break;
default:
return false;
break;
}
// Check for deprecated storage block form
if (storage_class == SpvStorageClassUniform) {
uint32_t var_ty_id = var_inst->type_id();
Instruction* var_ty_inst = get_def_use_mgr()->GetDef(var_ty_id);
uint32_t ptr_ty_id =
var_ty_inst->GetSingleWordInOperand(kSpvTypePtrTypeIdInIdx);
Instruction* ptr_ty_inst = get_def_use_mgr()->GetDef(ptr_ty_id);
SpvOp ptr_ty_op = ptr_ty_inst->opcode();
uint32_t block_ty_id =
(ptr_ty_op == SpvOpTypeArray || ptr_ty_op == SpvOpTypeRuntimeArray)
? ptr_ty_inst->GetSingleWordInOperand(kSpvTypeArrayTypeIdInIdx)
: ptr_ty_id;
assert(get_def_use_mgr()->GetDef(block_ty_id)->opcode() ==
SpvOpTypeStruct &&
"unexpected block type");
bool block_found = get_decoration_mgr()->FindDecoration(
block_ty_id, SpvDecorationBlock,
[](const Instruction&) { return true; });
if (!block_found) {
// If block decoration not found, verify deprecated form of SSBO
bool buffer_block_found = get_decoration_mgr()->FindDecoration(
block_ty_id, SpvDecorationBufferBlock,
[](const Instruction&) { return true; });
USE_ASSERT(buffer_block_found && "block decoration not found");
storage_class = SpvStorageClassStorageBuffer;
}
}
ref->strg_class = storage_class;
Instruction* desc_type_inst = GetPointeeTypeInst(var_inst);
switch (desc_type_inst->opcode()) {
case SpvOpTypeArray:
Expand Down Expand Up @@ -665,8 +695,10 @@ void InstBindlessCheckPass::GenDescInitCheckCode(
// for the referenced value.
Instruction* ult_inst =
builder.AddBinaryOp(GetBoolId(), SpvOpULessThan, ref_id, init_id);
uint32_t error =
init_check ? kInstErrorBindlessUninit : kInstErrorBindlessBuffOOB;
uint32_t error = init_check ? kInstErrorBindlessUninit
: (ref.strg_class == SpvStorageClassUniform
? kInstErrorBuffOOBUniform
: kInstErrorBuffOOBStorage);
uint32_t error_id = builder.GetUintConstantId(error);
GenCheckCode(ult_inst->result_id(), error_id, init_check ? 0 : ref_id,
init_check ? builder.GetUintConstantId(0u) : init_id, stage_idx,
Expand Down Expand Up @@ -732,7 +764,11 @@ void InstBindlessCheckPass::GenTexBuffCheckCode(
// for the referenced value.
Instruction* ult_inst =
builder.AddBinaryOp(GetBoolId(), SpvOpULessThan, coord_id, size_id);
uint32_t error_id = builder.GetUintConstantId(kInstErrorBindlessBuffOOB);
uint32_t error =
(image_ty_inst->GetSingleWordInOperand(kSpvTypeImageSampled) == 2)
? kInstErrorBuffOOBStorageTexel
: kInstErrorBuffOOBUniformTexel;
uint32_t error_id = builder.GetUintConstantId(error);
GenCheckCode(ult_inst->result_id(), error_id, coord_id, size_id, stage_idx,
&ref, new_blocks);
// Move original block's remaining code into remainder/merge block and add
Expand Down
1 change: 1 addition & 0 deletions source/opt/inst_bindless_check_pass.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ class InstBindlessCheckPass : public InstrumentPass {
uint32_t ptr_id;
uint32_t var_id;
uint32_t desc_idx_id;
uint32_t strg_class;
Instruction* ref_inst;
} RefAnalysis;

Expand Down
Loading