Introduce caching of diagnostics in BugReporter. This provides extra

pruning of diagnostics that may be emitted multiple times.  This is
accomplished by adding FoldingSet profiling support to PathDiagnostic,
and then having BugReporter record what diagnostics have been issued.

This was motived to a serious bug introduced by moving the
'divide-by-zero' checking outside of GRExprEngine into a separate
'Checker' class.  When analyzing code using the '-fobjc-gc' option, a
given function would be analyzed twice, but the second time various
"internal checks" would be disabled to avoid emitting multiple
diagnostics (e.g., "null dereference") for the same issue.  The
problem is that such checks also effect path pruning and don't just
emit diagnostics.  This resulted in an assertion failure involving a
real divide-by-zero in some analyzed code where we would get an
assertion failure in APInt because the 'DivZero' check was disabled
and didn't prune the logic that resulted in the divide-by-zero in the
analyzer.

The implemented solution is somewhat of a hack, and may not perform
extremely well.  This will need to be cleaned up over time.

As a regression test, 'misc-ps.m' has been modified so that its tests
are run using -fobjc-gc to test this diagnostic pruning behavior.

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

diff --git a/include/clang/Analysis/PathDiagnostic.h b/include/clang/Analysis/PathDiagnostic.h
index 809c83161f3efae63a0312cd97b405ef88a8d969..0635a01d7b37387992c3aa17d73ac0baa3a5e2da 100644 (file)
--- a/include/clang/Analysis/PathDiagnostic.h
+++ b/include/clang/Analysis/PathDiagnostic.h
@@ -17,6 +17,7 @@
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/Diagnostic.h"
 #include "llvm/ADT/OwningPtr.h"
+#include "llvm/ADT/FoldingSet.h"
 
 #include <vector>
 #include <deque>
@@ -24,12 +25,17 @@
 #include <algorithm>
 
 namespace clang {
-
+    
+class Stmt;
+class Decl;
+class Preprocessor;
+  
 //===----------------------------------------------------------------------===//
 // High-level interface for handlers of path-sensitive diagnostics.
 //===----------------------------------------------------------------------===//
 
 class PathDiagnostic;
+    
 class Stmt;
 class Decl;
 class Preprocessor;
@@ -38,12 +44,9 @@ class PathDiagnosticClient : public DiagnosticClient  {
 public:
   PathDiagnosticClient() {}
   virtual ~PathDiagnosticClient() {}
-
   virtual void SetPreprocessor(Preprocessor *PP) {}
-
   virtual void HandleDiagnostic(Diagnostic::Level DiagLevel,
                                 const DiagnosticInfo &Info);
-
   virtual void HandlePathDiagnostic(const PathDiagnostic* D) = 0;
 
   enum PathGenerationScheme { Minimal, Extensive };
@@ -125,6 +128,8 @@ public:
   void flatten();
 
   const SourceManager& getManager() const { assert(isValid()); return *SM; }
+  
+  void Profile(llvm::FoldingSetNodeID &ID) const;
 };
 
 class PathDiagnosticLocationPair {
@@ -142,6 +147,11 @@ public:
     Start.flatten();
     End.flatten();
   }
+  
+  void Profile(llvm::FoldingSetNodeID &ID) const {
+    Start.Profile(ID);
+    End.Profile(ID);
+  }
 };
 
 //===----------------------------------------------------------------------===//
@@ -220,6 +230,8 @@ public:
   static inline bool classof(const PathDiagnosticPiece* P) {
     return true;
   }
+  
+  virtual void Profile(llvm::FoldingSetNodeID &ID) const;
 };
 
 class PathDiagnosticSpotPiece : public PathDiagnosticPiece {
@@ -238,6 +250,8 @@ public:
 
   PathDiagnosticLocation getLocation() const { return Pos; }
   virtual void flattenLocations() { Pos.flatten(); }
+  
+  virtual void Profile(llvm::FoldingSetNodeID &ID) const;
 };
 
 class PathDiagnosticEventPiece : public PathDiagnosticSpotPiece {
@@ -317,6 +331,8 @@ public:
   static inline bool classof(const PathDiagnosticPiece* P) {
     return P->getKind() == ControlFlow;
   }
+  
+  virtual void Profile(llvm::FoldingSetNodeID &ID) const;
 };
 
 class PathDiagnosticMacroPiece : public PathDiagnosticSpotPiece {
@@ -347,12 +363,14 @@ public:
   static inline bool classof(const PathDiagnosticPiece* P) {
     return P->getKind() == Macro;
   }
+  
+  virtual void Profile(llvm::FoldingSetNodeID &ID) const;
 };
 
 /// PathDiagnostic - PathDiagnostic objects represent a single path-sensitive
 ///  diagnostic.  It represents an ordered-collection of PathDiagnosticPieces,
 ///  each which represent the pieces of the path.
-class PathDiagnostic {
+class PathDiagnostic : public llvm::FoldingSetNode {
   std::deque<PathDiagnosticPiece*> path;
   unsigned Size;
   std::string BugType;
@@ -386,11 +404,13 @@ public:
   }
 
   void push_front(PathDiagnosticPiece* piece) {
+    assert(piece);
     path.push_front(piece);
     ++Size;
   }
 
   void push_back(PathDiagnosticPiece* piece) {
+    assert(piece);
     path.push_back(piece);
     ++Size;
   }
@@ -453,7 +473,7 @@ public:
     bool operator==(const const_iterator& X) const { return I == X.I; }
     bool operator!=(const const_iterator& X) const { return I != X.I; }
 
-    reference operator*() const { return **I; }
+    reference operator*() const { assert(*I); return **I; }
     pointer operator->() const { return *I; }
 
     const_iterator& operator++() { ++I; return *this; }
@@ -480,8 +500,8 @@ public:
   void flattenLocations() {
     for (iterator I = begin(), E = end(); I != E; ++I) I->flattenLocations();
   }
-};
-
-
+  
+  void Profile(llvm::FoldingSetNodeID &ID) const;
+};  
 } //end clang namespace
 #endif

diff --git a/lib/Analysis/BugReporter.cpp b/lib/Analysis/BugReporter.cpp
index 38e982888ed4fe1246cb907c0b4740606151aedc..064fff47f4ac399a9f15a99dd4d663e7f489121f 100644 (file)
--- a/lib/Analysis/BugReporter.cpp
+++ b/lib/Analysis/BugReporter.cpp
@@ -1732,6 +1732,50 @@ static BugReport *FindReportInEquivalenceClass(BugReportEquivClass& EQ) {
   return NULL;
 }
 
+
+//===----------------------------------------------------------------------===//
+// DiagnosticCache.  This is a hack to cache analyzer diagnostics.  It
+// uses global state, which eventually should go elsewhere.
+//===----------------------------------------------------------------------===//
+namespace {
+class VISIBILITY_HIDDEN DiagCacheItem : public llvm::FoldingSetNode {
+  llvm::FoldingSetNodeID ID;
+public:
+  DiagCacheItem(BugReport *R, PathDiagnostic *PD) {
+    ID.AddString(R->getBugType().getName());
+    ID.AddString(R->getBugType().getCategory());
+    ID.AddString(R->getDescription());
+    ID.AddInteger(R->getLocation().getRawEncoding());
+    PD->Profile(ID);    
+  }
+  
+  void Profile(llvm::FoldingSetNodeID &id) {
+    id = ID;
+  }
+  
+  llvm::FoldingSetNodeID &getID() { return ID; }
+};
+}
+
+static bool IsCachedDiagnostic(BugReport *R, PathDiagnostic *PD) {
+  // FIXME: Eventually this diagnostic cache should reside in something
+  // like AnalysisManager instead of being a static variable.  This is
+  // really unsafe in the long term.
+  typedef llvm::FoldingSet<DiagCacheItem> DiagnosticCache;
+  static DiagnosticCache DC;
+  
+  void *InsertPos;
+  DiagCacheItem *Item = new DiagCacheItem(R, PD);
+  
+  if (DC.FindNodeOrInsertPos(Item->getID(), InsertPos)) {
+    delete Item;
+    return true;
+  }
+  
+  DC.InsertNode(Item, InsertPos);
+  return false;
+}
+
 void BugReporter::FlushReport(BugReportEquivClass& EQ) {
   BugReport *R = FindReportInEquivalenceClass(EQ);
 
@@ -1752,6 +1796,9 @@ void BugReporter::FlushReport(BugReportEquivClass& EQ) {
 
   GeneratePathDiagnostic(*D.get(), EQ);
 
+  if (IsCachedDiagnostic(R, D.get()))
+    return;
+  
   // Get the meta data.
   std::pair<const char**, const char**> Meta = R->getExtraDescriptiveText();
   for (const char** s = Meta.first; s != Meta.second; ++s)

diff --git a/lib/Analysis/PathDiagnostic.cpp b/lib/Analysis/PathDiagnostic.cpp
index ceead6a3ac2d4fb09c278bbf98152683432d3ffa..800496a1614292a35c37bdd6585c31f36d58ba21 100644 (file)
--- a/lib/Analysis/PathDiagnostic.cpp
+++ b/lib/Analysis/PathDiagnostic.cpp
@@ -239,4 +239,66 @@ void PathDiagnosticLocation::flatten() {
   }
 }
 
+//===----------------------------------------------------------------------===//
+// FoldingSet profiling methods.
+//===----------------------------------------------------------------------===//
+
+void PathDiagnosticLocation::Profile(llvm::FoldingSetNodeID &ID) const {
+  ID.AddInteger((unsigned) K);
+  switch (K) {
+    case RangeK:
+      ID.AddInteger(R.getBegin().getRawEncoding());
+      ID.AddInteger(R.getEnd().getRawEncoding());
+      break;      
+    case SingleLocK:
+      ID.AddInteger(R.getBegin().getRawEncoding());
+      break;
+    case StmtK:
+      ID.Add(S);
+      break;
+    case DeclK:
+      ID.Add(D);
+      break;
+  }
+  return;
+}
+
+void PathDiagnosticPiece::Profile(llvm::FoldingSetNodeID &ID) const {
+  ID.AddInteger((unsigned) getKind());
+  ID.AddString(str);
+  // FIXME: Add profiling support for code hints.
+  ID.AddInteger((unsigned) getDisplayHint());
+  for (range_iterator I = ranges_begin(), E = ranges_end(); I != E; ++I) {
+    ID.AddInteger(I->getBegin().getRawEncoding());
+    ID.AddInteger(I->getEnd().getRawEncoding());
+  }  
+}
+
+void PathDiagnosticSpotPiece::Profile(llvm::FoldingSetNodeID &ID) const {
+  PathDiagnosticPiece::Profile(ID);
+  ID.Add(Pos);
+}
 
+void PathDiagnosticControlFlowPiece::Profile(llvm::FoldingSetNodeID &ID) const {
+  PathDiagnosticPiece::Profile(ID);
+  for (const_iterator I = begin(), E = end(); I != E; ++I)
+    ID.Add(*I);
+}
+
+void PathDiagnosticMacroPiece::Profile(llvm::FoldingSetNodeID &ID) const {
+  PathDiagnosticSpotPiece::Profile(ID);
+  for (const_iterator I = begin(), E = end(); I != E; ++I)
+    ID.Add(**I);
+}
+
+void PathDiagnostic::Profile(llvm::FoldingSetNodeID &ID) const {
+  ID.AddInteger(Size);
+  ID.AddString(BugType);
+  ID.AddString(Desc);
+  ID.AddString(Category);
+  for (const_iterator I = begin(), E = end(); I != E; ++I)
+    ID.Add(*I);
+  
+  for (meta_iterator I = meta_begin(), E = meta_end(); I != E; ++I)
+    ID.AddString(*I);
+}

diff --git a/lib/Frontend/AnalysisConsumer.cpp b/lib/Frontend/AnalysisConsumer.cpp
index 034ff93f412601bec4df493c89cf5d2eaa95e35f..0e25de4ff87e0ea2c52a9c96034313d6e356157d 100644 (file)
--- a/lib/Frontend/AnalysisConsumer.cpp
+++ b/lib/Frontend/AnalysisConsumer.cpp
@@ -293,8 +293,7 @@ static void ActionWarnUninitVals(AnalysisManager& mgr, Decl *D) {
 
 
 static void ActionGRExprEngine(AnalysisManager& mgr, Decl *D, 
-                               GRTransferFuncs* tf,
-                               bool StandardWarnings = true) {
+                               GRTransferFuncs* tf) {
 
 
   llvm::OwningPtr<GRTransferFuncs> TF(tf);
@@ -303,17 +302,13 @@ static void ActionGRExprEngine(AnalysisManager& mgr, Decl *D,
   mgr.DisplayFunction(D);
 
   // Construct the analysis engine.
-  LiveVariables* L = mgr.getLiveVariables(D);
-  if (!L) return;
-
   GRExprEngine Eng(mgr);
 
   Eng.setTransferFunctions(tf);
+  Eng.RegisterInternalChecks(); // FIXME: Internal checks should just
+                                // automatically register.
+  RegisterAppleChecks(Eng, *D);
 
-  if (StandardWarnings) {
-    Eng.RegisterInternalChecks();
-    RegisterAppleChecks(Eng, *D);
-  }
 
   // Set the graph auditor.
   llvm::OwningPtr<ExplodedNode::Auditor> Auditor;
@@ -338,13 +333,13 @@ static void ActionGRExprEngine(AnalysisManager& mgr, Decl *D,
 }
 
 static void ActionCheckerCFRefAux(AnalysisManager& mgr, Decl *D,
-                                  bool GCEnabled, bool StandardWarnings) {
+                                  bool GCEnabled) {
 
   GRTransferFuncs* TF = MakeCFRefCountTF(mgr.getASTContext(),
                                          GCEnabled,
                                          mgr.getLangOptions());
 
-  ActionGRExprEngine(mgr, D, TF, StandardWarnings);
+  ActionGRExprEngine(mgr, D, TF);
 }
 
 static void ActionCheckerCFRef(AnalysisManager& mgr, Decl *D) {
@@ -353,16 +348,16 @@ static void ActionCheckerCFRef(AnalysisManager& mgr, Decl *D) {
    default:
      assert (false && "Invalid GC mode.");
    case LangOptions::NonGC:
-     ActionCheckerCFRefAux(mgr, D, false, true);
+     ActionCheckerCFRefAux(mgr, D, false);
      break;
 
    case LangOptions::GCOnly:
-     ActionCheckerCFRefAux(mgr, D, true, true);
+     ActionCheckerCFRefAux(mgr, D, true);
      break;
 
    case LangOptions::HybridGC:
-     ActionCheckerCFRefAux(mgr, D, false, true);
-     ActionCheckerCFRefAux(mgr, D, true, false);
+     ActionCheckerCFRefAux(mgr, D, false);
+     ActionCheckerCFRefAux(mgr, D, true);
      break;
  }
 }

diff --git a/test/Analysis/misc-ps.m b/test/Analysis/misc-ps.m
index 189fc44878c42c5d37330f2318f32bf3177fd61d..d7c3db7439d35bc9f74715253c069618367ea60f 100644 (file)
--- a/test/Analysis/misc-ps.m
+++ b/test/Analysis/misc-ps.m
@@ -1,4 +1,5 @@
-// RUN: clang-cc -analyze -checker-cfref --analyzer-store=basic -analyzer-constraints=basic --verify -fblocks %s &&
+// NOTE: Use '-fobjc-gc' to test the analysis being run twice, and multiple reports are not issued.
+// RUN: clang-cc -analyze -checker-cfref --analyzer-store=basic -fobjc-gc -analyzer-constraints=basic --verify -fblocks %s &&
 // RUN: clang-cc -analyze -checker-cfref --analyzer-store=basic -analyzer-constraints=range --verify -fblocks %s &&
 // RUN: clang-cc -analyze -checker-cfref --analyzer-store=region -analyzer-constraints=basic --verify -fblocks %s &&
 // RUN: clang-cc -analyze -checker-cfref --analyzer-store=region -analyzer-constraints=range --verify -fblocks %s