From: Jordy Rose <jediknil@belkadan.com>
Date: Thu, 3 May 2012 19:05:48 +0000 (+0000)
Subject: [analyzer] When promoting constant integers in a comparison, use the larger width... 
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=90a7126f76b7511b0a073cbbcde40d1334b40542;p=clang

[analyzer] When promoting constant integers in a comparison, use the larger width of the two to avoid truncation.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@156089 91177308-0d34-0410-b5e6-96231b3b80d8
---

diff --git a/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp b/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
index 057b8c0adb..7aa36821f2 100644
--- a/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ b/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -496,30 +496,48 @@ SVal SimpleSValBuilder::evalBinOpNN(ProgramStateRef state,
         QualType conversionType = resultTy;
         if (BinaryOperator::isComparisonOp(op))
           conversionType = lhsType;
+        const llvm::APSInt *conversionPrototype = NULL;
 
         // Does the symbol simplify to a constant?  If so, "fold" the constant
         // by setting 'lhs' to a ConcreteInt and try again.
         if (lhsType->isIntegerType())
           if (const llvm::APSInt *Constant = state->getSymVal(Sym)) {
-            // The symbol evaluates to a constant. If necessary, promote the
-            // folded constant (LHS) to the result type.
-            const llvm::APSInt &lhs_I = BasicVals.Convert(conversionType,
-                *Constant);
-            lhs = nonloc::ConcreteInt(lhs_I);
-
-            // Also promote the RHS (if necessary).
-
-            // For shifts, it is not necessary to promote the RHS.
-            if (BinaryOperator::isShiftOp(op))
-              continue;
-
-            // Other operators: do an implicit conversion.  This shouldn't be
-            // necessary once we support truncation/extension of symbolic values.
-            if (nonloc::ConcreteInt *rhs_I = dyn_cast<nonloc::ConcreteInt>(&rhs)){
-              rhs = nonloc::ConcreteInt(BasicVals.Convert(conversionType,
-                  rhs_I->getValue()));
+            // Promote the RHS if necessary. Shift operations do not
+            // need their arguments to match in type, but others do.
+            if (!BinaryOperator::isShiftOp(op)) {
+              // If the RHS is a constant, we need to promote it.
+              if (nonloc::ConcreteInt *rhs_I =
+                    dyn_cast<nonloc::ConcreteInt>(&rhs)) {
+                const llvm::APSInt &val = rhs_I->getValue();
+
+                // The RHS may have a better type for performing comparisons.
+                // Consider x == 0xF00, where x is a fully constrained char.
+                if (BinaryOperator::isComparisonOp(op)) {
+                  const ASTContext &Ctx = getContext();
+                  unsigned width = std::max((unsigned)Ctx.getTypeSize(lhsType),
+                                            (unsigned)val.getBitWidth());
+
+                  // Use the LHS's signedness.
+                  bool isUnsigned =
+                    lhsType->isUnsignedIntegerOrEnumerationType();
+
+                  conversionPrototype = &BasicVals.getValue(val.getSExtValue(),
+                                                            width, isUnsigned);
+                } else {
+                  conversionPrototype = &BasicVals.Convert(resultTy, val);
+                }
+
+                // Record the promoted value.
+                rhs = nonloc::ConcreteInt(*conversionPrototype);
+              }              
             }
 
+            // Promote the LHS constant to the appropriate type.
+            const llvm::APSInt &lhs_I = conversionPrototype ? 
+              BasicVals.Convert(*conversionPrototype, *Constant) :
+              BasicVals.Convert(conversionType, *Constant);
+            lhs = nonloc::ConcreteInt(lhs_I);
+
             continue;
           }
 
@@ -529,6 +547,8 @@ SVal SimpleSValBuilder::evalBinOpNN(ProgramStateRef state,
           if (RSym->getType(Context)->isIntegerType()) {
             if (const llvm::APSInt *Constant = state->getSymVal(RSym)) {
               // The symbol evaluates to a constant.
+              // FIXME: This may not be the proper conversion type. Consider
+              // x > y, where y is a fully constrained int but x is a char.
               const llvm::APSInt &rhs_I = BasicVals.Convert(conversionType,
                   *Constant);
               rhs = nonloc::ConcreteInt(rhs_I);
@@ -536,10 +556,9 @@ SVal SimpleSValBuilder::evalBinOpNN(ProgramStateRef state,
           }
         }
 
-        if (isa<nonloc::ConcreteInt>(rhs)) {
+        if (nonloc::ConcreteInt *rhs_I = dyn_cast<nonloc::ConcreteInt>(&rhs)) {
           return MakeSymIntVal(slhs->getSymbol(), op,
-              cast<nonloc::ConcreteInt>(rhs).getValue(),
-              resultTy);
+                               rhs_I->getValue(), resultTy);
         }
 
         return makeSymExprValNN(state, op, InputLHS, InputRHS, resultTy);
diff --git a/test/Analysis/additive-folding.cpp b/test/Analysis/additive-folding.cpp
index 1a87ccfe70..3c9bf3b2e6 100644
--- a/test/Analysis/additive-folding.cpp
+++ b/test/Analysis/additive-folding.cpp
@@ -240,3 +240,31 @@ void PR12206(int x) {
   if (1 == (local + 1))
     malloc(1); // expected-warning{{never executed}}
 }
+
+void PR12206_truncation(signed char x) {
+  // Build a SymIntExpr, dependent on x.
+  signed char local = x - 1;
+
+  // Constrain the value of x.
+  if (x != 1) return;
+
+  // Constant-folding will turn (local+1) back into the symbol for x.
+  // The point of this dance is to make SValBuilder be responsible for
+  // turning the symbol into a ConcreteInt, rather than ExprEngine.
+
+  // Construct a value that cannot be represented by 'char',
+  // but that has the same lower bits as x.
+  signed int value = 1 + (1 << 8);
+
+  // Test relational operators.
+  if ((local + 1) >= value)
+    malloc(1); // expected-warning{{never executed}}
+  if (value <= (local + 1))
+    malloc(1); // expected-warning{{never executed}}
+
+  // Test equality operators.
+  if ((local + 1) == value) 
+    malloc(1); // expected-warning{{never executed}}
+  if (value == (local + 1))
+    malloc(1); // expected-warning{{never executed}}
+}