🔒 Locked 🤖 Godot

Stack corruption problem, VC++ .NET 2002

Started by Arcibald Wearlot Sep 10, 2006 at 6:54 PM 3 replies 800+ views

Original Post

Sep 10, 2006 at 6:54 PM

Since I rewrote this function adding some functionality, I keep getting this debug error: "Run-time Check Failure #2 - Stack around the variable '$1' was corrupted." The strange thing is that there isn't a precise line that causes the error, rather it seems as if I run out of stack space because I'm using too many auto variables or something like that. I can say this because if I comment out one of the variables, or even make it static, I don't get the error. For example I can eliminate the FinalVelocity vector, and the problem disappears. But then if I create anywhere in the function (even outside loops) two or more float variables, the problem reappears, while it goes fine with just one float in this case. So it really seems that I'm reaching some stack limit, but look at the function, I don't think I'm doing nothing that should cause this. There are only two nested loops, and I'm getting this problem with only two entities in the game world. The function gets called once per frame. This is really driving me crazy. Do you have any idea of what I'm doing wrong?


void CEntityManager::UpdateEntities( float DeltaTime, IWorldModel* pWorld, CTerrain* pTerrain )
{
	if( DeltaTime == 0.0f )
	{
		return;
	}

	// Check for "noclip" CVar
	bool CheckCollisions = true;
	if( g_pCVarMgr->CVarExists( "noclip" ) && ( g_pCVarMgr->GetBool( "noclip" ) == true ) )
	{
		CheckCollisions = false;
	}
	
	if( CheckCollisions )
	{
		//---------------------------------------------------------------------
		//
		//	Pass 1: Gravity
		//
		//---------------------------------------------------------------------

		for( Map_HandleToEntity::iterator itor = m_Entities.begin(); itor != m_Entities.end(); ++itor )
		{
			CEntity* pEnt = itor->second;
			const D3DXVECTOR3& EntPos = pEnt->GetPosition();

			// Check if will collide with the world
			if( pWorld && pWorld->CheckGravityCollision( pEnt->GetPhysics(), DeltaTime ) )
			{
				pEnt->ResetGravity();
				continue;
			}

			// Check if we will collide with another entity
			bool Stopped = false;
			for( Map_HandleToEntity::iterator j = itor; j != m_Entities.end(); ++j )
			{
				if( itor == j )
				{
					continue;
				}

				CEntity* pSecond = j->second;
				if( pSecond->IsDead() )
				{
					continue;
				}

				if( math::SphereSphereCollide( pEnt->GetPhysics()->GetSphere(), pEnt->GetPhysics()->GetGravityVelocity() * DeltaTime,
					pSecond->GetPhysics()->GetSphere(), pSecond->GetPhysics()->GetGravityVelocity() * DeltaTime ) )
				{
					pEnt->ResetGravity();
					pSecond->ResetGravity();
					Stopped = true;
					break;
				}
			}
			if( Stopped )
			{
				continue;
			}

			// Go down
			pEnt->UpdateGravity( DeltaTime );

			// Align with the terrain
			if( pTerrain )
			{
				float Height = pTerrain->GetHeight( EntPos.x, EntPos.z ) + pEnt->GetPhysics()->GetSphere().GetRadius();

				if( EntPos.y <= Height )
				{
					pEnt->SetPosition( D3DXVECTOR3( EntPos.x, Height, EntPos.z ) );
					pEnt->ResetGravity();
				}
			}
		}

		//---------------------------------------------------------------------
		//
		//	Pass 2: Velocity
		//
		//---------------------------------------------------------------------

		// Check for collisions with the world

		for( Map_HandleToEntity::iterator itor = m_Entities.begin(); itor != m_Entities.end(); ++itor )
		{
			CEntity* pEnt = itor->second;

			if( pWorld )
			{
				pWorld->ResolveCollisions( pEnt->GetPhysics(), DeltaTime, WORLD_SLIDE );
			}

			// Set the new velocity modified by the terrain
			D3DXVECTOR3 FinalPos = pEnt->GetPosition() + ( pEnt->GetPhysics()->GetVelocity() * DeltaTime );
			float TerrainHeight = pTerrain->GetHeight( FinalPos.x, FinalPos.z ) + pEnt->GetPhysics()->GetSphere().GetRadius();
			if( FinalPos.y <= TerrainHeight )
			{
				FinalPos.y = TerrainHeight;
			}

			D3DXVECTOR3 FinalVelocity = ( FinalPos - pEnt->GetPosition() );
			D3DXVec3Normalize( &FinalVelocity, &FinalVelocity );
			FinalVelocity *= D3DXVec3Length( &pEnt->GetPhysics()->GetVelocity() );
			pEnt->GetPhysics()->SetVelocity( FinalVelocity );
		}

		// Check for collisions between entities

		for( Map_HandleToEntity::iterator itor = m_Entities.begin(); itor != m_Entities.end(); ++itor )
		{
			CEntity* pFirst = itor->second;
			if( pFirst->IsDead() )
			{
				continue;
			}
			
			for( Map_HandleToEntity::iterator j = itor; j != m_Entities.end(); ++j )
			{
				if( itor == j )
				{
					continue;
				}

				CEntity* pSecond = j->second;
				if( pSecond->IsDead() )
				{
					continue;
				}

				pFirst->ResolveCollision( pSecond, DeltaTime );
			}
		}
	}

	// Finally update each entity
	for( Map_HandleToEntity::iterator i = m_Entities.begin(); i != m_Entities.end(); ++i )
	{
		CEntity* e = i->second;
		e->Update( DeltaTime );
	}
}

ApochPiQ

Sep 10, 2006 at 7:17 PM

You're mangling stack data, not using too much of it. (For reference, a genuine stack overflow is pretty hard to do without resorting to unbounded recursion.) This may be an array bounds overflow, or (more likely, given your code) an invalidated iterator.

Is there any way that m_Entities could be changed somehow (adding/removing items) while this function runs? If so, that's your problem right there.

NotAYakk

Sep 11, 2006 at 1:22 AM

Invalid iterators tend to corrupt the heap, not the stack.

Are you getting reliable and repeatable stack corruption errors? I hope so -- those are easier to track down, and stack corruption is a bitch to track down.

One way that could help you track this down is making your function into a bunch of smaller functions. Note that this WILL NOT FIX IT -- if you are unluckly it will, however, CONCEAL it.

So save the current state of your code.

Next, take the code, and break it down into multiple smaller functions. Are you still getting stack corruption errors? By making your functions smaller, you can shrink the area of search.

Another approach would be looking at all of the objects you have stored on the stack. Probably some pointer at one of those objects is being used ... poorly. Either the pointer is persisting past the destruction of the object, or you are making a "fencepost" error and accessing memory beyond the end of the pointed to object.

Thinking about it, accessing memory that you no longer own may be the most likely bug.

...

Some suspicious lines:

			const D3DXVECTOR3& EntPos = pEnt->GetPosition();...				if( math::SphereSphereCollide( pEnt->GetPhysics()->GetSphere(), pEnt->GetPhysics()->GetGravityVelocity() * DeltaTime,					pSecond->GetPhysics()->GetSphere(), pSecond->GetPhysics()->GetGravityVelocity() * DeltaTime ) )...			FinalVelocity *= D3DXVec3Length( &pEnt->GetPhysics()->GetVelocity() );

Questions:
What type does pEnt->GetPosition() return? What is the lifetime of what it returns?
What type does pEnt->GetPhysics() return? What is the lifetime of what it returns?
Second->GetPhysics()->GetSphere() and pEnt->GetPhysics()->GetVelocity() as well.

Examine all of your functions that return pointers or references. What is the lifetime of the object you point to/refer to?

Of course, it could be something completely different. :)

Arcibald Wearlot

Sep 11, 2006 at 7:17 AM

Maybe I solved it, you are right NotAYakk, the problem was in these lines:

const D3DXVECTOR3& EntPos = pEnt->GetPosition();FinalVelocity *= D3DXVec3Length( &pEnt->GetPhysics()->GetVelocity() );

GetPosition() and GetVelocity() both return a D3DXVECTOR3.
In the first line I was binding a reference to a temporary variable, and maybe this is unsafe.
I was doing a similar thing here in the second line, I was getting the address of a temporary variable. I modified the lines to this:

D3DXVECTOR3 EntPos = pEnt->GetPosition();D3DXVECTOR3 Vel = pEnt->GetPhysics()->GetVelocity();FinalVelocity *= D3DXVec3Length( &Vel );

Anyway changing the second line had no effect, it looks like the problem was only caused by the first line.

I realize that it doesn't seem a safe thing to mess up with the addresses of temporary variables, but shouldn't a temporary variable be usable within the scope it is created in?

Edit: Regarding this line

math::SphereSphereCollide( pEnt->GetPhysics()->GetSphere(), pEnt->GetPhysics()->GetGravityVelocity() * DeltaTime,					pSecond->GetPhysics()->GetSphere(), pSecond->GetPhysics()->GetGravityVelocity() * DeltaTime ) )// Prototypebool math::SphereSphereCollide( const CSphere& First, const D3DXVECTOR3& FirstVel,							const CSphere& Second, const D3DXVECTOR3& SecondVel )

GetSphere() returns a const CSphere&, GetGravityVelocity() returns a D3DXVECTOR3. Is this safe?

NotAYakk

Sep 11, 2006 at 11:35 AM

Quote:
GetPosition() and GetVelocity() both return a D3DXVECTOR3.
GetSphere() returns a const CSphere&, GetGravityVelocity() returns a D3DXVECTOR3. Is this safe?

Are you certain that GetPosition()/GetVelocity()/GetGravityVelocity() all return D3DXVECTOR3 and none of them return const D3DXVECTOR3&s?

Quote:
I realize that it doesn't seem a safe thing to mess up with the addresses of temporary variables, but shouldn't a temporary variable be usable within the scope it is created in?

Reference and pointer variables are only safe to be used within the lifetime of the thing they are pointing or referring to.

Technically, if you have a temporary that is bound to a const reference, the lifetime of the temporary should be that of the const reference. But, first, sometimes compilers can screw this up, and second, if you then go and copy the const reference into another const reference, the lifetime of the temporary is only the first const reference -- not the second.

In essence, if your function returns pointers or const references, you have to make certain you are not returning a temporary.

Lastly:

Quote:
GetSphere() returns a const CSphere&

That CSphere -- is it a temporary created inside the GetSphere() function, or is it a persistant CSphere that you are returning a const reference to?

...

Note my advice of "write smaller functions" holds. Your function is between two and 10 times larger than I'd be comphy with.

You have large commented out blocks -- why not refactor those into functions?

Ie:
DoGravityAll() // do gravity for every object
--DoGravityFor() // do gravity for one particular object
DoVelocityAll()
--DoVelocityFor()
DoWorldCollisionsAll()
--DoWorldCollisionFor()
DoPairwiseCollisionsAll()
--DoPairwiseCollisionsFor()
----DoPairwiseCollisionsForBoth()
DoPhysicsUpdateAll()
--DoPhysicsUpdateFor()

The "All" functions consist of a loop and a call to a For function. It is quite possible that they can be turned into algorithms, and the dangerous loop code can be removed. (Loop code is dangerous because simple typos can result in fencepost memory errors. Memory corruption results in hard to track down bugs. Avoid writing code that can easily generate hard to track down bugs as much as is reasonable.)

The "For" functions would consist of the body of your for loops.

Being able to have an entire function on a screen, with an overview of what is done in the function, helps. It also means that when you get stack corruption problems, you have a much smaller window to look in. :)

Topic Locked

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