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

[Martin Gühmann]

I recently noticed that Fromafar started to work on some memory leaks, so I wold like to know what rhules are there to deal with destructors and functions that should remove stuff from stuff from the memory. For example how I have to deal with stuff that was created with new or other functions, what is the best way to deal with pointers?

Another thing I like to ask is that I noticed that Fromafar uses this format to indicate original and new code:

#if defined(ACTIVISION_ORIGINAL)
         //Original code that is no longer used
#else
         //New code
#endif

In my opinion it is a very clean solution and therefore I started to use it as well, it also allows to build the original *.exe if you define ACTIVISION_ORIGINAL. Well I am not so familiar with this format so therefore I accidently used this format:

#if(ACTIVISION_ORIGINAL)
         //Original code that is no longer used
#else
         //New code
#endif

So is there a difference between thos two formats or are they identical, it works in VC++. So is there something with it? Well in the end I stick to Fromafar's format for coherence reasons.

-Martin

[Fromafar]

There is a subtle difference between #if defined(A) and #if A.

#if defined(A) will test whether A has been defined at all. It is identical to #ifdef A, but allows combination with logical operators as in #if defined(A) || defined(B).
#if (A) will test whether A has been defined and has a value other than 0.

So, if you add -DA=0 to the compile options, the first test will succeed (A has been defined), but the second one will fail (A is 0).

Dealing with heap memory and pointers is much more complex. The ground rule is that you should release the memory when there are no objects referring to it any more. Release it too soon, and you will end up with an illegal reference somewhere later (program crash). Do not release it on the last one, and you have no way to release it later (memory leak).

Unfortunately, it can be very hard to know what the last referring object is. And it gets even more complex when the objects are stored in containers like lists.

Furthermore, you should release the memory with the correct complementary function. Some commonly used pairs are:
delete - new
delete[] - new[]
free - malloc
But you may also have CTP2 specific ones like:
DeleteHierarchyFromRoot - BuildHierarchyFromRoot

[Keygen]

Fromafar, perhaps we should let memory leak fixes for a later patch as it can be pretty hard to test as well.

[Martin Gühmann]

Originally posted by Keygen
Fromafar, perhaps we should let memory leak fixes for a later patch as it can be pretty hard to test as well.

Yeah I tried to do something with the single player game player screen, and it made my game crash with some invalid references, so far I know if you open that screen and quit the game right afterwards you get a lot assertions if you don't of open it you don't get them on quit. But before you play with it let me redesign it first.

-Martin

[Fromafar]

Originally posted by Keygen
Fromafar, perhaps we should let memory leak fixes for a later patch as it can be pretty hard to test as well.

With the debug version, it is not too difficult. All memory leaks are collected in a file. Can't remember the exact name, but it is located in the same directory as the executable and has leaks and 9999 in the name.

The next 2 weeks, I will not have access to a compiler, so I will not add any new code. Maybe I do have some time to play(test), though.

If you are making a new release, could you include the map file? That would enable some useful reporting when something crashes. Otherwise, the generated crash.txt will only contain a list of addresses.

[Martin Gühmann]

What is needed to include the map file, is it just some kind of text file that needs to be placed at a certain place and then the game reports more or something else?

The leaks file is called CTP_LEAKS_99999.TXT, how do I read this file?

-Martin

[Fromafar]

The map file is a translation of code addresses to more readible text, i.e. function names.
It should have been generated in the same directory as the executable (maybe you have to set the 'generate map file' option during compilation): look for *.map files.

Originally posted by Martin Gühmann
The leaks file is called CTP_LEAKS_99999.TXT, how do I read this file?

With a text editor . It contains a stack dump of the creation point for every piece of memory that has not been released when leaving the program.

[Martin Gühmann]

Originally posted by Fromafar
With a text editor . It contains a stack dump of the creation point for every piece of memory that has not been released when leaving the program.

With a text editor is clear, my question was rather how to read the content. At least it is a little bit clearer if you disable the line warp in your text editor.

-Martin

[Martin Gühmann]

Since I don't get any asserts at terminating CTP2 and therfore some usefull pieces of information can be now found in the CTP_LEAKS_99999.TXT, I would now like to know how I interpred such a line:

1 160176 160176 0x00453aa1 [void * __cdecl DebugMemory_GuardedBlockAlloc(char const *,int,struct MemoryHeapDescriptor *,int,bool,unsigned char,bool)+0x121] / 0x00455394 [DebugMemory_GuardedMalloc+0x34] / 0x004556d2 [void * __cdecl operator new(unsigned int)+0x22] / 0x00a85156 [private: void __thiscall std::valarray::_Grow(unsigned int,double const *,unsigned int,bool)+0xa6] / 0x00a84fe0 [public: void __thiscall std::valarray::resize(unsigned int,double const &)+0x20] / 0x00a841e7 [public: void __thiscall MapGrid::Resize(long const &,long const &,long const &)+0xb7] / 0x00a831f3 [public: void __thiscall SettleMap::Initialize(void)+0x63] / 0x00a9600b [public: static void __cdecl CtpAi::Initialize(void)+0x1b] / 0x0060752e [long __cdecl gameinit_Initialize(long,long,class CivArchive &)+0x3a6e] / 0x004658e2 [public: long __thiscall CivApp::InitializeGame(class CivArchive &)+0x222] / 0x00468666 [public: long __thiscall CivApp::StartGame(void)+0x16] / 0x0046a2d4 [public: virtual void __cdecl StartGameAction::Execute(class aui_Control *,unsigned long,unsigned long)+0x14] / 0x007fd553 [public: void __thiscall aui_UI::HandleActions(void)+0x63] / 0x007fd4bc [public: virtual enum AUI_ERRCODE __thiscall aui_UI::Process(void)+0x4c] / 0x00467e83 [public: long __thiscall CivApp::ProcessUI(unsigned long,unsigned long &)+0x203] / 0x0046840e [public: long __thiscall CivApp::Process(void)+0xce] / 0x0045bf52 [int __stdcall CivWinMain(struct HINSTANCE__ *,struct HINSTANCE__ *,char *,int)+0x3d2] / 0x0045b582 [WinMain@16+0x42] / 0x00abf793 [WinMainCRTStartup+0x1b3] / 0xbff8b560 [(kernel)+0x0] / private: void __thiscall std::valarray::_Grow(unsigned int,double const *,unsigned int,bool)

-Martin

[Fromafar]

Ouch, you're going for a difficult one here, Martin!

You will probably have guessed the first three numbers already, but for completeness:

Num (1): number of occurrences of this particular leak
Size (160176): memory lost by a single occurrence of this leak
Total (160176): Num * Size

After this, you get a call stack of the memory creation point, contining a list of program execution points, separated by slashes (/).

Each program execution point is basically an address (0x00453aa1), but for us mere humans, it has been decoded between brackets ([ ... ]) into a function name (void * __cdecl DebugMemory_GuardedBlockAlloc(char const *,int,struct MemoryHeapDescriptor *,int,bool,unsigned char,bool)) and an offset from the beginning of the function (+0x121).

So in your example, the function DebugMemory_GuardedBlockAlloc created the memory, it was called by DebugMemory_GuardedMalloc, which in its turn was called by new, which was called by std::valarray::_Grow, which etc.., all the way up to the kernel.

There is one special - not part of the stack. The last one (std::valarray::_Grow) indicates the "lowest" program function in the stack that is not part of the debug memory handler in the stack. This will usually be the first function in the list that has called new, and is also the first function that is of interest to us, unless we would want to improve the debug memory handler.

In your specific case, the use of containers complicates matters. The _Grow and resize from std::valarray are - in our case Microsoft - compiler-provided library functions, so the first function in the Activision code is MapGrid::Resize.

Actually, the error may be in the Microsoft part. I noticed that all resize-functions for containers cause leak reports. But I have not verified yet whether this is caused by leaking, or by not reporting freed blocks to the debug memory handler.

[Martin Gühmann]

So that means we shouldn't bother with such thinks like this not too much at the start:

long * __cdecl std::_Allocate(int,long *)

There were a lot of them in the CTP_LEAKS_99999.TXT when I quit shortly after game lunch. I played a game for a few turns and got a CTP_LEAKS_99999.TXT of a size of 2.7 MB. So we have to take these memory leaks seriously especially, because some of them also cause assertions.

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?

-Martin

[Martin Gühmann]

OK, I looked through some files, I found two thinks:

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.

-Martin

[ctplinuxfan]

Hi Martin,

Originally posted by Martin Gühmann
OK, I looked through some files, I found two thinks:

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

i also have the opinion, that the code has leaks. However, by just reading it, i often find possible points where memory isn't freed, and when analyzing it, it gets freed anyway.

The point is, that there seems not only to be leaking code, but you'll also find some references whose deallocation is hard to locate.

(An easy example, around classes in the same file: gfx/spritesys/director.cpp, Directors::ActionFinished())

I wondered, why the heck there is code after a Release in the second condition, i.e. thought of a possible memory protection fault. I looked at class Sequence and saw it has a reference counter, but does not use it for deallocation. Finally, i saw that the sequence gets deleted within class DQItem.

Your example also is like this: The GameEventHookNode gets added to a PointerList within class GameEventHook and hence a reference is held to that added object. Well, some people like containers deleting everything when they get deleted, but we need to know...

Well, conclusion: We should define a standard how to handle mm (i thought of the following basic idea, any replacement suggestion/further ideas welcome):

Define a self deleting root class CTP2Object, like this

class CTP2Object {
private:
  bool m_isAllocated;
  long m_RefCount;

public:
  CTP2Object(bool isAllocated = true)
    : m_isAllocated(isAllocated), m_RefCount(0) {
  }
protected:
  virtual ~CTP2Object() {};

private:
  virtual void destroy() {
    if (m_isAllocated) {
      delete(this);
    }
  }
public:
  virtual void Acquire() {
    m_RefCount++;
  }
  virtual void Release() {
    if (0 == --m_RefCount) {
      destroy();
    } else if (m_RefCount < 0) {
      destroy();
    }
  }
  friend void Release();
};

Not allocated instances of a class (objects) will use false as constructor parameter, though it would be nice if this will be omitted (for portability).

Based on a class rules like this could be defined
* Each Acquire() must have exactly one corresponding Release();
no more, no less
* If an Object holds a reference, that object must ensure that it Acquires on getting reference and Releases latest in destructor.
* If within a temporary context, Acquire() and Release must be
called prior working with reference and after having finished
work
* If any reference to an object is held, release may not be deactivated behaving so (contra-example: Sequence in directors.cpp)
* If references get stored into containers, upon destruction of container each element has to be released. If we pass the container around (which i don't hope), we could implement a container which hold objects of type CTP2Object and calls release on each object when his last release is called (i.e. within the containers' own destructor).
* When passing containers around, they need to be aquired before giving them as a parameter and must be released when the receiver of the message is done with working on that object. xor when the receiver gets destroyed

O.k., perhaps a rule for memory management sounds a little bit silly, but i am thinking of a way to remove COM (i.e. replacing e.g. IUnknown) for the linux port. So i have to consider memory management for this anyway, because c++ code with pointers to instances of classes is more portable than using objects (if thats portable at all).

Ciao
Holger

[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

[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