From 65d80fd4acfe65400b7ad594042adc08e972c405 Mon Sep 17 00:00:00 2001
From: Ted Kremenek <kremenek@apple.com>
Date: Mon, 4 May 2009 17:53:11 +0000
Subject: [PATCH] Fix false positive null dereference by unifying code paths in
 GRSimpleVals for '==' and '!=' (some code in the '!=' was not replicated in
 the '==' code, causing some constraints to get lost).

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@70885 91177308-0d34-0410-b5e6-96231b3b80d8
---
 lib/Analysis/GRSimpleVals.cpp | 81 ++++++++++-------------------------
 lib/Analysis/GRSimpleVals.h   |  6 +--
 test/Analysis/null-deref-ps.c |  6 +--
 3 files changed, 27 insertions(+), 66 deletions(-)

diff --git a/lib/Analysis/GRSimpleVals.cpp b/lib/Analysis/GRSimpleVals.cpp
index 860013872e..3d1c76412c 100644
--- a/lib/Analysis/GRSimpleVals.cpp
+++ b/lib/Analysis/GRSimpleVals.cpp
@@ -249,15 +249,11 @@ SVal GRSimpleVals::EvalBinOp(GRExprEngine& Eng, BinaryOperator::Opcode Op,
                              Loc L, Loc R) {
   
   switch (Op) {
-
     default:
-      return UnknownVal();
-      
+      return UnknownVal();      
     case BinaryOperator::EQ:
-      return EvalEQ(Eng, L, R);
-      
     case BinaryOperator::NE:
-      return EvalNE(Eng, L, R);      
+      return EvalEquality(Eng, L, R, Op == BinaryOperator::EQ);
   }
 }
 
@@ -286,14 +282,14 @@ SVal GRSimpleVals::EvalBinOp(GRExprEngine& Eng, BinaryOperator::Opcode Op,
 // FIXME: All this logic will be revamped when we have MemRegion::getLocation()
 // implemented.
 
-SVal GRSimpleVals::EvalEQ(GRExprEngine& Eng, Loc L, Loc R) {
+SVal GRSimpleVals::EvalEquality(GRExprEngine& Eng, Loc L, Loc R, bool isEqual) {
   
   BasicValueFactory& BasicVals = Eng.getBasicVals();
 
   switch (L.getSubKind()) {
 
     default:
-      assert(false && "EQ not implemented for this Loc.");
+      assert(false && "EQ/NE not implemented for this Loc.");
       return UnknownVal();
       
     case loc::ConcreteIntKind:
@@ -302,8 +298,21 @@ SVal GRSimpleVals::EvalEQ(GRExprEngine& Eng, Loc L, Loc R) {
         bool b = cast<loc::ConcreteInt>(L).getValue() ==
                  cast<loc::ConcreteInt>(R).getValue();
         
+        // Are we computing '!='?  Flip the result.
+        if (!isEqual)
+          b = !b;
+        
         return NonLoc::MakeIntTruthVal(BasicVals, b);
       }
+      else if (SymbolRef Sym = R.getAsSymbol()) {
+        const SymIntExpr * SE =
+        Eng.getSymbolManager().getSymIntExpr(Sym,
+                                             isEqual ? BinaryOperator::EQ
+                                                     : BinaryOperator::NE,
+                                             cast<loc::ConcreteInt>(L).getValue(),
+                                             Eng.getContext().IntTy);
+        return nonloc::SymExprVal(SE);
+      }
       
       break;
       
@@ -311,7 +320,9 @@ SVal GRSimpleVals::EvalEQ(GRExprEngine& Eng, Loc L, Loc R) {
       if (SymbolRef LSym = L.getAsLocSymbol()) {
         if (isa<loc::ConcreteInt>(R)) {
           const SymIntExpr *SE =
-            Eng.getSymbolManager().getSymIntExpr(LSym, BinaryOperator::EQ,
+            Eng.getSymbolManager().getSymIntExpr(LSym,
+                                           isEqual ? BinaryOperator::EQ
+                                                   : BinaryOperator::NE,
                                            cast<loc::ConcreteInt>(R).getValue(),
                                            Eng.getContext().IntTy);
         
@@ -323,58 +334,10 @@ SVal GRSimpleVals::EvalEQ(GRExprEngine& Eng, Loc L, Loc R) {
     // Fall-through.
       
     case loc::GotoLabelKind:
-      return NonLoc::MakeIntTruthVal(BasicVals, L == R);
-  }
-  
-  return NonLoc::MakeIntTruthVal(BasicVals, false);
-}
-
-SVal GRSimpleVals::EvalNE(GRExprEngine& Eng, Loc L, Loc R) {
-  
-  BasicValueFactory& BasicVals = Eng.getBasicVals();
-
-  switch (L.getSubKind()) {
-
-    default:
-      assert(false && "NE not implemented for this Loc.");
-      return UnknownVal();
-      
-    case loc::ConcreteIntKind:
-      
-      if (isa<loc::ConcreteInt>(R)) {
-        bool b = cast<loc::ConcreteInt>(L).getValue() !=
-                 cast<loc::ConcreteInt>(R).getValue();
-        
-        return NonLoc::MakeIntTruthVal(BasicVals, b);
-      }
-      else if (SymbolRef Sym = R.getAsSymbol()) {
-        const SymIntExpr * SE =
-          Eng.getSymbolManager().getSymIntExpr(Sym, BinaryOperator::NE,
-                                           cast<loc::ConcreteInt>(L).getValue(),
-                                             Eng.getContext().IntTy);
-        return nonloc::SymExprVal(SE);
-      }
-      
-      break;
-
-    case loc::MemRegionKind: {
-      if (SymbolRef LSym = L.getAsLocSymbol()) {
-        if (isa<loc::ConcreteInt>(R)) {
-          const SymIntExpr* SE =
-            Eng.getSymbolManager().getSymIntExpr(LSym, BinaryOperator::NE,
-                                          cast<loc::ConcreteInt>(R).getValue(),
-                                                 Eng.getContext().IntTy);
-          return nonloc::SymExprVal(SE);
-        }
-      }
-      // Fall through:
-    }
-      
-    case loc::GotoLabelKind:
-      return NonLoc::MakeIntTruthVal(BasicVals, L != R);
+      return NonLoc::MakeIntTruthVal(BasicVals, isEqual ? L == R : L != R);
   }
   
-  return NonLoc::MakeIntTruthVal(BasicVals, true);
+  return NonLoc::MakeIntTruthVal(BasicVals, isEqual ? false : true);
 }
 
 //===----------------------------------------------------------------------===//
diff --git a/lib/Analysis/GRSimpleVals.h b/lib/Analysis/GRSimpleVals.h
index efe7c631ab..1258cbc1bf 100644
--- a/lib/Analysis/GRSimpleVals.h
+++ b/lib/Analysis/GRSimpleVals.h
@@ -77,10 +77,8 @@ public:
   
 protected:
   
-  // Equality operators for Locs.
-  
-  SVal EvalEQ(GRExprEngine& Engine, Loc L, Loc R);
-  SVal EvalNE(GRExprEngine& Engine, Loc L, Loc R);
+  // Equality (==, !=) operators for Locs.  
+  SVal EvalEquality(GRExprEngine& Engine, Loc L, Loc R, bool isEqual);
 };
   
 } // end clang namespace
diff --git a/test/Analysis/null-deref-ps.c b/test/Analysis/null-deref-ps.c
index 7f1db8749c..09b9c2ffa3 100644
--- a/test/Analysis/null-deref-ps.c
+++ b/test/Analysis/null-deref-ps.c
@@ -139,9 +139,9 @@ int* f7c(int *x) {
   
   if (((void*)0) != x)
     return x;
-    
-  // THIS IS WRONG.  THIS NEEDS TO BE FIXED.
-  *p = 1; // expected-warning{{null}}
+
+  // If we reach here then 'p' is not null.
+  *p = 1; // no-warning
   return x;
 }
 
-- 
2.40.0