Merge pull request #2040 from apple/revert-2033-🍒/bastille/73811d32c72d

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

Author: fredriss

lldb/include/lldb/API/SBReproducer.h

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

lldb/include/lldb/Utility/Reproducer.h

@@ -243,9 +243,6 @@ 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,50 +90,38 @@ class YamlRecorder : public AbstractRecorder {
   }
 };
 
-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 {
     static const char *name;
     static const char *file;
   };
 
-  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();
-  }
+  FileProvider(const FileSpec &directory)
+      : Provider(directory),
+        m_collector(std::make_shared<llvm::FileCollector>(
+            directory.CopyByAppendingPathComponent("root").GetPath(),
+            directory.GetPath())) {}
 
-  std::shared_ptr<llvm::FileCollectorBase> GetFileCollector() {
+  std::shared_ptr<llvm::FileCollector> 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<FlushingFileCollector> m_collector;
+  std::shared_ptr<llvm::FileCollector> m_collector;
 };
 
 /// Provider for the LLDB version number.

lldb/source/API/SBReproducer.cpp

@@ -8,6 +8,7 @@
 
 #include "SBReproducerPrivate.h"
 
+#include "SBReproducerPrivate.h"
 #include "lldb/API/LLDB.h"
 #include "lldb/API/SBAddress.h"
 #include "lldb/API/SBAttachInfo.h"
@@ -265,27 +266,6 @@ 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()) {
@@ -305,11 +285,10 @@ bool SBReproducer::SetAutoGenerate(bool b) {
 }
 
 const char *SBReproducer::GetPath() {
-  ConstString path;
+  static std::string path;
   auto &r = Reproducer::Instance();
-  if (FileSpec reproducer_path = Reproducer::Instance().GetReproducerPath())
-    path = ConstString(r.GetReproducerPath().GetCString());
-  return path.GetCString();
+  path = r.GetReproducerPath().GetCString();
+  return path.c_str();
 }
 
 void SBReproducer::SetWorkingDirectory(const char *path) {

lldb/source/Commands/CommandObjectReproducer.cpp

@@ -192,10 +192,6 @@ class CommandObjectReproducerGenerate : public CommandObjectParsed {
     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<FileCollectorBase> collector) {
+void FileSystem::Initialize(std::shared_ptr<FileCollector> 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::FileCollectorBase> file_collector)
+      std::shared_ptr<llvm::FileCollector> file_collector)
       : clang::ModuleDependencyCollector(""), m_file_collector(file_collector) {
   }
 
@@ -33,7 +33,7 @@ class ModuleDependencyCollectorAdaptor
   void writeFileMap() override {}
 
 private:
-  std::shared_ptr<llvm::FileCollectorBase> m_file_collector;
+  std::shared_ptr<llvm::FileCollector> m_file_collector;
 };
 } // namespace lldb_private
 

lldb/source/Utility/Reproducer.cpp

@@ -176,12 +176,10 @@ Generator::Generator(FileSpec root) : m_root(MakeAbsolute(std::move(root))) {
 
 Generator::~Generator() {
   if (!m_done) {
-    if (m_auto_generate) {
+    if (m_auto_generate)
       Keep();
-      llvm::cantFail(Finalize(GetRoot()));
-    } else {
+    else
       Discard();
-    }
   }
 }
 
@@ -361,58 +359,3 @@ 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,7 +8,6 @@
 
 #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"
@@ -45,40 +44,6 @@ 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,6 +9,8 @@
 
 # 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