Re: r312851 - Fix ownership of the MemoryBuffer in a FrontendInputFile.
On Fri, 29 Mar 2019 at 05:41, Nico Weber via cfe-commits
wrote:
>
>
> (below)
>
> On Fri, Sep 8, 2017 at 9:15 PM Richard Smith via cfe-commits
> wrote:
>>
>> Author: rsmith
>> Date: Fri Sep 8 18:14:04 2017
>> New Revision: 312851
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=312851=rev
>> Log:
>> Fix ownership of the MemoryBuffer in a FrontendInputFile.
>>
>> This fixes a possible crash on certain kinds of corrupted AST file, but
>> checking in an AST file corrupted in just the right way will be a maintenance
>> nightmare because the format changes frequently.
>>
>> Modified:
>> cfe/trunk/include/clang/Basic/SourceManager.h
>> cfe/trunk/include/clang/Frontend/FrontendOptions.h
>> cfe/trunk/lib/Basic/SourceManager.cpp
>> cfe/trunk/lib/Frontend/CompilerInstance.cpp
>> cfe/trunk/lib/Frontend/FrontendAction.cpp
>>
>> Modified: cfe/trunk/include/clang/Basic/SourceManager.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/SourceManager.h?rev=312851=312850=312851=diff
>> ==
>> --- cfe/trunk/include/clang/Basic/SourceManager.h (original)
>> +++ cfe/trunk/include/clang/Basic/SourceManager.h Fri Sep 8 18:14:04 2017
>> @@ -212,12 +212,6 @@ namespace SrcMgr {
>> /// this content cache. This is used for performance analysis.
>> llvm::MemoryBuffer::BufferKind getMemoryBufferKind() const;
>>
>> -void setBuffer(std::unique_ptr B) {
>> - assert(!Buffer.getPointer() && "MemoryBuffer already set.");
>> - Buffer.setPointer(B.release());
>> - Buffer.setInt(0);
>> -}
>> -
>> /// \brief Get the underlying buffer, returning NULL if the buffer is
>> not
>> /// yet available.
>> llvm::MemoryBuffer *getRawBuffer() const { return Buffer.getPointer(); }
>> @@ -816,7 +810,22 @@ public:
>>SrcMgr::CharacteristicKind FileCharacter =
>> SrcMgr::C_User,
>>int LoadedID = 0, unsigned LoadedOffset = 0,
>>SourceLocation IncludeLoc = SourceLocation()) {
>> -return createFileID(createMemBufferContentCache(std::move(Buffer)),
>> +return createFileID(
>> +createMemBufferContentCache(Buffer.release(), /*DoNotFree*/ false),
>> +IncludeLoc, FileCharacter, LoadedID, LoadedOffset);
>> + }
>> +
>> + enum UnownedTag { Unowned };
>> +
>> + /// \brief Create a new FileID that represents the specified memory
>> buffer.
>> + ///
>> + /// This does no caching of the buffer and takes ownership of the
>
>
> Is the "and takes ownership" part correct for this new overload?
Oops, no. Comment fixed in r359158.
>> + /// MemoryBuffer, so only pass a MemoryBuffer to this once.
>> + FileID createFileID(UnownedTag, llvm::MemoryBuffer *Buffer,
>> + SrcMgr::CharacteristicKind FileCharacter =
>> SrcMgr::C_User,
>> + int LoadedID = 0, unsigned LoadedOffset = 0,
>> + SourceLocation IncludeLoc = SourceLocation()) {
>> +return createFileID(createMemBufferContentCache(Buffer,
>> /*DoNotFree*/true),
>> IncludeLoc, FileCharacter, LoadedID, LoadedOffset);
>>}
>>
>> @@ -1699,7 +1708,7 @@ private:
>>
>>/// \brief Create a new ContentCache for the specified memory buffer.
>>const SrcMgr::ContentCache *
>> - createMemBufferContentCache(std::unique_ptr Buf);
>> + createMemBufferContentCache(llvm::MemoryBuffer *Buf, bool DoNotFree);
>>
>>FileID getFileIDSlow(unsigned SLocOffset) const;
>>FileID getFileIDLocal(unsigned SLocOffset) const;
>>
>> Modified: cfe/trunk/include/clang/Frontend/FrontendOptions.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/FrontendOptions.h?rev=312851=312850=312851=diff
>> ==
>> --- cfe/trunk/include/clang/Frontend/FrontendOptions.h (original)
>> +++ cfe/trunk/include/clang/Frontend/FrontendOptions.h Fri Sep 8 18:14:04
>> 2017
>> @@ -128,21 +128,24 @@ class FrontendInputFile {
>>/// \brief The file name, or "-" to read from standard input.
>>std::string File;
>>
>> - llvm::MemoryBuffer *Buffer;
>> + /// The input, if it comes from a buffer rather than a file. This object
>> + /// does not own the buffer, and the caller is responsible for ensuring
>> + /// that it outlives any users.
>> + llvm::MemoryBuffer *Buffer = nullptr;
>>
>>/// \brief The kind of input, e.g., C source, AST file, LLVM IR.
>>InputKind Kind;
>>
>>/// \brief Whether we're dealing with a 'system' input (vs. a 'user'
>> input).
>> - bool IsSystem;
>> + bool IsSystem = false;
>>
>> public:
>> - FrontendInputFile() : Buffer(nullptr), Kind(), IsSystem(false) { }
>> + FrontendInputFile() { }
>>FrontendInputFile(StringRef File, InputKind Kind, bool IsSystem = false)
>> -: File(File.str()), Buffer(nullptr), Kind(Kind), IsSystem(IsSystem) { }
>> -
Re: r312851 - Fix ownership of the MemoryBuffer in a FrontendInputFile.
(below)
On Fri, Sep 8, 2017 at 9:15 PM Richard Smith via cfe-commits <
cfe-commits@lists.llvm.org> wrote:
> Author: rsmith
> Date: Fri Sep 8 18:14:04 2017
> New Revision: 312851
>
> URL: http://llvm.org/viewvc/llvm-project?rev=312851=rev
> Log:
> Fix ownership of the MemoryBuffer in a FrontendInputFile.
>
> This fixes a possible crash on certain kinds of corrupted AST file, but
> checking in an AST file corrupted in just the right way will be a
> maintenance
> nightmare because the format changes frequently.
>
> Modified:
> cfe/trunk/include/clang/Basic/SourceManager.h
> cfe/trunk/include/clang/Frontend/FrontendOptions.h
> cfe/trunk/lib/Basic/SourceManager.cpp
> cfe/trunk/lib/Frontend/CompilerInstance.cpp
> cfe/trunk/lib/Frontend/FrontendAction.cpp
>
> Modified: cfe/trunk/include/clang/Basic/SourceManager.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/SourceManager.h?rev=312851=312850=312851=diff
>
> ==
> --- cfe/trunk/include/clang/Basic/SourceManager.h (original)
> +++ cfe/trunk/include/clang/Basic/SourceManager.h Fri Sep 8 18:14:04 2017
> @@ -212,12 +212,6 @@ namespace SrcMgr {
> /// this content cache. This is used for performance analysis.
> llvm::MemoryBuffer::BufferKind getMemoryBufferKind() const;
>
> -void setBuffer(std::unique_ptr B) {
> - assert(!Buffer.getPointer() && "MemoryBuffer already set.");
> - Buffer.setPointer(B.release());
> - Buffer.setInt(0);
> -}
> -
> /// \brief Get the underlying buffer, returning NULL if the buffer is
> not
> /// yet available.
> llvm::MemoryBuffer *getRawBuffer() const { return
> Buffer.getPointer(); }
> @@ -816,7 +810,22 @@ public:
>SrcMgr::CharacteristicKind FileCharacter =
> SrcMgr::C_User,
>int LoadedID = 0, unsigned LoadedOffset = 0,
>SourceLocation IncludeLoc = SourceLocation()) {
> -return createFileID(createMemBufferContentCache(std::move(Buffer)),
> +return createFileID(
> +createMemBufferContentCache(Buffer.release(), /*DoNotFree*/
> false),
> +IncludeLoc, FileCharacter, LoadedID, LoadedOffset);
> + }
> +
> + enum UnownedTag { Unowned };
> +
> + /// \brief Create a new FileID that represents the specified memory
> buffer.
> + ///
> + /// This does no caching of the buffer and takes ownership of the
>
Is the "and takes ownership" part correct for this new overload?
> + /// MemoryBuffer, so only pass a MemoryBuffer to this once.
> + FileID createFileID(UnownedTag, llvm::MemoryBuffer *Buffer,
> + SrcMgr::CharacteristicKind FileCharacter =
> SrcMgr::C_User,
> + int LoadedID = 0, unsigned LoadedOffset = 0,
> + SourceLocation IncludeLoc = SourceLocation()) {
> +return createFileID(createMemBufferContentCache(Buffer,
> /*DoNotFree*/true),
> IncludeLoc, FileCharacter, LoadedID,
> LoadedOffset);
>}
>
> @@ -1699,7 +1708,7 @@ private:
>
>/// \brief Create a new ContentCache for the specified memory buffer.
>const SrcMgr::ContentCache *
> - createMemBufferContentCache(std::unique_ptr Buf);
> + createMemBufferContentCache(llvm::MemoryBuffer *Buf, bool DoNotFree);
>
>FileID getFileIDSlow(unsigned SLocOffset) const;
>FileID getFileIDLocal(unsigned SLocOffset) const;
>
> Modified: cfe/trunk/include/clang/Frontend/FrontendOptions.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/FrontendOptions.h?rev=312851=312850=312851=diff
>
> ==
> --- cfe/trunk/include/clang/Frontend/FrontendOptions.h (original)
> +++ cfe/trunk/include/clang/Frontend/FrontendOptions.h Fri Sep 8 18:14:04
> 2017
> @@ -128,21 +128,24 @@ class FrontendInputFile {
>/// \brief The file name, or "-" to read from standard input.
>std::string File;
>
> - llvm::MemoryBuffer *Buffer;
> + /// The input, if it comes from a buffer rather than a file. This object
> + /// does not own the buffer, and the caller is responsible for ensuring
> + /// that it outlives any users.
> + llvm::MemoryBuffer *Buffer = nullptr;
>
>/// \brief The kind of input, e.g., C source, AST file, LLVM IR.
>InputKind Kind;
>
>/// \brief Whether we're dealing with a 'system' input (vs. a 'user'
> input).
> - bool IsSystem;
> + bool IsSystem = false;
>
> public:
> - FrontendInputFile() : Buffer(nullptr), Kind(), IsSystem(false) { }
> + FrontendInputFile() { }
>FrontendInputFile(StringRef File, InputKind Kind, bool IsSystem = false)
> -: File(File.str()), Buffer(nullptr), Kind(Kind), IsSystem(IsSystem) {
> }
> - FrontendInputFile(llvm::MemoryBuffer *buffer, InputKind Kind,
> +: File(File.str()), Kind(Kind), IsSystem(IsSystem) { }
> + FrontendInputFile(llvm::MemoryBuffer *Buffer, InputKind Kind,
>
r312851 - Fix ownership of the MemoryBuffer in a FrontendInputFile.
Author: rsmith
Date: Fri Sep 8 18:14:04 2017
New Revision: 312851
URL: http://llvm.org/viewvc/llvm-project?rev=312851=rev
Log:
Fix ownership of the MemoryBuffer in a FrontendInputFile.
This fixes a possible crash on certain kinds of corrupted AST file, but
checking in an AST file corrupted in just the right way will be a maintenance
nightmare because the format changes frequently.
Modified:
cfe/trunk/include/clang/Basic/SourceManager.h
cfe/trunk/include/clang/Frontend/FrontendOptions.h
cfe/trunk/lib/Basic/SourceManager.cpp
cfe/trunk/lib/Frontend/CompilerInstance.cpp
cfe/trunk/lib/Frontend/FrontendAction.cpp
Modified: cfe/trunk/include/clang/Basic/SourceManager.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/SourceManager.h?rev=312851=312850=312851=diff
==
--- cfe/trunk/include/clang/Basic/SourceManager.h (original)
+++ cfe/trunk/include/clang/Basic/SourceManager.h Fri Sep 8 18:14:04 2017
@@ -212,12 +212,6 @@ namespace SrcMgr {
/// this content cache. This is used for performance analysis.
llvm::MemoryBuffer::BufferKind getMemoryBufferKind() const;
-void setBuffer(std::unique_ptr B) {
- assert(!Buffer.getPointer() && "MemoryBuffer already set.");
- Buffer.setPointer(B.release());
- Buffer.setInt(0);
-}
-
/// \brief Get the underlying buffer, returning NULL if the buffer is not
/// yet available.
llvm::MemoryBuffer *getRawBuffer() const { return Buffer.getPointer(); }
@@ -816,7 +810,22 @@ public:
SrcMgr::CharacteristicKind FileCharacter =
SrcMgr::C_User,
int LoadedID = 0, unsigned LoadedOffset = 0,
SourceLocation IncludeLoc = SourceLocation()) {
-return createFileID(createMemBufferContentCache(std::move(Buffer)),
+return createFileID(
+createMemBufferContentCache(Buffer.release(), /*DoNotFree*/ false),
+IncludeLoc, FileCharacter, LoadedID, LoadedOffset);
+ }
+
+ enum UnownedTag { Unowned };
+
+ /// \brief Create a new FileID that represents the specified memory buffer.
+ ///
+ /// This does no caching of the buffer and takes ownership of the
+ /// MemoryBuffer, so only pass a MemoryBuffer to this once.
+ FileID createFileID(UnownedTag, llvm::MemoryBuffer *Buffer,
+ SrcMgr::CharacteristicKind FileCharacter =
SrcMgr::C_User,
+ int LoadedID = 0, unsigned LoadedOffset = 0,
+ SourceLocation IncludeLoc = SourceLocation()) {
+return createFileID(createMemBufferContentCache(Buffer, /*DoNotFree*/true),
IncludeLoc, FileCharacter, LoadedID, LoadedOffset);
}
@@ -1699,7 +1708,7 @@ private:
/// \brief Create a new ContentCache for the specified memory buffer.
const SrcMgr::ContentCache *
- createMemBufferContentCache(std::unique_ptr Buf);
+ createMemBufferContentCache(llvm::MemoryBuffer *Buf, bool DoNotFree);
FileID getFileIDSlow(unsigned SLocOffset) const;
FileID getFileIDLocal(unsigned SLocOffset) const;
Modified: cfe/trunk/include/clang/Frontend/FrontendOptions.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/FrontendOptions.h?rev=312851=312850=312851=diff
==
--- cfe/trunk/include/clang/Frontend/FrontendOptions.h (original)
+++ cfe/trunk/include/clang/Frontend/FrontendOptions.h Fri Sep 8 18:14:04 2017
@@ -128,21 +128,24 @@ class FrontendInputFile {
/// \brief The file name, or "-" to read from standard input.
std::string File;
- llvm::MemoryBuffer *Buffer;
+ /// The input, if it comes from a buffer rather than a file. This object
+ /// does not own the buffer, and the caller is responsible for ensuring
+ /// that it outlives any users.
+ llvm::MemoryBuffer *Buffer = nullptr;
/// \brief The kind of input, e.g., C source, AST file, LLVM IR.
InputKind Kind;
/// \brief Whether we're dealing with a 'system' input (vs. a 'user' input).
- bool IsSystem;
+ bool IsSystem = false;
public:
- FrontendInputFile() : Buffer(nullptr), Kind(), IsSystem(false) { }
+ FrontendInputFile() { }
FrontendInputFile(StringRef File, InputKind Kind, bool IsSystem = false)
-: File(File.str()), Buffer(nullptr), Kind(Kind), IsSystem(IsSystem) { }
- FrontendInputFile(llvm::MemoryBuffer *buffer, InputKind Kind,
+: File(File.str()), Kind(Kind), IsSystem(IsSystem) { }
+ FrontendInputFile(llvm::MemoryBuffer *Buffer, InputKind Kind,
bool IsSystem = false)
-: Buffer(buffer), Kind(Kind), IsSystem(IsSystem) { }
+: Buffer(Buffer), Kind(Kind), IsSystem(IsSystem) { }
InputKind getKind() const { return Kind; }
bool isSystem() const { return IsSystem; }
Modified: cfe/trunk/lib/Basic/SourceManager.cpp
URL:
