Fix 10298: ValueFlow: Wrong known value, 'x == -1' implicit unsigned cast for rhs (#3277)

lib/checktype.cpp

@@ -241,7 +241,10 @@ void CheckType::checkSignConversion()
         for (const Token * tok1 : astOperands) {
             if (!tok1)
                 continue;
-            const ValueFlow::Value *negativeValue = tok1->getValueLE(-1,mSettings);
+            const ValueFlow::Value* negativeValue =
+                ValueFlow::findValue(tok1->values(), mSettings, [&](const ValueFlow::Value& v) {
+                    return !v.isImpossible() && v.isIntValue() && (v.intvalue <= -1 || v.wideintvalue <= -1);
+                });
             if (!negativeValue)
                 continue;
             if (tok1->valueType() && tok1->valueType()->sign != ValueType::Sign::UNSIGNED)

lib/token.cpp

@@ -1798,50 +1798,18 @@ const ValueFlow::Value * Token::getValueLE(const MathLib::bigint val, const Sett
 {
     if (!mImpl->mValues)
         return nullptr;
-    const ValueFlow::Value *ret = nullptr;
+    return ValueFlow::findValue(*mImpl->mValues, settings, [&](const ValueFlow::Value& v) {
-    std::list<ValueFlow::Value>::const_iterator it;
+        return !v.isImpossible() && v.isIntValue() && v.intvalue <= val;
-    for (it = mImpl->mValues->begin(); it != mImpl->mValues->end(); ++it) {
+    });
-        if (it->isImpossible())
-            continue;
-        if (it->isIntValue() && it->intvalue <= val) {
-            if (!ret || ret->isInconclusive() || (ret->condition && !it->isInconclusive()))
-                ret = &(*it);
-            if (!ret->isInconclusive() && !ret->condition)
-                break;
-        }
-    }
-    if (settings && ret) {
-        if (ret->isInconclusive() && !settings->certainty.isEnabled(Certainty::inconclusive))
-            return nullptr;
-        if (ret->condition && !settings->severity.isEnabled(Severity::warning))
-            return nullptr;
-    }
-    return ret;
 }
 
 const ValueFlow::Value * Token::getValueGE(const MathLib::bigint val, const Settings *settings) const
 {
     if (!mImpl->mValues)
         return nullptr;
-    const ValueFlow::Value *ret = nullptr;
+    return ValueFlow::findValue(*mImpl->mValues, settings, [&](const ValueFlow::Value& v) {
-    std::list<ValueFlow::Value>::const_iterator it;
+        return !v.isImpossible() && v.isIntValue() && v.intvalue >= val;
-    for (it = mImpl->mValues->begin(); it != mImpl->mValues->end(); ++it) {
+    });
-        if (it->isImpossible())
-            continue;
-        if (it->isIntValue() && it->intvalue >= val) {
-            if (!ret || ret->isInconclusive() || (ret->condition && !it->isInconclusive()))
-                ret = &(*it);
-            if (!ret->isInconclusive() && !ret->condition)
-                break;
-        }
-    }
-    if (settings && ret) {
-        if (ret->isInconclusive() && !settings->certainty.isEnabled(Certainty::inconclusive))
-            return nullptr;
-        if (ret->condition && !settings->severity.isEnabled(Severity::warning))
-            return nullptr;
-    }
-    return ret;
 }
 
 const ValueFlow::Value * Token::getInvalidValue(const Token *ftok, nonneg int argnr, const Settings *settings) const

lib/valueflow.cpp

@@ -444,13 +444,48 @@ static bool isCompatibleValues(const ValueFlow::Value& value1, const ValueFlow::
     return false;
 }
 
+static ValueFlow::Value truncateImplicitConversion(Token* parent, const ValueFlow::Value& value, const Settings* settings)
+{
+    if (!value.isIntValue() && !value.isFloatValue())
+        return value;
+    if (!parent)
+        return value;
+    if (!parent->isBinaryOp())
+        return value;
+    if (!parent->isConstOp())
+        return value;
+    if (!astIsIntegral(parent->astOperand1(), false))
+        return value;
+    if (!astIsIntegral(parent->astOperand2(), false))
+        return value;
+    const ValueType* vt1 = parent->astOperand1()->valueType();
+    const ValueType* vt2 = parent->astOperand2()->valueType();
+    // If the sign is the same there is no truncation
+    if (vt1->sign == vt2->sign)
+        return value;
+    size_t n1 = ValueFlow::getSizeOf(*vt1, settings);
+    size_t n2 = ValueFlow::getSizeOf(*vt2, settings);
+    ValueType::Sign sign = ValueType::Sign::UNSIGNED;
+    if (n1 < n2)
+        sign = vt2->sign;
+    else if (n1 > n2)
+        sign = vt1->sign;
+    ValueFlow::Value v = castValue(value, sign, std::max(n1, n2) * 8);
+    v.wideintvalue = value.intvalue;
+    return v;
+}
+
 /** set ValueFlow value and perform calculations if possible */
-static void setTokenValue(Token* tok, const ValueFlow::Value &value, const Settings *settings)
+static void setTokenValue(Token* tok, ValueFlow::Value value, const Settings* settings)
 {
     // Skip setting values that are too big since its ambiguous
     if (!value.isImpossible() && value.isIntValue() && value.intvalue < 0 && astIsUnsigned(tok) &&
         ValueFlow::getSizeOf(*tok->valueType(), settings) >= sizeof(MathLib::bigint))
         return;
+
+    if (!value.isImpossible() && value.isIntValue())
+        value = truncateImplicitConversion(tok->astParent(), value, settings);
+
     if (!tok->addValue(value))
         return;
 
@@ -6793,6 +6828,7 @@ ValueFlow::Value::Value(const Token* c, long long val)
       defaultArg(false),
       indirect(0),
       path(0),
+      wideintvalue(0),
       lifetimeKind(LifetimeKind::Object),
       lifetimeScope(LifetimeScope::Local),
       valueKind(ValueKind::Possible)
@@ -6934,3 +6970,25 @@ std::string ValueFlow::eitherTheConditionIsRedundant(const Token *condition)
     }
     return "Either the condition '" + condition->expressionString() + "' is redundant";
 }
+
+const ValueFlow::Value* ValueFlow::findValue(const std::list<ValueFlow::Value>& values,
+                                             const Settings* settings,
+                                             std::function<bool(const ValueFlow::Value&)> pred)
+{
+    const ValueFlow::Value* ret = nullptr;
+    for (const ValueFlow::Value& v : values) {
+        if (pred(v)) {
+            if (!ret || ret->isInconclusive() || (ret->condition && !v.isInconclusive()))
+                ret = &v;
+            if (!ret->isInconclusive() && !ret->condition)
+                break;
+        }
+    }
+    if (settings && ret) {
+        if (ret->isInconclusive() && !settings->certainty.isEnabled(Certainty::inconclusive))
+            return nullptr;
+        if (ret->condition && !settings->severity.isEnabled(Severity::warning))
+            return nullptr;
+    }
+    return ret;
+}

lib/valueflow.h

@@ -95,6 +95,7 @@ namespace ValueFlow {
               defaultArg(false),
               indirect(0),
               path(0),
+              wideintvalue(val),
               lifetimeKind(LifetimeKind::Object),
               lifetimeScope(LifetimeScope::Local),
               valueKind(ValueKind::Possible)
@@ -319,6 +320,9 @@ namespace ValueFlow {
         /** Path id */
         MathLib::bigint path;
 
+        /** int value before implicit truncation */
+        long long wideintvalue;
+
         enum class LifetimeKind {Object, SubObject, Lambda, Iterator, Address} lifetimeKind;
 
         enum class LifetimeScope { Local, Argument, SubFunction } lifetimeScope;
@@ -389,6 +393,10 @@ namespace ValueFlow {
     std::string eitherTheConditionIsRedundant(const Token *condition);
 
     size_t getSizeOf(const ValueType &vt, const Settings *settings);
+
+    const ValueFlow::Value* findValue(const std::list<ValueFlow::Value>& values,
+                                      const Settings* settings,
+                                      std::function<bool(const ValueFlow::Value&)> pred);
 }
 
 struct LifetimeToken {

test/testcondition.cpp

@@ -3626,6 +3626,12 @@ private:
               "    if ((100 - x) > 0) {}\n"
               "}\n");
         ASSERT_EQUALS("", errout.str());
+
+        // #10298
+        check("void foo(unsigned int x) {\n"
+              "    if (x == -1) {}\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
     }
 
     void alwaysTrueInfer() {

test/testtype.cpp

@@ -258,6 +258,8 @@ private:
     }
 
     void signConversion() {
+        Settings settings;
+        settings.platform(Settings::Unix64);
         check("x = -4 * (unsigned)y;");
         ASSERT_EQUALS("[test.cpp:1]: (warning) Expression '-4' has a negative value. That is converted to an unsigned value and used in an unsigned calculation.\n", errout.str());
 
@@ -267,7 +269,7 @@ private:
         check("unsigned int dostuff(int x) {\n" // x is signed
               "  if (x==0) {}\n"
               "  return (x-1)*sizeof(int);\n"
-              "}");
+              "}", &settings);
         ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (warning) Expression 'x-1' can have a negative value. That is converted to an unsigned value and used in an unsigned calculation.\n", errout.str());
 
         check("unsigned int f1(signed int x, unsigned int y) {" // x is signed