r204730 - [analyzer] Don't track retain counts of objects directly accessed through ivars.
Jordan Rose
jordan_rose at apple.com
Tue Mar 25 10:10:59 PDT 2014
Author: jrose
Date: Tue Mar 25 12:10:58 2014
New Revision: 204730
URL: http://llvm.org/viewvc/llvm-project?rev=204730&view=rev
Log:
[analyzer] Don't track retain counts of objects directly accessed through ivars.
A refinement of r198953 to handle cases where an object is accessed both through
a property getter and through direct ivar access. An object accessed through a
property should always be treated as +0, i.e. not owned by the caller. However,
an object accessed through an ivar may be at +0 or at +1, depending on whether
the ivar is a strong reference. Outside of ARC, we don't have that information,
so we just don't track objects accessed through ivars.
With this change, accessing an ivar directly will deliberately override the +0
provided by a getter, but only if the +0 hasn't participated in other retain
counting yet. That isn't perfect, but it's already unusual for people to be
mixing property access with direct ivar access. (The primary use case for this
is in setters, init methods, and -dealloc.)
Thanks to Ted for spotting a few mistakes in private review.
<rdar://problem/16333368>
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=204730&r1=204729&r2=204730&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp Tue Mar 25 12:10:58 2014
@@ -94,29 +94,70 @@ public:
};
private:
- Kind kind;
- RetEffect::ObjKind okind;
+ /// The number of outstanding retains.
unsigned Cnt;
+ /// The number of outstanding autoreleases.
unsigned ACnt;
+ /// The (static) type of the object at the time we started tracking it.
QualType T;
- RefVal(Kind k, RetEffect::ObjKind o, unsigned cnt, unsigned acnt, QualType t)
- : kind(k), okind(o), Cnt(cnt), ACnt(acnt), T(t) {}
+ /// The current state of the object.
+ ///
+ /// See the RefVal::Kind enum for possible values.
+ unsigned RawKind : 5;
+
+ /// The kind of object being tracked (CF or ObjC), if known.
+ ///
+ /// See the RetEffect::ObjKind enum for possible values.
+ unsigned RawObjectKind : 2;
+
+ /// True if the current state and/or retain count may turn out to not be the
+ /// best possible approximation of the reference counting state.
+ ///
+ /// If true, the checker may decide to throw away ("override") this state
+ /// in favor of something else when it sees the object being used in new ways.
+ ///
+ /// This setting should not be propagated to state derived from this state.
+ /// Once we start deriving new states, it would be inconsistent to override
+ /// them.
+ unsigned IsOverridable : 1;
+
+ RefVal(Kind k, RetEffect::ObjKind o, unsigned cnt, unsigned acnt, QualType t,
+ bool Overridable = false)
+ : Cnt(cnt), ACnt(acnt), T(t), RawKind(static_cast<unsigned>(k)),
+ RawObjectKind(static_cast<unsigned>(o)), IsOverridable(Overridable) {
+ assert(getKind() == k && "not enough bits for the kind");
+ assert(getObjKind() == o && "not enough bits for the object kind");
+ }
public:
- Kind getKind() const { return kind; }
+ Kind getKind() const { return static_cast<Kind>(RawKind); }
- RetEffect::ObjKind getObjKind() const { return okind; }
+ RetEffect::ObjKind getObjKind() const {
+ return static_cast<RetEffect::ObjKind>(RawObjectKind);
+ }
unsigned getCount() const { return Cnt; }
unsigned getAutoreleaseCount() const { return ACnt; }
unsigned getCombinedCounts() const { return Cnt + ACnt; }
- void clearCounts() { Cnt = 0; ACnt = 0; }
- void setCount(unsigned i) { Cnt = i; }
- void setAutoreleaseCount(unsigned i) { ACnt = i; }
+ void clearCounts() {
+ Cnt = 0;
+ ACnt = 0;
+ IsOverridable = false;
+ }
+ void setCount(unsigned i) {
+ Cnt = i;
+ IsOverridable = false;
+ }
+ void setAutoreleaseCount(unsigned i) {
+ ACnt = i;
+ IsOverridable = false;
+ }
QualType getType() const { return T; }
+ bool isOverridable() const { return IsOverridable; }
+
bool isOwned() const {
return getKind() == Owned;
}
@@ -133,20 +174,31 @@ public:
return getKind() == ReturnedNotOwned;
}
+ /// Create a state for an object whose lifetime is the responsibility of the
+ /// current function, at least partially.
+ ///
+ /// Most commonly, this is an owned object with a retain count of +1.
static RefVal makeOwned(RetEffect::ObjKind o, QualType t,
unsigned Count = 1) {
return RefVal(Owned, o, Count, 0, t);
}
+ /// Create a state for an object whose lifetime is not the responsibility of
+ /// the current function.
+ ///
+ /// Most commonly, this is an unowned object with a retain count of +0.
static RefVal makeNotOwned(RetEffect::ObjKind o, QualType t,
unsigned Count = 0) {
return RefVal(NotOwned, o, Count, 0, t);
}
- // Comparison, profiling, and pretty-printing.
-
- bool operator==(const RefVal& X) const {
- return kind == X.kind && Cnt == X.Cnt && T == X.T && ACnt == X.ACnt;
+ /// Create an "overridable" state for an unowned object at +0.
+ ///
+ /// An overridable state is one that provides a good approximation of the
+ /// reference counting state now, but which may be discarded later if the
+ /// checker sees the object being used in new ways.
+ static RefVal makeOverridableNotOwned(RetEffect::ObjKind o, QualType t) {
+ return RefVal(NotOwned, o, 0, 0, t, /*Overridable=*/true);
}
RefVal operator-(size_t i) const {
@@ -169,11 +221,24 @@ public:
getType());
}
+ // Comparison, profiling, and pretty-printing.
+
+ bool hasSameState(const RefVal &X) const {
+ return getKind() == X.getKind() && Cnt == X.Cnt && ACnt == X.ACnt;
+ }
+
+ bool operator==(const RefVal& X) const {
+ return T == X.T && hasSameState(X) && getObjKind() == X.getObjKind() &&
+ IsOverridable == X.IsOverridable;
+ }
+
void Profile(llvm::FoldingSetNodeID& ID) const {
- ID.AddInteger((unsigned) kind);
+ ID.Add(T);
+ ID.AddInteger(RawKind);
ID.AddInteger(Cnt);
ID.AddInteger(ACnt);
- ID.Add(T);
+ ID.AddInteger(RawObjectKind);
+ ID.AddBoolean(IsOverridable);
}
void print(raw_ostream &Out) const;
@@ -183,6 +248,9 @@ void RefVal::print(raw_ostream &Out) con
if (!T.isNull())
Out << "Tracked " << T.getAsString() << '/';
+ if (isOverridable())
+ Out << "(overridable) ";
+
switch (getKind()) {
default: llvm_unreachable("Invalid RefVal kind");
case Owned: {
@@ -1923,7 +1991,7 @@ PathDiagnosticPiece *CFRefReportVisitor:
if (!GCEnabled && std::find(AEffects.begin(), AEffects.end(), Dealloc) !=
AEffects.end()) {
// Determine if the object's reference count was pushed to zero.
- assert(!(PrevV == CurrV) && "The typestate *must* have changed.");
+ assert(!PrevV.hasSameState(CurrV) && "The state should have changed.");
// We may not have transitioned to 'release' if we hit an error.
// This case is handled elsewhere.
if (CurrV.getKind() == RefVal::Released) {
@@ -1944,7 +2012,7 @@ PathDiagnosticPiece *CFRefReportVisitor:
if (GCEnabled) {
// Determine if the object's reference count was pushed to zero.
- assert(!(PrevV == CurrV) && "The typestate *must* have changed.");
+ assert(!PrevV.hasSameState(CurrV) && "The state should have changed.");
os << "In GC mode a call to '" << *FD
<< "' decrements an object's retain count and registers the "
@@ -1969,7 +2037,7 @@ PathDiagnosticPiece *CFRefReportVisitor:
}
// Determine if the typestate has changed.
- if (!(PrevV == CurrV))
+ if (!PrevV.hasSameState(CurrV))
switch (CurrV.getKind()) {
case RefVal::Owned:
case RefVal::NotOwned:
@@ -2333,6 +2401,7 @@ class RetainCountChecker
check::PostStmt<ObjCArrayLiteral>,
check::PostStmt<ObjCDictionaryLiteral>,
check::PostStmt<ObjCBoxedExpr>,
+ check::PostStmt<ObjCIvarRefExpr>,
check::PostCall,
check::PreStmt<ReturnStmt>,
check::RegionChanges,
@@ -2482,6 +2551,8 @@ public:
void checkPostStmt(const ObjCDictionaryLiteral *DL, CheckerContext &C) const;
void checkPostStmt(const ObjCBoxedExpr *BE, CheckerContext &C) const;
+ void checkPostStmt(const ObjCIvarRefExpr *IRE, CheckerContext &C) const;
+
void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
void checkSummary(const RetainSummary &Summ, const CallEvent &Call,
@@ -2699,6 +2770,20 @@ void RetainCountChecker::checkPostStmt(c
C.addTransition(State);
}
+void RetainCountChecker::checkPostStmt(const ObjCIvarRefExpr *IRE,
+ CheckerContext &C) const {
+ ProgramStateRef State = C.getState();
+ // If an instance variable was previously accessed through a property,
+ // it may have a synthesized refcount of +0. Override right now that we're
+ // doing direct access.
+ if (Optional<Loc> IVarLoc = C.getSVal(IRE).getAs<Loc>())
+ if (SymbolRef Sym = State->getSVal(*IVarLoc).getAsSymbol())
+ if (const RefVal *RV = getRefBinding(State, Sym))
+ if (RV->isOverridable())
+ State = removeRefBinding(State, Sym);
+ C.addTransition(State);
+}
+
void RetainCountChecker::checkPostCall(const CallEvent &Call,
CheckerContext &C) const {
RetainSummaryManager &Summaries = getSummaryManager(C);
@@ -2785,12 +2870,16 @@ void RetainCountChecker::processSummaryO
state = removeRefBinding(state, Sym);
} else if (RE.getKind() == RetEffect::NotOwnedSymbol) {
if (wasSynthesizedProperty(MsgInvocation, C.getPredecessor())) {
- // Believe the summary if we synthesized the body and the return value is
- // untracked. This handles property getters.
+ // Believe the summary if we synthesized the body of a property getter
+ // and the return value is currently untracked. If the corresponding
+ // instance variable is later accessed directly, however, we're going to
+ // want to override this state, so that the owning object can perform
+ // reference counting operations on its own ivars.
SymbolRef Sym = CallOrMsg.getReturnValue().getAsSymbol();
if (Sym && !getRefBinding(state, Sym))
- state = setRefBinding(state, Sym, RefVal::makeNotOwned(RE.getObjKind(),
- Sym->getType()));
+ state = setRefBinding(state, Sym,
+ RefVal::makeOverridableNotOwned(RE.getObjKind(),
+ Sym->getType()));
}
}
Modified: cfe/trunk/test/Analysis/properties.m
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/properties.m?rev=204730&r1=204729&r2=204730&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/properties.m (original)
+++ cfe/trunk/test/Analysis/properties.m Tue Mar 25 12:10:58 2014
@@ -186,10 +186,12 @@ void rdar6611873() {
// Property accessor synthesis
//------
+extern void doSomethingWithPerson(Person *p);
+extern void doSomethingWithName(NSString *name);
+
void testConsistencyRetain(Person *p) {
clang_analyzer_eval(p.name == p.name); // expected-warning{{TRUE}}
- extern void doSomethingWithPerson(Person *p);
id origName = p.name;
clang_analyzer_eval(p.name == origName); // expected-warning{{TRUE}}
doSomethingWithPerson(p);
@@ -199,7 +201,6 @@ void testConsistencyRetain(Person *p) {
void testConsistencyAssign(Person *p) {
clang_analyzer_eval(p.friend == p.friend); // expected-warning{{TRUE}}
- extern void doSomethingWithPerson(Person *p);
id origFriend = p.friend;
clang_analyzer_eval(p.friend == origFriend); // expected-warning{{TRUE}}
doSomethingWithPerson(p);
@@ -208,11 +209,55 @@ void testConsistencyAssign(Person *p) {
#if !__has_feature(objc_arc)
void testOverrelease(Person *p, int coin) {
- if (coin)
+ switch (coin) {
+ case 0:
[p.name release]; // expected-warning{{not owned}}
- else
+ break;
+ case 1:
[p.friend release]; // expected-warning{{not owned}}
+ break;
+ case 2: {
+ id friend = p.friend;
+ doSomethingWithPerson(p);
+ [friend release]; // expected-warning{{not owned}}
+ }
+ }
+}
+
+// <rdar://problem/16333368>
+ at implementation Person (Rdar16333368)
+
+- (void)testDeliberateRelease:(Person *)other {
+ doSomethingWithName(self.name);
+ [_name release]; // no-warning
+ self->_name = 0;
+
+ doSomethingWithName(other->_name);
+ [other.name release]; // expected-warning{{not owned}}
+}
+
+- (void)deliberateReleaseFalseNegative {
+ // This is arguably a false negative because the result of p.friend shouldn't
+ // be released, even though we are manipulating the ivar in between the two
+ // actions.
+ id name = self.name;
+ _name = 0;
+ [name release];
+}
+
+- (void)testRetainAndRelease {
+ [self.name retain];
+ [self.name release];
+ [self.name release]; // expected-warning{{not owned}}
}
+
+- (void)testRetainAndReleaseIVar {
+ [self.name retain];
+ [_name release];
+ [_name release]; // expected-warning{{not owned}}
+}
+
+ at end
#endif
@interface IntWrapper
More information about the cfe-commits
mailing list