[Lldb-commits] [PATCH] D94811: [lldb] Fix fallthrough with strictly virtual working directory.

[Duncan P. N. Exon Smith via Phabricator via lldb-commits]

dexonsmith added a comment.

In D94811#2502504 <https://reviews.llvm.org/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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94811/new/

https://reviews.llvm.org/D94811