From 9af55c6d39f2390a6a9a7ca78c3474d9bf41f3b0 Mon Sep 17 00:00:00 2001
From: George Karpenkov <ekarpenkov@apple.com>
Date: Wed, 29 Aug 2018 20:28:33 +0000
Subject: [PATCH] [analyzer] Better retain count rules for OSObjects

Differential Revision: https://reviews.llvm.org/D51184

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@340961 91177308-0d34-0410-b5e6-96231b3b80d8
---
 .../RetainCountChecker/RetainCountChecker.cpp | 19 ++++---
 .../Core/RetainSummaryManager.cpp             | 56 +++++++++----------
 2 files changed, 38 insertions(+), 37 deletions(-)

diff --git a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
index 2b39ad6fa6..351816ae20 100644
--- a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
+++ b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
@@ -31,6 +31,7 @@ const RefVal *getRefBinding(ProgramStateRef State, SymbolRef Sym) {
 
 ProgramStateRef setRefBinding(ProgramStateRef State, SymbolRef Sym,
                                      RefVal Val) {
+  assert(Sym != nullptr);
   return State->set<RefBindings>(Sym, Val);
 }
 
@@ -418,17 +419,19 @@ void RetainCountChecker::processSummaryOfInlined(const RetainSummary &Summ,
   }
 
   // Consult the summary for the return value.
-  SymbolRef Sym = CallOrMsg.getReturnValue().getAsSymbol();
   RetEffect RE = Summ.getRetEffect();
-  if (const auto *MCall = dyn_cast<CXXMemberCall>(&CallOrMsg)) {
-    if (Optional<RefVal> updatedRefVal =
-            refValFromRetEffect(RE, MCall->getResultType())) {
-      state = setRefBinding(state, Sym, *updatedRefVal);
+
+  if (SymbolRef Sym = CallOrMsg.getReturnValue().getAsSymbol()) {
+    if (const auto *MCall = dyn_cast<CXXMemberCall>(&CallOrMsg)) {
+      if (Optional<RefVal> updatedRefVal =
+              refValFromRetEffect(RE, MCall->getResultType())) {
+        state = setRefBinding(state, Sym, *updatedRefVal);
+      }
     }
-  }
 
-  if (RE.getKind() == RetEffect::NoRetHard && Sym)
-    state = removeRefBinding(state, Sym);
+    if (RE.getKind() == RetEffect::NoRetHard)
+      state = removeRefBinding(state, Sym);
+  }
 
   C.addTransition(state);
 }
diff --git a/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp b/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
index e67a441a3d..c16b141e38 100644
--- a/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
+++ b/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
@@ -19,6 +19,7 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/ParentMap.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
 
 using namespace clang;
 using namespace ento;
@@ -53,29 +54,19 @@ RetainSummaryManager::getPersistentSummary(const RetainSummary &OldSumm) {
   return Summ;
 }
 
-static bool isOSObjectSubclass(QualType T);
-
-static bool isOSObjectSubclass(const CXXRecordDecl *RD) {
-  if (RD->getDeclName().getAsString() == "OSObject")
-    return true;
-
-  const CXXRecordDecl *RDD = RD->getDefinition();
-  if (!RDD)
-    return false;
+static bool isSubclass(const Decl *D,
+                       StringRef ClassName) {
+  using namespace ast_matchers;
+  DeclarationMatcher SubclassM = cxxRecordDecl(isSameOrDerivedFrom(ClassName));
+  return !(match(SubclassM, *D, D->getASTContext()).empty());
+}
 
-  for (const CXXBaseSpecifier Spec : RDD->bases()) {
-    if (isOSObjectSubclass(Spec.getType()))
-      return true;
-  }
-  return false;
+static bool isOSObjectSubclass(const Decl *D) {
+  return isSubclass(D, "OSObject");
 }
 
-/// \return Whether type represents an OSObject successor.
-static bool isOSObjectSubclass(QualType T) {
-  if (const auto *RD = T->getAsCXXRecordDecl()) {
-    return isOSObjectSubclass(RD);
-  }
-  return false;
+static bool isOSIteratorSubclass(const Decl *D) {
+  return isSubclass(D, "OSIterator");
 }
 
 static bool hasRCAnnotation(const Decl *D, StringRef rcAnnotation) {
@@ -221,15 +212,22 @@ RetainSummaryManager::generateSummary(const FunctionDecl *FD,
   }
 
   if (RetTy->isPointerType()) {
-    if (TrackOSObjects && isOSObjectSubclass(RetTy->getPointeeType())) {
+
+    const CXXRecordDecl *PD = RetTy->getPointeeType()->getAsCXXRecordDecl();
+    if (TrackOSObjects && PD && isOSObjectSubclass(PD)) {
       if (const IdentifierInfo *II = FD->getIdentifier()) {
-        StringRef FuncName = II->getName();
-        if (FuncName.contains_lower("with")
-            || FuncName.contains_lower("create")
-            || FuncName.contains_lower("copy"))
+
+        // All objects returned with functions starting with "get" are getters.
+        if (II->getName().startswith("get")) {
+
+          // ...except for iterators.
+          if (isOSIteratorSubclass(PD))
+            return getOSSummaryCreateRule(FD);
+          return getOSSummaryGetRule(FD);
+        } else {
           return getOSSummaryCreateRule(FD);
+        }
       }
-      return getOSSummaryGetRule(FD);
     }
 
     // For CoreFoundation ('CF') types.
@@ -279,11 +277,11 @@ RetainSummaryManager::generateSummary(const FunctionDecl *FD,
 
   if (const auto *MD = dyn_cast<CXXMethodDecl>(FD)) {
     const CXXRecordDecl *Parent = MD->getParent();
-    if (TrackOSObjects && isOSObjectSubclass(Parent)) {
-      if (isRelease(FD, FName))
+    if (TrackOSObjects && Parent && isOSObjectSubclass(Parent)) {
+      if (FName == "release")
         return getOSSummaryReleaseRule(FD);
 
-      if (isRetain(FD, FName))
+      if (FName == "retain")
         return getOSSummaryRetainRule(FD);
     }
   }
-- 
2.40.0