[analyzer] Allow a BugReport to be marked "invalid" during path generation.

This is intended to allow visitors to make decisions about whether a
BugReport is likely a false positive. Currently there are no visitors
making use of this feature, so there are no tests.

When a BugReport is marked invalid, the invalidator must provide a key
that identifies the invaliation (intended to be the visitor type and a
context pointer of some kind). This allows us to reverse the decision
later on. Being able to reverse a decision about invalidation gives us more
flexibility, and allows us to formulate conditions like "this report is
invalid UNLESS the original argument is 'foo'". We can use this to
fine-tune our false-positive suppression (coming soon).

llvm-svn: 164446

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h

@@ -116,6 +116,19 @@ protected:
   /// when reporting an issue.
   bool DoNotPrunePath;
 
+  /// Used to track unique reasons why a bug report might be invalid.
+  ///
+  /// \sa markInvalid
+  /// \sa removeInvalidation
+  typedef std::pair<const void *, const void *> InvalidationRecord;
+
+  /// If non-empty, this bug report is likely a false positive and should not be
+  /// shown to the user.
+  ///
+  /// \sa markInvalid
+  /// \sa removeInvalidation
+  llvm::SmallSet<InvalidationRecord, 4> Invalidations;
+
 private:
   // Used internally by BugReporter.
   Symbols &getInterestingSymbols();
@@ -152,7 +165,8 @@ public:
             PathDiagnosticLocation LocationToUnique)
     : BT(bt), DeclWithIssue(0), Description(desc),
       UniqueingLocation(LocationToUnique),
-      ErrorNode(errornode), ConfigurationChangeToken(0) {}
+      ErrorNode(errornode), ConfigurationChangeToken(0),
+      DoNotPrunePath(false) {}
 
   virtual ~BugReport();
 
@@ -189,6 +203,34 @@ public:
   unsigned getConfigurationChangeToken() const {
     return ConfigurationChangeToken;
   }
+
+  /// Returns whether or not this report should be considered valid.
+  ///
+  /// Invalid reports are those that have been classified as likely false
+  /// positives after the fact.
+  bool isValid() const {
+    return Invalidations.empty();
+  }
+
+  /// Marks the current report as invalid, meaning that it is probably a false
+  /// positive and should not be reported to the user.
+  ///
+  /// The \p Tag and \p Data arguments are intended to be opaque identifiers for
+  /// this particular invalidation, where \p Tag represents the visitor
+  /// responsible for invalidation, and \p Data represents the reason this
+  /// visitor decided to invalidate the bug report.
+  ///
+  /// \sa removeInvalidation
+  void markInvalid(const void *Tag, const void *Data) {
+    Invalidations.insert(std::make_pair(Tag, Data));
+  }
+
+  /// Reverses the effects of a previous invalidation.
+  ///
+  /// \sa markInvalid
+  void removeInvalidation(const void *Tag, const void *Data) {
+    Invalidations.erase(std::make_pair(Tag, Data));
+  }
   
   /// Return the canonical declaration, be it a method or class, where
   /// this issue semantically occurred.
@@ -392,9 +434,11 @@ public:
 
   SourceManager& getSourceManager() { return D.getSourceManager(); }
 
-  virtual void GeneratePathDiagnostic(PathDiagnostic& pathDiagnostic,
+  virtual bool generatePathDiagnostic(PathDiagnostic& pathDiagnostic,
                                       PathDiagnosticConsumer &PC,
-                                      ArrayRef<BugReport *> &bugReports) {}
+                                      ArrayRef<BugReport *> &bugReports) {
+    return true;
+  }
 
   bool RemoveUneededCalls(PathPieces &pieces, BugReport *R,
                           PathDiagnosticCallPiece *CallWithLoc = 0);
@@ -461,7 +505,15 @@ public:
   ///  engine.
   ProgramStateManager &getStateManager();
 
-  virtual void GeneratePathDiagnostic(PathDiagnostic &pathDiagnostic,
+  /// Generates a path corresponding to one of the given bug reports.
+  ///
+  /// Which report is used for path generation is not specified. The
+  /// bug reporter will try to pick the shortest path, but this is not
+  /// guaranteed.
+  ///
+  /// \return True if the report was valid and a path was generated,
+  ///         false if the reports should be considered invalid.
+  virtual bool generatePathDiagnostic(PathDiagnostic &PD,
                                       PathDiagnosticConsumer &PC,
                                       ArrayRef<BugReport*> &bugReports);
 

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

@@ -432,7 +432,7 @@ static void updateStackPiecesWithMessage(PathDiagnosticPiece *P,
 
 static void CompactPathDiagnostic(PathPieces &path, const SourceManager& SM);
 
-static void GenerateMinimalPathDiagnostic(PathDiagnostic& PD,
+static bool GenerateMinimalPathDiagnostic(PathDiagnostic& PD,
                                           PathDiagnosticBuilder &PDB,
                                           const ExplodedNode *N,
                                       ArrayRef<BugReporterVisitor *> visitors) {
@@ -756,9 +756,13 @@ static void GenerateMinimalPathDiagnostic(PathDiagnostic& PD,
     }
   }
 
+  if (!PDB.getBugReport()->isValid())
+    return false;
+
   // After constructing the full PathDiagnostic, do a pass over it to compact
   // PathDiagnosticPieces that occur within a macro.
   CompactPathDiagnostic(PD.getMutablePieces(), PDB.getSourceManager());
+  return true;
 }
 
 //===----------------------------------------------------------------------===//
@@ -1164,7 +1168,7 @@ static void reversePropagateInterestingSymbols(BugReport &R,
   }
 }
                                                
-static void GenerateExtensivePathDiagnostic(PathDiagnostic& PD,
+static bool GenerateExtensivePathDiagnostic(PathDiagnostic& PD,
                                             PathDiagnosticBuilder &PDB,
                                             const ExplodedNode *N,
                                       ArrayRef<BugReporterVisitor *> visitors) {
@@ -1337,6 +1341,8 @@ static void GenerateExtensivePathDiagnostic(PathDiagnostic& PD,
       }
     }
   }
+
+  return PDB.getBugReport()->isValid();
 }
 
 //===----------------------------------------------------------------------===//
@@ -1866,17 +1872,27 @@ static void CompactPathDiagnostic(PathPieces &path, const SourceManager& SM) {
     path.push_back(*I);
 }
 
-void GRBugReporter::GeneratePathDiagnostic(PathDiagnostic& PD,
+bool GRBugReporter::generatePathDiagnostic(PathDiagnostic& PD,
                                            PathDiagnosticConsumer &PC,
                                            ArrayRef<BugReport *> &bugReports) {
-
   assert(!bugReports.empty());
+
+  bool HasValid = false;
   SmallVector<const ExplodedNode *, 10> errorNodes;
   for (ArrayRef<BugReport*>::iterator I = bugReports.begin(),
                                       E = bugReports.end(); I != E; ++I) {
+    if ((*I)->isValid()) {
+      HasValid = true;
       errorNodes.push_back((*I)->getErrorNode());
+    } else {
+      errorNodes.push_back(0);
+    }
   }
 
+  // If all the reports have been marked invalid, we're done.
+  if (!HasValid)
+    return false;
+
   // Construct a new graph that contains only a single path from the error
   // node to a root.
   const std::pair<std::pair<ExplodedGraph*, NodeBackMap*>,
@@ -1887,6 +1903,7 @@ void GRBugReporter::GeneratePathDiagnostic(PathDiagnostic& PD,
   assert(GPair.second.second < bugReports.size());
   BugReport *R = bugReports[GPair.second.second];
   assert(R && "No original report found for sliced graph.");
+  assert(R->isValid() && "Report selected from trimmed graph marked invalid.");
 
   OwningPtr<ExplodedGraph> ReportGraph(GPair.first.first);
   OwningPtr<NodeBackMap> BackMap(GPair.first.second);
@@ -1932,14 +1949,24 @@ void GRBugReporter::GeneratePathDiagnostic(PathDiagnostic& PD,
     if (LastPiece)
       PD.setEndOfPath(LastPiece);
     else
-      return;
+      return false;
 
     switch (PDB.getGenerationScheme()) {
     case PathDiagnosticConsumer::Extensive:
-      GenerateExtensivePathDiagnostic(PD, PDB, N, visitors);
+      if (!GenerateExtensivePathDiagnostic(PD, PDB, N, visitors)) {
+        assert(!R->isValid() && "Failed on valid report");
+        // Try again. We'll filter out the bad report when we trim the graph.
+        // FIXME: It would be more efficient to use the same intermediate
+        // trimmed graph, and just repeat the shortest-path search.
+        return generatePathDiagnostic(PD, PC, bugReports);
+      }
       break;
     case PathDiagnosticConsumer::Minimal:
-      GenerateMinimalPathDiagnostic(PD, PDB, N, visitors);
+      if (!GenerateMinimalPathDiagnostic(PD, PDB, N, visitors)) {
+        assert(!R->isValid() && "Failed on valid report");
+        // Try again. We'll filter out the bad report when we trim the graph.
+        return generatePathDiagnostic(PD, PC, bugReports);
+      }
       break;
     case PathDiagnosticConsumer::None:
       llvm_unreachable("PathDiagnosticConsumer::None should never appear here");
@@ -1958,6 +1985,8 @@ void GRBugReporter::GeneratePathDiagnostic(PathDiagnostic& PD,
     assert(hasSomethingInteresting);
     (void) hasSomethingInteresting;
   }
+
+  return true;
 }
 
 void BugReporter::Register(BugType *BT) {
@@ -2129,7 +2158,8 @@ void BugReporter::FlushReport(BugReport *exampleReport,
   // specified by the PathDiagnosticConsumer.
   if (PD.getGenerationScheme() != PathDiagnosticConsumer::None) {
     if (!bugReports.empty())
-      GeneratePathDiagnostic(*D.get(), PD, bugReports);
+      if (!generatePathDiagnostic(*D.get(), PD, bugReports))
+        return;
   }
 
   // If the path is empty, generate a single step path with the location

clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp

@@ -332,8 +332,8 @@ ExplodedGraph::TrimInternal(const ExplodedNode* const* BeginSources,
 
   // ===- Pass 1 (reverse DFS) -===
   for (const ExplodedNode* const* I = BeginSources; I != EndSources; ++I) {
-    assert(*I);
+    if (*I)
-    WL1.push_back(*I);
+      WL1.push_back(*I);
   }
 
   // Process the first worklist until it is empty.  Because it is a std::list