ContentCache: Simplify by always owning the MemoryBuffer

[dexonsmith]

Summary

This changes ContentCache::Buffer to use
std::unique_ptr<MemoryBuffer> instead of the PointerIntPair. It
drops the (mostly unused) DoNotFree bit, instead creating a (new)
non-owning MemoryBuffer instance when passed a MemoryBufferRef.

[dexonsmith]

ping: Besides rebasing, any concerns here?

[dexonsmith]

Rebased on top of https://reviews.llvm.org/D89431 (the tests haven't finished yet; I'll update if necessary, but this is pretty straightforward).

[dexonsmith]

ping

In D67030#2331280, @dexonsmith wrote:

Rebased on top of https://reviews.llvm.org/D89431 (the tests haven't finished yet; I'll update if necessary, but this is pretty straightforward).

(FTR, the tests passed as expected.)

clang/include/clang/Basic/SourceManager.h

namespace SrcMgr {
   inline bool isModuleMap(CharacteristicKind CK) {
     return CK == C_User_ModuleMap || CK == C_System_ModuleMap;
   }
 
   /// One instance of this struct is kept for every file loaded or used.
   ///
   /// This object owns the MemoryBuffer object.
   class alignas(8) ContentCache {
-    enum CCFlags {
-      /// Whether the buffer should not be freed on destruction.
-      DoNotFreeFlag = 0x02
-    };
-
     /// The actual buffer containing the characters from the input
     /// file.
-    ///
-    /// This is owned by the ContentCache object.  The bits indicate
-    /// whether the buffer is invalid.
-    mutable llvm::PointerIntPair<const llvm::MemoryBuffer *, 2> Buffer;
+    mutable std::unique_ptr<llvm::MemoryBuffer> Buffer;
 
   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.
public:
     /// after serialization and deserialization.
     unsigned IsTransient : 1;
 
     mutable unsigned IsBufferInvalid : 1;
 
     ContentCache(const FileEntry *Ent = nullptr) : ContentCache(Ent, Ent) {}
 
     ContentCache(const FileEntry *Ent, const FileEntry *contentEnt)
-        : Buffer(nullptr, false), OrigEntry(Ent), ContentsEntry(contentEnt),
-          BufferOverridden(false), IsFileVolatile(false), IsTransient(false),
-          IsBufferInvalid(false) {}
+        : 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)
-        : Buffer(nullptr, false), BufferOverridden(false),
-          IsFileVolatile(false), IsTransient(false), IsBufferInvalid(false) {
+        : BufferOverridden(false), IsFileVolatile(false), IsTransient(false),
+          IsBufferInvalid(false) {
       OrigEntry = RHS.OrigEntry;
       ContentsEntry = RHS.ContentsEntry;
 
-      assert(RHS.Buffer.getPointer() == nullptr &&
-             RHS.SourceLineCache == nullptr &&
+      assert(!RHS.Buffer && RHS.SourceLineCache == nullptr &&
              "Passed ContentCache object cannot own a buffer.");
 
       NumLines = RHS.NumLines;
     }
 
     ContentCache &operator=(const ContentCache& RHS) = delete;
 
-    ~ContentCache();
-
     /// Returns the memory buffer for the associated content.
     ///
     /// \param Diag Object through which diagnostics will be emitted if the
     ///   buffer cannot be retrieved.
     ///
     /// \param Loc If specified, is the location that invalid file diagnostics
     ///   will be emitted at.
     llvm::Optional<llvm::MemoryBufferRef>
public:
     unsigned getSizeBytesMapped() const;
 
     /// Returns the kind of memory used to back the memory buffer for
     /// this content cache.  This is used for performance analysis.
     llvm::MemoryBuffer::BufferKind getMemoryBufferKind() const;
 
     /// Get the underlying buffer, returning NULL if the buffer is not
     /// yet available.
-    const llvm::MemoryBuffer *getRawBuffer() const {
-      return Buffer.getPointer();
-    }
+    const llvm::MemoryBuffer *getRawBuffer() const { return Buffer.get(); }
 
-    /// Replace the existing buffer (which will be deleted)
-    /// with the given buffer.
-    void replaceBuffer(const llvm::MemoryBuffer *B, bool DoNotFree = false);
+    /// Set the buffer.
+    void setBuffer(std::unique_ptr<llvm::MemoryBuffer> B) {
+      IsBufferInvalid = false;
+      Buffer = std::move(B);
+    }
 
-    /// Determine whether the buffer should be freed.
-    bool shouldFreeBuffer() const {
-      return (Buffer.getInt() & DoNotFreeFlag) == 0;
+    /// Set the buffer to one that's not owned (or to nullptr).
+    ///
+    /// \pre Buffer cannot already be set.
+    void setUnownedBuffer(const llvm::MemoryBuffer *B) {
+      assert(!Buffer && "Expected to be called right after construction");
+      if (B)
+        setBuffer(llvm::MemoryBuffer::getMemBuffer(B->getMemBufferRef()));
     }
 
     // If BufStr has an invalid BOM, returns the BOM name; otherwise, returns
     // nullptr
     static const char *getInvalidBOM(StringRef BufStr);
   };
 
   // Assert that the \c ContentCache objects will always be 8-byte aligned so
public:
 
   /// Override the contents of the given source file by providing an
   /// already-allocated buffer.
   ///
   /// \param SourceFile the source file whose contents will be overridden.
   ///
   /// \param Buffer the memory buffer whose contents will be used as the
   /// data in the given source file.
-  ///
-  /// \param DoNotFree If true, then the buffer will not be freed when the
-  /// source manager is destroyed.
-  void overrideFileContents(const FileEntry *SourceFile,
-                            llvm::MemoryBuffer *Buffer, bool DoNotFree);
   void overrideFileContents(const FileEntry *SourceFile,
-                            std::unique_ptr<llvm::MemoryBuffer> Buffer) {
-    overrideFileContents(SourceFile, Buffer.release(), /*DoNotFree*/ false);
+                            const llvm::MemoryBufferRef &Buffer) {
+    overrideFileContents(SourceFile, llvm::MemoryBuffer::getMemBuffer(Buffer));
   }
 
+  /// Override the contents of the given source file by providing an
+  /// already-allocated buffer.
+  ///
+  /// \param SourceFile the source file whose contents will be overridden.
+  ///
+  /// \param Buffer the memory buffer whose contents will be used as the
+  /// data in the given source file.
+  void overrideFileContents(const FileEntry *SourceFile,
+                            std::unique_ptr<llvm::MemoryBuffer> Buffer);
+
   /// 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);

clang/lib/Basic/SourceManager.cpp

 using namespace clang;
 using namespace SrcMgr;
 using llvm::MemoryBuffer;
 
 //===----------------------------------------------------------------------===//
 // SourceManager Helper Classes
 //===----------------------------------------------------------------------===//
 
-ContentCache::~ContentCache() {
-  if (shouldFreeBuffer())
-    delete Buffer.getPointer();
-}
-
 /// getSizeBytesMapped - Returns the number of bytes actually mapped for this
 /// ContentCache. This can be 0 if the MemBuffer was not actually expanded.
 unsigned ContentCache::getSizeBytesMapped() const {
-  return Buffer.getPointer() ? Buffer.getPointer()->getBufferSize() : 0;
+  return Buffer ? Buffer->getBufferSize() : 0;
 }
 
 /// Returns the kind of memory used to back the memory buffer for
 /// this content cache.  This is used for performance analysis.
 llvm::MemoryBuffer::BufferKind ContentCache::getMemoryBufferKind() const {
-  assert(Buffer.getPointer());
+  assert(Buffer);
 
   // Should be unreachable, but keep for sanity.
-  if (!Buffer.getPointer())
+  if (!Buffer)
     return llvm::MemoryBuffer::MemoryBuffer_Malloc;
 
-  const llvm::MemoryBuffer *buf = Buffer.getPointer();
-  return buf->getBufferKind();
+  return Buffer->getBufferKind();
 }
 
 /// getSize - Returns the size of the content encapsulated by this ContentCache.
 ///  This can be the size of the source file or the size of an arbitrary
 ///  scratch buffer.  If the ContentCache encapsulates a source file, that
 ///  file is not lazily brought in from disk to satisfy this query.
 unsigned ContentCache::getSize() const {
-  return Buffer.getPointer() ? (unsigned) Buffer.getPointer()->getBufferSize()
-                             : (unsigned) ContentsEntry->getSize();
+  return Buffer ? (unsigned)Buffer->getBufferSize()
+                : (unsigned)ContentsEntry->getSize();
 }
 
-void ContentCache::replaceBuffer(const llvm::MemoryBuffer *B, bool DoNotFree) {
-  if (B && B == Buffer.getPointer()) {
-    assert(0 && "Replacing with the same buffer");
-    Buffer.setInt(DoNotFree? DoNotFreeFlag : 0);
-    return;
-  }
-
-  if (shouldFreeBuffer())
-    delete Buffer.getPointer();
-  Buffer.setPointer(B);
-  Buffer.setInt((B && DoNotFree) ? DoNotFreeFlag : 0);
-}
-
 const char *ContentCache::getInvalidBOM(StringRef BufStr) {
   // If the buffer is valid, check to see if it has a UTF Byte Order Mark
   // (BOM).  We only support UTF-8 with and without a BOM right now.  See
   // http://en.wikipedia.org/wiki/Byte_order_mark for more information.
   const char *InvalidBOM =
       llvm::StringSwitch<const char *>(BufStr)
           .StartsWith(llvm::StringLiteral::withInnerNUL("\x00\x00\xFE\xFF"),
                       "UTF-32 (BE)")

 
 llvm::Optional<llvm::MemoryBufferRef>
 ContentCache::getBufferOrNone(DiagnosticsEngine &Diag, FileManager &FM,
                               SourceLocation Loc) const {
   // Lazily create the Buffer for ContentCaches that wrap files.  If we already
   // computed it, just return what we have.
   if (IsBufferInvalid)
     return None;
-  if (auto *B = Buffer.getPointer())
-    return B->getMemBufferRef();
+  if (Buffer)
+    return Buffer->getMemBufferRef();
   if (!ContentsEntry)
     return None;
 
   // Check that the file's size fits in an 'unsigned' (with room for a
   // past-the-end value). This is deeply regrettable, but various parts of
   // Clang (including elsewhere in this file!) use 'unsigned' to represent file
   // offsets, line numbers, string literal lengths, and so on, and fail
   // miserably on large source files.
if (!BufferOrError) {
     else
       Diag.Report(Loc, diag::err_cannot_open_file)
           << ContentsEntry->getName() << BufferOrError.getError().message();
 
     IsBufferInvalid = true;
     return None;
   }
 
-  Buffer.setPointer(BufferOrError->release());
+  Buffer = std::move(*BufferOrError);
 
   // Check that the file's size is the same as in the file entry (which may
   // have come from a stat cache).
   if (getRawBuffer()->getBufferSize() != (size_t)ContentsEntry->getSize()) {
     if (Diag.isDiagnosticInFlight())
       Diag.SetDelayedDiagnostic(diag::err_file_modified,
                                 ContentsEntry->getName());
     else
       Diag.Report(Loc, diag::err_file_modified)
         << ContentsEntry->getName();
 
     IsBufferInvalid = true;
     return None;
   }
 
   // If the buffer is valid, check to see if it has a UTF Byte Order Mark
   // (BOM).  We only support UTF-8 with and without a BOM right now.  See
   // http://en.wikipedia.org/wiki/Byte_order_mark for more information.
-  StringRef BufStr = Buffer.getPointer()->getBuffer();
+  StringRef BufStr = Buffer->getBuffer();
   const char *InvalidBOM = getInvalidBOM(BufStr);
 
   if (InvalidBOM) {
     Diag.Report(Loc, diag::err_unsupported_bom)
       << InvalidBOM << ContentsEntry->getName();
     IsBufferInvalid = true;
     return None;
   }
 
-  return Buffer.getPointer()->getMemBufferRef();
+  return Buffer->getMemBufferRef();
 }
 
 unsigned LineTableInfo::getLineTableFilenameID(StringRef Name) {
   auto IterBool = FilenameIDs.try_emplace(Name, FilenamesByID.size());
   if (IterBool.second)
     FilenamesByID.push_back(&*IterBool.first);
   return IterBool.first->second;
 }
void SourceManager::initializeForReplay(const SourceManager &Old) {
 
   auto CloneContentCache = [&](const ContentCache *Cache) -> ContentCache * {
     auto *Clone = new (ContentCacheAlloc.Allocate<ContentCache>()) ContentCache;
     Clone->OrigEntry = Cache->OrigEntry;
     Clone->ContentsEntry = Cache->ContentsEntry;
     Clone->BufferOverridden = Cache->BufferOverridden;
     Clone->IsFileVolatile = Cache->IsFileVolatile;
     Clone->IsTransient = Cache->IsTransient;
-    Clone->replaceBuffer(Cache->getRawBuffer(), /*DoNotFree*/true);
+    Clone->setUnownedBuffer(Cache->getRawBuffer());
     return Clone;
   };
 
   // Ensure all SLocEntries are loaded from the external source.
   for (unsigned I = 0, N = Old.LoadedSLocEntryTable.size(); I != N; ++I)
     if (!Old.SLocEntryLoaded[I])
       Old.loadSLocEntry(I, nullptr);
 

 /// Create a new ContentCache for the specified memory buffer.
 /// This does no caching.
 const ContentCache *SourceManager::createMemBufferContentCache(
     std::unique_ptr<llvm::MemoryBuffer> Buffer) {
   // Add a new ContentCache to the MemBufferInfos list and return it.
   ContentCache *Entry = ContentCacheAlloc.Allocate<ContentCache>();
   new (Entry) ContentCache();
   MemBufferInfos.push_back(Entry);
-  Entry->replaceBuffer(Buffer.release(), /*DoNotFree=*/false);
+  Entry->setBuffer(std::move(Buffer));
   return Entry;
 }
 
 const SrcMgr::SLocEntry &SourceManager::loadSLocEntry(unsigned Index,
                                                       bool *Invalid) const {
   assert(!SLocEntryLoaded[Index]);
   if (ExternalSLocEntries->ReadSLocEntry(-(static_cast<int>(Index) + 2))) {
     if (Invalid)

 }
 
 /// As part of recovering from missing or changed content, produce a
 /// fake content cache.
 const SrcMgr::ContentCache *
 SourceManager::getFakeContentCacheForRecovery() const {
   if (!FakeContentCacheForRecovery) {
     FakeContentCacheForRecovery = std::make_unique<SrcMgr::ContentCache>();
-    FakeContentCacheForRecovery->replaceBuffer(getFakeBufferForRecovery(),
-                                               /*DoNotFree=*/true);
+    FakeContentCacheForRecovery->setUnownedBuffer(getFakeBufferForRecovery());
   }
   return FakeContentCacheForRecovery.get();
 }
 
 /// Returns the previous in-order FileID or an invalid FileID if there
 /// is no previous one.
 FileID SourceManager::getPreviousFileID(FileID FID) const {
   if (FID.isInvalid())

 
 llvm::Optional<llvm::MemoryBufferRef>
 SourceManager::getMemoryBufferForFileOrNone(const FileEntry *File) {
   const SrcMgr::ContentCache *IR = getOrCreateContentCache(File);
   assert(IR && "getOrCreateContentCache() cannot return NULL");
   return IR->getBufferOrNone(Diag, getFileManager(), SourceLocation());
 }
 
-void SourceManager::overrideFileContents(const FileEntry *SourceFile,
-                                         llvm::MemoryBuffer *Buffer,
-                                         bool DoNotFree) {
-  const SrcMgr::ContentCache *IR = getOrCreateContentCache(SourceFile);
+void SourceManager::overrideFileContents(
+    const FileEntry *SourceFile, std::unique_ptr<llvm::MemoryBuffer> Buffer) {
+  auto *IR =
+      const_cast<SrcMgr::ContentCache *>(getOrCreateContentCache(SourceFile));
   assert(IR && "getOrCreateContentCache() cannot return NULL");
 
-  const_cast<SrcMgr::ContentCache *>(IR)->replaceBuffer(Buffer, DoNotFree);
-  const_cast<SrcMgr::ContentCache *>(IR)->BufferOverridden = true;
+  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() &&
          "Different sizes, use the FileManager to create a virtual file with "

clang/lib/Frontend/CompilerInstance.cpp

const FileEntry *FromFile =
         FileMgr.getVirtualFile(RB.first, RB.second->getBufferSize(), 0);
     if (!FromFile) {
       Diags.Report(diag::err_fe_remap_missing_from_file) << RB.first;
       if (!InitOpts.RetainRemappedFileBuffers)
         delete RB.second;
       continue;
     }
 
-    // Override the contents of the "from" file with the contents of
-    // the "to" file.
-    SourceMgr.overrideFileContents(FromFile, RB.second,
-                                   InitOpts.RetainRemappedFileBuffers);
+    // Override the contents of the "from" file with the contents of the
+    // "to" file. If the caller owns the buffers, then pass a MemoryBufferRef;
+    // otherwise, pass as a std::unique_ptr<MemoryBuffer> to transfer ownership
+    // to the SourceManager.
+    if (InitOpts.RetainRemappedFileBuffers)
+      SourceMgr.overrideFileContents(FromFile, RB.second->getMemBufferRef());
+    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);
     if (!ToFile) {
       Diags.Report(diag::err_fe_remap_missing_to_file) << RF.first << RF.second;