Fix validation of return value.

source/opcode.cpp

@@ -515,18 +515,6 @@ int32_t spvInstructionIsInBasicBlock(const spv_instruction_t* pFirstInst,
   return true;
 }
 
-int32_t spvOpcodeIsValue(SpvOp opcode) {
-  if (spvOpcodeIsPointer(opcode)) return true;
-  if (spvOpcodeIsConstant(opcode)) return true;
-  switch (opcode) {
-    case SpvOpLoad:
-      // TODO: Other Opcode's resulting in a value
-      return true;
-    default:
-      return false;
-  }
-}
-
 int32_t spvOpcodeGeneratesType(SpvOp op) {
   switch (op) {
     case SpvOpTypeVoid:

source/opcode.h

@@ -102,10 +102,6 @@ int32_t spvOpcodeIsBasicTypeNullable(SpvOp opcode);
 int32_t spvInstructionIsInBasicBlock(const spv_instruction_t* first_inst,
                                      const spv_instruction_t* inst);
 
-// Determines if the given opcode contains a value. Returns zero if false,
-// non-zero otherwise.
-int32_t spvOpcodeIsValue(SpvOp opcode);
-
 // Determines if the given opcode generates a type. Returns zero if false,
 // non-zero otherwise.
 int32_t spvOpcodeGeneratesType(SpvOp opcode);

source/validate_id.cpp

@@ -1671,13 +1671,25 @@ bool idUsage::isValid<SpvOpReturnValue>(const spv_instruction_t* inst,
                                         const spv_opcode_desc) {
   auto valueIndex = 1;
   auto value = usedefs_.FindDef(inst->words[valueIndex]);
-  if (!value.first || !spvOpcodeIsValue(value.second.opcode)) {
+  if (!value.first || !value.second.type_id) {
     DIAG(valueIndex) << "OpReturnValue Value <id> '" << inst->words[valueIndex]
                      << "' does not represent a value.";
     return false;
   }
   auto valueType = usedefs_.FindDef(value.second.type_id);
-  assert(valueType.first);
+  if (!valueType.first || SpvOpTypeVoid == valueType.second.opcode) {
+    DIAG(valueIndex) << "OpReturnValue value's type <id> '"
+                     << value.second.type_id << "' is missing or void.";
+    return false;
+  }
+  if (SpvOpTypePointer == valueType.second.opcode) {
+    DIAG(valueIndex) << "OpReturnValue value's type <id> '"
+                     << value.second.type_id
+                     << "' is a pointer, but a pointer can only be an operand "
+                        "to OpLoad, OpStore, OpAccessChain, or "
+                        "OpInBoundsAccessChain.";
+    return false;
+  }
   // NOTE: Find OpFunction
   const spv_instruction_t* function = inst - 1;
   while (firstInst != function) {
@@ -1688,22 +1700,11 @@ bool idUsage::isValid<SpvOpReturnValue>(const spv_instruction_t* inst,
            DIAG(valueIndex) << "OpReturnValue is not in a basic block.";
            return false);
   auto returnType = usedefs_.FindDef(function->words[1]);
-  assert(returnType.first);
-  if (SpvOpTypePointer == valueType.second.opcode) {
-    auto pointerValueType = usedefs_.FindDef(valueType.second.words[3]);
-    assert(pointerValueType.first);
-    spvCheck(returnType.second.id != pointerValueType.second.id,
-             DIAG(valueIndex)
-                 << "OpReturnValue Value <id> '" << inst->words[valueIndex]
-                 << "'s pointer type does not match OpFunction's return type.";
-             return false);
-  } else {
-    spvCheck(returnType.second.id != valueType.second.id,
-             DIAG(valueIndex)
-                 << "OpReturnValue Value <id> '" << inst->words[valueIndex]
-                 << "'s type does not match OpFunction's return type.";
-             return false);
-  }
+  spvCheck(!returnType.first || returnType.second.id != valueType.second.id,
+           DIAG(valueIndex)
+               << "OpReturnValue Value <id> '" << inst->words[valueIndex]
+               << "'s type does not match OpFunction's return type.";
+           return false);
   return true;
 }
 

test/ValidateID.cpp

@@ -1322,7 +1322,7 @@ TEST_F(ValidateID, OpReturnValueConstantGood) {
   const char* spirv = R"(
 %1 = OpTypeVoid
 %2 = OpTypeInt 32 0
-%3 = OpTypeFunction %2 %2
+%3 = OpTypeFunction %2
 %4 = OpConstant %2 42
 %5 = OpFunction %2 None %3
 %6 = OpLabel
@@ -1330,33 +1330,101 @@ TEST_F(ValidateID, OpReturnValueConstantGood) {
      OpFunctionEnd)";
   CHECK(spirv, SPV_SUCCESS);
 }
+
 TEST_F(ValidateID, OpReturnValueVariableGood) {
   const char* spirv = R"(
 %1 = OpTypeVoid
 %2 = OpTypeInt 32 0 ;10
-%3 = OpTypeFunction %2 %2 ;14
+%3 = OpTypeFunction %2
 %8 = OpTypePointer Function %2 ;18
 %4 = OpConstant %2 42 ;22
 %5 = OpFunction %2 None %3 ;27
 %6 = OpLabel ;29
 %7 = OpVariable %8 Function %4 ;34
-     OpReturnValue %7 ;36
+%9 = OpLoad %2 %7
+     OpReturnValue %9 ;36
      OpFunctionEnd)";
   CHECK(spirv, SPV_SUCCESS);
 }
-TEST_F(ValidateID, OpReturnValueBad) {
+
+TEST_F(ValidateID, OpReturnValueExpressionGood) {
   const char* spirv = R"(
 %1 = OpTypeVoid
 %2 = OpTypeInt 32 0
-%3 = OpTypeFunction %2 %2
+%3 = OpTypeFunction %2
 %4 = OpConstant %2 42
 %5 = OpFunction %2 None %3
+%6 = OpLabel
+%7 = OpIAdd %2 %4 %4
+     OpReturnValue %7
+     OpFunctionEnd)";
+  CHECK(spirv, SPV_SUCCESS);
+}
+
+TEST_F(ValidateID, OpReturnValueIsType) {
+  const char* spirv = R"(
+%1 = OpTypeVoid
+%2 = OpTypeInt 32 0
+%3 = OpTypeFunction %2
+%5 = OpFunction %2 None %3
 %6 = OpLabel
      OpReturnValue %1
      OpFunctionEnd)";
   CHECK(spirv, SPV_ERROR_INVALID_ID);
 }
 
+TEST_F(ValidateID, OpReturnValueIsLabel) {
+  const char* spirv = R"(
+%1 = OpTypeVoid
+%2 = OpTypeInt 32 0
+%3 = OpTypeFunction %2
+%5 = OpFunction %2 None %3
+%6 = OpLabel
+     OpReturnValue %6
+     OpFunctionEnd)";
+  CHECK(spirv, SPV_ERROR_INVALID_ID);
+}
+
+TEST_F(ValidateID, OpReturnValueIsVoid) {
+  const char* spirv = R"(
+%1 = OpTypeVoid
+%2 = OpTypeInt 32 0
+%3 = OpTypeFunction %1
+%5 = OpFunction %1 None %3
+%6 = OpLabel
+%7 = OpFunctionCall %1 %5
+     OpReturnValue %7
+     OpFunctionEnd)";
+  CHECK(spirv, SPV_ERROR_INVALID_ID);
+}
+
+TEST_F(ValidateID, OpReturnValueIsVariable) {
+  const char* spirv = R"(
+%1 = OpTypeVoid
+%2 = OpTypeInt 32 0
+%3 = OpTypePointer Private %2
+%4 = OpTypeFunction %3
+%5 = OpFunction %3 None %4
+%6 = OpLabel
+%7 = OpVariable %3 Function
+     OpReturnValue %7
+     OpFunctionEnd)";
+  CHECK(spirv, SPV_ERROR_INVALID_ID);
+}
+
+// TODO: enable when this bug is fixed: https://cvs.khronos.org/bugzilla/show_bug.cgi?id=15404
+TEST_F(ValidateID, DISABLED_OpReturnValueIsFunction) {
+  const char* spirv = R"(
+%1 = OpTypeVoid
+%2 = OpTypeInt 32 0
+%3 = OpTypeFunction %2
+%5 = OpFunction %2 None %3
+%6 = OpLabel
+     OpReturnValue %5
+     OpFunctionEnd)";
+  CHECK(spirv, SPV_ERROR_INVALID_ID);
+}
+
 TEST_F(ValidateID, UndefinedTypeId) {
   const char* spirv = R"(
 %f32 = OpTypeFloat 32