Skip to main content
gamedev.net
🔒 Locked

C++ Tips on debugging memory corruption

Started by Wavesonics Apr 9, 2007 at 11:12 PM 25 replies 3.2k views
Original Post
Wavesonics
Wavesonics
I have a very bad bug that I'm almost certain is stemming from memory corruption. I run a whole bunch of delete and delete [] and then some time very shortly afterwards the whole program crashes at a random point. Thats the tell tale signs of memory corruption as far as I know. So I was wondering if any one had any good tips on trying to locate or detect these types of bugs. Anything at all, I've tried a lot of things but maybe theres some basic technique people use that I don't know so please, anything no matter how simple tell me how you guys usually track these bugs.
Replicon
Replicon
Is it always the same 'random' point, or always different?

Zahlman
Zahlman
I (and many other people who know what they're doing) generally sidestep this by using the "basic technique" of RAII to avoid creating the problems in the first place. Show the code, and I'll try to show how these things can be fixed.
Byron
Byron
Quote:
Original post by Wavesonics
I have a very bad bug that I'm almost certain is stemming from memory corruption.

I run a whole bunch of delete and delete [] and then some time very shortly afterwards the whole program crashes at a random point. Thats the tell tale signs of memory corruption as far as I know.

So I was wondering if any one had any good tips on trying to locate or detect these types of bugs. Anything at all, I've tried a lot of things but maybe theres some basic technique people use that I don't know so please, anything no matter how simple tell me how you guys usually track these bugs.


It really depends on what system you are using to develop your code on. If it is Visual Studio then it has some pretty good CRT debug memory tools that you can enable - they will slow your code down a lot but it can do heap checks to make sure that any allocation/deallocation hasn't blown the heap and it also puts guard bands around allocations to make sure that you haven't walked off the end of an array and so on.

On top of that I also use a memory manager by Paul Nettle:

http://www.flipcode.org/cgi-bin/fcarticles.cgi?show=64444

to track all allocations and deallocations - its pretty good at what it does.

And finally, for the extreme bugs (i.e. those that are really hard to find) I start using PCLint... but I have to be desperate to get to this point since running PCLint on your code is like having the harshest critic look at your code and find nothing good about it.
ByronBoxes
Wavesonics
Wavesonics
Awesome, ya I had to go through a bit to find that memory manager, but I finially got it.

I've never used MSVC++ but it seems like there is quite a bit of code that is specific to the MS compiler.

But uh... wow i sound like a n00b. guess i am... How do you use it?
Wavesonics
Wavesonics
So I had an idea, I decided to override the global new and delete operators to keep a count, then at certain times print out the total:

// ===============================================// I put this in a header which all files include:// ===============================================#ifndef MEMREPORT#define MEMREPORT #include <new>#include <cstdlib> extern int32_t *g_iNew;extern int32_t *g_iDelete; extern void* operator new ( size_t size ); extern void operator delete (void *p); extern void memReport(); #endif // =================================// Then I define them in my main.cpp// in the global scope// ================================= int32_t *g_iNew;int32_t *g_iDelete; void* operator new ( size_t size ) {    ++(*g_iNew);     void *p = malloc( size );    return p;} void operator delete (void *p) {    ++(*g_iDelete);    free( p );}  // And finial I init them in mainint main(int argc, char *argv[]) {    g_iNew = (int32_t*)malloc( sizeof(int32_t) );    (*g_iNew) = 0;    g_iDelete = (int32_t*)malloc( sizeof(int32_t) );    (*g_iDelete) = 0; ... }


But the reports are producing what seem to be none sense results.

They are in the hundreds of thousands, and TOTALLY inconsistent, every run they are different. I print the memory report before a delete and immediately after and the number has changed by many hundreds and even new has changed. So clearly my code is ef-ed.

Can any of you see a problem with it?

[edit]
These are what the results are right now when they should at the most be around 100 News:

Heap Memory Report:
Total New: 23235
Total Del: 1094
[/edit]
Republicanist
Republicanist
If your trying to keep track of allocations and deallocations, you should also define new[] and delete[]. Why did you make g_iNew and g_iDelete pointers, anyway? A simple "int g_iNew = 0;" should work better (and they don't need to be known outside main.cpp - what's the code for memReport()?). If your trying to cut memory corruption, a good way to see if its happening is at the end of main(), g_iNew should equal g_iDelete. If it does not, than they are news that did not recieve a delete, or vice-versa.

And another thing: freeing already-freed data tends not to work.
Smart pointers, if you haven't heard of them, tend to work very well in situations like these.

Make sure every new() has a delete().

Is the program trying to access freed-data? Or null or dangling pointers?

I wonder if initializing them in main is causing havoc - or if they are being used before main() activates.
Wavesonics
Wavesonics
Ya idk, i had them declared like that originally, but when it wasn't working i tried pointers wondering if it might affect the linkage.

I reverted it back now, but still get the same none sense inconsistent results.

But your very correct I need to over ride new [] and delete [] as well. Still though, not even my new and delete over rides are working how I would think they should at the moment.
mattnewport
mattnewport
Memory breakpoints are useful for tracking down memory corruption problems. In VC++ you create a data breakpoint and have it fire if the value at a memory address that appears to be getting corrupted changes. You don't say what debugger you're using but I imagine most debuggers on the PC should have a similar facility.
jkleinecke
jkleinecke
I've had quite a few circumstances where bugs like this rear their ugly heads (through maintaining someone else's code).

While the above posters are correct that there are a plethora of tools available for helping to detect these errors, nothing is fool-proof. I've had a lot of experience trying to get a tool to work and it just ends up costing me more time.

The best thing to do (especially since you don't seem to be using VS) is to comment out all but one section of code. Verify that the problem doesn't exist anymore (if it does then you've narrowed it down to the stuff that isn't commented), then uncomment your code one piece at a time until the bug shows up. You can usually use this technique to narrow the culprit down to one allocation or deallocation (or a copy somewhere).

Hope this helps...
-----------------------------------Indium Studios, Inc.
Wavesonics
Wavesonics
thanks for all your posts.

Ya i've never had a bug like this before in my 5 years of programing. I tend to code well enough this sort of thing just doesn't crop up.

But the system is at a point of interwoven complexity that it is difficult to comment out whole sections and still remain testable.

My current install of GCC for some god only knows reason didn't come with GDB so i need to get that and try some break points. I'm going to start to take some more drastic measures in creating a "clean" environment to test each little piece in. It will be time consuming but it is seeming like the only option at this point.
Zahlman
Zahlman
Seriously, show the code (a relevant selection, if you can manage to boil the problem down by trial-and-error removal of unimportant stuff).
Wavesonics
Wavesonics
Ok, here is the code used to destroy a currently loaded level. This is where the trouble starts.
void BSPManagerService::Destroy(){    LOG->writeMsg( Logger::LVL_DEBUG, "Deleting BSP" );    m_bLoaded = false;    display( false );	// If we still have valid memory for our nodes, free them	if(nodes)  {		delete [] nodes;		nodes = NULL;		numNodes = 0;		LOG->writeMsg( Logger::LVL_DEBUG, "Nodes deleted" );	}	// If we still have valid memory for our leaves, free them	if(leaves)  {		string strLeafId = EMPTY_STRING;        for( int16_t i = 0; i < numLeaves; ++i ) {            strLeafId = BSP_LEAF_NODE_ID_BASE;            stringstream strBuf;            strBuf << i;            strLeafId += strBuf.str();            Leaf *ptrLeaf = dynamic_cast<Leaf*>( findElementById( strLeafId ) );            if( ptrLeaf != NULL )                delete ptrLeaf;            else                LOG->writeMsg( Logger::LVL_ERROR, "Tried to delete leaf but could not find: " + strLeafId );        }        numLeaves = 0;        delete [] leaves;        leaves = NULL;        delete [] visList;        visList = NULL;        LOG->writeMsg( Logger::LVL_DEBUG, "Leaves deleted" );	}		// If we still have valid memory for our markSurfaces, free them	if(markSurfaces)  {		delete [] markSurfaces;		markSurfaces = NULL;		numMarkSurfaces = 0;		LOG->writeMsg( Logger::LVL_DEBUG, "Mark Surfaces deleted" );	}	// If we still have valid memory for our faces, free them	if(faces) {		delete [] faces;		faces = NULL;		numFaces = 0;		LOG->writeMsg( Logger::LVL_DEBUG, "Faces deleted" );	}	// If we still have valid memory for our surfedges, free them	if(surfedges)  {		delete [] surfedges;		surfedges = NULL;		numSurfedges = 0;		LOG->writeMsg( Logger::LVL_DEBUG, "Surface Edges deleted" );	}	// If we still have valid memory for our edges, free them	if(edges)  {		delete [] edges;		edges = NULL;		numEdges = 0;		LOG->writeMsg( Logger::LVL_DEBUG, "Edges deleted" );	}	// If we still have valid memory for our vertices, free them	if(vertices)  {		delete [] vertices;		vertices = NULL;		numVerts = 0;		LOG->writeMsg( Logger::LVL_DEBUG, "Verticies deleted" );	}	if(planes) {	    delete [] planes;	    planes = NULL;	    numPlanes = 0;	    LOG->writeMsg( Logger::LVL_DEBUG, "Planes deleted" );	}	numEntities = 0;	if(textureHeader) {        // Delete the texture data from the texture manager	    for( uint32_t i = 0; i < textureHeader->numMipTextures; ++i )	        m_ptrTextureManagerService->deleteTexture( aryiTextureIds, getElementId() );	    delete textureHeader;        delete [] mipTextures;        delete [] mipTextureOffsets;        delete [] faceTextureCoordinates;        delete [] textureInfo;        delete [] aryiTextureIds;        delete [] textureObjects;        LOG->writeMsg( Logger::LVL_DEBUG, "Textures deleted" );	}    LOG->writeMsg( Logger::LVL_DEBUG, "BSP Delete OK, about to delete entities..." );	for( uint32_t i = 0; i < m_entityFactories.size(); ++i )        m_entityFactories.at(i)->destroyAllEntities();    delete m_ptrTreeRoot;    LOG->writeMsg( Logger::LVL_DEBUG, "Entities deleted!" );}


Now there are 3 entities that are deleted which are part of the class hierarchy.

Leaf is our first one:
Element -> Entity (essentially an interface) -> Resource -> Resource 3D -> Leaf

The entity factor's delete method:
void EntityFactory::destroyAllEntities() {    // Delete all memory    for( uint32_t i=0; i < m_entities.size(); ++i ) {LOG->writeMsg( Logger::LVL_DEBUG, "About to delete entity: " + m_entities.at(i)->getElementId() );        delete m_entities.at(i);    }    // Then clear the vector of the pointers    m_entities.clear();}


Which is used to delete the User entity:
Element -> Entity -> Resource -> Resource 3D -> User

and the camera entity:
Element -> Entity -> Resource -> Resource 3D -> Camera

Lastly the BSP Root is deleted:
Element -> Entity -> Resource -> BSPRoot

here are the destructors, they are trivial mostly except for Resource and Element, but I'll post them all:

User::~User() {    delete m_ptrTagText;    delete m_ptrFpsText;    delete m_ptrVelocityText;    delete m_ptrLeafText;    delete m_ptrPositionText;    delete m_ptrFrictionText;    delete m_ptrHudTag;}Leaf::~Leaf() {}Resource3D::~Resource3D() {}Resource::~Resource() {LOG->writeMsg( Logger::LVL_DEBUG, "Deleting Resource" );    m_ptrResourceManagerService->removeResource( getElementId() );}Entity::~Entity() {}Element::~Element() {LOG->writeMsg( Logger::LVL_DEBUG, "Deleting Element: " + getElementId() );    if( m_ptrElementManagerService->removeElementById( getElementId() ) )        LOG->writeMsg( Logger::LVL_DEBUG, "Element successfully removed from Element Manager Service: " + getElementId() );    else        LOG->writeMsg( Logger::LVL_ERROR, "Element FAILED to remove from Element Manager Service" );}


So the last 2 important parts of the removeResource() and removeElementById() functions,

This is the removeElementById() function and the hash table remove function it calls:
bool ElementManagerService::removeElementById( string strElementId ) {    bool success = false;LOG->writeMsg( Logger::LVL_DEBUG, "Element Manager removing element: " + strElementId );    Element *ptrElement = searchForElementById( strElementId );    // Find the elements entry in the ID hash table    ElementRegistryEntry *entry = m_ptrElementsByIDTable->retrieve( strElementId );    if( entry != NULL ) {        ClassRegistryEntry *ptrClassEntry = m_ptrElementsByClassTable->retrieve( ptrElement->getClassName() );        // Check if this is the last element        if( ptrClassEntry->m_elements.size() > 0 ) {            bool bFound = false;            for( uint32_t i=0; i < ptrClassEntry->m_elements.size() && !bFound; ++i ) {                if( ptrClassEntry->m_elements.at(i) == strElementId ) {                    ptrClassEntry->m_elements.erase( ptrClassEntry->m_elements.begin() + i );                    bFound = true;                }            }            // If the class is empty, remove it            if( ptrClassEntry->m_elements.size() == 0 )                m_ptrElementsByClassTable->removeByKey( ptrClassEntry->m_strClassName );        }        else            LOG->writeMsg( Logger::LVL_ERROR, "Element Manager Service: Element Class entry was empty before delete was preformed" );        // Delete the elements registry in the ID table        m_ptrElementsByIDTable->removeByKey( strElementId );        success = true;    }    return success;}


And the hash table remove function:
// Return: -true- if key was found and chain node was removed -false- if key was not found, nothing removedtemplate <typename TYPE> bool HashTable<TYPE>::removeByKey( string strKey ) {    bool success = false;    uint32_t iHashValue;    iHashValue = hashString( strKey );    HashTableChainNode<TYPE> *currentNode = &table[iHashValue];    // Advance through childeren untill key match is found    while( currentNode->getChild() != NULL && !success ) {        currentNode = currentNode->getChild();        // Key match found        if( currentNode->getKey() == strKey ) {            // If parent exists, set parent's child to this nodes child            if( currentNode->getParent() != NULL )                currentNode->getParent()->setChild( currentNode->getChild() );            delete currentNode;            --m_iCurrentSize;            success = true;        }    }    return success;}


Here is the removeResource() function:
bool ResourceManagerService::removeResource( string strElementId ) {    ResourceRegistryEntry *ptrResourceEntry = m_ptrResources->retrieve( strElementId );    // Remove scene graph roots    if( ptrResourceEntry->m_iResourceType == RMS_SG_3DROOT_NODE ) {        removeSceneGraphRoot( strElementId );    }    delete ptrResourceEntry->m_ptrSceneGraphNode;    m_ptrResources->removeByKey( strElementId );    delete ptrResourceEntry;}// This is conditionally called by removeResource()bool ResourceManagerService::removeSceneGraphRoot( string strElementId ) {    bool bSuccess = false;    // Ensure Id isn't blank    if( strElementId != "" ) {        // Find the Resource Registry Entry        ResourceRegistryEntry *ptrResource = m_ptrResources->retrieve( strElementId );        // Ensure the resource was found        if( ptrResource != NULL ) {            for( uint32_t i=0; i < m_sceneGraphRoots.size() && !bSuccess; ++i ) {                // If this is the node we are looking for                if( m_sceneGraphRoots.at(i)->getNodeId() == ptrResource->m_ptrSceneGraphNode->getNodeId() ) {                    m_sceneGraphRoots.erase( m_sceneGraphRoots.begin() + i );                    bSuccess = true;                }            }        }    }    return bSuccess;}


Now, the Scene Graph nodes that are deleted here call their own destructors:
// The resource node in the scene graph has a translation node and rotation node, so they must be cleaned up:Resource3DNode::~Resource3DNode() {    delete m_ptrTranslationNode;    delete m_ptrRotationNode;}// Then the base destructor for SceneGraphNode is called:SceneGraphNode::~SceneGraphNode() {    removeAllChilderen();    if( m_ptrParent != NULL ) {        m_ptrParent->removeChild( this );    }}


And Scene Graph Nodes are:
Element -> Entity -> SceneGraphNode

So uh... ya... There's a lot going on when the resources are deleted.

Anyone see any glaringly obvious mistakes?
Sharlin
Sharlin
There are WAY too many raw pointers, WAY too much manual memory management in that code. Why are all those members pointers? The cold fact is that humans suck at memory management, and that's why sensible people delegate that job to the computer whenever possible. In well-written C++, there are very very few instances where bare pointers are required or even desirable. RAII is your friend.

(BTW, do you know about the Rule of Three? Obeying it is absolutely necessary whenever you fiddle with bare pointers.)
Wavesonics
Wavesonics
My only formal language training as been in Java and a bit of C.

Everything else is self taught which obviously has it's down sides.

I don't have any explicit copy or assignment constructors, but I pass everything around as a pointer, you want that exact instance of a thing, not the values of it, so while I see the point of the rule of 3, i don't think it has had a large affect here by not being implemented. Even still I probably will at some point implement it. Along with const correctness and other things I'm only learning about now in C++, theres quite a few things I plan on changing about this code base. Like passing references instead of pointers.

So thanks for pointing me to this info, it's all stuff I really want to learn to become a better C++ coder. Any other tips are very welcome.

Those 2 links are good thanks.

Now down to the issue though, There are certain things I can see cutting back on as raw pointers. In User, all of those members could be stack allocated, and the Rot and Trans nodes could be stack allocated. ( There is no default constructor, so I was getting compile errors?)

But besides that things need to be dynamically sized and allocated and such, so how could this be improved to remove more bare pointers?

At the moment though I don't have time to go back and refactor the entire code base to use references and less raw pointers. I need to fix this bug and finish what I'm trying to get done first, so I'm going to boot into linux nad give: http://valgrind.org/ a try. Any other sugestions?
dmail
dmail
I see alot of this in your code but you do not show the constructors.
void BSPManagerService::Destroy(){    LOG->writeMsg( Logger::LVL_DEBUG, "Deleting BSP" );    m_bLoaded = false;    display( false );	// If we still have valid memory for our nodes, free them	if(nodes)  {		delete [] nodes;		nodes = NULL;		numNodes = 0;		LOG->writeMsg( Logger::LVL_DEBUG, "Nodes deleted" );	}

Deleting a null pointer is valid in C++, so the question maybe why are you checking these values and what are the values when you check. ie do you initialise them to null in the constructor and is there any reason why the destructor can not do the destroy functions work?
Zahlman
Zahlman
Quote:

So uh... ya... There's a lot going on when the resources are deleted.

Anyone see any glaringly obvious mistakes?


Yes: the most glaringly obvious mistake is that there is a lot going on. Well-written code is organized in such a way that the need for any destructor at all is actually quite rare (because the cleanup has been factored away into the contained object, and handled either by a standard library container or a smart pointer or perhaps one of the boost ptr_containers).



I'm more interested in the data structures, really - the headers rather than any function implementations. But I have seen enough to ask a few questions...

Apparently you do know about vectors. Why aren't you using them throughout, instead of these pairs of array and element-count? (But speaking of knowing about vectors... you know we have these lovely things called "iterators", that avoid the need to think in terms of index values? Or for that matter, standard library algorithms like std::for_each() designed to prevent you from needing to write loops iterating over a container?)

How come all your delete[]s are checked for NULL? As noted, you don't need to do this - but you seem to know that already, because none of your (scalar) deletes are checked for NULL!

Why do you have functions with names like 'Destroy'? Can't you just use the destructor? Clearly you know about destructors.

You *do* have a *virtual* destructor for Entity, right?

What's with this roundabout "finding" process for Leaf entities and dynamic_casting of them to delete them? First off - oh, that's a warning sign that you don't have that virtual destructor, isn't it. :( But anyway, aren't the Leaves all known to be contained in, well, the 'leaves' array?

What's with all these members whose names start with 'm_ptr'? Are you worried that you might somehow forget that they're pointers? Or that the compiler might somehow compile code that treats them as non-pointers? The C++ type system is quite a bit stronger than that of C, you know. (Actually, you have quite a bit of that Hungarian notation floating around, but it's very inconsistently used. Get rid of it. Please. It does nothing but add maintenance problems.)

And what kinds of things are those pointers in the User class pointing at, anyway? Does it *own* those pointed-at things? I.e. is there a separate set of whatever instances for each User? If they are sharing things, you are screwed.

In fact, if anything is sharing things, you are likely to be screwed. :) That is what boost::shared_ptr is for.

Did you really *need* to make your own hash table? Did you at least *try* std::map first to see if it was acceptable for associative lookup?
Wavesonics
Wavesonics
The leaves array does not contain leaf objects. It contains bare bones data from the file. That data is used to create the leaf objects. You might say it should be deleted after the leaves are created then, but it is used in the Potentially Visible Set operations.

Quote:
Well-written code is organized in such a way that the need for any destructor at all is actually quite rare

If you can show me a different way to delete objects loaded with a level that then need to be deleted when the level is destroyed please tell me.


Quote:
Why do you have functions with names like 'Destroy'? Can't you just use the destructor? Clearly you know about destructors.


I don't *just* have all the delete code in my deconstructor, because the code in the Destroy() function ONLY destroys the currently loaded level. The class that function is in is a Service which does not get destructed until the engine is being destructed, and you want to be able to destroy a level at any time.

Yes all the destructors are virtual.

Quote:
Are you worried that you might somehow forget that they're pointers? Or that the compiler might somehow compile code that treats them as non-pointers?


No i'm not worried about the compiler treating the pointers as non-pointers. I do that so me or any one else reading the code at any point knows what type of variable a variable is with out having to dig out it's declaration.


Quote:
Does it *own* those pointed-at things? I.e. is there a separate set of whatever instances for each User? If they are sharing things, you are screwed.


Yes it does *own* these things, no, multiple user objects do not share them. The reason in fact they are pointers is because they do not have default constructors and I was unclear on the syntax of calling those *owned* objects non-default constructors during the owning classes constructor. Doing them as pointers solved that.

Quote:
Did you really *need* to make your own hash table? Did you at least *try* std::map first to see if it was acceptable for associative lookup?


Nope, I didn't *need* to make a hash table, but I don't *need* to make a 3D engine either. I had to make this hash table for a class and decided i wanted to use it since I made it.

I'm using valgrind now and pretty sure I found my problem.

[Edited by - Wavesonics on April 12, 2007 1:36:14 PM]
Zakwayda
Zakwayda
Quote:
Original post by Wavesonics
Quote:
Well-written code is organized in such a way that the need for any destructor at all is actually quite rare

If you can show me a different way to delete objects loaded with a level that then need to be deleted when the level is destroyed please tell me.
The answer is that you don't delete them; this task should be left to the RAII containers that manage the objects (examples of such containers include std::vector, boost::shared_ptr, and so on).
Quote:
The reason in fact they are pointers is because they do not have default constructors and I was unclear on the syntax of calling those *owned* objects non-default constructors during the owning classes constructor. Doing them as pointers solved that.
It sounds like you were looking for 'member initializer lists'. Try Googling this term; using initializer lists where applicable should solve this particular problem for you, I think.
Wavesonics
Wavesonics
Awesome thank you so much jyk, you are by far one of the most helpful people on this forum!

I'm not totally clear on how a vector would control the construction and destruction of objects, but I'll Google it.

I'm also very interested in your math library, I've been coding my own up to this point but I'm very seriously considering changing over to yours.

Is the current release considered stable?

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.

Sign in to reply to this topic.