[llvm] r351050 - [VFS] Allow multiple RealFileSystem instances with independent CWDs.
Daniel Sanders via llvm-commits
llvm-commits at lists.llvm.org
Wed Jan 16 13:08:19 PST 2019
Hi,
I've hit the same problem as Amara on macOS. Just to add a bit of extra information in case you don't have a macOS machine available: The issue seems to be that the temp directory (/var/tmp) on macOS involves a symlink (/var -> private/var) which is being resolved on one side of the comparison but not the other.
> On Jan 14, 2019, at 10:36, Amara Emerson via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>
> I’ve reverted this and two dependent commits in r351091. Feel free to recommit once you’ve tested on macOS.
>
> Thanks,
> Amara
>
>> On Jan 14, 2019, at 9:56 AM, Amara Emerson via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>>
>> This commit also seems to have started breaking green dragon: http://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-incremental/57006/ <http://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-incremental/57006/>
>>
>> I’ll give it 30 mins for a response, but in case you’re in a different time zone after that time I’ll be reverting the commit. We’ve already had a few days of broken builds due to another clangd commit that was only fixed this morning, so we need to get back to green ASAP.
>>
>> Thanks,
>> Amara
>>
>>> On Jan 14, 2019, at 8:07 AM, Jeremy Morse via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>>>
>>> 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/steps/test/logs/stdio>
>>> http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/22856 <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 <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 <mailto: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 <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 <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 <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 <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 <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 <mailto:llvm-commits at lists.llvm.org>
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
> _______________________________________________
> 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/20190116/a04eafba/attachment.html>
More information about the llvm-commits
mailing list