[lldb] Move copying of files into reproducer out of process

For performance reasons the reproducers don't copy the files captured by
the file collector eagerly, but wait until the reproducer needs to be
generated.

This is a problematic when LLDB crashes and we have to do all this
signal-unsafe work in the signal handler. This patch uses a similar
trick to clang, which has the driver invoke a new cc1 instance to do all
this work out-of-process.

This patch moves the writing of the mapping file as well as copying over
the reproducers into a separate process spawned when lldb crashes.

Differential revision: https://reviews.llvm.org/D89600

lldb/include/lldb/API/SBReproducer.h

@@ -48,6 +48,7 @@ public:
   static const char *Replay(const char *path, bool skip_version_check);
   static const char *Replay(const char *path, const SBReplayOptions &options);
   static const char *PassiveReplay(const char *path);
+  static const char *Finalize(const char *path);
   static const char *GetPath();
   static bool SetAutoGenerate(bool b);
   static bool Generate();

lldb/include/lldb/Host/FileSystem.h

@@ -34,7 +34,7 @@ public:
   FileSystem()
       : m_fs(llvm::vfs::getRealFileSystem()), m_collector(nullptr),
         m_home_directory(), m_mapped(false) {}
-  FileSystem(std::shared_ptr<llvm::FileCollector> collector)
+  FileSystem(std::shared_ptr<llvm::FileCollectorBase> collector)
       : m_fs(llvm::vfs::getRealFileSystem()), m_collector(std::move(collector)),
         m_home_directory(), m_mapped(false) {}
   FileSystem(llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> fs,
@@ -48,7 +48,7 @@ public:
   static FileSystem &Instance();
 
   static void Initialize();
-  static void Initialize(std::shared_ptr<llvm::FileCollector> collector);
+  static void Initialize(std::shared_ptr<llvm::FileCollectorBase> collector);
   static llvm::Error Initialize(const FileSpec &mapping);
   static void Initialize(llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> fs);
   static void Terminate();
@@ -199,7 +199,7 @@ public:
 private:
   static llvm::Optional<FileSystem> &InstanceImpl();
   llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> m_fs;
-  std::shared_ptr<llvm::FileCollector> m_collector;
+  std::shared_ptr<llvm::FileCollectorBase> m_collector;
   std::string m_home_directory;
   bool m_mapped;
 };

lldb/include/lldb/Utility/Reproducer.h

@@ -243,6 +243,9 @@ struct ReplayOptions {
   bool check_version = true;
 };
 
+llvm::Error Finalize(Loader *loader);
+llvm::Error Finalize(const FileSpec &root);
+
 } // namespace repro
 } // namespace lldb_private
 

lldb/include/lldb/Utility/ReproducerProvider.h

@@ -90,6 +90,23 @@ public:
   }
 };
 
+class FlushingFileCollector : public llvm::FileCollectorBase {
+public:
+  FlushingFileCollector(llvm::StringRef files_path, llvm::StringRef dirs_path,
+                        std::error_code &ec);
+
+protected:
+  void addFileImpl(llvm::StringRef file) override;
+
+  llvm::vfs::directory_iterator
+  addDirectoryImpl(const llvm::Twine &dir,
+                   llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> vfs,
+                   std::error_code &dir_ec) override;
+
+  llvm::Optional<llvm::raw_fd_ostream> m_files_os;
+  llvm::Optional<llvm::raw_fd_ostream> m_dirs_os;
+};
+
 class FileProvider : public Provider<FileProvider> {
 public:
   struct Info {
@@ -97,31 +114,26 @@ public:
     static const char *file;
   };
 
-  FileProvider(const FileSpec &directory)
-      : Provider(directory),
-        m_collector(std::make_shared<llvm::FileCollector>(
-            directory.CopyByAppendingPathComponent("root").GetPath(),
-            directory.GetPath())) {}
+  FileProvider(const FileSpec &directory) : Provider(directory) {
+    std::error_code ec;
+    m_collector = std::make_shared<FlushingFileCollector>(
+        directory.CopyByAppendingPathComponent("files.txt").GetPath(),
+        directory.CopyByAppendingPathComponent("dirs.txt").GetPath(), ec);
+    if (ec)
+      m_collector.reset();
+  }
 
-  std::shared_ptr<llvm::FileCollector> GetFileCollector() {
+  std::shared_ptr<llvm::FileCollectorBase> GetFileCollector() {
     return m_collector;
   }
 
   void RecordInterestingDirectory(const llvm::Twine &dir);
   void RecordInterestingDirectoryRecursive(const llvm::Twine &dir);
 
-  void Keep() override {
-    auto mapping = GetRoot().CopyByAppendingPathComponent(Info::file);
-    // Temporary files that are removed during execution can cause copy errors.
-    if (auto ec = m_collector->copyFiles(/*stop_on_error=*/false))
-      return;
-    m_collector->writeMapping(mapping.GetPath());
-  }
-
   static char ID;
 
 private:
-  std::shared_ptr<llvm::FileCollector> m_collector;
+  std::shared_ptr<FlushingFileCollector> m_collector;
 };
 
 /// Provider for the LLDB version number.

lldb/source/API/SBReproducer.cpp

@@ -8,7 +8,6 @@
 
 #include "SBReproducerPrivate.h"
 
-#include "SBReproducerPrivate.h"
 #include "lldb/API/LLDB.h"
 #include "lldb/API/SBAddress.h"
 #include "lldb/API/SBAttachInfo.h"
@@ -266,6 +265,27 @@ const char *SBReproducer::Replay(const char *path,
   return nullptr;
 }
 
+const char *SBReproducer::Finalize(const char *path) {
+  static std::string error;
+  if (auto e = Reproducer::Initialize(ReproducerMode::Replay, FileSpec(path))) {
+    error = llvm::toString(std::move(e));
+    return error.c_str();
+  }
+
+  repro::Loader *loader = repro::Reproducer::Instance().GetLoader();
+  if (!loader) {
+    error = "unable to get replay loader.";
+    return error.c_str();
+  }
+
+  if (auto e = repro::Finalize(loader)) {
+    error = llvm::toString(std::move(e));
+    return error.c_str();
+  }
+
+  return nullptr;
+}
+
 bool SBReproducer::Generate() {
   auto &r = Reproducer::Instance();
   if (auto generator = r.GetGenerator()) {
@@ -285,10 +305,11 @@ bool SBReproducer::SetAutoGenerate(bool b) {
 }
 
 const char *SBReproducer::GetPath() {
-  static std::string path;
+  ConstString path;
   auto &r = Reproducer::Instance();
-  path = r.GetReproducerPath().GetCString();
-  return path.c_str();
+  if (FileSpec reproducer_path = Reproducer::Instance().GetReproducerPath())
+    path = ConstString(r.GetReproducerPath().GetCString());
+  return path.GetCString();
 }
 
 void SBReproducer::SetWorkingDirectory(const char *path) {

lldb/source/Commands/CommandObjectReproducer.cpp

@@ -192,6 +192,10 @@ protected:
     auto &r = Reproducer::Instance();
     if (auto generator = r.GetGenerator()) {
       generator->Keep();
+      if (llvm::Error e = repro::Finalize(r.GetReproducerPath())) {
+        SetError(result, std::move(e));
+        return result.Succeeded();
+      }
     } else if (r.IsReplaying()) {
       // Make this operation a NO-OP in replay mode.
       result.SetStatus(eReturnStatusSuccessFinishNoResult);

lldb/source/Host/common/FileSystem.cpp

@@ -49,7 +49,7 @@ void FileSystem::Initialize() {
   InstanceImpl().emplace();
 }
 
-void FileSystem::Initialize(std::shared_ptr<FileCollector> collector) {
+void FileSystem::Initialize(std::shared_ptr<FileCollectorBase> collector) {
   lldbassert(!InstanceImpl() && "Already initialized.");
   InstanceImpl().emplace(collector);
 }

lldb/source/Plugins/ExpressionParser/Clang/ModuleDependencyCollector.h

@@ -18,7 +18,7 @@ class ModuleDependencyCollectorAdaptor
     : public clang::ModuleDependencyCollector {
 public:
   ModuleDependencyCollectorAdaptor(
-      std::shared_ptr<llvm::FileCollector> file_collector)
+      std::shared_ptr<llvm::FileCollectorBase> file_collector)
       : clang::ModuleDependencyCollector(""), m_file_collector(file_collector) {
   }
 
@@ -33,7 +33,7 @@ public:
   void writeFileMap() override {}
 
 private:
-  std::shared_ptr<llvm::FileCollector> m_file_collector;
+  std::shared_ptr<llvm::FileCollectorBase> m_file_collector;
 };
 } // namespace lldb_private
 

lldb/source/Utility/Reproducer.cpp

@@ -176,10 +176,12 @@ Generator::Generator(FileSpec root) : m_root(MakeAbsolute(std::move(root))) {
 
 Generator::~Generator() {
   if (!m_done) {
-    if (m_auto_generate)
+    if (m_auto_generate) {
       Keep();
-    else
+      llvm::cantFail(Finalize(GetRoot()));
+    } else {
       Discard();
+    }
   }
 }
 
@@ -359,3 +361,58 @@ void Verifier::Verify(
     }
   }
 }
+
+static llvm::Error addPaths(StringRef path,
+                            function_ref<void(StringRef)> callback) {
+  auto buffer = llvm::MemoryBuffer::getFile(path);
+  if (!buffer)
+    return errorCodeToError(buffer.getError());
+
+  SmallVector<StringRef, 0> paths;
+  (*buffer)->getBuffer().split(paths, '\0');
+  for (StringRef p : paths) {
+    if (!p.empty())
+      callback(p);
+  }
+
+  return errorCodeToError(llvm::sys::fs::remove(path));
+}
+
+llvm::Error repro::Finalize(Loader *loader) {
+  if (!loader)
+    return make_error<StringError>("invalid loader",
+                                   llvm::inconvertibleErrorCode());
+
+  FileSpec reproducer_root = loader->GetRoot();
+  std::string files_path =
+      reproducer_root.CopyByAppendingPathComponent("files.txt").GetPath();
+  std::string dirs_path =
+      reproducer_root.CopyByAppendingPathComponent("dirs.txt").GetPath();
+
+  FileCollector collector(
+      reproducer_root.CopyByAppendingPathComponent("root").GetPath(),
+      reproducer_root.GetPath());
+
+  if (Error e =
+          addPaths(files_path, [&](StringRef p) { collector.addFile(p); }))
+    return e;
+
+  if (Error e =
+          addPaths(dirs_path, [&](StringRef p) { collector.addDirectory(p); }))
+    return e;
+
+  FileSpec mapping =
+      reproducer_root.CopyByAppendingPathComponent(FileProvider::Info::file);
+  if (auto ec = collector.copyFiles(/*stop_on_error=*/false))
+    return errorCodeToError(ec);
+  collector.writeMapping(mapping.GetPath());
+
+  return llvm::Error::success();
+}
+
+llvm::Error repro::Finalize(const FileSpec &root) {
+  Loader loader(root);
+  if (Error e = loader.LoadIndex())
+    return e;
+  return Finalize(&loader);
+}

lldb/source/Utility/ReproducerProvider.cpp

@@ -8,6 +8,7 @@
 
 #include "lldb/Utility/ReproducerProvider.h"
 #include "lldb/Utility/ProcessInfo.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
@@ -44,6 +45,40 @@ void VersionProvider::Keep() {
   os << m_version << "\n";
 }
 
+FlushingFileCollector::FlushingFileCollector(llvm::StringRef files_path,
+                                             llvm::StringRef dirs_path,
+                                             std::error_code &ec) {
+  auto clear = llvm::make_scope_exit([this]() {
+    m_files_os.reset();
+    m_dirs_os.reset();
+  });
+  m_files_os.emplace(files_path, ec, llvm::sys::fs::OF_Append);
+  if (ec)
+    return;
+  m_dirs_os.emplace(dirs_path, ec, llvm::sys::fs::OF_Append);
+  if (ec)
+    return;
+  clear.release();
+}
+
+void FlushingFileCollector::addFileImpl(StringRef file) {
+  if (m_files_os) {
+    *m_files_os << file << '\0';
+    m_files_os->flush();
+  }
+}
+
+llvm::vfs::directory_iterator
+FlushingFileCollector::addDirectoryImpl(const Twine &dir,
+                                        IntrusiveRefCntPtr<vfs::FileSystem> vfs,
+                                        std::error_code &dir_ec) {
+  if (m_dirs_os) {
+    *m_dirs_os << dir << '\0';
+    m_dirs_os->flush();
+  }
+  return vfs->dir_begin(dir, dir_ec);
+}
+
 void FileProvider::RecordInterestingDirectory(const llvm::Twine &dir) {
   if (m_collector)
     m_collector->addFile(dir);

lldb/test/Shell/Reproducer/TestCaptureEnvOverride.test

@@ -9,8 +9,6 @@
 
 # RUN: LLDB_CAPTURE_REPRODUCER=0 %lldb -b -o 'reproducer status' --capture --capture-path %t.repro | FileCheck %s --check-prefix OFF
 # RUN: LLDB_CAPTURE_REPRODUCER=0 %lldb -b -o 'reproducer status' --capture | FileCheck %s --check-prefix OFF
-# RUN: LLDB_CAPTURE_REPRODUCER=OFF %lldb -b -o 'reproducer status' --capture --capture-path %t.repro | FileCheck %s --check-prefix OFF
-# RUN: LLDB_CAPTURE_REPRODUCER=off %lldb -b -o 'reproducer status' --capture | FileCheck %s --check-prefix OFF
 
 # RUN: LLDB_CAPTURE_REPRODUCER=bogus %lldb -b -o 'reproducer status' --capture | FileCheck %s --check-prefix CAPTURE
 # RUN: LLDB_CAPTURE_REPRODUCER=bogus %lldb -b -o 'reproducer status' | FileCheck %s --check-prefix OFF

lldb/test/Shell/Reproducer/TestFinalize.test

@@ -0,0 +1,14 @@
+# RUN: mkdir -p %t.repro
+# RUN: touch %t.known.file
+# RUN: mkdir -p %t.known.dir
+# RUN: touch %t.repro/index.yaml
+# RUN: echo -n "%t.known.file" > %t.repro/files.txt
+# RUN: echo -n "%t.known.dir" > %t.repro/dirs.txt
+
+# RUN: %lldb --reproducer-finalize %t.repro 2>&1 | FileCheck %s
+# CHECK-NOT: error
+# CHECK: Reproducer written to
+
+# RUN: echo "CHECK-DAG: %t.known.file" > %t.filecheck
+# RUN: echo "CHECK-DAG %t.known.dir" >> %t.filecheck
+# RUN: cat %t.repro/files.yaml | FileCheck %t.filecheck

lldb/tools/driver/Driver.cpp

@@ -729,25 +729,10 @@ void sigcont_handler(int signo) {
   signal(signo, sigcont_handler);
 }
 
-void reproducer_handler(void *argv0) {
+void reproducer_handler(void *finalize_cmd) {
   if (SBReproducer::Generate()) {
-    auto exe = static_cast<const char *>(argv0);
-    llvm::outs() << "********************\n";
-    llvm::outs() << "Crash reproducer for ";
-    llvm::outs() << lldb::SBDebugger::GetVersionString() << '\n';
-    llvm::outs() << '\n';
-    llvm::outs() << "Reproducer written to '" << SBReproducer::GetPath()
-                 << "'\n";
-    llvm::outs() << '\n';
-    llvm::outs() << "Before attaching the reproducer to a bug report:\n";
-    llvm::outs() << " - Look at the directory to ensure you're willing to "
-                    "share its content.\n";
-    llvm::outs()
-        << " - Make sure the reproducer works by replaying the reproducer.\n";
-    llvm::outs() << '\n';
-    llvm::outs() << "Replay the reproducer with the following command:\n";
-    llvm::outs() << exe << " -replay " << SBReproducer::GetPath() << "\n";
-    llvm::outs() << "********************\n";
+    std::system(static_cast<const char *>(finalize_cmd));
+    fflush(stdout);
   }
 }
 
@@ -799,6 +784,31 @@ EXAMPLES:
 
 llvm::Optional<int> InitializeReproducer(llvm::StringRef argv0,
                                          opt::InputArgList &input_args) {
+  if (auto *finalize_path = input_args.getLastArg(OPT_reproducer_finalize)) {
+    if (const char *error = SBReproducer::Finalize(finalize_path->getValue())) {
+      WithColor::error() << "reproducer finalization failed: " << error << '\n';
+      return 1;
+    }
+
+    llvm::outs() << "********************\n";
+    llvm::outs() << "Crash reproducer for ";
+    llvm::outs() << lldb::SBDebugger::GetVersionString() << '\n';
+    llvm::outs() << '\n';
+    llvm::outs() << "Reproducer written to '" << SBReproducer::GetPath()
+                 << "'\n";
+    llvm::outs() << '\n';
+    llvm::outs() << "Before attaching the reproducer to a bug report:\n";
+    llvm::outs() << " - Look at the directory to ensure you're willing to "
+                    "share its content.\n";
+    llvm::outs()
+        << " - Make sure the reproducer works by replaying the reproducer.\n";
+    llvm::outs() << '\n';
+    llvm::outs() << "Replay the reproducer with the following command:\n";
+    llvm::outs() << argv0 << " -replay " << finalize_path->getValue() << "\n";
+    llvm::outs() << "********************\n";
+    return 0;
+  }
+
   if (auto *replay_path = input_args.getLastArg(OPT_replay)) {
     SBReplayOptions replay_options;
     replay_options.SetCheckVersion(!input_args.hasArg(OPT_no_version_check));
@@ -821,12 +831,6 @@ llvm::Optional<int> InitializeReproducer(llvm::StringRef argv0,
   }
 
   if (capture || capture_path) {
-    // Register the reproducer signal handler.
-    if (!input_args.hasArg(OPT_no_generate_on_signal)) {
-      llvm::sys::AddSignalHandler(reproducer_handler,
-                                  const_cast<char *>(argv0.data()));
-    }
-
     if (capture_path) {
       if (!capture)
         WithColor::warning() << "-capture-path specified without -capture\n";
@@ -843,6 +847,19 @@ llvm::Optional<int> InitializeReproducer(llvm::StringRef argv0,
     }
     if (generate_on_exit)
       SBReproducer::SetAutoGenerate(true);
+
+    // Register the reproducer signal handler.
+    if (!input_args.hasArg(OPT_no_generate_on_signal)) {
+      if (const char *reproducer_path = SBReproducer::GetPath()) {
+        // Leaking the string on purpose.
+        std::string *finalize_cmd = new std::string(argv0);
+        finalize_cmd->append(" --reproducer-finalize '");
+        finalize_cmd->append(reproducer_path);
+        finalize_cmd->append("'");
+        llvm::sys::AddSignalHandler(reproducer_handler,
+                                    const_cast<char *>(finalize_cmd->c_str()));
+      }
+    }
   }
 
   return llvm::None;

lldb/tools/driver/Options.td

@@ -232,6 +232,8 @@ def capture_path: Separate<["--", "-"], "capture-path">,
 def replay: Separate<["--", "-"], "replay">,
   MetaVarName<"<filename>">,
   HelpText<"Tells the debugger to replay a reproducer from <filename>.">;
+def reproducer_finalize: Separate<["--", "-"], "reproducer-finalize">,
+  MetaVarName<"<filename>">;
 def no_version_check: F<"reproducer-no-version-check">,
   HelpText<"Disable the reproducer version check.">;
 def no_verification: F<"reproducer-no-verify">,