From e1596d7d9be1659730403b95efb94dad13c8495a Mon Sep 17 00:00:00 2001 From: Florian Hahn Date: Fri, 26 May 2023 11:01:18 +0100 Subject: [PATCH] [Remarks] Retain all remarks by default, add option to drop without DL. At the moment, dsymutil drops all remarks without debug location. There are many cases where debug location may be missing for remarks, mostly due LLVM not preserving debug locations. When using bitstream remarks for statistical analysis, those missed remarks mean we get an incomplete picture. The patch flips the default to keeping all remarks and leaving it to tools that display remarks to filter out remarks without debug locations as needed. The new --remarks-drop-without-debug flag can be used to drop remarks without debug locations, i.e. restore the previous behavior. Reviewed By: thegameg Differential Revision: https://reviews.llvm.org/D151089 --- llvm/docs/CommandGuide/dsymutil.rst | 4 +++ llvm/include/llvm/Remarks/RemarkLinker.h | 13 +++++++++ llvm/lib/Remarks/RemarkLinker.cpp | 3 -- llvm/test/tools/dsymutil/cmdline.test | 1 + llvm/tools/dsymutil/DwarfLinkerForBinary.cpp | 1 + llvm/tools/dsymutil/LinkUtils.h | 3 ++ llvm/tools/dsymutil/Options.td | 5 ++++ llvm/tools/dsymutil/dsymutil.cpp | 3 ++ llvm/unittests/Remarks/RemarksLinkingTest.cpp | 42 +++++++++++++++++++++++++-- 9 files changed, 69 insertions(+), 6 deletions(-) diff --git a/llvm/docs/CommandGuide/dsymutil.rst b/llvm/docs/CommandGuide/dsymutil.rst index d3ae16d..0cc2a47 100644 --- a/llvm/docs/CommandGuide/dsymutil.rst +++ b/llvm/docs/CommandGuide/dsymutil.rst @@ -107,6 +107,10 @@ OPTIONS output stream. When enabled warnings are embedded in the linked DWARF debug information. +.. option:: --remarks-drop-without-debug + + Drop remarks without valid debug locations. Without this flags, all remarks are kept. + .. option:: --remarks-output-format Specify the format to be used when serializing the linked remarks. diff --git a/llvm/include/llvm/Remarks/RemarkLinker.h b/llvm/include/llvm/Remarks/RemarkLinker.h index 307eb3f..f538718 100644 --- a/llvm/include/llvm/Remarks/RemarkLinker.h +++ b/llvm/include/llvm/Remarks/RemarkLinker.h @@ -54,13 +54,26 @@ private: /// A path to append before the external file path found in remark metadata. std::optional PrependPath; + /// If true, keep all remarks, otherwise only keep remarks with valid debug + /// locations. + bool KeepAllRemarks = true; + /// Keep this remark. If it's already in the set, discard it. Remark &keep(std::unique_ptr Remark); + /// Returns true if \p R should be kept. If KeepAllRemarks is false, only + /// return true if \p R has a valid debug location. + bool shouldKeepRemark(const Remark &R) { + return KeepAllRemarks ? true : R.Loc.has_value(); + } + public: /// Set a path to prepend to the external file path. void setExternalFilePrependPath(StringRef PrependPath); + /// Set KeepAllRemarks to \p B. + void setKeepAllRemarks(bool B) { KeepAllRemarks = B; } + /// Link the remarks found in \p Buffer. /// If \p RemarkFormat is not provided, try to deduce it from the metadata in /// \p Buffer. diff --git a/llvm/lib/Remarks/RemarkLinker.cpp b/llvm/lib/Remarks/RemarkLinker.cpp index 74acb08..b70b06d 100644 --- a/llvm/lib/Remarks/RemarkLinker.cpp +++ b/llvm/lib/Remarks/RemarkLinker.cpp @@ -66,9 +66,6 @@ void RemarkLinker::setExternalFilePrependPath(StringRef PrependPathIn) { PrependPath = std::string(PrependPathIn); } -// Discard remarks with no source location. -static bool shouldKeepRemark(const Remark &R) { return R.Loc.has_value(); } - Error RemarkLinker::link(StringRef Buffer, std::optional RemarkFormat) { if (!RemarkFormat) { Expected ParserFormat = magicToFormat(Buffer); diff --git a/llvm/test/tools/dsymutil/cmdline.test b/llvm/test/tools/dsymutil/cmdline.test index bf256bd..dc716cf 100644 --- a/llvm/test/tools/dsymutil/cmdline.test +++ b/llvm/test/tools/dsymutil/cmdline.test @@ -22,6 +22,7 @@ CHECK: -oso-prepend-path CHECK: -out CHECK: {{-o }} CHECK: -papertrail +CHECK: -remarks-drop-without-debug CHECK: -remarks-output-format CHECK: -remarks-prepend-path CHECK: -reproducer diff --git a/llvm/tools/dsymutil/DwarfLinkerForBinary.cpp b/llvm/tools/dsymutil/DwarfLinkerForBinary.cpp index efa490e..cf772e5 100644 --- a/llvm/tools/dsymutil/DwarfLinkerForBinary.cpp +++ b/llvm/tools/dsymutil/DwarfLinkerForBinary.cpp @@ -571,6 +571,7 @@ bool DwarfLinkerForBinary::link(const DebugMap &Map) { remarks::RemarkLinker RL; if (!Options.RemarksPrependPath.empty()) RL.setExternalFilePrependPath(Options.RemarksPrependPath); + RL.setKeepAllRemarks(Options.RemarksKeepAll); GeneralLinker.setObjectPrefixMap(&Options.ObjectPrefixMap); std::function TranslationLambda = [&](StringRef Input) { diff --git a/llvm/tools/dsymutil/LinkUtils.h b/llvm/tools/dsymutil/LinkUtils.h index 77f3608..9d25190 100644 --- a/llvm/tools/dsymutil/LinkUtils.h +++ b/llvm/tools/dsymutil/LinkUtils.h @@ -98,6 +98,9 @@ struct LinkOptions { /// The output format of the remarks. remarks::Format RemarksFormat = remarks::Format::Bitstream; + /// Whether all remarks should be kept or only remarks with valid debug + /// locations. + bool RemarksKeepAll = true; /// @} LinkOptions() = default; diff --git a/llvm/tools/dsymutil/Options.td b/llvm/tools/dsymutil/Options.td index 654f598..57d117b 100644 --- a/llvm/tools/dsymutil/Options.td +++ b/llvm/tools/dsymutil/Options.td @@ -194,3 +194,8 @@ def remarks_output_format: Separate<["--", "-"], "remarks-output-format">, HelpText<"Specify the format to be used when serializing the linked remarks.">, Group; def: Joined<["--", "-"], "remarks-output-format=">, Alias; + +def remarks_drop_without_debug: Flag<["--", "-"], "remarks-drop-without-debug">, + HelpText<"Drop remarks without valid debug locations. Without this flags, " + "all remarks are kept.">, + Group; diff --git a/llvm/tools/dsymutil/dsymutil.cpp b/llvm/tools/dsymutil/dsymutil.cpp index f504ac0..9bd0bc6 100644 --- a/llvm/tools/dsymutil/dsymutil.cpp +++ b/llvm/tools/dsymutil/dsymutil.cpp @@ -387,6 +387,9 @@ static Expected getOptions(opt::InputArgList &Args) { return FormatOrErr.takeError(); } + Options.LinkOpts.RemarksKeepAll = + !Args.hasArg(OPT_remarks_drop_without_debug); + if (Error E = verifyOptions(Options)) return std::move(E); return Options; diff --git a/llvm/unittests/Remarks/RemarksLinkingTest.cpp b/llvm/unittests/Remarks/RemarksLinkingTest.cpp index 95c800d..ff2aec6 100644 --- a/llvm/unittests/Remarks/RemarksLinkingTest.cpp +++ b/llvm/unittests/Remarks/RemarksLinkingTest.cpp @@ -41,8 +41,11 @@ static void serializeAndCheck(remarks::RemarkLinker &RL, } static void check(remarks::Format InputFormat, StringRef Input, - remarks::Format OutputFormat, StringRef ExpectedOutput) { + remarks::Format OutputFormat, StringRef ExpectedOutput, + std::optional KeepAllRemarks = {}) { remarks::RemarkLinker RL; + if (KeepAllRemarks) + RL.setKeepAllRemarks(*KeepAllRemarks); EXPECT_FALSE(RL.link(Input, InputFormat)); serializeAndCheck(RL, OutputFormat, ExpectedOutput); } @@ -73,14 +76,28 @@ TEST(Remarks, LinkingGoodYAML) { "Function: foo\n" "...\n"); - // Check that we don't keep remarks without debug locations. + // Check that we don't keep remarks without debug locations, unless + // KeepAllRemarks is set. + check(remarks::Format::YAML, + "--- !Missed\n" + "Pass: inline\n" + "Name: NoDefinition\n" + "Function: foo\n" + "...\n", + remarks::Format::YAML, "", + /*KeepAllRemarks=*/false); check(remarks::Format::YAML, "--- !Missed\n" "Pass: inline\n" "Name: NoDefinition\n" "Function: foo\n" "...\n", - remarks::Format::YAML, ""); + remarks::Format::YAML, + "--- !Missed\n" + "Pass: inline\n" + "Name: NoDefinition\n" + "Function: foo\n" + "...\n"); // Check that we deduplicate remarks. check(remarks::Format::YAML, @@ -127,6 +144,25 @@ TEST(Remarks, LinkingGoodBitstream) { " \n" "\n"); + // Check that we keep remarks without debug info. + check(remarks::Format::YAML, + "--- !Missed\n" + "Pass: inline\n" + "Name: NoDefinition\n" + "Function: foo\n" + "...\n", + remarks::Format::Bitstream, + "\n" + "\n" + " \n" + " \n" + " blob data = " + "'inline\\x00NoDefinition\\x00foo\\x00'\n" + "\n" + "\n" + " \n" + "\n"); + // Check that we deduplicate remarks. check(remarks::Format::YAML, "--- !Missed\n" -- 2.7.4