diff --git a/clang/include/clang/Frontend/Utils.h b/clang/include/clang/Frontend/Utils.h --- a/clang/include/clang/Frontend/Utils.h +++ b/clang/include/clang/Frontend/Utils.h @@ -23,6 +23,7 @@ #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 #include @@ -151,9 +152,8 @@ bool HasErrors = false; llvm::StringSet<> Seen; llvm::vfs::YAMLVFSWriter VFSWriter; - llvm::StringMap SymLinkMap; + llvm::FileCollector::PathCanonicalizer Canonicalizer; - bool getRealPath(StringRef SrcPath, SmallVectorImpl &Result); std::error_code copyToRoot(StringRef Src, StringRef Dst = {}); public: diff --git a/clang/lib/Frontend/ModuleDependencyCollector.cpp b/clang/lib/Frontend/ModuleDependencyCollector.cpp --- a/clang/lib/Frontend/ModuleDependencyCollector.cpp +++ b/clang/lib/Frontend/ModuleDependencyCollector.cpp @@ -156,72 +156,32 @@ VFSWriter.write(OS); } -bool ModuleDependencyCollector::getRealPath(StringRef SrcPath, - SmallVectorImpl &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(Src); - // 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 @@ -229,7 +189,7 @@ // 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(); } diff --git a/llvm/include/llvm/Support/FileCollector.h b/llvm/include/llvm/Support/FileCollector.h --- a/llvm/include/llvm/Support/FileCollector.h +++ b/llvm/include/llvm/Support/FileCollector.h @@ -69,6 +69,27 @@ /// 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 &Path); + + StringMap 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. @@ -93,8 +114,6 @@ private: friend FileCollectorFileSystem; - bool getRealPath(StringRef SrcPath, SmallVectorImpl &Result); - void addFileToMapping(StringRef VirtualPath, StringRef RealPath) { if (sys::fs::is_directory(VirtualPath)) VFSWriter.addDirectoryMapping(VirtualPath, RealPath); @@ -119,8 +138,8 @@ /// The yaml mapping writer. vfs::YAMLVFSWriter VFSWriter; - /// Caches RealPath calls when resolving symlinks. - StringMap SymlinkMap; + /// Helper utility for canonicalizing paths. + PathCanonicalizer Canonicalizer; }; } // end namespace llvm diff --git a/llvm/lib/Support/FileCollector.cpp b/llvm/lib/Support/FileCollector.cpp --- a/llvm/lib/Support/FileCollector.cpp +++ b/llvm/lib/Support/FileCollector.cpp @@ -53,62 +53,82 @@ : Root(std::move(Root)), OverlayRoot(std::move(OverlayRoot)) { } -bool FileCollector::getRealPath(StringRef SrcPath, - SmallVectorImpl &Result) { +void FileCollector::PathCanonicalizer::updateWithRealPath( + SmallVectorImpl &Path) { + StringRef SrcPath(Path.begin(), Path.size()); + StringRef Filename = sys::path::filename(SrcPath); + StringRef Directory = sys::path::parent_path(SrcPath); + + // Use real_path to fix any symbolic link component present in the directory + // part of the path, caching the search because computing the real path is + // expensive. SmallString<256> RealPath; - StringRef FileName = sys::path::filename(SrcPath); - std::string Directory = sys::path::parent_path(SrcPath).str(); - auto DirWithSymlink = SymlinkMap.find(Directory); - - // 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()); + 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, RealPath)) + return; + CachedDirs[Directory] = std::string(RealPath.str()); } else { RealPath = DirWithSymlink->second; } - sys::path::append(RealPath, FileName); - Result.swap(RealPath); - return true; + // Finish recreating the path by appending the original filename, since we + // don't need to resolve symlinks in the filename. + // + // FIXME: If we can cope with this, maybe we can cope without calling + // getRealPath() at all when there's no ".." component. + sys::path::append(RealPath, Filename); + + // Swap to create the output. + Path.swap(RealPath); } -void FileCollector::addFileImpl(StringRef SrcPath) { +/// Make Path absolute. +static void makeAbsolute(SmallVectorImpl &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 diff --git a/llvm/unittests/Support/FileCollectorTest.cpp b/llvm/unittests/Support/FileCollectorTest.cpp --- a/llvm/unittests/Support/FileCollectorTest.cpp +++ b/llvm/unittests/Support/FileCollectorTest.cpp @@ -33,7 +33,6 @@ using FileCollector::FileCollector; using FileCollector::Root; using FileCollector::Seen; - using FileCollector::SymlinkMap; using FileCollector::VFSWriter; bool hasSeen(StringRef fs) {