From 22e6b1863e74136908842d71b4f942313d89b273 Mon Sep 17 00:00:00 2001
From: =?utf8?q?Duncan=20P=2E=20N=2E=20Exon=C2=A0Smith?=
 <dexonsmith@apple.com>
Date: Fri, 16 Oct 2020 14:39:28 -0400
Subject: [PATCH] SourceManager: Fix an SLocEntry memory regression introduced
 with FileEntryRef

4dc5573acc0d2e7c59d8bac2543eb25cb4b32984 added `FileEntryRef` in order to
help enable sharing of a `FileManager` between `CompilerInstance`s.

It also added a `StringRef` with the filename on `FileInfo`. This
doubled `sizeof(FileInfo)`, bloating `sizeof(SLocEntry)`, of which we
have one for each (loaded and unloaded) file and macro expansion. This
causes a memory regression in modules builds.

Move the filename down into the `ContentCache`, which is a side data
structure for `FileInfo` that does not impact `sizeof(SLocEntry)`. Once
`FileEntryRef` is used for `ContentCache::OrigEntry` this can go away.

Differential Revision: https://reviews.llvm.org/D89580
Radar-Id: rdar://59908826
---
 clang/include/clang/Basic/SourceManager.h | 28 +++++++++++++++++++++-------
 clang/lib/Basic/SourceManager.cpp         |  2 +-
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/clang/include/clang/Basic/SourceManager.h b/clang/include/clang/Basic/SourceManager.h
index 99428e6..ec9b2b7 100644
--- a/clang/include/clang/Basic/SourceManager.h
+++ b/clang/include/clang/Basic/SourceManager.h
@@ -134,6 +134,8 @@ namespace SrcMgr {
     ///
     /// 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;
 
     /// References the file which the contents were actually loaded from.
@@ -142,6 +144,11 @@ namespace SrcMgr {
     /// 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
@@ -266,7 +273,11 @@ namespace SrcMgr {
   /// from. This information encodes the \#include chain that a token was
   /// expanded from. The main include file has an invalid IncludeLoc.
   ///
-  /// FileInfos contain a "ContentCache *", with the contents of the file.
+  /// FileInfo should not grow larger than ExpansionInfo. Doing so will
+  /// cause memory to bloat in compilations with many unloaded macro
+  /// expansions, since the two data structurs are stored in a union in
+  /// SLocEntry. Extra fields should instead go in "ContentCache *", which
+  /// stores file contents and other bits on the side.
   ///
   class FileInfo {
     friend class clang::SourceManager;
@@ -291,10 +302,6 @@ namespace SrcMgr {
     llvm::PointerIntPair<const ContentCache*, 3, CharacteristicKind>
         ContentAndKind;
 
-    /// The filename that is used to access the file entry represented by the
-    /// content cache.
-    StringRef Filename;
-
   public:
     /// Return a FileInfo object.
     static FileInfo get(SourceLocation IL, ContentCache &Con,
@@ -305,7 +312,7 @@ namespace SrcMgr {
       X.HasLineDirectives = false;
       X.ContentAndKind.setPointer(&Con);
       X.ContentAndKind.setInt(FileCharacter);
-      X.Filename = Filename;
+      Con.Filename = Filename;
       return X;
     }
 
@@ -333,7 +340,7 @@ namespace SrcMgr {
 
     /// Returns the name of the file that was used when the file was loaded from
     /// the underlying file system.
-    StringRef getName() const { return Filename; }
+    StringRef getName() const { return getContentCache().Filename; }
   };
 
   /// Each ExpansionInfo encodes the expansion location - where
@@ -454,6 +461,13 @@ namespace SrcMgr {
     }
   };
 
+  // Assert that the \c FileInfo objects are no bigger than \c ExpansionInfo
+  // objects. This controls the size of \c SLocEntry, of which we have one for
+  // each macro expansion. The number of (unloaded) macro expansions can be
+  // very large. Any other fields needed in FileInfo should go in ContentCache.
+  static_assert(sizeof(FileInfo) <= sizeof(ExpansionInfo),
+                "FileInfo must be no larger than ExpansionInfo.");
+
   /// This is a discriminated union of FileInfo and ExpansionInfo.
   ///
   /// SourceManager keeps an array of these objects, and they are uniquely
diff --git a/clang/lib/Basic/SourceManager.cpp b/clang/lib/Basic/SourceManager.cpp
index 2ceb804..6eae0f0 100644
--- a/clang/lib/Basic/SourceManager.cpp
+++ b/clang/lib/Basic/SourceManager.cpp
@@ -1734,7 +1734,7 @@ void SourceManager::computeMacroArgsCache(MacroArgsMap &MacroArgsCache,
           // Predefined header doesn't have a valid include location in main
           // file, but any files created by it should still be skipped when
           // computing macro args expanded in the main file.
-          (FID == MainFileID && Entry.getFile().Filename == "<built-in>");
+          (FID == MainFileID && Entry.getFile().getName() == "<built-in>");
       if (IncludedInFID) {
         // Skip the files/macros of the #include'd file, we only care about
         // macros that lexed macro arguments from our file.
-- 
2.7.4