From: Jordy Rose <jediknil@belkadan.com>
Date: Thu, 3 May 2012 07:34:01 +0000 (+0000)
Subject: [analyzer] Equality ops are like relational ops in that the arguments shouldn't be... 
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=14d20b1dff6370f76279fcfb0fd780e2e5eb57bb;p=clang

[analyzer] Equality ops are like relational ops in that the arguments shouldn't be converted to the result type. Fixes PR12206 and dupe PR12510.

This was probably the original intent of r133041 (also me, a year ago).

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

diff --git a/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp b/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
index 4a4fcf3c1f..057b8c0adb 100644
--- a/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ b/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -345,7 +345,7 @@ SVal SimpleSValBuilder::evalBinOpNN(ProgramStateRef state,
           if (const llvm::APSInt *Constant = state->getSymVal(RSym)) {
             // The symbol evaluates to a constant.
             const llvm::APSInt *rhs_I;
-            if (BinaryOperator::isRelationalOp(op))
+            if (BinaryOperator::isComparisonOp(op))
               rhs_I = &BasicVals.Convert(lhsInt.getValue(), *Constant);
             else
               rhs_I = &BasicVals.Convert(resultTy, *Constant);
@@ -494,7 +494,7 @@ SVal SimpleSValBuilder::evalBinOpNN(ProgramStateRef state,
         // The conversion type is usually the result type, but not in the case
         // of relational expressions.
         QualType conversionType = resultTy;
-        if (BinaryOperator::isRelationalOp(op))
+        if (BinaryOperator::isComparisonOp(op))
           conversionType = lhsType;
 
         // Does the symbol simplify to a constant?  If so, "fold" the constant
diff --git a/test/Analysis/additive-folding.c b/test/Analysis/additive-folding.cpp
similarity index 71%
rename from test/Analysis/additive-folding.c
rename to test/Analysis/additive-folding.cpp
index 9a51d279ff..1a87ccfe70 100644
--- a/test/Analysis/additive-folding.c
+++ b/test/Analysis/additive-folding.cpp
@@ -201,3 +201,42 @@ void tautologyLE (unsigned a) {
     free(b);
   return; // no-warning
 }
+
+
+// PR12206/12510 - When SimpleSValBuilder figures out that a symbol is fully
+// constrained, it should cast the value to the result type in a binary
+// operation...unless the binary operation is a comparison, in which case the
+// two arguments should be the same type, but won't match the result type.
+//
+// This is easier to trigger in C++ mode, where the comparison result type is
+// 'bool' and is thus differently sized from int on pretty much every system.
+//
+// This is not directly related to additive folding, but we use SValBuilder's
+// additive folding to tickle the bug. ExprEngine will simplify fully-constrained
+// symbols, so SValBuilder will only see them if they are (a) part of an evaluated
+// SymExpr (e.g. with additive folding) or (b) generated by a checker (e.g.
+// unix.cstring's strlen() modelling).
+void PR12206(int x) {
+  // Build a SymIntExpr, dependent on x.
+  int local = x - 1;
+
+  // Constrain the value of x.
+  int value = 1 + (1 << (8 * sizeof(1 == 1))); // not representable by bool
+  if (x != value) 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.
+
+  // Test relational operators.
+  if ((local + 1) < 2)
+    malloc(1); // expected-warning{{never executed}}
+  if (2 > (local + 1))
+    malloc(1); // expected-warning{{never executed}}
+
+  // Test equality operators.
+  if ((local + 1) == 1) 
+    malloc(1); // expected-warning{{never executed}}
+  if (1 == (local + 1))
+    malloc(1); // expected-warning{{never executed}}
+}
diff --git a/test/Analysis/string.c b/test/Analysis/string.c
index c0814b89c1..24e29ebb02 100644
--- a/test/Analysis/string.c
+++ b/test/Analysis/string.c
@@ -1122,3 +1122,35 @@ void strncasecmp_embedded_null () {
 	if (strncasecmp("ab\0zz", "ab\0yy", 4) != 0)
 		(void)*(char*)0; // no-warning
 }
+
+//===----------------------------------------------------------------------===
+// Miscellaneous extras.
+//===----------------------------------------------------------------------===
+
+// See additive-folding.cpp for a description of this bug.
+// This test is insurance in case we significantly change how SymExprs are
+// evaluated. It isn't as good as additive-folding.cpp's version
+// because it will only actually be a test on systems where
+//   sizeof(1 == 1) < sizeof(size_t).
+// We could add a triple if it becomes necessary.
+void PR12206(const char *x) {
+  // This test is only useful under these conditions.
+  size_t comparisonSize = sizeof(1 == 1);
+  if (sizeof(size_t) <= comparisonSize) return;
+
+  // Create a value that requires more bits to store than a comparison result.
+  size_t value = 1UL;
+  value <<= 8 * comparisonSize;
+  value += 1;
+
+  // Constrain the length of x.
+  if (strlen(x) != value) return;
+
+  // Test relational operators.
+  if (strlen(x) < 2) { (void)*(char*)0; } // no-warning
+  if (2 > strlen(x)) { (void)*(char*)0; } // no-warning
+
+  // Test equality operators.
+  if (strlen(x) == 1) { (void)*(char*)0; } // no-warning
+  if (1 == strlen(x)) { (void)*(char*)0; } // no-warning
+}