[llvm] r211861 - ParseIR: don't take ownership of the MemoryBuffer
David Blaikie
dblaikie at gmail.com
Thu Jun 26 22:06:58 PDT 2014
On Thu, Jun 26, 2014 at 10:00 PM, Alp Toker <alp at nuanti.com> wrote:
>
> On 27/06/2014 07:52, David Blaikie wrote:
>>
>> On Thu, Jun 26, 2014 at 9:33 PM, Alp Toker <alp at nuanti.com> wrote:
>>>
>>> Author: alp
>>> Date: Thu Jun 26 23:33:58 2014
>>> New Revision: 211861
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=211861&view=rev
>>> Log:
>>> ParseIR: don't take ownership of the MemoryBuffer
>>>
>>> clang was needlessly duplicating whole memory buffer contents in an
>>> attempt to
>>> satisfy unclear ownership semantics. Let's just hide internal LLVM quirks
>>> and
>>> present a simple non-owning interface.
>>>
>>> The public C API preserves previous behaviour for stability.
>>>
>>> Modified:
>>> llvm/trunk/include/llvm/IRReader/IRReader.h
>>> llvm/trunk/lib/AsmParser/Parser.cpp
>>> llvm/trunk/lib/IRReader/IRReader.cpp
>>>
>>> Modified: llvm/trunk/include/llvm/IRReader/IRReader.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IRReader/IRReader.h?rev=211861&r1=211860&r2=211861&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/include/llvm/IRReader/IRReader.h (original)
>>> +++ llvm/trunk/include/llvm/IRReader/IRReader.h Thu Jun 26 23:33:58 2014
>>> @@ -40,9 +40,9 @@ Module *getLazyIRFileModule(const std::s
>>>
>>> /// If the given MemoryBuffer holds a bitcode image, return a Module
>>> /// for it. Otherwise, attempt to parse it as LLVM Assembly and return
>>> -/// a Module for it. This function *always* takes ownership of the given
>>> -/// MemoryBuffer.
>>> -Module *ParseIR(MemoryBuffer *Buffer, SMDiagnostic &Err, LLVMContext
>>> &Context);
>>> +/// a Module for it. This function *never* takes ownership of Buffer.
>>> +Module *ParseIR(const MemoryBuffer *Buffer, SMDiagnostic &Err,
>>> + LLVMContext &Context);
>>>
>>> /// If the given file holds a bitcode image, return a Module for it.
>>> /// Otherwise, attempt to parse it as LLVM Assembly and return a Module
>>>
>>> Modified: llvm/trunk/lib/AsmParser/Parser.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/AsmParser/Parser.cpp?rev=211861&r1=211860&r2=211861&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/lib/AsmParser/Parser.cpp (original)
>>> +++ llvm/trunk/lib/AsmParser/Parser.cpp Thu Jun 26 23:33:58 2014
>>> @@ -54,8 +54,7 @@ Module *llvm::ParseAssemblyFile(const st
>>> Module *llvm::ParseAssemblyString(const char *AsmString, Module *M,
>>> SMDiagnostic &Err, LLVMContext
>>> &Context) {
>>> MemoryBuffer *F =
>>> - MemoryBuffer::getMemBuffer(StringRef(AsmString, strlen(AsmString)),
>>> - "<string>");
>>> + MemoryBuffer::getMemBuffer(StringRef(AsmString), "<string>");
>>>
>>> return ParseAssembly(F, M, Err, Context);
>>> }
>>>
>>> Modified: llvm/trunk/lib/IRReader/IRReader.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IRReader/IRReader.cpp?rev=211861&r1=211860&r2=211861&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/lib/IRReader/IRReader.cpp (original)
>>> +++ llvm/trunk/lib/IRReader/IRReader.cpp Thu Jun 26 23:33:58 2014
>>> @@ -39,7 +39,7 @@ Module *llvm::getLazyIRModule(MemoryBuff
>>> if (std::error_code EC = ModuleOrErr.getError()) {
>>> Err = SMDiagnostic(Buffer->getBufferIdentifier(),
>>> SourceMgr::DK_Error,
>>> EC.message());
>>> - // ParseBitcodeFile does not take ownership of the Buffer in the
>>> + // getLazyBitcodeModule does not take ownership of the Buffer in
>>> the
>>> // case of an error.
>>> delete Buffer;
>>> return nullptr;
>>> @@ -62,13 +62,14 @@ Module *llvm::getLazyIRFileModule(const
>>> return getLazyIRModule(File.release(), Err, Context);
>>> }
>>>
>>> -Module *llvm::ParseIR(MemoryBuffer *Buffer, SMDiagnostic &Err,
>>> +Module *llvm::ParseIR(const MemoryBuffer *Buffer, SMDiagnostic &Err,
>>> LLVMContext &Context) {
>>> NamedRegionTimer T(TimeIRParsingName, TimeIRParsingGroupName,
>>> TimePassesIsEnabled);
>>> if (isBitcode((const unsigned char *)Buffer->getBufferStart(),
>>> (const unsigned char *)Buffer->getBufferEnd())) {
>>> - ErrorOr<Module *> ModuleOrErr = parseBitcodeFile(Buffer, Context);
>>> + ErrorOr<Module *> ModuleOrErr =
>>> + parseBitcodeFile(const_cast<MemoryBuffer *>(Buffer), Context);
>>
>> What's the reason for this const_cast? Does parseBitcodeFile actually
>> modify the MemoryBuffer? If so, I'd rather keep the ParseIR contract
>> truthful and keep the non-const pointer parameter. If not, a comment
>> to explain that parseBitcodeFile is actually non-mutating (either
>> statically (in which case a FIXME suggesting it should be changed to
>> be const correct), or dynamically for this particular caller)?
>
>
> It's probably just that the interfaces didn't aim for const-correctness,
> same as StringRef.
>
> Anyway, I've propagated the const a little further until
> getLazyBitcodeModule() which still doesn't modify, but can take ownership
> when used from other code paths so is best left non-const.
Just because it takes ownership doesn't mean it should be non-const.
Still gone from no const casts to one const cast - so a comment
(probably FIXME) would be appropriate. const_casts are a real code
smell.
- David
>
> r211864.
>
> Alp.
>
>
>
>>
>>> Module *M = nullptr;
>>> if (std::error_code EC = ModuleOrErr.getError())
>>> Err = SMDiagnostic(Buffer->getBufferIdentifier(),
>>> SourceMgr::DK_Error,
>>> @@ -76,11 +77,12 @@ Module *llvm::ParseIR(MemoryBuffer *Buff
>>> else
>>> M = ModuleOrErr.get();
>>> // parseBitcodeFile does not take ownership of the Buffer.
>>> - delete Buffer;
>>> return M;
>>> }
>>>
>>> - return ParseAssembly(Buffer, nullptr, Err, Context);
>>> + return ParseAssembly(MemoryBuffer::getMemBuffer(
>>> + Buffer->getBuffer(),
>>> Buffer->getBufferIdentifier()),
>>> + nullptr, Err, Context);
>>> }
>>>
>>> Module *llvm::ParseIRFile(const std::string &Filename, SMDiagnostic
>>> &Err,
>>> @@ -92,7 +94,7 @@ Module *llvm::ParseIRFile(const std::str
>>> return nullptr;
>>> }
>>>
>>> - return ParseIR(File.release(), Err, Context);
>>> + return ParseIR(File.get(), Err, Context);
>>> }
>>>
>>>
>>> //===----------------------------------------------------------------------===//
>>> @@ -104,7 +106,8 @@ LLVMBool LLVMParseIRInContext(LLVMContex
>>> char **OutMessage) {
>>> SMDiagnostic Diag;
>>>
>>> - *OutM = wrap(ParseIR(unwrap(MemBuf), Diag, *unwrap(ContextRef)));
>>> + std::unique_ptr<MemoryBuffer> MB(unwrap(MemBuf));
>>> + *OutM = wrap(ParseIR(MB.get(), Diag, *unwrap(ContextRef)));
>>>
>>> if(!*OutM) {
>>> if (OutMessage) {
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
> --
> http://www.nuanti.com
> the browser experts
>
More information about the llvm-commits
mailing list