From 0b21d8243785f3003f0cf944b9b5da402ca81841 Mon Sep 17 00:00:00 2001 From: Argyrios Kyrtzidis Date: Wed, 23 May 2012 21:50:04 +0000 Subject: [PATCH] [arcmt] Remove an unused -autorelease, without failing with error, for this idiom that is used commonly in setters: [backingValue autorelease]; backingValue = [newValue retain]; // in general a +1 assign rdar://9914061 llvm-svn: 157347 --- clang/lib/ARCMigrate/TransProperties.cpp | 11 +-- clang/lib/ARCMigrate/TransRetainReleaseDealloc.cpp | 92 ++++++++++++++++++++-- clang/lib/ARCMigrate/Transforms.cpp | 42 ++++++++++ clang/lib/ARCMigrate/Transforms.h | 2 + clang/test/ARCMT/Common.h | 11 +++ clang/test/ARCMT/autoreleases.m | 47 +++++++---- clang/test/ARCMT/autoreleases.m.result | 43 ++++++---- clang/test/ARCMT/checking.m | 3 + clang/test/ARCMT/dispatch.m | 11 --- clang/test/ARCMT/dispatch.m.result | 11 --- 10 files changed, 203 insertions(+), 70 deletions(-) diff --git a/clang/lib/ARCMigrate/TransProperties.cpp b/clang/lib/ARCMigrate/TransProperties.cpp index 8f65fc3..5ec918f 100644 --- a/clang/lib/ARCMigrate/TransProperties.cpp +++ b/clang/lib/ARCMigrate/TransProperties.cpp @@ -309,17 +309,8 @@ private: if (RE->getDecl() != Ivar) return true; - if (ObjCMessageExpr * - ME = dyn_cast(E->getRHS()->IgnoreParenCasts())) - if (ME->getMethodFamily() == OMF_retain) + if (isPlusOneAssign(E)) return false; - - ImplicitCastExpr *implCE = dyn_cast(E->getRHS()); - while (implCE && implCE->getCastKind() == CK_BitCast) - implCE = dyn_cast(implCE->getSubExpr()); - - if (implCE && implCE->getCastKind() == CK_ARCConsumeObject) - return false; } return true; diff --git a/clang/lib/ARCMigrate/TransRetainReleaseDealloc.cpp b/clang/lib/ARCMigrate/TransRetainReleaseDealloc.cpp index 11a6553..df3cd58 100644 --- a/clang/lib/ARCMigrate/TransRetainReleaseDealloc.cpp +++ b/clang/lib/ARCMigrate/TransRetainReleaseDealloc.cpp @@ -64,14 +64,16 @@ public: return true; case OMF_autorelease: if (isRemovable(E)) { - // An unused autorelease is badness. If we remove it the receiver - // will likely die immediately while previously it was kept alive - // by the autorelease pool. This is bad practice in general, leave it - // and emit an error to force the user to restructure his code. - Pass.TA.reportError("it is not safe to remove an unused 'autorelease' " - "message; its receiver may be destroyed immediately", - E->getLocStart(), E->getSourceRange()); - return true; + if (!isCommonUnusedAutorelease(E)) { + // An unused autorelease is badness. If we remove it the receiver + // will likely die immediately while previously it was kept alive + // by the autorelease pool. This is bad practice in general, leave it + // and emit an error to force the user to restructure his code. + Pass.TA.reportError("it is not safe to remove an unused 'autorelease' " + "message; its receiver may be destroyed immediately", + E->getLocStart(), E->getSourceRange()); + return true; + } } // Pass through. case OMF_retain: @@ -156,6 +158,80 @@ public: } private: + /// \brief Checks for idioms where an unused -autorelease is common. + /// + /// Currently only returns true for this idiom which is common in property + /// setters: + /// + /// [backingValue autorelease]; + /// backingValue = [newValue retain]; // in general a +1 assign + /// + bool isCommonUnusedAutorelease(ObjCMessageExpr *E) { + Expr *Rec = E->getInstanceReceiver(); + if (!Rec) + return false; + + Decl *RefD = getReferencedDecl(Rec); + if (!RefD) + return false; + + Stmt *OuterS = E, *InnerS; + do { + InnerS = OuterS; + OuterS = StmtMap->getParent(InnerS); + } + while (OuterS && (isa(OuterS) || + isa(OuterS) || + isa(OuterS))); + + if (!OuterS) + return false; + + // Find next statement after the -autorelease. + + Stmt::child_iterator currChildS = OuterS->child_begin(); + Stmt::child_iterator childE = OuterS->child_end(); + for (; currChildS != childE; ++currChildS) { + if (*currChildS == InnerS) + break; + } + if (currChildS == childE) + return false; + ++currChildS; + if (currChildS == childE) + return false; + + Stmt *nextStmt = *currChildS; + if (!nextStmt) + return false; + nextStmt = nextStmt->IgnoreImplicit(); + + // Check for "RefD = [+1 retained object];". + + if (BinaryOperator *Bop = dyn_cast(nextStmt)) { + if (RefD != getReferencedDecl(Bop->getLHS())) + return false; + if (isPlusOneAssign(Bop)) + return true; + } + return false; + } + + Decl *getReferencedDecl(Expr *E) { + if (!E) + return 0; + + E = E->IgnoreParenCasts(); + if (DeclRefExpr *DRE = dyn_cast(E)) + return DRE->getDecl(); + if (MemberExpr *ME = dyn_cast(E)) + return ME->getMemberDecl(); + if (ObjCIvarRefExpr *IRE = dyn_cast(E)) + return IRE->getDecl(); + + return 0; + } + /// \brief Check if the retain/release is due to a GCD/XPC macro that are /// defined as: /// diff --git a/clang/lib/ARCMigrate/Transforms.cpp b/clang/lib/ARCMigrate/Transforms.cpp index 50bea95..24b6505 100644 --- a/clang/lib/ARCMigrate/Transforms.cpp +++ b/clang/lib/ARCMigrate/Transforms.cpp @@ -14,6 +14,7 @@ #include "clang/AST/StmtVisitor.h" #include "clang/Lex/Lexer.h" #include "clang/Basic/SourceManager.h" +#include "clang/Analysis/DomainSpecific/CocoaConventions.h" #include "llvm/ADT/StringSwitch.h" #include "llvm/ADT/DenseSet.h" #include @@ -56,6 +57,47 @@ bool trans::canApplyWeak(ASTContext &Ctx, QualType type, return true; } +bool trans::isPlusOneAssign(const BinaryOperator *E) { + if (E->getOpcode() != BO_Assign) + return false; + + if (const ObjCMessageExpr * + ME = dyn_cast(E->getRHS()->IgnoreParenCasts())) + if (ME->getMethodFamily() == OMF_retain) + return true; + + if (const CallExpr * + callE = dyn_cast(E->getRHS()->IgnoreParenCasts())) { + if (const FunctionDecl *FD = callE->getDirectCallee()) { + if (FD->getAttr()) + return true; + + if (FD->isGlobal() && + FD->getIdentifier() && + FD->getParent()->isTranslationUnit() && + FD->getLinkage() == ExternalLinkage && + ento::cocoa::isRefType(callE->getType(), "CF", + FD->getIdentifier()->getName())) { + StringRef fname = FD->getIdentifier()->getName(); + if (fname.endswith("Retain") || + fname.find("Create") != StringRef::npos || + fname.find("Copy") != StringRef::npos) { + return true; + } + } + } + } + + const ImplicitCastExpr *implCE = dyn_cast(E->getRHS()); + while (implCE && implCE->getCastKind() == CK_BitCast) + implCE = dyn_cast(implCE->getSubExpr()); + + if (implCE && implCE->getCastKind() == CK_ARCConsumeObject) + return true; + + return false; +} + /// \brief 'Loc' is the end of a statement range. This returns the location /// immediately after the semicolon following the statement. /// If no semicolon is found or the location is inside a macro, the returned diff --git a/clang/lib/ARCMigrate/Transforms.h b/clang/lib/ARCMigrate/Transforms.h index 445c3e5..7abc030 100644 --- a/clang/lib/ARCMigrate/Transforms.h +++ b/clang/lib/ARCMigrate/Transforms.h @@ -154,6 +154,8 @@ public: bool canApplyWeak(ASTContext &Ctx, QualType type, bool AllowOnUnknownClass = false); +bool isPlusOneAssign(const BinaryOperator *E); + /// \brief 'Loc' is the end of a statement range. This returns the location /// immediately after the semicolon following the statement. /// If no semicolon is found or the location is inside a macro, the returned diff --git a/clang/test/ARCMT/Common.h b/clang/test/ARCMT/Common.h index 16856ed..708e407 100644 --- a/clang/test/ARCMT/Common.h +++ b/clang/test/ARCMT/Common.h @@ -68,3 +68,14 @@ typedef const void* objc_objectptr_t; extern __attribute__((ns_returns_retained)) id objc_retainedObject(objc_objectptr_t __attribute__((cf_consumed)) pointer); extern __attribute__((ns_returns_not_retained)) id objc_unretainedObject(objc_objectptr_t pointer); extern objc_objectptr_t objc_unretainedPointer(id object); + +#define dispatch_retain(object) ({ dispatch_object_t _o = (object); _dispatch_object_validate(_o); (void)[_o retain]; }) +#define dispatch_release(object) ({ dispatch_object_t _o = (object); _dispatch_object_validate(_o); [_o release]; }) +#define xpc_retain(object) ({ xpc_object_t _o = (object); _xpc_object_validate(_o); [_o retain]; }) +#define xpc_release(object) ({ xpc_object_t _o = (object); _xpc_object_validate(_o); [_o release]; }) + +typedef id dispatch_object_t; +typedef id xpc_object_t; + +void _dispatch_object_validate(dispatch_object_t object); +void _xpc_object_validate(xpc_object_t object); diff --git a/clang/test/ARCMT/autoreleases.m b/clang/test/ARCMT/autoreleases.m index 3acddb7..a131bc5 100644 --- a/clang/test/ARCMT/autoreleases.m +++ b/clang/test/ARCMT/autoreleases.m @@ -3,29 +3,21 @@ // RUN: diff %t %s.result // DISABLE: mingw32 -typedef unsigned char BOOL; +#include "Common.h" -@interface NSObject { - id isa; -} -+new; -+alloc; --init; --autorelease; -@end - -@interface NSAutoreleasePool : NSObject -- drain; -@end - @interface A : NSObject { @package id object; } @end -@interface B : NSObject +@interface B : NSObject { + id _prop; + xpc_object_t _xpc_prop; +} - (BOOL)containsSelf:(A*)a; +@property (retain) id prop; +@property (retain) xpc_object_t xpc_prop; @end @implementation A @@ -35,6 +27,26 @@ typedef unsigned char BOOL; - (BOOL)containsSelf:(A*)a { return a->object == self; } + +-(id) prop { + return _prop; +} +-(void) setProp:(id) newVal { + [_prop autorelease]; + _prop = [newVal retain]; +} +-(void) setProp2:(CFTypeRef) newVal { + [_prop autorelease]; + _prop = (id)CFRetain(newVal); +} + +-(id) xpc_prop { + return _xpc_prop; +} +-(void) setXpc_prop:(xpc_object_t) newVal { + [_xpc_prop autorelease]; + _xpc_prop = xpc_retain(newVal); +} @end void NSLog(id, ...); @@ -47,3 +59,8 @@ int main (int argc, const char * argv[]) { [pool drain]; return 0; } + +void test(A *prevVal, A *newVal) { + [prevVal autorelease]; + prevVal = [newVal retain]; +} diff --git a/clang/test/ARCMT/autoreleases.m.result b/clang/test/ARCMT/autoreleases.m.result index 49bc321..93ec971 100644 --- a/clang/test/ARCMT/autoreleases.m.result +++ b/clang/test/ARCMT/autoreleases.m.result @@ -3,29 +3,21 @@ // RUN: diff %t %s.result // DISABLE: mingw32 -typedef unsigned char BOOL; +#include "Common.h" -@interface NSObject { - id isa; -} -+new; -+alloc; --init; --autorelease; -@end - -@interface NSAutoreleasePool : NSObject -- drain; -@end - @interface A : NSObject { @package id object; } @end -@interface B : NSObject +@interface B : NSObject { + id _prop; + xpc_object_t _xpc_prop; +} - (BOOL)containsSelf:(A*)a; +@property (strong) id prop; +@property (strong) xpc_object_t xpc_prop; @end @implementation A @@ -35,6 +27,23 @@ typedef unsigned char BOOL; - (BOOL)containsSelf:(A*)a { return a->object == self; } + +-(id) prop { + return _prop; +} +-(void) setProp:(id) newVal { + _prop = newVal; +} +-(void) setProp2:(CFTypeRef) newVal { + _prop = (__bridge_transfer id)CFRetain(newVal); +} + +-(id) xpc_prop { + return _xpc_prop; +} +-(void) setXpc_prop:(xpc_object_t) newVal { + _xpc_prop = newVal; +} @end void NSLog(id, ...); @@ -47,3 +56,7 @@ int main (int argc, const char * argv[]) { } return 0; } + +void test(A *prevVal, A *newVal) { + prevVal = newVal; +} diff --git a/clang/test/ARCMT/checking.m b/clang/test/ARCMT/checking.m index cf71611..71c6796 100644 --- a/clang/test/ARCMT/checking.m +++ b/clang/test/ARCMT/checking.m @@ -91,6 +91,9 @@ void test1(A *a, BOOL b, struct UnsafeS *unsafeS) { [a release]; [a autorelease]; // expected-error {{it is not safe to remove an unused 'autorelease' message; its receiver may be destroyed immediately}} \ // expected-error {{ARC forbids explicit message send}} + [a autorelease]; // expected-error {{it is not safe to remove an unused 'autorelease' message; its receiver may be destroyed immediately}} \ + // expected-error {{ARC forbids explicit message send}} + a = 0; CFStringRef cfstr; NSString *str = (NSString *)cfstr; // expected-error {{cast of C pointer type 'CFStringRef' (aka 'const struct __CFString *') to Objective-C pointer type 'NSString *' requires a bridged cast}} \ diff --git a/clang/test/ARCMT/dispatch.m b/clang/test/ARCMT/dispatch.m index 75c4a83..58c7769 100644 --- a/clang/test/ARCMT/dispatch.m +++ b/clang/test/ARCMT/dispatch.m @@ -4,17 +4,6 @@ #include "Common.h" -#define dispatch_retain(object) ({ dispatch_object_t _o = (object); _dispatch_object_validate(_o); (void)[_o retain]; }) -#define dispatch_release(object) ({ dispatch_object_t _o = (object); _dispatch_object_validate(_o); [_o release]; }) -#define xpc_retain(object) ({ xpc_object_t _o = (object); _xpc_object_validate(_o); [_o retain]; }) -#define xpc_release(object) ({ xpc_object_t _o = (object); _xpc_object_validate(_o); [_o release]; }) - -typedef id dispatch_object_t; -typedef id xpc_object_t; - -void _dispatch_object_validate(dispatch_object_t object); -void _xpc_object_validate(xpc_object_t object); - dispatch_object_t getme(void); void func(dispatch_object_t o) { diff --git a/clang/test/ARCMT/dispatch.m.result b/clang/test/ARCMT/dispatch.m.result index e897672..55b6558 100644 --- a/clang/test/ARCMT/dispatch.m.result +++ b/clang/test/ARCMT/dispatch.m.result @@ -4,17 +4,6 @@ #include "Common.h" -#define dispatch_retain(object) ({ dispatch_object_t _o = (object); _dispatch_object_validate(_o); (void)[_o retain]; }) -#define dispatch_release(object) ({ dispatch_object_t _o = (object); _dispatch_object_validate(_o); [_o release]; }) -#define xpc_retain(object) ({ xpc_object_t _o = (object); _xpc_object_validate(_o); [_o retain]; }) -#define xpc_release(object) ({ xpc_object_t _o = (object); _xpc_object_validate(_o); [_o release]; }) - -typedef id dispatch_object_t; -typedef id xpc_object_t; - -void _dispatch_object_validate(dispatch_object_t object); -void _xpc_object_validate(xpc_object_t object); - dispatch_object_t getme(void); void func(dispatch_object_t o) { -- 2.7.4