Repository navigation
Fix #15032 FP knownConditionTrueFalse for bitwise negation of char - #8941
chrchr-github wants to merge 8 commits into
Conversation
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
I found no new correctness problems. On platforms where char is signed, the PR fixes the reported false positive. It also fixes a group of related wrong results for plain char, which used to be truncated as if it were unsigned. On a platform where char is unsigned, the results do not change. The full testrunner passes.
Two things remain:
--platform=unspecifiedstill gets wrong results. There,defaultSign == '\0', so plaincharstill falls back to unsigned truncation. The #15032 example still reports'~c==-256' is always true, andchar c = 0; c--; if (c == -1)still reports "always false". The signedness is unknown, so these values should not be treated as known. This was already the case before the PR, so it is not a regression, but the fix is incomplete for that platform.- The cause seems to be in
parsedecl, not intruncateIntValue.parsedecl(lib/symboldatabase.cpp:7751) leaves a variable declaredcharwithUNKNOWN_SIGN. Other paths (symboldatabase.cpp:6929,8337) already replace it withmDefaultSignedness. Doing the same inparsedeclwould fix this for every reader ofValueType::sign, not only the four truncation call sites. Note that it would also change theUNKNOWN_SIGNchecks atsymboldatabase.cpp:1934and:6020.
Testing
Build: I built cppcheck and testrunner in pr/ and cppcheck in base/. All builds succeeded.
Tests: ./testrunner TestValueFlow TestCondition TestOther TestType TestUninitVar TestNullPointer TestBufferOverrun passed. The full ./testrunner -q also passed: 5351 tests, 0 failed.
Manual tests: I compared base and PR with --enable=style on unix64, unspecified, and a custom platform XML with <default-sign>unsigned</default-sign>. The test files contain:
void f() { char c = -1; if (~c == -256) {} }
void f1() { char c = (char)200; if (c == -56) {} }
void f3() { char c = 200; c += 1; if (c < 0) {} }
void f6(struct S *s) { s->m = 200; if (s->m < 0) {} } // char member
void f7() { char c = 0; c--; if (c == -1) {} }
void f8() { char c = -1; int i = c; if (i == 255) {} }
void f9() { char a[2]; a[0] = 200; if (a[0] > 0) {} }
void h() { char c = 255; if (c == -1) {} }
void k() { char c = 127; c++; if (c == -128) {} }unix64(signedchar): on base, every one of these gave the unsigned answer, which is wrong. For example,~c==-256was "always true" andc==-1afterc--was "always false". With the PR, all of them give the correct signed answer.- Unsigned-
charplatform XML: base and PR give identical results, which are correct for unsignedchar. --platform=unspecified: base and PR give identical results, and they are still the unsigned ones. For example:This is the #15032 false positive, still present when the platform's$ pr/cppcheck -q --enable=style --platform=unspecified t1.c t1.c:3:12: style: Condition '~c==-256' is always true [knownConditionTrueFalse]charsignedness is unknown.
I found no crashes or hangs. Every call site already has a non-null dst before it calls truncateIntValue.
Automated review of 22d1c8c by Claude. The pull request was built and tested locally.
| const MathLib::biguint signBit = 1ULL << ((value_size * 8) - 1); | ||
| value &= unsignedMaxValue; | ||
| if (dst_sign == ValueType::Sign::SIGNED && (value & signBit)) | ||
| if ((dst->sign == ValueType::Sign::SIGNED || (dst->sign == ValueType::Sign::UNKNOWN_SIGN && dst->type == ValueType::Type::CHAR && platform.defaultSign == 's')) && (value & signBit)) |
There was a problem hiding this comment.
AI review
When platform.defaultSign == '\0' (--platform=unspecified), plain char still goes through unsigned truncation. So char c = -1; if (~c == -256) {} still reports Condition '~c==-256' is always true, and char c = 0; c--; if (c == -1) {} reports "always false". When signedness is unknown, a value that sets the sign bit has no single known result.
Also, the cause seems to be that parsedecl (symboldatabase.cpp:7751) leaves char variables with UNKNOWN_SIGN, while symboldatabase.cpp:6929/8337 already apply mDefaultSignedness. Applying it in parsedecl would fix this for every reader of sign, not only these truncation call sites. It would also change the UNKNOWN_SIGN checks at symboldatabase.cpp:1934 and :6020.
No description provided.