[analyzer] [NFC] Clean up messy handling of bug categories in RetainCountChecker

[george.karpenkov]

Summary

A class is not really a class if only one instance can ever exist.

[NoQ]

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

1268

This code is creepy. On at least one execution path reaching this spot there seems Pred is null due to caching out, and in this case we should not throw a bug report at all.

Also what do we mean by Ctx.addTransition(state, Pred) when Pred is null? I think it simply returns Ctx.getPredecessor() then, and in this case it should crash due to exploded node garbage collection.

But i guess it has always been that way.

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

340

& -> & :)

[george.karpenkov]

Committed in 2bdfe0ca7a5903d24a920c8e46db28261eed0543

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

: public Checker< check::Bind,
                     check::PostStmt<ObjCArrayLiteral>,
                     check::PostStmt<ObjCDictionaryLiteral>,
                     check::PostStmt<ObjCBoxedExpr>,
                     check::PostStmt<ObjCIvarRefExpr>,
                     check::PostCall,
                     check::RegionChanges,
                     eval::Assume,
                     eval::Call > {
-  mutable std::unique_ptr<RefCountBug> useAfterRelease, releaseNotOwned;
-  mutable std::unique_ptr<RefCountBug> deallocNotOwned;
-  mutable std::unique_ptr<RefCountBug> overAutorelease, returnNotOwnedForOwned;
-  mutable std::unique_ptr<RefCountBug> leakWithinFunction, leakAtReturn;
+
+  RefCountBug useAfterRelease{this, RefCountBug::UseAfterRelease};
+  RefCountBug releaseNotOwned{this, RefCountBug::ReleaseNotOwned};
+  RefCountBug deallocNotOwned{this, RefCountBug::DeallocNotOwned};
+  RefCountBug overAutorelease{this, RefCountBug::OverAutorelease};
+  RefCountBug returnNotOwnedForOwned{this, RefCountBug::ReturnNotOwnedForOwned};
+  RefCountBug leakWithinFunction{this, RefCountBug::LeakWithinFunction};
+  RefCountBug leakAtReturn{this, RefCountBug::LeakAtReturn};
 
   mutable std::unique_ptr<RetainSummaryManager> Summaries;
 public:
   static constexpr const char *DeallocTagDescription = "DeallocSent";
 
   /// Track Objective-C and CoreFoundation objects.
   bool TrackObjCAndCFObjects = false;
 
   /// Track sublcasses of OSObject.
   bool TrackOSObjects = false;
 
-  RetainCountChecker() {}
-
-  RefCountBug *getLeakWithinFunctionBug(const LangOptions &LOpts) const;
-
-  RefCountBug *getLeakAtReturnBug(const LangOptions &LOpts) const;
+  RetainCountChecker() {};
 
   RetainSummaryManager &getSummaryManager(ASTContext &Ctx) const {
     // FIXME: We don't support ARC being turned on and off during one analysis.
     // (nor, for that matter, do we support changing ASTContexts)
     bool ARCEnabled = (bool)Ctx.getLangOpts().ObjCAutoRefCount;
     if (!Summaries) {
       Summaries.reset(new RetainSummaryManager(
           Ctx, ARCEnabled, TrackObjCAndCFObjects, TrackOSObjects));
public:
   void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
   void checkBeginFunction(CheckerContext &C) const;
   void checkEndFunction(const ReturnStmt *RS, CheckerContext &C) const;
 
   ProgramStateRef updateSymbol(ProgramStateRef state, SymbolRef sym,
                                RefVal V, ArgEffect E, RefVal::Kind &hasErr,
                                CheckerContext &C) const;
 
+
+  const RefCountBug & errorKindToBugKind(RefVal::Kind ErrorKind) const;
+
   void processNonLeakError(ProgramStateRef St, SourceRange ErrorRange,
                            RefVal::Kind ErrorKind, SymbolRef Sym,
                            CheckerContext &C) const;
 
   void processObjCLiterals(CheckerContext &C, const Expr *Ex) const;
 
   ProgramStateRef handleSymbolDeath(ProgramStateRef state,
                                     SymbolRef sid, RefVal V,

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

static ProgramStateRef setRefBinding(ProgramStateRef State, SymbolRef Sym,
   assert(Sym != nullptr);
   return State->set<RefBindings>(Sym, Val);
 }
 
 static ProgramStateRef removeRefBinding(ProgramStateRef State, SymbolRef Sym) {
   return State->remove<RefBindings>(Sym);
 }
 
-class UseAfterRelease : public RefCountBug {
-public:
-  UseAfterRelease(const CheckerBase *checker)
-      : RefCountBug(checker, "Use-after-release") {}
-
-  const char *getDescription() const override {
-    return "Reference-counted object is used after it is released";
-  }
-};
-
-class BadRelease : public RefCountBug {
-public:
-  BadRelease(const CheckerBase *checker) : RefCountBug(checker, "Bad release") {}
-
-  const char *getDescription() const override {
-    return "Incorrect decrement of the reference count of an object that is "
-           "not owned at this point by the caller";
-  }
-};
-
-class DeallocNotOwned : public RefCountBug {
-public:
-  DeallocNotOwned(const CheckerBase *checker)
-      : RefCountBug(checker, "-dealloc sent to non-exclusively owned object") {}
-
-  const char *getDescription() const override {
-    return "-dealloc sent to object that may be referenced elsewhere";
-  }
-};
-
-class OverAutorelease : public RefCountBug {
-public:
-  OverAutorelease(const CheckerBase *checker)
-      : RefCountBug(checker, "Object autoreleased too many times") {}
-
-  const char *getDescription() const override {
-    return "Object autoreleased too many times";
-  }
-};
-
-class ReturnedNotOwnedForOwned : public RefCountBug {
-public:
-  ReturnedNotOwnedForOwned(const CheckerBase *checker)
-      : RefCountBug(checker, "Method should return an owned object") {}
-
-  const char *getDescription() const override {
-    return "Object with a +0 retain count returned to caller where a +1 "
-           "(owning) retain count is expected";
-  }
-};
-
-class Leak : public RefCountBug {
-public:
-  Leak(const CheckerBase *checker, StringRef name) : RefCountBug(checker, name) {
-    // Leaks should not be reported if they are post-dominated by a sink.
-    setSuppressOnSink(true);
-  }
-
-  const char *getDescription() const override { return ""; }
-
-  bool isLeak() const override { return true; }
-};
-
 } // end namespace retaincountchecker
 } // end namespace ento
 } // end namespace clang
 
 void RefVal::print(raw_ostream &Out) const {
   if (!T.isNull())
     Out << "Tracked " << T.getAsString() << " | ";
 
void RetainCountChecker::checkPostCall(const CallEvent &Call,
 
   if (C.wasInlined) {
     processSummaryOfInlined(*Summ, Call, C);
     return;
   }
   checkSummary(*Summ, Call, C);
 }
 
-RefCountBug *
-RetainCountChecker::getLeakWithinFunctionBug(const LangOptions &LOpts) const {
-  if (!leakWithinFunction)
-    leakWithinFunction.reset(new Leak(this, "Leak"));
-  return leakWithinFunction.get();
-}
-
-RefCountBug *
-RetainCountChecker::getLeakAtReturnBug(const LangOptions &LOpts) const {
-  if (!leakAtReturn)
-    leakAtReturn.reset(new Leak(this, "Leak of returned object"));
-  return leakAtReturn.get();
-}
-
 /// GetReturnType - Used to get the return type of a message expression or
 ///  function call with the intention of affixing that type to a tracked symbol.
 ///  While the return type can be queried directly from RetEx, when
 ///  invoking class methods we augment to the return type to be that of
 ///  a pointer to the class (as opposed it just being id).
 // FIXME: We may be able to do this with related result types instead.
 // This function is probably overestimating.
 static QualType GetReturnType(const Expr *RetE, ASTContext &Ctx) {
case DecRefAndStopTrackingHard:
           }
           break;
       }
       break;
   }
   return setRefBinding(state, sym, V);
 }
 
+const RefCountBug &
+RetainCountChecker::errorKindToBugKind(RefVal::Kind ErrorKind) const {
+  switch (ErrorKind) {
+    case RefVal::ErrorUseAfterRelease:
+      return useAfterRelease;
+    case RefVal::ErrorReleaseNotOwned:
+      return releaseNotOwned;
+    case RefVal::ErrorDeallocNotOwned:
+      return deallocNotOwned;
+    default:
+      llvm_unreachable("Unhandled error.");
+  }
+}
+
 void RetainCountChecker::processNonLeakError(ProgramStateRef St,
                                              SourceRange ErrorRange,
                                              RefVal::Kind ErrorKind,
                                              SymbolRef Sym,
                                              CheckerContext &C) const {
   // HACK: Ignore retain-count issues on values accessed through ivars,
   // because of cases like this:
   //   [_contentView retain];
   //   [_contentView removeFromSuperview];
   //   [self addSubview:_contentView]; // invalidates 'self'
   //   [_contentView release];
   if (const RefVal *RV = getRefBinding(St, Sym))
     if (RV->getIvarAccessHistory() != RefVal::IvarAccessHistory::None)
       return;
 
   ExplodedNode *N = C.generateErrorNode(St);
   if (!N)
     return;
 
-  RefCountBug *BT;
-  switch (ErrorKind) {
-    default:
-      llvm_unreachable("Unhandled error.");
-    case RefVal::ErrorUseAfterRelease:
-      if (!useAfterRelease)
-        useAfterRelease.reset(new UseAfterRelease(this));
-      BT = useAfterRelease.get();
-      break;
-    case RefVal::ErrorReleaseNotOwned:
-      if (!releaseNotOwned)
-        releaseNotOwned.reset(new BadRelease(this));
-      BT = releaseNotOwned.get();
-      break;
-    case RefVal::ErrorDeallocNotOwned:
-      if (!deallocNotOwned)
-        deallocNotOwned.reset(new DeallocNotOwned(this));
-      BT = deallocNotOwned.get();
-      break;
-  }
-
-  assert(BT);
   auto report = llvm::make_unique<RefCountReport>(
-      *BT, C.getASTContext().getLangOpts(), N, Sym);
+      errorKindToBugKind(ErrorKind), C.getASTContext().getLangOpts(), N, Sym);
   report->addRange(ErrorRange);
   C.emitReport(std::move(report));
 }
 
 //===----------------------------------------------------------------------===//
 // Handle the return values of retain-count-related functions.
 //===----------------------------------------------------------------------===//
 
if (RE.getKind() != RetEffect::NoRet) {
 
         // Generate an error node.
         state = setRefBinding(state, Sym, X);
 
         static CheckerProgramPointTag ReturnOwnLeakTag(this, "ReturnsOwnLeak");
         ExplodedNode *N = C.addTransition(state, Pred, &ReturnOwnLeakTag);
         if (N) {
           const LangOptions &LOpts = C.getASTContext().getLangOpts();
-          auto R = llvm::make_unique<RefLeakReport>(
-              *getLeakAtReturnBug(LOpts), LOpts, N, Sym, C);
+          auto R =
+              llvm::make_unique<RefLeakReport>(leakAtReturn, LOpts, N, Sym, C);
           C.emitReport(std::move(R));
         }
         return N;
       }
     }
   } else if (X.isReturnedNotOwned()) {
     if (RE.isOwned()) {
       if (X.getIvarAccessHistory() ==
             RefVal::IvarAccessHistory::AccessedDirectly) {
         // Assume the method was trying to transfer a +1 reference from a
         // strong ivar to the caller.
         state = setRefBinding(state, Sym,
                               X.releaseViaIvar() ^ RefVal::ReturnedOwned);
       } else {
         // Trying to return a not owned object to a caller expecting an
         // owned object.
         state = setRefBinding(state, Sym, X ^ RefVal::ErrorReturnedNotOwned);
 
         static CheckerProgramPointTag
             ReturnNotOwnedTag(this, "ReturnNotOwnedForOwned");
 
         ExplodedNode *N = C.addTransition(state, Pred, &ReturnNotOwnedTag);
         if (N) {
-          if (!returnNotOwnedForOwned)
-            returnNotOwnedForOwned.reset(new ReturnedNotOwnedForOwned(this));
-
           auto R = llvm::make_unique<RefCountReport>(
-              *returnNotOwnedForOwned, C.getASTContext().getLangOpts(), N, Sym);
+              returnNotOwnedForOwned, C.getASTContext().getLangOpts(), N, Sym);
           C.emitReport(std::move(R));
         }
         return N;
       }
     }
   }
   return Pred;
 }
if (N) {
     llvm::raw_svector_ostream os(sbuf);
     os << "Object was autoreleased ";
     if (V.getAutoreleaseCount() > 1)
       os << V.getAutoreleaseCount() << " times but the object ";
     else
       os << "but ";
     os << "has a +" << V.getCount() << " retain count";
 
-    if (!overAutorelease)
-      overAutorelease.reset(new OverAutorelease(this));
-
     const LangOptions &LOpts = Ctx.getASTContext().getLangOpts();
-    auto R = llvm::make_unique<RefCountReport>(*overAutorelease, LOpts, N, Sym,
+    auto R = llvm::make_unique<RefCountReport>(overAutorelease, LOpts, N, Sym,
                                                os.str());
     Ctx.emitReport(std::move(R));
   }
 
   return nullptr;
 }
 
 ProgramStateRef
 RetainCountChecker::handleSymbolDeath(ProgramStateRef state,
RetainCountChecker::processLeaks(ProgramStateRef state,
                                  CheckerContext &Ctx,
                                  ExplodedNode *Pred) const {
   // Generate an intermediate node representing the leak point.
   ExplodedNode *N = Ctx.addTransition(state, Pred);
   const LangOptions &LOpts = Ctx.getASTContext().getLangOpts();
 
   if (N) {
     for (SymbolRef L : Leaked) {
-      RefCountBug *BT = Pred ? getLeakWithinFunctionBug(LOpts)
-                          : getLeakAtReturnBug(LOpts);
-      assert(BT && "BugType not initialized.");
-
-      Ctx.emitReport(llvm::make_unique<RefLeakReport>(*BT, LOpts, N, L, Ctx));
+      const RefCountBug &BT = Pred ? leakWithinFunction : leakAtReturn;
+      Ctx.emitReport(llvm::make_unique<RefLeakReport>(BT, LOpts, N, L, Ctx));
     }
   }
 
   return N;
 }
 
 static bool isISLObjectRef(QualType Ty) {
   return StringRef(Ty.getAsString()).startswith("isl_");

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

 #include "clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h"
 #include "clang/StaticAnalyzer/Core/RetainSummaryManager.h"
 
 namespace clang {
 namespace ento {
 namespace retaincountchecker {
 
 class RefCountBug : public BugType {
-protected:
-  RefCountBug(const CheckerBase *checker, StringRef name)
-      : BugType(checker, name, categories::MemoryRefCount) {}
-
 public:
-  virtual const char *getDescription() const = 0;
+  enum RefCountBugType {
+    UseAfterRelease,
+    ReleaseNotOwned,
+    DeallocNotOwned,
+    OverAutorelease,
+    ReturnNotOwnedForOwned,
+    LeakWithinFunction,
+    LeakAtReturn,
+  };
+  RefCountBug(const CheckerBase *checker, RefCountBugType BT);
+  StringRef getDescription() const;
+  RefCountBugType getBugType() const {
+    return BT;
+  }
 
-  virtual bool isLeak() const { return false; }
+private:
+  RefCountBugType BT;
+  static StringRef bugTypeToName(RefCountBugType BT);
 };
 
 class RefCountReport : public BugReport {
 protected:
   SymbolRef Sym;
+  bool isLeak;
 
 public:
-  RefCountReport(RefCountBug &D, const LangOptions &LOpts,
+  RefCountReport(const RefCountBug &D, const LangOptions &LOpts,
               ExplodedNode *n, SymbolRef sym,
-              bool registerVisitor = true);
+              bool isLeak=false);
 
-  RefCountReport(RefCountBug &D, const LangOptions &LOpts,
+  RefCountReport(const RefCountBug &D, const LangOptions &LOpts,
               ExplodedNode *n, SymbolRef sym,
               StringRef endText);
 
   llvm::iterator_range<ranges_iterator> getRanges() override {
-    const RefCountBug& BugTy = static_cast<RefCountBug&>(getBugType());
-    if (!BugTy.isLeak())
+    if (!isLeak)
       return BugReport::getRanges();
     return llvm::make_range(ranges_iterator(), ranges_iterator());
   }
 };
 
 class RefLeakReport : public RefCountReport {
   const MemRegion* AllocBinding;
   const Stmt *AllocStmt;
 
   // Finds the function declaration where a leak warning for the parameter
   // 'sym' should be raised.
   void deriveParamLocation(CheckerContext &Ctx, SymbolRef sym);
   // Finds the location where a leak warning for 'sym' should be raised.
   void deriveAllocLocation(CheckerContext &Ctx, SymbolRef sym);
   // Produces description of a leak warning which is printed on the console.
   void createDescription(CheckerContext &Ctx);
 
 public:
-  RefLeakReport(RefCountBug &D, const LangOptions &LOpts, ExplodedNode *n,
+  RefLeakReport(const RefCountBug &D, const LangOptions &LOpts, ExplodedNode *n,
                 SymbolRef sym, CheckerContext &Ctx);
 
   PathDiagnosticLocation getLocation(const SourceManager &SM) const override {
     assert(Location.isValid());
     return Location;
   }
 };
 
 } // end namespace retaincountchecker
 } // end namespace ento
 } // end namespace clang
 
 #endif

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

 
 #include "RetainCountDiagnostics.h"
 #include "RetainCountChecker.h"
 
 using namespace clang;
 using namespace ento;
 using namespace retaincountchecker;
 
+StringRef RefCountBug::bugTypeToName(RefCountBug::RefCountBugType BT) {
+  switch (BT) {
+  case UseAfterRelease:
+    return "Use-after-release";
+  case ReleaseNotOwned:
+    return "Bad release";
+  case DeallocNotOwned:
+    return "-dealloc sent to non-exclusively owned object";
+  case OverAutorelease:
+    return "Object autoreleased too many times";
+  case ReturnNotOwnedForOwned:
+    return "Method should return an owned object";
+  case LeakWithinFunction:
+    return "Leak";
+  case LeakAtReturn:
+    return "Leak of returned object";
+  }
+}
+
+StringRef RefCountBug::getDescription() const {
+  switch (BT) {
+  case UseAfterRelease:
+    return "Reference-counted object is used after it is released";
+  case ReleaseNotOwned:
+    return "Incorrect decrement of the reference count of an object that is "
+           "not owned at this point by the caller";
+  case DeallocNotOwned:
+    return "-dealloc sent to object that may be referenced elsewhere";
+  case OverAutorelease:
+    return "Object autoreleased too many times";
+  case ReturnNotOwnedForOwned:
+    return "Object with a +0 retain count returned to caller where a +1 "
+           "(owning) retain count is expected";
+  case LeakWithinFunction:
+  case LeakAtReturn:
+    return "";
+  }
+}
+
+RefCountBug::RefCountBug(const CheckerBase *Checker, RefCountBugType BT)
+    : BugType(Checker, bugTypeToName(BT), categories::MemoryRefCount,
+              /*SupressOnSink=*/BT == LeakWithinFunction || BT == LeakAtReturn),
+      BT(BT) {}
+
 static bool isNumericLiteralExpression(const Expr *E) {
   // FIXME: This set of cases was copied from SemaExprObjC.
   return isa<IntegerLiteral>(E) ||
          isa<CharacterLiteral>(E) ||
          isa<FloatingLiteral>(E) ||
          isa<ObjCBoolLiteralExpr>(E) ||
          isa<CXXBoolLiteralExpr>(E);
 }
RefLeakReportVisitor::getEndPath(BugReporterContext &BRC,
   } else {
     os << " is not referenced later in this execution path and has a retain "
           "count of +" << RV->getCount();
   }
 
   return std::make_shared<PathDiagnosticEventPiece>(L, os.str());
 }
 
-RefCountReport::RefCountReport(RefCountBug &D, const LangOptions &LOpts,
+RefCountReport::RefCountReport(const RefCountBug &D, const LangOptions &LOpts,
                                ExplodedNode *n, SymbolRef sym,
-                               bool registerVisitor)
-    : BugReport(D, D.getDescription(), n), Sym(sym) {
-  if (registerVisitor)
+                               bool isLeak)
+    : BugReport(D, D.getDescription(), n), Sym(sym), isLeak(isLeak) {
+  if (!isLeak)
     addVisitor(llvm::make_unique<RefCountReportVisitor>(sym));
 }
 
-RefCountReport::RefCountReport(RefCountBug &D, const LangOptions &LOpts,
+RefCountReport::RefCountReport(const RefCountBug &D, const LangOptions &LOpts,
                                ExplodedNode *n, SymbolRef sym,
                                StringRef endText)
     : BugReport(D, D.getDescription(), endText, n) {
 
   addVisitor(llvm::make_unique<RefCountReportVisitor>(sym));
 }
 
 void RefLeakReport::deriveParamLocation(CheckerContext &Ctx, SymbolRef sym) {
if (RegionDescription) {
     os << " stored into '" << *RegionDescription << '\'';
   } else {
 
     // If we can't figure out the name, just supply the type information.
     os << " of type '" << getPrettyTypeName(Sym->getType()) << "'";
   }
 }
 
-RefLeakReport::RefLeakReport(RefCountBug &D, const LangOptions &LOpts,
+RefLeakReport::RefLeakReport(const RefCountBug &D, const LangOptions &LOpts,
                              ExplodedNode *n, SymbolRef sym,
                              CheckerContext &Ctx)
-    : RefCountReport(D, LOpts, n, sym, false) {
+    : RefCountReport(D, LOpts, n, sym, /*isLeak=*/true) {
 
   deriveAllocLocation(Ctx, sym);
   if (!AllocBinding)
     deriveParamLocation(Ctx, sym);
 
   createDescription(Ctx);
 
   addVisitor(llvm::make_unique<RefLeakReportVisitor>(sym));
 }