Signed integer optimisations; Also warn about 'x+y<x'

Author: danmar

lib/checktype.cpp

@@ -228,37 +228,58 @@ void CheckType::integerOverflowError(const Token *tok, const ValueFlow::Value &v
 
 void CheckType::checkIntegerOverflowOptimisations()
 {
-    // Various patterns are defined here:
+    // Interesting blogs:
     // https://kristerw.blogspot.com/2016/02/how-undefined-signed-overflow-enables.html
+    // https://research.checkpoint.com/2020/optout-compiler-undefined-behavior-optimizations/
 
-    // These optimisations seem to generate "unwanted" output
     // x + c < x       ->   false
     // x + c <= x      ->   false
     // x + c > x       ->   true
     // x + c >= x      ->   true
 
+    // x + y < x       ->   y < 0
+
+
     if (!mSettings->isEnabled(Settings::PORTABILITY))
         return;
 
     for (const Token *tok = mTokenizer->tokens(); tok; tok = tok->next()) {
         if (!Token::Match(tok, "<|<=|>=|>") || !tok->isBinaryOp())
             continue;
 
-        if (Token::Match(tok->astOperand1(), "[+-]") &&
-            tok->astOperand1()->valueType() &&
-            tok->astOperand1()->valueType()->isIntegral() &&
-            tok->astOperand1()->valueType()->sign == ValueType::Sign::SIGNED &&
-            tok->astOperand1()->isBinaryOp() &&
-            tok->astOperand1()->astOperand2()->isNumber() &&
-            Token::Match(tok->astOperand1()->astOperand1(), "%var%") &&
-            tok->astOperand1()->astOperand1()->str() == tok->astOperand2()->str()) {
+        const std::string cmp = tok->str();
+        const Token * const lhs = tok->astOperand1();
+        if (!Token::Match(lhs, "[+-]") || !lhs->isBinaryOp() || !lhs->valueType() || !lhs->valueType()->isIntegral() || lhs->valueType()->sign != ValueType::Sign::SIGNED)
+            continue;
+
+        const Token *expr = lhs->astOperand1();
+        const Token *other = lhs->astOperand2();
+        if (expr->varId() != lhs->astSibling()->varId())
+            continue;
+
+        // x [+-] c cmp x
+        if (other->isNumber() && other->getKnownIntValue() > 0) {
             bool result;
             if (tok->astOperand1()->str() == "+")
-                result = Token::Match(tok, ">|>=");
+                result = (cmp == ">" || cmp == ">=");
             else
-                result = Token::Match(tok, "<|<=");
+                result = (cmp == "<" || cmp == "<=");
             integerOverflowOptimisationError(tok, result ? "true" : "false");
         }
+
+        // x + y cmp x
+        if (lhs->str() == "+" && other->varId() > 0) {
+            const std::string result = other->str() + cmp + "0";
+            integerOverflowOptimisationError(tok, result);
+        }
+
+        // x - y cmp x
+        if (lhs->str() == "-" && other->varId() > 0) {
+            std::string cmp2 = cmp;
+            cmp2[0] = (cmp[0] == '<') ? '>' : '<';
+            const std::string result = other->str() + cmp2 + "0";
+            integerOverflowOptimisationError(tok, result);
+        }
     }
 }
 

test/testtype.cpp

@@ -253,6 +253,8 @@ class TestType : public TestFixture {
         Settings settings;
         settings.addEnabled("warning");
 
+        // x + c < x
+
         check("int f(int x) { return x + 10 > x; }", &settings);
         ASSERT_EQUALS("[test.cpp:1]: (portability) There is a danger that 'x+10>x' will be optimised into 'true'. Signed integer overflow is undefined behavior.\n", errout.str());
 
@@ -276,6 +278,32 @@ class TestType : public TestFixture {
 
         check("int f(int x) { return x - 10 <= x; }", &settings);
         ASSERT_EQUALS("[test.cpp:1]: (portability) There is a danger that 'x-10<=x' will be optimised into 'true'. Signed integer overflow is undefined behavior.\n", errout.str());
+
+        // x + y < x
+        check("int f(int x, int y) { return x + y < x; }", &settings);
+        ASSERT_EQUALS("[test.cpp:1]: (portability) There is a danger that 'x+y<x' will be optimised into 'y<0'. Signed integer overflow is undefined behavior.\n", errout.str());
+
+        check("int f(int x, int y) { return x + y <= x; }", &settings);
+        ASSERT_EQUALS("[test.cpp:1]: (portability) There is a danger that 'x+y<=x' will be optimised into 'y<=0'. Signed integer overflow is undefined behavior.\n", errout.str());
+
+        check("int f(int x, int y) { return x + y > x; }", &settings);
+        ASSERT_EQUALS("[test.cpp:1]: (portability) There is a danger that 'x+y>x' will be optimised into 'y>0'. Signed integer overflow is undefined behavior.\n", errout.str());
+
+        check("int f(int x, int y) { return x + y >= x; }", &settings);
+        ASSERT_EQUALS("[test.cpp:1]: (portability) There is a danger that 'x+y>=x' will be optimised into 'y>=0'. Signed integer overflow is undefined behavior.\n", errout.str());
+
+        // x - y < x
+        check("int f(int x, int y) { return x - y < x; }", &settings);
+        ASSERT_EQUALS("[test.cpp:1]: (portability) There is a danger that 'x-y<x' will be optimised into 'y>0'. Signed integer overflow is undefined behavior.\n", errout.str());
+
+        check("int f(int x, int y) { return x - y <= x; }", &settings);
+        ASSERT_EQUALS("[test.cpp:1]: (portability) There is a danger that 'x-y<=x' will be optimised into 'y>=0'. Signed integer overflow is undefined behavior.\n", errout.str());
+
+        check("int f(int x, int y) { return x - y > x; }", &settings);
+        ASSERT_EQUALS("[test.cpp:1]: (portability) There is a danger that 'x-y>x' will be optimised into 'y<0'. Signed integer overflow is undefined behavior.\n", errout.str());
+
+        check("int f(int x, int y) { return x - y >= x; }", &settings);
+        ASSERT_EQUALS("[test.cpp:1]: (portability) There is a danger that 'x-y>=x' will be optimised into 'y<=0'. Signed integer overflow is undefined behavior.\n", errout.str());
     }
 
     void signConversion() {