[analyzer] RetainCountChecker: for now, do not trust the summaries of inlined code

Trusting summaries of inlined code would require a more thorough work,
as the current approach was causing too many false positives, as the new
example in test.  The culprit lies in the fact that we currently escape
all variables written into a field (but not passed off to unknown
functions!), which can result in inconsistent behavior.

rdar://45655344

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

llvm-svn: 345746

3 files changed, 46 insertions, 37 deletions

diff --git a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
index 7db1465fa1b..3431b1554a1 100644
--- a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
@@ -420,13 +420,6 @@ void RetainCountChecker::processSummaryOfInlined(const RetainSummary &Summ,
   RetEffect RE = Summ.getRetEffect();
 
   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)
       state = removeRefBinding(state, Sym);
   }
@@ -1103,9 +1096,8 @@ RetainCountChecker::checkRegionChanges(ProgramStateRef state,
       WhitelistedSymbols.insert(SR->getSymbol());
   }
 
-  for (InvalidatedSymbols::const_iterator I=invalidated->begin(),
-       E = invalidated->end(); I!=E; ++I) {
-    SymbolRef sym = *I;
+  for (SymbolRef sym :
+       llvm::make_range(invalidated->begin(), invalidated->end())) {
     if (WhitelistedSymbols.count(sym))
       continue;
     // Remove any existing reference-count binding.
diff --git a/clang/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp b/clang/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
index efaab64c770..b0e26bae961 100644
--- a/clang/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
+++ b/clang/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
@@ -101,10 +101,11 @@ static bool isOSObjectRelated(const CXXMethodDecl *MD) {
     return true;
 
   for (ParmVarDecl *Param : MD->parameters()) {
-    QualType PT = Param->getType();
-    if (CXXRecordDecl *RD = PT->getPointeeType()->getAsCXXRecordDecl())
-      if (isOSObjectSubclass(RD))
-        return true;
+    QualType PT = Param->getType()->getPointeeType();
+    if (!PT.isNull())
+      if (CXXRecordDecl *RD = PT->getAsCXXRecordDecl())
+        if (isOSObjectSubclass(RD))
+          return true;
   }
 
   return false;
diff --git a/clang/test/Analysis/osobject-retain-release.cpp b/clang/test/Analysis/osobject-retain-release.cpp
index 1159eaed0d8..47601b0bada 100644
--- a/clang/test/Analysis/osobject-retain-release.cpp
+++ b/clang/test/Analysis/osobject-retain-release.cpp
@@ -2,10 +2,9 @@
 
 struct OSMetaClass;
 
-#define TRUSTED __attribute__((annotate("rc_ownership_trusted_implementation")))
-#define OS_CONSUME TRUSTED __attribute__((annotate("rc_ownership_consumed")))
-#define OS_RETURNS_RETAINED TRUSTED __attribute__((annotate("rc_ownership_returns_retained")))
-#define OS_RETURNS_NOT_RETAINED TRUSTED __attribute__((annotate("rc_ownership_returns_not_retained")))
+#define OS_CONSUME __attribute__((annotate("rc_ownership_consumed")))
+#define OS_RETURNS_RETAINED __attribute__((annotate("rc_ownership_returns_retained")))
+#define OS_RETURNS_NOT_RETAINED __attribute__((annotate("rc_ownership_returns_not_retained")))
 
 #define OSTypeID(type)   (type::metaClass)
 
@@ -62,6 +61,37 @@ void check_no_invalidation_other_struct() {
                           // expected-note@-1{{Object leaked}}
 }
 
+struct ArrayOwner : public OSObject {
+  OSArray *arr;
+  ArrayOwner(OSArray *arr) : arr(arr) {}
+
+  static ArrayOwner* create(OSArray *arr) {
+    return new ArrayOwner(arr);
+  }
+
+  OSArray *getArray() {
+    return arr;
+  }
+
+  OSArray *createArray() {
+    return OSArray::withCapacity(10);
+  }
+
+  OSArray *createArraySourceUnknown();
+
+  OSArray *getArraySourceUnknown();
+};
+
+void check_confusing_getters() {
+  OSArray *arr = OSArray::withCapacity(10);
+
+  ArrayOwner *AO = ArrayOwner::create(arr);
+  AO->getArray();
+
+  AO->release();
+  arr->release();
+}
+
 void check_rc_consumed() {
   OSArray *arr = OSArray::withCapacity(10);
   OSArray::consumeArray(arr);
@@ -140,31 +170,17 @@ void proper_cleanup() {
   arr->release(); // 0
 }
 
-struct ArrayOwner {
-  OSArray *arr;
-
-  OSArray *getArray() {
-    return arr;
-  }
-
-  OSArray *createArray() {
-    return OSArray::withCapacity(10);
-  }
-
-  OSArray *createArraySourceUnknown();
-
-  OSArray *getArraySourceUnknown();
-};
-
 unsigned int no_warning_on_getter(ArrayOwner *owner) {
   OSArray *arr = owner->getArray();
   return arr->getCount();
 }
 
 unsigned int warn_on_overrelease(ArrayOwner *owner) {
-  OSArray *arr = owner->getArray(); // expected-note{{function call returns an OSObject of type struct OSArray * with a +0 retain count}}
-  arr->release(); // expected-warning{{Incorrect decrement of the reference count of an object that is not owned at this point by the caller}}
-                  // expected-note@-1{{Incorrect decrement of the reference count of an object that is not owned at this point by the caller}}
+  // FIXME: summaries are not applied in case the source of the getter/setter
+  // is known.
+  // rdar://45681203
+  OSArray *arr = owner->getArray();
+  arr->release();
   return arr->getCount();
 }