A reproducible crash of CC

[Loaden]

Both under Linux or Windows, there is no way to find the cause.
Updated source code to the CB's HEAD, and then apply the patch.
Then opening any project CB will crash.

[Loaden]

I think I find the reason: call TRACE in a sub thread is not allowed!
And this maybe the final reason of all the random crash!

[Loaden]

I think we can't call: Manager::Get()->GetLogManager()->DebugLog(...) or Manager::Get()->GetLogManager()->Log(...) in any sub thread.
So, We need rewrite CC's Logger.
Any comments?

[ollydbg]

Good catch loaden!!!

looking at the code in include\logmanager.h

It seems the DebugLog function internally call:

Code

void LogInternal(const wxString& msg, int i, Logger::level lv) { if ((i>=0) && (i<=(int)max_logs) && (slot[i].log!=&g_null_log)) slot[i].log->Append(msg, lv); };

I can't find critical section or other resource locker there.

[MortenMacFly]

Remember, the trace macro was actually only for debug purposes. I know, we mis-use it. :lol:

However, please don't mess with LogManager. That's not something to change. instead, we should always use events to report CC infos from threads to the plugin "main loop" itself. So instead of calling Log(...) directly, fire a custom event (implemented inside the CC plugin) and handle the call outside the thread.

[ollydbg]

Remember, the trace macro was actually only for debug purposes. I know, we mis-use it. :lol:

Agree!!!

However, please don't mess with LogManager. That's not something to change. instead, we should always use events to report CC infos from threads to the plugin "main loop" itself. So instead of calling Log(...) directly, fire a custom event (implemented inside the CC plugin) and handle the call outside the thread.

Yes, correctly way we should use.

[Loaden]

we should always use events to report CC infos from threads to the plugin "main loop" itself.

Yes, this is what I called the CC's Logger.

[oBFusCATed]

Loaden: do you know that the singleton pattern is considered a bad pathern? Why are you doing CCLogger::Get(), instead of having m_logger or g_logger?

Some random links:
http://blogs.msdn.com/b/scottdensmore/archive/2004/05/25/140827.aspx
http://stackoverflow.com/questions/86654/whats-wrong-with-singleton
http://aabs.wordpress.com/2007/03/08/singleton-%E2%80%93-the-most-overused-pattern/

p.s. I don't like the Manager::Get() thing, too

[Loaden]

Loaden: do you know that the singleton pattern is considered a bad pathern? Why are you doing CCLogger::Get(), instead of having m_logger or g_logger?

Some random links:
http://blogs.msdn.com/b/scottdensmore/archive/2004/05/25/140827.aspx
http://stackoverflow.com/questions/86654/whats-wrong-with-singleton
http://aabs.wordpress.com/2007/03/08/singleton-%E2%80%93-the-most-overused-pattern/

p.s. I don't like the Manager::Get() thing, too

Singleton pattern in here, nothing bad.
I do not like global variables.
In here, we can't use member variables, It will be worse.

[oBFusCATed]

Singleton is a global variable!!! They call it glorified global variable.
What do you gain by using singleton in this case? Instead of global variable or global variable hidden behind two functions cc::DebugLog and cc::Log (and of course one init function)?

BTW: Your Get() method is not thread safe:)

Also don't use double locking if you try to make it thread safe:
http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
http://bartoszmilewski.wordpress.com/2008/08/04/multicores-and-publication-safety/

[Loaden]

Singleton is a global variable!!! They call it glorified global variable.
What do you gain by using singleton in this case? Instead of global variable or global variable hidden behind two functions cc::DebugLog and cc::Log (and of course one init function)?

BTW: Your Get() method is not thread safe:)

Also don't use double locking if you try to make it thread safe:
http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
http://bartoszmilewski.wordpress.com/2008/08/04/multicores-and-publication-safety/

In this case, we not need thread-safe for CCLogger::Get.
CCLogger::Get should call first in CodeCompletion::CodeCompletion():

Code

CodeCompletion::CodeCompletion() :
...
{
    CCLogger::Get()->Init(this, idCCLogger, idCCDebugLogger);

[ollydbg]

Also don't use double locking if you try to make it thread safe:

Which code use double locking? Can you give a direction in the CC's code(trunk HEAD)?

[Loaden]

Also don't use double locking if you try to make it thread safe:

Which code use double locking? Can you give a direction in the CC's code(trunk HEAD)?

if you try to make it thread safe:
 :lol:

[oBFusCATed]

In this case, we not need thread-safe for CCLogger::Get.
CCLogger::Get should call first in CodeCompletion::CodeCompletion():

So you're admitting that you're using this singleton as glorified global variable?

[thomas]

Seeing how I'm the guy who wrote that apparently incomprehensible code, here's an explanation of how this was conceived:

• Logging uses a singleton, yes. There is no issue with that, it does not need locks. Making singletons threadsafe is a stupid, useless endeavor, because it does not work. It isn't an issue either, because the LogManager has been initialized ages before the CC plugin is ever loaded. After that, it is no more than a forwarded pointer.
• If you call DebugLog, this gets redirected (via 2 layers, if I remember correctly) to the concrete Logger object that is currently plugged in the debug_log slot. This enables, in theory, the application to for example start up in "no log" mode in case Code::Blocks runs in absence of GUI or even in absence of a terminal, or it allows a hypothetical Code::Blocks to send you e-mails for log messages.
• Loggers have exactly two requirements: They must implement the pure virtual Append function, and they must work in presence and absence of GUI and in presence or absence of several threads without crashing. Whatever, they must work without crashing.
• Loggerss are explicitly not required to do validation, or to produce meaningful or correct output or any output at all. It is perfectly acceptable for a Logger to output something ill-formatted or nothing at all, as long as it does not crash. If you fill shit into a logger, it's perfectly good if shit comes out of it.
• The reasoning behind the above was that there was the long-existent wish to make a commandline version of Code::Blocks which was impossible because every darn tiny function in the entire program called a log macro which required a GUI, just to output a text that nobody wanted to read, and there was no way to turn this off.
• TextCtrlLogger and his brethren, which are the ones used by default do not use a critical section, that is correct. This means that there is a race condition between control->SetDefaultStyle and control->AppendText. Thus, if an "error" and an "info" are logged concurrently, it is possible that the info is colored as an error or the other way around.
  At the time of writing the class, I considered that since nobody would probably want to write thousands of log messages concurrently from half a dozen threads, this was unnecessary overhead, since the worst thing to happen would be wrongly colored messages, which I deemed a "so what" issue.
• There is no other reason (i.e. crash) which is obvious to me why you could not log concurrently. Unless wxWidgets GUI functions are not re-entrant, which I would seriously hope they are.
• There is, however, also no reason not to insert a critical section inside TextCtrlLogger::Append either, if you believe that this fixes a crash.

[thomas]

In this case, we not need thread-safe for CCLogger::Get.
CCLogger::Get should call first in CodeCompletion::CodeCompletion():

So you're admitting that you're using this singleton as glorified global variable?

That's exactly how the entire project is organized, every "...Manager" class, whether it's configuration, project, buildtargets or whatever.

You can debate whether that's bad design or not, whether singletons are better or worse or the same as global variables, but that's what it is, really. Someone (most probably Yiannis?) started it that way, and everyone (including me) continued that way. The fact that we use singletons everywhere didn't cause a lot of trouble during the years either. Yes, we won't win the first prize in the OOP design contest, so what. The point is, it works, and it works in a kind-of-defined way.

In at least one case (config) there is an urgent reason behind it too, in most other cases I guess one cold probably do otherwise, if one cares to spend many hours rewriting a lot of code.

[oBFusCATed]

• There is no other reason (i.e. crash) which is obvious to me why you could not log concurrently. Unless wxWidgets GUI functions are not re-entrant, which I would seriously hope they are.

They are not, all GUI libs I've seen (MFC/Win32api, QT, wxWidgets) are intended for single threaded usage.

In at least one case (config) there is an urgent reason behind it too, in most other cases I guess one cold probably do otherwise, if one cares to spend many hours rewriting a lot of code.

I've not said to rewrite all existing code. I know it is impossible.
I just want to not make the same mistake with the new code.
This CCLogger::Get()->BlaBla() is more typing, more instructions and more memory accesses for no added benefit/value.

[thomas]

Did you try whether a critical section in TextCtrlLogger::Append does anything?

I don't believe this makes any difference, but you never know if you haven't tried...

[oBFusCATed]

I haven't, the better approach is to send/post a message/event, this will be even asynchronous

[thomas]

Yep, we could send log messages via a TCP socket.

[oBFusCATed]

Hm, probably you forget the smiley in the previous post?