[llvm] r211184 - Fix a memory leak in the error path.

[David Blaikie]

On Mon, Jun 23, 2014 at 12:27 PM, Rafael Espíndola
<rafael.espindola at gmail.com> wrote:
> On 23 June 2014 14:23, David Blaikie <dblaikie at gmail.com> wrote:
>> (+lhames, as he and I have chatted a bit about MemoryBuffer ownership
>> semantics, libObject, RuntimeDyLd, etc recently)
>>
>> So the "interesting" part of this change are the callers who pass
>> "false" for ownership, such as llvm-ar.cpp::writeSymbolTable. Yeah, I
>> find this "loan" model rather unsatisfying because the ownership is
>> still actually shared - the owner (llvm-ar.cpp:performWriteOperation)
>> isn't participating in this loan - it's really just writeSymbolTable
>> lying to the createSymbolicFile function - saying it can own it, but
>> if it ever actually tries to use that power (eg: if a codepath lead to
>> releaseBuffer not being called, etc) it would be broken.
>>
>> I mean we could have the MemoryBuffers vector be a vector of
>> unique_ptrs and the unique_ptr could be moved into the SymbolicFile,
>> then moved out again... still doesn't seem particularly satisfying.
>
> Well, the movein/moveout pattern is intrinsic to using a unique_ptr, no?
>
>> (side note, the bit about createThing functions not being able to
>> cleanup - they should probably take a unique_ptr<MemoryBuffer> either
>> by reference or rvalue referenec - then conditionally move from it.
>> That way the caller wouldn't have to inspect the error result to
>> decide whether to cleanup, the unique_ptr would DTRT either way -
>> extending this further, that would force callers to pass ownership to
>> the Thing, then take it back later - rather than lying - but I'm still
>> not sure it's the right path in general)
>
> Good point about the create methods. I will give it a try.
>
>> How many different hierarchies, wrappers, and factories with this
>> conditional ownership are we dealing with? I can't quite keep them all
>> straight in my head. But my other idea would be to have the ownership
>> added by decoration - this would probably involve templating,
>> wrapping, or duplicating the factory functions in some way.
>
> Well, I would say that with this patch there is no conditional
> ownership any more. The buffer is moved from one owner to another.

Except it isn't. In the example I looked at (
llvm-ar.cpp::writeSymbolTable ) the caller still, for all intents and
purposes, still seems to have ownership - the pointers are still in
the vector and will still be deleted (indeed they might be replaced
with unique_ptrs in the vector at some point). It's not really
transferring ownership - it's pretending to. If the release never
occurred we wouldn't be in a state where the ObjectFile owned the
thing and the caller didn't, instead we'd be in the state where both
of them own it and a double delete would occur.

For example, to properly represent this ownership semantic,
llvm-ar.cpp::writeSymbolTable would have to take a
MutableArrayRef<unique_ptr<T>> rather than an ArrayRef<T*> - even
though the sequence /should/ be the same at the end of the function
call as at the beginning, because in the middle it'll move things out
of the sequence, then move them back... that doesn't seem like it
really reflects the contract of that function in the type system (we
have to make something mutable, which, semantically, it isn't).

>
>> For example - if it's only "createObjectFile" that's the entry point
>> with conditional ownership, we could make all the underlying
>> operations non-owning, the main entry point would be
>> createObjectFile(MemoryBuffer&, ...) (not taking ownership of the
>> MemoryBuffer - speaking of, should that be const? what're the mutable
>> operations on a MemoryBuffer anyway...) and then a wrapper:
>>
>> ... createObjectFile(std::unique_ptr<MemoryBuffer> m, ...) {
>>   auto OF = createObjectFile(m.get());
>>   return make_unique<OwningObjectFileDecorator>(std::move(m), std::move(OF));
>> }
>>
>> Where OwningObjectFileDecorator is just a trivial decorator (every
>> call falls through to the decorated object, plus ownership)?
>
> That seems more confusing then a model where the ObjectFile always
> owns the buffer but the buffer can be moved out of it.

If that were really what the callers wanted to do, I might be inclined
to agree - but it seems like the callers honestly never want the
ObjectFile to own the thing. They will unconditionally move it back
again anyway - but I'd be happier (if not exactly 'happy') with this
if it were actually moved around and the ownership transfer were more
clear, but still suspicious of it.

>
>> I mean I guess that's sort of equivalent to the world we're in today,
>> but hopefully separates concerns better. It does still make something
>> dynamic where it was static, but the only way to keep it static would
>> be for the whole hierarchy of ObjectFiles, their factories, etc,
>> templated, I think?
>>
>
> I would like to avoid the extra templates if possible.

Likewise - I'm just trying to enumerate the options that are possible
to make sure I haven't missed anything, etc.