[analyzer] Add a checker for loss of sign or precision in integral ca…

…sts.

This new checker tries to find execution paths on which implicit integral casts
cause definite loss of information: a certainly-negative integer is converted
to an unsigned integer, or an integer is definitely truncated to fit into
a smaller type.

Being implicit, such casts are likely to produce unexpected results.

Patch by Daniel Marjamäki!

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

llvm-svn: 278941

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

@@ -133,6 +133,10 @@ def CastToStructChecker : Checker<"CastToStruct">,
   HelpText<"Check for cast from non-struct pointer to struct pointer">,
   DescFile<"CastToStructChecker.cpp">;
 
+def ConversionChecker : Checker<"Conversion">,
+  HelpText<"Loss of sign/precision in implicit conversions">,
+  DescFile<"ConversionChecker.cpp">;
+
 def IdenticalExprChecker : Checker<"IdenticalExpr">,
   HelpText<"Warn about unintended use of identical expressions in operators">,
   DescFile<"IdenticalExprChecker.cpp">;

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

@@ -23,6 +23,7 @@ add_clang_library(clangStaticAnalyzerCheckers
   ChrootChecker.cpp
   ClangCheckers.cpp
   CloneChecker.cpp
+  ConversionChecker.cpp
   CXXSelfAssignmentChecker.cpp
   DeadStoresChecker.cpp
   DebugCheckers.cpp

clang/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp

@@ -0,0 +1,192 @@
+//=== ConversionChecker.cpp -------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// Check that there is no loss of sign/precision in assignments, comparisons
+// and multiplications.
+//
+// ConversionChecker uses path sensitive analysis to determine possible values
+// of expressions. A warning is reported when:
+// * a negative value is implicitly converted to an unsigned value in an
+//   assignment, comparison or multiplication.
+// * assignment / initialization when source value is greater than the max
+//   value of target
+//
+// Many compilers and tools have similar checks that are based on semantic
+// analysis. Those checks are sound but have poor precision. ConversionChecker
+// is an alternative to those checks.
+//
+//===----------------------------------------------------------------------===//
+#include "ClangSACheckers.h"
+#include "clang/AST/ParentMap.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+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;
+
+  // Is there loss of sign
+  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()))
+    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;
+
+  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) {
+      LossOfSign = isLossOfSign(Cast, C);
+      LossOfPrecision = isLossOfPrecision(Cast, C);
+    } else if (B->isRelationalOp() || B->isMultiplicativeOp()) {
+      LossOfSign = isLossOfSign(Cast, C);
+    }
+  } else if (isa<DeclStmt>(Parent)) {
+    LossOfSign = isLossOfSign(Cast, C);
+    LossOfPrecision = isLossOfPrecision(Cast, C);
+  }
+
+  if (LossOfSign || LossOfPrecision) {
+    // Generate an error node.
+    ExplodedNode *N = C.generateNonFatalErrorNode(C.getState());
+    if (!N)
+      return;
+    if (LossOfSign)
+      reportBug(N, C, "Loss of sign in implicit conversion");
+    if (LossOfPrecision)
+      reportBug(N, C, "Loss of precision in implicit conversion");
+  }
+}
+
+void ConversionChecker::reportBug(ExplodedNode *N, CheckerContext &C,
+                                  const char Msg[]) const {
+  if (!BT)
+    BT.reset(
+        new BuiltinBug(this, "Conversion", "Possible loss of sign/precision."));
+
+  // Generate a report for this bug.
+  auto R = llvm::make_unique<BugReport>(*BT, Msg, N);
+  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() || !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());
+  if (GE.isUnknownOrUndef())
+    return false;
+  ConstraintManager &CM = C.getConstraintManager();
+  ProgramStateRef StGE, StLT;
+  std::tie(StGE, StLT) = CM.assumeDual(State, GE.castAs<DefinedSVal>());
+  return StGE && !StLT;
+}
+
+// Is E value negative?
+static bool isNegative(CheckerContext &C, const Expr *E) {
+  ProgramStateRef State = C.getState();
+  SVal EVal = State->getSVal(E, C.getLocationContext());
+  if (EVal.isUnknownOrUndef() || !EVal.getAs<NonLoc>())
+    return false;
+  DefinedSVal DefinedEVal = EVal.castAs<DefinedSVal>();
+
+  SValBuilder &Bldr = C.getSValBuilder();
+  DefinedSVal V = Bldr.makeIntVal(0, false);
+
+  SVal LT =
+      Bldr.evalBinOp(State, BO_LT, DefinedEVal, V, Bldr.getConditionType());
+
+  // Is E value greater than MaxVal?
+  ConstraintManager &CM = C.getConstraintManager();
+  ProgramStateRef StNegative, StPositive;
+  std::tie(StNegative, StPositive) =
+      CM.assumeDual(State, LT.castAs<DefinedSVal>());
+
+  return StNegative && !StPositive;
+}
+
+bool ConversionChecker::isLossOfPrecision(const ImplicitCastExpr *Cast,
+                                        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())
+    return false;
+
+  if (C.getASTContext().getIntWidth(CastType) >=
+      C.getASTContext().getIntWidth(SubType))
+    return false;
+
+  unsigned W = C.getASTContext().getIntWidth(CastType);
+  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,
+                                   CheckerContext &C) const {
+  QualType CastType = Cast->getType();
+  QualType SubType = Cast->IgnoreParenImpCasts()->getType();
+
+  if (!CastType->isUnsignedIntegerType() || !SubType->isSignedIntegerType())
+    return false;
+
+  return isNegative(C, Cast->getSubExpr());
+}
+
+void ento::registerConversionChecker(CheckerManager &mgr) {
+  mgr.registerChecker<ConversionChecker>();
+}

clang/test/Analysis/conversion.c

@@ -0,0 +1,125 @@
+// RUN: %clang_cc1 -Wno-conversion -analyze -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;
+  if (U < 200)
+    S8 = U;
+}
+
+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 (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;
+  if (S < -10)
+    S = U * S; // expected-warning {{Loss of sign}}
+}
+
+void division(unsigned U, signed S) {
+  if (S > 5)
+    S = U / S;
+  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.
+
+  U8 = -1;  // Explicit conversion.
+  S8 = ~0U; // Explicit conversion.
+  if (U > 300)
+    U8 &= U; // No loss of precision since there is &=.
+}
+
+void dontwarn2(unsigned int U) {
+  if (U <= 4294967295) {
+  }
+  if (U <= (2147483647 * 2U + 1U)) {
+  }
+}
+
+void dontwarn3(int X) {
+  S8 = X ? 'a' : 'b';
+}
+
+// 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;
+}
+
+
+// false positives..
+
+int isascii(int c);
+void falsePositive1() {
+  char kb2[5];
+  int X = 1000;
+  if (isascii(X)) {
+    // FIXME: should not warn here:
+    kb2[0] = X; // expected-warning {{Loss of precision}}
+  }
+}
+
+
+typedef struct FILE {} FILE; int getc(FILE *stream);
+# define EOF (-1)
+char reply_string[8192];
+FILE *cin;
+extern int dostuff (void);
+int falsePositive2() {
+  int c, n;
+  int dig;
+  char *cp = reply_string;
+  int pflag = 0;
+  int code;
+
+  for (;;) {
+    dig = n = code = 0;
+    while ((c = getc(cin)) != '\n') {
+      if (dig < 4 && dostuff())
+        code = code * 10 + (c - '0');
+      if (!pflag && code == 227)
+        pflag = 1;
+      if (n == 0)
+        n = c;
+      if (c == EOF)
+        return(4);
+      if (cp < &reply_string[sizeof(reply_string) - 1])
+        // FIXME: should not warn here:
+        *cp++ = c; // expected-warning {{Loss of precision}}
+    }
+  }
+}
+