[analyzer] [NFC] Minor refactoring of RetainCountDiagnostics

[george.karpenkov]

Summary

Move visitors to the implementation file, move a complicated logic into a function.

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

public:
   bool IncludeAllocationLine;
   bool ShouldCheckOSObjectRetainCount;
 
   RetainCountChecker() : ShouldResetSummaryLog(false) {}
 
   ~RetainCountChecker() override { DeleteContainerSeconds(DeadSymbolTags); }
 
   void checkEndAnalysis(ExplodedGraph &G, BugReporter &BR,
-                        ExprEngine &Eng) const {
-    // FIXME: This is a hack to make sure the summary log gets cleared between
-    // analyses of different code bodies.
-    //
-    // Why is this necessary? Because a checker's lifetime is tied to a
-    // translation unit, but an ExplodedGraph's lifetime is just a code body.
-    // Once in a blue moon, a new ExplodedNode will have the same address as an
-    // old one with an associated summary, and the bug report visitor gets very
-    // confused. (To make things worse, the summary lifetime is currently also
-    // tied to a code body, so we get a crash instead of incorrect results.)
-    //
-    // Why is this a bad solution? Because if the lifetime of the ExplodedGraph
-    // changes, things will start going wrong again. Really the lifetime of this
-    // log needs to be tied to either the specific nodes in it or the entire
-    // ExplodedGraph, not to a specific part of the code being analyzed.
-    //
-    // (Also, having stateful local data means that the same checker can't be
-    // used from multiple threads, but a lot of checkers have incorrect
-    // assumptions about that anyway. So that wasn't a priority at the time of
-    // this fix.)
-    //
-    // This happens at the end of analysis, but bug reports are emitted /after/
-    // this point. So we can't just clear the summary log now. Instead, we mark
-    // that the next time we access the summary log, it should be cleared.
-
-    // If we never reset the summary log during /this/ code body analysis,
-    // there were no new summaries. There might still have been summaries from
-    // the /last/ analysis, so clear them out to make sure the bug report
-    // visitors don't get confused.
-    if (ShouldResetSummaryLog)
-      SummaryLog.clear();
-
-    ShouldResetSummaryLog = !SummaryLog.empty();
-  }
-
-  CFRefBug *getLeakWithinFunctionBug(const LangOptions &LOpts) const {
-    if (!leakWithinFunction)
-      leakWithinFunction.reset(new Leak(this, "Leak"));
-    return leakWithinFunction.get();
-  }
-
-  CFRefBug *getLeakAtReturnBug(const LangOptions &LOpts) const {
-      if (!leakAtReturn)
-        leakAtReturn.reset(new Leak(this, "Leak of returned object"));
-      return leakAtReturn.get();
-  }
+                        ExprEngine &Eng) const;
+
+  CFRefBug *getLeakWithinFunctionBug(const LangOptions &LOpts) const;
+
+  CFRefBug *getLeakAtReturnBug(const LangOptions &LOpts) const;
 
   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, ShouldCheckOSObjectRetainCount));

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

ProgramStateRef setRefBinding(ProgramStateRef State, SymbolRef Sym,
   assert(Sym != nullptr);
   return State->set<RefBindings>(Sym, Val);
 }
 
 ProgramStateRef removeRefBinding(ProgramStateRef State, SymbolRef Sym) {
   return State->remove<RefBindings>(Sym);
 }
 
+class UseAfterRelease : public CFRefBug {
+public:
+  UseAfterRelease(const CheckerBase *checker)
+      : CFRefBug(checker, "Use-after-release") {}
+
+  const char *getDescription() const override {
+    return "Reference-counted object is used after it is released";
+  }
+};
+
+class BadRelease : public CFRefBug {
+public:
+  BadRelease(const CheckerBase *checker) : CFRefBug(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 CFRefBug {
+public:
+  DeallocNotOwned(const CheckerBase *checker)
+      : CFRefBug(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 CFRefBug {
+public:
+  OverAutorelease(const CheckerBase *checker)
+      : CFRefBug(checker, "Object autoreleased too many times") {}
+
+  const char *getDescription() const override {
+    return "Object autoreleased too many times";
+  }
+};
+
+class ReturnedNotOwnedForOwned : public CFRefBug {
+public:
+  ReturnedNotOwnedForOwned(const CheckerBase *checker)
+      : CFRefBug(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 CFRefBug {
+public:
+  Leak(const CheckerBase *checker, StringRef name) : CFRefBug(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);
 }
 
+void RetainCountChecker::checkEndAnalysis(ExplodedGraph &G, BugReporter &BR,
+                                          ExprEngine &Eng) const {
+  // FIXME: This is a hack to make sure the summary log gets cleared between
+  // analyses of different code bodies.
+  //
+  // Why is this necessary? Because a checker's lifetime is tied to a
+  // translation unit, but an ExplodedGraph's lifetime is just a code body.
+  // Once in a blue moon, a new ExplodedNode will have the same address as an
+  // old one with an associated summary, and the bug report visitor gets very
+  // confused. (To make things worse, the summary lifetime is currently also
+  // tied to a code body, so we get a crash instead of incorrect results.)
+  //
+  // Why is this a bad solution? Because if the lifetime of the ExplodedGraph
+  // changes, things will start going wrong again. Really the lifetime of this
+  // log needs to be tied to either the specific nodes in it or the entire
+  // ExplodedGraph, not to a specific part of the code being analyzed.
+  //
+  // (Also, having stateful local data means that the same checker can't be
+  // used from multiple threads, but a lot of checkers have incorrect
+  // assumptions about that anyway. So that wasn't a priority at the time of
+  // this fix.)
+  //
+  // This happens at the end of analysis, but bug reports are emitted /after/
+  // this point. So we can't just clear the summary log now. Instead, we mark
+  // that the next time we access the summary log, it should be cleared.
+
+  // If we never reset the summary log during /this/ code body analysis,
+  // there were no new summaries. There might still have been summaries from
+  // the /last/ analysis, so clear them out to make sure the bug report
+  // visitors don't get confused.
+  if (ShouldResetSummaryLog)
+    SummaryLog.clear();
+
+  ShouldResetSummaryLog = !SummaryLog.empty();
+}
+
+CFRefBug *
+RetainCountChecker::getLeakWithinFunctionBug(const LangOptions &LOpts) const {
+  if (!leakWithinFunction)
+    leakWithinFunction.reset(new Leak(this, "Leak"));
+  return leakWithinFunction.get();
+}
+
+CFRefBug *
+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) {

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

 public:
 
   // FIXME: Eventually remove.
   virtual const char *getDescription() const = 0;
 
   virtual bool isLeak() const { return false; }
 };
 
-class UseAfterRelease : public CFRefBug {
-public:
-  UseAfterRelease(const CheckerBase *checker)
-      : CFRefBug(checker, "Use-after-release") {}
-
-  const char *getDescription() const override {
-    return "Reference-counted object is used after it is released";
-  }
-};
-
-class BadRelease : public CFRefBug {
-public:
-  BadRelease(const CheckerBase *checker) : CFRefBug(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 CFRefBug {
-public:
-  DeallocNotOwned(const CheckerBase *checker)
-      : CFRefBug(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 CFRefBug {
-public:
-  OverAutorelease(const CheckerBase *checker)
-      : CFRefBug(checker, "Object autoreleased too many times") {}
-
-  const char *getDescription() const override {
-    return "Object autoreleased too many times";
-  }
-};
-
-class ReturnedNotOwnedForOwned : public CFRefBug {
-public:
-  ReturnedNotOwnedForOwned(const CheckerBase *checker)
-      : CFRefBug(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 CFRefBug {
-public:
-  Leak(const CheckerBase *checker, StringRef name) : CFRefBug(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; }
-};
-
 typedef ::llvm::DenseMap<const ExplodedNode *, const RetainSummary *>
   SummaryLogTy;
 
-/// Visitors.
-
-class CFRefReportVisitor : public BugReporterVisitor {
-protected:
-  SymbolRef Sym;
-  const SummaryLogTy &SummaryLog;
-
-public:
-  CFRefReportVisitor(SymbolRef sym, const SummaryLogTy &log)
-     : Sym(sym), SummaryLog(log) {}
-
-  void Profile(llvm::FoldingSetNodeID &ID) const override {
-    static int x = 0;
-    ID.AddPointer(&x);
-    ID.AddPointer(Sym);
-  }
-
-  std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
-                                                 BugReporterContext &BRC,
-                                                 BugReport &BR) override;
-
-  std::shared_ptr<PathDiagnosticPiece> getEndPath(BugReporterContext &BRC,
-                                                  const ExplodedNode *N,
-                                                  BugReport &BR) override;
-};
-
-class CFRefLeakReportVisitor : public CFRefReportVisitor {
-public:
-  CFRefLeakReportVisitor(SymbolRef sym,
-                         const SummaryLogTy &log)
-     : CFRefReportVisitor(sym, log) {}
-
-  std::shared_ptr<PathDiagnosticPiece> getEndPath(BugReporterContext &BRC,
-                                                  const ExplodedNode *N,
-                                                  BugReport &BR) override;
-};
-
 class CFRefReport : public BugReport {
 protected:
   SymbolRef Sym;
 
 public:
   CFRefReport(CFRefBug &D, const LangOptions &LOpts,
               const SummaryLogTy &Log, ExplodedNode *n, SymbolRef sym,
-              bool registerVisitor = true)
-    : BugReport(D, D.getDescription(), n), Sym(sym) {
-    if (registerVisitor)
-      addVisitor(llvm::make_unique<CFRefReportVisitor>(sym, Log));
-  }
+              bool registerVisitor = true);
 
   CFRefReport(CFRefBug &D, const LangOptions &LOpts,
               const SummaryLogTy &Log, ExplodedNode *n, SymbolRef sym,
-              StringRef endText)
-    : BugReport(D, D.getDescription(), endText, n) {
-    addVisitor(llvm::make_unique<CFRefReportVisitor>(sym, Log));
-  }
+              StringRef endText);
 
   llvm::iterator_range<ranges_iterator> getRanges() override {
     const CFRefBug& BugTy = static_cast<CFRefBug&>(getBugType());
     if (!BugTy.isLeak())
       return BugReport::getRanges();
     return llvm::make_range(ranges_iterator(), ranges_iterator());
   }
 };

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

case RefVal::ReturnedNotOwned:
       return true;
 
     default:
       return false;
     }
   return true;
 }
 
+static void generateDiagnosticsForCallLike(
+  ProgramStateRef CurrSt,
+  const LocationContext *LCtx,
+  const RefVal &CurrV,
+  SymbolRef &Sym,
+  const Stmt *S,
+  llvm::raw_string_ostream &os) {
+  if (const CallExpr *CE = dyn_cast<CallExpr>(S)) {
+    // Get the name of the callee (if it is available)
+    // from the tracked SVal.
+    SVal X = CurrSt->getSValAsScalarOrLoc(CE->getCallee(), LCtx);
+    const FunctionDecl *FD = X.getAsFunctionDecl();
+
+    // If failed, try to get it from AST.
+    if (!FD)
+      FD = dyn_cast<FunctionDecl>(CE->getCalleeDecl());
+
+    if (FD) {
+      os << "Call to function '" << FD->getQualifiedNameAsString() << '\'';
+    } else {
+      os << "function call";
+    }
+  } else {
+    assert(isa<ObjCMessageExpr>(S));
+    CallEventManager &Mgr = CurrSt->getStateManager().getCallEventManager();
+    CallEventRef<ObjCMethodCall> Call =
+        Mgr.getObjCMethodCall(cast<ObjCMessageExpr>(S), CurrSt, LCtx);
+
+    switch (Call->getMessageKind()) {
+    case OCM_Message:
+      os << "Method";
+      break;
+    case OCM_PropertyAccess:
+      os << "Property";
+      break;
+    case OCM_Subscript:
+      os << "Subscript";
+      break;
+    }
+  }
+
+  if (CurrV.getObjKind() == RetEffect::CF) {
+    os << " returns a Core Foundation object of type "
+       << Sym->getType().getAsString() << " with a ";
+  } else if (CurrV.getObjKind() == RetEffect::OS) {
+    os << " returns an OSObject of type " << Sym->getType().getAsString()
+       << " with a ";
+  } else if (CurrV.getObjKind() == RetEffect::Generalized) {
+    os << " returns an object of type " << Sym->getType().getAsString()
+       << " with a ";
+  } else {
+    assert(CurrV.getObjKind() == RetEffect::ObjC);
+    QualType T = Sym->getType();
+    if (!isa<ObjCObjectPointerType>(T)) {
+      os << " returns an Objective-C object with a ";
+    } else {
+      const ObjCObjectPointerType *PT = cast<ObjCObjectPointerType>(T);
+      os << " returns an instance of " << PT->getPointeeType().getAsString()
+         << " with a ";
+    }
+  }
+
+  if (CurrV.isOwned()) {
+    os << "+1 retain count";
+  } else {
+    assert(CurrV.isNotOwned());
+    os << "+0 retain count";
+  }
+}
+
+namespace clang {
+namespace ento {
+namespace retaincountchecker {
+
+class CFRefReportVisitor : public BugReporterVisitor {
+protected:
+  SymbolRef Sym;
+  const SummaryLogTy &SummaryLog;
+
+public:
+  CFRefReportVisitor(SymbolRef sym, const SummaryLogTy &log)
+      : Sym(sym), SummaryLog(log) {}
+
+  void Profile(llvm::FoldingSetNodeID &ID) const override {
+    static int x = 0;
+    ID.AddPointer(&x);
+    ID.AddPointer(Sym);
+  }
+
+  std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
+                                                 BugReporterContext &BRC,
+                                                 BugReport &BR) override;
+
+  std::shared_ptr<PathDiagnosticPiece> getEndPath(BugReporterContext &BRC,
+                                                  const ExplodedNode *N,
+                                                  BugReport &BR) override;
+};
+
+class CFRefLeakReportVisitor : public CFRefReportVisitor {
+public:
+  CFRefLeakReportVisitor(SymbolRef sym,
+                         const SummaryLogTy &log)
+     : CFRefReportVisitor(sym, log) {}
+
+  std::shared_ptr<PathDiagnosticPiece> getEndPath(BugReporterContext &BRC,
+                                                  const ExplodedNode *N,
+                                                  BugReport &BR) override;
+};
+
+} // end namespace retaincountchecker
+} // end namespace ento
+} // end namespace clang
+
 std::shared_ptr<PathDiagnosticPiece>
 CFRefReportVisitor::VisitNode(const ExplodedNode *N,
                               BugReporterContext &BRC, BugReport &BR) {
   // FIXME: We will eventually need to handle non-statement-based events
   // (__attribute__((cleanup))).
   if (!N->getLocation().getAs<StmtPoint>())
     return nullptr;
 
   // Check if the type state has changed.
-  ProgramStateRef PrevSt = N->getFirstPred()->getState();
+  const ExplodedNode *PrevNode = N->getFirstPred();
+  ProgramStateRef PrevSt = PrevNode->getState();
   ProgramStateRef CurrSt = N->getState();
   const LocationContext *LCtx = N->getLocationContext();
 
   const RefVal* CurrT = getRefBinding(CurrSt, Sym);
   if (!CurrT) return nullptr;
 
   const RefVal &CurrV = *CurrT;
   const RefVal *PrevT = getRefBinding(PrevSt, Sym);
if (isa<ObjCArrayLiteral>(S)) {
           os << "B";
         }
 
         os << "oxed expression produces an object with a +0 retain count";
       }
     } else if (isa<ObjCIvarRefExpr>(S)) {
       os << "Object loaded from instance variable";
     } else {
-      if (const CallExpr *CE = dyn_cast<CallExpr>(S)) {
-        // Get the name of the callee (if it is available)
-        // from the tracked SVal.
-        SVal X = CurrSt->getSValAsScalarOrLoc(CE->getCallee(), LCtx);
-        const FunctionDecl *FD = X.getAsFunctionDecl();
-
-        // If failed, try to get it from AST.
-        if (!FD)
-          FD = dyn_cast<FunctionDecl>(CE->getCalleeDecl());
-
-        if (FD) {
-          os << "Call to function '" << FD->getQualifiedNameAsString() << '\'';
-        } else {
-          os << "function call";
-        }
-      } else {
-        assert(isa<ObjCMessageExpr>(S));
-        CallEventManager &Mgr = CurrSt->getStateManager().getCallEventManager();
-        CallEventRef<ObjCMethodCall> Call
-          = Mgr.getObjCMethodCall(cast<ObjCMessageExpr>(S), CurrSt, LCtx);
-
-        switch (Call->getMessageKind()) {
-        case OCM_Message:
-          os << "Method";
-          break;
-        case OCM_PropertyAccess:
-          os << "Property";
-          break;
-        case OCM_Subscript:
-          os << "Subscript";
-          break;
-        }
-      }
-
-      if (CurrV.getObjKind() == RetEffect::CF) {
-        os << " returns a Core Foundation object of type "
-           << Sym->getType().getAsString() << " with a ";
-      } else if (CurrV.getObjKind() == RetEffect::OS) {
-        os << " returns an OSObject of type "
-           << Sym->getType().getAsString() << " with a ";
-      } else if (CurrV.getObjKind() == RetEffect::Generalized) {
-        os << " returns an object of type " << Sym->getType().getAsString()
-           << " with a ";
-      } else {
-        assert (CurrV.getObjKind() == RetEffect::ObjC);
-        QualType T = Sym->getType();
-        if (!isa<ObjCObjectPointerType>(T)) {
-          os << " returns an Objective-C object with a ";
-        } else {
-          const ObjCObjectPointerType *PT = cast<ObjCObjectPointerType>(T);
-          os << " returns an instance of "
-             << PT->getPointeeType().getAsString() << " with a ";
-        }
-      }
-
-      if (CurrV.isOwned()) {
-        os << "+1 retain count";
-      } else {
-        assert (CurrV.isNotOwned());
-        os << "+0 retain count";
-      }
+      generateDiagnosticsForCallLike(CurrSt, LCtx, CurrV, Sym, S, os);
     }
 
     PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
                                   N->getLocationContext());
     return std::make_shared<PathDiagnosticEventPiece>(Pos, os.str());
   }
 
   // Gather up the effects that were performed on the object at this
CFRefLeakReportVisitor::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());
 }
 
+CFRefReport::CFRefReport(CFRefBug &D, const LangOptions &LOpts,
+                         const SummaryLogTy &Log, ExplodedNode *n,
+                         SymbolRef sym, bool registerVisitor)
+    : BugReport(D, D.getDescription(), n), Sym(sym) {
+  if (registerVisitor)
+    addVisitor(llvm::make_unique<CFRefReportVisitor>(sym, Log));
+}
+
+CFRefReport::CFRefReport(CFRefBug &D, const LangOptions &LOpts,
+                         const SummaryLogTy &Log, ExplodedNode *n,
+                         SymbolRef sym, StringRef endText)
+    : BugReport(D, D.getDescription(), endText, n) {
+
+  addVisitor(llvm::make_unique<CFRefReportVisitor>(sym, Log));
+}
+
 void CFRefLeakReport::deriveParamLocation(CheckerContext &Ctx, SymbolRef sym) {
   const SourceManager& SMgr = Ctx.getSourceManager();
 
   if (!sym->getOriginRegion())
     return;
 
   auto *Region = dyn_cast<DeclRegion>(sym->getOriginRegion());
   if (Region) {

clang/test/Analysis/osobject-retain-release.cpp

 struct OSIterator : public OSObject {
 
   static const OSMetaClass * const metaClass;
 };
 
 struct OSArray : public OSObject {
   unsigned int getCount();
 
+  static OSArray *generateArrayHasCode() {
+    return new OSArray;
+  }
+
   static OSArray *withCapacity(unsigned int capacity);
   static void consumeArray(OS_CONSUME OSArray * array);
 
   static OSArray* consumeArrayHasCode(OS_CONSUME OSArray * array) {
     return nullptr;
   }
 
   OSIterator * getIterator();