OpDecorate should not accept any number of literal operands.

This is a grammar fix.  The Decoration operand of OpDecorate (and
OpMemberDecorate) determines the remaining operands.  Don't just
allow any number of literal numbers as operands.

(The OperandVariableLiterals operand class as the last member
of the OpDecorate and OpMemberDecorate entries in in opcode.inc is
an artifact of how the spec generates the opcode descriptions. It's
not suitable for parsing those instructions.)

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

diff --git a/source/opcode.cpp b/source/opcode.cpp
index 9ddcafd..3027bcd 100644 (file)
--- a/source/opcode.cpp
+++ b/source/opcode.cpp
@@ -99,9 +99,18 @@ spv_operand_type_t convertOperandClassToType(SpvOp opcode,
         break;
     }
   } else if (operandClass == OperandVariableLiterals) {
-    if (opcode == SpvOpConstant || opcode == SpvOpSpecConstant) {
-      // The number type is determined by the type Id operand.
-      return SPV_OPERAND_TYPE_TYPED_LITERAL_NUMBER;
+    switch (opcode) {
+      case SpvOpConstant:
+      case SpvOpSpecConstant:
+        // The number type is determined by the type Id operand.
+        return SPV_OPERAND_TYPE_TYPED_LITERAL_NUMBER;
+      case SpvOpDecorate:
+      case SpvOpMemberDecorate:
+        // The operand types at the end of the instruction are
+        // determined instead by the decoration kind.
+        return SPV_OPERAND_TYPE_NONE;
+      default:
+        break;
     }
   }
 

diff --git a/test/BinaryParse.cpp b/test/BinaryParse.cpp
index 95b993b..0b94089 100644 (file)
--- a/test/BinaryParse.cpp
+++ b/test/BinaryParse.cpp
@@ -555,6 +555,42 @@ INSTANTIATE_TEST_CASE_P(
                       {0 /* this word does not belong*/}}),
          "Invalid instruction OpString starting at word 5: expected no more"
          " operands after 3 words, but stated word count is 4."},
+        // Word count is too large.  There are too many words after the string
+        // literal.  A linkage attribute decoration is the only case in SPIR-V
+        // where a string operand is followed by another operand.
+        {Concatenate({ExpectedHeaderForBound(2),
+                      {spvOpcodeMake(6, SpvOpDecorate), 1 /* target id */,
+                       uint32_t(SpvDecorationLinkageAttributes)},
+                      MakeVector("abc"),
+                      {uint32_t(SpvLinkageTypeImport),
+                       0 /* does not belong */}}),
+         "Invalid instruction OpDecorate starting at word 5: expected no more"
+         " operands after 5 words, but stated word count is 6."},
+        // Same as the previous case, but with OpMemberDecorate.
+        {Concatenate(
+             {ExpectedHeaderForBound(2),
+              {spvOpcodeMake(7, SpvOpMemberDecorate), 1 /* target id */,
+               42 /* member index */, uint32_t(SpvDecorationLinkageAttributes)},
+              MakeVector("abc"),
+              {uint32_t(SpvLinkageTypeImport), 0 /* does not belong */}}),
+         "Invalid instruction OpMemberDecorate starting at word 5: expected no"
+         " more operands after 6 words, but stated word count is 7."},
+        // Word count is too large.  There should be no more words
+        // after the RelaxedPrecision decoration.
+        {Concatenate({ExpectedHeaderForBound(2),
+                      {spvOpcodeMake(4, SpvOpDecorate), 1 /* target id */,
+                       uint32_t(SpvDecorationRelaxedPrecision),
+                       0 /* does not belong */}}),
+         "Invalid instruction OpDecorate starting at word 5: expected no"
+         " more operands after 3 words, but stated word count is 4."},
+        // Word count is too large.  There should be only one word after
+        // the SpecId decoration enum word.
+        {Concatenate({ExpectedHeaderForBound(2),
+                      {spvOpcodeMake(5, SpvOpDecorate), 1 /* target id */,
+                       uint32_t(SpvDecorationSpecId), 42 /* the spec id */,
+                       0 /* does not belong */}}),
+         "Invalid instruction OpDecorate starting at word 5: expected no"
+         " more operands after 4 words, but stated word count is 5."},
         {Concatenate({ExpectedHeaderForBound(2),
                       {spvOpcodeMake(2, SpvOpTypeVoid), 0}}),
          "Error: Result Id is 0"},

diff --git a/test/TextToBinary.Annotation.cpp b/test/TextToBinary.Annotation.cpp
index 717d41b..3e6d773 100644 (file)
--- a/test/TextToBinary.Annotation.cpp
+++ b/test/TextToBinary.Annotation.cpp
@@ -52,10 +52,13 @@ TEST_P(OpDecorateSimpleTest, AnySimpleDecoration) {
   std::stringstream input;
   input << "OpDecorate %1 " << GetParam().name();
   for (auto operand : GetParam().operands()) input << " " << operand;
+  input << std::endl;
   EXPECT_THAT(
       CompiledInstructions(input.str()),
       Eq(MakeInstruction(SpvOpDecorate, {1, uint32_t(GetParam().value())},
                          GetParam().operands())));
+  // Also check disassembly.
+  EXPECT_THAT(EncodeAndDecodeSuccessfully(input.str()), Eq(input.str()));
 }
 
 #define CASE(NAME) SpvDecoration##NAME, #NAME
@@ -111,6 +114,25 @@ TEST_F(OpDecorateSimpleTest, WrongDecoration) {
               Eq("Invalid decoration 'xxyyzz'."));
 }
 
+TEST_F(OpDecorateSimpleTest, ExtraOperandsOnDecorationExpectingNone) {
+  EXPECT_THAT(CompileFailure("OpDecorate %1 RelaxedPrecision 99"),
+              Eq("Expected <opcode> or <result-id> at the beginning of an "
+                 "instruction, found '99'."));
+}
+
+TEST_F(OpDecorateSimpleTest, ExtraOperandsOnDecorationExpectingOne) {
+  EXPECT_THAT(CompileFailure("OpDecorate %1 SpecId 99 100"),
+              Eq("Expected <opcode> or <result-id> at the beginning of an "
+                 "instruction, found '100'."));
+}
+
+TEST_F(OpDecorateSimpleTest, ExtraOperandsOnDecorationExpectingTwo) {
+  EXPECT_THAT(
+      CompileFailure("OpDecorate %1 LinkageAttributes \"abc\" Import 42"),
+      Eq("Expected <opcode> or <result-id> at the beginning of an "
+         "instruction, found '42'."));
+}
+
 // A single test case for an enum decoration.
 struct DecorateEnumCase {
   // Place the enum value first, so it's easier to read the binary dumps when
@@ -369,7 +391,100 @@ TEST_F(TextToBinaryTest, GroupMemberDecorateInvalidSecondTargetMemberNumber) {
               Eq("Invalid unsigned integer literal: %id2"));
 }
 
-// TODO(dneto): OpMemberDecorate
+// Test OpMemberDecorate
+
+using OpMemberDecorateSimpleTest = spvtest::TextToBinaryTestBase<
+    ::testing::TestWithParam<EnumCase<SpvDecoration>>>;
+
+TEST_P(OpMemberDecorateSimpleTest, AnySimpleDecoration) {
+  // This string should assemble, but should not validate.
+  std::stringstream input;
+  input << "OpMemberDecorate %1 42 " << GetParam().name();
+  for (auto operand : GetParam().operands()) input << " " << operand;
+  input << std::endl;
+  EXPECT_THAT(CompiledInstructions(input.str()),
+              Eq(MakeInstruction(SpvOpMemberDecorate,
+                                 {1, 42, uint32_t(GetParam().value())},
+                                 GetParam().operands())));
+  // Also check disassembly.
+  EXPECT_THAT(EncodeAndDecodeSuccessfully(input.str()), Eq(input.str()));
+}
+
+#define CASE(NAME) SpvDecoration##NAME, #NAME
+INSTANTIATE_TEST_CASE_P(
+    TextToBinaryDecorateSimple, OpMemberDecorateSimpleTest,
+    ::testing::ValuesIn(std::vector<EnumCase<SpvDecoration>>{
+        // The operand literal values are arbitrarily chosen,
+        // but there are the right number of them.
+        {CASE(RelaxedPrecision), {}},
+        {CASE(SpecId), {100}},
+        {CASE(Block), {}},
+        {CASE(BufferBlock), {}},
+        {CASE(RowMajor), {}},
+        {CASE(ColMajor), {}},
+        {CASE(ArrayStride), {4}},
+        {CASE(MatrixStride), {16}},
+        {CASE(GLSLShared), {}},
+        {CASE(GLSLPacked), {}},
+        {CASE(CPacked), {}},
+        // Placeholder line for enum value 12
+        {CASE(NoPerspective), {}},
+        {CASE(Flat), {}},
+        {CASE(Patch), {}},
+        {CASE(Centroid), {}},
+        {CASE(Sample), {}},
+        {CASE(Invariant), {}},
+        {CASE(Restrict), {}},
+        {CASE(Aliased), {}},
+        {CASE(Volatile), {}},
+        {CASE(Constant), {}},
+        {CASE(Coherent), {}},
+        {CASE(NonWritable), {}},
+        {CASE(NonReadable), {}},
+        {CASE(Uniform), {}},
+        {CASE(SaturatedConversion), {}},
+        {CASE(Stream), {2}},
+        {CASE(Location), {6}},
+        {CASE(Component), {3}},
+        {CASE(Index), {14}},
+        {CASE(Binding), {19}},
+        {CASE(DescriptorSet), {7}},
+        {CASE(Offset), {12}},
+        {CASE(XfbBuffer), {1}},
+        {CASE(XfbStride), {8}},
+        {CASE(NoContraction), {}},
+        {CASE(InputAttachmentIndex), {102}},
+        {CASE(Alignment), {16}},
+    }));
+#undef CASE
+
+TEST_F(OpMemberDecorateSimpleTest, WrongDecoration) {
+  EXPECT_THAT(CompileFailure("OpMemberDecorate %1 9 xxyyzz"),
+              Eq("Invalid decoration 'xxyyzz'."));
+}
+
+TEST_F(OpMemberDecorateSimpleTest, ExtraOperandsOnDecorationExpectingNone) {
+  EXPECT_THAT(CompileFailure("OpMemberDecorate %1 12 RelaxedPrecision 99"),
+              Eq("Expected <opcode> or <result-id> at the beginning of an "
+                 "instruction, found '99'."));
+}
+
+TEST_F(OpMemberDecorateSimpleTest, ExtraOperandsOnDecorationExpectingOne) {
+  EXPECT_THAT(CompileFailure("OpMemberDecorate %1 0 SpecId 99 100"),
+              Eq("Expected <opcode> or <result-id> at the beginning of an "
+                 "instruction, found '100'."));
+}
+
+TEST_F(OpMemberDecorateSimpleTest, ExtraOperandsOnDecorationExpectingTwo) {
+  EXPECT_THAT(CompileFailure(
+                  "OpMemberDecorate %1 1 LinkageAttributes \"abc\" Import 42"),
+              Eq("Expected <opcode> or <result-id> at the beginning of an "
+                 "instruction, found '42'."));
+}
+
+// TODO(dneto): OpMemberDecorate cases for decorations with parameters which
+// are: not just lists of literal numbers.
+
 // TODO(dneto): OpDecorationGroup
 // TODO(dneto): OpGroupDecorate