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.

[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

[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.