Author: Jonas Devlieghere Date: 2021-01-22T14:15:48-08:00 New Revision: 0be9ca7c0f9a733f846bb6bc4e8e36d46b518162
URL: https://github.com/llvm/llvm-project/commit/0be9ca7c0f9a733f846bb6bc4e8e36d46b518162 DIFF: https://github.com/llvm/llvm-project/commit/0be9ca7c0f9a733f846bb6bc4e8e36d46b518162.diff LOG: [VFS] Fix inconsistencies between relative paths and fallthrough. This patch addresses inconsistencies in the way fallthrough is handled in the RedirectingFileSystem. Rather than trying to change the working directory of the external filesystem, the RedirectingFileSystem will canonicalize every path before handing it down. This guarantees that relative paths are resolved relative to the RedirectingFileSystem's working directory. This allows us to have a strictly virtual working directory, and still fallthrough for absolute paths, but not for relative paths that would get resolved incorrectly at the lower layer (for example, in case of the RealFileSystem, because the strictly virtual path does not exist). Differential revision: https://reviews.llvm.org/D95188 Added: Modified: llvm/include/llvm/Support/VirtualFileSystem.h llvm/lib/Support/VirtualFileSystem.cpp llvm/unittests/Support/VirtualFileSystemTest.cpp Removed: ################################################################################ diff --git a/llvm/include/llvm/Support/VirtualFileSystem.h b/llvm/include/llvm/Support/VirtualFileSystem.h index 6547d2cdb629..c6ddbf60efdf 100644 --- a/llvm/include/llvm/Support/VirtualFileSystem.h +++ b/llvm/include/llvm/Support/VirtualFileSystem.h @@ -651,9 +651,12 @@ class RedirectingFileSystem : public vfs::FileSystem { friend class VFSFromYamlDirIterImpl; friend class RedirectingFileSystemParser; - bool shouldUseExternalFS() const { - return ExternalFSValidWD && IsFallthrough; - } + bool shouldUseExternalFS() const { return IsFallthrough; } + + /// Canonicalize path by removing ".", "..", "./", components. This is + /// a VFS request, do not bother about symlinks in the path components + /// but canonicalize in order to perform the correct entry search. + std::error_code makeCanonical(SmallVectorImpl<char> &Path) const; // In a RedirectingFileSystem, keys can be specified in Posix or Windows // style (or even a mixture of both), so this comparison helper allows @@ -672,9 +675,6 @@ class RedirectingFileSystem : public vfs::FileSystem { /// The current working directory of the file system. std::string WorkingDirectory; - /// Whether the current working directory is valid for the external FS. - bool ExternalFSValidWD = false; - /// The file system to use for external references. IntrusiveRefCntPtr<FileSystem> ExternalFS; @@ -722,7 +722,7 @@ class RedirectingFileSystem : public vfs::FileSystem { public: /// Looks up \p Path in \c Roots. - ErrorOr<Entry *> lookupPath(const Twine &Path) const; + ErrorOr<Entry *> lookupPath(StringRef Path) const; /// Parses \p Buffer, which is expected to be in YAML format and /// returns a virtual file system representing its contents. diff --git a/llvm/lib/Support/VirtualFileSystem.cpp b/llvm/lib/Support/VirtualFileSystem.cpp index c2ce63964f08..05332b00bd1f 100644 --- a/llvm/lib/Support/VirtualFileSystem.cpp +++ b/llvm/lib/Support/VirtualFileSystem.cpp @@ -1016,7 +1016,6 @@ RedirectingFileSystem::RedirectingFileSystem(IntrusiveRefCntPtr<FileSystem> FS) if (auto ExternalWorkingDirectory = ExternalFS->getCurrentWorkingDirectory()) { WorkingDirectory = *ExternalWorkingDirectory; - ExternalFSValidWD = true; } } @@ -1075,12 +1074,6 @@ RedirectingFileSystem::setCurrentWorkingDirectory(const Twine &Path) { if (!exists(Path)) return errc::no_such_file_or_directory; - // Always change the external FS but ignore its result. - if (ExternalFS) { - auto EC = ExternalFS->setCurrentWorkingDirectory(Path); - ExternalFSValidWD = !static_cast<bool>(EC); - } - SmallString<128> AbsolutePath; Path.toVector(AbsolutePath); if (std::error_code EC = makeAbsolute(AbsolutePath)) @@ -1089,8 +1082,14 @@ RedirectingFileSystem::setCurrentWorkingDirectory(const Twine &Path) { return {}; } -std::error_code RedirectingFileSystem::isLocal(const Twine &Path, +std::error_code RedirectingFileSystem::isLocal(const Twine &Path_, bool &Result) { + SmallString<256> Path; + Path_.toVector(Path); + + if (std::error_code EC = makeCanonical(Path)) + return {}; + return ExternalFS->isLocal(Path, Result); } @@ -1125,14 +1124,21 @@ std::error_code RedirectingFileSystem::makeAbsolute(SmallVectorImpl<char> &Path) directory_iterator RedirectingFileSystem::dir_begin(const Twine &Dir, std::error_code &EC) { - ErrorOr<RedirectingFileSystem::Entry *> E = lookupPath(Dir); + SmallString<256> Path; + Dir.toVector(Path); + + EC = makeCanonical(Path); + if (EC) + return {}; + + ErrorOr<RedirectingFileSystem::Entry *> E = lookupPath(Path); if (!E) { EC = E.getError(); if (shouldUseExternalFS() && EC == errc::no_such_file_or_directory) - return ExternalFS->dir_begin(Dir, EC); + return ExternalFS->dir_begin(Path, EC); return {}; } - ErrorOr<Status> S = status(Dir, *E); + ErrorOr<Status> S = status(Path, *E); if (!S) { EC = S.getError(); return {}; @@ -1145,7 +1151,7 @@ directory_iterator RedirectingFileSystem::dir_begin(const Twine &Dir, auto *D = cast<RedirectingFileSystem::RedirectingDirectoryEntry>(*E); return directory_iterator(std::make_shared<VFSFromYamlDirIterImpl>( - Dir, D->contents_begin(), D->contents_end(), + Path, D->contents_begin(), D->contents_end(), /*IterateExternalFS=*/shouldUseExternalFS(), *ExternalFS, EC)); } @@ -1739,22 +1745,22 @@ std::unique_ptr<RedirectingFileSystem> RedirectingFileSystem::create( return FS; } -ErrorOr<RedirectingFileSystem::Entry *> -RedirectingFileSystem::lookupPath(const Twine &Path_) const { - SmallString<256> Path; - Path_.toVector(Path); - - // Handle relative paths +std::error_code +RedirectingFileSystem::makeCanonical(SmallVectorImpl<char> &Path) const { if (std::error_code EC = makeAbsolute(Path)) return EC; - // Canonicalize path by removing ".", "..", "./", components. This is - // a VFS request, do not bother about symlinks in the path components - // but canonicalize in order to perform the correct entry search. - Path = canonicalize(Path); - if (Path.empty()) + llvm::SmallString<256> CanonicalPath = + canonicalize(StringRef(Path.data(), Path.size())); + if (CanonicalPath.empty()) return make_error_code(llvm::errc::invalid_argument); + Path.assign(CanonicalPath.begin(), CanonicalPath.end()); + return {}; +} + +ErrorOr<RedirectingFileSystem::Entry *> +RedirectingFileSystem::lookupPath(StringRef Path) const { sys::path::const_iterator Start = sys::path::begin(Path); sys::path::const_iterator End = sys::path::end(Path); for (const auto &Root : Roots) { @@ -1829,7 +1835,13 @@ ErrorOr<Status> RedirectingFileSystem::status(const Twine &Path, } } -ErrorOr<Status> RedirectingFileSystem::status(const Twine &Path) { +ErrorOr<Status> RedirectingFileSystem::status(const Twine &Path_) { + SmallString<256> Path; + Path_.toVector(Path); + + if (std::error_code EC = makeCanonical(Path)) + return EC; + ErrorOr<RedirectingFileSystem::Entry *> Result = lookupPath(Path); if (!Result) { if (shouldUseExternalFS() && @@ -1867,7 +1879,13 @@ class FileWithFixedStatus : public File { } // namespace ErrorOr<std::unique_ptr<File>> -RedirectingFileSystem::openFileForRead(const Twine &Path) { +RedirectingFileSystem::openFileForRead(const Twine &Path_) { + SmallString<256> Path; + Path_.toVector(Path); + + if (std::error_code EC = makeCanonical(Path)) + return EC; + ErrorOr<RedirectingFileSystem::Entry *> E = lookupPath(Path); if (!E) { if (shouldUseExternalFS() && @@ -1897,8 +1915,14 @@ RedirectingFileSystem::openFileForRead(const Twine &Path) { } std::error_code -RedirectingFileSystem::getRealPath(const Twine &Path, +RedirectingFileSystem::getRealPath(const Twine &Path_, SmallVectorImpl<char> &Output) const { + SmallString<256> Path; + Path_.toVector(Path); + + if (std::error_code EC = makeCanonical(Path)) + return EC; + ErrorOr<RedirectingFileSystem::Entry *> Result = lookupPath(Path); if (!Result) { if (shouldUseExternalFS() && diff --git a/llvm/unittests/Support/VirtualFileSystemTest.cpp b/llvm/unittests/Support/VirtualFileSystemTest.cpp index 6aff06b9a72f..86b747466b5b 100644 --- a/llvm/unittests/Support/VirtualFileSystemTest.cpp +++ b/llvm/unittests/Support/VirtualFileSystemTest.cpp @@ -2135,7 +2135,41 @@ TEST_F(VFSFromYAMLTest, WorkingDirectoryFallthroughInvalid) { EXPECT_TRUE(Status->exists()); Status = FS->status("foo/a"); - ASSERT_TRUE(Status.getError()); + ASSERT_FALSE(Status.getError()); + EXPECT_TRUE(Status->exists()); +} + +TEST_F(VFSFromYAMLTest, VirtualWorkingDirectory) { + IntrusiveRefCntPtr<ErrorDummyFileSystem> Lower(new ErrorDummyFileSystem()); + Lower->addDirectory("//root/"); + Lower->addDirectory("//root/foo"); + Lower->addRegularFile("//root/foo/a"); + Lower->addRegularFile("//root/foo/b"); + Lower->addRegularFile("//root/c"); + IntrusiveRefCntPtr<vfs::FileSystem> FS = getFromYAMLString( + "{ 'use-external-names': false,\n" + " 'roots': [\n" + "{\n" + " 'type': 'directory',\n" + " 'name': '//root/bar',\n" + " 'contents': [ {\n" + " 'type': 'file',\n" + " 'name': 'a',\n" + " 'external-contents': '//root/foo/a'\n" + " }\n" + " ]\n" + "}\n" + "]\n" + "}", + Lower); + ASSERT_TRUE(FS.get() != nullptr); + std::error_code EC = FS->setCurrentWorkingDirectory("//root/bar"); + ASSERT_FALSE(EC); + ASSERT_TRUE(FS.get() != nullptr); + + llvm::ErrorOr<vfs::Status> Status = FS->status("a"); + ASSERT_FALSE(Status.getError()); + EXPECT_TRUE(Status->exists()); } TEST_F(VFSFromYAMLTest, YAMLVFSWriterTest) { _______________________________________________ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits