Thanks 🙇

LGTM, thanks for doing this 🙇

The failure here is likely due to the hack in FileManager::getFileRef:

// FIXME: This hack ensures that `getDir()` will use the path that was
// used to lookup this file, even if we found a file by different path
// first. This is required in order to find a module's structure when its
// headers/module map are mapped in the VFS.

So the directory returned from the FileEntry is the *lookup* directory, but the FileEntryRef will be giving back the directory from the external path (if it has been remapped).

Failing test seems to be because . is turned into the full path at some point. It's possible that this is because getFileRef returns the absolute path (see the massive FIXME there). If that's the case we could fix that by fixing clang-apply-replacements and then change the Status.getName() == Filename to only check whether it's an externally mapped path.

Few small comments, LGTM otherwise.

Remove line for <unknown> check

Update title/message

After speaking with Ben, we decided it makes more sense to just remove the reference entirely.

Shadowing of FE almost tripped me up there 😅

Thanks for doing all these Jan!

Thanks Jan :)!

Does clang/test/VFS/external-names-multi-overlay.c need to be formatted or is it fine? It uses split-file so I'd really like to avoid the format here (makes it pretty silly).

Looks like there's more changes required for modulemap-collision.m to actually pass. I'll try figure those out when I have the time.

Added a potential plan to remove the FileManager hacks.

This broke crash reproducers in very specific circumstances, see https://reviews.llvm.org/D123104. I'll re-commit with adding ExposesExternalVFSPath instead of replacing IsVFSMapped.

Keep using the redirection hack for the time being

Rename IsVFSMapped as suggested by Duncan. Update comments.

In D122549#3412064, @dexonsmith wrote:

In D122549#3412021, @bnbarham wrote:

clang-apply-replacements/relative-paths.cpp is failing, I haven't looked into it but my guess would be that it's from the Status.getName() == Filename -> !Status.IsVFSMapped change. That seems very odd to me.

Is it just failing on Windows? I wonder (rather speculatively...) whether https://reviews.llvm.org/D121733 would help.

clang-apply-replacements/relative-paths.cpp is failing, I haven't looked into it but my guess would be that it's from the Status.getName() == Filename -> !Status.IsVFSMapped change. That seems very odd to me.

In D121733#3393546, @rnk wrote:

I've been somewhat afraid to touch this code because of symlinks. Is that something we need to worry about? Consider this path: root/symlink/../foo.h. remove_dots will turn this into root/foo.h, but if symlink points to some unrelated directory, the .. directory entry points to something other than root. I can't imagine that people rely on this behavior often, but I could be wrong.

In D121733#3392968, @ppluzhnikov wrote:

There's also some others where I wouldn't expect them to be failing in this patch, eg. the ones from / -> {{[/\\]}}.

These are failing because remove_dots (un-intuitively) also changes "foo/bar\\baz" to "foo\\bar\\baz"

Are there tests that we can't just fix to expect either / or \\?

That's what this change is doing (but there is more Winx64 cleanup pending; sorry, I should have sent this as a draft, but I am still learning the ropes here).

In D121733#3392931, @dexonsmith wrote:

However, FileManager changes sometimes have odd side effects... and it's possible that somewhere in clang relies on having FileManager::getFileRef() return precisely the same path that was requested. Tagging a few other people that have some context... please share your opinions!

Blocked on the dependents

Blocked on the dependent

We had some discussions around the issue of needing to replace the returned path with the original, but that not being possible because of use-external-names: true. I'm going to put up a separate patch that should make it possible to detect when an external path was returned and then not replace in that case. We can then use that here.

Remove RedirectingFileSystem::dump (got re-added in the rebase)

Slight description update

In D121425#3384492, @dexonsmith wrote:

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

In D121425#3384188, @bnbarham wrote:

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.

There's two failing tests with this change:

• VFSFromYAMLTest.ReturnsExternalPathVFSHit
• VFSFromYAMLTest.ReturnsInternalPathVFSHit

Merged into D121425 instead.

Re-order to be after D121425

Re-order to be before D121424 and merge with D121426

Pass absolute paths down into contained FS

Final review comments and small addition

In D121423#3383595, @dexonsmith wrote:

Do you really mean real path here (system call to realpath, that sees through symlinks), or just absolute path?

Is it only when RealFileSystem has a CWD? If RealFileSystem has a CWD, then in practice it needs to use absolute paths for calls to sys::fs since the process CWD cannot be trusted. I suspect it's just failing to model the working directory for the paths it returns (which is sometimes hard, in the presence of symlinks). Maybe that's fine for now.

Is this behaviour (overlayfilesystem's behaviour when wrapping a RealFileSystem) a regression/change or is it the same as before?

In D121423#3380920, @bnbarham wrote:

If we could resolve the path in OverlayFileSystem then we could just do a status + check that it's a directory on each FS and if so, set CWD in OverlayFileSystem and not any underlying FS.

Maybe we should just do that though with one exception - if the returned path is not the one we gave then *don't* replace the path...? External names is a bit of a hack anyway, so hack for a hack 😆?

In D121423#3376431, @dexonsmith wrote:

• The client's view of the OverlayFS *should* be a single (virtual) filesystem. But without top-level CWD modelling, we can't actually provide that. Consider:

overlay:
  base:
    /file
  memory:
    /dir/

operations:
  cd /dir
  cat ../file

This *should* give the content from /file in base. But even with our proposed changes it's not going to work correctly. I think one way to be correct would be:

• Find the "first" FS (like CWD?) where getRealPath(dirname(relativepath)) does not fail
• Then combine that real-path with filename(relativepath), and go through the overlays with that path

... but this would probably be too slow. Not sure if you have ideas on how to fix it; and I don't think it's made worse by this patch (off topic?). Maybe worth adding a FIXME or description somewhere documenting that it doesn't work.

(Regardless, I think your plan should work.)

Forgot to update dump (have a release build locally)

Address review comments

In D121426#3376442, @dexonsmith wrote:

Includes two test fixes (since chained mappings are no longer allowed)
and adds a new test for multiple overlays.

Are we sure no one needs to support chained mappings? Has there been a clang-dev discourse discussion about it already? Just concerned that some vendor might rely on being able to support this.

In D121424#3376440, @dexonsmith wrote:

Can you explain more why you don't think it's a good idea in this case?

For complicated changes, it's not uncommon to add the new API (with tests for it), and then do adoption later.

Actually, I wonder if you could even just land these as three separate commits by doing D121424 last:

• D121425: add new API (causes any created RedirectingFS to be created with these options turned off)
• D121426: change clang to use it
• D121424: remove now-unneeded RedirectingFS code and add tests proving that A->B->C case no longer works

Removed LLVM_DUMP_METHOD from .cpp

In D121424#3376381, @dexonsmith wrote:

In this case:

• I think there might be a way to re-sequence D121425 ahead of this commit.
  • The new API can be added and unit tested without deleting features from RedirectingFS.
  • There may be some tests that fail until this commit lands; you can either check for the temporary wrong behaviour and fix them in this commit, or shift those tests entirely to this commit if it's too confusing. But it'll help to see what tests suddenly start passing related to the new API.
• Maybe this commit could be made runtime-configurable (OverlayFS constructor, off-by-default, only used in the new API to start, then the old behaviour removed later), to land it ahead of D121426? Maybe that's too complicated; just pointing out the flexibility.
  • Else, I'd suggest merging D121426 with this one. The changes don't touch the same files and it's not a massive patch so I don't think it'll confuse the review much.

In D121423#3376347, @dexonsmith wrote:

I've thought of another concern: the "recursively call CWD" behaviour can result in CWD being called multiple times on a filesystem instance.

Added VFS.cpp, removed implementation from header

In D121424#3376299, @dexonsmith wrote:

This allowed:

• Overlay paths to be dependent on previous -ivfsoverlay, ie. a second -ivfsoverlay could specify a virtual path for its overlay if that path is in the first overlay.
• Paths from one overlay to affect those in an earlier, ie. if the right-most overlay specified a mapping from A -> B and the left-most a mapping from B -> C, A could now map to C.
• The real filesystem to be skipped, ie. all specified paths have to be contained in an overlay (which would still map to the real filesystem).

FileSystem also had a get/set added for the current working directory
to allow for filesystem-specific CWD rather than a process-wide CWD,
further complicating the RedirectingFileSystem implementation.

It's not clear from the description what the plan is for the RedirectingFileSystem's CWD. It sounds like you're suggesting dropping it. If so, what happens if the redirecting filesystem remaps things to a new directory /new/dir that doesn't exist in the underlying filesystem, and the client calls cd /new/dir?

In D121423#3376253, @dexonsmith wrote:

I think my explanation was a bit muddled.

• Before this patch:
  • cd /c would have returned "directory not found". Clients think we're in /.
  • cat a/x will give content aligning with /a/x.
  • This seems correct.
• After this patch:
  • cd /c has no error. Clients think we're in /c.
  • cat a/x will give content aligning with /a/x/.
  • This seems incorrect. Since the cd /c succeeded, we should get /c/a/x.

@dexonsmith added you to reviewers since you were automatically added as a subscriber :)