From 52491f1c5339a24eb50aa19cc5a6e7512882a65d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= <daniel.marjamaki@gmail.com>
Date: Sun, 3 Jan 2021 22:12:19 +0100
Subject: [PATCH] Signed integer optimisations; Also warn about 'x+y<x'

---
 lib/checktype.cpp | 45 +++++++++++++++++++++++++++++++++------------
 test/testtype.cpp | 28 ++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+), 12 deletions(-)

diff --git a/lib/checktype.cpp b/lib/checktype.cpp
index ef17e3593..3d8b12aec 100644
--- a/lib/checktype.cpp
+++ b/lib/checktype.cpp
@@ -228,15 +228,18 @@ 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;
 
@@ -244,21 +247,39 @@ void CheckType::checkIntegerOverflowOptimisations()
         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);
+        }
     }
 }
 
diff --git a/test/testtype.cpp b/test/testtype.cpp
index 87e8e6f12..790dd119b 100644
--- a/test/testtype.cpp
+++ b/test/testtype.cpp
@@ -253,6 +253,8 @@ private:
         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 @@ private:
 
         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() {