Unify validation of OpCopyMemory*HEADmaster

Fixes #1800

* Refactored duplication of code between OpCopyMemory and
OpCopyMemorySized validation
* Fixed some bugs in OpCopyMemorySized validation
* Replaced asserts with checks
* Added new tests

2 files changed, 346 insertions, 95 deletions

diff --git a/source/val/validate_memory.cpp b/source/val/validate_memory.cpp
index 55060921..18c3cca0 100644
--- a/source/val/validate_memory.cpp
+++ b/source/val/validate_memory.cpp
@@ -316,96 +316,87 @@ spv_result_t ValidateCopyMemory(ValidationState_t& _, const Instruction& inst) {
   }
 
   const auto target_pointer_type = _.FindDef(target->type_id());
-  assert(target_pointer_type);
-  const auto target_type = _.FindDef(target_pointer_type->words()[3]);
-  assert(target_type);
-  const auto source_pointer_type = _.FindDef(source->type_id());
-  assert(source_pointer_type);
-  const auto source_type = _.FindDef(source_pointer_type->words()[3]);
-  assert(source_type);
-  if (target_type->id() != source_type->id()) {
-    return _.diag(SPV_ERROR_INVALID_ID, &inst)
-           << "OpCopyMemory Target <id> '" << _.getIdName(source_id)
-           << "'s type does not match Source <id> '"
-           << _.getIdName(source_type->id()) << "'s type.";
-  }
-  return SPV_SUCCESS;
-}
-
-spv_result_t ValidateCopyMemorySized(ValidationState_t& _,
-                                     const Instruction& inst) {
-  const auto target_index = 0;
-  const auto target_id = inst.GetOperandAs<uint32_t>(target_index);
-  const auto target = _.FindDef(target_id);
-  if (!target) {
-    return _.diag(SPV_ERROR_INVALID_ID, &inst)
-           << "Target operand <id> '" << _.getIdName(target_id)
-           << "' is not defined.";
-  }
-
-  const auto source_index = 1;
-  const auto source_id = inst.GetOperandAs<uint32_t>(source_index);
-  const auto source = _.FindDef(source_id);
-  if (!source) {
-    return _.diag(SPV_ERROR_INVALID_ID, &inst)
-           << "Source operand <id> '" << _.getIdName(source_id)
-           << "' is not defined.";
-  }
-
-  const auto size_index = 2;
-  const auto size_id = inst.GetOperandAs<uint32_t>(size_index);
-  const auto size = _.FindDef(size_id);
-  if (!size) {
-    return _.diag(SPV_ERROR_INVALID_ID, &inst)
-           << "Size operand <id> '" << _.getIdName(size_id)
-           << "' is not defined.";
-  }
-
-  const auto target_pointer_type = _.FindDef(target->type_id());
   if (!target_pointer_type ||
-      SpvOpTypePointer != target_pointer_type->opcode()) {
+      target_pointer_type->opcode() != SpvOpTypePointer) {
     return _.diag(SPV_ERROR_INVALID_ID, &inst)
-           << "OpCopyMemorySized Target <id> '" << _.getIdName(target_id)
+           << "Target operand <id> '" << _.getIdName(target_id)
            << "' is not a pointer.";
   }
+
   const auto source_pointer_type = _.FindDef(source->type_id());
   if (!source_pointer_type ||
-      SpvOpTypePointer != source_pointer_type->opcode()) {
+      source_pointer_type->opcode() != SpvOpTypePointer) {
     return _.diag(SPV_ERROR_INVALID_ID, &inst)
-           << "OpCopyMemorySized Source <id> '" << _.getIdName(source_id)
+           << "Source operand <id> '" << _.getIdName(source_id)
            << "' is not a pointer.";
   }
-  switch (size->opcode()) {
-    // TODO: The following opcode's are assumed to be valid, refer to the
-    // following bug https://cvs.khronos.org/bugzilla/show_bug.cgi?id=13871 for
-    // clarification
-    case SpvOpConstant:
-    case SpvOpSpecConstant: {
-      auto size_type = _.FindDef(size->type_id());
-      assert(size_type);
-      if (SpvOpTypeInt != size_type->opcode()) {
-        return _.diag(SPV_ERROR_INVALID_ID, &inst)
-               << "OpCopyMemorySized Size <id> '" << _.getIdName(size_id)
-               << "'s type is not an integer type.";
-      }
-    } break;
-    case SpvOpVariable: {
-      auto pointer_type = _.FindDef(size->type_id());
-      assert(pointer_type);
-      auto size_type = _.FindDef(pointer_type->type_id());
-      if (!size_type || SpvOpTypeInt != size_type->opcode()) {
-        return _.diag(SPV_ERROR_INVALID_ID, &inst)
-               << "OpCopyMemorySized Size <id> '" << _.getIdName(size_id)
-               << "'s variable type is not an integer type.";
-      }
-    } break;
-    default:
+
+  if (inst.opcode() == SpvOpCopyMemory) {
+    const auto target_type =
+        _.FindDef(target_pointer_type->GetOperandAs<uint32_t>(2));
+    if (!target_type || target_type->opcode() == SpvOpTypeVoid) {
+      return _.diag(SPV_ERROR_INVALID_ID, &inst)
+             << "Target operand <id> '" << _.getIdName(target_id)
+             << "' cannot be a void pointer.";
+    }
+
+    const auto source_type =
+        _.FindDef(source_pointer_type->GetOperandAs<uint32_t>(2));
+    if (!source_type || source_type->opcode() == SpvOpTypeVoid) {
+      return _.diag(SPV_ERROR_INVALID_ID, &inst)
+             << "Source operand <id> '" << _.getIdName(source_id)
+             << "' cannot be a void pointer.";
+    }
+
+    if (target_type->id() != source_type->id()) {
+      return _.diag(SPV_ERROR_INVALID_ID, &inst)
+             << "Target <id> '" << _.getIdName(source_id)
+             << "'s type does not match Source <id> '"
+             << _.getIdName(source_type->id()) << "'s type.";
+    }
+  } else {
+    const auto size_id = inst.GetOperandAs<uint32_t>(2);
+    const auto size = _.FindDef(size_id);
+    if (!size) {
       return _.diag(SPV_ERROR_INVALID_ID, &inst)
-             << "OpCopyMemorySized Size <id> '" << _.getIdName(size_id)
-             << "' is not a constant or variable.";
+             << "Size operand <id> '" << _.getIdName(size_id)
+             << "' is not defined.";
+    }
+
+    const auto size_type = _.FindDef(size->type_id());
+    if (!_.IsIntScalarType(size_type->id())) {
+      return _.diag(SPV_ERROR_INVALID_ID, &inst)
+             << "Size operand <id> '" << _.getIdName(size_id)
+             << "' must be a scalar integer type.";
+    }
+
+    bool is_zero = true;
+    switch (size->opcode()) {
+      case SpvOpConstantNull:
+        return _.diag(SPV_ERROR_INVALID_ID, &inst)
+               << "Size operand <id> '" << _.getIdName(size_id)
+               << "' cannot be a constant zero.";
+      case SpvOpConstant:
+        if (size_type->word(3) == 1 &&
+            size->word(size->words().size() - 1) & 0x80000000) {
+          return _.diag(SPV_ERROR_INVALID_ID, &inst)
+                 << "Size operand <id> '" << _.getIdName(size_id)
+                 << "' cannot have the sign bit set to 1.";
+        }
+        for (size_t i = 3; is_zero && i < size->words().size(); ++i) {
+          is_zero &= (size->word(i) == 0);
+        }
+        if (is_zero) {
+          return _.diag(SPV_ERROR_INVALID_ID, &inst)
+                 << "Size operand <id> '" << _.getIdName(size_id)
+                 << "' cannot be a constant zero.";
+        }
+        break;
+      default:
+        // Cannot infer any other opcodes.
+        break;
+    }
   }
-  // TODO: Check that consant is a least size 1, see the same bug as above for
-  // clarification?
   return SPV_SUCCESS;
 }
 
@@ -576,10 +567,8 @@ spv_result_t ValidateMemoryInstructions(ValidationState_t& _,
       if (auto error = ValidateStore(_, *inst)) return error;
       break;
     case SpvOpCopyMemory:
-      if (auto error = ValidateCopyMemory(_, *inst)) return error;
-      break;
     case SpvOpCopyMemorySized:
-      if (auto error = ValidateCopyMemorySized(_, *inst)) return error;
+      if (auto error = ValidateCopyMemory(_, *inst)) return error;
       break;
     case SpvOpAccessChain:
     case SpvOpInBoundsAccessChain:
diff --git a/test/val/val_id_test.cpp b/test/val/val_id_test.cpp
index 6bdba1d4..6d483274 100644
--- a/test/val/val_id_test.cpp
+++ b/test/val/val_id_test.cpp
@@ -41,20 +41,31 @@ std::string kOpCapabilitySetup = R"(
      OpCapability Shader
      OpCapability Linkage
      OpCapability Addresses
-     OpCapability Pipes
-     OpCapability LiteralSampler
-     OpCapability DeviceEnqueue
-     OpCapability Vector16
      OpCapability Int8
      OpCapability Int16
      OpCapability Int64
      OpCapability Float64
+     OpCapability LiteralSampler
+     OpCapability Pipes
+     OpCapability DeviceEnqueue
+     OpCapability Vector16
 )";
 
 std::string kGLSL450MemoryModel = kOpCapabilitySetup + R"(
      OpMemoryModel Logical GLSL450
 )";
 
+std::string kNoKernelGLSL450MemoryModel = R"(
+     OpCapability Shader
+     OpCapability Linkage
+     OpCapability Addresses
+     OpCapability Int8
+     OpCapability Int16
+     OpCapability Int64
+     OpCapability Float64
+     OpMemoryModel Logical GLSL450
+)";
+
 std::string kOpenCLMemoryModel32 = R"(
      OpCapability Addresses
      OpCapability Linkage
@@ -2584,6 +2595,49 @@ TEST_F(ValidateIdWithMessage, OpCopyMemoryGood) {
   CompileSuccessfully(spirv.c_str());
   EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
 }
+
+TEST_F(ValidateIdWithMessage, OpCopyMemoryNonPointerTarget) {
+  const std::string spirv = kGLSL450MemoryModel + R"(
+%1 = OpTypeVoid
+%2 = OpTypeInt 32 0
+%3 = OpTypePointer Uniform %2
+%4 = OpTypeFunction %1 %2 %3
+%5 = OpFunction %1 None %4
+%6 = OpFunctionParameter %2
+%7 = OpFunctionParameter %3
+%8 = OpLabel
+OpCopyMemory %6 %7
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv);
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("Target operand <id> '6' is not a pointer."));
+}
+
+TEST_F(ValidateIdWithMessage, OpCopyMemoryNonPointerSource) {
+  const std::string spirv = kGLSL450MemoryModel + R"(
+%1 = OpTypeVoid
+%2 = OpTypeInt 32 0
+%3 = OpTypePointer Uniform %2
+%4 = OpTypeFunction %1 %2 %3
+%5 = OpFunction %1 None %4
+%6 = OpFunctionParameter %2
+%7 = OpFunctionParameter %3
+%8 = OpLabel
+OpCopyMemory %7 %6
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv);
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("Source operand <id> '6' is not a pointer."));
+}
+
 TEST_F(ValidateIdWithMessage, OpCopyMemoryBad) {
   std::string spirv = kGLSL450MemoryModel + R"(
  %1 = OpTypeVoid
@@ -2604,11 +2658,54 @@ TEST_F(ValidateIdWithMessage, OpCopyMemoryBad) {
   CompileSuccessfully(spirv.c_str());
   EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
   EXPECT_THAT(getDiagnosticString(),
-              HasSubstr("OpCopyMemory Target <id> '5's type does not match "
+              HasSubstr("Target <id> '5's type does not match "
                         "Source <id> '2's type."));
 }
 
-// TODO: OpCopyMemorySized
+TEST_F(ValidateIdWithMessage, OpCopyMemoryVoidTarget) {
+  const std::string spirv = kGLSL450MemoryModel + R"(
+%1 = OpTypeVoid
+%2 = OpTypeInt 32 0
+%3 = OpTypePointer Uniform %1
+%4 = OpTypePointer Uniform %2
+%5 = OpTypeFunction %1 %2 %3
+%6 = OpFunction %1 None %5
+%7 = OpFunctionParameter %3
+%8 = OpFunctionParameter %4
+%9 = OpLabel
+OpCopyMemory %7 %8
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv);
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("Target operand <id> '7' cannot be a void pointer."));
+}
+
+TEST_F(ValidateIdWithMessage, OpCopyMemoryVoidSource) {
+  const std::string spirv = kGLSL450MemoryModel + R"(
+%1 = OpTypeVoid
+%2 = OpTypeInt 32 0
+%3 = OpTypePointer Uniform %1
+%4 = OpTypePointer Uniform %2
+%5 = OpTypeFunction %1 %2 %3
+%6 = OpFunction %1 None %5
+%7 = OpFunctionParameter %3
+%8 = OpFunctionParameter %4
+%9 = OpLabel
+OpCopyMemory %8 %7
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv);
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("Source operand <id> '7' cannot be a void pointer."));
+}
+
 TEST_F(ValidateIdWithMessage, OpCopyMemorySizedGood) {
   std::string spirv = kGLSL450MemoryModel + R"(
  %1 = OpTypeVoid
@@ -2644,7 +2741,7 @@ TEST_F(ValidateIdWithMessage, OpCopyMemorySizedTargetBad) {
   CompileSuccessfully(spirv.c_str());
   EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
   EXPECT_THAT(getDiagnosticString(),
-              HasSubstr("OpCopyMemorySized Target <id> '9' is not a pointer."));
+              HasSubstr("Target operand <id> '9' is not a pointer."));
 }
 TEST_F(ValidateIdWithMessage, OpCopyMemorySizedSourceBad) {
   std::string spirv = kGLSL450MemoryModel + R"(
@@ -2663,7 +2760,7 @@ TEST_F(ValidateIdWithMessage, OpCopyMemorySizedSourceBad) {
   CompileSuccessfully(spirv.c_str());
   EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
   EXPECT_THAT(getDiagnosticString(),
-              HasSubstr("OpCopyMemorySized Source <id> '6' is not a pointer."));
+              HasSubstr("Source operand <id> '6' is not a pointer."));
 }
 TEST_F(ValidateIdWithMessage, OpCopyMemorySizedSizeBad) {
   std::string spirv = kGLSL450MemoryModel + R"(
@@ -2682,9 +2779,9 @@ TEST_F(ValidateIdWithMessage, OpCopyMemorySizedSizeBad) {
       OpFunctionEnd)";
   CompileSuccessfully(spirv.c_str());
   EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
-  EXPECT_THAT(getDiagnosticString(),
-              HasSubstr("OpCopyMemorySized Size <id> '6's variable type is not "
-                        "an integer type."));
+  EXPECT_THAT(
+      getDiagnosticString(),
+      HasSubstr("Size operand <id> '6' must be a scalar integer type."));
 }
 TEST_F(ValidateIdWithMessage, OpCopyMemorySizedSizeTypeBad) {
   std::string spirv = kGLSL450MemoryModel + R"(
@@ -2707,8 +2804,173 @@ TEST_F(ValidateIdWithMessage, OpCopyMemorySizedSizeTypeBad) {
   EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
   EXPECT_THAT(
       getDiagnosticString(),
-      HasSubstr(
-          "OpCopyMemorySized Size <id> '9's type is not an integer type."));
+      HasSubstr("Size operand <id> '9' must be a scalar integer type."));
+}
+
+TEST_F(ValidateIdWithMessage, OpCopyMemorySizedSizeConstantNull) {
+  const std::string spirv = kGLSL450MemoryModel + R"(
+%1 = OpTypeVoid
+%2 = OpTypeInt 32 0
+%3 = OpConstantNull %2
+%4 = OpTypePointer Uniform %2
+%5 = OpTypeFloat 32
+%6 = OpTypePointer UniformConstant %5
+%7 = OpTypeFunction %1 %4 %6
+%8 = OpFunction %1 None %7
+%9 = OpFunctionParameter %4
+%10 = OpFunctionParameter %6
+%11 = OpLabel
+OpCopyMemorySized %9 %10 %3
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv);
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("Size operand <id> '3' cannot be a constant zero."));
+}
+
+TEST_F(ValidateIdWithMessage, OpCopyMemorySizedSizeConstantZero) {
+  const std::string spirv = kGLSL450MemoryModel + R"(
+%1 = OpTypeVoid
+%2 = OpTypeInt 32 0
+%3 = OpConstant %2 0
+%4 = OpTypePointer Uniform %2
+%5 = OpTypeFloat 32
+%6 = OpTypePointer UniformConstant %5
+%7 = OpTypeFunction %1 %4 %6
+%8 = OpFunction %1 None %7
+%9 = OpFunctionParameter %4
+%10 = OpFunctionParameter %6
+%11 = OpLabel
+OpCopyMemorySized %9 %10 %3
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv);
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("Size operand <id> '3' cannot be a constant zero."));
+}
+
+TEST_F(ValidateIdWithMessage, OpCopyMemorySizedSizeConstantZero64) {
+  const std::string spirv = kGLSL450MemoryModel + R"(
+%1 = OpTypeVoid
+%2 = OpTypeInt 64 0
+%3 = OpConstant %2 0
+%4 = OpTypePointer Uniform %2
+%5 = OpTypeFloat 32
+%6 = OpTypePointer UniformConstant %5
+%7 = OpTypeFunction %1 %4 %6
+%8 = OpFunction %1 None %7
+%9 = OpFunctionParameter %4
+%10 = OpFunctionParameter %6
+%11 = OpLabel
+OpCopyMemorySized %9 %10 %3
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv);
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("Size operand <id> '3' cannot be a constant zero."));
+}
+
+TEST_F(ValidateIdWithMessage, OpCopyMemorySizedSizeConstantNegative) {
+  const std::string spirv = kNoKernelGLSL450MemoryModel + R"(
+%1 = OpTypeVoid
+%2 = OpTypeInt 32 1
+%3 = OpConstant %2 -1
+%4 = OpTypePointer Uniform %2
+%5 = OpTypeFloat 32
+%6 = OpTypePointer UniformConstant %5
+%7 = OpTypeFunction %1 %4 %6
+%8 = OpFunction %1 None %7
+%9 = OpFunctionParameter %4
+%10 = OpFunctionParameter %6
+%11 = OpLabel
+OpCopyMemorySized %9 %10 %3
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv);
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+  EXPECT_THAT(
+      getDiagnosticString(),
+      HasSubstr("Size operand <id> '3' cannot have the sign bit set to 1."));
+}
+
+TEST_F(ValidateIdWithMessage, OpCopyMemorySizedSizeConstantNegative64) {
+  const std::string spirv = kNoKernelGLSL450MemoryModel + R"(
+%1 = OpTypeVoid
+%2 = OpTypeInt 64 1
+%3 = OpConstant %2 -1
+%4 = OpTypePointer Uniform %2
+%5 = OpTypeFloat 32
+%6 = OpTypePointer UniformConstant %5
+%7 = OpTypeFunction %1 %4 %6
+%8 = OpFunction %1 None %7
+%9 = OpFunctionParameter %4
+%10 = OpFunctionParameter %6
+%11 = OpLabel
+OpCopyMemorySized %9 %10 %3
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv);
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+  EXPECT_THAT(
+      getDiagnosticString(),
+      HasSubstr("Size operand <id> '3' cannot have the sign bit set to 1."));
+}
+
+TEST_F(ValidateIdWithMessage, OpCopyMemorySizedSizeUnsignedNegative) {
+  const std::string spirv = kGLSL450MemoryModel + R"(
+%1 = OpTypeVoid
+%2 = OpTypeInt 32 0
+%3 = OpConstant %2 2147483648
+%4 = OpTypePointer Uniform %2
+%5 = OpTypeFloat 32
+%6 = OpTypePointer UniformConstant %5
+%7 = OpTypeFunction %1 %4 %6
+%8 = OpFunction %1 None %7
+%9 = OpFunctionParameter %4
+%10 = OpFunctionParameter %6
+%11 = OpLabel
+OpCopyMemorySized %9 %10 %3
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv);
+  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
+}
+
+TEST_F(ValidateIdWithMessage, OpCopyMemorySizedSizeUnsignedNegative64) {
+  const std::string spirv = kGLSL450MemoryModel + R"(
+%1 = OpTypeVoid
+%2 = OpTypeInt 64 0
+%3 = OpConstant %2 9223372036854775808
+%4 = OpTypePointer Uniform %2
+%5 = OpTypeFloat 32
+%6 = OpTypePointer UniformConstant %5
+%7 = OpTypeFunction %1 %4 %6
+%8 = OpFunction %1 None %7
+%9 = OpFunctionParameter %4
+%10 = OpFunctionParameter %6
+%11 = OpLabel
+OpCopyMemorySized %9 %10 %3
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv);
+  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
 }
 
 const char kDeeplyNestedStructureSetup[] = R"(