From: Andrew R. <an...@co...> - 2004-01-16 23:32:25
|
On Thu, Jan 15, 2004 at 11:12:00AM -0800, Alan Irwin wrote: > > There are two aspects to this patch. > > ** There is the plend change rezeroing lib_initialized which allows a user > to reinitialize the library. It makes sense to me to allow the user to > start fresh again like this. However, Andrew, you should be aware that > there are memory management issues still in the dynamic drivers code. > (Memory keeps being used up by malloc and never freed, and in one case freed > memory is accessed, if I recall correctly the detailed leak-check analysis > that is available from valgrind.) So far we have got away with not dealing > with these problems, but they may come back to haunt us when you allow the > user to re-initialize the library many times. The unfreed memory will just > keep increasing each time you do this ==> memory leak, and at some point the > access to memory which has already been freed will kill us. So that is a > downside, but I am willing to accept this additional risk for now since I > don't understand the dynamic drivers code well enough to deal with the > memory management issues there. > > ** Initializing npldynamicdevices and nloadabledrivers. > > Andrew, can you assure us you understand that dynamic drivers code well > enough to be sure this is the right thing to do? (I believe this was the > question Rafael was asking.) If so, then I think we should try your patch > as is to see if there is no obvious breakage in all our standard tests. I would not claim to be an expert on the dynamic drivers code but it is pretty clear to me on looking through the code that these variables are only used to count how many dynamic drivers and how many loadable drivers we have as we iterate through the list. Obviously if we intialise the library for a second time these variables need to be reset or we end up thinking we have too many drivers and the library crashes when we try to access these bogus elements in the driver list. I can categorically say that this patch will have _no_ effect unless dyndrivers are included _and_ the user tries to re-initialise the library so it won't break anything that currently works. > But that brings up an additional (post-release?) issue: > > If you do understand that dynamic drivers code well enough to keep track of > initialization, then I hope you would be willing to also look at the memory > management issues as well. In particular, the goal should be after plend is > executed, there is absolutely no malloc memory left to be freed. I highly > recommend using valgrind on the installed examples to help diagnose exactly > where the memory management problems occur. (Note, valgrind does not work > on examples built in the build tree with "make check" since those are > implemented with scripts.) There are undoubtedly some memory issues here. The fact that my code used to work with static drivers suggests that some things may not be freed by plend. I've already been using valgrind to track bugs in my own code so I'm quite happy to look at plplot a bit more closely as well. I'll report back to the list. Andrew |