From 554067f290282f366ccf65a27e0b914aa67a52c6 Mon Sep 17 00:00:00 2001 From: Anna Zaks Date: Wed, 29 Aug 2012 23:23:43 +0000 Subject: [PATCH] [analyzer] Stop tracking symbols based on a retain count summary of inlined function. This resolves retain count checker false positives that are caused by inlining ObjC and other methods. Essentially, if we are passing an object to a method with "delegate" in the selector or a function pointer as another argument, we should stop tracking the other parameters/return value as far as the retain count checker is concerned. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@162876 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../Checkers/RetainCountChecker.cpp | 128 ++++++++++++++---- test/Analysis/inlining/RetainCountExamples.m | 62 +++++++++ 2 files changed, 163 insertions(+), 27 deletions(-) diff --git a/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp b/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp index dc5731b1f0..93a77f440b 100644 --- a/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp @@ -48,9 +48,23 @@ using llvm::StrInStrNoCase; /// particular argument. enum ArgEffect { DoNothing, Autorelease, Dealloc, DecRef, DecRefMsg, DecRefBridgedTransfered, - DecRefAndStopTracking, DecRefMsgAndStopTracking, IncRefMsg, IncRef, MakeCollectable, MayEscape, - NewAutoreleasePool, StopTracking }; + NewAutoreleasePool, + + // Stop tracking the argument - the effect of the call is + // unknown. + StopTracking, + + // In some cases, we obtain a better summary for this checker + // by looking at the call site than by inlining the function. + // Signifies that we should stop tracking the symbol even if + // the function is inlined. + StopTrackingHard, + + // The function decrements the reference count and the checker + // should stop tracking the argument. + DecRefAndStopTrackingHard, DecRefMsgAndStopTrackingHard + }; namespace llvm { template <> struct FoldingSetTrait { @@ -72,7 +86,13 @@ class RetEffect { public: enum Kind { NoRet, OwnedSymbol, OwnedAllocatedSymbol, NotOwnedSymbol, GCNotOwnedSymbol, ARCNotOwnedSymbol, - OwnedWhenTrackedReceiver }; + OwnedWhenTrackedReceiver, + // Treat this function as returning a non-tracked symbol even if + // the function has been inlined. This is used where the call + // site summary is more presise than the summary indirectly produced + // by inlining the function + NoRetHard + }; enum ObjKind { CF, ObjC, AnyObj }; @@ -115,6 +135,9 @@ public: static RetEffect MakeNoRet() { return RetEffect(NoRet); } + static RetEffect MakeNoRetHard() { + return RetEffect(NoRetHard); + } void Profile(llvm::FoldingSetNodeID& ID) const { ID.AddInteger((unsigned) K); @@ -875,7 +898,7 @@ static bool isMakeCollectable(const FunctionDecl *FD, StringRef FName) { return FName.find("MakeCollectable") != StringRef::npos; } -static ArgEffect getStopTrackingEquivalent(ArgEffect E) { +static ArgEffect getStopTrackingHardEquivalent(ArgEffect E) { switch (E) { case DoNothing: case Autorelease: @@ -886,13 +909,14 @@ static ArgEffect getStopTrackingEquivalent(ArgEffect E) { case MayEscape: case NewAutoreleasePool: case StopTracking: - return StopTracking; + case StopTrackingHard: + return StopTrackingHard; case DecRef: - case DecRefAndStopTracking: - return DecRefAndStopTracking; + case DecRefAndStopTrackingHard: + return DecRefAndStopTrackingHard; case DecRefMsg: - case DecRefMsgAndStopTracking: - return DecRefMsgAndStopTracking; + case DecRefMsgAndStopTrackingHard: + return DecRefMsgAndStopTrackingHard; case Dealloc: return Dealloc; } @@ -903,19 +927,21 @@ static ArgEffect getStopTrackingEquivalent(ArgEffect E) { void RetainSummaryManager::updateSummaryForCall(const RetainSummary *&S, const CallEvent &Call) { if (Call.hasNonZeroCallbackArg()) { - ArgEffect RecEffect = getStopTrackingEquivalent(S->getReceiverEffect()); - ArgEffect DefEffect = getStopTrackingEquivalent(S->getDefaultArgEffect()); + ArgEffect RecEffect = + getStopTrackingHardEquivalent(S->getReceiverEffect()); + ArgEffect DefEffect = + getStopTrackingHardEquivalent(S->getDefaultArgEffect()); ArgEffects CustomArgEffects = S->getArgEffects(); for (ArgEffects::iterator I = CustomArgEffects.begin(), E = CustomArgEffects.end(); I != E; ++I) { - ArgEffect Translated = getStopTrackingEquivalent(I->second); + ArgEffect Translated = getStopTrackingHardEquivalent(I->second); if (Translated != DefEffect) ScratchArgs = AF.add(ScratchArgs, I->first, Translated); } - RetEffect RE = RetEffect::MakeNoRet(); + RetEffect RE = RetEffect::MakeNoRetHard(); // Special cases where the callback argument CANNOT free the return value. // This can generally only happen if we know that the callback will only be @@ -1436,9 +1462,9 @@ RetainSummaryManager::getStandardMethodSummary(const ObjCMethodDecl *MD, StringRef Slot = S.getNameForSlot(i); if (Slot.substr(Slot.size() - 8).equals_lower("delegate")) { if (ResultEff == ObjCInitRetE) - ResultEff = RetEffect::MakeNoRet(); + ResultEff = RetEffect::MakeNoRetHard(); else - ReceiverEff = StopTracking; + ReceiverEff = StopTrackingHard; } } } @@ -2469,6 +2495,10 @@ public: void checkSummary(const RetainSummary &Summ, const CallEvent &Call, CheckerContext &C) const; + void processSummaryOfInlined(const RetainSummary &Summ, + const CallEvent &Call, + CheckerContext &C) const; + bool evalCall(const CallExpr *CE, CheckerContext &C) const; ProgramStateRef evalAssume(ProgramStateRef state, SVal Cond, @@ -2494,8 +2524,8 @@ public: void checkEndPath(CheckerContext &C) const; ProgramStateRef updateSymbol(ProgramStateRef state, SymbolRef sym, - RefVal V, ArgEffect E, RefVal::Kind &hasErr, - CheckerContext &C) const; + RefVal V, ArgEffect E, RefVal::Kind &hasErr, + CheckerContext &C) const; void processNonLeakError(ProgramStateRef St, SourceRange ErrorRange, RefVal::Kind ErrorKind, SymbolRef Sym, @@ -2679,11 +2709,13 @@ void RetainCountChecker::checkPostStmt(const ObjCBoxedExpr *Ex, void RetainCountChecker::checkPostCall(const CallEvent &Call, CheckerContext &C) const { - if (C.wasInlined) - return; - RetainSummaryManager &Summaries = getSummaryManager(C); const RetainSummary *Summ = Summaries.getSummary(Call, C.getState()); + + if (C.wasInlined) { + processSummaryOfInlined(*Summ, Call, C); + return; + } checkSummary(*Summ, Call, C); } @@ -2715,6 +2747,46 @@ static QualType GetReturnType(const Expr *RetE, ASTContext &Ctx) { return RetTy; } +// We don't always get the exact modeling of the function with regards to the +// retain count checker even when the function is inlined. For example, we need +// to stop tracking the symbols which were marked with StopTrackingHard. +void RetainCountChecker::processSummaryOfInlined(const RetainSummary &Summ, + const CallEvent &CallOrMsg, + CheckerContext &C) const { + ProgramStateRef state = C.getState(); + + // Evaluate the effect of the arguments. + for (unsigned idx = 0, e = CallOrMsg.getNumArgs(); idx != e; ++idx) { + if (Summ.getArg(idx) == StopTrackingHard) { + SVal V = CallOrMsg.getArgSVal(idx); + if (SymbolRef Sym = V.getAsLocSymbol()) { + state = removeRefBinding(state, Sym); + } + } + } + + // Evaluate the effect on the message receiver. + const ObjCMethodCall *MsgInvocation = dyn_cast(&CallOrMsg); + if (MsgInvocation) { + if (SymbolRef Sym = MsgInvocation->getReceiverSVal().getAsLocSymbol()) { + if (Summ.getReceiverEffect() == StopTrackingHard) { + state = removeRefBinding(state, Sym); + } + } + } + + // Consult the summary for the return value. + RetEffect RE = Summ.getRetEffect(); + if (RE.getKind() == RetEffect::NoRetHard) { + SymbolRef Sym = state->getSVal(CallOrMsg.getOriginExpr(), + C.getLocationContext()).getAsSymbol(); + if (Sym) + state = removeRefBinding(state, Sym); + } + + C.addTransition(state); +} + void RetainCountChecker::checkSummary(const RetainSummary &Summ, const CallEvent &CallOrMsg, CheckerContext &C) const { @@ -2749,7 +2821,7 @@ void RetainCountChecker::checkSummary(const RetainSummary &Summ, if (const RefVal *T = getRefBinding(state, Sym)) { ReceiverIsTracked = true; state = updateSymbol(state, Sym, *T, Summ.getReceiverEffect(), - hasErr, C); + hasErr, C); if (hasErr) { ErrorRange = MsgInvocation->getOriginExpr()->getReceiverRange(); ErrorSym = Sym; @@ -2780,6 +2852,7 @@ void RetainCountChecker::checkSummary(const RetainSummary &Summ, llvm_unreachable("Unhandled RetEffect."); case RetEffect::NoRet: + case RetEffect::NoRetHard: // No work necessary. break; @@ -2858,8 +2931,8 @@ RetainCountChecker::updateSymbol(ProgramStateRef state, SymbolRef sym, case DecRefMsg: E = IgnoreRetainMsg ? DoNothing : DecRef; break; - case DecRefMsgAndStopTracking: - E = IgnoreRetainMsg ? StopTracking : DecRefAndStopTracking; + case DecRefMsgAndStopTrackingHard: + E = IgnoreRetainMsg ? StopTracking : DecRefAndStopTrackingHard; break; case MakeCollectable: E = C.isObjCGCEnabled() ? DecRef : DoNothing; @@ -2880,7 +2953,7 @@ RetainCountChecker::updateSymbol(ProgramStateRef state, SymbolRef sym, case DecRefMsg: case IncRefMsg: case MakeCollectable: - case DecRefMsgAndStopTracking: + case DecRefMsgAndStopTrackingHard: llvm_unreachable("DecRefMsg/IncRefMsg/MakeCollectable already converted"); case Dealloc: @@ -2929,6 +3002,7 @@ RetainCountChecker::updateSymbol(ProgramStateRef state, SymbolRef sym, break; case StopTracking: + case StopTrackingHard: return removeRefBinding(state, sym); case IncRef: @@ -2949,7 +3023,7 @@ RetainCountChecker::updateSymbol(ProgramStateRef state, SymbolRef sym, case DecRef: case DecRefBridgedTransfered: - case DecRefAndStopTracking: + case DecRefAndStopTrackingHard: switch (V.getKind()) { default: // case 'RefVal::Released' handled above. @@ -2960,7 +3034,7 @@ RetainCountChecker::updateSymbol(ProgramStateRef state, SymbolRef sym, if (V.getCount() == 1) V = V ^ (E == DecRefBridgedTransfered ? RefVal::NotOwned : RefVal::Released); - else if (E == DecRefAndStopTracking) + else if (E == DecRefAndStopTrackingHard) return removeRefBinding(state, sym); V = V - 1; @@ -2968,7 +3042,7 @@ RetainCountChecker::updateSymbol(ProgramStateRef state, SymbolRef sym, case RefVal::NotOwned: if (V.getCount() > 0) { - if (E == DecRefAndStopTracking) + if (E == DecRefAndStopTrackingHard) return removeRefBinding(state, sym); V = V - 1; } else { diff --git a/test/Analysis/inlining/RetainCountExamples.m b/test/Analysis/inlining/RetainCountExamples.m index c48e7b068d..276ab52095 100644 --- a/test/Analysis/inlining/RetainCountExamples.m +++ b/test/Analysis/inlining/RetainCountExamples.m @@ -15,6 +15,7 @@ typedef struct objc_object { -(id)copy; - (Class)class; -(id)retain; +- (oneway void)release; @end @interface SelfStaysLive : NSObject @@ -62,4 +63,65 @@ void selfStaysLive() { Cell *sharedCell4 = [[Cell alloc] initWithInt: 3]; // expected-warning {{leak}} } @end + +// We should stop tracking some objects even when we inline the call. +// Specialically, the objects passed into calls with delegate and callback +// parameters. +@class DelegateTest; +typedef void (*ReleaseCallbackTy) (DelegateTest *c); + +@interface Delegate : NSObject +@end + +@interface DelegateTest : NSObject { + Delegate *myDel; +} +// Object initialized with a delagate which could potentially release it. +- (id)initWithDelegate: (id) d; + +- (void) setDelegate: (id) d; + +// Releases object through callback. ++ (void)updateObject:(DelegateTest*)obj WithCallback:(ReleaseCallbackTy)rc; + ++ (void)test: (Delegate *)d; + +@property (assign) Delegate* myDel; +@end + +void releaseObj(DelegateTest *c); + +// Releases object through callback. +void updateObject(DelegateTest *c, ReleaseCallbackTy rel) { + rel(c); +} + +@implementation DelegateTest +@synthesize myDel; + +- (id) initWithDelegate: (id) d { + if ((self = [super init])) + myDel = d; + return self; +} + +- (void) setDelegate: (id) d { + myDel = d; +} + ++ (void)updateObject:(DelegateTest*)obj WithCallback:(ReleaseCallbackTy)rc { + rc(obj); +} + ++ (void) test: (Delegate *)d { + DelegateTest *obj1 = [[DelegateTest alloc] initWithDelegate: d]; // no-warning + DelegateTest *obj2 = [[DelegateTest alloc] init]; // no-warning + DelegateTest *obj3 = [[DelegateTest alloc] init]; // no-warning + updateObject(obj2, releaseObj); + [DelegateTest updateObject: obj3 + WithCallback: releaseObj]; + DelegateTest *obj4 = [[DelegateTest alloc] init]; // no-warning + [obj4 setDelegate: d]; +} +@end -- 2.40.0