[PATCH] Loss of Sign Checker

[Daniel Marjamäki]

> While the above is trying to assign a signed value to an unsigned variable, I am still not able to correctly establish if the value is negative for sure. The following is a reduced piece of code that still throws the diagnostic, but I am not sure what exactly am I missing in my check to avoid this. Note that the reported line of code seems to be unreachable:


Thank you!

The code is not unreachable as far as I see.. the function yylex() can be called from another source file for instance.

The warning is correct if we assume that the global variables have their default initialisation values. That is probably not the case in the real code.

For now I think the yylex warning can be ok. At least in the reduced code, we can't prove that rhs is never negative. If we want to fix these warnings later, I think that it must be solved in the Clang infrastructure. Then I would suggest we do that in a separate patch.

I will also run this checker on some projects..


================
Comment at: lib/StaticAnalyzer/Checkers/LossOfSignChecker.cpp:10
@@ +9,3 @@
+//
+// This defines LossOfSignChecker, a builtin check in ExprEngine that
+// performs checks for assignment of signed negative values to unsigned
----------------
I don't know see the connection against "ExprEngine".. so this comment looks confusing to me..  how about removing "a builtin check in ExprEngine"?

================
Comment at: lib/StaticAnalyzer/Checkers/LossOfSignChecker.cpp:39
@@ +38,3 @@
+
+// Strip off all intervening operations to get to the real RHS
+static const Expr* GetAbsoluteRHS(const Expr *Ex) {
----------------
Please end all comments with a period ".".

================
Comment at: lib/StaticAnalyzer/Checkers/LossOfSignChecker.cpp:40
@@ +39,3 @@
+// Strip off all intervening operations to get to the real RHS
+static const Expr* GetAbsoluteRHS(const Expr *Ex) {
+  while (Ex) {
----------------
the first character in function names should be lower case.

================
Comment at: lib/StaticAnalyzer/Checkers/LossOfSignChecker.cpp:71
@@ +70,3 @@
+                                                   CheckerContext &C) const {
+  ProgramStateRef state = C.getState(); 
+  SValBuilder &svalBuilder = C.getSValBuilder();
----------------
Variables should start with a uppercase letter. And abbreviations are preferred.

================
Comment at: lib/StaticAnalyzer/Checkers/LossOfSignChecker.cpp:85
@@ +84,3 @@
+  // Only look at binding of signed value type to unsigned variable type
+  if (varTy->isUnsignedIntegerType() && rhsTy->isSignedIntegerType()) {
+    //
----------------
this should probably be a early return.

================
Comment at: lib/StaticAnalyzer/Checkers/LossOfSignChecker.cpp:95
@@ +94,3 @@
+    SVal LessThanZeroVal = svalBuilder.evalBinOp(state, BO_LT, 
+                                                      castVal, Zero, rhsTy);
+    if (Optional<DefinedSVal> LessThanZeroDVal =
----------------
I assume you did not run clang-format on this?

    clang-format -i LossOfSignChecker.cpp


================
Comment at: lib/StaticAnalyzer/Checkers/LossOfSignChecker.cpp:104
@@ +103,3 @@
+        emitReport(StateNeg, C);
+        return;
+      }
----------------
as far as I see this return is redundant

================
Comment at: lib/StaticAnalyzer/Checkers/LossOfSignChecker.cpp:108
@@ +107,3 @@
+      // If it reaches here, we cannot reason about it well
+      return;
+    }
----------------
as far as I see this return is redundant

================
Comment at: lib/StaticAnalyzer/Checkers/LossOfSignChecker.cpp:128
@@ +127,3 @@
+
+    if (DeclRefExpr *DR = dyn_cast<DeclRefExpr>(B->getLHS()))
+      if (VarDecl *VD = dyn_cast<VarDecl>(DR->getDecl())) {
----------------
 Please use { on this outer if also. I'd say the { on the inner if is optional.

================
Comment at: lib/StaticAnalyzer/Checkers/LossOfSignChecker.cpp:158
@@ +157,3 @@
+  // Only look at binding of signed value type to unsigned variable type
+  if (varTy->isUnsignedIntegerType() && rhsTy->isSignedIntegerType()) {
+    //
----------------
early return

http://reviews.llvm.org/D10634

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/