When tracking the dirty lockers in CC, I hit a crash, which has the following back-trace
[debug]> bt 30
[debug]#0 0x77c47fd4 in wcslen () from C:\WINDOWS\system32\msvcrt.dll
[debug]#1 0x01d37e3f in wxPrintfConvSpec<wchar_t>::Process(wchar_t*, unsigned int, wxPrintfArg*, unsigned int) () from E:\code\wx\wxWidgets-3.0.0\lib\gcc_dll\wxmsw30u_gcc_cb.dll
[debug]#2 0x0148362e in int wxDoVsnprintf<wchar_t>(wchar_t*, unsigned int, wchar_t const*, char*) () from E:\code\wx\wxWidgets-3.0.0\lib\gcc_dll\wxmsw30u_gcc_cb.dll
[debug]#3 0x01481de1 in wxVsnprintf(wchar_t*, unsigned int, wxString const&, char*) () from E:\code\wx\wxWidgets-3.0.0\lib\gcc_dll\wxmsw30u_gcc_cb.dll
[debug]#4 0x0144f9cc in DoStringPrintfV(wxString&, wxString const&, char*) () from E:\code\wx\wxWidgets-3.0.0\lib\gcc_dll\wxmsw30u_gcc_cb.dll
[debug]#5 0x0144fd02 in wxString::PrintfV(wxString const&, char*) () from E:\code\wx\wxWidgets-3.0.0\lib\gcc_dll\wxmsw30u_gcc_cb.dll
[debug]#6 0x0144fd54 in wxString::FormatV(wxString const&, char*) () from E:\code\wx\wxWidgets-3.0.0\lib\gcc_dll\wxmsw30u_gcc_cb.dll
[debug]#7 0x051ecd47 in F (msg=0x9178d44 L"%lss.Lock().Success() :%lsls()%ls%ls, %d") at F:\cb_sf_git\trunk\src\include\logmanager.h:26
[debug]#8 0x051c4484 in Parser::IsFileParsed (this=0x88c8b70, filename=...) at F:\cb_sf_git\trunk\src\plugins\codecompletion\parser\parser.cpp:1168
[debug]#9 0x05174dc7 in CodeCompletion::ParseFunctionsAndFillToolbar (this=0x4400320) at F:\cb_sf_git\trunk\src\plugins\codecompletion\codecompletion.cpp:3444
[debug]#10 0x0517782d in CodeCompletion::OnToolbarTimer (this=0x4400320, event=...) at F:\cb_sf_git\trunk\src\plugins\codecompletion\codecompletion.cpp:3836
[debug]#11 0x013a2e62 in wxAppConsoleBase::HandleEvent(wxEvtHandler*, void (wxEvtHandler::*)(wxEvent&), wxEvent&) const () from E:\code\wx\wxWidgets-3.0.0\lib\gcc_dll\wxmsw30u_gcc_cb.dll
[debug]#12 0x013a31cd in wxAppConsoleBase::CallEventHandler(wxEvtHandler*, wxEventFunctor&, wxEvent&) const () from E:\code\wx\wxWidgets-3.0.0\lib\gcc_dll\wxmsw30u_gcc_cb.dll
[debug]#13 0x014e1b21 in wxEvtHandler::ProcessEventIfMatchesId(wxEventTableEntryBase const&, wxEvtHandler*, wxEvent&) () from E:\code\wx\wxWidgets-3.0.0\lib\gcc_dll\wxmsw30u_gcc_cb.dll
[debug]#14 0x014e1e86 in wxEvtHandler::SearchDynamicEventTable(wxEvent&) () from E:\code\wx\wxWidgets-3.0.0\lib\gcc_dll\wxmsw30u_gcc_cb.dll
[debug]#15 0x014e1f26 in wxEvtHandler::TryHereOnly(wxEvent&) () from E:\code\wx\wxWidgets-3.0.0\lib\gcc_dll\wxmsw30u_gcc_cb.dll
[debug]#16 0x014e1fcd in wxEvtHandler::ProcessEventLocally(wxEvent&) () from E:\code\wx\wxWidgets-3.0.0\lib\gcc_dll\wxmsw30u_gcc_cb.dll
[debug]#17 0x014e2023 in wxEvtHandler::ProcessEvent(wxEvent&) () from E:\code\wx\wxWidgets-3.0.0\lib\gcc_dll\wxmsw30u_gcc_cb.dll
[debug]#18 0x014e1db0 in wxEvtHandler::SafelyProcessEvent(wxEvent&) () from E:\code\wx\wxWidgets-3.0.0\lib\gcc_dll\wxmsw30u_gcc_cb.dll
[debug]#19 0x01462df8 in wxTimerImpl::SendEvent() () from E:\code\wx\wxWidgets-3.0.0\lib\gcc_dll\wxmsw30u_gcc_cb.dll
[debug]#20 0x014ca912 in wxTimerWndProc(HWND__*, unsigned int, unsigned int, long)@16 () from E:\code\wx\wxWidgets-3.0.0\lib\gcc_dll\wxmsw30u_gcc_cb.dll
[debug]#21 0x7e418734 in USER32!GetDC () from C:\WINDOWS\system32\user32.dll
[debug]#22 0x000e0a12 in ?? ()
[debug]#23 0x7e418816 in USER32!GetDC () from C:\WINDOWS\system32\user32.dll
[debug]#24 0x014ca880 in wxProcessTimer(wxMSWTimerImpl&) () from E:\code\wx\wxWidgets-3.0.0\lib\gcc_dll\wxmsw30u_gcc_cb.dll
[debug]#25 0x00000113 in ?? ()
[debug]#26 0x7e4189cd in USER32!GetWindowLongW () from C:\WINDOWS\system32\user32.dll
[debug]#27 0x00000000 in ?? ()
Track down to the file here:
/*
* This file is part of the Code::Blocks IDE and licensed under the GNU Lesser General Public License, version 3
* http://www.gnu.org/licenses/lgpl-3.0.html
*/
#ifndef LOGMGR_H
#define LOGMGR_H
#include "manager.h"
#include "logger.h"
#include <map>
//namespace cb
//{
inline wxString F(const wxChar* msg, ...)
{
va_list arg_list;
va_start(arg_list, msg);
#if wxCHECK_VERSION(2,9,0) && wxUSE_UNICODE
// in wx >= 2.9 unicode-build (default) we need the %ls here, or the strings get
// cut after the first character
::temp_string = msg;
::temp_string.Replace(_T("%s"), _T("%ls"));
msg = ::temp_string.wx_str();
#endif
::temp_string = wxString::FormatV(msg, arg_list); //******************************************
va_end(arg_list);
return ::temp_string;
}
//} // namespace cb
It use a global wxString, this is too bad as I can see, especially in a multiply thread application.
I see that one thread is calling here:
bool Parser::IsFileParsed(const wxString& filename)
{
bool isParsed = false;
CC_LOCKER_TRACK_TT_MTX_LOCK(s_TokenTreeMutex) //***********************************here
isParsed = m_TokenTree->IsFileParsed(filename);
Another thread is calling here:
bool Parser::Parse(const wxString& filename, bool isLocal, bool locked, LoaderBase* loader)
{
ParserThreadOptions opts;
opts.useBuffer = false;
opts.bufferSkipBlocks = false;
opts.bufferSkipOuterBlocks = false;
opts.followLocalIncludes = m_Options.followLocalIncludes;
opts.followGlobalIncludes = m_Options.followGlobalIncludes;
opts.wantPreprocessor = m_Options.wantPreprocessor;
opts.parseComplexMacros = m_Options.parseComplexMacros;
opts.storeDocumentation = m_Options.storeDocumentation;
opts.loader = loader; // maybe 0 at this point
bool result = false;
do
{
bool canparse = false;
{
if (!locked)
CC_LOCKER_TRACK_TT_MTX_LOCK(s_TokenTreeMutex)
canparse = !m_TokenTree->IsFileParsed(filename);
if (canparse)
canparse = m_TokenTree->ReserveFileForParsing(filename, true) != 0;
if (!locked)
CC_LOCKER_TRACK_TT_MTX_UNLOCK(s_TokenTreeMutex) //**********************************here
}
So, they are both calling the same function.
Maybe a wx native log function is better? like wxLogMessage(), which support the printf style format string.
What's your opinion?
Found another issue, static variables in header files, see, the logger.h has those contents.
/*
* This file is part of the Code::Blocks IDE and licensed under the GNU Lesser General Public License, version 3
* http://www.gnu.org/licenses/lgpl-3.0.html
*/
#ifndef LOGGER_H
#define LOGGER_H
#include "prep.h"
#include <wx/string.h>
#include "settings.h" // DLLIMPORT
class wxMenu;
class wxWindow;
namespace
{
static wxString temp_string(_T('\0'), 250);
static wxString newline_string(_T("\n"));
}
So, mostly, F() from different translation units have their own temp_string.
But look at my crash issue, two different thread call functions in a single translation unit (both in parser.cpp), so it crashed.
This seems to work fine (at least it doesn't work worse than the pristine sources for me, no compile error and no crash):
Index: include/logmanager.h
===================================================================
--- include/logmanager.h (revision 9550)
+++ include/logmanager.h (working copy)
@@ -10,25 +10,32 @@
#include "logger.h"
#include <map>
-//namespace cb
-//{
- inline wxString F(const wxChar* msg, ...)
+ namespace pfffffffrt
+ {
+ // avoid calling wxString::wxString(const char*) twice for every single message
+ static const wxString s(_T("%s"));
+ static const wxString ls(_T("%ls"));
+ }
+
+ inline wxString const& F(const wxChar* msg, ...)
{
va_list arg_list;
va_start(arg_list, msg);
+
#if wxCHECK_VERSION(2,9,0) && wxUSE_UNICODE
-// in wx >= 2.9 unicode-build (default) we need the %ls here, or the strings get
-// cut after the first character
- ::temp_string = msg;
- ::temp_string.Replace(_T("%s"), _T("%ls"));
- msg = ::temp_string.wx_str();
+ // in wx >= 2.9 unicode-build (default) we need the %ls here, or the strings get
+ // cut after the first character
+ using namespace pfffffffrt;
+ wxString tmp(msg);
+ tmp.Replace(s, ls);
+ tmp = wxString::FormatV(tmp.wx_str(), arg_list)
+#else
+ wxString tmp(wxString::FormatV(msg, arg_list));
#endif
- ::temp_string = wxString::FormatV(msg, arg_list);
+
va_end(arg_list);
-
- return ::temp_string;
+ return tmp;
}
-//} // namespace cb
struct LogSlot
There is however another, more serious issue insofar as the text control loggers (which I didn't write, luckily, so I can blame Yiannis!) seem to use those globals all over the place, too. For no apparent reason as well, no idea why it's done that way. We'll probably need to search through those too.
You can make them local functions statics, I think...
Yes, except function local statics effectively result in a sort of if(initialized) foo = initializer; code sequence, and in C++11 these are required to be initialized thread-safe, and GCC since about version 4.6 or 4.7 does that regardless of C++11 anyway (unless you compile with -fno-threadsafe-statics), using a mutex. Now of course, locking a mutex makes the point of trying to avoid initilizing a string somewhat... redundant.
Not sure if the compiler is smart enough to elide the copy anyway.
Compiler writers, tells us that they are smart enough... http://cpp-next.com/archive/2009/08/want-speed-pass-by-value/
Do you agree that the return type should be changed?
I'm know this article, but I'm not convinced that it applies to us. For example, we don't follow the guideline of not copying arguments (obviously) and your return value is not anonymous, so at best NRVO can be used, not RVO. Also, var-args are a kind of ugly hack, and I'm not sure inhowfar it influences optimizations such as NRVO.
But I'm OK if you think that returning by value is a better choice, even if it does create a copy. A verbatim copy on a wxString is only incrementing a reference counter.
Feel free to choose whatever makes you feel most comfortable.
What worries me more than those micro-optimizations is code like this:
void TextCtrlLogger::Append(const wxString& msg, Logger::level lv)
{
if (!control)
return;
::temp_string.assign(msg);
::temp_string.append(_T("\n"));
if (lv == caption)
{
control->SetDefaultStyle(style[info]);
control->AppendText(::newline_string);
control->SetDefaultStyle(style[lv]);
control->AppendText(::temp_string);
control->SetDefaultStyle(style[spacer]);
control->AppendText(::newline_string);
}
else
{
control->SetDefaultStyle(style[lv]);
control->AppendText(::temp_string);
}
}
The F function as it was is kind of "semi threadsafe" insofar as all modules that were supposed to use it (mostly compiler, and maybe project mgr) run at most one thread per module. Now the TextCtrlLogger such as being used for the debug log on the other hand is definitely thread-unsafe, but I don't even understand why it uses the global here at all. Appending a newline would make much more sense on a plain normal local variable, no?
Well, the standard is totally fine with returning a const reference to a local variable, since this extends the lifetime of the local to the lifetime of the reference.
You are so wrong here....
See an example:
#include <stdio.h>
#include <string>
const std::string & func()
{
std::string temp("value");
return temp;
}
int main()
{
std::string s=func();
return 0;
}
g++ -Wall -Wextra return_ref.cpp
return_ref.cpp: In function ‘const string& func()’:
return_ref.cpp:6:14: warning: reference to local variable ‘temp’ returned [enabled by default]
http://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-const/
What worries me more than those micro-optimizations is code like this:
void TextCtrlLogger::Append(const wxString& msg, Logger::level lv)
{
if (!control)
return;
::temp_string.assign(msg);
::temp_string.append(_T("\n"));
if (lv == caption)
{
control->SetDefaultStyle(style[info]);
control->AppendText(::newline_string);
control->SetDefaultStyle(style[lv]);
control->AppendText(::temp_string);
control->SetDefaultStyle(style[spacer]);
control->AppendText(::newline_string);
}
else
{
control->SetDefaultStyle(style[lv]);
control->AppendText(::temp_string);
}
}
The F function as it was is kind of "semi threadsafe" insofar as all modules that were supposed to use it (mostly compiler, and maybe project mgr) run at most one thread per module. Now the TextCtrlLogger such as being used for the debug log on the other hand is definitely thread-unsafe, but I don't even understand why it uses the global here at all. Appending a newline would make much more sense on a plain normal local variable, no?
The function:
Manager::Get()->GetLogManager()->DebugLog()
Manager::Get()->GetLogManager()->Log()
are totally thread un-safe, so they can only be used in the main GUI thread.
So, if you have debug messages to be sent from a worker thread, you need to implement your own thread safe log function.
BTW: Is it possible to use wxLog functions in wxWidgets, such as wxLogMessage, since they can redirect the messages to a wxTextCtrl, and I see in wx document, they are thread safe.
I'd prefer if you start a branch on github or show a patch before committing.
OK, I will show a patch file here later.
And please name the function SafeF or ThreadedF.
Also why don't you just use wxString::Format it does the same but it is a bit longer to type?
Oh, yes, We can use this build-in wxWidgets functions, but I'm not sure it handle the %ls and %s replacement well:
+#if wxCHECK_VERSION(2,9,0) && wxUSE_UNICODE
+// in wx >= 2.9 unicode-build (default) we need the %ls here, or the strings get
+// cut after the first character
+ wxString tmp(msg);
+ tmp.Replace(s, ls);
+ tmp = wxString::FormatV(tmp.wx_str(), arg_list)
+#else
+ wxString tmp(wxString::FormatV(msg, arg_list));
+#endif
I don't really understand why we need %ls for wx verison >2.9.0.
If we use wxString::Format, what should we use there? %ls or %s ?
Then probably place a upper limit to the version check macro or just remove it.
I prefer remove it.
But I see this already happens years ago, but later reverted.
See:
Revision: a3cea10c6e1ba93287aef84a7dc3c34ab879e8da
Author: jenslody <jenslody@2a5c6006-c6dd-42ca-98ab-0921f2732cef>
Date: 2013-6-1 1:14:14
Message:
* Revert last commit, because logging is still broken in wx2.9 unicode (at least in 64bit)
git-svn-id: http://svn.code.sf.net/p/codeblocks/code/trunk@9125 2a5c6006-c6dd-42ca-98ab-0921f2732cef
----
Modified: src/include/logmanager.h
Modified: src/plugins/contrib/envvars/envvars_common.cpp
Revision: 67a9f7cee0ca671cc7baafe78436f319b89395be
Author: biplab <biplab@2a5c6006-c6dd-42ca-98ab-0921f2732cef>
Date: 2013-5-31 22:20:59
Message:
* Reverted: Rev 8259 as it may not be needed anymore. However if original bug persists then revert this commit.
git-svn-id: http://svn.code.sf.net/p/codeblocks/code/trunk@9124 2a5c6006-c6dd-42ca-98ab-0921f2732cef
----
Modified: src/include/logmanager.h
Modified: src/plugins/contrib/envvars/envvars_common.cpp
@Jens or other devs, can you confirm change in rev9124 is OK with 64 bit Linux system (with wx 3.0)? Thanks.
[OT] I think wxWidgets has made the badest decision they could have made. Switching to wchar_t is the box of the Pandora, because you can't say it it is 16bit or 32bit or even 64bit. The only right decision would be to have used UTF8 with char also on windows, and add some translation api... [\OT]
I don't have an option, because the different OS have too many string encodings. Maybe, wx guys just want wxString in wx 3.0 be consistent with 2.8.x.
I recommend you to not use the buffer directly but always strings and the append or stream functions to combine strings and other values. This can maybe slow down the whole process, but it is a clean and save way...
greetings
Not only the F() function, but there are many places in CC use directly wxChar*. It is OK if wxString use fixed length code encoding, in this case, wxChar == wchar_t.