Skip to main content
gamedev.net
🔒 Locked

Dynamic Memory Problems

Started by Downie Mar 3, 2009 at 6:09 AM 12 replies 1.1k views
Original Post
Downie
Downie
Hey all. I am having a memory corruption problem and I'm not sure why. I have a linked list containing pointers to a struct (Not PoD's). (I use my own linked list but the stl list doesnt fix the problem.) The values on the list are correct and everything seems to work fine. I then want to grab the list data using my return function which returns the pointer to the struct. Struct* Temp = new Struct(); Temp = List.GetBack(); However as soon as I use 'new' the list pointers become corrupt. This happens no matter how I assign new memory, either using malloc() or even when doing std::vector.reserve(); I have used a memmgr to check all my new calls and they all have matching deletes and the mgr tells me I have no memory leaks so I really don't know what the problem is.
Black Knight
Black Knight
Quote:
Original post by Downie
Struct* Temp = new Struct();(1)
Temp = List.GetBack();(2)


You are allocating memory at (1) and then pointing the pointer to what List.GetBack() returns at(2),that means you can't free the memory you allocated at (1).
Are you trying something like this :

Struct* tmp = List.GetBack();
tmp->doStuff();

Seeing more code would be helpful.
Downie
Downie

Quote:
You are allocating memory at (1) and then pointing the pointer to what List.GetBack() returns at(2),that means you can't free the memory you allocated at (1).
Are you trying something like this :

Struct* tmp = List.GetBack();
tmp->doStuff();


I realised that pointing to the pointer would cause issues. Yeah I would be using the pointer in that function which I realise would change the values in the list; would using a copy ctor to make a deep copy of it work?

An outline of what I am doing -

Temp = List.GetBack();

VertexStruct Vertex;
Vertex.Data = Temp->Data;
vector.push_back(Vertex);

Anyway the problem arises regardless of whether I assign the pointer to the list item. It is as soon as I use new.

Struct* temp = new Struct; //As soon as this line is executed the list becomes corrupt
Black Knight
Black Knight
Well,yes if you have multiple pointers which all point to the same area of memory and when you change the contents, all pointers will now see the modified values.A more dangerous thing is if you free the memory by one of the pointers,the other pointers will not point to thin air and become dangling pointers.

Can you post the whole method instead of bits of code.You can use the source tags to post code.
Downie
Downie
Cheers for taking the time to have a look at this. I know that storing pointers to pointers and then using them is bad but I don't know the best way to get round it. I am trying to take data from one list and put it into a vector so that I can render it using a vertex buffer. The vertex buffer needs only the position vector stored in the list and this is what I am trying to access. Oh I only delete the list pointers in the destructor.


void CWorldTerrain::UpdateVertexArray(){   LODTri* pTri = new LODTri; //CORRUPTS HERE   //Normalise all the position vectors, multiply them by the Radius to maintain sphere shape and add them to the list.    if(!mVertexList.empty())   {     mVertexList.clear();   }   //Allocate the vector enough memory   mVertexList.reserve(mNumVerts);  //CORRUPTS HERE ASWELL   for(int i=0; i<mTriList.Size(); i++)   {      //Grab the next triangle      pTri = mTriList.GetByIndex(i);	for(int j=0; j<3; j++)	{	  Vertex.Pos = pTri->Pos[j];	//Make the position vector of unit length; as they are all different lengths the sphere will become distorted.	  Normalise(&Vertex.Pos, &Vertex.Pos);	  Vertex.Pos *= RADIUS;	  //Set the texture coordinates 	  Vertex.TexCoord = D3DXVECTOR2(0, 1);	  //Calculate the normals for the faces of triangles	  CrossProduct(&Vertex.Normal, &pTri->Pos[0], &pTri->Pos[2]);	  //Add the vertices to a vector array so that they can be copied into the VB	  mVertexList.push_back(Vertex); //CORRUPTS HERE	}    } }
Evil Steve
Evil Steve
And where is mTriList set up? It looks to me like you have heap corruption, or more likely as Black Knight said; you've deleted a pointer but still have a reference to it somewhere.
Black Knight
Black Knight
Ok couple of things I see; first of all I don't see what type is Vertex and where it is defined.What does the contructor of LODTri do?
Also if you are storing elements in a vector by value the objects you store should have correct copy contructors if they contain data on the heap,because the vector may copy elements around when you add new ones.

Again you don't need to create the LODTri with new.

void CWorldTerrain::UpdateVertexArray(){   //Normalise all the position vectors, multiply them by the Radius to maintain sphere shape and add them to the list.    if(!mVertexList.empty())   {     mVertexList.clear();   }   //Allocate the vector enough memory   mVertexList.reserve(mNumVerts);  //CORRUPTS HERE ASWELL      for(int i=0; i<mTriList.Size(); i++)   {      //Grab the next triangle      LODTri* pTri = mTriList.GetByIndex(i);	for(int j=0; j<3; j++)	{	 ///where does vertex come from?         SomeStruct Vertex;       Vertex.Pos = pTri->Pos[j];	//Make the position vector of unit length; as they are all different lengths the sphere will become distorted.	  Normalise(&Vertex.Pos, &Vertex.Pos);	  Vertex.Pos *= RADIUS;	  //Set the texture coordinates 	  Vertex.TexCoord = D3DXVECTOR2(0, 1);	  //Calculate the normals for the faces of triangles	  CrossProduct(&Vertex.Normal, &pTri->Pos[0], &pTri->Pos[2]);	  //Add the vertices to a vector array so that they can be copied into the VB	  mVertexList.push_back(Vertex); //CORRUPTS HERE	}    } }





Downie
Downie
Quote:
Original post by Evil Steve
And where is mTriList set up? It looks to me like you have heap corruption, or more likely as Black Knight said; you've deleted a pointer but still have a reference to it somewhere.


mTriList is a memeber of the WorldTerrain class.

CLinkedList<LODTri*> mTriList;void CWorldTerrain::AddTriangleToList(LODTri* pTri){  mTriList.PushBack(pTri);  mNumVerts = int(mTriList.Size() * 3);}void CWorldTerrain::BuildBaseShape(){  LODTri Tri[20];  Tri[0].Pos[0]  = D3DXVECTOR3(0, b, -a);  Tri[0].Pos[1]  = D3DXVECTOR3(b, a, 0);   Tri[0].Pos[2]  = D3DXVECTOR3(-b, a, 0); //................... etc for(int i=o; i<20; i++)   AddTriangleToList(&Tri);}
Evil Steve
Evil Steve
Quote:
Original post by Downie
Quote:
Original post by Evil Steve
And where is mTriList set up? It looks to me like you have heap corruption, or more likely as Black Knight said; you've deleted a pointer but still have a reference to it somewhere.


mTriList is a memeber of the WorldTerrain class.

CLinkedList<LODTri*> mTriList;void CWorldTerrain::AddTriangleToList(LODTri* pTri){  mTriList.PushBack(pTri);  mNumVerts = int(mTriList.Size() * 3);}void CWorldTerrain::BuildBaseShape(){  LODTri Tri[20];  Tri[0].Pos[0]  = D3DXVECTOR3(0, b, -a);  Tri[0].Pos[1]  = D3DXVECTOR3(b, a, 0);   Tri[0].Pos[2]  = D3DXVECTOR3(-b, a, 0); //................... etc for(int i=o; i<20; i++)   AddTriangleToList(&Tri);}
BuildBaseShape() calls AddTriangleToList() with the address of a variable on the stack. Since AddTriangleToList() doesn't copy the contents of the pointer, only the pointer itself, the contents are destroyed as soon as BuildBaseShape() returns.

Is there a reason you need to store a vector of LODTri's by pointer instead of value?
Black Knight
Black Knight
Ok you are declaring 20 on stack but after that method ends those are freed and your list points to nothing.(ninjaed big time)
Downie
Downie
Quote:
Original post by Black Knight
Ok couple of things I see; first of all I don't see what type is Vertex and where it is defined.What does the contructor of LODTri do?
Also if you are storing elements in a vector by value the objects you store should have correct copy contructors if they contain data on the heap,because the vector may copy elements around when you add new ones.

Again you don't need to create the LODTri with new.


Vertex is a member of the class. struct VertexData{  D3DXVECTOR3 Pos, Normal;  D3DXVECTOR2 Texcoordl}////////////////////private:VertexData Vertex;


It is a default constructor, but I am implementing a copy constructor and copy assignment for LODTri struct.
rip-off
rip-off
You are taking the address of a temporary and storing it outside the scope of the object. The LODTri array in BuildBaseShape() are destroyed when the function ends and pointers to elements inside it become invalidated.

I don't think you need to use pointers here at all. It adds complexity without benefits.
// What is CLinkedList, why not std::list?std::list<LODTri> mTriList;void CWorldTerrain::BuildBaseShape(){    LODTri Tri[20];    Tri[0].Pos[0]  = D3DXVECTOR3(0, b, -a);    Tri[0].Pos[1]  = D3DXVECTOR3(b, a, 0);    Tri[0].Pos[2]  = D3DXVECTOR3(-b, a, 0);    //................... etc    for(int i= 0; i< 20; i++)    {        mTriList.push_back(tri);    }}

And you don't need to store the num vertices if you can calculate it from the number of triangles:
class CWorldTerrain{private:    std::list<LODTri> mTriList;    int numVertices() const { return mTriList.size() * 3; } };
blog | twitter
Downie
Downie
Quote:

BuildBaseShape() calls AddTriangleToList() with the address of a variable on the stack. Since AddTriangleToList() doesn't copy the contents of the pointer, only the pointer itself, the contents are destroyed as soon as BuildBaseShape() returns.

Is there a reason you need to store a vector of LODTri's by pointer instead of value?


Ah of course, the 20 base triangles will be destroyed when the function returns. I want to store them by value on the vector, but they need to be stored by pointer on the list. I want to take the data that the list points too...make a copy of it and push it onto the vector by value.
Downie
Downie
Quote:
Rip-Off:
I don't think you need to use pointers here at all. It adds complexity without benefits.


It needs to be quick so I though pointers would boost the performance of my recursive split function.

Anyway the problem was that I was declaring the base triangles locally. Thanks for all the help getting that bit solved. I had been looking at the code for so long I couldnt see the wood for the trees if you know what I mean.

Topic Locked

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

Sign in to reply to this topic.