[llvm] r211861 - ParseIR: don't take ownership of the MemoryBuffer

[Alp Toker]

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.

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