[llvm] r211861 - ParseIR: don't take ownership of the MemoryBuffer
Alp Toker
alp at nuanti.com
Thu Jun 26 22:41:30 PDT 2014
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. (I don't actually
see a code smell here, other than that perhaps "const" isn't the best
thought out aspect of C/C++.)
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.
Also finding your reviews to be slightly zealous ;-)
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