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