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

[David Blaikie]

On Mon, Jun 23, 2014 at 12:56 PM, Rafael Espíndola
<rafael.espindola at gmail.com> wrote:
>>> 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).
>
> OK, I agree we are still not at the stage of always explicitly passing
> ownership, but this should be a step in the right direction, no?

That's the thing - I'm not entirely sure it is. I think it might be
papering over the wart and just hiding the problem a bit deeper/more
subtly. Though to be clear I'm not adamant/immovable/"over my dead
body" about this... I'm just really not sure. You've certainly got
more context (& more immediate interest in its maintainability - I
don't work in this code much/at all) in this part of the codebase than
I do, so I'm happy to defer to you. Just trying to express my concerns
insofar as they might help choose a good direction forward.

To jump off from Alp's comments - is there a longer term
plan/goal/end-state you've got in mind, even if it's a bit vague?

I'm not really sure I see anything better, long-term, than what I was
suggesting - where a factory wrapper adds ownership via decoration,
isolating the ownership aspect in a way that's orthogonal and
non-invasive to the ObjectFile hierarchy, ensuring the caller has the
right API, etc.

>> 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.
>
> Having ObjectFile own the buffer seems to be the more common case
> actually. Same goes for the IR reader.
>
>>>
>>>> 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.
>
> OK.
>
> IMHO the patch as is in already an improvement since with it no object
> ever has to keep a distinct boolean saying it owns the buffer. I will
> try to update the patch a bit further down the unique_ptr path by
> changing the create methods as you suggested.
>
> Cheers,
> Rafael