This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGfe2478d44e4f: [VFS] RedirectingFileSystem only replace path
if not already mapped (authored by bnbarham).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123398/new/
https://reviews.llvm.org/D123398
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
@@ -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<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 @@
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<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 @@
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);
}
Index: llvm/lib/Support/VirtualFileSystem.cpp
===================================================================
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ 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<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 @@
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);
Index: llvm/include/llvm/Support/VirtualFileSystem.h
===================================================================
--- llvm/include/llvm/Support/VirtualFileSystem.h
+++ 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,
Index: clang/test/VFS/external-names-multi-overlay.c
===================================================================
--- /dev/null
+++ 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': []
+}
Index: clang/lib/Basic/FileManager.cpp
===================================================================
--- clang/lib/Basic/FileManager.cpp
+++ 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();
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
