diff --git a/CMakeLists.txt b/CMakeLists.txt index 051afa97e..e1f01bd04 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -174,6 +174,7 @@ if (NOT ${SPIRV_SKIP_EXECUTABLES}) ${CMAKE_CURRENT_SOURCE_DIR}/test/BinaryToText.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test/Comment.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test/DiagnosticPrint.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/test/DiagnosticStream.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test/ExtInstGLSLstd450.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test/FixWord.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test/ImmediateInt.cpp diff --git a/source/diagnostic.h b/source/diagnostic.h index 4e6d37896..27be491e4 100644 --- a/source/diagnostic.h +++ b/source/diagnostic.h @@ -31,6 +31,7 @@ #include #include +#include class diagnostic_helper { public: @@ -51,20 +52,26 @@ class diagnostic_helper { spv_diagnostic *pDiagnostic; }; -// On destruction of the diagnostic stream, a diagnostic message will be -// written to pDiagnostic containing all of the data written to the stream. +// A DiagnosticStream remembers the current position of the input and an error +// code, and captures diagnostic messages via the left-shift operator. +// Captured messages are emitted during the destructor. // TODO(awoloszyn): This is very similar to diagnostic_helper, and hides // the data more easily. Replace diagnostic_helper elsewhere // eventually. class DiagnosticStream { public: - DiagnosticStream(spv_position position, spv_diagnostic *pDiagnostic) - : position_(position), pDiagnostic_(pDiagnostic) {} + DiagnosticStream(spv_position position, spv_diagnostic *pDiagnostic, + spv_result_t error) + : position_(position), pDiagnostic_(pDiagnostic), error_(error) {} - DiagnosticStream(DiagnosticStream &&other) : position_(other.position_) { - stream_.str(other.stream_.str()); - other.stream_.str(""); - pDiagnostic_ = other.pDiagnostic_; + DiagnosticStream(DiagnosticStream &&other) + : stream_(other.stream_.str()), + position_(other.position_), + pDiagnostic_(other.pDiagnostic_), + error_(other.error_) { + // The other object's destructor will emit the text in its stream_ + // member if its pDiagnostic_ member is non-null. Prevent that, + // since emitting that text is now the responsibility of *this. other.pDiagnostic_ = nullptr; } @@ -77,10 +84,14 @@ class DiagnosticStream { return *this; } + // Conversion operator to spv_result, returning the error code. + operator spv_result_t() { return error_; } + private: std::stringstream stream_; spv_position position_; spv_diagnostic *pDiagnostic_; + spv_result_t error_; }; #define DIAGNOSTIC \ diff --git a/source/text.cpp b/source/text.cpp index 1a2ddca23..ddea72489 100644 --- a/source/text.cpp +++ b/source/text.cpp @@ -166,12 +166,10 @@ spv_result_t encodeImmediate(libspirv::AssemblyContext* context, const uint64_t parseResult = strtoull(begin, &end, 0); size_t length = end - begin; if (length != strlen(begin)) { - context->diagnostic() << "Invalid immediate integer '" << text << "'."; - return SPV_ERROR_INVALID_TEXT; + return context->diagnostic() << "Invalid immediate integer '" << text << "'."; } else if (length > 10 || (parseResult >> 32) != 0) { - context->diagnostic() << "Immediate integer '" << text + return context->diagnostic() << "Immediate integer '" << text << "' is outside the unsigned 32-bit range."; - return SPV_ERROR_INVALID_TEXT; } context->binaryEncodeU32(parseResult, pInst); context->seekForward(strlen(text)); @@ -217,12 +215,10 @@ spv_result_t spvTextEncodeOperand(const libspirv::AssemblyGrammar& grammar, if ('%' == textValue[0]) { textValue++; } else { - context->diagnostic() << "Expected id to start with %."; - return SPV_ERROR_INVALID_TEXT; + return context->diagnostic() << "Expected id to start with %."; } if (!spvIsValidID(textValue)) { - context->diagnostic() << "Invalid ID " << textValue; - return SPV_ERROR_INVALID_TEXT; + return context->diagnostic() << "Invalid ID " << textValue; } const uint32_t id = context->spvNamedIdAssignOrGet(textValue); if (type == SPV_OPERAND_TYPE_TYPE_ID) pInst->resultTypeId = id; @@ -233,9 +229,8 @@ spv_result_t spvTextEncodeOperand(const libspirv::AssemblyGrammar& grammar, if (OpExtInst == pInst->opcode) { spv_ext_inst_desc extInst; if (grammar.lookupExtInst(pInst->extInstType, textValue, &extInst)) { - context->diagnostic() << "Invalid extended instruction name '" - << textValue << "'."; - return SPV_ERROR_INVALID_TEXT; + return context->diagnostic() << "Invalid extended instruction name '" + << textValue << "'."; } spvInstructionAddWord(pInst, extInst->ext_inst); @@ -253,9 +248,8 @@ spv_result_t spvTextEncodeOperand(const libspirv::AssemblyGrammar& grammar, if (error != SPV_SUCCESS) { if (error == SPV_ERROR_OUT_OF_MEMORY) return error; if (spvOperandIsOptional(type)) return SPV_FAILED_MATCH; - context->diagnostic() << "Invalid literal number '" << textValue - << "'."; - return SPV_ERROR_INVALID_TEXT; + return context->diagnostic() << "Invalid literal number '" << textValue + << "'."; } // The encoding for OpConstant, OpSpecConstant and OpSwitch all @@ -272,20 +266,18 @@ spv_result_t spvTextEncodeOperand(const libspirv::AssemblyGrammar& grammar, if (SPV_SUCCESS == grammar.lookupOpcode(pInst->opcode, &d)) { opcode_name = d->name; } - context->diagnostic() - << "Type for " << opcode_name - << " must be a scalar floating point or integer type"; - return SPV_ERROR_INVALID_TEXT; + return context->diagnostic() + << "Type for " << opcode_name + << " must be a scalar floating point or integer type"; } } else if (pInst->opcode == OpSwitch) { // We need to know the value of the selector. libspirv::IdType type = context->getTypeOfValueInstruction(pInst->words[1]); if (type.type_class != libspirv::IdTypeClass::kScalarIntegerType) { - context->diagnostic() - << "The selector operand for OpSwitch must be the result" - " of an instruction that generates an integer scalar"; - return SPV_ERROR_INVALID_TEXT; + return context->diagnostic() + << "The selector operand for OpSwitch must be the result" + " of an instruction that generates an integer scalar"; } } // TODO(awoloszyn): Generate the correct assembly for arbitrary @@ -295,44 +287,36 @@ spv_result_t spvTextEncodeOperand(const libspirv::AssemblyGrammar& grammar, // We do not have to print diagnostics here because spvBinaryEncode* // prints diagnostic messages on failure. case SPV_LITERAL_TYPE_INT_32: - if (context->binaryEncodeU32(BitwiseCast(literal.value.i32), - pInst)) - return SPV_ERROR_INVALID_TEXT; + context->binaryEncodeU32(BitwiseCast(literal.value.i32), + pInst); break; case SPV_LITERAL_TYPE_INT_64: { - if (context->binaryEncodeU64(BitwiseCast(literal.value.i64), - pInst)) - return SPV_ERROR_INVALID_TEXT; + context->binaryEncodeU64(BitwiseCast(literal.value.i64), + pInst); } break; case SPV_LITERAL_TYPE_UINT_32: { - if (context->binaryEncodeU32(literal.value.u32, pInst)) - return SPV_ERROR_INVALID_TEXT; + context->binaryEncodeU32(literal.value.u32, pInst); } break; case SPV_LITERAL_TYPE_UINT_64: { - if (context->binaryEncodeU64(BitwiseCast(literal.value.u64), - pInst)) - return SPV_ERROR_INVALID_TEXT; + context->binaryEncodeU64(BitwiseCast(literal.value.u64), + pInst); } break; case SPV_LITERAL_TYPE_FLOAT_32: { - if (context->binaryEncodeU32(BitwiseCast(literal.value.f), - pInst)) - return SPV_ERROR_INVALID_TEXT; + context->binaryEncodeU32(BitwiseCast(literal.value.f), + pInst); } break; case SPV_LITERAL_TYPE_FLOAT_64: { - if (context->binaryEncodeU64(BitwiseCast(literal.value.d), - pInst)) - return SPV_ERROR_INVALID_TEXT; + context->binaryEncodeU64(BitwiseCast(literal.value.d), + pInst); } break; case SPV_LITERAL_TYPE_STRING: { - context->diagnostic() + return context->diagnostic(SPV_FAILED_MATCH) << "Expected literal number, found literal string '" << textValue << "'."; - return SPV_FAILED_MATCH; } break; default: - context->diagnostic() << "Invalid literal number '" << textValue - << "'"; - return SPV_ERROR_INVALID_TEXT; + return context->diagnostic() << "Invalid literal number '" + << textValue << "'"; } } break; case SPV_OPERAND_TYPE_LITERAL_STRING: @@ -342,15 +326,13 @@ spv_result_t spvTextEncodeOperand(const libspirv::AssemblyGrammar& grammar, if (error != SPV_SUCCESS) { if (error == SPV_ERROR_OUT_OF_MEMORY) return error; if (spvOperandIsOptional(type)) return SPV_FAILED_MATCH; - context->diagnostic() << "Invalid literal string '" << textValue - << "'."; - return SPV_ERROR_INVALID_TEXT; + return context->diagnostic() << "Invalid literal string '" << textValue + << "'."; } if (literal.type != SPV_LITERAL_TYPE_STRING) { - context->diagnostic() - << "Expected literal string, found literal number '" << textValue - << "'."; - return SPV_FAILED_MATCH; + return context->diagnostic(SPV_FAILED_MATCH) + << "Expected literal string, found literal number '" << textValue + << "'."; } // NOTE: Special case for extended instruction library import @@ -369,9 +351,8 @@ spv_result_t spvTextEncodeOperand(const libspirv::AssemblyGrammar& grammar, case SPV_OPERAND_TYPE_SELECTION_CONTROL: { uint32_t value; if (grammar.parseMaskOperand(type, textValue, &value)) { - context->diagnostic() << "Invalid " << spvOperandTypeStr(type) << " '" - << textValue << "'."; - return SPV_ERROR_INVALID_TEXT; + return context->diagnostic() << "Invalid " << spvOperandTypeStr(type) + << " '" << textValue << "'."; } if (auto error = context->binaryEncodeU32(value, pInst)) return error; // Prepare to parse the operands for this logical operand. @@ -394,9 +375,8 @@ spv_result_t spvTextEncodeOperand(const libspirv::AssemblyGrammar& grammar, textValue, pInst, pExpectedOperands); } if (error) { - context->diagnostic() << "Invalid word following !: " - << textValue; - return error; + return context->diagnostic(error) + << "Invalid word following !: " << textValue; } if (pExpectedOperands->empty()) { pExpectedOperands->push_back(SPV_OPERAND_TYPE_OPTIONAL_CIV); @@ -407,14 +387,12 @@ spv_result_t spvTextEncodeOperand(const libspirv::AssemblyGrammar& grammar, // table. spv_operand_desc entry; if (grammar.lookupOperand(type, textValue, strlen(textValue), &entry)) { - context->diagnostic() << "Invalid " << spvOperandTypeStr(type) << " '" - << textValue << "'."; - return SPV_ERROR_INVALID_TEXT; + return context->diagnostic() << "Invalid " << spvOperandTypeStr(type) + << " '" << textValue << "'."; } if (context->binaryEncodeU32(entry->value, pInst)) { - context->diagnostic() << "Invalid " << spvOperandTypeStr(type) << " '" - << textValue << "'."; - return SPV_ERROR_INVALID_TEXT; + return context->diagnostic() << "Invalid " << spvOperandTypeStr(type) + << " '" << textValue << "'."; } // Prepare to parse the operands for this logical operand. @@ -437,10 +415,7 @@ spv_result_t encodeInstructionStartingWithImmediate( std::string firstWord; spv_position_t nextPosition = {}; auto error = context->getWord(firstWord, &nextPosition); - if (error) { - context->diagnostic() << "Internal Error"; - return error; - } + if (error) return context->diagnostic(error) << "Internal Error"; if ((error = encodeImmediate(context, firstWord.c_str(), pInst))) { return error; @@ -452,15 +427,11 @@ spv_result_t encodeInstructionStartingWithImmediate( // Otherwise, there must be an operand that's either a literal, an ID, or // an immediate. std::string operandValue; - if ((error = context->getWord(operandValue, &nextPosition))) { - context->diagnostic() << "Internal Error"; - return error; - } + if ((error = context->getWord(operandValue, &nextPosition))) + return context->diagnostic(error) << "Internal Error"; - if (operandValue == "=") { - context->diagnostic() << firstWord << " not allowed before =."; - return SPV_ERROR_INVALID_TEXT; - } + if (operandValue == "=") + return context->diagnostic() << firstWord << " not allowed before =."; // Needed to pass to spvTextEncodeOpcode(), but it shouldn't ever be // expanded. @@ -501,10 +472,7 @@ spv_result_t spvTextEncodeOpcode(const libspirv::AssemblyGrammar& grammar, std::string firstWord; spv_position_t nextPosition = {}; spv_result_t error = context->getWord(firstWord, &nextPosition); - if (error) { - context->diagnostic() << "Internal Error"; - return error; - } + if (error) return context->diagnostic() << "Internal Error"; std::string opcodeName; std::string result_id; @@ -515,49 +483,39 @@ spv_result_t spvTextEncodeOpcode(const libspirv::AssemblyGrammar& grammar, // If the first word of this instruction is not an opcode, we must be // processing AAF now. if (SPV_ASSEMBLY_SYNTAX_FORMAT_ASSIGNMENT != format) { - context->diagnostic() - << "Expected at the beginning of an instruction, found '" - << firstWord << "'."; - return SPV_ERROR_INVALID_TEXT; + return context->diagnostic() + << "Expected at the beginning of an instruction, found '" + << firstWord << "'."; } result_id = firstWord; if ('%' != result_id.front()) { - context->diagnostic() - << "Expected or at the beginning " - "of an instruction, found '" - << result_id << "'."; - return SPV_ERROR_INVALID_TEXT; + return context->diagnostic() + << "Expected or at the beginning " + "of an instruction, found '" + << result_id << "'."; } result_id_position = context->position(); // The '=' sign. context->setPosition(nextPosition); - if (context->advance()) { - context->diagnostic() << "Expected '=', found end of stream."; - return SPV_ERROR_INVALID_TEXT; - } + if (context->advance()) + return context->diagnostic() << "Expected '=', found end of stream."; std::string equal_sign; error = context->getWord(equal_sign, &nextPosition); - if ("=" != equal_sign) { - context->diagnostic() << "'=' expected after result id."; - return SPV_ERROR_INVALID_TEXT; - } + if ("=" != equal_sign) + return context->diagnostic() << "'=' expected after result id."; // The after the '=' sign. context->setPosition(nextPosition); - if (context->advance()) { - context->diagnostic() << "Expected opcode, found end of stream."; - return SPV_ERROR_INVALID_TEXT; - } + if (context->advance()) + return context->diagnostic() << "Expected opcode, found end of stream."; error = context->getWord(opcodeName, &nextPosition); - if (error) { - context->diagnostic() << "Internal Error"; - return error; - } + if (error) + return context->diagnostic(error) << "Internal Error"; if (!context->startsWithOp()) { - context->diagnostic() << "Invalid Opcode prefix '" << opcodeName << "'."; - return SPV_ERROR_INVALID_TEXT; + return context->diagnostic() << "Invalid Opcode prefix '" << opcodeName + << "'."; } } @@ -567,17 +525,16 @@ spv_result_t spvTextEncodeOpcode(const libspirv::AssemblyGrammar& grammar, spv_opcode_desc opcodeEntry; error = grammar.lookupOpcode(pInstName, &opcodeEntry); if (error) { - context->diagnostic() << "Invalid Opcode name '" << context->getWord() - << "'"; - return error; + return context->diagnostic(error) << "Invalid Opcode name '" + << context->getWord() << "'"; } if (SPV_ASSEMBLY_SYNTAX_FORMAT_ASSIGNMENT == format) { // If this instruction has , check it follows AAF. if (opcodeEntry->hasResult && result_id.empty()) { - context->diagnostic() << "Expected at the beginning of an " - "instruction, found '" - << firstWord << "'."; - return SPV_ERROR_INVALID_TEXT; + return context->diagnostic() + << "Expected at the beginning of an " + "instruction, found '" + << firstWord << "'."; } } pInst->opcode = opcodeEntry->opcode; @@ -624,8 +581,8 @@ spv_result_t spvTextEncodeOpcode(const libspirv::AssemblyGrammar& grammar, // and we didn't find one. We're finished parsing this instruction. break; } else { - context->diagnostic() << "Expected operand, found end of stream."; - return SPV_ERROR_INVALID_TEXT; + return context->diagnostic() + << "Expected operand, found end of stream."; } } assert(error == SPV_SUCCESS && "Somebody added another way to fail"); @@ -634,18 +591,14 @@ spv_result_t spvTextEncodeOpcode(const libspirv::AssemblyGrammar& grammar, if (spvOperandIsOptional(type)) { break; } else { - context->diagnostic() - << "Expected operand, found next instruction instead."; - return SPV_ERROR_INVALID_TEXT; + return context->diagnostic() + << "Expected operand, found next instruction instead."; } } std::string operandValue; error = context->getWord(operandValue, &nextPosition); - if (error) { - context->diagnostic() << "Internal Error"; - return error; - } + if (error) return context->diagnostic(error) << "Internal Error"; error = spvTextEncodeOperand(grammar, context, type, operandValue.c_str(), pInst, &expectedOperands); @@ -672,10 +625,10 @@ spv_result_t spvTextEncodeOpcode(const libspirv::AssemblyGrammar& grammar, } if (pInst->words.size() > SPV_LIMIT_INSTRUCTION_WORD_COUNT_MAX) { - context->diagnostic() << "Instruction too long: " << pInst->words.size() - << " words, but the limit is " - << SPV_LIMIT_INSTRUCTION_WORD_COUNT_MAX; - return SPV_ERROR_INVALID_TEXT; + return context->diagnostic() + << "Instruction too long: " << pInst->words.size() + << " words, but the limit is " + << SPV_LIMIT_INSTRUCTION_WORD_COUNT_MAX; } pInst->words[0] = spvOpcodeMake(pInst->words.size(), opcodeEntry->opcode); @@ -695,10 +648,9 @@ spv_result_t spvTextToBinaryInternal(const libspirv::AssemblyGrammar& grammar, spv_diagnostic* pDiagnostic) { if (!pDiagnostic) return SPV_ERROR_INVALID_DIAGNOSTIC; libspirv::AssemblyContext context(text, pDiagnostic); - if (!text->str || !text->length) { - context.diagnostic() << "Text stream is empty."; - return SPV_ERROR_INVALID_TEXT; - } + if (!text->str || !text->length) + return context.diagnostic() << "Text stream is empty."; + if (!grammar.isValid()) { return SPV_ERROR_INVALID_TABLE; } @@ -709,10 +661,7 @@ spv_result_t spvTextToBinaryInternal(const libspirv::AssemblyGrammar& grammar, std::vector instructions; - if (context.advance()) { - context.diagnostic() << "Text stream is empty."; - return SPV_ERROR_INVALID_TEXT; - } + if (context.advance()) return context.diagnostic() << "Text stream is empty."; spv_ext_inst_type_t extInstType = SPV_EXT_INST_TYPE_NONE; while (context.hasText()) { diff --git a/source/text_handler.cpp b/source/text_handler.cpp index 0f6f1354c..329a6eae6 100644 --- a/source/text_handler.cpp +++ b/source/text_handler.cpp @@ -333,11 +333,9 @@ spv_result_t AssemblyContext::binaryEncodeU64(const uint64_t value, spv_instruction_t *pInst) { uint32_t low = (uint32_t)(0x00000000ffffffff & value); uint32_t high = (uint32_t)((0xffffffff00000000 & value) >> 32); - spv_result_t err = binaryEncodeU32(low, pInst); - if (err != SPV_SUCCESS) { - return err; - } - return binaryEncodeU32(high, pInst); + binaryEncodeU32(low, pInst); + binaryEncodeU32(high, pInst); + return SPV_SUCCESS; } spv_result_t AssemblyContext::binaryEncodeString( diff --git a/source/text_handler.h b/source/text_handler.h index e67bb8240..5ff9f46ec 100644 --- a/source/text_handler.h +++ b/source/text_handler.h @@ -168,10 +168,16 @@ class AssemblyContext { bool isStartOfNewInst(); // Returns a diagnostic object initialized with current position in the input - // stream. Any data written to this object will show up in pDiagnsotic on - // destruction. + // stream, and for the given error code. Any data written to this object will + // show up in pDiagnsotic on destruction. + DiagnosticStream diagnostic(spv_result_t error) { + return DiagnosticStream(¤t_position_, pDiagnostic_, error); + } + + // Returns a diagnostic object with the default assembly error code. DiagnosticStream diagnostic() { - return DiagnosticStream(¤t_position_, pDiagnostic_); + // The default failure for assembly is invalid text. + return diagnostic(SPV_ERROR_INVALID_TEXT); } // Returns then next characted in the input stream. diff --git a/test/DiagnosticStream.cpp b/test/DiagnosticStream.cpp new file mode 100644 index 000000000..f7641f19b --- /dev/null +++ b/test/DiagnosticStream.cpp @@ -0,0 +1,46 @@ +// 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. + +#include "UnitSPIRV.h" + +namespace { + +TEST(DiagnosticStream, ConversionToResultType) { + // Check after the DiagnosticStream object is destroyed. + spv_result_t value; + { value = DiagnosticStream(0, 0, SPV_ERROR_INVALID_TEXT); } + EXPECT_EQ(SPV_ERROR_INVALID_TEXT, value); + + // Check implicit conversion via plain assignment. + value = DiagnosticStream(0, 0, SPV_SUCCESS); + EXPECT_EQ(SPV_SUCCESS, value); + + // Check conversion via constructor. + EXPECT_EQ(SPV_FAILED_MATCH, + spv_result_t(DiagnosticStream(0, 0, SPV_FAILED_MATCH))); +} + +} // anonymous namespace