[llvm] r211861 - ParseIR: don't take ownership of the MemoryBuffer
David Blaikie
dblaikie at gmail.com
Thu Jun 26 22:24:15 PDT 2014
Thinking about this further, MemoryBuffer has no mutating operations,
does it? So const MemoryBuffer and non-const MemoryBuffer are the same
thing, aren't they?
Unfortunately, imho, the LLVM convention is not to specify "const" on
immutable types (some of my earliest patches were to contribute to the
removal of const on llvm::Type after it became immutable) so might I
suggest you just remove the const you added - it adds no additional
safety.
(yes, personally I'd prefer we put const everywhere, but I've failed
to fight that fight & would rather consistency at least - yeah, I know
there's some "const MemoryBuffer" usage scattered around the place,
but it's well in the minority)
On Thu, Jun 26, 2014 at 10:20 PM, David Blaikie <dblaikie at gmail.com> wrote:
> 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