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

[pfultz2]

Summary

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.

[george.karpenkov]

Looks reasonable. Have you tried on any large codebases? You relax two guards, so I would be wary of explosion in false positives.

lib/StaticAnalyzer/Checkers/ConversionChecker.cpp

80–83

Sorry, I don't quite follow: why we don't want to warn when RHS is an integer constant?

[whisperity]

While I understand extending the analyzer to cover more is a good approach, there is -Wconversion which seemingly covers this -- or at least the trivial case(?):

#include <cstdio>
#include <vector>

void foo(unsigned x)
{
  printf("%u\n", x);
}

int main()
{
  int i = -1;
  foo(i);

  std::vector<int> x;
  x[i] = 5;
}

$ clang++ -Wconversion -o/dev/null check.cpp 
check.cpp:12:7: warning: implicit conversion changes signedness: 'int' to 'unsigned int' [-Wsign-conversion]
  foo(i);
  ~~~ ^
check.cpp:15:5: warning: implicit conversion changes signedness: 'int' to 'std::vector::size_type' (aka 'unsigned long') [-Wsign-conversion]
  x[i] = 5;
  ~ ^

[pfultz2]

While I understand extending the analyzer to cover more is a good approach, there is -Wconversion which seemingly covers this -- or at least the trivial case(?):

Yes, but it won't catch something like this, which is more interesting:

void g(unsigned);
void f(int x) {
    if (x < 0) return;
    g(x - 1);
}

[pfultz2]

lib/StaticAnalyzer/Checkers/ConversionChecker.cpp

80–83

Because we want to treat unsigned i = -1 as an explicit conversion. Ideally, I would like to just check that the RHS is a literal, but this seems like the closest way to do that.

[pfultz2]

Have you tried on any large codebases?

This check is not available to the user yet. I can move it to core if you want.

[pfultz2]

So moving to core will require explicit casts in some of the tests, especially for things like: memcpy(a262.s1, input, -1). Or this could be moved to another section instead of core. In D45532 there is talk of adding a bugprone section. I think this would be good there.

[pfultz2]

So I ran this on clang/llvm code base and fixed some false positives.

[NoQ]

Yep, looks good. This limitation to DeclRefExprs was super conservative.

I'll follow up regarding the bugprone category in a week or so on the mailing lists. It's been a long-standing historical decision that the Analyzer, for the sake of good user experience, should only warn on real bugs and have low false positive rates, but there definitely is a desire for more advanced users to opt-in into reasonable lint / coding standard checks, so we really need some room for them, and a way of discriminating them from checks that are simply bad due to large amounts of inevitable false positives.

In D46081#1079231, @whisperity wrote:

While I understand extending the analyzer to cover more is a good approach, there is -Wconversion which seemingly covers this -- or at least the trivial case(?):

Yeah, this check historically was an attempt on a quieter and at the same time more powerful alternative to -Wconversion.

[Szelethus]

To me it seems like @pfultz2 hasn't been on the site for a couple months. Is this patch going to be commited anytime?

[whisperity]

Ping.
Shouldn't let this thing go to waste.

[whisperity]

Pinging my people here, perhaps you being more knowledgeable in the analyser can say if this patch is salvageable in any way? Or did the state of the art moved past this in the years that went by? The patch itself doesn't look huge, but outside my direct expertise.

[martong]

@whisperity Thank you for pinging! This seems still feasible. Besides, this is valuable for us, as it could pass some SEI CERT test cases, notably from INT31-C.

[martong]

lib/StaticAnalyzer/Checkers/ConversionChecker.cpp

102

[pfultz2]

Thanks for merging as I dont have commit access.

lib/StaticAnalyzer/Checkers/ConversionChecker.cpp

private:
   bool isLossOfSign(const ImplicitCastExpr *Cast, CheckerContext &C) const;
 
   void reportBug(ExplodedNode *N, CheckerContext &C, const char Msg[]) const;
 };
 }
 
 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.
   if (Cast->getExprLoc().isMacroID())
     return;
 
   // Get Parent.
   const ParentMap &PM = C.getLocationContext()->getParentMap();
   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;
 
   // Loss of sign/precision in binary operation.
   if (const auto *B = dyn_cast<BinaryOperator>(Parent)) {
     BinaryOperator::Opcode Opc = B->getOpcode();
     if (Opc == BO_Assign) {
+      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);
     } else if (Opc == BO_MulAssign) {
       LossOfSign = isLossOfSign(Cast, C);
       LossOfPrecision = isLossOfPrecision(Cast, B->getLHS()->getType(), C);
     } else if (Opc == BO_DivAssign || Opc == BO_RemAssign) {
       LossOfSign = isLossOfSign(Cast, C);
       // No loss of precision.
     } else if (Opc == BO_AndAssign) {
       LossOfSign = isLossOfSign(Cast, C);
       // No loss of precision.
     } else if (Opc == BO_OrAssign || Opc == BO_XorAssign) {
       LossOfSign = isLossOfSign(Cast, C);
       LossOfPrecision = isLossOfPrecision(Cast, B->getLHS()->getType(), C);
     } else if (B->isRelationalOp() || B->isMultiplicativeOp()) {
       LossOfSign = isLossOfSign(Cast, C);
     }
-  } else if (isa<DeclStmt>(Parent)) {
+  } else if (isa<DeclStmt>(Parent) || isa<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);
   }
 
   if (LossOfSign || LossOfPrecision) {
     // Generate an error node.
     ExplodedNode *N = C.generateNonFatalErrorNode(C.getState());
     if (!N)

test/Analysis/conversion.c

 
 void division(unsigned U, signed S) {
   if (S > 5)
     S = U / S; // no-warning
   if (S < -10)
     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.
 
   U8 = -1;  // Explicit conversion.
   S8 = ~0U; // Explicit conversion.
   if (U > 300)
     U8 &= U; // No loss of precision since there is &=.

 }
 
 // don't warn for macros
 #define DOSTUFF ({ unsigned X = 1000; U8 = X; })
 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;
+}
 
 // false positives..
 
 int isascii(int c);
 void falsePositive1() {
   char kb2[5];
   int X = 1000;
   if (isascii(X)) {

test/Analysis/conversion.cpp

+// 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));
+}