diff --git a/docs/Remarks.rst b/docs/Remarks.rst index 56465476d01..c573eb2dec5 100644 --- a/docs/Remarks.rst +++ b/docs/Remarks.rst @@ -218,6 +218,28 @@ support this format. .. _optviewer: +YAML metadata +------------- + +The metadata used together with the YAML format is: + +* a magic number: "REMARKS\\0" +* the version number: a little-endian uint64_t +* the total size of the string table (the size itself excluded): + little-endian uint64_t +* a list of null-terminated strings + +Optional: + +* the absolute file path to the serialized remark diagnostics: a + null-terminated string. + +When the metadata is serialized separately from the remarks, the file path +should be present and point to the file where the remarks are serialized to. + +In case the metadata only acts as a header to the remarks, the file path can be +omitted. + opt-viewer ========== @@ -295,15 +317,8 @@ Emitting remark diagnostics in the object file ============================================== A section containing metadata on remark diagnostics will be emitted when --remarks-section is passed. The section contains: - -* a magic number: "REMARKS\\0" -* the version number: a little-endian uint64_t -* the total size of the string table (the size itself excluded): - little-endian uint64_t -* a list of null-terminated strings -* the absolute file path to the serialized remark diagnostics: a - null-terminated string. +-remarks-section is passed. The section contains the metadata associated to the +format used to serialize the remarks. The section is named: diff --git a/include/llvm/Remarks/RemarkParser.h b/include/llvm/Remarks/RemarkParser.h index f73671214f0..551d20a8a0a 100644 --- a/include/llvm/Remarks/RemarkParser.h +++ b/include/llvm/Remarks/RemarkParser.h @@ -80,6 +80,10 @@ Expected> createRemarkParser(Format ParserFormat, StringRef Buf, ParsedStringTable StrTab); +Expected> +createRemarkParserFromMeta(Format ParserFormat, StringRef Buf, + Optional StrTab = None); + } // end namespace remarks } // end namespace llvm diff --git a/lib/Remarks/RemarkParser.cpp b/lib/Remarks/RemarkParser.cpp index e7f174ce02c..1ff9cf626b6 100644 --- a/lib/Remarks/RemarkParser.cpp +++ b/lib/Remarks/RemarkParser.cpp @@ -80,6 +80,21 @@ llvm::remarks::createRemarkParser(Format ParserFormat, StringRef Buf, llvm_unreachable("unhandled ParseFormat"); } +Expected> +llvm::remarks::createRemarkParserFromMeta(Format ParserFormat, StringRef Buf, + Optional StrTab) { + switch (ParserFormat) { + // Depending on the metadata, the format can be either yaml or yaml-strtab, + // regardless of the input argument. + case Format::YAML: + case Format::YAMLStrTab: + return createYAMLParserFromMeta(Buf, std::move(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; diff --git a/lib/Remarks/YAMLRemarkParser.cpp b/lib/Remarks/YAMLRemarkParser.cpp index f677bfc04ce..3402dac1c99 100644 --- a/lib/Remarks/YAMLRemarkParser.cpp +++ b/lib/Remarks/YAMLRemarkParser.cpp @@ -14,6 +14,7 @@ #include "YAMLRemarkParser.h" #include "llvm/ADT/StringSwitch.h" #include "llvm/Remarks/RemarkParser.h" +#include "llvm/Support/Endian.h" using namespace llvm; using namespace llvm::remarks; @@ -54,6 +55,110 @@ static SourceMgr setupSM(std::string &LastErrorMessage) { return SM; } +// Parse the magic number. This function returns true if this represents remark +// metadata, false otherwise. +static Expected parseMagic(StringRef &Buf) { + if (!Buf.consume_front(remarks::Magic)) + return false; + + if (Buf.size() < 1 || !Buf.consume_front(StringRef("\0", 1))) + return createStringError(std::errc::illegal_byte_sequence, + "Expecting \\0 after magic number."); + return true; +} + +static Expected parseVersion(StringRef &Buf) { + if (Buf.size() < sizeof(uint64_t)) + return createStringError(std::errc::illegal_byte_sequence, + "Expecting version number."); + + uint64_t Version = + support::endian::read( + Buf.data()); + if (Version != remarks::Version) + return createStringError( + std::errc::illegal_byte_sequence, + "Mismatching remark version. Got %u, expected %u.", Version, + remarks::Version); + Buf = Buf.drop_front(sizeof(uint64_t)); + return Version; +} + +static Expected parseStrTabSize(StringRef &Buf) { + if (Buf.size() < sizeof(uint64_t)) + return createStringError(std::errc::illegal_byte_sequence, + "Expecting string table size."); + uint64_t StrTabSize = + support::endian::read( + Buf.data()); + Buf = Buf.drop_front(sizeof(uint64_t)); + return StrTabSize; +} + +static Expected parseStrTab(StringRef &Buf, + uint64_t StrTabSize) { + if (Buf.size() < StrTabSize) + return createStringError(std::errc::illegal_byte_sequence, + "Expecting string table."); + + // Attach the string table to the parser. + ParsedStringTable Result(StringRef(Buf.data(), StrTabSize)); + Buf = Buf.drop_front(StrTabSize); + return Expected(std::move(Result)); +} + +Expected> +remarks::createYAMLParserFromMeta(StringRef Buf, + Optional StrTab) { + // We now have a magic number. The metadata has to be correct. + Expected isMeta = parseMagic(Buf); + if (!isMeta) + return isMeta.takeError(); + // If it's not recognized as metadata, roll back. + std::unique_ptr SeparateBuf; + if (*isMeta) { + Expected Version = parseVersion(Buf); + if (!Version) + return Version.takeError(); + + Expected StrTabSize = parseStrTabSize(Buf); + if (!StrTabSize) + return StrTabSize.takeError(); + + // If the size of string table is not 0, try to build one. + if (*StrTabSize != 0) { + if (StrTab) + return createStringError(std::errc::illegal_byte_sequence, + "String table already provided."); + Expected MaybeStrTab = parseStrTab(Buf, *StrTabSize); + if (!MaybeStrTab) + return MaybeStrTab.takeError(); + StrTab = std::move(*MaybeStrTab); + } + // If it starts with "---", there is no external file. + if (!Buf.startswith("---")) { + // At this point, we expect Buf to contain the external file path. + // Try to open the file and start parsing from there. + ErrorOr> BufferOrErr = + MemoryBuffer::getFile(Buf); + if (std::error_code EC = BufferOrErr.getError()) + return errorCodeToError(EC); + + // Keep the buffer alive. + SeparateBuf = std::move(*BufferOrErr); + Buf = SeparateBuf->getBuffer(); + } + } + + std::unique_ptr Result = + StrTab + ? llvm::make_unique(Buf, std::move(*StrTab)) + : llvm::make_unique(Buf); + if (SeparateBuf) + Result->SeparateBuf = std::move(SeparateBuf); + return std::move(Result); +} + YAMLRemarkParser::YAMLRemarkParser(StringRef Buf) : YAMLRemarkParser(Buf, None) {} diff --git a/lib/Remarks/YAMLRemarkParser.h b/lib/Remarks/YAMLRemarkParser.h index e8f0edc50f4..ff03abbb576 100644 --- a/lib/Remarks/YAMLRemarkParser.h +++ b/lib/Remarks/YAMLRemarkParser.h @@ -18,6 +18,7 @@ #include "llvm/Remarks/Remark.h" #include "llvm/Remarks/RemarkParser.h" #include "llvm/Support/Error.h" +#include "llvm/Support/MemoryBuffer.h" #include "llvm/Support/SourceMgr.h" #include "llvm/Support/YAMLParser.h" #include "llvm/Support/YAMLTraits.h" @@ -58,6 +59,9 @@ struct YAMLRemarkParser : public RemarkParser { yaml::Stream Stream; /// Iterator in the YAML stream. yaml::document_iterator YAMLIt; + /// If we parse remark metadata in separate mode, we need to open a new file + /// and parse that. + std::unique_ptr SeparateBuf; YAMLRemarkParser(StringRef Buf); @@ -104,6 +108,11 @@ protected: /// Parse one value to a string. Expected parseStr(yaml::KeyValueNode &Node) override; }; + +Expected> +createYAMLParserFromMeta(StringRef Buf, + Optional StrTab = None); + } // end namespace remarks } // end namespace llvm diff --git a/unittests/Remarks/YAMLRemarksParsingTest.cpp b/unittests/Remarks/YAMLRemarksParsingTest.cpp index 967ac965069..eb5f24ecb98 100644 --- a/unittests/Remarks/YAMLRemarksParsingTest.cpp +++ b/unittests/Remarks/YAMLRemarksParsingTest.cpp @@ -29,6 +29,22 @@ template void parseGood(const char (&Buf)[N]) { EXPECT_TRUE(errorToBool(std::move(E))); // Check for parsing errors. } +void parseGoodMeta(StringRef Buf) { + Expected> MaybeParser = + remarks::createRemarkParserFromMeta(remarks::Format::YAML, Buf); + EXPECT_FALSE(errorToBool(MaybeParser.takeError())); + EXPECT_TRUE(*MaybeParser != nullptr); + + remarks::RemarkParser &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.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) { Expected> MaybeParser = @@ -47,6 +63,17 @@ bool parseExpectError(const char (&Buf)[N], const char *Error) { return StringRef(Stream.str()).contains(Error); } +void parseExpectErrorMeta(StringRef Buf, const char *Error) { + std::string ErrorStr; + raw_string_ostream Stream(ErrorStr); + + Expected> MaybeParser = + remarks::createRemarkParserFromMeta(remarks::Format::YAML, Buf); + handleAllErrors(MaybeParser.takeError(), + [&](const ErrorInfoBase &EIB) { EIB.log(Stream); }); + EXPECT_EQ(Stream.str(), Error); +} + TEST(YAMLRemarks, ParsingEmpty) { EXPECT_TRUE(parseExpectError("\n\n", "document root is not of mapping type.")); } @@ -619,3 +646,62 @@ TEST(YAMLRemarks, ParsingBadStringTableIndex) { StringRef(Stream.str()) .contains("String with index 50 is out of bounds (size = 1).")); } + +TEST(YAMLRemarks, ParsingGoodMeta) { + // No metadata should also work. + parseGoodMeta("--- !Missed\n" + "Pass: inline\n" + "Name: NoDefinition\n" + "Function: foo\n"); + + // No string table. + parseGoodMeta(StringRef("REMARKS\0" + "\0\0\0\0\0\0\0\0" + "\0\0\0\0\0\0\0\0" + "--- !Missed\n" + "Pass: inline\n" + "Name: NoDefinition\n" + "Function: foo\n", + 82)); + + // Use the string table from the metadata. + parseGoodMeta(StringRef("REMARKS\0" + "\0\0\0\0\0\0\0\0" + "\x02\0\0\0\0\0\0\0" + "a\0" + "--- !Missed\n" + "Pass: 0\n" + "Name: 0\n" + "Function: 0\n", + 66)); +} + +TEST(YAMLRemarks, ParsingBadMeta) { + parseExpectErrorMeta(StringRef("REMARKSS", 9), + "Expecting \\0 after magic number."); + + parseExpectErrorMeta(StringRef("REMARKS\0", 8), "Expecting version number."); + + parseExpectErrorMeta(StringRef("REMARKS\0" + "\x09\0\0\0\0\0\0\0", + 16), + "Mismatching remark version. Got 9, expected 0."); + + parseExpectErrorMeta(StringRef("REMARKS\0" + "\0\0\0\0\0\0\0\0", + 16), + "Expecting string table size."); + + parseExpectErrorMeta(StringRef("REMARKS\0" + "\0\0\0\0\0\0\0\0" + "\x01\0\0\0\0\0\0\0", + 24), + "Expecting string table."); + + parseExpectErrorMeta(StringRef("REMARKS\0" + "\0\0\0\0\0\0\0\0" + "\0\0\0\0\0\0\0\0" + "/path/", + 28), + "No such file or directory"); +}