[llvm] fe2478d - [VFS] RedirectingFileSystem only replace path if not already mapped
Ben Barham via llvm-commits
llvm-commits at lists.llvm.org
Mon Apr 11 14:53:26 PDT 2022
Author: Ben Barham
Date: 2022-04-11T14:52:48-07:00
New Revision: fe2478d44e4f7f191c43fef629ac7a23d0251e72
URL: https://github.com/llvm/llvm-project/commit/fe2478d44e4f7f191c43fef629ac7a23d0251e72
DIFF: https://github.com/llvm/llvm-project/commit/fe2478d44e4f7f191c43fef629ac7a23d0251e72.diff
LOG: [VFS] RedirectingFileSystem only replace path if not already mapped
If the `ExternalFS` has already remapped to an external path then
`RedirectingFileSystem` should not change it to the originally provided
path. This fixes the original path always being used if multiple VFS
overlays were provided and the path wasn't found in the highest (ie.
first in the chain).
For now this is accomplished through the use of a new
`ExposesExternalVFSPath` field on `vfs::Status`. This flag is true when
the `Status` has an external path that's different from its virtual
path, ie. the contained path is the external path. See the plan in
`FileManager::getFileRef` for where this is going - eventually we won't
need `IsVFSMapped` any more and all returned paths should be virtual.
Resolves rdar://90578880 and llvm-project#53306.
Reviewed By: dexonsmith
Differential Revision: https://reviews.llvm.org/D123398
Added:
clang/test/VFS/external-names-multi-overlay.c
Modified:
clang/lib/Basic/FileManager.cpp
llvm/include/llvm/Support/VirtualFileSystem.h
llvm/lib/Support/VirtualFileSystem.cpp
llvm/unittests/Support/VirtualFileSystemTest.cpp
Removed:
################################################################################
diff --git a/clang/lib/Basic/FileManager.cpp b/clang/lib/Basic/FileManager.cpp
index baabb322482c3..b66780a1d1d1d 100644
--- a/clang/lib/Basic/FileManager.cpp
+++ b/clang/lib/Basic/FileManager.cpp
@@ -287,11 +287,48 @@ FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
// 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 @@ FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
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
diff erent 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
index 0000000000000..338131b905897
--- /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 at NAME_DIR@%{/t:regex_replacement}/A at g" -e "s at EXTERNAL_DIR@%{/t:regex_replacement}/B at g" -e "s at REDIRECT_WITH@fallthrough at g" %t/vfs/base.yaml > %t/vfs/a-b-ft.yaml
+// RUN: sed -e "s at NAME_DIR@%{/t:regex_replacement}/A at g" -e "s at EXTERNAL_DIR@%{/t:regex_replacement}/B at g" -e "s at REDIRECT_WITH@fallback at 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
index 7e93a2146286d..b475d3d1be329 100644
--- a/llvm/include/llvm/Support/VirtualFileSystem.h
+++ b/llvm/include/llvm/Support/VirtualFileSystem.h
@@ -58,6 +58,17 @@ class Status {
// FIXME: remove when files support multiple names
bool IsVFSMapped = false;
+ /// Whether this entity has an external path
diff erent 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
index cb34fa60d8018..c9273f00ad734 100644
--- a/llvm/lib/Support/VirtualFileSystem.cpp
+++ b/llvm/lib/Support/VirtualFileSystem.cpp
@@ -2163,9 +2163,16 @@ RedirectingFileSystem::lookupPathImpl(
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<Status> RedirectingFileSystem::status(
ErrorOr<Status>
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<Status> RedirectingFileSystem::status(const Twine &OriginalPath) {
@@ -2268,7 +2277,9 @@ class FileWithFixedStatus : public File {
ErrorOr<std::unique_ptr<File>>
File::getWithPath(ErrorOr<std::unique_ptr<File>> 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<std::unique_ptr<File>> F = std::move(*Result);
diff --git a/llvm/unittests/Support/VirtualFileSystemTest.cpp b/llvm/unittests/Support/VirtualFileSystemTest.cpp
index 9f73e89662419..52eba15e7a4b1 100644
--- a/llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ b/llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -1443,11 +1443,13 @@ TEST_F(VFSFromYAMLTest, MappedFiles) {
ASSERT_FALSE(S.getError());
EXPECT_EQ("//root/foo/bar/a", S->getName());
EXPECT_TRUE(S->IsVFSMapped);
+ EXPECT_TRUE(S->ExposesExternalVFSPath);
ErrorOr<vfs::Status> 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 @@ TEST_F(VFSFromYAMLTest, MappedFiles) {
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 @@ TEST_F(VFSFromYAMLTest, MappedFiles) {
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 @@ TEST_F(VFSFromYAMLTest, MappedFiles) {
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 @@ TEST_F(VFSFromYAMLTest, MappedFiles) {
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 @@ TEST_F(VFSFromYAMLTest, MappedRoot) {
ASSERT_FALSE(S.getError());
EXPECT_EQ("//root/foo/bar/a", S->getName());
EXPECT_TRUE(S->IsVFSMapped);
+ EXPECT_TRUE(S->ExposesExternalVFSPath);
ErrorOr<vfs::Status> 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 @@ TEST_F(VFSFromYAMLTest, MappedRoot) {
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 @@ TEST_F(VFSFromYAMLTest, ReturnsRequestedPathVFSMiss) {
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 @@ TEST_F(VFSFromYAMLTest, ReturnsExternalPathVFSHit) {
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 @@ TEST_F(VFSFromYAMLTest, ReturnsInternalPathVFSHit) {
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);
}
More information about the llvm-commits
mailing list