Refactor the check for bad divide into a checker.

Also fix a checker context bug: the Dst set is not always empty initially. 
Because in GRExprEngine::CheckerVisit(), *CurrSet is used repeatedly. 
So we removed the Dst.empty() condition in ~CheckerContext() when deciding
whether to do autotransision.

llvm-svn: 80786

3 files changed, 76 insertions, 75 deletions

diff --git a/clang/lib/Analysis/BugReporterVisitors.cpp b/clang/lib/Analysis/BugReporterVisitors.cpp
index fce31e70f0b..8b3502805d8 100644
--- a/clang/lib/Analysis/BugReporterVisitors.cpp
+++ b/clang/lib/Analysis/BugReporterVisitors.cpp
@@ -55,7 +55,7 @@ clang::bugreporter::GetReceiverExpr(const ExplodedNode *N){
 
 const Stmt*
 clang::bugreporter::GetDenomExpr(const ExplodedNode *N) {
-  const Stmt *S = N->getLocationAs<PostStmt>()->getStmt();
+  const Stmt *S = N->getLocationAs<PreStmt>()->getStmt();
   if (const BinaryOperator *BE = dyn_cast<BinaryOperator>(S))
     return BE->getRHS();
   return NULL;
diff --git a/clang/lib/Analysis/GRExprEngine.cpp b/clang/lib/Analysis/GRExprEngine.cpp
index a6b580b865a..a36916ba2eb 100644
--- a/clang/lib/Analysis/GRExprEngine.cpp
+++ b/clang/lib/Analysis/GRExprEngine.cpp
@@ -2700,44 +2700,6 @@ void GRExprEngine::VisitReturnStmt(ReturnStmt* S, ExplodedNode* Pred, ExplodedNo
 // Transfer functions: Binary operators.
 //===----------------------------------------------------------------------===//
 
-const GRState* GRExprEngine::CheckDivideZero(Expr* Ex, const GRState* state,
-                                             ExplodedNode* Pred, SVal Denom) {
-  
-  // Divide by undefined? (potentially zero)
-  
-  if (Denom.isUndef()) {
-    ExplodedNode* DivUndef = Builder->generateNode(Ex, state, Pred);
-    
-    if (DivUndef) {
-      DivUndef->markAsSink();
-      ExplicitBadDivides.insert(DivUndef);
-    }
-    
-    return 0;
-  }
-  
-  // Check for divide/remainder-by-zero.
-  // First, "assume" that the denominator is 0 or undefined.            
-  const GRState* zeroState =  state->assume(Denom, false);
-  
-  // Second, "assume" that the denominator cannot be 0.            
-  state = state->assume(Denom, true);
-  
-  // Create the node for the divide-by-zero (if it occurred).  
-  if (zeroState)
-    if (ExplodedNode* DivZeroNode = Builder->generateNode(Ex, zeroState, Pred)) {
-      DivZeroNode->markAsSink();
-      
-      if (state)
-        ImplicitBadDivides.insert(DivZeroNode);
-      else
-        ExplicitBadDivides.insert(DivZeroNode);
-      
-    }
-  
-  return state;
-}
-
 void GRExprEngine::VisitBinaryOperator(BinaryOperator* B,
                                        ExplodedNode* Pred,
                                        ExplodedNodeSet& Dst) {
@@ -2765,10 +2727,14 @@ void GRExprEngine::VisitBinaryOperator(BinaryOperator* B,
     
     ExplodedNodeSet Tmp2;
     Visit(RHS, *I1, Tmp2);
+
+    ExplodedNodeSet CheckedSet;
+    CheckerVisit(B, CheckedSet, Tmp2, true);
     
     // With both the LHS and RHS evaluated, process the operation itself.
     
-    for (ExplodedNodeSet::iterator I2=Tmp2.begin(), E2=Tmp2.end(); I2 != E2; ++I2) {
+    for (ExplodedNodeSet::iterator I2=CheckedSet.begin(), E2=CheckedSet.end(); 
+         I2 != E2; ++I2) {
 
       const GRState* state = GetState(*I2);
       const GRState* OldSt = state;
@@ -2799,17 +2765,6 @@ void GRExprEngine::VisitBinaryOperator(BinaryOperator* B,
           continue;
         }
           
-        case BinaryOperator::Div:
-        case BinaryOperator::Rem:
-          
-          // Special checking for integer denominators.          
-          if (RHS->getType()->isIntegerType() && 
-              RHS->getType()->isScalarType()) {
-            
-            state = CheckDivideZero(B, state, *I2, RightV);
-            if (!state) continue;
-          }
-          
           // FALL-THROUGH.
 
         default: {
@@ -2875,24 +2830,12 @@ void GRExprEngine::VisitBinaryOperator(BinaryOperator* B,
       SVal location = state->getSVal(LHS);
       EvalLoad(Tmp3, LHS, *I2, state, location);
       
-      for (ExplodedNodeSet::iterator I3=Tmp3.begin(), E3=Tmp3.end(); I3!=E3; ++I3) {
+      for (ExplodedNodeSet::iterator I3=Tmp3.begin(), E3=Tmp3.end(); I3!=E3; 
+           ++I3) {
         
         state = GetState(*I3);
         SVal V = state->getSVal(LHS);
 
-        // Check for divide-by-zero.
-        if ((Op == BinaryOperator::Div || Op == BinaryOperator::Rem)
-            && RHS->getType()->isIntegerType()
-            && RHS->getType()->isScalarType()) {
-          
-          // CheckDivideZero returns a new state where the denominator
-          // is assumed to be non-zero.
-          state = CheckDivideZero(B, state, *I3, RightV);
-          
-          if (!state)
-            continue;
-        }
-        
         // Propagate undefined values (left-side).          
         if (V.isUndef()) {
           EvalStore(Dst, B, LHS, *I3, state->BindExpr(B, V), 
diff --git a/clang/lib/Analysis/GRExprEngineInternalChecks.cpp b/clang/lib/Analysis/GRExprEngineInternalChecks.cpp
index 4c6371c44c7..28e763376a0 100644
--- a/clang/lib/Analysis/GRExprEngineInternalChecks.cpp
+++ b/clang/lib/Analysis/GRExprEngineInternalChecks.cpp
@@ -193,14 +193,10 @@ public:
 
 class VISIBILITY_HIDDEN DivZero : public BuiltinBug {
 public:
-  DivZero(GRExprEngine* eng)
+  DivZero(GRExprEngine* eng = 0)
     : BuiltinBug(eng,"Division-by-zero",
                  "Division by zero or undefined value.") {}
   
-  void FlushReportsImpl(BugReporter& BR, GRExprEngine& Eng) {
-    Emit(BR, Eng.explicit_bad_divides_begin(), Eng.explicit_bad_divides_end());
-  }
-  
   void registerInitialVisitors(BugReporterContext& BRC,
                                const ExplodedNode* N,
                                BuiltinBugReport *R) {
@@ -575,7 +571,7 @@ public:
       if (stateNull && !stateNotNull) {
         // Generate an error node.  Check for a null node in case
         // we cache out.
-        if (ExplodedNode *errorNode = C.generateNode(CE, stateNull, true)) {
+        if (ExplodedNode *errorNode = C.GenerateNode(CE, stateNull, true)) {
                   
           // Lazily allocate the BugType object if it hasn't already been
           // created. Ownership is transferred to the BugReporter object once
@@ -611,7 +607,7 @@ public:
     // If we reach here all of the arguments passed the nonnull check.
     // If 'state' has been updated generated a new node.
     if (state != originalState)
-      C.addTransition(C.generateNode(CE, state));
+      C.addTransition(C.GenerateNode(CE, state));
   }
 };
 } // end anonymous namespace
@@ -639,7 +635,7 @@ void CheckUndefinedArg::PreVisitCallExpr(CheckerContext &C, const CallExpr *CE){
   for (CallExpr::const_arg_iterator I = CE->arg_begin(), E = CE->arg_end();
        I != E; ++I) {
     if (C.getState()->getSVal(*I).isUndef()) {
-      if (ExplodedNode *ErrorNode = C.generateNode(CE, C.getState(), true)) {
+      if (ExplodedNode *ErrorNode = C.GenerateNode(CE, true)) {
         if (!BT)
           BT = new BadArg();
         // Generate a report for this bug.
@@ -672,7 +668,7 @@ void CheckBadCall::PreVisitCallExpr(CheckerContext &C, const CallExpr *CE) {
   SVal L = C.getState()->getSVal(Callee);
 
   if (L.isUndef() || isa<loc::ConcreteInt>(L)) {
-    if (ExplodedNode *N = C.generateNode(CE, C.getState(), true)) {
+    if (ExplodedNode *N = C.GenerateNode(CE, true)) {
       if (!BT)
         BT = new BadCall();
       C.EmitReport(new BuiltinBugReport(*BT, BT->getDescription().c_str(), N));
@@ -680,6 +676,68 @@ void CheckBadCall::PreVisitCallExpr(CheckerContext &C, const CallExpr *CE) {
   }
 }
 
+class VISIBILITY_HIDDEN CheckBadDiv : public CheckerVisitor<CheckBadDiv> {
+  DivZero *BT;
+public:
+  CheckBadDiv() : BT(0) {}
+  ~CheckBadDiv() {}
+
+  const void *getTag() {
+    static int x;
+    return &x;
+  }
+
+  void PreVisitBinaryOperator(CheckerContext &C, const BinaryOperator *B);
+};
+
+void CheckBadDiv::PreVisitBinaryOperator(CheckerContext &C, 
+                                         const BinaryOperator *B) {
+  BinaryOperator::Opcode Op = B->getOpcode();
+  if (Op != BinaryOperator::Div &&
+      Op != BinaryOperator::Rem &&
+      Op != BinaryOperator::DivAssign &&
+      Op != BinaryOperator::RemAssign)
+    return;
+
+  if (!B->getRHS()->getType()->isIntegerType() ||
+      !B->getRHS()->getType()->isScalarType())
+    return;
+
+  printf("should not check!\n");
+
+  // Check for divide by undefined.
+  SVal Denom = C.getState()->getSVal(B->getRHS());
+
+  if (Denom.isUndef()) {
+    if (ExplodedNode *N = C.GenerateNode(B, true)) {
+      if (!BT)
+        BT = new DivZero();
+
+      C.EmitReport(new BuiltinBugReport(*BT, BT->getDescription().c_str(), N));
+    }
+    return;
+  }
+
+  // Check for divide by zero.
+  ConstraintManager &CM = C.getConstraintManager();
+  const GRState *stateNotZero, *stateZero;
+  llvm::tie(stateNotZero, stateZero) = CM.AssumeDual(C.getState(), 
+                                                     cast<DefinedSVal>(Denom));
+  
+  if (stateZero && !stateNotZero) {
+    if (ExplodedNode *N = C.GenerateNode(B, stateZero, true)) {
+      if (!BT)
+        BT = new DivZero();
+   
+      C.EmitReport(new BuiltinBugReport(*BT, BT->getDescription().c_str(), N));
+    }
+    return;
+  }
+
+  // If we get here, then the denom should not be zero.
+  if (stateNotZero != C.getState())
+    C.addTransition(C.GenerateNode(B, stateNotZero));
+}
 }
 //===----------------------------------------------------------------------===//
 // Check registration.
@@ -694,7 +752,6 @@ void GRExprEngine::RegisterInternalChecks() {
   BR.Register(new NullDeref(this));
   BR.Register(new UndefinedDeref(this));
   BR.Register(new UndefBranch(this));
-  BR.Register(new DivZero(this));
   BR.Register(new UndefResult(this));
   BR.Register(new RetStack(this));
   BR.Register(new RetUndef(this));
@@ -713,4 +770,5 @@ void GRExprEngine::RegisterInternalChecks() {
   registerCheck(new CheckAttrNonNull());
   registerCheck(new CheckUndefinedArg());
   registerCheck(new CheckBadCall());
+  registerCheck(new CheckBadDiv());
 }