[llvm] r367155 - Reland: [Remarks] Support parsing remark metadata in the YAML remark parser
Yvan Roux via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 30 00:47:31 PDT 2019
On Mon, 29 Jul 2019 at 19:49, Francis Visoiu Mistrih
<francisvm at yahoo.com> wrote:
>
> Scratch that, I managed to fix it in r367243. It passes in http://lab.llvm.org:8011/builders/clang-cmake-armv8-quick/builds/14558/steps/ninja%20check%201/logs/stdio.
Great, Thanks Francis
> > On Jul 29, 2019, at 10:19 AM, Francis Visoiu Mistrih <francisvm at yahoo.com> wrote:
> >
> > Yvan,
> >
> > I can’t seem to reproduce this locally. What seems to happen is that the arguments of the format string got swapped which could be due to a miscompile? I will try to poke at it to see if I can find out.
> >
> >> On Jul 29, 2019, at 6:02 AM, Francis Visoiu Mistrih via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> >>
> >> Thanks Yvan. I’ll take a look. If it’s taking too long feel free to revert it, thanks!
> >>
> >> --
> >> Francis
> >>
> >>> On Jul 29, 2019, at 1:38 AM, Yvan Roux <yvan.roux at linaro.org> wrote:
> >>>
> >>> Hi Francis,
> >>>
> >>> ARM bots were broken by the first commit and still are after this
> >>> reland, logs are available here:
> >>>
> >>> http://lab.llvm.org:8011/builders/clang-cmake-armv7-quick/builds/9359/steps/ninja%20check%201/logs/FAIL%3A%20LLVM-Unit%3A%3AYAMLRemarks.ParsingBadMeta
> >>>
> >>> Thanks
> >>> Yvan
> >>>
> >>> On Fri, 26 Jul 2019 at 23:01, Francis Visoiu Mistrih via llvm-commits
> >>> <llvm-commits at lists.llvm.org> wrote:
> >>>>
> >>>> Author: thegameg
> >>>> Date: Fri Jul 26 14:02:02 2019
> >>>> New Revision: 367155
> >>>>
> >>>> URL: http://llvm.org/viewvc/llvm-project?rev=367155&view=rev
> >>>> Log:
> >>>> Reland: [Remarks] Support parsing remark metadata in the YAML remark parser
> >>>>
> >>>> This adds support to the yaml remark parser to be able to parse remarks
> >>>> directly from the metadata.
> >>>>
> >>>> This supports parsing separate metadata and following the external file
> >>>> with the associated metadata, and also a standalone file containing
> >>>> metadata + remarks all together.
> >>>>
> >>>> Original llvm-svn: 367148
> >>>> Revert llvm-svn: 367151
> >>>>
> >>>> This has a fix for gcc builds.
> >>>>
> >>>> Modified:
> >>>> llvm/trunk/docs/Remarks.rst
> >>>> llvm/trunk/include/llvm/Remarks/RemarkParser.h
> >>>> llvm/trunk/lib/Remarks/RemarkParser.cpp
> >>>> llvm/trunk/lib/Remarks/YAMLRemarkParser.cpp
> >>>> llvm/trunk/lib/Remarks/YAMLRemarkParser.h
> >>>> llvm/trunk/unittests/Remarks/YAMLRemarksParsingTest.cpp
> >>>>
> >>>> Modified: llvm/trunk/docs/Remarks.rst
> >>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/docs/Remarks.rst?rev=367155&r1=367154&r2=367155&view=diff
> >>>> ==============================================================================
> >>>> --- llvm/trunk/docs/Remarks.rst (original)
> >>>> +++ llvm/trunk/docs/Remarks.rst Fri Jul 26 14:02:02 2019
> >>>> @@ -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 objec
> >>>> ==============================================
> >>>>
> >>>> 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:
> >>>>
> >>>>
> >>>> Modified: llvm/trunk/include/llvm/Remarks/RemarkParser.h
> >>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Remarks/RemarkParser.h?rev=367155&r1=367154&r2=367155&view=diff
> >>>> ==============================================================================
> >>>> --- llvm/trunk/include/llvm/Remarks/RemarkParser.h (original)
> >>>> +++ llvm/trunk/include/llvm/Remarks/RemarkParser.h Fri Jul 26 14:02:02 2019
> >>>> @@ -80,6 +80,10 @@ Expected<std::unique_ptr<RemarkParser>>
> >>>> createRemarkParser(Format ParserFormat, StringRef Buf,
> >>>> ParsedStringTable StrTab);
> >>>>
> >>>> +Expected<std::unique_ptr<RemarkParser>>
> >>>> +createRemarkParserFromMeta(Format ParserFormat, StringRef Buf,
> >>>> + Optional<ParsedStringTable> StrTab = None);
> >>>> +
> >>>> } // end namespace remarks
> >>>> } // end namespace llvm
> >>>>
> >>>>
> >>>> Modified: llvm/trunk/lib/Remarks/RemarkParser.cpp
> >>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Remarks/RemarkParser.cpp?rev=367155&r1=367154&r2=367155&view=diff
> >>>> ==============================================================================
> >>>> --- llvm/trunk/lib/Remarks/RemarkParser.cpp (original)
> >>>> +++ llvm/trunk/lib/Remarks/RemarkParser.cpp Fri Jul 26 14:02:02 2019
> >>>> @@ -80,6 +80,21 @@ llvm::remarks::createRemarkParser(Format
> >>>> llvm_unreachable("unhandled ParseFormat");
> >>>> }
> >>>>
> >>>> +Expected<std::unique_ptr<RemarkParser>>
> >>>> +llvm::remarks::createRemarkParserFromMeta(Format ParserFormat, StringRef Buf,
> >>>> + Optional<ParsedStringTable> 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<RemarkParser> TheParser;
> >>>>
> >>>> Modified: llvm/trunk/lib/Remarks/YAMLRemarkParser.cpp
> >>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Remarks/YAMLRemarkParser.cpp?rev=367155&r1=367154&r2=367155&view=diff
> >>>> ==============================================================================
> >>>> --- llvm/trunk/lib/Remarks/YAMLRemarkParser.cpp (original)
> >>>> +++ llvm/trunk/lib/Remarks/YAMLRemarkParser.cpp Fri Jul 26 14:02:02 2019
> >>>> @@ -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 &La
> >>>> return SM;
> >>>> }
> >>>>
> >>>> +// Parse the magic number. This function returns true if this represents remark
> >>>> +// metadata, false otherwise.
> >>>> +static Expected<bool> 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<uint64_t> 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<uint64_t, support::little, support::unaligned>(
> >>>> + 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<uint64_t> 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<uint64_t, support::little, support::unaligned>(
> >>>> + Buf.data());
> >>>> + Buf = Buf.drop_front(sizeof(uint64_t));
> >>>> + return StrTabSize;
> >>>> +}
> >>>> +
> >>>> +static Expected<ParsedStringTable> 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<ParsedStringTable>(std::move(Result));
> >>>> +}
> >>>> +
> >>>> +Expected<std::unique_ptr<YAMLRemarkParser>>
> >>>> +remarks::createYAMLParserFromMeta(StringRef Buf,
> >>>> + Optional<ParsedStringTable> StrTab) {
> >>>> + // We now have a magic number. The metadata has to be correct.
> >>>> + Expected<bool> isMeta = parseMagic(Buf);
> >>>> + if (!isMeta)
> >>>> + return isMeta.takeError();
> >>>> + // If it's not recognized as metadata, roll back.
> >>>> + std::unique_ptr<MemoryBuffer> SeparateBuf;
> >>>> + if (*isMeta) {
> >>>> + Expected<uint64_t> Version = parseVersion(Buf);
> >>>> + if (!Version)
> >>>> + return Version.takeError();
> >>>> +
> >>>> + Expected<uint64_t> 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<ParsedStringTable> 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<std::unique_ptr<MemoryBuffer>> 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<YAMLRemarkParser> Result =
> >>>> + StrTab
> >>>> + ? llvm::make_unique<YAMLStrTabRemarkParser>(Buf, std::move(*StrTab))
> >>>> + : llvm::make_unique<YAMLRemarkParser>(Buf);
> >>>> + if (SeparateBuf)
> >>>> + Result->SeparateBuf = std::move(SeparateBuf);
> >>>> + return std::move(Result);
> >>>> +}
> >>>> +
> >>>> YAMLRemarkParser::YAMLRemarkParser(StringRef Buf)
> >>>> : YAMLRemarkParser(Buf, None) {}
> >>>>
> >>>>
> >>>> Modified: llvm/trunk/lib/Remarks/YAMLRemarkParser.h
> >>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Remarks/YAMLRemarkParser.h?rev=367155&r1=367154&r2=367155&view=diff
> >>>> ==============================================================================
> >>>> --- llvm/trunk/lib/Remarks/YAMLRemarkParser.h (original)
> >>>> +++ llvm/trunk/lib/Remarks/YAMLRemarkParser.h Fri Jul 26 14:02:02 2019
> >>>> @@ -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 RemarkP
> >>>> 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<MemoryBuffer> SeparateBuf;
> >>>>
> >>>> YAMLRemarkParser(StringRef Buf);
> >>>>
> >>>> @@ -104,6 +108,11 @@ protected:
> >>>> /// Parse one value to a string.
> >>>> Expected<StringRef> parseStr(yaml::KeyValueNode &Node) override;
> >>>> };
> >>>> +
> >>>> +Expected<std::unique_ptr<YAMLRemarkParser>>
> >>>> +createYAMLParserFromMeta(StringRef Buf,
> >>>> + Optional<ParsedStringTable> StrTab = None);
> >>>> +
> >>>> } // end namespace remarks
> >>>> } // end namespace llvm
> >>>>
> >>>>
> >>>> Modified: llvm/trunk/unittests/Remarks/YAMLRemarksParsingTest.cpp
> >>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Remarks/YAMLRemarksParsingTest.cpp?rev=367155&r1=367154&r2=367155&view=diff
> >>>> ==============================================================================
> >>>> --- llvm/trunk/unittests/Remarks/YAMLRemarksParsingTest.cpp (original)
> >>>> +++ llvm/trunk/unittests/Remarks/YAMLRemarksParsingTest.cpp Fri Jul 26 14:02:02 2019
> >>>> @@ -29,6 +29,22 @@ template <size_t N> void parseGood(const
> >>>> EXPECT_TRUE(errorToBool(std::move(E))); // Check for parsing errors.
> >>>> }
> >>>>
> >>>> +void parseGoodMeta(StringRef Buf) {
> >>>> + Expected<std::unique_ptr<remarks::RemarkParser>> MaybeParser =
> >>>> + remarks::createRemarkParserFromMeta(remarks::Format::YAML, Buf);
> >>>> + EXPECT_FALSE(errorToBool(MaybeParser.takeError()));
> >>>> + EXPECT_TRUE(*MaybeParser != nullptr);
> >>>> +
> >>>> + remarks::RemarkParser &Parser = **MaybeParser;
> >>>> + Expected<std::unique_ptr<remarks::Remark>> 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<remarks::EndOfFileError>());
> >>>> + EXPECT_TRUE(errorToBool(std::move(E))); // Check for parsing errors.
> >>>> +}
> >>>> +
> >>>> template <size_t N>
> >>>> bool parseExpectError(const char (&Buf)[N], const char *Error) {
> >>>> Expected<std::unique_ptr<remarks::RemarkParser>> MaybeParser =
> >>>> @@ -47,6 +63,17 @@ bool parseExpectError(const char (&Buf)[
> >>>> return StringRef(Stream.str()).contains(Error);
> >>>> }
> >>>>
> >>>> +void parseExpectErrorMeta(StringRef Buf, const char *Error) {
> >>>> + std::string ErrorStr;
> >>>> + raw_string_ostream Stream(ErrorStr);
> >>>> +
> >>>> + Expected<std::unique_ptr<remarks::RemarkParser>> 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, ParsingBadStringTableI
> >>>> 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");
> >>>> +}
> >>>>
> >>>>
> >>>> _______________________________________________
> >>>> llvm-commits mailing list
> >>>> llvm-commits at lists.llvm.org
> >>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> >>
> >> _______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at lists.llvm.org
> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> >
>
More information about the llvm-commits
mailing list