-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Fix #15000 Propagate integral cast ranges to condition analysis #8819
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,7 +46,7 @@ namespace ValueFlow | |
| if (!vt || !vt->isIntegral() || vt->pointer) | ||
| return false; | ||
|
|
||
| std::uint8_t bits; | ||
| std::size_t bits; | ||
| switch (vt->type) { | ||
| case ValueType::Type::BOOL: | ||
| bits = 1; | ||
|
|
@@ -66,29 +66,37 @@ namespace ValueFlow | |
| case ValueType::Type::LONGLONG: | ||
| bits = platform.long_long_bit; | ||
| break; | ||
| case ValueType::Type::WCHAR_T: | ||
| bits = platform.sizeof_wchar_t * platform.char_bit; | ||
| break; | ||
| default: | ||
| return false; | ||
| } | ||
|
|
||
| if (bits == 0) { | ||
| return false; | ||
| } | ||
| if (bits == 1) { | ||
| minValue = 0; | ||
| maxValue = 1; | ||
| } else if (bits < 62) { | ||
| if (vt->sign == ValueType::Sign::UNSIGNED) { | ||
| minValue = 0; | ||
| maxValue = (1LL << bits) - 1; | ||
| } else { | ||
| } else if (vt->sign == ValueType::Sign::SIGNED) { | ||
| minValue = -(1LL << (bits - 1)); | ||
| maxValue = (1LL << (bits - 1)) - 1; | ||
| } | ||
| } else | ||
| return false; | ||
| } else if (bits == 64) { | ||
| if (vt->sign == ValueType::Sign::UNSIGNED) { | ||
| minValue = 0; | ||
| maxValue = LLONG_MAX; // todo max unsigned value | ||
| } else { | ||
| maxValue = LLONG_MAX; // MathLib::bigint cannot represent ULLONG_MAX; conservative max for unsigned 64-bit | ||
| } else if (vt->sign == ValueType::Sign::SIGNED) { | ||
| minValue = LLONG_MIN; | ||
| maxValue = LLONG_MAX; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand how signed 64 bit and unsigned 64 bit have the same max value. Maybe for unsigned you meant ULLONG_MAX?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, ULLONG_MAX is the true maximum for unsigned 64-bit values. getMinMaxValues currently returns bigint, which is signed and cannot represent ULLONG_MAX. I will update the legacy comment "todo max unsigned value" to "bigint cannot represent ULLONG_MAX; conservative max for unsigned 64-bit" A biguint-based API can be introduced separately if the exact unsigned 64-bit maximum is needed ? A quick grep shows there will be some updates needed in some call-sites. |
||
| } | ||
| } else | ||
| return false; | ||
| } else { | ||
| return false; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an AI review take it with a grain of salt. Please feel free to reject it by clicking on Resolve button
The new upper bound for unsigned casts can create a false positive when the cast is followed by unsigned arithmetic that wraps around. I built this branch and its merge-base:
For
x == 0the condition is true:0u - 1uwraps to0xFFFFFFFF. The impossible>= 4294967296value on the cast is shifted by- 1uto an impossible>= 4294967295, with no wrap-around.To be fair, the merge-base already has this problem for the lower bound (
!<= -1of unsigned expressions). These already give wrong "always false" results there:(uint32_t)x + 1u == 0u,(unsigned)x + 1u < 1u, and(uint32_t)x + 0x80000000u < 0x80000000u. So the root cause is that impossible ranges go through+/-/*on unsigned operands of rankintor higher, where the result wraps instead of being promoted. The PR adds the same problem at the other end of the range.To keep the PR conservative, maybe don't propagate impossible bound values through arithmetic whose result is an unsigned type that wraps (
unsigned intand larger).unsigned char/unsigned shortare promoted toint, so(uint16_t)x + 1 == 0stays a correct warning. The #15000 reproducer should still work, since its arithmetic is done inlong long. A test for the wrap-around case above would be good.For reference: the comparison on
lib/,cli/,gui/,test/cfg/,samples/and simplecpp gave identical results for the PR and its merge-base. I also found no problems with unsigned 64-bit casts ((uint64_t)x > 0x8000000000000000ULL,== ULLONG_MAX, etc.).