bnbarham created this revision.
bnbarham added reviewers: dexonsmith, keith, JDevlieghere, vsapsai, sammccall.
Herald added a subscriber: hiraditya.
Herald added a project: All.
bnbarham requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.
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 changes `IsVFSMapped` to mean the returned path is a *mapped*
path, rather than just from a virtual filesystem. But it was only being used
by a hack in `FileManager` for module searching, where `external-names: true`
is always used.
Resolves rdar://90578880 and llvm-project#53306.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D122549
Files:
clang/lib/Basic/FileManager.cpp
clang/test/VFS/external-names-multi-overlay.c
llvm/include/llvm/Support/VirtualFileSystem.h
llvm/lib/Support/VirtualFileSystem.cpp
llvm/unittests/Support/VirtualFileSystemTest.cpp
Index: llvm/unittests/Support/VirtualFileSystemTest.cpp
===================================================================
--- llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -1478,16 +1478,16 @@
// 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_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->IsVFSMapped);
+ EXPECT_EQ("//root/mappeddir2/a", S->getName());
// file contents in remapped directory
OpenedF = O->openFileForRead("//root/mappeddir/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->IsVFSMapped);
// broken mapping
EXPECT_EQ(O->status("//root/file2").getError(),
@@ -1776,12 +1776,12 @@
auto OpenedS = (*OpenedF)->status();
ASSERT_FALSE(OpenedS.getError());
EXPECT_EQ("vfsname", OpenedS->getName());
- EXPECT_TRUE(OpenedS->IsVFSMapped);
+ EXPECT_FALSE(OpenedS->IsVFSMapped);
auto DirectS = FS->status("vfsname");
ASSERT_FALSE(DirectS.getError());
EXPECT_EQ("vfsname", DirectS->getName());
- EXPECT_TRUE(DirectS->IsVFSMapped);
+ EXPECT_FALSE(DirectS->IsVFSMapped);
EXPECT_EQ(0, NumDiagnostics);
}
Index: llvm/lib/Support/VirtualFileSystem.cpp
===================================================================
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ 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.IsVFSMapped)
+ return ExternalStatus;
+
Status S = ExternalStatus;
if (!UseExternalNames)
S = Status::copyWithNewName(S, OriginalPath);
- S.IsVFSMapped = true;
+ else
+ S.IsVFSMapped = true;
return S;
}
@@ -2268,7 +2274,9 @@
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()->IsVFSMapped)
return Result;
ErrorOr<std::unique_ptr<File>> F = std::move(*Result);
Index: llvm/include/llvm/Support/VirtualFileSystem.h
===================================================================
--- llvm/include/llvm/Support/VirtualFileSystem.h
+++ llvm/include/llvm/Support/VirtualFileSystem.h
@@ -55,8 +55,12 @@
llvm::sys::fs::perms Perms;
public:
- // FIXME: remove when files support multiple names
+ /// Whether the path in this status has been mapped by a VFS to another path.
bool IsVFSMapped = false;
+ // Note: Currently used by a hack in \c FileManager and internally in
+ // \c RedirectingFileSystem. We should change this to "HasVFSMapping" and
+ // *always* return the virtual path, only providing the mapped/external path
+ // when requested.
Status() = default;
Status(const llvm::sys::fs::file_status &Status);
Index: clang/test/VFS/external-names-multi-overlay.c
===================================================================
--- /dev/null
+++ 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'
+ }
+ ]
+}
Index: clang/lib/Basic/FileManager.cpp
===================================================================
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -270,12 +270,11 @@
// This occurs when one dir is symlinked to another, for example.
FileEntry &UFE = UniqueRealFiles[Status.getUniqueID()];
- if (Status.getName() == Filename) {
+ if (!Status.IsVFSMapped) {
// 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
@@ -312,8 +311,11 @@
// 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.
+ //
+ // This can be removed once \c HeaderSearch uses Refs *and* the redirection
+ // case above is removed. That one can be removed once we always return
+ // virtual paths and anywhere that needs external paths specifically
+ // requests them.
if (&DirInfo.getDirEntry() != UFE.Dir && Status.IsVFSMapped)
UFE.Dir = &DirInfo.getDirEntry();
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
