 |
| |
|
|
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
|
|
Athens, Hellas
Jan 2000
time: 07:32
|
|
Fromafar, perhaps we should let memory leak fixes for a later patch as it can be pretty hard to test as well.
|
|
| |
|
|
|
Fromafar
|
|
quote:
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
|
 |
Berlin, Germany
Mar 2001
time: 06:32
|
|
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:
quote:
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.
|
|
| |
|
|
|
ctplinuxfan
|
|
Hi Martin,
quote:
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
code:
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
|
|
| |
|
|
|
ctplinuxfan
|
|
Hi,
quote:
Originally posted by Martin Gühmann
If I understand it correctly nulling s_checkBox[i] like here:
code:
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,
quote:
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.
quote:
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.
quote:
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.
quote:
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.
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.
quote:
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
|
|
Berlin, Germany
Mar 2001
time: 06:32
|
|
quote:
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:
code:
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
|
|
| |
|
|
|
ctplinuxfan
|
|
Hi Fromafar,
hi Martin,
quote:
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.
quote:
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
|
|
| |
|
|
|
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!
quote:
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.
|
|
| |
|
|
|
MrBaggins
|
|
The ANSI recommendation is to do it the 0 way.
quote:
Do not compare a pointer to NULL or assign NULL to a pointer; use 0 instead. |
|
|
| |
|
|
|
ctplinuxfan
|
|
Hi,
quote:
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
|
|
quote:
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,
quote:
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
|
|
| |
All times are GMT. The time now is 05:32.
Apolyton Time is 00:32. |
top of page
|
|
|
Forum Rules:
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts
|
HTML code is ON
vB code is ON
Smilies are ON
[IMG] code is ON
|
|
|
|
|
|
20-12-2003 20:29
|