[llvm] r211595 - Pass a unique_ptr<MemoryBuffer> to the constructors in the Binary hierarchy.

Alp Toker alp at nuanti.com
Mon Jul 7 16:21:00 PDT 2014

On 08/07/2014 02:00, David Blaikie wrote:
> On Fri, Jul 4, 2014 at 7:35 PM, Alp Toker <alp at nuanti.com> wrote:
>> On 05/07/2014 04:59, Rafael EspĂ­ndola wrote:
>>>> Where? *All* current uses on ToT seem to mmap their own private
>>>> MemoryBuffer
>>>> and un-mmap it immediately afterwards.
>>> Where? Show an strace where an **ObjectFile** is reopened.
>> Trivially: just name them multiple times in llvm-dwarfdump/lli. That's
>> probably fine, but less so with the proposed LTO object file patch where
>> there's will be real cost.
>>> With object files we don't have the complexities of a source manager.
>>> There are two reasonable ownership models: It owns the buffer and it
>>> don't own the buffer. Not owning it might be more convenient,, I am
>>> looking into it. A reference count makes no sense for the uses we
>>> have.
>> Reference counting is tangential to the discussion.
>> The caller can do what they want to provision the MemoryBuffer instances --
>> that could be reference counting, malloc or who knows, even stack
>> allocation. It's invisible to ObjectFile as long as it only passes around
>> pointers.
>> Refcounting was just my suggestion for the slicing problem -- not a
>> requirement or demand in any way, and you already said it's not needed..
>>> If the complexities of a source manager require a reference count to
>>> avoid duplicating read and stats, then it should hold a list of
>>> std::shared_ptr, or a list of a RefCountedMemoryBuffer. Changing how
>>> ObjectFile works because a *source* manager wants to refcount buffers
>>> is not OK.
>> My point is that it's not OK to delete the caller's MemoryBuffer, nor does
>> it make sense to pass it around in a std::unique_ptr given that the buffer
>> is clearly shareable and holds no state.
>> If you like you can store a pointer, keep a reference, or refcount -- but
>> two commits look wrong because they enforce unique ownership on a resource
>> that clearly shouldn't be uniquely owned.
> The answer is fairly simple: Rafael's change didn't change any
> ownership semantics, it merely made them more explicit.

There's no doubt the commit made the ownership semantics explicit. 
Indeed the commit made it very explicit across several LLVM modules..

> IMHO, this is
> always an acceptable/good change. If someone wants to /change/ the
> ownership, that's welcome (if the change is good), but just making the
> ownership clear/explicit in the API should be an acceptable change,
> even if that ownership is in some way not good. It's better that it's
> clear.

Disagree. It would be better to fix the dubious ownership rather than to 
propagate the design flaw by making it explicit in so many interfaces.

We can argue until we're blue in the face about whether it's good to 
expose this, but at the heart of the matter I don't think it makes sense 
for these types to unmap OS files when they're done.

These are lightweight objects whereas OS resources are heavy/shareable 
so unique_ptr is "obviously" the wrong as far as I can tell.

> (as evidence that this change did not change the ownership, merely
> made it explicit:
> -Binary::Binary(unsigned int Type, MemoryBuffer *Source)
> -    : TypeID(Type), Data(Source) {}
> +Binary::Binary(unsigned int Type, std::unique_ptr<MemoryBuffer> Source)
> +    : TypeID(Type), Data(std::move(Source)) {}
> (the type of the "Data" member was, before and after this patch,
> std::unique_ptr<MemoryBuffer>))

The point is, why should a Binary own a unique data buffer reference? It 
could either hold a pointer, a reference or maybe a refcount -- all 
would be legitimate -- but a unique pointer is the one that doesn't make 

I agree this class already had the behaviour but I'm not happy that the 
sketchy ownership is now embodied formally in the interface.


>> Alp.
>>> Cheers,
>>> Rafael
>> --
>> http://www.nuanti.com
>> the browser experts
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

the browser experts

More information about the llvm-commits mailing list