Jump to content

  • Log In with Google      Sign In   
  • Create Account


Debug Assertion Failed!


Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.

  • You cannot reply to this topic
9 replies to this topic

#1 Flaise   Members   -  Reputation: 165

Like
0Likes
Like

Posted 03 August 2006 - 03:23 AM

Here's the pseudoenglish I'm getting from a message box: Debug Assertion Failed! Program: ...ENTS\PROGRAMMING\CPPStuff06\FiniteState\Debug\FiniteState.exe File: dbgdel.cpp Line: 47 Expression: _BLOCK_TYPE_IS_VALID(pHead->nBlockUse) I've isolated the problem to the deletion in this piece of code:
			~Array<I>() {
				if(ptr) {
					delete[] ptr;
					ptr=0;
				}
			}


Apparently, I'm deleting something that has already been deleted, but I made sure each array has a pointer to its own primitive array:
	//Backslashes removed for legibility.
	#define setPtr(_size) {
		assert(static_cast<bool>(_size>0),"New array size negative.",true)
		if(ptr)
			delete[] ptr;
		if(_size) {
			ptr=new I[_size];
			assert(ptr,"Memory allocation error.",true)
		} else {
			ptr=0;
		}
	}


[help]


Sponsor:

#2 Josh Petrie   Moderators   -  Reputation: 2716

Like
0Likes
Like

Posted 03 August 2006 - 03:31 AM

First of all, that macro is gross and you should replace it with an inline function... although I think its questionable whether or not you actually need that kind of multi-responsibility functionality, especially in a function or macro so poorly named.

In any case, the likely cause of such errors has nothing to do with allocating or releasing the memory. Instead, you're probably writing past the end of the array somewhere. You'll have to examine every place you access the array in question and ensure you don't write beyond the space you allocated.

#3 superpig   Staff Emeritus   -  Reputation: 1825

Like
0Likes
Like

Posted 03 August 2006 - 03:31 AM

What are you doing with that #define?



#4 Flaise   Members   -  Reputation: 165

Like
0Likes
Like

Posted 03 August 2006 - 03:57 AM

The "setPtr" macro deletes the underlying primitive array and then allocates a new one of length _size, all the while testing to make sure no errors occured.

But more importantly, the error occurs here:


~Array<I>() {
if(ptr) {
delete[] ptr; //Right here; I step over this line in the
//Array<char> instance of this class and
//there goes that same error message.
ptr=0;
}
}




operator delete:
00404CC0 push ebp
00404CC1 mov ebp,esp
00404CC3 push ecx
00404CC4 push ebx
00404CC5 push esi
00404CC6 push edi
00404CC7 cmp dword ptr [pUserData],0
00404CCB jne operator delete+0Fh (00404ccf)
00404CCD jmp operator delete+83h (00404d43)
00404CCF mov eax,dword ptr [pUserData]
00404CD2 sub eax,20h
00404CD5 mov dword ptr [pHead],eax
00404CD8 mov ecx,dword ptr [pHead]
00404CDB mov edx,dword ptr [ecx+14h]
00404CDE and edx,0FFFFh
00404CE4 cmp edx,4
00404CE7 je operator delete+6Ah (00404d2a)
00404CE9 mov eax,dword ptr [pHead]
00404CEC cmp dword ptr [eax+14h],1
00404CF0 je operator delete+6Ah (00404d2a)
00404CF2 mov ecx,dword ptr [pHead]
00404CF5 mov edx,dword ptr [ecx+14h]
00404CF8 and edx,0FFFFh
00404CFE cmp edx,2
00404D01 je operator delete+6Ah (00404d2a)
00404D03 mov eax,dword ptr [pHead]
00404D06 cmp dword ptr [eax+14h],3
00404D0A je operator delete+6Ah (00404d2a)
00404D0C push offset string "_BLOCK_TYPE_IS_VALID(pHead->;;nBlo"... (004284a0)
00404D11 push 0
00404D13 push 2Fh
00404D15 push offset string "dbgdel.cpp" (00428494)
00404D1A push 2
00404D1C call _CrtDbgReport (00408f60)



I can't read that assembly, but I know that something in there is raising the problem.

#5 Evil Steve   Members   -  Reputation: 1946

Like
0Likes
Like

Posted 03 August 2006 - 04:07 AM

The problem is that you're almost certainly overrunning the bounds of your array. The only reason it's caught at the delete[] call is because that's the only til the CRT touches the memory, and so the first time it notices it's trashed.

As others have said, look at where you write to that array, you're probably runninf over the end of it.

Also, what's wrong with an inline function instead of that macro?

#6 Flaise   Members   -  Reputation: 165

Like
0Likes
Like

Posted 03 August 2006 - 04:23 AM

Quote:
Original post by Evil Steve
Also, what's wrong with an inline function instead of that macro?

Well... nothing. Which is why I incorporated it into the setSize() function.

Quote:
The problem is that you're almost certainly overrunning the bounds of your array. The only reason it's caught at the delete[] call is because that's the only til the CRT touches the memory, and so the first time it notices it's trashed.

Oh, I get it.

So here's where I'm writing to the underlying primitive array:



//POST: If this array is shrunk, its contents are lost.
void setSize(int _size) {
assert(static_cast<bool>(_size>=0),"New array size negative.",true)
if(size>_size) {
if(ptr)
delete[] ptr;
ptr=new I[_size];
assert(ptr,"Memory allocation error.",true)
size=_size;
} else if(size<_size) {
I* old=ptr;
ptr=new I[_size];
assert(ptr,"Memory allocation error.",true)
if(old) {
for(int i=0;i<size;++i) //Go up to old size to copy existing elements.
ptr[i]=old[i];
delete[] old;
}
size=_size;
} //Else do nothing because the size is the same.
}

//Overlying functions omitted.

setSize(_size);
for(int i=0;i<size;++i)
ptr[i]=arr[i];

setSize(other.size);
for(int i=0;i<size;++i)
ptr[i]=other.ptr[i];

setSize(size+1);
ptr[size-1]=add;

int temp=size;
setSize(size+other.size);
for(int i=temp;i<size;++i)
ptr[i]=other.ptr[i];

assertIndex()
for(int i=index;i<size-1;++i)
ptr[i]=ptr[i+1];
--size;





I'm not seeing any problems, are you?

EDIT: Except that it doesn't compile. Fixed that.

#7 superpig   Staff Emeritus   -  Reputation: 1825

Like
0Likes
Like

Posted 03 August 2006 - 05:08 AM

That function could be made somewhat neater:


void setSize(int _size) // _size should be an unsigned int, but I won't break your interface
{
assert(static_cast<bool>(_size>=0),"New array size negative.",true)
if(size == _size) return; // Nothing to do

I* newStorage = new I[_size];
if(ptr && (_size > size))
{
// Copy what we can to the new array
for(int i=0; i < min(size, _size); ++i) newStorage[i] = ptr[i];
}
delete[] ptr;
ptr = newStorage;
size = _size;
}



What's the rest of that code supposed to do? You've omitted any comments or meaningful names and I don't feel like trying to figure it out myself right now.

#8 JohnBolton   Members   -  Reputation: 1368

Like
0Likes
Like

Posted 03 August 2006 - 05:28 AM

I'm surprised I'm the first to say this...

Use std::vector!



John BoltonLocomotive Games (THQ)Current Project: Destroy All Humans (Wii). IN STORES NOW!

#9 Zahlman   Moderators   -  Reputation: 1674

Like
0Likes
Like

Posted 03 August 2006 - 05:55 AM

0) Don't do this stuff yourself, except for practice. Use std::vector.

1) 'new' - at least as normally invoked - CANNOT return NULL. If there was a failure, it will throw an exception. Also, assertions shouldn't be used to detect runtime failures of that sort; they should be used in cases where you are pretty sure it's your fault if it ever gets tripped (i.e. they're a debugging tool).

2) Calling delete[] on a NULL pointer is well-defined, standard and safe: it does nothing. You should not include this check.

3) The postcondition feels awkward. Why not just keep the elements that will still fit? (This is what std::vector will do BTW)

4) You don't need to loop to copy elements. Use std::copy, as shown. But of course, you should be using std::vector anyway :)

5) Why allow the pointer to be NULL inside the class anyway? Why not just have it point at a zero-size allocation instead?

6) You don't need to static_cast the result of relational operators to bool. It already is bool. But you wouldn't need to static_cast it anyway, as long as it's convertible-to-bool.


//POST: If this array is shrunk, its contents are lost.
void setSize(int _size) {
assert(_size >= 0, "New array size negative.", true);
assert(ptr); // fix the constructor to make sure it's always initialized to
// point to some allocation.
if (size == _size) { return; } // nothing to do.

I* old = ptr;
ptr = new I[_size];
std::copy(old, // at
old + std::min(size, _size), // until
ptr); // to
delete[] old;
size = _size;
}

7) The erroneous code appears to be here:


// concatenate two objects?
int temp=size;
setSize(size+other.size);
for(int i=temp;i<size;++i)
ptr[i]=other.ptr[i]; // ERROR! wrong indices into 'other'!

You presumably want '=other.ptr[i - temp]'.

8) Don't do this stuff yourself, except for practice. Use std::vector. Incidentally, it has a smarter way of handling the memory allocations, to avoid having to re-allocate for every appended element.

#10 Flaise   Members   -  Reputation: 165

Like
0Likes
Like

Posted 05 August 2006 - 06:33 AM

I'm doing this for practice, so please calm down. ;)

What file should I include to get std::copy?

I didn't know I could have a pointer to nothing, so I changed that.

assert(_size >= 0, "New array size negative.", true);
You noted the excessive use of the static_cast here, and I used it because of an evil macro:

#define assert(expression,info,fatal) if(!expression) { ...

When the macro was processed, I got
if(!_size >= 0) {
...

Which causes a metaphorical explosion in the compiler with lots of injured and bleeding text being evacuated to the console; (!_size) is evaluated first, which is bad. So I thought I had to cast it, but then I found out that I could just do this:

if(!(expression)) { ...


And yes, you are right; I copied the wrong indicies from the concatenated element, so I ended up with "Hello, world." followed by 'squared' symbols.


Quote:
8) Don't do this stuff yourself, except for practice. Use std::vector. Incidentally, it has a smarter way of handling the memory allocations, to avoid having to re-allocate for every appended element.


You mean like this?


void realloc(int _length) {
++_length; //Leave trailing null for cout.
assert(_length>0,"New primitive array length negative.")
delete[] ptr;
ptr=new I[_length];
length=_length;
}

//POST: All elements at end of array are lost if array is shrunk.
void setSize(int _size) {
assert(_size>=0,"New array size negative.")
if(size==_size)
return;

int i=0;
if(length>=_size) {
//#### I'm maintaining extra space, so
// I just increase the size.
i=size=_size; //Set counter, i, here to clear all elements after.
} else {
I* old=ptr;
if(old) {
int oldSize=size;
ptr=0; //Zero-out to prevent deletion of 'old' during realloc.
//Either double the length or
//match the needed space.
realloc(__max(_size,length+length));
for(;i<oldSize;++i)
ptr[i]=old[i];
delete[] old;
} else {
realloc(_size);
}
size=_size;
}
for(;i<length;++i)
ptr[i]=0;
}
};



And finally,

Superpig: I originally used 'unsigned int's, but the new operator appeared not to like them. Did I do something wrong?




Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.



PARTNERS