[llvm][vfs] Avoid silent fallback to process-wide working directory

[benlangmuir]

Summary

In createPhysicalFileSystem, preserve the per-instance working directory, even after the first call to getcwd fails.

rdar://108213753

[owenv]

LGTM thanks! But please get a second review from someone more familiar with this code

[jansvoboda11]

This is a step in the right direction, so LGTM.

But I think it would be nicer to fail earlier (in the constructor), or propagate the error out of adjustPath(). What do you think? Would be nice to add a FIXME for this.

[benlangmuir]

But I think it would be nicer to fail earlier (in the constructor), or propagate the error out of adjustPath(). What do you think?

Returning an error from adjustPath makes sense to me if we don't have a usable WD, but the constructor is not so obvious. I could see a few options

• Pass a std::error_code out parameter, but still construct the filesystem so you can ignore the error if desired -- we certainly don't care about the error in clang-scan-deps since we are going to set our own WD later.
• Remove the getcwd call and require passing in an initial WD string. This doesn't really match how we use the filesystem in scan-deps, because we have a per-TU working directory not per-TU filesystem, so I think this could be awkward, or we end up doing something silly like passing "/"
• Don't set any initial working directory in the constructor and reject any calls using relative paths until one is provided via setCurrentWorkingDirectory.

[ro]

The new test FAILs on Solaris like so:

FAIL: LLVM-Unit :: Support/./SupportTests/12/42 (70024 of 75206)
[...]
/var/llvm/dist-amd64-release-stage2-A-flang/tools/clang/stage2-bins/unittests/Support/./SupportTests --gtest_filter=VirtualFileSystemTest.PhysicalFileSystemWorkingDirFailure
--
/vol/llvm/src/llvm-project/dist/llvm/unittests/Support/VirtualFileSystemTest.cpp:546: Failure
Expected equality of these values:
  FS1->getCurrentWorkingDirectory().getError()
    Which is: system:0
  errc::no_such_file_or_directory
    Which is: 4-byte object <02-00 00-00>

Running it under truss (the Solaris equivalent of strace), one sees

24717:  unlinkat(AT_FDCWD, "/var/tmp/d1-67ee3b", AT_REMOVEDIR) Err#22 EINVAL

As documented in rmdir(2), this is expected:

EINVAL          The  directory  to be removed is the current directory,
                or the final component of path is ".".

This is left unspecified in XPG7; however in case there is an error, it's expected to be EBUSY. I suspect Solaris kept EINVAL for backwards compatibility.

[benlangmuir]

@ro thanks for letting me know; this should skip the test if the removal fails: https://reviews.llvm.org/D149760. I looked to see if there was another way to get getcwd to fail inside createPhysicalFileSystem but it seems the only other idea would be EACCESS, but I see a note "This is only checked in limited cases, depending on implementation details." so it doesn't seem like a great choice either.

llvm/lib/Support/VirtualFileSystem.cpp

 /// 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 (llvm::sys::fs::real_path(PWD, RealPWD))
-        WD = {PWD, PWD};
+      if (std::error_code EC = llvm::sys::fs::current_path(PWD))
+        WD = EC;
+      else if (llvm::sys::fs::real_path(PWD, RealPWD))
+        WD = WorkingDirectory{PWD, PWD};
       else
-        WD = {PWD, RealPWD};
+        WD = WorkingDirectory{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;
 
   llvm::ErrorOr<std::string> getCurrentWorkingDirectory() const override;
   std::error_code setCurrentWorkingDirectory(const Twine &Path) override;
   std::error_code isLocal(const Twine &Path, bool &Result) override;
   std::error_code getRealPath(const Twine &Path,
                               SmallVectorImpl<char> &Output) const override;
 
 protected:
   void printImpl(raw_ostream &OS, PrintType Type,
                  unsigned IndentLevel) const override;
 
 private:
   // 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)
+    if (!WD || !*WD)
       return Path;
     Path.toVector(Storage);
-    sys::fs::make_absolute(WD->Resolved, Storage);
+    sys::fs::make_absolute(WD->get().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;
   };
-  std::optional<WorkingDirectory> WD;
+  std::optional<llvm::ErrorOr<WorkingDirectory>> WD;
 };
 
 } // namespace
 
 ErrorOr<Status> RealFileSystem::status(const Twine &Path) {
   SmallString<256> Storage;
   sys::fs::file_status RealStatus;
   if (std::error_code EC =
Expected<file_t> FDOrErr = sys::fs::openNativeFileForRead(
       adjustPath(Name, Storage), sys::fs::OF_None, &RealName);
   if (!FDOrErr)
     return errorToErrorCode(FDOrErr.takeError());
   return std::unique_ptr<File>(
       new RealFile(*FDOrErr, Name.str(), RealName.str()));
 }
 
 llvm::ErrorOr<std::string> RealFileSystem::getCurrentWorkingDirectory() const {
+  if (WD && *WD)
+    return std::string(WD->get().Specified.str());
   if (WD)
-    return std::string(WD->Specified.str());
+    return WD->getError();
 
   SmallString<128> Dir;
   if (std::error_code EC = llvm::sys::fs::current_path(Dir))
     return EC;
   return std::string(Dir.str());
 }
 
 std::error_code RealFileSystem::setCurrentWorkingDirectory(const Twine &Path) {
   if (!WD)
     return llvm::sys::fs::set_current_path(Path);
 
   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};
+  WD = WorkingDirectory{Absolute, Resolved};
   return std::error_code();
 }
 
 std::error_code RealFileSystem::isLocal(const Twine &Path, bool &Result) {
   SmallString<256> Storage;
   return llvm::sys::fs::is_local(adjustPath(Path, Storage), Result);
 }
 

llvm/unittests/Support/VirtualFileSystemTest.cpp

 //===- unittests/Support/VirtualFileSystem.cpp -------------- VFS tests ---===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/VirtualFileSystem.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/Config/llvm-config.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/SourceMgr.h"
 #include "llvm/TargetParser/Host.h"
 #include "llvm/TargetParser/Triple.h"
 #include "llvm/Testing/Support/SupportHelpers.h"
TEST(VirtualFileSystemTest, MultipleWorkingDirs) {
   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());
 }
 
+TEST(VirtualFileSystemTest, PhysicalFileSystemWorkingDirFailure) {
+  TempDir D2("d2", /*Unique*/ true);
+  SmallString<128> WD, PrevWD;
+  ASSERT_EQ(sys::fs::current_path(PrevWD), std::error_code());
+  ASSERT_EQ(sys::fs::createUniqueDirectory("d1", WD), std::error_code());
+  ASSERT_EQ(sys::fs::set_current_path(WD), std::error_code());
+  auto Restore =
+      llvm::make_scope_exit([&] { sys::fs::set_current_path(PrevWD); });
+
+  // Delete the working directory to create an error.
+  ASSERT_EQ(sys::fs::remove_directories(WD), std::error_code());
+
+  // Verify that we still get two separate working directories.
+  auto FS1 = vfs::createPhysicalFileSystem();
+  auto FS2 = vfs::createPhysicalFileSystem();
+  ASSERT_EQ(FS1->getCurrentWorkingDirectory().getError(),
+            errc::no_such_file_or_directory);
+  ASSERT_EQ(FS1->setCurrentWorkingDirectory(D2.path()), std::error_code());
+  ASSERT_EQ(FS1->getCurrentWorkingDirectory().get(), D2.path());
+  EXPECT_EQ(FS2->getCurrentWorkingDirectory().getError(),
+            errc::no_such_file_or_directory);
+  SmallString<128> WD2;
+  EXPECT_EQ(sys::fs::current_path(WD2), errc::no_such_file_or_directory);
+}
+
 TEST(VirtualFileSystemTest, BrokenSymlinkRealFSIteration) {
   TempDir TestDirectory("virtual-file-system-test", /*Unique*/ true);
   IntrusiveRefCntPtr<vfs::FileSystem> FS = vfs::getRealFileSystem();
 
   TempLink _a("no_such_file", TestDirectory.path("a"));
   TempDir _b(TestDirectory.path("b"));
   TempLink _c("no_such_file", TestDirectory.path("c"));