I've reported the problem here:
http://forums.next.codeblocks.org/index.php/topic,19753.msg134925.html#msg134925
http://forums.next.codeblocks.org/index.php/topic,19753.msg134942.html#msg134942
And I've debugged it a bit more and I know what code pattern breaks it or at least this is what cause it to loop.
You can see it in the following workspace http://cmpt.benbmp.org/codeblocks/nightlies/cc_marco_infinite.zip
Unfortunately when this is extracted from the original code it doesn't have the same effect.
So this project cannot be used to reproduce the problem. :(
ollydbg: Can you look at the project and see if it can give you some hints what is going wrong with this code?
Quote from: oBFusCATed on November 30, 2014, 05:06:36 PM
ollydbg: Can you look at the project and see if it can give you some hints what is going wrong with this code?
I have tried your test workspace, but I don't get much hints. :(
Any hints how to debug it further or how to find what is the thing that causes the problem?
Quote from: oBFusCATed on December 01, 2014, 03:06:49 PM
Any hints how to debug it further or how to find what is the thing that causes the problem?
I have an idea. You can check how many times the function DoAddToken() was called. So, just set a breakpoint in the function, and count the times, if it get a very large number, there indicates a bad situation. E.g. If you workspace contains about 10000 tokens(this can be seen from an old version of C::B which don't cause the infinite loop), and you see DoAddToken() has been called 20000 times in the newer C::B, then it may be in an infinite loop now, and you can step in and see where the infinite loop are.
OK, I've spend some more time and now I was able to make a project that reproduces the problem.
Here it is http://cmpt.benbmp.org/codeblocks/nightlies/cc_marco_infinite2.tar.gz
ollydbg: Can you try to see if you can reproduce and fix the problem?
OK. I will test it.
I see infinite loop too. :(
Bug located. Patch here:
src/plugins/codecompletion/parser/tokenizer.cpp | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/plugins/codecompletion/parser/tokenizer.cpp b/src/plugins/codecompletion/parser/tokenizer.cpp
index 764bcbe..6202eda 100644
--- a/src/plugins/codecompletion/parser/tokenizer.cpp
+++ b/src/plugins/codecompletion/parser/tokenizer.cpp
@@ -1794,6 +1794,10 @@ bool Tokenizer::ReplaceBufferText(const wxString& target)
m_TokenIndex = m_BufferLen - m_FirstRemainingLength;
m_PeekAvailable = false;
SkipToEOL(false);
+ SkipToOneOfChars(_T(",;}"), true, false, false);
+ m_SavedTokenIndex = m_UndoTokenIndex = m_TokenIndex;
+ m_SavedLineNumber = m_UndoLineNumber = m_LineNumber;
+ m_SavedNestingLevel = m_UndoNestLevel = m_NestLevel;
return false;
}
else
It is a workaround patch and will cause parsing the Test class body wrongly, you will notice one extra member in the symbol tree :). But it works around a bug that macro replacement cause infinite loop when parsing. The fundamental solution is to ONLY do macro replacement in the Tokenizer level. (We currently do in both ParserThread and Tokenizer).
Can you test it?
I noticed another issue is that token numbers are becomes large because we save all the "macro usage tokens". I think they should not be saved in the tokentree, they are just like function call. Since we only restore the function declaration/definition, we don't save the function call, I think we also don't need to store the macro usage.
About macro usage check in the Tokenizer level, I have locally implement this feature, and it solve the bug Macro replacement in CC should remember the used macro definition (https://sourceforge.net/p/codeblocks/tickets/46/)(this is the similar issue as OBF's test case), but my local implementation contains many commits and massive code change in Tokenizer and Parserthread class. :(
Yes, it does prevent the infinite loop and yes it screws the members of the struct that are declared after the broken member.
If you have some code that needs testing post a branch on github or even a patch and I'll give it a try.
Quote from: oBFusCATed on January 23, 2015, 07:35:18 PM
Yes, it does prevent the infinite loop and yes it screws the members of the struct that are declared after the broken member.
If you have some code that needs testing post a branch on github or even a patch and I'll give it a try.
OK, after taking several hours to clean up my local git(merge some patches, split some patches, move some patches up and down) , I create a patches serials which contains 31 patches which are against SVN rev 10081.
Any comments are welcome. :)
Quote from: ollydbg on January 24, 2015, 02:45:42 PM
Any comments are welcome. :)
No patches against SVN, and/or does not apply against SVN, so I am out (as usual)...
Morten: Check this branch out https://github.com/obfuscated/codeblocks_sf/tree/ollydbg/cc_macro it has all the commits applied.
@ollydbg:
I don't see any patches that add tests.
Am I blind or this is the case (and why)?
@Morten: There is a link for svn friendly (hope so) patch http://cmpt.benbmp.org/codeblocks/patches/cc.marco.patch
Quote from: oBFusCATed on January 25, 2015, 09:52:59 PM
@ollydbg:
I don't see any patches that add tests.
Am I blind or this is the case (and why)?
I will add one(new cpp file) to the src\plugins\codecompletion\testing folder to test the Macro expansion infinite loop issue. You will need to build the CCTest project to run all the tests.
Quote
@Morten: There is a link for svn friendly (hope so) patch http://cmpt.benbmp.org/codeblocks/patches/cc.marco.patch
Thanks, OBF.
@Morten, this is a merged SVN style patch containing all my 31 Git style patches. I think it is too hard for reviewing such a single patch.
Quote from: ollydbg on January 26, 2015, 03:15:24 AM
Quote
@Morten: There is a link for svn friendly (hope so) patch http://cmpt.benbmp.org/codeblocks/patches/cc.marco.patch
Thanks, OBF.
@Morten, this is a merged SVN style patch containing all my 31 Git style patches. I think it is too hard for reviewing such a single patch.
Oh yes, this one works. :-) I can start testing...
I'm testing it for a second day and I don't see major problems (crashes, infinite loops).
I can't comment much about the quality of the parser.
Quote from: oBFusCATed on January 27, 2015, 09:08:51 PM
I'm testing it for a second day and I don't see major problems (crashes, infinite loops).
I can't comment much about the quality of the parser.
I found it still has some problems to handling "#undef" and other conditional directives.
I will fix them those days.
Quote from: ollydbg on January 30, 2015, 08:24:03 AM
Quote from: oBFusCATed on January 27, 2015, 09:08:51 PM
I'm testing it for a second day and I don't see major problems (crashes, infinite loops).
I can't comment much about the quality of the parser.
I found it still has some problems to handling "#undef" and other conditional directives.
I will fix them those days.
Fixed by the V3 patches(you can apply V3 after applying V2, which means it can apply on https://github.com/obfuscated/codeblocks_sf/tree/ollydbg/cc_macro)
Still no test cases are added!
Quote from: oBFusCATed on January 31, 2015, 04:05:13 PM
Still no test cases are added!
Will be done tomorrow. It is midnight now...
Quote from: ollydbg on January 31, 2015, 04:15:22 PM
Quote from: oBFusCATed on January 31, 2015, 04:05:13 PM
Still no test cases are added!
Will be done tomorrow. It is midnight now...
Here is the test case, which cause cctest to infinite loop in our CC trunk, but it works OK after applying my patch serials.
OK, what about the test for all the other bugs/cases you've fixed?
Quote from: ollydbg on January 31, 2015, 03:57:40 PM
Fixed by the V3 patches(you can apply V3 after applying V2, which means it can apply on https://github.com/obfuscated/codeblocks_sf/tree/ollydbg/cc_macro)
...can I get them there? They won't apply otherwise.
Morten: Check again...
Quote from: oBFusCATed on February 01, 2015, 01:02:09 PM
OK, what about the test for all the other bugs/cases you've fixed?
There many tests file under src\plugins\codecompletion\testing, but running the cctest don't seem to improve much. ;)
--- C:/before-patch.txt Sun Feb 01 22:09:43 2015
+++ C:/after-patch.txt Sun Feb 01 22:00:48 2015
@@ -77,15 +77,15 @@
Total 1 tests, 1 PASS, 0 FAIL
--------------------------------------------------------
********************************************************
Testing in file: F:\cb_sf_git\trunk\src\plugins\codecompletion\testing\cc_mfc_expand_event_table.cpp
********************************************************
-*FAIL: mymiss mymissingfunc1
-*FAIL: mymiss mymissingfunc2
-*FAIL: mymiss mymissingfunc3
+-PASS: mymiss mymissingfunc1
+-PASS: mymiss mymissingfunc2
+-PASS: mymiss mymissingfunc3
--------------------------------------------------------
-Total 3 tests, 0 PASS, 3 FAIL
+Total 3 tests, 3 PASS, 0 FAIL
--------------------------------------------------------
********************************************************
Testing in file: F:\cb_sf_git\trunk\src\plugins\codecompletion\testing\cc_namespaces.cpp
********************************************************
-PASS: my_namespace.nested_namespace. variable
Look, it only fixes three Fails.
But please note that checking every token in the Tokenizer level is the only correct way to handle all the C-preprocessor directives and macro expansions, currently the higher level(Parserthread) class has many hacks to do this. If you look at the ParserThread.cpp, there are many code to check a token to see whether it is a macro usage or not. My patch serials just do all the macro expansion in the Tokenizer level, and it will make Parserthread's code much clean and simple.
Ok, so your changes fix already failing tests. ;D
For me writing parsers is a task, when test-driven-development can be applied with great success.
The idea is simple:
1. first write a test that fails
2. then write the code to fix the failure.
3. repeat 1
Always make sure that no change is made if there is not test for it.
And of course make sure that after a change is made all test pass.
Doing this prevents introducing changes that break already working cases/features.
p.s. CC code is one place of C::B I have no intention to dig in and make myself familiar with, so I cannot comment if the changes you're making are good
p.p.s. This is the reason I'm asking you to add tests. It is an easy way to check if everything is working :)
FYI: I've just upgraded my work build to one that includes the latest patches and a crash I was seeing with the old build now doesn't happen.
Quote from: oBFusCATed on February 05, 2015, 01:51:24 PM
FYI: I've just upgraded my work build to one that includes the latest patches and a crash I was seeing with the old build now doesn't happen.
Good to hear, thanks for the testing.
Quote from: oBFusCATed on February 01, 2015, 05:42:33 PM
Ok, so your changes fix already failing tests. ;D
p.p.s. This is the reason I'm asking you to add tests. It is an easy way to check if everything is working :)
I will see if I can add more tests.
Question:
If I pick some commits in my patch serials, and commit them to the official SVN, is it possible(easy) to rebase the github cc_macro branch (https://github.com/obfuscated/codeblocks_sf/tree/ollydbg/cc_macro)? I mean maybe all the commits need to be changed.
Quote from: ollydbg on February 06, 2015, 02:20:35 PM
If I pick some commits in my patch serials, and commit them to the official SVN, is it possible(easy) to rebase the github cc_macro branch (https://github.com/obfuscated/codeblocks_sf/tree/ollydbg/cc_macro)? I mean maybe all the commits need to be changed.
Yes, interactive rebase is designed exactly for this purpose. (Although, if you have never used it before, you may want to practice on a copy first.)
Yes, I can rebase the branch, the history will be rewritten and might cause problems for people has commits that are not in the branch. But I doubt this is the case, so I think it is safe to rebase it.
Any progress with this branch?
Do you plan committing fixes any time soon?
Quote from: oBFusCATed on March 03, 2015, 11:28:28 PM
Any progress with this branch?
Do you plan committing fixes any time soon?
This is a big change, but I don't have much time to test this branch, sorry, so I think it won't happen soon.
I'm testing it every day. For me it is better than the old infinite-loop version.
Quote from: oBFusCATed on March 05, 2015, 01:15:20 AM
I'm testing it every day. For me it is better than the old infinite-loop version.
Same for me until the changes in trunk were no longer compatible with the branch.
A rebase and merge would help to continue testing. However, what I noticed was that many methods of base classes were missing. That's what the wx replacements were for, for example.
Quote from: ollydbg on January 23, 2015, 03:16:19 PM
I see infinite loop too. :(
Bug located. Patch here:
src/plugins/codecompletion/parser/tokenizer.cpp | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/plugins/codecompletion/parser/tokenizer.cpp b/src/plugins/codecompletion/parser/tokenizer.cpp
index 764bcbe..6202eda 100644
--- a/src/plugins/codecompletion/parser/tokenizer.cpp
+++ b/src/plugins/codecompletion/parser/tokenizer.cpp
@@ -1794,6 +1794,10 @@ bool Tokenizer::ReplaceBufferText(const wxString& target)
m_TokenIndex = m_BufferLen - m_FirstRemainingLength;
m_PeekAvailable = false;
SkipToEOL(false);
+ SkipToOneOfChars(_T(",;}"), true, false, false);
+ m_SavedTokenIndex = m_UndoTokenIndex = m_TokenIndex;
+ m_SavedLineNumber = m_UndoLineNumber = m_LineNumber;
+ m_SavedNestingLevel = m_UndoNestLevel = m_NestLevel;
return false;
}
else
It is a workaround patch and will cause parsing the Test class body wrongly, you will notice one extra member in the symbol tree :). But it works around a bug that macro replacement cause infinite loop when parsing. The fundamental solution is to ONLY do macro replacement in the Tokenizer level. (We currently do in both ParserThread and Tokenizer).
From this patch I see the bug is due to incorrect handling when the expansion limit (s_MaxRepeatReplaceCount) is reached. I remember we added this commit in r9933 (see diff (http://sourceforge.net/p/codeblocks/code/9933/tree//trunk/src/plugins/codecompletion/parser/tokenizer.cpp?diff=51421f1dc431436b6eb31d56:9932)):
QuoteCC: disable the sanity check when expanding the macro usage, this try to work around an issue reported here: http://forums.next.codeblocks.org/index.php/topic,19661.msg134291.html#msg134291. We still have a expansion limit to avoid the infinite loop.
See the bolded part in the above quote. Our commit removed the sanity check, so we depend on the expansion limit handling to avoid infinite loop. But we now know the expansion limit is not handled properly and needed to be hacked (by ollydbg's patch) when it's reached. Can someone confirm whether reverting commit 9933 (i.e., adding back the sanity check) solves the issue?
You can try it yourself, I've posted a minimal project that reproduces the problem.
Quote from: Huki on March 06, 2015, 10:22:15 AM
...
Can someone confirm whether reverting commit 9933 (i.e., adding back the sanity check) solves the issue?
I did it in my pc, and it still hangs on the minimal project by OBF (http://cmpt.benbmp.org/codeblocks/nightlies/cc_marco_infinite2.tar.gz).
Quote from: MortenMacFly on March 05, 2015, 05:38:21 AM
Same for me until the changes in trunk were no longer compatible with the branch.
A rebase and merge would help to continue testing.
I have rebased in my local git copy, do I need to release all the rebased patches?
Quote
However, what I noticed was that many methods of base classes were missing. That's what the wx replacements were for, for example.
I remember wx's macros are handled correctly with my patches. I will test it again.
Quote from: ollydbg on March 07, 2015, 08:00:30 AM
...
I remember wx's macros are handled correctly with my patches. I will test it again.
I don't see some thing wrong with a simple wx(wx3.0) project created by wizard. So, I what is exact problem you have?
I did test with my patches on our codeblocks.cbp. I notice one issue, in the file sdk\configmanager.cpp
In the function:
TiXmlElement* ConfigManager::AssertPath(wxString& path)
{
Collapse(path);
wxString illegal(_T(" -:.\"\'$&()[]<>+#")); // our parser will skip to the end of this source file
It looks like the parser will skip to the end of the file, and comment out of the line don't expose the issue, so I will try to see what is the problem is. I think it is not hard to fix. (Maybe, this bug also exists on our trunk, I haven't tested it yet).
Quote from: ollydbg on March 07, 2015, 08:25:26 AM
...
I notice one issue, in the file sdk\configmanager.cpp
In the function:
TiXmlElement* ConfigManager::AssertPath(wxString& path)
{
Collapse(path);
wxString illegal(_T(" -:.\"\'$&()[]<>+#")); // our parser will skip to the end of this source file
It looks like the parser will skip to the end of the file, and comment out of the line don't expose the issue, so I will try to see what is the problem is. I think it is not hard to fix. (Maybe, this bug also exists on our trunk, I haven't tested it yet).
Here is a test case for our cc which fails in my patch serials.
#define wxCONCAT_HELPER(text, line) text ## line
#define wxT(x) wxCONCAT_HELPER(L, x)
#define _T(x) wxT(x)
wxString illegal(_T(" -:.\"\'$&()[]<>+#"));
int aaa;
//a //aaa
EDIT:
I found the reason, it caused by those code when expansion macros:
// 5. handling stringizing operator #
for (int pos = expandedText.Find(_T("#"));
pos != wxNOT_FOUND;
pos = expandedText.Find(_T("#")))
{
In-fact we should skip the cases that # is inside a literal
Git patches attached against(rebased on) current svn trunk head.
It also fixes the "#" issue in my previous post. :)
[attachment deleted by admin]
Quote from: ollydbg on March 07, 2015, 08:00:30 AM
Quote from: Huki on March 06, 2015, 10:22:15 AM
...
Can someone confirm whether reverting commit 9933 (i.e., adding back the sanity check) solves the issue?
I did it in my pc, and it still hangs on the minimal project by OBF (http://cmpt.benbmp.org/codeblocks/nightlies/cc_marco_infinite2.tar.gz).
You're right, commit 9933 is not related with this issue: it only deals with macros like "
#define AAA x+y+AAA".
I tried the obf's minimal project and I can already suggest a fix:
In ReplaceBufferText() we have:
if (m_RepeatReplaceCount >= s_MaxRepeatReplaceCount)
{
m_TokenIndex = m_BufferLen - m_FirstRemainingLength;
m_PeekAvailable = false;
SkipToEOL(false);
return false;
}
Simply change the return to "
return true;" and it will fix the problem. It works by skipping (i..e, not parsing) the problem variable "
int min;" in the Test struct. If we return false, we stop the current repeat replacement, but later in ParserThread() we will start a new replacement on the same token. :( That's why we get the infinite loop.
It's also a good idea to reset the UndoToken like you did in your original patch. But I am not sure if SkipToEOL() is really required. I suggest a patch like this:
@@ -1812,13 +1812,18 @@ bool Tokenizer::ReplaceBufferText(const wxString& target)
if (m_RepeatReplaceCount > 0)
{
if (m_RepeatReplaceCount >= s_MaxRepeatReplaceCount)
{
m_TokenIndex = m_BufferLen - m_FirstRemainingLength;
+
+ // Reset undo token
+ m_SavedTokenIndex = m_UndoTokenIndex = m_TokenIndex;
+ m_SavedLineNumber = m_UndoLineNumber = m_LineNumber;
+ m_SavedNestingLevel = m_UndoNestLevel = m_NestLevel;
+
m_PeekAvailable = false;
- SkipToEOL(false);
- return false;
+ return true; // NOTE: we have to skip the problem token by returning true.
}
else
++m_RepeatReplaceCount;
}
else // Set replace parsing state, and save first replace token index
Although my humble contribution is not directly related to the infinite loop, it may help the codecompletion a bit.
If I run cppcheck I get a bunch of remarks:
$ cppcheck --enable=all *.cpp |grep -v Checking |grep -v 'files checked'
[cclogger.h:49]: (warning) Member variable 'CCLogger::m_Parent' is not assigned a value in 'CCLogger::operator='.
[cclogger.h:49]: (warning) Member variable 'CCLogger::m_LogId' is not assigned a value in 'CCLogger::operator='.
[cclogger.h:49]: (warning) Member variable 'CCLogger::m_DebugLogId' is not assigned a value in 'CCLogger::operator='.
[cclogger.h:49]: (warning) Member variable 'CCLogger::m_AddTokenId' is not assigned a value in 'CCLogger::operator='.
[tokenizer.h:249]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[expression.cpp:125]: (style) Expression is always false because 'else if' condition matches previous condition at line 104.
[parser.cpp:782] -> [parser.cpp:786]: (performance) Variable 'isParsed' is reassigned a value before the old one has been used.
[parser_base.cpp:180]: (warning) Member variable 'ParserBase::m_BrowserOptions' is not initialized in the constructor.
[parserthread.cpp:1883] -> [parserthread.cpp:1884]: (performance) Variable 'current' is reassigned a value before the old one has been used.
[parserthread.cpp:2819]: (performance) Possible inefficient checking for 'components' emptiness.
[parserthread.cpp:2822]: (performance) Possible inefficient checking for 'components' emptiness.
[parserthread.cpp:3500]: (performance) Possible inefficient checking for 'results' emptiness.
[searchtree.cpp:598]: (style) The scope of the variable 'nchild' can be reduced.
[searchtree.cpp:670]: (style) The scope of the variable 'n' can be reduced.
[searchtree.cpp:759]: (style) The scope of the variable 'matches' can be reduced.
[searchtree.cpp:989]: (style) The scope of the variable 'u' can be reduced.
[searchtree.cpp:77]: (performance) Possible inefficient checking for 'm_Children' emptiness.
[searchtree.cpp:403]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[searchtree.cpp:411]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[searchtree.cpp:460]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[searchtree.cpp:471]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[searchtree.cpp:484]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[searchtree.cpp:510]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[token.cpp:238]: (performance) Possible inefficient checking for 'files' emptiness.
[token.cpp:295]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[tokentree.cpp:150]: (style) The scope of the variable 'result' can be reduced.
[tokentree.cpp:178]: (style) The scope of the variable 'result' can be reduced.
[tokentree.cpp:210]: (style) The scope of the variable 'result' can be reduced.
[tokentree.cpp:243]: (style) The scope of the variable 'result' can be reduced.
[cctreectrl.cpp:86]: (style) C-style pointer casting
[cctreectrl.cpp:166]: (style) C-style pointer casting
[cctreectrl.cpp:167]: (style) C-style pointer casting
[classbrowser.cpp:286]: (style) C-style pointer casting
[classbrowser.cpp:477]: (style) C-style pointer casting
[classbrowser.cpp:492]: (style) C-style pointer casting
[classbrowser.cpp:539]: (style) C-style pointer casting
[classbrowser.cpp:544]: (style) C-style pointer casting
[classbrowser.cpp:807]: (style) C-style pointer casting
[classbrowserbuilderthread.cpp:66]: (warning) Member variable 'ClassBrowserBuilderThread::m_idThreadEvent' is not initialized in the constructor.
[codecompletion.cpp:1964]: (performance) Possible inefficient checking for 'result' emptiness.
[codecompletion.cpp:1749]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[coderefactoring.cpp:438]: (style) The scope of the variable 'pos' can be reduced.
[doxygen_parser.cpp:845]: (performance) Possible inefficient checking for 'tokensIdx' emptiness.
[doxygen_parser.cpp:964]: (performance) Possible inefficient checking for 'result' emptiness.
[nativeparser_base.cpp:1217] -> [nativeparser_base.cpp:1219]: (warning) Possible null pointer dereference: token - otherwise it is redundant to check it against null.
[nativeparser_base.cpp:1247] -> [nativeparser_base.cpp:1249]: (warning) Possible null pointer dereference: token - otherwise it is redundant to check it against null.
[nativeparser_base.cpp:869]: (style) Unused variable: autualTypeResult
[nativeparser_base.cpp:141]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[nativeparser_base.cpp:167]: (performance) Prefer prefix ++/-- operators for non-primitive types.
IMHO, most of them are little obvious things can be corrected with little effort. This might save ollydbg and huki time for the harder things.
Hope, I do not upset somebody in hijacking the thread to some extent. Thought it might fit here and was not worth opening a new one.
Thanks.
Hello, blauzahn, thank you very much!
I fixed most of them in the trunk, see below:
[cclogger.h:49]: (warning) Member variable 'CCLogger::m_Parent' is not assigned a value in 'CCLogger::operator='.
[cclogger.h:49]: (warning) Member variable 'CCLogger::m_LogId' is not assigned a value in 'CCLogger::operator='.
[cclogger.h:49]: (warning) Member variable 'CCLogger::m_DebugLogId' is not assigned a value in 'CCLogger::operator='.
[cclogger.h:49]: (warning) Member variable 'CCLogger::m_AddTokenId' is not assigned a value in 'CCLogger::operator='.
Not sure what is a correct fix.
[tokenizer.h:249]: (performance) Prefer prefix ++/-- operators for non-primitive types.
No need to fix
[expression.cpp:125]: (style) Expression is always false because 'else if' condition matches previous condition at line 104.
fixed
[parser.cpp:782] -> [parser.cpp:786]: (performance) Variable 'isParsed' is reassigned a value before the old one has been used.
no need to fix
[parser_base.cpp:180]: (warning) Member variable 'ParserBase::m_BrowserOptions' is not initialized in the constructor.
fixed
[parserthread.cpp:1883] -> [parserthread.cpp:1884]: (performance) Variable 'current' is reassigned a value before the old one has been used.
fixed.
[parserthread.cpp:2819]: (performance) Possible inefficient checking for 'components' emptiness.
[parserthread.cpp:2822]: (performance) Possible inefficient checking for 'components' emptiness.
no need to fix
[parserthread.cpp:3500]: (performance) Possible inefficient checking for 'results' emptiness.
no need to fix
[searchtree.cpp:598]: (style) The scope of the variable 'nchild' can be reduced.
fixed
[searchtree.cpp:670]: (style) The scope of the variable 'n' can be reduced.
fixed, but maybe n get reassigned?
[searchtree.cpp:759]: (style) The scope of the variable 'matches' can be reduced.
fixed
[searchtree.cpp:989]: (style) The scope of the variable 'u' can be reduced.
fixed
[searchtree.cpp:77]: (performance) Possible inefficient checking for 'm_Children' emptiness.
no need to fix
[searchtree.cpp:403]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[searchtree.cpp:411]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[searchtree.cpp:460]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[searchtree.cpp:471]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[searchtree.cpp:484]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[searchtree.cpp:510]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[token.cpp:238]: (performance) Possible inefficient checking for 'files' emptiness.
[token.cpp:295]: (performance) Prefer prefix ++/-- operators for non-primitive types.
all no need to fix.
[tokentree.cpp:150]: (style) The scope of the variable 'result' can be reduced.
fixed
[tokentree.cpp:178]: (style) The scope of the variable 'result' can be reduced.
fixed
[tokentree.cpp:210]: (style) The scope of the variable 'result' can be reduced.
fixed
[tokentree.cpp:243]: (style) The scope of the variable 'result' can be reduced.
fixed
[cctreectrl.cpp:86]: (style) C-style pointer casting
[cctreectrl.cpp:166]: (style) C-style pointer casting
[cctreectrl.cpp:167]: (style) C-style pointer casting
[classbrowser.cpp:286]: (style) C-style pointer casting
[classbrowser.cpp:477]: (style) C-style pointer casting
[classbrowser.cpp:492]: (style) C-style pointer casting
[classbrowser.cpp:539]: (style) C-style pointer casting
[classbrowser.cpp:544]: (style) C-style pointer casting
[classbrowser.cpp:807]: (style) C-style pointer casting
Not currently yet. (No need to fix?)
[classbrowserbuilderthread.cpp:66]: (warning) Member variable 'ClassBrowserBuilderThread::m_idThreadEvent' is not initialized in the constructor.
fixed
[codecompletion.cpp:1964]: (performance) Possible inefficient checking for 'result' emptiness.
[codecompletion.cpp:1749]: (performance) Prefer prefix ++/-- operators for non-primitive types.
no need to fix
[coderefactoring.cpp:438]: (style) The scope of the variable 'pos' can be reduced.
fixed
[doxygen_parser.cpp:845]: (performance) Possible inefficient checking for 'tokensIdx' emptiness.
fixed by using empty() function.
[doxygen_parser.cpp:964]: (performance) Possible inefficient checking for 'result' emptiness.
no need to fix
[nativeparser_base.cpp:1217] -> [nativeparser_base.cpp:1219]: (warning) Possible null pointer dereference: token - otherwise it is redundant to check it against null.
This is alrady checked in if (token && MatchType(token->m_TokenKind, kindMask))
[nativeparser_base.cpp:1247] -> [nativeparser_base.cpp:1249]: (warning) Possible null pointer dereference: token - otherwise it is redundant to check it against null.
This is alrady checked in if (token && MatchType(token->m_TokenKind, kindMask))
[nativeparser_base.cpp:869]: (style) Unused variable: autualTypeResult
fixed
[nativeparser_base.cpp:141]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[nativeparser_base.cpp:167]: (performance) Prefer prefix ++/-- operators for non-primitive types.
no need to fix
The "no need to fix" means I don't think it is necessary to fix them. such as "it++" and "++it" should be the same thing.
The others are that I don't know a correct fix. :)
Thanks for applying this.
Quote
The "no need to fix" means I don't think it is necessary to fix them. such as "it++" and "++it" should be the same thing.
Even then I would change them. Prefer ++it. Make it a habit unless there is a reason to use it++. And it makes usage
of tools like cppcheck easier because it keeps the list short so that the chances to miss a real problem in a long list
are lower. I do not eliminate every warning of a tool slavishly but here it makes sense.
In cases where it++ would actually create a (often unwanted temporary) it may make the program slower.
Rationale: see Herb Sutter C++Coding Standards §28 "[...] prefer calling the prefix forms", §9 "Don't pessimize prematurely."
The same applies to C-style pointer casting. Rationale see Herb Sutter §95 "Don't use C-Style casts."
Quote
[cclogger.h:49]: (warning) Member variable 'CCLogger::m_Parent' is not assigned a value in 'CCLogger::operator='.
[cclogger.h:49]: (warning) Member variable 'CCLogger::m_LogId' is not assigned a value in 'CCLogger::operator='.
[cclogger.h:49]: (warning) Member variable 'CCLogger::m_DebugLogId' is not assigned a value in 'CCLogger::operator='.
[cclogger.h:49]: (warning) Member variable 'CCLogger::m_AddTokenId' is not assigned a value in 'CCLogger::operator='.
Not sure what is a correct fix.
As for c++11 I would say for the cctor and op= =default to express the intent of really copying the non-owning pointer member m_Parent
and the ints. Even if the ugly static global variable is the only instance (is it?). In c++98 I would write the cctor explicitly and a canonical
op= and swap. A bit of boilerplate but makes life safer.
Why is there a virtual dtor. Apart from the dtor there is no other virtual method. So, the class is no polymorphic interface and probably
does not need a virtual dtor (including vtable etc.).
Having to remember to call the Init method (from cc ctor) after receiving a pointer to the global variable via Get() is a bit unfortunate since this is
rather bug prone. Since the data-member are private and no method changes any of them I'd make the methods const. If it never changes, may
be Get should return a (cheap and safe) copy instead of a pointer.
Quote from: blauzahn on March 10, 2015, 07:43:08 AM
Even then I would change them. Prefer ++it. ...
We're using optimizing compilers, so it won't make any difference! If you hate them so much post a patch. It is trivial to do.
And please do it in another topic. This one is related to something else and your post is quite off topic. Also this topic you've started is prone to flaming/religion and so on, so the original one will be swamped by off topic posts!
Fixing warnings is good but fixing real problems that have user visible effects is even better.
@admins: Please separate post related to cppcheck in a separate topic.
Quote from: Huki on March 08, 2015, 08:48:18 AM
...
In ReplaceBufferText() we have:
if (m_RepeatReplaceCount >= s_MaxRepeatReplaceCount)
{
m_TokenIndex = m_BufferLen - m_FirstRemainingLength;
m_PeekAvailable = false;
SkipToEOL(false);
return false;
}
Simply change the return to "return true;" and it will fix the problem. It works by skipping (i..e, not parsing) the problem variable "int min;" in the Test struct. If we return false, we stop the current repeat replacement, but later in ParserThread() we will start a new replacement on the same token. :( That's why we get the infinite loop.
...
Does return true cause other issue? I don't know. Maybe, we could add some test like
change this
void Tokenizer::GetReplacedToken(wxString& str)
{
// this indicates we are already in macro replacement mode
if (m_RepeatReplaceCount > 0)
{
to
void Tokenizer::GetReplacedToken(wxString& str)
{
// this indicates we are already in macro replacement mode
if (m_RepeatReplaceCount > 0 && m_RepeatReplaceCount < s_MaxRepeatReplaceCount )
{
But I haven't tested yet.
Quote from: Huki on March 08, 2015, 08:48:18 AM
...
I suggest a patch like this:
@@ -1812,13 +1812,18 @@ bool Tokenizer::ReplaceBufferText(const wxString& target)
if (m_RepeatReplaceCount > 0)
{
if (m_RepeatReplaceCount >= s_MaxRepeatReplaceCount)
{
m_TokenIndex = m_BufferLen - m_FirstRemainingLength;
+
+ // Reset undo token
+ m_SavedTokenIndex = m_UndoTokenIndex = m_TokenIndex;
+ m_SavedLineNumber = m_UndoLineNumber = m_LineNumber;
+ m_SavedNestingLevel = m_UndoNestLevel = m_NestLevel;
+
m_PeekAvailable = false;
- SkipToEOL(false);
- return false;
+ return true; // NOTE: we have to skip the problem token by returning true.
}
else
++m_RepeatReplaceCount;
}
else // Set replace parsing state, and save first replace token index
Hi, Huki, after debug and reading the source code for a while, and found your patch is great!
I have committed to trunk with many description added as commit log. Thanks.
Have rebuilt my work cb to the latest revision and it seems the bug has been fixed without the need to use the separate patches.
Quote from: oBFusCATed on March 16, 2015, 08:55:58 PM
Have rebuilt my work cb to the latest revision and it seems the bug has been fixed without the need to use the separate patches.
Yes, the broken token which cause infinite loop is skipped, this is a workaround to avoid the infinite loop, but does not follow the preprocessor' rules. My patches follow the rules, and won't expand the used macros . ;)
I am getting crashes like the following now:
C:\Devel\CodeBlocks\codeblocks.exe caused an Access Violation at location 6275ce54 in module C:\Devel\CodeBlocks\wxmsw28u_gcc_custom.dll Reading from location 00003a2e.
Registers:
eax=0028ed0c ebx=61a82e24 ecx=0028ed0c edx=00003a3a esi=62c92074 edi=0ae23720
eip=6275ce54 esp=0028ec20 ebp=0028ec78 iopl=0 nv up ei pl nz ac po nc
cs=0023 ss=002b ds=002b es=002b fs=0053 gs=002b efl=00010216
Call stack:
6275CE54 C:\Devel\CodeBlocks\wxmsw28u_gcc_custom.dll:6275CE54 _ZN12wxStringBaseaSERKS_
618034E1 C:\Devel\CodeBlocks\codeblocks.dll:618034E1 _ZN9CCManager13OnShowCallTipER15CodeBlocksEvent
61A82E7F C:\Devel\CodeBlocks\codeblocks.dll:61A82E7F _ZN14cbEventFunctorI9CCManager15CodeBlocksEventE4CallERS1_
6188CB4E C:\Devel\CodeBlocks\codeblocks.dll:6188CB4E _ZN7Manager12ProcessEventER15CodeBlocksEvent
61802827 C:\Devel\CodeBlocks\codeblocks.dll:61802827 _ZN9CCManager12OnEditorHookEP8cbEditorR16wxScintillaEvent
61AEF324 C:\Devel\CodeBlocks\codeblocks.dll:61AEF324 _ZNK11EditorHooks11HookFunctorI9CCManagerE4CallEP8cbEditorR16wxScintillaEvent
618431D5 C:\Devel\CodeBlocks\codeblocks.dll:618431D5 _ZN11EditorHooks9CallHooksEP8cbEditorR16wxScintillaEvent
617E1AB8 C:\Devel\CodeBlocks\codeblocks.dll:617E1AB8 _ZN8cbEditor16OnScintillaEventER16wxScintillaEvent
617E07F8 C:\Devel\CodeBlocks\codeblocks.dll:617E07F8 _ZN8cbEditor17OnEditorCharAddedER16wxScintillaEvent
62701262 C:\Devel\CodeBlocks\wxmsw28u_gcc_custom.dll:62701262 _ZNK12wxAppConsole11HandleEventEP12wxEvtHandlerMS0_FvR7wxEventES3_
62926CB8 C:\Devel\CodeBlocks\wxmsw28u_gcc_custom.dll:62926CB8 _ZN12wxWindowBase9TryParentER7wxEvent
61912F28 C:\Devel\CodeBlocks\codeblocks.dll:61912F28 _ZN11wxScintilla12NotifyParentEP14SCNotification
61915A91 C:\Devel\CodeBlocks\codeblocks.dll:61915A91 _ZN11ScintillaWX12NotifyParentE14SCNotification
619B8818 C:\Devel\CodeBlocks\codeblocks.dll:619B8818 _ZN6Editor10NotifyCharEi
619B621B C:\Devel\CodeBlocks\codeblocks.dll:619B621B _ZN6Editor10AddCharUTFEPKcjb
619D277A C:\Devel\CodeBlocks\codeblocks.dll:619D277A _ZN13ScintillaBase10AddCharUTFEPKcjb
61918645 C:\Devel\CodeBlocks\codeblocks.dll:61918645 _ZN11ScintillaWX9DoAddCharEi
61912059 C:\Devel\CodeBlocks\codeblocks.dll:61912059 _ZN11wxScintilla6OnCharER10wxKeyEvent
62701262 C:\Devel\CodeBlocks\wxmsw28u_gcc_custom.dll:62701262 _ZNK12wxAppConsole11HandleEventEP12wxEvtHandlerMS0_FvR7wxEventES3_
62701262 C:\Devel\CodeBlocks\wxmsw28u_gcc_custom.dll:62701262 _ZNK12wxAppConsole11HandleEventEP12wxEvtHandlerMS0_FvR7wxEventES3_
6280D82D C:\Devel\CodeBlocks\wxmsw28u_gcc_custom.dll:6280D82D _ZN8wxWindow10HandleCharEjlb
62811BB8 C:\Devel\CodeBlocks\wxmsw28u_gcc_custom.dll:62811BB8 _ZN8wxWindow13MSWWindowProcEjjl
628092B6 C:\Devel\CodeBlocks\wxmsw28u_gcc_custom.dll:628092B6 _Z9wxWndProcP6HWND__jjl@16
752C62FA C:\Windows\syswow64\USER32.dll:752C62FA gapfnScSendMessage
752C6D3A C:\Windows\syswow64\USER32.dll:752C6D3A GetThreadDesktop
752C77C4 C:\Windows\syswow64\USER32.dll:752C77C4 CharPrevW
752C788A C:\Windows\syswow64\USER32.dll:752C788A DispatchMessageW
752EC81F C:\Windows\syswow64\USER32.dll:752EC81F IsDialogMessageW
6280823D C:\Devel\CodeBlocks\wxmsw28u_gcc_custom.dll:6280823D _ZN8wxWindow17MSWProcessMessageEP6tagMSG
627E3AFD C:\Devel\CodeBlocks\wxmsw28u_gcc_custom.dll:627E3AFD _ZN11wxEventLoop17PreProcessMessageEP6tagMSG
627E386D C:\Devel\CodeBlocks\wxmsw28u_gcc_custom.dll:627E386D _ZN11wxEventLoop8DispatchEv
...resolved:
******************************
* Found (another) call stack *
******************************
C:\Devel\CodeBlocks\MinGW\bin\addr2line.exe -e C:\Devel\CodeBlocks\src\devel\wxmsw28u_gcc_custom.dll 6275CE54:
Error for: C:\Devel\CodeBlocks\MinGW\bin\addr2line.exe -e C:\Devel\CodeBlocks\src\devel\wxmsw28u_gcc_custom.dll 6275CE54
:C:\Devel\CodeBlocks\MinGW\bin\addr2line.exe: C:\Devel\CodeBlocks\src\devel\wxmsw28u_gcc_custom.dll: File truncated
----------------------------------------
C:\Devel\CodeBlocks\MinGW\bin\addr2line.exe -e C:\Devel\CodeBlocks\src\devel\codeblocks.dll 618034E1:
C:\Devel\CodeBlocks\codeblocks.dll[618034E1]:
C:/Devel/CodeBlocks/src/sdk/ccmanager.cpp:882
----------------------------------------
C:\Devel\CodeBlocks\MinGW\bin\addr2line.exe -e C:\Devel\CodeBlocks\src\devel\codeblocks.dll 61A82E7F:
C:\Devel\CodeBlocks\codeblocks.dll[61A82E7F]:
C:/Devel/CodeBlocks/src/include/cbfunctor.h:49
----------------------------------------
C:\Devel\CodeBlocks\MinGW\bin\addr2line.exe -e C:\Devel\CodeBlocks\src\devel\codeblocks.dll 6188CB4E:
C:\Devel\CodeBlocks\codeblocks.dll[6188CB4E]:
C:/Devel/CodeBlocks/src/sdk/manager.cpp:263
----------------------------------------
C:\Devel\CodeBlocks\MinGW\bin\addr2line.exe -e C:\Devel\CodeBlocks\src\devel\codeblocks.dll 61802827:
C:\Devel\CodeBlocks\codeblocks.dll[61802827]:
C:/Devel/CodeBlocks/src/sdk/ccmanager.cpp:722
----------------------------------------
C:\Devel\CodeBlocks\MinGW\bin\addr2line.exe -e C:\Devel\CodeBlocks\src\devel\codeblocks.dll 61AEF324:
C:\Devel\CodeBlocks\codeblocks.dll[61AEF324]:
C:\Devel\CodeBlocks\src/include/editor_hooks.h:61
----------------------------------------
C:\Devel\CodeBlocks\MinGW\bin\addr2line.exe -e C:\Devel\CodeBlocks\src\devel\codeblocks.dll 618431D5:
C:\Devel\CodeBlocks\codeblocks.dll[618431D5]:
C:/Devel/CodeBlocks/src/sdk/editor_hooks.cpp:122
----------------------------------------
C:\Devel\CodeBlocks\MinGW\bin\addr2line.exe -e C:\Devel\CodeBlocks\src\devel\codeblocks.dll 617E1AB8:
C:\Devel\CodeBlocks\codeblocks.dll[617E1AB8]:
C:/Devel/CodeBlocks/src/sdk/cbeditor.cpp:3440
----------------------------------------
C:\Devel\CodeBlocks\MinGW\bin\addr2line.exe -e C:\Devel\CodeBlocks\src\devel\codeblocks.dll 617E07F8:
C:\Devel\CodeBlocks\codeblocks.dll[617E07F8]:
C:/Devel/CodeBlocks/src/sdk/cbeditor.cpp:3172
----------------------------------------
C:\Devel\CodeBlocks\MinGW\bin\addr2line.exe -e C:\Devel\CodeBlocks\src\devel\wxmsw28u_gcc_custom.dll 62701262:
Error for: C:\Devel\CodeBlocks\MinGW\bin\addr2line.exe -e C:\Devel\CodeBlocks\src\devel\wxmsw28u_gcc_custom.dll 62701262
:C:\Devel\CodeBlocks\MinGW\bin\addr2line.exe: C:\Devel\CodeBlocks\src\devel\wxmsw28u_gcc_custom.dll: File truncated
----------------------------------------
C:\Devel\CodeBlocks\MinGW\bin\addr2line.exe -e C:\Devel\CodeBlocks\src\devel\wxmsw28u_gcc_custom.dll 62926CB8:
Error for: C:\Devel\CodeBlocks\MinGW\bin\addr2line.exe -e C:\Devel\CodeBlocks\src\devel\wxmsw28u_gcc_custom.dll 62926CB8
:C:\Devel\CodeBlocks\MinGW\bin\addr2line.exe: C:\Devel\CodeBlocks\src\devel\wxmsw28u_gcc_custom.dll: File truncated
----------------------------------------
C:\Devel\CodeBlocks\MinGW\bin\addr2line.exe -e C:\Devel\CodeBlocks\src\devel\codeblocks.dll 61912F28:
C:\Devel\CodeBlocks\codeblocks.dll[61912F28]:
C:/Devel/CodeBlocks/src/sdk/wxscintilla/src/wxscintilla.cpp:5750
----------------------------------------
C:\Devel\CodeBlocks\MinGW\bin\addr2line.exe -e C:\Devel\CodeBlocks\src\devel\codeblocks.dll 61915A91:
C:\Devel\CodeBlocks\codeblocks.dll[61915A91]:
C:/Devel/CodeBlocks/src/sdk/wxscintilla/src/ScintillaWX.cpp:534
----------------------------------------
C:\Devel\CodeBlocks\MinGW\bin\addr2line.exe -e C:\Devel\CodeBlocks\src\devel\codeblocks.dll 619B8818:
C:\Devel\CodeBlocks\codeblocks.dll[619B8818]:
C:/Devel/CodeBlocks/src/sdk/wxscintilla/src/scintilla/src/Editor.cxx:2240
----------------------------------------
C:\Devel\CodeBlocks\MinGW\bin\addr2line.exe -e C:\Devel\CodeBlocks\src\devel\codeblocks.dll 619B621B:
C:\Devel\CodeBlocks\codeblocks.dll[619B621B]:
C:/Devel/CodeBlocks/src/sdk/wxscintilla/src/scintilla/src/Editor.cxx:1898
----------------------------------------
C:\Devel\CodeBlocks\MinGW\bin\addr2line.exe -e C:\Devel\CodeBlocks\src\devel\codeblocks.dll 619D277A:
C:\Devel\CodeBlocks\codeblocks.dll[619D277A]:
C:/Devel/CodeBlocks/src/sdk/wxscintilla/src/scintilla/src/ScintillaBase.cxx:83
----------------------------------------
C:\Devel\CodeBlocks\MinGW\bin\addr2line.exe -e C:\Devel\CodeBlocks\src\devel\codeblocks.dll 61918645:
C:\Devel\CodeBlocks\codeblocks.dll[61918645]:
C:/Devel/CodeBlocks/src/sdk/wxscintilla/src/ScintillaWX.cpp:1197
----------------------------------------
C:\Devel\CodeBlocks\MinGW\bin\addr2line.exe -e C:\Devel\CodeBlocks\src\devel\codeblocks.dll 61912059:
C:\Devel\CodeBlocks\codeblocks.dll[61912059]:
C:/Devel/CodeBlocks/src/sdk/wxscintilla/src/wxscintilla.cpp:5462
----------------------------------------
C:\Devel\CodeBlocks\MinGW\bin\addr2line.exe -e C:\Devel\CodeBlocks\src\devel\wxmsw28u_gcc_custom.dll 62701262:
Error for: C:\Devel\CodeBlocks\MinGW\bin\addr2line.exe -e C:\Devel\CodeBlocks\src\devel\wxmsw28u_gcc_custom.dll 62701262
:C:\Devel\CodeBlocks\MinGW\bin\addr2line.exe: C:\Devel\CodeBlocks\src\devel\wxmsw28u_gcc_custom.dll: File truncated
I doubt it is related to the patch actually, probably just now it becomes visible.
Looks like another wxString thread instability...
Quote from: MortenMacFly on March 18, 2015, 07:49:42 AM
...
I doubt it is related to the patch actually, probably just now it becomes visible.
Looks like another wxString thread instability...
You mean the crash happens when you apply my "try to expand every identifier like macro token" patch?
To solve the wxString thread issue, let switch to wx3.x. ;)
Quote from: ollydbg on March 08, 2015, 07:43:36 AM
Git patches attached against(rebased on) current svn trunk head.
It also fixes the "#" issue in my previous post. :)
[attachment deleted by admin]
Hi,
I'm interested in the CC macro serial patches (to do all macro expansion at the Tokenizer level), but the attachment happens to be deleted. Could you repost them?
I believe it's in the git branch now. Check obfuscated's git repo and then the ollydbg branch. I am not sure if it is in sync with current svn trunk though.
Hi, Huki, I attach the git patch serials against the latest trunk rev 10182.
Comments are welcome :)