Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 50 additions & 1 deletion lib/valueflow.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1087,6 +1087,24 @@ static void valueFlowImpossibleValues(TokenList& tokenList, const Settings& sett
upper.bound = ValueFlow::Value::Bound::Lower;
upper.setImpossible();
setTokenValue(tok, std::move(upper), settings);
} else if (tok->isCast() && tok->valueType() && tok->valueType()->isIntegral() && !tok->valueType()->pointer) {

Copy link
Copy Markdown
Collaborator

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:

#include <stdint.h>
void use(int);
void f(int x) { if ((uint32_t)x - 1u == 0xFFFFFFFFu) use(6); }
this PR:     Condition '(uint32_t)x-1u==0xFFFFFFFFu' is always false [knownConditionTrueFalse]
merge-base:  no warning

For x == 0 the condition is true: 0u - 1u wraps to 0xFFFFFFFF. The impossible >= 4294967296 value on the cast is shifted by - 1u to an impossible >= 4294967295, with no wrap-around.

To be fair, the merge-base already has this problem for the lower bound (!<= -1 of 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 rank int or 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 int and larger). unsigned char/unsigned short are promoted to int, so (uint16_t)x + 1 == 0 stays a correct warning. The #15000 reproducer should still work, since its arithmetic is done in long 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.).

MathLib::bigint minValue;
MathLib::bigint maxValue;
if (!ValueFlow::getMinMaxValues(tok->valueType(), settings.platform, minValue, maxValue))
continue;

if (minValue > std::numeric_limits<MathLib::bigint>::min()) {
ValueFlow::Value lower{minValue - 1};
lower.bound = ValueFlow::Value::Bound::Upper;
lower.setImpossible();
setTokenValue(tok, std::move(lower), settings);
}
if (maxValue < std::numeric_limits<MathLib::bigint>::max()) {
ValueFlow::Value upper{maxValue + 1};
upper.bound = ValueFlow::Value::Bound::Lower;
upper.setImpossible();
setTokenValue(tok, std::move(upper), settings);
}
} else if (astIsUnsigned(tok) && !astIsPointer(tok)) {
std::vector<MathLib::bigint> minvalue = minUnsignedValue(tok);
if (minvalue.empty())
Expand Down Expand Up @@ -5134,6 +5152,26 @@ static bool isIntegralOrPointer(const Token* tok)
return false;
}

/**
* @brief Check if the token is an arithmetic operation whose result type is
* an unsigned integer, i.e. arithmetic that may wrap around.
*
* Impossible bounds cannot be propagated through such arithmetic because
* wrap-around invalidates the bounds.
*/
static bool isUnsignedArithmeticResult(const Token* tok)
{
if (!Token::Match(tok, "+|-|*"))
return false;
const ValueType* vt = tok->valueType();
return vt && vt->isIntegral() && vt->pointer == 0 &&
vt->sign == ValueType::Sign::UNSIGNED &&
(vt->type == ValueType::Type::INT ||
vt->type == ValueType::Type::LONG ||
vt->type == ValueType::Type::LONGLONG ||
vt->type == ValueType::Type::UNKNOWN_INT);
}

static void valueFlowInferCondition(TokenList& tokenlist, const Settings& settings)
{
for (Token* tok = tokenlist.front(); tok; tok = tok->next()) {
Expand All @@ -5154,8 +5192,19 @@ static void valueFlowInferCondition(TokenList& tokenlist, const Settings& settin
}
}
} else if (isIntegralOrPointer(tok->astOperand1()) && isIntegralOrPointer(tok->astOperand2())) {
std::list<ValueFlow::Value> lhsValues = tok->astOperand1()->values();
std::list<ValueFlow::Value> rhsValues = tok->astOperand2()->values();
// Impossible bounds cannot be propagated through arithmetic whose
// result type is unsigned because wrap-around invalidates the bound
if (isUnsignedArithmeticResult(tok)) {
const auto isImpossibleIntegralBound = [](const ValueFlow::Value& v) {
return v.isIntValue() && v.isImpossible() && v.bound != ValueFlow::Value::Bound::Point;
};
lhsValues.remove_if(isImpossibleIntegralBound);
rhsValues.remove_if(isImpossibleIntegralBound);
}
std::vector<ValueFlow::Value> result =
infer(makeIntegralInferModel(), tok->str(), tok->astOperand1()->values(), tok->astOperand2()->values());
infer(makeIntegralInferModel(), tok->str(), lhsValues, rhsValues);
for (ValueFlow::Value& value : result) {
setTokenValue(tok, std::move(value), settings);
}
Expand Down
20 changes: 14 additions & 6 deletions lib/vf_common.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

@aadanen aadanen Sep 12, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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;
}
Expand Down
18 changes: 18 additions & 0 deletions lib/vf_settokenvalue.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,18 @@ namespace ValueFlow
return;
}

// Impossible bounds cannot be propagated through arithmetic whose
// result type is unsigned, because wrap-around invalidates the bound
const ValueType* resultType = parent->valueType();
const bool wraps =
resultType && resultType->isIntegral() &&
resultType->sign == ValueType::Sign::UNSIGNED && resultType->pointer == 0 &&
(resultType->type == ValueType::Type::INT ||
resultType->type == ValueType::Type::LONG ||
resultType->type == ValueType::Type::LONGLONG ||
resultType->type == ValueType::Type::UNKNOWN_INT);
const bool skipImpossibleBounds = wraps && Token::Match(parent, "+|-|*");

for (const Value &value1 : parent->astOperand1()->values()) {
if (!isComputableValue(parent, value1))
continue;
Expand All @@ -500,6 +512,12 @@ namespace ValueFlow
continue;
if (!isCompatibleValues(value1, value2))
continue;
// Skip impossible bounds on arithmetic with unsigned result type
const bool operandHasImpossibleBound =
(value1.isIntValue() && value1.isImpossible() && value1.bound != Value::Bound::Point) ||
(value2.isIntValue() && value2.isImpossible() && value2.bound != Value::Bound::Point);
if (skipImpossibleBounds && operandHasImpossibleBound)
continue;
Value result(0);
combineValueProperties(value1, value2, result);
if (astIsFloat(parent, false)) {
Expand Down
93 changes: 93 additions & 0 deletions test/testcondition.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6612,6 +6612,99 @@ class TestCondition : public TestFixture {
"[test.cpp:4:13]: (style) Comparing expression of type 'const unsigned int &' against value 4294967295. Condition is always false. [compareValueOutOfTypeRangeError]\n",
errout_str());

// PR review: no false positive when unsigned arithmetic wraps around
check("void f(int x) {\n"
" if ((unsigned int)x - 1u == 0xFFFFFFFFu) {}\n"
"}\n", settingsUnix64);
ASSERT_EQUALS("", errout_str());

check("void f(int x) {\n"
" if ((unsigned short)x - 1u == 0xFFFFFFFFu) {}\n"
"}\n", settingsUnix64);
ASSERT_EQUALS("", errout_str());

// Signed result type after integer promotion must still warn
check("void f(int x) {\n"
" if ((unsigned short)x + 1 == 0) {}\n"
"}\n", settingsUnix64);
ASSERT_EQUALS("[test.cpp:2:29]: (style) Condition '(unsigned short)x+1==0' is always false [knownConditionTrueFalse]\n",
errout_str());

// Direct cast bounds are preserved
check("void f(int x) {\n"
" if ((unsigned int)x > 4294967295ULL) {}\n"
"}\n", settingsUnix64);
ASSERT_EQUALS("[test.cpp:2:25]: (style) Comparing expression of type 'unsigned int' against value 4294967295. Condition is always false. [compareValueOutOfTypeRangeError]\n",
errout_str());

// Reproduce the original typedef-heavy Simulink-generated pattern from #15000.
check("void f(unsigned int x) {\n"
" unsigned long long tmp = ((unsigned long long)x) + 1ULL;\n"
" if (tmp > 4294967295ULL)\n"
" tmp = 4294967295ULL;\n"
" if ((((long long)((unsigned int)tmp)) - 1LL) < 0LL) {}\n"
" if ((((long long)((unsigned int)tmp)) - 1LL) > 4294967295LL) {}\n"
"}\n", settingsUnix64);
ASSERT_EQUALS("[test.cpp:6:50]: (style) Condition '(((long long)((unsigned int)tmp))-1LL)>4294967295LL' is always false [knownConditionTrueFalse]\n",
errout_str());

check("void f(unsigned long long tmp) {\n"
" if (tmp > 4294967295ULL)\n"
" tmp = 4294967295ULL;\n"
" if (static_cast<long long>(static_cast<unsigned int>(tmp)) - 1LL > 4294967295LL) {}\n"
"}\n", settingsUnix64);
ASSERT_EQUALS("[test.cpp:4:70]: (style) Condition 'static_cast<long long>(static_cast<unsigned int>(tmp))-1LL>4294967295LL' is always false [knownConditionTrueFalse]\n",
errout_str());

// cast directly around variable: both the range-based and the declared-type
// analysis can prove the condition invariant; diag() must prevent duplicates
check("void f(unsigned char c) {\n"
" if ((unsigned char)c > 255) {}\n"
"}\n", settingsUnix64);
ASSERT_EQUALS("[test.cpp:2:28]: (style) Comparing expression of type 'unsigned char' against value 255. Condition is always false. [compareValueOutOfTypeRangeError]\n",
errout_str());

check("void f(unsigned char c) {\n"
" if ((unsigned char)c == 256) {}\n"
"}\n", settingsUnix64);
ASSERT_EQUALS("[test.cpp:2:29]: (style) Comparing expression of type 'unsigned char' against value 256. Condition is always false. [compareValueOutOfTypeRangeError]\n",
errout_str());

check("void f(unsigned int u) {\n"
" if ((unsigned int)u > 4294967295ULL) {}\n"
"}\n", settingsUnix64);
ASSERT_EQUALS("[test.cpp:2:27]: (style) Comparing expression of type 'unsigned int' against value 4294967295. Condition is always false. [compareValueOutOfTypeRangeError]\n",
errout_str());

check("void f(unsigned short s) {\n"
" if ((unsigned int)s > 4294967295ULL) {}\n"
"}\n", settingsUnix64);
ASSERT_EQUALS("[test.cpp:2:27]: (style) Comparing expression of type 'unsigned int' against value 4294967295. Condition is always false. [compareValueOutOfTypeRangeError]\n",
errout_str());

// wchar_t range is derived through ValueType::getSizeOf()
check("void f(wchar_t c) {\n"
" if ((wchar_t)c > 0x7fffffff) {}\n"
"}\n", settingsUnix64);
ASSERT_EQUALS("[test.cpp:2:20]: (style) Condition '(wchar_t)c>0x7fffffff' is always false [knownConditionTrueFalse]\n",
errout_str());

check("void f(unsigned int x) {\n"
" if (-(signed char)x < -129) {}\n"
"}\n", settingsUnix64);
ASSERT_EQUALS("[test.cpp:2:25]: (style) Condition '-(char)x<-129' is always false [knownConditionTrueFalse]\n",
errout_str());

check("void f(int x) {\n"
" if ((x) > 0) {}\n"
"}\n", settingsUnix64);
ASSERT_EQUALS("", errout_str());

check("void f(unsigned int x) {\n"
" if ((unsigned int)x > 0) {}\n"
"}\n", settingsUnix64);
ASSERT_EQUALS("", errout_str());

check("void f() {\n"
" long long ll = 1024 * 1024 * 1024;\n"
" if (ll * 8 < INT_MAX) {}\n"
Expand Down
43 changes: 43 additions & 0 deletions test/testvalueflow.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9604,6 +9604,49 @@ class TestValueFlow : public TestFixture {
"}\n";
ASSERT_EQUALS(true, testValueOfXImpossible(code, 3U, "a", -1));
ASSERT_EQUALS(true, testValueOfXImpossible(code, 3U, -1));

const Settings settingsUnix64 = settingsBuilder().platform(Platform::Type::Unix64).build();
code = "void f(unsigned long long x) {\n"
" return (unsigned int)x;\n"
"}\n";
SimpleTokenizer tokenizer(settingsUnix64, *this);
ASSERT(tokenizer.tokenize(code));
const Token* returnTok = Token::findmatch(tokenizer.tokens(), "return (");
ASSERT(returnTok && returnTok->next());
const std::list<ValueFlow::Value>& castValues = returnTok->next()->values();
ASSERT(std::any_of(castValues.cbegin(), castValues.cend(), [](const ValueFlow::Value& value) {
return value.isImpossible() && value.intvalue == -1;
}));
ASSERT(std::any_of(castValues.cbegin(), castValues.cend(), [](const ValueFlow::Value& value) {
return value.isImpossible() && value.intvalue == 4294967296;
}));

// PR review: impossible bounds on a cast must not be propagated through
// arithmetic with unsigned result type, since wrap-around invalidates the bound
code = "void f(int x) {\n"
" return (unsigned int)x - 1u;\n"
"}\n";
SimpleTokenizer tokenizer2(settingsUnix64, *this);
ASSERT(tokenizer2.tokenize(code));
const Token* minusTok = Token::findsimplematch(tokenizer2.tokens(), "-");
ASSERT(minusTok);
for (const ValueFlow::Value& value : minusTok->values()) {
ASSERT(!(value.isImpossible() && value.isIntValue() &&
value.bound != ValueFlow::Value::Bound::Point && value.intvalue == 4294967295));
}

// Known values are still propagated through unsigned arithmetic
code = "void f(int x) {\n"
" unsigned int y = 5u;\n"
" return y - 1u;\n"
"}\n";
SimpleTokenizer tokenizer3(settingsUnix64, *this);
ASSERT(tokenizer3.tokenize(code));
const Token* minusTok3 = Token::findsimplematch(tokenizer3.tokens(), "-");
ASSERT(minusTok3);
ASSERT(std::any_of(minusTok3->values().cbegin(), minusTok3->values().cend(), [](const ValueFlow::Value& value) {
return value.isKnown() && value.isIntValue() && value.intvalue == 4;
}));
}

void valueFlowImpossibleIncDec()
Expand Down
Loading