Fix 10404: FP knownConditionTrueFalse after subtraction (#3390)

lib/analyzer.h

@@ -19,7 +19,9 @@
 #ifndef analyzerH
 #define analyzerH
 
+#include "config.h"
 #include <string>
+#include <type_traits>
 #include <vector>
 
 class Token;
@@ -32,7 +34,11 @@ struct Analyzer {
         Action() : mFlag(0) {}
 
         // cppcheck-suppress noExplicitConstructor
-        Action(unsigned int f) : mFlag(f) {}
+        template<class T,
+                 REQUIRES("T must be convertible to unsigned int", std::is_convertible<T, unsigned int> ),
+                 REQUIRES("T must not be a bool", !std::is_same<T, bool> )>
+        Action(T f) : mFlag(f)
+        {}
 
         enum {
             None = 0,

lib/forwardanalyzer.cpp

@@ -7,6 +7,7 @@
 #include "valueptr.h"
 
 #include <algorithm>
+#include <cstdio>
 #include <functional>
 #include <tuple>
 #include <utility>

lib/valueflow.cpp

@@ -80,6 +80,7 @@
 #include "analyzer.h"
 #include "astutils.h"
 #include "checkuninitvar.h"
+#include "config.h"
 #include "errorlogger.h"
 #include "errortypes.h"
 #include "forwardanalyzer.h"
@@ -112,6 +113,7 @@
 #include <stdexcept>
 #include <string>
 #include <tuple>
+#include <type_traits>
 #include <vector>
 
 static void bailoutInternal(const std::string& type, TokenList *tokenlist, ErrorLogger *errorLogger, const Token *tok, const std::string &what, const std::string &file, int line, std::string function)
@@ -380,7 +382,7 @@ static bool isZero(T x)
 }
 
 template<class R, class T>
-static R calculate(const std::string& s, const T& x, const T& y)
+static R calculate(const std::string& s, const T& x, const T& y, bool* error = nullptr)
 {
     auto wrap = [](T z) {
         return R{z};
@@ -393,11 +395,19 @@ static R calculate(const std::string& s, const T& x, const T& y)
     case '*':
         return wrap(x * y);
     case '/':
-        return isZero(y) ? R{} :
+        if (isZero(y)) {
-               wrap(x / y);
+            if (error)
+                *error = true;
+            return R{};
+        }
+        return wrap(x / y);
     case '%':
-        return isZero(y) ? R{} :
+        if (isZero(y)) {
-               wrap(MathLib::bigint(x) % MathLib::bigint(y));
+            if (error)
+                *error = true;
+            return R{};
+        }
+        return wrap(MathLib::bigint(x) % MathLib::bigint(y));
     case '&':
         return wrap(MathLib::bigint(x) & MathLib::bigint(y));
     case '|':
@@ -409,11 +419,19 @@ static R calculate(const std::string& s, const T& x, const T& y)
     case '<':
         return wrap(x < y);
     case '<<':
-        return (y >= sizeof(MathLib::bigint) * 8) ? R{} :
+        if (y >= sizeof(MathLib::bigint) * 8) {
-               wrap(MathLib::bigint(x) << MathLib::bigint(y));
+            if (error)
+                *error = true;
+            return R{};
+        }
+        return wrap(MathLib::bigint(x) << MathLib::bigint(y));
     case '>>':
-        return (y >= sizeof(MathLib::bigint) * 8) ? R{} :
+        if (y >= sizeof(MathLib::bigint) * 8) {
-               wrap(MathLib::bigint(x) >> MathLib::bigint(y));
+            if (error)
+                *error = true;
+            return R{};
+        }
+        return wrap(MathLib::bigint(x) >> MathLib::bigint(y));
     case '&&':
         return wrap(!isZero(x) && !isZero(y));
     case '||':
@@ -431,9 +449,9 @@ static R calculate(const std::string& s, const T& x, const T& y)
 }
 
 template<class T>
-static T calculate(const std::string& s, const T& x, const T& y)
+static T calculate(const std::string& s, const T& x, const T& y, bool* error = nullptr)
 {
-    return calculate<T, T>(s, x, y);
+    return calculate<T, T>(s, x, y, error);
 }
 
 /** Set token value for cast */
@@ -1711,9 +1729,14 @@ static bool isConditionKnown(const Token* tok, bool then)
 
 static const std::string& invertAssign(const std::string& assign)
 {
-    static std::unordered_map<std::string, std::string> lookup = {
+    static std::unordered_map<std::string, std::string> lookup = {{"=", "="},
-        {"+=", "-="}, {"-=", "+="}, {"*=", "/="}, {"/=", "*="}, {"<<=", ">>="}, {">>=", "<<="}, {"^=", "^="}
+        {"+=", "-="},
-    };
+        {"-=", "+="},
+        {"*=", "/="},
+        {"/=", "*="},
+        {"<<=", ">>="},
+        {">>=", "<<="},
+        {"^=", "^="}};
     static std::string empty;
     auto it = lookup.find(assign);
     if (it == lookup.end())
@@ -1722,58 +1745,50 @@ static const std::string& invertAssign(const std::string& assign)
         return it->second;
 }
 
-static bool evalAssignment(ValueFlow::Value &lhsValue, const std::string &assign, const ValueFlow::Value &rhsValue)
+static std::string removeAssign(const std::string& assign) {
+    return std::string{assign.begin(), assign.end() - 1};
+}
+
+template<class T, class U>
+static T calculateAssign(const std::string& assign, const T& x, const U& y, bool* error = nullptr)
 {
-    if (lhsValue.isIntValue()) {
+    if (assign.empty() || assign.back() != '=') {
-        if (assign == "=")
+        if (error)
-            lhsValue.intvalue = rhsValue.intvalue;
+            *error = true;
-        else if (assign == "+=")
+        return T{};
-            lhsValue.intvalue += rhsValue.intvalue;
+    }
-        else if (assign == "-=")
+    if (assign == "=")
-            lhsValue.intvalue -= rhsValue.intvalue;
+        return y;
-        else if (assign == "*=")
+    return calculate<T, T>(removeAssign(assign), x, y, error);
-            lhsValue.intvalue *= rhsValue.intvalue;
+}
-        else if (assign == "/=") {
+
-            if (rhsValue.intvalue == 0)
+template<class T, class U>
-                return false;
+static void assignValueIfMutable(T& x, const U& y)
-            lhsValue.intvalue /= rhsValue.intvalue;
+{
-        } else if (assign == "%=") {
+    x = y;
-            if (rhsValue.intvalue == 0)
+}
-                return false;
+
-            lhsValue.intvalue %= rhsValue.intvalue;
+template<class T, class U>
-        } else if (assign == "&=")
+static void assignValueIfMutable(const T&, const U&)
-            lhsValue.intvalue &= rhsValue.intvalue;
+{}
-        else if (assign == "|=")
+
-            lhsValue.intvalue |= rhsValue.intvalue;
+template<class Value, REQUIRES("Value must ValueFlow::Value", std::is_convertible<Value&, const ValueFlow::Value&> )>
-        else if (assign == "^=")
+static bool evalAssignment(Value& lhsValue, const std::string& assign, const ValueFlow::Value& rhsValue)
-            lhsValue.intvalue ^= rhsValue.intvalue;
+{
-        else if (assign == "<<=") {
+    bool error = false;
-            if (rhsValue.intvalue < 0)
+    if (lhsValue.isSymbolicValue() && rhsValue.isIntValue()) {
-                return false;
+        if (assign != "+=" && assign != "-=")
-            lhsValue.intvalue <<= rhsValue.intvalue;
-        } else if (assign == ">>=") {
-            if (rhsValue.intvalue < 0)
-                return false;
-            lhsValue.intvalue >>= rhsValue.intvalue;
-        } else
-            return false;
-    } else if (lhsValue.isFloatValue()) {
-        if (assign == "=")
-            lhsValue.intvalue = rhsValue.intvalue;
-        else if (assign == "+=")
-            lhsValue.floatValue += rhsValue.intvalue;
-        else if (assign == "-=")
-            lhsValue.floatValue -= rhsValue.intvalue;
-        else if (assign == "*=")
-            lhsValue.floatValue *= rhsValue.intvalue;
-        else if (assign == "/=")
-            lhsValue.floatValue /= rhsValue.intvalue;
-        else
             return false;
+        assignValueIfMutable(lhsValue.intvalue, calculateAssign(assign, lhsValue.intvalue, rhsValue.intvalue, &error));
+    } else if (lhsValue.isIntValue() && rhsValue.isIntValue()) {
+        assignValueIfMutable(lhsValue.intvalue, calculateAssign(assign, lhsValue.intvalue, rhsValue.intvalue, &error));
+    } else if (lhsValue.isFloatValue() && rhsValue.isIntValue()) {
+        assignValueIfMutable(lhsValue.floatValue,
+                             calculateAssign(assign, lhsValue.floatValue, rhsValue.intvalue, &error));
     } else {
         return false;
     }
-    return true;
+    return !error;
 }
 
 template<class T>
@@ -1876,18 +1891,6 @@ static bool isAliasOf(const Variable * var, const Token *tok, nonneg int varid,
     return false;
 }
 
-static const ValueFlow::Value* getKnownValue(const Token* tok, ValueFlow::Value::ValueType type)
-{
-    if (!tok)
-        return nullptr;
-    auto it = std::find_if(tok->values().begin(), tok->values().end(), [&](const ValueFlow::Value& v) {
-        return v.isKnown() && v.valueType == type;
-    });
-    if (it != tok->values().end())
-        return &*it;
-    return nullptr;
-}
-
 static bool bifurcate(const Token* tok, const std::set<nonneg int>& varids, const Settings* settings, int depth = 20);
 
 static bool bifurcateVariableChanged(const Variable* var,
@@ -2031,6 +2034,14 @@ struct ValueFlowAnalyzer : Analyzer {
         return Action::None;
     }
 
+    static const std::string& getAssign(const Token* tok, Direction d)
+    {
+        if (d == Direction::Forward)
+            return tok->str();
+        else
+            return invertAssign(tok->str());
+    }
+
     virtual Action isWritable(const Token* tok, Direction d) const {
         const ValueFlow::Value* value = getValue(tok);
         if (!value)
@@ -2041,13 +2052,10 @@ struct ValueFlowAnalyzer : Analyzer {
 
         if (parent && parent->isAssignmentOp() && astIsLHS(tok) &&
             parent->astOperand2()->hasKnownValue()) {
-            // If the operator is invertible
-            if (d == Direction::Reverse && (parent->str() == "&=" || parent->str() == "|=" || parent->str() == "%="))
-                return Action::None;
             const Token* rhs = parent->astOperand2();
-            const ValueFlow::Value* rhsValue = getKnownValue(rhs, ValueFlow::Value::ValueType::INT);
+            const ValueFlow::Value* rhsValue = rhs->getKnownValue(ValueFlow::Value::ValueType::INT);
             Action a;
-            if (!rhsValue)
+            if (!rhsValue || !evalAssignment(*value, getAssign(parent, d), *rhsValue))
                 a = Action::Invalid;
             else
                 a = Action::Write;
@@ -2068,29 +2076,23 @@ struct ValueFlowAnalyzer : Analyzer {
         return Action::None;
     }
 
-    static const std::string& getAssign(const Token* tok, Direction d) {
-        if (d == Direction::Forward)
-            return tok->str();
-        else
-            return invertAssign(tok->str());
-    }
-
     virtual void writeValue(ValueFlow::Value* value, const Token* tok, Direction d) const {
         if (!value)
             return;
         if (!tok->astParent())
             return;
         if (tok->astParent()->isAssignmentOp()) {
-            // TODO: Check result
+            const ValueFlow::Value* rhsValue =
-            if (evalAssignment(*value,
+                tok->astParent()->astOperand2()->getKnownValue(ValueFlow::Value::ValueType::INT);
-                               getAssign(tok->astParent(), d),
+            assert(rhsValue);
-                               *getKnownValue(tok->astParent()->astOperand2(), ValueFlow::Value::ValueType::INT))) {
+            if (evalAssignment(*value, getAssign(tok->astParent(), d), *rhsValue)) {
                 const std::string info("Compound assignment '" + tok->astParent()->str() + "', assigned value is " +
                                        value->infoString());
                 if (tok->astParent()->str() == "=")
                     value->errorPath.clear();
                 value->errorPath.emplace_back(tok, info);
             } else {
+                assert(false && "Writable value cannot be evaluated");
                 // TODO: Don't set to zero
                 value->intvalue = 0;
             }

test/testcondition.cpp

@@ -3782,6 +3782,17 @@ private:
               "  if (e) {}\n"
               "}\n");
         ASSERT_EQUALS("", errout.str());
+
+        check("int g(int i) {\n"
+              "  if (i < 256)\n"
+              "    return 1;\n"
+              "  const int N = 2 * i;\n"
+              "  i -= 256;\n"
+              "  if (i == 0)\n"
+              "    return 0;\n"
+              "  return N;\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
     }
 
     void alwaysTrueInfer() {