From d81fc2e8c389f46a1f2e0fbca35e72b7f845bee4 Mon Sep 17 00:00:00 2001 From: Francis Visoiu Mistrih Date: Mon, 16 Sep 2019 22:45:17 +0000 Subject: [PATCH] [Remarks] Allow remarks::Format::YAML to take a string table It should be allowed to take a string table in case all the strings in the remarks point there, but it shouldn't use it during serialization. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@372042 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../llvm/Remarks/BitstreamRemarkSerializer.h | 4 + include/llvm/Remarks/RemarkSerializer.h | 10 +- include/llvm/Remarks/YAMLRemarkSerializer.h | 32 ++++-- lib/Remarks/BitstreamRemarkSerializer.cpp | 4 +- lib/Remarks/RemarkSerializer.cpp | 8 +- lib/Remarks/RemarkStringTable.cpp | 1 - lib/Remarks/YAMLRemarkSerializer.cpp | 54 ++++++---- .../Remarks/YAMLRemarksSerializerTest.cpp | 100 ++++++++++++------ 8 files changed, 139 insertions(+), 74 deletions(-) diff --git a/include/llvm/Remarks/BitstreamRemarkSerializer.h b/include/llvm/Remarks/BitstreamRemarkSerializer.h index 5e5fd0eb99a..62a175a1db0 100644 --- a/include/llvm/Remarks/BitstreamRemarkSerializer.h +++ b/include/llvm/Remarks/BitstreamRemarkSerializer.h @@ -148,6 +148,10 @@ struct BitstreamRemarkSerializer : public RemarkSerializer { std::unique_ptr metaSerializer(raw_ostream &OS, Optional ExternalFilename = None) override; + + static bool classof(const RemarkSerializer *S) { + return S->SerializerFormat == Format::Bitstream; + } }; /// Serializer of metadata for bitstream remarks. diff --git a/include/llvm/Remarks/RemarkSerializer.h b/include/llvm/Remarks/RemarkSerializer.h index cea2afb2d37..35752cd5f6f 100644 --- a/include/llvm/Remarks/RemarkSerializer.h +++ b/include/llvm/Remarks/RemarkSerializer.h @@ -36,6 +36,8 @@ struct MetaSerializer; /// This is the base class for a remark serializer. /// It includes support for using a string table while emitting. struct RemarkSerializer { + /// The format of the serializer. + Format SerializerFormat; /// The open raw_ostream that the remark diagnostics are emitted to. raw_ostream &OS; /// The serialization mode. @@ -44,8 +46,9 @@ struct RemarkSerializer { /// The table can be serialized to be consumed after the compilation. Optional StrTab; - RemarkSerializer(raw_ostream &OS, SerializerMode Mode) - : OS(OS), Mode(Mode), StrTab() {} + RemarkSerializer(Format SerializerFormat, raw_ostream &OS, + SerializerMode Mode) + : SerializerFormat(SerializerFormat), OS(OS), Mode(Mode), StrTab() {} /// This is just an interface. virtual ~RemarkSerializer() = default; @@ -71,7 +74,8 @@ struct MetaSerializer { /// Create a remark serializer. Expected> -createRemarkSerializer(Format RemarksFormat, SerializerMode Mode, raw_ostream &OS); +createRemarkSerializer(Format RemarksFormat, SerializerMode Mode, + raw_ostream &OS); /// Create a remark serializer that uses a pre-filled string table. Expected> diff --git a/include/llvm/Remarks/YAMLRemarkSerializer.h b/include/llvm/Remarks/YAMLRemarkSerializer.h index 77b78380c07..f1213beab15 100644 --- a/include/llvm/Remarks/YAMLRemarkSerializer.h +++ b/include/llvm/Remarks/YAMLRemarkSerializer.h @@ -34,12 +34,22 @@ struct YAMLRemarkSerializer : public RemarkSerializer { /// The YAML streamer. yaml::Output YAMLOutput; - YAMLRemarkSerializer(raw_ostream &OS, SerializerMode Mode); + YAMLRemarkSerializer(raw_ostream &OS, SerializerMode Mode, + Optional StrTab = None); void emit(const Remark &Remark) override; std::unique_ptr metaSerializer(raw_ostream &OS, Optional ExternalFilename = None) override; + + static bool classof(const RemarkSerializer *S) { + return S->SerializerFormat == Format::YAML; + } + +protected: + YAMLRemarkSerializer(Format SerializerFormat, raw_ostream &OS, + SerializerMode Mode, + Optional StrTab = None); }; struct YAMLMetaSerializer : public MetaSerializer { @@ -60,15 +70,13 @@ struct YAMLStrTabRemarkSerializer : public YAMLRemarkSerializer { bool DidEmitMeta = false; YAMLStrTabRemarkSerializer(raw_ostream &OS, SerializerMode Mode) - : YAMLRemarkSerializer(OS, Mode) { - // Having a string table set up enables the serializer to use it. + : YAMLRemarkSerializer(Format::YAMLStrTab, OS, Mode) { + // We always need a string table for this type of serializer. StrTab.emplace(); } YAMLStrTabRemarkSerializer(raw_ostream &OS, SerializerMode Mode, - StringTable StrTabIn) - : YAMLRemarkSerializer(OS, Mode) { - StrTab = std::move(StrTabIn); - } + StringTable StrTab) + : YAMLRemarkSerializer(Format::YAMLStrTab, OS, Mode, std::move(StrTab)) {} /// Override to emit the metadata if necessary. void emit(const Remark &Remark) override; @@ -76,16 +84,20 @@ struct YAMLStrTabRemarkSerializer : public YAMLRemarkSerializer { std::unique_ptr metaSerializer(raw_ostream &OS, Optional ExternalFilename = None) override; + + static bool classof(const RemarkSerializer *S) { + return S->SerializerFormat == Format::YAMLStrTab; + } }; struct YAMLStrTabMetaSerializer : public YAMLMetaSerializer { /// The string table is part of the metadata. - StringTable StrTab; + const StringTable &StrTab; YAMLStrTabMetaSerializer(raw_ostream &OS, Optional ExternalFilename, - StringTable StrTab) - : YAMLMetaSerializer(OS, ExternalFilename), StrTab(std::move(StrTab)) {} + const StringTable &StrTab) + : YAMLMetaSerializer(OS, ExternalFilename), StrTab(StrTab) {} void emit() override; }; diff --git a/lib/Remarks/BitstreamRemarkSerializer.cpp b/lib/Remarks/BitstreamRemarkSerializer.cpp index 03c5697109e..d02782c7954 100644 --- a/lib/Remarks/BitstreamRemarkSerializer.cpp +++ b/lib/Remarks/BitstreamRemarkSerializer.cpp @@ -326,7 +326,7 @@ StringRef BitstreamRemarkSerializerHelper::getBuffer() { BitstreamRemarkSerializer::BitstreamRemarkSerializer(raw_ostream &OS, SerializerMode Mode) - : RemarkSerializer(OS, Mode), + : RemarkSerializer(Format::Bitstream, OS, Mode), Helper(BitstreamRemarkContainerType::SeparateRemarksFile) { assert(Mode == SerializerMode::Separate && "For SerializerMode::Standalone, a pre-filled string table needs to " @@ -338,7 +338,7 @@ BitstreamRemarkSerializer::BitstreamRemarkSerializer(raw_ostream &OS, BitstreamRemarkSerializer::BitstreamRemarkSerializer(raw_ostream &OS, SerializerMode Mode, StringTable StrTabIn) - : RemarkSerializer(OS, Mode), + : RemarkSerializer(Format::Bitstream, OS, Mode), Helper(Mode == SerializerMode::Separate ? BitstreamRemarkContainerType::SeparateRemarksFile : BitstreamRemarkContainerType::Standalone) { diff --git a/lib/Remarks/RemarkSerializer.cpp b/lib/Remarks/RemarkSerializer.cpp index caba7bbdc0e..ab19c84bbad 100644 --- a/lib/Remarks/RemarkSerializer.cpp +++ b/lib/Remarks/RemarkSerializer.cpp @@ -42,15 +42,13 @@ remarks::createRemarkSerializer(Format RemarksFormat, SerializerMode Mode, return createStringError(std::errc::invalid_argument, "Unknown remark serializer format."); case Format::YAML: - return createStringError(std::errc::invalid_argument, - "Unable to use a string table with the yaml " - "format. Use 'yaml-strtab' instead."); + return std::make_unique(OS, Mode, std::move(StrTab)); case Format::YAMLStrTab: return std::make_unique(OS, Mode, - std::move(StrTab)); + std::move(StrTab)); case Format::Bitstream: return std::make_unique(OS, Mode, - std::move(StrTab)); + std::move(StrTab)); } llvm_unreachable("Unknown remarks::Format enum"); } diff --git a/lib/Remarks/RemarkStringTable.cpp b/lib/Remarks/RemarkStringTable.cpp index 0f8a95acdba..51156465be5 100644 --- a/lib/Remarks/RemarkStringTable.cpp +++ b/lib/Remarks/RemarkStringTable.cpp @@ -46,7 +46,6 @@ void StringTable::internalize(Remark &R) { if (R.Loc) Impl(R.Loc->SourceFilePath); for (Argument &Arg : R.Args) { - // We need to mutate elements from an ArrayRef here. Impl(Arg.Key); Impl(Arg.Val); if (Arg.Loc) diff --git a/lib/Remarks/YAMLRemarkSerializer.cpp b/lib/Remarks/YAMLRemarkSerializer.cpp index a4d67cc6715..66eb06bbc4f 100644 --- a/lib/Remarks/YAMLRemarkSerializer.cpp +++ b/lib/Remarks/YAMLRemarkSerializer.cpp @@ -56,11 +56,14 @@ template <> struct MappingTraits { else llvm_unreachable("Unknown remark type"); - if (Optional &StrTab = - reinterpret_cast(io.getContext())->StrTab) { - unsigned PassID = StrTab->add(Remark->PassName).first; - unsigned NameID = StrTab->add(Remark->RemarkName).first; - unsigned FunctionID = StrTab->add(Remark->FunctionName).first; + if (auto *Serializer = dyn_cast( + reinterpret_cast(io.getContext()))) { + assert(Serializer->StrTab.hasValue() && + "YAMLStrTabSerializer with no StrTab."); + StringTable &StrTab = *Serializer->StrTab; + unsigned PassID = StrTab.add(Remark->PassName).first; + unsigned NameID = StrTab.add(Remark->RemarkName).first; + unsigned FunctionID = StrTab.add(Remark->FunctionName).first; mapRemarkHeader(io, PassID, NameID, Remark->Loc, FunctionID, Remark->Hotness, Remark->Args); } else { @@ -78,9 +81,12 @@ template <> struct MappingTraits { unsigned Line = RL.SourceLine; unsigned Col = RL.SourceColumn; - if (Optional &StrTab = - reinterpret_cast(io.getContext())->StrTab) { - unsigned FileID = StrTab->add(File).first; + if (auto *Serializer = dyn_cast( + reinterpret_cast(io.getContext()))) { + assert(Serializer->StrTab.hasValue() && + "YAMLStrTabSerializer with no StrTab."); + StringTable &StrTab = *Serializer->StrTab; + unsigned FileID = StrTab.add(File).first; io.mapRequired("File", FileID); } else { io.mapRequired("File", File); @@ -130,9 +136,12 @@ template <> struct MappingTraits { static void mapping(IO &io, Argument &A) { assert(io.outputting() && "input not yet implemented"); - if (Optional &StrTab = - reinterpret_cast(io.getContext())->StrTab) { - auto ValueID = StrTab->add(A.Val).first; + if (auto *Serializer = dyn_cast( + reinterpret_cast(io.getContext()))) { + assert(Serializer->StrTab.hasValue() && + "YAMLStrTabSerializer with no StrTab."); + StringTable &StrTab = *Serializer->StrTab; + auto ValueID = StrTab.add(A.Val).first; io.mapRequired(A.Key.data(), ValueID); } else if (StringRef(A.Val).count('\n') > 1) { StringBlockVal S(A.Val); @@ -149,8 +158,17 @@ template <> struct MappingTraits { LLVM_YAML_IS_SEQUENCE_VECTOR(Argument) -YAMLRemarkSerializer::YAMLRemarkSerializer(raw_ostream &OS, SerializerMode Mode) - : RemarkSerializer(OS, Mode), YAMLOutput(OS, reinterpret_cast(this)) {} +YAMLRemarkSerializer::YAMLRemarkSerializer(raw_ostream &OS, SerializerMode Mode, + Optional StrTabIn) + : YAMLRemarkSerializer(Format::YAML, OS, Mode, std::move(StrTabIn)) {} + +YAMLRemarkSerializer::YAMLRemarkSerializer(Format SerializerFormat, + raw_ostream &OS, SerializerMode Mode, + Optional StrTabIn) + : RemarkSerializer(SerializerFormat, OS, Mode), + YAMLOutput(OS, reinterpret_cast(this)) { + StrTab = std::move(StrTabIn); +} void YAMLRemarkSerializer::emit(const Remark &Remark) { // Again, YAMLTraits expect a non-const object for inputting, but we're not @@ -183,7 +201,7 @@ std::unique_ptr YAMLStrTabRemarkSerializer::metaSerializer( raw_ostream &OS, Optional ExternalFilename) { assert(StrTab); return std::make_unique(OS, ExternalFilename, - std::move(*StrTab)); + *StrTab); } static void emitMagic(raw_ostream &OS) { @@ -200,9 +218,9 @@ static void emitVersion(raw_ostream &OS) { OS.write(Version.data(), Version.size()); } -static void emitStrTab(raw_ostream &OS, const Optional &StrTab) { +static void emitStrTab(raw_ostream &OS, Optional StrTab) { // Emit the string table in the section. - uint64_t StrTabSize = StrTab ? StrTab->SerializedSize : 0; + uint64_t StrTabSize = StrTab ? (*StrTab)->SerializedSize : 0; // Emit the total size of the string table (the size itself excluded): // little-endian uint64_t. // Note: even if no string table is used, emit 0. @@ -210,7 +228,7 @@ static void emitStrTab(raw_ostream &OS, const Optional &StrTab) { support::endian::write64le(StrTabSizeBuf.data(), StrTabSize); OS.write(StrTabSizeBuf.data(), StrTabSizeBuf.size()); if (StrTab) - StrTab->serialize(OS); + (*StrTab)->serialize(OS); } static void emitExternalFile(raw_ostream &OS, StringRef Filename) { @@ -233,7 +251,7 @@ void YAMLMetaSerializer::emit() { void YAMLStrTabMetaSerializer::emit() { emitMagic(OS); emitVersion(OS); - emitStrTab(OS, std::move(StrTab)); + emitStrTab(OS, &StrTab); if (ExternalFilename) emitExternalFile(OS, *ExternalFilename); } diff --git a/unittests/Remarks/YAMLRemarksSerializerTest.cpp b/unittests/Remarks/YAMLRemarksSerializerTest.cpp index be923e86595..0ebb47809f5 100644 --- a/unittests/Remarks/YAMLRemarksSerializerTest.cpp +++ b/unittests/Remarks/YAMLRemarksSerializerTest.cpp @@ -22,21 +22,18 @@ using namespace llvm; -static void check(remarks::SerializerMode Mode, ArrayRef Rs, +static void check(remarks::Format SerializerFormat, + remarks::SerializerMode Mode, ArrayRef Rs, StringRef ExpectedR, Optional ExpectedMeta, - bool UseStrTab = false, Optional StrTab = None) { std::string Buf; raw_string_ostream OS(Buf); Expected> MaybeS = [&] { - if (UseStrTab) { - if (StrTab) - return createRemarkSerializer(remarks::Format::YAMLStrTab, Mode, OS, - std::move(*StrTab)); - else - return createRemarkSerializer(remarks::Format::YAMLStrTab, Mode, OS); - } else - return createRemarkSerializer(remarks::Format::YAML, Mode, OS); + if (StrTab) + return createRemarkSerializer(SerializerFormat, Mode, OS, + std::move(*StrTab)); + else + return createRemarkSerializer(SerializerFormat, Mode, OS); }(); EXPECT_FALSE(errorToBool(MaybeS.takeError())); std::unique_ptr S = std::move(*MaybeS); @@ -54,18 +51,20 @@ static void check(remarks::SerializerMode Mode, ArrayRef Rs, } } -static void check(const remarks::Remark &R, StringRef ExpectedR, - StringRef ExpectedMeta, bool UseStrTab = false, +static void check(remarks::Format SerializerFormat, const remarks::Remark &R, + StringRef ExpectedR, StringRef ExpectedMeta, Optional StrTab = None) { - return check(remarks::SerializerMode::Separate, makeArrayRef(&R, &R + 1), ExpectedR, ExpectedMeta, - UseStrTab, std::move(StrTab)); + return check(SerializerFormat, remarks::SerializerMode::Separate, + makeArrayRef(&R, &R + 1), ExpectedR, ExpectedMeta, + std::move(StrTab)); } -static void checkStandalone(const remarks::Remark &R, StringRef ExpectedR, +static void checkStandalone(remarks::Format SerializerFormat, + const remarks::Remark &R, StringRef ExpectedR, Optional StrTab = None) { - bool UseStrTab = StrTab.hasValue(); - return check(remarks::SerializerMode::Standalone, makeArrayRef(&R, &R +1), ExpectedR, - /*ExpectedMeta=*/None, UseStrTab, std::move(StrTab)); + return check(SerializerFormat, remarks::SerializerMode::Standalone, + makeArrayRef(&R, &R + 1), ExpectedR, + /*ExpectedMeta=*/None, std::move(StrTab)); } TEST(YAMLRemarks, SerializerRemark) { @@ -83,7 +82,7 @@ TEST(YAMLRemarks, SerializerRemark) { R.Args.back().Key = "keydebug"; R.Args.back().Val = "valuedebug"; R.Args.back().Loc = remarks::RemarkLocation{"argpath", 6, 7}; - check(R, + check(remarks::Format::YAML, R, "--- !Missed\n" "Pass: pass\n" "Name: name\n" @@ -97,8 +96,7 @@ TEST(YAMLRemarks, SerializerRemark) { "...\n", StringRef("REMARKS\0" "\0\0\0\0\0\0\0\0" - "\0\0\0\0\0\0\0\0" - EXTERNALFILETESTPATH"\0", + "\0\0\0\0\0\0\0\0" EXTERNALFILETESTPATH "\0", 38)); } @@ -118,7 +116,7 @@ TEST(YAMLRemarks, SerializerRemarkStandalone) { R.Args.back().Val = "valuedebug"; R.Args.back().Loc = remarks::RemarkLocation{"argpath", 6, 7}; checkStandalone( - R, + remarks::Format::YAML, R, StringRef("--- !Missed\n" "Pass: pass\n" "Name: name\n" @@ -147,7 +145,7 @@ TEST(YAMLRemarks, SerializerRemarkStrTab) { R.Args.back().Key = "keydebug"; R.Args.back().Val = "valuedebug"; R.Args.back().Loc = remarks::RemarkLocation{"argpath", 6, 7}; - check(R, + check(remarks::Format::YAMLStrTab, R, "--- !Missed\n" "Pass: 0\n" "Name: 1\n" @@ -162,10 +160,9 @@ TEST(YAMLRemarks, SerializerRemarkStrTab) { StringRef("REMARKS\0" "\0\0\0\0\0\0\0\0" "\x2d\0\0\0\0\0\0\0" - "pass\0name\0func\0path\0value\0valuedebug\0argpath\0" - EXTERNALFILETESTPATH"\0", - 83), - /*UseStrTab=*/true); + "pass\0name\0func\0path\0value\0valuedebug\0argpath" + "\0" EXTERNALFILETESTPATH "\0", + 83)); } TEST(YAMLRemarks, SerializerRemarkParsedStrTab) { @@ -184,7 +181,7 @@ TEST(YAMLRemarks, SerializerRemarkParsedStrTab) { R.Args.back().Key = "keydebug"; R.Args.back().Val = "valuedebug"; R.Args.back().Loc = remarks::RemarkLocation{"argpath", 6, 7}; - check(R, + check(remarks::Format::YAMLStrTab, R, "--- !Missed\n" "Pass: 0\n" "Name: 1\n" @@ -199,13 +196,47 @@ TEST(YAMLRemarks, SerializerRemarkParsedStrTab) { StringRef("REMARKS\0" "\0\0\0\0\0\0\0\0" "\x2d\0\0\0\0\0\0\0" - "pass\0name\0func\0path\0value\0valuedebug\0argpath\0" - EXTERNALFILETESTPATH"\0", + "pass\0name\0func\0path\0value\0valuedebug\0argpath" + "\0" EXTERNALFILETESTPATH "\0", 83), - /*UseStrTab=*/true, remarks::StringTable(remarks::ParsedStringTable(StrTab))); } +TEST(YAMLRemarks, SerializerRemarkParsedStrTabStandaloneNoStrTab) { + // Check that we don't use the string table even if it was provided. + StringRef StrTab("pass\0name\0func\0path\0value\0valuedebug\0argpath\0", 45); + remarks::ParsedStringTable ParsedStrTab(StrTab); + remarks::StringTable PreFilledStrTab(ParsedStrTab); + remarks::Remark R; + R.RemarkType = remarks::Type::Missed; + R.PassName = "pass"; + R.RemarkName = "name"; + R.FunctionName = "func"; + R.Loc = remarks::RemarkLocation{"path", 3, 4}; + R.Hotness = 5; + R.Args.emplace_back(); + R.Args.back().Key = "key"; + R.Args.back().Val = "value"; + R.Args.emplace_back(); + R.Args.back().Key = "keydebug"; + R.Args.back().Val = "valuedebug"; + R.Args.back().Loc = remarks::RemarkLocation{"argpath", 6, 7}; + checkStandalone( + remarks::Format::YAML, R, + StringRef("--- !Missed\n" + "Pass: pass\n" + "Name: name\n" + "DebugLoc: { File: path, Line: 3, Column: 4 }\n" + "Function: func\n" + "Hotness: 5\n" + "Args:\n" + " - key: value\n" + " - keydebug: valuedebug\n" + " DebugLoc: { File: argpath, Line: 6, Column: 7 }\n" + "...\n"), + std::move(PreFilledStrTab)); +} + TEST(YAMLRemarks, SerializerRemarkParsedStrTabStandalone) { StringRef StrTab("pass\0name\0func\0path\0value\0valuedebug\0argpath\0", 45); remarks::ParsedStringTable ParsedStrTab(StrTab); @@ -225,7 +256,7 @@ TEST(YAMLRemarks, SerializerRemarkParsedStrTabStandalone) { R.Args.back().Val = "valuedebug"; R.Args.back().Loc = remarks::RemarkLocation{"argpath", 6, 7}; checkStandalone( - R, + remarks::Format::YAMLStrTab, R, StringRef("REMARKS\0" "\0\0\0\0\0\0\0\0" "\x2d\0\0\0\0\0\0\0" @@ -266,7 +297,7 @@ TEST(YAMLRemarks, SerializerRemarkParsedStrTabStandaloneMultipleRemarks) { R.Args.back().Loc = remarks::RemarkLocation{"argpath", 6, 7}; Rs.emplace_back(R.clone()); Rs.emplace_back(std::move(R)); - check(remarks::SerializerMode::Standalone, Rs, + check(remarks::Format::YAMLStrTab, remarks::SerializerMode::Standalone, Rs, StringRef("REMARKS\0" "\0\0\0\0\0\0\0\0" "\x2d\0\0\0\0\0\0\0" @@ -294,6 +325,5 @@ TEST(YAMLRemarks, SerializerRemarkParsedStrTabStandaloneMultipleRemarks) { " DebugLoc: { File: 6, Line: 6, Column: 7 }\n" "...\n", 561), - /*ExpectedMeta=*/None, - /*UseStrTab=*/true, std::move(PreFilledStrTab)); + /*ExpectedMeta=*/None, std::move(PreFilledStrTab)); } -- 2.40.0