[clang] f65b0b5 - Revert "[VFS] RedirectingFileSystem only replace path if not already mapped"
Ben Barham via cfe-commits
cfe-commits at lists.llvm.org
Tue Apr 5 14:25:00 PDT 2022
Author: Ben Barham
Date: 2022-04-05T14:24:40-07:00
New Revision: f65b0b5dcfeb04e9e6794b32a075432ce3de1ccd
URL: https://github.com/llvm/llvm-project/commit/f65b0b5dcfeb04e9e6794b32a075432ce3de1ccd
DIFF: https://github.com/llvm/llvm-project/commit/f65b0b5dcfeb04e9e6794b32a075432ce3de1ccd.diff
LOG: Revert "[VFS] RedirectingFileSystem only replace path if not already mapped"
This reverts commit 3fda0edc51fd68192a30e302d45db081bb02d7f9, which
breaks crash reproducers in very specific circumstances. Specifically,
since crash reproducers have `UseExternalNames` set to false, the
`File->getFileEntry().getDir()->getName()` call in `DoFrameworkLookup`
would use the *cached* directory name instead of the directory of the
looked-up file.
The plan is to re-commit this patch but to *add*
`ExposesExternalVFSPath` rather than replace `IsVFSMapped`.
Differential Revision: https://reviews.llvm.org/D123103
Added:
Modified:
clang/lib/Basic/FileManager.cpp
llvm/include/llvm/Support/VirtualFileSystem.h
llvm/lib/Support/VirtualFileSystem.cpp
llvm/unittests/Support/VirtualFileSystemTest.cpp
Removed:
clang/test/VFS/external-names-multi-overlay.c
################################################################################
diff --git a/clang/lib/Basic/FileManager.cpp b/clang/lib/Basic/FileManager.cpp
index 7c2cc582009c3..b955e02f1711e 100644
--- a/clang/lib/Basic/FileManager.cpp
+++ b/clang/lib/Basic/FileManager.cpp
@@ -274,15 +274,12 @@ FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
if (!UFE)
UFE = new (FilesAlloc.Allocate()) FileEntry();
- // 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 {
- // We need a redirect. First grab the actual entry we want to return.
+ // Name mismatch. 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
@@ -292,11 +289,9 @@ 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. 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.
+ // 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.
auto &Redirection =
*SeenFileEntries
.insert({Status.getName(), FileEntryRef::MapValue(*UFE, DirInfo)})
@@ -317,16 +312,13 @@ 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 `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)
+ // 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)
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
deleted file mode 100644
index f481b8ad651f7..0000000000000
--- a/clang/test/VFS/external-names-multi-overlay.c
+++ /dev/null
@@ -1,39 +0,0 @@
-// 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 366c71103f34e..7e93a2146286d 100644
--- a/llvm/include/llvm/Support/VirtualFileSystem.h
+++ b/llvm/include/llvm/Support/VirtualFileSystem.h
@@ -55,19 +55,8 @@ class Status {
llvm::sys::fs::perms Perms;
public:
- /// 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;
+ // FIXME: remove when files support multiple names
+ bool IsVFSMapped = 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 14e9f0277f98b..cb34fa60d8018 100644
--- a/llvm/lib/Support/VirtualFileSystem.cpp
+++ b/llvm/lib/Support/VirtualFileSystem.cpp
@@ -2163,16 +2163,10 @@ 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);
- else
- S.ExposesExternalVFSPath = true;
+ S.IsVFSMapped = true;
return S;
}
@@ -2274,9 +2268,7 @@ class FileWithFixedStatus : public File {
ErrorOr<std::unique_ptr<File>>
File::getWithPath(ErrorOr<std::unique_ptr<File>> Result, const Twine &P) {
- // See \c getRedirectedFileStatus - don't update path if it's already been
- // mapped.
- if (!Result || (*Result)->status()->ExposesExternalVFSPath)
+ if (!Result)
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 61cc37a70e4f6..1eb7fe6b27611 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->ExposesExternalVFSPath);
+ EXPECT_TRUE(S->IsVFSMapped);
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->ExposesExternalVFSPath);
+ EXPECT_FALSE(SLower->IsVFSMapped);
// 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->ExposesExternalVFSPath);
+ EXPECT_TRUE(OpenedS->IsVFSMapped);
// 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->ExposesExternalVFSPath);
+ EXPECT_TRUE(S->IsVFSMapped);
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->ExposesExternalVFSPath);
+ EXPECT_FALSE(SLower->IsVFSMapped);
// file in remapped directory
S = O->status("//root/mappeddir/a");
ASSERT_FALSE(S.getError());
- EXPECT_FALSE(S->isDirectory());
- EXPECT_TRUE(S->ExposesExternalVFSPath);
- EXPECT_EQ("//root/foo/bar/a", S->getName());
+ ASSERT_FALSE(S->isDirectory());
+ ASSERT_TRUE(S->IsVFSMapped);
+ ASSERT_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());
- EXPECT_FALSE(S->isDirectory());
- EXPECT_FALSE(S->ExposesExternalVFSPath);
- EXPECT_EQ("//root/mappeddir2/a", S->getName());
+ ASSERT_FALSE(S->isDirectory());
+ ASSERT_TRUE(S->IsVFSMapped);
+ ASSERT_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->ExposesExternalVFSPath);
+ EXPECT_TRUE(OpenedS->IsVFSMapped);
// 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_FALSE(OpenedS->ExposesExternalVFSPath);
+ EXPECT_TRUE(OpenedS->IsVFSMapped);
// 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->ExposesExternalVFSPath);
+ EXPECT_TRUE(S->IsVFSMapped);
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->ExposesExternalVFSPath);
+ EXPECT_FALSE(SLower->IsVFSMapped);
// 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->ExposesExternalVFSPath);
+ EXPECT_TRUE(OpenedS->IsVFSMapped);
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->ExposesExternalVFSPath);
+ EXPECT_FALSE(OpenedS->IsVFSMapped);
auto DirectS = RemappedFS->status("a");
ASSERT_FALSE(DirectS.getError());
EXPECT_EQ("a", DirectS->getName());
- EXPECT_FALSE(DirectS->ExposesExternalVFSPath);
+ EXPECT_FALSE(DirectS->IsVFSMapped);
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->ExposesExternalVFSPath);
+ EXPECT_TRUE(OpenedS->IsVFSMapped);
auto DirectS = FS->status("vfsname");
ASSERT_FALSE(DirectS.getError());
EXPECT_EQ("realname", DirectS->getName());
- EXPECT_TRUE(DirectS->ExposesExternalVFSPath);
+ EXPECT_TRUE(DirectS->IsVFSMapped);
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_FALSE(OpenedS->ExposesExternalVFSPath);
+ EXPECT_TRUE(OpenedS->IsVFSMapped);
auto DirectS = FS->status("vfsname");
ASSERT_FALSE(DirectS.getError());
EXPECT_EQ("vfsname", DirectS->getName());
- EXPECT_FALSE(DirectS->ExposesExternalVFSPath);
+ EXPECT_TRUE(DirectS->IsVFSMapped);
EXPECT_EQ(0, NumDiagnostics);
}
More information about the cfe-commits
mailing list