Jump to content

  • Log In with Google      Sign In   
  • Create Account


Memory overwrite after allocating 4k ints


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
16 replies to this topic

#1 Icebone1000   Members   -  Reputation: 980

Like
0Likes
Like

Posted 02 March 2014 - 02:00 AM

After debugging some really weird behaviors, like vectors going from size 0 to negative values out of nowhere, I realized this was happening after allocating 4000 ints with new. I dont understand, 4k ints (~15KB) is not that huge, and even if it was, why its messing with my mem?

Video:

https://drive.google.com/file/d/0B_iN9pcbyoiKQV9lNllLWktfOGc/edit?usp=sharing

struct TileMap{

		int * pTileMap; // indexes on m_vTiles
		int mapW, mapH;
		float hSpacing, vSpacing;

		sprite::InstancedSprites m_tileInstSprites;
		sprite::InstancesVertexBuffer m_IVB;

		TileMap():pTileMap(nullptr){}
		~TileMap(){ if(pTileMap) delete [] pTileMap; }
	};
	std::vector<TileMap> m_vTileMaps;

This is the structure you see in the vid. 

Both InstancedSprites and  InstancesVertexBuffer hold vectors on its internals, those are getting screwed..that doesnt make any sense, theres a memory constrain Idont know about? Dx

Im hopping is something really stupid, but the vid pretty much shows Im not overwriting stuff myself right?



Sponsor:

#2 Icebone1000   Members   -  Reputation: 980

Like
0Likes
Like

Posted 02 March 2014 - 02:04 AM

Tsc, nevermind the allocation, after commeting it out, the same thing occurs, just two or three steps later... >_<

Any tip on how can I detect wheres the problem?


Edited by Icebone1000, 02 March 2014 - 02:08 AM.


#3 BitMaster   Crossbones+   -  Reputation: 3651

Like
4Likes
Like

Posted 02 March 2014 - 02:36 AM

Your class violates the Rule of Three (depending on your needs Rule of Five in C++11). Either the class must be noncopyable or implement sane copy semantics.

 

If you have access to C++11 I would rather suggest to use std::vector instead of an int* pointer in the class and trust the compiler-generated copy/move constructors/assignment operators.


Edited by BitMaster, 02 March 2014 - 03:01 AM.


#4 DaBono   Members   -  Reputation: 926

Like
1Likes
Like

Posted 02 March 2014 - 05:15 AM

You are storing your Tilemaps in a std::vector. As soon as the memory initially reserved in that vector runs out, it will reallocate.

It does this by creating new objects and destroying the old ones, causing your destructor to clean up the memory pointed to by pTileMap.

 

You can fix this in a number of ways depending on your situation:

 - Have your tile map not own the pointer, and make sure it gets cleaned up in another manner

 - Use a shared pointer instead of a raw one

 - Change the array of ints to a copyable structure, e.g. std::vector<int>.



#5 Icebone1000   Members   -  Reputation: 980

Like
0Likes
Like

Posted 02 March 2014 - 09:47 AM

The problem is not the pointer, damnit, I new I was confident with that pointer u_u*

 

Its more ninja than that..

 

check this out:

InstancedSprites ctor:

sprite::InstancedSprites::InstancedSprites()
	:
m_drawCall(6),
m_drawCall_warpException(6)
{
	m_drawable.AddPipelineState( &m_pipeState );
	m_drawable.SetDrawCall( &m_drawCall );

	m_drawable_warpException.AddPipelineState( &m_pipeState );
	m_drawable_warpException.SetDrawCall( &m_drawCall_warpException );
}

Now, what annoys me, is that everything inside m_drawable IS a std::vector...I cant quite tell exactly what happens that invalidates my shit.

Here how I "solved"

void sprite::InstancedSprites::Initialize( dx::BindPSShaderResourceView & pTextureBinder_p, dx::BindOMBlendState & pBlendBinder, dx::BindPSSampler & pSampleState, InstancesVertexBuffer & pIVB_p )
{
	// DBG
	m_drawable.Clear();
	m_drawable.AddPipelineState( &m_pipeState );
	m_drawable.SetDrawCall( &m_drawCall );
	m_drawable_warpException.Clear();
	m_drawable_warpException.AddPipelineState( &m_pipeState );
	m_drawable_warpException.SetDrawCall( &m_drawCall_warpException );


	m_pipeState.Reset();
	m_pipeState.AddBinderCommand( &pTextureBinder_p );
	m_pipeState.AddBinderCommand( &pBlendBinder );
	m_pipeState.AddBinderCommand( &pSampleState );
	m_pipeState.AddBinderCommand( pIVB_p.GetBinder() );
	// missing, IA and camera binds

	m_pIVBref = &pIVB_p;
}

See? I already had that method, but it didnt have that same code as the ctor (the upper part), this solved the issue, basicly, my drawables where getting invalid.

 

Oh yeah, deleting the pointer in the dctor was stupid, for sure ¦D, the thing is, Im only allocating a single map for now, its not that Im forcing reallocation on already build vector elements..at max Im forcing on the first/only one, but then, the ctor should be doing its job? I just dont get it @.@


Edited by Icebone1000, 02 March 2014 - 09:52 AM.


#6 Icebone1000   Members   -  Reputation: 980

Like
-1Likes
Like

Posted 02 March 2014 - 10:28 AM

Ok, the problem was the copy ctor, that didnt exist, so I basically create a cpy ctor that doesnt copy anything (act just like a normal ctor).

This worked.

 

The rule of tree is quite annoying, having to implement all those methods every time a new class is born? not knowing if you will or will not ever use it...whats the policy for this? do it fanatically no matter what?

I wish I could spot this kind of issue ahead of time and do it just when necessary like in this case.

 

Hmmm.. I think will create a macro that create private empty "rule of tree" methods and put that on every new class, so things like that will popup sooner, and then I can implement on demand..



#7 BitMaster   Crossbones+   -  Reputation: 3651

Like
0Likes
Like

Posted 02 March 2014 - 10:42 AM

Creating empty copy constructor/assignment operators (you need both) does not fix your problem. It hides your problems because undefined behavior sometimes appears to work in your favour.

 

Apart from the obvious (putting the data you're putting in pTileMap instead into a std::vector<int>) DaBono also offered other possibilities.

 

As a general rule, if you feel 'every of your classes' needs special work to stick to the Rule of Three (or Five), you are doing something very wrong with your design. In all normal cases the automatically generated copy constructor/assignment operator does the exactly right thing. You usually don't manage memory in classes, you use an std::container or some sort of smart pointer (boost::shared_ptr, boost::shared_array, boost::intrusive_ptr, std::shared_ptr, std::unique_ptr, ...).

If the standard language constructs aren't enough or you need something extremely specialized then you write a class (or template class) which implements that functionality. That class needs to obey the Rule of Three. Every class using it does so automatically.



#8 Brother Bob   Moderators   -  Reputation: 7782

Like
0Likes
Like

Posted 02 March 2014 - 10:43 AM

The rule of tree is quite annoying, having to implement all those methods every time a new class is born? not knowing if you will or will not ever use it...whats the policy for this? do it fanatically no matter what?
I wish I could spot this kind of issue ahead of time and do it just when necessary like in this case.
 
Hmmm.. I think will create a macro that create private empty "rule of tree" methods and put that on every new class, so things like that will popup sooner, and then I can implement on demand..

That's exactly what BitMaster said; either implement sane copy semantics or make it uncopyable. Unless I know for sure that my class will be trivially copyable from the very beginning, I almost always begin my class by inheriting from boost::noncopyable to ensure that my class doesn't get value semantics until I actually need it and go about to implement it.



#9 King Mir   Members   -  Reputation: 1910

Like
0Likes
Like

Posted 02 March 2014 - 10:53 AM

You could write a macro to delete the default copy and assignment operator, if that suits your favor. But it doesn't do any good to put in empty methods.

The rule of three doesn't say that every class needs those three functions. It says that if it needs one of the three, then it probably needs the other two.

#10 Icebone1000   Members   -  Reputation: 980

Like
0Likes
Like

Posted 02 March 2014 - 11:01 AM

Its not empty, its a copy  ctor that works like the already implemented ctor.

Im basically forcing the class to work inside vectors. (that Im assuming to require a copy ctor)

 

I will check on the implementation of boost::noncopyable.



#11 Icebone1000   Members   -  Reputation: 980

Like
0Likes
Like

Posted 02 March 2014 - 11:04 AM

You could write a macro to delete the default copy and assignment operator, if that suits your favor. But it doesn't do any good to put in empty methods.

The rule of three doesn't say that every class needs those three functions. It says that if it needs one of the three, then it probably needs the other two.

Yeah, but the issue is knowing when you going to need it, thus the private ones would show you instantly.



#12 Icebone1000   Members   -  Reputation: 980

Like
0Likes
Like

Posted 02 March 2014 - 11:28 AM

Thinking deeper on it, that class should be never copied..

 

See, the thing is, I merely wanted to allocate n tilemaps at beginning and then never mess with it again.

TileMap is a private struct, so its use is really scoped, its merely an aux.

There would never be the need of copying stuff around, if it where not by std::vector.

 

Copying InstancedSprites doesnt make any sense and should not be done. And it doesnt need to be done in my code.

 

Basically, the usage of std::vector was the problem in the first place. The fact that it have a friendly interface but with implicity complexities pisses me off.

Now, I dont want to have a cpy ctor on InstancedSprites, but I do want an array of tile maps, and I need to choose the size at initialization..What should I do?

Having used another raw  array (for TileMap) in the first place would have never caused me problems, isnt it ironic?



#13 rip-off   Moderators   -  Reputation: 7661

Like
3Likes
Like

Posted 02 March 2014 - 11:29 AM

Im basically forcing the class to work inside vectors. (that Im assuming to require a copy ctor)

In C++ parlance, std::vector requires that the object it stores are copyable. std::vector expects to be able to copy objects as it reallocates or if existing objects are added to it.

So yes, you'll need to ensure you are correctly handling the rule of three in order to use it with std::vector.

Yeah, but the issue is knowing when you going to need it...

C++ is a value oriented language by default. This means that unless you take care, C++ is going to create copies of things. In addition, as you found out here, C++ is going to generate copy constructors (and assignment operators) where possible to enable this behaviour.

As a result, you'll almost always need to think about whether you want to allow copying your classes. For simple classes like a Vector3, copying is perfectly reasonable behaviour. For "large" objects (either with lots of members, or where the members are large dynamic allocations) you probably want to be very careful about when you actually copy them. For instance, having a noncopyable implementation with an explicit member function to copy where really necessary. You should note that polymorphic objects also need a lot of care if you really need them to be copyable.

#14 rip-off   Moderators   -  Reputation: 7661

Like
0Likes
Like

Posted 02 March 2014 - 11:32 AM

What should I do?

I'd probably start with a std::vector of std::unique_ptr<TileMap>, and also make TileMap noncopyable to prevent mistakes.

Having used another raw array (for TileMap) in the first place would have never caused me problems, isnt it ironic?

Well, given that a raw array is totally different from a dynamic container, this isn't exactly a fair comparison. You could create a std::vector, reserve() sufficient memory and emplace_back() whatever instances you need.

In any case, you could still have a similar problem with a raw array if you accidentally assigned to or copied from the array.

#15 SiCrane   Moderators   -  Reputation: 9393

Like
3Likes
Like

Posted 02 March 2014 - 01:50 PM

Im basically forcing the class to work inside vectors. (that Im assuming to require a copy ctor)

In C++ parlance, std::vector requires that the object it stores are copyable. std::vector expects to be able to copy objects as it reallocates or if existing objects are added to it.

This changes with C++11, which allows non-copyable objects if they are moveable. Of course you would need to properly implement your move constructor and move assignment operators so the amount of work is about the same.

#16 Pink Horror   Members   -  Reputation: 1087

Like
0Likes
Like

Posted 02 March 2014 - 07:15 PM

Basically, the usage of std::vector was the problem in the first place. The fact that it have a friendly interface but with implicity complexities pisses me off.


You would prefer an unfriendly interface? I'd glad you didn't write the STL.

#17 King Mir   Members   -  Reputation: 1910

Like
0Likes
Like

Posted 02 March 2014 - 10:04 PM

Its not empty, its a copy  ctor that works like the already implemented ctor.

Im basically forcing the class to work inside vectors. (that Im assuming to require a copy ctor)

A copy constructor is defaultly generated for you, so there's no point in writing it out yourself if there's no difference from the default behavior. Which is enough if your class doesn't manage a resource.

 

 

You could write a macro to delete the default copy and assignment operator, if that suits your favor. But it doesn't do any good to put in empty methods.

The rule of three doesn't say that every class needs those three functions. It says that if it needs one of the three, then it probably needs the other two.

Yeah, but the issue is knowing when you going to need it, thus the private ones would show you instantly.

 

You need it when your class manages a resource. For example, when you dynamically allocate an array. You should use the most narrow class you can to manage each resource, with at most one resource per class. Dynamic arrays are almost never necessary because you can use std::vector.

 

Thinking deeper on it, that class should be never copied..

 

See, the thing is, I merely wanted to allocate n tilemaps at beginning and then never mess with it again.

TileMap is a private struct, so its use is really scoped, its merely an aux.

There would never be the need of copying stuff around, if it where not by std::vector.

 

Copying InstancedSprites doesnt make any sense and should not be done. And it doesnt need to be done in my code.

 

Basically, the usage of std::vector was the problem in the first place. The fact that it have a friendly interface but with implicity complexities pisses me off.

Now, I dont want to have a cpy ctor on InstancedSprites, but I do want an array of tile maps, and I need to choose the size at initialization..What should I do?

Having used another raw  array (for TileMap) in the first place would have never caused me problems, isnt it ironic?

You should have an std::array<TileMap> each tile having a std::vector<int>. You should also avoid raw pointers that signify ownership. Prefer std::vector or smart pointers like std::unique_ptr.


Edited by King Mir, 02 March 2014 - 10:08 PM.





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