From: Jordan Rose Date: Thu, 23 Jan 2014 03:59:10 +0000 (+0000) Subject: [analyzer] Tighten up sanity checks on Objective-C property getter synthesis. X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=8eca7d4e604cfa929ca9d237cffe5e99c80e3d5e;p=clang [analyzer] Tighten up sanity checks on Objective-C property getter synthesis. If there are non-trivially-copyable types /other/ than C++ records, we won't have a synthesized copy expression, but we can't just use a simple load/return. Also, add comments and shore up tests, making sure to test in both ARC and non-ARC. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@199869 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/Analysis/BodyFarm.cpp b/lib/Analysis/BodyFarm.cpp index 6ec63c7221..737e15061e 100644 --- a/lib/Analysis/BodyFarm.cpp +++ b/lib/Analysis/BodyFarm.cpp @@ -387,12 +387,20 @@ Stmt *BodyFarm::getBody(const FunctionDecl *D) { static Stmt *createObjCPropertyGetter(ASTContext &Ctx, const ObjCPropertyDecl *Prop) { + // First, find the backing ivar. const ObjCIvarDecl *IVar = Prop->getPropertyIvarDecl(); if (!IVar) return 0; + + // Ignore weak variables, which have special behavior. if (Prop->getPropertyAttributes() & ObjCPropertyDecl::OBJC_PR_weak) return 0; + // Look to see if Sema has synthesized a body for us. This happens in + // Objective-C++ because the return value may be a C++ class type with a + // non-trivial copy constructor. We can only do this if we can find the + // @synthesize for this property, though (or if we know it's been auto- + // synthesized). const ObjCImplementationDecl *ImplDecl = IVar->getContainingInterface()->getImplementation(); if (ImplDecl) { @@ -410,10 +418,18 @@ static Stmt *createObjCPropertyGetter(ASTContext &Ctx, } } - if (IVar->getType().getCanonicalType() != - Prop->getType().getNonReferenceType().getCanonicalType()) + // Sanity check that the property is the same type as the ivar, or a + // reference to it, and that it is either an object pointer or trivially + // copyable. + if (!Ctx.hasSameUnqualifiedType(IVar->getType(), + Prop->getType().getNonReferenceType())) + return 0; + if (!IVar->getType()->isObjCLifetimeType() && + !IVar->getType().isTriviallyCopyableType(Ctx)) return 0; + // Generate our body: + // return self->_ivar; ASTMaker M(Ctx); const VarDecl *selfVar = Prop->getGetterMethodDecl()->getSelfDecl(); @@ -431,7 +447,8 @@ static Stmt *createObjCPropertyGetter(ASTContext &Ctx, return M.makeReturn(loadedIVar); } -Stmt *BodyFarm::getBody(const ObjCMethodDecl *D, const ObjCPropertyDecl *Prop) { +Stmt *BodyFarm::getBody(const ObjCMethodDecl *D) { + // We currently only know how to synthesize property accessors. if (!D->isPropertyAccessor()) return 0; @@ -442,11 +459,11 @@ Stmt *BodyFarm::getBody(const ObjCMethodDecl *D, const ObjCPropertyDecl *Prop) { return Val.getValue(); Val = 0; - if (!Prop) - Prop = D->findPropertyDecl(); + const ObjCPropertyDecl *Prop = D->findPropertyDecl(); if (!Prop) return 0; + // For now, we only synthesize getters. if (D->param_size() != 0) return 0; diff --git a/lib/Analysis/BodyFarm.h b/lib/Analysis/BodyFarm.h index c4f3d1599e..2d200fb755 100644 --- a/lib/Analysis/BodyFarm.h +++ b/lib/Analysis/BodyFarm.h @@ -36,7 +36,7 @@ public: Stmt *getBody(const FunctionDecl *D); /// Factory method for creating bodies for Objective-C properties. - Stmt *getBody(const ObjCMethodDecl *D, const ObjCPropertyDecl *Prop = 0); + Stmt *getBody(const ObjCMethodDecl *D); private: typedef llvm::DenseMap > BodyMap; diff --git a/test/Analysis/properties.m b/test/Analysis/properties.m index b3e654f377..5ac20c18c9 100644 --- a/test/Analysis/properties.m +++ b/test/Analysis/properties.m @@ -1,4 +1,5 @@ // RUN: %clang_cc1 -analyze -analyzer-checker=core,osx.cocoa.RetainCount,debug.ExprInspection -analyzer-store=region -verify -Wno-objc-root-class %s +// RUN: %clang_cc1 -analyze -analyzer-checker=core,osx.cocoa.RetainCount,debug.ExprInspection -analyzer-store=region -verify -Wno-objc-root-class -fobjc-arc %s void clang_analyzer_eval(int); @@ -39,6 +40,8 @@ typedef struct _NSZone NSZone; @end +#if !__has_feature(objc_arc) + @implementation Test1 @synthesize text; @@ -117,6 +120,8 @@ NSNumber* numberFromMyNumberProperty(MyNumber* aMyNumber) return [result autorelease]; // expected-warning {{Object autoreleased too many times}} } +#endif + // rdar://6611873 @@ -124,18 +129,23 @@ NSNumber* numberFromMyNumberProperty(MyNumber* aMyNumber) NSString *_name; } @property (retain) NSString * name; +@property (assign) id friend; @end @implementation Person @synthesize name = _name; @end +#if !__has_feature(objc_arc) void rdar6611873() { Person *p = [[[Person alloc] init] autorelease]; p.name = [[NSString string] retain]; // expected-warning {{leak}} p.name = [[NSString alloc] init]; // expected-warning {{leak}} + + p.friend = [[Person alloc] init]; // expected-warning {{leak}} } +#endif @interface SubPerson : Person -(NSString *)foo; @@ -147,6 +157,8 @@ void rdar6611873() { } @end + +#if !__has_feature(objc_arc) // Static analyzer doesn't detect uninitialized variable issues for property accesses @interface RDar9241180 @property (readwrite,assign) id x; @@ -167,13 +179,14 @@ void rdar6611873() { self.x = y; // expected-warning {{Argument for property setter is an uninitialized value}} } @end +#endif //------ // Property accessor synthesis //------ -void testConsistency(Person *p) { +void testConsistencyRetain(Person *p) { clang_analyzer_eval(p.name == p.name); // expected-warning{{TRUE}} extern void doSomethingWithPerson(Person *p); @@ -183,9 +196,24 @@ void testConsistency(Person *p) { clang_analyzer_eval(p.name == origName); // expected-warning{{UNKNOWN}} } -void testOverrelease(Person *p) { - [p.name release]; // expected-warning{{not owned}} +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); + clang_analyzer_eval(p.friend == origFriend); // expected-warning{{UNKNOWN}} +} + +#if !__has_feature(objc_arc) +void testOverrelease(Person *p, int coin) { + if (coin) + [p.name release]; // expected-warning{{not owned}} + else + [p.friend release]; // expected-warning{{not owned}} } +#endif @interface IntWrapper @property int value; @@ -212,6 +240,32 @@ void testConsistencyInt2(IntWrapper *w) { clang_analyzer_eval(w.value == 42); // expected-warning{{TRUE}} } + +@interface IntWrapperAuto +@property int value; +@end + +@implementation IntWrapperAuto +@end + +void testConsistencyIntAuto(IntWrapperAuto *w) { + clang_analyzer_eval(w.value == w.value); // expected-warning{{TRUE}} + + int origValue = w.value; + if (origValue != 42) + return; + + clang_analyzer_eval(w.value == 42); // expected-warning{{TRUE}} +} + +void testConsistencyIntAuto2(IntWrapperAuto *w) { + if (w.value != 42) + return; + + clang_analyzer_eval(w.value == 42); // expected-warning{{TRUE}} +} + + typedef struct { int value; } IntWrapperStruct; diff --git a/test/Analysis/properties.mm b/test/Analysis/properties.mm index 57aacd4f9d..e49d034f26 100644 --- a/test/Analysis/properties.mm +++ b/test/Analysis/properties.mm @@ -1,4 +1,5 @@ // RUN: %clang_cc1 -analyze -analyzer-checker=core,osx.cocoa.RetainCount,debug.ExprInspection -analyzer-store=region -verify -Wno-objc-root-class %s +// RUN: %clang_cc1 -analyze -analyzer-checker=core,osx.cocoa.RetainCount,debug.ExprInspection -analyzer-store=region -verify -Wno-objc-root-class -fobjc-arc %s void clang_analyzer_eval(bool); void clang_analyzer_checkInlined(bool); @@ -27,8 +28,6 @@ void testReferenceAssignment(IntWrapper *w) { } -// FIXME: Handle C++ structs, which need to go through the copy constructor. - struct IntWrapperStruct { int value; }; @@ -66,7 +65,7 @@ public: @end @implementation CustomCopyWrapper -@synthesize inner; +//@synthesize inner; @end void testConsistencyCustomCopy(CustomCopyWrapper *w) {