Clean up code for encoding literal operands.

diff --git a/CMakeLists.txt b/CMakeLists.txt
index f2f00de..431300d 100644 (file)
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -191,6 +191,7 @@ if (NOT ${SPIRV_SKIP_EXECUTABLES})
       ${CMAKE_CURRENT_SOURCE_DIR}/test/TextToBinary.Debug.cpp
       ${CMAKE_CURRENT_SOURCE_DIR}/test/TextToBinary.Function.cpp
       ${CMAKE_CURRENT_SOURCE_DIR}/test/TextToBinary.Group.cpp
+      ${CMAKE_CURRENT_SOURCE_DIR}/test/TextToBinary.Literal.cpp
       ${CMAKE_CURRENT_SOURCE_DIR}/test/TextToBinary.Memory.cpp
       ${CMAKE_CURRENT_SOURCE_DIR}/test/TextToBinary.Miscellaneous.cpp
       ${CMAKE_CURRENT_SOURCE_DIR}/test/TextToBinary.ModeSetting.cpp

diff --git a/include/libspirv/libspirv.h b/include/libspirv/libspirv.h
index 99df36f..af67d3e 100644 (file)
--- a/include/libspirv/libspirv.h
+++ b/include/libspirv/libspirv.h
@@ -144,6 +144,9 @@ typedef enum spv_operand_type_t {
   SPV_OPERAND_TYPE_NONE = 0,
   SPV_OPERAND_TYPE_ID,
   SPV_OPERAND_TYPE_RESULT_ID,
+  // TODO(antiagainst): Instructions in opcode.inc that use
+  // SPV_OPERAND_TYPE_LITERAL should in fact use LITERAL_NUMBER. So
+  // SPV_OPERAND_TYPE_LITERAL can be removed.
   SPV_OPERAND_TYPE_LITERAL,  // Either a literal number or literal string
   SPV_OPERAND_TYPE_LITERAL_NUMBER,
   // A literal number that can (but is not required to) expand multiple words.
@@ -185,6 +188,7 @@ typedef enum spv_operand_type_t {
   SPV_OPERAND_TYPE_OPTIONAL_ID,
   SPV_OPERAND_TYPE_OPTIONAL_IMAGE,
   // A literal number or string, but optional.
+  // TODO(antiagainst): change to SPV_OPERAND_TYPE_OPTIONAL_LITERAL_NUMBER.
   SPV_OPERAND_TYPE_OPTIONAL_LITERAL,
   // An optional literal string.
   SPV_OPERAND_TYPE_OPTIONAL_LITERAL_STRING,

diff --git a/source/text.cpp b/source/text.cpp
index 0cdeb7d..afb50a9 100644 (file)
--- a/source/text.cpp
+++ b/source/text.cpp
@@ -478,30 +478,19 @@ spv_result_t spvTextEncodeOperand(
 
         return SPV_SUCCESS;
       }
-
-      // TODO: Literal numbers can be any number up to 64 bits wide. This
-      // includes integers and floating point numbers.
-      // TODO(dneto): Suggest using spvTextToLiteral and looking for an
-      // appropriate result type.
-      if (spvTextToUInt32(textValue, &pInst->words[pInst->wordCount++])) {
-        DIAGNOSTIC << "Invalid literal number '" << textValue << "'.";
-        return SPV_ERROR_INVALID_TEXT;
-      }
-    } break;
-    // TODO(antiagainst): the handling of literal numbers in this function need
-    // to be reorganized.
+    }  // Fall through for the general case.
     case SPV_OPERAND_TYPE_MULTIWORD_LITERAL_NUMBER:
+    // TODO(antiagainst): SPV_OPERAND_TYPE_LITERAL should be removed.
     case SPV_OPERAND_TYPE_LITERAL:
     case SPV_OPERAND_TYPE_LITERAL_IN_OPTIONAL_TUPLE:
     case SPV_OPERAND_TYPE_OPTIONAL_LITERAL: {
       spv_literal_t literal = {};
-      if (spvTextToLiteral(textValue, &literal) != SPV_SUCCESS) {
-        if (spvOperandIsOptional(type)) {
-          return SPV_FAILED_MATCH;
-        } else {
-          DIAGNOSTIC << "Invalid literal '" << textValue << "'.";
-          return SPV_ERROR_INVALID_TEXT;
-        }
+      spv_result_t error = spvTextToLiteral(textValue, &literal);
+      if (error != SPV_SUCCESS) {
+        if (error == SPV_ERROR_OUT_OF_MEMORY) return error;
+        if (spvOperandIsOptional(type)) return SPV_FAILED_MATCH;
+        DIAGNOSTIC << "Invalid literal number '" << textValue << "'.";
+        return SPV_ERROR_INVALID_TEXT;
       }
       switch (literal.type) {
         // We do not have to print diagnostics here because spvBinaryEncode*
@@ -537,33 +526,38 @@ spv_result_t spvTextEncodeOperand(
             return SPV_ERROR_INVALID_TEXT;
         } break;
         case SPV_LITERAL_TYPE_STRING: {
-          if (spvBinaryEncodeString(literal.value.str, pInst, position,
-                                    pDiagnostic))
-            return SPV_ERROR_INVALID_TEXT;
+          DIAGNOSTIC << "Expected literal number, found literal string '"
+                     << textValue << "'.";
+          return SPV_FAILED_MATCH;
         } break;
         default:
-          DIAGNOSTIC << "Invalid literal '" << textValue << "'";
+          DIAGNOSTIC << "Invalid literal number '" << textValue << "'";
           return SPV_ERROR_INVALID_TEXT;
       }
     } break;
     case SPV_OPERAND_TYPE_LITERAL_STRING:
     case SPV_OPERAND_TYPE_OPTIONAL_LITERAL_STRING: {
-      size_t len = strlen(textValue);
-      if ('"' != textValue[0] && '"' != textValue[len - 1]) {
+      spv_literal_t literal = {};
+      spv_result_t error = spvTextToLiteral(textValue, &literal);
+      if (error != SPV_SUCCESS) {
+        if (error == SPV_ERROR_OUT_OF_MEMORY) return error;
         if (spvOperandIsOptional(type)) return SPV_FAILED_MATCH;
-        DIAGNOSTIC << "Invalid literal string '" << textValue
-                   << "', expected quotes.";
+        DIAGNOSTIC << "Invalid literal string '" << textValue << "'.";
         return SPV_ERROR_INVALID_TEXT;
       }
-      // NOTE: Strip quotes
-      std::string text(textValue + 1, len - 2);
+      if (literal.type != SPV_LITERAL_TYPE_STRING) {
+        DIAGNOSTIC << "Expected literal string, found literal number '"
+                   << textValue << "'.";
+        return SPV_FAILED_MATCH;
+      }
 
       // NOTE: Special case for extended instruction library import
       if (OpExtInstImport == pInst->opcode) {
-        pInst->extInstType = spvExtInstImportTypeGet(text.c_str());
+        pInst->extInstType = spvExtInstImportTypeGet(literal.value.str);
       }
 
-      if (spvBinaryEncodeString(text.c_str(), pInst, position, pDiagnostic))
+      if (spvBinaryEncodeString(literal.value.str, pInst, position,
+                                pDiagnostic))
         return SPV_ERROR_INVALID_TEXT;
     } break;
     case SPV_OPERAND_TYPE_OPTIONAL_IMAGE:
@@ -661,10 +655,19 @@ spv_result_t encodeInstructionStartingWithImmediate(
     // expanded.
     spv_operand_pattern_t dummyExpectedOperands;
     error = spvTextEncodeOperand(
+        // TODO(antiagainst): SPV_OPERAND_TYPE_OPTIONAL_LITERAL should be
+        // substituted by SPV_OPERAND_TYPE_OPTIONAL_LITERAL_NUMBER.
         SPV_OPERAND_TYPE_OPTIONAL_LITERAL, operandValue.c_str(), operandTable,
         extInstTable, namedIdTable, pInst, &dummyExpectedOperands, pBound,
         position, pDiagnostic);
     if (error == SPV_FAILED_MATCH) {
+      // It's not a literal number -- is it a literal string?
+      error = spvTextEncodeOperand(
+          SPV_OPERAND_TYPE_OPTIONAL_LITERAL_STRING, operandValue.c_str(),
+          operandTable, extInstTable, namedIdTable, pInst,
+          &dummyExpectedOperands, pBound, position, pDiagnostic);
+    }
+    if (error == SPV_FAILED_MATCH) {
       // It's not a literal -- is it an ID?
       error = spvTextEncodeOperand(
           SPV_OPERAND_TYPE_OPTIONAL_ID, operandValue.c_str(), operandTable,

diff --git a/test/TextToBinary.Literal.cpp b/test/TextToBinary.Literal.cpp
new file mode 100644 (file)
index 0000000..50cc2ca
--- /dev/null
+++ b/test/TextToBinary.Literal.cpp
@@ -0,0 +1,64 @@
+// Copyright (c) 2015 The Khronos Group Inc.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and/or associated documentation files (the
+// "Materials"), to deal in the Materials without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Materials, and to
+// permit persons to whom the Materials are furnished to do so, subject to
+// the following conditions:
+//
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Materials.
+//
+// MODIFICATIONS TO THIS FILE MAY MEAN IT NO LONGER ACCURATELY REFLECTS
+// KHRONOS STANDARDS. THE UNMODIFIED, NORMATIVE VERSIONS OF KHRONOS
+// SPECIFICATIONS AND HEADER INFORMATION ARE LOCATED AT
+//    https://www.khronos.org/registry/
+//
+// THE MATERIALS ARE PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+// EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.
+// IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY
+// CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
+// TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
+// MATERIALS OR THE USE OR OTHER DEALINGS IN THE MATERIALS.
+
+// Assembler tests for literal numbers and literal strings.
+
+#include "TestFixture.h"
+
+namespace {
+
+using test_fixture::TextToBinaryTest;
+
+TEST_F(TextToBinaryTest, LiteralStringInPlaceOfLiteralNumber) {
+  EXPECT_EQ(
+      R"(Expected literal number, found literal string '"I shouldn't be a string"'.)",
+      CompileFailure(R"(OpSource GLSL "I shouldn't be a string")"));
+}
+
+TEST_F(TextToBinaryTest, GarbageInPlaceOfLiteralString) {
+  EXPECT_EQ(
+      R"(Invalid literal string 'nice-source-code'.)",
+      CompileFailure(R"(OpSourceExtension nice-source-code)"));
+}
+
+TEST_F(TextToBinaryTest, LiteralNumberInPlaceOfLiteralString) {
+  EXPECT_EQ(
+      R"(Expected literal string, found literal number '1000'.)",
+      CompileFailure(R"(OpSourceExtension 1000)"));
+}
+
+// TODO(antiagainst): libspirv.h defines SPV_LIMIT_INSTRUCTION_WORD_COUNT_MAX
+// to be 0x108. Lift that limit and enable the following test.
+TEST_F(TextToBinaryTest, DISABLED_LiteralStringTooLong) {
+  const std::string code =
+      "OpSourceExtension \"" + std::string(65534, 'o') + "\"\n";
+  const std::string header =
+      "; SPIR-V\n; Version: 99\n; Generator: Khronos\n; "
+      "Bound: 1\n; Schema: 0\n";
+  EXPECT_EQ(header + code, EncodeAndDecodeSuccessfully(code));
+}
+
+}  // anonymous namespace