[LLVMdev] LLVM and memory leaks

[Chris Lattner]

On Mon, 15 Nov 2004, Morten Ofstad wrote:
> > If this is a serious issue, it would be straight-forward to add a global
> > function to flush out unused constants.  I would be happy to accept a
> > patch to do that.
>
> Here is an implementation of this -- there are some things you might not
> like about it:
>
> 1) I had to make the private destroyConstantImpl method public to access
> it from the template class ValueMap since there is no good way to
> declare it as a friend.

That's fine in principle, though if you make the clearAllValueMaps
function be a static method in Type this is unneeded I think.

> 2) I keep the types in a std::vector so I can easily drop all references
> and delete them without going through all the type maps - this adds a
> little bit of overhead, but there shouldn't be that many types (compared
> to constants), so to me it seems acceptable. This also means I can get
> by with a 'friend' declaration to get at the private data of the types
> when I want to forcefully drop references.

Yeah, that's kinda gross.  :(

> I dropped the memory pool thing because the use lists always 'new' one
> element, so I could not avoid calling the destructors of the constants
> even if they have no (real) uses...

Ok, here's some feedback.  This patch looks like a combination of several
different things.  If you resubmit the patch in pieces, we can get the
undebatable ones applied first:

1. Some changes of unsigned to size_t and other things to make VC happy.
   These are fine.
2. The clearAllValueMaps related changes.  These are also fine, but please
   make clearAllValueMaps be a static function in the Constant class,
   Constant::ClearAllValueMaps().
3. The LeakDetector changes: These are not ok, because the order of
   construction of the static objects is not defined (these maps may be
   initialized after other things that put stuff into them).

   If you change the static objects in getObjects() and getLLVMObjects()
   to be LLVMObjects themselves instead of pointers, I think everything
   should work fine (they will be initialized on first use that way).  In
   other words, getObjects becomes:

   Objects &getObjects() {
     static Objects Objs;
     return Objs;
   }
4. The Types change, including the vector.  As you guessed, I'm not a fan
   of this at all: it adds overhead to the normal case.  Don't the *Types
   maps contain all of the information that you need to do the clearing?
   In clearAllTypeMaps (which should become a static method in Type,
   allowing you to avoid the friend issues), as a first pass, can't you
   loop over FunctionTypes, PointerTypes, etc and build the vector there?

As a general note, you don't need to use '(void)' as arguments in C++,
this is a C thing.  Please just use '()'.

-Chris

-- 
http://llvm.org/
http://nondot.org/sabre/