From: George Karpenkov Date: Tue, 12 Jun 2018 20:50:44 +0000 (+0000) Subject: [analyzer] [NFC] Remove most usages of getEndPath X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=adbb1b6dd3017aae49c0c9ad63ff6d5861d2f078;p=clang [analyzer] [NFC] Remove most usages of getEndPath getEndPath is a problematic API, because it's not clear when it's called (hint: not always at the end of the path), it crashes at runtime with more than one non-nullptr returning implementation, and diagnostics internal depend on it being called at some exact place. However, most visitors don't actually need that: all they want is a function consistently called after all nodes are traversed, to perform finalization and to decide whether invalidation is needed. Differential Revision: https://reviews.llvm.org/D48042 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@334540 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h index 80a6763783..9662d91fea 100644 --- a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h +++ b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h @@ -73,12 +73,17 @@ public: VisitNode(const ExplodedNode *Succ, const ExplodedNode *Pred, BugReporterContext &BRC, BugReport &BR) = 0; + /// Last function called on the visitor, no further calls to VisitNode + /// would follow. + virtual void finalizeVisitor(BugReporterContext &BRC, + const ExplodedNode *EndPathNode, + BugReport &BR); + /// Provide custom definition for the final diagnostic piece on the /// path - the piece, which is displayed before the path is expanded. /// - /// If returns NULL the default implementation will be used. - /// Also note that at most one visitor of a BugReport should generate a - /// non-NULL end of path diagnostic piece. + /// NOTE that this function can be implemented on at most one used visitor, + /// and otherwise it crahes at runtime. virtual std::unique_ptr getEndPath(BugReporterContext &BRC, const ExplodedNode *N, BugReport &BR); @@ -268,9 +273,8 @@ public: return nullptr; } - std::unique_ptr getEndPath(BugReporterContext &BRC, - const ExplodedNode *N, - BugReport &BR) override; + void finalizeVisitor(BugReporterContext &BRC, const ExplodedNode *N, + BugReport &BR) override; }; /// When a region containing undefined value or '0' value is passed diff --git a/lib/StaticAnalyzer/Core/BugReporter.cpp b/lib/StaticAnalyzer/Core/BugReporter.cpp index 98c90559e2..22185d2d03 100644 --- a/lib/StaticAnalyzer/Core/BugReporter.cpp +++ b/lib/StaticAnalyzer/Core/BugReporter.cpp @@ -1270,7 +1270,9 @@ static bool generatePathDiagnostics( PathDiagnostic &PD, PathDiagnosticBuilder &PDB, const ExplodedNode *N, LocationContextMap &LCM, ArrayRef> visitors, + BugReport *R, PathDiagnosticConsumer::PathGenerationScheme ActiveScheme) { + const ExplodedNode *LastNode = N; BugReport *report = PDB.getBugReport(); StackDiagVector CallStack; InterestingExprs IE; @@ -1289,8 +1291,12 @@ static bool generatePathDiagnostics( generatePathDiagnosticsForNode( N, PD, PrevLoc, PDB, LCM, CallStack, IE, AddPathEdges); - if (!NextNode) + if (!NextNode) { + for (auto &V : visitors) { + V->finalizeVisitor(PDB, LastNode, *R); + } continue; + } // Add pieces from custom visitors. llvm::FoldingSet DeduplicationSet; @@ -2583,7 +2589,7 @@ bool GRBugReporter::generatePathDiagnostic(PathDiagnostic& PD, // hold onto old mappings. LCM.clear(); - generatePathDiagnostics(PD, PDB, N, LCM, visitors, ActiveScheme); + generatePathDiagnostics(PD, PDB, N, LCM, visitors, R, ActiveScheme); // Clean up the visitors we used. visitors.clear(); diff --git a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp index fe1a711a82..75126c6c37 100644 --- a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -181,6 +181,11 @@ BugReporterVisitor::getEndPath(BugReporterContext &BRC, return nullptr; } +void +BugReporterVisitor::finalizeVisitor(BugReporterContext &BRC, + const ExplodedNode *EndPathNode, + BugReport &BR) {}; + std::unique_ptr BugReporterVisitor::getDefaultEndPath( BugReporterContext &BRC, const ExplodedNode *EndPathNode, BugReport &BR) { PathDiagnosticLocation L = @@ -866,12 +871,10 @@ public: llvm_unreachable("Invalid visit mode!"); } - std::unique_ptr getEndPath(BugReporterContext &BRC, - const ExplodedNode *N, - BugReport &BR) override { + void finalizeVisitor(BugReporterContext &BRC, const ExplodedNode *N, + BugReport &BR) override { if (EnableNullFPSuppression) BR.markInvalid(ReturnVisitor::getTag(), StackFrame); - return nullptr; } }; @@ -2144,10 +2147,8 @@ bool ConditionBRVisitor::isPieceMessageGeneric( Piece->getString() == GenericFalseMessage; } -std::unique_ptr -LikelyFalsePositiveSuppressionBRVisitor::getEndPath(BugReporterContext &BRC, - const ExplodedNode *N, - BugReport &BR) { +void LikelyFalsePositiveSuppressionBRVisitor::finalizeVisitor( + BugReporterContext &BRC, const ExplodedNode *N, BugReport &BR) { // Here we suppress false positives coming from system headers. This list is // based on known issues. ExprEngine &Eng = BRC.getBugReporter().getEngine(); @@ -2161,7 +2162,7 @@ LikelyFalsePositiveSuppressionBRVisitor::getEndPath(BugReporterContext &BRC, // TR1, Boost, or llvm/ADT. if (Options.shouldSuppressFromCXXStandardLibrary()) { BR.markInvalid(getTag(), nullptr); - return nullptr; + return; } else { // If the complete 'std' suppression is not enabled, suppress reports // from the 'std' namespace that are known to produce false positives. @@ -2173,7 +2174,7 @@ LikelyFalsePositiveSuppressionBRVisitor::getEndPath(BugReporterContext &BRC, const CXXRecordDecl *CD = MD->getParent(); if (CD->getName() == "list") { BR.markInvalid(getTag(), nullptr); - return nullptr; + return; } } @@ -2183,7 +2184,7 @@ LikelyFalsePositiveSuppressionBRVisitor::getEndPath(BugReporterContext &BRC, const CXXRecordDecl *CD = MD->getParent(); if (CD->getName() == "__independent_bits_engine") { BR.markInvalid(getTag(), nullptr); - return nullptr; + return; } } @@ -2202,7 +2203,7 @@ LikelyFalsePositiveSuppressionBRVisitor::getEndPath(BugReporterContext &BRC, // data structure. if (CD->getName() == "basic_string") { BR.markInvalid(getTag(), nullptr); - return nullptr; + return; } // The analyzer issues a false positive on @@ -2210,7 +2211,7 @@ LikelyFalsePositiveSuppressionBRVisitor::getEndPath(BugReporterContext &BRC, // because it does not reason properly about temporary destructors. if (CD->getName() == "shared_ptr") { BR.markInvalid(getTag(), nullptr); - return nullptr; + return; } } } @@ -2224,11 +2225,9 @@ LikelyFalsePositiveSuppressionBRVisitor::getEndPath(BugReporterContext &BRC, Loc = Loc.getSpellingLoc(); if (SM.getFilename(Loc).endswith("sys/queue.h")) { BR.markInvalid(getTag(), nullptr); - return nullptr; + return; } } - - return nullptr; } std::shared_ptr