[lldb] Fix fallthrough with strictly virtual working directory.

[JDevlieghere]

Summary

LLDB needs to change the virtual current working directory without it
breaking fallthrough. Most reproducers will contain a virtual working
directory that does not exist in the real file system during replay.

Currently, calling setCurrentWorkingDirectory on RedirectingFileSystem
will attempt to change the working directory for the external FS and if
that fails, it will set ExternalFSValidWD to false which prevents
fallthrough.

[dexonsmith]

I'm wondering about this:

Currently, calling setCurrentWorkingDirectory on RedirectingFileSystem
will attempt to change the working directory for the external FS and if
that fails, it will set ExternalFSValidWD to false which prevents
fallthrough.

I'm worried having inconsistent working directories could be worse than blocking fallthrough. Consider:

# physical
/a/b/c/d.c
/c/README

# virtual
/x/README

What if someone does setWorkingDirectory() for /a/b/c/d.c followed by /x, and then looks up the relative path d.c? Or changes directory to ../c?

I wonder if there's a way to split up shouldUseExternalFS() (or refactor other code) to allow the external FS to be partially used without adding this kind of inconsistency.

• Absolute paths should still work just fine.
• Relative paths can be made absolute by the virtual FS first (if the WD isn't valid), then passed through.

llvm/include/llvm/Support/VirtualFileSystem.h

764

I think this could use a comment to explain its behaviour since it's not obvious.

llvm/lib/Support/VirtualFileSystem.cpp

1079

This comment looks out-of-date.

[dexonsmith]

I missed a few words:

In D94811#2502192, @dexonsmith wrote:

What if someone does setWorkingDirectory() for /a/b/c/d.c followed by

"changing directory to"

/x, and then looks up the relative path d.c? Or changes directory to ../c?

[JDevlieghere]

Personally I'd just like to get rid of ExternalFSValidWD inside of shouldUseExternalFS and let the underlying FS take care of it, which I believe would better fit the abstraction but would be a change in behavior (as pointed out in https://reviews.llvm.org/D65677#inline-604694).

[dexonsmith]

In D94811#2502504, @JDevlieghere wrote:

Personally I'd just like to get rid of ExternalFSValidWD inside of shouldUseExternalFS and let the underlying FS take care of it, which I believe would better fit the abstraction but would be a change in behavior (as pointed out in https://reviews.llvm.org/D65677#inline-604694).

I'm not quite following why that would work; when the underlying FS fails to cd, then subsequent calls could fall through to the wrong place.

I just checked the code in OverlayFileSystem and it doesn't seem much better about this:

std::error_code
OverlayFileSystem::setCurrentWorkingDirectory(const Twine &Path) {
  for (auto &FS : FSList)
    if (std::error_code EC = FS->setCurrentWorkingDirectory(Path))
      return EC;
  return {};
}

That'll leave things horribly inconsistent as well.

The abstraction as I see it is that the caller sees the OverlayFileSystem / RedirectingFileSystem as the *only* filesystem, a single thing that can be cd'ed around in. A cd to any directory that exists in that virtual view should work as if that were true. Subsequent calls to setWorkingDirectory() or getFile() should behave as if that were the case.

I think one way to get that, at least for your use case, is to extract out RealFileSystem's handling for a custom working directory and reuse it.

• getRealFileSystem() still returns a RealFileSystem, but that class gets stripped down.
• WorkingDirectoryFileSystem<BaseT> gets that support, which can be layered on top of anything.
• getPhysicalFileSystem() returns a WorkingDirectoryFileSystem<RealFileSystem>.
• LLDB uses a WorkingDirectoryFileSystem<ProxyFileSystem> on top of the last VFS overlay.

Probably clang could take advantage of this abstraction as well, but that seems outside the scope of what you're trying to do.

WDYT?

[JDevlieghere]

Superseded by D95188

lldb/source/Initialization/SystemInitializerCommon.cpp

if (vfs_mapping) {
       FileSystem::Initialize();
     }
 
     // Set the current working directory form the reproducer.
     llvm::Expected<std::string> working_dir =
         repro::GetDirectoryFrom<WorkingDirectoryProvider>(loader);
     if (!working_dir)
       return working_dir.takeError();
-    if (std::error_code ec = FileSystem::Instance()
-                                 .GetVirtualFileSystem()
-                                 ->setCurrentWorkingDirectory(*working_dir)) {
+
+    auto &redirecting_vfs = static_cast<llvm::vfs::RedirectingFileSystem &>(
+        *FileSystem::Instance().GetVirtualFileSystem());
+    if (std::error_code ec =
+            redirecting_vfs.setVirtualWorkingDirectory(*working_dir)) {
       return llvm::errorCodeToError(ec);
     }
 
     // Set the home directory from the reproducer.
     llvm::Expected<std::string> home_dir =
         repro::GetDirectoryFrom<HomeDirectoryProvider>(loader);
     if (!home_dir)
       return home_dir.takeError();

lldb/source/Utility/Reproducer.cpp

void Verifier::Verify(
   redirecting_vfs.setFallthrough(false);
 
   {
     llvm::Expected<std::string> working_dir =
         GetDirectoryFrom<WorkingDirectoryProvider>(m_loader);
     if (working_dir) {
       if (!vfs->exists(*working_dir))
         warning_callback("working directory '" + *working_dir + "' not in VFS");
-      vfs->setCurrentWorkingDirectory(*working_dir);
+      redirecting_vfs.setVirtualWorkingDirectory(*working_dir);
     } else {
       warning_callback("no working directory in reproducer: " +
                        toString(working_dir.takeError()));
     }
   }
 
   {
     llvm::Expected<std::string> home_dir =

llvm/include/llvm/Support/VirtualFileSystem.h

#endif
   /// recursing into the contents of \p From if it is a directory.
   ErrorOr<Entry *> lookupPath(llvm::sys::path::const_iterator Start,
                               llvm::sys::path::const_iterator End,
                               Entry *From) const;
 
   /// Get the status of a given an \c Entry.
   ErrorOr<Status> status(const Twine &Path, Entry *E);
 
+  std::error_code setCurrentWorkingDirectoryImpl(const Twine &Path,
+                                                 bool SetExternalCWD);
+
 public:
   /// Looks up \p Path in \c Roots.
   ErrorOr<Entry *> lookupPath(const Twine &Path) const;
 
   /// Parses \p Buffer, which is expected to be in YAML format and
   /// returns a virtual file system representing its contents.
   static std::unique_ptr<RedirectingFileSystem>
   create(std::unique_ptr<MemoryBuffer> Buffer,
public:
   directory_iterator dir_begin(const Twine &Dir, std::error_code &EC) override;
 
   void setExternalContentsPrefixDir(StringRef PrefixDir);
 
   StringRef getExternalContentsPrefixDir() const;
 
   void setFallthrough(bool Fallthrough);
 
+  std::error_code setVirtualWorkingDirectory(const Twine &Path);
+
   std::vector<llvm::StringRef> getRoots() const;
 
   void dump(raw_ostream &OS) const;
   void dumpEntry(raw_ostream &OS, Entry *E, int NumSpaces = 0) const;
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
   LLVM_DUMP_METHOD void dump() const;
 #endif
 };

llvm/lib/Support/VirtualFileSystem.cpp

 };
 
 llvm::ErrorOr<std::string>
 RedirectingFileSystem::getCurrentWorkingDirectory() const {
   return WorkingDirectory;
 }
 
 std::error_code
-RedirectingFileSystem::setCurrentWorkingDirectory(const Twine &Path) {
+RedirectingFileSystem::setCurrentWorkingDirectoryImpl(const Twine &Path,
+                                                      bool SetExternalCWD) {
   // Don't change the working directory if the path doesn't exist.
   if (!exists(Path))
     return errc::no_such_file_or_directory;
 
   // Always change the external FS but ignore its result.
-  if (ExternalFS) {
+  if (ExternalFS && SetExternalCWD) {
     auto EC = ExternalFS->setCurrentWorkingDirectory(Path);
     ExternalFSValidWD = !static_cast<bool>(EC);
   }
 
   SmallString<128> AbsolutePath;
   Path.toVector(AbsolutePath);
   if (std::error_code EC = makeAbsolute(AbsolutePath))
     return EC;
   WorkingDirectory = std::string(AbsolutePath.str());
   return {};
 }
 
+std::error_code
+RedirectingFileSystem::setCurrentWorkingDirectory(const Twine &Path) {
+  return setCurrentWorkingDirectoryImpl(Path, true);
+}
+
+std::error_code
+RedirectingFileSystem::setVirtualWorkingDirectory(const Twine &Path) {
+  return setCurrentWorkingDirectoryImpl(Path, false);
+}
+
 std::error_code RedirectingFileSystem::isLocal(const Twine &Path,
                                                bool &Result) {
   return ExternalFS->isLocal(Path, Result);
 }
 
 std::error_code RedirectingFileSystem::makeAbsolute(SmallVectorImpl<char> &Path) const {
   if (llvm::sys::path::is_absolute(Path, llvm::sys::path::Style::posix) ||
       llvm::sys::path::is_absolute(Path, llvm::sys::path::Style::windows))