Support: Remove duplicated code in {File,clang::ModulesDependency}Collector, NFC

[dexonsmith]

Summary

Refactor the duplicated canonicalize-path logic in FileCollector and
ModulesDependencyCollector into a new utility called
PathCanonicalizer that's shared. This popped up when tracking down a
bug common to both in https://reviews.llvm.org/D95202.

As drive-bys, update a few names and comments to better reflect the
effect of the code, delay removal of ..s to avoid an unnecessary extra
string copy, and leave behind a couple of FIXMEs for future
consideration.

[JDevlieghere]

LGTM, thanks for cleaning this up!

clang/include/clang/Frontend/Utils.h

 #include "clang/Driver/OptionUtils.h"
 #include "clang/Frontend/DependencyOutputOptions.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/Option/OptSpecifier.h"
+#include "llvm/Support/FileCollector.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include <cstdint>
 #include <memory>
 #include <string>
 #include <system_error>
 #include <utility>
 #include <vector>
 

 
 /// Collects the dependencies for imported modules into a directory.  Users
 /// should attach to the AST reader whenever a module is loaded.
 class ModuleDependencyCollector : public DependencyCollector {
   std::string DestDir;
   bool HasErrors = false;
   llvm::StringSet<> Seen;
   llvm::vfs::YAMLVFSWriter VFSWriter;
-  llvm::StringMap<std::string> SymLinkMap;
+  llvm::FileCollector::PathCanonicalizer Canonicalizer;
 
-  bool getRealPath(StringRef SrcPath, SmallVectorImpl<char> &Result);
   std::error_code copyToRoot(StringRef Src, StringRef Dst = {});
 
 public:
   ModuleDependencyCollector(std::string DestDir)
       : DestDir(std::move(DestDir)) {}
   ~ModuleDependencyCollector() override { writeFileMap(); }
 
   StringRef getDest() { return DestDir; }

clang/lib/Frontend/ModuleDependencyCollector.cpp

void ModuleDependencyCollector::writeFileMap() {
   llvm::raw_fd_ostream OS(YAMLPath, EC, llvm::sys::fs::OF_Text);
   if (EC) {
     HasErrors = true;
     return;
   }
   VFSWriter.write(OS);
 }
 
-bool ModuleDependencyCollector::getRealPath(StringRef SrcPath,
-                                            SmallVectorImpl<char> &Result) {
-  using namespace llvm::sys;
-  SmallString<256> RealPath;
-  StringRef FileName = path::filename(SrcPath);
-  std::string Dir = path::parent_path(SrcPath).str();
-  auto DirWithSymLink = SymLinkMap.find(Dir);
-
-  // Use real_path to fix any symbolic link component present in a path.
-  // Computing the real path is expensive, cache the search through the
-  // parent path directory.
-  if (DirWithSymLink == SymLinkMap.end()) {
-    if (llvm::sys::fs::real_path(Dir, RealPath))
-      return false;
-    SymLinkMap[Dir] = std::string(RealPath.str());
-  } else {
-    RealPath = DirWithSymLink->second;
-  }
-
-  path::append(RealPath, FileName);
-  Result.swap(RealPath);
-  return true;
-}
-
 std::error_code ModuleDependencyCollector::copyToRoot(StringRef Src,
                                                       StringRef Dst) {
   using namespace llvm::sys;
+  llvm::FileCollector::PathCanonicalizer::PathStorage Paths =
+      Canonicalizer.canonicalize(SrcPath);
 
-  // We need an absolute src path to append to the root.
-  SmallString<256> AbsoluteSrc = Src;
-  fs::make_absolute(AbsoluteSrc);
-  // Canonicalize src to a native path to avoid mixed separator styles.
-  path::native(AbsoluteSrc);
-  // Remove redundant leading "./" pieces and consecutive separators.
-  StringRef TrimmedAbsoluteSrc = path::remove_leading_dotslash(AbsoluteSrc);
-
-  // Canonicalize the source path by removing "..", "." components.
-  SmallString<256> VirtualPath = TrimmedAbsoluteSrc;
-  path::remove_dots(VirtualPath, /*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> CopyFrom;
-  if (!getRealPath(TrimmedAbsoluteSrc, CopyFrom))
-    CopyFrom = VirtualPath;
   SmallString<256> CacheDst = getDest();
 
   if (Dst.empty()) {
     // The common case is to map the virtual path to the same path inside the
     // cache.
-    path::append(CacheDst, path::relative_path(CopyFrom));
+    path::append(CacheDst, path::relative_path(Paths.CopyFrom));
   } else {
     // When collecting entries from input vfsoverlays, copy the external
     // contents into the cache but still map from the source.
     if (!fs::exists(Dst))
       return std::error_code();
     path::append(CacheDst, Dst);
-    CopyFrom = Dst;
+    Paths.CopyFrom = Dst;
   }
 
   // Copy the file into place.
   if (std::error_code EC = fs::create_directories(path::parent_path(CacheDst),
                                                   /*IgnoreExisting=*/true))
     return EC;
-  if (std::error_code EC = fs::copy_file(CopyFrom, CacheDst))
+  if (std::error_code EC = fs::copy_file(Paths.CopyFrom, CacheDst))
     return EC;
 
   // 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.
-  addFileMapping(VirtualPath, CacheDst);
+  addFileMapping(Paths.VirtualPath, CacheDst);
   return std::error_code();
 }
 
 void ModuleDependencyCollector::addFile(StringRef Filename, StringRef FileDst) {
   if (insertSeen(Filename))
     if (copyToRoot(Filename, FileDst))
       HasErrors = true;
 }

llvm/include/llvm/Support/FileCollector.h

 /// 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 : public FileCollectorBase {
 public:
+  /// Helper utility that encapsulates the logic for canonicalizing a virtual
+  /// path and a path to copy from.
+  class PathCanonicalizer {
+  public:
+    struct PathStorage {
+      SmallString<256> CopyFrom;
+      SmallString<256> VirtualPath;
+    };
+
+    /// Canonicalize a pair of virtual and real paths.
+    PathStorage canonicalize(StringRef SrcPath);
+
+  private:
+    /// Replace with a (mostly) real path, or don't modify. Resolves symlinks
+    /// in the directory, using \a CachedDirs to avoid redundant lookups, but
+    /// leaves the filename as a possible symlink.
+    void updateWithRealPath(SmallVectorImpl<char> &Path);
+
+    StringMap<std::string> CachedDirs;
+  };
+
   /// \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);
 
   /// 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 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) override;
 
   llvm::vfs::directory_iterator
   addDirectoryImpl(const llvm::Twine &Dir,
                    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;
 
   /// The yaml mapping writer.
   vfs::YAMLVFSWriter VFSWriter;
 
-  /// Caches RealPath calls when resolving symlinks.
-  StringMap<std::string> SymlinkMap;
+  /// Helper utility for canonicalizing paths.
+  PathCanonicalizer Canonicalizer;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_SUPPORT_FILE_COLLECTOR_H

llvm/lib/Support/FileCollector.cpp

if (sys::fs::real_path(UpperDest, RealDest) && Path.equals(RealDest))
     return false;
   return true;
 }
 
 FileCollector::FileCollector(std::string Root, std::string OverlayRoot)
     : Root(std::move(Root)), OverlayRoot(std::move(OverlayRoot)) {
 }
 
-bool FileCollector::getRealPath(StringRef SrcPath,
-                                SmallVectorImpl<char> &Result) {
-  SmallString<256> RealPath;
+void FileCollector::PathCanonicalizer::updateWithRealPath(
+    SmallVectorImpl<char> &Path) {
+  StringRef SrcPath(Path.begin(), Path.size());
   StringRef FileName = sys::path::filename(SrcPath);
-  std::string Directory = sys::path::parent_path(SrcPath).str();
-  auto DirWithSymlink = SymlinkMap.find(Directory);
+  StringRef Directory = sys::path::parent_path(SrcPath);
 
   // Use real_path to fix any symbolic link component present in a path.
   // Computing the real path is expensive, cache the search through the parent
   // path Directory.
-  if (DirWithSymlink == SymlinkMap.end()) {
-    auto EC = sys::fs::real_path(Directory, RealPath);
-    if (EC)
-      return false;
-    SymlinkMap[Directory] = std::string(RealPath.str());
+  SmallString<256> RealDirectory;
+  auto DirWithSymlink = CachedDirs.find(Directory);
+  if (DirWithSymlink == CachedDirs.end()) {
+    // FIXME: Should this be a call to FileSystem::getRealpath(), in some
+    // cases? What if there is nothing on disk?
+    if (sys::fs::real_path(Directory, RealDirectory))
+      return;
+    CachedDirs[Directory] = std::string(RealDirectory.str());
   } else {
-    RealPath = DirWithSymlink->second;
+    RealDirectory = DirWithSymlink->second;
   }
 
-  sys::path::append(RealPath, FileName);
-  Result.swap(RealPath);
-  return true;
+  // Use the symlink-resolved directory.
+  Path.swap(RealDirectory);
+
+  // Symlinks in the filename don't matter.
+  //
+  // FIXME: If we can cope with this, maybe we can cope without calling
+  // getRealPath() at all when there's no ".." component.
+  sys::path::append(Path, FileName);
 }
 
-void FileCollector::addFileImpl(StringRef SrcPath) {
+/// Make Path absolute.
+static void makeAbsolute(SmallVectorImpl<char> &Path) {
   // We need an absolute src path to append to the root.
-  SmallString<256> AbsoluteSrc = SrcPath;
-  sys::fs::make_absolute(AbsoluteSrc);
+  sys::fs::make_absolute(Path);
 
   // Canonicalize src to a native path to avoid mixed separator styles.
-  sys::path::native(AbsoluteSrc);
+  sys::path::native(Path);
 
   // Remove redundant leading "./" pieces and consecutive separators.
-  StringRef TrimmedAbsoluteSrc =
-      sys::path::remove_leading_dotslash(AbsoluteSrc);
+  Path.erase(Path.begin(), sys::path::remove_leading_dotslash(
+                               StringRef(Path.begin(), Path.size()))
+                               .begin());
+}
 
-  // Canonicalize the source path by removing "..", "." components.
-  SmallString<256> VirtualPath = TrimmedAbsoluteSrc;
-  sys::path::remove_dots(VirtualPath, /*remove_dot_dot=*/true);
+FileCollector::PathCanonicalizer::PathStorage
+FileCollector::PathCanonicalizer::canonicalize(StringRef SrcPath) {
+  PathStorage Paths;
+  Paths.VirtualPath = SrcPath;
+  makeAbsolute(Paths.VirtualPath);
 
   // 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> CopyFrom;
-  if (!getRealPath(TrimmedAbsoluteSrc, CopyFrom))
-    CopyFrom = VirtualPath;
+  Paths.CopyFrom = Paths.VirtualPath;
+  updateWithRealPath(Paths.CopyFrom);
+
+  // Canonicalize the virtual path by removing "..", "." components.
+  sys::path::remove_dots(Paths.VirtualPath, /*remove_dot_dot=*/true);
+
+  return Paths;
+}
+
+void FileCollector::addFileImpl(StringRef SrcPath) {
+  PathCanonicalizer::PathStorage Paths = Canonicalizer.canonicalize(SrcPath);
 
   SmallString<256> DstPath = StringRef(Root);
-  sys::path::append(DstPath, sys::path::relative_path(CopyFrom));
+  sys::path::append(DstPath, sys::path::relative_path(Paths.CopyFrom));
 
   // 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(VirtualPath, DstPath);
+  addFileToMapping(Paths.VirtualPath, DstPath);
 }
 
 llvm::vfs::directory_iterator
 FileCollector::addDirectoryImpl(const llvm::Twine &Dir,
                                 IntrusiveRefCntPtr<vfs::FileSystem> FS,
                                 std::error_code &EC) {
   auto It = FS->dir_begin(Dir, EC);
   if (EC)