[dsymutil] Implement the --threads option

[JDevlieghere]

Summary

This patch adds the --threads option to dsymutil to process architectures in parallel. The feature is already present in the version distributed with Xcode, but was not yet upstreamed.

This is NFC as far as the linking behavior is concerned. As threads are used automatically, the current tests cover the change in implementation.

[vsk]

tools/dsymutil/dsymutil.cpp

70 ↗

(On Diff #120526)

Could you check that dsymutil produces the same output for some test input with/without threading enabled?

330 ↗

(On Diff #120526)

If NumThreads isn't set, why not set it to hardware_concurrency()? Later, we can set NumThreads to the min of itself and the debug map size, since it seems like there's no need to spawn any more threads than that.

356 ↗

(On Diff #120526)

I think there's a chance that an exit from one thread can leave partially-written files from another thread on the FS. Maybe as a first step, we should migrate to ToolOutputFile?

365 ↗

(On Diff #120526)

Wdyt of wrapping the ThreadPool in a helper class that only constructs the pool if NumThreads > 1? It's not a big deal, but it's strange to me that we'd spawn/tear down a thread but never use it.

[aprantl]

tools/dsymutil/dsymutil.cpp

67 ↗

(On Diff #120526)

this mentions (n) but doesn't set the value_desc() to n.
It might be nice to add a test similar to the llvm-dwarfdump cmdline.test.

[JDevlieghere]

• Feedback @vsk
• Feedback @aprantl

tools/dsymutil/dsymutil.cpp

365 ↗

(On Diff #120526)

I'd prefer not to do this, as this should really only be a temporary workaround util the recursion stuff is fixed.

[jgravelle-google]

llvm/trunk/tools/dsymutil/dsymutil.cpp

66

For reasons I don't fully understand, this flag breaks the WebAssembly waterfall's build. https://wasm-stat.us/builders/linux/builds/25787

Our build uses -DLLVM_BUILD_LLVM_DYLIB=ON and -DLLVM_LINK_LLVM_DYLIB=ON, and we include lld, which my theory is then that both options are statically included in libLLVM, even though they should otherwise share no code. And that's problematic.
Setting those to OFF, and even using -DBUILD_SHARED_LIBS=ON instead causes no issues.

I'm not entirely sure what to do about that and I'm pretty sure this isn't the right place for it, but this might get more eyes involved.

[vsk]

llvm/trunk/tools/dsymutil/dsymutil.cpp

66

That's strange. llvm-cov and llvm-profdata both have a num-threads cl::opt, and we're not seeing the same issue there.

[aprantl]

Our build uses -DLLVM_BUILD_LLVM_DYLIB=ON and -DLLVM_LINK_LLVM_DYLIB=ON, and we include lld, which my theory is then that both options are statically included in libLLVM, even though they should otherwise share no code.

dsymutil is a tool — I'm surprised that it makes it into libLLVM at all. Could this be a bug in the CMake scripts?

[JDevlieghere]

Thanks Jacob for reporting this. I can indeed reproduce the problem locally by passing those flags to cmake. I'll have a look tomorrow and see what I can find out.

[JDevlieghere]

The conflicting option is defined in lib/LTO/ThinLTOCodeGenerator.cpp. I'm thinking we should just make llvm-dsymutil consistent with llvm-cov and llvm-profdata and rename the option to -num-threads and -j?

[aprantl]

How does the conflict manifest? The option registry shouldn't conflict because dsymutil's option is in a separate category. Or is it the alias that is conflicting? It would be good to understand this. Renaming the option would break compatibility for existing users of dsymutil, so we should avoid that.

[JDevlieghere]

In D39355#912745, @aprantl wrote:

How does the conflict manifest? The option registry shouldn't conflict because dsymutil's option is in a separate category. Or is it the alias that is conflicting? It would be good to understand this. Renaming the option would break compatibility for existing users of dsymutil, so we should avoid that.

It's definitely the threads option that conflict. Unfortunately you cannot have two options with the same name, even if they belong to a different category.

As discussed offline, I'll rename the options, add aliases for the internal version and mark them as deprecated/for compatibility.

llvm/trunk/test/tools/dsymutil/cmdline.test

+RUN: llvm-dsymutil -help 2>&1 | FileCheck --check-prefix=HELP %s
+HELP: OVERVIEW: manipulate archived DWARF debug symbol files.
+HELP: USAGE: llvm-dsymutil [options] <input files>
+HELP-NOT: -reverse-iterate
+HELP: Specific Options:
+HELP: -arch=<string>
+HELP: -dump-debug-map
+HELP: -flat
+HELP: -no-odr
+HELP: -no-output
+HELP: -no-swiftmodule-timestamp
+HELP: -o=<filename>
+HELP: -oso-prepend-path=<path>
+HELP: -symtab
+HELP: -threads=<n>
+HELP: -verbose
+HELP: -y
+HELP-NOT: -reverse-iterate
+
+RUN: llvm-dsymutil --version 2>&1 | FileCheck --check-prefix=VERSION %s
+VERSION: {{ version }}

llvm/trunk/tools/dsymutil/DwarfLinker.cpp

 #include "llvm/MC/MCObjectFileInfo.h"
 #include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/MC/MCTargetOptionsCommandFlags.h"
 #include "llvm/Object/MachO.h"
 #include "llvm/Support/LEB128.h"
 #include "llvm/Support/TargetRegistry.h"
+#include "llvm/Support/ToolOutputFile.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Target/TargetOptions.h"
 #include <memory>
 #include <string>
 #include <tuple>
 
 namespace llvm {
 namespace dsymutil {
class DwarfStreamer {
   std::unique_ptr<MCSubtargetInfo> MSTI;
   MCCodeEmitter *MCE; // Owned by MCStreamer
   MCStreamer *MS;     // Owned by AsmPrinter
   std::unique_ptr<TargetMachine> TM;
   std::unique_ptr<AsmPrinter> Asm;
   /// @}
 
   /// The file we stream the linked Dwarf to.
-  std::unique_ptr<raw_fd_ostream> OutFile;
+  std::unique_ptr<ToolOutputFile> OutFile;
 
   uint32_t RangesSectionSize;
   uint32_t LocSectionSize;
   uint32_t LineSectionSize;
   uint32_t FrameSectionSize;
 
   /// Emit the pubnames or pubtypes section contribution for \p
   /// Unit into \p Sec. The data is provided in \p Names.
bool DwarfStreamer::init(Triple TheTriple, StringRef OutputFilename) {
 
   MCE = TheTarget->createMCCodeEmitter(*MII, *MRI, *MC);
   if (!MCE)
     return error("no code emitter for target " + TripleName, Context);
 
   // Create the output file.
   std::error_code EC;
   OutFile =
-      llvm::make_unique<raw_fd_ostream>(OutputFilename, EC, sys::fs::F_None);
+      llvm::make_unique<ToolOutputFile>(OutputFilename, EC, sys::fs::F_None);
   if (EC)
     return error(Twine(OutputFilename) + ": " + EC.message(), Context);
 
   MCTargetOptions MCOptions = InitMCTargetOptionsFromFlags();
   MS = TheTarget->createMCObjectStreamer(
-      TheTriple, *MC, std::unique_ptr<MCAsmBackend>(MAB), *OutFile,
+      TheTriple, *MC, std::unique_ptr<MCAsmBackend>(MAB), OutFile->os(),
       std::unique_ptr<MCCodeEmitter>(MCE), *MSTI, MCOptions.MCRelaxAll,
       MCOptions.MCIncrementalLinkerCompatible,
       /*DWARFMustBeAtTheEnd*/ false);
   if (!MS)
     return error("no object streamer for target " + TripleName, Context);
 
   // Finally create the AsmPrinter we'll use to emit the DIEs.
   TM.reset(TheTarget->createTargetMachine(TripleName, "", "", TargetOptions(),
bool DwarfStreamer::init(Triple TheTriple, StringRef OutputFilename) {
   LocSectionSize = 0;
   LineSectionSize = 0;
   FrameSectionSize = 0;
 
   return true;
 }
 
 bool DwarfStreamer::finish(const DebugMap &DM) {
+  bool Result = true;
   if (DM.getTriple().isOSDarwin() && !DM.getBinaryPath().empty())
-    return MachOUtils::generateDsymCompanion(DM, *MS, *OutFile);
-
+    Result = MachOUtils::generateDsymCompanion(DM, *MS, OutFile->os());
+  else
     MS->Finish();
-  return true;
+
+  // Declare success.
+  OutFile->keep();
+
+  return Result;
 }
 
 /// Set the current output section to debug_info and change
 /// the MC Dwarf version to \p DwarfVersion.
 void DwarfStreamer::switchToDebugInfoSection(unsigned DwarfVersion) {
   MS->SwitchSection(MOFI->getDwarfInfoSection());
   MC->setDwarfVersion(DwarfVersion);
 }

llvm/trunk/tools/dsymutil/dsymutil.cpp

 //===-- dsymutil.cpp - Debug info dumping utility for llvm ----------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This program is a utility that aims to be a dropin replacement for
 // Darwin's dsymutil.
 //
 //===----------------------------------------------------------------------===//
 
+#include "dsymutil.h"
 #include "DebugMap.h"
 #include "MachOUtils.h"
-#include "dsymutil.h"
 #include "llvm/Object/MachO.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/Options.h"
 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/Signals.h"
-#include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/TargetSelect.h"
+#include "llvm/Support/ThreadPool.h"
+#include "llvm/Support/raw_ostream.h"
 #include <cstdint>
 #include <string>
 
 using namespace llvm::dsymutil;
 
 namespace {
 using namespace llvm::cl;
 
static opt<bool> DumpStab(
     init(false), cat(DsymCategory));
 static alias DumpStabA("s", desc("Alias for --symtab"), aliasopt(DumpStab));
 
 static opt<bool> FlatOut("flat",
                          desc("Produce a flat dSYM file (not a bundle)."),
                          init(false), cat(DsymCategory));
 static alias FlatOutA("f", desc("Alias for --flat"), aliasopt(FlatOut));
 
+static opt<unsigned> Threads(
+    "threads",
+    desc("Specifies the maximum number (n) of simultaneous threads to use\n"
+         "when linking multiple architectures."),
+    value_desc("n"), init(0), cat(DsymCategory));
+static alias ThreadsA("t", desc("Alias for --threads"), aliasopt(Threads));
+
 static opt<bool> Verbose("verbose", desc("Verbosity level"), init(false),
                          cat(DsymCategory));
 
 static opt<bool>
     NoOutput("no-output",
              desc("Do the link in memory, but do not emit the result file."),
              init(false), cat(DsymCategory));
 static opt<bool>
if (auto EC = DebugMapPtrsOrErr.getError()) {
       exitDsymutil(1);
     }
 
     if (DebugMapPtrsOrErr->empty()) {
       llvm::errs() << "error: no architecture to link\n";
       exitDsymutil(1);
     }
 
+    unsigned NumThreads = Threads;
+    if (!NumThreads)
+      NumThreads = llvm::thread::hardware_concurrency();
+    if (DumpDebugMap || Verbose)
+      NumThreads = 1;
+    NumThreads = std::min(NumThreads, (unsigned)DebugMapPtrsOrErr->size());
+
+    llvm::ThreadPool Threads(NumThreads);
+
     // If there is more than one link to execute, we need to generate
     // temporary files.
     bool NeedsTempFiles = !DumpDebugMap && (*DebugMapPtrsOrErr).size() != 1;
     llvm::SmallVector<MachOUtils::ArchAndFilename, 4> TempFiles;
     for (auto &Map : *DebugMapPtrsOrErr) {
       if (Verbose || DumpDebugMap)
         Map->print(llvm::outs());
 
       if (DumpDebugMap)
         continue;
 
       if (Map->begin() == Map->end())
         llvm::errs() << "warning: no debug symbols in executable (-arch "
                      << MachOUtils::getArchName(Map->getTriple().getArchName())
                      << ")\n";
 
       std::string OutputFile = getOutputFileName(InputFile, NeedsTempFiles);
+
+      auto LinkLambda = [OutputFile, Options, &Map]() {
         if (OutputFile.empty() || !linkDwarf(OutputFile, *Map, Options))
           exitDsymutil(1);
+      };
+
+      // FIXME: The DwarfLinker can have some very deep recursion that can max
+      // out the (significantly smaller) stack when using threads. We don't
+      // want this limitation when we only have a single thread.
+      if (NumThreads == 1)
+        LinkLambda();
+      else
+        Threads.async(LinkLambda);
 
       if (NeedsTempFiles)
         TempFiles.emplace_back(Map->getTriple().getArchName().str(),
                                OutputFile);
     }
 
+    Threads.wait();
+
     if (NeedsTempFiles &&
         !MachOUtils::generateUniversalBinary(
             TempFiles, getOutputFileName(InputFile), Options, SDKPath))
       exitDsymutil(1);
   }
 
   exitDsymutil(0);
 }