[llvm] r351050 - [VFS] Allow multiple RealFileSystem instances with independent CWDs.
Amara Emerson via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 14 10:36:58 PST 2019
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> 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
>
> _______________________________________________
> 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/3f47a756/attachment.html>
More information about the llvm-commits
mailing list