Update val to handle reversed instruction sections. (KhronosGroup#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.

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:

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;
-      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;
-      }
+  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 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);
 }

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&);
 

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) {

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