Skip to content

Add validation for invalid layout decoration usage #6012

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
merged 1 commit into from
Feb 27, 2025
Merged

Add validation for invalid layout decoration usage #6012

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
192 changes: 192 additions & 0 deletions source/val/validate_decorations.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2083,6 +2083,197 @@ spv_result_t CheckDecorationsFromDecoration(ValidationState_t& vstate) {
return SPV_SUCCESS;
}

bool AllowsLayout(ValidationState_t& vstate, const spv::StorageClass sc) {
switch (sc) {
case spv::StorageClass::StorageBuffer:
case spv::StorageClass::Uniform:
case spv::StorageClass::PhysicalStorageBuffer:
case spv::StorageClass::PushConstant:
// Always explicitly laid out.
return true;
case spv::StorageClass::UniformConstant:
return false;
case spv::StorageClass::Workgroup:
return vstate.HasCapability(
spv::Capability::WorkgroupMemoryExplicitLayoutKHR);
case spv::StorageClass::Function:
case spv::StorageClass::Private:
return vstate.version() < SPV_SPIRV_VERSION_WORD(1, 4);
Copy link
Contributor

Choose a reason for hiding this comment

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

@alan-baker so for something in GLSL like

#version 450

struct Bar {
    uint x;
    uint y;
    uint z[2];
};

layout(set = 0, binding = 0, std430) buffer foo {
    uvec4 a;
    Bar b; // size 16 at offset 16
    uint c;
};

void main() {
    Bar new_bar;
    b = new_bar;
}

this will pass spirv-val if using --target-env vulkan1.1 (https://godbolt.org/z/bjKodYPKG)

but fail here with 1.2

I guess trying to figure out what spec change causes this to fail from 1.1 to 1.2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This chapter describes valid uses for a set of SPIR-V decorations. Any other use of one of these decorations is invalid, with the exception that, when using SPIR-V versions 1.4 and earlier: Block, BufferBlock, Offset, ArrayStride, and MatrixStride can also decorate types and type members used by variables in the Private and Function storage classes.

Vulkan 1.2 is considered SPIR-V 1.5 (for the assembler) so that prevents putting layouts on types used in function variables.

Looks like I misread the spec though and should change the conditions to <= 1.4.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, this was in the Shader Interfaces section of the Vulkan Spec (I was searching only in the SPIR-V spec)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That fix is #6023

case spv::StorageClass::Input:
case spv::StorageClass::Output:
// Block is used generally and mesh shaders use Offset.
return true;
default:
// TODO: Some storage classes in ray tracing use explicit layout
// decorations, but it is not well documented which. For now treat other
// storage classes as allowed to be laid out. See Vulkan internal issue
// 4192.
return true;
}
}

bool UsesExplicitLayout(ValidationState_t& vstate, uint32_t type_id,
std::unordered_map<uint32_t, bool>& cache) {
if (type_id == 0) {
return false;
}

if (cache.count(type_id)) {
return cache[type_id];
}

bool res = false;
const auto type_inst = vstate.FindDef(type_id);
if (type_inst->opcode() == spv::Op::OpTypeStruct ||
type_inst->opcode() == spv::Op::OpTypeArray ||
type_inst->opcode() == spv::Op::OpTypeRuntimeArray ||
type_inst->opcode() == spv::Op::OpTypePointer ||
type_inst->opcode() == spv::Op::OpTypeUntypedPointerKHR) {
const auto& id_decs = vstate.id_decorations();
const auto iter = id_decs.find(type_id);
if (iter != id_decs.end()) {
res = std::any_of(iter->second.begin(), iter->second.end(),
[](const Decoration& d) {
return d.dec_type() == spv::Decoration::Block ||
d.dec_type() == spv::Decoration::BufferBlock ||
d.dec_type() == spv::Decoration::Offset ||
d.dec_type() == spv::Decoration::ArrayStride ||
d.dec_type() == spv::Decoration::MatrixStride;
});
}

if (!res) {
switch (type_inst->opcode()) {
case spv::Op::OpTypeStruct:
for (uint32_t i = 1; !res && i < type_inst->operands().size(); i++) {
res = UsesExplicitLayout(
vstate, type_inst->GetOperandAs<uint32_t>(i), cache);
}
break;
case spv::Op::OpTypeArray:
case spv::Op::OpTypeRuntimeArray:
res = UsesExplicitLayout(vstate, type_inst->GetOperandAs<uint32_t>(1),
cache);
break;
case spv::Op::OpTypePointer: {
const auto sc = type_inst->GetOperandAs<spv::StorageClass>(1);
if (!AllowsLayout(vstate, sc)) {
res = UsesExplicitLayout(
vstate, type_inst->GetOperandAs<uint32_t>(2), cache);
}
}
default:
break;
}
}
}

cache[type_id] = res;
return res;
}

spv_result_t CheckInvalidVulkanExplicitLayout(ValidationState_t& vstate) {
if (!spvIsVulkanEnv(vstate.context()->target_env)) {
return SPV_SUCCESS;
}

std::unordered_map<uint32_t, bool> cache;
for (const auto& inst : vstate.ordered_instructions()) {
const auto type_id = inst.type_id();
const auto type_inst = vstate.FindDef(type_id);
uint32_t fail_id = 0;
// Variables are the main place to check for improper decorations, but some
// untyped pointer instructions must also be checked since those types may
// never be instantiated by a variable. Unlike verifying a valid layout,
// physical storage buffer does not need checked here since it is always
// explicitly laid out.
switch (inst.opcode()) {
case spv::Op::OpVariable:
case spv::Op::OpUntypedVariableKHR: {
const auto sc = inst.GetOperandAs<spv::StorageClass>(2);
auto check_id = type_id;
if (inst.opcode() == spv::Op::OpUntypedVariableKHR) {
if (inst.operands().size() > 3) {
check_id = inst.GetOperandAs<uint32_t>(3);
}
}
if (!AllowsLayout(vstate, sc) &&
UsesExplicitLayout(vstate, check_id, cache)) {
fail_id = check_id;
}
break;
}
case spv::Op::OpUntypedAccessChainKHR:
case spv::Op::OpUntypedInBoundsAccessChainKHR:
case spv::Op::OpUntypedPtrAccessChainKHR:
case spv::Op::OpUntypedInBoundsPtrAccessChainKHR: {
// Check both the base type and return type. The return type may have an
// invalid array stride.
const auto sc = type_inst->GetOperandAs<spv::StorageClass>(1);
const auto base_type_id = inst.GetOperandAs<uint32_t>(2);
if (!AllowsLayout(vstate, sc)) {
if (UsesExplicitLayout(vstate, base_type_id, cache)) {
fail_id = base_type_id;
} else if (UsesExplicitLayout(vstate, type_id, cache)) {
fail_id = type_id;
}
}
break;
}
case spv::Op::OpUntypedArrayLengthKHR: {
// Check the data type.
const auto ptr_ty_id =
vstate.FindDef(inst.GetOperandAs<uint32_t>(3))->type_id();
const auto ptr_ty = vstate.FindDef(ptr_ty_id);
const auto sc = ptr_ty->GetOperandAs<spv::StorageClass>(1);
const auto base_type_id = inst.GetOperandAs<uint32_t>(2);
if (!AllowsLayout(vstate, sc) &&
UsesExplicitLayout(vstate, base_type_id, cache)) {
fail_id = base_type_id;
}
break;
}
case spv::Op::OpLoad: {
const auto ptr_id = inst.GetOperandAs<uint32_t>(2);
const auto ptr_type = vstate.FindDef(vstate.FindDef(ptr_id)->type_id());
if (ptr_type->opcode() == spv::Op::OpTypeUntypedPointerKHR) {
// For untyped pointers check the return type for an invalid layout.
const auto sc = ptr_type->GetOperandAs<spv::StorageClass>(1);
if (!AllowsLayout(vstate, sc) &&
UsesExplicitLayout(vstate, type_id, cache)) {
fail_id = type_id;
}
}
break;
}
case spv::Op::OpStore: {
const auto ptr_id = inst.GetOperandAs<uint32_t>(1);
const auto ptr_type = vstate.FindDef(vstate.FindDef(ptr_id)->type_id());
if (ptr_type->opcode() == spv::Op::OpTypeUntypedPointerKHR) {
// For untyped pointers, check the type of the data operand for an
// invalid layout.
const auto sc = ptr_type->GetOperandAs<spv::StorageClass>(1);
const auto data_type_id = vstate.GetOperandTypeId(&inst, 2);
if (!AllowsLayout(vstate, sc) &&
UsesExplicitLayout(vstate, data_type_id, cache)) {
fail_id = inst.GetOperandAs<uint32_t>(2);
}
}
break;
}
default:
break;
}
if (fail_id != 0) {
return vstate.diag(SPV_ERROR_INVALID_ID, &inst)
<< "Invalid explicit layout decorations on type for operand "
<< vstate.getIdName(fail_id);
}
}

return SPV_SUCCESS;
}

} // namespace

spv_result_t ValidateDecorations(ValidationState_t& vstate) {
Expand All @@ -2094,6 +2285,7 @@ spv_result_t ValidateDecorations(ValidationState_t& vstate) {
if (auto error = CheckVulkanMemoryModelDeprecatedDecorations(vstate))
return error;
if (auto error = CheckDecorationsFromDecoration(vstate)) return error;
if (auto error = CheckInvalidVulkanExplicitLayout(vstate)) return error;
return SPV_SUCCESS;
}

Expand Down
74 changes: 45 additions & 29 deletions test/fuzz/transformation_set_memory_operands_mask_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,37 +333,45 @@ TEST(TransformationSetMemoryOperandsMaskTest, Spirv14OrHigher) {
%3 = OpTypeFunction %2
%6 = OpTypeFloat 32
%7 = OpTypeStruct %6 %6 %6
%1001 = OpTypeStruct %6 %6 %6
%8 = OpTypeInt 32 0
%9 = OpConstant %8 12
%10 = OpTypeArray %7 %9
%11 = OpTypePointer Private %10
%1002 = OpTypeArray %1001 %9
%11 = OpTypePointer Private %1002
%12 = OpVariable %11 Private
%15 = OpTypeStruct %10 %7
%16 = OpTypePointer Uniform %15
%17 = OpVariable %16 Uniform
%18 = OpTypeInt 32 1
%19 = OpConstant %18 0
%20 = OpTypePointer Uniform %10
%24 = OpTypePointer Private %7
%24 = OpTypePointer Private %1001
%27 = OpTypePointer Private %6
%30 = OpConstant %18 1
%132 = OpTypePointer Function %10
%132 = OpTypePointer Function %1002
%135 = OpTypePointer Uniform %7
%145 = OpTypePointer Function %7
%145 = OpTypePointer Function %1001
%4 = OpFunction %2 None %3
%5 = OpLabel
%133 = OpVariable %132 Function
%21 = OpAccessChain %20 %17 %19
OpCopyMemory %12 %21 Aligned 16 Nontemporal|Aligned 16
%1003 = OpLoad %10 %21 Nontemporal|Aligned 16
%1004 = OpCopyLogical %1002 %1003
OpStore %12 %1004 Aligned 16
OpCopyMemory %133 %12 Volatile
OpCopyMemory %133 %12
OpCopyMemory %133 %12
%136 = OpAccessChain %135 %17 %30
%138 = OpAccessChain %24 %12 %19
OpCopyMemory %138 %136 None Aligned 16
OpCopyMemory %138 %136 Aligned 16
%1005 = OpLoad %7 %136 Aligned 16
%1006 = OpCopyLogical %1001 %1005
OpStore %138 %1006
%1007 = OpLoad %7 %136
%1008 = OpCopyLogical %1001 %1007
OpStore %138 %1008 Aligned 16
%146 = OpAccessChain %145 %133 %30
%147 = OpLoad %7 %146 Volatile|Nontemporal|Aligned 16
%147 = OpLoad %1001 %146 Volatile|Nontemporal|Aligned 16
%148 = OpAccessChain %24 %12 %19
OpStore %148 %147 Nontemporal
OpReturn
Expand All @@ -382,14 +390,14 @@ TEST(TransformationSetMemoryOperandsMaskTest, Spirv14OrHigher) {
MakeUnique<FactManager>(context.get()), validator_options);
{
TransformationSetMemoryOperandsMask transformation(
MakeInstructionDescriptor(21, spv::Op::OpCopyMemory, 0),
MakeInstructionDescriptor(21, spv::Op::OpLoad, 0),
(uint32_t)spv::MemoryAccessMask::Aligned |
(uint32_t)spv::MemoryAccessMask::Volatile,
1);
0);
// Bad: cannot remove aligned
ASSERT_FALSE(TransformationSetMemoryOperandsMask(
MakeInstructionDescriptor(21, spv::Op::OpCopyMemory, 0),
(uint32_t)spv::MemoryAccessMask::Volatile, 1)
MakeInstructionDescriptor(21, spv::Op::OpLoad, 0),
(uint32_t)spv::MemoryAccessMask::Volatile, 0)
.IsApplicable(context.get(), transformation_context));
ASSERT_TRUE(
transformation.IsApplicable(context.get(), transformation_context));
Expand All @@ -399,13 +407,13 @@ TEST(TransformationSetMemoryOperandsMaskTest, Spirv14OrHigher) {

{
TransformationSetMemoryOperandsMask transformation(
MakeInstructionDescriptor(21, spv::Op::OpCopyMemory, 1),
MakeInstructionDescriptor(21, spv::Op::OpCopyMemory, 0),
(uint32_t)spv::MemoryAccessMask::Nontemporal |
(uint32_t)spv::MemoryAccessMask::Volatile,
1);
// Bad: cannot remove volatile
ASSERT_FALSE(TransformationSetMemoryOperandsMask(
MakeInstructionDescriptor(21, spv::Op::OpCopyMemory, 1),
MakeInstructionDescriptor(21, spv::Op::OpCopyMemory, 0),
(uint32_t)spv::MemoryAccessMask::Nontemporal, 0)
.IsApplicable(context.get(), transformation_context));
ASSERT_TRUE(
Expand All @@ -417,7 +425,7 @@ TEST(TransformationSetMemoryOperandsMaskTest, Spirv14OrHigher) {
{
// Creates the first operand.
TransformationSetMemoryOperandsMask transformation(
MakeInstructionDescriptor(21, spv::Op::OpCopyMemory, 2),
MakeInstructionDescriptor(21, spv::Op::OpCopyMemory, 1),
(uint32_t)spv::MemoryAccessMask::Nontemporal |
(uint32_t)spv::MemoryAccessMask::Volatile,
0);
Expand All @@ -430,7 +438,7 @@ TEST(TransformationSetMemoryOperandsMaskTest, Spirv14OrHigher) {
{
// Creates both operands.
TransformationSetMemoryOperandsMask transformation(
MakeInstructionDescriptor(21, spv::Op::OpCopyMemory, 3),
MakeInstructionDescriptor(21, spv::Op::OpCopyMemory, 2),
(uint32_t)spv::MemoryAccessMask::Nontemporal |
(uint32_t)spv::MemoryAccessMask::Volatile,
1);
Expand All @@ -442,13 +450,13 @@ TEST(TransformationSetMemoryOperandsMaskTest, Spirv14OrHigher) {

{
TransformationSetMemoryOperandsMask transformation(
MakeInstructionDescriptor(138, spv::Op::OpCopyMemory, 0),
MakeInstructionDescriptor(138, spv::Op::OpLoad, 0),
(uint32_t)spv::MemoryAccessMask::Aligned |
(uint32_t)spv::MemoryAccessMask::Nontemporal,
1);
0);
// Bad: the first mask is None, so Aligned cannot be added to it.
ASSERT_FALSE(TransformationSetMemoryOperandsMask(
MakeInstructionDescriptor(138, spv::Op::OpCopyMemory, 0),
MakeInstructionDescriptor(138, spv::Op::OpStore, 0),
(uint32_t)spv::MemoryAccessMask::Aligned |
(uint32_t)spv::MemoryAccessMask::Nontemporal,
0)
Expand All @@ -461,8 +469,8 @@ TEST(TransformationSetMemoryOperandsMaskTest, Spirv14OrHigher) {

{
TransformationSetMemoryOperandsMask transformation(
MakeInstructionDescriptor(138, spv::Op::OpCopyMemory, 1),
(uint32_t)spv::MemoryAccessMask::Volatile, 1);
MakeInstructionDescriptor(138, spv::Op::OpLoad, 1),
(uint32_t)spv::MemoryAccessMask::Volatile, 0);
ASSERT_TRUE(
transformation.IsApplicable(context.get(), transformation_context));
ApplyAndCheckFreshIds(transformation, context.get(),
Expand Down Expand Up @@ -522,37 +530,45 @@ TEST(TransformationSetMemoryOperandsMaskTest, Spirv14OrHigher) {
%3 = OpTypeFunction %2
%6 = OpTypeFloat 32
%7 = OpTypeStruct %6 %6 %6
%1001 = OpTypeStruct %6 %6 %6
%8 = OpTypeInt 32 0
%9 = OpConstant %8 12
%10 = OpTypeArray %7 %9
%11 = OpTypePointer Private %10
%1002 = OpTypeArray %1001 %9
%11 = OpTypePointer Private %1002
%12 = OpVariable %11 Private
%15 = OpTypeStruct %10 %7
%16 = OpTypePointer Uniform %15
%17 = OpVariable %16 Uniform
%18 = OpTypeInt 32 1
%19 = OpConstant %18 0
%20 = OpTypePointer Uniform %10
%24 = OpTypePointer Private %7
%24 = OpTypePointer Private %1001
%27 = OpTypePointer Private %6
%30 = OpConstant %18 1
%132 = OpTypePointer Function %10
%132 = OpTypePointer Function %1002
%135 = OpTypePointer Uniform %7
%145 = OpTypePointer Function %7
%145 = OpTypePointer Function %1001
%4 = OpFunction %2 None %3
%5 = OpLabel
%133 = OpVariable %132 Function
%21 = OpAccessChain %20 %17 %19
OpCopyMemory %12 %21 Aligned 16 Aligned|Volatile 16
%1003 = OpLoad %10 %21 Aligned|Volatile 16
%1004 = OpCopyLogical %1002 %1003
OpStore %12 %1004 Aligned 16
OpCopyMemory %133 %12 Volatile Nontemporal|Volatile
OpCopyMemory %133 %12 Nontemporal|Volatile
OpCopyMemory %133 %12 None Nontemporal|Volatile
%136 = OpAccessChain %135 %17 %30
%138 = OpAccessChain %24 %12 %19
OpCopyMemory %138 %136 None Aligned|Nontemporal 16
OpCopyMemory %138 %136 Aligned 16 Volatile
%1005 = OpLoad %7 %136 Aligned|Nontemporal 16
%1006 = OpCopyLogical %1001 %1005
OpStore %138 %1006
%1007 = OpLoad %7 %136 Volatile
%1008 = OpCopyLogical %1001 %1007
OpStore %138 %1008 Aligned 16
%146 = OpAccessChain %145 %133 %30
%147 = OpLoad %7 %146 Volatile|Aligned 16
%147 = OpLoad %1001 %146 Volatile|Aligned 16
%148 = OpAccessChain %24 %12 %19
OpStore %148 %147 None
OpReturn
Expand Down
Loading