alpha.core.Conversion - Fix false positive for 'U32 += S16;' expression, that is not unsafe

[danielmarjamaki]

Summary

This patch fix false positives for loss of sign in addition and subtraction assignment operators.

[zaks.anna]

Looks like you've also added handling of Xor, Or , Div, and Rem. Should there be tests for those?

[danielmarjamaki]

I added more testcases. There are several undetected "TODO: loss of precision" right now in the tests that I would like to fix. If this patch to fix FP is accepted I will commit it and continue working on the TODO tests. If it's not accepted I will continue investigating the TODO tests anyway..

[danielmarjamaki]

Updated the patch so all the loss of precision are detected also

[danielmarjamaki]

Ping

[NoQ]

There are several undetected "TODO: loss of precision" right now in the tests that I would like to fix.

I think it's a good idea to have // no-warning comments as well when testing for lack of false positives.

I've a feeling that the original false positive that you've been fixing initially has disappeared from the final tests (we've got stuff with unsigned long += int, but large types behave quite differently when it comes to integral promotion). If i'm wrong, please commit :)

cfe/trunk/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp

 class ConversionChecker : public Checker<check::PreStmt<ImplicitCastExpr>> {
 public:
   void checkPreStmt(const ImplicitCastExpr *Cast, CheckerContext &C) const;
 
 private:
   mutable std::unique_ptr<BuiltinBug> BT;
 
   // Is there loss of precision
-  bool isLossOfPrecision(const ImplicitCastExpr *Cast, CheckerContext &C) const;
+  bool isLossOfPrecision(const ImplicitCastExpr *Cast, QualType DestType,
+                         CheckerContext &C) const;
 
   // Is there loss of sign
   bool isLossOfSign(const ImplicitCastExpr *Cast, CheckerContext &C) const;
 
   void reportBug(ExplodedNode *N, CheckerContext &C, const char Msg[]) const;
 };
 }
 
if (!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 || Opc == BO_AddAssign || Opc == BO_SubAssign ||
-        Opc == BO_MulAssign) {
+    if (Opc == BO_Assign) {
       LossOfSign = isLossOfSign(Cast, C);
-      LossOfPrecision = isLossOfPrecision(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)) {
     LossOfSign = isLossOfSign(Cast, C);
-    LossOfPrecision = isLossOfPrecision(Cast, C);
+    LossOfPrecision = isLossOfPrecision(Cast, Cast->getType(), C);
   }
 
   if (LossOfSign || LossOfPrecision) {
     // Generate an error node.
     ExplodedNode *N = C.generateNonFatalErrorNode(C.getState());
     if (!N)
       return;
     if (LossOfSign)
void ConversionChecker::reportBug(ExplodedNode *N, CheckerContext &C,
   C.emitReport(std::move(R));
 }
 
 // Is E value greater or equal than Val?
 static bool isGreaterEqual(CheckerContext &C, const Expr *E,
                            unsigned long long Val) {
   ProgramStateRef State = C.getState();
   SVal EVal = C.getSVal(E);
+  if (EVal.isUnknownOrUndef())
+    return false;
+  if (!EVal.getAs<NonLoc>() && EVal.getAs<Loc>()) {
+    ProgramStateManager &Mgr = C.getStateManager();
+    EVal =
+        Mgr.getStoreManager().getBinding(State->getStore(), EVal.castAs<Loc>());
+  }
   if (EVal.isUnknownOrUndef() || !EVal.getAs<NonLoc>())
     return false;
 
   SValBuilder &Bldr = C.getSValBuilder();
   DefinedSVal V = Bldr.makeIntVal(Val, C.getASTContext().LongLongTy);
 
   // Is DefinedEVal greater or equal with V?
   SVal GE = Bldr.evalBinOp(State, BO_GE, EVal, V, Bldr.getConditionType());
static bool isNegative(CheckerContext &C, const Expr *E) {
   ProgramStateRef StNegative, StPositive;
   std::tie(StNegative, StPositive) =
       CM.assumeDual(State, LT.castAs<DefinedSVal>());
 
   return StNegative && !StPositive;
 }
 
 bool ConversionChecker::isLossOfPrecision(const ImplicitCastExpr *Cast,
+                                          QualType DestType,
                                           CheckerContext &C) const {
   // Don't warn about explicit loss of precision.
   if (Cast->isEvaluatable(C.getASTContext()))
     return false;
 
-  QualType CastType = Cast->getType();
   QualType SubType = Cast->IgnoreParenImpCasts()->getType();
 
-  if (!CastType->isIntegerType() || !SubType->isIntegerType())
+  if (!DestType->isIntegerType() || !SubType->isIntegerType())
     return false;
 
-  if (C.getASTContext().getIntWidth(CastType) >=
+  if (C.getASTContext().getIntWidth(DestType) >=
       C.getASTContext().getIntWidth(SubType))
     return false;
 
-  unsigned W = C.getASTContext().getIntWidth(CastType);
+  unsigned W = C.getASTContext().getIntWidth(DestType);
   if (W == 1 || W >= 64U)
     return false;
 
   unsigned long long MaxVal = 1ULL << W;
   return isGreaterEqual(C, Cast->getSubExpr(), MaxVal);
 }
 
 bool ConversionChecker::isLossOfSign(const ImplicitCastExpr *Cast,

cfe/trunk/test/Analysis/conversion.c

 // RUN: %clang_analyze_cc1 -Wno-conversion -analyzer-checker=core,alpha.core.Conversion -verify %s
 
 unsigned char U8;
 signed char S8;
 
 void assign(unsigned U, signed S) {
   if (S < -10)
     U8 = S; // expected-warning {{Loss of sign in implicit conversion}}
   if (U > 300)
     S8 = U; // expected-warning {{Loss of precision in implicit conversion}}
   if (S > 10)
-    U8 = S;
+    U8 = S; // no-warning
   if (U < 200)
-    S8 = U;
+    S8 = U; // no-warning
+}
+
+void addAssign() {
+  unsigned long L = 1000;
+  int I = -100;
+  U8 += L; // expected-warning {{Loss of precision in implicit conversion}}
+  L += I; // no-warning
+}
+
+void subAssign() {
+  unsigned long L = 1000;
+  int I = -100;
+  U8 -= L; // expected-warning {{Loss of precision in implicit conversion}}
+  L -= I; // no-warning
+}
+
+void mulAssign() {
+  unsigned long L = 1000;
+  int I = -1;
+  U8 *= L; // expected-warning {{Loss of precision in implicit conversion}}
+  L *= I;  // expected-warning {{Loss of sign in implicit conversion}}
+  I = 10;
+  L *= I; // no-warning
+}
+
+void divAssign() {
+  unsigned long L = 1000;
+  int I = -1;
+  U8 /= L; // no-warning
+  L /= I; // expected-warning {{Loss of sign in implicit conversion}}
+}
+
+void remAssign() {
+  unsigned long L = 1000;
+  int I = -1;
+  U8 %= L; // no-warning
+  L %= I; // expected-warning {{Loss of sign in implicit conversion}}
+}
+
+void andAssign() {
+  unsigned long L = 1000;
+  int I = -1;
+  U8 &= L; // no-warning
+  L &= I; // expected-warning {{Loss of sign in implicit conversion}}
+}
+
+void orAssign() {
+  unsigned long L = 1000;
+  int I = -1;
+  U8 |= L; // expected-warning {{Loss of precision in implicit conversion}}
+  L |= I;  // expected-warning {{Loss of sign in implicit conversion}}
+}
+
+void xorAssign() {
+  unsigned long L = 1000;
+  int I = -1;
+  U8 ^= L; // expected-warning {{Loss of precision in implicit conversion}}
+  L ^= I;  // expected-warning {{Loss of sign in implicit conversion}}
 }
 
 void init1() {
   long long A = 1LL << 60;
   short X = A; // expected-warning {{Loss of precision in implicit conversion}}
 }
 
 void relational(unsigned U, signed S) {
   if (S > 10) {
-    if (U < S) {
+    if (U < S) { // no-warning
     }
   }
   if (S < -10) {
     if (U < S) { // expected-warning {{Loss of sign in implicit conversion}}
     }
   }
 }
 
 void multiplication(unsigned U, signed S) {
   if (S > 5)
-    S = U * S;
+    S = U * S; // no-warning
   if (S < -10)
     S = U * S; // expected-warning {{Loss of sign}}
 }
 
 void division(unsigned U, signed S) {
   if (S > 5)
-    S = U / S;
+    S = U / S; // no-warning
   if (S < -10)
     S = U / S; // expected-warning {{Loss of sign}}
 }
 
 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.