Skip to content

Commit

Permalink
[VFS] RedirectingFileSystem only replace path if not already mapped
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@bnbarham
bnbarham committed Apr 11, 2022
1 parent 8b5e4c0 commit fe2478d
Show file tree
Hide file tree
Showing 5 changed files with 139 additions and 23 deletions.
61 changes: 50 additions & 11 deletions clang/lib/Basic/FileManager.cpp
View file
Expand Up @@ -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)})
Expand All @@ -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 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();

Expand Down
37 changes: 37 additions & 0 deletions clang/test/VFS/external-names-multi-overlay.c
View file
@@ -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': []
}
11 changes: 11 additions & 0 deletions llvm/include/llvm/Support/VirtualFileSystem.h
View file
Expand Up @@ -58,6 +58,17 @@ class Status {
// 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,
Expand Down
23 changes: 17 additions & 6 deletions llvm/lib/Support/VirtualFileSystem.cpp
View file
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
Expand Down
30 changes: 24 additions & 6 deletions llvm/unittests/Support/VirtualFileSystemTest.cpp
View file
Expand Up @@ -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");
Expand All @@ -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/");
Expand All @@ -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");
Expand All @@ -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");
Expand All @@ -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(),
Expand Down Expand Up @@ -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");
Expand All @@ -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);
}
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
}
Expand Down

0 comments on commit fe2478d

Please sign in to comment.