Update val to handle reversed instruction sections. (#3887)
Currently the validator, when checking an instruction is in the correct
section, always advances the current section. This means if we have an
instruction from a previous section we'll end up reporting it as invalid
in a function definition. This error is confusing.
This CL updates the validator to check if the given opcode is from a
previous layout section before advancing the current section. If it is
from a previous layout section an error is emitted.
diff --git a/source/val/validate_layout.cpp b/source/val/validate_layout.cpp
index 707ad52..b53f991 100644
--- a/source/val/validate_layout.cpp
+++ b/source/val/validate_layout.cpp
@@ -107,6 +107,11 @@
}
while (_.IsOpcodeInCurrentLayoutSection(opcode) == false) {
+ if (_.IsOpcodeInPreviousLayoutSection(opcode)) {
+ return _.diag(SPV_ERROR_INVALID_LAYOUT, inst)
+ << spvOpcodeString(opcode) << " is in an invalid layout section";
+ }
+
_.ProgressToNextLayoutSectionOrder();
switch (_.current_layout_section()) {
@@ -135,6 +140,20 @@
// encountered inside of a function.
spv_result_t FunctionScopedInstructions(ValidationState_t& _,
const Instruction* inst, SpvOp opcode) {
+ // Make sure we advance into the function definitions when we hit
+ // non-function declaration instructions.
+ if (_.current_layout_section() == kLayoutFunctionDeclarations &&
+ !_.IsOpcodeInCurrentLayoutSection(opcode)) {
+ _.ProgressToNextLayoutSectionOrder();
+
+ if (_.in_function_body()) {
+ if (auto error = _.current_function().RegisterSetFunctionDeclType(
+ FunctionDecl::kFunctionDeclDefinition)) {
+ return error;
+ }
+ }
+ }
+
if (_.IsOpcodeInCurrentLayoutSection(opcode)) {
switch (opcode) {
case SpvOpFunction: {
@@ -208,12 +227,6 @@
return _.diag(SPV_ERROR_INVALID_LAYOUT, inst)
<< "A block must end with a branch instruction.";
}
- if (_.current_layout_section() == kLayoutFunctionDeclarations) {
- _.ProgressToNextLayoutSectionOrder();
- if (auto error = _.current_function().RegisterSetFunctionDeclType(
- FunctionDecl::kFunctionDeclDefinition))
- return error;
- }
break;
case SpvOpExtInst:
diff --git a/source/val/validation_state.cpp b/source/val/validation_state.cpp
index 72793ec..e190b3c 100644
--- a/source/val/validation_state.cpp
+++ b/source/val/validation_state.cpp
@@ -30,114 +30,74 @@
namespace val {
namespace {
-bool IsInstructionInLayoutSection(ModuleLayoutSection layout, SpvOp op) {
+ModuleLayoutSection InstructionLayoutSection(
+ ModuleLayoutSection current_section, SpvOp op) {
// See Section 2.4
- bool out = false;
- // clang-format off
- switch (layout) {
- case kLayoutCapabilities: out = op == SpvOpCapability; break;
- case kLayoutExtensions: out = op == SpvOpExtension; break;
- case kLayoutExtInstImport: out = op == SpvOpExtInstImport; break;
- case kLayoutMemoryModel: out = op == SpvOpMemoryModel; break;
- case kLayoutEntryPoint: out = op == SpvOpEntryPoint; break;
- case kLayoutExecutionMode:
- out = op == SpvOpExecutionMode || op == SpvOpExecutionModeId;
+ if (spvOpcodeGeneratesType(op) || spvOpcodeIsConstant(op))
+ return kLayoutTypes;
+
+ switch (op) {
+ case SpvOpCapability:
+ return kLayoutCapabilities;
+ case SpvOpExtension:
+ return kLayoutExtensions;
+ case SpvOpExtInstImport:
+ return kLayoutExtInstImport;
+ case SpvOpMemoryModel:
+ return kLayoutMemoryModel;
+ case SpvOpEntryPoint:
+ return kLayoutEntryPoint;
+ case SpvOpExecutionMode:
+ case SpvOpExecutionModeId:
+ return kLayoutExecutionMode;
+ case SpvOpSourceContinued:
+ case SpvOpSource:
+ case SpvOpSourceExtension:
+ case SpvOpString:
+ return kLayoutDebug1;
+ case SpvOpName:
+ case SpvOpMemberName:
+ return kLayoutDebug2;
+ case SpvOpModuleProcessed:
+ return kLayoutDebug3;
+ case SpvOpDecorate:
+ case SpvOpMemberDecorate:
+ case SpvOpGroupDecorate:
+ case SpvOpGroupMemberDecorate:
+ case SpvOpDecorationGroup:
+ case SpvOpDecorateId:
+ case SpvOpDecorateStringGOOGLE:
+ case SpvOpMemberDecorateStringGOOGLE:
+ return kLayoutAnnotations;
+ case SpvOpTypeForwardPointer:
+ return kLayoutTypes;
+ case SpvOpVariable:
+ if (current_section == kLayoutTypes) return kLayoutTypes;
+ return kLayoutFunctionDefinitions;
+ case SpvOpExtInst:
+ // SpvOpExtInst is only allowed in types section for certain extended
+ // instruction sets. This will be checked separately.
+ if (current_section == kLayoutTypes) return kLayoutTypes;
+ return kLayoutFunctionDefinitions;
+ case SpvOpLine:
+ case SpvOpNoLine:
+ case SpvOpUndef:
+ if (current_section == kLayoutTypes) return kLayoutTypes;
+ return kLayoutFunctionDefinitions;
+ case SpvOpFunction:
+ case SpvOpFunctionParameter:
+ case SpvOpFunctionEnd:
+ if (current_section == kLayoutFunctionDeclarations)
+ return kLayoutFunctionDeclarations;
+ return kLayoutFunctionDefinitions;
+ default:
break;
- case kLayoutDebug1:
- switch (op) {
- case SpvOpSourceContinued:
- case SpvOpSource:
- case SpvOpSourceExtension:
- case SpvOpString:
- out = true;
- break;
- default: break;
- }
- break;
- case kLayoutDebug2:
- switch (op) {
- case SpvOpName:
- case SpvOpMemberName:
- out = true;
- break;
- default: break;
- }
- break;
- case kLayoutDebug3:
- // Only OpModuleProcessed is allowed here.
- out = (op == SpvOpModuleProcessed);
- break;
- case kLayoutAnnotations:
- switch (op) {
- case SpvOpDecorate:
- case SpvOpMemberDecorate:
- case SpvOpGroupDecorate:
- case SpvOpGroupMemberDecorate:
- case SpvOpDecorationGroup:
- case SpvOpDecorateId:
- case SpvOpDecorateStringGOOGLE:
- case SpvOpMemberDecorateStringGOOGLE:
- out = true;
- break;
- default: break;
- }
- break;
- case kLayoutTypes:
- if (spvOpcodeGeneratesType(op) || spvOpcodeIsConstant(op)) {
- out = true;
- break;
- }
- switch (op) {
- case SpvOpTypeForwardPointer:
- case SpvOpVariable:
- case SpvOpLine:
- case SpvOpNoLine:
- case SpvOpUndef:
- // SpvOpExtInst is only allowed here for certain extended instruction
- // sets. This will be checked separately
- case SpvOpExtInst:
- out = true;
- break;
- default: break;
- }
- break;
- case kLayoutFunctionDeclarations:
- case kLayoutFunctionDefinitions:
- // NOTE: These instructions should NOT be in these layout sections
- if (spvOpcodeGeneratesType(op) || spvOpcodeIsConstant(op)) {
- out = false;
- break;
- }
- switch (op) {
- case SpvOpCapability:
- case SpvOpExtension:
- case SpvOpExtInstImport:
- case SpvOpMemoryModel:
- case SpvOpEntryPoint:
- case SpvOpExecutionMode:
- case SpvOpExecutionModeId:
- case SpvOpSourceContinued:
- case SpvOpSource:
- case SpvOpSourceExtension:
- case SpvOpString:
- case SpvOpName:
- case SpvOpMemberName:
- case SpvOpModuleProcessed:
- case SpvOpDecorate:
- case SpvOpMemberDecorate:
- case SpvOpGroupDecorate:
- case SpvOpGroupMemberDecorate:
- case SpvOpDecorationGroup:
- case SpvOpTypeForwardPointer:
- out = false;
- break;
- default:
- out = true;
- break;
- }
}
- // clang-format on
- return out;
+ return kLayoutFunctionDefinitions;
+}
+
+bool IsInstructionInLayoutSection(ModuleLayoutSection layout, SpvOp op) {
+ return layout == InstructionLayoutSection(layout, op);
}
// Counts the number of instructions and functions in the file.
@@ -311,6 +271,12 @@
}
}
+bool ValidationState_t::IsOpcodeInPreviousLayoutSection(SpvOp op) {
+ ModuleLayoutSection section =
+ InstructionLayoutSection(current_layout_section_, op);
+ return section < current_layout_section_;
+}
+
bool ValidationState_t::IsOpcodeInCurrentLayoutSection(SpvOp op) {
return IsInstructionInLayoutSection(current_layout_section_, op);
}
diff --git a/source/val/validation_state.h b/source/val/validation_state.h
index e852c52..aeb1ca8 100644
--- a/source/val/validation_state.h
+++ b/source/val/validation_state.h
@@ -195,6 +195,9 @@
/// Increments the module_layout_order_section_
void ProgressToNextLayoutSectionOrder();
+ /// Determines if the op instruction is in a previous layout section
+ bool IsOpcodeInPreviousLayoutSection(SpvOp op);
+
/// Determines if the op instruction is part of the current section
bool IsOpcodeInCurrentLayoutSection(SpvOp op);
@@ -718,6 +721,11 @@
// https://github.com/KhronosGroup/Vulkan-Guide/blob/master/chapters/validation_overview.md
std::string VkErrorID(uint32_t id, const char* reference = nullptr) const;
+ // Testing method to allow setting the current layout section.
+ void SetCurrentLayoutSectionForTesting(ModuleLayoutSection section) {
+ current_layout_section_ = section;
+ }
+
private:
ValidationState_t(const ValidationState_t&);
diff --git a/test/val/val_layout_test.cpp b/test/val/val_layout_test.cpp
index 9d4491c..43fa046 100644
--- a/test/val/val_layout_test.cpp
+++ b/test/val/val_layout_test.cpp
@@ -567,6 +567,26 @@
EXPECT_THAT(getDiagnosticString(), Eq(""));
}
+TEST_F(ValidateLayout, LayoutOrderMixedUp) {
+ char str[] = R"(
+ OpCapability Shader
+ OpCapability Linkage
+ OpMemoryModel Logical GLSL450
+ OpEntryPoint Fragment %fragmentFloat "fragmentFloat"
+ OpExecutionMode %fragmentFloat OriginUpperLeft
+ OpEntryPoint Fragment %fragmentUint "fragmentUint"
+ OpExecutionMode %fragmentUint OriginUpperLeft
+)";
+
+ CompileSuccessfully(str, SPV_ENV_UNIVERSAL_1_1);
+ ASSERT_EQ(SPV_ERROR_INVALID_LAYOUT,
+ ValidateInstructions(SPV_ENV_UNIVERSAL_1_1));
+ // By the mechanics of the validator, we assume ModuleProcessed is in the
+ // right spot, but then that OpName is in the wrong spot.
+ EXPECT_THAT(getDiagnosticString(),
+ HasSubstr("EntryPoint is in an invalid layout section"));
+}
+
TEST_F(ValidateLayout, ModuleProcessedBeforeLastNameIsTooEarly) {
char str[] = R"(
OpCapability Shader
@@ -583,7 +603,7 @@
// By the mechanics of the validator, we assume ModuleProcessed is in the
// right spot, but then that OpName is in the wrong spot.
EXPECT_THAT(getDiagnosticString(),
- HasSubstr("Name cannot appear in a function declaration"));
+ HasSubstr("Name is in an invalid layout section"));
}
TEST_F(ValidateLayout, ModuleProcessedInvalidAfterFirstAnnotation) {
@@ -599,9 +619,8 @@
CompileSuccessfully(str, SPV_ENV_UNIVERSAL_1_1);
ASSERT_EQ(SPV_ERROR_INVALID_LAYOUT,
ValidateInstructions(SPV_ENV_UNIVERSAL_1_1));
- EXPECT_THAT(
- getDiagnosticString(),
- HasSubstr("ModuleProcessed cannot appear in a function declaration"));
+ EXPECT_THAT(getDiagnosticString(),
+ HasSubstr("ModuleProcessed is in an invalid layout section"));
}
TEST_F(ValidateLayout, ModuleProcessedInvalidInFunctionBeforeLabel) {
diff --git a/test/val/val_state_test.cpp b/test/val/val_state_test.cpp
index 18a4ef9..b2d2604 100644
--- a/test/val/val_state_test.cpp
+++ b/test/val/val_state_test.cpp
@@ -134,6 +134,57 @@
EXPECT_FALSE(state_.HasAnyOfExtensions(set2));
}
+// A test of ValidationState_t::IsOpcodeInCurrentLayoutSection().
+using ValidationState_InLayoutState = ValidationStateTest;
+
+TEST_F(ValidationState_InLayoutState, Variable) {
+ state_.SetCurrentLayoutSectionForTesting(kLayoutTypes);
+ EXPECT_TRUE(state_.IsOpcodeInCurrentLayoutSection(SpvOpVariable));
+
+ state_.SetCurrentLayoutSectionForTesting(kLayoutFunctionDefinitions);
+ EXPECT_TRUE(state_.IsOpcodeInCurrentLayoutSection(SpvOpVariable));
+}
+
+TEST_F(ValidationState_InLayoutState, ExtInst) {
+ state_.SetCurrentLayoutSectionForTesting(kLayoutTypes);
+ EXPECT_TRUE(state_.IsOpcodeInCurrentLayoutSection(SpvOpExtInst));
+
+ state_.SetCurrentLayoutSectionForTesting(kLayoutFunctionDefinitions);
+ EXPECT_TRUE(state_.IsOpcodeInCurrentLayoutSection(SpvOpExtInst));
+}
+
+TEST_F(ValidationState_InLayoutState, Undef) {
+ state_.SetCurrentLayoutSectionForTesting(kLayoutTypes);
+ EXPECT_TRUE(state_.IsOpcodeInCurrentLayoutSection(SpvOpUndef));
+
+ state_.SetCurrentLayoutSectionForTesting(kLayoutFunctionDefinitions);
+ EXPECT_TRUE(state_.IsOpcodeInCurrentLayoutSection(SpvOpUndef));
+}
+
+TEST_F(ValidationState_InLayoutState, Function) {
+ state_.SetCurrentLayoutSectionForTesting(kLayoutFunctionDeclarations);
+ EXPECT_TRUE(state_.IsOpcodeInCurrentLayoutSection(SpvOpFunction));
+
+ state_.SetCurrentLayoutSectionForTesting(kLayoutFunctionDefinitions);
+ EXPECT_TRUE(state_.IsOpcodeInCurrentLayoutSection(SpvOpFunction));
+}
+
+TEST_F(ValidationState_InLayoutState, FunctionParameter) {
+ state_.SetCurrentLayoutSectionForTesting(kLayoutFunctionDeclarations);
+ EXPECT_TRUE(state_.IsOpcodeInCurrentLayoutSection(SpvOpFunctionParameter));
+
+ state_.SetCurrentLayoutSectionForTesting(kLayoutFunctionDefinitions);
+ EXPECT_TRUE(state_.IsOpcodeInCurrentLayoutSection(SpvOpFunctionParameter));
+}
+
+TEST_F(ValidationState_InLayoutState, FunctionEnd) {
+ state_.SetCurrentLayoutSectionForTesting(kLayoutFunctionDeclarations);
+ EXPECT_TRUE(state_.IsOpcodeInCurrentLayoutSection(SpvOpFunctionEnd));
+
+ state_.SetCurrentLayoutSectionForTesting(kLayoutFunctionDefinitions);
+ EXPECT_TRUE(state_.IsOpcodeInCurrentLayoutSection(SpvOpFunctionEnd));
+}
+
} // namespace
} // namespace val
} // namespace spvtools
