Developer forums (C::B DEVELOPMENT STRICTLY!) > CodeCompletion redesign
F() function is not thread safe?
ollydbg:
When tracking the dirty lockers in CC, I hit a crash, which has the following back-trace
--- Code: ---[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 ?? ()
--- End code ---
Track down to the file here:
--- Code: ---/*
* 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
--- End code ---
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:
--- Code: ---bool Parser::IsFileParsed(const wxString& filename)
{
bool isParsed = false;
CC_LOCKER_TRACK_TT_MTX_LOCK(s_TokenTreeMutex) //***********************************here
isParsed = m_TokenTree->IsFileParsed(filename);
--- End code ---
Another thread is calling here:
--- Code: ---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
}
--- End code ---
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?
ollydbg:
Found another issue, static variables in header files, see, the logger.h has those contents.
--- Code: ---/*
* 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"));
}
--- End code ---
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.
thomas:
Function named F found in a file that contains the string LOGGER looks typically like something I may have written some years ago as most for internal use. Though #if wxCHECK_VERSION(...) suggests that some edits have been made.
Though I don't remember now what the intent was, need to look (and try to understand) first. I think the whole purpose of this function was for formatting debug messages with a timestamp or something before they are appended to the log.
I don't remember why this function was designed in such an obtuse way, I don't think anything returned by F should need to be modified afterwards. But quite possibly, that was the very reason for this design (I seem to remember that the use of one static per translation unit was a deliberate choice to make it "quasi-threadsafe").
If the returned strings are not needed to be modifiable, one should be able to simply return a const& to a local object, which would make F threadsafe.
thomas:
This seems to work fine (at least it doesn't work worse than the pristine sources for me, no compile error and no crash):
--- Code: ---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
--- End code ---
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.
MortenMacFly:
--- Quote from: thomas on January 09, 2014, 06:00:02 pm ---This seems to work fine (at least it doesn't work worse than the pristine sources for me, no compile error and no crash):
--- End quote ---
Its always fun to see how you manage to make the code more trustfully and readable... pfffffffrt... ;-)
Navigation
[0] Message Index
[#] Next page
Go to full version