FileManager: Shrink FileEntryRef to the size of a pointer

[dexonsmith]

Summary

Shrink FileEntryRef to the size of a pointer, by having it directly
reference the StringMapEntry the same way that DirectoryEntryRef
does. This makes FileEntryRef::FileEntryRef private as a side effect
(FileManager is a friend!).

There are two helper types added within FileEntryRef:

• FileEntryRef::MapValue is the type stored in FileManager::SeenFileEntries. It's a replacement for SeenFileEntryOrRedirect, where the second pointer type has been changed from StringRef* to MapEntry* (see next bullet).
• FileEntryRef::MapEntry is the instantiation of StringMapEntry<> where MapValue is stored. This is what FileEntryRef has a pointer to, in order to grab the name in addition to the value.

[dexonsmith]

I'd still appreciate feedback on the direction, but clang/test/ClangScanDeps/modules-full.cpp is failing (getting "<built-in>" listed twice under "file-deps" for each translation unit) so I'm marking this as needing changes.

[dexonsmith]

In D89488#2332998, @dexonsmith wrote:

I'd still appreciate feedback on the direction, but clang/test/ClangScanDeps/modules-full.cpp is failing (getting "<built-in>" listed twice under "file-deps" for each translation unit) so I'm marking this as needing changes.

Found the bug; it was pretty subtle. See https://reviews.llvm.org/D89508 for details.

[dexonsmith]

Rebased on top of https://reviews.llvm.org/D89508. Still some things to split apart, but the bug is fixed and this is a fair bit smaller.

[dexonsmith]

Removing some noise from the patch that I missed in the last update.

[dexonsmith]

Peeled off the clangd change into https://reviews.llvm.org/D89514 and rebased.

[dexonsmith]

Rebase on top of https://reviews.llvm.org/D89521 to further reduce noise. I think this is ready to go.

[dexonsmith]

Improved the naming of the helper types for FileEntryRef and the SeenFileEntries map and removed dead code for the old typedef. Found when working on a follow-up.

[arphaman]

clang/lib/Basic/FileManager.cpp

215–218

Can you use isa here instead of dyn_cast?

217

It looks like this accounts for one level of redirections, but the previous implementation could support multiple levels of redirections. Can multiple levels of redirections still be supported here too?

313

Can you rewrite the FIXME above, and also the assignment return makes it less readable. Maybe separate out the return onto the next line?

[dexonsmith]

Adding a test for multi-level indirection.

[dexonsmith]

clang/lib/Basic/FileManager.cpp

217

I augmented the test for getFileRef to check for multi-level redirections. It asserts both before and after this patch. Here is a partial backtrace from before:

Assertion failed: (is<T>() && "Invalid accessor called"), function get, file llvm/include/llvm/ADT/PointerUnion.h, line 188.
[...]
8  BasicTests               0x0000000103fb5ce9 clang::FileEntry* llvm::PointerUnion<clang::FileEntry*, llvm::StringRef const*>::get<clang::FileEntry*>() const + 105
9  BasicTests               0x0000000103fb4f80 clang::FileManager::getFileRef(llvm::StringRef, bool, bool) + 2240
10 BasicTests               0x0000000103d27d57 (anonymous namespace)::FileManagerTest_getFileRefReturnsCorrectNameForDifferentStatPath_Test::TestBody() + 711

Can you confirm this is the right test to add, or did you mean something different?

clang/unittests/Basic/FileManagerTest.cpp

303

@arphaman, this is the line the test asserts on (with or without my patch).

[dexonsmith]

Updated to address remaining review comments.

Note that we discussed double-redirection use case offline, and agreed it doesn't come up in practice (even with multiple RedirectingFileSystem layers) so we shouldn't support it. I added a targeted assertion and added an EXPECT_DEATH with a comment in the test.

[dexonsmith]

clang/lib/Basic/FileManager.cpp

215–218

Yup, done (comment has strangely detached from the code though...)

217

We discussed offline and agreed this double-redirection doesn't happen practice.

313

Thanks, fixed.

[thakis]

clang/include/clang/Basic/FileManager.h

101

It looks like this is too clever for gcc5.3: https://bugs.chromium.org/p/chromium/issues/detail?id=1143022

Ideas on how to work around that?

[thakis]

clang/include/clang/Basic/FileManager.h

101

Looks like the following might do it. I'll finish building locally and push this if it works:

$ git diff
diff --git a/clang/include/clang/Basic/FileManager.h b/clang/include/clang/Basic/FileManager.h
index d27b4260cca..12848f42770 100644
--- a/clang/include/clang/Basic/FileManager.h
+++ b/clang/include/clang/Basic/FileManager.h
@@ -107,7 +107,9 @@ public:
     /// VFSs that use external names. In that case, the \c FileEntryRef
     /// returned by the \c FileManager will have the external name, and not the
     /// name that was used to lookup the file.
-    llvm::PointerUnion<FileEntry *, const MapEntry *> V;
+    // The second type is really a `const MapEntry *`, but that confuses gcc5.3.
+    // Once that's no longer supported, change this back.
+    llvm::PointerUnion<FileEntry *, const void *> V;
 
     MapValue() = delete;
     MapValue(FileEntry &FE) : V(&FE) {}
diff --git a/clang/lib/Basic/FileManager.cpp b/clang/lib/Basic/FileManager.cpp
index d26ead4a5b0..4d84c3102ec 100644
--- a/clang/lib/Basic/FileManager.cpp
+++ b/clang/lib/Basic/FileManager.cpp
@@ -215,7 +215,7 @@ FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
     FileEntryRef::MapValue Value = *SeenFileInsertResult.first->second;
     if (LLVM_LIKELY(Value.V.is<FileEntry *>()))
       return FileEntryRef(*SeenFileInsertResult.first);
-    return FileEntryRef(*Value.V.get<const FileEntryRef::MapEntry *>());
+    return FileEntryRef(*reinterpret_cast<const FileEntryRef::MapEntry *>(Value.V.get<const void *>()));
   }
 
   // We've not seen this before. Fill it in.
@@ -347,7 +347,7 @@ FileManager::getVirtualFile(StringRef Filename, off_t Size,
     FileEntry *FE;
     if (LLVM_LIKELY(FE = Value.V.dyn_cast<FileEntry *>()))
       return FE;
-    return &FileEntryRef(*Value.V.get<const FileEntryRef::MapEntry *>())
+    return &FileEntryRef(*reinterpret_cast<const FileEntryRef::MapEntry *>(Value.V.get<const void *>()))
                 .getFileEntry();
   }

[dexonsmith]

clang/include/clang/Basic/FileManager.h

101

Thanks Nico!

clang/include/clang/Basic/FileManager.h

 };
 
 class FileEntry;
 
 /// A reference to a \c FileEntry that includes the name of the file as it was
 /// accessed by the FileManager's client.
 class FileEntryRef {
 public:
-  FileEntryRef() = delete;
-  FileEntryRef(StringRef Name, const FileEntry &Entry)
-      : Name(Name), Entry(&Entry) {}
-
-  const StringRef getName() const { return Name; }
-  const FileEntry &getFileEntry() const { return *Entry; }
+  const StringRef getName() const { return Entry->first(); }
+  const FileEntry &getFileEntry() const {
+    return *Entry->second->V.get<FileEntry *>();
+  }
 
   inline bool isValid() const;
   inline off_t getSize() const;
   inline unsigned getUID() const;
   inline const llvm::sys::fs::UniqueID &getUniqueID() const;
   inline time_t getModificationTime() const;
 
   friend bool operator==(const FileEntryRef &LHS, const FileEntryRef &RHS) {
-    return LHS.Entry == RHS.Entry && LHS.Name == RHS.Name;
+    return LHS.Entry == RHS.Entry;
   }
   friend bool operator!=(const FileEntryRef &LHS, const FileEntryRef &RHS) {
     return !(LHS == RHS);
   }
 
+  struct MapValue;
+
+  /// Type used in the StringMap.
+  using MapEntry = llvm::StringMapEntry<llvm::ErrorOr<MapValue>>;
+
+  /// Type stored in the StringMap.
+  struct MapValue {
+    /// The pointer at another MapEntry is used when the FileManager should
+    /// silently forward from one name to another, which occurs in Redirecting
+    /// VFSs that use external names. In that case, the \c FileEntryRef
+    /// returned by the \c FileManager will have the external name, and not the
+    /// name that was used to lookup the file.
+    llvm::PointerUnion<FileEntry *, const MapEntry *> V;
+
+    MapValue() = delete;
+    MapValue(FileEntry &FE) : V(&FE) {}
+    MapValue(MapEntry &ME) : V(&ME) {}
+  };
+
 private:
-  StringRef Name;
-  const FileEntry *Entry;
+  friend class FileManager;
+
+  FileEntryRef() = delete;
+  explicit FileEntryRef(const MapEntry &Entry)
+      : Entry(&Entry) {
+    assert(Entry.second && "Expected payload");
+    assert(Entry.second->V && "Expected non-null");
+    assert(Entry.second->V.is<FileEntry *>() && "Expected FileEntry");
+  }
+
+  const MapEntry *Entry;
 };
 
 /// Cached information about one file (either on disk
 /// or in the virtual file system).
 ///
 /// If the 'File' member is valid, then this FileEntry has an open file
 /// descriptor for the file.
 class FileEntry {
   friend class FileManager;
 
-  StringRef Name;             // Name of the file.
   std::string RealPathName;   // Real path to the file; could be empty.
   off_t Size;                 // File size in bytes.
   time_t ModTime;             // Modification time of file.
   const DirectoryEntry *Dir;  // Directory file lives in.
   llvm::sys::fs::UniqueID UniqueID;
   unsigned UID;               // A unique (small) ID for the file.
   bool IsNamedPipe;
   bool IsValid;               // Is this \c FileEntry initialized and valid?
 
   /// The open file, if it is owned by the \p FileEntry.
   mutable std::unique_ptr<llvm::vfs::File> File;
 
+  // First access name for this FileEntry.
+  //
+  // This is Optional only to allow delayed construction (FileEntryRef has no
+  // default constructor). It should always have a value in practice.
+  //
+  // TODO: remote this once everyone that needs a name uses FileEntryRef.
+  Optional<FileEntryRef> LastRef;
+
 public:
   FileEntry()
       : UniqueID(0, 0), IsNamedPipe(false), IsValid(false)
   {}
 
   FileEntry(const FileEntry &) = delete;
   FileEntry &operator=(const FileEntry &) = delete;
 
-  StringRef getName() const { return Name; }
+  StringRef getName() const { return LastRef->getName(); }
+  FileEntryRef getLastRef() const { return *LastRef; }
+
   StringRef tryGetRealPathName() const { return RealPathName; }
   bool isValid() const { return IsValid; }
   off_t getSize() const { return Size; }
   unsigned getUID() const { return UID; }
   const llvm::sys::fs::UniqueID &getUniqueID() const { return UniqueID; }
   time_t getModificationTime() const { return ModTime; }
 
   /// Return the directory the file lives in.
class FileManager : public RefCountedBase<FileManager> {
   /// The actual Entries for real directories/files are
   /// owned by UniqueRealDirs/UniqueRealFiles above, while the Entries
   /// for virtual directories/files are owned by
   /// VirtualDirectoryEntries/VirtualFileEntries above.
   ///
   llvm::StringMap<llvm::ErrorOr<DirectoryEntry &>, llvm::BumpPtrAllocator>
   SeenDirEntries;
 
-  /// A reference to the file entry that is associated with a particular
-  /// filename, or a reference to another filename that should be looked up
-  /// instead of the accessed filename.
-  ///
-  /// The reference to another filename is specifically useful for Redirecting
-  /// VFSs that use external names. In that case, the \c FileEntryRef returned
-  /// by the \c FileManager will have the external name, and not the name that
-  /// was used to lookup the file.
-  using SeenFileEntryOrRedirect =
-      llvm::PointerUnion<FileEntry *, const StringRef *>;
-
   /// A cache that maps paths to file entries (either real or
   /// virtual) we have looked up, or an error that occurred when we looked up
   /// the file.
   ///
   /// \see SeenDirEntries
-  llvm::StringMap<llvm::ErrorOr<SeenFileEntryOrRedirect>,
-                  llvm::BumpPtrAllocator>
+  llvm::StringMap<llvm::ErrorOr<FileEntryRef::MapValue>, llvm::BumpPtrAllocator>
       SeenFileEntries;
 
+  /// A mirror of SeenFileEntries to give fake answers for getBypassFile().
+  ///
+  /// Don't bother hooking up a BumpPtrAllocator. This should be rarely used,
+  /// and only on error paths.
+  std::unique_ptr<llvm::StringMap<llvm::ErrorOr<FileEntryRef::MapValue>>>
+      SeenBypassFileEntries;
+
   /// The canonical names of files and directories .
   llvm::DenseMap<const void *, llvm::StringRef> CanonicalNames;
 
   /// Storage for canonical names that we have computed.
   llvm::BumpPtrAllocator CanonicalNameStorage;
 
   /// Each FileEntry we create is assigned a unique ID #.
   ///

clang/lib/Basic/FileManager.cpp

FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
   auto SeenFileInsertResult =
       SeenFileEntries.insert({Filename, std::errc::no_such_file_or_directory});
   if (!SeenFileInsertResult.second) {
     if (!SeenFileInsertResult.first->second)
       return llvm::errorCodeToError(
           SeenFileInsertResult.first->second.getError());
     // Construct and return and FileEntryRef, unless it's a redirect to another
     // filename.
-    SeenFileEntryOrRedirect Value = *SeenFileInsertResult.first->second;
-    FileEntry *FE;
-    if (LLVM_LIKELY(FE = Value.dyn_cast<FileEntry *>()))
-      return FileEntryRef(SeenFileInsertResult.first->first(), *FE);
-    return getFileRef(*Value.get<const StringRef *>(), openFile, CacheFailure);
+    FileEntryRef::MapValue Value = *SeenFileInsertResult.first->second;
+    if (LLVM_LIKELY(Value.V.is<FileEntry *>()))
+      return FileEntryRef(*SeenFileInsertResult.first);
+    return FileEntryRef(*Value.V.get<const FileEntryRef::MapEntry *>());
   }
 
   // We've not seen this before. Fill it in.
   ++NumFileCacheMisses;
   auto *NamedFileEnt = &*SeenFileInsertResult.first;
   assert(!NamedFileEnt->second && "should be newly-created");
 
   // Get the null-terminated file name as stored as the key of the
FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
   }
 
   assert((openFile || !F) && "undesired open file");
 
   // 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()];
 
-  NamedFileEnt->second = &UFE;
-
-  // If the name returned by getStatValue is different than Filename, re-intern
-  // the name.
-  if (Status.getName() != Filename) {
-    auto &NewNamedFileEnt =
-        *SeenFileEntries.insert({Status.getName(), &UFE}).first;
-    assert((*NewNamedFileEnt.second).get<FileEntry *>() == &UFE &&
+  if (Status.getName() == Filename) {
+    // The name matches. Set the FileEntry.
+    NamedFileEnt->second = FileEntryRef::MapValue(UFE);
+  } else {
+    // Name mismatch. We need a redirect. First grab the actual entry we want
+    // to return.
+    auto &Redirection =
+        *SeenFileEntries.insert({Status.getName(), FileEntryRef::MapValue(UFE)})
+             .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");
-    InterndFileName = NewNamedFileEnt.first().data();
-    // In addition to re-interning the name, construct a redirecting seen file
-    // entry, that will point to the name the filesystem actually wants to use.
-    StringRef *Redirect = new (CanonicalNameStorage) StringRef(InterndFileName);
-    auto SeenFileInsertResultIt = SeenFileEntries.find(Filename);
-    assert(SeenFileInsertResultIt != SeenFileEntries.end() &&
-           "unexpected SeenFileEntries cache miss");
-    SeenFileInsertResultIt->second = Redirect;
-    NamedFileEnt = &*SeenFileInsertResultIt;
+
+    // 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 (UFE.isValid()) { // 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.
     if (DirInfo != UFE.Dir && Status.IsVFSMapped)
       UFE.Dir = DirInfo;
 
-    // Always update the name to use the last name by which a file was accessed.
-    // FIXME: Neither this nor always using the first name is correct; we want
-    // to switch towards a design where we return a FileName object that
+    // 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: The Name should be removed from FileEntry once all clients
-    // adopt FileEntryRef.
-    UFE.Name = InterndFileName;
+    // FIXME: LastRef should be removed from FileEntry once all clients adopt
+    // FileEntryRef.
+    UFE.LastRef = ReturnedRef;
 
-    return FileEntryRef(InterndFileName, UFE);
+    return ReturnedRef;
   }
 
   // Otherwise, we don't have this file yet, add it.
-  UFE.Name    = InterndFileName;
+  UFE.LastRef = ReturnedRef;
   UFE.Size    = Status.getSize();
   UFE.ModTime = llvm::sys::toTimeT(Status.getLastModificationTime());
   UFE.Dir     = DirInfo;
   UFE.UID     = NextFileUID++;
   UFE.UniqueID = Status.getUniqueID();
   UFE.IsNamedPipe = Status.getType() == llvm::sys::fs::file_type::fifo_file;
   UFE.File = std::move(F);
   UFE.IsValid = true;
 
   if (UFE.File) {
     if (auto PathName = UFE.File->getName())
       fillRealPathName(&UFE, *PathName);
   } else if (!openFile) {
     // We should still fill the path even if we aren't opening the file.
     fillRealPathName(&UFE, InterndFileName);
   }
-  return FileEntryRef(InterndFileName, UFE);
+  return ReturnedRef;
 }
 
 const FileEntry *
 FileManager::getVirtualFile(StringRef Filename, off_t Size,
                             time_t ModificationTime) {
   ++NumFileLookups;
 
   // See if there is already an entry in the map for an existing file.
   auto &NamedFileEnt = *SeenFileEntries.insert(
       {Filename, std::errc::no_such_file_or_directory}).first;
   if (NamedFileEnt.second) {
-    SeenFileEntryOrRedirect Value = *NamedFileEnt.second;
+    FileEntryRef::MapValue Value = *NamedFileEnt.second;
     FileEntry *FE;
-    if (LLVM_LIKELY(FE = Value.dyn_cast<FileEntry *>()))
+    if (LLVM_LIKELY(FE = Value.V.dyn_cast<FileEntry *>()))
       return FE;
-    return getVirtualFile(*Value.get<const StringRef *>(), Size,
-                          ModificationTime);
+    return &FileEntryRef(*Value.V.get<const FileEntryRef::MapEntry *>())
+                .getFileEntry();
   }
 
   // We've not seen this before, or the file is cached as non-existent.
   ++NumFileCacheMisses;
   addAncestorsAsVirtualDirs(Filename);
   FileEntry *UFE = nullptr;
 
   // Now that all ancestors of Filename are in the cache, the
FileManager::getVirtualFile(StringRef Filename, off_t Size,
   if (!getStatValue(InterndFileName, Status, true, nullptr)) {
     UFE = &UniqueRealFiles[Status.getUniqueID()];
     Status = llvm::vfs::Status(
       Status.getName(), Status.getUniqueID(),
       llvm::sys::toTimePoint(ModificationTime),
       Status.getUser(), Status.getGroup(), Size,
       Status.getType(), Status.getPermissions());
 
-    NamedFileEnt.second = UFE;
+    NamedFileEnt.second = FileEntryRef::MapValue(*UFE);
 
     // If we had already opened this file, close it now so we don't
     // leak the descriptor. We're not going to use the file
     // descriptor anyway, since this is a virtual file.
     if (UFE->File)
       UFE->closeFile();
 
     // If we already have an entry with this inode, return it.
+    //
+    // FIXME: Surely this should add a reference by the new name, and return
+    // it instead...
     if (UFE->isValid())
       return UFE;
 
     UFE->UniqueID = Status.getUniqueID();
     UFE->IsNamedPipe = Status.getType() == llvm::sys::fs::file_type::fifo_file;
     fillRealPathName(UFE, Status.getName());
   } else {
     VirtualFileEntries.push_back(std::make_unique<FileEntry>());
     UFE = VirtualFileEntries.back().get();
-    NamedFileEnt.second = UFE;
+    NamedFileEnt.second = FileEntryRef::MapValue(*UFE);
   }
 
-  UFE->Name    = InterndFileName;
+  UFE->LastRef = FileEntryRef(NamedFileEnt);
   UFE->Size    = Size;
   UFE->ModTime = ModificationTime;
   UFE->Dir     = *DirInfo;
   UFE->UID     = NextFileUID++;
   UFE->IsValid = true;
   UFE->File.reset();
   return UFE;
 }
 
 llvm::Optional<FileEntryRef> FileManager::getBypassFile(FileEntryRef VF) {
   // Stat of the file and return nullptr if it doesn't exist.
   llvm::vfs::Status Status;
   if (getStatValue(VF.getName(), Status, /*isFile=*/true, /*F=*/nullptr))
     return None;
 
-  // Fill it in from the stat.
+  if (!SeenBypassFileEntries)
+    SeenBypassFileEntries = std::make_unique<
+        llvm::StringMap<llvm::ErrorOr<FileEntryRef::MapValue>>>();
+
+  // If we've already bypassed just use the existing one.
+  auto Insertion = SeenBypassFileEntries->insert(
+      {VF.getName(), std::errc::no_such_file_or_directory});
+  if (!Insertion.second)
+    return FileEntryRef(*Insertion.first);
+
+  // Fill in the new entry from the stat.
   BypassFileEntries.push_back(std::make_unique<FileEntry>());
   const FileEntry &VFE = VF.getFileEntry();
   FileEntry &BFE = *BypassFileEntries.back();
-  BFE.Name = VFE.getName();
+  Insertion.first->second = FileEntryRef::MapValue(BFE);
+  BFE.LastRef = FileEntryRef(*Insertion.first);
   BFE.Size = Status.getSize();
   BFE.Dir = VFE.Dir;
   BFE.ModTime = llvm::sys::toTimeT(Status.getLastModificationTime());
   BFE.UID = NextFileUID++;
   BFE.IsValid = true;
-  return FileEntryRef(VF.getName(), BFE);
+
+  // Save the entry in the bypass table and return.
+  return FileEntryRef(*Insertion.first);
 }
 
 bool FileManager::FixupRelativePath(SmallVectorImpl<char> &path) const {
   StringRef pathRef(path.data(), path.size());
 
   if (FileSystemOpts.WorkingDir.empty()
       || llvm::sys::path::is_absolute(pathRef))
     return false;

 }
 
 void FileManager::GetUniqueIDMapping(
                    SmallVectorImpl<const FileEntry *> &UIDToFiles) const {
   UIDToFiles.clear();
   UIDToFiles.resize(NextFileUID);
 
   // Map file entries
-  for (llvm::StringMap<llvm::ErrorOr<SeenFileEntryOrRedirect>,
+  for (llvm::StringMap<llvm::ErrorOr<FileEntryRef::MapValue>,
                        llvm::BumpPtrAllocator>::const_iterator
            FE = SeenFileEntries.begin(),
            FEEnd = SeenFileEntries.end();
        FE != FEEnd; ++FE)
-    if (llvm::ErrorOr<SeenFileEntryOrRedirect> Entry = FE->getValue()) {
-      if (const auto *FE = (*Entry).dyn_cast<FileEntry *>())
+    if (llvm::ErrorOr<FileEntryRef::MapValue> Entry = FE->getValue()) {
+      if (const auto *FE = Entry->V.dyn_cast<FileEntry *>())
         UIDToFiles[FE->getUID()] = FE;
     }
 
   // Map virtual file entries
   for (const auto &VFE : VirtualFileEntries)
     UIDToFiles[VFE->getUID()] = VFE.get();
 }
 

clang/lib/Basic/SourceManager.cpp

assert(FileInfos.count(SourceFile) == 0 &&
          "any parsing occurs.");
   getOverriddenFilesInfo().OverriddenFiles[SourceFile] = NewFile;
 }
 
 const FileEntry *
 SourceManager::bypassFileContentsOverride(const FileEntry &File) {
   assert(isFileOverridden(&File));
   llvm::Optional<FileEntryRef> BypassFile =
-      FileMgr.getBypassFile(FileEntryRef(File.getName(), File));
+      FileMgr.getBypassFile(File.getLastRef());
 
   // If the file can't be found in the FS, give up.
   if (!BypassFile)
     return nullptr;
 
   const FileEntry *FE = &BypassFile->getFileEntry();
   (void)getOrCreateContentCache(FE);
   return FE;

clang/unittests/Basic/FileManagerTest.cpp

TEST_F(FileManagerTest, getFileRefReturnsCorrectNameForDifferentStatPath) {
   // triggered by the RedirectingFileSystem that FileManager::getFileRef has
   // special logic for.
   auto StatCache = std::make_unique<FakeStatCache>();
   StatCache->InjectDirectory("dir", 40);
   StatCache->InjectFile("dir/f1.cpp", 41);
   StatCache->InjectFile("dir/f1-alias.cpp", 41, "dir/f1.cpp");
   StatCache->InjectFile("dir/f2.cpp", 42);
   StatCache->InjectFile("dir/f2-alias.cpp", 42, "dir/f2.cpp");
+
+  // This unintuitive rename-the-file-on-stat behaviour supports how the
+  // RedirectingFileSystem VFS layer responds to stats. However, even if you
+  // have two layers, you should only get a single filename back. As such the
+  // following stat cache behaviour is not supported (the correct stat entry
+  // for a double-redirection would be "dir/f1.cpp") and the getFileRef below
+  // should assert.
+  StatCache->InjectFile("dir/f1-alias-alias.cpp", 41, "dir/f1-alias.cpp");
+
   manager.setStatCache(std::move(StatCache));
 
-  // With F2, test accessing the non-redirected name first.
+  // With F1, test accessing the non-redirected name first.
   auto F1 = manager.getFileRef("dir/f1.cpp");
   auto F1Alias = manager.getFileRef("dir/f1-alias.cpp");
   auto F1Alias2 = manager.getFileRef("dir/f1-alias.cpp");
   ASSERT_FALSE(!F1);
   ASSERT_FALSE(!F1Alias);
   ASSERT_FALSE(!F1Alias2);
   EXPECT_EQ("dir/f1.cpp", F1->getName());
   EXPECT_EQ("dir/f1.cpp", F1->getFileEntry().getName());
   EXPECT_EQ("dir/f1.cpp", F1Alias->getName());
   EXPECT_EQ("dir/f1.cpp", F1Alias2->getName());
   EXPECT_EQ(&F1->getFileEntry(), &F1Alias->getFileEntry());
   EXPECT_EQ(&F1->getFileEntry(), &F1Alias2->getFileEntry());
 
+#if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
+  EXPECT_DEATH((void)manager.getFileRef("dir/f1-alias-alias.cpp"),
+               "filename redirected to a non-canonical filename?");
+#endif
+
   // With F2, test accessing the redirected name first.
   auto F2Alias = manager.getFileRef("dir/f2-alias.cpp");
   auto F2 = manager.getFileRef("dir/f2.cpp");
   auto F2Alias2 = manager.getFileRef("dir/f2-alias.cpp");
   ASSERT_FALSE(!F2);
   ASSERT_FALSE(!F2Alias);
   ASSERT_FALSE(!F2Alias2);
   EXPECT_EQ("dir/f2.cpp", F2->getName());
#endif
   auto Cache = std::make_unique<FakeStatCache>();
   Cache->InjectDirectory("/tmp", 42);
   Cache->InjectFile("/tmp/test", 43);
   Manager.setStatCache(std::move(Cache));
 
   // Set up a virtual file with a different size than FakeStatCache uses.
   const FileEntry *File = Manager.getVirtualFile("/tmp/test", /*Size=*/10, 0);
   ASSERT_TRUE(File);
-  FileEntryRef Ref("/tmp/test", *File);
-  EXPECT_TRUE(Ref.isValid());
-  EXPECT_EQ(Ref.getSize(), 10);
+  const FileEntry &FE = *File;
+  EXPECT_TRUE(FE.isValid());
+  EXPECT_EQ(FE.getSize(), 10);
 
   // Calling a second time should not affect the UID or size.
-  unsigned VirtualUID = Ref.getUID();
-  EXPECT_EQ(*expectedToOptional(Manager.getFileRef("/tmp/test")), Ref);
-  EXPECT_EQ(Ref.getUID(), VirtualUID);
-  EXPECT_EQ(Ref.getSize(), 10);
+  unsigned VirtualUID = FE.getUID();
+  EXPECT_EQ(
+      &FE,
+      &expectedToOptional(Manager.getFileRef("/tmp/test"))->getFileEntry());
+  EXPECT_EQ(FE.getUID(), VirtualUID);
+  EXPECT_EQ(FE.getSize(), 10);
 
   // Bypass the file.
-  llvm::Optional<FileEntryRef> BypassRef = Manager.getBypassFile(Ref);
+  llvm::Optional<FileEntryRef> BypassRef =
+      Manager.getBypassFile(File->getLastRef());
   ASSERT_TRUE(BypassRef);
   EXPECT_TRUE(BypassRef->isValid());
-  EXPECT_EQ(BypassRef->getName(), Ref.getName());
+  EXPECT_EQ("/tmp/test", BypassRef->getName());
 
   // Check that it's different in the right ways.
   EXPECT_NE(&BypassRef->getFileEntry(), File);
   EXPECT_NE(BypassRef->getUID(), VirtualUID);
-  EXPECT_NE(BypassRef->getSize(), Ref.getSize());
+  EXPECT_NE(BypassRef->getSize(), FE.getSize());
 
   // The virtual file should still be returned when searching.
-  EXPECT_EQ(*expectedToOptional(Manager.getFileRef("/tmp/test")), Ref);
+  EXPECT_EQ(
+      &FE,
+      &expectedToOptional(Manager.getFileRef("/tmp/test"))->getFileEntry());
 }
 
 } // anonymous namespace