[PATCH] D121425: [VFS] Add a new getVFSFromYAMLs API

[Ben Barham via Phabricator via llvm-commits]

bnbarham added a comment.

In D121425#3384492 <https://reviews.llvm.org/D121425#3384492>, @dexonsmith wrote:

> Can you be more detailed about the overall state at the time of `cat`, which causes it to fail?

Sure. I think there's also some confusion with names here but I'm generally trying to use the names as they are used in the overlay YAML or the name of the members in RedirectingFS.

"external-contents" is just the path that we're mapping to, so in my example above (/a/foo -> bar), "bar" is what I was referring to by "external-contents". So let's use that again -

  OverlayFileSystem
    RedirectingFileSystem
      /a/foo -> bar
  
    RealFileSystem
      /a/bar
      /b/bar

After D121423 <https://reviews.llvm.org/D121423>, `cd /a` will result in the following -

- OverlayFS: CWD set to `/a`
- RedirectingFS and RealFS: CWD unchanged, so let's assume they were `/` at the time of creation and that's what they are now

`cat foo` -

- OverlayFS canonicalises to `/a/foo` and passes that to `openFileForRead` in RedirectingFS
- RedirectingFS has a `/a/foo` -> `bar` mapping, so it now runs `openFileForRead` on its ExternalFS (which is the RealFS)
- RealFS is at `/` and there is no `/bar` and thus fails
- RedirectingFS also fails
- Back in OverlayFS now, and because we failed we now try RealFS with `/a/foo`
- RealFS has no `/a/foo`, it fails
- Back in OverlayFS, all FS have now failed, return a failure

How this used to work really depends on when we're talking about 😆. Prior to D121423 <https://reviews.llvm.org/D121423> the FS would have looked like:

  RedirectingFileSystem
    /a/foo -> bar
  ExternalFS
    RealFileSystem
      /a/bar
      /b/bar

Prior to Jonas' change, `setCWD` on RedirectingFS would run `setCWD` on `ExternalFS`. So `cd /a` would set CWD to `/a` for both RedirectingFS and RealFileSystem. Then `cat /a/foo` would give `bar` and `openFileForRead` on `ExternalFS` (RealFS) (which has `/a` CWD) would open `/a/bar`.

After Jonas' *and* Keith's change, `setCWD` no longer runs on `ExternalFS` but instead the path was canonicalised before sending it along. So `/a/foo` maps to `bar` as before, but then it would be canonicalised using `RedirectingFS` CWD and thus do an open for `/a/bar` on `ExternalFS`.

In D121425#3384499 <https://reviews.llvm.org/D121425#3384499>, @dexonsmith wrote:

> (Would this mean resolving everything in the `.yaml` file eagerly at launch? That sounds a bit scary...)

Yes, depending on what you mean by "resolving". All I mean is "prefix relative paths with CWD". The main issue with doing this is that it prohibits canonicalising paths to a virtual CWD, since you wouldn't be able to set CWD to a virtual path before making the overlay. Perhaps that's what you mean by "a bit scary" though.

We currently prefix relative paths with `ExternalContentsPrefixDir` if `overlay-relative: true` is set. That path is a combination of CWD + the directory to the overlay. This is to handle overlays that have absolute paths that we want to remap (specifically for the crash dump use case I believe). But if `overlay-relative: false` is set then we just canonicalise the path on open/etc.

I just ran a few tests with the frontend though:

- By default, paths come in relative and will become absolute from the process-wide CWD
- If `-working-directory` *is* set then paths come in absolute from `FileManager` - `setCWD` is never run

I'm not sure if there's a good reason it's done this way or it's just that CWD was only added to FileSystem relatively recently, but it does mean that that we're actually canonicalising the paths with the process-wide CWD regardless at the moment (at least from the frontend). This isn't the case if something was to use the API directly, but in that case I'm not sure that using the current CWD makes any more sense than using the CWD when the overlay is created. The semantics would be less confusing if that was the case (IMO), but again would prohibit canonicalising to a virtual CWD.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121425