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.
2020-10-08 13:10:12 -04:00 · 2020-10-08 13:10:12 -04:00 · 11d5924227
--- a/source/val/validate_layout.cpp
+++ b/source/val/validate_layout.cpp
@ -107,6 +107,11 @@ spv_result_t ModuleScopedInstructions(ValidationState_t& _,
  }

  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 @@ spv_result_t ModuleScopedInstructions(ValidationState_t& _,
 // 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 @@ spv_result_t FunctionScopedInstructions(ValidationState_t& _,
          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:
--- a/source/val/validation_state.cpp
+++ b/source/val/validation_state.cpp
@ -30,114 +30,74 @@ namespace spvtools {
 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 @@ void ValidationState_t::ProgressToNextLayoutSectionOrder() {
  }
 }

+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);
 }
--- a/source/val/validation_state.h
+++ b/source/val/validation_state.h
@ -195,6 +195,9 @@ class ValidationState_t {
  /// 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 @@ class ValidationState_t {
  // 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&);

--- a/test/val/val_layout_test.cpp
+++ b/test/val/val_layout_test.cpp
@ -567,6 +567,26 @@ TEST_F(ValidateLayout, ModuleProcessedValidIn11) {
  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 @@ TEST_F(ValidateLayout, ModuleProcessedBeforeLastNameIsTooEarly) {
  // 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 @@ TEST_F(ValidateLayout, ModuleProcessedInvalidAfterFirstAnnotation) {
  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) {
--- a/test/val/val_state_test.cpp
+++ b/test/val/val_state_test.cpp
@ -134,6 +134,57 @@ TEST_F(ValidationState_HasAnyOfExtensions, MultiCapMask) {
  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