Validate all type ids. (#1868)

* Validate all type ids.

The validator does not check if the type of an instruction is actually
a type unless the OpCode has a specific requirement.  For example,
OpFAdd is checked, but OpUndef is not.

The commit add a generic check that if there is a type id then the id
defines a type.

http://crbug.com/876694

* Merge other checks for type into new one.

There are a couple check that the type id is a type for specific
opcodes.  Those have been mereged into 1.

Small changes to other test cases to make them valid enough for the
purpose of the test.

source/val/validate_cfg.cpp

@@ -40,13 +40,6 @@ namespace val {
 namespace {
 
 spv_result_t ValidatePhi(ValidationState_t& _, const Instruction* inst) {
-  SpvOp type_op = _.GetIdOpcode(inst->type_id());
-  if (!spvOpcodeGeneratesType(type_op)) {
-    return _.diag(SPV_ERROR_INVALID_ID, inst)
-           << "OpPhi's type <id> " << _.getIdName(inst->type_id())
-           << " is not a type instruction.";
-  }
-
   auto block = inst->block();
   size_t num_in_ops = inst->words().size() - 3;
   if (num_in_ops % 2 != 0) {

source/val/validate_composites.cpp

@@ -371,11 +371,6 @@ spv_result_t ValidateCompositeInsert(ValidationState_t& _,
 
 spv_result_t ValidateCopyObject(ValidationState_t& _, const Instruction* inst) {
   const uint32_t result_type = inst->type_id();
-  if (!spvOpcodeGeneratesType(_.GetIdOpcode(result_type))) {
-    return _.diag(SPV_ERROR_INVALID_DATA, inst)
-           << "Expected Result Type to be a type";
-  }
-
   const uint32_t operand_type = _.GetOperandTypeId(inst, 2);
   if (operand_type != result_type) {
     return _.diag(SPV_ERROR_INVALID_DATA, inst)

source/val/validate_id.cpp

@@ -162,7 +162,6 @@ spv_result_t IdPass(ValidationState_t& _, Instruction* inst) {
         ret = SPV_SUCCESS;
         break;
       case SPV_OPERAND_TYPE_ID:
-      case SPV_OPERAND_TYPE_TYPE_ID:
       case SPV_OPERAND_TYPE_MEMORY_SEMANTICS_ID:
       case SPV_OPERAND_TYPE_SCOPE_ID:
         if (_.IsDefinedId(operand_word)) {
@@ -175,6 +174,21 @@ spv_result_t IdPass(ValidationState_t& _, Instruction* inst) {
                 << " has not been defined";
         }
         break;
+      case SPV_OPERAND_TYPE_TYPE_ID:
+        if (_.IsDefinedId(operand_word)) {
+          auto* def = _.FindDef(operand_word);
+          if (!spvOpcodeGeneratesType(def->opcode())) {
+            ret = _.diag(SPV_ERROR_INVALID_ID, inst)
+                  << "ID " << _.getIdName(operand_word) << " is not a type id";
+          } else {
+            ret = SPV_SUCCESS;
+          }
+        } else {
+          ret = _.diag(SPV_ERROR_INVALID_ID, inst)
+                << "ID " << _.getIdName(operand_word)
+                << " has not been defined";
+        }
+        break;
       default:
         ret = SPV_SUCCESS;
         break;

test/val/val_composites_test.cpp

@@ -537,9 +537,8 @@ TEST_F(ValidateComposites, CopyObjectResultTypeNotType) {
 )";
 
   CompileSuccessfully(GenerateShaderCode(body).c_str());
-  ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
+  ASSERT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
-  EXPECT_THAT(getDiagnosticString(),
+  EXPECT_THAT(getDiagnosticString(), HasSubstr("ID 19 is not a type id"));
-              HasSubstr("Expected Result Type to be a type"));
 }
 
 TEST_F(ValidateComposites, CopyObjectWrongOperandType) {

test/val/val_id_test.cpp

@@ -31,9 +31,9 @@ namespace spvtools {
 namespace val {
 namespace {
 
+using spvtest::ScopedContext;
 using ::testing::HasSubstr;
 using ::testing::ValuesIn;
-using spvtest::ScopedContext;
 
 using ValidateIdWithMessage = spvtest::ValidateBase<bool>;
 
@@ -4212,8 +4212,7 @@ OpFunctionEnd
 
   CompileSuccessfully(spirv.c_str());
   EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
-  EXPECT_THAT(getDiagnosticString(),
+  EXPECT_THAT(getDiagnosticString(), HasSubstr("ID 3 is not a type id"));
-              HasSubstr("OpPhi's type <id> 3 is not a type instruction."));
 }
 
 TEST_F(ValidateIdWithMessage, OpPhiSamePredecessor) {
@@ -4876,7 +4875,7 @@ OpDecorate %1 SpecId 200
 %2 = OpTypeFunction %void
 %int = OpTypeInt 32 0
 %1 = OpConstant %int 3
-%main = OpFunction %1 None %2
+%main = OpFunction %void None %2
 %4 = OpLabel
 OpReturnValue %1
 OpFunctionEnd
@@ -4897,7 +4896,7 @@ OpDecorate %1 SpecId 200
 %3 = OpConstant %int 1
 %4 = OpConstant %int 2
 %1 = OpSpecConstantOp %int IAdd %3 %4
-%main = OpFunction %1 None %2
+%main = OpFunction %void None %2
 %6 = OpLabel
 OpReturnValue %3
 OpFunctionEnd
@@ -4917,7 +4916,7 @@ OpDecorate %1 SpecId 200
 %int = OpTypeInt 32 0
 %3 = OpConstant %int 1
 %1 = OpSpecConstantComposite %int
-%main = OpFunction %1 None %2
+%main = OpFunction %void None %2
 %4 = OpLabel
 OpReturnValue %3
 OpFunctionEnd
@@ -5005,6 +5004,24 @@ TEST_F(ValidateIdWithMessage, TypeFunctionBadUse) {
               HasSubstr("Invalid use of function type result id 2."));
 }
 
+TEST_F(ValidateIdWithMessage, BadTypeId) {
+  std::string spirv = kGLSL450MemoryModel + R"(
+          %1 = OpTypeVoid
+          %2 = OpTypeFunction %1
+          %3 = OpTypeFloat 32
+          %4 = OpConstant %3 0
+          %5 = OpFunction %1 None %2
+          %6 = OpLabel
+          %7 = OpUndef %4
+               OpReturn
+               OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv);
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+  EXPECT_THAT(getDiagnosticString(), HasSubstr("ID 4 is not a type id"));
+}
+
 // TODO: OpLifetimeStart
 // TODO: OpLifetimeStop
 // TODO: OpAtomicInit