[FileCollector] Move interface into FileCollectorBase (NFC)

[JDevlieghere]

Summary

For the reproducers in LLDB we want to switch to an "immediate mode" FileCollector that writes every file encountered straight to disk so we can generate the actual mapping out-of-process. This patch moves the interface into a separate base class.

[dexonsmith]

This mostly looks good, I'm just wondering about tests. I hunted around for a FileCollector unit test and didn't see one, but it seems like it might be a good fit, especially if it gets you coverage things otherwise only used in LLDB. WDYT?

llvm/include/llvm/Support/FileCollector.h

106–110

Seems like these should now have override on them.

[JDevlieghere]

In D89742#2340590, @dexonsmith wrote:

This mostly looks good, I'm just wondering about tests. I hunted around for a FileCollector unit test and didn't see one, but it seems like it might be a good fit, especially if it gets you coverage things otherwise only used in LLDB. WDYT?

I think you might have missed FileCollectorTest.cpp. I guess I could add some test for just the base class, but I'm not sure if that would add much value.

[JDevlieghere]

Add override

[dexonsmith]

In D89742#2340628, @JDevlieghere wrote:

In D89742#2340590, @dexonsmith wrote:

This mostly looks good, I'm just wondering about tests. I hunted around for a FileCollector unit test and didn't see one, but it seems like it might be a good fit, especially if it gets you coverage things otherwise only used in LLDB. WDYT?

I think you might have missed FileCollectorTest.cpp. I guess I could add some test for just the base class, but I'm not sure if that would add much value.

Weird, git grep seems to be returning different results now :). Agreed, it seems like enough.

This LGTM now with the fix for override.

[JDevlieghere]

In D89742#2340769, @dexonsmith wrote:

In D89742#2340628, @JDevlieghere wrote:

In D89742#2340590, @dexonsmith wrote:

This mostly looks good, I'm just wondering about tests. I hunted around for a FileCollector unit test and didn't see one, but it seems like it might be a good fit, especially if it gets you coverage things otherwise only used in LLDB. WDYT?

I think you might have missed FileCollectorTest.cpp. I guess I could add some test for just the base class, but I'm not sure if that would add much value.

Weird, git grep seems to be returning different results now :). Agreed, it seems like enough.

This LGTM now with the fix for override.

Been there... Thanks for the review!

llvm/include/llvm/Support/FileCollector.h

 #include "llvm/Support/VirtualFileSystem.h"
 #include <mutex>
 #include <string>
 
 namespace llvm {
 class FileCollectorFileSystem;
 class Twine;
 
+class FileCollectorBase {
+public:
+  FileCollectorBase();
+  virtual ~FileCollectorBase();
+
+  void addFile(const Twine &file);
+  void addDirectory(const Twine &Dir);
+
+protected:
+  bool markAsSeen(StringRef Path) {
+    if (Path.empty())
+      return false;
+    return Seen.insert(Path).second;
+  }
+
+  virtual void addFileImpl(StringRef SrcPath) = 0;
+
+  virtual llvm::vfs::directory_iterator
+  addDirectoryImpl(const llvm::Twine &Dir,
+                   IntrusiveRefCntPtr<vfs::FileSystem> FS,
+                   std::error_code &EC) = 0;
+
+  /// Synchronizes access to internal data structures.
+  std::mutex Mutex;
+
+  /// Tracks already seen files so they can be skipped.
+  StringSet<> Seen;
+};
+
 /// Captures file system interaction and generates data to be later replayed
 /// with the RedirectingFileSystem.
 ///
 /// For any file that gets accessed we eventually create:
 /// - a copy of the file inside Root
 /// - a record in RedirectingFileSystem mapping that maps:
 ///   current real path -> path to the copy in Root
 ///
 /// That intent is that later when the mapping is used by RedirectingFileSystem
 /// it simulates the state of FS that we collected.
 ///
 /// We generate file copies and mapping lazily - see writeMapping and copyFiles.
 /// We don't try to capture the state of the file at the exact time when it's
 /// accessed. Files might get changed, deleted ... we record only the "final"
 /// state.
 ///
 /// In order to preserve the relative topology of files we use their real paths
 /// as relative paths inside of the Root.
-class FileCollector {
+class FileCollector : public FileCollectorBase {
 public:
   /// \p Root is the directory where collected files are will be stored.
   /// \p OverlayRoot is VFS mapping root.
   /// \p Root directory gets created in copyFiles unless it already exists.
   FileCollector(std::string Root, std::string OverlayRoot);
 
-  void addFile(const Twine &file);
-  void addDirectory(const Twine &Dir);
-
   /// Write the yaml mapping (for the VFS) to the given file.
   std::error_code writeMapping(StringRef MappingFile);
 
   /// Copy the files into the root directory.
   ///
   /// When StopOnError is true (the default) we abort as soon as one file
   /// cannot be copied. This is relatively common, for example when a file was
   /// removed after it was added to the mapping.
   std::error_code copyFiles(bool StopOnError = true);
 
   /// Create a VFS that uses \p Collector to collect files accessed via \p
   /// BaseFS.
   static IntrusiveRefCntPtr<vfs::FileSystem>
   createCollectorVFS(IntrusiveRefCntPtr<vfs::FileSystem> BaseFS,
                      std::shared_ptr<FileCollector> Collector);
 
 private:
   friend FileCollectorFileSystem;
 
-  bool markAsSeen(StringRef Path) {
-    if (Path.empty())
-      return false;
-    return Seen.insert(Path).second;
-  }
-
   bool getRealPath(StringRef SrcPath, SmallVectorImpl<char> &Result);
 
   void addFileToMapping(StringRef VirtualPath, StringRef RealPath) {
     if (sys::fs::is_directory(VirtualPath))
       VFSWriter.addDirectoryMapping(VirtualPath, RealPath);
     else
       VFSWriter.addFileMapping(VirtualPath, RealPath);
   }
 
 protected:
-  void addFileImpl(StringRef SrcPath);
+  void addFileImpl(StringRef SrcPath) override;
 
   llvm::vfs::directory_iterator
   addDirectoryImpl(const llvm::Twine &Dir,
-                   IntrusiveRefCntPtr<vfs::FileSystem> FS, std::error_code &EC);
-
-  /// Synchronizes access to Seen, VFSWriter and SymlinkMap.
-  std::mutex Mutex;
+                   IntrusiveRefCntPtr<vfs::FileSystem> FS,
+                   std::error_code &EC) override;
 
   /// The directory where collected files are copied to in copyFiles().
   const std::string Root;
 
   /// The root directory where the VFS overlay lives.
   const std::string OverlayRoot;
 
-  /// Tracks already seen files so they can be skipped.
-  StringSet<> Seen;
-
   /// The yaml mapping writer.
   vfs::YAMLVFSWriter VFSWriter;
 
   /// Caches RealPath calls when resolving symlinks.
   StringMap<std::string> SymlinkMap;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_SUPPORT_FILE_COLLECTOR_H

llvm/lib/Support/FileCollector.cpp

 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 
 using namespace llvm;
 
+FileCollectorBase::FileCollectorBase() = default;
+FileCollectorBase::~FileCollectorBase() = default;
+
+void FileCollectorBase::addFile(const Twine &File) {
+  std::lock_guard<std::mutex> lock(Mutex);
+  std::string FileStr = File.str();
+  if (markAsSeen(FileStr))
+    addFileImpl(FileStr);
+}
+
+void FileCollectorBase::addDirectory(const Twine &Dir) {
+  assert(sys::fs::is_directory(Dir));
+  std::error_code EC;
+  addDirectoryImpl(Dir, vfs::getRealFileSystem(), EC);
+}
+
 static bool isCaseSensitivePath(StringRef Path) {
   SmallString<256> TmpDest = Path, UpperDest, RealDest;
 
   // Remove component traversals, links, etc.
   if (!sys::fs::real_path(Path, TmpDest))
     return true; // Current default value in vfs.yaml
   Path = TmpDest;
 
if (DirWithSymlink == SymlinkMap.end()) {
     RealPath = DirWithSymlink->second;
   }
 
   sys::path::append(RealPath, FileName);
   Result.swap(RealPath);
   return true;
 }
 
-void FileCollector::addFile(const Twine &File) {
-  std::lock_guard<std::mutex> lock(Mutex);
-  std::string FileStr = File.str();
-  if (markAsSeen(FileStr))
-    addFileImpl(FileStr);
-}
-
-void FileCollector::addDirectory(const Twine &Dir) {
-  assert(sys::fs::is_directory(Dir));
-  std::error_code EC;
-  addDirectoryImpl(Dir, vfs::getRealFileSystem(), EC);
-}
-
 void FileCollector::addFileImpl(StringRef SrcPath) {
   // We need an absolute src path to append to the root.
   SmallString<256> AbsoluteSrc = SrcPath;
   sys::fs::make_absolute(AbsoluteSrc);
 
   // Canonicalize src to a native path to avoid mixed separator styles.
   sys::path::native(AbsoluteSrc);