Memory leaks revisited (and many fixed)

Hi,

I finally took the time to track down the remaining memory leaks beside the LeakDetector objects and TypeMaps/ValueMaps I already knew about. It turns out almost all of them came from the command line options module, so I cleaned up the patch for LeakDetector and made a new patch for CommandLine. The patches are attached to this message.

That got rid of all but 4 leaks I'm getting, but of course there are more of the same waiting to happen - the pattern of creating singletons with new instead of having static objects inside accessor functions is very common in LLVM. The four leaks are caused by the ConstantBool::True and ConstantBool::False (1 for each object + 1 for the use list dummy) - unfortunately they are not so easy to wrap with accessor functions since they are public member variables of the ConstantBool class... If only everyone used the ConstantBool::get() there would be no problem, but that's not the case. Any suggestions?

m.

leaks.patch (6.11 KB)

I finally took the time to track down the remaining memory leaks beside
the LeakDetector objects and TypeMaps/ValueMaps I already knew about. It
turns out almost all of them came from the command line options module,
so I cleaned up the patch for LeakDetector and made a new patch for
CommandLine. The patches are attached to this message.

Looks great, applied:
http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20041115/020970.html
http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20041115/020971.html

That got rid of all but 4 leaks I'm getting, but of course there are
more of the same waiting to happen - the pattern of creating singletons
with new instead of having static objects inside accessor functions is
very common in LLVM.

Yes it is. I'm still mostly of the opinion that memory live at exit
shouldn't have to be free'd, but I can appreciate tidiness :slight_smile:

The four leaks are caused by the ConstantBool::True
and ConstantBool::False (1 for each object + 1 for the use list dummy) -
  unfortunately they are not so easy to wrap with accessor functions
since they are public member variables of the ConstantBool class... If
only everyone used the ConstantBool::get() there would be no problem,
but that's not the case. Any suggestions?

These should just be destroyed along with the rest of the constants, by
Constant::clearAllValueMaps.

-Chris

Chris Lattner wrote:

The four leaks are caused by the ConstantBool::True
and ConstantBool::False (1 for each object + 1 for the use list dummy) -
unfortunately they are not so easy to wrap with accessor functions
since they are public member variables of the ConstantBool class... If
only everyone used the ConstantBool::get() there would be no problem,
but that's not the case. Any suggestions?

These should just be destroyed along with the rest of the constants, by
Constant::clearAllValueMaps.

Hmmm, I was planning to call clearAllValueMaps once in a while to clear out the float constants which are building up on me... This shouldn't break anything as it stands, since the constants will be regenerated. However, deleting True and False will cause big problems. I think I'm just going to delete True and False directly from our own shutdown code. That way the leak detector won't complain and I can use clearAllValueMaps as I planned...

m.

Morten Ofstad wrote:
} Chris Lattner wrote:
} >>The four leaks are caused by the ConstantBool::True
} >>and ConstantBool::False (1 for each object + 1 for the use list dummy) -
} >> unfortunately they are not so easy to wrap with accessor functions
} >>since they are public member variables of the ConstantBool class... If
} >>only everyone used the ConstantBool::get() there would be no problem,
} >>but that's not the case. Any suggestions?
} >
} >These should just be destroyed along with the rest of the constants, by
} >Constant::clearAllValueMaps.
}
} Hmmm, I was planning to call clearAllValueMaps once in a while to clear
} out the float constants which are building up on me... This shouldn't
} break anything as it stands, since the constants will be regenerated.
} However, deleting True and False will cause big problems. I think I'm
} just going to delete True and False directly from our own shutdown code.
} That way the leak detector won't complain and I can use
} clearAllValueMaps as I planned...
}
Would it work better just to replace all direct references to
"ConstantBool::{True,False}" with the appropriate
"ConstantBool::get({true,false})" and then remove public access to the
variables?

-bw

Bill Wendling wrote:

} Hmmm, I was planning to call clearAllValueMaps once in a while to clear } out the float constants which are building up on me... This shouldn't } break anything as it stands, since the constants will be regenerated. } However, deleting True and False will cause big problems. I think I'm } just going to delete True and False directly from our own shutdown code. } That way the leak detector won't complain and I can use } clearAllValueMaps as I planned...
} Would it work better just to replace all direct references to
"ConstantBool::{True,False}" with the appropriate
"ConstantBool::get({true,false})" and then remove public access to the
variables?

Yes, this would be the proper way to do it. It would also help prevent problems related to static initializer order. But I searched the code and there are a lot of direct references - it seems it's not worth the trouble. At least for me it's not necessary, since I managed to make a hack to shut up the leak detector anyway...

m.