From d9a26939d3ab8e00f65a5e182ec8a21a6a6008f9 Mon Sep 17 00:00:00 2001 From: Francis Visoiu Mistrih Date: Tue, 16 Jul 2019 15:25:05 +0000 Subject: [PATCH] [Remarks] Simplify and refactor the RemarkParser interface Before, everything was based on some kind of type erased parser implementation which container a lot of boilerplate code when multiple formats were to be supported. This simplifies it by: * the remark now owns its arguments * *always* returning an error from the implementation side * working around the way the YAML parser reports errors: catch them through callbacks and re-insert them in a proper llvm::Error * add a CParser wrapper that is used when implementing the C API to avoid cluttering the C++ API with useless state * LLVMRemarkParserGetNext now returns an object that needs to be released to avoid leaking resources * add a new API to dispose of a remark entry: LLVMRemarkEntryDispose llvm-svn: 366217 --- docs/Remarks.rst | 1 + include/llvm-c/Remarks.h | 23 +- include/llvm/IR/RemarkStreamer.h | 12 +- include/llvm/Remarks/Remark.h | 15 +- include/llvm/Remarks/RemarkParser.h | 42 +- include/llvm/Support/SourceMgr.h | 2 + lib/IR/RemarkStreamer.cpp | 14 +- lib/Remarks/Remark.cpp | 4 + lib/Remarks/RemarkParser.cpp | 141 ++---- lib/Remarks/RemarkParserImpl.h | 33 -- lib/Remarks/YAMLRemarkParser.cpp | 439 +++++++++++-------- lib/Remarks/YAMLRemarkParser.h | 130 ++---- tools/llvm-opt-report/OptReport.cpp | 26 +- tools/remarks-shlib/Remarks.exports | 1 + unittests/Remarks/YAMLRemarksParsingTest.cpp | 101 +++-- 15 files changed, 488 insertions(+), 496 deletions(-) delete mode 100644 lib/Remarks/RemarkParserImpl.h diff --git a/docs/Remarks.rst b/docs/Remarks.rst index 8215efbeebc..e3d088d777d 100644 --- a/docs/Remarks.rst +++ b/docs/Remarks.rst @@ -295,6 +295,7 @@ The typical usage through the C API is like the following: LLVMRemarkEntryRef Remark = NULL; while ((Remark = LLVMRemarkParserGetNext(Parser))) { // use Remark + LLVMRemarkEntryDispose(Remark); // Release memory. } bool HasError = LLVMRemarkParserHasError(Parser); LLVMRemarkParserDispose(Parser); diff --git a/include/llvm-c/Remarks.h b/include/llvm-c/Remarks.h index 7fb16656a9a..88eb5120c57 100644 --- a/include/llvm-c/Remarks.h +++ b/include/llvm-c/Remarks.h @@ -136,6 +136,13 @@ extern LLVMRemarkDebugLocRef LLVMRemarkArgGetDebugLoc(LLVMRemarkArgRef Arg); */ typedef struct LLVMRemarkOpaqueEntry *LLVMRemarkEntryRef; +/** + * Free the resources used by the remark entry. + * + * \since REMARKS_API_VERSION=0 + */ +extern void LLVMRemarkEntryDispose(LLVMRemarkEntryRef Remark); + /** * The type of the remark. For example, it can allow users to only keep the * missed optimizations from the compiler. @@ -161,7 +168,7 @@ extern LLVMRemarkStringRef LLVMRemarkEntryGetRemarkName(LLVMRemarkEntryRef Remark); /** - * Get the name of the function being processsed when the remark was emitted. + * Get the name of the function being processed when the remark was emitted. * * \since REMARKS_API_VERSION=0 */ @@ -199,6 +206,8 @@ extern uint32_t LLVMRemarkEntryGetNumArgs(LLVMRemarkEntryRef Remark); * * If there are no arguments in \p Remark, the return value will be `NULL`. * + * The lifetime of the returned value is bound to the lifetime of \p Remark. + * * \since REMARKS_API_VERSION=0 */ extern LLVMRemarkArgRef LLVMRemarkEntryGetFirstArg(LLVMRemarkEntryRef Remark); @@ -208,6 +217,8 @@ extern LLVMRemarkArgRef LLVMRemarkEntryGetFirstArg(LLVMRemarkEntryRef Remark); * * Returns `NULL` if there are no more arguments available. * + * The lifetime of the returned value is bound to the lifetime of \p Remark. + * * \since REMARKS_API_VERSION=0 */ extern LLVMRemarkArgRef LLVMRemarkEntryGetNextArg(LLVMRemarkArgRef It, @@ -232,8 +243,11 @@ extern LLVMRemarkParserRef LLVMRemarkParserCreateYAML(const void *Buf, /** * Returns the next remark in the file. * - * The value pointed to by the return value is invalidated by the next call to - * LLVMRemarkParserGetNext(). + * The value pointed to by the return value needs to be disposed using a call to + * LLVMRemarkEntryDispose(). + * + * All the entries in the returned value that are of LLVMRemarkStringRef type + * will become invalidated once a call to LLVMRemarkParserDispose is made. * * If the parser reaches the end of the buffer, the return value will be `NULL`. * @@ -258,8 +272,9 @@ extern LLVMRemarkParserRef LLVMRemarkParserCreateYAML(const void *Buf, * ``` * LLVMRemarkParserRef Parser = LLVMRemarkParserCreateYAML(Buf, Size); * LLVMRemarkEntryRef Remark = NULL; - * while ((Remark == LLVMRemarkParserGetNext(Parser))) { + * while ((Remark = LLVMRemarkParserGetNext(Parser))) { * // use Remark + * LLVMRemarkEntryDispose(Remark); // Release memory. * } * bool HasError = LLVMRemarkParserHasError(Parser); * LLVMRemarkParserDispose(Parser); diff --git a/include/llvm/IR/RemarkStreamer.h b/include/llvm/IR/RemarkStreamer.h index c84de9aea35..f34cc660b2f 100644 --- a/include/llvm/IR/RemarkStreamer.h +++ b/include/llvm/IR/RemarkStreamer.h @@ -32,15 +32,9 @@ class RemarkStreamer { /// The object used to serialize the remarks to a specific format. std::unique_ptr Serializer; - /// Temporary buffer for converting diagnostics into remark objects. This is - /// used for the remark arguments that are converted from a vector of - /// diagnostic arguments to a vector of remark arguments. - SmallVector TmpArgs; - /// Convert diagnostics into remark objects. The result uses \p TmpArgs as a - /// temporary buffer for the remark arguments, and relies on all the strings - /// to be kept in memory until the next call to `toRemark`. - /// The lifetime of the members of the result is bound to the lifetime of both - /// the remark streamer and the LLVM diagnostics. + /// Convert diagnostics into remark objects. + /// The lifetime of the members of the result is bound to the lifetime of + /// the LLVM diagnostics. remarks::Remark toRemark(const DiagnosticInfoOptimizationBase &Diag); public: diff --git a/include/llvm/Remarks/Remark.h b/include/llvm/Remarks/Remark.h index 4241fb1fda3..05d0ea60acc 100644 --- a/include/llvm/Remarks/Remark.h +++ b/include/llvm/Remarks/Remark.h @@ -85,10 +85,23 @@ struct Remark { Optional Hotness; /// Arguments collected via the streaming interface. - ArrayRef Args; + SmallVector Args; + + Remark() = default; + Remark(Remark &&) = default; + Remark &operator=(Remark &&) = default; /// Return a message composed from the arguments as a string. std::string getArgsAsMsg() const; + + /// Clone this remark to explicitly ask for a copy. + Remark clone() const { return *this; } + +private: + /// In order to avoid unwanted copies, "delete" the copy constructor. + /// If a copy is needed, it should be done through `Remark::clone()`. + Remark(const Remark &) = default; + Remark& operator=(const Remark &) = default; }; // Create wrappers for C Binding types (see CBindingWrapping.h). diff --git a/include/llvm/Remarks/RemarkParser.h b/include/llvm/Remarks/RemarkParser.h index b956f0c4025..671e1abe5ec 100644 --- a/include/llvm/Remarks/RemarkParser.h +++ b/include/llvm/Remarks/RemarkParser.h @@ -26,27 +26,33 @@ namespace remarks { struct ParserImpl; struct ParsedStringTable; +class EndOfFileError : public ErrorInfo { +public: + static char ID; + + EndOfFileError() {} + + void log(raw_ostream &OS) const override { OS << "End of file reached."; } + std::error_code convertToErrorCode() const override { + return inconvertibleErrorCode(); + } +}; + /// Parser used to parse a raw buffer to remarks::Remark objects. struct Parser { - /// The hidden implementation of the parser. - std::unique_ptr Impl; + /// The format of the parser. + Format ParserFormat; - /// Create a parser parsing \p Buffer to Remark objects. - /// This constructor should be only used for parsing remarks without a string - /// table. - Parser(Format ParserFormat, StringRef Buffer); + Parser(Format ParserFormat) : ParserFormat(ParserFormat) {} - /// Create a parser parsing \p Buffer to Remark objects, using \p StrTab as a - /// string table. - Parser(Format ParserFormat, StringRef Buffer, - const ParsedStringTable &StrTab); + /// If no error occurs, this returns a valid Remark object. + /// If an error of type EndOfFileError occurs, it is safe to recover from it + /// by stopping the parsing. + /// If any other error occurs, it should be propagated to the user. + /// The pointer should never be null. + virtual Expected> next() = 0; - // Needed because ParserImpl is an incomplete type. - ~Parser(); - - /// Returns an empty Optional if it reached the end. - /// Returns a valid remark otherwise. - Expected getNext() const; + virtual ~Parser() = default; }; /// In-memory representation of the string table parsed from a buffer (e.g. the @@ -61,6 +67,10 @@ struct ParsedStringTable { ParsedStringTable(StringRef Buffer); }; +Expected> +createRemarkParser(Format ParserFormat, StringRef Buf, + Optional StrTab = None); + } // end namespace remarks } // end namespace llvm diff --git a/include/llvm/Support/SourceMgr.h b/include/llvm/Support/SourceMgr.h index 7b081d32f99..aa6026c23d0 100644 --- a/include/llvm/Support/SourceMgr.h +++ b/include/llvm/Support/SourceMgr.h @@ -106,6 +106,8 @@ public: SourceMgr() = default; SourceMgr(const SourceMgr &) = delete; SourceMgr &operator=(const SourceMgr &) = delete; + SourceMgr(SourceMgr &&) = default; + SourceMgr &operator=(SourceMgr &&) = default; ~SourceMgr() = default; void setIncludeDirs(const std::vector &Dirs) { diff --git a/lib/IR/RemarkStreamer.cpp b/lib/IR/RemarkStreamer.cpp index 32adef181f4..5b4c7e72b47 100644 --- a/lib/IR/RemarkStreamer.cpp +++ b/lib/IR/RemarkStreamer.cpp @@ -72,9 +72,6 @@ toRemarkLocation(const DiagnosticLocation &DL) { /// LLVM Diagnostic -> Remark remarks::Remark RemarkStreamer::toRemark(const DiagnosticInfoOptimizationBase &Diag) { - // Re-use the buffer. - TmpArgs.clear(); - remarks::Remark R; // The result. R.RemarkType = toRemarkType(static_cast(Diag.getKind())); R.PassName = Diag.getPassName(); @@ -84,15 +81,12 @@ RemarkStreamer::toRemark(const DiagnosticInfoOptimizationBase &Diag) { R.Loc = toRemarkLocation(Diag.getLocation()); R.Hotness = Diag.getHotness(); - // Use TmpArgs to build the list of arguments and re-use the memory allocated - // from previous remark conversions. for (const DiagnosticInfoOptimizationBase::Argument &Arg : Diag.getArgs()) { - TmpArgs.emplace_back(); - TmpArgs.back().Key = Arg.Key; - TmpArgs.back().Val = Arg.Val; - TmpArgs.back().Loc = toRemarkLocation(Arg.Loc); + R.Args.emplace_back(); + R.Args.back().Key = Arg.Key; + R.Args.back().Val = Arg.Val; + R.Args.back().Loc = toRemarkLocation(Arg.Loc); } - R.Args = TmpArgs; // This is valid until the next call to this function. return R; } diff --git a/lib/Remarks/Remark.cpp b/lib/Remarks/Remark.cpp index b4be19f47a5..401ac514b01 100644 --- a/lib/Remarks/Remark.cpp +++ b/lib/Remarks/Remark.cpp @@ -66,6 +66,10 @@ LLVMRemarkArgGetDebugLoc(LLVMRemarkArgRef Arg) { return nullptr; } +extern "C" void LLVMRemarkEntryDispose(LLVMRemarkEntryRef Remark) { + delete unwrap(Remark); +} + extern "C" LLVMRemarkType LLVMRemarkEntryGetType(LLVMRemarkEntryRef Remark) { // Assume here that the enums can be converted both ways. return static_cast(unwrap(Remark)->RemarkType); diff --git a/lib/Remarks/RemarkParser.cpp b/lib/Remarks/RemarkParser.cpp index 41ed64d022b..46130d28f72 100644 --- a/lib/Remarks/RemarkParser.cpp +++ b/lib/Remarks/RemarkParser.cpp @@ -20,69 +20,7 @@ using namespace llvm; using namespace llvm::remarks; -static std::unique_ptr formatToParserImpl(Format ParserFormat, - StringRef Buf) { - switch (ParserFormat) { - case Format::YAML: - return llvm::make_unique(Buf); - case Format::Unknown: - llvm_unreachable("Unhandled llvm::remarks::ParserFormat enum"); - return nullptr; - }; -} - -static std::unique_ptr -formatToParserImpl(Format ParserFormat, StringRef Buf, - const ParsedStringTable &StrTab) { - switch (ParserFormat) { - case Format::YAML: - return llvm::make_unique(Buf, &StrTab); - case Format::Unknown: - llvm_unreachable("Unhandled llvm::remarks::ParserFormat enum"); - return nullptr; - }; -} - -Parser::Parser(Format ParserFormat, StringRef Buf) - : Impl(formatToParserImpl(ParserFormat, Buf)) {} - -Parser::Parser(Format ParserFormat, StringRef Buf, - const ParsedStringTable &StrTab) - : Impl(formatToParserImpl(ParserFormat, Buf, StrTab)) {} - -Parser::~Parser() = default; - -static Expected getNextYAML(YAMLParserImpl &Impl) { - YAMLRemarkParser &YAMLParser = Impl.YAMLParser; - // Check for EOF. - if (Impl.YAMLIt == Impl.YAMLParser.Stream.end()) - return nullptr; - - auto CurrentIt = Impl.YAMLIt; - - // Try to parse an entry. - if (Error E = YAMLParser.parseYAMLElement(*CurrentIt)) { - // Set the iterator to the end, in case the user calls getNext again. - Impl.YAMLIt = Impl.YAMLParser.Stream.end(); - return std::move(E); - } - - // Move on. - ++Impl.YAMLIt; - - // Return the just-parsed remark. - if (const Optional &State = YAMLParser.State) - return &State->TheRemark; - else - return createStringError(std::make_error_code(std::errc::invalid_argument), - "unexpected error while parsing."); -} - -Expected Parser::getNext() const { - if (auto *Impl = dyn_cast(this->Impl.get())) - return getNextYAML(*Impl); - llvm_unreachable("Get next called with an unknown parsing implementation."); -} +char EndOfFileError::ID = 0; ParsedStringTable::ParsedStringTable(StringRef InBuffer) : Buffer(InBuffer) { while (!InBuffer.empty()) { @@ -109,59 +47,70 @@ Expected ParsedStringTable::operator[](size_t Index) const { return StringRef(Buffer.data() + Offset, NextOffset - Offset - 1); } +Expected> +llvm::remarks::createRemarkParser(Format ParserFormat, StringRef Buf, + Optional StrTab) { + switch (ParserFormat) { + case Format::YAML: + return llvm::make_unique(Buf, StrTab); + case Format::Unknown: + return createStringError(std::make_error_code(std::errc::invalid_argument), + "Unknown remark parser format."); + } +} + +// Wrapper that holds the state needed to interact with the C API. +struct CParser { + std::unique_ptr TheParser; + Optional Err; + + CParser(Format ParserFormat, StringRef Buf, + Optional StrTab = None) + : TheParser(cantFail(createRemarkParser(ParserFormat, Buf, StrTab))) {} + + void handleError(Error E) { Err.emplace(toString(std::move(E))); } + bool hasError() const { return Err.hasValue(); } + const char *getMessage() const { return Err ? Err->c_str() : nullptr; }; +}; + // Create wrappers for C Binding types (see CBindingWrapping.h). -DEFINE_SIMPLE_CONVERSION_FUNCTIONS(remarks::Parser, LLVMRemarkParserRef) +DEFINE_SIMPLE_CONVERSION_FUNCTIONS(CParser, LLVMRemarkParserRef) extern "C" LLVMRemarkParserRef LLVMRemarkParserCreateYAML(const void *Buf, uint64_t Size) { - return wrap(new remarks::Parser( - remarks::Format::YAML, StringRef(static_cast(Buf), Size))); -} - -static void handleYAMLError(remarks::YAMLParserImpl &Impl, Error E) { - handleAllErrors( - std::move(E), - [&](const YAMLParseError &PE) { - Impl.YAMLParser.Stream.printError(&PE.getNode(), - Twine(PE.getMessage()) + Twine('\n')); - }, - [&](const ErrorInfoBase &EIB) { EIB.log(Impl.YAMLParser.ErrorStream); }); - Impl.HasErrors = true; + return wrap(new CParser(Format::YAML, + StringRef(static_cast(Buf), Size))); } extern "C" LLVMRemarkEntryRef LLVMRemarkParserGetNext(LLVMRemarkParserRef Parser) { - remarks::Parser &TheParser = *unwrap(Parser); + CParser &TheCParser = *unwrap(Parser); + remarks::Parser &TheParser = *TheCParser.TheParser; - Expected RemarkOrErr = TheParser.getNext(); - if (!RemarkOrErr) { - // Error during parsing. - if (auto *Impl = dyn_cast(TheParser.Impl.get())) - handleYAMLError(*Impl, RemarkOrErr.takeError()); - else - llvm_unreachable("unkown parser implementation."); + Expected> MaybeRemark = TheParser.next(); + if (Error E = MaybeRemark.takeError()) { + if (E.isA()) { + consumeError(std::move(E)); + return nullptr; + } + + // Handle the error. Allow it to be checked through HasError and + // GetErrorMessage. + TheCParser.handleError(std::move(E)); return nullptr; } - if (*RemarkOrErr == nullptr) - return nullptr; // Valid remark. - return wrap(*RemarkOrErr); + return wrap(MaybeRemark->release()); } extern "C" LLVMBool LLVMRemarkParserHasError(LLVMRemarkParserRef Parser) { - if (auto *Impl = - dyn_cast(unwrap(Parser)->Impl.get())) - return Impl->HasErrors; - llvm_unreachable("unkown parser implementation."); + return unwrap(Parser)->hasError(); } extern "C" const char * LLVMRemarkParserGetErrorMessage(LLVMRemarkParserRef Parser) { - if (auto *Impl = - dyn_cast(unwrap(Parser)->Impl.get())) - return Impl->YAMLParser.ErrorStream.str().c_str(); - llvm_unreachable("unkown parser implementation."); + return unwrap(Parser)->getMessage(); } extern "C" void LLVMRemarkParserDispose(LLVMRemarkParserRef Parser) { diff --git a/lib/Remarks/RemarkParserImpl.h b/lib/Remarks/RemarkParserImpl.h deleted file mode 100644 index 5f8c21dcdd4..00000000000 --- a/lib/Remarks/RemarkParserImpl.h +++ /dev/null @@ -1,33 +0,0 @@ -//===-- RemarkParserImpl.h - Implementation details -------------*- C++/-*-===// -// -// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. -// See https://llvm.org/LICENSE.txt for license information. -// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -// -//===----------------------------------------------------------------------===// -// -// This file provides implementation details for the remark parser. -// -//===----------------------------------------------------------------------===// - -#ifndef LLVM_REMARKS_REMARK_PARSER_IMPL_H -#define LLVM_REMARKS_REMARK_PARSER_IMPL_H - -#include "llvm/Remarks/RemarkParser.h" - -namespace llvm { -namespace remarks { -/// This is used as a base for any parser implementation. -struct ParserImpl { - explicit ParserImpl(Format ParserFormat) : ParserFormat(ParserFormat) {} - // Virtual destructor prevents mismatched deletes - virtual ~ParserImpl() {} - - // The parser format. This is used as a tag to safely cast between - // implementations. - Format ParserFormat; -}; -} // end namespace remarks -} // end namespace llvm - -#endif /* LLVM_REMARKS_REMARK_PARSER_IMPL_H */ diff --git a/lib/Remarks/YAMLRemarkParser.cpp b/lib/Remarks/YAMLRemarkParser.cpp index c70eef556ff..ed78b7ba5d9 100644 --- a/lib/Remarks/YAMLRemarkParser.cpp +++ b/lib/Remarks/YAMLRemarkParser.cpp @@ -20,60 +20,141 @@ using namespace llvm::remarks; char YAMLParseError::ID = 0; -Error YAMLRemarkParser::parseKey(StringRef &Result, yaml::KeyValueNode &Node) { - if (auto *Key = dyn_cast(Node.getKey())) { - Result = Key->getRawValue(); +static void handleDiagnostic(const SMDiagnostic &Diag, void *Ctx) { + assert(Ctx && "Expected non-null Ctx in diagnostic handler."); + std::string &Message = *static_cast(Ctx); + assert(Message.empty() && "Expected an empty string."); + raw_string_ostream OS(Message); + Diag.print(/*ProgName=*/nullptr, OS, /*ShowColors*/ false, + /*ShowKindLabels*/ true); + OS << '\n'; + OS.flush(); +} + +YAMLParseError::YAMLParseError(StringRef Msg, SourceMgr &SM, + yaml::Stream &Stream, yaml::Node &Node) { + // 1) Set up a diagnostic handler to avoid errors being printed out to + // stderr. + // 2) Use the stream to print the error with the associated node. + // 3) The stream will use the source manager to print the error, which will + // call the diagnostic handler. + // 4) The diagnostic handler will stream the error directly into this object's + // Message member, which is used when logging is asked for. + auto OldDiagHandler = SM.getDiagHandler(); + auto OldDiagCtx = SM.getDiagContext(); + SM.setDiagHandler(handleDiagnostic, &Message); + Stream.printError(&Node, Twine(Msg) + Twine('\n')); + // Restore the old handlers. + SM.setDiagHandler(OldDiagHandler, OldDiagCtx); +} + +static SourceMgr setupSM(std::string &LastErrorMessage) { + SourceMgr SM; + SM.setDiagHandler(handleDiagnostic, &LastErrorMessage); + return SM; +} + +YAMLRemarkParser::YAMLRemarkParser(StringRef Buf, + Optional StrTab) + : Parser{Format::YAML}, StrTab(StrTab), LastErrorMessage(), + SM(setupSM(LastErrorMessage)), Stream(Buf, SM), YAMLIt(Stream.begin()) {} + +Error YAMLRemarkParser::error(StringRef Message, yaml::Node &Node) { + return make_error(Message, SM, Stream, Node); +} + +Error YAMLRemarkParser::error() { + if (LastErrorMessage.empty()) return Error::success(); + Error E = make_error(LastErrorMessage); + LastErrorMessage.clear(); + return E; +} + +Expected> +YAMLRemarkParser::parseRemark(yaml::Document &RemarkEntry) { + if (Error E = error()) + return std::move(E); + + yaml::Node *YAMLRoot = RemarkEntry.getRoot(); + if (!YAMLRoot) { + return createStringError(std::make_error_code(std::errc::invalid_argument), + "not a valid YAML file."); } - return make_error("key is not a string.", Node); -} + auto *Root = dyn_cast(YAMLRoot); + if (!Root) + return error("document root is not of mapping type.", *YAMLRoot); -template -Error YAMLRemarkParser::parseStr(T &Result, yaml::KeyValueNode &Node) { - auto *Value = dyn_cast(Node.getValue()); - if (!Value) - return make_error("expected a value of scalar type.", Node); - StringRef Tmp; - if (!StrTab) { - Tmp = Value->getRawValue(); - } else { - // If we have a string table, parse it as an unsigned. - unsigned StrID = 0; - if (Error E = parseUnsigned(StrID, Node)) - return E; - if (Expected Str = (**StrTab)[StrID]) - Tmp = *Str; - else - return Str.takeError(); + std::unique_ptr Result = llvm::make_unique(); + Remark &TheRemark = *Result; + + // First, the type. It needs special handling since is not part of the + // key-value stream. + Expected T = parseType(*Root); + if (!T) + return T.takeError(); + else + TheRemark.RemarkType = *T; + + // Then, parse the fields, one by one. + for (yaml::KeyValueNode &RemarkField : *Root) { + Expected MaybeKey = parseKey(RemarkField); + if (!MaybeKey) + return MaybeKey.takeError(); + StringRef KeyName = *MaybeKey; + + if (KeyName == "Pass") { + if (Expected MaybeStr = parseStr(RemarkField)) + TheRemark.PassName = *MaybeStr; + else + return MaybeStr.takeError(); + } else if (KeyName == "Name") { + if (Expected MaybeStr = parseStr(RemarkField)) + TheRemark.RemarkName = *MaybeStr; + else + return MaybeStr.takeError(); + } else if (KeyName == "Function") { + if (Expected MaybeStr = parseStr(RemarkField)) + TheRemark.FunctionName = *MaybeStr; + else + return MaybeStr.takeError(); + } else if (KeyName == "Hotness") { + if (Expected MaybeU = parseUnsigned(RemarkField)) + TheRemark.Hotness = *MaybeU; + else + return MaybeU.takeError(); + } else if (KeyName == "DebugLoc") { + if (Expected MaybeLoc = parseDebugLoc(RemarkField)) + TheRemark.Loc = *MaybeLoc; + else + return MaybeLoc.takeError(); + } else if (KeyName == "Args") { + auto *Args = dyn_cast(RemarkField.getValue()); + if (!Args) + return error("wrong value type for key.", RemarkField); + + for (yaml::Node &Arg : *Args) { + if (Expected MaybeArg = parseArg(Arg)) + TheRemark.Args.push_back(*MaybeArg); + else + return MaybeArg.takeError(); + } + } else { + return error("unknown key.", RemarkField); + } } - if (Tmp.front() == '\'') - Tmp = Tmp.drop_front(); + // Check if any of the mandatory fields are missing. + if (TheRemark.RemarkType == Type::Unknown || TheRemark.PassName.empty() || + TheRemark.RemarkName.empty() || TheRemark.FunctionName.empty()) + return error("Type, Pass, Name or Function missing.", + *RemarkEntry.getRoot()); - if (Tmp.back() == '\'') - Tmp = Tmp.drop_back(); - - Result = Tmp; - - return Error::success(); + return std::move(Result); } -template -Error YAMLRemarkParser::parseUnsigned(T &Result, yaml::KeyValueNode &Node) { - SmallVector Tmp; - auto *Value = dyn_cast(Node.getValue()); - if (!Value) - return make_error("expected a value of scalar type.", Node); - unsigned UnsignedValue = 0; - if (Value->getValue(Tmp).getAsInteger(10, UnsignedValue)) - return make_error("expected a value of integer type.", - *Value); - Result = UnsignedValue; - return Error::success(); -} - -Error YAMLRemarkParser::parseType(Type &Result, yaml::MappingNode &Node) { +Expected YAMLRemarkParser::parseType(yaml::MappingNode &Node) { auto Type = StringSwitch(Node.getRawTag()) .Case("!Passed", remarks::Type::Passed) .Case("!Missed", remarks::Type::Missed) @@ -83,192 +164,164 @@ Error YAMLRemarkParser::parseType(Type &Result, yaml::MappingNode &Node) { .Case("!Failure", remarks::Type::Failure) .Default(remarks::Type::Unknown); if (Type == remarks::Type::Unknown) - return make_error("expected a remark tag.", Node); - Result = Type; - return Error::success(); + return error("expected a remark tag.", Node); + return Type; } -Error YAMLRemarkParser::parseDebugLoc(Optional &Result, - yaml::KeyValueNode &Node) { +Expected YAMLRemarkParser::parseKey(yaml::KeyValueNode &Node) { + if (auto *Key = dyn_cast(Node.getKey())) + return Key->getRawValue(); + + return error("key is not a string.", Node); +} + +Expected YAMLRemarkParser::parseStr(yaml::KeyValueNode &Node) { + auto *Value = dyn_cast(Node.getValue()); + if (!Value) + return error("expected a value of scalar type.", Node); + StringRef Result; + if (!StrTab) { + Result = Value->getRawValue(); + } else { + // If we have a string table, parse it as an unsigned. + unsigned StrID = 0; + if (Expected MaybeStrID = parseUnsigned(Node)) + StrID = *MaybeStrID; + else + return MaybeStrID.takeError(); + + if (Expected Str = (**StrTab)[StrID]) + Result = *Str; + else + return Str.takeError(); + } + + if (Result.front() == '\'') + Result = Result.drop_front(); + + if (Result.back() == '\'') + Result = Result.drop_back(); + + return Result; +} + +Expected YAMLRemarkParser::parseUnsigned(yaml::KeyValueNode &Node) { + SmallVector Tmp; + auto *Value = dyn_cast(Node.getValue()); + if (!Value) + return error("expected a value of scalar type.", Node); + unsigned UnsignedValue = 0; + if (Value->getValue(Tmp).getAsInteger(10, UnsignedValue)) + return error("expected a value of integer type.", *Value); + return UnsignedValue; +} + +Expected +YAMLRemarkParser::parseDebugLoc(yaml::KeyValueNode &Node) { auto *DebugLoc = dyn_cast(Node.getValue()); if (!DebugLoc) - return make_error("expected a value of mapping type.", - Node); + return error("expected a value of mapping type.", Node); Optional File; Optional Line; Optional Column; for (yaml::KeyValueNode &DLNode : *DebugLoc) { - StringRef KeyName; - if (Error E = parseKey(KeyName, DLNode)) - return E; + Expected MaybeKey = parseKey(DLNode); + if (!MaybeKey) + return MaybeKey.takeError(); + StringRef KeyName = *MaybeKey; + if (KeyName == "File") { - if (Error E = parseStr(File, DLNode)) - return E; + if (Expected MaybeStr = parseStr(DLNode)) + File = *MaybeStr; + else + return MaybeStr.takeError(); } else if (KeyName == "Column") { - if (Error E = parseUnsigned(Column, DLNode)) - return E; + if (Expected MaybeU = parseUnsigned(DLNode)) + Column = *MaybeU; + else + return MaybeU.takeError(); } else if (KeyName == "Line") { - if (Error E = parseUnsigned(Line, DLNode)) - return E; + if (Expected MaybeU = parseUnsigned(DLNode)) + Line = *MaybeU; + else + return MaybeU.takeError(); } else { - return make_error("unknown entry in DebugLoc map.", - DLNode); + return error("unknown entry in DebugLoc map.", DLNode); } } // If any of the debug loc fields is missing, return an error. if (!File || !Line || !Column) - return make_error("DebugLoc node incomplete.", Node); + return error("DebugLoc node incomplete.", Node); - Result = RemarkLocation{*File, *Line, *Column}; - - return Error::success(); + return RemarkLocation{*File, *Line, *Column}; } -Error YAMLRemarkParser::parseRemarkField(yaml::KeyValueNode &RemarkField) { - - StringRef KeyName; - if (Error E = parseKey(KeyName, RemarkField)) - return E; - - if (KeyName == "Pass") { - if (Error E = parseStr(State->TheRemark.PassName, RemarkField)) - return E; - } else if (KeyName == "Name") { - if (Error E = parseStr(State->TheRemark.RemarkName, RemarkField)) - return E; - } else if (KeyName == "Function") { - if (Error E = parseStr(State->TheRemark.FunctionName, RemarkField)) - return E; - } else if (KeyName == "Hotness") { - State->TheRemark.Hotness = 0; - if (Error E = parseUnsigned(*State->TheRemark.Hotness, RemarkField)) - return E; - } else if (KeyName == "DebugLoc") { - if (Error E = parseDebugLoc(State->TheRemark.Loc, RemarkField)) - return E; - } else if (KeyName == "Args") { - auto *Args = dyn_cast(RemarkField.getValue()); - if (!Args) - return make_error("wrong value type for key.", - RemarkField); - - for (yaml::Node &Arg : *Args) - if (Error E = parseArg(State->Args, Arg)) - return E; - - State->TheRemark.Args = State->Args; - } else { - return make_error("unknown key.", RemarkField); - } - - return Error::success(); -} - -Error YAMLRemarkParser::parseArg(SmallVectorImpl &Args, - yaml::Node &Node) { +Expected YAMLRemarkParser::parseArg(yaml::Node &Node) { auto *ArgMap = dyn_cast(&Node); if (!ArgMap) - return make_error("expected a value of mapping type.", - Node); + return error("expected a value of mapping type.", Node); - StringRef KeyStr; - StringRef ValueStr; + Optional KeyStr; + Optional ValueStr; Optional Loc; - for (yaml::KeyValueNode &ArgEntry : *ArgMap) - if (Error E = parseArgEntry(ArgEntry, KeyStr, ValueStr, Loc)) - return E; + for (yaml::KeyValueNode &ArgEntry : *ArgMap) { + Expected MaybeKey = parseKey(ArgEntry); + if (!MaybeKey) + return MaybeKey.takeError(); + StringRef KeyName = *MaybeKey; - if (KeyStr.empty()) - return make_error("argument key is missing.", *ArgMap); - if (ValueStr.empty()) - return make_error("argument value is missing.", *ArgMap); + // Try to parse debug locs. + if (KeyName == "DebugLoc") { + // Can't have multiple DebugLoc entries per argument. + if (Loc) + return error("only one DebugLoc entry is allowed per argument.", + ArgEntry); - Args.push_back(Argument{KeyStr, ValueStr, Loc}); + if (Expected MaybeLoc = parseDebugLoc(ArgEntry)) { + Loc = *MaybeLoc; + continue; + } else + return MaybeLoc.takeError(); + } - return Error::success(); -} + // If we already have a string, error out. + if (ValueStr) + return error("only one string entry is allowed per argument.", ArgEntry); -Error YAMLRemarkParser::parseArgEntry(yaml::KeyValueNode &ArgEntry, - StringRef &KeyStr, StringRef &ValueStr, - Optional &Loc) { - StringRef KeyName; - if (Error E = parseKey(KeyName, ArgEntry)) - return E; + // Try to parse the value. + if (Expected MaybeStr = parseStr(ArgEntry)) + ValueStr = *MaybeStr; + else + return MaybeStr.takeError(); - // Try to parse debug locs. - if (KeyName == "DebugLoc") { - // Can't have multiple DebugLoc entries per argument. - if (Loc) - return make_error( - "only one DebugLoc entry is allowed per argument.", ArgEntry); - - if (Error E = parseDebugLoc(Loc, ArgEntry)) - return E; - return Error::success(); + // Keep the key from the string. + KeyStr = KeyName; } - // If we already have a string, error out. - if (!ValueStr.empty()) - return make_error( - "only one string entry is allowed per argument.", ArgEntry); + if (!KeyStr) + return error("argument key is missing.", *ArgMap); + if (!ValueStr) + return error("argument value is missing.", *ArgMap); - // Try to parse a string. - if (Error E = parseStr(ValueStr, ArgEntry)) - return E; - - // Keep the key from the string. - KeyStr = KeyName; - return Error::success(); + return Argument{*KeyStr, *ValueStr, Loc}; } -Error YAMLRemarkParser::parseYAMLElement(yaml::Document &Remark) { - // Parsing a new remark, clear the previous one by re-constructing the state - // in-place in the Optional. - State.emplace(TmpArgs); +Expected> YAMLRemarkParser::next() { + if (YAMLIt == Stream.end()) + return make_error(); - yaml::Node *YAMLRoot = Remark.getRoot(); - if (!YAMLRoot) - return createStringError(std::make_error_code(std::errc::invalid_argument), - "not a valid YAML file."); + Expected> MaybeResult = parseRemark(*YAMLIt); + if (!MaybeResult) { + // Avoid garbage input, set the iterator to the end. + YAMLIt = Stream.end(); + return MaybeResult.takeError(); + } - auto *Root = dyn_cast(YAMLRoot); - if (!Root) - return make_error("document root is not of mapping type.", - *YAMLRoot); + ++YAMLIt; - if (Error E = parseType(State->TheRemark.RemarkType, *Root)) - return E; - - for (yaml::KeyValueNode &RemarkField : *Root) - if (Error E = parseRemarkField(RemarkField)) - return E; - - // If the YAML parsing failed, don't even continue parsing. We might - // encounter malformed YAML. - if (Stream.failed()) - return make_error("YAML parsing failed.", - *Remark.getRoot()); - - // Check if any of the mandatory fields are missing. - if (State->TheRemark.RemarkType == Type::Unknown || - State->TheRemark.PassName.empty() || - State->TheRemark.RemarkName.empty() || - State->TheRemark.FunctionName.empty()) - return make_error("Type, Pass, Name or Function missing.", - *Remark.getRoot()); - - return Error::success(); -} - -/// Handle a diagnostic from the YAML stream. Records the error in the -/// YAMLRemarkParser class. -void YAMLRemarkParser::HandleDiagnostic(const SMDiagnostic &Diag, void *Ctx) { - assert(Ctx && "Expected non-null Ctx in diagnostic handler."); - auto *Parser = static_cast(Ctx); - Diag.print(/*ProgName=*/nullptr, Parser->ErrorStream, /*ShowColors*/ false, - /*ShowKindLabels*/ true); + return std::move(*MaybeResult); } diff --git a/lib/Remarks/YAMLRemarkParser.h b/lib/Remarks/YAMLRemarkParser.h index 14698bbd3ca..cea76e63e75 100644 --- a/lib/Remarks/YAMLRemarkParser.h +++ b/lib/Remarks/YAMLRemarkParser.h @@ -13,7 +13,6 @@ #ifndef LLVM_REMARKS_YAML_REMARK_PARSER_H #define LLVM_REMARKS_YAML_REMARK_PARSER_H -#include "RemarkParserImpl.h" #include "llvm/ADT/Optional.h" #include "llvm/ADT/SmallVector.h" #include "llvm/Remarks/Remark.h" @@ -27,112 +26,69 @@ namespace llvm { namespace remarks { -/// Parses and holds the state of the latest parsed remark. -struct YAMLRemarkParser { - /// Source manager for better error messages. - SourceMgr SM; - /// Stream for yaml parsing. - yaml::Stream Stream; - /// Storage for the error stream. - std::string ErrorString; - /// The error stream. - raw_string_ostream ErrorStream; - /// Temporary parsing buffer for the arguments. - SmallVector TmpArgs; - /// The string table used for parsing strings. - Optional StrTab; - /// The state used by the parser to parse a remark entry. Invalidated with - /// every call to `parseYAMLElement`. - struct ParseState { - /// Temporary parsing buffer for the arguments. - /// The parser itself is owning this buffer in order to reduce the number of - /// allocations. - SmallVectorImpl &Args; - Remark TheRemark; - - ParseState(SmallVectorImpl &Args) : Args(Args) {} - /// Use Args only as a **temporary** buffer. - ~ParseState() { Args.clear(); } - }; - - /// The current state of the parser. If the parsing didn't start yet, it will - /// not be containing any value. - Optional State; - - YAMLRemarkParser(StringRef Buf, - Optional StrTab = None) - : SM(), Stream(Buf, SM), ErrorString(), ErrorStream(ErrorString), - TmpArgs(), StrTab(StrTab) { - SM.setDiagHandler(YAMLRemarkParser::HandleDiagnostic, this); - } - - /// Parse a YAML element. - Error parseYAMLElement(yaml::Document &Remark); - -private: - /// Parse one key to a string. - /// otherwise. - Error parseKey(StringRef &Result, yaml::KeyValueNode &Node); - /// Parse one value to a string. - template Error parseStr(T &Result, yaml::KeyValueNode &Node); - /// Parse one value to an unsigned. - template - Error parseUnsigned(T &Result, yaml::KeyValueNode &Node); - /// Parse the type of a remark to an enum type. - Error parseType(Type &Result, yaml::MappingNode &Node); - /// Parse a debug location. - Error parseDebugLoc(Optional &Result, - yaml::KeyValueNode &Node); - /// Parse a remark field and update the parsing state. - Error parseRemarkField(yaml::KeyValueNode &RemarkField); - /// Parse an argument. - Error parseArg(SmallVectorImpl &TmpArgs, yaml::Node &Node); - /// Parse an entry from the contents of an argument. - Error parseArgEntry(yaml::KeyValueNode &ArgEntry, StringRef &KeyStr, - StringRef &ValueStr, Optional &Loc); - - /// Handle a diagnostic from the YAML stream. Records the error in the - /// YAMLRemarkParser class. - static void HandleDiagnostic(const SMDiagnostic &Diag, void *Ctx); -}; class YAMLParseError : public ErrorInfo { public: static char ID; - YAMLParseError(StringRef Message, yaml::Node &Node) - : Message(Message), Node(Node) {} + YAMLParseError(StringRef Message, SourceMgr &SM, yaml::Stream &Stream, + yaml::Node &Node); + + YAMLParseError(StringRef Message) : Message(Message) {} void log(raw_ostream &OS) const override { OS << Message; } std::error_code convertToErrorCode() const override { return inconvertibleErrorCode(); } - StringRef getMessage() const { return Message; } - yaml::Node &getNode() const { return Node; } - private: - StringRef Message; // No need to hold a full copy of the buffer. - yaml::Node &Node; + std::string Message; }; /// Regular YAML to Remark parser. -struct YAMLParserImpl : public ParserImpl { - /// The object parsing the YAML. - YAMLRemarkParser YAMLParser; +struct YAMLRemarkParser : public Parser { + /// The string table used for parsing strings. + Optional StrTab; + /// Last error message that can come from the YAML parser diagnostics. + /// We need this for catching errors in the constructor. + std::string LastErrorMessage; + /// Source manager for better error messages. + SourceMgr SM; + /// Stream for yaml parsing. + yaml::Stream Stream; /// Iterator in the YAML stream. yaml::document_iterator YAMLIt; - /// Set to `true` if we had any errors during parsing. - bool HasErrors = false; - YAMLParserImpl(StringRef Buf, - Optional StrTab = None) - : ParserImpl{Format::YAML}, YAMLParser(Buf, StrTab), - YAMLIt(YAMLParser.Stream.begin()), HasErrors(false) {} + YAMLRemarkParser(StringRef Buf, + Optional StrTab = None); - static bool classof(const ParserImpl *PI) { - return PI->ParserFormat == Format::YAML; + Expected> next() override; + + static bool classof(const Parser *P) { + return P->ParserFormat == Format::YAML; } + +private: + /// Create a YAMLParseError error from an existing error generated by the YAML + /// parser. + /// If there is no error, this returns Success. + Error error(); + /// Create a YAMLParseError error referencing a specific node. + Error error(StringRef Message, yaml::Node &Node); + /// Parse a YAML remark to a remarks::Remark object. + Expected> parseRemark(yaml::Document &Remark); + /// Parse the type of a remark to an enum type. + Expected parseType(yaml::MappingNode &Node); + /// Parse one key to a string. + Expected parseKey(yaml::KeyValueNode &Node); + /// Parse one value to a string. + Expected parseStr(yaml::KeyValueNode &Node); + /// Parse one value to an unsigned. + Expected parseUnsigned(yaml::KeyValueNode &Node); + /// Parse a debug location. + Expected parseDebugLoc(yaml::KeyValueNode &Node); + /// Parse an argument. + Expected parseArg(yaml::Node &Node); }; } // end namespace remarks } // end namespace llvm diff --git a/tools/llvm-opt-report/OptReport.cpp b/tools/llvm-opt-report/OptReport.cpp index 80d0b73664d..5662c9fbd7b 100644 --- a/tools/llvm-opt-report/OptReport.cpp +++ b/tools/llvm-opt-report/OptReport.cpp @@ -150,20 +150,32 @@ static bool readLocationInfo(LocationInfoTy &LocationInfo) { return false; } - remarks::Parser Parser(remarks::Format::YAML, (*Buf)->getBuffer()); + Expected> MaybeParser = + remarks::createRemarkParser(remarks::Format::YAML, (*Buf)->getBuffer()); + if (!MaybeParser) { + handleAllErrors(MaybeParser.takeError(), [&](const ErrorInfoBase &PE) { + PE.log(WithColor::error()); + }); + return false; + } + remarks::Parser &Parser = **MaybeParser; while (true) { - Expected RemarkOrErr = Parser.getNext(); - if (!RemarkOrErr) { - handleAllErrors(RemarkOrErr.takeError(), [&](const ErrorInfoBase &PE) { + Expected> MaybeRemark = Parser.next(); + if (!MaybeRemark) { + Error E = MaybeRemark.takeError(); + if (E.isA()) { + // EOF. + consumeError(std::move(E)); + break; + } + handleAllErrors(MaybeRemark.takeError(), [&](const ErrorInfoBase &PE) { PE.log(WithColor::error()); }); return false; } - if (!*RemarkOrErr) // End of file. - break; - const remarks::Remark &Remark = **RemarkOrErr; + const remarks::Remark &Remark = **MaybeRemark; bool Transformed = Remark.RemarkType == remarks::Type::Passed; diff --git a/tools/remarks-shlib/Remarks.exports b/tools/remarks-shlib/Remarks.exports index 7260f9a543d..9ec1e73a471 100644 --- a/tools/remarks-shlib/Remarks.exports +++ b/tools/remarks-shlib/Remarks.exports @@ -6,6 +6,7 @@ LLVMRemarkDebugLocGetSourceColumn LLVMRemarkArgGetKey LLVMRemarkArgGetValue LLVMRemarkArgGetDebugLoc +LLVMRemarkEntryDispose LLVMRemarkEntryGetType LLVMRemarkEntryGetPassName LLVMRemarkEntryGetRemarkName diff --git a/unittests/Remarks/YAMLRemarksParsingTest.cpp b/unittests/Remarks/YAMLRemarksParsingTest.cpp index e3c7cdf881e..8b79dfd814f 100644 --- a/unittests/Remarks/YAMLRemarksParsingTest.cpp +++ b/unittests/Remarks/YAMLRemarksParsingTest.cpp @@ -14,20 +14,31 @@ using namespace llvm; template void parseGood(const char (&Buf)[N]) { - remarks::Parser Parser(remarks::Format::YAML, {Buf, N - 1}); - Expected Remark = Parser.getNext(); + Expected> MaybeParser = + remarks::createRemarkParser(remarks::Format::YAML, {Buf, N - 1}); + EXPECT_FALSE(errorToBool(MaybeParser.takeError())); + EXPECT_TRUE(*MaybeParser != nullptr); + + remarks::Parser &Parser = **MaybeParser; + Expected> Remark = Parser.next(); EXPECT_FALSE(errorToBool(Remark.takeError())); // Check for parsing errors. EXPECT_TRUE(*Remark != nullptr); // At least one remark. - Remark = Parser.getNext(); - EXPECT_FALSE(errorToBool(Remark.takeError())); // Check for parsing errors. - EXPECT_TRUE(*Remark == nullptr); // Check that there are no more remarks. + Remark = Parser.next(); + Error E = Remark.takeError(); + EXPECT_TRUE(E.isA()); + EXPECT_TRUE(errorToBool(std::move(E))); // Check for parsing errors. } template bool parseExpectError(const char (&Buf)[N], const char *Error) { - remarks::Parser Parser(remarks::Format::YAML, {Buf, N - 1}); - Expected Remark = Parser.getNext(); - EXPECT_FALSE(Remark); // Expect an error here. + Expected> MaybeParser = + remarks::createRemarkParser(remarks::Format::YAML, {Buf, N - 1}); + EXPECT_FALSE(errorToBool(MaybeParser.takeError())); + EXPECT_TRUE(*MaybeParser != nullptr); + + remarks::Parser &Parser = **MaybeParser; + Expected> Remark = Parser.next(); + EXPECT_FALSE(Remark); // Check for parsing errors. std::string ErrorStr; raw_string_ostream Stream(ErrorStr); @@ -42,7 +53,7 @@ TEST(YAMLRemarks, ParsingEmpty) { TEST(YAMLRemarks, ParsingNotYAML) { EXPECT_TRUE( - parseExpectError("\x01\x02\x03\x04\x05\x06", "not a valid YAML file.")); + parseExpectError("\x01\x02\x03\x04\x05\x06", "Got empty plain scalar")); } TEST(YAMLRemarks, ParsingGood) { @@ -309,17 +320,6 @@ TEST(YAMLRemarks, ParsingWrongArgs) { "", "only one string entry is allowed per argument.")); // No arg value. - EXPECT_TRUE(parseExpectError("\n" - "--- !Missed\n" - "Pass: inline\n" - "Name: NoDefinition\n" - "Function: foo\n" - "Args:\n" - " - Callee: ''\n" - " - DebugLoc: { File: a, Line: 1, Column: 2 }\n" - "", - "argument value is missing.")); - // No arg value. EXPECT_TRUE(parseExpectError("\n" "--- !Missed\n" "Pass: inline\n" @@ -354,12 +354,18 @@ TEST(YAMLRemarks, Contents) { " - String: ' because its definition is unavailable'\n" "\n"; - remarks::Parser Parser(remarks::Format::YAML, Buf); - Expected RemarkOrErr = Parser.getNext(); - EXPECT_FALSE(errorToBool(RemarkOrErr.takeError())); - EXPECT_TRUE(*RemarkOrErr != nullptr); + Expected> MaybeParser = + remarks::createRemarkParser(remarks::Format::YAML, Buf); + EXPECT_FALSE(errorToBool(MaybeParser.takeError())); + EXPECT_TRUE(*MaybeParser != nullptr); - const remarks::Remark &Remark = **RemarkOrErr; + remarks::Parser &Parser = **MaybeParser; + Expected> MaybeRemark = Parser.next(); + EXPECT_FALSE( + errorToBool(MaybeRemark.takeError())); // Check for parsing errors. + EXPECT_TRUE(*MaybeRemark != nullptr); // At least one remark. + + const remarks::Remark &Remark = **MaybeRemark; EXPECT_EQ(Remark.RemarkType, remarks::Type::Missed); EXPECT_EQ(checkStr(Remark.PassName, 6), "inline"); EXPECT_EQ(checkStr(Remark.RemarkName, 12), "NoDefinition"); @@ -408,9 +414,10 @@ TEST(YAMLRemarks, Contents) { ++ArgID; } - RemarkOrErr = Parser.getNext(); - EXPECT_FALSE(errorToBool(RemarkOrErr.takeError())); - EXPECT_EQ(*RemarkOrErr, nullptr); + MaybeRemark = Parser.next(); + Error E = MaybeRemark.takeError(); + EXPECT_TRUE(E.isA()); + EXPECT_TRUE(errorToBool(std::move(E))); // Check for parsing errors. } static inline StringRef checkStr(LLVMRemarkStringRef Str, @@ -487,6 +494,8 @@ TEST(YAMLRemarks, ContentsCAPI) { ++ArgID; } while ((Arg = LLVMRemarkEntryGetNextArg(Arg, Remark))); + LLVMRemarkEntryDispose(Remark); + EXPECT_EQ(LLVMRemarkParserGetNext(Parser), nullptr); EXPECT_FALSE(LLVMRemarkParserHasError(Parser)); @@ -516,12 +525,18 @@ TEST(YAMLRemarks, ContentsStrTab) { 115); remarks::ParsedStringTable StrTab(StrTabBuf); - remarks::Parser Parser(remarks::Format::YAML, Buf, StrTab); - Expected RemarkOrErr = Parser.getNext(); - EXPECT_FALSE(errorToBool(RemarkOrErr.takeError())); - EXPECT_TRUE(*RemarkOrErr != nullptr); + Expected> MaybeParser = + remarks::createRemarkParser(remarks::Format::YAML, Buf, &StrTab); + EXPECT_FALSE(errorToBool(MaybeParser.takeError())); + EXPECT_TRUE(*MaybeParser != nullptr); - const remarks::Remark &Remark = **RemarkOrErr; + remarks::Parser &Parser = **MaybeParser; + Expected> MaybeRemark = Parser.next(); + EXPECT_FALSE( + errorToBool(MaybeRemark.takeError())); // Check for parsing errors. + EXPECT_TRUE(*MaybeRemark != nullptr); // At least one remark. + + const remarks::Remark &Remark = **MaybeRemark; EXPECT_EQ(Remark.RemarkType, remarks::Type::Missed); EXPECT_EQ(checkStr(Remark.PassName, 6), "inline"); EXPECT_EQ(checkStr(Remark.RemarkName, 12), "NoDefinition"); @@ -570,9 +585,10 @@ TEST(YAMLRemarks, ContentsStrTab) { ++ArgID; } - RemarkOrErr = Parser.getNext(); - EXPECT_FALSE(errorToBool(RemarkOrErr.takeError())); - EXPECT_EQ(*RemarkOrErr, nullptr); + MaybeRemark = Parser.next(); + Error E = MaybeRemark.takeError(); + EXPECT_TRUE(E.isA()); + EXPECT_TRUE(errorToBool(std::move(E))); // Check for parsing errors. } TEST(YAMLRemarks, ParsingBadStringTableIndex) { @@ -584,13 +600,18 @@ TEST(YAMLRemarks, ParsingBadStringTableIndex) { StringRef StrTabBuf = StringRef("inline"); remarks::ParsedStringTable StrTab(StrTabBuf); - remarks::Parser Parser(remarks::Format::YAML, Buf, StrTab); - Expected Remark = Parser.getNext(); - EXPECT_FALSE(Remark); // Expect an error here. + Expected> MaybeParser = + remarks::createRemarkParser(remarks::Format::YAML, Buf, &StrTab); + EXPECT_FALSE(errorToBool(MaybeParser.takeError())); + EXPECT_TRUE(*MaybeParser != nullptr); + + remarks::Parser &Parser = **MaybeParser; + Expected> MaybeRemark = Parser.next(); + EXPECT_FALSE(MaybeRemark); // Expect an error here. std::string ErrorStr; raw_string_ostream Stream(ErrorStr); - handleAllErrors(Remark.takeError(), + handleAllErrors(MaybeRemark.takeError(), [&](const ErrorInfoBase &EIB) { EIB.log(Stream); }); EXPECT_TRUE( StringRef(Stream.str())