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

[David Blaikie]

(+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.

(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)

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.

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)?

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?

On Mon, Jun 23, 2014 at 7:24 AM, Rafael Espíndola
<rafael.espindola at gmail.com> wrote:
> ping.  A rebased patch is attached.
>
>
>
> On 19 June 2014 00:15, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
>>> Incidentally, I noticed this while trying out a patch to cleanup
>>> another use of a isOwned flag, so I agree it is problematic :-(
>>
>> A bot convinced me that this needed fixing sooner rather than later :-(
>>
>> The general issue is that we normally want the convenience of the
>> buffer being destroyed by the thing that is using it (materializer,
>> ObjectFile), but sometimes we want to reuse the buffer.
>>
>> The solution that is currently implemented is mostly:
>>
>> * The thing using the buffer has a Boolean member saying if it owns it or not.
>> * The createThing methods take an equivalent Boolean argument.
>>
>> The end result is somewhat brittle and fairly non idiomatic. A more
>> C++11ish solution would be for the users to have a
>> std::unique_ptr<MemoryBuffer> and a way of releasing ownership when
>> requested. That way the user buffer can "lend" the buffer and take it
>> back afterwards.
>>
>> The attached patch implements it and IMHO is an improvement,  but it
>> is not without its ugly corner cases:
>>
>> * The createThing methods now must not delete the buffer if they fail
>> to construct a Thing. This makes error handling on the common case
>> (Thing owns buffer) a bit more cumbersome.
>> * We need to add a way to release a buffer to the GVMaterializer
>> interface. This is a really odd single implementation interface and
>> needs a fairly thorough refactoring, so I am not too worried about it.
>> * materializeAllPermanently needs to know if it should release the
>> buffer before destroying the materializer.
>>
>> Cheers,
>> Rafael