From d23232975fcd38b0ed64b8d12c5cacfe83e5aff0 Mon Sep 17 00:00:00 2001
From: Jordy Rose <jediknil@belkadan.com>
Date: Thu, 3 May 2012 07:34:01 +0000
Subject: [PATCH] [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).

llvm-svn: 156062
---
 .../StaticAnalyzer/Core/SimpleSValBuilder.cpp |  4 +-
 ...dditive-folding.c => additive-folding.cpp} | 39 +++++++++++++++++++
 clang/test/Analysis/string.c                  | 32 +++++++++++++++
 3 files changed, 73 insertions(+), 2 deletions(-)
 rename clang/test/Analysis/{additive-folding.c => additive-folding.cpp} (71%)

diff --git a/clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp b/clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
index 4a4fcf3c1fc6..057b8c0adb85 100644
--- a/clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ b/clang/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/clang/test/Analysis/additive-folding.c b/clang/test/Analysis/additive-folding.cpp
similarity index 71%
rename from clang/test/Analysis/additive-folding.c
rename to clang/test/Analysis/additive-folding.cpp
index 9a51d279ff9c..1a87ccfe7056 100644
--- a/clang/test/Analysis/additive-folding.c
+++ b/clang/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/clang/test/Analysis/string.c b/clang/test/Analysis/string.c
index c0814b89c17c..24e29ebb02b3 100644
--- a/clang/test/Analysis/string.c
+++ b/clang/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
+}