[clang] bd9e239 - [analyzer] Expand conversion check to check more expressions for overflow and underflow

[Gabor Marton via cfe-commits]

Author: Gabor Marton
Date: 2021-12-15T11:41:34+01:00
New Revision: bd9e23943a2299804172da69b308533241f99b60

URL: https://github.com/llvm/llvm-project/commit/bd9e23943a2299804172da69b308533241f99b60
DIFF: https://github.com/llvm/llvm-project/commit/bd9e23943a2299804172da69b308533241f99b60.diff

LOG: [analyzer] Expand conversion check to check more expressions for overflow and underflow

This expands checking for more expressions. This will check underflow
and loss of precision when using call expressions like:

  void foo(unsigned);
  int i = -1;
  foo(i);

This also includes other expressions as well, so it can catch negative
indices to std::vector since it uses unsigned integers for [] and .at()
function.

Patch by: @pfultz2

Differential Revision: https://reviews.llvm.org/D46081

Added: 
    clang/test/Analysis/conversion.cpp

Modified: 
    clang/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
    clang/test/Analysis/conversion.c

Removed: 
    


################################################################################
diff  --git a/clang/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
index 8da482a2aec95..6ea39fb95e9a8 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
@@ -56,9 +56,8 @@ class ConversionChecker : public Checker<check::PreStmt<ImplicitCastExpr>> {
 
 void ConversionChecker::checkPreStmt(const ImplicitCastExpr *Cast,
                                      CheckerContext &C) const {
-  // TODO: For now we only warn about DeclRefExpr, to avoid noise. Warn for
-  // calculations also.
-  if (!isa<DeclRefExpr>(Cast->IgnoreParenImpCasts()))
+  // Don't warn for implicit conversions to bool
+  if (Cast->getType()->isBooleanType())
     return;
 
   // Don't warn for loss of sign/precision in macros.
@@ -70,6 +69,9 @@ void ConversionChecker::checkPreStmt(const ImplicitCastExpr *Cast,
   const Stmt *Parent = PM.getParent(Cast);
   if (!Parent)
     return;
+  // Dont warn if this is part of an explicit cast
+  if (isa<ExplicitCastExpr>(Parent))
+    return;
 
   bool LossOfSign = false;
   bool LossOfPrecision = false;
@@ -78,8 +80,10 @@ void ConversionChecker::checkPreStmt(const ImplicitCastExpr *Cast,
   if (const auto *B = dyn_cast<BinaryOperator>(Parent)) {
     BinaryOperator::Opcode Opc = B->getOpcode();
     if (Opc == BO_Assign) {
-      LossOfSign = isLossOfSign(Cast, C);
-      LossOfPrecision = isLossOfPrecision(Cast, Cast->getType(), C);
+      if (!Cast->IgnoreParenImpCasts()->isEvaluatable(C.getASTContext())) {
+        LossOfSign = isLossOfSign(Cast, C);
+        LossOfPrecision = isLossOfPrecision(Cast, Cast->getType(), C);
+      }
     } else if (Opc == BO_AddAssign || Opc == BO_SubAssign) {
       // No loss of sign.
       LossOfPrecision = isLossOfPrecision(Cast, B->getLHS()->getType(), C);
@@ -98,7 +102,12 @@ void ConversionChecker::checkPreStmt(const ImplicitCastExpr *Cast,
     } else if (B->isRelationalOp() || B->isMultiplicativeOp()) {
       LossOfSign = isLossOfSign(Cast, C);
     }
-  } else if (isa<DeclStmt>(Parent)) {
+  } else if (isa<DeclStmt, ReturnStmt>(Parent)) {
+    if (!Cast->IgnoreParenImpCasts()->isEvaluatable(C.getASTContext())) {
+      LossOfSign = isLossOfSign(Cast, C);
+      LossOfPrecision = isLossOfPrecision(Cast, Cast->getType(), C);
+    }
+  } else {
     LossOfSign = isLossOfSign(Cast, C);
     LossOfPrecision = isLossOfPrecision(Cast, Cast->getType(), C);
   }

diff  --git a/clang/test/Analysis/conversion.c b/clang/test/Analysis/conversion.c
index 84eccb7e2f506..f6b0c11a15aea 100644
--- a/clang/test/Analysis/conversion.c
+++ b/clang/test/Analysis/conversion.c
@@ -105,6 +105,23 @@ void division(unsigned U, signed S) {
     S = U / S; // expected-warning {{Loss of sign}}
 }
 
+void f(unsigned x) {}
+void g(unsigned x) {}
+
+void functioncall1() {
+  long x = -1;
+  int y = 0;
+  f(x); // expected-warning {{Loss of sign in implicit conversion}}
+  f(y);
+}
+
+void functioncall2(int x, int y) {
+  if (x < 0)
+    f(x); // expected-warning {{Loss of sign in implicit conversion}}
+  f(y);
+  f(x); // expected-warning {{Loss of sign in implicit conversion}}
+}
+
 void dontwarn1(unsigned U, signed S) {
   U8 = S; // It might be known that S is always 0x00-0xff.
   S8 = U; // It might be known that U is always 0x00-0xff.
@@ -132,15 +149,38 @@ void dontwarn4() {
   DOSTUFF;
 }
 
-// don't warn for calculations
-// seen some fp. For instance:  c2 = (c2 >= 'A' && c2 <= 'Z') ? c2 - 'A' + 'a' : c2;
-// there is a todo in the checker to handle calculations
 void dontwarn5() {
-  signed S = -32;
-  U8 = S + 10;
+  unsigned char c1 = 'A';
+  c1 = (c1 >= 'A' && c1 <= 'Z') ? c1 - 'A' + 'a' : c1;
+  unsigned char c2 = 0;
+  c2 = (c2 >= 'A' && c2 <= 'Z') ? c2 - 'A' + 'a' : c2;
+  unsigned char c3 = 'Z';
+  c3 = (c3 >= 'A' && c3 <= 'Z') ? c3 - 'A' + 'a' : c3;
+  unsigned char c4 = 'a';
+  c4 = (c4 >= 'A' && c4 <= 'Z') ? c4 - 'A' + 'a' : c4;
+  unsigned char c5 = '@';
+  c5 = (c5 >= 'A' && c5 <= 'Z') ? c5 - 'A' + 'a' : c5;
+}
+
+void dontwarn6() {
+  int x = ~0;
+  unsigned y = ~0;
+}
+
+void dontwarn7(unsigned x) {
+  if (x == (unsigned)-1) {
+  }
+}
+
+void dontwarn8() {
+  unsigned x = (unsigned)-1;
+}
+
+unsigned dontwarn9() {
+  return ~0;
 }
 
-char dontwarn6(long long x) {
+char dontwarn10(long long x) {
   long long y = 42;
   y += x;
   return y == 42;

diff  --git a/clang/test/Analysis/conversion.cpp b/clang/test/Analysis/conversion.cpp
new file mode 100644
index 0000000000000..eab6dfc566ec3
--- /dev/null
+++ b/clang/test/Analysis/conversion.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_analyze_cc1 -Wno-conversion -Wno-tautological-constant-compare -analyzer-checker=core,alpha.core.Conversion -verify %s
+
+// expected-no-diagnostics
+
+void dontwarn1() {
+  unsigned long x = static_cast<unsigned long>(-1);
+}
+
+void dontwarn2(unsigned x) {
+  if (x == static_cast<unsigned>(-1)) {
+  }
+}
+
+struct C {
+  C(unsigned x, unsigned long y) {}
+};
+
+void f(C) {}
+
+void functioncall1(long x) {
+  f(C(64, x));
+}