From: Anna Zaks Date: Sat, 29 Sep 2012 00:20:38 +0000 (+0000) Subject: [analyzer] Re-implement IvarInvalidationChecker so that it verifies that X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=31f69cc770888ec0f0f7012212e5df7979aba4f3;p=clang [analyzer] Re-implement IvarInvalidationChecker so that it verifies that the validation occurred. The original implementation was pessimistic - we assumed that ivars which escape are invalidated. This version is optimistic, it assumes that the ivars will always be explicitly invalidated: either set to nil or sent an invalidation message. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@164868 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/StaticAnalyzer/Checkers/IvarInvalidationChecker.cpp b/lib/StaticAnalyzer/Checkers/IvarInvalidationChecker.cpp index 836860e7c8..e269f46422 100644 --- a/lib/StaticAnalyzer/Checkers/IvarInvalidationChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/IvarInvalidationChecker.cpp @@ -1,4 +1,4 @@ -//=- IvarInvalidationChecker.cpp - -*- C++ ----*-==// +//=- IvarInvalidationChecker.cpp - -*- C++ -------------------------------*-==// // // The LLVM Compiler Infrastructure // @@ -13,10 +13,12 @@ // __attribute__((annotate("objc_instance_variable_invalidator"))); // all the "ivalidatable" instance variables of this class should be // invalidated. We call an instance variable ivalidatable if it is an object of -// a class which contains an invalidation method. -// -// Note, this checker currently only checks if an ivar was accessed by the -// method, we do not currently support any deeper invalidation checking. +// a class which contains an invalidation method. There could be multiple +// methods annotated with such annotations per class, either one can be used +// to invalidate the ivar. An ivar or property are considered to be +// invalidated if they are being assigned 'nil' or an invalidation method has +// been called on them. An invalidation method should either invalidate all +// the ivars or call another invalidation method (on self). // //===----------------------------------------------------------------------===// @@ -36,7 +38,7 @@ namespace { class IvarInvalidationChecker : public Checker > { - typedef llvm::DenseMap IvarSet; + typedef llvm::DenseSet MethodSet; typedef llvm::DenseMap MethToIvarMapTy; typedef llvm::DenseMap IvarToPropMapTy; + + struct IvarInfo { + /// Has the ivar been invalidated? + bool IsInvalidated; + + /// The methods which can be used to invalidate the ivar. + MethodSet InvalidationMethods; + + IvarInfo() : IsInvalidated(false) {} + void addInvalidationMethod(const ObjCMethodDecl *MD) { + InvalidationMethods.insert(MD); + } + + bool needsInvalidation() const { + return !InvalidationMethods.empty(); + } + + void markInvalidated() { + IsInvalidated = true; + } + + bool markInvalidated(const ObjCMethodDecl *MD) { + if (IsInvalidated) + return true; + for (MethodSet::iterator I = InvalidationMethods.begin(), + E = InvalidationMethods.end(); I != E; ++I) { + if (*I == MD) { + IsInvalidated = true; + return true; + } + } + return false; + } + + bool isInvalidated() const { + return IsInvalidated; + } + }; + + typedef llvm::DenseMap IvarSet; + /// Statement visitor, which walks the method body and flags the ivars /// referenced in it (either directly or via property). class MethodCrawler : public ConstStmtVisitor { + const ObjCMethodDecl *EnclosingMethod; /// The set of Ivars which need to be invalidated. IvarSet &IVars; - /// Property setter/getter to ivar mapping. - MethToIvarMapTy &PropertyAccessorToIvarMap; + /// Flag is set as the result of a message send to another + /// invalidation method. + bool &CalledAnotherInvalidationMethod; + + /// Property setter to ivar mapping. + const MethToIvarMapTy &PropertySetterToIvarMap; + + /// Property getter to ivar mapping. + const MethToIvarMapTy &PropertyGetterToIvarMap; + + /// Property to ivar mapping. + const PropToIvarMapTy &PropertyToIvarMap; + + /// The invalidation method being currently processed. + const ObjCMethodDecl *InvalidationMethod; + + /// Peel off parents, casts, OpaqueValueExpr, and PseudoObjectExpr. + const Expr *peel(const Expr *E) const; - // Property to ivar mapping. - PropToIvarMapTy &PropertyToIvarMap; + /// Does this expression represent zero: '0'? + bool isZero(const Expr *E) const; + + /// Mark the given ivar as invalidated. + void markInvalidated(const ObjCIvarDecl *Iv); + + /// Checks if IvarRef refers to the tracked IVar, if yes, marks it as + /// invalidated. + void checkObjCIvarRefExpr(const ObjCIvarRefExpr *IvarRef); + + /// Checks if ObjCPropertyRefExpr refers to the tracked IVar, if yes, marks + /// it as invalidated. + void checkObjCPropertyRefExpr(const ObjCPropertyRefExpr *PA); + + /// Checks if ObjCMessageExpr refers to (is a getter for) the tracked IVar, + /// if yes, marks it as invalidated. + void checkObjCMessageExpr(const ObjCMessageExpr *ME); + + /// Checks if the Expr refers to an ivar, if yes, marks it as invalidated. + void check(const Expr *E); public: - MethodCrawler(const ObjCInterfaceDecl *InID, + MethodCrawler(const ObjCMethodDecl *InMeth, IvarSet &InIVars, - MethToIvarMapTy &InPropertyAccessorToIvarMap, - PropToIvarMapTy &InPropertyToIvarMap) - : IVars(InIVars), - PropertyAccessorToIvarMap(InPropertyAccessorToIvarMap), - PropertyToIvarMap(InPropertyToIvarMap) {} + bool &InCalledAnotherInvalidationMethod, + const MethToIvarMapTy &InPropertySetterToIvarMap, + const MethToIvarMapTy &InPropertyGetterToIvarMap, + const PropToIvarMapTy &InPropertyToIvarMap) + : EnclosingMethod(InMeth), + IVars(InIVars), + CalledAnotherInvalidationMethod(InCalledAnotherInvalidationMethod), + PropertySetterToIvarMap(InPropertySetterToIvarMap), + PropertyGetterToIvarMap(InPropertyGetterToIvarMap), + PropertyToIvarMap(InPropertyToIvarMap), + InvalidationMethod(0) {} void VisitStmt(const Stmt *S) { VisitChildren(S); } - void VisitObjCIvarRefExpr(const ObjCIvarRefExpr *IvarRef); + void VisitBinaryOperator(const BinaryOperator *BO); void VisitObjCMessageExpr(const ObjCMessageExpr *ME); - void VisitObjCPropertyRefExpr(const ObjCPropertyRefExpr *PA); - void VisitChildren(const Stmt *S) { - for (Stmt::const_child_range I = S->children(); I; ++I) + for (Stmt::const_child_range I = S->children(); I; ++I) { if (*I) this->Visit(*I); + if (CalledAnotherInvalidationMethod) + return; + } } }; /// Check if the any of the methods inside the interface are annotated with - /// the invalidation annotation. - static bool containsInvalidationMethod(const ObjCContainerDecl *D); + /// the invalidation annotation, update the IvarInfo accordingly. + static void containsInvalidationMethod(const ObjCContainerDecl *D, + IvarInfo &Out); /// Check if ivar should be tracked and add to TrackedIvars if positive. /// Returns true if ivar should be tracked. @@ -104,7 +190,7 @@ public: // TODO: We are currently ignoring the ivars coming from class extensions. }; -bool isInvalidationMethod(const ObjCMethodDecl *M) { +static bool isInvalidationMethod(const ObjCMethodDecl *M) { for (specific_attr_iterator AI = M->specific_attr_begin(), AE = M->specific_attr_end(); AI != AE; ++AI) { @@ -115,13 +201,13 @@ bool isInvalidationMethod(const ObjCMethodDecl *M) { return false; } -bool IvarInvalidationChecker::containsInvalidationMethod ( - const ObjCContainerDecl *D) { +void IvarInvalidationChecker::containsInvalidationMethod( + const ObjCContainerDecl *D, IvarInfo &OutInfo) { // TODO: Cache the results. if (!D) - return false; + return; // Check all methods. for (ObjCContainerDecl::method_iterator @@ -129,7 +215,8 @@ bool IvarInvalidationChecker::containsInvalidationMethod ( E = D->meth_end(); I != E; ++I) { const ObjCMethodDecl *MDI = *I; if (isInvalidationMethod(MDI)) - return true; + OutInfo.addInvalidationMethod( + cast(MDI->getCanonicalDecl())); } // If interface, check all parent protocols and super. @@ -139,10 +226,10 @@ bool IvarInvalidationChecker::containsInvalidationMethod ( for (ObjCInterfaceDecl::protocol_iterator I = InterfaceD->protocol_begin(), E = InterfaceD->protocol_end(); I != E; ++I) { - if (containsInvalidationMethod(*I)) - return true; + containsInvalidationMethod(*I, OutInfo); } - return containsInvalidationMethod(InterfaceD->getSuperClass()); + containsInvalidationMethod(InterfaceD->getSuperClass(), OutInfo); + return; } // If protocol, check all parent protocols. @@ -150,10 +237,9 @@ bool IvarInvalidationChecker::containsInvalidationMethod ( for (ObjCInterfaceDecl::protocol_iterator I = ProtD->protocol_begin(), E = ProtD->protocol_end(); I != E; ++I) { - if (containsInvalidationMethod(*I)) - return true; + containsInvalidationMethod(*I, OutInfo); } - return false; + return; } llvm_unreachable("One of the casts above should have succeeded."); @@ -166,8 +252,11 @@ bool IvarInvalidationChecker::trackIvar(const ObjCIvarDecl *Iv, if (!IvTy) return false; const ObjCInterfaceDecl *IvInterf = IvTy->getInterfaceDecl(); - if (containsInvalidationMethod(IvInterf)) { - TrackedIvars[cast(Iv->getCanonicalDecl())] = false; + + IvarInfo Info; + containsInvalidationMethod(IvInterf, Info); + if (Info.needsInvalidation()) { + TrackedIvars[cast(Iv->getCanonicalDecl())] = Info; return true; } return false; @@ -234,7 +323,8 @@ void IvarInvalidationChecker::checkASTDecl(const ObjCMethodDecl *D, // Construct Property/Property Accessor to Ivar maps to assist checking if an // ivar which is backing a property has been reset. - MethToIvarMapTy PropAccessorToIvarMap; + MethToIvarMapTy PropSetterToIvarMap; + MethToIvarMapTy PropGetterToIvarMap; PropToIvarMapTy PropertyToIvarMap; IvarToPropMapTy IvarToPopertyMap; for (ObjCInterfaceDecl::prop_iterator @@ -256,25 +346,31 @@ void IvarInvalidationChecker::checkASTDecl(const ObjCMethodDecl *D, const ObjCMethodDecl *SetterD = PD->getSetterMethodDecl(); if (SetterD) { SetterD = cast(SetterD->getCanonicalDecl()); - PropAccessorToIvarMap[SetterD] = ID; + PropSetterToIvarMap[SetterD] = ID; } const ObjCMethodDecl *GetterD = PD->getGetterMethodDecl(); if (GetterD) { GetterD = cast(GetterD->getCanonicalDecl()); - PropAccessorToIvarMap[GetterD] = ID; + PropGetterToIvarMap[GetterD] = ID; } } - // Check which ivars have been accessed by the method. - // We assume that if ivar was at least accessed, it was not forgotten. - MethodCrawler(InterfaceD, Ivars, - PropAccessorToIvarMap, PropertyToIvarMap).VisitStmt(D->getBody()); + // Check which ivars have been invalidated in the method body. + bool CalledAnotherInvalidationMethod = false; + MethodCrawler(D, Ivars, + CalledAnotherInvalidationMethod, + PropSetterToIvarMap, + PropGetterToIvarMap, + PropertyToIvarMap).VisitStmt(D->getBody()); + + if (CalledAnotherInvalidationMethod) + return; // Warn on the ivars that were not accessed by the method. for (IvarSet::const_iterator I = Ivars.begin(), E = Ivars.end(); I != E; ++I){ - if (I->second == false) { + if (!I->second.isInvalidated()) { const ObjCIvarDecl *IvarDecl = I->first; PathDiagnosticLocation IvarDecLocation = @@ -303,38 +399,56 @@ void IvarInvalidationChecker::checkASTDecl(const ObjCMethodDecl *D, } } -/// Handle the case when an ivar is directly accessed. -void IvarInvalidationChecker::MethodCrawler::VisitObjCIvarRefExpr( - const ObjCIvarRefExpr *IvarRef) { - const Decl *D = IvarRef->getDecl(); - if (D) - IVars[cast(D->getCanonicalDecl())] = true; - VisitStmt(IvarRef); +void IvarInvalidationChecker::MethodCrawler::markInvalidated( + const ObjCIvarDecl *Iv) { + IvarSet::iterator I = IVars.find(Iv); + if (I != IVars.end()) { + // If InvalidationMethod is present, we are processing the message send and + // should ensure we are invalidating with the appropriate method, + // otherwise, we are processing setting to 'nil'. + if (InvalidationMethod) + I->second.markInvalidated(InvalidationMethod); + else + I->second.markInvalidated(); + } } +const Expr *IvarInvalidationChecker::MethodCrawler::peel(const Expr *E) const { + E = E->IgnoreParenCasts(); + if (const PseudoObjectExpr *POE = dyn_cast(E)) + E = POE->getSyntacticForm()->IgnoreParenCasts(); + if (const OpaqueValueExpr *OVE = dyn_cast(E)) + E = OVE->getSourceExpr()->IgnoreParenCasts(); + return E; +} -/// Handle the case when the property backing ivar is set via a direct call -/// to the setter. -void IvarInvalidationChecker::MethodCrawler::VisitObjCMessageExpr( +void IvarInvalidationChecker::MethodCrawler::checkObjCIvarRefExpr( + const ObjCIvarRefExpr *IvarRef) { + if (const Decl *D = IvarRef->getDecl()) + markInvalidated(cast(D->getCanonicalDecl())); +} + +void IvarInvalidationChecker::MethodCrawler::checkObjCMessageExpr( const ObjCMessageExpr *ME) { const ObjCMethodDecl *MD = ME->getMethodDecl(); if (MD) { MD = cast(MD->getCanonicalDecl()); - IVars[PropertyAccessorToIvarMap[MD]] = true; + MethToIvarMapTy::const_iterator IvI = PropertyGetterToIvarMap.find(MD); + if (IvI != PropertyGetterToIvarMap.end()) + markInvalidated(IvI->second); } - VisitStmt(ME); } -/// Handle the case when the property backing ivar is set via the dot syntax. -void IvarInvalidationChecker::MethodCrawler::VisitObjCPropertyRefExpr( +void IvarInvalidationChecker::MethodCrawler::checkObjCPropertyRefExpr( const ObjCPropertyRefExpr *PA) { if (PA->isExplicitProperty()) { const ObjCPropertyDecl *PD = PA->getExplicitProperty(); if (PD) { PD = cast(PD->getCanonicalDecl()); - IVars[PropertyToIvarMap[PD]] = true; - VisitStmt(PA); + PropToIvarMapTy::const_iterator IvI = PropertyToIvarMap.find(PD); + if (IvI != PropertyToIvarMap.end()) + markInvalidated(IvI->second); return; } } @@ -343,12 +457,95 @@ void IvarInvalidationChecker::MethodCrawler::VisitObjCPropertyRefExpr( const ObjCMethodDecl *MD = PA->getImplicitPropertySetter(); if (MD) { MD = cast(MD->getCanonicalDecl()); - IVars[PropertyAccessorToIvarMap[MD]] = true; - VisitStmt(PA); + MethToIvarMapTy::const_iterator IvI =PropertyGetterToIvarMap.find(MD); + if (IvI != PropertyGetterToIvarMap.end()) + markInvalidated(IvI->second); + return; + } + } +} + +bool IvarInvalidationChecker::MethodCrawler::isZero(const Expr *E) const { + E = peel(E); + if (const IntegerLiteral *IL = dyn_cast(E)) + return IL->getValue() == 0; + + if (const CastExpr *ICE = dyn_cast(E)) + return ICE->getCastKind() == CK_NullToPointer; + + return false; +} + +void IvarInvalidationChecker::MethodCrawler::check(const Expr *E) { + E = peel(E); + + if (const OpaqueValueExpr *OVE = dyn_cast(E)) + E = OVE->getSourceExpr(); + + if (const ObjCIvarRefExpr *IvarRef = dyn_cast(E)) { + checkObjCIvarRefExpr(IvarRef); + return; + } + + if (const ObjCPropertyRefExpr *PropRef = dyn_cast(E)) { + checkObjCPropertyRefExpr(PropRef); + return; + } + + if (const ObjCMessageExpr *MsgExpr = dyn_cast(E)) { + checkObjCMessageExpr(MsgExpr); + return; + } +} + +void IvarInvalidationChecker::MethodCrawler::VisitBinaryOperator( + const BinaryOperator *BO) { + if (!BO->isAssignmentOp()) + return; + + // Do we assign zero? + if (!isZero(BO->getRHS())) + return; + + // Check the variable we are assigning to. + check(BO->getLHS()); + + VisitStmt(BO); +} + +void IvarInvalidationChecker::MethodCrawler::VisitObjCMessageExpr( + const ObjCMessageExpr *ME) { + const ObjCMethodDecl *MD = ME->getMethodDecl(); + const Expr *Receiver = ME->getInstanceReceiver(); + + // Stop if we are calling '[self invalidate]'. + if (Receiver && isInvalidationMethod(MD)) + if (const DeclRefExpr *RD = + dyn_cast(Receiver->IgnoreParenCasts())) { + if (RD->getDecl() == EnclosingMethod->getSelfDecl()) { + CalledAnotherInvalidationMethod = true; + return; + } + } + + // Check if we call a setter and set the property to 'nil'. + if (MD && (ME->getNumArgs() == 1) && isZero(ME->getArg(0))) { + MD = cast(MD->getCanonicalDecl()); + MethToIvarMapTy::const_iterator IvI = PropertySetterToIvarMap.find(MD); + if (IvI != PropertySetterToIvarMap.end()) { + markInvalidated(IvI->second); return; } } - VisitStmt(PA); + + // Check if we call the 'invalidation' routine on the ivar. + if (Receiver) { + InvalidationMethod = MD; + check(Receiver->IgnoreParenCasts()); + InvalidationMethod = 0; + } + + VisitStmt(ME); } } diff --git a/test/Analysis/objc_invalidation.m b/test/Analysis/objc_invalidation.m index ededae64be..adcbbd62af 100644 --- a/test/Analysis/objc_invalidation.m +++ b/test/Analysis/objc_invalidation.m @@ -10,7 +10,11 @@ -(id)copy; - (Class)class; -(id)retain; +-(id)description; @end +@class NSString; + +extern void NSLog(NSString *format, ...) __attribute__((format(__NSString__, 1, 2))); @protocol Invalidation1 - (void) invalidate __attribute__((annotate("objc_instance_variable_invalidator"))); @@ -22,6 +26,7 @@ @protocol Invalidation3 - (void) invalidate __attribute__((annotate("objc_instance_variable_invalidator"))); +- (void) invalidate2 __attribute__((annotate("objc_instance_variable_invalidator"))); @end @interface Invalidation2Class @@ -31,7 +36,7 @@ @end @interface SomeInvalidationImplementingObject: NSObject { - SomeInvalidationImplementingObject *ObjA; + SomeInvalidationImplementingObject *ObjA; // invalidation in the parent } @end @@ -39,21 +44,28 @@ - (void)invalidate{ ObjA = 0; } +- (void)invalidate2 { + [self invalidate]; +} @end @interface SomeSubclassInvalidatableObject : SomeInvalidationImplementingObject { - SomeInvalidationImplementingObject *Obj1; - SomeInvalidationImplementingObject *Obj2; - SomeInvalidationImplementingObject *Obj3; - SomeInvalidationImplementingObject *_Prop1; - SomeInvalidationImplementingObject *_Prop4; - SomeInvalidationImplementingObject *_propIvar; - Invalidation1Class *MultipleProtocols; - Invalidation2Class *MultInheritance; - SomeInvalidationImplementingObject *_Prop5; // invalidate via getter method + SomeInvalidationImplementingObject *Ivar1; // regular ivar + SomeInvalidationImplementingObject *Ivar2; // regular ivar, sending invalidate message + SomeInvalidationImplementingObject *_Ivar3; // no property, call -description + SomeInvalidationImplementingObject *_Ivar4; // no property, provide as argument to NSLog() + + SomeInvalidationImplementingObject *_Prop1; // partially implemented property, set to 0 with dot syntax + SomeInvalidationImplementingObject *_Prop2; // fully implemented prop, set to 0 with dot syntax + SomeInvalidationImplementingObject *_propIvar; // property with custom named ivar, set to 0 via setter + Invalidation1Class *MultipleProtocols; // regular ivar belonging to a different class + Invalidation2Class *MultInheritance; // regular ivar belonging to a different class + SomeInvalidationImplementingObject *_Prop3; // property, invalidate via sending a message to a getter method + SomeInvalidationImplementingObject *_Prop4; // property with @synthesize, invalidate via property + SomeInvalidationImplementingObject *_Prop5; // property with @synthesize, invalidate via getter method - // No warnings on these. - NSObject *NObj1; + // No warnings on these as they are not invalidatable. + NSObject *NIvar1; NSObject *NObj2; NSObject *_NProp1; NSObject *_NpropIvar; @@ -63,10 +75,12 @@ @property (nonatomic, assign) SomeInvalidationImplementingObject* Prop1; @property (assign) SomeInvalidationImplementingObject* Prop2; @property (assign) SomeInvalidationImplementingObject* Prop3; -@property (assign) SomeInvalidationImplementingObject* Prop4; -@property (assign) SomeInvalidationImplementingObject* Prop5; -@property (assign) SomeInvalidationImplementingObject *SynthIvarProp; +@property (assign) SomeInvalidationImplementingObject *Prop5; +@property (assign) SomeInvalidationImplementingObject *Prop4; +@property (assign) SomeInvalidationImplementingObject* Prop6; // automatically synthesized prop +@property (assign) SomeInvalidationImplementingObject* Prop7; // automatically synthesized prop +@property (assign) SomeInvalidationImplementingObject *SynthIvarProp; @property (assign) NSObject* NProp0; @property (nonatomic, assign) NSObject* NProp1; @@ -81,18 +95,21 @@ @implementation SomeSubclassInvalidatableObject -@synthesize Prop2 = _propIvar; -@synthesize Prop3; +@synthesize Prop7 = _propIvar; +@synthesize Prop3 = _Prop3; +@synthesize Prop5 = _Prop5; +@synthesize Prop4 = _Prop4; + - (void) setProp1: (SomeInvalidationImplementingObject*) InObj { _Prop1 = InObj; } -- (void) setProp4: (SomeInvalidationImplementingObject*) InObj { - _Prop4 = InObj; +- (void) setProp2: (SomeInvalidationImplementingObject*) InObj { + _Prop2 = InObj; } -- (SomeInvalidationImplementingObject*) Prop4 { - return _Prop4; +- (SomeInvalidationImplementingObject*) Prop2 { + return _Prop2; } @synthesize NProp2 = _NpropIvar; @@ -102,17 +119,24 @@ } - (void) invalidate { - [Obj3 invalidate]; + [Ivar2 invalidate]; self.Prop0 = 0; self.Prop1 = 0; [self setProp2:0]; [self setProp3:0]; - self.Prop4 = 0; - [[self Prop5] invalidate]; + [[self Prop5] invalidate2]; + [self.Prop4 invalidate]; + self.Prop6 = 0; + [[self Prop7] invalidate]; + + [_Ivar3 description]; + NSLog(@"%@", _Ivar4); [super invalidate]; -}// expected-warning {{Instance variable Obj1 needs to be invalidated}} - // expected-warning@-1 {{Instance variable Obj2 needs to be invalidated}} +} +// expected-warning@-1 {{Instance variable Ivar1 needs to be invalidated}} // expected-warning@-2 {{Instance variable MultipleProtocols needs to be invalidated}} // expected-warning@-3 {{Instance variable MultInheritance needs to be invalidated}} // expected-warning@-4 {{Property SynthIvarProp needs to be invalidated}} + // expected-warning@-5 {{Instance variable _Ivar3 needs to be invalidated}} + // expected-warning@-6 {{Instance variable _Ivar4 needs to be invalidated}} @end