[analyzer][NFCI] Remove ad-hoc program point tagging

clang/include/clang/StaticAnalyzer/Core/Checker.h

@@ -608,20 +608,6 @@ class EventDispatcher {
   }
 };
 
-/// Tag that can use a checker name as a message provider
-/// (see SimpleProgramPointTag).
-/// FIXME: This is a cargo cult class which is copied into several checkers but
-/// does not provide anything useful.
-/// The only added functionality provided by this class (compared to
-/// SimpleProgramPointTag) is that it composes the tag description string from
-/// two arguments -- but tag descriptions only appear in debug output so there
-/// is no reason to bother with this.
-class CheckerProgramPointTag : public SimpleProgramPointTag {
-public:
-  CheckerProgramPointTag(StringRef CheckerName, StringRef Msg);
-  CheckerProgramPointTag(const CheckerBase *Checker, StringRef Msg);
-};
-
 /// We dereferenced a location that may be null.
 struct ImplicitNullDerefEvent {
   SVal Location;

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h

@@ -171,6 +171,9 @@ class CheckerContext {
   ///        tag is specified, a default tag, unique to the given checker,
   ///        will be used. Tags are used to prevent states generated at
   ///        different sites from caching out.
+  /// NOTE: If the State is unchanged and the Tag is nullptr, this may return a
+  /// node which is not tagged (instead of using the default tag corresponding
+  /// to the active checker). This is arguably a bug and should be fixed.
   ExplodedNode *addTransition(ProgramStateRef State = nullptr,
                               const ProgramPointTag *Tag = nullptr) {
     return addTransitionImpl(State ? State : getState(), false, nullptr, Tag);
@@ -183,6 +186,9 @@ class CheckerContext {
   /// @param Pred The transition will be generated from the specified Pred node
   ///             to the newly generated node.
   /// @param Tag The tag to uniquely identify the creation site.
+  /// NOTE: If the State is unchanged and the Tag is nullptr, this may return a
+  /// node which is not tagged (instead of using the default tag corresponding
+  /// to the active checker). This is arguably a bug and should be fixed.
   ExplodedNode *addTransition(ProgramStateRef State, ExplodedNode *Pred,
                               const ProgramPointTag *Tag = nullptr) {
     return addTransitionImpl(State, false, Pred, Tag);

clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp

@@ -671,7 +671,6 @@ void CallAndMessageChecker::HandleNilReceiver(CheckerContext &C,
                                               ProgramStateRef state,
                                               const ObjCMethodCall &Msg) const {
   ASTContext &Ctx = C.getASTContext();
-  static CheckerProgramPointTag Tag(this, "NilReceiver");
 
   // Check the return type of the message expression.  A message to nil will
   // return different values depending on the return type and the architecture.
@@ -682,7 +681,7 @@ void CallAndMessageChecker::HandleNilReceiver(CheckerContext &C,
   if (CanRetTy->isStructureOrClassType()) {
     // Structure returns are safe since the compiler zeroes them out.
     SVal V = C.getSValBuilder().makeZeroVal(RetTy);
-    C.addTransition(state->BindExpr(Msg.getOriginExpr(), LCtx, V), &Tag);
+    C.addTransition(state->BindExpr(Msg.getOriginExpr(), LCtx, V));
     return;
   }
 
@@ -701,7 +700,7 @@ void CallAndMessageChecker::HandleNilReceiver(CheckerContext &C,
             Ctx.LongDoubleTy == CanRetTy ||
             Ctx.LongLongTy == CanRetTy ||
             Ctx.UnsignedLongLongTy == CanRetTy)))) {
-      if (ExplodedNode *N = C.generateErrorNode(state, &Tag))
+      if (ExplodedNode *N = C.generateErrorNode(state))
         emitNilReceiverBug(C, Msg, N);
       return;
     }
@@ -720,7 +719,7 @@ void CallAndMessageChecker::HandleNilReceiver(CheckerContext &C,
     // of this case unless we have *a lot* more knowledge.
     //
     SVal V = C.getSValBuilder().makeZeroVal(RetTy);
-    C.addTransition(state->BindExpr(Msg.getOriginExpr(), LCtx, V), &Tag);
+    C.addTransition(state->BindExpr(Msg.getOriginExpr(), LCtx, V));
     return;
   }
 

clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp

@@ -674,9 +674,16 @@ void DynamicTypePropagation::checkPostStmt(const CastExpr *CE,
   if (TrackedType &&
       !ASTCtxt.canAssignObjCInterfaces(DestObjectPtrType, *TrackedType) &&
       !ASTCtxt.canAssignObjCInterfaces(*TrackedType, DestObjectPtrType)) {
-    static CheckerProgramPointTag IllegalConv(this, "IllegalConversion");
-    ExplodedNode *N = C.addTransition(State, AfterTypeProp, &IllegalConv);
-    reportGenericsBug(*TrackedType, DestObjectPtrType, N, Sym, C);
+    // This distinct program point tag is needed because `State` can be
+    // identical to the state of the node `AfterTypeProp`, and in that case
+    // `generateNonFatalErrorNode` would "cache out" and return nullptr
+    // (instead of re-creating an already existing node).
+    static SimpleProgramPointTag IllegalConv("DynamicTypePropagation",
+                                             "IllegalConversion");
+    ExplodedNode *N =
+        C.generateNonFatalErrorNode(State, AfterTypeProp, &IllegalConv);
+    if (N)
+      reportGenericsBug(*TrackedType, DestObjectPtrType, N, Sym, C);
     return;
   }
 
@@ -885,8 +892,7 @@ void DynamicTypePropagation::checkPreObjCMessage(const ObjCMethodCall &M,
     // Warn when argument is incompatible with the parameter.
     if (!ASTCtxt.canAssignObjCInterfaces(ParamObjectPtrType,
                                          ArgObjectPtrType)) {
-      static CheckerProgramPointTag Tag(this, "ArgTypeMismatch");
-      ExplodedNode *N = C.addTransition(State, &Tag);
+      ExplodedNode *N = C.generateNonFatalErrorNode(State);
       reportGenericsBug(ArgObjectPtrType, ParamObjectPtrType, N, Sym, C, Arg);
       return;
     }

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

@@ -1043,8 +1043,7 @@ bool GenericTaintChecker::generateReportIfTainted(const Expr *E, StringRef Msg,
 
   // Generate diagnostic.
   assert(BT);
-  static CheckerProgramPointTag Tag(BT->getCheckerName(), Msg);
-  if (ExplodedNode *N = C.generateNonFatalErrorNode(C.getState(), &Tag)) {
+  if (ExplodedNode *N = C.generateNonFatalErrorNode(C.getState())) {
     auto report = std::make_unique<PathSensitiveBugReport>(*BT, Msg, N);
     report->addRange(E->getSourceRange());
     for (auto TaintedSym : getTaintedSymbols(C.getState(), *TaintedSVal)) {

clang/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp

@@ -747,9 +747,7 @@ void NonLocalizedStringChecker::reportLocalizationError(
   if (isDebuggingContext(C))
     return;
 
-  static CheckerProgramPointTag Tag("NonLocalizedStringChecker",
-                                    "UnlocalizedString");
-  ExplodedNode *ErrNode = C.addTransition(C.getState(), C.getPredecessor(), &Tag);
+  ExplodedNode *ErrNode = C.generateNonFatalErrorNode();
 
   if (!ErrNode)
     return;

clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp

@@ -73,7 +73,6 @@ void MPIChecker::checkUnmatchedWaits(const CallEvent &PreCallEvent,
     return;
 
   ProgramStateRef State = Ctx.getState();
-  static CheckerProgramPointTag Tag("MPI-Checker", "UnmatchedWait");
   ExplodedNode *ErrorNode{nullptr};
 
   // Check all request regions used by the wait function.
@@ -82,7 +81,7 @@ void MPIChecker::checkUnmatchedWaits(const CallEvent &PreCallEvent,
     State = State->set<RequestMap>(ReqRegion, Request::State::Wait);
     if (!Req) {
       if (!ErrorNode) {
-        ErrorNode = Ctx.generateNonFatalErrorNode(State, &Tag);
+        ErrorNode = Ctx.generateNonFatalErrorNode(State);
         State = ErrorNode->getState();
       }
       // A wait has no matching nonblocking call.
@@ -105,7 +104,6 @@ void MPIChecker::checkMissingWaits(SymbolReaper &SymReaper,
   if (Requests.isEmpty())
     return;
 
-  static CheckerProgramPointTag Tag("MPI-Checker", "MissingWait");
   ExplodedNode *ErrorNode{nullptr};
 
   auto ReqMap = State->get<RequestMap>();
@@ -114,7 +112,7 @@ void MPIChecker::checkMissingWaits(SymbolReaper &SymReaper,
       if (Req.second.CurrentState == Request::State::Nonblocking) {
 
         if (!ErrorNode) {
-          ErrorNode = Ctx.generateNonFatalErrorNode(State, &Tag);
+          ErrorNode = Ctx.generateNonFatalErrorNode(State);
           State = ErrorNode->getState();
         }
         BReporter.reportMissingWait(Req.second, Req.first, ErrorNode,

clang/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp

@@ -556,8 +556,7 @@ void MacOSKeychainAPIChecker::checkDeadSymbols(SymbolReaper &SR,
     return;
   }
 
-  static CheckerProgramPointTag Tag(this, "DeadSymbolsLeak");
-  ExplodedNode *N = C.generateNonFatalErrorNode(C.getState(), &Tag);
+  ExplodedNode *N = C.generateNonFatalErrorNode(C.getState());
   if (!N)
     return;
 

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

@@ -3098,8 +3098,7 @@ void MallocChecker::checkDeadSymbols(SymbolReaper &SymReaper,
   // Generate leak node.
   ExplodedNode *N = C.getPredecessor();
   if (!Errors.empty()) {
-    static CheckerProgramPointTag Tag("MallocChecker", "DeadSymbolsLeak");
-    N = C.generateNonFatalErrorNode(C.getState(), &Tag);
+    N = C.generateNonFatalErrorNode(C.getState());
     if (N) {
       for (SymbolRef Sym : Errors) {
         HandleLeak(Sym, N, C);

clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

@@ -69,6 +69,7 @@ const char *getNullabilityString(Nullability Nullab) {
 }
 
 // These enums are used as an index to ErrorMessages array.
+// FIXME: ErrorMessages no longer exists, perhaps remove this as well?
 enum class ErrorKind : int {
   NilAssignedToNonnull,
   NilPassedToNonnull,
@@ -714,8 +715,7 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
   if (ChecksEnabled[CK_NullReturnedFromNonnull] && NullReturnedFromNonNull &&
       RetExprTypeLevelNullability != Nullability::Nonnull &&
       !InSuppressedMethodFamily) {
-    static CheckerProgramPointTag Tag(this, "NullReturnedFromNonnull");
-    ExplodedNode *N = C.generateErrorNode(State, &Tag);
+    ExplodedNode *N = C.generateErrorNode(State);
     if (!N)
       return;
 
@@ -750,8 +750,7 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
         Nullness != NullConstraint::IsNotNull &&
         TrackedNullabValue == Nullability::Nullable &&
         RequiredNullability == Nullability::Nonnull) {
-      static CheckerProgramPointTag Tag(this, "NullableReturnedFromNonnull");
-      ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag);
+      ExplodedNode *N = C.addTransition(State, C.getPredecessor());
 
       SmallString<256> SBuf;
       llvm::raw_svector_ostream OS(SBuf);
@@ -1299,8 +1298,7 @@ void NullabilityChecker::checkBind(SVal L, SVal V, const Stmt *S,
       ValNullability != Nullability::Nonnull &&
       ValueExprTypeLevelNullability != Nullability::Nonnull &&
       !isARCNilInitializedLocal(C, S)) {
-    static CheckerProgramPointTag Tag(this, "NullPassedToNonnull");
-    ExplodedNode *N = C.generateErrorNode(State, &Tag);
+    ExplodedNode *N = C.generateErrorNode(State);
     if (!N)
       return;
 
@@ -1342,8 +1340,7 @@ void NullabilityChecker::checkBind(SVal L, SVal V, const Stmt *S,
       return;
     if (ChecksEnabled[CK_NullablePassedToNonnull] &&
         LocNullability == Nullability::Nonnull) {
-      static CheckerProgramPointTag Tag(this, "NullablePassedToNonnull");
-      ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag);
+      ExplodedNode *N = C.addTransition(State, C.getPredecessor());
       reportBugIfInvariantHolds("Nullable pointer is assigned to a pointer "
                                 "which is expected to have non-null value",
                                 ErrorKind::NullableAssignedToNonnull,

clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp

@@ -1031,8 +1031,7 @@ ExplodedNode * RetainCountChecker::processReturn(const ReturnStmt *S,
     return nullptr;
 
   // Update the autorelease counts.
-  static CheckerProgramPointTag AutoreleaseTag(this, "Autorelease");
-  state = handleAutoreleaseCounts(state, Pred, &AutoreleaseTag, C, Sym, X, S);
+  state = handleAutoreleaseCounts(state, Pred, C, Sym, X, S);
 
   // Have we generated a sink node?
   if (!state)
@@ -1089,8 +1088,7 @@ ExplodedNode * RetainCountChecker::checkReturnWithRetEffect(const ReturnStmt *S,
         // Generate an error node.
         state = setRefBinding(state, Sym, X);
 
-        static CheckerProgramPointTag ReturnOwnLeakTag(this, "ReturnsOwnLeak");
-        ExplodedNode *N = C.addTransition(state, Pred, &ReturnOwnLeakTag);
+        ExplodedNode *N = C.addTransition(state, Pred);
         if (N) {
           const LangOptions &LOpts = C.getASTContext().getLangOpts();
           auto R =
@@ -1113,10 +1111,7 @@ ExplodedNode * RetainCountChecker::checkReturnWithRetEffect(const ReturnStmt *S,
         // owned object.
         state = setRefBinding(state, Sym, X ^ RefVal::ErrorReturnedNotOwned);
 
-        static CheckerProgramPointTag
-            ReturnNotOwnedTag(this, "ReturnNotOwnedForOwned");
-
-        ExplodedNode *N = C.addTransition(state, Pred, &ReturnNotOwnedTag);
+        ExplodedNode *N = C.addTransition(state, Pred);
         if (N) {
           auto R = std::make_unique<RefCountReport>(
               *ReturnNotOwnedForOwned, C.getASTContext().getLangOpts(), N, Sym);
@@ -1202,14 +1197,9 @@ ProgramStateRef RetainCountChecker::checkRegionChanges(
   return state;
 }
 
-ProgramStateRef
-RetainCountChecker::handleAutoreleaseCounts(ProgramStateRef state,
-                                            ExplodedNode *Pred,
-                                            const ProgramPointTag *Tag,
-                                            CheckerContext &Ctx,
-                                            SymbolRef Sym,
-                                            RefVal V,
-                                            const ReturnStmt *S) const {
+ProgramStateRef RetainCountChecker::handleAutoreleaseCounts(
+    ProgramStateRef state, ExplodedNode *Pred, CheckerContext &Ctx,
+    SymbolRef Sym, RefVal V, const ReturnStmt *S) const {
   unsigned ACnt = V.getAutoreleaseCount();
 
   // No autorelease counts?  Nothing to be done.
@@ -1260,7 +1250,7 @@ RetainCountChecker::handleAutoreleaseCounts(ProgramStateRef state,
   V = V ^ RefVal::ErrorOverAutorelease;
   state = setRefBinding(state, Sym, V);
 
-  ExplodedNode *N = Ctx.generateSink(state, Pred, Tag);
+  ExplodedNode *N = Ctx.generateSink(state, Pred);
   if (N) {
     SmallString<128> sbuf;
     llvm::raw_svector_ostream os(sbuf);
@@ -1383,8 +1373,7 @@ void RetainCountChecker::checkEndFunction(const ReturnStmt *RS,
   }
 
   for (auto &I : B) {
-    state = handleAutoreleaseCounts(state, Pred, /*Tag=*/nullptr, Ctx,
-                                    I.first, I.second);
+    state = handleAutoreleaseCounts(state, Pred, Ctx, I.first, I.second);
     if (!state)
       return;
   }
@@ -1416,9 +1405,8 @@ void RetainCountChecker::checkDeadSymbols(SymbolReaper &SymReaper,
   for (const auto &I: state->get<RefBindings>()) {
     SymbolRef Sym = I.first;
     if (SymReaper.isDead(Sym)) {
-      static CheckerProgramPointTag Tag(this, "DeadSymbolAutorelease");
       const RefVal &V = I.second;
-      state = handleAutoreleaseCounts(state, Pred, &Tag, C, Sym, V);
+      state = handleAutoreleaseCounts(state, Pred, C, Sym, V);
       if (!state)
         return;
 
@@ -1472,15 +1460,15 @@ void RetainCountChecker::printState(raw_ostream &Out, ProgramStateRef State,
 // Checker registration.
 //===----------------------------------------------------------------------===//
 
-std::unique_ptr<CheckerProgramPointTag> RetainCountChecker::DeallocSentTag;
-std::unique_ptr<CheckerProgramPointTag> RetainCountChecker::CastFailTag;
+std::unique_ptr<SimpleProgramPointTag> RetainCountChecker::DeallocSentTag;
+std::unique_ptr<SimpleProgramPointTag> RetainCountChecker::CastFailTag;
 
 void ento::registerRetainCountBase(CheckerManager &Mgr) {
   auto *Chk = Mgr.registerChecker<RetainCountChecker>();
-  Chk->DeallocSentTag =
-      std::make_unique<CheckerProgramPointTag>(Chk, "DeallocSent");
-  Chk->CastFailTag =
-      std::make_unique<CheckerProgramPointTag>(Chk, "DynamicCastFail");
+  Chk->DeallocSentTag = std::make_unique<SimpleProgramPointTag>(
+      "RetainCountChecker", "DeallocSent");
+  Chk->CastFailTag = std::make_unique<SimpleProgramPointTag>(
+      "RetainCountChecker", "DynamicCastFail");
 }
 
 bool ento::shouldRegisterRetainCountBase(const CheckerManager &mgr) {

clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h

@@ -262,8 +262,8 @@ class RetainCountChecker
 
   mutable std::unique_ptr<RetainSummaryManager> Summaries;
 
-  static std::unique_ptr<CheckerProgramPointTag> DeallocSentTag;
-  static std::unique_ptr<CheckerProgramPointTag> CastFailTag;
+  static std::unique_ptr<SimpleProgramPointTag> DeallocSentTag;
+  static std::unique_ptr<SimpleProgramPointTag> CastFailTag;
 
   /// Track Objective-C and CoreFoundation objects.
   bool TrackObjCAndCFObjects = false;
@@ -347,23 +347,22 @@ class RetainCountChecker
                                     SymbolRef sid, RefVal V,
                                     SmallVectorImpl<SymbolRef> &Leaked) const;
 
-  ProgramStateRef
-  handleAutoreleaseCounts(ProgramStateRef state, ExplodedNode *Pred,
-                          const ProgramPointTag *Tag, CheckerContext &Ctx,
-                          SymbolRef Sym,
-                          RefVal V,
-                          const ReturnStmt *S=nullptr) const;
+  ProgramStateRef handleAutoreleaseCounts(ProgramStateRef state,
+                                          ExplodedNode *Pred,
+                                          CheckerContext &Ctx, SymbolRef Sym,
+                                          RefVal V,
+                                          const ReturnStmt *S = nullptr) const;
 
   ExplodedNode *processLeaks(ProgramStateRef state,
                              SmallVectorImpl<SymbolRef> &Leaked,
                              CheckerContext &Ctx,
                              ExplodedNode *Pred = nullptr) const;
 
-  static const CheckerProgramPointTag &getDeallocSentTag() {
+  static const SimpleProgramPointTag &getDeallocSentTag() {
     return *DeallocSentTag;
   }
 
-  static const CheckerProgramPointTag &getCastFailTag() { return *CastFailTag; }
+  static const SimpleProgramPointTag &getCastFailTag() { return *CastFailTag; }
 
 private:
   /// Perform the necessary checks and state adjustments at the end of the

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

@@ -2164,6 +2164,7 @@ PathSensitiveBugReport::PathSensitiveBugReport(
     : BugReport(Kind::PathSensitive, bt, shortDesc, desc), ErrorNode(errorNode),
       ErrorNodeRange(getStmt() ? getStmt()->getSourceRange() : SourceRange()),
       UniqueingLocation(LocationToUnique), UniqueingDecl(DeclToUnique) {
+  assert(ErrorNode && "The error node must be non-null!");
   assert(!isDependency(ErrorNode->getState()
                            ->getAnalysisManager()
                            .getCheckerManager()

clang/lib/StaticAnalyzer/Core/Checker.cpp

@@ -21,11 +21,3 @@ StringRef CheckerBase::getDebugTag() const { return getName(); }
 
 void CheckerBackend::printState(raw_ostream &Out, ProgramStateRef State,
                                 const char *NL, const char *Sep) const {}
-
-CheckerProgramPointTag::CheckerProgramPointTag(StringRef CheckerName,
-                                               StringRef Msg)
-  : SimpleProgramPointTag(CheckerName, Msg) {}
-
-CheckerProgramPointTag::CheckerProgramPointTag(const CheckerBase *Checker,
-                                               StringRef Msg)
-    : SimpleProgramPointTag(Checker->getDebugTag(), Msg) {}