From a1f095a9dbcd425f6fadf39da6f42594edebaddd Mon Sep 17 00:00:00 2001 From: Jordan Rose Date: Fri, 20 Mar 2015 21:12:27 +0000 Subject: [PATCH] [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 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@232849 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../Checkers/RetainCountChecker.cpp | 64 +++++++++--- test/Analysis/properties.m | 97 +++++++++++++++++++ 2 files changed, 149 insertions(+), 12 deletions(-) diff --git a/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp b/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp index 1991d2bad1..6b8596efb1 100644 --- a/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp @@ -2844,6 +2844,40 @@ static const ObjCPropertyDecl *findPropForIvar(const ObjCIvarDecl *Ivar) { return nullptr; } +namespace { + enum Retaining_t { + NonRetaining, + Retaining + }; +} + +static Optional 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(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 IVarLoc = C.getSVal(IRE).getAs(); @@ -2884,8 +2918,9 @@ void RetainCountChecker::checkPostStmt(const ObjCIvarRefExpr *IRE, // 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(const ObjCIvarRefExpr *IRE, 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); } diff --git a/test/Analysis/properties.m b/test/Analysis/properties.m index 6f92c6ec32..ca6f253d94 100644 --- a/test/Analysis/properties.m +++ b/test/Analysis/properties.m @@ -356,6 +356,9 @@ void testOpaqueConsistency(OpaqueIntWrapper *w) { @property (strong) id ownedProp; @property (unsafe_unretained) id unownedProp; @property (nonatomic, strong) id manualProp; +@property (readonly) id readonlyProp; +@property (nonatomic, readwrite/*, assign */) id implicitManualProp; // expected-warning {{'assign' is assumed}} expected-warning {{'assign' not appropriate}} +@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(OpaqueIntWrapper *w) { return _manualProp; } +- (void)setImplicitManualProp:(id)newValue {} + - (void)testOverreleaseOwnedIvar { [_ownedProp retain]; [_ownedProp release]; @@ -387,6 +392,26 @@ void testOpaqueConsistency(OpaqueIntWrapper *w) { [_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(OpaqueIntWrapper *w) { 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(OpaqueIntWrapper *w) { 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(OpaqueIntWrapper *w) { 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(); -- 2.40.0