From d393458c3316257e5c4bf9dfdb9ace8c426edce8 Mon Sep 17 00:00:00 2001 From: Jordan Rose Date: Fri, 28 Sep 2012 22:21:30 +0000 Subject: [PATCH] Add a warning (off by default) for repeated use of the same weak property. The motivating example: if (self.weakProp) use(self.weakProp); As with any non-atomic test-then-use, it is possible a weak property to be non-nil at the 'if', but be deallocated by the time it is used. The correct way to write this example is as follows: id tmp = self.weakProp; if (tmp) use(tmp); The warning is controlled by -Warc-repeated-use-of-receiver, and uses the property name and base to determine if the same property on the same object is being accessed multiple times. In cases where the base is more complicated than just a single Decl (e.g. 'foo.bar.weakProp'), it picks a Decl for some degree of uniquing and reports the problem under a subflag, -Warc-maybe-repeated-use-of-receiver. This gives a way to tune the aggressiveness of the warning for a particular project. The warning is not on by default because it is not flow-sensitive and thus may have a higher-than-acceptable rate of false positives, though it is less noisy than -Wreceiver-is-weak. On the other hand, it will not warn about some cases that may be legitimate issues that -Wreceiver-is-weak will catch, and it does not attempt to reason about methods returning weak values. Even though this is not a real "analysis-based" check I've put the bug emission code in AnalysisBasedWarnings for two reasons: (1) to run on every kind of code body (function, method, block, or lambda), and (2) to suggest that it may be enhanced by flow-sensitive analysis in the future. The second (smaller) half of this work is to extend it to weak locals and weak ivars. This should use most of the same infrastructure. Part of llvm-svn: 164854 --- clang/include/clang/Basic/DiagnosticGroups.td | 3 + clang/include/clang/Basic/DiagnosticSemaKinds.td | 12 ++ clang/include/clang/Sema/ScopeInfo.h | 153 +++++++++++++++++ clang/lib/Sema/AnalysisBasedWarnings.cpp | 120 ++++++++++++++ clang/lib/Sema/Sema.cpp | 126 ++++++++++++++ clang/lib/Sema/SemaDecl.cpp | 15 ++ clang/lib/Sema/SemaExpr.cpp | 13 ++ clang/lib/Sema/SemaPseudoObject.cpp | 35 +++- clang/test/SemaObjC/arc-repeated-weak.mm | 203 +++++++++++++++++++++++ 9 files changed, 679 insertions(+), 1 deletion(-) create mode 100644 clang/test/SemaObjC/arc-repeated-weak.mm diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td index 32c03f7..156cf54 100644 --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/clang/include/clang/Basic/DiagnosticGroups.td @@ -282,6 +282,9 @@ def AutomaticReferenceCounting : DiagGroup<"arc", [ARCUnsafeRetainedAssign, ARCRetainCycles, ARCNonPodMemAccess]>; +def ARCRepeatedUseOfWeakMaybe : DiagGroup<"arc-maybe-repeated-use-of-weak">; +def ARCRepeatedUseOfWeak : DiagGroup<"arc-repeated-use-of-weak", + [ARCRepeatedUseOfWeakMaybe]>; def Selector : DiagGroup<"selector">; def Protocol : DiagGroup<"protocol">; def SuperSubClassMismatch : DiagGroup<"super-class-method-mismatch">; diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 6c92a6d..fa49216 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -709,6 +709,18 @@ def warn_receiver_is_weak : Warning < "weak %select{receiver|property|implicit property}0 may be " "unpredictably null in ARC mode">, InGroup>, DefaultIgnore; +def warn_arc_repeated_use_of_weak : Warning < + "weak property is accessed multiple times in this " + "%select{function|method|block|lambda}0 but may be unpredictably set to nil; " + "assign to a strong variable to keep the object alive">, + InGroup, DefaultIgnore; +def warn_arc_possible_repeated_use_of_weak : Warning < + "weak property may be accessed multiple times in this " + "%select{function|method|block|lambda}0 but may be unpredictably set to nil; " + "assign to a strong variable to keep the object alive">, + InGroup, DefaultIgnore; +def note_arc_weak_also_accessed_here : Note< + "also accessed here">; def err_incomplete_synthesized_property : Error< "cannot synthesize property %0 with incomplete type %1">; diff --git a/clang/include/clang/Sema/ScopeInfo.h b/clang/include/clang/Sema/ScopeInfo.h index b4752f5..e4a663c 100644 --- a/clang/include/clang/Sema/ScopeInfo.h +++ b/clang/include/clang/Sema/ScopeInfo.h @@ -21,6 +21,7 @@ namespace clang { +class Decl; class BlockDecl; class CXXMethodDecl; class IdentifierInfo; @@ -29,6 +30,7 @@ class ReturnStmt; class Scope; class SwitchStmt; class VarDecl; +class ObjCPropertyRefExpr; namespace sema { @@ -113,6 +115,125 @@ public: /// prior to being emitted. SmallVector PossiblyUnreachableDiags; +public: + /// Represents a simple identification of a weak object. + /// + /// Part of the implementation of -Wrepeated-use-of-weak. + /// + /// This is used to determine if two weak accesses refer to the same object. + /// Here are some examples of how various accesses are "profiled": + /// + /// Access Expression | "Base" Decl | "Property" Decl + /// :---------------: | :-----------------: | :------------------------------: + /// self.property | self (VarDecl) | property (ObjCPropertyDecl) + /// self.implicitProp | self (VarDecl) | -implicitProp (ObjCMethodDecl) + /// self->ivar.prop | ivar (ObjCIvarDecl) | prop (ObjCPropertyDecl) + /// cxxObj.obj.prop | obj (FieldDecl) | prop (ObjCPropertyDecl) + /// [self foo].prop | 0 (unknown) | prop (ObjCPropertyDecl) + /// self.prop1.prop2 | prop1 (ObjCPropertyDecl) | prop2 (ObjCPropertyDecl) + /// MyClass.prop | MyClass (ObjCInterfaceDecl) | -prop (ObjCMethodDecl) + /// + /// Objects are identified with only two Decls to make it reasonably fast to + /// compare them. + class WeakObjectProfileTy { + /// The base object decl, as described in the class documentation. + /// + /// The extra flag is "true" if the Base and Property are enough to uniquely + /// identify the object in memory. + /// + /// \sa isExactProfile() + typedef llvm::PointerIntPair BaseInfoTy; + BaseInfoTy Base; + + /// The "property" decl, as described in the class documentation. + /// + /// Note that this may not actually be an ObjCPropertyDecl, e.g. in the + /// case of "implicit" properties (regular methods accessed via dot syntax). + const NamedDecl *Property; + + // For use in DenseMap. + friend struct llvm::DenseMapInfo; + inline WeakObjectProfileTy(); + static inline WeakObjectProfileTy getSentinel(); + + public: + WeakObjectProfileTy(const ObjCPropertyRefExpr *RE); + + const NamedDecl *getProperty() const { return Property; } + + /// Returns true if the object base specifies a known object in memory, + /// rather than, say, an instance variable or property of another object. + /// + /// Note that this ignores the effects of aliasing; that is, \c foo.bar is + /// considered an exact profile if \c foo is a local variable, even if + /// another variable \c foo2 refers to the same object as \c foo. + /// + /// For increased precision, accesses with base variables that are + /// properties or ivars of 'self' (e.g. self.prop1.prop2) are considered to + /// be exact, though this is not true for arbitrary variables + /// (foo.prop1.prop2). + bool isExactProfile() const { + return Base.getInt(); + } + + bool operator==(const WeakObjectProfileTy &Other) const { + return Base == Other.Base && Property == Other.Property; + } + }; + + /// Represents a single use of a weak object. + /// + /// Stores both the expression and whether the access is potentially unsafe + /// (i.e. it could potentially be warned about). + /// + /// Part of the implementation of -Wrepeated-use-of-weak. + class WeakUseTy { + llvm::PointerIntPair Rep; + public: + WeakUseTy(const Expr *Use, bool IsRead) : Rep(Use, IsRead) {} + + const Expr *getUseExpr() const { return Rep.getPointer(); } + bool isUnsafe() const { return Rep.getInt(); } + void markSafe() { Rep.setInt(false); } + + bool operator==(const WeakUseTy &Other) const { + return Rep == Other.Rep; + } + }; + + /// Used to collect uses of a particular weak object in a function body. + /// + /// Part of the implementation of -Wrepeated-use-of-weak. + typedef SmallVector WeakUseVector; + + /// Used to collect all uses of weak objects in a function body. + /// + /// Part of the implementation of -Wrepeated-use-of-weak. + typedef llvm::SmallDenseMap + WeakObjectUseMap; + +private: + /// Used to collect all uses of weak objects in this function body. + /// + /// Part of the implementation of -Wrepeated-use-of-weak. + WeakObjectUseMap WeakObjectUses; + +public: + /// Record that a weak property was accessed. + /// + /// Part of the implementation of -Wrepeated-use-of-weak. + void recordUseOfWeak(const ObjCPropertyRefExpr *PropE); + + /// Record that a given expression is a "safe" access of a weak object (e.g. + /// assigning it to a strong variable.) + /// + /// Part of the implementation of -Wrepeated-use-of-weak. + void markSafeWeakUse(const Expr *E); + + const WeakObjectUseMap &getWeakObjectUses() const { + return WeakObjectUses; + } + void setHasBranchIntoScope() { HasBranchIntoScope = true; } @@ -387,7 +508,39 @@ public: }; +FunctionScopeInfo::WeakObjectProfileTy::WeakObjectProfileTy() + : Base(0, false), Property(0) {} + +FunctionScopeInfo::WeakObjectProfileTy +FunctionScopeInfo::WeakObjectProfileTy::getSentinel() { + FunctionScopeInfo::WeakObjectProfileTy Result; + Result.Base.setInt(true); + return Result; +} + } } +namespace llvm { + template <> + struct DenseMapInfo { + typedef clang::sema::FunctionScopeInfo::WeakObjectProfileTy ProfileTy; + static inline ProfileTy getEmptyKey() { + return ProfileTy(); + } + static inline ProfileTy getTombstoneKey() { + return ProfileTy::getSentinel(); + } + + static unsigned getHashValue(const ProfileTy &Val) { + typedef std::pair Pair; + return DenseMapInfo::getHashValue(Pair(Val.Base, Val.Property)); + } + + static bool isEqual(const ProfileTy &LHS, const ProfileTy &RHS) { + return LHS == RHS; + } + }; +} // end namespace llvm + #endif diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index a3aee9a..86e9dc2 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -871,6 +871,121 @@ static void DiagnoseSwitchLabelsFallthrough(Sema &S, AnalysisDeclContext &AC, } namespace { + typedef std::pair + StmtUsesPair; +} + +template<> +class BeforeThanCompare { + const SourceManager &SM; + +public: + explicit BeforeThanCompare(const SourceManager &SM) : SM(SM) { } + + bool operator()(const StmtUsesPair &LHS, const StmtUsesPair &RHS) { + return SM.isBeforeInTranslationUnit(LHS.first->getLocStart(), + RHS.first->getLocStart()); + } +}; + + +static void diagnoseRepeatedUseOfWeak(Sema &S, + const sema::FunctionScopeInfo *CurFn, + const Decl *D) { + typedef sema::FunctionScopeInfo::WeakObjectProfileTy WeakObjectProfileTy; + typedef sema::FunctionScopeInfo::WeakObjectUseMap WeakObjectUseMap; + typedef sema::FunctionScopeInfo::WeakUseVector WeakUseVector; + + const WeakObjectUseMap &WeakMap = CurFn->getWeakObjectUses(); + + // Extract all weak objects that are referenced more than once. + SmallVector UsesByStmt; + for (WeakObjectUseMap::const_iterator I = WeakMap.begin(), E = WeakMap.end(); + I != E; ++I) { + const WeakUseVector &Uses = I->second; + if (Uses.size() <= 1) + continue; + + // Find the first read of the weak object. + WeakUseVector::const_iterator UI = Uses.begin(), UE = Uses.end(); + for ( ; UI != UE; ++UI) { + if (UI->isUnsafe()) + break; + } + + // If there were only writes to this object, don't warn. + if (UI == UE) + continue; + + UsesByStmt.push_back(StmtUsesPair(UI->getUseExpr(), I)); + } + + if (UsesByStmt.empty()) + return; + + // Sort by first use so that we emit the warnings in a deterministic order. + std::sort(UsesByStmt.begin(), UsesByStmt.end(), + BeforeThanCompare(S.getSourceManager())); + + // Classify the current code body for better warning text. + // This enum should stay in sync with the cases in + // warn_arc_repeated_use_of_weak and warn_arc_possible_repeated_use_of_weak. + // FIXME: Should we use a common classification enum and the same set of + // possibilities all throughout Sema? + enum { + Function, + Method, + Block, + Lambda + } FunctionKind; + + if (isa(CurFn)) + FunctionKind = Block; + else if (isa(CurFn)) + FunctionKind = Lambda; + else if (isa(D)) + FunctionKind = Method; + else + FunctionKind = Function; + + // Iterate through the sorted problems and emit warnings for each. + for (SmallVectorImpl::const_iterator I = UsesByStmt.begin(), + E = UsesByStmt.end(); + I != E; ++I) { + const Stmt *FirstRead = I->first; + const WeakObjectProfileTy &Key = I->second->first; + const WeakUseVector &Uses = I->second->second; + + // For complicated expressions like self.foo.bar, it's hard to keep track + // of whether 'self.foo' is the same between two cases. We can only be + // 100% sure of a repeated use if the "base" part of the key is a variable, + // rather than, say, another property. + unsigned DiagKind; + if (Key.isExactProfile()) + DiagKind = diag::warn_arc_repeated_use_of_weak; + else + DiagKind = diag::warn_arc_possible_repeated_use_of_weak; + + // Show the first time the object was read. + S.Diag(FirstRead->getLocStart(), DiagKind) + << FunctionKind + << FirstRead->getSourceRange(); + + // Print all the other accesses as notes. + for (WeakUseVector::const_iterator UI = Uses.begin(), UE = Uses.end(); + UI != UE; ++UI) { + if (UI->getUseExpr() == FirstRead) + continue; + S.Diag(UI->getUseExpr()->getLocStart(), + diag::note_arc_weak_also_accessed_here) + << UI->getUseExpr()->getSourceRange(); + } + } +} + + +namespace { struct SLocSort { bool operator()(const UninitUse &a, const UninitUse &b) { // Prefer a more confident report over a less confident one. @@ -1364,6 +1479,11 @@ AnalysisBasedWarnings::IssueWarnings(sema::AnalysisBasedWarnings::Policy P, DiagnoseSwitchLabelsFallthrough(S, AC, !FallThroughDiagFull); } + if (S.getLangOpts().ObjCARCWeak && + Diags.getDiagnosticLevel(diag::warn_arc_repeated_use_of_weak, + D->getLocStart()) != DiagnosticsEngine::Ignored) + diagnoseRepeatedUseOfWeak(S, fscope, D); + // Collect statistics about the CFG if it was built. if (S.CollectStats && AC.isCFGBuilt()) { ++NumFunctionsAnalyzed; diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp index 08ccfa4..84cdb2b 100644 --- a/clang/lib/Sema/Sema.cpp +++ b/clang/lib/Sema/Sema.cpp @@ -54,6 +54,132 @@ void FunctionScopeInfo::Clear() { Returns.clear(); ErrorTrap.reset(); PossiblyUnreachableDiags.clear(); + WeakObjectUses.clear(); +} + +static const NamedDecl *getBestPropertyDecl(const ObjCPropertyRefExpr *PropE) { + if (PropE->isExplicitProperty()) + return PropE->getExplicitProperty(); + + return PropE->getImplicitPropertyGetter(); +} + +static bool isSelfExpr(const Expr *E) { + E = E->IgnoreParenImpCasts(); + + const DeclRefExpr *DRE = dyn_cast(E); + if (!DRE) + return false; + + const ImplicitParamDecl *Param = dyn_cast(DRE->getDecl()); + if (!Param) + return false; + + const ObjCMethodDecl *M = dyn_cast(Param->getDeclContext()); + if (!M) + return false; + + return M->getSelfDecl() == Param; +} + +FunctionScopeInfo::WeakObjectProfileTy::WeakObjectProfileTy( + const ObjCPropertyRefExpr *PropE) + : Base(0, false), Property(getBestPropertyDecl(PropE)) { + + if (PropE->isObjectReceiver()) { + const OpaqueValueExpr *OVE = cast(PropE->getBase()); + const Expr *E = OVE->getSourceExpr()->IgnoreParenCasts(); + + switch (E->getStmtClass()) { + case Stmt::DeclRefExprClass: + Base.setPointer(cast(E)->getDecl()); + Base.setInt(isa(Base.getPointer())); + break; + case Stmt::MemberExprClass: { + const MemberExpr *ME = cast(E); + Base.setPointer(ME->getMemberDecl()); + Base.setInt(isa(ME->getBase()->IgnoreParenImpCasts())); + break; + } + case Stmt::ObjCIvarRefExprClass: { + const ObjCIvarRefExpr *IE = cast(E); + Base.setPointer(IE->getDecl()); + if (isSelfExpr(IE->getBase())) + Base.setInt(true); + break; + } + case Stmt::PseudoObjectExprClass: { + const PseudoObjectExpr *POE = cast(E); + const ObjCPropertyRefExpr *BaseProp = + dyn_cast(POE->getSyntacticForm()); + if (BaseProp) { + Base.setPointer(getBestPropertyDecl(BaseProp)); + + const Expr *DoubleBase = BaseProp->getBase(); + if (const OpaqueValueExpr *OVE = dyn_cast(DoubleBase)) + DoubleBase = OVE->getSourceExpr(); + + if (isSelfExpr(DoubleBase)) + Base.setInt(true); + } + break; + } + default: + break; + } + } else if (PropE->isClassReceiver()) { + Base.setPointer(PropE->getClassReceiver()); + Base.setInt(true); + } else { + assert(PropE->isSuperReceiver()); + Base.setInt(true); + } +} + +void FunctionScopeInfo::recordUseOfWeak(const ObjCPropertyRefExpr *RefExpr) { + assert(RefExpr); + WeakUseVector &Uses = + WeakObjectUses[FunctionScopeInfo::WeakObjectProfileTy(RefExpr)]; + Uses.push_back(WeakUseTy(RefExpr, RefExpr->isMessagingGetter())); +} + +void FunctionScopeInfo::markSafeWeakUse(const Expr *E) { + E = E->IgnoreParenImpCasts(); + + if (const PseudoObjectExpr *POE = dyn_cast(E)) { + markSafeWeakUse(POE->getSyntacticForm()); + return; + } + + if (const ConditionalOperator *Cond = dyn_cast(E)) { + markSafeWeakUse(Cond->getTrueExpr()); + markSafeWeakUse(Cond->getFalseExpr()); + return; + } + + if (const BinaryConditionalOperator *Cond = + dyn_cast(E)) { + markSafeWeakUse(Cond->getCommon()); + markSafeWeakUse(Cond->getFalseExpr()); + return; + } + + if (const ObjCPropertyRefExpr *RefExpr = dyn_cast(E)) { + // Has this property been seen as a weak property? + FunctionScopeInfo::WeakObjectUseMap::iterator Uses = + WeakObjectUses.find(FunctionScopeInfo::WeakObjectProfileTy(RefExpr)); + if (Uses == WeakObjectUses.end()) + return; + + // Has there been a read from the property using this Expr? + FunctionScopeInfo::WeakUseVector::reverse_iterator ThisUse = + std::find(Uses->second.rbegin(), Uses->second.rend(), WeakUseTy(E, true)); + if (ThisUse == Uses->second.rend()) + return; + + ThisUse->markSafe(); + return; + } } BlockScopeInfo::~BlockScopeInfo() { } diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index 01aaf8b..592956b 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -6588,6 +6588,21 @@ void Sema::AddInitializerToDecl(Decl *RealDecl, Expr *Init, if (VDecl->hasAttr()) checkRetainCycles(VDecl, Init); + + // It is safe to assign a weak reference into a strong variable. + // Although this code can still have problems: + // id x = self.weakProp; + // id y = self.weakProp; + // we do not warn to warn spuriously when 'x' and 'y' are on separate + // paths through the function. This should be revisited if + // -Wrepeated-use-of-weak is made flow-sensitive. + if (VDecl->getType().getObjCLifetime() == Qualifiers::OCL_Strong) { + DiagnosticsEngine::Level Level = + Diags.getDiagnosticLevel(diag::warn_arc_repeated_use_of_weak, + Init->getLocStart()); + if (Level != DiagnosticsEngine::Ignored) + getCurFunction()->markSafeWeakUse(Init); + } } Init = MaybeCreateExprWithCleanups(Init); diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp index 630281a..597e4d6 100644 --- a/clang/lib/Sema/SemaExpr.cpp +++ b/clang/lib/Sema/SemaExpr.cpp @@ -7726,6 +7726,19 @@ QualType Sema::CheckAssignmentOperands(Expr *LHSExpr, ExprResult &RHS, if (!DRE || DRE->getDecl()->hasAttr()) checkRetainCycles(LHSExpr, RHS.get()); + // It is safe to assign a weak reference into a strong variable. + // Although this code can still have problems: + // id x = self.weakProp; + // id y = self.weakProp; + // we do not warn to warn spuriously when 'x' and 'y' are on separate + // paths through the function. This should be revisited if + // -Wrepeated-use-of-weak is made flow-sensitive. + DiagnosticsEngine::Level Level = + Diags.getDiagnosticLevel(diag::warn_arc_repeated_use_of_weak, + RHS.get()->getLocStart()); + if (Level != DiagnosticsEngine::Ignored) + getCurFunction()->markSafeWeakUse(RHS.get()); + } else if (getLangOpts().ObjCAutoRefCount) { checkUnsafeExprAssigns(Loc, LHSExpr, RHS.get()); } diff --git a/clang/lib/Sema/SemaPseudoObject.cpp b/clang/lib/Sema/SemaPseudoObject.cpp index 31496b3..b07a59b 100644 --- a/clang/lib/Sema/SemaPseudoObject.cpp +++ b/clang/lib/Sema/SemaPseudoObject.cpp @@ -31,6 +31,7 @@ //===----------------------------------------------------------------------===// #include "clang/Sema/SemaInternal.h" +#include "clang/Sema/ScopeInfo.h" #include "clang/Sema/Initialization.h" #include "clang/AST/ExprObjC.h" #include "clang/Lex/Preprocessor.h" @@ -186,7 +187,7 @@ namespace { UnaryOperatorKind opcode, Expr *op); - ExprResult complete(Expr *syntacticForm); + virtual ExprResult complete(Expr *syntacticForm); OpaqueValueExpr *capture(Expr *op); OpaqueValueExpr *captureValueAsResult(Expr *op); @@ -238,6 +239,9 @@ namespace { Expr *rebuildAndCaptureObject(Expr *syntacticBase); ExprResult buildGet(); ExprResult buildSet(Expr *op, SourceLocation, bool); + ExprResult complete(Expr *SyntacticForm); + + bool isWeakProperty() const; }; /// A PseudoOpBuilder for Objective-C array/dictionary indexing. @@ -471,6 +475,23 @@ static ObjCMethodDecl *LookupMethodInReceiverType(Sema &S, Selector sel, return S.LookupMethodInObjectType(sel, IT, false); } +bool ObjCPropertyOpBuilder::isWeakProperty() const { + QualType T; + if (RefExpr->isExplicitProperty()) { + const ObjCPropertyDecl *Prop = RefExpr->getExplicitProperty(); + if (Prop->getPropertyAttributes() & ObjCPropertyDecl::OBJC_PR_weak) + return true; + + T = Prop->getType(); + } else if (Getter) { + T = Getter->getResultType(); + } else { + return false; + } + + return T.getObjCLifetime() == Qualifiers::OCL_Weak; +} + bool ObjCPropertyOpBuilder::findGetter() { if (Getter) return true; @@ -818,6 +839,18 @@ ObjCPropertyOpBuilder::buildIncDecOperation(Scope *Sc, SourceLocation opcLoc, return PseudoOpBuilder::buildIncDecOperation(Sc, opcLoc, opcode, op); } +ExprResult ObjCPropertyOpBuilder::complete(Expr *SyntacticForm) { + if (S.getLangOpts().ObjCAutoRefCount && isWeakProperty()) { + DiagnosticsEngine::Level Level = + S.Diags.getDiagnosticLevel(diag::warn_arc_repeated_use_of_weak, + SyntacticForm->getLocStart()); + if (Level != DiagnosticsEngine::Ignored) + S.getCurFunction()->recordUseOfWeak(SyntacticRefExpr); + } + + return PseudoOpBuilder::complete(SyntacticForm); +} + // ObjCSubscript build stuff. // diff --git a/clang/test/SemaObjC/arc-repeated-weak.mm b/clang/test/SemaObjC/arc-repeated-weak.mm new file mode 100644 index 0000000..728ffcb --- /dev/null +++ b/clang/test/SemaObjC/arc-repeated-weak.mm @@ -0,0 +1,203 @@ +// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime-has-weak -fobjc-arc -fblocks -Wno-objc-root-class -Warc-repeated-use-of-weak -verify %s + +@interface Test { +@public + Test *ivar; +} +@property(weak) Test *weakProp; +@property(strong) Test *strongProp; + +- (__weak id)implicitProp; + ++ (__weak id)weakProp; +@end + +extern void use(id); +extern id get(); +extern bool condition(); +#define nil ((id)0) + +void sanity(Test *a) { + use(a.weakProp); // expected-warning{{weak property is accessed multiple times in this function but may be unpredictably set to nil; assign to a strong variable to keep the object alive}} + use(a.weakProp); // expected-note{{also accessed here}} + + use(a.strongProp); + use(a.strongProp); // no-warning + + use(a.weakProp); // expected-note{{also accessed here}} +} + +void singleUse(Test *a) { + use(a.weakProp); // no-warning + use(a.strongProp); // no-warning +} + +void assignsOnly(Test *a) { + a.weakProp = get(); // no-warning + + id next = get(); + if (next) + a.weakProp = next; // no-warning +} + +void assignThenRead(Test *a) { + a.weakProp = get(); // expected-note{{also accessed here}} + use(a.weakProp); // expected-warning{{weak property is accessed multiple times}} +} + +void twoVariables(Test *a, Test *b) { + use(a.weakProp); // no-warning + use(b.weakProp); // no-warning +} + +void doubleLevelAccess(Test *a) { + use(a.strongProp.weakProp); // expected-warning{{weak property may be accessed multiple times in this function but may be unpredictably set to nil; assign to a strong variable to keep the object alive}} + use(a.strongProp.weakProp); // expected-note{{also accessed here}} +} + +void doubleLevelAccessIvar(Test *a) { + use(a.strongProp.weakProp); // expected-warning{{weak property may be accessed multiple times}} + use(a.strongProp.weakProp); // expected-note{{also accessed here}} +} + +void implicitProperties(Test *a) { + use(a.implicitProp); // expected-warning{{weak property is accessed multiple times}} + use(a.implicitProp); // expected-note{{also accessed here}} +} + +void classProperties() { + use(Test.weakProp); // expected-warning{{weak property is accessed multiple times}} + use(Test.weakProp); // expected-note{{also accessed here}} +} + +void classPropertiesAreDifferent(Test *a) { + use(Test.weakProp); // no-warning + use(a.weakProp); // no-warning + use(a.strongProp.weakProp); // no-warning +} + + +void assignToStrongWrongInit(Test *a) { + id val = a.weakProp; // expected-note{{also accessed here}} + use(a.weakProp); // expected-warning{{weak property is accessed multiple times}} +} + +void assignToStrongWrong(Test *a) { + id val; + val = a.weakProp; // expected-note{{also accessed here}} + use(a.weakProp); // expected-warning{{weak property is accessed multiple times}} +} + +void assignToStrongOK(Test *a) { + if (condition()) { + id val = a.weakProp; // no-warning + (void)val; + } else { + id val; + val = a.weakProp; // no-warning + (void)val; + } +} + +void assignToStrongConditional(Test *a) { + id val = (condition() ? a.weakProp : a.weakProp); // no-warning + id val2 = a.implicitProp ?: a.implicitProp; // no-warning +} + +void testBlock(Test *a) { + use(a.weakProp); // no-warning + + use(^{ + use(a.weakProp); // expected-warning{{weak property is accessed multiple times in this block}} + use(a.weakProp); // expected-note{{also accessed here}} + }); +} + + +@interface Test (Methods) +@end + +@implementation Test (Methods) +- (void)sanity { + use(self.weakProp); // expected-warning{{weak property is accessed multiple times in this method but may be unpredictably set to nil; assign to a strong variable to keep the object alive}} + use(self.weakProp); // expected-note{{also accessed here}} +} + +- (void)doubleLevelAccessForSelf { + use(self.strongProp.weakProp); // expected-warning{{weak property is accessed multiple times}} + use(self.strongProp.weakProp); // expected-note{{also accessed here}} + + use(self->ivar.weakProp); // expected-warning{{weak property is accessed multiple times}} + use(self->ivar.weakProp); // expected-note{{also accessed here}} +} + +- (void)distinctFromOther:(Test *)other { + use(self.strongProp.weakProp); // no-warning + use(other.strongProp.weakProp); // no-warning + + use(self->ivar.weakProp); // no-warning + use(other->ivar.weakProp); // no-warning +} +@end + + +class Wrapper { + Test *a; + +public: + void fields() { + use(a.weakProp); // expected-warning{{weak property is accessed multiple times in this function but may be unpredictably set to nil; assign to a strong variable to keep the object alive}} + use(a.weakProp); // expected-note{{also accessed here}} + } + + void distinctFromOther(Test *b, const Wrapper &w) { + use(a.weakProp); // no-warning + use(b.weakProp); // no-warning + use(w.a.weakProp); // no-warning + } + + static void doubleLevelAccessField(const Wrapper &x, const Wrapper &y) { + use(x.a.weakProp); // expected-warning{{weak property may be accessed multiple times}} + use(y.a.weakProp); // expected-note{{also accessed here}} + } +}; + + +// ----------------------- +// False positives +// ----------------------- + +// Most of these would require flow-sensitive analysis to silence correctly. + +void assignAfterRead(Test *a) { + if (!a.weakProp) // expected-warning{{weak property is accessed multiple times}} + a.weakProp = get(); // expected-note{{also accessed here}} +} + +void assignNil(Test *a) { + if (condition()) + a.weakProp = nil; // expected-note{{also accessed here}} + + use(a.weakProp); // expected-warning{{weak property is accessed multiple times}} +} + +void branch(Test *a) { + if (condition()) + use(a.weakProp); // expected-warning{{weak property is accessed multiple times}} + else + use(a.weakProp); // expected-note{{also accessed here}} +} + +void doubleLevelAccess(Test *a, Test *b) { + use(a.strongProp.weakProp); // expected-warning{{weak property may be accessed multiple times}} + use(b.strongProp.weakProp); // expected-note{{also accessed here}} + + use(a.weakProp.weakProp); // no-warning +} + +void doubleLevelAccessIvar(Test *a, Test *b) { + use(a->ivar.weakProp); // expected-warning{{weak property may be accessed multiple times}} + use(b->ivar.weakProp); // expected-note{{also accessed here}} + + use(a.strongProp.weakProp); // no-warning +} -- 2.7.4