Author Topic: Serious bug in revision 7355  (Read 7365 times)

Offline jens

  • Administrator
  • Lives here!
  • *****
  • Posts: 7255
Serious bug in revision 7355
« on: August 10, 2011, 07:23:32 pm »
I just got a mail from a user who found a serious bug in CC.

I was able to track it down to svn r7355:
if using "One parser for whole workspace" with a workspace, that contains two or more projects, CC hangs after parsing the first project.
Last message is: Class browser updated.

It does not happen with "One parser per project".

No time to dig into it deeper at the moment, have to bring my children to bed.

Offline jens

  • Administrator
  • Lives here!
  • *****
  • Posts: 7255
Re: Serious bug in revision 7355
« Reply #1 on: August 10, 2011, 09:40:01 pm »
I fixed the deadlock in trunk (svn r7361).
Not sure whether it's the best way, but it works.

Here is the patch I used:

Code
Index: parser.cpp
===================================================================
--- parser.cpp (Revision 7360)
+++ parser.cpp (Arbeitskopie)
@@ -1001,12 +1001,14 @@
 
 bool Parser::ForceStartParsing()
 {
-    wxCriticalSectionLocker locker(s_ParserCritical);
     if (!m_IsParsing && !m_BatchTimer.IsRunning() && !Done())
     {
-        m_BatchTimer.Start(100, wxTIMER_ONE_SHOT);
         if (m_ParsingType == ptUndefined)
+        {
+            wxCriticalSectionLocker locker(s_ParserCritical);
             m_ParsingType = ptAddFileToParser;
+        }
+        m_BatchTimer.Start(100, wxTIMER_ONE_SHOT);
         return true;
     }
     else

The deadlock has occured, because ForceStartParsing uses s_ParserCrtical as wxCriticalSectionLocker and Done() does the same.
I moved the locker into the body of the if-clause, because that's the only place where something is changed.
I also moved the timer start, so it is surely fired after changing the parsing-type.

@cc-gurus (Loaden and ollydbg) :
please check the changes, I did not find any side-effects.

Offline Loaden

  • Lives here!
  • ****
  • Posts: 1014
Re: Serious bug in revision 7355
« Reply #2 on: August 11, 2011, 12:48:59 am »
Many thanks!
But I have a question: m_IsParsing will access from a sub-thread.
If we only read a value, like m_IsParsing, m_TaskPool (stl::set), and do not write it, you can sure we not need add a locker for thread safe?
e.g. Done function:
Code
bool Parser::Done()
{
    wxCriticalSectionLocker locker(s_ParserCritical);

    bool done =    m_PriorityHeaders.empty()
                && m_SystemPriorityHeaders.empty()
                && m_BatchParseFiles.empty()
                && m_PredefinedMacros.IsEmpty()
                && !m_NeedMarkFileAsLocal
                && m_PoolTask.empty()
                && m_Pool.Done();

    return done;
}

In Here the function only read the members, no write, so we must use a locker or noneed?
« Last Edit: August 11, 2011, 12:57:30 am by Loaden »

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9660
Re: Serious bug in revision 7355
« Reply #3 on: August 11, 2011, 07:29:19 am »
In Here the function only read the members, no write, so we must use a locker or noneed?
If another thread might change the content of any of the variables you are checking, a lock is needed. Basically you don't want parallel read and write access. If you can ensure all threads are only reading at the same time, the locker would not be needed.
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: http://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: http://wiki.codeblocks.org/index.php?title=FAQ

Offline jens

  • Administrator
  • Lives here!
  • *****
  • Posts: 7255
Re: Serious bug in revision 7355
« Reply #4 on: August 11, 2011, 12:23:32 pm »
In Here the function only read the members, no write, so we must use a locker or noneed?
If another thread might change the content of any of the variables you are checking, a lock is needed. Basically you don't want parallel read and write access. If you can ensure all threads are only reading at the same time, the locker would not be needed.
Is it possible, that m_IsParsing is changed from false to true, while we are in ForceStartParsing ?

If yes and we need the locker, we could probably split the if-clause, like this:

Code
    if (!Done())
    {
        wxCriticalSectionLocker locker(s_ParserCritical);
        if (!m_IsParsing && !m_BatchTimer.IsRunning())
        {
            if (m_ParsingType == ptUndefined)
                m_ParsingType = ptAddFileToParser;
            m_BatchTimer.Start(100, wxTIMER_ONE_SHOT);
            return true;
        }
    }

That should also avoid the deadlock, but lock m_IsParsing and m_BatchTimer.IsRunning() (if the second is needed).