[analyzer] A quick fix on top of D12901/r257464

[NoQ]

Summary

Sorry for being a bit slow, i should have had a look at the review earlier; i noticed some stuff after the recent patch by Pierre Gousseau was committed.

1. There's an off-by-one in the comparison evalBinOp in evalIntegralCast, the patch attached to this revision fixes it, and tests it via bool-assignment checker.

2. There's also a FIXME test for the same checker, which is a regression after this patch (used to throw a warning, now it doesn't); seems not as bad as the crash, so it can probably be addressed later.

3. I noticed that it's still possible to trigger the crash with the following test, not sure what to do with it:

void f1(long foo)
{
  unsigned index = -1;
  if (index < foo - 1) index = foo;
  if (index + 1 == 0) {}
}

So my best idea is to push this quick fix for off-by-one and then think where to go further. Probably we need to teach RangeConstraintManager to work with SymbolCast's correctly (eg. understand how their range is related to a parent symbol's range). Eventually, once it gets better, we'd probably want to change the following in evalIntegralCast:

   std::tie(IsNotTruncated, IsTruncated) = state->assume(CompVal);
-  if (!IsNotTruncated && IsTruncated) {
+  if (IsTruncated) {

but right now it breaks tests (not only bool-assignment, but also other checkers that start failing can probably be shown to loose warnings after this patch, but i didn't have a look at this yet, so not sure, just hand-waving).

Anyway, generally, overally, i think r257464 is a step in the very right direction if we want to work with casts better, and i was actually very happy to see it went that way :)

[pgousseau]

LGTM!
Thanks Artom for finding the bug and new test case, sorry for missing those in my patch!
Removing "!IsNotTruncated" is definitely worth trying.
Please commit if code owners are happy with it too.

Pierre.

cfe/trunk/lib/StaticAnalyzer/Core/SValBuilder.cpp

NonLoc ToTypeMaxVal =
                                         : ToTypeMax.getSExtValue(),
                  castTy)
           .castAs<NonLoc>();
   // Check the range of the symbol being casted against the maximum value of the
   // target type.
   NonLoc FromVal = val.castAs<NonLoc>();
   QualType CmpTy = getConditionType();
   NonLoc CompVal =
-      evalBinOpNN(state, BO_LT, FromVal, ToTypeMaxVal, CmpTy).castAs<NonLoc>();
+      evalBinOpNN(state, BO_LE, FromVal, ToTypeMaxVal, CmpTy).castAs<NonLoc>();
   ProgramStateRef IsNotTruncated, IsTruncated;
   std::tie(IsNotTruncated, IsTruncated) = state->assume(CompVal);
   if (!IsNotTruncated && IsTruncated) {
     // Symbol is truncated so we evaluate it as a cast.
     NonLoc CastVal = makeNonLoc(se, originalTy, castTy);
     return CastVal;
   }
   return evalCast(val, castTy, originalTy);

cfe/trunk/test/Analysis/bool-assignment.c

 typedef signed char BOOL;
 
 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) {
+    // FIXME: Currently we are loosing this warning due to a SymbolCast in RHS.
+    BOOL x = y; // no-warning
+    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}}
     return;
   }
   BOOL x = y; // no-warning
 }
 
 void test_BOOL_assignment(int y) {