From 6bc0df2908e99bdf0c4a98bec81a2b55c31a872f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= <daniel.marjamaki@gmail.com>
Date: Tue, 19 Sep 2017 09:21:20 +0200
Subject: [PATCH] checkTooBigBitwiseShift: Separate id for signed shift
 overflow

---
 lib/checktype.cpp | 33 ++++++++++++++++++++++++++++-----
 lib/checktype.h   |  2 ++
 test/testtype.cpp |  2 +-
 3 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/lib/checktype.cpp b/lib/checktype.cpp
index 76d8c8a9e..1191f5c3f 100644
--- a/lib/checktype.cpp
+++ b/lib/checktype.cpp
@@ -82,20 +82,24 @@ void CheckType::checkTooBigBitwiseShift()
         else
             continue;
 
-        if (lhstype->sign == ValueType::Sign::SIGNED)
-            --lhsbits;
-
         // Get biggest rhs value. preferably a value which doesn't have 'condition'.
         const ValueFlow::Value *value = tok->astOperand2()->getValueGE(lhsbits, _settings);
         if (value && _settings->isEnabled(value, false))
             tooBigBitwiseShiftError(tok, lhsbits, *value);
+        else if (lhstype->sign == ValueType::Sign::SIGNED) {
+            value = tok->astOperand2()->getValueGE(lhsbits-1, _settings);
+            if (value && _settings->isEnabled(value, false))
+                tooBigSignedBitwiseShiftError(tok, lhsbits, *value);
+        }
     }
 }
 
 void CheckType::tooBigBitwiseShiftError(const Token *tok, int lhsbits, const ValueFlow::Value &rhsbits)
 {
+    const char id[] = "shiftTooManyBits";
+
     if (!tok) {
-        reportError(tok, Severity::error, "shiftTooManyBits", "Shifting 32-bit value by 40 bits is undefined behaviour", CWE758, false);
+        reportError(tok, Severity::error, id, "Shifting 32-bit value by 40 bits is undefined behaviour", CWE758, false);
         return;
     }
 
@@ -106,7 +110,26 @@ void CheckType::tooBigBitwiseShiftError(const Token *tok, int lhsbits, const Val
     if (rhsbits.condition)
         errmsg << ". See condition at line " << rhsbits.condition->linenr() << ".";
 
-    reportError(errorPath, rhsbits.errorSeverity() ? Severity::error : Severity::warning, "shiftTooManyBits", errmsg.str(), CWE758, rhsbits.inconclusive);
+    reportError(errorPath, rhsbits.errorSeverity() ? Severity::error : Severity::warning, id, errmsg.str(), CWE758, rhsbits.inconclusive);
+}
+
+void CheckType::tooBigSignedBitwiseShiftError(const Token *tok, int lhsbits, const ValueFlow::Value &rhsbits)
+{
+    const char id[] = "shiftTooManyBitsSigned";
+
+    if (!tok) {
+        reportError(tok, Severity::error, id, "Shifting signed 32-bit value by 31 bits is undefined behaviour", CWE758, false);
+        return;
+    }
+
+    const ErrorPath errorPath = getErrorPath(tok, &rhsbits, "Shift");
+
+    std::ostringstream errmsg;
+    errmsg << "Shifting signed " << lhsbits << "-bit value by " << rhsbits.intvalue << " bits is undefined behaviour";
+    if (rhsbits.condition)
+        errmsg << ". See condition at line " << rhsbits.condition->linenr() << ".";
+
+    reportError(errorPath, rhsbits.errorSeverity() ? Severity::error : Severity::warning, id, errmsg.str(), CWE758, rhsbits.inconclusive);
 }
 
 //---------------------------------------------------------------------------
diff --git a/lib/checktype.h b/lib/checktype.h
index ba13e80e5..a39e32b1f 100644
--- a/lib/checktype.h
+++ b/lib/checktype.h
@@ -84,6 +84,7 @@ private:
 
     // Error messages..
     void tooBigBitwiseShiftError(const Token *tok, int lhsbits, const ValueFlow::Value &rhsbits);
+    void tooBigSignedBitwiseShiftError(const Token *tok, int lhsbits, const ValueFlow::Value &rhsbits);
     void integerOverflowError(const Token *tok, const ValueFlow::Value &value);
     void signConversionError(const Token *tok, const bool constvalue);
     void longCastAssignError(const Token *tok);
@@ -93,6 +94,7 @@ private:
     void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const {
         CheckType c(nullptr, settings, errorLogger);
         c.tooBigBitwiseShiftError(nullptr, 32, ValueFlow::Value(64));
+        c.tooBigSignedBitwiseShiftError(nullptr, 31, ValueFlow::Value(31));
         c.integerOverflowError(nullptr, ValueFlow::Value(1LL<<32));
         c.signConversionError(nullptr, false);
         c.longCastAssignError(nullptr);
diff --git a/test/testtype.cpp b/test/testtype.cpp
index 7011f4f9a..cebbb7941 100644
--- a/test/testtype.cpp
+++ b/test/testtype.cpp
@@ -71,7 +71,7 @@ private:
         ASSERT_EQUALS("[test.cpp:2]: (error) Shifting 32-bit value by 32 bits is undefined behaviour\n", errout.str());
 
         check("x = (short)x << 31;",&settings);
-        ASSERT_EQUALS("[test.cpp:1]: (error) Shifting 31-bit value by 31 bits is undefined behaviour\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:1]: (error) Shifting signed 32-bit value by 31 bits is undefined behaviour\n", errout.str());
 
         check("int foo(int x) {\n"
               "   return x << 2;\n"