From 1a18739650fffa25a786bc35caeb34d4e82aad9f Mon Sep 17 00:00:00 2001
From: David Neto <dneto@google.com>
Date: Tue, 14 Jun 2016 16:41:27 -0400
Subject: [PATCH] Fix ExtInst parsing: no IdRef* at end

The operands following the extended instruction literal
number are determined by the extended instruction itself.
So drop the zero-or-more IdRef pattern at the end of OpExtInst.

It's arguable whether this should actually be a grammar fix.  I've
chosen to patch this in SPIRV-Tools instead of in the grammar file.

Fixes https://github.com/KhronosGroup/SPIRV-Tools/issues/233

Also fix two test cases for OpenCL extended instructions.  These
errors of supplying too many operands are now detected.
---
 test/BinaryParse.cpp             |  7 +++++++
 test/ExtInst.OpenCL.std.cpp      |  4 ++--
 test/TextToBinary.Extension.cpp  |  6 ++++++
 utils/generate_grammar_tables.py | 16 ++++++++++++++++
 4 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/test/BinaryParse.cpp b/test/BinaryParse.cpp
index ecffca9..e018752 100644
--- a/test/BinaryParse.cpp
+++ b/test/BinaryParse.cpp
@@ -663,6 +663,13 @@ INSTANTIATE_TEST_CASE_P(
                       MakeInstruction(SpvOpExtInst, {2, 3, 100, 4, 5})}),
          "OpExtInst set Id 100 does not reference an OpExtInstImport result "
          "Id"},
+        {Concatenate({ExpectedHeaderForBound(101),
+                      MakeInstruction(SpvOpExtInstImport, {100},
+                                      MakeVector("OpenCL.std")),
+                      // OpenCL cos is #14
+                      MakeInstruction(SpvOpExtInst, {2, 3, 100, 14, 5, 999})}),
+         "Invalid instruction OpExtInst starting at word 10: expected no "
+         "more operands after 6 words, but stated word count is 7."},
         // In this case, the OpSwitch selector refers to an invalid ID.
         {Concatenate({ExpectedHeaderForBound(3),
                       MakeInstruction(SpvOpSwitch, {1, 2, 42, 3})}),
diff --git a/test/ExtInst.OpenCL.std.cpp b/test/ExtInst.OpenCL.std.cpp
index b4ddaae..7c6e350 100644
--- a/test/ExtInst.OpenCL.std.cpp
+++ b/test/ExtInst.OpenCL.std.cpp
@@ -233,8 +233,8 @@ INSTANTIATE_TEST_CASE_P(
         CASE1(Popcount, popcount),
         CASE3(SMad24, s_mad24),
         CASE3(UMad24, u_mad24),
-        CASE3(SMul24, s_mul24),
-        CASE3(UMul24, u_mul24), // enum value 170
+        CASE2(SMul24, s_mul24),
+        CASE2(UMul24, u_mul24), // enum value 170
         CASE1(UAbs, u_abs), // enum value 201
         CASE2(UAbs_diff, u_abs_diff),
         CASE2(UMul_hi, u_mul_hi),
diff --git a/test/TextToBinary.Extension.cpp b/test/TextToBinary.Extension.cpp
index 18a91a8..7003608 100644
--- a/test/TextToBinary.Extension.cpp
+++ b/test/TextToBinary.Extension.cpp
@@ -68,6 +68,12 @@ TEST_F(TextToBinaryTest, MultiImport) {
               Eq("Import Id is being defined a second time"));
 }
 
+TEST_F(TextToBinaryTest, TooManyArguments) {
+  const std::string input = R"(%opencl = OpExtInstImport "OpenCL.std"
+                               %2 = OpExtInst %float %opencl cos %x %oops")";
+  EXPECT_THAT(CompileFailure(input), Eq("Expected '=', found end of stream."));
+}
+
 TEST_F(TextToBinaryTest, ExtInstFromTwoDifferentImports) {
   const std::string input = R"(%1 = OpExtInstImport "OpenCL.std"
 %2 = OpExtInstImport "GLSL.std.450"
diff --git a/utils/generate_grammar_tables.py b/utils/generate_grammar_tables.py
index 3c0b05e..f91f3ad 100755
--- a/utils/generate_grammar_tables.py
+++ b/utils/generate_grammar_tables.py
@@ -141,10 +141,26 @@ class InstInitializer(object):
         self.caps_mask = compose_capability_mask(caps)
         self.operands = [convert_operand_kind(o) for o in operands]
 
+        self.fix_syntax()
+
         operands = [o[0] for o in operands]
         self.ref_type_id = 'IdResultType' in operands
         self.def_result_id = 'IdResult' in operands
 
+
+    def fix_syntax(self):
+        """Fix an instruction's syntax, adjusting for differences between
+        the officially released grammar and how SPIRV-Tools uses the grammar.
+
+        Fixes:
+            - ExtInst should not end with SPV_OPERAND_VARIABLE_ID.
+            https://github.com/KhronosGroup/SPIRV-Tools/issues/233
+        """
+        if (self.opname == 'ExtInst'
+            and self.operands[-1] == 'SPV_OPERAND_TYPE_VARIABLE_ID'):
+           self.operands.pop()
+
+
     def __str__(self):
         template = ['{{"{opname}"', 'SpvOp{opname}', '{caps_mask}',
                     '{num_operands}', '{{{operands}}}',
-- 
2.7.4