Repository navigation
Fix #11592 FN compareValueOutOfTypeRangeError with char - #8940
chrchr-github wants to merge 4 commits into
Conversation
|
Test results for commit ca3eda6 (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8940/ Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong. |
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
One new problem: a false positive on plain char compared with an unsigned value. When char is signed (unix64, native on x86), c > 127u and c > sizeof(...) are now reported as "always false". The usual arithmetic conversions turn a negative c into a huge unsigned value, so these conditions are true whenever c < 0. I confirmed this by compiling and running the code. The same false positive already exists for signed char and short, but the PR extends it to plain char, which is far more common, especially with sizeof operands. A fix would be to skip the relational operators (or allow for wraparound) when the value is unsigned with a rank of at least int and the type expression is signed. == and != against an unsigned value are still reported correctly.
Minor gap: the new condition only checks for a lowercase 's'. SymbolDatabase also accepts 'S', and with <default-sign>S</default-sign> in a platform file the new warning never appears.
Otherwise the new warnings for c == 128, c > 127, c == 255 and p[0] == 200 are correct on platforms where char is signed, and they correctly do not appear with <default-sign>u</default-sign>.
Testing
Builds and unit tests
- Built
cppcheckandtestrunnerinpr/, andcppcheckinbase/. ./testrunner TestCondition: 75 tests, 0 failures../testrunner TestOther TestType: 0 failures.
scratch/t1.cpp: plain, signed and unsigned char, with literal and pointer operands, on four platforms
Results on unix64 and native (x86):
- The PR adds warnings for
c == 128,c > 127,c == 255andp[0] == 200(const char *p). They are correct for a signedchar. c >= 127is still not reported, which is also correct.
Platform files:
<default-sign>u</default-sign>: base and PR give the same output, as intended.<default-sign>S</default-sign>: no new warnings, because only lowercase's'is checked.
False positive with unsigned operands (scratch/t2.cpp)
void f(char c, signed char s, short sh) {
if (c > 127u) {}
if (c == 200u) {}
if (s > 127u) {}
if (sh > 32767u) {}
if (c > sizeof(int) * 50) {}
}
int main(){ char c=-1; return c > 127u; }Command:
cppcheck -q --enable=style --platform=unix64 t2.cpp
Output, base (only signed char and short, a false positive that already exists):
t2.cpp:4:13: style: Comparing expression of type 'signed char' against value 127. Condition is always false. [compareValueOutOfTypeRangeError]
t2.cpp:5:14: style: Comparing expression of type 'signed short' against value 32767. Condition is always false. [compareValueOutOfTypeRangeError]
Output, PR (adds these lines to base's two):
t2.cpp:2:13: style: Comparing expression of type 'char' against value 127. Condition is always false. [compareValueOutOfTypeRangeError]
t2.cpp:3:14: style: Comparing expression of type 'char' against value 200. Condition is always false. [compareValueOutOfTypeRangeError]
t2.cpp:6:25: style: Comparing expression of type 'char' against value 200. Condition is always false. [compareValueOutOfTypeRangeError]
t2.cpp:8:35: style: Comparing expression of type 'char' against value 127. Condition is always false. [compareValueOutOfTypeRangeError]
- The warnings on lines 2, 6 and 8 are wrong.
g++ t2.cpp && ./t2exits with code 1, soc > 127uis true forc = -1. - The warning on line 3 (
c == 200u) is correct.
Automated review of ca3eda6 by Claude. The pull request was built and tested locally.
| const auto unsignedTypeMaxValue = (1LL << bits) - 1LL; | ||
| long long typeMaxValue; | ||
| if (typeTok->valueType()->sign != ValueType::Sign::SIGNED) | ||
| if (typeVt->sign != ValueType::Sign::SIGNED && !(typeVt->sign == ValueType::Sign::UNKNOWN_SIGN && typeVt->type == ValueType::Type::CHAR && mSettings.platform.defaultSign == 's')) |
There was a problem hiding this comment.
AI review
Plain char now gets the signed range, but the operand conversions are not considered. When valueTok is unsigned with a rank of at least int (127u, sizeof(...)), a negative c becomes a huge unsigned value, so c > 127u is true for c == -1. The PR still reports it as always false:
int main(){ char c=-1; return c > 127u; } // exits with 1 at runtime
void f(char c){ if (c > sizeof(int) * 50) {} }--platform=unix64 gives Comparing expression of type 'char' against value 127. Condition is always false. and ... against value 200 .... Base already has this false positive for signed char and short, but this change extends it to plain char, where c > sizeof(...) is common. == and != against an unsigned value are still correct. The relational operators should be skipped, or should allow for wraparound, when the value operand is unsigned with a rank of at least int.
There was a problem hiding this comment.
| const auto unsignedTypeMaxValue = (1LL << bits) - 1LL; | ||
| long long typeMaxValue; | ||
| if (typeTok->valueType()->sign != ValueType::Sign::SIGNED) | ||
| if (typeVt->sign != ValueType::Sign::SIGNED && !(typeVt->sign == ValueType::Sign::UNKNOWN_SIGN && typeVt->type == ValueType::Type::CHAR && mSettings.platform.defaultSign == 's')) |
There was a problem hiding this comment.
AI review
Only lowercase 's' is checked here. SymbolDatabase (lib/symboldatabase.cpp:67) also accepts 'S', and a platform file with <default-sign>S</default-sign> does not get the new warning (I confirmed this with c == 128).
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
New false positive: a plain char compared with a value cast to char. ValueFlow gives (char)200, static_cast<char>(0x80) and macros such as #define MARK ((char)0xE2) the unsigned values 200, 128 and 226. The cause is that ValueFlow::truncateIntValue (lib/vf_common.cpp:107) sign-extends only when dst_sign == SIGNED, and plain char has UNKNOWN_SIGN. Before this PR that did not matter, because the check took plain char to have a range of up to 255. Now that it uses a signed range when defaultSign == 's', c == (char)200 is reported as "always false". The condition is actually true for c = (char)200, and a compiled run confirms it. Comparing a char with a cast byte constant is a common pattern in UTF-8 and byte-parsing code. A fix is either to sign-extend UNKNOWN_SIGN char in the cast truncation when defaultSign is signed, or to skip the new signed range when the value operand is itself of type char.
The 'S' case and the unsigned-operand (c > 127u) false positive were already raised in comments.md, so I left them out.
Testing
- Built
cppcheckandtestrunnerinpr/, andcppcheckinbase/. Both builds succeeded. ./testrunner TestCondition: 75 tests, 0 failures.
False positive with a cast to char (scratch/b.cpp):
#define MARK ((char)0xE2)
int f(char c) {
if (c == (char)200) return 1;
if (c == static_cast<char>(0x80)) return 2;
if (c == MARK) return 3;
return 0;
}
int main() { return f((char)200) + 10*f((char)0x80); }Command: cppcheck -q --enable=style --platform=unix64 b.cpp
Output with the PR:
b.cpp:3:14: style: Comparing expression of type 'char' against value 200. Condition is always false. [compareValueOutOfTypeRangeError]
b.cpp:4:31: style: Comparing expression of type 'char' against value 128. Condition is always false. [compareValueOutOfTypeRangeError]
b.cpp:5:14: style: Comparing expression of type 'char' against value 226. Condition is always false. [compareValueOutOfTypeRangeError]
- Base gives no output for this file. The PR gives the same result with
--platform=native. g++ b.cpp -o b && ./bexits with code 21, so the conditions on lines 3 and 4 are true at runtime.--debug-normalshows( always 200for(char)200. ValueFlow does not sign-extend the cast value for plainchar.
Other checks
c == '\xff'and*p == '\xe2'give no warning, which is correct.c == 0xffandc == L'\xff'are reported, which is correct.
Automated review of 3a9288b by Claude. The pull request was built and tested locally.
| const auto unsignedTypeMaxValue = (1LL << bits) - 1LL; | ||
| long long typeMaxValue; | ||
| if (typeTok->valueType()->sign != ValueType::Sign::SIGNED) | ||
| if (typeVt->sign != ValueType::Sign::SIGNED && !(typeVt->sign == ValueType::Sign::UNKNOWN_SIGN && typeVt->type == ValueType::Type::CHAR && mSettings.platform.defaultSign == 's')) |
There was a problem hiding this comment.
AI review
Plain char now gets the signed range, but ValueFlow does not sign-extend a value cast to plain char. ValueFlow::truncateIntValue (lib/vf_common.cpp:107) sign-extends only when the sign is SIGNED, so (char)200 becomes the known value 200 instead of -56. Comparisons like these are now reported as always false, but they can be true:
#define MARK ((char)0xE2)
int f(char c) {
if (c == (char)200) return 1; // "against value 200. Condition is always false"
if (c == static_cast<char>(0x80)) return 2; // "against value 128 ..."
if (c == MARK) return 3; // "against value 226 ..."
return 0;
}f((char)200) returns 1 when compiled with g++ on x86_64. Base does not warn here. One fix is to sign-extend UNKNOWN_SIGN char in the cast truncation when defaultSign is signed. Another is to skip this new case when valueTok is itself of type char.
|
Test results for commit 3a9288b (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8940/ Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong. |
No description provided.