parser test rev 7157 error?

[ollydbg]

Hi, morten, I found that in the rev 7157, you enabled the parsing include files. but look at this:

Code

bool ParserThread::Parse()
{
    wxCriticalSectionLocker locker(g_ParserThreadCritical);

    if (!IS_ALIVE || !InitTokenizer())
        return false;

    TRACE(_T("Parse() : Parsing '%s'"), m_Filename.wx_str());

    bool result = false;
    m_ParsingTypedef = false;

    do
    {
        if (!m_TokensTree || !m_Tokenizer.IsOK())
            break;

        if (!m_Options.useBuffer) // Parse a file
        {
            wxCriticalSectionLocker locker(s_TokensTreeCritical);
            m_FileIdx = m_TokensTree->ReserveFileForParsing(m_Filename);
            if (!m_FileIdx)
                break;
        }

        DoParse();

        if (!m_Options.useBuffer) // Parsing a file
        {
            wxCriticalSectionLocker locker(s_TokensTreeCritical);
            m_TokensTree->FlagFileAsParsed(m_Filename);
        }

        result = true;
    }
    while (false);

    return result;
}

This function will be called recursively, but why the first statement:

Code

wxCriticalSectionLocker locker(g_ParserThreadCritical);

It seems this locker should block the recursive call, but in-fact, it doesn't. I test under mingw+windows. That's was quite strange...

Any ideas?

[MortenMacFly]

It seems this locker should block the recursive call, but in-fact, it doesn't.
Any ideas?

I am aware of this and no, I havent' looked into it. I could imagine that another (wrong) mutex is locked, or, that the mutex is created multiple times. I've marked another (possible) crash anyways, as this still is a work-in-progress. My "masterplan" was to collect the included files and parse them one-by-one to avoid this and (most important) the other crash.

But as it works as it is now I wanted to do an interim-commit, so people see what's happening and we are avoiding conflicts. This is a test project after all, so it's not critical. My intention is to fix the bug with the parser being stalled.

[Loaden]

This function will be called recursively,

No, The DoParse will be called recursively, not is Parse.

It seems this locker should block the recursive call, but in-fact, it doesn't. I test under mingw+windows. That's was quite strange...

A source file, *.h, or *.c/cpp, should call Parse() once.
This locker make them parsed one by one.

[Loaden]

I've marked another (possible) crash anyways, as this still is a work-in-progress.

Sorry, I can't solved the random crash.
I am very happy to see you watched the same issue.

[ollydbg]

Here is my test example:
let the test.h be like below:

Code

int aaaaa;
float bbbbb;
#include <a.h>
#include <b.h>

create "a.h" and "b.h" along with "test.h". (in the same folder)

here is a.h

Code

int in_a_h;
#include "b.h"

Here is b.h

Code

int in_b_h;

Then debug the parsertest project, see the screenshot:


Why does the function "Parse" recursively called???

[Loaden]

One Parser per one thread.
So in here, there has three thread was started.
We use threadpool for batch parsing.

[ollydbg]

One Parser per one thread.
So in here, there has three thread was started.
We use threadpool for batch parsing.

but g_ParserThreadCritical is a global variable, it only allowed one entry.

[MortenMacFly]

Why does the function "Parse" recursively called???

Your example is OK. The implementation of ParserTest provides the global variable "g_ParserCritical". As ParserTest is intatiated several times (inspect the addresses in your debug - they differ) the locker is also created multiple times. And that is OK, because the lock shall apply just the one object itself.

[Loaden]

One Parser per one thread.
So in here, there has three thread was started.
We use threadpool for batch parsing.

but g_ParserThreadCritical is a global variable, it only allowed one entry.

Right, That's the final want.

[ollydbg]

One Parser per one thread.
So in here, there has three thread was started.
We use threadpool for batch parsing.

@loaden, In the parsertest.cbp, we do NOT use any kind of threadpool for batch parsing, do we? The parsertest project does not use multithread, does it?

As ParserTest is intatiated several times (inspect the addresses in your debug - they differ) the locker is also created multiple times. And that is OK, because the lock shall apply just the one object itself.

Yes, they are many different locker instances, but all the instances use the same "static wxCriticalSection g_ParserThreadCritical;"
I have never known about this is a correct thing before, I will try to learn more things about multi-thread programming.

[Loaden]

@loaden, In the parsertest.cbp, we do NOT use any kind of threadpool for batch parsing, do we? The parsertest project does not use multithread, does it?

In ParserTest, we are not use threadpool.
But in CB, we use threadpool, see here.

bool Parser::Parse(const wxString& bufferOrFilename, bool isLocal, ParserThreadOptions& opts)
{
    bool result = false;
    do
    {
        if (!opts.useBuffer)
        {
            wxCriticalSectionLocker locker(s_TokensTreeCritical);

            bool canparse = !m_TokensTree->IsFileParsed(bufferOrFilename);
            if (canparse)
                canparse = m_TokensTree->ReserveFileForParsing(bufferOrFilename, true) != 0;

            if (!canparse)
            {
               if (opts.loader) // if a loader is already open at this point, the caller must clean it up
                   Manager::Get()->GetLogManager()->DebugLog(_T("Parse() : CodeCompletion Plugin: FileLoader memory leak likely while loading file ")+bufferOrFilename);
               break;
            }

            if (!opts.loader) // this should always be true (memory will leak if a loader has already been initialized before this point)
                opts.loader = Manager::Get()->GetFileManager()->Load(bufferOrFilename, m_NeedsReparse);
        }

        TRACE(_T("Parse() : Creating task for: %s"), bufferOrFilename.wx_str());
        ParserThread* thread = new ParserThread(this, bufferOrFilename, isLocal, opts, m_TokensTree);
        bool doParseNow = opts.useBuffer;
#if CC_PARSER_PROFILE_TEST
        doParseNow = true;
#endif
        //if we are parsing a memory buffer or Parser is under Profile
        // CC_PARSER_PROFILE_TEST is defined as 1), then just call Parse()
        // directly and thread pool is NOT used.
        // Otherwise, we are parsing a local file, so the thread pool is used.
        // The ParserThread generated was pushed to the memory pool.

        if (doParseNow)
        {
            result = thread->Parse();
            LinkInheritance(true);
            delete thread;
            break;
        }

        TRACE(_T("Parse() : Parsing %s"), bufferOrFilename.wx_str());

        if (!m_IsPriority)
        {
            TRACE(_T("Parse() : Parallel Parsing %s"), bufferOrFilename.wx_str());

            // Add a task for all project files
            if (m_IsFirstBatch)
            {
                m_IsFirstBatch = false;
                m_PoolTask.push(PTVector());
            }

            if (m_IsParsing)
                m_Pool.AddTask(thread, true);
            else
                m_PoolTask.back().push_back(thread);
        }
        else if (m_IsPriority)
        {
            if (isLocal) // Parsing priority files
            {
                TRACE(_T("Parse() : Parsing priority header, %s"), bufferOrFilename.wx_str());
                result = thread->Parse();
                delete thread;
                break;
            }
            else // Add task when parsing priority files
            {
                TRACE(_T("Parse() : Add task for priority header, %s"), bufferOrFilename.wx_str());
                m_PoolTask.push(PTVector());
                m_PoolTask.back().push_back(thread);
            }
        }

        result = true;
    }
    while (false);

    return result;
}

One header/source file per one ParserThread.
And operating them one by one.

[Loaden]

So, you can create a stand project, and debug again.
This case only happen with parsertest project.

[Loaden]

So, you can create a stand project, and debug again.
This case only happen with parsertest project.

OMG, Seems still have some problem.

bool Parser::Parse(const wxString& bufferOrFilename, bool isLocal, ParserThreadOptions& opts)
{
    bool result = false;
    do
    {
        if (!opts.useBuffer)
        {
            wxCriticalSectionLocker locker(s_TokensTreeCritical);

            bool canparse = !m_TokensTree->IsFileParsed(bufferOrFilename);
            if (canparse)
                canparse = m_TokensTree->ReserveFileForParsing(bufferOrFilename, true) != 0;

            if (!canparse)
            {
               if (opts.loader) // if a loader is already open at this point, the caller must clean it up
                   Manager::Get()->GetLogManager()->DebugLog(_T("Parse() : CodeCompletion Plugin: FileLoader memory leak likely while loading file ")+bufferOrFilename);
               break;
            }

            if (!opts.loader) // this should always be true (memory will leak if a loader has already been initialized before this point)
                opts.loader = Manager::Get()->GetFileManager()->Load(bufferOrFilename, m_NeedsReparse);
        }

        TRACE(_T("Parse() : Creating task for: %s"), bufferOrFilename.wx_str());
        ParserThread* thread = new ParserThread(this, bufferOrFilename, isLocal, opts, m_TokensTree);
        bool doParseNow = opts.useBuffer;
#if CC_PARSER_PROFILE_TEST
        doParseNow = true;
#endif
        //if we are parsing a memory buffer or Parser is under Profile
        // CC_PARSER_PROFILE_TEST is defined as 1), then just call Parse()
        // directly and thread pool is NOT used.
        // Otherwise, we are parsing a local file, so the thread pool is used.
        // The ParserThread generated was pushed to the memory pool.

        if (doParseNow)
        {
            result = thread->Parse();
            LinkInheritance(true);
            delete thread;
            break;
        }

        TRACE(_T("Parse() : Parsing %s"), bufferOrFilename.wx_str());

        if (!m_IsPriority)
        {
            TRACE(_T("Parse() : Parallel Parsing %s"), bufferOrFilename.wx_str());

            // Add a task for all project files
            if (m_IsFirstBatch)
            {
                m_IsFirstBatch = false;
                m_PoolTask.push(PTVector());
            }

            if (m_IsParsing)
                m_Pool.AddTask(thread, true);
            else
                m_PoolTask.back().push_back(thread);
        }
        else if (m_IsPriority)
        {
            if (isLocal) // Parsing priority files
            {
                TRACE(_T("Parse() : Parsing priority header, %s"), bufferOrFilename.wx_str());
                result = thread->Parse(); // ### HERE, some thing is wrong!
                delete thread;
                break;
            }
            else // Add task when parsing priority files
            {
                TRACE(_T("Parse() : Add task for priority header, %s"), bufferOrFilename.wx_str());
                m_PoolTask.push(PTVector());
                m_PoolTask.back().push_back(thread);
            }
        }

        result = true;
    }
    while (false);

    return result;
}

[ollydbg]

@loaden, let's focus our discussion on the ParserTest project.
As there is no multi-thread running thread pool, all the parsing is done in the main thread. and there are some recursive called if the parser meets a #include directive. My puzzle is still the same, as it is only one global critical section, why it cam create different critical section lockers? and it seems these sections were entered several times?

[Loaden]

I'm removed the locker in my local commit (git-svn).
And commit them on tonight.

[oBFusCATed]

ollydbg: on some OSes mutexes could be recursive.
You can do

Code

mutex.lock();
some code
mutex.lock();

some code
mutex.unlock();
mutex.unclock();

This is not recommended, but it is possible. (http://docs.wxwidgets.org/stable/wx_wxmutex.html#wxmutex)

[ollydbg]

ollydbg: on some OSes mutexes could be recursive.
You can do

Code

mutex.lock();
some code
mutex.lock();

some code
mutex.unlock();
mutex.unclock();

This is not recommended, but it is possible. (http://docs.wxwidgets.org/stable/wx_wxmutex.html#wxmutex)

Hi, thanks for the great explanation, I will look into it. I used to know that a critical section can only entered once.

[ollydbg]

I'm removed the locker in my local commit (git-svn).
And commit them on tonight.

I see your latest commit.
As obf said, if the locker can entered several times, it can NOT "lock" any thing. , so they can be removed.

[Loaden]

I'm removed the locker in my local commit (git-svn).
And commit them on tonight.

I see your latest commit.
As obf said, if the locker can entered several times, it can NOT "lock" any thing. , so they can be removed.

But after remove the locker, there should have some issue.
i.g. When do CC search for call tips, and another batch parse is running.
Then, there is not thread safe.
In some where, we shoud add a locker.
But now, I have no idea.

[ollydbg]

CC search for tip should only works when there is NO batch parsing.
Are there any state variable indicate that batch parsing is running?

[Loaden]

CC search for tip should only works when there is NO batch parsing.
Are there any state variable indicate that batch parsing is running?

You can open the CB workspace, and open some project, then worked on first batch parsing project.
You will catch the case.

You can do call tip search, or other local buffer parsing, but another (one or many) is still running.
If any one have good idea, pls let's me know.

[ollydbg]

@loaden, if a workspace contains many projects, and you use only ONE TokensTree for all the cbps, this could be a big issue.

Because the TokensTree is the only resource we should take care.
Every Token collected from the batch parsing was recorded in this TokensTree. So, even some projects are already parsed, there are still projects remaining working and adding Tokens to TokensTree. In the mean while, if you do some search algorithm on the TokensTree, there may be some access conflict.

[oBFusCATed]

Just to note:
The general principals:
1. Shared state/resource should be protected, so only one thread could access it.
2. Protecting (locking) the share state/resource should be done for minimal periods of time. To implement this most of a time something like this can be used:

Code

// do some processing on a temp var;
MySharedResource temp;
temp.process();
mutex.lock()
shared.swap(temp);
mutex.unlock();

3. Using hacks == pain! Don't do it!

You need to identify the shared state and protect it in the best possible way!

[Loaden]

@loaden, if a workspace contains many projects, and you use only ONE TokensTree for all the cbps, this could be a big issue.

No, In current use one parser per one TokensTree.
But there still exist thread-safe issue.
Include the symbols browser.

[Loaden]

Just to note:
The general principals:
1. Shared state/resource should be protected, so only one thread could access it.
2. Protecting (locking) the share state/resource should be done for minimal periods of time. To implement this most of a time something like this can be used:
3. Using hacks == pain! Don't do it!

You need to identify the shared state and protect it in the best possible way!

Thanks, I know that, but I can't find another place need protected again.
oBFusCATed , If you have some time, could you help me check the Parser of CC?

[Loaden]

And I personal think all the random crash related about thread-safe issue.
I can't find a best way to handle thread-safe issue.

[oBFusCATed]

No, no time at the moment, unfortunately.
Another general strategy is to start with one big lock, which does good protection, but makes performance to suffer and then later split this lock in many smaller locks.

Something like:

Code

MyParser::Parse() 
{
   lock();
   all the parsing code();
   unlock();
}

MyCC::DoCompletion()
{
   lock(); // this could use timeout and return if it expires
   gather completion info
   unlock();
}

[ollydbg]

I was aware that we use "critical section" in cc, not mutex.
I thing generally, critical section is only allowed ONE entry.
From the document:
http://docs.wxwidgets.org/stable/wx_wxcriticalsectionlocker.html
a critical section locker is just enter the section on its constructor, and leave the section in its destructor.
So, I still get puzzled what why they can still re-entry.

As morten said:

Your example is OK. The implementation of ParserTest provides the global variable "g_ParserCritical". As ParserTest is intatiated several times (inspect the addresses in your debug - they differ) the locker is also created multiple times. And that is OK, because the lock shall apply just the one object itself.

As a locker is created, this means it will enter the critical section on its constructor. This means the same critical section entered several times. why?

[oBFusCATed]

Ollydbg: you're messing the concepts

There is wxMutex and wxMutexLocker... wxMutexLocker is to have a RAII style locking/unlocking. wxMutex is the real synchronization object.
Same is for wxCriticalSection and wxCriticalSectionLocker.

You can read about RAII here: http://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization

[ollydbg]

Hi, OBF, thanks for your help.
In-fact, I know RAII mechanism.

Now, I can solve my puzzle by reading this page:
Critical Section Objects (Windows)

and look:

When a thread owns a critical section, it can make additional calls to EnterCriticalSection or TryEnterCriticalSection without blocking its execution. This prevents a thread from deadlocking itself while waiting for a critical section that it already owns. To release its ownership, the thread must call LeaveCriticalSection one time for each time that it entered the critical section. There is no guarantee about the order in which waiting threads will acquire ownership of the critical section.

That is to say: In the same thread, one critical section can be re-entry. , , so a critical section is only try to lock ANOTHER thread, not the one itself.

[ollydbg]

@morten and loaden:
return back to my example code:
http://forums.codeblocks.org/index.php/topic,14733.msg98909.html#msg98909

I also enabled the log:

In the file parserdummy.cpp, line 102

Code

    ParserTest pt;
    pt.Clear();
    ParserTrace(_T("-----------I-n-t-e-r-i-m--L-o-g-----------"));
    pt.Start(filename);
    // TODO: The following lines cause a crash in
    ParserTrace(_T("--------------T-r-e-e--L-o-g--------------"));
    pt.PrintTree();
    ParserTrace(_T("--------------L-i-s-t--L-o-g--------------"));
    pt.PrintList();

it works fine.
But you can see the log:

--------------M-a-i-n--L-o-g--------------

000001. ParserDummy() : Parser() : Instantiation of Parser object.
000002. InitTokenizer() : m_Filename='test.h', m_FileSize=60.
000003. Init() : m_Filename='test.h'
000004. Parse() : Parsing 'test.h'
000005. DoParse() : Loop:m_Str='', token='int'
000006. DoParse() : Loop:m_Str='int ', token='aaaaa'
000007. DoAddToken() : Created token='aaaaa', file_idx=1, line=2, ticket=
000008. GetActualTokenType() : Searching within m_Str='int'
000009. GetActualTokenType() : Compensated m_Str='int'
000010. GetActualTokenType() : Found 'int'
000011. DoAddToken() : Prepending ''
000012. DoAddToken() : Added/updated token 'aaaaa' (0), kind 'variable', type 'int', actual 'int'. Parent is  (-1)
000013. DoParse() : Loop:m_Str='', token='float'
000014. DoParse() : Loop:m_Str='float ', token='bbbbb'
000015. DoAddToken() : Created token='bbbbb', file_idx=1, line=3, ticket=
000016. GetActualTokenType() : Searching within m_Str='float'
000017. GetActualTokenType() : Compensated m_Str='float'
000018. GetActualTokenType() : Found 'float'
000019. DoAddToken() : Prepending ''
000020. DoAddToken() : Added/updated token 'bbbbb' (1), kind 'variable', type 'float', actual 'float'. Parent is  (-1)
000021. DoParse() : Loop:m_Str='', token='#'
000022. HandleIncludes() : Found include file 'a.h'
000023. ParserDummy() : GetFullFileName() : Querying full file name for source 'test.h', target 'a.h' (isGlobal=true).
000024. HandleIncludes() : Adding include file 'a.h'
000025. ParserDummy() : DoParseFile() : Parse file request for file name 'a.h' (isGlobal=true)
000026. ParserDummy() : Parse() : Parsing file 'a.h' (isLocal=false).
000027. ParserDummy() : Parse() : Creating new parser thread for 'a.h'
000028. -----------I-n-t-e-r-i-m--L-o-g-----------
000029. ParserDummy() : Parser() : Instantiation of Parser object.
000030. InitTokenizer() : m_Filename='a.h', m_FileSize=29.
000031. Init() : m_Filename='a.h'
000032. Parse() : Parsing 'a.h'
000033. DoParse() : Loop:m_Str='', token='int'
000034. DoParse() : Loop:m_Str='int ', token='in_a_h'
000035. DoAddToken() : Created token='in_a_h', file_idx=1, line=1, ticket=
000036. GetActualTokenType() : Searching within m_Str='int'
000037. GetActualTokenType() : Compensated m_Str='int'
000038. GetActualTokenType() : Found 'int'
000039. DoAddToken() : Prepending ''
000040. DoAddToken() : Added/updated token 'in_a_h' (0), kind 'variable', type 'int', actual 'int'. Parent is  (-1)
000041. DoParse() : Loop:m_Str='', token='#'
000042. HandleIncludes() : Found include file 'b.h'
000043. ParserDummy() : GetFullFileName() : Querying full file name for source 'a.h', target 'b.h' (isGlobal=true).
000044. HandleIncludes() : Adding include file 'b.h'
000045. ParserDummy() : DoParseFile() : Parse file request for file name 'b.h' (isGlobal=true)
000046. ParserDummy() : Parse() : Parsing file 'b.h' (isLocal=false).
000047. ParserDummy() : Parse() : Creating new parser thread for 'b.h'
000048. -----------I-n-t-e-r-i-m--L-o-g-----------
000049. ParserDummy() : Parser() : Instantiation of Parser object.
000050. InitTokenizer() : m_Filename='b.h', m_FileSize=13.
000051. Init() : m_Filename='b.h'
000052. Parse() : Parsing 'b.h'
000053. DoParse() : Loop:m_Str='', token='int'
000054. DoParse() : Loop:m_Str='int ', token='in_b_h'
000055. DoAddToken() : Created token='in_b_h', file_idx=1, line=1, ticket=
000056. GetActualTokenType() : Searching within m_Str='int'
000057. GetActualTokenType() : Compensated m_Str='int'
000058. GetActualTokenType() : Found 'int'
000059. DoAddToken() : Prepending ''
000060. DoAddToken() : Added/updated token 'in_b_h' (0), kind 'variable', type 'int', actual 'int'. Parent is  (-1)
000061. ParserDummy() : ~Parser() : Destruction of Parser object.
000062. --------------T-r-e-e--L-o-g--------------
000063. int in_b_h   [1,0]
000064. --------------L-i-s-t--L-o-g--------------
000065. variable int in_b_h   [1,0]
000066. ParserDummy() : ~Parser() : Destruction of Parser object.
000067. --------------T-r-e-e--L-o-g--------------
000068. int in_a_h   [1,0]
000069. --------------L-i-s-t--L-o-g--------------
000070. variable int in_a_h   [1,0]
000071. DoParse() : Loop:m_Str='', token='#'
000072. HandleIncludes() : Found include file 'b.h'
000073. ParserDummy() : GetFullFileName() : Querying full file name for source 'test.h', target 'b.h' (isGlobal=true).
000074. HandleIncludes() : Adding include file 'b.h'
000075. ParserDummy() : DoParseFile() : Parse file request for file name 'b.h' (isGlobal=true)
000076. ParserDummy() : Parse() : Parsing file 'b.h' (isLocal=false).
000077. ParserDummy() : Parse() : File 'b.h' has already been parsed
000078. ParserDummy() : ~Parser() : Destruction of Parser object.


--------------T-r-e-e--L-o-g--------------

000079. int aaaaa   [2,0]
000080. float bbbbb   [3,0]


--------------L-i-s-t--L-o-g--------------

000081. variable int aaaaa   [2,0]
000082. variable float bbbbb   [3,0]

Which means: the whole parsing stage will have Three TokensTree instants to be allocated and then destroyed. This is not necessory, I think One TokensTree is enough.