Validate duplicate decorations and execution modes (#5641)
* Disallow duplicate decorations generally
* Only FuncParamAttr and UserSemantic can be applied to the same target
multiple times
* Unchecked: completely duplicate UserSemantic and FuncParamAttr
* Disallow duplicate execution modes generally
* Exceptions for float controls, float controls2 and some intel
execution modes
* Fix invalid fuzzer transformsdiff --git a/source/fuzz/transformation_add_no_contraction_decoration.cpp b/source/fuzz/transformation_add_no_contraction_decoration.cpp
index 07a31e5..87393e9 100644
--- a/source/fuzz/transformation_add_no_contraction_decoration.cpp
+++ b/source/fuzz/transformation_add_no_contraction_decoration.cpp
@@ -36,6 +36,11 @@
if (!instr) {
return false;
}
+ // |instr| must not be decorated with NoContraction.
+ if (ir_context->get_decoration_mgr()->HasDecoration(
+ message_.result_id(), spv::Decoration::NoContraction)) {
+ return false;
+ }
// The instruction must be arithmetic.
return IsArithmetic(instr->opcode());
}
diff --git a/source/fuzz/transformation_add_relaxed_decoration.cpp b/source/fuzz/transformation_add_relaxed_decoration.cpp
index 6cd4ecb..601546c 100644
--- a/source/fuzz/transformation_add_relaxed_decoration.cpp
+++ b/source/fuzz/transformation_add_relaxed_decoration.cpp
@@ -36,6 +36,11 @@
if (!instr) {
return false;
}
+ // |instr| must not be decorated with RelaxedPrecision.
+ if (ir_context->get_decoration_mgr()->HasDecoration(
+ message_.result_id(), spv::Decoration::RelaxedPrecision)) {
+ return false;
+ }
opt::BasicBlock* cur_block = ir_context->get_instr_block(instr);
// The instruction must have a block.
if (cur_block == nullptr) {
@@ -46,6 +51,7 @@
cur_block->id()))) {
return false;
}
+
// The instruction must be numeric.
return IsNumeric(instr->opcode());
}
diff --git a/source/val/validate.cpp b/source/val/validate.cpp
index ccf26fb..3236807 100644
--- a/source/val/validate.cpp
+++ b/source/val/validate.cpp
@@ -144,6 +144,9 @@
if (auto error = ValidateFloatControls2(_)) {
return error;
}
+ if (auto error = ValidateDuplicateExecutionModes(_)) {
+ return error;
+ }
return SPV_SUCCESS;
}
diff --git a/source/val/validate.h b/source/val/validate.h
index 52267c8..78093ce 100644
--- a/source/val/validate.h
+++ b/source/val/validate.h
@@ -94,6 +94,13 @@
/// @return SPV_SUCCESS if no errors are found.
spv_result_t ValidateFloatControls2(ValidationState_t& _);
+/// @brief Validates duplicated execution modes for each entry point.
+///
+/// @param[in] _ the validation state of the module
+///
+/// @return SPV_SUCCESS if no errors are found.
+spv_result_t ValidateDuplicateExecutionModes(ValidationState_t& _);
+
/// @brief Validates memory instructions
///
/// @param[in] _ the validation state of the module
diff --git a/source/val/validate_decorations.cpp b/source/val/validate_decorations.cpp
index bb1fea5..0a7df65 100644
--- a/source/val/validate_decorations.cpp
+++ b/source/val/validate_decorations.cpp
@@ -1325,21 +1325,14 @@
// Returns true if |decoration| cannot be applied to the same id more than once.
bool AtMostOncePerId(spv::Decoration decoration) {
- return decoration == spv::Decoration::ArrayStride;
+ return decoration != spv::Decoration::UserSemantic &&
+ decoration != spv::Decoration::FuncParamAttr;
}
// Returns true if |decoration| cannot be applied to the same member more than
// once.
bool AtMostOncePerMember(spv::Decoration decoration) {
- switch (decoration) {
- case spv::Decoration::Offset:
- case spv::Decoration::MatrixStride:
- case spv::Decoration::RowMajor:
- case spv::Decoration::ColMajor:
- return true;
- default:
- return false;
- }
+ return decoration != spv::Decoration::UserSemantic;
}
spv_result_t CheckDecorationsCompatibility(ValidationState_t& vstate) {
diff --git a/source/val/validate_interfaces.cpp b/source/val/validate_interfaces.cpp
index 54ebfd7..ace548a 100644
--- a/source/val/validate_interfaces.cpp
+++ b/source/val/validate_interfaces.cpp
@@ -254,37 +254,24 @@
// equal. Also track Patch and PerTaskNV decorations.
bool has_location = false;
uint32_t location = 0;
- bool has_component = false;
uint32_t component = 0;
bool has_index = false;
uint32_t index = 0;
bool has_patch = false;
bool has_per_task_nv = false;
bool has_per_vertex_khr = false;
+ // Duplicate Location, Component, Index are checked elsewhere.
for (auto& dec : _.id_decorations(variable->id())) {
if (dec.dec_type() == spv::Decoration::Location) {
- if (has_location && dec.params()[0] != location) {
- return _.diag(SPV_ERROR_INVALID_DATA, variable)
- << "Variable has conflicting location decorations";
- }
has_location = true;
location = dec.params()[0];
} else if (dec.dec_type() == spv::Decoration::Component) {
- if (has_component && dec.params()[0] != component) {
- return _.diag(SPV_ERROR_INVALID_DATA, variable)
- << "Variable has conflicting component decorations";
- }
- has_component = true;
component = dec.params()[0];
} else if (dec.dec_type() == spv::Decoration::Index) {
if (!is_output || !is_fragment) {
return _.diag(SPV_ERROR_INVALID_DATA, variable)
<< "Index can only be applied to Fragment output variables";
}
- if (has_index && dec.params()[0] != index) {
- return _.diag(SPV_ERROR_INVALID_DATA, variable)
- << "Variable has conflicting index decorations";
- }
has_index = true;
index = dec.params()[0];
} else if (dec.dec_type() == spv::Decoration::BuiltIn) {
diff --git a/source/val/validate_mode_setting.cpp b/source/val/validate_mode_setting.cpp
index 199d8ed..8502fda 100644
--- a/source/val/validate_mode_setting.cpp
+++ b/source/val/validate_mode_setting.cpp
@@ -724,6 +724,25 @@
return SPV_SUCCESS;
}
+bool PerEntryExecutionMode(spv::ExecutionMode mode) {
+ switch (mode) {
+ // These execution modes can be specified multiple times per entry point.
+ case spv::ExecutionMode::DenormPreserve:
+ case spv::ExecutionMode::DenormFlushToZero:
+ case spv::ExecutionMode::SignedZeroInfNanPreserve:
+ case spv::ExecutionMode::RoundingModeRTE:
+ case spv::ExecutionMode::RoundingModeRTZ:
+ case spv::ExecutionMode::FPFastMathDefault:
+ case spv::ExecutionMode::RoundingModeRTPINTEL:
+ case spv::ExecutionMode::RoundingModeRTNINTEL:
+ case spv::ExecutionMode::FloatingPointModeALTINTEL:
+ case spv::ExecutionMode::FloatingPointModeIEEEINTEL:
+ return false;
+ default:
+ return true;
+ }
+}
+
} // namespace
spv_result_t ValidateFloatControls2(ValidationState_t& _) {
@@ -808,5 +827,52 @@
return SPV_SUCCESS;
}
+spv_result_t ValidateDuplicateExecutionModes(ValidationState_t& _) {
+ using PerEntryKey = std::tuple<spv::ExecutionMode, uint32_t>;
+ using PerOperandKey = std::tuple<spv::ExecutionMode, uint32_t, uint32_t>;
+ std::set<PerEntryKey> seen_per_entry;
+ std::set<PerOperandKey> seen_per_operand;
+
+ const auto lookupMode = [&_](spv::ExecutionMode mode) -> std::string {
+ spv_operand_desc desc = nullptr;
+ if (_.grammar().lookupOperand(SPV_OPERAND_TYPE_EXECUTION_MODE,
+ static_cast<uint32_t>(mode),
+ &desc) == SPV_SUCCESS) {
+ return std::string(desc->name);
+ }
+ return "Unknown";
+ };
+
+ for (const auto& inst : _.ordered_instructions()) {
+ if (inst.opcode() != spv::Op::OpExecutionMode &&
+ inst.opcode() != spv::Op::OpExecutionModeId) {
+ continue;
+ }
+
+ const auto entry = inst.GetOperandAs<uint32_t>(0);
+ const auto mode = inst.GetOperandAs<spv::ExecutionMode>(1);
+ if (PerEntryExecutionMode(mode)) {
+ if (!seen_per_entry.insert(std::make_tuple(mode, entry)).second) {
+ return _.diag(SPV_ERROR_INVALID_ID, &inst)
+ << lookupMode(mode)
+ << " execution mode must not be specified multiple times per "
+ "entry point";
+ }
+ } else {
+ // Execution modes allowed multiple times all take a single operand.
+ const auto operand = inst.GetOperandAs<uint32_t>(2);
+ if (!seen_per_operand.insert(std::make_tuple(mode, entry, operand))
+ .second) {
+ return _.diag(SPV_ERROR_INVALID_ID, &inst)
+ << lookupMode(mode)
+ << " execution mode must not be specified multiple times for "
+ "the same entry point and operands";
+ }
+ }
+ }
+
+ return SPV_SUCCESS;
+}
+
} // namespace val
} // namespace spvtools
diff --git a/test/fuzz/transformation_add_no_contraction_decoration_test.cpp b/test/fuzz/transformation_add_no_contraction_decoration_test.cpp
index 4fc9d2d..1280b81 100644
--- a/test/fuzz/transformation_add_no_contraction_decoration_test.cpp
+++ b/test/fuzz/transformation_add_no_contraction_decoration_test.cpp
@@ -36,7 +36,6 @@
OpName %8 "x"
OpName %10 "y"
OpName %14 "i"
- OpDecorate %32 NoContraction
%2 = OpTypeVoid
%3 = OpTypeFunction %2
%6 = OpTypeFloat 32
@@ -110,9 +109,8 @@
ASSERT_FALSE(TransformationAddNoContractionDecoration(24).IsApplicable(
context.get(), transformation_context));
- // It is valid to add NoContraction to each of these ids (and it's fine to
- // have duplicates of the decoration, in the case of 32).
- for (uint32_t result_id : {32u, 32u, 27u, 29u, 39u}) {
+ // It is valid to add NoContraction to each of these ids.
+ for (uint32_t result_id : {32u, 27u, 29u, 39u}) {
TransformationAddNoContractionDecoration transformation(result_id);
ASSERT_TRUE(
transformation.IsApplicable(context.get(), transformation_context));
@@ -134,8 +132,6 @@
OpName %10 "y"
OpName %14 "i"
OpDecorate %32 NoContraction
- OpDecorate %32 NoContraction
- OpDecorate %32 NoContraction
OpDecorate %27 NoContraction
OpDecorate %29 NoContraction
OpDecorate %39 NoContraction
diff --git a/test/fuzz/transformation_add_relaxed_decoration_test.cpp b/test/fuzz/transformation_add_relaxed_decoration_test.cpp
index c440882..979eeb7 100644
--- a/test/fuzz/transformation_add_relaxed_decoration_test.cpp
+++ b/test/fuzz/transformation_add_relaxed_decoration_test.cpp
@@ -86,9 +86,8 @@
// Invalid: 28 is in a dead block, but returns bool (not numeric).
ASSERT_FALSE(TransformationAddRelaxedDecoration(28).IsApplicable(
context.get(), transformation_context));
- // It is valid to add RelaxedPrecision to 25 (and it's fine to
- // have a duplicate).
- for (uint32_t result_id : {25u, 25u}) {
+ // It is valid to add RelaxedPrecision to 25
+ for (uint32_t result_id : {25u}) {
TransformationAddRelaxedDecoration transformation(result_id);
ASSERT_TRUE(
transformation.IsApplicable(context.get(), transformation_context));
@@ -110,7 +109,6 @@
OpName %10 "b"
OpName %14 "c"
OpDecorate %25 RelaxedPrecision
- OpDecorate %25 RelaxedPrecision
%2 = OpTypeVoid
%3 = OpTypeFunction %2
%6 = OpTypeInt 32 1
diff --git a/test/val/val_interfaces_test.cpp b/test/val/val_interfaces_test.cpp
index d75c20c..4f62be7 100644
--- a/test/val/val_interfaces_test.cpp
+++ b/test/val/val_interfaces_test.cpp
@@ -419,9 +419,10 @@
)";
CompileSuccessfully(text, SPV_ENV_VULKAN_1_0);
- EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_0));
- EXPECT_THAT(getDiagnosticString(),
- HasSubstr("Variable has conflicting location decorations"));
+ EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_0));
+ EXPECT_THAT(
+ getDiagnosticString(),
+ HasSubstr("decorated with Location multiple times is not allowed"));
}
TEST_F(ValidateInterfacesTest, VulkanLocationsVariableAndMemberAssigned) {
@@ -505,9 +506,10 @@
)";
CompileSuccessfully(text, SPV_ENV_VULKAN_1_0);
- EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_0));
- EXPECT_THAT(getDiagnosticString(),
- HasSubstr("Member index 1 has conflicting location assignments"));
+ EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_0));
+ EXPECT_THAT(
+ getDiagnosticString(),
+ HasSubstr("decorated with Location multiple times is not allowed"));
}
TEST_F(ValidateInterfacesTest, VulkanLocationsMissingAssignmentStructMember) {
@@ -1157,9 +1159,10 @@
)";
CompileSuccessfully(text, SPV_ENV_VULKAN_1_0);
- EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_0));
- EXPECT_THAT(getDiagnosticString(),
- HasSubstr("Variable has conflicting component decorations"));
+ EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_0));
+ EXPECT_THAT(
+ getDiagnosticString(),
+ HasSubstr("decorated with Component multiple times is not allowed"));
}
TEST_F(ValidateInterfacesTest,
@@ -1186,10 +1189,10 @@
)";
CompileSuccessfully(text, SPV_ENV_VULKAN_1_0);
- EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_0));
+ EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_0));
EXPECT_THAT(
getDiagnosticString(),
- HasSubstr("Member index 0 has conflicting component assignments"));
+ HasSubstr("decorated with Component multiple times is not allowed"));
}
TEST_F(ValidateInterfacesTest, VulkanLocationsVariableConflictOutputIndex1) {
diff --git a/test/val/val_modes_test.cpp b/test/val/val_modes_test.cpp
index a0ea428..83a0503 100644
--- a/test/val/val_modes_test.cpp
+++ b/test/val/val_modes_test.cpp
@@ -1027,6 +1027,162 @@
"constant instructions."));
}
+using AllowMultipleExecutionModes = spvtest::ValidateBase<std::string>;
+
+TEST_P(AllowMultipleExecutionModes, DifferentOperand) {
+ const std::string mode = GetParam();
+ const std::string spirv = R"(
+OpCapability Shader
+OpCapability DenormPreserve
+OpCapability DenormFlushToZero
+OpCapability SignedZeroInfNanPreserve
+OpCapability RoundingModeRTE
+OpCapability RoundingModeRTZ
+OpExtension "SPV_KHR_float_controls"
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %main "main"
+OpExecutionMode %main LocalSize 1 1 1
+OpExecutionMode %main )" + mode +
+ R"( 16
+OpExecutionMode %main )" + mode +
+ R"( 32
+%void = OpTypeVoid
+%void_fn = OpTypeFunction %void
+%main = OpFunction %void None %void_fn
+%entry = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+ CompileSuccessfully(spirv);
+ EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
+}
+
+TEST_P(AllowMultipleExecutionModes, SameOperand) {
+ const std::string mode = GetParam();
+ const std::string spirv = R"(
+OpCapability Shader
+OpCapability DenormPreserve
+OpCapability DenormFlushToZero
+OpCapability SignedZeroInfNanPreserve
+OpCapability RoundingModeRTE
+OpCapability RoundingModeRTZ
+OpExtension "SPV_KHR_float_controls"
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %main "main"
+OpExecutionMode %main LocalSize 1 1 1
+OpExecutionMode %main )" + mode +
+ R"( 32
+OpExecutionMode %main )" + mode +
+ R"( 32
+%void = OpTypeVoid
+%void_fn = OpTypeFunction %void
+%main = OpFunction %void None %void_fn
+%entry = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+ CompileSuccessfully(spirv);
+ EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+ EXPECT_THAT(getDiagnosticString(),
+ HasSubstr("execution mode must not be specified multiple times "
+ "for the same entry point and operands"));
+}
+
+INSTANTIATE_TEST_SUITE_P(MultipleFloatControlsExecModes,
+ AllowMultipleExecutionModes,
+ Values("DenormPreserve", "DenormFlushToZero",
+ "SignedZeroInfNanPreserve", "RoundingModeRTE",
+ "RoundingModeRTZ"));
+
+using MultipleExecModes = spvtest::ValidateBase<std::string>;
+
+TEST_P(MultipleExecModes, DuplicateMode) {
+ const std::string mode = GetParam();
+ const std::string spirv = R"(
+OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint Fragment %main "main"
+OpExecutionMode %main OriginUpperLeft
+OpExecutionMode %main )" + mode +
+ R"(
+OpExecutionMode %main )" + mode +
+ R"(
+%void = OpTypeVoid
+%void_fn = OpTypeFunction %void
+%main = OpFunction %void None %void_fn
+%entry = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+ CompileSuccessfully(spirv);
+ EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+ EXPECT_THAT(getDiagnosticString(),
+ HasSubstr("execution mode must not be specified multiple times "
+ "per entry point"));
+}
+
+INSTANTIATE_TEST_SUITE_P(MultipleFragmentExecMode, MultipleExecModes,
+ Values("DepthReplacing", "DepthGreater", "DepthLess",
+ "DepthUnchanged"));
+
+TEST_F(ValidateMode, FloatControls2FPFastMathDefaultSameOperand) {
+ const std::string spirv = R"(
+OpCapability Shader
+OpCapability FloatControls2
+OpExtension "SPV_KHR_float_controls2"
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %main "main"
+OpExecutionMode %main LocalSize 1 1 1
+OpExecutionModeId %main FPFastMathDefault %float %none
+OpExecutionModeId %main FPFastMathDefault %float %none
+%void = OpTypeVoid
+%float = OpTypeFloat 32
+%int = OpTypeInt 32 0
+%none = OpConstant %int 0
+%void_fn = OpTypeFunction %void
+%main = OpFunction %void None %void_fn
+%entry = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+ CompileSuccessfully(spirv, SPV_ENV_UNIVERSAL_1_2);
+ EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_UNIVERSAL_1_2));
+ EXPECT_THAT(getDiagnosticString(),
+ HasSubstr("execution mode must not be specified multiple times "
+ "for the same entry point and operands"));
+}
+
+TEST_F(ValidateMode, FloatControls2FPFastMathDefaultDifferentOperand) {
+ const std::string spirv = R"(
+OpCapability Shader
+OpCapability Float16
+OpCapability FloatControls2
+OpExtension "SPV_KHR_float_controls2"
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %main "main"
+OpExecutionMode %main LocalSize 1 1 1
+OpExecutionModeId %main FPFastMathDefault %float %none
+OpExecutionModeId %main FPFastMathDefault %half %none
+%void = OpTypeVoid
+%float = OpTypeFloat 32
+%int = OpTypeInt 32 0
+%none = OpConstant %int 0
+%half = OpTypeFloat 16
+%void_fn = OpTypeFunction %void
+%main = OpFunction %void None %void_fn
+%entry = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+ CompileSuccessfully(spirv, SPV_ENV_UNIVERSAL_1_2);
+ EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_UNIVERSAL_1_2));
+}
+
TEST_F(ValidateMode, FragmentShaderInterlockVertexBad) {
const std::string spirv = R"(
OpCapability Shader
