PROJECT: How to deal with memory leaks and obsolete code?

[ctplinuxfan]

Hi,

Originally posted by Martin Gühmann
If I understand it correctly nulling s_checkBox[i] like here:

		for (sint32 i = 0; i < k_NUM_MAPSHAPEBOXES; i++ ) 
		{
			delete s_checkBox[i];
			// NULLing unnecessary: deleting the container next
		}
		delete [] s_checkBox;
		s_checkBox = NULL;

Would give me a list of null pointers that are unrefferenced afterwards I delete s_checkBox itsself.
-Martin

No, before deletion of the array, you have an array of possibly invalid references.

As long as a reference to a reference of an element of s_checkbox is not stored somewhere else, this is no problem and i assume that is why the comment exists.

If somewhere else just a copy of a reference is stored, you'd have no influence on it anyway, because by setting a pointer to null the copy of that copy will not become null.

Ciao
Holger

[ctplinuxfan]

Hi Martin,

Originally posted by Martin Gühmann
ctplinuxfan, can you guess how much efforts are needed to rewrite the whole system so that we can your system?

On another look it is amazing (or not) how huge the CTP_LEAKS_99999.TXT gets if you just open the ranking tab, I just duplicated some code in it, so that can't be the cause, a file of 2.3 MB and some 3 asserts.

-Martin

I suppose this is a major task not suitable for coming into the first patch, perhaps with a timeframe until end of april, if the time we have left for it isn't cut to much.
The problem is not replacing IUnknown derivated classes to instantiate from CTP2Object, references to these components get Acquire()'d and Release()'d anyway, so that would nearly need no work, except for bug hunting...
Most certainly, main work is determined by reimplementing interface definitions to classes or abstract classes and making that code work.

In a second stage - as a aid restricted to debugging - we could implement a kind of garbage collector, either removing forgotten CTP2Objects on termination and possibly throwing Assertions for each forgotten object; just dumping ptr and refcount and not avoid leaks (i.e. free unfreed memory on termination) for use with memory debuggers, etc.

To prevent misuse: If such a class would be implemented, it's not for getting rid of memory leaks in one blast, but help concentrating on other bugs first or determine, how much Release() calls have been forgotten, and avoid destabilizing system when working on changes. Everything else will be told by a memory debugger anyway (e.g. electric fence/valgrind on linux).

That's also easy by registering at gc within constructor of CTP2Object and deregistering when destructor gets called.

Ciao
Holger

[Fromafar]

I guess I have to login more often. There sure are a lot of new questions here. Well, let's have a go at it.

So that means we shouldn't bother with such thinks like this not too much at the start:
long * __cdecl std::_Allocate(int,long *)

Probably you are right. The size of the problem is determined by the number of occurrences mainly. There are lots of items that by laziness - or design - are only created once, and are never released. But this will not have impact on the behaviour of the program at all. This holds e.g. for things like the main map, the tribes and relations between them, etc. Only loading a - differently sized - game will change these items ever during the course of the game, but that will probably be handled in the load (delete 1 + create 1 new).
So, the most interesting reports to solve are those with Num not being equal to 1 or the number of players in the game (or some other fixed array constant). But causing assertions is also a good indication that it is a real bug.

Another question are these functions that you can find at the end of each line the function were something is not deleted properly or the place were something is initialized that wasn't deleted at the end of the program but must/can be deleted somewhere else to free the memory?

The latter. All reports are places where something was initialised. It is up to us to find out whether it is really necessary to delete it properly before the end of the program (a bug), or we can get away with being lazy.

1. Empty destructors
2. Something like this:
m_callbacks->AddTail(new GameEventHookNode(cb, pri));

Just the creation of a new object but it was not assigned, I already wrote a program that caused trouble, because of this, y solution was to declare a temporary variable explicity, I think as field of an object.

This is not necessarily an error. But it is definitely a spot to look at carefully. Usually, by design, you define a specific object to be the owner of the allocated memory.

If this is a simple member (pointer) variable, it makes sense to assign it to the variable at the call of new, and delete the object when you don't need it anymore, assigning NULL to the variable after the delete. As a safeguard/good programming practice, I would encourage adding a delete of the member variable in the destructor. When the object goes out of scope, its member variables are inaccessible, so if there was still memory allocated, it is leaked. Note that delete of a NULL-assigned member does nothing by definition, and will not harm at all, so testing for non-NULL is unnecessary in the destructor. And setting the member variable to NULL in the destructor is also unnecessary, because the whole object is destroyed and can not be referenced to any more.

If you design the owner to be a container (e.g. list), it is perfectly sound to not assign it directly, but use an insert (e.g . AddTail) operation of the container. In this case, "the right thing" to do is to clear the container in the destructor (using an iterator/loop, or something like a DeleteAll operation). This kind of handling is particularly useful for things where the list handler is not the creating function, so e.g. for (SLIC) commands, game events, Director events, messages to the user. The inserted object will then get deleted either after handling it, or in the destructor of the handler.

reference counter

A reference counter will not help you to prevent leaks at all. If you have forgotten to delete a simple (non-reference-counted) new, you will forget to Release an Acquired reference counted object as well. Actually, you have even more chances of leaking.
Reference counters are useful when you do not have a clear single owner by design, but rather have multiple owners, and want each owner to be able to access the object still after some other owners have released it. In this case you either have to give each owner its own copy (requiring lots of memory and synchronisation to make sure the copies remain consistent), or share the copies with a reference counter, so that the object does not really get deleted until the last owner releases it.
It is definitely a valid design concept, but it is just one step more difficult.

before deletion of the array, you have an array of possibly invalid references.

Quite right, and not only possibly. Any references like (*s_checkBox[0]) in or after the loop are invalid. But after the line delete [] s_checkBox, even the reference to s_checkBox[0] (without the * to derefence) has become invalid. So, adding s_checkBox[i] = NULL in the loop will just write in some soon-to-become-invalid memory, and is unnecessary/wastes time. It is similar to the i = 0 statement in a block of code like
{
int i;
[normal use of i]
i = 0;
}

If there would have been a reference somewhere else, it would become illegal, regardless of the presence or absence of the NULL-assignment in the loop. And you might have to use reference-counting to avoid that

[Martin Gühmann]

Originally posted by Fromafar
Quite right, and not only possibly. Any references like (*s_checkBox[0]) in or after the loop are invalid. But after the line delete [] s_checkBox, even the reference to s_checkBox[0] (without the * to derefence) has become invalid. So, adding s_checkBox[i] = NULL in the loop will just write in some soon-to-become-invalid memory, and is unnecessary/wastes time. It is similar to the i = 0 statement in a block of code like
{
int i;
[normal use of i]
i = 0;
}

If there would have been a reference somewhere else, it would become illegal, regardless of the presence or absence of the NULL-assignment in the loop. And you might have to use reference-counting to avoid that

If it just a waste of time then I don't understand why it is reported if it does only to waste time, it must also cause a memory leak.

I look a little bit further thourgh the CTP_LEAKS_99999.TXT looks like that these are the important ones:

LineGraph::SetLineData(long,long,double * *,long *)
SlicSymbolData::SetString(char *)
SlicSymbolData::SetValueFromStackValue(enum SS_TYPE,union SlicStackValue)
PointerList::AddTail(class SlicSymbolData *)
StrategyRecord::operator=(class StrategyRecord const &)
DynamicArray::DynamicArray(void)
tech_WLList::tech_WLList(unsigned long)
tech_Memory::Link>::Block::Block(unsigned long)

My CTP_LEAKS_99999.TXT is filled of these ones of course you have to use the power graph, and it looks like each time you use it, more memory gets lost. So far I was able to fix something in greatlibarywindow.cpp and in SlicStruct.cpp. In addition a lot of aui_* stuff causes leaks.

-Martin

[Fromafar]

Originally posted by Martin Gühmann
If it just a waste of time then I don't understand why it is reported if it does only to waste time, it must also cause a memory leak.

Neither do I. But the problem must be somewhere else, and not in this part. There can be a subtle difference between adding and not adding the NULL-assignment, but this will be caused by bad code elsewhere.

Using the simple example I gave in my earlier post: suppose you add

char * p;
*p = 'A'; // Bad code: p has not been initialised.

after it, and your friendly compiler decides to reuse the memory location of i for p. Now if you have added i = 0, your program will crash always (derefencing NULL). If you did not add i = 0, your program may just happily write 'A' to some location (dependent on the last value of i), and proceed seemingly undisturbed.

Would you say that adding i = 0 causes a bug?

[Martin Gühmann]

Originally posted by Fromafar
Would you say that adding i = 0 causes a bug?

Maybe it does not cause the bug but it makes the bug appear.

One note about your "harmless" c++ example, you could imagine compilers that could do some stuff with the i or do not some stuff with the i. So let me port this example to slic:

HandleEvent(BeginTurn)'SomeEvent'post{
      int_t i;
      i = 0;
      //Do some stuff
      //At the end this might be true: i == 100
}

The i is initialized in this event handler, according the slic documentation integers are initialized with 0. So the i = 0; seems to be a waste of time. But in fact the i is just initialized once per slic reload, that means afterwards the the event handler is closed i keeps its value. And has the same value when the event handler is executed again. So i is 100 the next time and not 0 as you might expect. You can observe the same behaviour for slic arrays, they keep their size and their values as well.

Well of course this is a bug, you can even see it in the memory leak report, but this is something you as slicer don't have controll about it. So for us this means that we maybe only have at that place where we fixed the stuff have the controll.

-Martin

[ctplinuxfan]

Hi Fromafar,
hi Martin,

Originally posted by Fromafar
A reference counter will not help you to prevent leaks at all.

Sure. We have reference counters in the code, and have memory leaks.
I wanted to point out that we need a memory management standard, perhaps i was unclear. The solution i thought of is killing two birds with one stone: clear mm scheme and getting rid of com stuff, still allowing n:m relationships between references and holders of references.

Originally posted by Martin Gühmann
So the i = 0; seems to be a waste of time. But in fact the i is just initialized once per slic reload, that means afterwards the the event handler is closed i keeps its value.
-Martin

Well, from a programmers point of view i would share your opinion. On the other hand, slic is also meant to be a language for writing scenarios. There, you might need variables which keep their value (e.g. a scenario which can only be won if you beat all other civs after a certain number of years = turns, produce a defined amount of units and defending your cities while doing so, etc.). On the other hand, implementing global variables is also easier than binding variables to contexts they appear at, i.e. make them local.
That's why i think variables are global and persistent in slic (see slic doc). We'd break compability if we'd remove that.

Ciao
Holger

[Martin Gühmann]

Originally posted by ctplinuxfan
Well, from a programmers point of view i would share your opinion. On the other hand, slic is also meant to be a language for writing scenarios. There, you might need variables which keep their value (e.g. a scenario which can only be won if you beat all other civs after a certain number of years = turns, produce a defined amount of units and defending your cities while doing so, etc.). On the other hand, implementing global variables is also easier than binding variables to contexts they appear at, i.e. make them local.
That's why i think variables are global and persistent in slic (see slic doc). We'd break compability if we'd remove that.

Well at least they could stated it a little bit more clearly so that I hadn't spend ages to figure out that if I leave a scope that next time when I enter the scope the variable is still there with the same value. Well actual the whole slic documentation needs a rework for non programmers.

-Martin

[Fromafar]

Ah, this is nasty! I would say that the SLIC documentation needs a rework for programmers in particular. As the documentation only talks about the scope and the 0-initialisation, and says nothing about lifetime/persistency, I have more or less automatically assumed it would have C style semantics.

And now you are telling me that SLIC code
{
int_t i;
//Do some stuff
}

does not correspond to
{
int i = 0;
//Do some stuff
}

but rather to
{
static int i = 0;
//Do some stuff
}

Good to know for the next time I start messing around in SLIC files!

Originally posted by ctplinuxfan
I wanted to point out that we need a memory management standard, perhaps i was unclear. The solution i thought of is killing two birds with one stone: clear mm scheme and getting rid of com stuff, still allowing n:m relationships between references and holders of references.

I understand that we have to get rid of the COM stuff if we want to port the code to other platforms. For the memory management OTOH, I would like to keep things as simple as possible. So I would prefer designing objects to have a single owner whenever possible, rather than introducing reference counting for every object. For the cases that do require multiple owners, defining a standard way of handling would be best. Either something like your proposal, or something like a boost::shared_ptr could do the trick.

[Martin Gühmann]

Here is an example from tech_memory.h:

		virtual ~Block()
		{
			if ( used )
			{
				delete[ usedSize ] used;
		//		used = 0;
			}

			if ( data )
			{
				delete[ dataSize ] data;
		//		data = 0;
			}
		}

What I like to know is when you should use delete[ usedSize ] used; instead of delete[] used; or what is the difference.

Another thing I wonder about is why they nulled the two pointers here with the integer 0 instead of the NULL pointer, or are in that case NULL and 0 identic?

-Martin

[MrBaggins]

The ANSI recommendation is to do it the 0 way.

Do not compare a pointer to NULL or assign NULL to a pointer; use 0 instead.

[ctplinuxfan]

Hi,

Originally posted by Martin Gühmann
Another thing I wonder about is why they nulled the two pointers here with the integer 0 instead of the NULL pointer, or are in that case NULL and 0 identic?
-Martin

NULL is c-related, 0 is ansi c++.
Well, NULL is defined as 0 when using in c++ code, but in c NULL can be something like (void *) 0, (__malloc_ptr_t) 0, etc.
So better use 0 instead.

Ciao
Holger

[Fromafar]

Originally posted by Martin Gühmann
What I like to know is when you should use delete[ usedSize ] used; instead of delete[] used; or what is the difference.

Another thing I wonder about is why they nulled the two pointers here with the integer 0 instead of the NULL pointer, or are in that case NULL and 0 identic?

delete [usedSize] is very old-fashioned. If you use a modern compiler, you will most likely get a warning about using an anachronistic feature. As long as usedSize is the whole array size, it should do the same as delete[], which is the only official version to clean up the memory of an array. I am not sure what should happen when usedSize is smaller (or larger?) than the allocated array size. Anyway: better replace it with delete [], if only to save the GNU guys from drowning in warnings.

NULL is something similar, though not that old - and you will not get compiler warnings when using it. In the ANSI C++ definition (first edition of 1998, which probably was the one in effect when the VC++ 6.0 compiler was designed) it has been defined as a macro that will give you an implementation-dependent null-pointer.
But, as ctplinuxfan and MrBaggins remarked, a later ANSI recommendation is to use 0. Old habits are slow to die, though.

[ctplinuxfan]

Hi,

Originally posted by Fromafar
For the memory management OTOH, I would like to keep things as simple as possible. So I would prefer designing objects to have a single owner whenever possible, rather than introducing reference counting for every object. For the cases that do require multiple owners, defining a standard way of handling would be best. Either something like your proposal, or something like a boost::shared_ptr could do the trick.

My model does not work when using pointers to superclasses as a passing argument, and AddRef() / Release() on that superclass. Then, the destructor chain beginning at that of the superclass is started, instead of beginning at the class.
I noticed that, when i finished implementing the plugin mechanism, returning a pointer to CTP2Plugin * for plugins (deriving from CTP2Plugin) for the load mechanism.

For objects having multiple owners, i use the same interfaced concept as we currently have (deriving from IC2Interface instead of IUnknown, because IUnknown will be linked to the windows version for DirectX support used by SDL).

Theoretically, smart pointers can also be used by wrapping them around a IC2Interface derived class. So a template class will do the AddRef() / Release() stuff. However, the AddRef() calls by QueryInterface() / createInstance() would have to be removed or compensated, for that (i.e. each instance starts with a 0 reference counter). Atm., my started com reimplementation uses the same allocation scheme like com (each createInstance(), queryInterface(), and AddRef() of an object has to be followed by the same amount of Release() calls on that object).

Ciao
Holger

[ctplinuxfan]

Hi,

a short note: The COM replacement finally is implemented. It is not as bloated as com, thus only supports in-process instance creation and loading components on the fly using shared libraries.

Further information in thread: COMPILE: Linux port

The networked features would be compiler specific anyway (mainly creating stub proxy objects during runtime (i.e. reconstructuring vtable the compiler uses), implementing bridges that support calls from one side to the other using assembler for stack initialization and jumps to the corresponding vtable entry, an idl, etc... Hey, i want to port and play ).

Ciao
Holger