diff --git a/clang/lib/Basic/FileManager.cpp b/clang/lib/Basic/FileManager.cpp --- a/clang/lib/Basic/FileManager.cpp +++ b/clang/lib/Basic/FileManager.cpp @@ -270,12 +270,15 @@ // This occurs when one dir is symlinked to another, for example. FileEntry &UFE = UniqueRealFiles[Status.getUniqueID()]; + // FIXME: This should just check `!Status.ExposesExternalVFSPath`, but the + // else branch also ends up fixing up relative paths to be the actually + // looked up absolute path. This isn't necessarily desired, but does seem to + // be relied on in some clients. if (Status.getName() == Filename) { // The name matches. Set the FileEntry. NamedFileEnt->second = FileEntryRef::MapValue(UFE, DirInfo); } else { - // Name mismatch. We need a redirect. First grab the actual entry we want - // to return. + // We need a redirect. First grab the actual entry we want to return. // // This redirection logic intentionally leaks the external name of a // redirected file that uses 'use-external-name' in \a @@ -285,9 +288,11 @@ // // FIXME: This is pretty complicated. It's also inconsistent with how // "real" filesystems behave and confuses parts of clang expect to see the - // name-as-accessed on the \a FileEntryRef. Maybe the returned \a - // FileEntryRef::getName() could return the accessed name unmodified, but - // make the external name available via a separate API. + // name-as-accessed on the \a FileEntryRef. To remove this we should + // implement the FIXME on `ExposesExternalVFSPath`, ie. update the + // `FileEntryRef::getName()` path to *always* be the virtual path and have + // clients request the external path only when required through a separate + // API. auto &Redirection = *SeenFileEntries .insert({Status.getName(), FileEntryRef::MapValue(UFE, DirInfo)}) @@ -308,13 +313,16 @@ FileEntryRef ReturnedRef(*NamedFileEnt); if (UFE.isValid()) { // Already have an entry with this inode, return it. - // FIXME: this hack ensures that if we look up a file by a virtual path in - // the VFS that the getDir() will have the virtual path, even if we found - // the file by a 'real' path first. This is required in order to find a - // module's structure when its headers/module map are mapped in the VFS. - // We should remove this as soon as we can properly support a file having - // multiple names. - if (&DirInfo.getDirEntry() != UFE.Dir && Status.IsVFSMapped) + // 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. + // + // This should be removed once `HeaderSearch` is updated to use `*Ref`s + // *and* the redirection hack above is removed. The removal of the latter + // is required since otherwise the ref will have the exposed external VFS + // path still. + if (&DirInfo.getDirEntry() != UFE.Dir && Status.ExposesExternalVFSPath) UFE.Dir = &DirInfo.getDirEntry(); // Always update LastRef to the last name by which a file was accessed. diff --git a/clang/test/VFS/external-names-multi-overlay.c b/clang/test/VFS/external-names-multi-overlay.c new file mode 100644 --- /dev/null +++ b/clang/test/VFS/external-names-multi-overlay.c @@ -0,0 +1,39 @@ +// RUN: rm -rf %t +// RUN: split-file %s %t +// RUN: sed -e "s@NAME_DIR@%{/t:regex_replacement}/A@g" -e "s@EXTERNAL_DIR@%{/t:regex_replacement}/B@g" %t/vfs/base.yaml > %t/vfs/a-b.yaml +// RUN: sed -e "s@NAME_DIR@%{/t:regex_replacement}/C@g" -e "s@EXTERNAL_DIR@%{/t:regex_replacement}/D@g" %t/vfs/base.yaml > %t/vfs/c-d.yaml + +// Check that the external name is given when multiple overlays are provided + +// RUN: %clang_cc1 -Werror -I %t/A -ivfsoverlay %t/vfs/c-d.yaml -ivfsoverlay %t/vfs/a-b.yaml -fsyntax-only -E -C %t/main.c 2>&1 | FileCheck --check-prefix=FROM_B %s +// FROM_B: # 1 "{{.*(/|\\\\)B(/|\\\\)}}Header.h" +// FROM_B: // Header.h in B + +// RUN: %clang_cc1 -Werror -I %t/B -ivfsoverlay %t/vfs/c-d.yaml -ivfsoverlay %t/vfs/a-b.yaml -fsyntax-only -E -C %t/main.c 2>&1 | FileCheck --check-prefix=FROM_B %s + +// RUN: %clang_cc1 -Werror -I %t/C -ivfsoverlay %t/vfs/c-d.yaml -ivfsoverlay %t/vfs/a-b.yaml -fsyntax-only -E -C %t/main.c 2>&1 | FileCheck --check-prefix=FROM_D %s +// FROM_D: # 1 "{{.*(/|\\\\)D(/|\\\\)}}Header.h" +// FROM_D: // Header.h in D + +// RUN: %clang_cc1 -Werror -I %t/C -ivfsoverlay %t/vfs/c-d.yaml -ivfsoverlay %t/vfs/a-b.yaml -fsyntax-only -E -C %t/main.c 2>&1 | FileCheck --check-prefix=FROM_D %s + +//--- main.c +#include "Header.h" + +//--- B/Header.h +// Header.h in B + +//--- D/Header.h +// Header.h in D + +//--- vfs/base.yaml +{ + 'version': 0, + 'redirecting-with': 'fallthrough', + 'roots': [ + { 'name': 'NAME_DIR', + 'type': 'directory-remap', + 'external-contents': 'EXTERNAL_DIR' + } + ] +} diff --git a/llvm/include/llvm/Support/VirtualFileSystem.h b/llvm/include/llvm/Support/VirtualFileSystem.h --- a/llvm/include/llvm/Support/VirtualFileSystem.h +++ b/llvm/include/llvm/Support/VirtualFileSystem.h @@ -55,8 +55,19 @@ llvm::sys::fs::perms Perms; public: - // FIXME: remove when files support multiple names - bool IsVFSMapped = false; + /// Whether this entity has an external path different from the virtual path, + /// and the external path is exposed by leaking it through the abstraction. + /// For example, a RedirectingFileSystem will set this for paths where + /// UseExternalName is true. + /// + /// FIXME: Currently the external path is exposed by replacing the virtual + /// path in this Status object. Instead, we should leave the path in the + /// Status intact (matching the requested virtual path), and just use this + /// flag as hint that a new API, FileSystem::getExternalVFSPath(), will not + /// return `None`. Clients can then request the external path only where + /// needed (e.g. for diagnostics, or to report discovered dependencies to + /// external tools). + bool ExposesExternalVFSPath = false; Status() = default; Status(const llvm::sys::fs::file_status &Status); diff --git a/llvm/lib/Support/VirtualFileSystem.cpp b/llvm/lib/Support/VirtualFileSystem.cpp --- a/llvm/lib/Support/VirtualFileSystem.cpp +++ b/llvm/lib/Support/VirtualFileSystem.cpp @@ -2163,10 +2163,16 @@ static Status getRedirectedFileStatus(const Twine &OriginalPath, bool UseExternalNames, Status ExternalStatus) { + // The path has been mapped by some nested VFS, don't override it with the + // original path. + if (ExternalStatus.ExposesExternalVFSPath) + return ExternalStatus; + Status S = ExternalStatus; if (!UseExternalNames) S = Status::copyWithNewName(S, OriginalPath); - S.IsVFSMapped = true; + else + S.ExposesExternalVFSPath = true; return S; } @@ -2268,7 +2274,9 @@ ErrorOr> File::getWithPath(ErrorOr> Result, const Twine &P) { - if (!Result) + // See \c getRedirectedFileStatus - don't update path if it's already been + // mapped. + if (!Result || (*Result)->status()->ExposesExternalVFSPath) return Result; ErrorOr> F = std::move(*Result); diff --git a/llvm/unittests/Support/VirtualFileSystemTest.cpp b/llvm/unittests/Support/VirtualFileSystemTest.cpp --- a/llvm/unittests/Support/VirtualFileSystemTest.cpp +++ b/llvm/unittests/Support/VirtualFileSystemTest.cpp @@ -1442,12 +1442,12 @@ ErrorOr S = O->status("//root/file1"); ASSERT_FALSE(S.getError()); EXPECT_EQ("//root/foo/bar/a", S->getName()); - EXPECT_TRUE(S->IsVFSMapped); + EXPECT_TRUE(S->ExposesExternalVFSPath); ErrorOr SLower = O->status("//root/foo/bar/a"); EXPECT_EQ("//root/foo/bar/a", SLower->getName()); EXPECT_TRUE(S->equivalent(*SLower)); - EXPECT_FALSE(SLower->IsVFSMapped); + EXPECT_FALSE(SLower->ExposesExternalVFSPath); // file after opening auto OpenedF = O->openFileForRead("//root/file1"); @@ -1455,7 +1455,7 @@ auto OpenedS = (*OpenedF)->status(); ASSERT_FALSE(OpenedS.getError()); EXPECT_EQ("//root/foo/bar/a", OpenedS->getName()); - EXPECT_TRUE(OpenedS->IsVFSMapped); + EXPECT_TRUE(OpenedS->ExposesExternalVFSPath); // directory S = O->status("//root/"); @@ -1467,27 +1467,27 @@ S = O->status("//root/mappeddir"); ASSERT_FALSE(S.getError()); EXPECT_TRUE(S->isDirectory()); - EXPECT_TRUE(S->IsVFSMapped); + EXPECT_TRUE(S->ExposesExternalVFSPath); EXPECT_TRUE(S->equivalent(*O->status("//root/foo/bar"))); SLower = O->status("//root/foo/bar"); EXPECT_EQ("//root/foo/bar", SLower->getName()); EXPECT_TRUE(S->equivalent(*SLower)); - EXPECT_FALSE(SLower->IsVFSMapped); + EXPECT_FALSE(SLower->ExposesExternalVFSPath); // file in remapped directory S = O->status("//root/mappeddir/a"); ASSERT_FALSE(S.getError()); - ASSERT_FALSE(S->isDirectory()); - ASSERT_TRUE(S->IsVFSMapped); - ASSERT_EQ("//root/foo/bar/a", S->getName()); + EXPECT_FALSE(S->isDirectory()); + EXPECT_TRUE(S->ExposesExternalVFSPath); + EXPECT_EQ("//root/foo/bar/a", S->getName()); // file in remapped directory, with use-external-name=false S = O->status("//root/mappeddir2/a"); ASSERT_FALSE(S.getError()); - ASSERT_FALSE(S->isDirectory()); - ASSERT_TRUE(S->IsVFSMapped); - ASSERT_EQ("//root/mappeddir2/a", S->getName()); + EXPECT_FALSE(S->isDirectory()); + EXPECT_FALSE(S->ExposesExternalVFSPath); + EXPECT_EQ("//root/mappeddir2/a", S->getName()); // file contents in remapped directory OpenedF = O->openFileForRead("//root/mappeddir/a"); @@ -1495,7 +1495,7 @@ OpenedS = (*OpenedF)->status(); ASSERT_FALSE(OpenedS.getError()); EXPECT_EQ("//root/foo/bar/a", OpenedS->getName()); - EXPECT_TRUE(OpenedS->IsVFSMapped); + EXPECT_TRUE(OpenedS->ExposesExternalVFSPath); // file contents in remapped directory, with use-external-name=false OpenedF = O->openFileForRead("//root/mappeddir2/a"); @@ -1503,7 +1503,7 @@ OpenedS = (*OpenedF)->status(); ASSERT_FALSE(OpenedS.getError()); EXPECT_EQ("//root/mappeddir2/a", OpenedS->getName()); - EXPECT_TRUE(OpenedS->IsVFSMapped); + EXPECT_FALSE(OpenedS->ExposesExternalVFSPath); // broken mapping EXPECT_EQ(O->status("//root/file2").getError(), @@ -1535,12 +1535,12 @@ ErrorOr S = O->status("//mappedroot/a"); ASSERT_FALSE(S.getError()); EXPECT_EQ("//root/foo/bar/a", S->getName()); - EXPECT_TRUE(S->IsVFSMapped); + EXPECT_TRUE(S->ExposesExternalVFSPath); ErrorOr SLower = O->status("//root/foo/bar/a"); EXPECT_EQ("//root/foo/bar/a", SLower->getName()); EXPECT_TRUE(S->equivalent(*SLower)); - EXPECT_FALSE(SLower->IsVFSMapped); + EXPECT_FALSE(SLower->ExposesExternalVFSPath); // file after opening auto OpenedF = O->openFileForRead("//mappedroot/a"); @@ -1548,7 +1548,7 @@ auto OpenedS = (*OpenedF)->status(); ASSERT_FALSE(OpenedS.getError()); EXPECT_EQ("//root/foo/bar/a", OpenedS->getName()); - EXPECT_TRUE(OpenedS->IsVFSMapped); + EXPECT_TRUE(OpenedS->ExposesExternalVFSPath); EXPECT_EQ(0, NumDiagnostics); } @@ -1696,12 +1696,12 @@ auto OpenedS = (*OpenedF)->status(); ASSERT_FALSE(OpenedS.getError()); EXPECT_EQ("a", OpenedS->getName()); - EXPECT_FALSE(OpenedS->IsVFSMapped); + EXPECT_FALSE(OpenedS->ExposesExternalVFSPath); auto DirectS = RemappedFS->status("a"); ASSERT_FALSE(DirectS.getError()); EXPECT_EQ("a", DirectS->getName()); - EXPECT_FALSE(DirectS->IsVFSMapped); + EXPECT_FALSE(DirectS->ExposesExternalVFSPath); EXPECT_EQ(0, NumDiagnostics); } @@ -1736,12 +1736,12 @@ auto OpenedS = (*OpenedF)->status(); ASSERT_FALSE(OpenedS.getError()); EXPECT_EQ("realname", OpenedS->getName()); - EXPECT_TRUE(OpenedS->IsVFSMapped); + EXPECT_TRUE(OpenedS->ExposesExternalVFSPath); auto DirectS = FS->status("vfsname"); ASSERT_FALSE(DirectS.getError()); EXPECT_EQ("realname", DirectS->getName()); - EXPECT_TRUE(DirectS->IsVFSMapped); + EXPECT_TRUE(DirectS->ExposesExternalVFSPath); EXPECT_EQ(0, NumDiagnostics); } @@ -1776,12 +1776,12 @@ auto OpenedS = (*OpenedF)->status(); ASSERT_FALSE(OpenedS.getError()); EXPECT_EQ("vfsname", OpenedS->getName()); - EXPECT_TRUE(OpenedS->IsVFSMapped); + EXPECT_FALSE(OpenedS->ExposesExternalVFSPath); auto DirectS = FS->status("vfsname"); ASSERT_FALSE(DirectS.getError()); EXPECT_EQ("vfsname", DirectS->getName()); - EXPECT_TRUE(DirectS->IsVFSMapped); + EXPECT_FALSE(DirectS->ExposesExternalVFSPath); EXPECT_EQ(0, NumDiagnostics); }