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

[Daniel Sanders]

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

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