From 10fa696af77287ab87c6bf5115af93ffeb36594d Mon Sep 17 00:00:00 2001
From: Dejan Mircevski <deki@google.com>
Date: Wed, 3 Feb 2016 10:26:59 -0500
Subject: [PATCH] Allow Op[No]Line between OpFunctionParameters.

Also ensure we don't get thrown off by too many parameters.
---
 source/validate_id.cpp    | 19 +++++++++++-----
 test/Validate.Layout.cpp  | 57 ++++++++++++++++++++++++++++++++++++++++++-----
 test/ValidateFixtures.cpp |  2 ++
 3 files changed, 67 insertions(+), 11 deletions(-)

diff --git a/source/validate_id.cpp b/source/validate_id.cpp
index ab09185..b45b6a8 100644
--- a/source/validate_id.cpp
+++ b/source/validate_id.cpp
@@ -964,19 +964,26 @@ bool idUsage::isValid<SpvOpFunctionParameter>(const spv_instruction_t* inst,
   size_t paramIndex = 0;
   assert(firstInst < inst && "Invalid instruction pointer");
   while (firstInst != --inst) {
-    spvCheck(
-        SpvOpFunction != inst->opcode && SpvOpFunctionParameter != inst->opcode,
-        DIAG(0) << "OpFunctionParameter is not preceded by OpFunction or "
-                   "OpFunctionParameter sequence.";
-        return false);
+    spvCheck(SpvOpFunction != inst->opcode &&
+                 SpvOpFunctionParameter != inst->opcode &&
+                 SpvOpLine != inst->opcode && SpvOpNoLine != inst->opcode,
+             DIAG(0) << "OpFunctionParameter is not preceded by OpFunction or "
+                        "OpFunctionParameter sequence.";
+             return false);
     if (SpvOpFunction == inst->opcode) {
       break;
-    } else {
+    } else if (SpvOpFunctionParameter == inst->opcode) {
       paramIndex++;
     }
   }
   auto functionType = usedefs_.FindDef(inst->words[4]);
   assert(functionType.first);
+  if (paramIndex >= functionType.second.words.size() - 3) {
+    DIAG(0) << "Too many OpFunctionParameters for " << inst->words[2]
+            << ": expected " << functionType.second.words.size() - 3
+            << " based on the function's type";
+    return false;
+  }
   auto paramType = usedefs_.FindDef(functionType.second.words[paramIndex + 3]);
   assert(paramType.first);
   spvCheck(resultType.second.id != paramType.second.id,
diff --git a/test/Validate.Layout.cpp b/test/Validate.Layout.cpp
index 3524d15..44ea7a5 100644
--- a/test/Validate.Layout.cpp
+++ b/test/Validate.Layout.cpp
@@ -26,16 +26,17 @@
 
 // Validation tests for Logical Layout
 
-#include "gmock/gmock.h"
-#include "source/diagnostic.h"
-#include "UnitSPIRV.h"
-#include "ValidateFixtures.h"
-
 #include <functional>
 #include <sstream>
 #include <string>
 #include <utility>
 
+#include "gmock/gmock.h"
+
+#include "UnitSPIRV.h"
+#include "ValidateFixtures.h"
+#include "source/diagnostic.h"
+
 using std::function;
 using std::ostream;
 using std::ostream_iterator;
@@ -306,5 +307,51 @@ TEST_F(ValidateLayout, FuncParameterNotImmediatlyAfterFuncBad) {
   ASSERT_EQ(SPV_ERROR_INVALID_LAYOUT, ValidateInstructions());
 }
 
+using ValidateOpFunctionParameter =
+    spvtest::ValidateBase<int, SPV_VALIDATE_LAYOUT_BIT | SPV_VALIDATE_ID_BIT>;
+
+TEST_F(ValidateOpFunctionParameter, OpLineBetweenParameters) {
+  const auto s = R"(
+OpCapability Shader
+OpMemoryModel Logical GLSL450
+%foo_frag = OpString "foo.frag"
+%i32 = OpTypeInt 32 1
+%tf = OpTypeFunction %i32 %i32 %i32
+%c = OpConstant %i32 123
+%f = OpFunction %i32 None %tf
+OpLine %foo_frag 1 1
+%p1 = OpFunctionParameter %i32
+OpNoLine
+%p2 = OpFunctionParameter %i32
+%l = OpLabel
+OpReturnValue %c
+OpFunctionEnd
+)";
+  CompileSuccessfully(s);
+  ASSERT_EQ(SPV_SUCCESS, ValidateInstructions());
+}
+
+TEST_F(ValidateOpFunctionParameter, TooManyParameters) {
+  const auto s = R"(
+OpCapability Shader
+OpMemoryModel Logical GLSL450
+%i32 = OpTypeInt 32 1
+%tf = OpTypeFunction %i32 %i32 %i32
+%c = OpConstant %i32 123
+%f = OpFunction %i32 None %tf
+%p1 = OpFunctionParameter %i32
+%p2 = OpFunctionParameter %i32
+%xp3 = OpFunctionParameter %i32
+%xp4 = OpFunctionParameter %i32
+%xp5 = OpFunctionParameter %i32
+%xp6 = OpFunctionParameter %i32
+%xp7 = OpFunctionParameter %i32
+%l = OpLabel
+OpReturnValue %c
+OpFunctionEnd
+)";
+  CompileSuccessfully(s);
+  ASSERT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+}
 // TODO(umar): Test optional instructions
 }
diff --git a/test/ValidateFixtures.cpp b/test/ValidateFixtures.cpp
index 98bb2be..91200ca 100644
--- a/test/ValidateFixtures.cpp
+++ b/test/ValidateFixtures.cpp
@@ -92,6 +92,8 @@ template class spvtest::ValidateBase<
     std::tuple<int, std::tuple<std::string, std::function<spv_result_t(int)>,
                                std::function<spv_result_t(int)>>>,
     SPV_VALIDATE_LAYOUT_BIT>;
+template class spvtest::ValidateBase<int, SPV_VALIDATE_LAYOUT_BIT |
+                                              SPV_VALIDATE_ID_BIT>;
 
 template class spvtest::ValidateBase<
     std::tuple<std::string, std::pair<std::string, std::vector<std::string>>>,
-- 
2.7.4