[llvm] r211861 - ParseIR: don't take ownership of the MemoryBuffer
David Blaikie
dblaikie at gmail.com
Thu Jun 26 22:20:26 PDT 2014
On Thu, Jun 26, 2014 at 10:14 PM, Alp Toker <alp at nuanti.com> wrote:
>
> 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
Sure, but it doesn't explain what the issue is - given that this code
was previously (before your recent commits) const correct in a loose
sense (the consts agreed) I'd like an explanation in the code why
adding const and casting it away is correct, not just blindly working
around a compiler error. Which is either "this API doesn't mutate the
argument for callers under these conditions that this call satisfies"
or "this API doesn't mutate the argument at all, it's just not const
correct right now, fix it at some point".
- David
> 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