[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

diff --git a/lldb/include/lldb/API/SBReproducer.h b/lldb/include/lldb/API/SBReproducer.h
index 5578162..ecf28d6 100644 (file)
--- a/lldb/include/lldb/API/SBReproducer.h
+++ b/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();

diff --git a/lldb/include/lldb/Host/FileSystem.h b/lldb/include/lldb/Host/FileSystem.h
index 811a10e..02ff5f3 100644 (file)
--- a/lldb/include/lldb/Host/FileSystem.h
+++ b/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;
 };

diff --git a/lldb/include/lldb/Utility/Reproducer.h b/lldb/include/lldb/Utility/Reproducer.h
index 7e55914..4659254 100644 (file)
--- a/lldb/include/lldb/Utility/Reproducer.h
+++ b/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
 

diff --git a/lldb/include/lldb/Utility/ReproducerProvider.h b/lldb/include/lldb/Utility/ReproducerProvider.h
index abb13f0..221c0eb 100644 (file)
--- a/lldb/include/lldb/Utility/ReproducerProvider.h
+++ b/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.

diff --git a/lldb/source/API/SBReproducer.cpp b/lldb/source/API/SBReproducer.cpp
index ec1c85d..4d25fcc 100644 (file)
--- a/lldb/source/API/SBReproducer.cpp
+++ b/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) {

diff --git a/lldb/source/Commands/CommandObjectReproducer.cpp b/lldb/source/Commands/CommandObjectReproducer.cpp
index ae48940..55f34c0 100644 (file)
--- a/lldb/source/Commands/CommandObjectReproducer.cpp
+++ b/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);

diff --git a/lldb/source/Host/common/FileSystem.cpp b/lldb/source/Host/common/FileSystem.cpp
index b8c962c..8a6c03f 100644 (file)
--- a/lldb/source/Host/common/FileSystem.cpp
+++ b/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);
 }

diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ModuleDependencyCollector.h b/lldb/source/Plugins/ExpressionParser/Clang/ModuleDependencyCollector.h
index b7b6640..4fe7274 100644 (file)
--- a/lldb/source/Plugins/ExpressionParser/Clang/ModuleDependencyCollector.h
+++ b/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
 

diff --git a/lldb/source/Utility/Reproducer.cpp b/lldb/source/Utility/Reproducer.cpp
index 1f9ab8d..f238a2f 100644 (file)
--- a/lldb/source/Utility/Reproducer.cpp
+++ b/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);
+}

diff --git a/lldb/source/Utility/ReproducerProvider.cpp b/lldb/source/Utility/ReproducerProvider.cpp
index d67c886..ed01682 100644 (file)
--- a/lldb/source/Utility/ReproducerProvider.cpp
+++ b/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);

diff --git a/lldb/test/Shell/Reproducer/TestCaptureEnvOverride.test b/lldb/test/Shell/Reproducer/TestCaptureEnvOverride.test
index ef06bce..122a059 100644 (file)
--- a/lldb/test/Shell/Reproducer/TestCaptureEnvOverride.test
+++ b/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

diff --git a/lldb/test/Shell/Reproducer/TestFinalize.test b/lldb/test/Shell/Reproducer/TestFinalize.test
new file mode 100644 (file)
index 0000000..4bfe82b
--- /dev/null
+++ b/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

diff --git a/lldb/tools/driver/Driver.cpp b/lldb/tools/driver/Driver.cpp
index 79720dd..e6b725d 100644 (file)
--- a/lldb/tools/driver/Driver.cpp
+++ b/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;

diff --git a/lldb/tools/driver/Options.td b/lldb/tools/driver/Options.td
index b3ffc2d..8bcb0e7 100644 (file)
--- a/lldb/tools/driver/Options.td
+++ b/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">,