[llvm] e1596d7 - [Remarks] Retain all remarks by default, add option to drop without DL.
Florian Hahn via llvm-commits
llvm-commits at lists.llvm.org
Fri May 26 03:02:39 PDT 2023
Author: Florian Hahn
Date: 2023-05-26T11:01:20+01:00
New Revision: e1596d7d9be1659730403b95efb94dad13c8495a
URL: https://github.com/llvm/llvm-project/commit/e1596d7d9be1659730403b95efb94dad13c8495a
DIFF: https://github.com/llvm/llvm-project/commit/e1596d7d9be1659730403b95efb94dad13c8495a.diff
LOG: [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
Added:
Modified:
llvm/docs/CommandGuide/dsymutil.rst
llvm/include/llvm/Remarks/RemarkLinker.h
llvm/lib/Remarks/RemarkLinker.cpp
llvm/test/tools/dsymutil/cmdline.test
llvm/tools/dsymutil/DwarfLinkerForBinary.cpp
llvm/tools/dsymutil/LinkUtils.h
llvm/tools/dsymutil/Options.td
llvm/tools/dsymutil/dsymutil.cpp
llvm/unittests/Remarks/RemarksLinkingTest.cpp
Removed:
################################################################################
diff --git a/llvm/docs/CommandGuide/dsymutil.rst b/llvm/docs/CommandGuide/dsymutil.rst
index d3ae16d03679e..0cc2a472bbd06 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 <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 307eb3f6e84a2..f538718941c5d 100644
--- a/llvm/include/llvm/Remarks/RemarkLinker.h
+++ b/llvm/include/llvm/Remarks/RemarkLinker.h
@@ -54,13 +54,26 @@ struct RemarkLinker {
/// A path to append before the external file path found in remark metadata.
std::optional<std::string> 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> 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 74acb0835ff83..b70b06d706bdf 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<Format> RemarkFormat) {
if (!RemarkFormat) {
Expected<Format> ParserFormat = magicToFormat(Buffer);
diff --git a/llvm/test/tools/dsymutil/cmdline.test b/llvm/test/tools/dsymutil/cmdline.test
index bf256bd4a9b70..dc716cf25b21f 100644
--- a/llvm/test/tools/dsymutil/cmdline.test
+++ b/llvm/test/tools/dsymutil/cmdline.test
@@ -22,6 +22,7 @@ CHECK: -oso-prepend-path <path>
CHECK: -out <filename>
CHECK: {{-o <filename>}}
CHECK: -papertrail
+CHECK: -remarks-drop-without-debug
CHECK: -remarks-output-format <format>
CHECK: -remarks-prepend-path <path>
CHECK: -reproducer <mode>
diff --git a/llvm/tools/dsymutil/DwarfLinkerForBinary.cpp b/llvm/tools/dsymutil/DwarfLinkerForBinary.cpp
index efa490ebbb40f..cf772e534aaa6 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<StringRef(StringRef)> TranslationLambda = [&](StringRef Input) {
diff --git a/llvm/tools/dsymutil/LinkUtils.h b/llvm/tools/dsymutil/LinkUtils.h
index 77f3608881416..9d25190919fa7 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 654f5982f8bf7..57d117bdd6121 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<grp_general>;
def: Joined<["--", "-"], "remarks-output-format=">, Alias<remarks_output_format>;
+
+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<grp_general>;
diff --git a/llvm/tools/dsymutil/dsymutil.cpp b/llvm/tools/dsymutil/dsymutil.cpp
index f504ac052e936..9bd0bc6b98002 100644
--- a/llvm/tools/dsymutil/dsymutil.cpp
+++ b/llvm/tools/dsymutil/dsymutil.cpp
@@ -387,6 +387,9 @@ static Expected<DsymutilOptions> 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 95c800d2d10d4..ff2aec669f2fd 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<bool> 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) {
" <Remark debug location codeid=6 abbrevid=5 op0=3 op1=3 op2=12/>\n"
"</Remark>\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,
+ "<BLOCKINFO_BLOCK/>\n"
+ "<Meta BlockID=8 NumWords=10 BlockCodeSize=3>\n"
+ " <Container info codeid=1 abbrevid=4 op0=0 op1=2/>\n"
+ " <Remark version codeid=2 abbrevid=5 op0=0/>\n"
+ " <String table codeid=3 abbrevid=6/> blob data = "
+ "'inline\\x00NoDefinition\\x00foo\\x00'\n"
+ "</Meta>\n"
+ "<Remark BlockID=9 NumWords=1 BlockCodeSize=4>\n"
+ " <Remark header codeid=5 abbrevid=4 op0=2 op1=1 op2=0 op3=2/>\n"
+ "</Remark>\n");
+
// Check that we deduplicate remarks.
check(remarks::Format::YAML,
"--- !Missed\n"
More information about the llvm-commits
mailing list