Sign in to follow this  
sj

Problems with my memory tracker

Recommended Posts

sj    127
I have a rather mysterious problem with a simple memory tracker I created. It seems to work correctly most of the time, but now I have found a case where it seems to ignore a memory leak. Here's some code (warning: lots of code):
//LOGGER.HPP
class Logger
{
public:
        //...
	void getLogName( char*& tostring );
        //...
};

//LOGGER.CPP
void Logger::getLogName( char*& tostring )
{
	if( tostring != NULL )
		return;

        //tostring is intended to be cleaned up by the user
	SAFE_NEWARR(tostring, char, strlen(LogName)+1);
	memset(tostring, '\0', strlen(LogName)+1);
	strcpy(tostring, LogName);
}

//DUMMY.HPP
//Test object:
class Dummy
{
public:
	Logger* dlog;

	Dummy()
	{
                //the last argument to Logger() is the LogName
		SAFE_NEW(dlog, Logger("dummylog.txt", 0, "Dummy Log"));
	}
	~Dummy()
	{
		SAFE_DEL(dlog);
	}
};

//MAIN.CPP
//main:
int main()
{
	char* cp = NULL;

	Dummy d;
        //this should create a mem leak as SAFE_DELARR(cp) is never called:
	d.dlog->getLogName(cp);

	atexit(&writeMemInfo); //writes the memory leak info to a file

	getch();
	
	return 0;
}

//UTILS.HPP
//Memory tracker code:
vector<AllocInfo> gAllocInfos;


//This removes AllocInfos from gAllocInfos when the user calls SAFE_DELARR()
void removeAllocInfo(void* ptr, bool arr = false)
{
	for( DWORD i = 0; i < gAllocInfos.size(); i++ )
	{
		if( gAllocInfos[i].MemAddr == (DWORD)ptr )
		{
			if( gAllocInfos[i].IsArray == arr )
			{
				gAllocInfos.erase(gAllocInfos.begin()+i);
				return;
			}
			else return;
		}
	}
}


//These are the macros for memoryalloc/dealloc:
#define SAFE_NEWARR(ptr, object, num) {	ptr = new object[num]; 	char* str = #object; 	gAllocInfos.push_back(AllocInfo(str, (DWORD)ptr, num, __FILE__, __LINE__));}


#define SAFE_DELARR(ptr) {	if(ptr){		removeAllocInfo(ptr, true); 		delete[] ptr; 		ptr = NULL;	}}
So that's about it. Oh, and by the way my complier is Visual Studio 2005 if that matters. Hopefully someone can figure out what's wrong as I am completely out of ideas. Juha

Share this post


Link to post
Share on other sites
Enigma    1410
  1. Can we see the implementation of writeMemInfo?

  2. I hope you're only using raw char *s in order to test your memory tracking code.

  3. In your SAFE_NEWARR macro char * str = #object; should be char const * str = #object; (the implicit conversion from char const[] to char * is legal but deprecated).
Σnigma

Share this post


Link to post
Share on other sites
sj    127
Sure, here's writeMemInfo():


void writeMemInfo()
{
FILE* memlog = NULL;
fopen_s(&memlog, "Memory tracker info.txt", "w");
if( gAllocInfos.size() > 0 )
{
fprintf_s(memlog, "Unreleased allocations (%u in total):\n-------------------------------------------------------------------------------------------\n\n", gAllocInfos.size());
for( DWORD i = 0; i < gAllocInfos.size(); i++ )
fprintf_s(memlog, "%i. Object: %s, Address: %u, Count: %u\nIn file: \"%s\" on line %u\n\n", i+1, gAllocInfos[i].Name.c_str(), gAllocInfos[i].MemAddr, gAllocInfos[i].NumObjs, gAllocInfos[i].File.c_str(), gAllocInfos[i].LineNum);
}
else
fprintf_s(memlog, "Congratulations! No unreleased allocations detected.\n");

fclose(memlog);
}

Share this post


Link to post
Share on other sites
sj    127
Hmm, I'm not sure what you mean by "raw" char*s. Are they like this: char* cptr = new char;? If so, I'm pretty sure I only use them.

Juha

Share this post


Link to post
Share on other sites
Enigma    1410
I'm unable to reproduce your error from what you've posted. Can you post a link to a zip file containing all your code?

When I say "I hope you're only using raw char *s in order to test your memory tracking code." I mean you should normally be using std::string in your Logger class, except if you're deliberately using char * instead in order to test your memory tracker.

Σnigma

Share this post


Link to post
Share on other sites
sj    127
Allright, I got the zip online. Here's the link: http://www.savefile.com/files/8417309 You can find the download link/button at the bottom of the page that opens.

Hope this helps.
Juha

Share this post


Link to post
Share on other sites
sj    127
Hmm, strangely the memory tracker doesn't miss an improper array of ints deallocation which I put in the place of the char array. And neither does it miss a rewritten case of the char array leak. I rewrote it to use memcpy instead of strcpy. And when I again replaced memcpy with strcpy, the tracker still caught the error.

Well anyway, thanks Enigma for pointing me to std strings! They make life much easier. But let me know if you manage to figure out the error with the original test code, I'd very much like to know what was causing it.

Juha

Share this post


Link to post
Share on other sites
Enigma    1410
Well, the problem with the zip you posted is that the Logger.cpp translation unit never #defines UTIL_DEBUGMEM. If you add that as a global project define you'll end up with linker errors in util.h and once you solve those by splitting util.h to util.h and util.cpp your code appears to work perfectly (on that one test at least, I haven't tested it thoroughly).

In general you should always prefer to use RAII wrappers rather than raw pointers (i.e. std::string instead of char *, std::vector, boost::array or another container instead of operator new[], std::auto_ptr, boost::shared_ptr or another smart pointer to wrap calls to operator new, etc). As long as your careful about pathalogical cases (i.e. pointer cycles) you won't ever have to worry about memory leaks again!

There are a few other issues I noticed in your code which shows you're still transitioning from "C with Classes" towards true modern C++:
  • Do not underestimate the advantages of C++ streams for I/O. They eliminate a whole family of common errors. Boost.Format provides a more printf like syntax whilst retaining the type safety of iostreams.

  • The "dread ellipses" (...) construct is generally frowned upon in C++ because you cannot pass objects through it (another strength of streams).

  • Names beginning with an underscore followed by a capital letter (i.e. like _UTILS_H_) are reserved by the C++ standard for use by compiler and standard library implementors. You should not use such names. The same is true for any name containing a double underscore.

  • #include <stdlib.h> should be #include <cstdlib>. A lot of your C headers are correctly C++ified, was that just one you missed?

  • using namespace std; should rarely, if ever, be placed in a header file. Instead you should manually qualify names with their namespaces in header files. The reason for this is that a using declaration cannot be undone, do if one header contains a using declaration then every file that includes that header and every file which is included after that header in any particular translation unit will also implicitly be using that using declaration, whether it wanted to or not.

  • (A bug): What happens when you try the following code with your memory tracker?:
    int * c;
    SAFE_NEWARR(c, int, 1);
    SAFE_DELARR(c);

    Hint: What is the allocation record's IsArray set to?

  • You may already be aware of these but I can't tell just by looking at the code in your zip. Make sure you know about constructor initialiser lists and the "rule of three" (any class requiring a custom-written version of at least one of destructor, copy-constructor and copy-assignment operator usually needs all three).

Happy coding.

Σnigma

Share this post


Link to post
Share on other sites
sj    127
Thank you for investing time in helping me out, and thanks also for an indepth post. I will try to adjust my code according to your suggestions.

Quote:
(A bug): What happens when you try the following code with your memory tracker?: ...


Doh! Missed that one ;) Going to fix it.

Thanks again.
Juha

[Edited by - sj on April 25, 2006 1:26:08 AM]

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this