[dsymutil] Add support for linking remarks

[thegameg]

Summary

This adds support to dsymutil for linking remark files and placing them in the final .dSYM bundle.

The result will be placed in:

• a.out.dSYM/Contents/Resources/Remarks/a.out

or

• a.out.dSYM/Contents/Resources/Remarks/a.out-<arch> for universal binaries

When multi-threaded, this runs a third thread which loops over all the object files and parses remarks as it finds __remarks sections.

Testing this involves running dsymutil on pre-built binaries and object files, then running llvm-bcanalyzer on the final result to check for remarks.

[JDevlieghere]

llvm/tools/dsymutil/DwarfLinker.cpp

2554

I'm not sure if this comment is more helpful than saying "create the Remarks directory in the Resources directory", and below saying "append the file name, possibly followed by a dash and its architecture in case we're dealing with a fat file", or something like that :-)

2563

Maybe say that this is for fat binaries.

2565

This looks like you're iterating over something. If you go this route I think one is fine.

llvm/tools/dsymutil/LinkUtils.h

72

Doxygen supports "grouping" with @{ and @}. Can we create a group and specify that this is used for the remarks?

llvm/tools/dsymutil/Options.td

148

Please update cmdline.test with these new options.

[thegameg]

Address comments.

[thakis]

Tests are still broken on non-mac even after 86cdf74dc8. Maybe the space between REQUIRES and : in that change confuses lit. If you can't fix soon, please revert; the tree's been broken for several hours by now.

llvm/test/tools/dsymutil/Inputs/basic1.c

LTO compilation:
          clang -g -c -flto $FILE -o ${FILE%.c}-lto.o
       done
       clang basic1-lto.o basic2-lto.o basic3-lto.o -o basic-lto.macho.x86_64 -Wl,-object_path_lto,$PWD/basic-lto.macho.x86_64.o -Wl,-dead_strip
       rm basic1-lto.o basic2-lto.o basic3-lto.o
 
    Archive compilation (after basic compilation):
       ar -q libbasic.a basic2.macho.x86_64.o basic3.macho.x86_64.o
       clang basic1.macho.x86_64.o -lbasic -o basic-archive.macho.x86_64 -Wl,-dead_strip -L.
+
+   Remarks compilation:
+      for FILE in basic1.c basic2.c basic3.c; do
+         clang -gline-tables-only -c $FILE -fsave-optimization-record=bitstream -foptimization-remarks-file=/tmp/${FILE%.c}.macho.remarks.x86_64.opt.bitstream -mllvm -remarks-section -o ${FILE%.c}.macho.remarks.x86_64.o
+      done
+      clang basic1.macho.remarks.x86_64.o basic2.macho.remarks.x86_64.o basic3.macho.remarks.x86_64.o -o basic.macho.remarks.x86_64 -Wl,-dead_strip
+
 */
 
 int foo(int);
 
 int main(int argc, const char *argv[]) {
   return foo(argc);
 }

llvm/test/tools/dsymutil/Inputs/remarks/fat.macho.remarks.x86.c

+/* This file was used to generate:
+ * - fat.macho.remarks.x86.o
+ * - fat.macho.remarks.x86.opt.bitstream
+ * - fat.macho.remarks.x86
+ */
+
+/* for ARCH in x86_64 x86_64h i386
+ * do
+ *   clang -gline-tables-only -c fat.macho.remarks.x86.c -fsave-optimization-record=bitstream -foptimization-record-file=fat.macho.remarks."$ARCH".opt.bitstream -mllvm -remarks-section -arch "$ARCH"
+ * done
+ * lipo -create -output fat.macho.remarks.x86.o fat.macho.remarks.x86_64.o fat.macho.remarks.x86_64h.o fat.macho.remarks.i386.o
+ * clang -gline-tables-only fat.macho.remarks.x86.o -arch x86_64 -arch x86_64h -arch i386 -o fat.macho.remarks.x86
+ */
+int main(void) { return 0; }

llvm/test/tools/dsymutil/X86/basic-linking-bundle.test

 RUN: rm -rf %t
 RUN: mkdir -p %t/dsymdest
 RUN: cat %p/../Inputs/basic.macho.x86_64 > %t/basic.macho.x86_64
 
 RUN: dsymutil -oso-prepend-path=%p/.. %t/basic.macho.x86_64
 
 Check that the object file in the bundle exists and is sane:
 RUN: llvm-dwarfdump -a %t/basic.macho.x86_64.dSYM/Contents/Resources/DWARF/basic.macho.x86_64 | FileCheck %S/basic-linking-x86.test
 
+Check that we don't create an empty Remarks directory if there are no remarks.
+RUN: not ls %t/basic.macho.x86_64.dSYM/Contents/Resources/Remarks
+
 Check that llvm-dwarfdump -a recognizes the bundle as a dSYM:
 RUN: llvm-dwarfdump -a %t/basic.macho.x86_64.dSYM | FileCheck %S/basic-linking-x86.test
 
 RUN: FileCheck %s --input-file %t/basic.macho.x86_64.dSYM/Contents/Info.plist
 
 RUN: dsymutil -oso-prepend-path=%p/.. %t/basic.macho.x86_64 -o %t/dsymdest/basic.macho.x86_64.dSYM
 RUN: llvm-dwarfdump -a %t/dsymdest/basic.macho.x86_64.dSYM/Contents/Resources/DWARF/basic.macho.x86_64 | FileCheck %S/basic-linking-x86.test
 RUN: FileCheck %s --input-file %t/dsymdest/basic.macho.x86_64.dSYM/Contents/Info.plist

llvm/test/tools/dsymutil/X86/remarks-linking-bundle.test

+RUN: rm -rf %t
+RUN: mkdir -p %t
+RUN: cat %p/../Inputs/remarks/basic.macho.remarks.x86_64 > %t/basic.macho.remarks.x86_64
+
+RUN: dsymutil -oso-prepend-path=%p/../Inputs -remarks-prepend-path=%p/../Inputs %t/basic.macho.remarks.x86_64
+
+Check that the remark file in the bundle exists and is sane:
+RUN: llvm-bcanalyzer %t/basic.macho.remarks.x86_64.dSYM/Contents/Resources/Remarks/basic.macho.remarks.x86_64 | FileCheck %s
+
+Now emit it in a different format: YAML.
+RUN: dsymutil -remarks-output-format=yaml -oso-prepend-path=%p/../Inputs -remarks-prepend-path=%p/../Inputs %t/basic.macho.remarks.x86_64
+RUN: cat %t/basic.macho.remarks.x86_64.dSYM/Contents/Resources/Remarks/basic.macho.remarks.x86_64 | FileCheck %s --check-prefix=CHECK-YAML
+
+CHECK: <Meta
+CHECK: <Remark Num
+CHECK: <Remark Num
+CHECK: <Remark Num
+CHECK: <Remark Num
+CHECK: <Remark Num
+CHECK: <Remark Num
+CHECK: <Remark Num
+CHECK: <Remark Num
+CHECK: <Remark Num
+CHECK: <Remark Num
+CHECK: <Remark Num
+CHECK: <Remark Num
+CHECK: <Remark Num
+CHECK: <Remark Num
+CHECK: <Remark Num
+CHECK: <Remark Num
+CHECK: <Remark Num
+CHECK: <Remark Num
+CHECK: <Remark Num
+CHECK: <Remark Num
+CHECK: <Remark Num
+CHECK-NOT: <Remark Num
+
+CHECK-YAML:--- !Missed
+CHECK-YAML:--- !Missed
+CHECK-YAML:--- !Missed
+CHECK-YAML:--- !Missed
+CHECK-YAML:--- !Missed
+CHECK-YAML:--- !Missed
+CHECK-YAML:--- !Missed
+CHECK-YAML:--- !Analysis
+CHECK-YAML:--- !Analysis
+CHECK-YAML:--- !Analysis
+CHECK-YAML:--- !Analysis
+CHECK-YAML:--- !Analysis
+CHECK-YAML:--- !Analysis
+CHECK-YAML:--- !Analysis
+CHECK-YAML:--- !Analysis
+CHECK-YAML:--- !Analysis
+CHECK-YAML:--- !Analysis
+CHECK-YAML:--- !Analysis
+CHECK-YAML:--- !Analysis
+CHECK-YAML:--- !Analysis
+CHECK-YAML:--- !Analysis
+CHECK-NOT: --- !

llvm/test/tools/dsymutil/X86/remarks-linking-fat-bundle.test

+RUN: rm -rf %t
+RUN: mkdir -p %t
+RUN: cat %p/../Inputs/remarks/fat.macho.remarks.x86 > %t/fat.macho.remarks.x86
+
+RUN: dsymutil -oso-prepend-path=%p/../Inputs -remarks-prepend-path=%p/../Inputs %t/fat.macho.remarks.x86
+
+Check that the remark files in the bundle exist and are all sane:
+RUN: llvm-bcanalyzer %t/fat.macho.remarks.x86.dSYM/Contents/Resources/Remarks/fat.macho.remarks.x86-x86_64h | FileCheck %s
+RUN: llvm-bcanalyzer %t/fat.macho.remarks.x86.dSYM/Contents/Resources/Remarks/fat.macho.remarks.x86-x86_64 | FileCheck %s
+RUN: llvm-bcanalyzer %t/fat.macho.remarks.x86.dSYM/Contents/Resources/Remarks/fat.macho.remarks.x86-i386 | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-i386
+
+CHECK: <Meta
+CHECK: <Remark Num
+CHECK: <Remark Num
+CHECK-i386: <Remark Num
+CHECK-NOT: <Remark Num

llvm/test/tools/dsymutil/cmdline.test

 HELP: -minimize
 HELP: -no-odr
 HELP: -no-output
 HELP: -no-swiftmodule-timestamp
 HELP: -num-threads <threads>
 HELP: -oso-prepend-path <path>
 HELP: -o <filename>
 HELP: -papertrail
+HELP: -remarks-output-format <format>
+HELP: -remarks-prepend-path <path>
 HELP: -symbol-map
 HELP: -symtab
 HELP: -toolchain
 HELP: -update
 HELP: -verbose
 HELP: -verify
 HELP: -y
 HELP-NOT: -reverse-iterate

llvm/tools/dsymutil/CMakeLists.txt

 set(LLVM_TARGET_DEFINITIONS Options.td)
 tablegen(LLVM Options.inc -gen-opt-parser-defs)
 add_public_tablegen_target(DsymutilTableGen)
 
 set(LLVM_LINK_COMPONENTS
   AllTargetsAsmPrinters
   AllTargetsCodeGens
   AllTargetsDescs
   AllTargetsInfos
   AsmPrinter
   DebugInfoDWARF
   MC
   Object
   Option
+  Remarks
   Support
   Target
   )
 
 add_llvm_tool(dsymutil
   dsymutil.cpp
   BinaryHolder.cpp
   CFBundle.cpp

llvm/tools/dsymutil/DwarfLinker.cpp

 #include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/MC/MCSection.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/MC/MCTargetOptions.h"
 #include "llvm/Object/MachO.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Object/SymbolicFile.h"
+#include "llvm/Remarks/RemarkFormat.h"
+#include "llvm/Remarks/RemarkLinker.h"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/DJB.h"
 #include "llvm/Support/DataExtractor.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ErrorOr.h"
if ((EC = sys::fs::copy_file(InterfaceFile, Path.str())))
       warn(Twine("cannot copy parseable Swift interface ") +
            InterfaceFile + ": " +
            toString(errorCodeToError(EC)));
     Path.resize(BaseLength);
   }
   return Error::success();
 }
 
+static Error emitRemarks(const LinkOptions &Options, StringRef BinaryPath,
+                         StringRef ArchName,
+                         const remarks::RemarkLinker &RL) {
+  // Make sure we don't create the directories and the file if there is nothing
+  // to serialize.
+  if (RL.empty())
+    return Error::success();
+
+  SmallString<128> InputPath;
+  SmallString<128> Path;
+  // Create the "Remarks" directory in the "Resources" directory.
+  sys::path::append(Path, *Options.ResourceDir, "Remarks");
+  if (std::error_code EC = sys::fs::create_directories(Path.str(), true,
+                                                       sys::fs::perms::all_all))
+    return errorCodeToError(EC);
+
+  // Append the file name.
+  // For fat binaries, also append a dash and the architecture name.
+  sys::path::append(Path, sys::path::filename(BinaryPath));
+  if (Options.NumDebugMaps > 1) {
+    // More than one debug map means we have a fat binary.
+    Path += '-';
+    Path += ArchName;
+  }
+
+  std::error_code EC;
+  raw_fd_ostream OS(Options.NoOutput ? "-" : Path.str(), EC, sys::fs::OF_None);
+  if (EC)
+    return errorCodeToError(EC);
+
+  if (Error E = RL.serialize(OS, Options.RemarksFormat))
+    return E;
+
+  return Error::success();
+}
+
 bool DwarfLinker::link(const DebugMap &Map) {
   if (!createStreamer(Map.getTriple(), OutFile))
     return false;
 
   // Size of the DIEs (and headers) generated for the linked output.
   OutputDebugInfoSize = 0;
   // A unique ID that identifies each compile unit.
   unsigned UnitID = 0;
if (!Options.NoOutput) {
         break;
       case AccelTableKind::Default:
         llvm_unreachable("Default should have already been resolved.");
         break;
       }
     }
   };
 
+  remarks::RemarkLinker RL;
+  if (!Options.RemarksPrependPath.empty())
+    RL.setExternalFilePrependPath(Options.RemarksPrependPath);
+  auto RemarkLinkLambda = [&](size_t i) {
+    // Link remarks from one object file.
+    auto &LinkContext = ObjectContexts[i];
+    if (const object::ObjectFile *Obj = LinkContext.ObjectFile)
+      if (Error E = RL.link(*Obj))
+        return E;
+    return Error(Error::success());
+  };
+
   auto AnalyzeAll = [&]() {
     for (unsigned i = 0, e = NumObjects; i != e; ++i) {
       AnalyzeLambda(i);
 
       std::unique_lock<std::mutex> LockGuard(ProcessedFilesMutex);
       ProcessedFiles.set(i);
       ProcessedFilesConditionVariable.notify_one();
     }
for (unsigned i = 0, e = NumObjects; i != e; ++i) {
         }
       }
 
       CloneLambda(i);
     }
     EmitLambda();
   };
 
+  auto EmitRemarksLambda = [&]() {
+    StringRef ArchName = Map.getTriple().getArchName();
+    return emitRemarks(Options, Map.getBinaryPath(), ArchName, RL);
+  };
+
+  // Instead of making error handling a lot more complicated using futures,
+  // write to one llvm::Error instance if something went wrong.
+  // We're assuming RemarkLinkAllError is alive longer than the thread
+  // executing RemarkLinkAll.
+  auto RemarkLinkAll = [&](Error &RemarkLinkAllError) {
+    // Allow assigning to the error only within the lambda.
+    ErrorAsOutParameter EAO(&RemarkLinkAllError);
+    for (unsigned i = 0, e = NumObjects; i != e; ++i)
+      if ((RemarkLinkAllError = RemarkLinkLambda(i)))
+        return;
+
+    if ((RemarkLinkAllError = EmitRemarksLambda()))
+      return;
+  };
+
   // To limit memory usage in the single threaded case, analyze and clone are
   // run sequentially so the LinkContext is freed after processing each object
   // in endDebugObject.
   if (Options.Threads == 1) {
     for (unsigned i = 0, e = NumObjects; i != e; ++i) {
       AnalyzeLambda(i);
       CloneLambda(i);
+
+      if (Error E = RemarkLinkLambda(i))
+        return error(toString(std::move(E)));
     }
     EmitLambda();
+
+    if (Error E = EmitRemarksLambda())
+      return error(toString(std::move(E)));
+
   } else {
-    ThreadPool pool(2);
+    // This should not be constructed on the single-threaded path to avoid fatal
+    // errors from unchecked llvm::Error objects.
+    Error RemarkLinkAllError = Error::success();
+
+    ThreadPool pool(3);
     pool.async(AnalyzeAll);
     pool.async(CloneAll);
+    pool.async(RemarkLinkAll, std::ref(RemarkLinkAllError));
     pool.wait();
+
+    // Report errors from RemarkLinkAll, if any.
+    if (Error E = std::move(RemarkLinkAllError))
+      return error(toString(std::move(E)));
   }
 
   if (Options.NoOutput)
     return true;
 
   if (Options.ResourceDir && !ParseableSwiftInterfaces.empty()) {
     StringRef ArchName = Triple::getArchTypeName(Map.getTriple().getArch());
     if (auto E =

llvm/tools/dsymutil/LinkUtils.h

 //===- tools/dsymutil/LinkUtils.h - Dwarf linker utilities ------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_TOOLS_DSYMUTIL_LINKOPTIONS_H
 #define LLVM_TOOLS_DSYMUTIL_LINKOPTIONS_H
 
 #include "SymbolMap.h"
 
 #include "llvm/ADT/Twine.h"
+#include "llvm/Remarks/RemarkFormat.h"
 #include "llvm/Support/WithColor.h"
 
 #include <string>
 
 namespace llvm {
 namespace dsymutil {
 
 enum class OutputFileType {
struct LinkOptions {
   std::string PrependPath;
 
   /// The Resources directory in the .dSYM bundle.
   Optional<std::string> ResourceDir;
 
   /// Symbol map translator.
   SymbolMapTranslator Translator;
 
+  /// Fields used for linking and placing remarks into the .dSYM bundle.
+  /// @{
+
+  /// Number of debug maps processed in total.
+  unsigned NumDebugMaps = 0;
+
+  /// -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;
+
+  /// @}
+
   LinkOptions() = default;
 };
 
 inline void warn(Twine Warning, Twine Context = {}) {
   WithColor::warning() << Warning + "\n";
   if (!Context.isTriviallyEmpty())
     WithColor::note() << Twine("while processing ") + Context + "\n";
 }

llvm/tools/dsymutil/Options.td

 def threads: Separate<["--", "-"], "num-threads">,
   MetaVarName<"<threads>">,
   HelpText<"Specifies the maximum number of simultaneous threads to use when linking multiple architectures.">,
   Group<grp_general>;
 def: Separate<["-"], "j">,
   Alias<threads>,
   HelpText<"Alias for --num-threads">,
   Group<grp_general>;
+
+def remarks_prepend_path: Separate<["--", "-"], "remarks-prepend-path">,
+  MetaVarName<"<path>">,
+  HelpText<"Specify a directory to prepend to the paths of the external remark files.">,
+  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>;

llvm/tools/dsymutil/dsymutil.cpp

else
     Options.LinkOpts.Threads = thread::hardware_concurrency();
 
   if (Options.DumpDebugMap || Options.LinkOpts.Verbose)
     Options.LinkOpts.Threads = 1;
 
   if (getenv("RC_DEBUG_OPTIONS"))
     Options.PaperTrailWarnings = true;
 
+  if (opt::Arg *RemarksPrependPath = Args.getLastArg(OPT_remarks_prepend_path))
+    Options.LinkOpts.RemarksPrependPath = RemarksPrependPath->getValue();
+
+  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();
+  }
+
   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.
auto DebugMapPtrsOrErr =
                       Options.InputIsYAMLDebugMap);
 
     if (auto EC = DebugMapPtrsOrErr.getError()) {
       WithColor::error() << "cannot parse the debug map for '" << InputFile
                          << "': " << EC.message() << '\n';
       return 1;
     }
 
+    // Remember the number of debug maps that are being processed to decide how
+    // to name the remark files.
+    Options.LinkOpts.NumDebugMaps = DebugMapPtrsOrErr->size();
+
     if (Options.LinkOpts.Update) {
       // The debug map should be empty. Add one object file corresponding to
       // the input file.
       for (auto &Map : *DebugMapPtrsOrErr)
         Map->addDebugMapObject(InputFile,
                                sys::TimePoint<std::chrono::seconds>());
     }