[cfe-commits] r162719 - in /cfe/trunk: include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

[Jordan Rose]

Author: jrose
Date: Mon Aug 27 19:50:45 2012
New Revision: 162719

URL: http://llvm.org/viewvc/llvm-project?rev=162719&view=rev
Log:
[analyzer] Refactor FindLastStoreBRVisitor to not find the store ahead of time.

As Anna pointed out to me offline, it's a little silly to walk backwards through
the graph to find the store site when BugReporter will do the exact same walk
as part of path diagnostic generation.

Modified:
    cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h
    cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h?rev=162719&r1=162718&r2=162719&view=diff
==============================================================================
--- cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h (original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h Mon Aug 27 19:50:45 2012
@@ -100,7 +100,6 @@
   const MemRegion *R;
   SVal V;
   bool satisfied;
-  const ExplodedNode *StoreSite;
 
 public:
   /// \brief Convenience method to create a visitor given only the MemRegion.
@@ -114,7 +113,7 @@
   static void registerStatementVarDecls(BugReport &BR, const Stmt *S);
 
   FindLastStoreBRVisitor(SVal v, const MemRegion *r)
-  : R(r), V(v), satisfied(false), StoreSite(0) {
+  : R(r), V(v), satisfied(false) {
     assert (!V.isUnknown() && "Cannot track unknown value.");
 
     // TODO: Does it make sense to allow undef values here?

Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp?rev=162719&r1=162718&r2=162719&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp Mon Aug 27 19:50:45 2012
@@ -245,79 +245,63 @@
   ID.Add(V);
 }
 
-PathDiagnosticPiece *FindLastStoreBRVisitor::VisitNode(const ExplodedNode *N,
-                                                     const ExplodedNode *PrevN,
-                                                     BugReporterContext &BRC,
-                                                     BugReport &BR) {
+PathDiagnosticPiece *FindLastStoreBRVisitor::VisitNode(const ExplodedNode *Succ,
+                                                       const ExplodedNode *Pred,
+                                                       BugReporterContext &BRC,
+                                                       BugReport &BR) {
 
   if (satisfied)
     return NULL;
 
-  if (!StoreSite) {
-    // Make sure the region is actually bound to value V here.
-    // This is necessary because the region may not actually be live at the
-    // report's error node.
-    if (N->getState()->getSVal(R) != V)
-      return NULL;
-
-    const ExplodedNode *Node = N, *Last = N;
-    const Expr *InitE = 0;
-
-    // Now look for the store of V.
-    for ( ; Node ; Node = Node->getFirstPred()) {
-      if (const VarRegion *VR = dyn_cast<VarRegion>(R)) {
-        if (const PostStmt *P = Node->getLocationAs<PostStmt>())
-          if (const DeclStmt *DS = P->getStmtAs<DeclStmt>())
-            if (DS->getSingleDecl() == VR->getDecl()) {
-              // Record the last seen initialization point.
-              Last = Node;
-              InitE = VR->getDecl()->getInit();
-              break;
-            }
-      }
+  const ExplodedNode *StoreSite = 0;
+  const Expr *InitE = 0;
 
-      // Does the region still bind to value V?  If not, we are done
-      // looking for store sites.
-      SVal Current = Node->getState()->getSVal(R);
-      if (Current != V) {
-        // If this is an assignment expression, we can track the value
-        // being assigned.
-        if (const StmtPoint *SP = Last->getLocationAs<StmtPoint>()) {
-          const Stmt *S = SP->getStmt();
-          if (const BinaryOperator *BO = dyn_cast<BinaryOperator>(S))
-            if (BO->isAssignmentOp())
-              InitE = BO->getRHS();
+  // First see if we reached the declaration of the region.
+  if (const VarRegion *VR = dyn_cast<VarRegion>(R)) {
+    if (const PostStmt *P = Pred->getLocationAs<PostStmt>()) {
+      if (const DeclStmt *DS = P->getStmtAs<DeclStmt>()) {
+        if (DS->getSingleDecl() == VR->getDecl()) {
+          StoreSite = Pred;
+          InitE = VR->getDecl()->getInit();
         }
-
-        break;
       }
-
-      Last = Node;
     }
+  }
 
-    if (!Node) {
-      satisfied = true;
+  // Otherwise, check that Succ has this binding and Pred does not, i.e. this is
+  // where the binding first occurred.
+  if (!StoreSite) {
+    if (Succ->getState()->getSVal(R) != V)
+      return NULL;
+    if (Pred->getState()->getSVal(R) == V)
       return NULL;
-    }
 
-    StoreSite = Last;
+    StoreSite = Succ;
 
-    // If the value that was stored came from an inlined call, make sure we
-    // step into the call.
-    if (InitE) {
-      InitE = InitE->IgnoreParenCasts();
-      ReturnVisitor::addVisitorIfNecessary(StoreSite, InitE, BR);
-    }
+    // If this is an assignment expression, we can track the value
+    // being assigned.
+    if (const PostStmt *P = Succ->getLocationAs<PostStmt>())
+      if (const BinaryOperator *BO = P->getStmtAs<BinaryOperator>())
+        if (BO->isAssignmentOp())
+          InitE = BO->getRHS();
   }
 
-  if (StoreSite != N)
+  if (!StoreSite)
     return NULL;
-
   satisfied = true;
+
+  // If the value that was stored came from an inlined call, make sure we
+  // step into the call.
+  if (InitE) {
+    InitE = InitE->IgnoreParenCasts();
+    ReturnVisitor::addVisitorIfNecessary(StoreSite, InitE, BR);
+  }
+
+  // Okay, we've found the binding. Emit an appropriate message.
   SmallString<256> sbuf;
   llvm::raw_svector_ostream os(sbuf);
 
-  if (const PostStmt *PS = N->getLocationAs<PostStmt>()) {
+  if (const PostStmt *PS = StoreSite->getLocationAs<PostStmt>()) {
     if (const DeclStmt *DS = PS->getStmtAs<DeclStmt>()) {
 
       if (const VarRegion *VR = dyn_cast<VarRegion>(R)) {
@@ -391,7 +375,7 @@
   }
 
   // Construct a new PathDiagnosticPiece.
-  ProgramPoint P = N->getLocation();
+  ProgramPoint P = StoreSite->getLocation();
   PathDiagnosticLocation L =
     PathDiagnosticLocation::create(P, BRC.getSourceManager());
   if (!L.isValid())