[lld] r264022 - Use owning pointers instead of raw pointers for Atom's to fix leaks.

[Pete Cooper via llvm-commits]

> On Mar 21, 2016, at 9:11 PM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Mon, Mar 21, 2016 at 9:08 PM, Pete Cooper <peter_cooper at apple.com <mailto:peter_cooper at apple.com>> wrote:
> 
>> On Mar 21, 2016, at 9:05 PM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
>> 
>> 
>> 
>> On Mon, Mar 21, 2016 at 8:44 PM, Pete Cooper via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>> Author: pete
>> Date: Mon Mar 21 22:44:32 2016
>> New Revision: 264022
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=264022&view=rev <http://llvm.org/viewvc/llvm-project?rev=264022&view=rev>
>> Log:
>> Use owning pointers instead of raw pointers for Atom's to fix leaks.
>> 
>> Currently each File contains an BumpPtrAllocator in which Atom's are
>> allocated.  Some Atom's contain data structures like std::vector which
>> leak as we don't run ~Atom when they are BumpPtrAllocate'd.
>> 
>> FWIW, if the only thing allocated in the BumpPtrAllocator is Atoms, you could use a SpecificBumpPtrAllocator, which does run the doors.
> I think there might be Reference’s in there, but worth investigating doing it this way.  Unfortunately the Window’s bots didn’t like what I did here so i’ve reverted for now.
>> 
>> (& if the BumpPtrAllocator doesn't contain only Atoms, you could change it so it does)
>>  
>> 
>> Now each File actually owns its Atom's using an OwningAtomPtr.  This
>> is analygous to std::unique_ptr and may be replaced by it if possible.
>> 
>> Yeah, this looks like it could just be a typedef of unique_ptr with a custom deleter that only runs the dtor but doesn't delete, etc.
> Just tried this as a quick fix to get the bots to work.  Unfortunately the YAML parser doesn’t like that solution. The problem is that the YAML parser wants to take a reference to a pointer, so i had to make the get() methods be defined as follows to support that.  std::unique_ptr doesn’t return a reference from get() so the YAML parser stops working with them.
> 
> Ah, yeah, that's not a safe operation (in the mutable case, especially) for a smart pointer - so if we're going to smart pointerify these things, probably need to adjust that code no matter which smart pointer we use.
Yeah, Lang and I just went over that code.  We probably need to rewrite the YAML code, possibly all the YAML code, to not require the references.  I’ve recommitted the code as it was for now to get us something in that doesn’t leak all over the place.  Then we can refactor it with the bots backing us up when we do it wrong.
> 
> Did you try making the dtor virtual to fix MSVC's issues? If you are destroying derived objects your dtor does need to be virtual if you're calling it from a base object expression. 
Turns out the dtor was virtual, but that MSVC didn’t like calling the base class dtor on a derived class.  So given ‘DefinedAtom *a’, 'a->~Atom()’ generated an error.  I’ve fixed that in the recommit to see if that appeases the bots.

Cheers,
Pete
>>  
>> 
>> +  T *const &get() const {
>> +    return atom;
>> +  }
>> +
>> +  T *&get() {
>> +    return atom;
>> +  }
>> +
>> 
> 
> Cheers,
> Pete

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160322/816a69a0/attachment.html>