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 @@ -287,11 +287,48 @@ // name to users (in diagnostics) and to tools that don't have access to // the VFS (in debug info and dependency '.d' files). // - // 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. + // FIXME: This is pretty complex and has some very complicated interactions + // with the rest of clang. 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. + // + // Further, it isn't *just* external names, but will also give back absolute + // paths when a relative path was requested - the check is comparing the + // name from the status, which is passed an absolute path resolved from the + // current working directory. `clang-apply-replacements` appears to depend + // on this behaviour, though it's adjusting the working directory, which is + // definitely not supported. Once that's fixed this hack should be able to + // be narrowed to only when there's an externally mapped name given back. + // + // A potential plan to remove this is as follows - + // - Add API to determine if the name has been rewritten by the VFS. + // - Fix `clang-apply-replacements` to pass down the absolute path rather + // than changing the CWD. Narrow this hack down to just externally + // mapped paths. + // - Expose the requested filename. One possibility would be to allow + // redirection-FileEntryRefs to be returned, rather than returning + // the pointed-at-FileEntryRef, and customizing `getName()` to look + // through the indirection. + // - Update callers such as `HeaderSearch::findUsableModuleForHeader()` + // to explicitly use the requested filename rather than just using + // `getName()`. + // - Add a `FileManager::getExternalPath` API for explicitly getting the + // remapped external filename when there is one available. Adopt it in + // callers like diagnostics/deps reporting instead of calling + // `getName()` directly. + // - Switch the meaning of `FileEntryRef::getName()` to get the requested + // name, not the external name. Once that sticks, revert callers that + // want the requested name back to calling `getName()`. + // - Update the VFS to always return the requested name. This could also + // return the external name, or just have an API to request it + // lazily. The latter has the benefit of making accesses of the + // external path easily tracked, but may also require extra work than + // just returning up front. + // - (Optionally) Add an API to VFS to get the external filename lazily + // and update `FileManager::getExternalPath()` to use it instead. This + // has the benefit of making such accesses easily tracked, though isn't + // necessarily required (and could cause extra work than just adding to + // eg. `vfs::Status` up front). auto &Redirection = *SeenFileEntries .insert({Status.getName(), FileEntryRef::MapValue(*UFE, DirInfo)}) @@ -312,12 +349,14 @@ FileEntryRef ReturnedRef(*NamedFileEnt); if (ReusingEntry) { // 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. + // 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. + // + // See above for how this will eventually be removed. `IsVFSMapped` + // *cannot* be narrowed to `ExposesExternalVFSPath` as crash reproducers + // also depend on this logic and they have `use-external-paths: false`. if (&DirInfo.getDirEntry() != UFE->Dir && Status.IsVFSMapped) UFE->Dir = &DirInfo.getDirEntry(); 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,37 @@ +// 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" -e "s@REDIRECT_WITH@fallthrough@g" %t/vfs/base.yaml > %t/vfs/a-b-ft.yaml +// RUN: sed -e "s@NAME_DIR@%{/t:regex_replacement}/A@g" -e "s@EXTERNAL_DIR@%{/t:regex_replacement}/B@g" -e "s@REDIRECT_WITH@fallback@g" %t/vfs/base.yaml > %t/vfs/a-b-fb.yaml + +// Check that the external name is given when multiple overlays are provided + +// RUN: %clang_cc1 -Werror -I %t/A -ivfsoverlay %t/vfs/a-b-ft.yaml -ivfsoverlay %t/vfs/empty.yaml -fsyntax-only -E -C %t/main.c 2>&1 | FileCheck --check-prefix=FROM_B %s +// RUN: %clang_cc1 -Werror -I %t/A -ivfsoverlay %t/vfs/a-b-fb.yaml -ivfsoverlay %t/vfs/empty.yaml -fsyntax-only -E -C %t/main.c 2>&1 | FileCheck --check-prefix=FROM_B %s +// RUN: %clang_cc1 -Werror -I %t/B -ivfsoverlay %t/vfs/a-b-ft.yaml -ivfsoverlay %t/vfs/empty.yaml -fsyntax-only -E -C %t/main.c 2>&1 | FileCheck --check-prefix=FROM_B %s +// RUN: %clang_cc1 -Werror -I %t/B -ivfsoverlay %t/vfs/a-b-fb.yaml -ivfsoverlay %t/vfs/empty.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 + +//--- main.c +#include "Header.h" + +//--- B/Header.h +// Header.h in B + +//--- vfs/base.yaml +{ + 'version': 0, + 'redirecting-with': 'REDIRECT_WITH', + 'roots': [ + { 'name': 'NAME_DIR', + 'type': 'directory-remap', + 'external-contents': 'EXTERNAL_DIR' + } + ] +} + +//--- vfs/empty.yaml +{ + 'version': 0, + 'roots': [] +} 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 @@ -58,6 +58,17 @@ // 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) - see + /// FileManager::getFileRef for how how we plan to fix this. + bool ExposesExternalVFSPath = false; + Status() = default; Status(const llvm::sys::fs::file_status &Status); Status(const Twine &Name, llvm::sys::fs::UniqueID UID, 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,9 +2163,16 @@ static Status getRedirectedFileStatus(const Twine &OriginalPath, bool UseExternalNames, Status ExternalStatus) { + // The path has been mapped by some nested VFS and exposes an external path, + // don't override it with the original path. + if (ExternalStatus.ExposesExternalVFSPath) + return ExternalStatus; + Status S = ExternalStatus; if (!UseExternalNames) S = Status::copyWithNewName(S, OriginalPath); + else + S.ExposesExternalVFSPath = true; S.IsVFSMapped = true; return S; } @@ -2194,11 +2201,13 @@ ErrorOr RedirectingFileSystem::getExternalStatus(const Twine &CanonicalPath, const Twine &OriginalPath) const { - if (auto Result = ExternalFS->status(CanonicalPath)) { - return Result.get().copyWithNewName(Result.get(), OriginalPath); - } else { - return Result.getError(); - } + auto Result = ExternalFS->status(CanonicalPath); + + // The path has been mapped by some nested VFS, don't override it with the + // original path. + if (!Result || Result->ExposesExternalVFSPath) + return Result; + return Status::copyWithNewName(Result.get(), OriginalPath); } ErrorOr RedirectingFileSystem::status(const Twine &OriginalPath) { @@ -2268,7 +2277,9 @@ ErrorOr> File::getWithPath(ErrorOr> Result, const Twine &P) { - if (!Result) + // See \c getRedirectedFileStatus - don't update path if it's exposing an + // external path. + 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 @@ -1443,11 +1443,13 @@ 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"); @@ -1456,6 +1458,7 @@ 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/"); @@ -1468,26 +1471,30 @@ 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->IsVFSMapped); + 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_TRUE(S->IsVFSMapped); + EXPECT_FALSE(S->ExposesExternalVFSPath); + EXPECT_EQ("//root/mappeddir2/a", S->getName()); // file contents in remapped directory OpenedF = O->openFileForRead("//root/mappeddir/a"); @@ -1496,6 +1503,7 @@ 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"); @@ -1504,6 +1512,7 @@ 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(), @@ -1536,11 +1545,13 @@ 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"); @@ -1549,6 +1560,7 @@ ASSERT_FALSE(OpenedS.getError()); EXPECT_EQ("//root/foo/bar/a", OpenedS->getName()); EXPECT_TRUE(OpenedS->IsVFSMapped); + EXPECT_TRUE(OpenedS->ExposesExternalVFSPath); EXPECT_EQ(0, NumDiagnostics); } @@ -1697,11 +1709,13 @@ 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); } @@ -1737,11 +1751,13 @@ 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); } @@ -1777,11 +1793,13 @@ 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); }