Validate PushConstants annotation and type (#2140)
* Validate PushConstants have Block annotation and are struct or array of structs
* Add passing test and split into universal/vulkan environment tests
diff --git a/source/val/validate_decorations.cpp b/source/val/validate_decorations.cpp
index a5c2b62..96dd33d 100644
--- a/source/val/validate_decorations.cpp
+++ b/source/val/validate_decorations.cpp
@@ -795,6 +795,19 @@
MemberConstraints constraints;
ComputeMemberConstraintsForStruct(&constraints, id, LayoutConstraints(),
vstate);
+
+ // Vulkan 14.5.1: Check Block annotation for PushConstant variables.
+ if (spvIsVulkanEnv(vstate.context()->target_env)) {
+ if (push_constant && !hasDecoration(id, SpvDecorationBlock, vstate)) {
+ return vstate.diag(SPV_ERROR_INVALID_ID, vstate.FindDef(id))
+ << "PushConstant id '" << id
+ << "' is missing Block decoration.\n"
+ << "From Vulkan spec, section 14.5.1:\n"
+ << "Such variables must be identified with a Block "
+ "decoration";
+ }
+ }
+
// Prepare for messages
const char* sc_str =
uniform ? "Uniform"
diff --git a/source/val/validate_memory.cpp b/source/val/validate_memory.cpp
index 8bb9eed..21b8c8b 100644
--- a/source/val/validate_memory.cpp
+++ b/source/val/validate_memory.cpp
@@ -73,9 +73,9 @@
bool HaveLayoutCompatibleMembers(ValidationState_t& _, const Instruction* type1,
const Instruction* type2) {
assert(type1->opcode() == SpvOpTypeStruct &&
- "type1 must be and OpTypeStruct instruction.");
+ "type1 must be an OpTypeStruct instruction.");
assert(type2->opcode() == SpvOpTypeStruct &&
- "type2 must be and OpTypeStruct instruction.");
+ "type2 must be an OpTypeStruct instruction.");
const auto& type1_operands = type1->operands();
const auto& type2_operands = type2->operands();
if (type1_operands.size() != type2_operands.size()) {
@@ -100,9 +100,9 @@
bool HaveSameLayoutDecorations(ValidationState_t& _, const Instruction* type1,
const Instruction* type2) {
assert(type1->opcode() == SpvOpTypeStruct &&
- "type1 must be and OpTypeStruct instruction.");
+ "type1 must be an OpTypeStruct instruction.");
assert(type2->opcode() == SpvOpTypeStruct &&
- "type2 must be and OpTypeStruct instruction.");
+ "type2 must be an OpTypeStruct instruction.");
const std::vector<Decoration>& type1_decorations =
_.id_decorations(type1->id());
const std::vector<Decoration>& type2_decorations =
@@ -373,8 +373,20 @@
}
}
+ // Vulkan 14.5.1: Check type of PushConstant variables.
// Vulkan 14.5.2: Check type of UniformConstant and Uniform variables.
if (spvIsVulkanEnv(_.context()->target_env)) {
+ if (storage_class == SpvStorageClassPushConstant) {
+ if (!IsAllowedTypeOrArrayOfSame(_, pointee, {SpvOpTypeStruct})) {
+ return _.diag(SPV_ERROR_INVALID_ID, inst)
+ << "PushConstant OpVariable <id> '" << _.getIdName(inst->id())
+ << "' has illegal type.\n"
+ << "From Vulkan spec, section 14.5.1:\n"
+ << "Such variables must be typed as OpTypeStruct, "
+ << "or an array of this type";
+ }
+ }
+
if (storage_class == SpvStorageClassUniformConstant) {
if (!IsAllowedTypeOrArrayOfSame(
_, pointee,
@@ -382,7 +394,7 @@
SpvOpTypeAccelerationStructureNV})) {
return _.diag(SPV_ERROR_INVALID_ID, inst)
<< "UniformConstant OpVariable <id> '" << _.getIdName(inst->id())
- << " 'has illegal type.\n"
+ << "' has illegal type.\n"
<< "From Vulkan spec, section 14.5.2:\n"
<< "Variables identified with the UniformConstant storage class "
<< "are used only as handles to refer to opaque resources. Such "
@@ -396,7 +408,7 @@
if (!IsAllowedTypeOrArrayOfSame(_, pointee, {SpvOpTypeStruct})) {
return _.diag(SPV_ERROR_INVALID_ID, inst)
<< "Uniform OpVariable <id> '" << _.getIdName(inst->id())
- << " 'has illegal type.\n"
+ << "' has illegal type.\n"
<< "From Vulkan spec, section 14.5.2:\n"
<< "Variables identified with the Uniform storage class are "
"used "
diff --git a/test/val/val_decoration_test.cpp b/test/val/val_decoration_test.cpp
index bbf7e94..c67cf45 100644
--- a/test/val/val_decoration_test.cpp
+++ b/test/val/val_decoration_test.cpp
@@ -2219,6 +2219,65 @@
"rules: member 1 at offset 4 is not aligned to 16"));
}
+TEST_F(ValidateDecorations, PushConstantMissingBlockGood) {
+ std::string spirv = R"(
+ OpCapability Shader
+ OpMemoryModel Logical GLSL450
+ OpEntryPoint Fragment %1 "main"
+ OpExecutionMode %1 OriginUpperLeft
+
+ OpMemberDecorate %struct 0 Offset 0
+
+ %void = OpTypeVoid
+ %voidfn = OpTypeFunction %void
+ %float = OpTypeFloat 32
+ %struct = OpTypeStruct %float
+ %ptr = OpTypePointer PushConstant %struct
+ %pc = OpVariable %ptr PushConstant
+
+ %1 = OpFunction %void None %voidfn
+ %label = OpLabel
+ OpReturn
+ OpFunctionEnd
+)";
+
+ CompileSuccessfully(spirv);
+ EXPECT_EQ(SPV_SUCCESS, ValidateAndRetrieveValidationState())
+ << getDiagnosticString();
+}
+
+TEST_F(ValidateDecorations, VulkanPushConstantMissingBlockBad) {
+ std::string spirv = R"(
+ OpCapability Shader
+ OpMemoryModel Logical GLSL450
+ OpEntryPoint Fragment %1 "main"
+ OpExecutionMode %1 OriginUpperLeft
+
+ OpMemberDecorate %struct 0 Offset 0
+
+ %void = OpTypeVoid
+ %voidfn = OpTypeFunction %void
+ %float = OpTypeFloat 32
+ %struct = OpTypeStruct %float
+ %ptr = OpTypePointer PushConstant %struct
+ %pc = OpVariable %ptr PushConstant
+
+ %1 = OpFunction %void None %voidfn
+ %label = OpLabel
+ OpReturn
+ OpFunctionEnd
+)";
+
+ CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_1);
+ EXPECT_EQ(SPV_ERROR_INVALID_ID,
+ ValidateAndRetrieveValidationState(SPV_ENV_VULKAN_1_1));
+ EXPECT_THAT(getDiagnosticString(),
+ HasSubstr("PushConstant id '2' is missing Block decoration.\n"
+ "From Vulkan spec, section 14.5.1:\n"
+ "Such variables must be identified with a Block "
+ "decoration"));
+}
+
TEST_F(ValidateDecorations, StorageBufferStorageClassArrayBaseAlignmentGood) {
// Spot check buffer rules when using StorageBuffer storage class with Block
// decoration.
diff --git a/test/val/val_memory_test.cpp b/test/val/val_memory_test.cpp
index a2e8bf3..8001523 100644
--- a/test/val/val_memory_test.cpp
+++ b/test/val/val_memory_test.cpp
@@ -838,6 +838,82 @@
"The Struture's type in OpArrayLength <id> '12' must be a pointer to "
"an OpTypeStruct.\n %12 = OpArrayLength %uint %11 0\n"));
}
+
+TEST_F(ValidateMemory, PushConstantNotStructGood) {
+ std::string spirv = R"(
+ OpCapability Shader
+ OpMemoryModel Logical GLSL450
+ OpEntryPoint Fragment %1 "main"
+ OpExecutionMode %1 OriginUpperLeft
+
+ %void = OpTypeVoid
+ %voidfn = OpTypeFunction %void
+ %float = OpTypeFloat 32
+ %ptr = OpTypePointer PushConstant %float
+ %pc = OpVariable %ptr PushConstant
+
+ %1 = OpFunction %void None %voidfn
+ %label = OpLabel
+ OpReturn
+ OpFunctionEnd
+)";
+ CompileSuccessfully(spirv);
+ EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
+}
+
+TEST_F(ValidateMemory, VulkanPushConstantNotStructBad) {
+ std::string spirv = R"(
+ OpCapability Shader
+ OpMemoryModel Logical GLSL450
+ OpEntryPoint Fragment %1 "main"
+ OpExecutionMode %1 OriginUpperLeft
+
+ %void = OpTypeVoid
+ %voidfn = OpTypeFunction %void
+ %float = OpTypeFloat 32
+ %ptr = OpTypePointer PushConstant %float
+ %pc = OpVariable %ptr PushConstant
+
+ %1 = OpFunction %void None %voidfn
+ %label = OpLabel
+ OpReturn
+ OpFunctionEnd
+)";
+ CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_1);
+ EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_1));
+ EXPECT_THAT(getDiagnosticString(),
+ HasSubstr("PushConstant OpVariable <id> '6' has illegal type.\n"
+ "From Vulkan spec, section 14.5.1:\n"
+ "Such variables must be typed as OpTypeStruct, "
+ "or an array of this type"));
+}
+
+TEST_F(ValidateMemory, VulkanPushConstant) {
+ std::string spirv = R"(
+ OpCapability Shader
+ OpMemoryModel Logical GLSL450
+ OpEntryPoint Fragment %1 "main"
+ OpExecutionMode %1 OriginUpperLeft
+
+ OpDecorate %struct Block
+ OpMemberDecorate %struct 0 Offset 0
+
+ %void = OpTypeVoid
+ %voidfn = OpTypeFunction %void
+ %float = OpTypeFloat 32
+ %struct = OpTypeStruct %float
+ %ptr = OpTypePointer PushConstant %struct
+ %pc = OpVariable %ptr PushConstant
+
+ %1 = OpFunction %void None %voidfn
+ %label = OpLabel
+ OpReturn
+ OpFunctionEnd
+)";
+ CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_1);
+ EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_VULKAN_1_1));
+}
+
} // namespace
} // namespace val
} // namespace spvtools
