[analyzer] Replace most usages of getEndPath

[george.karpenkov]

Summary

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.

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

public:
   /// given node.
   ///
   /// The last parameter can be used to register a new visitor with the given
   /// BugReport while processing a node.
   virtual std::shared_ptr<PathDiagnosticPiece>
   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<PathDiagnosticPiece>
   getEndPath(BugReporterContext &BRC, const ExplodedNode *N, BugReport &BR);
 
   virtual void Profile(llvm::FoldingSetNodeID &ID) const = 0;
 
   /// Generates the default final diagnostic piece.
   static std::unique_ptr<PathDiagnosticPiece>
   getDefaultEndPath(BugReporterContext &BRC, const ExplodedNode *N,
public:
 
   std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
                                                  const ExplodedNode *Prev,
                                                  BugReporterContext &BRC,
                                                  BugReport &BR) override {
     return nullptr;
   }
 
-  std::unique_ptr<PathDiagnosticPiece> getEndPath(BugReporterContext &BRC,
-                                                  const ExplodedNode *N,
-                                                  BugReport &BR) override;
+  void finalizeVisitor(BugReporterContext &BRC, const ExplodedNode *N,
 };
 
 /// When a region containing undefined value or '0' value is passed 
 /// as an argument in a call, marks the call as interesting.
 ///
 /// As a result, BugReporter will not prune the path through the function even
 /// if the region's contents are not modified/accessed by the call.
 class UndefOrNullArgVisitor final

lib/StaticAnalyzer/Core/BugReporter.cpp

 /// the path is modified to insert artificially generated
 /// edges.
 /// Otherwise, more detailed diagnostics is emitted for block edges, explaining
 /// the transitions in words.
 static bool generatePathDiagnostics(
     PathDiagnostic &PD, PathDiagnosticBuilder &PDB, const ExplodedNode *N,
     LocationContextMap &LCM,
     ArrayRef<std::unique_ptr<BugReporterVisitor>> visitors,
+    BugReport *R,
     PathDiagnosticConsumer::PathGenerationScheme ActiveScheme) {
+  const ExplodedNode *LastNode = N;
   BugReport *report = PDB.getBugReport();
   StackDiagVector CallStack;
   InterestingExprs IE;
   bool AddPathEdges = (ActiveScheme == PathDiagnosticConsumer::Extensive);
   bool GenerateDiagnostics = (ActiveScheme != PathDiagnosticConsumer::None);
 
   PathDiagnosticLocation PrevLoc = GenerateDiagnostics ?
     PD.getLocation() : PathDiagnosticLocation();
 
   const ExplodedNode *NextNode = N->getFirstPred();
   while (NextNode) {
     N = NextNode;
     NextNode = N->getFirstPred();
 
     if (GenerateDiagnostics)
       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<PathDiagnosticPiece> DeduplicationSet;
     for (auto &V : visitors) {
       if (auto p = V->VisitNode(N, NextNode, PDB, *report)) {
 
         if (!GenerateDiagnostics)
           continue;
do { // TODO: dump statistics on the MAX number of iterations of this loop.
         assert(LastPiece);
         PD.setEndOfPath(std::move(LastPiece));
       }
 
       // Make sure we get a clean location context map so we don't
       // 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();
 
       // Did anything change while generating this path?
       finalReportConfigToken = R->getConfigurationChangeToken();
     } while (finalReportConfigToken != origReportConfigToken);
 

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

 //===----------------------------------------------------------------------===//
 
 std::unique_ptr<PathDiagnosticPiece>
 BugReporterVisitor::getEndPath(BugReporterContext &BRC,
                                const ExplodedNode *EndPathNode, BugReport &BR) {
   return nullptr;
 }
 
+void
+BugReporterVisitor::finalizeVisitor(BugReporterContext &BRC,
+                                    const ExplodedNode *EndPathNode,
+                                    BugReport &BR) {};
+
 std::unique_ptr<PathDiagnosticPiece> BugReporterVisitor::getDefaultEndPath(
     BugReporterContext &BRC, const ExplodedNode *EndPathNode, BugReport &BR) {
   PathDiagnosticLocation L =
     PathDiagnosticLocation::createEndOfPath(EndPathNode,BRC.getSourceManager());
 
   const auto &Ranges = BR.getRanges();
 
   // Only add the statement itself as a range if we didn't specify any
case MaybeUnsuppress:
       return visitNodeMaybeUnsuppress(N, PrevN, BRC, BR);
     case Satisfied:
       return nullptr;
     }
 
     llvm_unreachable("Invalid visit mode!");
   }
 
-  std::unique_ptr<PathDiagnosticPiece> getEndPath(BugReporterContext &BRC,
-                                                  const ExplodedNode *N,
-                                                  BugReport &BR) override {
+  void finalizeVisitor(BugReporterContext &BRC, const ExplodedNode *N,
     if (EnableNullFPSuppression)
       BR.markInvalid(ReturnVisitor::getTag(), StackFrame);
-    return nullptr;
   }
 };
 
 } // namespace
 
 void FindLastStoreBRVisitor::Profile(llvm::FoldingSetNodeID &ID) const {
   static int tag = 0;
   ID.AddPointer(&tag);
const char *const ConditionBRVisitor::GenericFalseMessage =
     "Assuming the condition is false";
 
 bool ConditionBRVisitor::isPieceMessageGeneric(
     const PathDiagnosticPiece *Piece) {
   return Piece->getString() == GenericTrueMessage ||
          Piece->getString() == GenericFalseMessage;
 }
 
-std::unique_ptr<PathDiagnosticPiece>
-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();
   AnalyzerOptions &Options = Eng.getAnalysisManager().options;
   const Decl *D = N->getLocationContext()->getDecl();
 
   if (AnalysisDeclContext::isInStdNamespace(D)) {
     // Skip reports within the 'std' namespace. Although these can sometimes be
     // the user's fault, we currently don't report them very well, and
     // Note that this will not help for any other data structure libraries, like
     // 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.
 
       // The analyzer issues a false use-after-free when std::list::pop_front
       // or std::list::pop_back are called multiple times because we cannot
       // reason about the internal invariants of the data structure.
       if (const auto *MD = dyn_cast<CXXMethodDecl>(D)) {
         const CXXRecordDecl *CD = MD->getParent();
         if (CD->getName() == "list") {
           BR.markInvalid(getTag(), nullptr);
-          return nullptr;
+          return;
         }
       }
 
       // The analyzer issues a false positive when the constructor of
       // std::__independent_bits_engine from algorithms is used.
       if (const auto *MD = dyn_cast<CXXConstructorDecl>(D)) {
         const CXXRecordDecl *CD = MD->getParent();
         if (CD->getName() == "__independent_bits_engine") {
           BR.markInvalid(getTag(), nullptr);
-          return nullptr;
+          return;
         }
       }
 
       for (const LocationContext *LCtx = N->getLocationContext(); LCtx;
            LCtx = LCtx->getParent()) {
         const auto *MD = dyn_cast<CXXMethodDecl>(LCtx->getDecl());
         if (!MD)
           continue;
 
         const CXXRecordDecl *CD = MD->getParent();
         // The analyzer issues a false positive on
         //   std::basic_string<uint8_t> v; v.push_back(1);
         // and
         //   std::u16string s; s += u'a';
         // because we cannot reason about the internal invariants of the
         // data structure.
         if (CD->getName() == "basic_string") {
           BR.markInvalid(getTag(), nullptr);
-          return nullptr;
+          return;
         }
 
         // The analyzer issues a false positive on
         //    std::shared_ptr<int> p(new int(1)); p = nullptr;
         // because it does not reason properly about temporary destructors.
         if (CD->getName() == "shared_ptr") {
           BR.markInvalid(getTag(), nullptr);
-          return nullptr;
+          return;
         }
       }
     }
   }
 
   // Skip reports within the sys/queue.h macros as we do not have the ability to
   // reason about data structure shapes.
   SourceManager &SM = BRC.getSourceManager();
   FullSourceLoc Loc = BR.getLocation(SM).asLocation();
   while (Loc.isMacroID()) {
     Loc = Loc.getSpellingLoc();
     if (SM.getFilename(Loc).endswith("sys/queue.h")) {
       BR.markInvalid(getTag(), nullptr);
-      return nullptr;
+      return;
     }
   }
-
-  return nullptr;
 }
 
 std::shared_ptr<PathDiagnosticPiece>
 UndefOrNullArgVisitor::VisitNode(const ExplodedNode *N,
                                  const ExplodedNode *PrevN,
                                  BugReporterContext &BRC, BugReport &BR) {
   ProgramStateRef State = N->getState();
   ProgramPoint ProgLoc = N->getLocation();