Fix #12861 Hang in valueFlowCondition() with huge array #7757

clock999 · 2025-08-20T10:58:09Z

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 · 2025-08-20T11:13:24Z

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

firewave · 2025-08-20T11:33:37Z

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 · 2025-08-20T11:58:32Z

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

clock999 · 2025-08-20T23:37:49Z

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 · 2025-08-22T11:04:54Z

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

clock999 · 2025-08-23T03:00:54Z

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 · 2025-08-23T10:28:49Z

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

lib/astutils.cpp

lib/programmemory.cpp

firewave · 2025-08-25T14:04:33Z

lib/token.h

 #include "templatesimplifier.h"
 #include "utils.h"
 #include "vfvalue.h"
+#include "tokenlist.h"


This seems unnecessary.

lib/astutils.cpp

danmar · 2025-08-28T18:50:39Z

lib/token.h


    }
-    RET_NONNULL Token *astTop() {
+    /** If the ast tree has not been created, pls make sure to use cache=false,


remove the "pls" :-)

sonarqubecloud · 2025-08-29T01:48:01Z

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 · 2025-09-14T01:51:53Z

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

clock999 · 2026-01-03T03:00:34Z

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 · 2026-01-03T04:44:06Z

lib/token.h

+        }
        while (ret->mImpl->mAstParent)
            ret = ret->mImpl->mAstParent;
+        mImpl->mAstTop = ret;


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 · 2026-01-03T04:47:32Z

lib/tokenlist.cpp

+            Token * top = init;
+            while (top->astParent()) {
+                top = top->astParent();
+            }


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

firewave · 2026-01-03T05:44:33Z

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 · 2026-01-05T15:31:01Z

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.

lib/token.h

clock999 · 2026-01-06T14:34:10Z

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 · 2026-01-07T03:55:09Z

lib/tokenlist.cpp

+            Token * top = tok;
+            while (top->astParent()) {
+                top = top->astParent();
+            }


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 · 2026-01-07T03:56:02Z

lib/token.cpp

-        tok = tok->astTop();
+        while (tok->mImpl->mAstParent) {
+            tok = tok->mImpl->mAstParent;
+        }


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

pfultz2 · 2026-01-07T03:56:44Z

lib/token.h

     */
    void astParent(Token* tok);
+    void astTop(Token * tok) {
+        if (tok) {


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

pfultz2 · 2026-01-07T03:57:56Z

lib/tokenlist.cpp

+                top = top->astParent();
+            }
+            semicolon1->astOperand1(top);
+        }


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 · 2026-01-07T06:08:38Z

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 · 2026-01-07T07:28:16Z

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

chrchr-github · 2026-01-08T17:34:28Z

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

clock999 · 2026-01-09T01:13:00Z

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 · 2026-01-09T18:37:11Z

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 · 2026-01-10T01:44:25Z

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 · 2026-01-10T08:35:56Z

See here:

cppcheck/test/cli/performance_test.py

Line 225 in 574fffa

@pytest.mark.timeout(20)

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 · 2026-01-10T11:31:47Z

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 · 2026-01-10T11:39:44Z

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 · 2026-01-10T11:41:29Z

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 · 2026-01-10T11:41:45Z

I adjusted the timeout value set by the code line:

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

sonarqubecloud · 2026-01-11T02:01:44Z

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 force-pushed the wy_dev_12861 branch from 3ae4054 to e190ffa Compare August 20, 2025 22:59

clock999 force-pushed the wy_dev_12861 branch from e190ffa to 2964ff8 Compare August 21, 2025 01:55

clock999 force-pushed the wy_dev_12861 branch from 2964ff8 to d5f0f15 Compare August 24, 2025 10:53

github-advanced-security bot found potential problems Aug 24, 2025

View reviewed changes

lib/astutils.cpp Fixed Show fixed Hide fixed

lib/programmemory.cpp Fixed Show fixed Hide fixed

clock999 force-pushed the wy_dev_12861 branch 2 times, most recently from 7032692 to ca60f59 Compare August 25, 2025 01:54

firewave reviewed Aug 25, 2025

View reviewed changes

clock999 force-pushed the wy_dev_12861 branch from ca60f59 to bf40b4a Compare August 26, 2025 01:51

danmar reviewed Aug 28, 2025

View reviewed changes

clock999 force-pushed the wy_dev_12861 branch from bf40b4a to ca7ced4 Compare August 29, 2025 01:41

clock999 force-pushed the wy_dev_12861 branch from ca7ced4 to c612061 Compare January 3, 2026 02:29

pfultz2 reviewed Jan 3, 2026

View reviewed changes

clock999 force-pushed the wy_dev_12861 branch from c612061 to db31440 Compare January 6, 2026 14:10

github-advanced-security bot found potential problems Jan 6, 2026

View reviewed changes

lib/token.h Fixed Show fixed Hide fixed

lib/token.h Fixed Show fixed Hide fixed

clock999 force-pushed the wy_dev_12861 branch from db31440 to f9cb1b4 Compare January 6, 2026 14:25

clock999 force-pushed the wy_dev_12861 branch from f9cb1b4 to 5aaa6b3 Compare January 6, 2026 16:05

clock999 force-pushed the wy_dev_12861 branch from 5aaa6b3 to 2c0b32f Compare January 7, 2026 02:22

pfultz2 reviewed Jan 7, 2026

View reviewed changes

clock999 force-pushed the wy_dev_12861 branch 2 times, most recently from 04eb081 to 379f0aa Compare January 7, 2026 06:03

pfultz2 approved these changes Jan 8, 2026

View reviewed changes

clock999 force-pushed the wy_dev_12861 branch 3 times, most recently from a5ce2f9 to aff2cca Compare January 10, 2026 10:38

clock999 force-pushed the wy_dev_12861 branch 3 times, most recently from eae44c5 to 5261c77 Compare January 11, 2026 01:41

Fix #12861 Hang in valueFlowCondition() with huge array

c3b1b6a

clock999 force-pushed the wy_dev_12861 branch from 5261c77 to c3b1b6a Compare January 11, 2026 01:54

chrchr-github merged commit 34b9c45 into danmar:main Jan 11, 2026
63 checks passed

Fix #12861 Hang in valueFlowCondition() with huge array #7757

Fix #12861 Hang in valueFlowCondition() with huge array #7757

Conversation

clock999 commented Aug 20, 2025

Uh oh!

chrchr-github commented Aug 20, 2025

Uh oh!

firewave commented Aug 20, 2025

Uh oh!

clock999 commented Aug 20, 2025

Uh oh!

clock999 commented Aug 20, 2025

Uh oh!

chrchr-github commented Aug 22, 2025

Uh oh!

clock999 commented Aug 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrchr-github commented Aug 23, 2025

Uh oh!

Uh oh!

Uh oh!

firewave Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

danmar Aug 28, 2025

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Aug 29, 2025

Quality Gate passed

Uh oh!

firewave commented Sep 14, 2025

Uh oh!

clock999 commented Jan 3, 2026

Uh oh!

pfultz2 Jan 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pfultz2 Jan 3, 2026

Choose a reason for hiding this comment

Uh oh!

firewave commented Jan 3, 2026

Uh oh!

pfultz2 commented Jan 5, 2026

Uh oh!

Uh oh!

Uh oh!

clock999 commented Jan 6, 2026

Uh oh!

pfultz2 Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

pfultz2 Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

pfultz2 Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

pfultz2 Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jan 7, 2026

Quality Gate passed

Uh oh!

clock999 commented Jan 7, 2026

Uh oh!

chrchr-github commented Jan 8, 2026

Uh oh!

clock999 commented Jan 9, 2026

Uh oh!

chrchr-github commented Jan 9, 2026

Uh oh!

clock999 commented Jan 10, 2026

Uh oh!

chrchr-github commented Jan 10, 2026

Uh oh!

chrchr-github commented Jan 10, 2026

Uh oh!

clock999 commented Aug 23, 2025 •

edited

Loading

pfultz2 Jan 3, 2026 •

edited

Loading