Skip to content

Commit

Permalink
clang/Basic: Replace ContentCache::getBuffer with Optional semantics
Browse files Browse the repository at this point in the history 
Remove `ContentCache::getBuffer`, which always returned a
dereferenceable `MemoryBuffer*` and had a `bool*Invalid` out parameter,
and replace it with:

- `ContentCache::getBufferOrNone`, which returns
  `Optional<MemoryBufferRef>`. This is the new API that consumers should
  use. Later it could be renamed to `getBuffer`, but intentionally using
  a different name to root out any unexpected callers.
- `ContentCache::getBufferPointer`, which returns `MemoryBuffer*` with
  "optional" semantics. This is `private` to avoid growing callers and
  `SourceManager` has temporarily been made a `friend` to access it.
  Later paches will update the transitive callers to not need a raw
  pointer, and eventually this will be deleted.

No functionality change intended here.

Differential Revision: https://reviews.llvm.org/D89348
  • Loading branch information
@dexonsmith
dexonsmith committed Oct 14, 2020
1 parent 24bf6ff commit d758f79
Show file tree
Hide file tree
Showing 5 changed files with 126 additions and 98 deletions.
18 changes: 2 additions & 16 deletions clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -305,22 +305,8 @@ static bool IsNOLINTFound(StringRef NolintDirectiveText, StringRef Line,

static llvm::Optional<StringRef> getBuffer(const SourceManager &SM, FileID File,
bool AllowIO) {
// This is similar to the implementation of SourceManager::getBufferData(),
// but uses ContentCache::getRawBuffer() rather than getBuffer() if
// AllowIO=false, to avoid triggering file I/O if the file contents aren't
// already mapped.
bool CharDataInvalid = false;
const SrcMgr::SLocEntry &Entry = SM.getSLocEntry(File, &CharDataInvalid);
if (CharDataInvalid || !Entry.isFile())
return llvm::None;
const SrcMgr::ContentCache *Cache = Entry.getFile().getContentCache();
const llvm::MemoryBuffer *Buffer =
AllowIO ? Cache->getBuffer(SM.getDiagnostics(), SM.getFileManager(),
SourceLocation(), &CharDataInvalid)
: Cache->getRawBuffer();
if (!Buffer || CharDataInvalid)
return llvm::None;
return Buffer->getBuffer();
return AllowIO ? SM.getBufferDataOrNone(File)
: SM.getBufferDataIfLoaded(File);
}

static bool LineIsMarkedWithNOLINT(const SourceManager &SM, SourceLocation Loc,
Expand Down
54 changes: 47 additions & 7 deletions clang/include/clang/Basic/SourceManager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,13 +184,21 @@ namespace SrcMgr {
///
/// \param Loc If specified, is the location that invalid file diagnostics
/// will be emitted at.
llvm::Optional<llvm::MemoryBufferRef>
getBufferOrNone(DiagnosticsEngine &Diag, FileManager &FM,
SourceLocation Loc = SourceLocation()) const;

private:
/// Returns pointer to memory buffer.
///
/// \param Invalid If non-NULL, will be set \c true if an error occurred.
const llvm::MemoryBuffer *getBuffer(DiagnosticsEngine &Diag,
FileManager &FM,
SourceLocation Loc = SourceLocation(),
bool *Invalid = nullptr) const;
/// TODO: SourceManager needs access to this for now, but once that's done
/// we should remove this API.
const llvm::MemoryBuffer *getBufferPointer(DiagnosticsEngine &Diag,
FileManager &FM,
SourceLocation Loc) const;
friend class clang::SourceManager;

public:
/// Returns the size of the content encapsulated by this
/// ContentCache.
///
Expand Down Expand Up @@ -958,10 +966,27 @@ class SourceManager : public RefCountedBase<SourceManager> {
// FileID manipulation methods.
//===--------------------------------------------------------------------===//

/// Return the buffer for the specified FileID.
///
/// If there is an error opening this buffer the first time, return None.
llvm::Optional<llvm::MemoryBufferRef>
getBufferOrNone(FileID FID, SourceLocation Loc = SourceLocation()) const {
bool MyInvalid = false;
const SrcMgr::SLocEntry &Entry = getSLocEntry(FID, &MyInvalid);
if (MyInvalid || !Entry.isFile())
return None;

return Entry.getFile().getContentCache()->getBufferOrNone(
Diag, getFileManager(), Loc);
}

/// Return the buffer for the specified FileID.
///
/// If there is an error opening this buffer the first time, this
/// manufactures a temporary buffer and returns a non-empty error string.
///
/// TODO: Update users of Invalid to call getBufferOrNone and change return
/// type to MemoryBufferRef.
const llvm::MemoryBuffer *getBuffer(FileID FID, SourceLocation Loc,
bool *Invalid = nullptr) const {
bool MyInvalid = false;
Expand All @@ -973,8 +998,11 @@ class SourceManager : public RefCountedBase<SourceManager> {
return getFakeBufferForRecovery();
}

return Entry.getFile().getContentCache()->getBuffer(Diag, getFileManager(),
Loc, Invalid);
auto *B = Entry.getFile().getContentCache()->getBufferPointer(
Diag, getFileManager(), Loc);
if (Invalid)
*Invalid = !B;
return B ? B : getFakeBufferForRecovery();
}

const llvm::MemoryBuffer *getBuffer(FileID FID,
Expand Down Expand Up @@ -1014,6 +1042,18 @@ class SourceManager : public RefCountedBase<SourceManager> {
/// \param Invalid If non-NULL, will be set true if an error occurred.
StringRef getBufferData(FileID FID, bool *Invalid = nullptr) const;

/// Return a StringRef to the source buffer data for the
/// specified FileID, returning None if invalid.
///
/// \param FID The file ID whose contents will be returned.
llvm::Optional<StringRef> getBufferDataOrNone(FileID FID) const;

/// Return a StringRef to the source buffer data for the
/// specified FileID, returning None if it's not yet loaded.
///
/// \param FID The file ID whose contents will be returned.
llvm::Optional<StringRef> getBufferDataIfLoaded(FileID FID) const;

/// Get the number of FileIDs (files and macros) that were created
/// during preprocessing of \p FID, including it.
unsigned getNumCreatedFIDsForFileID(FileID FID) const {
Expand Down
10 changes: 4 additions & 6 deletions clang/lib/AST/ASTImporter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8696,12 +8696,10 @@ Expected<FileID> ASTImporter::Import(FileID FromID, bool IsBuiltin) {

if (ToID.isInvalid() || IsBuiltin) {
// FIXME: We want to re-use the existing MemoryBuffer!
bool Invalid = true;
const llvm::MemoryBuffer *FromBuf =
Cache->getBuffer(FromContext.getDiagnostics(),
FromSM.getFileManager(), SourceLocation{}, &Invalid);
if (!FromBuf || Invalid)
// FIXME: Use a new error kind?
llvm::Optional<llvm::MemoryBufferRef> FromBuf =
Cache->getBufferOrNone(FromContext.getDiagnostics(),
FromSM.getFileManager(), SourceLocation{});
if (!FromBuf)
return llvm::make_error<ImportError>(ImportError::Unknown);

std::unique_ptr<llvm::MemoryBuffer> ToBuf =
Expand Down
130 changes: 66 additions & 64 deletions clang/lib/Basic/SourceManager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,18 +118,25 @@ const char *ContentCache::getInvalidBOM(StringRef BufStr) {
return InvalidBOM;
}

const llvm::MemoryBuffer *ContentCache::getBuffer(DiagnosticsEngine &Diag,
FileManager &FM,
SourceLocation Loc,
bool *Invalid) const {
llvm::Optional<llvm::MemoryBufferRef>
ContentCache::getBufferOrNone(DiagnosticsEngine &Diag, FileManager &FM,
SourceLocation Loc) const {
if (auto *B = getBufferPointer(Diag, FM, Loc))
return B->getMemBufferRef();
return None;
}

const llvm::MemoryBuffer *
ContentCache::getBufferPointer(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 (Buffer.getPointer() || !ContentsEntry) {
if (Invalid)
*Invalid = isBufferInvalid();

return Buffer.getPointer();
}
if (isBufferInvalid())
return nullptr;
if (auto *B = Buffer.getPointer())
return B;
if (!ContentsEntry)
return nullptr;

// 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
Expand All @@ -138,13 +145,6 @@ const llvm::MemoryBuffer *ContentCache::getBuffer(DiagnosticsEngine &Diag,
// miserably on large source files.
if ((uint64_t)ContentsEntry->getSize() >=
std::numeric_limits<unsigned>::max()) {
// We can't make a memory buffer of the required size, so just make a small
// one. We should never hit a situation where we've already parsed to a
// later offset of the file, so it shouldn't matter that the buffer is
// smaller than the file.
Buffer.setPointer(
llvm::MemoryBuffer::getMemBuffer("", ContentsEntry->getName())
.release());
if (Diag.isDiagnosticInFlight())
Diag.SetDelayedDiagnostic(diag::err_file_too_large,
ContentsEntry->getName());
Expand All @@ -153,8 +153,7 @@ const llvm::MemoryBuffer *ContentCache::getBuffer(DiagnosticsEngine &Diag,
<< ContentsEntry->getName();

Buffer.setInt(Buffer.getInt() | InvalidFlag);
if (Invalid) *Invalid = true;
return Buffer.getPointer();
return nullptr;
}

auto BufferOrError = FM.getBufferForFile(ContentsEntry, IsFileVolatile);
Expand All @@ -164,20 +163,7 @@ const llvm::MemoryBuffer *ContentCache::getBuffer(DiagnosticsEngine &Diag,
// exists. Most likely, we were using a stat cache with an invalid entry but
// the file could also have been removed during processing. Since we can't
// really deal with this situation, just create an empty buffer.
//
// FIXME: This is definitely not ideal, but our immediate clients can't
// currently handle returning a null entry here. Ideally we should detect
// that we are in an inconsistent situation and error out as quickly as
// possible.
if (!BufferOrError) {
StringRef FillStr("<<<MISSING SOURCE FILE>>>\n");
auto BackupBuffer = llvm::WritableMemoryBuffer::getNewUninitMemBuffer(
ContentsEntry->getSize(), "<invalid>");
char *Ptr = BackupBuffer->getBufferStart();
for (unsigned i = 0, e = ContentsEntry->getSize(); i != e; ++i)
Ptr[i] = FillStr[i % FillStr.size()];
Buffer.setPointer(BackupBuffer.release());

if (Diag.isDiagnosticInFlight())
Diag.SetDelayedDiagnostic(diag::err_cannot_open_file,
ContentsEntry->getName(),
Expand All @@ -187,9 +173,7 @@ const llvm::MemoryBuffer *ContentCache::getBuffer(DiagnosticsEngine &Diag,
<< ContentsEntry->getName() << BufferOrError.getError().message();

Buffer.setInt(Buffer.getInt() | InvalidFlag);

if (Invalid) *Invalid = true;
return Buffer.getPointer();
return nullptr;
}

Buffer.setPointer(BufferOrError->release());
Expand All @@ -205,8 +189,7 @@ const llvm::MemoryBuffer *ContentCache::getBuffer(DiagnosticsEngine &Diag,
<< ContentsEntry->getName();

Buffer.setInt(Buffer.getInt() | InvalidFlag);
if (Invalid) *Invalid = true;
return Buffer.getPointer();
return nullptr;
}

// If the buffer is valid, check to see if it has a UTF Byte Order Mark
Expand All @@ -219,11 +202,9 @@ const llvm::MemoryBuffer *ContentCache::getBuffer(DiagnosticsEngine &Diag,
Diag.Report(Loc, diag::err_unsupported_bom)
<< InvalidBOM << ContentsEntry->getName();
Buffer.setInt(Buffer.getInt() | InvalidFlag);
return nullptr;
}

if (Invalid)
*Invalid = isBufferInvalid();

return Buffer.getPointer();
}

Expand Down Expand Up @@ -727,7 +708,10 @@ const llvm::MemoryBuffer *
SourceManager::getMemoryBufferForFile(const FileEntry *File, bool *Invalid) {
const SrcMgr::ContentCache *IR = getOrCreateContentCache(File);
assert(IR && "getOrCreateContentCache() cannot return NULL");
return IR->getBuffer(Diag, getFileManager(), SourceLocation(), Invalid);
auto *B = IR->getBufferPointer(Diag, getFileManager(), SourceLocation());
if (Invalid)
*Invalid = !B;
return B ? B : getFakeBufferForRecovery();
}

void SourceManager::overrideFileContents(const FileEntry *SourceFile,
Expand Down Expand Up @@ -786,23 +770,35 @@ Optional<FileEntryRef> SourceManager::getFileEntryRefForID(FileID FID) const {
}

StringRef SourceManager::getBufferData(FileID FID, bool *Invalid) const {
auto B = getBufferDataOrNone(FID);
if (Invalid)
*Invalid = !B;
return B ? *B : "<<<<<INVALID SOURCE LOCATION>>>>>";
}

llvm::Optional<StringRef>
SourceManager::getBufferDataIfLoaded(FileID FID) const {
bool MyInvalid = false;
const SLocEntry &SLoc = getSLocEntry(FID, &MyInvalid);
if (!SLoc.isFile() || MyInvalid) {
if (Invalid)
*Invalid = true;
return "<<<<<INVALID SOURCE LOCATION>>>>>";
}
if (!SLoc.isFile() || MyInvalid)
return None;

const llvm::MemoryBuffer *Buf = SLoc.getFile().getContentCache()->getBuffer(
Diag, getFileManager(), SourceLocation(), &MyInvalid);
if (Invalid)
*Invalid = MyInvalid;
if (const llvm::MemoryBuffer *Buf =
SLoc.getFile().getContentCache()->getRawBuffer())
return Buf->getBuffer();
return None;
}

if (MyInvalid)
return "<<<<<INVALID SOURCE LOCATION>>>>>";
llvm::Optional<StringRef> SourceManager::getBufferDataOrNone(FileID FID) const {
bool MyInvalid = false;
const SLocEntry &SLoc = getSLocEntry(FID, &MyInvalid);
if (!SLoc.isFile() || MyInvalid)
return None;

return Buf->getBuffer();
if (auto B = SLoc.getFile().getContentCache()->getBufferOrNone(
Diag, getFileManager(), SourceLocation()))
return B->getBuffer();
return None;
}

//===----------------------------------------------------------------------===//
Expand Down Expand Up @@ -1219,12 +1215,13 @@ const char *SourceManager::getCharacterData(SourceLocation SL,

return "<<<<INVALID BUFFER>>>>";
}
const llvm::MemoryBuffer *Buffer =
Entry.getFile().getContentCache()->getBuffer(
Diag, getFileManager(), SourceLocation(), &CharDataInvalid);
llvm::Optional<llvm::MemoryBufferRef> Buffer =
Entry.getFile().getContentCache()->getBufferOrNone(Diag, getFileManager(),
SourceLocation());
if (Invalid)
*Invalid = CharDataInvalid;
return Buffer->getBufferStart() + (CharDataInvalid? 0 : LocInfo.second);
*Invalid = !Buffer;
return Buffer ? Buffer->getBufferStart() + LocInfo.second
: "<<<<INVALID BUFFER>>>>";
}

/// getColumnNumber - Return the column # for the specified file position.
Expand Down Expand Up @@ -1317,8 +1314,9 @@ static void ComputeLineNumbers(DiagnosticsEngine &Diag, ContentCache *FI,
llvm::BumpPtrAllocator &Alloc,
const SourceManager &SM, bool &Invalid) {
// Note that calling 'getBuffer()' may lazily page in the file.
const MemoryBuffer *Buffer =
FI->getBuffer(Diag, SM.getFileManager(), SourceLocation(), &Invalid);
llvm::Optional<llvm::MemoryBufferRef> Buffer =
FI->getBufferOrNone(Diag, SM.getFileManager(), SourceLocation());
Invalid = !Buffer;
if (Invalid)
return;

Expand Down Expand Up @@ -1543,8 +1541,8 @@ PresumedLoc SourceManager::getPresumedLoc(SourceLocation Loc,
StringRef Filename;
if (C->OrigEntry)
Filename = C->OrigEntry->getName();
else
Filename = C->getBuffer(Diag, getFileManager())->getBufferIdentifier();
else if (auto Buffer = C->getBufferOrNone(Diag, getFileManager()))
Filename = Buffer->getBufferIdentifier();

unsigned LineNo = getLineNumber(LocInfo.first, LocInfo.second, &Invalid);
if (Invalid)
Expand Down Expand Up @@ -1739,14 +1737,18 @@ SourceLocation SourceManager::translateLineCol(FileID FID,
return SourceLocation();
}

llvm::Optional<llvm::MemoryBufferRef> Buffer =
Content->getBufferOrNone(Diag, getFileManager());
if (!Buffer)
return SourceLocation();

if (Line > Content->NumLines) {
unsigned Size = Content->getBuffer(Diag, getFileManager())->getBufferSize();
unsigned Size = Buffer->getBufferSize();
if (Size > 0)
--Size;
return FileLoc.getLocWithOffset(Size);
}

const llvm::MemoryBuffer *Buffer = Content->getBuffer(Diag, getFileManager());
unsigned FilePos = Content->SourceLineCache[Line - 1];
const char *Buf = Buffer->getBufferStart() + FilePos;
unsigned BufLength = Buffer->getBufferSize() - FilePos;
Expand Down
12 changes: 7 additions & 5 deletions clang/lib/Serialization/ASTWriter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2002,9 +2002,9 @@ void ASTWriter::WriteSourceManagerBlock(SourceManager &SourceMgr,
// We add one to the size so that we capture the trailing NULL
// that is required by llvm::MemoryBuffer::getMemBuffer (on
// the reader side).
const llvm::MemoryBuffer *Buffer =
Content->getBuffer(PP.getDiagnostics(), PP.getFileManager());
StringRef Name = Buffer->getBufferIdentifier();
llvm::Optional<llvm::MemoryBufferRef> Buffer =
Content->getBufferOrNone(PP.getDiagnostics(), PP.getFileManager());
StringRef Name = Buffer ? Buffer->getBufferIdentifier() : "";
Stream.EmitRecordWithBlob(SLocBufferAbbrv, Record,
StringRef(Name.data(), Name.size() + 1));
EmitBlob = true;
Expand All @@ -2016,8 +2016,10 @@ void ASTWriter::WriteSourceManagerBlock(SourceManager &SourceMgr,
if (EmitBlob) {
// Include the implicit terminating null character in the on-disk buffer
// if we're writing it uncompressed.
const llvm::MemoryBuffer *Buffer =
Content->getBuffer(PP.getDiagnostics(), PP.getFileManager());
llvm::Optional<llvm::MemoryBufferRef> Buffer =
Content->getBufferOrNone(PP.getDiagnostics(), PP.getFileManager());
if (!Buffer)
Buffer = llvm::MemoryBufferRef("<<<INVALID BUFFER>>>", "");
StringRef Blob(Buffer->getBufferStart(), Buffer->getBufferSize() + 1);
emitBlob(Stream, Blob, SLocBufferBlobCompressedAbbrv,
SLocBufferBlobAbbrv);
Expand Down

0 comments on commit d758f79

Please sign in to comment.