[llvm] r211861 - ParseIR: don't take ownership of the MemoryBuffer
Alp Toker
alp at nuanti.com
Thu Jun 26 22:14:21 PDT 2014
On 27/06/2014 08:06, David Blaikie wrote:
> 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.
A const_cast<> *always* indicates a constness issue so there's no real
need for an additional TODO. Of course feel free to poke around if you
want to garden in this area..
Alp.
>
> - 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
>>
--
http://www.nuanti.com
the browser experts
More information about the llvm-commits
mailing list