Repository navigation
Conversation
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
The new recursive evaluator crashes with a stack overflow on deeply nested #if expressions, and base handles the same input. The recursion depth has no limit. Every ( goes through about 12 stack frames (conditional → logicalOr → … → unary → primary → conditional), and a chain of unary !/~ goes one frame per operator. With the default 8 MB stack, about 20000 nested parentheses (debug build) or 50000 (-O2) give a SIGSEGV. Both gcc and clang accept this input, and so does base simplecpp, which evaluates the condition iteratively. Threads with smaller stacks will crash much sooner, for example cppcheck worker threads (on Windows the default thread stack is 1 MB). A fuzzer will also hit this. A nesting-depth counter that throws std::runtime_error past some limit would fix it.
Apart from that, the short-circuit behaviour is correct in every case I tried, including nested ?: and combinations of &&/||/?:. Results for the valid expressions I compared match base, except where the PR intends a change. I found no other new problems.
Testing
Built /work/pr with make -j8 testrunner simplecpp CXXOPTS=-Werror without warnings, and built base too. In pr/:
./testrunnerpasses.python3 run-tests.py: 176 tests, 0 failed (the same as base).python3 -m pytest integration_test.py: 128 passed../selfcheck.shpasses.
Stack overflow (regression):
cd /work/scratch
python3 -c "n=20000; open('d2.c','w').write('#if '+'('*n+'1'+')'*n+'\nyes\n#endif\n')"
/work/pr/simplecpp d2.c # Segmentation fault (rc 139)
/work/base/simplecpp d2.c # prints: yes
gcc -E d2.c; clang -E d2.c # both print: yesA -O2 build (g++ -O2 -std=c++11 main.cpp simplecpp.cpp) still segfaults at n=50000. A unary chain such as '#if ' + '~'*200000 + '1' (or '! '*200000) also segfaults in pr.
Short-circuit comparison with gcc -E / g++ -E and base: I compared about 50 expressions. All of these now match gcc, where base reported division by zero:
1 ? 1 : 0 || 1/0(0 && 1/0) || 11 && 0 ? 1/0 : 50 ? (1/0) : (0 ? 1/0 : 1)1 ? 0 ? 1/0 : 2 : 3defined(X) && X/0(1) ? (0) : (1/0)
Precedence, associativity, alternative tokens and suffixed, hex and char literals give the same results as base. Some differences from gcc exist in base as well, so the PR does not cause them: -1 > 0u, 1 << 64, 1 and not 0 in C++, 0b101, 1 , 2, and invalid expressions being silently false.
Automated review of 79d0f4e by Claude. The pull request was built and tested locally.
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
The nesting limit added in 5310bad rejects valid #if expressions that base, gcc and clang all accept. It fixes the stack overflow, but the limit of 256 is low, and one counter is shared by parentheses, every prefix unary operator and every ?:. Three kinds of input now fail with failed to evaluate #if condition, expression is nested too deeply:
- a chain of more than 256
?:, which is a common pattern in generated code and lookup macros; - macros defined in terms of each other, where each one wraps the previous one in parentheses (
#define Xn (Xn-1 + 1)), once the chain is deeper than 256; - only about 128 levels of
-( … ), because the unary operator and the parenthesis each count as one level.
Base evaluated all of these without trouble. Annex B's "256 nesting levels of parenthesized expressions" is a minimum that implementations must support, not a maximum. cppcheck will now skip or mis-evaluate such conditions. A much larger limit would fix this, and so would counting only real recursion through (: handle prefix operators in a loop and right-nested ?: iteratively.
I found no other new problems. Short-circuiting and the other tests behave correctly.
Testing
I built /work/pr with make -j8 testrunner simplecpp CXXOPTS=-Werror and got no warnings. I also built base.
./testrunner: passes, includingifShortCircuitandifNestingDepth.python3 run-tests.py: 0 failed.python3 -m pytest integration_test.py: 128 passed../selfcheck.sh: passes.
Regression: valid deep expressions are rejected. The test files are generated in /work/scratch:
n=300
open('chain.c','w').write('#define X0 0\n'+''.join('#define X%d (X%d+1)\n'%(i,i-1) for i in range(1,n+1))+'#if X%d == %d\nyes\n#else\nno\n#endif\n'%(n,n))
open('tern.c','w').write('#define V 299\n#if '+''.join('V==%d ? %d : '%(i,i) for i in range(300))+'-1\nyes\n#endif\n')
open('neg.c','w').write('#if '+'-('*150+'1'+')'*150+' == 1\nyes\n#endif\n')For example, tern.c contains #if V==0 ? 0 : V==1 ? 1 : ... : -1, and chain.c contains #define X1 (X0+1) … #if X300 == 300.
$ /work/pr/simplecpp chain.c
chain.c:302: syntax error: failed to evaluate #if condition, expression is nested too deeply
$ /work/pr/simplecpp tern.c
tern.c:2: syntax error: failed to evaluate #if condition, expression is nested too deeply
$ /work/pr/simplecpp neg.c
neg.c:1: syntax error: failed to evaluate #if condition, expression is nested too deeply
/work/base/simplecpp, gcc -E and clang -E print yes for all three files.
Automated review of 5310bad by Claude. The pull request was built and tested locally.
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
The new right-to-left ?: folding is quadratic in the length of a ?: chain. For every ? it folds, constFoldQuestionOp scans the whole range again from tok1 to find the last ?. A generated lookup chain such as #if V==0 ? 0 : V==1 ? 1 : ... : -1 with 20000 entries takes 10 s instead of 0.1 s in base, and 5000 entries take 0.56 s instead of 0.025 s. cppcheck preprocesses every file with this code, so the cost matters.
I found no other new problems. Short-circuiting through &&, || and ?: now matches gcc and clang in every case I tried, including nested and chained ?: and error values that pass through unary, arithmetic, shift, comparison and bitwise operators. A small side note: the PR description still says "Replace the constFold* functions with a recursive-descent evaluator", but the current code keeps the constFold* functions and uses error values instead.
Testing
I built /work/pr with make -j8 testrunner simplecpp CXXOPTS=-Werror and got no warnings. I also built base/.
./testrunner: 0 failed.python3 run-tests.py: 0 failed.python3 -m pytest integration_test.py: 128 passed../selfcheck.sh: passes.
Comparison with gcc -E, clang -E and base. I compared about 35 #if expressions, for example 0 && 1/0 || 1, 1 || 1/0 && 1, 0 ? 1 : 0 ? 1/0 : 1, 0 && ~(1/0), 0 && 1 << (1/0), 0 && (1/0 == 1/0), (0 && 1/0) ? 1/0 : 1, (1 ? 2 : 3) + (0 ? 1/0 : 4) == 6 and 0 && (-9223372036854775807-1)/-1. All of them match gcc and clang now, where base reported division by zero. Expressions in which the division is evaluated, such as 1 ? 1/0 : 0, -(1/0) and 1 && (0 || 1/0), still give an error. The remaining differences from gcc also exist in base (the comma operator, invalid expressions being silently false).
Performance regression with long ?: chains:
cd /work/scratch
python3 -c "
n=20000
open('tc.c','w').write('#define V 5\n#if '+''.join('V==%d ? %d : '%(i,i) for i in range(n))+'-1\nyes\n#endif\n')"
time /work/base/simplecpp tc.c # yes, real 0m0.107s
time /work/pr/simplecpp tc.c # yes, real 0m10.053s?: entries in the chain |
base | pr |
|---|---|---|
| 1000 | 0.008 s | 0.030 s |
| 5000 | 0.025 s | 0.564 s |
| 20000 | 0.107 s | 10.05 s |
The time grows quadratically.
I also checked 20000 parenthesised (0 && 1) terms joined by +. Base and pr both take about 4.2 s, so that slowdown already exists in base and the PR does not cause it.
Automated review of 1d63864 by Claude. The pull request was built and tested locally.
A division by zero no longer throws while folding. It gives an error value, that is passed on by the other operators. && || and ?: discard it when the operand is not evaluated. The error is thrown if the error value is still there after folding. Also fold chained ?: from the right, since ?: is right-associative.
After a ?: is folded, search backward from the result for the next ? instead of rescanning the whole range.
…nd ^ isAlternativeBinaryOp() did not accept an error value as operand, so "0 and (1/0)" was not converted to "0 && (1/0)" and the error was reported. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Replace the constFold* functions with a recursive-descent evaluator. Operands that are not evaluated are only parsed, so they can't cause division by zero errors.