memory leak problem...

[gampalu]

Hi!

I built this class, however when I executed my program the memory run out quickly, because I wasn't making any "deletes", so I added the delete in the destructor, but It crashes there... what's wrong?

class CMatrix  
{
public:
	void Identity();
	void SetMaxColRow(int nRow, int nCol);
	void SetElement(int nRow, int nCol, long double element);
	void Show();

	long double GetElement(int nRow, int nCol);

	CMatrix operator+(CMatrix tmpMatriz);
	CMatrix operator-(CMatrix tmpMatriz);
	CMatrix operator*(long double scalar);
	CMatrix operator*(CMatrix tmpMatriz);
	CMatrix Transpose();
	void CMatrix::Invert();
	CMatrix GetInverted() const;
	CMatrix(int nRows, int nCols);
	CMatrix();
	
	virtual ~CMatrix();
	
	int m_nRows;
	int m_nCols;
	

private:
	long double *m_pMatriz;
		
};

//////////////////////////////////////////////////////////////////////
// Construction/Destruction
//////////////////////////////////////////////////////////////////////

// constructor
CMatrix::CMatrix(int nRows, int nCols)
{
	SetMaxColRow(nRows, nCols);
}

// constructor
CMatrix::CMatrix()
{
	m_nCols = NULL;
	m_nRows = NULL;
	m_pMatriz = NULL;
}


// destructor
CMatrix::~CMatrix()
{
	if (m_pMatriz != NULL)
		delete[] m_pMatriz;
}

[Salem]

What does SetMaxColRow() actually do?

If you just construct and destruct this object in a test program, does it still crash?

Have you looked at what other things are doing elsewhere? Just because it crashes here doesn't mean the problem is here, when dealing with dynamic memory.

[Ancient Dragon]

Hi!

I built this class, however when I executed my program the memory run out quickly, because I wasn't making any "deletes", so I added the delete in the destructor, but It crashes there... what's wrong?

Nothing wrong with what you posted -- its impossible to tell what the proglem is because you didn't post enough code. Check for (1) writing outside allocated array bounds, (2) using uninitialized pointers.

[gampalu]

What does SetMaxColRow() actually do?

Here it is the code:

void CMatrix::SetMaxColRow(int nRows, int nCols)
{
	m_nCols = nCols;
	m_nRows = nRows;
	
	m_pMatriz = new long double[m_nRows * m_nCols];
	memset(m_pMatriz, 0.0, sizeof(long double) * m_nRows * m_nCols); 
}

Am I doing the correct delete , concerning the "new's" I made?

If you just construct and destruct this object in a test program, does it still crash?

I haven't tried that yet... I'll try now!

Have you looked at what other things are doing elsewhere? Just because it crashes here doesn't mean the problem is here, when dealing with dynamic memory.

I have looked arround the code and found no problems, at least for me, an inexperienced programmer...

[Salem]

> m_nCols = NULL;
> m_nRows = NULL;
NULL typically signifies a pointer, so this is sending the wrong message to the reader
Just do

m_nCols = 0;
	m_nRows = 0;

> memset(m_pMatriz, 0.0, sizeof(long double) * m_nRows * m_nCols);
http://c-faq.com/malloc/calloc.html
Filling the memory with 0 (even though you said 0.0, this is not what you get) does not guarantee a useful floating point value.
A simple loop is enough

int lim = m_nRows * m_nCols;
for ( int i = 0 ; i < lim ; i++ ) m_pMatriz[i] = 0.0;

> I have looked arround the code and found no problems, at least for me, an inexperienced programmer
Without lots of tools, pros find this hard to do as well.
Lots of testing at all stages, and walkthroughs with the debugger help you to understand what's really happening.

[gampalu]

Nothing wrong with what you posted -- its impossible to tell what the proglem is because you didn't post enough code. Check for (1) writing outside allocated array bounds, (2) using uninitialized pointers.

(1) Could be this, but I don't see how... :sad:

(2) I haven't uninitialized pointers.

I'll put all the code of the class:

// constructor
CMatrix::CMatrix(int nRows, int nCols)
{
	SetMaxColRow(nRows, nCols);
}

// constructor
CMatrix::CMatrix()
{
	m_nCols = 0;
	m_nRows = 0;
	m_pMatriz = NULL;
}


// destructor
CMatrix::~CMatrix()
{
	if (m_pMatriz != NULL)
		delete[] m_pMatriz;

	m_pMatriz = NULL;
}


// adding 2 matrixes
CMatrix CMatrix::operator+(CMatrix tmpMatriz)
{
	// first check for same size
	if ((m_nCols != tmpMatriz.m_nCols) || (m_nRows != tmpMatriz.m_nRows))
	{
		printf("\nError in sum: Matrices do not have same size.\n\n");
		exit(1);
	}

	CMatrix ResMatriz(m_nRows,m_nCols);
	
	// now add in the other matrix
	for (int i=0; i<m_nRows; i++)
	{
		for (int j=0; j<m_nCols; j++)
			ResMatriz.SetElement(i,j, *(m_pMatriz + i * m_nCols + j) + tmpMatriz.GetElement(i,j));
	}
	return ResMatriz;
}


// subtracting 2 matrixes
CMatrix CMatrix::operator-(CMatrix tmpMatriz)
{
	// first check for same size
	if ((m_nCols != tmpMatriz.m_nCols) || (m_nRows != tmpMatriz.m_nRows))
	{
		printf("\nError in subtract: Matrices do not have same size.\n\n");
		exit(1);
	}
		
	CMatrix ResMatriz(m_nRows, m_nCols);
	
	for (int i=0; i<m_nRows; i++)
		for (int j=0; j<m_nCols; j++)
			ResMatriz.SetElement(i,j, *(m_pMatriz + i * m_nCols + j) - tmpMatriz.GetElement(i,j));
	
	return ResMatriz;
}


// multiplication of a scalar with a matrix
CMatrix CMatrix::operator*(long double scalar)
{
	CMatrix ResMatriz(m_nRows, m_nCols);
	
	for (int i=0; i<m_nRows; i++)
		for (int j=0; j<m_nCols; j++)
			*(ResMatriz.m_pMatriz + i * m_nCols + j) = scalar * (*(m_pMatriz + i * m_nCols + j));
	
	return ResMatriz;
}


// multiplication of 2 matrixes
CMatrix CMatrix::operator*(CMatrix tmpMatriz)
{
	// first check for a valid multiplication operation
	if (m_nCols != tmpMatriz.m_nRows)
	{
		printf("\nError in multipication: Matrices do not have common size.\n\n");
		exit(1);
	}
			
	// construct the object we are going to return
	CMatrix ResMatriz(m_nRows, tmpMatriz.m_nCols);

	// e.g.
	// [A][B][C]   [G][H]     [A*G + B*I + C*K][A*H + B*J + C*L]
	// [D][E][F] * [I][J] =   [D*G + E*I + F*K][D*H + E*J + F*L]
	//             [K][L]
	//
	
	
	long double	value = 0;
	for (int i=0; i<ResMatriz.m_nCols; i++)
		for (int j=0; j<ResMatriz.m_nRows; j++)
		{
			value = 0.0 ;
			for (int k=0; k<m_nCols; k++)
				value += *(m_pMatriz + j * m_nCols + k) * tmpMatriz.GetElement(k,i);
			
			ResMatriz.SetElement(j, i, value) ;
		}
	
	return ResMatriz;
}



// sets a specific element
void CMatrix::SetElement(int nRow, int nCol, long double element)
{
	if ((nCol >= m_nCols) || (nRow >= m_nRows) || (nRow < 0 ) || (nCol < 0))
	{
		printf("\nError in SetElement.\n\n");
		exit(1);
	}

	*(m_pMatriz + nRow * m_nCols + nCol) = element;
	
}


// gets one specific element
long double CMatrix::GetElement(int nRow, int nCol)
{
	long double element = 0;

	if ((nCol >= m_nCols) || (nRow >= m_nRows) || (nRow < 0 ) || (nCol < 0))
	{
		printf("\nError in GetElement.\n\n");
		exit(1);
	}

	
	element = *(m_pMatriz + nRow * m_nCols + nCol);
	
	return element;
}



// prints a matrix
void CMatrix::Show()
{
	for (int i=0; i<m_nRows; i++)
	{
		for (int j=0; j<m_nCols; j++)
			printf("%8lf\t", *(m_pMatriz + i * m_nCols + j));
		printf("\n");
	}
	printf("\n");
}

void CMatrix::SetMaxColRow(int nRows, int nCols)
{
	m_nCols = nCols;
	m_nRows = nRows;
	
	m_pMatriz = new long double[m_nRows * m_nCols];
	
	int lim = m_nRows * m_nCols;
	for ( int i = 0 ; i < lim ; i++ )
		m_pMatriz[i] = 0.0;
}

void CMatrix::Identity()
{
	// first check if matrix is square
	if (m_nCols != m_nRows)
	{
		printf("\nMatrix is not square.\n\n");
		exit(1);
	}
	
	for (int i=0; i<m_nRows; i++)
		for (int j=0; j<m_nRows; j++)
			*(m_pMatriz + i * m_nCols + j) = 0.0;

	//memset(m_pMatriz, 0.0, sizeof(long double)  * m_nRows * m_nCols); 
	
	for (int k=0; k<m_nRows; k++)
		*(m_pMatriz + k * m_nCols + k) = 1.0;

}



// matrix tranpose
CMatrix CMatrix::Transpose()
{
	// construct the object we are going to return
	CMatrix ResMatriz(m_nCols, m_nRows);
	
	// copy across the transposed data
	for (int i=0; i<m_nCols; i++)
		for (int j=0; j<m_nRows; j++)
			ResMatriz.SetElement(i, j, *(m_pMatriz + j * m_nCols + i));
	
	return ResMatriz;
}



CMatrix CMatrix::GetInverted() const
{
	// matrix inversion will only work on square matrices
	if (m_nCols != m_nRows)
	{
		printf("\nMatrix is not square.\n\n");
		exit(1);
	}

	// construct the object we are going to return
	CMatrix ResMatriz(m_nCols, m_nCols);
	
	// return this matrix inverted	
	CMatrix	copy(*this);
	copy.Invert();

	return copy;
}

void CMatrix::Invert()
{
	// matrix inversion will only work on square matrices
	// invert ourselves
	if (m_nCols != m_nRows)
	{
		printf("\nMatrix is not square.\n\n");
		exit(1);
	}
	long double	e = 0;

	for (int k = 0 ; k < m_nCols ; ++k)
	{
		e = GetElement(k, k) ;
		SetElement(k, k, 1.0) ;
		if (e == 0.0)
		{
			printf("\nMatrix inversion error\n");
			exit(1);
		}
			
		for (int j = 0 ; j < m_nCols ; ++j)
			SetElement(k, j, GetElement(k, j) / e) ;
		for (int i = 0 ; i < m_nCols ; ++i)
		{
			if (i != k)
			{
				e = GetElement(i, k) ;
				SetElement(i, k, 0.0) ;
				for (j = 0 ; j < m_nCols ; ++j)
					SetElement(i, j, GetElement(i, j) - e * GetElement(k, j)) ;
			}
		}
	}
}

Thanks in advance for helping

[gampalu]

> m_nCols = NULL;
> m_nRows = NULL;
NULL typically signifies a pointer, so this is sending the wrong message to the reader
Just do

m_nCols = 0;
	m_nRows = 0;

Thanks for the tip, I've already correct it in other places ;)

> memset(m_pMatriz, 0.0, sizeof(long double) * m_nRows * m_nCols);
http://c-faq.com/malloc/calloc.html
Filling the memory with 0 (even though you said 0.0, this is not what you get) does not guarantee a useful floating point value.
A simple loop is enough

int lim = m_nRows * m_nCols;
for ( int i = 0 ; i < lim ; i++ ) m_pMatriz[i] = 0.0;

Didn't know that memset wasn't reliable, updated ;)

> I have looked arround the code and found no problems, at least for me, an inexperienced programmer
Without lots of tools, pros find this hard to do as well.
Lots of testing at all stages, and walkthroughs with the debugger help you to understand what's really happening.

I've tried to test it with a single object as show and it worked, but not in my program...

int main(int argc, char* argv[])
{
	
	CMatrix m1(2,2);
	m1.Identity();
	m1.Show();
	return 0;
}

output:

1.000000        0.000000
0.000000        1.000000

Press any key to continue

[Salem]

> *(m_pMatriz + k * m_nCols + k) = 1.0;
In many places in your class member functions, you have this complex row/col calculation.
In other places, you call SetElement() and GetElement()

With the exception of the Set and Get functions, nothing should be aware of how the data is actually accessed. That is, all your other member functions should use the Set and Get methods.

Which OS and compiler are you using?
For Linux, consider using 'valgrind' or 'electric fence'

#include <iostream>

int main ( ) {
  int *mem = new int[5];
  for ( int i = 0 ; i <= 5 ; i++ ) {  // oops, 1 too far
    mem[i] = 0;
  }
  delete [] mem;
  return 0;
}

$ g++ foo.cpp
$ ./a.out
*** glibc detected *** ./a.out: free(): invalid next size (fast): 0x091ed008 ***
======= Backtrace: =========
/lib/libc.so.6[0x5b4424]
/lib/libc.so.6(__libc_free+0x77)[0x5b495f]
/usr/lib/libstdc++.so.6(_ZdlPv+0x21)[0x9a2669]
/usr/lib/libstdc++.so.6(_ZdaPv+0x1d)[0x9a26b5]
./a.out(__gxx_personality_v0+0x134)[0x80485c4]
/lib/libc.so.6(__libc_start_main+0xc6)[0x565de6]
./a.out(__gxx_personality_v0+0x51)[0x80484e1]
======= Memory map: ========
0052f000-00549000 r-xp 00000000 fd:00 5342221    /lib/ld-2.3.5.so
00549000-0054a000 r-xp 00019000 fd:00 5342221    /lib/ld-2.3.5.so
0054a000-0054b000 rwxp 0001a000 fd:00 5342221    /lib/ld-2.3.5.so
00551000-00675000 r-xp 00000000 fd:00 5342973    /lib/libc-2.3.5.so
00675000-00677000 r-xp 00124000 fd:00 5342973    /lib/libc-2.3.5.so
00677000-00679000 rwxp 00126000 fd:00 5342973    /lib/libc-2.3.5.so
00679000-0067b000 rwxp 00679000 00:00 0
0067d000-0069f000 r-xp 00000000 fd:00 5342974    /lib/libm-2.3.5.so
0069f000-006a0000 r-xp 00021000 fd:00 5342974    /lib/libm-2.3.5.so
006a0000-006a1000 rwxp 00022000 fd:00 5342974    /lib/libm-2.3.5.so
008ac000-008b5000 r-xp 00000000 fd:00 5342977    /lib/libgcc_s-4.0.0-20050520.so.1
008b5000-008b6000 rwxp 00009000 fd:00 5342977    /lib/libgcc_s-4.0.0-20050520.so.1
008b6000-008b7000 r-xp 008b6000 00:00 0
008ee000-009cd000 r-xp 00000000 fd:00 9460718    /usr/lib/libstdc++.so.6.0.4
009cd000-009d2000 rwxp 000df000 fd:00 9460718    /usr/lib/libstdc++.so.6.0.4
009d2000-009d7000 rwxp 009d2000 00:00 0
08048000-08049000 r-xp 00000000 fd:00 7274627    /home/me/a.out
08049000-0804a000 rw-p 00000000 fd:00 7274627    /home/me/a.out
091ed000-0920e000 rw-p 091ed000 00:00 0          [heap]
b7e00000-b7e21000 rw-p b7e00000 00:00 0
b7e21000-b7f00000 ---p b7e21000 00:00 0
b7f38000-b7f3a000 rw-p b7f38000 00:00 0
b7f5d000-b7f5e000 rw-p b7f5d000 00:00 0
bfe49000-bfe5e000 rw-p bfe49000 00:00 0          [stack]
Aborted

$ valgrind ./a.out
==29215== Memcheck, a memory error detector for x86-linux.
==29215== Copyright (C) 2002-2005, and GNU GPL'd, by Julian Seward et al.
==29215== Using valgrind-2.4.0, a program supervision framework for x86-linux.
==29215== Copyright (C) 2000-2005, and GNU GPL'd, by Julian Seward et al.
==29215== For more details, rerun with: -v
==29215==
==29215== Invalid write of size 4
==29215==    at 0x80485A2: main (in /home/me/a.out)
==29215==  Address 0x1B93503C is 0 bytes after a block of size 20 alloc'd
==29215==    at 0x1B9095DA: operator new[](unsigned) (vg_replace_malloc.c:138)
==29215==    by 0x8048589: main (in /home/me/a.out)
==29215==
==29215== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 16 from 1)
==29215== malloc/free: in use at exit: 0 bytes in 0 blocks.
==29215== malloc/free: 1 allocs, 1 frees, 20 bytes allocated.
==29215== For counts of detected errors, rerun with: -v
==29215== No malloc'd blocks -- no leaks are possible.

$ g++ -g foo.cpp -lefence
$ gdb ./a.out
GNU gdb Red Hat Linux (6.3.0.0-1.21rh)
Copyright 2004 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "i386-redhat-linux-gnu"...Using host libthread_db library "/lib/libthread_db.so.1".

(gdb) run
Starting program: /home/me/a.out
Reading symbols from shared object read from target memory...done.
Loaded system supplied DSO at 0x135000

  Electric Fence 2.2.0 Copyright (C) 1987-1999 Bruce Perens <[e-mail]>

Program received signal SIGSEGV, Segmentation fault.
0x0804869a in main () at foo.cpp:6
6           mem[i] = 0;
(gdb) quit
The program is running.  Exit anyway? (y or n) y
$

> int main
It would be really useful if you could produce a simple main() which did show up a problem.

[gampalu]

> *(m_pMatriz + k * m_nCols + k) = 1.0;
In many places in your class member functions, you have this complex row/col calculation.
In other places, you call SetElement() and GetElement()

With the exception of the Set and Get functions, nothing should be aware of how the data is actually accessed. That is, all your other member functions should use the Set and Get methods.

I understand your point, but there's one problem, the variable m_pMatriz is double* and the GetElement and SetElement are methods for CMatrix types... So I can't use it, right? At least not directly... But how I don't know...

Which OS and compiler are you using?

Windows XP

#include <iostream>

int main ( ) {
  int *mem = new int[5];
  for ( int i = 0 ; i <= 5 ; i++ ) {  // oops, 1 too far
    mem[i] = 0;
  }
  delete [] mem;
  return 0;
}

I've tested this exact code and I now understand the problem when we allocate less space than necessary ;)
So the problem should be that, having nothing to do with delete!

How do you suggest me to find?

Thank you for all your attention and patiente :)

[Salem]

> the variable m_pMatriz is double* and the GetElement and SetElement are methods for CMatrix types
But those functions take and return doubles, so there should be no problem.
They're used fine in some contexts already anyway.

I've attached your code with Set/Get used in many more places, and it compiles fine here.
The //!! comments are what my g++ compiler spotted with
$ g++ -c -W -Wall -ansi -pedantic -O2 foo.cpp

Oh, you might want to rename all your loop variables from i,j to be say r,c to denote row and column. It's not always clear whether you're accessing the correct element of the array (when you're doing it directly), and r/c would make it more readable IMO.
At least by always using the set/get functions, every access is properly range checked (as it should be).

[gampalu]

the variable m_pMatriz is double* and the GetElement and SetElement are methods for CMatrix types
But those functions take and return doubles, so there should be no problem.
They're used fine in some contexts already anyway.
I've attached your code with Set/Get used in many more places, and it compiles fine here.
The //!! comments are what my g++ compiler spotted with
$ g++ -c -W -Wall -ansi -pedantic -O2 foo.cpp

Thanks :)

Oh, you might want to rename all your loop variables from i,j to be say r,c to denote row and column. It's not always clear whether you're accessing the correct element of the array (when you're doing it directly), and r/c would make it more readable IMO.

Yes you're right.

At least by always using the set/get functions, every access is properly range checked (as it should be).

After making changes my problem continues :sad:
I've tried to put the delete in some methods and commented the delete in the destructor, however wtihout sucess...

what do you suggest?
Sorry if I'm getting boring to you...

thanks

[Salem]

Post your latest - can you make a simple example fail yet?