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

David Blaikie dblaikie at gmail.com
Thu Jun 26 22:50:34 PDT 2014 

On Thu, Jun 26, 2014 at 10:41 PM, Alp Toker <alp at nuanti.com> wrote:
>
> On 27/06/2014 08:24, David Blaikie wrote:
>>
>> 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.
>
>
> David,
>
> We do occasionally follow the malloc tradition of non-constness to indicate
> capture/ownership transfer in LLVM, especially on types like this where
> const doesn't otherwise have significance.

We do? It's certainly not something I've seen in any
deliberate/thought-out way in the LLVM codebase, so far as I know.
Certainly not in the style guide or something I've heard anyone
articulate as being an intention (for their code, or the project as a
whole). It's not really something I'd like to see encouraged/used in
the project, personally, either.

> (I don't actually see a code
> smell here, other than that perhaps "const" isn't the best thought out
> aspect of C/C++.)

I think we already agreed that const_cast is a smell itself - perhaps
to different degrees. For me, const_cast is to the point of, unless
it's a classic idiom like like strstr (where the const violation is
contained to basically a single line, too - so it's easy to inspect
for correctness at a glance), I'd expect to see an explanation as to
why it's justified (or what large cleanup work would need to be done
to remove it) - so when I see it I have a sense of whether someone
made a short sighted decision and I should go & fix what they didn't
bother to, or whether it's a giant rats-nest of const-incorrectness I
should just leave alone until I have a spare afternoon/day/weekend.

> The change was simply made to give a visual cue to hapless embedders that
> the semantics have changed but I'm perfectly fine to remove it.

Give it a day or two, or a few weeks and no one will know why one
MemoryBuffer is const and another isn't - it'll just be another
inconsistency in the codebase without much in the way of a clue as to
how/why it got there & whether or not it should be removed or kept
around for some reason.

> Also finding your reviews to be slightly zealous ;-)

Sorry about that - code cleanup sort of changes are prone to a bit
more bikeshedding due to their accessibility (it's easy for me (or
anyone else) to read them, understand them, and critique them) it's
sort of a weird interaction where the changes that least need review
(because, in the grand scheme of things they don't matter that much)
get the most scrutiny. But also somewhat useful, since they tend to be
a good place for newer contributors to get started, so a little more
scrutiny in these sort of things is a great way to hash out project
style issues, make sure everyone's on the same page, etc, so when more
substantial/significant changes are made, these sort of issues aren't
such a major point of contention.

That being said, sometimes even seasoned contributors are on both
sides - see, for example, my discussion/review with Manuel regarding
FrontendActionFactories on cfe-dev/cfe-commits.

- David

>
> Alp.
>
>
>
>>
>> (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
>>>>
>
> --
> http://www.nuanti.com
> the browser experts
>

More information about the llvm-commits mailing list