• Advertisement
Sign in to follow this  

"C++ for Game Programmers" Book has a Gotcha!

This topic is 3828 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Well after having long puzzled over why my latest DX9 project crashed, I thought I'd revamp my memory manger (fun fun fun... NOT!), which was partly based on what Noel Llopis, the author of "C++ for Game Programmers", suggested in Chapter 7. Now don't get me wrong, I like this book and would easily recommend it to all intermediate level coders, because even though you probably already know a few good tricks already, this book, like many others, always seems to teach you a few more... Saying that however, I seemed to have come across the following Gotcha! Noel suggests that one could start implementing the memory manager by obviously overloading the new and delete operators and puts forward the following:
// A 'unqiue' integer constant identifying
// allocations linked to the heap memory manager.
const int MEMSYSTEM_SIGNATURE = 0xDEADC0DE;

// Structure for a memory allocation header.
// Holds bookeeping information as well as
// providing access to the heap manager class 'Heap'.
struct AllocHeader
{
	int  nSignature;
	int  nSize;
	Heap *pHeap;
};

// Overloaded new operator
void *operator new (size_t size, Heap *pHeap)
{
	size_t      nRequestedBytes = size + sizeof (AllocHeader);
	char        *pMem           = (char *) malloc (nRequestedBytes);
	AllocHeader *pHeader        = (AllocHeader *) pMem;
	
	pHeader->nSignature = MEMSYSTEM_SIGNATURE;
	pHeader->pHeap      = pHeap;
	pHeader->nSize      = size;

	pHeap->AddAllocation (size);
	void *pStartMemBlock = pMem + sizeof (AllocHeader);
	return pStartMemBlock;
}

Apart from the comments and spacing style, this is exactly the code laid out in the book, so please no comments on why I didn't do an allocation check or resorted to the use of that dreaded C style of casting... This AIN'T my code! OK so far so good you may ask, but then look at the next code:
// Correspoding delete operator (GOTCHA!)
void operator delete (void *pMem)
{
	AllocHeader *pHeader = (AllocHeader *) ((char *) pMem - sizeof (AllocHeader));
	assert (pHeader->nSignature == MEMSYSTEM_SIGNATURE);
	pHeader->pHeap->RemoveAllocation (pHeader->nSize);
	free (pHeader);
}

Do you see the problem? It's subtle I give it that AND it won't necessarily crash your app. But it IS unsafe and it bit me in the proverbial ass when one of my DLL's tried to delete memory. Basically the line;
AllocHeader *pHeader = (AllocHeader *) ((char *) pMem - sizeof (AllocHeader));

will cause memory that isn't part of your program to be accessed if it attempts to delete memory that WASN'T allocated via the heap manager. I must admit I overlooked this bug as I assumed a general protection fault (GPF) wouldn't be generated if one merely read from, but didn't write to, areas of memory outside program scope. Anyway, to give him credit, Noel did intend to use these constructs for class-specific allocations, where further safe guards can be put in place, like over-riding default new and delete so that ALL objects from the class are handled by the heap manager. But still, for normal allocations, it's a GPF waiting to happen.

Share this post


Link to post
Share on other sites
Advertisement
Quote:
But it IS unsafe and it bit me in the proverbial ass when one of my DLL's tried to delete memory.

How did you new the memory and if it could not see the overloaded new, how could it see the overloaded delete?

I would assume that you have a problem with your dlls(maybe wrong), you are creating on one side of the dll boundary and deleting on the other.

Share this post


Link to post
Share on other sites
The new is not the problem. It's overloaded so its connection to the heap manager is obvious.

However, for non user data types, or classes that don't use the overloaded new operator, when you use "delete" the DEFAULT delete operator is ALWAYS called regardless. You can of course over ride with "operator delete (PVOID, HEAP *)", but calling delete on allocations not using the manager will cause problems.

It did for me.

EDIT:

With regards to DLL allocations, it would be quite noob of me to delete memory outside of the dll that created it. No, in fact this was an internal problem in the dll itself that caused the runtime crash. Which was fortunate because this error never showed up before, perhaps because only “garbage” memory was being read and so the OS didn’t complain?

I'm assuming that the dll in question was placed in a tight area of memory so that the bug was caught.

Anyway, the solution to this is simple. Simply leave the default delete op alone. It causes far too much hassle and creates dependencies with specialized memory allocations. As Noel did intend, he was going to override default new and delete ops in classes, however he made the mistake of altering the global delete op that’s all.

[Edited by - TerrorFLOP on October 28, 2007 5:14:11 PM]

Share this post


Link to post
Share on other sites
I'm sorry you have given far too little information for me to say if you are correct or not. There was an update to the memory code if I remember correctly.
[edit]see bottom of Book Page for update.
Quote:
Anyway, the solution to this is simple. Simply leave the default delete op alone.

How does this solve anything, the delete removes the memory size from the heap manager in this example does it not? Would this not mean you can not track leaking memory which is the purpose of the code.

[Edited by - dmail on October 28, 2007 7:40:55 PM]

Share this post


Link to post
Share on other sites
This problem is solved by the way; I only mentioned it to make others aware of it.

But basically the problem is this. If you tinker around with GLOBAL new and delete ops, you can introduce certain bugs into your program. With regards to MY code, I decided to go ahead with Noel's suggestion as I wanted to overload new so that I could create aligned objects on the heap (using _aligned_malloc and _aligned_offset_malloc).

The problem I had was that I wanted to make this available to ANY data type, not just class ones and so, upon reading the book, I went ahead with overriding global delete (big mistake!).

Now obviously, not ALL data needs to be aligned, and to make the code transparent and portable, both aligned and unaligned memory would need to be deleted by the SAME default delete op. Hence the memory allocation header to flag 'special' allocations from normal ones.

But therein lays the problem. Normal memory would STILL have to be read offset in order to check for its malloc signature, and that offset could well crash your program. In most cases, it probably won't happen since this offset is VERY small (only 12 bytes in the example I gave), but still, in a release build or if this offset read takes place in a tight dll (which was my case), that may be enough to cause a crash.

So simply leaving the global delete op (or littering your code with ugly "operator delete (...)" overloads) is the only way to go methinks. So right now, I'm just going to limit myself to aligning class objects only where calling delete WILL cause their own delete routines to execute.

Share this post


Link to post
Share on other sites
Well if you are happy then play on, personally I think you have some problems and will not admit to the possibility.

Quote:
In most cases, it probably won't happen since this offset is VERY small (only 12 bytes in the example I gave), but still, in a release build or if this offset read takes place in a tight dll (which was my case), that may be enough to cause a crash.

As a final question, why on earth would you be doing this in a release build?

Share this post


Link to post
Share on other sites
Errr.... Doing what dude?

I already said that I've opted for the safer version and don't intend to override the global delete op (which as I also said was the cause of the problem and oversight in the book).

So of course I'll "play on" because, even though I can't align non user data types using new, I've stuck with just letting classes correctly align themselves, thus bypassing global delete alltogether. So the code is correct and safe despite losing a little flexibility. And besides, there are easy ways around that anyway since simple data types don't require constructors to initialise them... A simple _aligned_malloc and your away... Messy perhaps but works no problem.

Again, I only mentioned it to allow others to be aware of the potential pitfall in the book. Reading before or indeed after allocated memory is dangerous in any code design.

Share this post


Link to post
Share on other sites
Quote:
Quote:
In most cases, it probably won't happen since this offset is VERY small (only 12 bytes in the example I gave), but still, in a release build or if this offset read takes place in a tight dll (which was my case), that may be enough to cause a crash.


As a final question, why on earth would you be doing this in a release build?

Quote:
Errr.... Doing what dude?


Why would you be adding all this overhead in a release build?

// Overloaded new operator
void *operator new (size_t size, Heap *pHeap)
{
size_t nRequestedBytes = size + sizeof (AllocHeader);
char *pMem = (char *) malloc (nRequestedBytes);
AllocHeader *pHeader = (AllocHeader *) pMem;

pHeader->nSignature = MEMSYSTEM_SIGNATURE;
pHeader->pHeap = pHeap;
pHeader->nSize = size;

pHeap->AddAllocation (size);
void *pStartMemBlock = pMem + sizeof (AllocHeader);
return pStartMemBlock;
}




instead of something like:


// Overloaded new operator
void *operator new (size_t size, Heap *pHeap)
{
#ifdef DEBUG
size_t nRequestedBytes = size + sizeof (AllocHeader);
char *pMem = (char *) malloc (nRequestedBytes);
AllocHeader *pHeader = (AllocHeader *) pMem;

pHeader->nSignature = MEMSYSTEM_SIGNATURE;
pHeader->pHeap = pHeap;
pHeader->nSize = size;

pHeap->AddAllocation (size);
return pMem + sizeof (AllocHeader);
#else
//get from (normally)preallocated pool memory, not incurring the header overhead and runtime costs.
return pHeap->alloc(size);
#endif
}


Share this post


Link to post
Share on other sites
KK first up, this wasn't MY code as previously stated. I was trying to point something out in a book that caused me to have a nasty stealth bug lurking in my code, which never exposed itself until I got "lucky" when one of my dlls crashed (I guess it was in the right place at the right time kinda thing).

Point 2. The author of the book was trying to implement a memory manager. I don't actually have a full blown one myself (it's not too high on my priority list at the moment), but as the author states, and I happen to agree, memory managers can be great in speeding up your code. So I would assume you'd want that code in a Release build, not the heap statistics reports of course, but management for memory pools and the like.

That's all... I'm not stuck or anything. I was actually trying to be helpful. If you're interested in exactly how he went about memory managers, then I suggest you get the book. All books have errors so please, despite me flagging up this mishap, "C++ for Game Programmers" is a damn good read and has plenty useful optimizing concepts explained in it.

And please let me reiterate one last time... I am fine thank you. The problem was solved. I merely created this thread to pass on "useful" knowledge regarding an error in a book.

EDIT: Remember dmail, I chose snippets of code from the book JUST to highlight the error only. Try not to get too bogged down in the code's details. It's straight outta a book that was trying to explain how one could begin to implement a memory manager. Of course as the book develops, he does go on to discuss variant builds in the code. However that was neither my concern nor related to the problem that I had. The error in the code was my main focus. Hope we cleared that up ;-).


[Edited by - TerrorFLOP on October 29, 2007 1:58:48 AM]

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement