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