Revert "[VFS] RedirectingFileSystem only replace path if not already mapped"

[bnbarham]

Summary

This reverts commit 3fda0edc51fd68192a30e302d45db081bb02d7f9.

[bnbarham]

This broke crash reproducers in very specific circumstances, see https://reviews.llvm.org/D123104. I'll re-commit with adding ExposesExternalVFSPath instead of replacing IsVFSMapped.

[dexonsmith]

In D123103#3428298, @bnbarham wrote:

This broke crash reproducers in very specific circumstances, see https://reviews.llvm.org/D123104. I'll re-commit with adding ExposesExternalVFSPath instead of replacing IsVFSMapped.

Please expand the commit message to include some details (such as these) so git-log is more useful. With that, LGTM.

clang/lib/Basic/FileManager.cpp

FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
 
   // It exists.  See if we have already opened a file with the same inode.
   // This occurs when one dir is symlinked to another, for example.
   FileEntry *&UFE = UniqueRealFiles[Status.getUniqueID()];
   bool ReusingEntry = UFE != nullptr;
   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
     // vfs::RedirectionFileSystem. This allows clang to report the external
     // 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. 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)})
              .first;
     assert(Redirection.second->V.is<FileEntry *>() &&
            "filename redirected to a non-canonical filename?");
     assert(Redirection.second->V.get<FileEntry *>() == UFE &&
            "filename from getStatValue() refers to wrong file");
 
     // Cache the redirection in the previously-inserted entry, still available
     // in the tentative return value.
     NamedFileEnt->second = FileEntryRef::MapValue(Redirection);
 
     // Fix the tentative return value.
     NamedFileEnt = &Redirection;
   }
 
   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 different 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.
     // FIXME: Neither this nor always using the first reference is correct; we
     // want to switch towards a design where we return a FileName object that
     // encapsulates both the name by which the file was accessed and the
     // corresponding FileEntry.
     // FIXME: LastRef should be removed from FileEntry once all clients adopt

clang/test/VFS/external-names-multi-overlay.c

-// 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'
-    }
-  ]
-}

llvm/include/llvm/Support/VirtualFileSystem.h

class Status {
   llvm::sys::TimePoint<> MTime;
   uint32_t User;
   uint32_t Group;
   uint64_t Size;
   llvm::sys::fs::file_type Type = llvm::sys::fs::file_type::status_error;
   llvm::sys::fs::perms Perms;
 
 public:
-  /// 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), 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);
   Status(const Twine &Name, llvm::sys::fs::UniqueID UID,
          llvm::sys::TimePoint<> MTime, uint32_t User, uint32_t Group,
          uint64_t Size, llvm::sys::fs::file_type Type,
          llvm::sys::fs::perms Perms);
 

llvm/lib/Support/VirtualFileSystem.cpp

RedirectingFileSystem::lookupPathImpl(
   }
 
   return make_error_code(llvm::errc::no_such_file_or_directory);
 }
 
 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;
 }
 
 ErrorOr<Status> RedirectingFileSystem::status(
     const Twine &CanonicalPath, const Twine &OriginalPath,
     const RedirectingFileSystem::LookupResult &Result) {
   if (Optional<StringRef> ExtRedirect = Result.getExternalRedirect()) {
     SmallString<256> CanonicalRemappedPath((*ExtRedirect).str());
public:
 
   void setPath(const Twine &Path) override { S = S.copyWithNewName(S, Path); }
 };
 
 } // namespace
 
 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);
   auto Name = F->get()->getName();
   if (Name && Name.get() != P.str())
     F->get()->setPath(P);
   return F;
 }

llvm/unittests/Support/VirtualFileSystemTest.cpp

TEST_F(VFSFromYAMLTest, MappedFiles) {
   IntrusiveRefCntPtr<vfs::OverlayFileSystem> O(
       new vfs::OverlayFileSystem(Lower));
   O->pushOverlay(FS);
 
   // file
   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");
   ASSERT_FALSE(OpenedF.getError());
   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/");
   ASSERT_FALSE(S.getError());
   EXPECT_TRUE(S->isDirectory());
   EXPECT_TRUE(S->equivalent(*O->status("//root/"))); // non-volatile UniqueID
 
   // remapped directory
   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");
   ASSERT_FALSE(OpenedF.getError());
   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");
   ASSERT_FALSE(OpenedF.getError());
   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(),
             llvm::errc::no_such_file_or_directory);
   EXPECT_EQ(0, NumDiagnostics);
 }
 
 TEST_F(VFSFromYAMLTest, MappedRoot) {
TEST_F(VFSFromYAMLTest, MappedRoot) {
   IntrusiveRefCntPtr<vfs::OverlayFileSystem> O(
       new vfs::OverlayFileSystem(Lower));
   O->pushOverlay(FS);
 
   // file
   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");
   ASSERT_FALSE(OpenedF.getError());
   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);
 }
 
 TEST_F(VFSFromYAMLTest, RemappedDirectoryOverlay) {
   IntrusiveRefCntPtr<DummyFileSystem> Lower(new DummyFileSystem());
   Lower->addDirectory("//root/foo");
   Lower->addRegularFile("//root/foo/a");
TEST_F(VFSFromYAMLTest, ReturnsRequestedPathVFSMiss) {
   ASSERT_FALSE(OpenedF.getError());
   llvm::ErrorOr<std::string> Name = (*OpenedF)->getName();
   ASSERT_FALSE(Name.getError());
   EXPECT_EQ("a", Name.get());
 
   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);
 }
 
 TEST_F(VFSFromYAMLTest, ReturnsExternalPathVFSHit) {
   IntrusiveRefCntPtr<vfs::InMemoryFileSystem> BaseFS(
       new vfs::InMemoryFileSystem);
   BaseFS->addFile("//root/foo/realname", 0,
TEST_F(VFSFromYAMLTest, ReturnsExternalPathVFSHit) {
   ASSERT_FALSE(OpenedF.getError());
   llvm::ErrorOr<std::string> Name = (*OpenedF)->getName();
   ASSERT_FALSE(Name.getError());
   EXPECT_EQ("realname", Name.get());
 
   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);
 }
 
 TEST_F(VFSFromYAMLTest, ReturnsInternalPathVFSHit) {
   IntrusiveRefCntPtr<vfs::InMemoryFileSystem> BaseFS(
       new vfs::InMemoryFileSystem);
   BaseFS->addFile("//root/foo/realname", 0,
TEST_F(VFSFromYAMLTest, ReturnsInternalPathVFSHit) {
   ASSERT_FALSE(OpenedF.getError());
   llvm::ErrorOr<std::string> Name = (*OpenedF)->getName();
   ASSERT_FALSE(Name.getError());
   EXPECT_EQ("vfsname", Name.get());
 
   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);
 }
 
 TEST_F(VFSFromYAMLTest, CaseInsensitive) {
   IntrusiveRefCntPtr<DummyFileSystem> Lower(new DummyFileSystem());
   Lower->addRegularFile("//root/foo/bar/a");
   IntrusiveRefCntPtr<vfs::FileSystem> FS = getFromYAMLString(