From eb6e64ca8fd72725153fc25914f94a771d9c8572 Mon Sep 17 00:00:00 2001 From: Douglas Gregor Date: Fri, 19 Jun 2015 23:17:46 +0000 Subject: [PATCH] Allow the cf_returns_[not_]retained attributes to appear on out-parameters. Includes a simple static analyzer check and not much else, but we'll also be able to take advantage of this in Swift. This feature can be tested for using __has_feature(cf_returns_on_parameters). This commit also contains two fixes: - Look through non-typedef sugar when deciding whether something is a CF type. - When (cf|ns)_returns(_not)?_retained is applied to invalid properties, refer to "property" instead of "method" in the error message. rdar://problem/18742441 llvm-svn: 240185 --- clang/include/clang/Basic/DiagnosticSemaKinds.td | 4 +- .../StaticAnalyzer/Checkers/ObjCRetainCount.h | 8 +++ clang/lib/Analysis/CocoaConventions.cpp | 2 +- clang/lib/Lex/PPMacroExpansion.cpp | 1 + clang/lib/Sema/SemaDeclAttr.cpp | 48 ++++++++++++-- .../StaticAnalyzer/Checkers/RetainCountChecker.cpp | 74 ++++++++++++++++++++-- clang/test/Analysis/retain-release.m | 27 ++++++++ clang/test/SemaObjC/attr-cf_returns.m | 49 ++++++++++++++ 8 files changed, 200 insertions(+), 13 deletions(-) create mode 100644 clang/test/SemaObjC/attr-cf_returns.m diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 18fe09a..803203c 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -2751,8 +2751,8 @@ def warn_ns_attribute_wrong_return_type : Warning< "return %select{an Objective-C object|a pointer|a non-retainable pointer}2">, InGroup; def warn_ns_attribute_wrong_parameter_type : Warning< - "%0 attribute only applies to %select{Objective-C object|pointer}1 " - "parameters">, + "%0 attribute only applies to " + "%select{Objective-C object|pointer|pointer-to-CF-pointer}1 parameters">, InGroup; def warn_objc_requires_super_protocol : Warning< "%0 attribute cannot be applied to %select{methods in protocols|dealloc}1">, diff --git a/clang/include/clang/StaticAnalyzer/Checkers/ObjCRetainCount.h b/clang/include/clang/StaticAnalyzer/Checkers/ObjCRetainCount.h index ab92a24..5850656 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/ObjCRetainCount.h +++ b/clang/include/clang/StaticAnalyzer/Checkers/ObjCRetainCount.h @@ -69,6 +69,14 @@ enum ArgEffect { /// transfers the object to the Garbage Collector under GC. MakeCollectable, + /// The argument is a pointer to a retain-counted object; on exit, the new + /// value of the pointer is a +0 value or NULL. + UnretainedOutParameter, + + /// The argument is a pointer to a retain-counted object; on exit, the new + /// value of the pointer is a +1 value or NULL. + RetainedOutParameter, + /// The argument is treated as potentially escaping, meaning that /// even when its reference count hits 0 it should be treated as still /// possibly being alive as someone else *may* be holding onto the object. diff --git a/clang/lib/Analysis/CocoaConventions.cpp b/clang/lib/Analysis/CocoaConventions.cpp index 0db3cac..be1262d 100644 --- a/clang/lib/Analysis/CocoaConventions.cpp +++ b/clang/lib/Analysis/CocoaConventions.cpp @@ -25,7 +25,7 @@ using namespace ento; bool cocoa::isRefType(QualType RetTy, StringRef Prefix, StringRef Name) { // Recursively walk the typedef stack, allowing typedefs of reference types. - while (const TypedefType *TD = dyn_cast(RetTy.getTypePtr())) { + while (const TypedefType *TD = RetTy->getAs()) { StringRef TDName = TD->getDecl()->getIdentifier()->getName(); if (TDName.startswith(Prefix) && TDName.endswith("Ref")) return true; diff --git a/clang/lib/Lex/PPMacroExpansion.cpp b/clang/lib/Lex/PPMacroExpansion.cpp index ad115ba..5e0b283 100644 --- a/clang/lib/Lex/PPMacroExpansion.cpp +++ b/clang/lib/Lex/PPMacroExpansion.cpp @@ -1059,6 +1059,7 @@ static bool HasFeature(const Preprocessor &PP, const IdentifierInfo *II) { .Case("attribute_availability_app_extension", true) .Case("attribute_cf_returns_not_retained", true) .Case("attribute_cf_returns_retained", true) + .Case("attribute_cf_returns_on_parameters", true) .Case("attribute_deprecated_with_message", true) .Case("attribute_ext_vector_type", true) .Case("attribute_ns_returns_not_retained", true) diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp index 0c2f9cf..43790c2 100644 --- a/clang/lib/Sema/SemaDeclAttr.cpp +++ b/clang/lib/Sema/SemaDeclAttr.cpp @@ -3730,10 +3730,31 @@ static void handleNSReturnsRetainedAttr(Sema &S, Decl *D, returnType = PD->getType(); else if (FunctionDecl *FD = dyn_cast(D)) returnType = FD->getReturnType(); - else { + else if (auto *Param = dyn_cast(D)) { + returnType = Param->getType()->getPointeeType(); + if (returnType.isNull()) { + S.Diag(D->getLocStart(), diag::warn_ns_attribute_wrong_parameter_type) + << Attr.getName() << /*pointer-to-CF*/2 + << Attr.getRange(); + return; + } + } else { + AttributeDeclKind ExpectedDeclKind; + switch (Attr.getKind()) { + default: llvm_unreachable("invalid ownership attribute"); + case AttributeList::AT_NSReturnsRetained: + case AttributeList::AT_NSReturnsAutoreleased: + case AttributeList::AT_NSReturnsNotRetained: + ExpectedDeclKind = ExpectedFunctionOrMethod; + break; + + case AttributeList::AT_CFReturnsRetained: + case AttributeList::AT_CFReturnsNotRetained: + ExpectedDeclKind = ExpectedFunctionMethodOrParameter; + break; + } S.Diag(D->getLocStart(), diag::warn_attribute_wrong_decl_type) - << Attr.getRange() << Attr.getName() - << ExpectedFunctionOrMethod; + << Attr.getRange() << Attr.getName() << ExpectedDeclKind; return; } @@ -3760,8 +3781,25 @@ static void handleNSReturnsRetainedAttr(Sema &S, Decl *D, } if (!typeOK) { - S.Diag(D->getLocStart(), diag::warn_ns_attribute_wrong_return_type) - << Attr.getRange() << Attr.getName() << isa(D) << cf; + if (isa(D)) { + S.Diag(D->getLocStart(), diag::warn_ns_attribute_wrong_parameter_type) + << Attr.getName() << /*pointer-to-CF*/2 + << Attr.getRange(); + } else { + // Needs to be kept in sync with warn_ns_attribute_wrong_return_type. + enum : unsigned { + Function, + Method, + Property + } SubjectKind = Function; + if (isa(D)) + SubjectKind = Method; + else if (isa(D)) + SubjectKind = Property; + S.Diag(D->getLocStart(), diag::warn_ns_attribute_wrong_return_type) + << Attr.getName() << SubjectKind << cf + << Attr.getRange(); + } return; } diff --git a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp index 58c27d4..49eef23 100644 --- a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp @@ -906,6 +906,8 @@ static ArgEffect getStopTrackingHardEquivalent(ArgEffect E) { case IncRef: case IncRefMsg: case MakeCollectable: + case UnretainedOutParameter: + case RetainedOutParameter: case MayEscape: case StopTracking: case StopTrackingHard: @@ -1335,7 +1337,18 @@ RetainSummaryManager::updateSummaryFromAnnotations(const RetainSummary *&Summ, if (pd->hasAttr()) Template->addArg(AF, parm_idx, DecRefMsg); else if (pd->hasAttr()) - Template->addArg(AF, parm_idx, DecRef); + Template->addArg(AF, parm_idx, DecRef); + else if (pd->hasAttr()) { + QualType PointeeTy = pd->getType()->getPointeeType(); + if (!PointeeTy.isNull()) + if (coreFoundation::isCFObjectRef(PointeeTy)) + Template->addArg(AF, parm_idx, RetainedOutParameter); + } else if (pd->hasAttr()) { + QualType PointeeTy = pd->getType()->getPointeeType(); + if (!PointeeTy.isNull()) + if (coreFoundation::isCFObjectRef(PointeeTy)) + Template->addArg(AF, parm_idx, UnretainedOutParameter); + } } QualType RetTy = FD->getReturnType(); @@ -1366,7 +1379,17 @@ RetainSummaryManager::updateSummaryFromAnnotations(const RetainSummary *&Summ, Template->addArg(AF, parm_idx, DecRefMsg); else if (pd->hasAttr()) { Template->addArg(AF, parm_idx, DecRef); - } + } else if (pd->hasAttr()) { + QualType PointeeTy = pd->getType()->getPointeeType(); + if (!PointeeTy.isNull()) + if (coreFoundation::isCFObjectRef(PointeeTy)) + Template->addArg(AF, parm_idx, RetainedOutParameter); + } else if (pd->hasAttr()) { + QualType PointeeTy = pd->getType()->getPointeeType(); + if (!PointeeTy.isNull()) + if (coreFoundation::isCFObjectRef(PointeeTy)) + Template->addArg(AF, parm_idx, UnretainedOutParameter); + } } QualType RetTy = MD->getReturnType(); @@ -2746,7 +2769,6 @@ void RetainCountChecker::checkPostStmt(const CastExpr *CE, if (hasErr) { // FIXME: If we get an error during a bridge cast, should we report it? - // Should we assert that there is no error? return; } @@ -2951,6 +2973,40 @@ void RetainCountChecker::processSummaryOfInlined(const RetainSummary &Summ, C.addTransition(state); } +static ProgramStateRef updateOutParameter(ProgramStateRef State, + SVal ArgVal, + ArgEffect Effect) { + auto *ArgRegion = dyn_cast_or_null(ArgVal.getAsRegion()); + if (!ArgRegion) + return State; + + QualType PointeeTy = ArgRegion->getValueType(); + if (!coreFoundation::isCFObjectRef(PointeeTy)) + return State; + + SVal PointeeVal = State->getSVal(ArgRegion); + SymbolRef Pointee = PointeeVal.getAsLocSymbol(); + if (!Pointee) + return State; + + switch (Effect) { + case UnretainedOutParameter: + State = setRefBinding(State, Pointee, + RefVal::makeNotOwned(RetEffect::CF, PointeeTy)); + break; + case RetainedOutParameter: + // Do nothing. Retained out parameters will either point to a +1 reference + // or NULL, but the way you check for failure differs depending on the API. + // Consequently, we don't have a good way to track them yet. + break; + + default: + llvm_unreachable("only for out parameters"); + } + + return State; +} + void RetainCountChecker::checkSummary(const RetainSummary &Summ, const CallEvent &CallOrMsg, CheckerContext &C) const { @@ -2964,9 +3020,12 @@ void RetainCountChecker::checkSummary(const RetainSummary &Summ, for (unsigned idx = 0, e = CallOrMsg.getNumArgs(); idx != e; ++idx) { SVal V = CallOrMsg.getArgSVal(idx); - if (SymbolRef Sym = V.getAsLocSymbol()) { + ArgEffect Effect = Summ.getArg(idx); + if (Effect == RetainedOutParameter || Effect == UnretainedOutParameter) { + state = updateOutParameter(state, V, Effect); + } else if (SymbolRef Sym = V.getAsLocSymbol()) { if (const RefVal *T = getRefBinding(state, Sym)) { - state = updateSymbol(state, Sym, *T, Summ.getArg(idx), hasErr, C); + state = updateSymbol(state, Sym, *T, Effect, hasErr, C); if (hasErr) { ErrorRange = CallOrMsg.getArgSourceRange(idx); ErrorSym = Sym; @@ -3115,6 +3174,11 @@ RetainCountChecker::updateSymbol(ProgramStateRef state, SymbolRef sym, case DecRefMsgAndStopTrackingHard: llvm_unreachable("DecRefMsg/IncRefMsg/MakeCollectable already converted"); + case UnretainedOutParameter: + case RetainedOutParameter: + llvm_unreachable("Applies to pointer-to-pointer parameters, which should " + "not have ref state."); + case Dealloc: // Any use of -dealloc in GC is *bad*. if (C.isObjCGCEnabled()) { diff --git a/clang/test/Analysis/retain-release.m b/clang/test/Analysis/retain-release.m index 6973f9b..e3ad409 100644 --- a/clang/test/Analysis/retain-release.m +++ b/clang/test/Analysis/retain-release.m @@ -2153,6 +2153,33 @@ id returnNSNull() { return [NSNull null]; // no-warning } +//===----------------------------------------------------------------------===// +// cf_returns_[not_]retained on parameters +//===----------------------------------------------------------------------===// + +void testCFReturnsNotRetained() { + extern void getViaParam(CFTypeRef * CF_RETURNS_NOT_RETAINED outObj); + CFTypeRef obj; + getViaParam(&obj); + CFRelease(obj); // // expected-warning {{Incorrect decrement of the reference count of an object that is not owned at this point by the caller}} +} + +void testCFReturnsRetained() { + extern int copyViaParam(CFTypeRef * CF_RETURNS_RETAINED outObj); + CFTypeRef obj; + copyViaParam(&obj); + CFRelease(obj); + CFRelease(obj); // // FIXME-warning {{Incorrect decrement of the reference count of an object that is not owned at this point by the caller}} +} + +void testCFReturnsRetainedError() { + extern int copyViaParam(CFTypeRef * CF_RETURNS_RETAINED outObj); + CFTypeRef obj; + if (copyViaParam(&obj) == -42) + return; // no-warning + CFRelease(obj); +} + // CHECK: diagnostics // CHECK-NEXT: // CHECK-NEXT: diff --git a/clang/test/SemaObjC/attr-cf_returns.m b/clang/test/SemaObjC/attr-cf_returns.m new file mode 100644 index 0000000..560d40d --- /dev/null +++ b/clang/test/SemaObjC/attr-cf_returns.m @@ -0,0 +1,49 @@ +// RUN: %clang_cc1 -verify -fsyntax-only -fobjc-arc -fblocks %s + +#if __has_feature(attribute_cf_returns_on_parameters) +# error "okay!" +// expected-error@-1 {{okay!}} +#else +# error "uh-oh" +#endif + +#define CF_RETURNS_RETAINED __attribute__((cf_returns_retained)) +#define CF_RETURNS_NOT_RETAINED __attribute__((cf_returns_not_retained)) + +int x CF_RETURNS_RETAINED; // expected-warning{{'cf_returns_retained' attribute only applies to functions, methods, and parameters}} +int y CF_RETURNS_NOT_RETAINED; // expected-warning{{'cf_returns_not_retained' attribute only applies to functions, methods, and parameters}} + +typedef struct __CFFoo *CFFooRef; + +int invalid1() CF_RETURNS_RETAINED; // expected-warning{{'cf_returns_retained' attribute only applies to functions that return a pointer}} +void invalid2() CF_RETURNS_RETAINED; // expected-warning{{'cf_returns_retained' attribute only applies to functions that return a pointer}} + +CFFooRef valid1() CF_RETURNS_RETAINED; +id valid2() CF_RETURNS_RETAINED; + +@interface Test +- (int)invalid1 CF_RETURNS_RETAINED; // expected-warning{{'cf_returns_retained' attribute only applies to methods that return a pointer}} +- (void)invalid2 CF_RETURNS_RETAINED; // expected-warning{{'cf_returns_retained' attribute only applies to methods that return a pointer}} + +- (CFFooRef)valid1 CF_RETURNS_RETAINED; +- (id)valid2 CF_RETURNS_RETAINED; + +@property int invalidProp1 CF_RETURNS_RETAINED; // expected-warning{{'cf_returns_retained' attribute only applies to properties that return a pointer}} +@property void invalidProp2 CF_RETURNS_RETAINED; // expected-warning{{'cf_returns_retained' attribute only applies to properties that return a pointer}} + +@property CFFooRef valid1 CF_RETURNS_RETAINED; +@property id valid2 CF_RETURNS_RETAINED; +@end + +void invalidParam(int a CF_RETURNS_RETAINED, // expected-warning{{'cf_returns_retained' attribute only applies to pointer-to-CF-pointer parameters}} + int *b CF_RETURNS_RETAINED, // expected-warning{{'cf_returns_retained' attribute only applies to pointer-to-CF-pointer parameters}} + id c CF_RETURNS_RETAINED, // expected-warning{{'cf_returns_retained' attribute only applies to pointer-to-CF-pointer parameters}} + void *d CF_RETURNS_RETAINED, // expected-warning{{'cf_returns_retained' attribute only applies to pointer-to-CF-pointer parameters}} + CFFooRef e CF_RETURNS_RETAINED); // expected-warning{{'cf_returns_retained' attribute only applies to pointer-to-CF-pointer parameters}} + +void validParam(id *a CF_RETURNS_RETAINED, + CFFooRef *b CF_RETURNS_RETAINED, + void **c CF_RETURNS_RETAINED); +void validParam2(id *a CF_RETURNS_NOT_RETAINED, + CFFooRef *b CF_RETURNS_NOT_RETAINED, + void **c CF_RETURNS_NOT_RETAINED); -- 2.7.4