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

217–219

Can you use isa here instead of dyn_cast?

219

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?

309

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

219

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

292

@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

217–219

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

219

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

309

Thanks, fixed.

[thakis]

clang/include/clang/Basic/FileManager.h

77

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

77

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

77

Thanks Nico!

clang-tools-extra/clangd/ParsedAST.cpp

void FileChanged(SourceLocation Loc, FileChangeReason Reason,
     // It'd be nice if there was a better way to identify built-in headers...
     if (Reason == FileChangeReason::ExitFile &&
         SM.getBufferOrFake(PrevFID).getBufferIdentifier() == "<built-in>")
       replay();
   }
 
   void replay() {
     for (const auto &Inc : Includes) {
-      const FileEntry *File = nullptr;
+      llvm::Optional<FileEntryRef> File;
       if (Inc.Resolved != "")
-        if (auto FE = SM.getFileManager().getFile(Inc.Resolved))
-          File = *FE;
+        File = SM.getFileManager().getFileRef(Inc.Resolved);
 
       // Re-lex the #include directive to find its interesting parts.
       auto HashLoc = SM.getComposedLoc(SM.getMainFileID(), Inc.HashOffset);
       auto HashTok = llvm::partition_point(MainFileTokens,
                                            [&HashLoc](const syntax::Token &T) {
                                              return T.location() < HashLoc;
                                            });
       assert(HashTok != MainFileTokens.end() && HashTok->kind() == tok::hash);
for (const auto &Inc : Includes) {
       // Note that we can't make use of FileTok->length/text in here as in the
       // case of angled includes this will contain tok::less instead of
       // filename. Whereas Inc.Written contains the full header name including
       // quotes/angles.
       SynthesizedFilenameTok.setLength(Inc.Written.length());
       SynthesizedFilenameTok.setKind(tok::header_name);
       SynthesizedFilenameTok.setLiteralData(Inc.Written.data());
 
+      const FileEntry *FE = File ? &File.getEntry() : nullptr;
       llvm::StringRef WrittenFilename =
           llvm::StringRef(Inc.Written).drop_front().drop_back();
       Delegate->InclusionDirective(HashTok->location(), SynthesizedIncludeTok,
                                    WrittenFilename, Inc.Written.front() == '<',
-                                   FileTok->range(SM).toCharRange(SM), File,
+                                   FileTok->range(SM).toCharRange(SM), FE,
                                    "SearchPath", "RelPath",
                                    /*Imported=*/nullptr, Inc.FileKind);
       if (File)
         // FIXME: Use correctly named FileEntryRef.
-        Delegate->FileSkipped(FileEntryRef(File->getName(), *File),
-                              SynthesizedFilenameTok, Inc.FileKind);
+        Delegate->FileSkipped(File, SynthesizedFilenameTok, Inc.FileKind);
       else {
         llvm::SmallString<1> UnusedRecovery;
         Delegate->FileNotFound(WrittenFilename, UnusedRecovery);
       }
     }
   }
 
   const std::vector<Inclusion> Includes;

clang/include/clang/Basic/FileManager.h

private:
 
   DirectoryEntryRef(
       llvm::StringMapEntry<llvm::ErrorOr<DirectoryEntry &>> *Entry)
       : Entry(Entry) {}
 
   const llvm::StringMapEntry<llvm::ErrorOr<DirectoryEntry &>> *Entry;
 };
 
+class FileEntry;
+
+/// A reference to the file entry that is associated with a particular
+/// filename, or a reference to another FileEntryRef that should be used
+/// instead.
+///
+/// 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.
+struct SeenFileTableValue {
+  using MapEntryTy = llvm::StringMapEntry<llvm::ErrorOr<SeenFileTableValue>>;
+  llvm::PointerUnion<FileEntry *, const MapEntryTy *> V;
+
+  SeenFileTableValue() = delete;
+  SeenFileTableValue(FileEntry &FE) : V(&FE) {}
+  SeenFileTableValue(MapEntryTy &ME) : V(&ME) {}
+};
+
+/// 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:
+  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;
+  }
+  friend bool operator!=(const FileEntryRef &LHS, const FileEntryRef &RHS) {
+    return !(LHS == RHS);
+  }
+
+private:
+  friend class FileManager;
+
+  FileEntryRef() = delete;
+  explicit FileEntryRef(const SeenFileTableValue::MapEntryTy &Entry)
+      : Entry(&Entry) {
+    assert(Entry.second && "Expected payload");
+    assert(Entry.second->V && "Expected non-null");
+    assert(Entry.second->V.is<FileEntry *>() && "Expected FileEntry");
+  }
+
+  const SeenFileTableValue::MapEntryTy *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.
void closeFile() const {
     File.reset(); // rely on destructor to close File
   }
 
   // Only for use in tests to see if deferred opens are happening, rather than
   // relying on RealPathName being empty.
   bool isOpenForTests() const { return File != nullptr; }
 };
 
-/// 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) {}
+bool FileEntryRef::isValid() const { return getFileEntry().isValid(); }
 
-  const StringRef getName() const { return Name; }
+off_t FileEntryRef::getSize() const { return getFileEntry().getSize(); }
 
-  bool isValid() const { return Entry->isValid(); }
+unsigned FileEntryRef::getUID() const { return getFileEntry().getUID(); }
 
-  const FileEntry &getFileEntry() const { return *Entry; }
-
-  off_t getSize() const { return Entry->getSize(); }
-
-  unsigned getUID() const { return Entry->getUID(); }
-
-  const llvm::sys::fs::UniqueID &getUniqueID() const {
-    return Entry->getUniqueID();
-  }
+const llvm::sys::fs::UniqueID &FileEntryRef::getUniqueID() const {
+  return getFileEntry().getUniqueID();
 
-  time_t getModificationTime() const { return Entry->getModificationTime(); }
-
-  friend bool operator==(const FileEntryRef &LHS, const FileEntryRef &RHS) {
-    return LHS.Entry == RHS.Entry && LHS.Name == RHS.Name;
-  }
-  friend bool operator!=(const FileEntryRef &LHS, const FileEntryRef &RHS) {
-    return !(LHS == RHS);
-  }
-
-private:
-  StringRef Name;
-  const FileEntry *Entry;
-};
+time_t FileEntryRef::getModificationTime() const {
+  return getFileEntry().getModificationTime();
 
 /// Implements support for file system lookup, file system caching,
 /// and directory search management.
 ///
 /// This also handles more advanced properties, such as uniquing files based
 /// on "inode", so that a file with two names (e.g. symlinked) will be treated
 /// as a single file.
 ///
class FileManager : public RefCountedBase<FileManager> {
   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<SeenFileTableValue>, 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<SeenFileTableValue>>>
+      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;
+    SeenFileTableValue 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);
+    if (LLVM_LIKELY(FE = Value.V.dyn_cast<FileEntry *>()))
+      return FileEntryRef(*SeenFileInsertResult.first);
+    return FileEntryRef(*Value.V.get<const SeenFileTableValue::MapEntryTy *>());
   }
 
   // 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 = SeenFileTableValue(UFE);
+  } else {
+    // Name mismatch. We need a redirect. First grab the actual entry we want
+    // to return.
+    auto &Redirection =
+        *SeenFileEntries.insert({Status.getName(), SeenFileTableValue(UFE)})
+             .first;
+    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 = SeenFileTableValue(Redirection);
+
+    // Fix the tentative return value.
+    NamedFileEnt = &Redirection;
   }
 
   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
     // 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;
-
-    return FileEntryRef(InterndFileName, UFE);
+    return *(UFE.LastRef = FileEntryRef(*NamedFileEnt));
   }
 
   // Otherwise, we don't have this file yet, add it.
-  UFE.Name    = InterndFileName;
   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 *(UFE.LastRef = FileEntryRef(*NamedFileEnt));
 }
 
 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;
+    SeenFileTableValue 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 SeenFileTableValue::MapEntryTy *>())
+                .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 = SeenFileTableValue(*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 = SeenFileTableValue(*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<SeenFileTableValue>>>();
+
+  // 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 = SeenFileTableValue(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<SeenFileTableValue>,
                        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<SeenFileTableValue> 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) {
   StatCache->InjectFile("dir/f2.cpp", 42);
   StatCache->InjectFile("dir/f2-alias.cpp", 42, "dir/f2.cpp");
   manager.setStatCache(std::move(StatCache));
 
   // With F2, 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());
#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