[analyzer] Simplify BoolAssignmentChecker

[mikhail.ramalho]

Summary

Instead of checking the range manually, changed the checker to use assumeInclusiveRangeDual instead.

This patch was part of D28955.

[NoQ]

Thanks!!! Small patches are so nice to review.

clang/lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp

91

I believe this should be !StIn. I.e., the right check to do is "the value is known to be not boolean" (this is definitely a buggy execution path the static analyzer managed to discover), whereas "the value is not known to be definitely boolean" is the wrong check to do (the static analyzer doesn't have enough information to conclude that the value is definitely boolean, which might be because the value may be non-boolean, but it's more likely that this happens because the static analyzer simply doesn't know enough about the program).

I think this change should fix the regressions on the test suite.

[mikhail.ramalho]

Update the condition to flag an issue.

[NoQ]

That's more like it! :)

clang/lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp

void BoolAssignmentChecker::checkBind(SVal loc, SVal val, const Stmt *S,
   QualType valTy = TR->getValueType();
 
   if (!isBooleanType(valTy))
     return;
 
   // Get the value of the right-hand side.  We only care about values
   // that are defined (UnknownVals and UndefinedVals are handled by other
   // checkers).
-  Optional<DefinedSVal> DV = val.getAs<DefinedSVal>();
-  if (!DV)
+  Optional<NonLoc> NV = val.getAs<NonLoc>();
+  if (!NV)
     return;
 
   // Check if the assigned value meets our criteria for correctness.  It must
   // be a value that is either 0 or 1.  One way to check this is to see if
   // the value is possibly < 0 (for a negative value) or greater than 1.
   ProgramStateRef state = C.getState();
   SValBuilder &svalBuilder = C.getSValBuilder();
+  BasicValueFactory &BVF = svalBuilder.getBasicValueFactory();
   ConstraintManager &CM = C.getConstraintManager();
 
-  // First, ensure that the value is >= 0.
-  DefinedSVal zeroVal = svalBuilder.makeIntVal(0, valTy);
-  SVal greaterThanOrEqualToZeroVal =
-    svalBuilder.evalBinOp(state, BO_GE, *DV, zeroVal,
-                          svalBuilder.getConditionType());
-
-  Optional<DefinedSVal> greaterThanEqualToZero =
-      greaterThanOrEqualToZeroVal.getAs<DefinedSVal>();
-
-  if (!greaterThanEqualToZero) {
-    // The SValBuilder cannot construct a valid SVal for this condition.
-    // This means we cannot properly reason about it.
-    return;
-  }
-
-  ProgramStateRef stateLT, stateGE;
-  std::tie(stateGE, stateLT) = CM.assumeDual(state, *greaterThanEqualToZero);
-
-  // Is it possible for the value to be less than zero?
-  if (stateLT) {
-    // It is possible for the value to be less than zero.  We only
-    // want to emit a warning, however, if that value is fully constrained.
-    // If it it possible for the value to be >= 0, then essentially the
-    // value is underconstrained and there is nothing left to be done.
-    if (!stateGE)
-      emitReport(stateLT, C);
-
-    // In either case, we are done.
-    return;
-  }
+  llvm::APSInt Zero = BVF.getValue(0, valTy);
+  llvm::APSInt One = BVF.getValue(1, valTy);
 
-  // If we reach here, it must be the case that the value is constrained
-  // to only be >= 0.
-  assert(stateGE == state);
-
-  // At this point we know that the value is >= 0.
-  // Now check to ensure that the value is <= 1.
-  DefinedSVal OneVal = svalBuilder.makeIntVal(1, valTy);
-  SVal lessThanEqToOneVal =
-    svalBuilder.evalBinOp(state, BO_LE, *DV, OneVal,
-                          svalBuilder.getConditionType());
-
-  Optional<DefinedSVal> lessThanEqToOne =
-      lessThanEqToOneVal.getAs<DefinedSVal>();
-
-  if (!lessThanEqToOne) {
-    // The SValBuilder cannot construct a valid SVal for this condition.
-    // This means we cannot properly reason about it.
-    return;
-  }
-
-  ProgramStateRef stateGT, stateLE;
-  std::tie(stateLE, stateGT) = CM.assumeDual(state, *lessThanEqToOne);
-
-  // Is it possible for the value to be greater than one?
-  if (stateGT) {
-    // It is possible for the value to be greater than one.  We only
-    // want to emit a warning, however, if that value is fully constrained.
-    // If it is possible for the value to be <= 1, then essentially the
-    // value is underconstrained and there is nothing left to be done.
-    if (!stateLE)
-      emitReport(stateGT, C);
-
-    // In either case, we are done.
-    return;
-  }
+  ProgramStateRef StIn, StOut;
+  std::tie(StIn, StOut) = CM.assumeInclusiveRangeDual(state, *NV, Zero, One);
 
-  // If we reach here, it must be the case that the value is constrained
-  // to only be <= 1.
-  assert(stateLE == state);
+  if (StOut)
+    emitReport(StOut, C);
 }
 
 void ento::registerBoolAssignmentChecker(CheckerManager &mgr) {
     mgr.registerChecker<BoolAssignmentChecker>();
 }
 
 bool ento::shouldRegisterBoolAssignmentChecker(const LangOptions &LO) {
   return true;
 }

clang/test/Analysis/bool-assignment.c

 
 void test_BOOL_initialization(int y) {
   BOOL constant = 2; // expected-warning {{Assignment of a non-Boolean value}}
   if (y < 0) {
     BOOL x = y; // expected-warning {{Assignment of a non-Boolean value}}
     return;
   }
   if (y > 200 && y < 250) {
-#ifdef ANALYZER_CM_Z3
     BOOL x = y; // expected-warning {{Assignment of a non-Boolean value}}
-#else
-    BOOL x = y; // no-warning
-#endif
     return;
   }
   if (y >= 127 && y < 150) {
     BOOL x = y; // expected-warning{{Assignment of a non-Boolean value}}
     return;
   }
   if (y > 1) {
     BOOL x = y; // expected-warning {{Assignment of a non-Boolean value}}

clang/test/Analysis/dead-stores.m

 void foo_rdar8527823();
 @interface RDar8527823
 - (void) bar_rbar8527823;
 @end
 @implementation RDar8527823
 - (void) bar_rbar8527823
 {
  @synchronized(self) {
-   BOOL isExec = baz_rdar8527823(); // no-warning
+   BOOL isExec = baz_rdar8527823(); // expected-warning {{Assignment of a non-Boolean value}}
    if (isExec) foo_rdar8527823();
  }
 }
 @end
 
 // Don't flag dead stores to assignments to self within a nested assignment.
 @interface Rdar7947686
 - (id) init;

clang/test/Analysis/misc-ps-eager-assume.m

 // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core -analyzer-store=region -verify -fblocks %s
-// expected-no-diagnostics
 
 // Delta-reduced header stuff (needed for test cases).
 typedef signed char BOOL;
 typedef unsigned int NSUInteger;
 typedef struct _NSZone NSZone;
 @class NSInvocation, NSMethodSignature, NSCoder, NSString, NSEnumerator;
 @protocol NSObject  - (BOOL)isEqual:(id)object;
 - (oneway void)release;

 // promotions of symbolic values, so this test case tests the logic in
 // 'recoverCastedSymbol()' (GRExprEngine.cpp) to test that we recover
 // path-sensitivity and use the symbol for 'needsAnArray' in the branch
 // condition.
 //
 void handle_symbolic_cast_in_condition(void) {
   NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init];
 
-  BOOL needsAnArray = [@"aString" isEqualToString:@"anotherString"];
+  BOOL needsAnArray = [@"aString" isEqualToString:@"anotherString"]; // expected-warning {{Assignment of a non-Boolean value}}
   NSMutableArray* array = needsAnArray ? [[NSMutableArray alloc] init] : 0;
   if(needsAnArray)
     [array release];
 
   [pool drain];
 }
 
 // From PR 3836 (http://llvm.org/bugs/show_bug.cgi?id=3836)