[llvm] r351050 - [VFS] Allow multiple RealFileSystem instances with independent CWDs.
Jeremy Morse via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 14 08:07:08 PST 2019
Hi Sam,
The VirtualFileSystemTest.MultipleWorkingDirs unit test added here appears
to croak on Windows:
http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/22856/steps/test/logs/stdio
http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/22856
also on
http://lab.llvm.org:8011/builders/clang-x64-ninja-win7/builds/15020
although there seems to be other stuff going on with that buildbot.
--
Thanks,
Jeremy
On Mon, Jan 14, 2019 at 11:00 AM Sam McCall via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
> Author: sammccall
> Date: Mon Jan 14 02:56:35 2019
> New Revision: 351050
>
> URL: http://llvm.org/viewvc/llvm-project?rev=351050&view=rev
> Log:
> [VFS] Allow multiple RealFileSystem instances with independent CWDs.
>
> Summary:
> Previously only one RealFileSystem instance was available, and its working
> directory is shared with the process. This doesn't work well for
> multithreaded
> programs that want to work with relative paths - the vfs::FileSystem is
> assumed
> to provide the working directory, but a thread cannot control this
> exclusively.
>
> The new vfs::createPhysicalFileSystem() factory copies the process's
> working
> directory initially, and then allows it to be independently modified.
>
> This implementation records the working directory path, and glues it to
> relative
> paths to provide the correct absolute path to the sys::fs:: functions.
> This will give different results in unusual situations (e.g. the CWD is
> moved).
>
> The main alternative is the use of openat(), fstatat(), etc to ask the OS
> to
> resolve paths relative to a directory handle which can be kept open. This
> is
> more robust. There are two reasons not to do this initially:
> 1. these functions are not available on all supported Unixes, and are
> somewhere
> between difficult and unavailable on Windows. So we need a path-based
> fallback anyway.
> 2. this would mean also adding support at the llvm::sys::fs level, which
> is a
> larger project. My clearest idea is an OS-specific `BaseDirectory`
> object
> that can be optionally passed to functions there. Eventually this could
> be
> backed by either paths or a fd where openat() is supported.
> This is a large project, and demonstrating here that a path-based
> fallback
> works is a useful prerequisite.
>
> There is some subtlety to the path-manipulation mechanism:
> - when setting the working directory, both Specified=makeAbsolute(path)
> and
> Resolved=realpath(path) are recorded. These may differ in the presence
> of
> symlinks.
> - getCurrentWorkingDirectory() and makeAbsolute() use Specified - this is
> similar to the behavior of $PWD and sys::path::current_path
> - IO operations like openFileForRead use Resolved. This is similar to the
> behavior of an openat() based implementation, that doesn't see changes
> in symlinks.
> There may still be combinations of operations and FS states that yield
> unhelpful
> behavior. This is hard to avoid with symlinks and FS abstractions :(
>
> The caching behavior of the current working directory is removed in this
> patch.
> getRealFileSystem() is now specified to link to the process CWD, so the
> caching
> is incorrect.
> The user who needed this so far is clangd, which will immediately switch to
> createPhysicalFileSystem().
>
> Reviewers: ilya-biryukov, bkramer, labath
>
> Subscribers: ioeric, kadircet, kristina, llvm-commits
>
> Differential Revision: https://reviews.llvm.org/D56545
>
> Modified:
> llvm/trunk/include/llvm/Support/VirtualFileSystem.h
> llvm/trunk/lib/Support/VirtualFileSystem.cpp
> llvm/trunk/unittests/Support/VirtualFileSystemTest.cpp
>
> Modified: llvm/trunk/include/llvm/Support/VirtualFileSystem.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/VirtualFileSystem.h?rev=351050&r1=351049&r2=351050&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/Support/VirtualFileSystem.h (original)
> +++ llvm/trunk/include/llvm/Support/VirtualFileSystem.h Mon Jan 14
> 02:56:35 2019
> @@ -298,8 +298,16 @@ public:
>
> /// Gets an \p vfs::FileSystem for the 'real' file system, as seen by
> /// the operating system.
> +/// The working directory is linked to the process's working directory.
> +/// (This is usually thread-hostile).
> IntrusiveRefCntPtr<FileSystem> getRealFileSystem();
>
> +/// Create an \p vfs::FileSystem for the 'real' file system, as seen by
> +/// the operating system.
> +/// It has its own working directory, independent of (but initially equal
> to)
> +/// that of the process.
> +std::unique_ptr<FileSystem> createPhysicalFileSystem();
> +
> /// A file system that allows overlaying one \p AbstractFileSystem on top
> /// of another.
> ///
>
> Modified: llvm/trunk/lib/Support/VirtualFileSystem.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/VirtualFileSystem.cpp?rev=351050&r1=351049&r2=351050&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Support/VirtualFileSystem.cpp (original)
> +++ llvm/trunk/lib/Support/VirtualFileSystem.cpp Mon Jan 14 02:56:35 2019
> @@ -228,9 +228,28 @@ std::error_code RealFile::close() {
>
> namespace {
>
> -/// The file system according to your operating system.
> +/// A file system according to your operating system.
> +/// This may be linked to the process's working directory, or maintain
> its own.
> +///
> +/// Currently, its own working directory is emulated by storing the path
> and
> +/// sending absolute paths to llvm::sys::fs:: functions.
> +/// A more principled approach would be to push this down a level,
> modelling
> +/// the working dir as an llvm::sys::fs::WorkingDir or similar.
> +/// This would enable the use of openat()-style functions on some
> platforms.
> class RealFileSystem : public FileSystem {
> public:
> + explicit RealFileSystem(bool LinkCWDToProcess) {
> + if (!LinkCWDToProcess) {
> + SmallString<128> PWD, RealPWD;
> + if (llvm::sys::fs::current_path(PWD))
> + return; // Awful, but nothing to do here.
> + if (auto Err = llvm::sys::fs::real_path(PWD, RealPWD))
> + WD = {PWD, PWD};
> + else
> + WD = {PWD, RealPWD};
> + }
> + }
> +
> ErrorOr<Status> status(const Twine &Path) override;
> ErrorOr<std::unique_ptr<File>> openFileForRead(const Twine &Path)
> override;
> directory_iterator dir_begin(const Twine &Dir, std::error_code &EC)
> override;
> @@ -242,15 +261,32 @@ public:
> SmallVectorImpl<char> &Output) const
> override;
>
> private:
> - mutable std::mutex CWDMutex;
> - mutable std::string CWDCache;
> + // If this FS has its own working dir, use it to make Path absolute.
> + // The returned twine is safe to use as long as both Storage and Path
> live.
> + Twine adjustPath(const Twine &Path, SmallVectorImpl<char> &Storage)
> const {
> + if (!WD)
> + return Path;
> + Path.toVector(Storage);
> + sys::fs::make_absolute(WD->Resolved, Storage);
> + return Storage;
> + }
> +
> + struct WorkingDirectory {
> + // The current working directory, without symlinks resolved. (echo
> $PWD).
> + SmallString<128> Specified;
> + // The current working directory, with links resolved. (readlink .).
> + SmallString<128> Resolved;
> + };
> + Optional<WorkingDirectory> WD;
> };
>
> } // namespace
>
> ErrorOr<Status> RealFileSystem::status(const Twine &Path) {
> + SmallString<256> Storage;
> sys::fs::file_status RealStatus;
> - if (std::error_code EC = sys::fs::status(Path, RealStatus))
> + if (std::error_code EC =
> + sys::fs::status(adjustPath(Path, Storage), RealStatus))
> return EC;
> return Status::copyWithNewName(RealStatus, Path.str());
> }
> @@ -258,56 +294,61 @@ ErrorOr<Status> RealFileSystem::status(c
> ErrorOr<std::unique_ptr<File>>
> RealFileSystem::openFileForRead(const Twine &Name) {
> int FD;
> - SmallString<256> RealName;
> - if (std::error_code EC =
> - sys::fs::openFileForRead(Name, FD, sys::fs::OF_None, &RealName))
> + SmallString<256> RealName, Storage;
> + if (std::error_code EC = sys::fs::openFileForRead(
> + adjustPath(Name, Storage), FD, sys::fs::OF_None, &RealName))
> return EC;
> return std::unique_ptr<File>(new RealFile(FD, Name.str(),
> RealName.str()));
> }
>
> llvm::ErrorOr<std::string> RealFileSystem::getCurrentWorkingDirectory()
> const {
> - std::lock_guard<std::mutex> Lock(CWDMutex);
> - if (!CWDCache.empty())
> - return CWDCache;
> - SmallString<256> Dir;
> + if (WD)
> + return WD->Specified.str();
> +
> + SmallString<128> Dir;
> if (std::error_code EC = llvm::sys::fs::current_path(Dir))
> return EC;
> - CWDCache = Dir.str();
> - return CWDCache;
> + return Dir.str();
> }
>
> std::error_code RealFileSystem::setCurrentWorkingDirectory(const Twine
> &Path) {
> - // FIXME: chdir is thread hostile; on the other hand, creating the same
> - // behavior as chdir is complex: chdir resolves the path once, thus
> - // guaranteeing that all subsequent relative path operations work
> - // on the same path the original chdir resulted in. This makes a
> - // difference for example on network filesystems, where symlinks might
> be
> - // switched during runtime of the tool. Fixing this depends on having a
> - // file system abstraction that allows openat() style interactions.
> - if (auto EC = llvm::sys::fs::set_current_path(Path))
> - return EC;
> + if (!WD)
> + return llvm::sys::fs::set_current_path(Path);
>
> - // Invalidate cache.
> - std::lock_guard<std::mutex> Lock(CWDMutex);
> - CWDCache.clear();
> + SmallString<128> Absolute, Resolved, Storage;
> + adjustPath(Path, Storage).toVector(Absolute);
> + bool IsDir;
> + if (auto Err = llvm::sys::fs::is_directory(Absolute, IsDir))
> + return Err;
> + if (!IsDir)
> + return std::make_error_code(std::errc::not_a_directory);
> + if (auto Err = llvm::sys::fs::real_path(Absolute, Resolved))
> + return Err;
> + WD = {Absolute, Resolved};
> return std::error_code();
> }
>
> std::error_code RealFileSystem::isLocal(const Twine &Path, bool &Result) {
> - return llvm::sys::fs::is_local(Path, Result);
> + SmallString<256> Storage;
> + return llvm::sys::fs::is_local(adjustPath(Path, Storage), Result);
> }
>
> std::error_code
> RealFileSystem::getRealPath(const Twine &Path,
> SmallVectorImpl<char> &Output) const {
> - return llvm::sys::fs::real_path(Path, Output);
> + SmallString<256> Storage;
> + return llvm::sys::fs::real_path(adjustPath(Path, Storage), Output);
> }
>
> IntrusiveRefCntPtr<FileSystem> vfs::getRealFileSystem() {
> - static IntrusiveRefCntPtr<FileSystem> FS = new RealFileSystem();
> + static IntrusiveRefCntPtr<FileSystem> FS(new RealFileSystem(true));
> return FS;
> }
>
> +std::unique_ptr<FileSystem> vfs::createPhysicalFileSystem() {
> + return llvm::make_unique<RealFileSystem>(false);
> +}
> +
> namespace {
>
> class RealFSDirIter : public llvm::vfs::detail::DirIterImpl {
> @@ -333,7 +374,9 @@ public:
>
> directory_iterator RealFileSystem::dir_begin(const Twine &Dir,
> std::error_code &EC) {
> - return directory_iterator(std::make_shared<RealFSDirIter>(Dir, EC));
> + SmallString<128> Storage;
> + return directory_iterator(
> + std::make_shared<RealFSDirIter>(adjustPath(Dir, Storage), EC));
> }
>
>
> //===-----------------------------------------------------------------------===/
>
> Modified: llvm/trunk/unittests/Support/VirtualFileSystemTest.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/VirtualFileSystemTest.cpp?rev=351050&r1=351049&r2=351050&view=diff
>
> ==============================================================================
> --- llvm/trunk/unittests/Support/VirtualFileSystemTest.cpp (original)
> +++ llvm/trunk/unittests/Support/VirtualFileSystemTest.cpp Mon Jan 14
> 02:56:35 2019
> @@ -382,6 +382,25 @@ struct ScopedLink {
> }
> operator StringRef() { return Path.str(); }
> };
> +
> +struct ScopedFile {
> + SmallString<128> Path;
> + ScopedFile(const Twine &Path, StringRef Contents) {
> + Path.toVector(this->Path);
> + std::error_code EC;
> + raw_fd_ostream OS(this->Path, EC);
> + EXPECT_FALSE(EC);
> + OS << Contents;
> + OS.flush();
> + EXPECT_FALSE(OS.error());
> + if (EC || OS.error())
> + this->Path = "";
> + }
> + ~ScopedFile() {
> + if (Path != "")
> + EXPECT_FALSE(llvm::sys::fs::remove(Path.str()));
> + }
> +};
> } // end anonymous namespace
>
> TEST(VirtualFileSystemTest, BasicRealFSIteration) {
> @@ -411,6 +430,67 @@ TEST(VirtualFileSystemTest, BasicRealFSI
> EXPECT_EQ(vfs::directory_iterator(), I);
> }
>
> +TEST(VirtualFileSystemTest, MultipleWorkingDirs) {
> + // Our root contains a/aa, b/bb, c, where c is a link to a/.
> + // Run tests both in root/b/ and root/c/ (to test "normal" and symlink
> dirs).
> + // Interleave operations to show the working directories are
> independent.
> + ScopedDir Root("r", true), ADir(Root.Path + "/a"), BDir(Root.Path +
> "/b");
> + ScopedLink C(ADir.Path, Root.Path + "/c");
> + ScopedFile AA(ADir.Path + "/aa", "aaaa"), BB(BDir.Path + "/bb", "bbbb");
> + std::unique_ptr<vfs::FileSystem> BFS = vfs::createPhysicalFileSystem(),
> + CFS = vfs::createPhysicalFileSystem();
> +
> + ASSERT_FALSE(BFS->setCurrentWorkingDirectory(BDir.Path));
> + ASSERT_FALSE(CFS->setCurrentWorkingDirectory(C.Path));
> + EXPECT_EQ(BDir.Path, *BFS->getCurrentWorkingDirectory());
> + EXPECT_EQ(C.Path, *CFS->getCurrentWorkingDirectory());
> +
> + // openFileForRead(), indirectly.
> + auto BBuf = BFS->getBufferForFile("bb");
> + ASSERT_TRUE(BBuf);
> + EXPECT_EQ("bbbb", (*BBuf)->getBuffer());
> +
> + auto ABuf = CFS->getBufferForFile("aa");
> + ASSERT_TRUE(ABuf);
> + EXPECT_EQ("aaaa", (*ABuf)->getBuffer());
> +
> + // status()
> + auto BStat = BFS->status("bb");
> + ASSERT_TRUE(BStat);
> + EXPECT_EQ("bb", BStat->getName());
> +
> + auto AStat = CFS->status("aa");
> + ASSERT_TRUE(AStat);
> + EXPECT_EQ("aa", AStat->getName()); // unresolved name
> +
> + // getRealPath()
> + SmallString<128> BPath;
> + ASSERT_FALSE(BFS->getRealPath("bb", BPath));
> + EXPECT_EQ(BB.Path, BPath);
> +
> + SmallString<128> APath;
> + ASSERT_FALSE(CFS->getRealPath("aa", APath));
> + EXPECT_EQ(AA.Path, APath); // Reports resolved name.
> +
> + // dir_begin
> + std::error_code EC;
> + auto BIt = BFS->dir_begin(".", EC);
> + ASSERT_FALSE(EC);
> + ASSERT_NE(BIt, vfs::directory_iterator());
> + EXPECT_EQ((BDir.Path + "/./bb").str(), BIt->path());
> + BIt.increment(EC);
> + ASSERT_FALSE(EC);
> + ASSERT_EQ(BIt, vfs::directory_iterator());
> +
> + auto CIt = CFS->dir_begin(".", EC);
> + ASSERT_FALSE(EC);
> + ASSERT_NE(CIt, vfs::directory_iterator());
> + EXPECT_EQ((ADir.Path + "/./aa").str(), CIt->path()); // Partly resolved
> name!
> + CIt.increment(EC); // Because likely to read through this path.
> + ASSERT_FALSE(EC);
> + ASSERT_EQ(CIt, vfs::directory_iterator());
> +}
> +
> #ifdef LLVM_ON_UNIX
> TEST(VirtualFileSystemTest, BrokenSymlinkRealFSIteration) {
> ScopedDir TestDirectory("virtual-file-system-test", /*Unique*/ true);
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190114/ea2ee4de/attachment.html>
More information about the llvm-commits
mailing list