[dsymutil] Apply recursion workaround for threading

[JDevlieghere]

Summary

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. We already have this
workaround for the architecture-related threading. This patch applies
the same workaround to the parallel analysis and cloning.

[davide]

I see you're also changing the default from 2 threads to 1 thread. Is this expected?

[friss]

llvm/tools/dsymutil/DwarfLinker.cpp

4304 ↗

(On Diff #140647)

There will never be more than 2 threads running from that pool, right? Why not leave a hardcoded '2' here?

[JDevlieghere]

Feedback Davide & Fred

[davide]

LGTM.

llvm/trunk/tools/dsymutil/DwarfLinker.cpp

for (LinkContext &LinkContext : ObjectContexts) {
     }
   }
 
   // If we haven't seen any CUs, pick an arbitrary valid Dwarf version anyway,
   // to be able to emit papertrail warnings.
   if (MaxDwarfVersion == 0)
     MaxDwarfVersion = 3;
 
-  ThreadPool pool(2);
-
   // These variables manage the list of processed object files.
   // The mutex and condition variable are to ensure that this is thread safe.
   std::mutex ProcessedFilesMutex;
   std::condition_variable ProcessedFilesConditionVariable;
   BitVector ProcessedFiles(NumObjects, false);
 
   // Now do analyzeContextInfo in parallel as it is particularly expensive.
-  pool.async([&]() {
+  auto AnalyzeLambda = [&]() {
     for (unsigned i = 0, e = NumObjects; i != e; ++i) {
       auto &LinkContext = ObjectContexts[i];
 
       if (!LinkContext.ObjectFile) {
         std::unique_lock<std::mutex> LockGuard(ProcessedFilesMutex);
         ProcessedFiles.set(i);
         ProcessedFilesConditionVariable.notify_one();
         continue;
       }
 
       // Now build the DIE parent links that we will use during the next phase.
       for (auto &CurrentUnit : LinkContext.CompileUnits)
         analyzeContextInfo(CurrentUnit->getOrigUnit().getUnitDIE(), 0,
                            *CurrentUnit, &ODRContexts.getRoot(),
                            UniquingStringPool, ODRContexts);
 
       std::unique_lock<std::mutex> LockGuard(ProcessedFilesMutex);
       ProcessedFiles.set(i);
       ProcessedFilesConditionVariable.notify_one();
     }
-  });
+  };
 
   // And then the remaining work in serial again.
   // Note, although this loop runs in serial, it can run in parallel with
   // the analyzeContextInfo loop so long as we process files with indices >=
   // than those processed by analyzeContextInfo.
-  pool.async([&]() {
+  auto CloneLambda = [&]() {
     for (unsigned i = 0, e = NumObjects; i != e; ++i) {
       {
         std::unique_lock<std::mutex> LockGuard(ProcessedFilesMutex);
         if (!ProcessedFiles[i]) {
           ProcessedFilesConditionVariable.wait(
               LockGuard, [&]() { return ProcessedFiles[i]; });
         }
       }
auto CloneLambda = [&]() {
     if (!Options.NoOutput) {
       Streamer->emitAbbrevs(Abbreviations, MaxDwarfVersion);
       Streamer->emitStrings(OffsetsStringPool);
       Streamer->emitAppleNames(AppleNames);
       Streamer->emitAppleNamespaces(AppleNamespaces);
       Streamer->emitAppleTypes(AppleTypes);
       Streamer->emitAppleObjc(AppleObjc);
     }
-  });
+  };
 
+  // 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 (Options.Threads == 1) {
+    AnalyzeLambda();
+    CloneLambda();
+  } else {
+    ThreadPool pool(2);
+    pool.async(AnalyzeLambda);
+    pool.async(CloneLambda);
     pool.wait();
+  }
 
   return Options.NoOutput ? true : Streamer->finish(Map);
 }
 
 bool linkDwarf(raw_fd_ostream &OutFile, const DebugMap &DM,
                const LinkOptions &Options) {
   DwarfLinker Linker(OutFile, Options);
   return Linker.link(DM);
 }
 
 } // namespace dsymutil
 } // namespace llvm

llvm/trunk/tools/dsymutil/dsymutil.h

struct LinkOptions {
   bool Update = false;
 
   /// Minimize
   bool Minimize = false;
 
   /// Do not check swiftmodule timestamp
   bool NoTimestamp = false;
 
+  /// Number of threads.
+  unsigned Threads = 1;
+
   /// -oso-prepend-path
   std::string PrependPath;
 
   LinkOptions() = default;
 };
 
 /// Extract the DebugMaps from the given file.
 /// The file has to be a MachO object file. Multiple debug maps can be

llvm/trunk/tools/dsymutil/dsymutil.cpp

if (Options.Update && std::find(InputFiles.begin(), InputFiles.end(), "-") !=
     // then we will want to consume it again in DwarfLinker. If we
     // used a unique BinaryHolder object that could cache multiple
     // binaries this restriction would go away.
     return make_error<StringError>(
         "standard input cannot be used as input for a dSYM update.",
         inconvertibleErrorCode());
   }
 
+  if (NumThreads == 0)
+    Options.Threads = llvm::thread::hardware_concurrency();
+  if (DumpDebugMap || Verbose)
+    Options.Threads = 1;
+
   return Options;
 }
 
 /// Return a list of input files. This function has logic for dealing with the
 /// special case where we might have dSYM bundles as input. The function
 /// returns an error when the directory structure doesn't match that of a dSYM
 /// bundle.
 static Expected<std::vector<std::string>> getInputs(bool DsymAsInput) {
for (auto &InputFile : *InputsOrErr) {
     }
 
     // Ensure that the debug map is not empty (anymore).
     if (DebugMapPtrsOrErr->empty()) {
       error_ostream() << "no architecture to link\n";
       return 1;
     }
 
-    if (NumThreads == 0)
-      NumThreads = llvm::thread::hardware_concurrency();
-    if (DumpDebugMap || Verbose)
-      NumThreads = 1;
-    NumThreads = std::min<unsigned>(NumThreads, DebugMapPtrsOrErr->size());
-
+    NumThreads =
+        std::min<unsigned>(OptionsOrErr->Threads, DebugMapPtrsOrErr->size());
     llvm::ThreadPool Threads(NumThreads);
 
     // If there is more than one link to execute, we need to generate
     // temporary files.
     bool NeedsTempFiles =
         !DumpDebugMap && (OutputFileOpt != "-") &&
         (DebugMapPtrsOrErr->size() != 1 || OptionsOrErr->Update);