|
Memory leaks according to _CrtDumpMemoryLeaks |
TeaRDoWN
Member #8,518
April 2007
|
I get memory leak detection with _CrtDumpMemoryLeaks() even after using free() in the char*. What am I doing wrong or is the dump just not reading the memory correct? I get a memory dump similar to this: Detected memory leaks! Dumping objects -> {544042} normal block at 0x122FB3E0, 256 bytes long. Data: < > CD CD CD CD CD CD CD CD CD CD CD CD CD CD CD CD Object dump complete. Another related question, does anyone know how to put a breakpoint on a memory allaction number? I tried to follow the steps on MSDN (http://msdn.microsoft.com/en-us/library/w2fhc9a3.aspx) but I don't know if I'm doing it wrong or whatever. Using Visual C++ 2008 Express edition. |
Sol Blast
Member #9,655
April 2008
|
It looks like the 'text' you're dynamically allocating is a local. You're then freeing the unused global variable 'mLog.text' which you've not yet allocated anything too, while the local hangs on to the memory. Maybe you meant your code to look like this? (i'm not familiar with malloc though). Maybe i'm wrong. Like i said, i'm not familiar with C. |
Ron Novy
Member #6,982
March 2006
|
What does the '...' mean? Post code. ---- |
TeaRDoWN
Member #8,518
April 2007
|
Sol Blast: If everything was public it could be done like that. But I want log() to be the only function that modifies the content of the mLog.text value. Ron Novy: Actually the code is much more complex but it cut it out to try to make it easier to gasp. Therefor the "..." in the middle of the main - but you could actually ignore this. Thought that 17 lines of code that causes the same error as 100 lines were easier to discuss. Sorry for the confusion... |
Sol Blast
Member #9,655
April 2008
|
TeaRDoWN
Member #8,518
April 2007
|
Sol Blast: But if I do that then the mLog.text will be cleared too and I want to store the data in that variable. Ok here is basically the entire code, at least more detailed. 1// game.c content
2LOG {
3 char* text[50];
4};
5
6LOG mLog;
7
8void log(char* text)
9{
10 for(int l = 49; l > 0; l--)
11 {
12 mLog.text[l] = mLog.text[l-1];
13 }
14 mLog.text = text;
15}
16
17void clearLog()
18{
19 for (int l = 0; l < 50; l++)
20 free(mLog.text[l]);
21}
22
23void main()
24{
25 ...
26 SND_playSample(snd_id)
27 ...
28 clearLog();
29}
30END_OF_MAIN()
31
32
33
34
35// sound.c content (no able to access the mLog struct, only log())
36void SND_playSample(int id)
37{
38 if (mSnd[id] == NULL)
39 {
40 char *text = (char*)malloc(sizeof(text)*64);
41 sprintf(text, "An error has occured trying to play sound #%i", id);
42 log(text);
43 }
44 else
45 playSample(mSnd[id]);
46 }
47}
|
BAF
Member #2,981
December 2002
|
Ah, the masochistic pleasures of coding in C. |
Sol Blast
Member #9,655
April 2008
|
TeaRDoWN said: But if I do that then the mLog.text will be cleared too and I want to store the data in that variable. Ah i see. Well if you really want to do it that way, then you'd probably be better having mLog responsible for freeing its own memory, rather than doing it yourself. Personally though i'd copy the memory like so... 1char* mLog.text;
2
3void log( char* text )
4{
5 for(int l = 49; l > 0; l--)
6 {
7 mLog.text[l] = mLog.text[l-1];
8 } //I don't really understand why you do this...
9
10 sprintf(mLog.text, text ); //Instead of referencing it. Copy it. This gives the source of 'str' responsibility for freeing the memory instead of mLog itself.
11}
12
13main()
14{
15char* text = (char*)malloc(sizeof(char)*64); //New memory is allocated.
16sprintf(text, "asdf");
17log(text); //Duplicate it into mLog.text.
18...
19free(text); //Since the string memory has been duplicated, we no longer need to hold on to the local. So free it. mLog.text will not be affected.
20_CrtDumpMemoryLeaks();
21}
I think that'd solve the problem... |
TeaRDoWN
Member #8,518
April 2007
|
BAF: Thanks I'll try that. Sol Blast: Actually, I tried that before but got the same result. That's when I got really confused and changed it back since the code for this is less complex. |
LennyLen
Member #5,313
December 2004
|
Sol Blast
Member #9,655
April 2008
|
I'll bear that in mind. Thanks LennyLen . |
GullRaDriel
Member #3,861
September 2003
|
Nonconstructive comment #2635267 for BAF. TeaRDoWN, when you copy your pointer mLog.text to mLog.text - 1, you don't move the data, just the pointer. Don't checking for the data who'll be out of range is a mistake. I am making you a C version which (I hope) will be without memory leak. I'll test it with Valgrind and dbx. "Code is like shit - it only smells if it is not yours" |
TeaRDoWN
Member #8,518
April 2007
|
Ok, changed this:
Still get the same memory leak reports: Did anyone know how to breakpoint a memory allocation? It should be possible to use the allocation id (544044) and break at the memory leak IF you execute the program exactly the same way again. --- GullRaDriel: I know about the pointers thing. I added a check in the for-loop to skip NULL points, made no difference. Here this the actual code without simplifications. 1void log(char *text, int color)
2{
3 if (mLog.text[LOGS-1] != NULL)
4 free(mLog.text[LOGS-1]);
5
6 for(int l = LOGS-1; l > 0; l--)
7 {
8 if (mLog.text[l-1] != NULL)
9 {
10 mLog.text[l] = mLog.text[l-1];
11 mLog.color[l] = mLog.color[l-1];
12 }
13 }
14 mLog.text[0] = (char*)malloc(sizeof(mLog.text[0])*64);
15 strcpy(mLog.text[0], text);
16 free(text);
17}
|
LennyLen
Member #5,313
December 2004
|
Are you going to be logging to a file? If so, here's the logging code I use, which is leak free. log.h: #ifndef LOG_H #define LOG_H void begin_logging(char *filename); void end_logging(); void add_to_log(char *info); #endif log.c: 1#include <time.h>
2#include <string.h>
3#include <stdio.h>
4#include <stdlib.h>
5#include "log.h"
6
7
8FILE *log_file;
9char temp[64], log_time[64];
10time_t now;
11
12
13void begin_logging(char *filename) {
14
15 log_file = fopen(filename, "w");
16 if (log_file == NULL) {
17
18 printf("Error creating log file: %s.\n", filename);
19 exit(1);
20 }
21
22 add_to_log("Logging begun.\n");
23
24}
25
26
27void end_logging() {
28
29 add_to_log("Logging ends.\n");
30 fclose(log_file);
31
32}
33
34
35void add_to_log(char *info) {
36
37 time(&now);
38 strcpy(temp, ctime(&now));
39 strncpy(log_time, temp, strlen(temp) - 1); // remove trailing \n from temp
40 fprintf(log_file, "%s: %s", log_time, info);
41 fflush(log_file);
42
43}
|
TeaRDoWN
Member #8,518
April 2007
|
LennyLen: Thanks, but I'm logging mainly to screen but also to file if you quit the game and resume later. The log has been used during development but we've now added a lot of information to the player what happens so it is more a information service than a log system. |
GullRaDriel
Member #3,861
September 2003
|
Here is something like you want + explanations. 1/******** Courtesy of GullRaDriel in his spare time ;-) ******/
2
3
4#include <stdio.h>
5#include <stdlib.h>
6#include <string.h>
7
8#ifndef true
9 #define true 1
10#endif
11
12#ifndef false
13 #define false 0
14#endif
15
16
17typedef struct LOG
18{
19 char* text[50]; /* please initialize that fucking pointer array to NULL. */
20}LOG;
21
22
23
24LOG *new_log()
25{
26 int it = 0 ;
27 LOG *tmplog = NULL ;
28
29 tmplog = (LOG *)malloc( 1 * sizeof( LOG ) );
30 if( !tmplog )
31 return NULL ; /* never forget to check against malloc */
32
33 for( it = 0 ; it < 50 ; it ++ )
34 tmplog -> text[ it ] = NULL ;
35
36 return tmplog;
37}
38
39
40int _log(char* text , LOG *mLog )
41{
42 char *tmpstr = NULL ; /* a temporary pointer to use for creating a new memory block for text */
43 int l = 0 ;
44
45 if( !text )
46 return false ;
47
48 /* instead of manually malloc and sprintf each time, just add it to your log function. */
49 tmpstr = (char *)calloc( (strlen( text ) + 1 ) , sizeof( char ) );
50 if( !tmpstr )
51 return false;
52
53 strcpy( tmpstr , text );
54
55 /* here is your error, you shift entries, but do not take care of the one who'll be 'forgotten' by your shifting. */
56 /* So, without doing nothing before, please first FREEEEEEe the last line of your log board */
57 /* That was the point to fill the array with NULL: we will not double free doing so */
58 if( mLog -> text[ 49 ] )
59 free( mLog -> text[ 49 ] );
60
61 for( l = 49; l > 0; l--)
62 {
63 mLog -> text[ l ] = mLog -> text[ l-1 ];
64 }
65 /* you want the last entry to be the first in the board. */
66 mLog -> text[ 0 ] = tmpstr ;
67
68 return true;
69}
70
71int clearLog( LOG **mLog )
72{
73 int l = 0 ;
74
75 if( !(*mLog) )
76 return false;
77
78 for( l = 0; l < 50; l++)
79 {
80 if( (*mLog) -> text[ l ] )
81 free( (*mLog) -> text[ l ] );
82 }
83 free( (*mLog) );
84 return true;
85}
86
87LOG *my_log_data1 = NULL ; /* to test for log that are < 50 lines */
88LOG *my_log_data2 = NULL ; /* to test for log that are > 50 lines */
89
90int main( void )
91{
92 int it = 0 ;
93
94 my_log_data1 = new_log();
95 my_log_data2 = new_log();
96
97 for( it = 0 ; it < 40 ; it ++ )
98 _log( "test" , my_log_data1 );
99
100 for( it = 0 ; it < 40 ; it ++ )
101 _log( "test" , my_log_data2 );
102
103 clearLog( &my_log_data1 );
104 clearLog( &my_log_data2 );
105
106 exit( 1 );
107}
"Code is like shit - it only smells if it is not yours" |
TeaRDoWN
Member #8,518
April 2007
|
Ok, that's like replacing every single line of code... It would be nice to know what I was doing wrong so I don't make the same mistake again...or do you mean that everything I've posted is bad? Btw, all 50 strings in text[] is set to NULL when starting the program. So I did that. ------------------------- ARRRRRRGH! I'm so stupied!! Found the problem... 1void logsnd(int id = -1)
2{
3 char *text = (char*)malloc(sizeof(text)*64); // Wow, this will leak like the economy of General Motors!
4 if (id != -1)
5 {
6 // Let's move it in here instead! FFS!!
7 if (id == 1337)
8 sprintf(text, "Play VO...", id);
9 else if (id == 0)
10 sprintf(text, "Play music...", id);
11 else
12 sprintf(text, "Play sample: %i...", id);
13
14 log(text, LOG_SOUND);
15 }
16 else
17 strcat(mLog.text[0], "Done!");
18}
The logsnd(int) was made since I have different functions for playing voice overs, effects and music. But I want to log them all. Everytime the logsnd() was accessed it did a malloc but only when id != -1 it actually used the allocated memory by sending the pointer to log(). Sorry guys. And I guess if the code wouldn't have been so huge I could have posted it all here. But at least I found the problem. Thanks for the support! |
GullRaDriel
Member #3,861
September 2003
|
You still have a memory leak. See the lines 55 to 50 of the code I posted before. I told you what was the problem with the code you gave: if you have more than 50 lines of log, you'll leak a line each time you add a new log line. I can't think of another way to explain you your problem. "Code is like shit - it only smells if it is not yours" |
TeaRDoWN
Member #8,518
April 2007
|
GullRaDriel
Member #3,861
September 2003
|
This part never been in the code you posted before. "Code is like shit - it only smells if it is not yours" |
LennyLen
Member #5,313
December 2004
|
GullRaDriel said: This part never been in the code you posted before. It was, though not in the original couple of posts. Though why anyone would ask for help looking for problems in their code, and then omit all the important parts of that code is beyond me.
|
GullRaDriel
Member #3,861
September 2003
|
Yeah, I read the whole topic a new time and saw it was added just after my post. "Code is like shit - it only smells if it is not yours" |
LennyLen
Member #5,313
December 2004
|
GullRaDriel said: (BTW, are you a special Debian Distro with some chatting ability ? ;-) ) I wonder if I could sue them for impinging my good name.
|
torhu
Member #2,727
September 2002
|
A bit late to the party, but I can't help myself... You could change the logging function into this: int log(const char *fmt, ...); That way you don't need to bother with providing buffers for formatting when you use it. It can call vsnprintf to the do formatting itself. And instead of using dynamic memory, you could use statically allocated buffers, and truncate strings that are too long. It's a lot cleaner and less bug-prone. Your screen is only so wide anyway. |
|