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

[Rafael Espíndola]

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.

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

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

Cheers,
Rafael