[Remarks] Add option to retain all remarks during remark linking.

[fhahn]

Summary

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 new remarks-keep-all flag can be used to retain all remarks during
linking.

[JDevlieghere]

Looks fine to me but @thegameg should LGTM this.

[thegameg]

LGTM, thank you!

I don't mind having this enabled by default, fwiw. We can change some of our users that only need remarks with source locations later to pass the flag to dsymutil to enable them.

[fhahn]

Flip the default to keep remarks without debug locs by default. I am planning on landing this tomorrow unless there are any additional comments :)

llvm/docs/CommandGuide/dsymutil.rst

 
 .. option:: --papertrail
 
  When running dsymutil as part of your build system, it can be desirable for
  warnings to be part of the end product, rather than just being emitted to the
  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.
 
 .. option:: --remarks-prepend-path <path>
 
  Specify a directory to prepend the paths of the external remark files.
 

llvm/include/llvm/Remarks/RemarkLinker.h

private:
   /// FIXME: std::set is probably not the most appropriate data structure here.
   /// Due to the limitation of having a move-only key, there isn't another
   /// obvious choice for now.
   std::set<std::unique_ptr<Remark>, RemarkPtrCompare> Remarks;
 
   /// 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.
   /// \p Buffer can be either a standalone remark container or just
   /// metadata. This takes care of uniquing and merging the remarks.
   Error link(StringRef Buffer,
              std::optional<Format> RemarkFormat = std::nullopt);
 

llvm/lib/Remarks/RemarkLinker.cpp

Remark &RemarkLinker::keep(std::unique_ptr<Remark> Remark) {
   auto Inserted = Remarks.insert(std::move(Remark));
   return **Inserted.first;
 }
 
 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);
     if (!ParserFormat)
       return ParserFormat.takeError();
     RemarkFormat = *ParserFormat;
   }
 

llvm/test/tools/dsymutil/cmdline.test

 CHECK: -no-output
 CHECK: -no-swiftmodule-timestamp
 CHECK: -num-threads  <threads>
 CHECK: -object-prefix-map <prefix=remapped>
 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>
 CHECK: -statistics
 CHECK: -symbol-map
 CHECK: -symtab
 CHECK: {{-S}}
 CHECK: -toolchain

llvm/tools/dsymutil/DwarfLinkerForBinary.cpp

bool DwarfLinkerForBinary::link(const DebugMap &Map) {
 
   DebugMap DebugMap(Map.getTriple(), Map.getBinaryPath());
 
   DWARFLinker GeneralLinker(Streamer.get(), DwarfLinkerClient::Dsymutil);
 
   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) {
     assert(Options.Translator);
     return Options.Translator(Input);
   };
 
   GeneralLinker.setVerbosity(Options.Verbose);

llvm/tools/dsymutil/LinkUtils.h

struct LinkOptions {
 
   /// -remarks-prepend-path: prepend a path to all the external remark file
   /// paths found in remark metadata.
   std::string RemarksPrependPath;
 
   /// 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;
 };
 
 inline void warn(Twine Warning, Twine Context = {}) {
   WithColor::warning() << Warning + "\n";
   if (!Context.isTriviallyEmpty())

llvm/tools/dsymutil/Options.td

def remarks_prepend_path: Separate<["--", "-"], "remarks-prepend-path">,
   Group<grp_general>;
 def: Joined<["--", "-"], "remarks-prepend-path=">, Alias<remarks_prepend_path>;
 
 def remarks_output_format: Separate<["--", "-"], "remarks-output-format">,
   MetaVarName<"<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>;

llvm/tools/dsymutil/dsymutil.cpp

if (opt::Arg *RemarksOutputFormat =
           Args.getLastArg(OPT_remarks_output_format)) {
     if (Expected<remarks::Format> FormatOrErr =
             remarks::parseFormat(RemarksOutputFormat->getValue()))
       Options.LinkOpts.RemarksFormat = *FormatOrErr;
     else
       return FormatOrErr.takeError();
   }
 
+  Options.LinkOpts.RemarksKeepAll =
+      !Args.hasArg(OPT_remarks_drop_without_debug);
+
   if (Error E = verifyOptions(Options))
     return std::move(E);
   return Options;
 }
 
 static Error createPlistFile(StringRef Bin, StringRef BundleRoot,
                              StringRef Toolchain) {
   // Create plist file to write to.

llvm/unittests/Remarks/RemarksLinkingTest.cpp

if (OutputFormat == remarks::Format::Bitstream) {
     EXPECT_FALSE(BA.analyze(O)); // Expect no errors.
     EXPECT_EQ(AnalyzeOS.str(), ExpectedOutput);
   } else {
     EXPECT_EQ(OS.str(), ExpectedOutput);
   }
 }
 
 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);
 }
 
 static void check(remarks::Format InputFormat, StringRef Input,
                   remarks::Format InputFormat2, StringRef Input2,
                   remarks::Format OutputFormat, StringRef ExpectedOutput) {
   remarks::RemarkLinker RL;
check(remarks::Format::YAML,
         remarks::Format::YAML,
         "--- !Missed\n"
         "Pass:            inline\n"
         "Name:            NoDefinition\n"
         "DebugLoc:        { File: file.c, Line: 3, Column: 12 }\n"
         "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,
         "--- !Missed\n"
         "Pass:            inline\n"
         "Name:            NoDefinition\n"
         "DebugLoc:        { File: file.c, Line: 3, Column: 12 }\n"
         "Function:        foo\n"
check(remarks::Format::YAML,
         "  <String table codeid=3 abbrevid=6/> blob data = "
         "'inline\\x00NoDefinition\\x00foo\\x00file.c\\x00'\n"
         "</Meta>\n"
         "<Remark BlockID=9 NumWords=4 BlockCodeSize=4>\n"
         "  <Remark header codeid=5 abbrevid=4 op0=2 op1=1 op2=0 op3=2/>\n"
         "  <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"
         "Pass:            inline\n"
         "Name:            NoDefinition\n"
         "DebugLoc:        { File: file.c, Line: 3, Column: 12 }\n"
         "Function:        foo\n"
         "...\n"