[clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

[benlangmuir]

Summary

Update SourceManager::ContentCache::OrigEntry to keep the original FileEntryRef, and use that to enable ModuleMap::getModuleMapFile* to return the original FileEntryRef. This change should be NFC for most users of SourceManager::ContentCache, but it could affect behaviour for users of getNameAsRequested such as in compileModuleImpl. I have not found a way to detect that difference without additional functional changes, other than incidental cases like changes from / to \ on Windows so there is no new test.

Note: this should fix the Windows failure on https://reviews.llvm.org/D134923 which was caused by an incidental getFile call mutating LastRef on the FileEntry.

[jansvoboda11]

LGTM with a couple of small suggestions.

clang/include/clang/Basic/SourceManager.h

1062

Could you wrap F in OptionalFileEntryRefDegradesToFileEntryPtr to handle the conversion for you?

clang/lib/Basic/SourceManager.cpp

402

Nit: Could use auto.

clang/lib/Lex/ModuleMap.cpp

628

Nit: Use OptionalFileEntryRefDegradesToFileEntryPtr?

647

Nit: Use OptionalFileEntryRefDegradesToFileEntryPtr?

1030

Nit: Use OptionalFileEntryRefDegradesToFileEntryPtr? Maybe we should return that from getModuleMapFileForUniquing in the first place.

[bnbarham]

clang/include/clang/Basic/SourceManager.h

146–155

I haven't checked all uses, but do we still need all of OrigEntry/ContentsEntry/Filename? Seems like a single FileEntryRef should encapsulate all the information we need there.

1062

OrigEntry is a OptionalFileEntryRefDegradesToFileEntryPtr, so this should just be able to be return slot.getFile().getContentCache().OrigEntry?

clang/lib/Lex/ModuleMap.cpp

1030

I like the idea of returning OptionalFileEntryRefDegradesToFileEntryPtr until we move all this to use FileEntryRef as well.

[benlangmuir]

• Used OptionalFileEntryRefDegradesToFileEntryPtr at two callsites
• Attempted to fix Windows path / vs \ in test.

[benlangmuir]

clang/include/clang/Basic/SourceManager.h

146–155

Yes, see my change to the FIXME above. Before we can get rid of Filename, OrigEntry has to be non-Optional, which means the code that works with virtual buffers needs to use a virtual file as well. I don't think it's a huge deal, but it seemed orthogonal from the rest of this change. ContentsEntry is unrelated, as it is not used for its filename only for its contents.

1062

Correct, this was just a leftover from before I decided to make OrigEntry use OptionalFileEntryRefDegradesToFileEntryPtr.

clang/lib/Lex/ModuleMap.cpp

1030

I would prefer to keep returning Optional<FileEntryRef>. My reasoning is that OptionalFileEntryRefDegradesToFileEntryPtr should only be used on an API like this function when it would otherwise cause a lot of unnecessary workarounds at the callsites. For ContentCache::OrigEntry, for example, that is clearly the case. For getModuleMapFileForUniquing in my latest patch there are 11 total calls to this function, and only two of them would be improved by OptionalFileEntryRefDegradesToFileEntryPtr. On balance, think it's better to just put the degrading pointer at those 2 callsites and keep the API itself clean.

[goncharov]

That change might be problematic for content addressing storages. E.g. clang/test/Driver/cl-pch-showincludes.cpp started to fail in my setup as clang/test/Driver/header{0,1,3,4}.h are all identical and can be symlinked. cc @dexonsmith

[benlangmuir]

That change might be problematic for content addressing storages. E.g. clang/test/Driver/cl-pch-showincludes.cpp started to fail in my setup as clang/test/Driver/header{0,1,3,4}.h are all identical and can be symlinked

What is the failure you're seeing? I would expect this change to make clang more consistent about symlinks, because it preserves which path was originally accessed. But maybe there's an edge case somewhere.

[dexonsmith]

In D135220#3839671, @goncharov wrote:

That change might be problematic for content addressing storages. E.g. clang/test/Driver/cl-pch-showincludes.cpp started to fail in my setup as clang/test/Driver/header{0,1,3,4}.h are all identical and can be symlinked. cc @dexonsmith

FWIW, I agree with Ben that this change seems like it should improve consistency for symlinked content. Knowing the failure mode / having reproduction steps would be helpful to track down your corner case!

(There are many subtle interactions around this stuff in clang so it’s hard to make forward progress.)

[goncharov]

In D135220#3840177, @dexonsmith wrote:

In D135220#3839671, @goncharov wrote:

That change might be problematic for content addressing storages. E.g. clang/test/Driver/cl-pch-showincludes.cpp started to fail in my setup as clang/test/Driver/header{0,1,3,4}.h are all identical and can be symlinked. cc @dexonsmith

FWIW, I agree with Ben that this change seems like it should improve consistency for symlinked content. Knowing the failure mode / having reproduction steps would be helpful to track down your corner case!

(There are many subtle interactions around this stuff in clang so it’s hard to make forward progress.)

What I see in this clang/test/Driver/cl-pch-showincludes.cpp is that e.g. for run on :25 it now emits

Note: including file: ...header2.h
Note: including file: ...header1.h
Note: including file: ...header1.h

instead of

Note: including file: ...header2.h
Note: including file: ...header1.h
Note: including file: ...header3.h

as header3 -> header1. It's possbile to make it deterministic by making headers unique though.

[dexonsmith]

In D135220#3840257, @goncharov wrote:

In D135220#3840177, @dexonsmith wrote:

In D135220#3839671, @goncharov wrote:

That change might be problematic for content addressing storages. E.g. clang/test/Driver/cl-pch-showincludes.cpp started to fail in my setup as clang/test/Driver/header{0,1,3,4}.h are all identical and can be symlinked. cc @dexonsmith

FWIW, I agree with Ben that this change seems like it should improve consistency for symlinked content. Knowing the failure mode / having reproduction steps would be helpful to track down your corner case!

(There are many subtle interactions around this stuff in clang so it’s hard to make forward progress.)

What I see in this clang/test/Driver/cl-pch-showincludes.cpp is that e.g. for run on :25 it now emits

Note: including file: ...header2.h
Note: including file: ...header1.h
Note: including file: ...header1.h

instead of

Note: including file: ...header2.h
Note: including file: ...header1.h
Note: including file: ...header3.h

as header3 -> header1. It's possbile to make it deterministic by making headers unique though.

And can you share environment details?

• What OS are you on?
• Which test files are linked to which?
• Are these symbolic links or hard links?
• (etc.)

Maybe you can even share steps to reproduce your environment?

[dexonsmith]

In D135220#3840257, @goncharov wrote:

It's possbile to make it deterministic by making headers unique though.

Also, this is the first time you've mentioned non-determinism. Is this non-deterministic?

[benlangmuir]

Okay, I was able to reproduce by symlinking all the 0-byte header files to header0 (any choice probably works). The behaviour is deterministic before and after my change.

This was only passing by luck in this setup, because it was relying on mutation of FileEntry->LastRef which mutates the path of header1 to header3. We do not actually track the difference in filename here, it just happened to match the behaviour without symlinks for this case because of the mutation. Note: the mutation is not triggered specifically by the include, it's anything that looks up that path in the file manager, so there is no guarantee that you would get header3 if something triggered accessing the filename header1 again at the wrong time.

I think my change is fine here and we should just update the test files so they will not be accidentally linked. If someone wants to work on tracking the filenames independently for each include that would be fine, but as long as we only have one name it should be the first one not the "one whose path was most recently accessed in FileManager".

@goncharov Was this the only test effected? If so, here's a fix: https://reviews.llvm.org/D135373

[goncharov]

In D135220#3840354, @benlangmuir wrote:

Okay, I was able to reproduce by symlinking all the 0-byte header files to header0 (any choice probably works). The behaviour is deterministic before and after my change.

This was only passing by luck in this setup, because it was relying on mutation of FileEntry->LastRef which mutates the path of header1 to header3. We do not actually track the difference in filename here, it just happened to match the behaviour without symlinks for this case because of the mutation. Note: the mutation is not triggered specifically by the include, it's anything that looks up that path in the file manager, so there is no guarantee that you would get header3 if something triggered accessing the filename header1 again at the wrong time.

I think my change is fine here and we should just update the test files so they will not be accidentally linked. If someone wants to work on tracking the filenames independently for each include that would be fine, but as long as we only have one name it should be the first one not the "one whose path was most recently accessed in FileManager".

@goncharov Was this the only test effected? If so, here's a fix: https://reviews.llvm.org/D135373

I believe so, thank you for the fix! (I was mostly trying to confirm that symlink setups will be fine)

[benlangmuir]

I was mostly trying to confirm that symlink setups will be fine

I don't know if we promise this is supported anywhere, but I know we've made this kind of test change to force unique files several times in the past.

[hans]

Heads up that we're seeing a problem in Chromium that bisects to this change. (http://crbug.com/1373836)

In our case what's happening is that the compiler is getting the name of an included file wrong, which ends up in the debug info (and also shows in the preprocessor output). What makes it interesting is that this only reproduces on a remote build system, not when building locally.

I see there's discussion about symlinks of identical files, and I just realized that in our case the files are indeed identical. I haven't heard back from the build system folks, but I wouldn't be surprised if they symlink these against some content-addressed storage system.

[hans]

Here's a reproducer of what's happening in our case. It's using the preprocessor because that's the easiest, but the real problem for us is that the debug info is pointing to the wrong file (and that it's different depending on the symlinkedness of the include).

$ cat /tmp/foo.h
#ifdef X
#undef X
int x;
#endif

#ifdef Y
#undef Y
int y;
#endif

$ ln -s /tmp/foo.h /tmp/bar.h

$ cat /tmp/a.cc
#define X
#include "foo.h"
#define Y
#include "bar.h"

$ build/bin/clang -E -o - /tmp/a.cc
# 1 "/tmp/a.cc"
# 1 "<built-in>" 1
# 1 "<built-in>" 3
# 437 "<built-in>" 3
# 1 "<command line>" 1
# 1 "<built-in>" 2
# 1 "/tmp/a.cc" 2

# 1 "/tmp/foo.h" 1


int x;
# 3 "/tmp/a.cc" 2

# 1 "/tmp/foo.h" 1







int y;
# 5 "/tmp/a.cc" 2

Note that the second # 1 "/tmp/foo.h" 1 should really be bar.h.

[hans]

The build system folks replied saying they're not using symlinks, but hard links for compiler inputs. Dropping the -s flag in the repro above demonstrates this.

Here's a new version of the reproducer that's a little less convoluted:

$ echo 1, 2, 3 > /tmp/foo.h
$ ln /tmp/foo.h /tmp/bar.h
$ cat > /tmp/a.cc
int foo_table[] = {
#include "/tmp/foo.h"
};
int bar_table[] = {
#include "/tmp/bar.h"
};
$ build/bin/clang -E -o - /tmp/a.cc
# 1 "/tmp/a.cc"
# 1 "<built-in>" 1
# 1 "<built-in>" 3
# 437 "<built-in>" 3
# 1 "<command line>" 1
# 1 "<built-in>" 2
# 1 "/tmp/a.cc" 2
int foo_table[] = {
# 1 "/tmp/foo.h" 1
1, 2, 3
# 3 "/tmp/a.cc" 2
};
int bar_table[] = {
# 1 "/tmp/foo.h" 1                   <------ Should be bar.h.
1, 2, 3
# 6 "/tmp/a.cc" 2
};

[dexonsmith]

In D135220#3862058, @hans wrote:

The build system folks replied saying they're not using symlinks, but hard links for compiler inputs. Dropping the -s flag in the repro above demonstrates this.

I think this only happened to work before. If /tmp/a.cc changes to these file contents:

int foo_table[] = {
  #include "/tmp/foo.h"
};
int bar_table[] = {
  #include "/tmp/bar.h"
};
int foo2_table[] = {
  #include "/tmp/foo.h"
};

then I'm getting the following with my system clang:

# 1 "/tmp/a.cc"
# 1 "<built-in>" 1
# 1 "<built-in>" 3
# 414 "<built-in>" 3
# 1 "<command line>" 1
# 1 "<built-in>" 2
# 1 "/tmp/a.cc" 2
 int foo_table[] = {
# 1 "/tmp/foo.h" 1
1, 2, 3
# 3 "/tmp/a.cc" 2
 };
 int bar_table[] = {
# 1 "/tmp/bar.h" 1
1, 2, 3
# 6 "/tmp/a.cc" 2
 };
 int foo2_table[] = {
# 1 "/tmp/bar.h" 1
1, 2, 3
# 9 "/tmp/a.cc" 2
 };

Note that foo2_table now points at bar.h but should point at foo.h. I presume after the present patch it'll point at foo.h (although I admit I didn't test).

The root cause is the same, and matches @benlangmuir's analysis in response to the testcase failure on Windows:

• The FileManager is merging files based on their observed inode (regardless of symlink or hardlink).
• Previously, filenames were reported based on the most recent access path: "last-ref".
• Now, they're reported based on the original access path: "first-ref".
• In neither case is it sound.

Here are a few solutions I can see:

1. Change FileManager to only merge files with matching "realpath", seeing through symlinks but not hardlinks (could use observed inode collisions as a hint to do an lstat to avoid running an lstat every time). Then SourceManager will give a different FileID to these files.
2. Change SourceManager to have different FileID to these files, even though they were merged by FileManager.
3. Change the #include stack to track the accessed filename (the FileEntryRef) in addition to the SLoc, even though they have the same FileID.
4. Require clients to keep contents distinct if they it matters (the (surprising) status quo).

What's in tree (and has been for a long time) is (4). (1) seems like the right solution to me.

As a heuristic on top of (4), "last-ref" (before this patch) probably gets the answer the user expects more often than "first-ref" (this patch) does, which I assume is why it was originally implemented. However, it's unpredictable and requires mutating a bunch of state where mutations are hard to reason about.

IMO, "first-ref" is easier to make sound, since it's an immutable property, so this patch is an incremental improvement; it makes the fuzzy modeling easier to observe but also perhaps more predictable. If we care about fixing hardlink include names ("correct-ref") we should fix them in all cases (ideally with (1)), but I wonder how urgent it is.

@hans, WDYT?

[benlangmuir]

(catching up after I was away last week) I agree with @dexonsmith's analysis above. One historical note:

As a heuristic on top of (4), "last-ref" (before this patch) probably gets the answer the user expects more often than "first-ref" (this patch) does, which I assume is why it was originally implemented.

It was originally implemented this way because it works out more often for module-related files, and vfs overlays; debug info and preprocessor output of this kind was not considered. This is part of a workaround for lack of proper FileEntryRef support across the compiler that we should unwind over time.

[hans]

In D135220#3862893, @dexonsmith wrote:

In D135220#3862058, @hans wrote:

The build system folks replied saying they're not using symlinks, but hard links for compiler inputs. Dropping the -s flag in the repro above demonstrates this.

I think this only happened to work before. If /tmp/a.cc changes to these file contents:

int foo_table[] = {
  #include "/tmp/foo.h"
};
int bar_table[] = {
  #include "/tmp/bar.h"
};
int foo2_table[] = {
  #include "/tmp/foo.h"
};

then I'm getting the following with my system clang:

# 1 "/tmp/a.cc"
# 1 "<built-in>" 1
# 1 "<built-in>" 3
# 414 "<built-in>" 3
# 1 "<command line>" 1
# 1 "<built-in>" 2
# 1 "/tmp/a.cc" 2
 int foo_table[] = {
# 1 "/tmp/foo.h" 1
1, 2, 3
# 3 "/tmp/a.cc" 2
 };
 int bar_table[] = {
# 1 "/tmp/bar.h" 1
1, 2, 3
# 6 "/tmp/a.cc" 2
 };
 int foo2_table[] = {
# 1 "/tmp/bar.h" 1
1, 2, 3
# 9 "/tmp/a.cc" 2
 };

Note that foo2_table now points at bar.h but should point at foo.h. I presume after the present patch it'll point at foo.h (although I admit I didn't test).

Interesting, thanks!

The root cause is the same, and matches @benlangmuir's analysis in response to the testcase failure on Windows:

• The FileManager is merging files based on their observed inode (regardless of symlink or hardlink).
• Previously, filenames were reported based on the most recent access path: "last-ref".
• Now, they're reported based on the original access path: "first-ref".
• In neither case is it sound.

Agreed.

Here are a few solutions I can see:

1. Change FileManager to only merge files with matching "realpath", seeing through symlinks but not hardlinks (could use observed inode collisions as a hint to do an lstat to avoid running an lstat every time). Then SourceManager will give a different FileID to these files.
2. Change SourceManager to have different FileID to these files, even though they were merged by FileManager.
3. Change the #include stack to track the accessed filename (the FileEntryRef) in addition to the SLoc, even though they have the same FileID.
4. Require clients to keep contents distinct if they it matters (the (surprising) status quo).

What's in tree (and has been for a long time) is (4). (1) seems like the right solution to me.

As a heuristic on top of (4), "last-ref" (before this patch) probably gets the answer the user expects more often than "first-ref" (this patch) does, which I assume is why it was originally implemented. However, it's unpredictable and requires mutating a bunch of state where mutations are hard to reason about.

IMO, "first-ref" is easier to make sound, since it's an immutable property, so this patch is an incremental improvement; it makes the fuzzy modeling easier to observe but also perhaps more predictable. If we care about fixing hardlink include names ("correct-ref") we should fix them in all cases (ideally with (1)), but I wonder how urgent it is.

@hans, WDYT?

I feel I don't have enough background here to say whether (1) would work. Why is FileManager merging files in the first place?

This is not urgent (at least not for us), but it's a pretty surprising behavior.

One thought, which I'm not sure is relevant, is that this is only observable for headers which are included more than once, which is rare because normally there are include guards (or pragma once). isFileMultipleIncludeGuarded isn't tracked by the FileManager, but otherwise maybe that could have been a useful heuristic: don't merge header files without include guards?

[hans]

Relatedly, we ran into a problem with clang-cl /showIncludes not including all files in its output when they're linked: https://github.com/llvm/llvm-project/issues/58726

[dexonsmith]

In D135220#3870991, @hans wrote:

One thought, which I'm not sure is relevant, is that this is only observable for headers which are included more than once, which is rare because normally there are include guards (or pragma once). isFileMultipleIncludeGuarded isn't tracked by the FileManager, but otherwise maybe that could have been a useful heuristic: don't merge header files without include guards?

Maybe that heuristic would work a level up, in SourceManager.

• If multiple-include-guarded, create one FileID per FileEntry.
• Else, create one FileID per FileEntryRef.

Although it gets complicated with language features like #import in Objective-C, where textual inclusion is implicitly multiple-include-guarded. Consider a file included both using #include (not guarded) and as #import (guarded).

And I'm not sure we really want to split the FileIDs... that seems like a potential performance regression. Instead, I think we just want to track (at the use site) how the file was referenced.

[dexonsmith]

In D135220#3898849, @hans wrote:

Relatedly, we ran into a problem with clang-cl /showIncludes not including all files in its output when they're linked: https://github.com/llvm/llvm-project/issues/58726

Interestingly, that discussion points out that -MD works correctly:

$ clang -MD -c /tmp/a.cc && cat a.d     
a.o: /tmp/a.cc /tmp/foo.h /tmp/bar.h

I presume this is because we've pushed FileEntryRef through dependency tracking. Just need to push it through more places, I think.

[benlangmuir]

In D135220#3899674, @dexonsmith wrote:

In D135220#3898849, @hans wrote:

Relatedly, we ran into a problem with clang-cl /showIncludes not including all files in its output when they're linked: https://github.com/llvm/llvm-project/issues/58726

Interestingly, that discussion points out that -MD works correctly:

I commented on the issue about what's going on here.

clang/include/clang/Basic/FileEntry.h

public:
   }
 };
 
 static_assert(
     std::is_trivially_copyable<
         OptionalFileEntryRefDegradesToFileEntryPtr>::value,
     "OptionalFileEntryRefDegradesToFileEntryPtr should be trivially copyable");
 
+inline bool operator==(const FileEntry *LHS,
+                       const Optional<FileEntryRef> &RHS) {
+  return LHS == (RHS ? &RHS->getFileEntry() : nullptr);
+}
+inline bool operator==(const Optional<FileEntryRef> &LHS,
+                       const FileEntry *RHS) {
+  return (LHS ? &LHS->getFileEntry() : nullptr) == RHS;
+}
+inline bool operator!=(const FileEntry *LHS,
+                       const Optional<FileEntryRef> &RHS) {
+  return !(LHS == RHS);
+}
+inline bool operator!=(const Optional<FileEntryRef> &LHS,
+                       const FileEntry *RHS) {
+  return !(LHS == RHS);
+}
+
 /// 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;
   friend class FileEntryTestHelper;

clang/include/clang/Basic/SourceManager.h

 public:
   /// Reference to the file entry representing this ContentCache.
   ///
   /// This reference does not own the FileEntry object.
   ///
   /// It is possible for this to be NULL if the ContentCache encapsulates
   /// an imaginary text buffer.
   ///
-  /// FIXME: Turn this into a FileEntryRef and remove Filename.
-  const FileEntry *OrigEntry;
+  /// FIXME: Make non-optional using a virtual file as needed, remove \c
+  /// Filename and use \c OrigEntry.getNameAsRequested() instead.
+  OptionalFileEntryRefDegradesToFileEntryPtr OrigEntry;
 
   /// References the file which the contents were actually loaded from.
   ///
   /// Can be different from 'Entry' if we overridden the contents of one file
   /// with the contents of another file.
   const FileEntry *ContentsEntry;
 
   /// The filename that is used to access OrigEntry.
   ///
   /// FIXME: Remove this once OrigEntry is a FileEntryRef with a stable name.
   StringRef Filename;
 
   /// A bump pointer allocated array of offsets for each source line.
   ///
   /// This is lazily computed.  The lines are owned by the SourceManager
   /// BumpPointerAllocator object.
   mutable LineOffsetMapping SourceLineCache;
 
   /// Indicates whether the buffer itself was provided to override
public:
 
   /// True if this file may be transient, that is, if it might not
   /// exist at some later point in time when this content entry is used,
   /// after serialization and deserialization.
   unsigned IsTransient : 1;
 
   mutable unsigned IsBufferInvalid : 1;
 
-  ContentCache(const FileEntry *Ent = nullptr) : ContentCache(Ent, Ent) {}
+  ContentCache()
+      : OrigEntry(None), ContentsEntry(nullptr), BufferOverridden(false),
+        IsFileVolatile(false), IsTransient(false), IsBufferInvalid(false) {}
+
+  ContentCache(FileEntryRef Ent) : ContentCache(Ent, Ent) {}
 
-  ContentCache(const FileEntry *Ent, const FileEntry *contentEnt)
+  ContentCache(FileEntryRef Ent, const FileEntry *contentEnt)
       : OrigEntry(Ent), ContentsEntry(contentEnt), BufferOverridden(false),
         IsFileVolatile(false), IsTransient(false), IsBufferInvalid(false) {}
 
   /// The copy ctor does not allow copies where source object has either
   /// a non-NULL Buffer or SourceLineCache.  Ownership of allocated memory
   /// is not transferred, so this is a logical error.
   ContentCache(const ContentCache &RHS)
       : BufferOverridden(false), IsFileVolatile(false), IsTransient(false),
class SourceManager : public RefCountedBase<SourceManager> {
   /// True if all files read during this compilation should be treated
   /// as transient (may not be present in later compilations using a module
   /// file created from this compilation). Defaults to false.
   bool FilesAreTransient = false;
 
   struct OverriddenFilesInfoTy {
     /// Files that have been overridden with the contents from another
     /// file.
-    llvm::DenseMap<const FileEntry *, const FileEntry *> OverriddenFiles;
+    llvm::DenseMap<const FileEntry *, FileEntryRef> OverriddenFiles;
 
     /// Files that were overridden with a memory buffer.
     llvm::DenseSet<const FileEntry *> OverriddenFilesWithBuffer;
   };
 
   /// Lazily create the object keeping overridden files info, since
   /// it is uncommonly used.
   std::unique_ptr<OverriddenFilesInfoTy> OverriddenFilesInfo;
public:
   }
 
   /// Override the given source file with another one.
   ///
   /// \param SourceFile the source file which will be overridden.
   ///
   /// \param NewFile the file whose contents will be used as the
   /// data instead of the contents of the given source file.
-  void overrideFileContents(const FileEntry *SourceFile,
-                            const FileEntry *NewFile);
+  void overrideFileContents(const FileEntry *SourceFile, FileEntryRef NewFile);
 
   /// Returns true if the file contents have been overridden.
   bool isFileOverridden(const FileEntry *File) const {
     if (OverriddenFilesInfo) {
       if (OverriddenFilesInfo->OverriddenFilesWithBuffer.count(File))
         return true;
       if (OverriddenFilesInfo->OverriddenFiles.find(File) !=
           OverriddenFilesInfo->OverriddenFiles.end())
public:
   const FileEntry *getFileEntryForID(FileID FID) const {
     if (auto *Entry = getSLocEntryForFile(FID))
       return Entry->getFile().getContentCache().OrigEntry;
     return nullptr;
   }
 
   /// Returns the FileEntryRef for the provided FileID.
   Optional<FileEntryRef> getFileEntryRefForID(FileID FID) const {
-    if (auto *Entry = getFileEntryForID(FID))
-      return Entry->getLastRef();
+    if (auto *Entry = getSLocEntryForFile(FID))
+      return Entry->getFile().getContentCache().OrigEntry;
     return None;
   }
 
   /// Returns the filename for the provided FileID, unless it's a built-in
   /// buffer that's not represented by a filename.
   ///
   /// Returns None for non-files and built-in files.
   Optional<StringRef> getNonBuiltinFilenameForID(FileID FID) const;
 
   /// Returns the FileEntry record for the provided SLocEntry.
   const FileEntry *getFileEntryForSLocEntry(const SrcMgr::SLocEntry &sloc) const
   {
-    return sloc.getFile().getContentCache().OrigEntry;
+    Optional<FileEntryRef> F = sloc.getFile().getContentCache().OrigEntry;
+    return F ? &F->getFileEntry() : nullptr;
   }
 
   /// Return a StringRef to the source buffer data for the
   /// specified FileID.
   ///
   /// \param FID The file ID whose contents will be returned.
   /// \param Invalid If non-NULL, will be set true if an error occurred.
   StringRef getBufferData(FileID FID, bool *Invalid = nullptr) const;

clang/include/clang/Lex/ModuleMap.h

public:
 
   /// Retrieve the module map file containing the definition of the given
   /// module.
   ///
   /// \param Module The module whose module map file will be returned, if known.
   ///
   /// \returns The file entry for the module map file containing the given
   /// module, or nullptr if the module definition was inferred.
-  const FileEntry *getContainingModuleMapFile(const Module *Module) const;
+  Optional<FileEntryRef> getContainingModuleMapFile(const Module *Module) const;
 
   /// Get the module map file that (along with the module name) uniquely
   /// identifies this module.
   ///
   /// The particular module that \c Name refers to may depend on how the module
   /// was found in header search. However, the combination of \c Name and
   /// this module map will be globally unique for top-level modules. In the case
   /// of inferred modules, returns the module map that allowed the inference
   /// (e.g. contained 'module *'). Otherwise, returns
   /// getContainingModuleMapFile().
-  const FileEntry *getModuleMapFileForUniquing(const Module *M) const;
+  Optional<FileEntryRef> getModuleMapFileForUniquing(const Module *M) const;
 
   void setInferredModuleAllowedBy(Module *M, const FileEntry *ModMap);
 
   /// Get any module map files other than getModuleMapFileForUniquing(M)
   /// that define submodules of a top-level module \p M. This is cheaper than
   /// getting the module map file for each submodule individually, since the
   /// expected number of results is very small.
   AdditionalModMapsSet *getAdditionalModuleMapFiles(const Module *M) {

clang/lib/Basic/SourceManager.cpp

if (Entry)
     return *Entry;
 
   // Nope, create a new Cache entry.
   Entry = ContentCacheAlloc.Allocate<ContentCache>();
 
   if (OverriddenFilesInfo) {
     // If the file contents are overridden with contents from another file,
     // pass that file to ContentCache.
-    llvm::DenseMap<const FileEntry *, const FileEntry *>::iterator
-        overI = OverriddenFilesInfo->OverriddenFiles.find(FileEnt);
+    llvm::DenseMap<const FileEntry *, FileEntryRef>::iterator overI =
+        OverriddenFilesInfo->OverriddenFiles.find(FileEnt);
     if (overI == OverriddenFilesInfo->OverriddenFiles.end())
       new (Entry) ContentCache(FileEnt);
     else
       new (Entry) ContentCache(OverridenFilesKeepOriginalName ? FileEnt
                                                               : overI->second,
                                overI->second);
   } else {
     new (Entry) ContentCache(FileEnt);
void SourceManager::overrideFileContents(
 
   IR.setBuffer(std::move(Buffer));
   IR.BufferOverridden = true;
 
   getOverriddenFilesInfo().OverriddenFilesWithBuffer.insert(SourceFile);
 }
 
 void SourceManager::overrideFileContents(const FileEntry *SourceFile,
-                                         const FileEntry *NewFile) {
-  assert(SourceFile->getSize() == NewFile->getSize() &&
+                                         FileEntryRef NewFile) {
+  assert(SourceFile->getSize() == NewFile.getSize() &&
          "Different sizes, use the FileManager to create a virtual file with "
          "the correct size");
   assert(FileInfos.count(SourceFile) == 0 &&
          "This function should be called at the initialization stage, before "
          "any parsing occurs.");
-  getOverriddenFilesInfo().OverriddenFiles[SourceFile] = NewFile;
+  // FileEntryRef is not default-constructible.
+  auto Pair = getOverriddenFilesInfo().OverriddenFiles.insert(
+      std::make_pair(SourceFile, NewFile));
+  if (!Pair.second)
+    Pair.first->second = NewFile;
 }
 
 Optional<FileEntryRef>
 SourceManager::bypassFileContentsOverride(FileEntryRef File) {
   assert(isFileOverridden(&File.getFileEntry()));
   llvm::Optional<FileEntryRef> BypassFile = FileMgr.getBypassFile(File);
 
   // If the file can't be found in the FS, give up.

clang/lib/Frontend/CompilerInstance.cpp

else
       SourceMgr.overrideFileContents(
           FromFile, std::unique_ptr<llvm::MemoryBuffer>(
                         const_cast<llvm::MemoryBuffer *>(RB.second)));
   }
 
   // Remap files in the source manager (with other files).
   for (const auto &RF : InitOpts.RemappedFiles) {
     // Find the file that we're mapping to.
-    auto ToFile = FileMgr.getFile(RF.second);
+    Optional<FileEntryRef> ToFile = FileMgr.getOptionalFileRef(RF.second);
     if (!ToFile) {
       Diags.Report(diag::err_fe_remap_missing_to_file) << RF.first << RF.second;
       continue;
     }
 
     // Create the file entry for the file that we're mapping from.
     const FileEntry *FromFile =
-        FileMgr.getVirtualFile(RF.first, (*ToFile)->getSize(), 0);
+        FileMgr.getVirtualFile(RF.first, ToFile->getSize(), 0);
     if (!FromFile) {
       Diags.Report(diag::err_fe_remap_missing_from_file) << RF.first;
       continue;
     }
 
     // Override the contents of the "from" file with the contents of
     // the "to" file.
     SourceMgr.overrideFileContents(FromFile, *ToFile);
compileModuleImpl(CompilerInstance &ImportingInstance, SourceLocation ImportLoc,
   }
 
   // If \p AllowPCMWithCompilerErrors is set return 'success' even if errors
   // occurred.
   return !Instance.getDiagnostics().hasErrorOccurred() ||
          Instance.getFrontendOpts().AllowPCMWithCompilerErrors;
 }
 
-static const FileEntry *getPublicModuleMap(const FileEntry *File,
+static Optional<FileEntryRef> getPublicModuleMap(FileEntryRef File,
                                                  FileManager &FileMgr) {
-  StringRef Filename = llvm::sys::path::filename(File->getName());
-  SmallString<128> PublicFilename(File->getDir()->getName());
+  StringRef Filename = llvm::sys::path::filename(File.getName());
+  SmallString<128> PublicFilename(File.getDir().getName());
   if (Filename == "module_private.map")
     llvm::sys::path::append(PublicFilename, "module.map");
   else if (Filename == "module.private.modulemap")
     llvm::sys::path::append(PublicFilename, "module.modulemap");
   else
-    return nullptr;
-  if (auto FE = FileMgr.getFile(PublicFilename))
-    return *FE;
-  return nullptr;
+    return None;
+  return FileMgr.getOptionalFileRef(PublicFilename);
 }
 
 /// Compile a module file for the given module in a separate compiler instance,
 /// using the options provided by the importing compiler instance. Returns true
 /// if the module was built without errors.
 static bool compileModule(CompilerInstance &ImportingInstance,
                           SourceLocation ImportLoc, Module *Module,
                           StringRef ModuleFileName) {
   InputKind IK(getLanguageFromOptions(ImportingInstance.getLangOpts()),
                InputKind::ModuleMap);
 
   // Get or create the module map that we'll use to build this module.
   ModuleMap &ModMap
     = ImportingInstance.getPreprocessor().getHeaderSearchInfo().getModuleMap();
   bool Result;
-  if (const FileEntry *ModuleMapFile =
+  if (Optional<FileEntryRef> ModuleMapFile =
           ModMap.getContainingModuleMapFile(Module)) {
     // Canonicalize compilation to start with the public module map. This is
     // vital for submodules declarations in the private module maps to be
     // correctly parsed when depending on a top level module in the public one.
-    if (const FileEntry *PublicMMFile = getPublicModuleMap(
-            ModuleMapFile, ImportingInstance.getFileManager()))
+    if (Optional<FileEntryRef> PublicMMFile = getPublicModuleMap(
+            *ModuleMapFile, ImportingInstance.getFileManager()))
       ModuleMapFile = PublicMMFile;
 
-    // FIXME: Update header search to keep FileEntryRef rather than rely on
-    // getLastRef().
-    StringRef ModuleMapFilePath =
-        ModuleMapFile->getLastRef().getNameAsRequested();
+    StringRef ModuleMapFilePath = ModuleMapFile->getNameAsRequested();
 
     // Use the module map where this module resides.
     Result = compileModuleImpl(
         ImportingInstance, ImportLoc, Module->getTopLevelModuleName(),
         FrontendInputFile(ModuleMapFilePath, IK, +Module->IsSystem),
         ModMap.getModuleMapFileForUniquing(Module)->getName(), ModuleFileName);
   } else {
     // FIXME: We only need to fake up an input file here as a way of
if (Optional<FileEntryRef> ModuleMapFile =
     Result = compileModuleImpl(
         ImportingInstance, ImportLoc, Module->getTopLevelModuleName(),
         FrontendInputFile(FakeModuleMapFile, IK, +Module->IsSystem),
         ModMap.getModuleMapFileForUniquing(Module)->getName(),
         ModuleFileName,
         [&](CompilerInstance &Instance) {
       std::unique_ptr<llvm::MemoryBuffer> ModuleMapBuffer =
           llvm::MemoryBuffer::getMemBuffer(InferredModuleMapContent);
-      ModuleMapFile = Instance.getFileManager().getVirtualFile(
+      const FileEntry *ModuleMapFile = Instance.getFileManager().getVirtualFile(
           FakeModuleMapFile, InferredModuleMapContent.size(), 0);
       Instance.getSourceManager().overrideFileContents(
           ModuleMapFile, std::move(ModuleMapBuffer));
     });
   }
 
   // We've rebuilt a module. If we're allowed to generate or update the global
   // module index, record that fact in the importing compiler instance.

clang/lib/Lex/HeaderSearch.cpp

 /// Get filenames for all registered header maps.
 void HeaderSearch::getHeaderMapFileNames(
     SmallVectorImpl<std::string> &Names) const {
   for (auto &HM : HeaderMaps)
     Names.push_back(std::string(HM.first->getName()));
 }
 
 std::string HeaderSearch::getCachedModuleFileName(Module *Module) {
-  const FileEntry *ModuleMap =
+  Optional<FileEntryRef> ModuleMap =
       getModuleMap().getModuleMapFileForUniquing(Module);
   // The ModuleMap maybe a nullptr, when we load a cached C++ module without
   // *.modulemap file. In this case, just return an empty string.
-  if (ModuleMap == nullptr)
+  if (!ModuleMap)
     return {};
   return getCachedModuleFileName(Module->Name, ModuleMap->getName());
 }
 
 std::string HeaderSearch::getPrebuiltModuleFileName(StringRef ModuleName,
                                                     bool FileMapOnly) {
   // First check the module name to pcm file map.
   auto i(HSOpts->PrebuiltModuleFiles.find(ModuleName));
for (const std::string &Dir : HSOpts->PrebuiltModulePaths) {
     if (getFileMgr().getFile(Result.str()))
       return std::string(Result);
   }
 
   return {};
 }
 
 std::string HeaderSearch::getPrebuiltImplicitModuleFileName(Module *Module) {
-  const FileEntry *ModuleMap =
+  Optional<FileEntryRef> ModuleMap =
       getModuleMap().getModuleMapFileForUniquing(Module);
   StringRef ModuleName = Module->Name;
   StringRef ModuleMapPath = ModuleMap->getName();
   StringRef ModuleCacheHash = HSOpts->DisableModuleHash ? "" : getModuleHash();
   for (const std::string &Dir : HSOpts->PrebuiltModulePaths) {
     SmallString<256> CachePath(Dir);
     llvm::sys::fs::make_absolute(CachePath);
     llvm::sys::path::append(CachePath, ModuleCacheHash);

clang/lib/Lex/ModuleMap.cpp

if (H) {
 
     // Search up the module stack until we find a module with an umbrella
     // directory.
     Module *UmbrellaModule = Result;
     while (!UmbrellaModule->getUmbrellaDir() && UmbrellaModule->Parent)
       UmbrellaModule = UmbrellaModule->Parent;
 
     if (UmbrellaModule->InferSubmodules) {
-      const FileEntry *UmbrellaModuleMap =
+      Optional<FileEntryRef> UmbrellaModuleMap =
           getModuleMapFileForUniquing(UmbrellaModule);
 
       // Infer submodules for each of the directories we found between
       // the directory of the umbrella header and the directory where
       // the actual header is located.
       bool Explicit = UmbrellaModule->InferExplicitSubmodules;
 
       for (const DirectoryEntry *SkippedDir : llvm::reverse(SkippedDirs)) {
         // Find or create the module that corresponds to this directory name.
         SmallString<32> NameBuf;
         StringRef Name = sanitizeFilenameAsIdentifier(
             llvm::sys::path::stem(SkippedDir->getName()), NameBuf);
         Result = findOrCreateModule(Name, Result, /*IsFramework=*/false,
                                     Explicit).first;
-        InferredModuleAllowedBy[Result] = UmbrellaModuleMap;
+        InferredModuleAllowedBy[Result] =
+            UmbrellaModuleMap ? *UmbrellaModuleMap : nullptr;
         Result->IsInferred = true;
 
         // Associate the module and the directory.
         UmbrellaDirs[SkippedDir] = Result;
 
         // If inferred submodules export everything they import, add a
         // wildcard to the set of exports.
         if (UmbrellaModule->InferExportWildcard && Result->Exports.empty())
           Result->Exports.push_back(Module::ExportDecl(nullptr, true));
       }
 
       // Infer a submodule with the same name as this header file.
       SmallString<32> NameBuf;
       StringRef Name = sanitizeFilenameAsIdentifier(
                          llvm::sys::path::stem(File->getName()), NameBuf);
       Result = findOrCreateModule(Name, Result, /*IsFramework=*/false,
                                   Explicit).first;
-      InferredModuleAllowedBy[Result] = UmbrellaModuleMap;
+      InferredModuleAllowedBy[Result] =
+          UmbrellaModuleMap ? *UmbrellaModuleMap : nullptr;
       Result->IsInferred = true;
       Result->addTopHeader(File);
 
       // If inferred submodules export everything they import, add a
       // wildcard to the set of exports.
       if (UmbrellaModule->InferExportWildcard && Result->Exports.empty())
         Result->Exports.push_back(Module::ExportDecl(nullptr, true));
     } else {
if (llvm::sys::path::has_parent_path(FrameworkDirName)) {
           ModuleMapFile = inferred->second.ModuleMapFile;
         }
       }
     }
 
     // If we're not allowed to infer a framework module, don't.
     if (!canInfer)
       return nullptr;
-  } else
-    ModuleMapFile = getModuleMapFileForUniquing(Parent);
-
+  } else {
+    Optional<FileEntryRef> ModuleMapRef = getModuleMapFileForUniquing(Parent);
+    ModuleMapFile = ModuleMapRef ? *ModuleMapRef : nullptr;
+  }
 
   // Look for an umbrella header.
   SmallString<128> UmbrellaName = StringRef(FrameworkDir->getName());
   llvm::sys::path::append(UmbrellaName, "Headers", ModuleName + ".h");
   auto UmbrellaHeader = FileMgr.getFile(UmbrellaName);
 
   // FIXME: If there's no umbrella header, we could probably scan the
   // framework to load *everything*. But, it's not clear that this is a good
void ModuleMap::excludeHeader(Module *Mod, Module::Header Header) {
   // umbrella directory module.
   // FIXME: Should we only exclude it from umbrella modules within the
   // specified module?
   Headers[Header.Entry].push_back(KH);
 
   Mod->Headers[Module::HK_Excluded].push_back(std::move(Header));
 }
 
-const FileEntry *
+Optional<FileEntryRef>
 ModuleMap::getContainingModuleMapFile(const Module *Module) const {
   if (Module->DefinitionLoc.isInvalid())
-    return nullptr;
+    return None;
 
-  return SourceMgr.getFileEntryForID(
-           SourceMgr.getFileID(Module->DefinitionLoc));
+  return SourceMgr.getFileEntryRefForID(
 }
 
-const FileEntry *ModuleMap::getModuleMapFileForUniquing(const Module *M) const {
+Optional<FileEntryRef>
+ModuleMap::getModuleMapFileForUniquing(const Module *M) const {
   if (M->IsInferred) {
     assert(InferredModuleAllowedBy.count(M) && "missing inferred module map");
-    return InferredModuleAllowedBy.find(M)->second;
+    // FIXME: Update InferredModuleAllowedBy to use FileEntryRef.
+    return InferredModuleAllowedBy.find(M)->second->getLastRef();
   }
   return getContainingModuleMapFile(M);
 }
 
 void ModuleMap::setInferredModuleAllowedBy(Module *M, const FileEntry *ModMap) {
   assert(M->IsInferred && "module not inferred");
   InferredModuleAllowedBy[M] = ModMap;
 }

clang/lib/Serialization/ASTReader.cpp

ASTReader::ReadModuleMapFileBlock(RecordData &Record, ModuleFile &F,
   assert(!F.ModuleName.empty() &&
          "MODULE_NAME should come before MODULE_MAP_FILE");
   if (F.Kind == MK_ImplicitModule && ModuleMgr.begin()->Kind != MK_MainFile) {
     // An implicitly-loaded module file should have its module listed in some
     // module map file that we've already loaded.
     Module *M =
         PP.getHeaderSearchInfo().lookupModule(F.ModuleName, F.ImportLoc);
     auto &Map = PP.getHeaderSearchInfo().getModuleMap();
-    const FileEntry *ModMap = M ? Map.getModuleMapFileForUniquing(M) : nullptr;
+    Optional<FileEntryRef> ModMap =
+        M ? Map.getModuleMapFileForUniquing(M) : None;
     // Don't emit module relocation error if we have -fno-validate-pch
     if (!bool(PP.getPreprocessorOpts().DisablePCHOrModuleValidation &
               DisableValidationForModuleKind::Module) &&
         !ModMap) {
       if (!canRecoverFromOutOfDate(F.FileName, ClientLoadCapabilities)) {
         if (auto ASTFE = M ? M->getASTFile() : None) {
           // This module was defined by an imported (explicit) module.
           Diag(diag::err_module_file_conflict) << F.ModuleName << F.FileName

clang/lib/Serialization/ASTWriter.cpp

for (const auto &KH : HS.findAllModulesForHeader(File)) {
       ModulesToProcess.push_back(KH.getModule());
     }
   }
 
   while (!ModulesToProcess.empty()) {
     auto *CurrentModule = ModulesToProcess.pop_back_val();
     ProcessedModules.insert(CurrentModule);
 
-    auto *ModuleMapFile =
+    Optional<FileEntryRef> ModuleMapFile =
         HS.getModuleMap().getModuleMapFileForUniquing(CurrentModule);
     if (!ModuleMapFile) {
       continue;
     }
 
-    ModuleMaps.insert(ModuleMapFile);
+    ModuleMaps.insert(*ModuleMapFile);
 
     for (auto *ImportedModule : (CurrentModule)->Imports) {
       if (!ImportedModule ||
           ProcessedModules.find(ImportedModule) != ProcessedModules.end()) {
         continue;
       }
       ModulesToProcess.push_back(ImportedModule);
     }

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

 }
 
 void ModuleDepCollector::applyDiscoveredDependencies(CompilerInvocation &CI) {
   CI.clearImplicitModuleBuildOptions();
 
   if (llvm::any_of(CI.getFrontendOpts().Inputs, needsModules)) {
     Preprocessor &PP = ScanInstance.getPreprocessor();
     if (Module *CurrentModule = PP.getCurrentModuleImplementation())
-      if (const FileEntry *CurrentModuleMap =
+      if (Optional<FileEntryRef> CurrentModuleMap =
               PP.getHeaderSearchInfo()
                   .getModuleMap()
                   .getModuleMapFileForUniquing(CurrentModule))
         CI.getFrontendOpts().ModuleMapFiles.emplace_back(
             CurrentModuleMap->getName());
 
     SmallVector<ModuleID> DirectDeps;
     for (const auto &KV : ModularDeps)
ModuleID ModuleDepCollectorPP::handleTopLevelModule(const Module *M) {
   ModI.first->second = std::make_unique<ModuleDeps>();
   ModuleDeps &MD = *ModI.first->second;
 
   MD.ID.ModuleName = M->getFullModuleName();
   MD.ImportedByMainFile = DirectModularDeps.contains(M);
   MD.ImplicitModulePCMPath = std::string(M->getASTFile()->getName());
   MD.IsSystem = M->IsSystem;
 
-  const FileEntry *ModuleMap = MDC.ScanInstance.getPreprocessor()
+  Optional<FileEntryRef> ModuleMap = MDC.ScanInstance.getPreprocessor()
                                          .getHeaderSearchInfo()
                                          .getModuleMap()
                                          .getModuleMapFileForUniquing(M);
 
   if (ModuleMap) {
-    StringRef Path = ModuleMap->tryGetRealPathName();
+    StringRef Path = ModuleMap->getFileEntry().tryGetRealPathName();
     if (Path.empty())
       Path = ModuleMap->getName();
     MD.ClangModuleMapFile = std::string(Path);
   }
 
   serialization::ModuleFile *MF =
       MDC.ScanInstance.getASTReader()->getModuleManager().lookup(
           M->getASTFile());

clang/tools/libclang/CIndexInclusionStack.cpp

for (unsigned i = 0 ; i < n ; ++i) {
 
     // If there is a preamble, the last entry is the "inclusion" of that
     // preamble into the main file, which has the bogus entry of main.c:1:1
     if (HasPreamble && !InclusionStack.empty())
       InclusionStack.pop_back();
 
     // Callback to the client.
     // FIXME: We should have a function to construct CXFiles.
-    CB(static_cast<CXFile>(
-           const_cast<FileEntry *>(FI.getContentCache().OrigEntry)),
+    CB(static_cast<CXFile>(const_cast<FileEntry *>(
+           static_cast<const FileEntry *>(FI.getContentCache().OrigEntry))),
        InclusionStack.data(), InclusionStack.size(), clientData);
   }
 }
 } // namespace
 
 void clang_getInclusions(CXTranslationUnit TU, CXInclusionVisitor CB,
                          CXClientData clientData) {
   if (cxtu::isNotUsableTU(TU)) {