[Lldb-commits] [PATCH] D121425: [VFS] Add a new getVFSFromYAMLs API

[Ben Barham via Phabricator via lldb-commits]

bnbarham added a comment.

There's two failing tests with this change:

- VFSFromYAMLTest.ReturnsExternalPathVFSHit
- VFSFromYAMLTest.ReturnsInternalPathVFSHit

Apparently we allow relative paths in external-contents *without* specifying `overlay-relative: true`. In this case the relative paths are resolved based on the CWD at the time of the operation. Do we really want that? I believe this wouldn't have worked prior to https://reviews.llvm.org/D109128 as there was no canonicalising before then as well. @keith was that change intentional? If we want it then it will *require* setting CWD on each FS in OverlayFS, which I was really hoping to avoid.

To give the concrete case:

  OverlayFileSystem
    RedirectingFileSystem
      /a/foo -> bar
    RealFileSystem
      /a/bar
      /b/bar
  
  cd /a
  cat foo # /a/bar
  
  cd /b # would fail previously since it doesn't exist in RedirectingFS but would still setCWD for RealFS
  cat foo # and thus return /a/bar since RedirectingFS is still in /a

This now fails completely because OverlayFS doesn't setCWD on the underlying filesystems. So RedirectingFS has the CWD of whatever ExternalFS was at creation. In D121424 <https://reviews.llvm.org/D121424> it will fail because there's no longer any canonicalise after mapping the path in RedirectingFS (I assumed this was unintentional and missed the test case).

If we want to allow this then I think we'll need to go with the previous plan for OverlayFS, ie. keep track of filesystems that setCWD failed on and don't call them at all until they succeed again. That then doesn't allow certain cases that the current method allows.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121425