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

[Alp Toker]

On 23/06/2014 22:56, Rafael Espíndola 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?

My two pence..

I felt that Rafael's patch, though not more correct than the previous 
ownership, can be considered a reasonable step towards achieving a 
proper fix by virtue of reducing the footprint of ugly ownership code 
that we definitely don't want.

I suspect Rafael won't leave it this way for long -- if that's indeed 
the case, perhaps it's not so bad to land this as an intermediate step?

Rafael, did I get this right?

Alp.



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