[llvm] r208293 - Use a vector of unique_ptrs to fix a memory leak introduced in r208179.

[David Blaikie]

On Thu, May 8, 2014 at 11:35 AM, Daniel Sanders
<Daniel.Sanders at imgtec.com> wrote:
>> -----Original Message-----
>> From: David Blaikie [mailto:dblaikie at gmail.com]
>> Sent: 08 May 2014 19:14
>> To: Daniel Sanders
>> Cc: llvm-commits at cs.uiuc.edu
>> Subject: Re: [llvm] r208293 - Use a vector of unique_ptrs to fix a memory
>> leak introduced in r208179.
>>
>> On Thu, May 8, 2014 at 2:29 AM, Daniel Sanders
>> <daniel.sanders at imgtec.com> wrote:
>> > Author: dsanders
>> > Date: Thu May  8 04:29:28 2014
>> > New Revision: 208293
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=208293&view=rev
>> > Log:
>> > Use a vector of unique_ptrs to fix a memory leak introduced in r208179.
>> >
>> > Also removed an inaccurate comment that stated that a DenseMap was
>> > used as storage for the ListInit*'s. It's currently using a FoldingSet.
>>
>> So FoldingSet<T> is really a set of raw, non-owning, T*? Should we make it
>> FoldingSet<pointer-like-thing> (that would require updating all existing
>> uses from T to explicitly specify T*) and allow it to handle unique_ptr<T>
>> correctly as another "pointer-like-thing" so it can hold ownership?
>
> That's right, it holds T*'s but doesn't own them. For some reason it didn't occur to me to try FoldingSet<unique_ptr<T>> even though I thought of a container of unique_ptr's. I'll rework the commit in the morning.

I doubt that'll work - I'd wager you'll just get non-owning
"unique_ptr<T>*" (pointers to smart pointers... ).

>
>> > I expect there's a better way to fix this but I haven't found it yet.
>> > FoldingSet is incompatible with the Pool template
>>
>> Which Pool is that? (can't find a Pool.h... ) Does there seem to be a
>> fundamental reason FoldingSet+Pool can't play together? If not, should we
>> just fix that incompatibility?
>
> It's in an anonymous namespace in Record.cpp (line 561). It's a template class that subclasses its argument and adds a destructor to delete the items in the container. The main problem I had was that it expects the iterator to return pairs but it also expected a T::value_type which FoldingSet doesn't provide. I expect that all uses of Pool could be rewritten as containers of unique_ptr's easily but I'll have to come back to that after my MIPS64r6 work.

*nod* there's lots of 11 cleanup/simplification to do. No rush.

>
>> > and I'm not sure if FoldingSet can be
>> > safely replaced with a DenseMap of computed FoldingSetID's to ListInit*'s.
>> >
>> >
>> > Modified:
>> >     llvm/trunk/lib/TableGen/Record.cpp
>> >
>> > Modified: llvm/trunk/lib/TableGen/Record.cpp
>> > URL:
>> > http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/TableGen/Record.cpp
>> > ?rev=208293&r1=208292&r2=208293&view=diff
>> >
>> ===================================================================
>> ===
>> > ========
>> > --- llvm/trunk/lib/TableGen/Record.cpp (original)
>> > +++ llvm/trunk/lib/TableGen/Record.cpp Thu May  8 04:29:28 2014
>> > @@ -623,9 +623,8 @@ static void ProfileListInit(FoldingSetNo  ListInit
>> > *ListInit::get(ArrayRef<Init *> Range, RecTy *EltTy) {
>> >    typedef FoldingSet<ListInit> Pool;
>> >    static Pool ThePool;
>> > +  static std::vector<std::unique_ptr<ListInit>> TheActualPool;
>> >
>> > -  // Just use the FoldingSetNodeID to compute a hash.  Use a DenseMap
>> > -  // for actual storage.
>> >    FoldingSetNodeID ID;
>> >    ProfileListInit(ID, Range, EltTy);
>> >
>> > @@ -635,6 +634,7 @@ ListInit *ListInit::get(ArrayRef<Init *>
>> >
>> >    ListInit *I = new ListInit(Range, EltTy);
>> >    ThePool.InsertNode(I, IP);
>> > +  TheActualPool.push_back(std::unique_ptr<ListInit>(I));
>>
>> Prefer llvm::make_unique rather than raw new (makes it easier to ensure
>> ownership is handled everywhere):
>>
>> auto I = make_unique<ListInit>(Range, EltTy); ThePool.InsertNode(I.get(),
>> IP); TheActualPool.push_back(std::move(I));
>>
>> >    return I;
>> >  }
>> >
>> >
>> >
>> > _______________________________________________
>> > llvm-commits mailing list
>> > llvm-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits