[Support] move FileCollector from LLDB to llvm/Support

[arphaman]

Summary

The file collector class is useful for creating reproducers, not just for LLDB, but for other tools. That's why it should live in LLVM. I'm planning to use it in Clang to generate reproducers for clang-scan-deps.

[arphaman]

Small bug fix.

[jkorous]

llvm/include/llvm/Support/FileCollector.h

41

TLDR: private?

I'm just wondering if we could make the class safer or the correct use more obvious for classes deriving from it (if we want to support that).

The couple protected members and methods seem suggesting it's fine to use these from a derived class implementation. IIUC m_mutex is guarding m_vfs_writer, m_seen and m_symlink_map and while it is being locked in implementation of public method AddFile, protected methods seem to be assuming their callers lock the mutex. Specifically there's a potential for a data race in AddFileImpl (calling GetRealPath which uses m_symlink_map) and in AddFileToMapping if either is called directly from a derived class.

Since lldb_private::FileCollector seems to be using only the public interface we might just declare the above private (and maybe add a doc comment when caller of a method is expected to lock the mutex)?

[arphaman]

Use private as suggested by Jan.

lldb/include/lldb/Utility/FileCollector.h

 //===-- FileCollector.h -----------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_UTILITY_FILE_COLLECTOR_H
 #define LLDB_UTILITY_FILE_COLLECTOR_H
 
 #include "lldb/Utility/FileSpec.h"
 
-#include "llvm/ADT/SmallVector.h"
-#include "llvm/ADT/StringMap.h"
-#include "llvm/ADT/StringSet.h"
-#include "llvm/ADT/Twine.h"
-#include "llvm/Support/VirtualFileSystem.h"
-
-#include <mutex>
+#include "llvm/Support/FileCollector.h"
 
 namespace lldb_private {
 
 /// Collects files into a directory and generates a mapping that can be used by
 /// the VFS.
-class FileCollector {
+class FileCollector : public llvm::FileCollector {
 public:
-  FileCollector(const FileSpec &root, const FileSpec &overlay);
-
-  void AddFile(const llvm::Twine &file);
-  void AddFile(const FileSpec &file) { return AddFile(file.GetPath()); }
+  FileCollector(const FileSpec &root, const FileSpec &overlay) :
+    llvm::FileCollector(root.GetPath(), overlay.GetPath()) {}
 
-  /// Write the yaml mapping (for the VFS) to the given file.
-  std::error_code WriteMapping(const FileSpec &mapping_file);
+  using llvm::FileCollector::AddFile;
 
-  /// Copy the files into the root directory.
-  ///
-  /// When stop_on_error 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 stop_on_error = true);
-
-protected:
-  void AddFileImpl(llvm::StringRef src_path);
-
-  bool MarkAsSeen(llvm::StringRef path) { return m_seen.insert(path).second; }
-
-  bool GetRealPath(llvm::StringRef src_path,
-                   llvm::SmallVectorImpl<char> &result);
-
-  void AddFileToMapping(llvm::StringRef virtual_path,
-                        llvm::StringRef real_path) {
-    m_vfs_writer.addFileMapping(virtual_path, real_path);
+  void AddFile(const FileSpec &file) {
+      std::string path = file.GetPath();
+      llvm::FileCollector::AddFile(path);
   }
 
-  /// Synchronizes adding files.
-  std::mutex m_mutex;
-
-  /// The root directory where files are copied.
-  FileSpec m_root;
-
-  /// The root directory where the VFS overlay lives.
-  FileSpec m_overlay_root;
-
-  /// Tracks already seen files so they can be skipped.
-  llvm::StringSet<> m_seen;
-
-  /// The yaml mapping writer.
-  llvm::vfs::YAMLVFSWriter m_vfs_writer;
-
-  /// Caches real_path calls when resolving symlinks.
-  llvm::StringMap<std::string> m_symlink_map;
+  /// Write the yaml mapping (for the VFS) to the given file.
+  std::error_code WriteMapping(const FileSpec &mapping_file) {
+    std::string path = mapping_file.GetPath();
+    return llvm::FileCollector::WriteMapping(path);
+  }
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_UTILITY_FILE_COLLECTOR_H

lldb/source/Utility/CMakeLists.txt

add_lldb_library(lldbUtility
   Connection.cpp
   ConstString.cpp
   CompletionRequest.cpp
   DataBufferHeap.cpp
   DataBufferLLVM.cpp
   DataEncoder.cpp
   DataExtractor.cpp
   Environment.cpp
-  FileCollector.cpp
   Event.cpp
   FileSpec.cpp
   IOObject.cpp
   JSON.cpp
   LLDBAssert.cpp
   Listener.cpp
   Log.cpp
   Logging.cpp

lldb/unittests/Utility/CMakeLists.txt

 add_lldb_unittest(UtilityTests
   AnsiTerminalTest.cpp
   ArgsTest.cpp
   OptionsWithRawTest.cpp
   ArchSpecTest.cpp
   BroadcasterTest.cpp
   CleanUpTest.cpp
   ConstStringTest.cpp
   CompletionRequestTest.cpp
   DataExtractorTest.cpp
   EnvironmentTest.cpp
   EventTest.cpp
-  FileCollectorTest.cpp
   FileSpecTest.cpp
   FlagsTest.cpp
   JSONTest.cpp
   ListenerTest.cpp
   LogTest.cpp
   NameMatchesTest.cpp
   PredicateTest.cpp
   ProcessInfoTest.cpp

llvm/include/llvm/Support/FileCollector.h

+//===-- FileCollector.h -----------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_SUPPORT_FILE_COLLECTOR_H
+#define LLVM_SUPPORT_FILE_COLLECTOR_H
+
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringSet.h"
+#include "llvm/ADT/Twine.h"
+#include "llvm/Support/VirtualFileSystem.h"
+
+#include <mutex>
+
+namespace llvm {
+
+/// Collects files into a directory and generates a mapping that can be used by
+/// the VFS.
+class FileCollector {
+public:
+  FileCollector(std::string root, std::string overlay);
+
+  void AddFile(const Twine &file);
+
+  /// Write the yaml mapping (for the VFS) to the given file.
+  std::error_code WriteMapping(StringRef mapping_file);
+
+  /// Copy the files into the root directory.
+  ///
+  /// When stop_on_error 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 stop_on_error = true);
+
+protected:
+  void AddFileImpl(StringRef src_path);
+
+  bool MarkAsSeen(StringRef path) { return m_seen.insert(path).second; }
+
+  bool GetRealPath(StringRef src_path,
+                   SmallVectorImpl<char> &result);
+
+  void AddFileToMapping(StringRef virtual_path,
+                        StringRef real_path) {
+    m_vfs_writer.addFileMapping(virtual_path, real_path);
+  }
+
+  /// Synchronizes adding files.
+  std::mutex m_mutex;
+
+  /// The root directory where files are copied.
+  std::string m_root;
+
+  /// The root directory where the VFS overlay lives.
+  std::string m_overlay_root;
+
+  /// Tracks already seen files so they can be skipped.
+  StringSet<> m_seen;
+
+  /// The yaml mapping writer.
+  vfs::YAMLVFSWriter m_vfs_writer;
+
+  /// Caches real_path calls when resolving symlinks.
+  StringMap<std::string> m_symlink_map;
+};
+
+} // end namespace llvm
+
+#endif // LLVM_SUPPORT_FILE_COLLECTOR_H

llvm/lib/Support/CMakeLists.txt

add_llvm_library(LLVMSupport
   Debug.cpp
   DebugCounter.cpp
   DeltaAlgorithm.cpp
   DAGDeltaAlgorithm.cpp
   DJB.cpp
   Error.cpp
   ErrorHandling.cpp
   FileCheck.cpp
+  FileCollector.cpp
   FileUtilities.cpp
   FileOutputBuffer.cpp
   FoldingSet.cpp
   FormattedStream.cpp
   FormatVariadic.cpp
   GlobPattern.cpp
   GraphWriter.cpp
   Hashing.cpp

llvm/lib/Support/FileCollector.cpp

 //===-- FileCollector.cpp ---------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
-#include "lldb/Utility/FileCollector.h"
-
+#include "llvm/Support/FileCollector.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 
-using namespace lldb_private;
 using namespace llvm;
 
 static bool IsCaseSensitivePath(StringRef path) {
   SmallString<256> tmp_dest = path, upper_dest, real_dest;
 
   // Remove component traversals, links, etc.
   if (!sys::fs::real_path(path, tmp_dest))
     return true; // Current default value in vfs.yaml
   path = tmp_dest;
 
   // Change path to all upper case and ask for its real path, if the latter
   // exists and is equal to path, it's not case sensitive. Default to case
   // sensitive in the absence of real_path, since this is the YAMLVFSWriter
   // default.
   upper_dest = path.upper();
   if (sys::fs::real_path(upper_dest, real_dest) && path.equals(real_dest))
     return false;
   return true;
 }
 
-FileCollector::FileCollector(const FileSpec &root, const FileSpec &overlay_root)
-    : m_root(root), m_overlay_root(overlay_root) {
-  sys::fs::create_directories(m_root.GetPath(), true);
+FileCollector::FileCollector(std::string root, std::string overlay_root)
+    : m_root(std::move(root)), m_overlay_root(std::move(overlay_root)) {
+  sys::fs::create_directories(this->m_root, true);
 }
 
 bool FileCollector::GetRealPath(StringRef src_path,
                                 SmallVectorImpl<char> &result) {
   SmallString<256> real_path;
   StringRef FileName = sys::path::filename(src_path);
   std::string directory = sys::path::parent_path(src_path).str();
   auto dir_with_symlink = m_symlink_map.find(directory);

 void FileCollector::AddFile(const Twine &file) {
   std::lock_guard<std::mutex> lock(m_mutex);
   std::string file_str = file.str();
   if (MarkAsSeen(file_str))
     AddFileImpl(file_str);
 }
 
 void FileCollector::AddFileImpl(StringRef src_path) {
-  std::string root = m_root.GetPath();
-
   // We need an absolute src path to append to the root.
   SmallString<256> absolute_src = src_path;
   sys::fs::make_absolute(absolute_src);
 
   // Canonicalize src to a native path to avoid mixed separator styles.
   sys::path::native(absolute_src);
 
   // Remove redundant leading "./" pieces and consecutive separators.
   absolute_src = sys::path::remove_leading_dotslash(absolute_src);
 
   // Canonicalize the source path by removing "..", "." components.
   SmallString<256> virtual_path = absolute_src;
   sys::path::remove_dots(virtual_path, /*remove_dot_dot=*/true);
 
   // If a ".." component is present after a symlink component, remove_dots may
   // lead to the wrong real destination path. Let the source be canonicalized
   // like that but make sure we always use the real path for the destination.
   SmallString<256> copy_from;
   if (!GetRealPath(absolute_src, copy_from))
     copy_from = virtual_path;
 
-  SmallString<256> dst_path = StringRef(root);
+  SmallString<256> dst_path = StringRef(m_root);
   sys::path::append(dst_path, sys::path::relative_path(copy_from));
 
   // Always map a canonical src path to its real path into the YAML, by doing
   // this we map different virtual src paths to the same entry in the VFS
   // overlay, which is a way to emulate symlink inside the VFS; this is also
   // needed for correctness, not doing that can lead to module redefinition
   // errors.
   AddFileToMapping(virtual_path, dst_path);
if (std::error_code ec = sys::fs::status(entry.VPath, stat)) {
         return ec;
       continue;
     }
     CopyAccessAndModificationTime(entry.RPath, stat);
   }
   return {};
 }
 
-std::error_code FileCollector::WriteMapping(const FileSpec &mapping_file) {
+std::error_code FileCollector::WriteMapping(StringRef mapping_file) {
   std::lock_guard<std::mutex> lock(m_mutex);
 
-  std::string root = m_overlay_root.GetPath();
-
+  StringRef root = m_overlay_root;
   m_vfs_writer.setOverlayDir(root);
   m_vfs_writer.setCaseSensitivity(IsCaseSensitivePath(root));
   m_vfs_writer.setUseExternalNames(false);
 
   std::error_code ec;
-  raw_fd_ostream os(mapping_file.GetPath(), ec, sys::fs::F_Text);
+  raw_fd_ostream os(mapping_file, ec, sys::fs::F_Text);
   if (ec)
     return ec;
 
   m_vfs_writer.write(os);
 
   return {};
 }
+

llvm/unittests/Support/CMakeLists.txt

add_llvm_unittest(SupportTests
   DebugCounterTest.cpp
   DJBTest.cpp
   EndianStreamTest.cpp
   EndianTest.cpp
   ErrnoTest.cpp
   ErrorOrTest.cpp
   ErrorTest.cpp
   FileCheckTest.cpp
+  FileCollectorTest.cpp
   FileOutputBufferTest.cpp
   FormatVariadicTest.cpp
   GlobPatternTest.cpp
   Host.cpp
   ItaniumManglingCanonicalizerTest.cpp
   JSONTest.cpp
   KnownBitsTest.cpp
   LEB128Test.cpp

llvm/unittests/Support/FileCollectorTest.cpp

 //===-- FileCollectorTest.cpp -----------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
-#include "lldb/Utility/FileCollector.h"
-#include "lldb/Utility/FileSpec.h"
-
+#include "llvm/Support/FileCollector.h"
 #include "llvm/Support/FileSystem.h"
 
 using namespace llvm;
-using namespace lldb_private;
 
 namespace llvm {
 namespace vfs {
 inline bool operator==(const llvm::vfs::YAMLVFSEntry &LHS,
                        const llvm::vfs::YAMLVFSEntry &RHS) {
   return LHS.VPath == RHS.VPath && LHS.RPath == RHS.RPath;
 }
 } // namespace vfs
 } // namespace llvm
 
 namespace {
 class TestingFileCollector : public FileCollector {
 public:
   using FileCollector::FileCollector;
   using FileCollector::m_root;
   using FileCollector::m_seen;
   using FileCollector::m_symlink_map;
   using FileCollector::m_vfs_writer;
 
-  bool HasSeen(FileSpec fs) {
-    return m_seen.find(fs.GetPath()) != m_seen.end();
+  bool HasSeen(StringRef fs) {
+    return m_seen.find(fs) != m_seen.end();
   }
 };
 
 struct ScopedDir {
   SmallString<128> Path;
   ScopedDir(const Twine &Name, bool Unique = false) {
     std::error_code EC;
     if (Unique) {
~ScopedFile() {
     }
   }
   operator StringRef() { return Path.str(); }
 };
 } // end anonymous namespace
 
 TEST(FileCollectorTest, AddFile) {
   ScopedDir root("add_file_root", true);
-  FileSpec root_fs(root.Path);
+  std::string root_fs = root.Path.str();
   TestingFileCollector file_collector(root_fs, root_fs);
 
-  file_collector.AddFile(FileSpec("/path/to/a"));
-  file_collector.AddFile(FileSpec("/path/to/b"));
-  file_collector.AddFile(FileSpec("/path/to/c"));
+  file_collector.AddFile("/path/to/a");
+  file_collector.AddFile("/path/to/b");
+  file_collector.AddFile("/path/to/c");
 
   // Make sure the root is correct.
   EXPECT_EQ(file_collector.m_root, root_fs);
 
   // Make sure we've seen all the added files.
-  EXPECT_TRUE(file_collector.HasSeen(FileSpec("/path/to/a")));
-  EXPECT_TRUE(file_collector.HasSeen(FileSpec("/path/to/b")));
-  EXPECT_TRUE(file_collector.HasSeen(FileSpec("/path/to/c")));
+  EXPECT_TRUE(file_collector.HasSeen("/path/to/a"));
+  EXPECT_TRUE(file_collector.HasSeen("/path/to/b"));
+  EXPECT_TRUE(file_collector.HasSeen("/path/to/c"));
 
   // Make sure we've only seen the added files.
-  EXPECT_FALSE(file_collector.HasSeen(FileSpec("/path/to/d")));
+  EXPECT_FALSE(file_collector.HasSeen("/path/to/d"));
 }
 
 TEST(FileCollectorTest, CopyFiles) {
   ScopedDir file_root("file_root", true);
   ScopedFile a(file_root + "/aaa");
   ScopedFile b(file_root + "/bbb");
   ScopedFile c(file_root + "/ccc");
 
   // Create file collector and add files.
   ScopedDir root("copy_files_root", true);
-  FileSpec root_fs(root.Path);
+  std::string root_fs = root.Path.str();
   TestingFileCollector file_collector(root_fs, root_fs);
   file_collector.AddFile(a.Path);
   file_collector.AddFile(b.Path);
   file_collector.AddFile(c.Path);
 
   // Make sure we can copy the files.
   std::error_code ec = file_collector.CopyFiles(true);
   EXPECT_FALSE(ec);
TEST(FileCollectorTest, Symlinks) {
   // Create a file eee in the foo's parent directory.
   ScopedFile e(foo + "/../eee");
 
   // Create a symlink bar pointing to foo.
   ScopedLink symlink(file_root + "/foo", file_root + "/bar");
 
   // Root where files are copied to.
   ScopedDir reproducer_root("reproducer_root", true);
-  FileSpec root_fs(reproducer_root.Path);
+  std::string root_fs = reproducer_root.Path.str();
   TestingFileCollector file_collector(root_fs, root_fs);
 
   // Add all the files to the collector.
   file_collector.AddFile(a.Path);
   file_collector.AddFile(b.Path);
   file_collector.AddFile(c.Path);
   file_collector.AddFile(d.Path);
   file_collector.AddFile(e.Path);