]> granicus.if.org Git - clang/commitdiff
[analyzer] Restart path diagnostic generation if any of the visitors change the repor...
authorJordy Rose <jediknil@belkadan.com>
Sat, 24 Mar 2012 03:03:29 +0000 (03:03 +0000)
committerJordy Rose <jediknil@belkadan.com>
Sat, 24 Mar 2012 03:03:29 +0000 (03:03 +0000)
This required adding a change count token to BugReport, but also allowed us to ditch ImmutableList as the BugReporterVisitor data type.

Also, remove the hack from MallocChecker, now that visitors appear in the opposite order. This is not exactly a fix, but the common case -- custom diagnostics after generic ones -- is now the default behavior.

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

include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
lib/StaticAnalyzer/Core/BugReporter.cpp

index b607fe8b2fc9ccf6be1d6c63544f53483f47e793..57ffea99dd6f3e96af13ebf5f20f70a3d30b319b 100644 (file)
@@ -61,7 +61,8 @@ public:
   };
 
   typedef const SourceRange *ranges_iterator;
-  typedef llvm::ImmutableList<BugReporterVisitor*>::iterator visitor_iterator;
+  typedef SmallVector<BugReporterVisitor *, 8> VisitorList;
+  typedef VisitorList::iterator visitor_iterator;
   typedef SmallVector<StringRef, 2> ExtraTextList;
 
 protected:
@@ -90,25 +91,30 @@ protected:
   /// to include when constructing the final path diagnostic.
   Regions interestingRegions;
 
-  // Not the most efficient data structure, but we use an ImmutableList for the
-  // Callbacks because it is safe to make additions to list during iteration.
-  llvm::ImmutableList<BugReporterVisitor*>::Factory F;
-  llvm::ImmutableList<BugReporterVisitor*> Callbacks;
+  /// A set of custom visitors which generate "event" diagnostics at
+  /// interesting points in the path.
+  VisitorList Callbacks;
+
+  /// Used for ensuring the visitors are only added once.
   llvm::FoldingSet<BugReporterVisitor> CallbacksSet;
 
+  /// Used for clients to tell if the report's configuration has changed
+  /// since the last time they checked.
+  unsigned ConfigurationChangeToken;
+
 public:
   BugReport(BugType& bt, StringRef desc, const ExplodedNode *errornode)
     : BT(bt), Description(desc), ErrorNode(errornode),
-      Callbacks(F.getEmptyList()) {}
+      ConfigurationChangeToken(0) {}
 
   BugReport(BugType& bt, StringRef shortDesc, StringRef desc,
             const ExplodedNode *errornode)
     : BT(bt), ShortDescription(shortDesc), Description(desc),
-      ErrorNode(errornode), Callbacks(F.getEmptyList()) {}
+      ErrorNode(errornode), ConfigurationChangeToken(0) {}
 
   BugReport(BugType& bt, StringRef desc, PathDiagnosticLocation l)
     : BT(bt), Description(desc), Location(l), ErrorNode(0),
-      Callbacks(F.getEmptyList()) {}
+      ConfigurationChangeToken(0) {}
 
   /// \brief Create a BugReport with a custom uniqueing location.
   ///
@@ -120,7 +126,7 @@ public:
   BugReport(BugType& bt, StringRef desc, const ExplodedNode *errornode,
             PathDiagnosticLocation LocationToUnique)
     : BT(bt), Description(desc), UniqueingLocation(LocationToUnique),
-      ErrorNode(errornode), Callbacks(F.getEmptyList()) {}
+      ErrorNode(errornode), ConfigurationChangeToken(0) {}
 
   virtual ~BugReport();
 
@@ -142,6 +148,10 @@ public:
   bool isInteresting(SymbolRef sym) const;
   bool isInteresting(const MemRegion *R) const;
   bool isInteresting(SVal V) const;
+
+  unsigned getConfigurationChangeToken() const {
+    return ConfigurationChangeToken;
+  }
   
   /// \brief This allows for addition of meta data to the diagnostic.
   ///
index 8f4502ca31942d5903b1b8bd3e249d770b11643b..f5218746befc4429eb48996f2ddb6b3356821028 100644 (file)
@@ -875,10 +875,6 @@ void MallocChecker::reportLeak(SymbolRef Sym, ExplodedNode *N,
 
   BugReport *R = new BugReport(*BT_Leak, os.str(), N, LocUsedForUniqueing);
   R->markInteresting(Sym);
-  // FIXME: This is a hack to make sure the MallocBugVisitor gets to look at
-  // the ExplodedNode chain first, in order to mark any failed realloc symbols
-  // as interesting for ConditionBRVisitor.
-  R->addVisitor(new ConditionBRVisitor());
   R->addVisitor(new MallocBugVisitor(Sym));
   C.EmitReport(R);
 }
index 67dfcf51d26e88fa886c3d251909d15080239ccd..643469d7e5687e65a3b3f3e382696a70b7a0d758 100644 (file)
@@ -410,7 +410,8 @@ static void CompactPathDiagnostic(PathPieces &path, const SourceManager& SM);
 
 static void GenerateMinimalPathDiagnostic(PathDiagnostic& PD,
                                           PathDiagnosticBuilder &PDB,
-                                          const ExplodedNode *N) {
+                                          const ExplodedNode *N,
+                                      ArrayRef<BugReporterVisitor *> visitors) {
 
   SourceManager& SMgr = PDB.getSourceManager();
   const LocationContext *LC = PDB.LC;
@@ -712,8 +713,9 @@ static void GenerateMinimalPathDiagnostic(PathDiagnostic& PD,
     if (NextNode) {
       // Add diagnostic pieces from custom visitors.
       BugReport *R = PDB.getBugReport();
-      for (BugReport::visitor_iterator I = R->visitor_begin(),
-           E = R->visitor_end(); I!=E; ++I) {
+      for (ArrayRef<BugReporterVisitor *>::iterator I = visitors.begin(),
+                                                    E = visitors.end();
+           I != E; ++I) {
         if (PathDiagnosticPiece *p = (*I)->VisitNode(N, NextNode, PDB, *R)) {
           PD.getActivePath().push_front(p);
           updateStackPiecesWithMessage(p, CallStack);
@@ -1051,7 +1053,8 @@ void EdgeBuilder::addContext(const Stmt *S) {
 
 static void GenerateExtensivePathDiagnostic(PathDiagnostic& PD,
                                             PathDiagnosticBuilder &PDB,
-                                            const ExplodedNode *N) {
+                                            const ExplodedNode *N,
+                                      ArrayRef<BugReporterVisitor *> visitors) {
   EdgeBuilder EB(PD, PDB);
   const SourceManager& SM = PDB.getSourceManager();
   StackDiagVector CallStack;
@@ -1183,8 +1186,9 @@ static void GenerateExtensivePathDiagnostic(PathDiagnostic& PD,
 
     // Add pieces from custom visitors.
     BugReport *R = PDB.getBugReport();
-    for (BugReport::visitor_iterator I = R->visitor_begin(),
-                                     E = R->visitor_end(); I!=E; ++I) {
+    for (ArrayRef<BugReporterVisitor *>::iterator I = visitors.begin(),
+                                                  E = visitors.end();
+         I != E; ++I) {
       if (PathDiagnosticPiece *p = (*I)->VisitNode(N, NextNode, PDB, *R)) {
         const PathDiagnosticLocation &Loc = p->getLocation();
         EB.addEdge(Loc, true);
@@ -1227,7 +1231,8 @@ void BugReport::addVisitor(BugReporterVisitor* visitor) {
   }
 
   CallbacksSet.InsertNode(visitor, InsertPos);
-  Callbacks = F.add(visitor, Callbacks);
+  Callbacks.push_back(visitor);
+  ++ConfigurationChangeToken;
 }
 
 BugReport::~BugReport() {
@@ -1261,7 +1266,10 @@ void BugReport::Profile(llvm::FoldingSetNodeID& hash) const {
 void BugReport::markInteresting(SymbolRef sym) {
   if (!sym)
     return;
-  interestingSymbols.insert(sym);  
+
+  // If the symbol wasn't already in our set, note a configuration change.
+  if (interestingSymbols.insert(sym).second)
+    ++ConfigurationChangeToken;
 
   if (const SymbolMetadata *meta = dyn_cast<SymbolMetadata>(sym))
     interestingRegions.insert(meta->getRegion());
@@ -1270,8 +1278,11 @@ void BugReport::markInteresting(SymbolRef sym) {
 void BugReport::markInteresting(const MemRegion *R) {
   if (!R)
     return;
+
+  // If the base region wasn't already in our set, note a configuration change.
   R = R->getBaseRegion();
-  interestingRegions.insert(R);
+  if (interestingRegions.insert(R).second)
+    ++ConfigurationChangeToken;
 
   if (const SymbolicRegion *SR = dyn_cast<SymbolicRegion>(R))
     interestingSymbols.insert(SR->getSymbol());
@@ -1696,33 +1707,57 @@ void GRBugReporter::GeneratePathDiagnostic(PathDiagnostic& PD,
   R->addVisitor(new NilReceiverBRVisitor());
   R->addVisitor(new ConditionBRVisitor());
 
-  // Generate the very last diagnostic piece - the piece is visible before 
-  // the trace is expanded.
-  PathDiagnosticPiece *LastPiece = 0;
-  for (BugReport::visitor_iterator I = R->visitor_begin(),
-                                   E = R->visitor_end(); I!=E; ++I) {
-    if (PathDiagnosticPiece *Piece = (*I)->getEndPath(PDB, N, *R)) {
-      assert (!LastPiece &&
-              "There can only be one final piece in a diagnostic.");
-      LastPiece = Piece;
+  BugReport::VisitorList visitors;
+  unsigned originalReportConfigToken, finalReportConfigToken;
+
+  // While generating diagnostics, it's possible the visitors will decide
+  // new symbols and regions are interesting, or add other visitors based on
+  // the information they find. If they do, we need to regenerate the path
+  // based on our new report configuration.
+  do {
+    // Get a clean copy of all the visitors.
+    for (BugReport::visitor_iterator I = R->visitor_begin(),
+                                     E = R->visitor_end(); I != E; ++I)
+       visitors.push_back((*I)->clone());
+
+    // Clear out the active path from any previous work.
+    PD.getActivePath().clear();
+    originalReportConfigToken = R->getConfigurationChangeToken();
+
+    // Generate the very last diagnostic piece - the piece is visible before 
+    // the trace is expanded.
+    PathDiagnosticPiece *LastPiece = 0;
+    for (BugReport::visitor_iterator I = visitors.begin(), E = visitors.end();
+         I != E; ++I) {
+      if (PathDiagnosticPiece *Piece = (*I)->getEndPath(PDB, N, *R)) {
+        assert (!LastPiece &&
+                "There can only be one final piece in a diagnostic.");
+        LastPiece = Piece;
+      }
     }
-  }
-  if (!LastPiece)
-    LastPiece = BugReporterVisitor::getDefaultEndPath(PDB, N, *R);
-  if (LastPiece)
-    PD.getActivePath().push_back(LastPiece);
-  else
-    return;
+    if (!LastPiece)
+      LastPiece = BugReporterVisitor::getDefaultEndPath(PDB, N, *R);
+    if (LastPiece)
+      PD.getActivePath().push_back(LastPiece);
+    else
+      return;
 
-  switch (PDB.getGenerationScheme()) {
+    switch (PDB.getGenerationScheme()) {
     case PathDiagnosticConsumer::Extensive:
-      GenerateExtensivePathDiagnostic(PD, PDB, N);
+      GenerateExtensivePathDiagnostic(PD, PDB, N, visitors);
       break;
     case PathDiagnosticConsumer::Minimal:
-      GenerateMinimalPathDiagnostic(PD, PDB, N);
+      GenerateMinimalPathDiagnostic(PD, PDB, N, visitors);
       break;
-  }
-  
+    }
+
+    // Clean up the visitors we used.
+    llvm::DeleteContainerPointers(visitors);
+
+    // Did anything change while generating this path?
+    finalReportConfigToken = R->getConfigurationChangeToken();
+  } while(finalReportConfigToken != originalReportConfigToken);
+
   // Finally, prune the diagnostic path of uninteresting stuff.
   bool hasSomethingInteresting = RemoveUneededCalls(PD.getMutablePieces());
   assert(hasSomethingInteresting);