[clang] 2963145 - ContentCache: Simplify by always owning the MemoryBuffer
Duncan P. N. Exon Smith via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 20 18:04:04 PDT 2020
Author: Duncan P. N. Exon Smith
Date: 2020-10-20T21:03:53-04:00
New Revision: 296314516d103f8eeb987a08b509c1381cfeef89
URL: https://github.com/llvm/llvm-project/commit/296314516d103f8eeb987a08b509c1381cfeef89
DIFF: https://github.com/llvm/llvm-project/commit/296314516d103f8eeb987a08b509c1381cfeef89.diff
LOG: ContentCache: Simplify by always owning the MemoryBuffer
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`.
Differential Revision: https://reviews.llvm.org/D67030
Added:
Modified:
clang/include/clang/Basic/SourceManager.h
clang/lib/Basic/SourceManager.cpp
clang/lib/Frontend/CompilerInstance.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Basic/SourceManager.h b/clang/include/clang/Basic/SourceManager.h
index 569ef002d387..65c3de5a1e41 100644
--- a/clang/include/clang/Basic/SourceManager.h
+++ b/clang/include/clang/Basic/SourceManager.h
@@ -94,17 +94,9 @@ namespace SrcMgr {
///
/// 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.
@@ -153,21 +145,19 @@ namespace SrcMgr {
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;
@@ -175,8 +165,6 @@ namespace SrcMgr {
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
@@ -208,17 +196,21 @@ namespace SrcMgr {
/// 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
@@ -905,16 +897,21 @@ class SourceManager : public RefCountedBase<SourceManager> {
///
/// \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.
diff --git a/clang/lib/Basic/SourceManager.cpp b/clang/lib/Basic/SourceManager.cpp
index c831d26c380c..84315bf3f8ae 100644
--- a/clang/lib/Basic/SourceManager.cpp
+++ b/clang/lib/Basic/SourceManager.cpp
@@ -49,28 +49,22 @@ 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.
@@ -78,21 +72,8 @@ llvm::MemoryBuffer::BufferKind ContentCache::getMemoryBufferKind() const {
/// 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();
-}
-
-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);
+ return Buffer ? (unsigned)Buffer->getBufferSize()
+ : (unsigned)ContentsEntry->getSize();
}
const char *ContentCache::getInvalidBOM(StringRef BufStr) {
@@ -125,8 +106,8 @@ ContentCache::getBufferOrNone(DiagnosticsEngine &Diag, FileManager &FM,
// 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;
@@ -168,7 +149,7 @@ ContentCache::getBufferOrNone(DiagnosticsEngine &Diag, FileManager &FM,
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).
@@ -187,7 +168,7 @@ ContentCache::getBufferOrNone(DiagnosticsEngine &Diag, FileManager &FM,
// 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) {
@@ -197,7 +178,7 @@ ContentCache::getBufferOrNone(DiagnosticsEngine &Diag, FileManager &FM,
return None;
}
- return Buffer.getPointer()->getMemBufferRef();
+ return Buffer->getMemBufferRef();
}
unsigned LineTableInfo::getLineTableFilenameID(StringRef Name) {
@@ -380,7 +361,7 @@ void SourceManager::initializeForReplay(const SourceManager &Old) {
Clone->BufferOverridden = Cache->BufferOverridden;
Clone->IsFileVolatile = Cache->IsFileVolatile;
Clone->IsTransient = Cache->IsTransient;
- Clone->replaceBuffer(Cache->getRawBuffer(), /*DoNotFree*/true);
+ Clone->setUnownedBuffer(Cache->getRawBuffer());
return Clone;
};
@@ -441,7 +422,7 @@ const ContentCache *SourceManager::createMemBufferContentCache(
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;
}
@@ -493,8 +474,7 @@ 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();
}
@@ -700,14 +680,14 @@ SourceManager::getMemoryBufferForFileOrNone(const FileEntry *File) {
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);
}
diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index 3aa456a8726e..030ef42fc11d 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -346,10 +346,16 @@ static void InitializeFileRemapping(DiagnosticsEngine &Diags,
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).
More information about the cfe-commits
mailing list