Code::Blocks Forums

Developer forums (C::B DEVELOPMENT STRICTLY!) => Development => Topic started by: rickg22 on June 20, 2007, 03:08:16 pm

Title: HUGE MEMORY LEAK pinpointed!
Post by: rickg22 on June 20, 2007, 03:08:16 pm
Hey guys, I think I know why some of the plugins (like the TODO list and perhaps even Code Completion) waste too much memory!

Look!

Code
bool cbRead(wxFile& file, wxString& st, wxFontEncoding encoding)
{
    st.Empty();
    if (!file.IsOpened())
        return false;
    int len = file.Length();
    if(!len)
    {
        file.Close();
        return true;
    }

    char* buff = new char[len+1];
    if (!buff)
    {
        file.Close();
        return false;
    }
    file.Read((void*)buff, len);
    file.Close();
    buff[len]='\0';

DetectEncodingAndConvert(buff, st, encoding);
// NOW WHERE'S DELETE BUFF?????????

    return true;
}

Notice that DetectEncodingAndConvert takes for parameter a const char*, so it means it doesn't do anything to buff. However, buff isn't released at the end. Hence, the leak.

I'll add a delete operator and see if nothing breaks.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: rickg22 on June 20, 2007, 03:12:06 pm
Update: It works!!!  :o

Memory consumption with the TODO plugin remains *steady* at 42MB!

I wonder how will code completion be affected with this... hmmm :)
Holy Cow...  :shock: 42MB was *WITH* code completion!!!! (No global includes tho, with them it's around 100mb). Without it's *JUST* 27.6MB! Woo hoo!!!!!!!

I'll commit ASAP.

(Hmm.......... code completion does leak. Everytime i parse, memory consumption is increased. But that'll be another adventure! :D )
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: David Perfors on June 20, 2007, 03:13:23 pm
wow, that is a nice catch :) great work rick :D
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: MortenMacFly on June 20, 2007, 03:19:56 pm
Wow... how ugly... must patch my C::B to have some testing... Well done, Rick! :P
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: jpaterso on June 20, 2007, 03:24:48 pm
Nicely picked up.
Recompiling my C::B now!
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: rickg22 on June 20, 2007, 03:26:21 pm
Everybody update their SVN's!

Fix posted at revision 4120!
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: jpaterso on June 20, 2007, 03:48:53 pm
Should it be delete [] instead of delete?
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: TDragon on June 20, 2007, 03:54:34 pm
Should it be delete [] instead of delete?
YES (http://www.parashift.com/c++-faq-lite/freestore-mgmt.html#faq-16.12).
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: Jan van den Borst on June 20, 2007, 04:13:13 pm
Typing in delete by hand is almost always bad practice. Use f.e. auto_ptr.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: byo on June 20, 2007, 04:18:59 pm
Should it be delete [] instead of delete?
YES (http://www.parashift.com/c++-faq-lite/freestore-mgmt.html#faq-16.12).

Yup, should be called. Fortunately in this particular case it will work with normal delete since there's no destructor called (binary code will probably look the same after switching to delete[]). So there's no need to panic ;).

Ok, now it's time to build the new c::B. Thanks rickg22 for the fix :)

BYO
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: Grom on June 20, 2007, 04:42:03 pm
Guys you have to use std::string. That will fix lot's of problems... Specially in that stupid example.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: Alturin on June 20, 2007, 04:49:58 pm
Why aren't you using wxString everywhere anyway?
Either that or as Grom said, use std::string!

Also, maybe someone with the required knowledge could run C::B through valgrind or similar?
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: byo on June 20, 2007, 05:08:32 pm
Why aren't you using wxString everywhere anyway?
Either that or as Grom said, use std::string!

wxString CAN NOT be used here because it is used to BUILD proper wxString. And std::string can allocate much more memory than needed. And this code was put into "global functions" because it does some tricky / risky thing and is provided to all developers to prevent them from writing their own (risky/tricky) code. Only because such bug was found in critical function doesn't mean that it's common technique in other parts of code.

Quote
Also, maybe someone with the required knowledge could run C::B through valgrind or similar?

That would require some knowledge since wxWidgets would have to compiled with valgrind first. It would be much easier to use wxWidget's built-in memory leak search features.I remember that when using wx with Visual Studio, such things was turned on automatically and gave nice reports at the end of execution.

BYO
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: TDragon on June 20, 2007, 05:18:25 pm
It would be much easier to use wxWidget's built-in memory leak search features.

Just make sure you get rid of:
Quote from: Seven C::B source files
#undef new
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: thomas on June 20, 2007, 05:22:15 pm
Quote
Why aren't you using wxString everywhere anyway?
Well, we could use wxString for that, file.Read(wxStringBuffer(buff, len+1), len); does just that... but it only adds a lot of overhead.

Rather than using a function that allocates buffers and reads data from disk, then makes a copy only to discard it a millisecond later, it would be better to use the FileManager for loading, anyway. This would make use of editors that are already loaded into memory, remove two allocations and memory copies, offer better I/O balancing, and be able to read from URLs.

But for the meantime, this is a nice catch, Rick.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: Biplab on June 20, 2007, 05:32:58 pm
Couple of Intel's tools are now free for Non-commercial use on Linux. Can anyone tell us whether Intel VTune Performance Analyser be used to detect Memory Leak or so? :)
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: rickg22 on June 20, 2007, 05:38:54 pm
it would be better to use the FileManager for loading, anyway.

Except this function is low level. The Todo plugin, for example, distinguishes between open files and in-disk files. File manager only opens one copy....  :o Oh, I get your point now.

So, should I change delete for delete[] in next SVN? er, I read the link. I'll do it tonight.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: thomas on June 20, 2007, 06:08:13 pm
Hmm... still 20.4 MB having Codeblocks.cbp open with no code completion enabled, and 105.4 with it.  8)

So, should I change delete for delete[] in next SVN? er, I read the link. I'll do it tonight.
Yes definitely, the other one is wrong and only works accidentially.


Except this function is low level. The Todo plugin, for example, distinguishes between open files and in-disk files.
The todo plugin should not really have to do with anything low level, I think. No component should, unless that's really necessary.
Some of the problems we have come from the fact that every component does its own low-level stuff, with filenames, strings, mangling arraystrings into strings and back, loading files, converting... adding a slash here, converting there, normalizing here but not there, pushing an event handler, tampering here and there, you get the idea.

Ideally, a plugin (or every other component) should be saying "ok, I am being told this is a path, I have no idea how it is encoded or what it corresponds to, now give me the data that belongs to this file, and no, I don't care how you do it".
You should not care whether a pathname is absolute or relative to your project directory or some subdirectory of it, or the root of your partition. The new file wizards *still* don't get this right (I've just had it fail again 5 minutes ago), and ProjectManager doesn't either, if the ScriptManager is involved. Ideally, they should all be saying "hey, I don't care... I have this 'thing' here, now you tell me what it is".
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: killerbot on June 20, 2007, 06:54:00 pm
I hadn't seen this post but when I looked at the change I saw the leak was not fixed, so the delete [] has already been put there by me. ==> really fixed now ;-)
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: Grom on June 20, 2007, 08:05:00 pm
malloc,free works faster then new delete[] :lol:
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: MortenMacFly on June 20, 2007, 08:32:13 pm
BTW guys; In wxScintilla there are at least two delete's instead of delete[]'s, too. Should we fix that?!

Edit: Don't worry: It's only in my wxScintilla version (1.71.1) the one from SVN seems to be OK though... I should file a bug report to the wxScintilla authors...)
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: Grom on June 20, 2007, 08:48:56 pm
If you will continue in the same way - that will be one more studio :lol:.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: rjmyst3 on June 20, 2007, 09:53:26 pm
Quote
Also, maybe someone with the required knowledge could run C::B through valgrind or similar?

That would require some knowledge since wxWidgets would have to compiled with valgrind first. It would be much easier to use wxWidget's built-in memory leak search features.I remember that when using wx with Visual Studio, such things was turned on automatically and gave nice reports at the end of execution.

BYO

valgrind does not require a recompile of anything - it is not intrusive. As long as the executable has debugging symbols it will work.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: byo on June 20, 2007, 10:04:56 pm
valgrind does not require a recompile of anything - it is not intrusive. As long as the executable has debugging symbols it will work.

Hmm, that's really nice :) Maybe I've confused it with something else or used very early release in the past so I could swear that it redeclare new/free :oops:.

BYO
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: killerbot on June 20, 2007, 10:17:59 pm
idea : valgrind plug-in in linux, which allows you to run the debug target.
I have never used it, so maybe the idea is not even possible, dunno. But this would be super cool  ...
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: MortenMacFly on June 20, 2007, 10:18:24 pm
valgrind does not require a recompile of anything - it is not intrusive. As long as the executable has debugging symbols it will work.
Now that we have that topic: Does anyone know something  similar (and free) for Windows?!

idea : valgrind plug-in in linux, which allows you to run the debug target.
Vote++. ;-)

With regards, Morten.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: Der Meister on June 20, 2007, 10:25:24 pm
Quote
Also, maybe someone with the required knowledge could run C::B through valgrind or similar?

That would require some knowledge since wxWidgets would have to compiled with valgrind first.
No, this should not be necessary. At least it was not as I hunted some memory leaks in Code::Blocks with valgrind more than one year ago.

Couple of Intel's tools are now free for Non-commercial use on Linux. Can anyone tell us whether Intel VTune Performance Analyser be used to detect Memory Leak or so? :)
No, I can't tell you anything about VTune but I would guess that you have to compile Code::Blocks with the Intel Compiler to use this program. And last time I tried to compile Code::Blocks with icc I had to fix quite a lot of issues to get it compiled. And if I remember correctly I was not able to run it after compilation finished. Don't know why, I had given up at this point and just fixed a few things for which the compiler showed some warnings.

Unfortunately Code::Blocks heavily relies on gcc and because of this it is not really possible to use tools for other compilers like VTune or the DevPartner Profiler which uses the Microsoft Compiler.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: killerbot on June 20, 2007, 10:27:59 pm
insider information.
DevPartner/BoundsChecker (Numega/Compuware) should be coming to linux and gcc ...

DevPartner is really good. It helped me spot memory leaks in code from a colleague today very quickly.

EDIT : just read the quick starters guide of valgrind. Some basic stuff should be up and running quickly.

- start the valgrind run command of our 'to-be' plugin
- select the target to (eventually warn if not a debug ('-g') build)
- launch valgrind with the correct arguments (command line arguments can be taken over ??are they exported in the sdk ???)
- capture all the output of valgrind and dump them in a special valgrind message panel [in the future parse and have a valgrind messages list, so double click thakes you to the code line, like with build message list]
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: Der Meister on June 20, 2007, 10:50:06 pm
insider information.
DevPartner/BoundsChecker (Numega/Compuware) should be coming to linux and gcc ...
Where did you get these information?  :shock:

DevPartner is really good. It helped me spot memory leaks in code from a colleague today very quickly.
Yes it is. Actually, I only used the profiler, but this one is *really* nice. With it's help I got huge speed improvements for my little raytracer. Would probably be nice to run it on Code::Blocks.

Just another comment to valgrind: Running Code::Blocks in valgrind was really easy. If I remember correctly I just used a few command-line options to get more output but then it worked really well and most of the time even provided quite accurate line numbers if it found some problem.
But: Simply starting Code::Blocks in valgrind took ages. Should have been around 5 minutes (!) on my Pentium 4.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: killerbot on June 20, 2007, 11:28:57 pm
I got this info from a compuware sales guy. But it is not finished yet, they are working on it.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: tiwag on June 21, 2007, 05:42:13 pm
there is another memleak in codecompletion plugin (CCplugin)

loading and closing a big workspace (CB and all plugins projects e.g.),
with CCplugin enabled shows,
that the CCplugin leaks about 56MB of memory at each loading & closing cycle.

CB memory usage [MB]
with CCplugin
disabled   enabled      comment
37,7      34,7      initial startup of CB
39,9      150,5      load workspace
37,4      121,7      close ws
40,0      206,4      load workspace
37,8      183,2      close ws
40,1      262,1      load workspace
39,0      242,0      close ws
40,1      318,1      load workspace
38,9      299,4      close ws
40,3      373,8      load workspace
39,8      355,4      close ws
39,9      150,5      load workspace
40,5      429,7      close ws



tested with CB svn 4132, wx284 unicode, win-XP
memory usage measured with "Process Explorer" (sysinternals)
CCplugin set to parse local and global includes

brgds
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: MortenMacFly on June 21, 2007, 06:09:09 pm
there is another memleak in codecompletion plugin (CCplugin)
BT: I just remembered something I realised sometime back: A possible crash candidate in parser.cpp, lines 690-696 (and on some other places I don't recall, too):
Code
  token = new Token();
  if (!token->SerializeIn(f))
  {
    delete token;
    token = 0;
    break;
  }
What if token could not be created (and is NULL therefore)? Shouldn't the if-check better be:
Code
if (token && !token->SerializeIn(f))
or something? (Need to implement an appropriate action anyway then...)
With regards, Morten.
Ps.: Should be go for a corporate bug-hunting for CC only?! ;-)
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: Der Meister on June 21, 2007, 06:41:40 pm
there is another memleak in codecompletion plugin (CCplugin)
BT: I just remembered something I realised sometime back: A possible crash candidate in parser.cpp, lines 690-696 (and on some other places I don't recall, too):
Code
  token = new Token();
  if (!token->SerializeIn(f))
  {
    delete token;
    token = 0;
    break;
  }
What if token could not be created (and is NULL therefore)?
token can't be NULL (or better: 0) even if it could not be created because new throws an exception (std::bad_alloc if I remember correctly) if it failes to allocate enough memory. Another possible problem could arise inside the construktor of the class Token, but again the only way for aborting it is throwing an exception. That means in the line "if (!token->SerializeIn(f))" token always points to a valid Token instance.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: mandrav on June 21, 2007, 07:09:31 pm
there is another memleak in codecompletion plugin (CCplugin)
BT: I just remembered something I realised sometime back: A possible crash candidate in parser.cpp, lines 690-696 (and on some other places I don't recall, too):
Code
  token = new Token();
  if (!token->SerializeIn(f))
  {
    delete token;
    token = 0;
    break;
  }
What if token could not be created (and is NULL therefore)? Shouldn't the if-check better be:
Code
if (token && !token->SerializeIn(f))
or something? (Need to implement an appropriate action anyway then...)
With regards, Morten.
Ps.: Should be go for a corporate bug-hunting for CC only?! ;-)

The serialization functions are not used anymore (for a long time now).
This means ReadFromCache() and WriteToCache() are not used so what you pointed at is effectively dead code.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: Biplab on June 21, 2007, 08:40:23 pm
Here is some "Meat" for "Memory-Leak-Hunters". ;)

I'm attaching the Valgrind log of Code::Blocks. I've run debug build of C::B (Rev 4145) with valgrind with the following options.
Code
valgrind --leak-check=full --show-reachable=yes

BTW, I'm a newbie with valgrind. So I'm posting the log which I received. Please feel free to suggest improvements, if any.

Best Regards,

Biplab

[attachment deleted by admin]
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: rickg22 on June 22, 2007, 04:31:26 am
w00t! Excellent!

Now... how do we interpret it? :P
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: rickg22 on June 22, 2007, 04:43:30 am
Oh my, look at this.

==29618== 10,748,090 bytes in 774 blocks are indirectly lost in loss record 244 of 244
==29618==    at 0x400591C: operator new[](unsigned) (vg_replace_malloc.c:195)
==29618==    by 0x49D3584: FileLoader::operator()() (filemanager.cpp:47)
==29618==    by 0x49D5727: BackgroundThread::Entry() (backgroundthread.h:153)
==29618==    by 0x4286217: wxThreadInternal::PthreadStart(wxThread*) (in /usr/local/lib/libwx_gtk2u-2.8.so.0.1.1)
==29618==    by 0x428628C: wxPthreadStart (in /usr/local/lib/libwx_gtk2u-2.8.so.0.1.1)
==29618==    by 0x48E532FA: start_thread (in /lib/libpthread-2.6.so)
==29618==    by 0x42CC993D: clone (in /lib/libc-2.6.so)

There's the culprit! According to the report, there is a new[] without delete[] in FileLoader::operator()().

Ah, there it is. Since it's multithreaded, it does NOT dispose the memory when the file's loaded successfully. But then it's the client which must do it, right? So that means code completion does NOT dispose the data once it's read... or does it? :(
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: Biplab on June 22, 2007, 04:54:44 am
There's the culprit! According to the report, there is a new[] without delete[] in FileLoader::operator()().

Great Ricks. :)

I hope this report can be used to clean up memory leak to a great extent.

BTW, I forgot to mention that to generate this report, I opened CodeBlocks-unix.cbp file with about 5-6 files open in Editor. Then I allowed Code Completion plugin to parse the project. Then I closed the project and C::B.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: rickg22 on June 22, 2007, 04:58:32 am
I have a question.

There's a Delete( ) operation done.

But I can't find ANYWHERE the function "Delete" (with uppercase). Is that C++? because I'd only seen delete in lowercase. Now I'm confused  :(
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: Biplab on June 22, 2007, 05:02:15 am
There's a Delete( ) operation done.

I believe this is for thread deletion, not for deallocating memory. :)
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: rickg22 on June 22, 2007, 05:13:37 am
I'm really not sure, ParserThread is cbThreadedTask and not wxThread. So Delete() can't really belong to wxThread.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: Biplab on June 22, 2007, 05:27:27 am
I'm really not sure, ParserThread is cbThreadedTask and not wxThread. So Delete() can't really belong to wxThread.

Where is this exactly?
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: TDragon on June 22, 2007, 05:59:14 am
You guys are barking up the wrong tree if you're talking about that new[] leak.

I think the problem is that the destructor in LoaderBase, which is responsible for delete[]-ing data, isn't virtual. In sdk/filemanager.cpp:139,143, FileLoader* are pushed onto BackgroundThread queues. Then in include/backgroundthread.h:145-158 they're popped off as AbstractJob* and, if owned by the BackgroundThread, deleted. The LoaderBase destructor isn't in the object's vtable and we're delete-ing by way of an AbstractJob*, so ~LoaderBase() never happens and we have a missing delete[].


Edit:
Nope, I was wrong; a base class' virtual destructor forces derived classes' destructors to be virtual also.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: Der Meister on June 22, 2007, 05:17:25 pm
As there is a delete[] for this reported memory leak located in the destructor of LoaderBase I think there are only two possible problems:
- The destructor does not get called.
- FileLoader::operator() gets called more than once before the instance is destroyed. This way the operator() would just request more memory but not release the old one.

I don't know which of this two things happens but I hope for the second one as it should be easy to solve. I'm just running valgrind to check this.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: rickg22 on June 22, 2007, 05:23:10 pm
Thomas, any ideas?
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: Der Meister on June 22, 2007, 06:00:55 pm
Bad news: Just putting a delete[] data; in in front of data = new char[len+1]; didn't solve the memory leak. I don't know if it changed anything as I didn't save the ouput of the valgrind run before this patch - shame on me. But still, just starting Code::Blocks, opening codeblocks.cbp, waiting until CC-plugin finished parsing and closing Code::Blocks left about 10MB non-freed memory behind just because of this function. According to this I would guess that the destrcutor of LoaderBase never gets called.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: rickg22 on June 22, 2007, 06:12:44 pm
Der Meister: Try doing the following experiment.

Open and close the Code::Blocks project several times, open it again and close the app. That should leave us with 70 or so megabytes of data floating around. That'll be much easier to locate in the valgrind logs.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: Biplab on June 22, 2007, 06:20:08 pm
Der Meister: Try doing the following experiment.

Open and close the Code::Blocks project several times, open it again and close the app. That should leave us with 70 or so megabytes of data floating around. That'll be much easier to locate in the valgrind logs.

Repeating valgrind tests can be painful. ;)

Valgrind runs the app approx. 10-20x times slower. I ran that test in a Virtual Machine. So it took a lot of time to get one complete log.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: Der Meister on June 22, 2007, 06:32:34 pm
No, I really don't want to do this. As Biplab said, valgrind runs are painful.

Anyway, I *hope* I found the reason for this. TDragon's post pointed me to the right direction: The BackgroundThread instance does not delete the jobs as it does not own them. Therefore the caller of FileManager::Load has to free the memory but it looks like this was never done. I just pachted a few places and will start another valgrind run to check if it works.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: thomas on June 22, 2007, 06:42:18 pm
There's the culprit! According to the report, there is a new[] without delete[] in FileLoader::operator()().
No, this is just like it has to be.

There's a Delete( ) operation done.
But I can't find ANYWHERE the function "Delete" (with uppercase).
It is a function that has been in globals.h for many months, and which is now in prep.h (so it's available everywhere).
What it does is, it calls the C++ delete operator on a pointer-to-object and (within thread scope) atomically zeroes the pointer.

I added this many months ago in response to the crashes that were caused by code like

if(pointer)
   delete pointer;


...which does not only contain an unnecessary if() but which was also the reason for regular mysterious crashes when a pointer was not set to zero after being delted, and subsequently was deleted a second time.

Except for member pointers that are deleted in class destructors, Delete() should always be used in preference to delete.
Using Delete() in a class destructor is of course possible, but since the members are invalid after this, there is unnecessary overhead in zeroing the pointer.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: Der Meister on June 22, 2007, 07:36:27 pm
Again bad news: I spotted only one location with a call to FileManager::Load without freeing the reserved memory:
- src/app.cpp:512

But adding this delete did not solve our big memory lead. Perhaps a small one, but not our real one. I have no idea where there is a call to delete missing. *Maybe* somewhere in the CC-plugin but I didn't find it. Anyway, if I did not overlook something the CC-plugin delegates the ownership of this objects to the cbThreadPool. I did not understand this class completely but it has calls to delete but I'm not sure if they are really used. There is at least one in the destructor of the thread pool which should free all memory but this one would not solve our problem as it would only free the memory when Code::Blocks is closed. But we have to free the memory after the CC-plugin finished its parsing.


Edit: Found two minor possible memory leaks. Shouldn't there be deletes in the following two functions?
Code
bool Parser::ParseBufferForFunctions(const wxString& buffer)
{
    ParserThreadOptions opts;
    opts.wantPreprocessor = m_Options.wantPreprocessor;
    opts.useBuffer = true;
    opts.bufferSkipBlocks = true;
    opts.handleFunctions = true;
    ParserThread* thread = new ParserThread(this,
                                            buffer,
                                            false,
                                            opts,
                                            m_pTempTokens);
    return thread->Parse();
}

bool Parser::ParseBufferForUsingNamespace(const wxString& buffer, wxArrayString& result)
{
    ParserThreadOptions opts;
//    opts.wantPreprocessor = m_Options.wantPreprocessor; // ignored
//    opts.useBuffer = false; // ignored
//    opts.bufferSkipBlocks = false; // ignored
    ParserThread* thread = new ParserThread(this,
                                            wxEmptyString,
                                            false,
                                            opts,
                                            m_pTempTokens);
    return thread->ParseBufferForUsingNamespace(buffer, result);
}
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: rickg22 on June 22, 2007, 08:51:45 pm
Yes, you're right about that one. Still small, but better than nothing.

However I still have the impression that those memory leaks must be found elsewhere. This is quite a sleuth case...  :?
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: Der Meister on June 22, 2007, 09:00:36 pm
Yes, they really should be found and fixed. But digging throw the CC-plugin isn't really easy and I didn't find a problem yet.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: dmoore on June 23, 2007, 03:13:25 pm
i ran some quick constructor/destructor counting tests and confirmed file loaders weren't being released as per valgrind logs. with CC enabled, this lead to thousands of open file loaders being left behind after opening and closing the CB main project a couple of times. I think similar problems could plague other plugins if they use the loader (filemanager seems to require callers to release loaders, but only after they have finished loading??)

hacky patch here: http://developer.berlios.de/patch/index.php?func=detailpatch&patch_id=2075&group_id=5358

please test. I think the problem boils down to the parser code only releasing file loaders if the thread is actually parsed (not all files will get the ok for parsing). i changed the code to only start a file loader if parsing is actually required. the affected functions are onparsefile and batchparse and bool Parser::Parse(const wxString& bufferOrFilename, bool isLocal, ParserThreadOptions& opts)

PS: the cc code is spaghetti with (IMHO) an unnecessarily complex threading model.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: Der Meister on June 23, 2007, 07:09:15 pm
Thank you for the patch, it seems to work quite well. Just compiled Code::Blocks with it and did a short test within valgrind. I didn't look through the whole output but the big leak from the FileLoader seems to be gone.  8)
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: rickg22 on June 23, 2007, 08:05:29 pm
PS: the cc code is spaghetti with (IMHO) an unnecessarily complex threading model.

The complex threading model was necessary, trust us. Parsing 1,000 files in the main thread is simply out of the question.
But I agree 100% with you on the spaghetti. I'm researching and testing a new model for Code Completion to implement after v1.0 is released. It's a longterm project.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: rickg22 on June 23, 2007, 08:11:55 pm
hacky patch here: http://developer.berlios.de/patch/index.php?func=detailpatch&patch_id=2075&group_id=5358

The patch has an error.

Code
opts.loader=Manager::Get()->GetFileManager()->Load(bufferOrFilename, false);

Should be:

Code
if(!opts.loader)
{
    opts.loader=Manager::Get()->GetFileManager()->Load(bufferOrFilename, false);
}

Otherwise you'd be overwriting the loader in case the function gets called somewhere else. Please fix and resend the patch :)
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: thomas on June 24, 2007, 01:42:09 am
(filemanager seems to require callers to release loaders, but only after they have finished loading??)
Yes, and yes. However, in the case of deleting loaders before they have finished loading, the memory leak is the least of your problems, since you're referencing unallocated memory all the way.
This is a well-known behaviour of FileManager that was discussed when it was first created.

I haven't looked into the code completion code, but if it does something like open many files and delete the loaders right away, then this sure explains why memory is leaked.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: rickg22 on June 24, 2007, 02:00:50 am
Thomas, what do you think about the patch? Is it OK with you?
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: dmoore on June 24, 2007, 03:15:36 am
The patch has an error.

Code
opts.loader=Manager::Get()->GetFileManager()->Load(bufferOrFilename, false);

Should be:

Code
if(!opts.loader)
{
    opts.loader=Manager::Get()->GetFileManager()->Load(bufferOrFilename, false);
}

Otherwise you'd be overwriting the loader in case the function gets called somewhere else. Please fix and resend the patch :)

in my patch, I NULLed out the loader member of opts in all instances where Parse is called and needs a loader, so (!opts.loader) should always be true. More importantly, in the present state of the plugin, the loader should never be called (using Load) outside of Parse or there will be memory leaks, so loader must always be NULL unless you come up with a more elaborate patch.

(filemanager seems to require callers to release loaders, but only after they have finished loading??)
Yes, and yes. However, in the case of deleting loaders before they have finished loading, the memory leak is the least of your problems, since you're referencing unallocated memory all the way.
This is a well-known behaviour of FileManager that was discussed when it was first created.

I haven't looked into the code completion code, but if it does something like open many files and delete the loaders right away, then this sure explains why memory is leaked.

No, CC never deletes the loaders that aren't complete (this causes seg faults), it just fails to delete some loaders and loaders don't delete themselves because they have to store the file data for the caller to use. There are lots of different ways to address this issue - my approach was just the simplest that came to mind.

The complex threading model was necessary, trust us. Parsing 1,000 files in the main thread is simply out of the question.
But I agree 100% with you on the spaghetti. I'm researching and testing a new model for Code Completion to implement after v1.0 is released. It's a longterm project.

I wasn't claiming that you could parse files on the main thread, I was just claiming the threading could be a lot simpler, which would make it more maintainable. the actual parser code, by comparison, looks quite readable.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: rickg22 on June 24, 2007, 07:26:34 am
[
in my patch, I NULLed out the loader member of opts in all instances where Parse is called and needs a loader, so (!opts.loader) should always be true. More importantly, in the present state of the plugin, the loader should never be called (using Load) outside of Parse or there will be memory leaks, so loader must always be NULL unless you come up with a more elaborate patch.

Yes, but since we're dealing with critical code here, we can't leave anything to luck or past decisions. The code might be changed later, and this line could be forgotten. So, please put the if(!opts.loader) there. It's safe and it doesn't harm.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: dmoore on June 24, 2007, 03:02:13 pm
ok, and i've added a couple of lines to produce a debug warning about memory leaks if the loader has already been set.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: thomas on June 24, 2007, 04:06:25 pm
Thomas, what do you think about the patch? Is it OK with you?
I don't understand what either the original code or the patch is doing :)
In particular, I don't understand what Parse(...., NULL) actually parses, but if it works, that's ok with me :)

deletes the loaders that aren't complete (this causes seg faults)
Yes, necessarily. The loader object that you get back is aliased in the file manager's queue, so its worker thread knows what to load. If you delete the loader before it has finished loading, FileManager will read from and write to unallocated memory.
This behaviour was accepted when the class was created, as it keeps the class a lot less complicated and it shouldn't be an issue, normally. You don't usually load a dozen files if you don't really want to do anything with them. However, it seems like this assumption may have been wrong.

Quote
There are lots of different ways to address this issue - my approach was just the simplest that came to mind.
Probably the best solution will be to take away object ownership from the user. It just doesn't work...
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: Der Meister on June 24, 2007, 04:14:21 pm
Quote
There are lots of different ways to address this issue - my approach was just the simplest that came to mind.
Probably the best solution will be to take away object ownership from the user. It just doesn't work...
I agree with that. But that means that the FileLoader is resonsible for freeing the memory. But therefore it has to know whether it is still in use or not required any more. But I see only one way to implement this: smart pointers... Well...
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: dmoore on June 24, 2007, 04:51:07 pm
Quote
There are lots of different ways to address this issue - my approach was just the simplest that came to mind.
Probably the best solution will be to take away object ownership from the user. It just doesn't work...
I agree with that. But that means that the FileLoader is resonsible for freeing the memory. But therefore it has to know whether it is still in use or not required any more. But I see only one way to implement this: smart pointers... Well...

is it not possible to use events? when file loading is completed the loader could send a message to the caller saying "here's the data, let me know when you are done (by calling certain member of the loader) so i can clean up"
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: Der Meister on June 24, 2007, 05:06:53 pm
Well, this would again put the ownership of the object to the caller as it has to call this certain member function to clean things up. This isn't much different from requiring the caller to delete the FileLoader as soon as it does not need it any more but requires a much more complicated interface.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: dmoore on June 24, 2007, 05:27:37 pm
Well, this would again put the ownership of the object to the caller as it has to call this certain member function to clean things up. This isn't much different from requiring the caller to delete the FileLoader as soon as it does not need it any more but requires a much more complicated interface.

If the caller is going to process the data they are going to have to take ownership of it (an auto_ptr may help, of course)

I don't think ownership is that big a problem. IMO, the big problem is that you have to wait until a loader is done loading before you can delete it, but the loader doesn't tell you when its done, so you have create messy threads to wait for it. messaging would be the most consistent way of dealing with this (considering CB is a wxwidgets app)
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: killerbot on June 24, 2007, 05:32:32 pm
can a loader be queried to check it's state wrt finished loading ?
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: Der Meister on June 24, 2007, 06:08:58 pm
I didn't see such a method in its interface but the function to retrieve the data is synchronized with the background-thread (well, what a surprise).
Anyway, I don't see any use from such method. Care to explain?
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: killerbot on June 24, 2007, 06:21:52 pm
stupid as it is, query to know when finished so you can delete it
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: dmoore on June 24, 2007, 06:30:07 pm
IMHO, without messages (or ability to query), threaded fileloaders are pointless because the only way to use them without blocking the main thread is to create another thread (then why do you need the fileloader threaded?)
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: thomas on June 24, 2007, 06:34:02 pm
Committed the following changes:

1. Added a Delete() function to AbstractJob (base class of LoaderBase).
Calling Delete() will:
In other words, you now call loader_ptr->Delete(); rather than delete loader_ptr;. Existing code still works as before.

2. Added Loader as an (untested!) wrapper for LoaderBase*. Basically, Loader can be thought of std::auto_ptr<LoaderBase*> with the exceptions that:
I don't have time to test that class right now, but it should be simple enough to just work... in any case it compiles without errors ;)

So... from now on, we should be able to write code like:

{
LoaderBase* load = FileManager::Load(_T("foo.bar"));
if(strange_condition)
   load->Delete();  // discard, resources will be freed when opportune
}


or

{
Loader load = FileManager::Load(_T("foo.bar"));
wxString x = load.GetData();
} // loader resources freed at end of scope


Obviously, accessing any member on a LoaderBase* after calling Delete() is illegal and may or may not write to unallocated memory.


can a loader be queried to check it's state wrt finished loading ?
Not in trunk.

EDIT:
Test suite for Delete() attached. The patch creates a file that is 1 MB in size and loads it 100 times. Memory consumption is +100 MB temporarily, and +/-0 after calling Delete().
The test has been performed with and without a Sleep(2000) to give FileManager some time to load the data, to the same result.

[attachment deleted by admin]
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: thomas on June 24, 2007, 06:56:06 pm
IMHO, without messages (or ability to query), threaded fileloaders are pointless because the only way to use them without blocking the main thread is to create another thread (then why do you need the fileloader threaded?)
They are not pointless. You tell FileManager that you will be wanting some file (Load()) at some point in the future. FileManager may load it immediately or later, or it might even not do anything at all before you ask for the data.
At some later point, you ask FileManger for a pointer to the data (GetData()) that you want, and FileManager gives you back a valid pointer (or null in case of an error). It is not your problem what FileManager has to do to return that pointer, you can rely that the data will just be there.
Basically, this can be seen as the poor man's implementation of file mapping.

Behind the scenes, FileManager will queue all requests that access local storage into one queue, so to make optimal use of the hardware (keeping the drive as busy as possible, but avoiding multi-thread seek-thrashing), and all requests that involve network into another queue. While the PC is doing mainly DMA or is waiting for the hard drive to seek (or network packets to arrive), the main thread can put CPU cycles into parsing a file, or similar tasks, instead of blocking on fread().

In the example of Code Completion, this parallelism reduces load times from several minutes to a dozen of seconds, as Yiannis has benchmarked a while ago.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: thomas on June 25, 2007, 10:09:15 am
I'll remove the Loader wrapper class again, it seems to make more problems than it's worth.

On a different note:
void Parser::BatchParse(const wxArrayString& filenames)
{
    m_batchtimer.Stop();
    m_IsBatch = true;
    m_Pool.BatchBegin();
    for (unsigned int i = 0; i < filenames.GetCount(); ++i)
    {
        LoaderBase* loader = Manager::Get()->GetFileManager()->Load(filenames, false);
        Parse(filenames[ i ], true, loader);
    }

    // Allow future parses to take place in this same run
    m_batchtimer.Start(batch_timer_delay,wxTIMER_ONE_SHOT);
}


Is this ever deleted anywhere? I see that the pointer is added to a ParserThreadOptions struct, but I don't see that it is ever released. This would explain why the memory is never freed.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: dmoore on June 25, 2007, 12:21:11 pm
this is what the patch addresses (and an identical line in OnParseFile). it moves the Load call into Parse (the 2nd one), and Load only gets called if a ParserThread is created (that classes destructor releases the loader object). Not only does that avoid memory leaks, but it also saves hundreds of unnecessary loads (part of the disk thrashing?)
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: mandrav on June 25, 2007, 01:21:45 pm
I will have a look at this today.

Patch applied, thanks dmoore.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: thomas on June 25, 2007, 04:54:51 pm
I will have a look at this today.

Patch applied, thanks dmoore.
Something is not good with that, I think... I am getting access violations at startup from within BackgroundThread::Entry with r4170 now, but only if CC is enabled, and if I had a project open when closing Code::Blocks in the previous session.
I'm not sure what it is (something being deleted too early?), but it's strange, and it didn't happen before...  :(
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: Der Meister on June 25, 2007, 05:38:18 pm
Really strange as the code from the patch doesn't delete anything. It just does not create FileLoader instances in some situations.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: dmoore on June 25, 2007, 05:39:01 pm
the patch doesn't change the placement of any deletes. the only way this patch could be directly responsible is if (a) the  loader pointer is being acessed when NULL or (b) the Tokenizer class expects to find a loader for a given file, but it hasn't been instantiated because of the new placement of the Load call,  causing unexpected behavior. Trawling through the only other places the loader is accessed: ParserThread::InitTokenizer, Tokenizer::Init, Tokenizer::Read, and ParserThread::~ParserThread, I don't think this is the case.

there has been lots of other CC activity...
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: dmoore on June 25, 2007, 05:42:02 pm
Der Meister: do you have the same problem with the patch (without other recent CC changes)?
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: Der Meister on June 25, 2007, 05:51:27 pm
No, I had no problem with your first version of the patch. I never tested your changed versions.
But I only did a rather small check: Starting Code::Blocks, loading the Code::Blocks workspace, waiting until the parser finished and then closing Code::Blocks. This worked. After that I did the same procedure in valgrind to check whether the memory leak was really gone.

I just did another quick test: Opening and closing several projects without waiting for the parser to finish its work. No crash. But still I use revision 4161 with the first version of your patch on Linux.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: rickg22 on June 25, 2007, 06:22:26 pm

Something is not good with that, I think... I am getting access violations at startup from within BackgroundThread::Entry with r4170 now, but only if CC is enabled, and if I had a project open when closing Code::Blocks in the previous session.
I'm not sure what it is (something being deleted too early?), but it's strange, and it didn't happen before...  :(

I confirm, it happened to me only once - 1 or 2 days ago. But I couldn't reproduce it :(. Perhaps it had something to do with some changes i did to CC parsing startup (I don't use a timer anymore). But it's strange indeed (The other times I get segfaults are if i close / open projects TOO FAST and/or close the app before the parser finishes, but i have to be real quick to get them and doesn't happen always).

With Revision 4168 (without the patch applied) I couldn't reproduce the backgroundthread crash. Thomas, can you confirm that the crash does NOT occur if the patch is _NOT_ applied? (i.e. changing it like it was)

Another test: Apply the patch to revision 4163 (before my changes) and test.

I'll test tonight, since i only have 4168 in my machine.
BTW... i have the hunch it has to do with building the class browser. Thomas, here's a quick test. Comment the part that invokes the building of class browser, and see if the crash remains.

EDIT: The code belongs to the loader, not the class browser (class browser is wxThread,not Backgroundthread). Thomas, I recall you saying that you added a Delete part to BackgroundThread, right? Perhaps this new part is the cause of the conflict? i.e. trying to delete something that isn't there, or not initializing the deleted field correctly?
Also, a stack log (codeblocks.RPT) could help.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: rickg22 on June 25, 2007, 06:48:38 pm
...but only if CC is enabled,

with CC you mean the plugin, or the code completion functionality ONLY?
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: thomas on June 25, 2007, 07:48:12 pm
The plugin.

Steps to reproduce:
1. load Code::Blocks
2. click on codeblocks.cbp on the start here page to open it
3. wait a few seconds
4. close Code::Blocks
5. load Code::Blocks ---> access violation
6. load Code::Blocks ---> works fine
7. try (6) any number of times ---> works fine
8. try again from step (2) ---> access violation

Disable the CC plugin, and there is never any crash.

I've been using r4120 before, and never had any visible issues.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: rickg22 on June 25, 2007, 08:14:39 pm
Thanks, I'll test it tonight. OH, btw, can you post a copy of codeblocks.RPT here?
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: Der Meister on June 25, 2007, 08:27:24 pm
Steps to reproduce:
1. load Code::Blocks
2. click on codeblocks.cbp on the start here page to open it
3. wait a few seconds
4. close Code::Blocks
5. load Code::Blocks ---> access violation
6. load Code::Blocks ---> works fine
7. try (6) any number of times ---> works fine
8. try again from step (2) ---> access violation

Disable the CC plugin, and there is never any crash.

I've been using r4120 before, and never had any visible issues.
This has to be a windows issue or it was introduced after revision 4161 as I can't reproduce it here with revision 4161 on Linux.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: rickg22 on June 25, 2007, 08:31:13 pm
Yes, it's a problem that appeared after 4164 or greater (no need to post "me neither", guys). It's either my code or Thomas'. Or perhaps a strange situation where both changes trigger the crash.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: darthdespotism on June 25, 2007, 09:06:35 pm
My Codeblocks (actual SVN) crashes on each startup /Ubuntu-Linux 7.04

Is this related to the discused bug?

[attachment deleted by admin]
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: rickg22 on June 25, 2007, 09:17:12 pm
It's most probable.

Code
<stack>
  <frame level="0" function="wxFatalSignalHandler" offset="00000026" />
  <frame level="1" />
  <frame level="2" function="EditorLexerLoader::Load(LoaderBase*)" offset="0000001d" />
  <frame level="3" function="EditorColourSet::LoadAvailableSets()" offset="00000362" />
  <frame level="4" function="EditorColourSet::EditorColourSet(wxString const&)" offset="00000067" />
  <frame level="5" function="EditorManager::EditorManager()" offset="000003e9" />
  <frame level="6" function="Manager::GetEditorManager() const" offset="00000056" />
  <frame level="7" function="MainFrame::OnStartHereLink(wxCommandEvent&)" offset="00000000" />
  <frame level="8" function="MainFrame::OnPluginInstalled(CodeBlocksEvent&)" offset="00000000" />
  <frame level="9" function="MainFrame::OnPluginInstalled(CodeBlocksEvent&)" offset="00000000" />
  <frame level="10" function="CodeBlocksApp::OnAppActivate(wxActivateEvent&)" offset="00000000" />
  <frame level="11" function="CodeBlocksApp::OnBatchBuildDone(CodeBlocksEvent&)" offset="00000000" />
  <frame level="12" function="wxAppConsole::CallOnInit()" offset="00000000" />
  </stack>

LoaderBase* seems to be the culprit, but having the source line numbers would be better.

Can you do us a favor? On your codeblocks directory, there should be a file called "codeblocks.RPT".
Open it with a text editor and browse to the latest entry. Can you copy and paste the lines here, please? Thanks! :)
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: dmoore on June 25, 2007, 09:35:45 pm
i can't replicate at all with latest svn on win32. did you guys clear out your devel dir?

correction

copied output dir back to my main install location and reran cb: what do you know -- crash

C:\Codeblocks\codeblocks.exe caused an Access Violation at location 00000011 Reading from location 00000011.

Registers:
eax=00000011 ebx=00c02e00 ecx=00000001 edx=00fcbe2c esi=00f12444 edi=ffffffff
eip=00000011 esp=01cafec4 ebp=01cafed0 iopl=0         nv up ei pl nz na pe nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00010202

Call stack:
00000011
606C3CF1  C:\Codeblocks\codeblocks.dll:606C3CF1  _ZN16BackgroundThread5EntryEv
100BF5CB  C:\Codeblocks\wxmsw28u_gcc_custom.dll:100BF5CB  _ZN11wxSemaphore7TryWaitEv
100BF6DD  C:\Codeblocks\wxmsw28u_gcc_custom.dll:100BF6DD  _ZN11wxSemaphore7TryWaitEv
77C3A3B0  C:\WINDOWS\system32\msvcrt.dll:77C3A3B0  _endthreadex
7C80B683  C:\WINDOWS\system32\kernel32.dll:7C80B683  GetModuleFileNameA
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: darthdespotism on June 25, 2007, 10:08:27 pm

LoaderBase* seems to be the culprit, but having the source line numbers would be better.

Can you do us a favor? On your codeblocks directory, there should be a file called "codeblocks.RPT".
Open it with a text editor and browse to the latest entry. Can you copy and paste the lines here, please? Thanks! :)


If I could locate the file I would post it.
Can you guide me to the location with Ubuntu/Linux?

EDIT://
I'll be off for today.
Trying to locate it tomorrow
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: Ceniza on June 25, 2007, 10:33:04 pm
Useless to locate it... it's NOT created on *nix, only Windows. The XML file is what you get on *nix.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: dmoore on June 25, 2007, 10:43:37 pm
line 170 of backgroundthread.h causes the problem (delete job), so presumably job is being deleted twice...

Code
class BackgroundThread ...
    ExitCode Entry()
    {
        AbstractJob* job;
        for(;;)
        {
            semaphore->Wait();
            if(die)
                break;

            job = queue->Pop();

            if(job->deleted)
            {
                delete job;
                continue;
            }

            (*job)();
            job->done = true;

            if(job->deleted || ownsJobs)
            {
******         delete job; // CRASH
            }
        }
        return 0;
    };

The recent svn history for that file might help:

http://svn.berlios.de/wsvn/codeblocks/trunk/src/include/backgroundthread.h?op=diff&rev=0&sc=0
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: rickg22 on June 25, 2007, 10:49:41 pm
dmoore, if you turn off codecompletion, the crash goes away? Because i hadn't modified that code yet after those changes in LoaderBase were introduced, and there's a part in Code completion that does a Delete( ... ). But then again, what about that crash reported by darthdespotism?
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: Der Meister on June 25, 2007, 10:53:56 pm
Well, I don't think that this is the cause for the crash here but I think we have a possible race condition here: After the job has finished and job->done is set to true it is possible to delete the job using AbstractJob::Delete() (which will be called from a different thread). This will set AbstractJob::deleted to true which in case allows the BackgroundThread to delete the job. Now we have two deletes for just one piece of memory.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: dmoore on June 25, 2007, 11:01:37 pm
dmoore, if you turn off codecompletion, the crash goes away? Because i hadn't modified that code yet after those changes in LoaderBase were introduced, and there's a part in Code completion that does a Delete( ... ). But then again, what about that crash reported by darthdespotism?

i can't get it to crash consistently even with the CC plugin switched on. I haven't had any crashes with the CC plugin turned off.

I reverted the backgroundthread changes and testing...
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: rickg22 on June 26, 2007, 02:01:36 am
dmoore, any news?
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: rickg22 on June 26, 2007, 04:24:13 am
I've tried to analyze the Abstractjob code, but it's too complicated for me to understand the program flow. I don't even know if the ORIGINAL code is correct!  :?

I vote for a revert. I'm compiling the reverted code and see if there are any problems. If I don't find any crashes, I'll commit.
Edit: Everything seems fine. I'll commit after dinner :)
Edit: Done.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: thomas on June 26, 2007, 09:46:36 am
Reverting the code because you don't understand it sounds like the way to go :)

I think DerMeister is right, but we will now never find out.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: thomas on June 26, 2007, 10:57:26 am
And this is what r4182 has to say about opening a project with CC enabled:
Error occured on Tuesday, June 26, 2007 at 10:52:40.

C:\CodeBlocks\codeblocks.exe caused an Access Violation at location 6199d7cb in module C:\CodeBlocks\codeblocks.dll Writing to location 77d5624f.

Registers:
eax=77d5624f ebx=06545a00 ecx=00000000 edx=0022f2a8 esi=05059ef0 edi=ffffffff
eip=6199d7cb esp=0831fd40 ebp=0831fd40 iopl=0         nv up ei pl nz ac po nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00010216

Call stack:
6199D7CB  C:\CodeBlocks\codeblocks.dll:6199D7CB  _ZN7SQLexer6ReadIDEv
61A4CA9D  C:\CodeBlocks\codeblocks.dll:61A4CA9D  _ZNSt4listIN12cbThreadPool21cbThreadedTaskElementESaIS1_EE8_M_eraseESt14_List_iteratorIS1_E
61A4CB1E  C:\CodeBlocks\codeblocks.dll:61A4CB1E  _ZNSt4listIN12cbThreadPool21cbThreadedTaskElementESaIS1_EE9pop_frontEv
617EDEF9  C:\CodeBlocks\codeblocks.dll:617EDEF9  _ZN12cbThreadPool11GetNextTaskEv
617EE63F  C:\CodeBlocks\codeblocks.dll:617EE63F  _ZN12cbThreadPool14cbWorkerThread5EntryEv
627BF82B  C:\CodeBlocks\wxmsw28u_gcc_custom.dll:627BF82B  _ZN11wxSemaphore7TryWaitEv
627BF93D  C:\CodeBlocks\wxmsw28u_gcc_custom.dll:627BF93D  _ZN11wxSemaphore7TryWaitEv
77C0A3B0  C:\WINDOWS\system32\msvcrt.dll:77C0A3B0  _endthreadex
7C80B683  C:\WINDOWS\system32\kernel32.dll:7C80B683  GetModuleFileNameA


I can't reproduce it reliably, seems to happen randomly opening the same project... no idea what it is now. But no crashes with CC disabled, again.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: dmoore on June 26, 2007, 01:23:41 pm
sorry i had to dash yesterday guys. i didn't test thoroughly, but didn't have any crashes with the abstractjob code reverted. i'm now trying latest svn on my home win32 and linux boxes without troubles so far... (I haven't had time to compile contrib plugins. I did clean out devel, also it might help to check out a pristine copy or fully revert any local changes: svn revert -R *).
Title: I FOUND THE CAUSE!
Post by: rickg22 on June 26, 2007, 03:23:10 pm
I can't reproduce it reliably, seems to happen randomly opening the same project... no idea what it is now. But no crashes with CC disabled, again.

Well, now that the workerthread code is gone, at least it'll be easier for us to identify the culprit. After some deductions, the only possibility left is that it's my fault, and I have understood why  :oops:. I wasn't still not sure why, until the crash was reproduced without Thomas' code.

K, here goes...

In my latest change to Code Completion, to fix the class browser not being updated, i moved the code that parses the active projects, from a wxTimerEvent handler, to the OnWorkspaceLoaded event handler. After looking at the app.cpp code a few minutes ago, I noticed that the parsing starts BEFORE it should, making these random crashes appear. Since it's multithreaded, the error only gets triggered either on very fast machines, or on very slow machines.

(The problem is that these 3 changes took place in the same build - from 4165 to 4169: 1) Thomas' workerthread change, 2) dmoore's memleak fix, and 3) My apparently-harm change to the parsing)

Anyway...

Code
bool CodeBlocksApp::OnInit()
{
 ...
        MainFrame* frame = 0; frame = InitFrame();
        m_Frame = frame;
...
        Manager::Get()->GetMessageManager()->DebugLog(_T("Initializing plugins..."));
        CodeBlocksEvent event(cbEVT_APP_STARTUP_DONE);
        Manager::Get()->ProcessEvent(event);
        Manager::ProcessPendingEvents();
        splash.Hide();
}
The problem is that m_Frame isn't set in the middle of InitFrame, and guess what happens in there:
Code
MainFrame* CodeBlocksApp::InitFrame()
{
...
            Manager::Get()->GetProjectManager()->LoadWorkspace();
...
}

And in LoadWorkspace...
Code
        CodeBlocksEvent event(cbEVT_WORKSPACE_LOADED);
        Manager::Get()->GetPluginManager()->NotifyPlugins(event);

So, Thomas, you were both right and wrong: The error was triggered by Code Completion because it's the ONLY plugin yet that uses the new EVT_WORKSPACE_LOADED; but the fault lies within my changes to ProjectManager, because I didn't take into account that the app hadn't been initialized yet in all workspace loads.

And *THIS* is why it happened *ONLY* if you had a workspace open beforehand.

I'll fix ProjectManager so that it only sends the event after the main frame has been initialized. But please don't re-commit your backgroundthread change until we have tested my change... just to be on the safe side :mrgreen:

Edit: I modified the code and am compiling, but I have to go to the job now :( I'll commit in around 10 hours, sorry...
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: Pecan on June 26, 2007, 04:05:13 pm
Twice, I got the crash described but I have CC disabled.
It occurred after the splash and just as the CB window appeared.
I have no auto loading of files. So how could this be a CC crash?

I'll run under gdb from now on. See If I can catch it.

SVN4169 XpSp2

Code
 Tuesday, June 26, 2007 at 09:32:52.

c:\Usr\Proj\cbBeta\trunk\src\devel\codeblocks.exe caused an Access Violation at location 00000011 Reading from location 00000011.

Registers:
eax=00000011 ebx=01725b00 ecx=00000001 edx=01759d0c esi=015e8194 edi=ffffffff
eip=00000011 esp=0202fec4 ebp=0202fed0 iopl=0         nv up ei pl nz na po nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00010206

Call stack:
00000011
606C34C1  c:\Usr\Proj\cbBeta\trunk\src\devel\codeblocks.dll:606C34C1  _ZN16BackgroundThread5EntryEv
100BE97B  c:\Usr\Proj\cbBeta\trunk\src\devel\wxmsw28u_gcc_custom.dll:100BE97B  _ZN11wxSemaphore7TryWaitEv
100BEA8D  c:\Usr\Proj\cbBeta\trunk\src\devel\wxmsw28u_gcc_custom.dll:100BEA8D  _ZN11wxSemaphore7TryWaitEv
77C3A3B0  C:\WINDOWS\system32\msvcrt.dll:77C3A3B0  _endthreadex
7C80B50B  C:\WINDOWS\system32\kernel32.dll:7C80B50B  GetModuleFileNameA



Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: rickg22 on June 26, 2007, 04:11:24 pm
Pecan: Thanks for clearing that up!

Anyway.

I tested my fix, and this is what happens:

Code
[09:08:28.875]: Initializing plugins...
[09:08:28.890]: Add project Code::Blocks in parsing queue

This proves that the parsing doesn't start until the plugins have been correctly initialized. In other words, it works! :)

I'm committing the changes right now... done. Revision 4185.
Title: Re: HUGE MEMORY LEAK pinpointed!
Post by: David Perfors on June 26, 2007, 04:46:59 pm
Those 10 hours did passed by verry quickly :P