[dsymutil] Re-enable threading

[JDevlieghere]

Summary

Threading was disabled in r317263 because it broke a test in combination
with -DLLVM_ENABLE_THREADS=OFF. This was because a ThreadPool warning
was piped to llvm-dwarfdump which was expecting to read an object from
stdin.

This patch re-enables threading and fixes the offending test.
Unfortunately this required more than just moving the ThreadPool out of
the for loop because of the TempFile refactoring that took place in the
meantime.

[JDevlieghere]

• Improve explanation for using std::shared_ptr

[aprantl]

seems good to me but I'm not very familiar with Threadpool.

test/tools/dsymutil/ARM/fat-threading.test

3 ↗

(On Diff #126760)

Can you add a comment explaining what is being tested here? It's non-obvious because the a casual reader might not know that we spawn one thread per arch.

tools/dsymutil/dsymutil.cpp

414 ↗

(On Diff #126760)

remove extra {

[davide]

test/tools/dsymutil/X86/alias.test

2 ↗

(On Diff #126760)

do you really need to use a relative path here?
Also, do you need -o - even thoughh you're piping to another tool?

tools/dsymutil/dsymutil.cpp

416 ↗

(On Diff #126760)

you don't need the braces around the else as well.

[JDevlieghere]

I didn't see you had requested changes between seeing Adrian's accept and actually landing it. I'm happy to address this in a follow-up commit!

test/tools/dsymutil/X86/alias.test

2 ↗

(On Diff #126760)

How else would you specify the path?

Omitting -o - would not pipe the output to stdout but rather create inputfile.dwarf.

[davide]

In D41180#954071, @JDevlieghere wrote:

I didn't see you had requested changes between seeing Adrian's accept and actually landing it. I'm happy to address this in a follow-up commit!

I see, I think you already addressed my issue, no worries. Thanks!

llvm/trunk/test/tools/dsymutil/ARM/fat-threading.test

+# REQUIRES: object-emission
+# By default, dsymutil spawns one thread per architecture and this test just
+# ensures that things don't break when processing multiple archs.
+# RUN: llvm-dsymutil -no-output %p/../Inputs/fat-test.arm.dylib -o %t.brief.dSYM 2>&1
+# RUN: llvm-dsymutil -verify %p/../Inputs/fat-test.arm.dylib -o %t.verify.dSYM 2>&1
+
+# CHECK: warning: no debug symbols in executable (-arch armv7)
+# CHECK: warning: no debug symbols in executable (-arch armv7s)
+# CHECK: warning: no debug symbols in executable (-arch arm64)
+
+# CHECK: Running lipo
+# CHECK-NEXT: lipo -create
+# CHECK-SAME: -segalign armv7
+# CHECK-SAME: -segalign armv7s
+# CHECK-SAME: -segalign arm64

llvm/trunk/test/tools/dsymutil/X86/alias.test

 # RUN: llvm-dsymutil -f -oso-prepend-path=%p/../Inputs/alias \
-# RUN: %p/../Inputs/alias/foobar -o - 2>&1 | llvm-dwarfdump - | FileCheck %s
+# RUN: %p/../Inputs/alias/foobar -o - | llvm-dwarfdump - 2>&1 | FileCheck %s
 # CHECK-NOT: could not find object file symbol for symbol
 # CHECK: DW_AT_name ("foo.c")
 # CHECK: DW_AT_name ("bar.c")
 
 # Source:
 #   $ cat foo.c
 #   int foo = 1;
 #   $ cat bar.c

llvm/trunk/tools/dsymutil/dsymutil.cpp

NoTimestamp("no-swiftmodule-timestamp",
                 desc("Don't check timestamp for swiftmodule files."),
                 init(false), cat(DsymCategory));
 
 static list<std::string> ArchFlags(
     "arch",
     desc("Link DWARF debug information only for specified CPU architecture\n"
          "types. This option can be specified multiple times, once for each\n"
          "desired architecture. All CPU architectures will be linked by\n"
-         "default."), value_desc("arch"),
-    ZeroOrMore, cat(DsymCategory));
+         "default."),
+    value_desc("arch"), ZeroOrMore, cat(DsymCategory));
 
 static opt<bool>
     NoODR("no-odr",
           desc("Do not use ODR (One Definition Rule) for type uniquing."),
           init(false), cat(DsymCategory));
 
 static opt<bool> DumpDebugMap(
     "dump-debug-map",
static bool verify(llvm::StringRef OutputFile, llvm::StringRef Arch) {
   Binary &Binary = *BinOrErr.get().getBinary();
   if (auto *Obj = dyn_cast<MachOObjectFile>(&Binary)) {
     raw_ostream &os = Verbose ? errs() : nulls();
     os << "Verifying DWARF for architecture: " << Arch << "\n";
     std::unique_ptr<DWARFContext> DICtx = DWARFContext::create(*Obj);
     DIDumpOptions DumpOpts;
     bool success = DICtx->verify(os, DumpOpts.noImplicitRecursion());
     if (!success)
-        errs() << "error: verification failed for " << Arch << '\n';
     return success;
   }
 
   return false;
 }
 
 static std::string getOutputFileName(llvm::StringRef InputFile) {
   if (FlatOut) {
for (auto &InputFile : InputFiles) {
     }
 
     if (NumThreads == 0)
       NumThreads = llvm::thread::hardware_concurrency();
     if (DumpDebugMap || Verbose)
       NumThreads = 1;
     NumThreads = std::min<unsigned>(NumThreads, 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;
     TempFileVector TempFileStore;
+    std::atomic_char AllOK(1);
     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";
 
+      // Using a std::shared_ptr rather than std::unique_ptr because move-only
+      // types don't work with std::bind in the ThreadPool implementation.
+      std::shared_ptr<raw_fd_ostream> OS;
       std::string OutputFile = getOutputFileName(InputFile);
-      std::unique_ptr<raw_fd_ostream> OS;
       if (NeedsTempFiles) {
         Expected<sys::fs::TempFile> T = createTempFile();
         if (!T) {
           errs() << toString(T.takeError());
           return 1;
         }
-        OS = llvm::make_unique<raw_fd_ostream>(T->FD, /*shouldClose*/ false);
+        OS = std::make_shared<raw_fd_ostream>(T->FD, /*shouldClose*/ false);
         OutputFile = T->TmpName;
         TempFileStore.Files.push_back(std::move(*T));
+        TempFiles.emplace_back(Map->getTriple().getArchName().str(),
+                               OutputFile);
       } else {
         std::error_code EC;
-        OS = llvm::make_unique<raw_fd_ostream>(NoOutput ? "-" : OutputFile, EC,
+        OS = std::make_shared<raw_fd_ostream>(NoOutput ? "-" : OutputFile, EC,
                                               sys::fs::F_None);
         if (EC) {
           errs() << OutputFile << ": " << EC.message();
           return 1;
         }
       }
 
-      std::atomic_char AllOK(1);
-      auto LinkLambda = [&]() {
-        AllOK.fetch_and(linkDwarf(*OS, *Map, Options));
-        OS->flush();
+      auto LinkLambda = [&,
+                         OutputFile](std::shared_ptr<raw_fd_ostream> Stream) {
+        AllOK.fetch_and(linkDwarf(*Stream, *Map, Options));
+        Stream->flush();
+        if (Verify && !NoOutput)
+          AllOK.fetch_and(verify(OutputFile, Map->getTriple().getArchName()));
       };
 
       // 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 {
-        llvm::ThreadPool Threads(NumThreads);
-        Threads.async(LinkLambda);
-        Threads.wait();
-      }
-      if (!AllOK)
-        return 1;
-
-      if (Verify && !NoOutput &&
-          !verify(OutputFile, Map->getTriple().getArchName()))
-        return 1;
+      if (NumThreads == 1)
+        LinkLambda(OS);
+      else
+        Threads.async(LinkLambda, OS);
 
-      if (NeedsTempFiles)
-        TempFiles.emplace_back(Map->getTriple().getArchName().str(),
-                               OutputFile);
-    }
+    Threads.wait();
 
+    if (!AllOK)
+      return 1;
 
     if (NeedsTempFiles &&
         !MachOUtils::generateUniversalBinary(
             TempFiles, getOutputFileName(InputFile), Options, SDKPath))
       return 1;
   }
 
   return 0;
 }