r340961 - [analyzer] Better retain count rules for OSObjects

[George Karpenkov via cfe-commits]

Author: george.karpenkov
Date: Wed Aug 29 13:28:33 2018
New Revision: 340961

URL: http://llvm.org/viewvc/llvm-project?rev=340961&view=rev
Log:
[analyzer] Better retain count rules for OSObjects

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

Modified:
    cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
    cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp?rev=340961&r1=340960&r2=340961&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp Wed Aug 29 13:28:33 2018
@@ -31,6 +31,7 @@ const RefVal *getRefBinding(ProgramState
 
 ProgramStateRef setRefBinding(ProgramStateRef State, SymbolRef Sym,
                                      RefVal Val) {
+  assert(Sym != nullptr);
   return State->set<RefBindings>(Sym, Val);
 }
 
@@ -418,17 +419,19 @@ void RetainCountChecker::processSummaryO
   }
 
   // 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);
 }

Modified: cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp?rev=340961&r1=340960&r2=340961&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp Wed Aug 29 13:28:33 2018
@@ -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::getPersistentSumma
   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(co
   }
 
   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(co
 
   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);
     }
   }