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

Mon Jul 7 20:17:31 PDT 2014

On Mon, Jul 7, 2014 at 4:21 PM, Alp Toker <alp at nuanti.com> wrote:
>
> 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.

I don't disagree with that (I agree with it in theory, but I haven't
looked enough to know whether the ownership you're proposing is
possible or an improvement - I think it might be both possible and an
improvement) - but that's up to whether Rafael wants to do that work.

I think this relatively small amount of work is better than not doing
it - the code is better for having its contracts explicit in the type
system. That there may be different contracts it could use is mostly
orthogonal to this - Rafael reasonably volunteered to do the work of
making the ownershp explicit here, and I don't really think it makes
it substantially harder to change the ownership later - you'd still
have to visit all the callers and implementations anyway. It's fairly
mechanical transformation if someone does eventually make these
entirely non-owning pointers all the way down.

So I'd rather leave this patch in than revert it for some abstract
future in which the ownership semantics are changed here.

If that abstract future happens really soon, then it'll be easy to
basically revert this patch locally and build the non-owning solution
in place (if that's any easier than just building the non-owning
solution from wherever the source tree stands on that day).

Rafael's change here makes the change in ownership no more or less
pressing (though I like to think pushing ownership into the interfaces
helps highlight just what the ownership is, cause people to ask
questions like the ones your asking, and perhaps motivates someone to
change it when the ownership looks sufficiently weird - and its easy
for implicit ownership to appear less weird because you don't realize
just what's going on when you look at the code) and isn't Rafael's
responsibility as the committer of this patch, to fix the ownership
issues.

Essentially from my perspective a discussion that sounds very much
like "this patch is wrong/we shouldn't do this" isn't really suited to
this commit. The commit improved the state of things, perhaps exposed
explicitly just how weird that state is, but that's fine. Any
discussion we have from here should be "this is weird, it'd be good to
make it less weird, here's why we should do that" with the hopes that
you can convince someone to do this work you think is important (by
convincing them that it's important) or "hey, that's weird, I'll go
fix it". Neither of these revolve around negative sentiments about
this patch, or argue that it should be reverted.

- David

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