Fix #12861 Hang in valueFlowCondition() with huge array

[clock999]

I tested the fix based on 4617bc2.

Use the cppcheck to check the below code which is submitted on the ticket #12861.

#define ROW A, A, A, A, A, A, A, A,
#define ROW8 ROW ROW ROW ROW ROW ROW ROW ROW
#define ROW64 ROW8 ROW8 ROW8 ROW8 ROW8 ROW8 ROW8 ROW8
#define ROW512 ROW64 ROW64 ROW64 ROW64 ROW64 ROW64 ROW64 ROW64
void f() {
static const char A = 'a';
const char a[] = {
ROW512	ROW512 ROW512	ROW512
};
}

With the fix, the overall time is 1.52582s.
Without the fix, the time is 25.9674s.

I also tested the testrunner for running the whole of the unit tests. There is some performance improment with the fix, but not remarkable.

[chrchr-github]

Please add a test in test/cli/performance_test.py.

[firewave]

Please add a test in test/cli/performance_test.py.

The test already exists and it is the one failing with this change applied. So somehow this actually makes things worse.

[clock999]

Yes, I will check it again. If I can't fix it, I will ignore this PR.

[clock999]

I updated the commit. For the test case submitted by the ticket #12861, the performance is improved a lot, at least the consumed time can be reduced to less than 2 seconds. For other test cases, I don't have the exact testing data, but I think this can be helpful for the performance.

[chrchr-github]

Is there a reason not to implement caching in the regular astTop() function (i.e. why add astFinalTop())?

[clock999]

Hi CHR, the process of creating the AST tree is a little complicated for me currently, and I can't figure out the details, that is also why I didn't modify the createAst(). As I understand, during TokenList::createAst(), the astTop() is used. But at that time, the createAst() has not been finished, so the top is a temporary one, which can not be cached as the final top. So we need two versions of the function, one is for creating ast, and another with the cache function is for the usage after the ast is created.
Maybe we can replaced the astTop with a new function name for createAst without cache function. And we keep astTop() added the cache that may be better for no confusion. Or we can add a param to astTop(bool iscache).

[chrchr-github]

I think this sounds reasonable: astTop(bool iscache). And please add a comment explaining the parameter.

[firewave]

This seems unnecessary.

[danmar]

remove the "pls" :-)

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[firewave]

There is a less intrusive way to address this by exiting early - see #7822.

[clock999]

I do think the function ‘Token *astTop()’ should be updated. It is very apparent that if we can figoure out the top of a token by parsing an AST tree, there is no need to loop the parents each time we getting the top, and we should just return the top.
I updated the commit. My idea is that the astTop() is just for getting the real final top of a token. During creating the AST tree, we just use some lines of codes with simple logic to implement the goal.
If there is any other improment, it is good for the performance, and the performance can be better and better.

[pfultz2]

We shouldn't update the mAstTop in the astTop getter. There should be a astTop(Token* tok) method(similar to astParent setter) to set the top and you can set them in createAst after the loop is finished(so there wont be any new ast updates).

This can also be done with a more efficient algorithm as well as you can start at the top node and traverse down, whereas setting it in the getter requires it to traverse up multiple times for the same top node.

[pfultz2]

By moving the setting of mAstTop out of the getter, then you dont need to copy and paste the astTop loop everywhere.

[firewave]

As per my previous comment these changes are not necessary at all - see #7822. I will give that a small run with test_my_pr.py later and if that looks fine we should be able to merge it and check daca if it has another detrimental effect.

[pfultz2]

As per my previous comment these changes are not necessary at all - see #7822.

I think precalculating astTop is a more robust solution. #7822 relies on probability that parse will be faster than astTop, which is possible to build scenarios where that isnt the case(like lots of conditions outside of if statements). This should also improve the performance in other places we use astTop.

[clock999]

Updated the commit. I am not familiar with details about how the ast tree is created. So I set the tops for the tokens at the time after the tree is created instead of during that process. That means the tree is parsed again. Even though, the benefit I think is good enough. The time of running the case in ticket 12861 is less than 1 second. And I simply recorded the time of running the cppcheck/testrunner. It is improved a lot. Anyway, with resolving the astTops problem, the performance I think can be improved a lot.

[pfultz2]

There is no need to traverse up. We only need to traverse down from the top nodes(which are nodes that dont have a parent but have children):

for (Token *tok = mTokensFrontBack->front; tok; tok = tok ? tok->next() : nullptr) {
if(tok->astParent())
continue;
if(!tok->astOperand1() && !tok->astOperand2())
continue;
visitAstNodes(tok, [&](Token* child) {
child->astTop(tok);
return ChildrenToVisit::op1_and_op2;
});
}

[pfultz2]

This should just use astTop since it falls back to the loop when the top is not set.

[pfultz2]

Dont check for null, we should be able to use this to clear the top if we want to.

[pfultz2]

This change should be reverted, it cam be written as semicolon1->astOperand1(init->astTop()) as astTop can still be called before it stores the top.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[clock999]

Thanks for the comments! The commit is updated. There is a scriptcheck failing which seems not the submitted code problem.

[chrchr-github]

What is the performance impact? Is it possible to reduce the timeout of the existing test?

[clock999]

Yes. I tested the case posted on the ticket 12861 (https://trac.cppcheck.net/ticket/12861), the consumed time can be reduced to less than 1 second.

#define ROW A, A, A, A, A, A, A, A,
#define ROW8 ROW ROW ROW ROW ROW ROW ROW ROW
#define ROW64 ROW8 ROW8 ROW8 ROW8 ROW8 ROW8 ROW8 ROW8
#define ROW512 ROW64 ROW64 ROW64 ROW64 ROW64 ROW64 ROW64 ROW64
void f() {
static const char A = 'a';
const char a[] = {
ROW512	ROW512 ROW512	ROW512
};
}

[chrchr-github]

Yes. I tested the case posted on the ticket 12861 (https://trac.cppcheck.net/ticket/12861), the consumed time can be reduced to less than 1 second.

Ok, so please reduce the timeout for the existing test in test/cli/performance_test.py.

[clock999]

Sorry, I am a little confused about the requirement. I run the test/cli/performance_test.py locally, and this commit passed the tests. Do you mean we need to add one more case to test/cli/performance_test.py? If so, how to set the case and the expected result? Can I completely copy the case posted on the ticket 12861 directly?

[chrchr-github]

See here: https://github.com/danmar/cppcheck/blob/574fffaeb56155e52bb2349fa6649c623d1d17ca/test/cli/performance_test.py#L225
That's the code from https://trac.cppcheck.net/ticket/12861 with an added namespace. Please reduce the timeout appropriately. The test should now also run on MacOS.

[chrchr-github]

I have done some experiments. The existing test (array in namespace) shows no improvement by this change (~8.8 s on my system). So the timeout should stay the same.

On the other hand, the code from #12861 (array in function) is now checked in 11 s vs. practically a hang without the change.
So please add that code to a new test in performance_test.py

[clock999]

I adjusted the timeout value set by the code line:

@pytest.mark.timeout(20)

I reduced the value from 20 to 15.
With the option --showtime=summary open, running cppcheck with the case locally on my pc, the time is less than 1 second with the commit comparing 10 seconds without it. It is a big improvement. But the case will fail with setting the timeout as 10 while running the script check on github. Not sure if it is related with MacOS.
I am afraid that I really can't check it on the MacOS as I almost have no experience on Mac and I don't have a Mac device.
Seems there is some problem with running performance_test.py locally on my pc, and I will check that later.

[clock999]

I have done some experiments. The existing test (array in namespace) shows no improvement by this change (~8.8 s on my system). So the timeout should stay the same.

On the other hand, the code from #12861 (array in function) is now checked in 11 s vs. practically a hang without the change. So please add that code to a new test in performance_test.py

Ok, I just saw this comment. I will check this later.

[chrchr-github]

I adjusted the timeout value set by the code line:

Please refer to my previous comment #7757 (comment)
Sorry for the confusion.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud