r232849 - [analyzer] RetainCountChecker: Don't assume +0 for ivars backing readonly properties.

[Jordan Rose]

Author: jrose
Date: Fri Mar 20 16:12:27 2015
New Revision: 232849

URL: http://llvm.org/viewvc/llvm-project?rev=232849&view=rev
Log:
[analyzer] RetainCountChecker: Don't assume +0 for ivars backing readonly properties.

Similarly, don't assume +0 if the property's setter is manually implemented.
In both cases, if the property's ownership is explicitly written, then we /do/
assume the ivar has the same ownership.

rdar://problem/20218183

Modified:
    cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
    cfe/trunk/test/Analysis/properties.m

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp?rev=232849&r1=232848&r2=232849&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp Fri Mar 20 16:12:27 2015
@@ -2844,6 +2844,40 @@ static const ObjCPropertyDecl *findPropF
   return nullptr;
 }
 
+namespace {
+  enum Retaining_t {
+    NonRetaining,
+    Retaining
+  };
+}
+
+static Optional<Retaining_t> getRetainSemantics(const ObjCPropertyDecl *Prop) {
+  assert(Prop->getPropertyIvarDecl() &&
+         "should only be used for properties with synthesized implementations");
+
+  if (!Prop->hasWrittenStorageAttribute()) {
+    // Don't assume anything about the retain semantics of readonly properties.
+    if (Prop->isReadOnly())
+      return None;
+
+    // Don't assume anything about readwrite properties with manually-supplied
+    // setters.
+    const ObjCMethodDecl *Setter = Prop->getSetterMethodDecl();
+    bool HasManualSetter = std::any_of(Setter->redecls_begin(),
+                                       Setter->redecls_end(),
+                                       [](const Decl *SetterRedecl) -> bool {
+      return cast<ObjCMethodDecl>(SetterRedecl)->hasBody();
+    });
+    if (HasManualSetter)
+      return None;
+
+    // If the setter /is/ synthesized, we're already relying on the retain
+    // semantics of the property. Continue as normal.
+  }
+
+  return Prop->isRetaining() ? Retaining : NonRetaining;
+}
+
 void RetainCountChecker::checkPostStmt(const ObjCIvarRefExpr *IRE,
                                        CheckerContext &C) const {
   Optional<Loc> IVarLoc = C.getSVal(IRE).getAs<Loc>();
@@ -2884,8 +2918,9 @@ void RetainCountChecker::checkPostStmt(c
     // there's no outstanding retain count for the value.
     if (Kind == RetEffect::ObjC)
       if (const ObjCPropertyDecl *Prop = findPropForIvar(IRE->getDecl()))
-        if (!Prop->isRetaining())
-          return;
+        if (auto retainSemantics = getRetainSemantics(Prop))
+          if (retainSemantics.getValue() == NonRetaining)
+            return;
 
     // Note that this value has been loaded from an ivar.
     C.addTransition(setRefBinding(State, Sym, RV->withIvarAccess()));
@@ -2900,18 +2935,23 @@ void RetainCountChecker::checkPostStmt(c
     return;
   }
 
-  // Try to find the property associated with this ivar.
-  if (Kind != RetEffect::ObjC) {
-    State = setRefBinding(State, Sym, PlusZero.withIvarAccess());
-  } else {
-    const ObjCPropertyDecl *Prop = findPropForIvar(IRE->getDecl());
-
-    if (Prop && !Prop->isRetaining())
-      State = setRefBinding(State, Sym, PlusZero);
-    else
-      State = setRefBinding(State, Sym, PlusZero.withIvarAccess());
+  bool didUpdateState = false;
+  if (Kind == RetEffect::ObjC) {
+    // Check if the ivar is known to be unretained. If so, we know that
+    // there's no outstanding retain count for the value.
+    if (const ObjCPropertyDecl *Prop = findPropForIvar(IRE->getDecl())) {
+      if (auto retainSemantics = getRetainSemantics(Prop)) {
+        if (retainSemantics.getValue() == NonRetaining) {
+          State = setRefBinding(State, Sym, PlusZero);
+          didUpdateState = true;
+        }
+      }
+    }
   }
 
+  if (!didUpdateState)
+    State = setRefBinding(State, Sym, PlusZero.withIvarAccess());
+
   C.addTransition(State);
 }
 

Modified: cfe/trunk/test/Analysis/properties.m
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/properties.m?rev=232849&r1=232848&r2=232849&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/properties.m (original)
+++ cfe/trunk/test/Analysis/properties.m Fri Mar 20 16:12:27 2015
@@ -356,6 +356,9 @@ void testOpaqueConsistency(OpaqueIntWrap
 @property (strong) id ownedProp;
 @property (unsafe_unretained) id unownedProp;
 @property (nonatomic, strong) id manualProp;
+ at property (readonly) id readonlyProp;
+ at property (nonatomic, readwrite/*, assign */) id implicitManualProp; // expected-warning {{'assign' is assumed}} expected-warning {{'assign' not appropriate}}
+ at property (nonatomic, readwrite/*, assign */) id implicitSynthProp; // expected-warning {{'assign' is assumed}} expected-warning {{'assign' not appropriate}}
 @property CFTypeRef cfProp;
 @end
 
@@ -367,6 +370,8 @@ void testOpaqueConsistency(OpaqueIntWrap
   return _manualProp;
 }
 
+- (void)setImplicitManualProp:(id)newValue {}
+
 - (void)testOverreleaseOwnedIvar {
   [_ownedProp retain];
   [_ownedProp release];
@@ -387,6 +392,26 @@ void testOpaqueConsistency(OpaqueIntWrap
   [_ivarOnly release]; // expected-warning{{used after it is released}}
 }
 
+- (void)testOverreleaseReadonlyIvar {
+  [_readonlyProp retain];
+  [_readonlyProp release];
+  [_readonlyProp release];
+  [_readonlyProp release]; // expected-warning{{used after it is released}}
+}
+
+- (void)testOverreleaseImplicitManualIvar {
+  [_implicitManualProp retain];
+  [_implicitManualProp release];
+  [_implicitManualProp release];
+  [_implicitManualProp release]; // expected-warning{{used after it is released}}
+}
+
+- (void)testOverreleaseImplicitSynthIvar {
+  [_implicitSynthProp retain];
+  [_implicitSynthProp release];
+  [_implicitSynthProp release]; // expected-warning{{not owned at this point by the caller}}
+}
+
 - (void)testOverreleaseCF {
   CFRetain(_cfProp);
   CFRelease(_cfProp);
@@ -501,6 +526,48 @@ void testOpaqueConsistency(OpaqueIntWrap
   clang_analyzer_eval(owned == fromIvar); // expected-warning{{TRUE}}
 }
 
+- (void)testPropertyAccessThenReleaseReadonly {
+  id prop = [self.readonlyProp retain];
+  [prop release];
+  [_readonlyProp release]; // no-warning
+}
+
+- (void)testPropertyAccessThenReleaseReadonly2 {
+  id fromIvar = _readonlyProp;
+  id prop = [self.readonlyProp retain];
+  [prop release];
+  clang_analyzer_eval(prop == fromIvar); // expected-warning{{TRUE}}
+  [fromIvar release]; // no-warning
+}
+
+- (void)testPropertyAccessThenReleaseImplicitManual {
+  id prop = [self.implicitManualProp retain];
+  [prop release];
+  [_implicitManualProp release]; // no-warning
+}
+
+- (void)testPropertyAccessThenReleaseImplicitManual2 {
+  id fromIvar = _implicitManualProp;
+  id prop = [self.implicitManualProp retain];
+  [prop release];
+  clang_analyzer_eval(prop == fromIvar); // expected-warning{{TRUE}}
+  [fromIvar release]; // no-warning
+}
+
+- (void)testPropertyAccessThenReleaseImplicitSynth {
+  id prop = [self.implicitSynthProp retain];
+  [prop release];
+  [_implicitSynthProp release]; // expected-warning{{not owned}}
+}
+
+- (void)testPropertyAccessThenReleaseImplicitSynth2 {
+  id fromIvar = _implicitSynthProp;
+  id prop = [self.implicitSynthProp retain];
+  [prop release];
+  clang_analyzer_eval(prop == fromIvar); // expected-warning{{TRUE}}
+  [fromIvar release]; // expected-warning{{not owned}}
+}
+
 - (id)getUnownedFromProperty {
   [_ownedProp retain];
   [_ownedProp autorelease];
@@ -539,6 +606,21 @@ void testOpaqueConsistency(OpaqueIntWrap
   CFRelease(_cfProp); // FIXME: no-warning{{not owned}}
 }
 
+- (void)testAssignReadonly:(id)newValue {
+  _readonlyProp = newValue;
+  [_readonlyProp release]; // FIXME: no-warning{{not owned}}
+}
+
+- (void)testAssignImplicitManual:(id)newValue {
+  _implicitManualProp = newValue;
+  [_implicitManualProp release]; // FIXME: no-warning{{not owned}}
+}
+
+- (void)testAssignImplicitSynth:(id)newValue {
+  _implicitSynthProp = newValue;
+  [_implicitSynthProp release]; // FIXME: no-warning{{not owned}}
+}
+
 - (void)testAssignOwnedOkay:(id)newValue {
   _ownedProp = [newValue retain];
   [_ownedProp release]; // no-warning
@@ -559,6 +641,21 @@ void testOpaqueConsistency(OpaqueIntWrap
   CFRelease(_cfProp); // no-warning
 }
 
+- (void)testAssignReadonlyOkay:(id)newValue {
+  _readonlyProp = [newValue retain];
+  [_readonlyProp release]; // FIXME: no-warning{{not owned}}
+}
+
+- (void)testAssignImplicitManualOkay:(id)newValue {
+  _implicitManualProp = [newValue retain];
+  [_implicitManualProp release]; // FIXME: no-warning{{not owned}}
+}
+
+- (void)testAssignImplicitSynthOkay:(id)newValue {
+  _implicitSynthProp = [newValue retain];
+  [_implicitSynthProp release]; // FIXME: no-warning{{not owned}}
+}
+
 // rdar://problem/19862648
 - (void)establishIvarIsNilDuringLoops {
   extern id getRandomObject();