From 8347d3d45e6f128bba19821f0d2f54cadd4d49bb Mon Sep 17 00:00:00 2001 From: Jordan Rose Date: Sat, 22 Sep 2012 01:24:53 +0000 Subject: [PATCH] [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). git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@164446 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../Core/BugReporter/BugReporter.h | 60 +++++++++++++++++-- lib/StaticAnalyzer/Core/BugReporter.cpp | 46 +++++++++++--- lib/StaticAnalyzer/Core/ExplodedGraph.cpp | 4 +- 3 files changed, 96 insertions(+), 14 deletions(-) diff --git a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h index 4d054df705..f397faab14 100644 --- a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h +++ b/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 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 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 &bugReports) {} + ArrayRef &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 &bugReports); diff --git a/lib/StaticAnalyzer/Core/BugReporter.cpp b/lib/StaticAnalyzer/Core/BugReporter.cpp index e95f31c0d6..e45d43e313 100644 --- a/lib/StaticAnalyzer/Core/BugReporter.cpp +++ b/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 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 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 &bugReports) { - assert(!bugReports.empty()); + + bool HasValid = false; SmallVector errorNodes; for (ArrayRef::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, @@ -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 ReportGraph(GPair.first.first); OwningPtr 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 diff --git a/lib/StaticAnalyzer/Core/ExplodedGraph.cpp b/lib/StaticAnalyzer/Core/ExplodedGraph.cpp index 7e8a02872f..39440ccc0a 100644 --- a/lib/StaticAnalyzer/Core/ExplodedGraph.cpp +++ b/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); - WL1.push_back(*I); + if (*I) + WL1.push_back(*I); } // Process the first worklist until it is empty. Because it is a std::list -- 2.40.0