[analyzer] Fix modeling of bool based types

[alexander-shaposhnikov]

Summary

This is a follow up for one of the previous diffs https://reviews.llvm.org/D32328.
getTypeSize and with getIntWidth are not equivalent for bool
(see https://clang.llvm.org/doxygen/ASTContext_8cpp_source.html#l08444),
this causes a number of issues
(for instance, if APint X representing a bool is created with the wrong bit width then X is not comparable against Min/Max
(because of the different bit width), that results in crashes (triggered asserts) inside assume* methods) (for example see the test case).
P.S. I have found some other suspicious places where getIntWidth probably should be used instead of getTypeSize,
however fixing all of them in one go is not trivial
(a couple of tests have failed on the larger diff (which might indicate some issues inside those checkers), but I have not investigated it yet,
so decided to break up my changes into smaller chunks and keep all the tests green.

Test plan:
make check-all

[NoQ]

Thanks, these are very helpful.

Sending changes in smaller chunks is great even if you already have one large chunk perfectly working :)

The test covers only the BasicValueFactory-related part of your patch; the Loc-related changes are still to be tested. I suspect the following may work:

1. For evalCastFromNonLoc: casting a symbolic pointer to an integer, then casting this integer to the enum class.
2. For evalCastFromLoc: Casting a concrete pointer (eg. null pointer) to the enum class (not sure if you can trick the analyzer into such cast directly while following the c++ standard).

[alexander-shaposhnikov]

thanks,

1. evalCastFromNonLoc is actually called there (and that's how i ran into this issue) because of (a bit unexpected to me) ImplicitCastExpr 'LValueToRValue' inside switch:
   

(to be honest i don't know why Clang inserts this cast into AST here, but i have checked several versions of clang - the behavior is the same)

2. evalCastFromLoc - that's hard to me - so far i have not found a valid compilable c++ code to cast a pointer to bool enum.

[NoQ]

In D35041#801073, @alexshap wrote:

evalCastFromNonLoc is actually called there (and that's how i ran into this issue) because of (a bit unexpected to me) ImplicitCastExpr 'LValueToRValue' inside switch

Yeah, but the NonLoc that's being casted is definitely not nonloc::LocAsInteger.
And i also tried to run your test with only BasicValueFactory fix and it passed.

In D35041#801073, @alexshap wrote:

evalCastFromLoc - that's hard to me - so far i have not found a valid compilable c++ code to cast a pointer to bool enum.

I didn't see if that's exactly what we're looking for, but the following compiles and crashes:

EnumBool E = static_cast<EnumBool>((intptr_t)nullptr);
switch (E) {
case EnumBool::F:
  return false;
}
return true;

[alexander-shaposhnikov]

In D35041#801073, @alexshap wrote:
evalCastFromNonLoc is actually called there (and that's how i ran into this issue) because of (a bit unexpected to me) ImplicitCastExpr 'LValueToRValue' inside switch

Yeah, but the NonLoc that's being casted is definitely not nonloc::LocAsInteger.

yea, you are right, sorry about the confusion.

[alexander-shaposhnikov]

Add a test where evalCastFromLoc is called
and the line unsigned BitWidth = Context.getIntWidth(castTy);
is hit.
I am planning to update this patch once again with a proper test where
the path inside evalCastFromNonLoc is executed (need a bit more time)

[alexander-shaposhnikov]

Add a test where SimpleSValBuilder::evalCastFromNonLoc is called and we hit the line unsigned castSize = Context.getIntWidth(castTy);

[alexander-shaposhnikov]

ping

[NoQ]

test/Analysis/enum.cpp

50 ↗

(On Diff #105597)

Nice! Will use in my tests.

cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h

public:
 
   const llvm::APSInt& getValue(const llvm::APSInt& X);
   const llvm::APSInt& getValue(const llvm::APInt& X, bool isUnsigned);
   const llvm::APSInt& getValue(uint64_t X, QualType T);
 
   /// Returns the type of the APSInt used to store values of the given QualType.
   APSIntType getAPSIntType(QualType T) const {
     assert(T->isIntegralOrEnumerationType() || Loc::isLocType(T));
-    return APSIntType(Ctx.getTypeSize(T),
+    return APSIntType(Ctx.getIntWidth(T),
                       !T->isSignedIntegerOrEnumerationType());
   }
 
   /// Convert - Create a new persistent APSInt with the same value as 'From'
   ///  but with the bitwidth and signedness of 'To'.
   const llvm::APSInt &Convert(const llvm::APSInt& To,
                               const llvm::APSInt& From) {
     APSIntType TargetType(To);

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

 
 SVal SimpleSValBuilder::dispatchCast(SVal Val, QualType CastTy) {
   assert(Val.getAs<Loc>() || Val.getAs<NonLoc>());
   return Val.getAs<Loc>() ? evalCastFromLoc(Val.castAs<Loc>(), CastTy)
                            : evalCastFromNonLoc(Val.castAs<NonLoc>(), CastTy);
 }
 
 SVal SimpleSValBuilder::evalCastFromNonLoc(NonLoc val, QualType castTy) {
-
   bool isLocType = Loc::isLocType(castTy);
-
   if (val.getAs<nonloc::PointerToMember>())
     return val;
 
   if (Optional<nonloc::LocAsInteger> LI = val.getAs<nonloc::LocAsInteger>()) {
     if (isLocType)
       return LI->getLoc();
-
     // FIXME: Correctly support promotions/truncations.
-    unsigned castSize = Context.getTypeSize(castTy);
+    unsigned castSize = Context.getIntWidth(castTy);
     if (castSize == LI->getNumBits())
       return val;
     return makeLocAsInteger(LI->getLoc(), castSize);
   }
 
   if (const SymExpr *se = val.getAsSymbolicExpression()) {
     QualType T = Context.getCanonicalType(se->getType());
     // If types are the same or both are integers, ignore the cast.
switch (val.getSubKind()) {
 
       case loc::GotoLabelKind:
         // Labels and non-symbolic memory regions are always true.
         return makeTruthVal(true, castTy);
     }
   }
 
   if (castTy->isIntegralOrEnumerationType()) {
-    unsigned BitWidth = Context.getTypeSize(castTy);
+    unsigned BitWidth = Context.getIntWidth(castTy);
 
     if (!val.getAs<loc::ConcreteInt>())
       return makeLocAsInteger(val, BitWidth);
 
     llvm::APSInt i = val.castAs<loc::ConcreteInt>().getValue();
     BasicVals.getAPSIntType(castTy).apply(i);
     return makeIntVal(i);
   }

cfe/trunk/test/Analysis/enum.cpp

 
 bool testNoCrashOnSwitchEnumBool(EnumBool E) {
   switch (E) {
   case EnumBool::F:
     return false;
   }
   return true;
 }
+
+bool testNoCrashOnSwitchEnumBoolConstant() {
+  EnumBool E = EnumBool::F;
+  switch (E) {
+    case EnumBool::F:
+      return false; 
+  }
+  return true; 
+}
+
+typedef __INTPTR_TYPE__ intptr_t;
+bool testNoCrashOnSwitchEnumBoolConstantCastedFromNullptr() {
+  EnumBool E = static_cast<EnumBool>((intptr_t)nullptr);
+  switch (E) {
+  case EnumBool::F:
+    return false;
+  }
+  return true;
+}
+
+bool testNoCrashOnSwitchEnumBoolConstantCastedFromPtr() {
+  int X;
+  intptr_t P = (intptr_t)&X;
+  EnumBool E = static_cast<EnumBool>(P);
+  switch (E) {
+  case EnumBool::F:
+    return false;
+  }
+  return true;
+}