[llvm] 3fda0ed - [VFS] RedirectingFileSystem only replace path if not already mapped
Ben Barham via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 30 11:55:53 PDT 2022
Author: Ben Barham
Date: 2022-03-30T11:52:41-07:00
New Revision: 3fda0edc51fd68192a30e302d45db081bb02d7f9
URL: https://github.com/llvm/llvm-project/commit/3fda0edc51fd68192a30e302d45db081bb02d7f9
DIFF: https://github.com/llvm/llvm-project/commit/3fda0edc51fd68192a30e302d45db081bb02d7f9.diff
LOG: [VFS] RedirectingFileSystem only replace path if not already mapped
If the `ExternalFS` has already remapped a path then the
`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).
This also renames `IsVFSMapped` to `ExposesExternalVFSPath` and only
sets it if `UseExternalName` is true. This flag then represents that the
`Status` has an external path that's different from its virtual path.
Right now the contained path is still the external path, but further PRs
will change this to *always* be the virtual path. Clients that need the
external can then request it specifically.
Note that even though `ExposesExternalVFSPath` isn't set for all
VFS-mapped paths, `IsVFSMapped` was only being used by a hack in
`FileManager` that was specific to module searching. In that case
`UseExternalNames` is always `true` and so that hack still applies.
Resolves rdar://90578880 and llvm-project#53306.
Differential Revision: https://reviews.llvm.org/D122549
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 f4cf27848d7d9..d30a5f72b9836 100644
--- a/clang/lib/Basic/FileManager.cpp
+++ b/clang/lib/Basic/FileManager.cpp
@@ -270,12 +270,15 @@ FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
// 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 @@ FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
//
// 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 @@ FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
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
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.
+ //
+ // 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
index 0000000000000..f481b8ad651f7
--- /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 at NAME_DIR@%{/t:regex_replacement}/A at g" -e "s at EXTERNAL_DIR@%{/t:regex_replacement}/B at g" %t/vfs/base.yaml > %t/vfs/a-b.yaml
+// RUN: sed -e "s at NAME_DIR@%{/t:regex_replacement}/C at g" -e "s at EXTERNAL_DIR@%{/t:regex_replacement}/D at 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
index 7e93a2146286d..366c71103f34e 100644
--- a/llvm/include/llvm/Support/VirtualFileSystem.h
+++ b/llvm/include/llvm/Support/VirtualFileSystem.h
@@ -55,8 +55,19 @@ class Status {
llvm::sys::fs::perms Perms;
public:
- // 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), 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
index cb34fa60d8018..14e9f0277f98b 100644
--- a/llvm/lib/Support/VirtualFileSystem.cpp
+++ b/llvm/lib/Support/VirtualFileSystem.cpp
@@ -2163,10 +2163,16 @@ RedirectingFileSystem::lookupPathImpl(
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 @@ 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 already been
+ // mapped.
+ 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 1eb7fe6b27611..61cc37a70e4f6 100644
--- a/llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ b/llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -1442,12 +1442,12 @@ TEST_F(VFSFromYAMLTest, MappedFiles) {
ErrorOr<vfs::Status> 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<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");
@@ -1455,7 +1455,7 @@ TEST_F(VFSFromYAMLTest, MappedFiles) {
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 @@ TEST_F(VFSFromYAMLTest, MappedFiles) {
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 @@ TEST_F(VFSFromYAMLTest, MappedFiles) {
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 @@ TEST_F(VFSFromYAMLTest, MappedFiles) {
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 @@ TEST_F(VFSFromYAMLTest, MappedRoot) {
ErrorOr<vfs::Status> 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<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");
@@ -1548,7 +1548,7 @@ TEST_F(VFSFromYAMLTest, MappedRoot) {
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 @@ TEST_F(VFSFromYAMLTest, ReturnsRequestedPathVFSMiss) {
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 @@ TEST_F(VFSFromYAMLTest, ReturnsExternalPathVFSHit) {
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 @@ TEST_F(VFSFromYAMLTest, ReturnsInternalPathVFSHit) {
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);
}
More information about the llvm-commits
mailing list