On Fri, Sep 28, 2012 at 3:21 PM, Jordan Rose <[email protected]> wrote: > Author: jrose > Date: Fri Sep 28 17:21:30 2012 > New Revision: 164854 > > URL: http://llvm.org/viewvc/llvm-project?rev=164854&view=rev > Log: > Add a warning (off by default) for repeated use of the same weak property. > > The motivating example: > > if (self.weakProp) > use(self.weakProp);
I wonder if this could be generalized/modified to help for things like std::weak_ptr too (obviously weak_ptr would need some kind of annotation to specify its test/use operations I suppose). > 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, I believe we're generally trying to avoid adding off-by-default warnings (Doug seems to feel strongly that if it's off-by-default it's simply not good enough). Is there a good reason to make an exception here? > 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 <rdar://problem/12280249> > > Added: > cfe/trunk/test/SemaObjC/arc-repeated-weak.mm > Modified: > cfe/trunk/include/clang/Basic/DiagnosticGroups.td > cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td > cfe/trunk/include/clang/Sema/ScopeInfo.h > cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp > cfe/trunk/lib/Sema/Sema.cpp > cfe/trunk/lib/Sema/SemaDecl.cpp > cfe/trunk/lib/Sema/SemaExpr.cpp > cfe/trunk/lib/Sema/SemaPseudoObject.cpp > > Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=164854&r1=164853&r2=164854&view=diff > ============================================================================== > --- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original) > +++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Fri Sep 28 17:21:30 2012 > @@ -282,6 +282,9 @@ > [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">; > > Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=164854&r1=164853&r2=164854&view=diff > ============================================================================== > --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original) > +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Fri Sep 28 17:21:30 > 2012 > @@ -709,6 +709,18 @@ > "weak %select{receiver|property|implicit property}0 may be " > "unpredictably null in ARC mode">, > InGroup<DiagGroup<"receiver-is-weak">>, 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<ARCRepeatedUseOfWeak>, 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<ARCRepeatedUseOfWeakMaybe>, 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">; > > > Modified: cfe/trunk/include/clang/Sema/ScopeInfo.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/ScopeInfo.h?rev=164854&r1=164853&r2=164854&view=diff > ============================================================================== > --- cfe/trunk/include/clang/Sema/ScopeInfo.h (original) > +++ cfe/trunk/include/clang/Sema/ScopeInfo.h Fri Sep 28 17:21:30 2012 > @@ -21,6 +21,7 @@ > > namespace clang { > > +class Decl; > class BlockDecl; > class CXXMethodDecl; > class IdentifierInfo; > @@ -29,6 +30,7 @@ > class Scope; > class SwitchStmt; > class VarDecl; > +class ObjCPropertyRefExpr; > > namespace sema { > > @@ -113,6 +115,125 @@ > /// prior to being emitted. > SmallVector<PossiblyUnreachableDiag, 4> 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<const NamedDecl *, 1, bool> 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<WeakObjectProfileTy>; > + 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<const Expr *, 1, bool> 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<WeakUseTy, 4> 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<WeakObjectProfileTy, WeakUseVector, 8> > + 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 @@ > > }; > > +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<clang::sema::FunctionScopeInfo::WeakObjectProfileTy> { > + 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<ProfileTy::BaseInfoTy, const clang::NamedDecl *> > Pair; > + return DenseMapInfo<Pair>::getHashValue(Pair(Val.Base, Val.Property)); > + } > + > + static bool isEqual(const ProfileTy &LHS, const ProfileTy &RHS) { > + return LHS == RHS; > + } > + }; > +} // end namespace llvm > + > #endif > > Modified: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp?rev=164854&r1=164853&r2=164854&view=diff > ============================================================================== > --- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp (original) > +++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp Fri Sep 28 17:21:30 2012 > @@ -871,6 +871,121 @@ > } > > namespace { > + typedef std::pair<const Stmt *, > + > sema::FunctionScopeInfo::WeakObjectUseMap::const_iterator> > + StmtUsesPair; > +} > + > +template<> > +class BeforeThanCompare<StmtUsesPair> { > + 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<StmtUsesPair, 8> 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<StmtUsesPair>(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<sema::BlockScopeInfo>(CurFn)) > + FunctionKind = Block; > + else if (isa<sema::LambdaScopeInfo>(CurFn)) > + FunctionKind = Lambda; > + else if (isa<ObjCMethodDecl>(D)) > + FunctionKind = Method; > + else > + FunctionKind = Function; > + > + // Iterate through the sorted problems and emit warnings for each. > + for (SmallVectorImpl<StmtUsesPair>::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 @@ > 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; > > Modified: cfe/trunk/lib/Sema/Sema.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/Sema.cpp?rev=164854&r1=164853&r2=164854&view=diff > ============================================================================== > --- cfe/trunk/lib/Sema/Sema.cpp (original) > +++ cfe/trunk/lib/Sema/Sema.cpp Fri Sep 28 17:21:30 2012 > @@ -54,6 +54,132 @@ > 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<DeclRefExpr>(E); > + if (!DRE) > + return false; > + > + const ImplicitParamDecl *Param = > dyn_cast<ImplicitParamDecl>(DRE->getDecl()); > + if (!Param) > + return false; > + > + const ObjCMethodDecl *M = > dyn_cast<ObjCMethodDecl>(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<OpaqueValueExpr>(PropE->getBase()); > + const Expr *E = OVE->getSourceExpr()->IgnoreParenCasts(); > + > + switch (E->getStmtClass()) { > + case Stmt::DeclRefExprClass: > + Base.setPointer(cast<DeclRefExpr>(E)->getDecl()); > + Base.setInt(isa<VarDecl>(Base.getPointer())); > + break; > + case Stmt::MemberExprClass: { > + const MemberExpr *ME = cast<MemberExpr>(E); > + Base.setPointer(ME->getMemberDecl()); > + Base.setInt(isa<CXXThisExpr>(ME->getBase()->IgnoreParenImpCasts())); > + break; > + } > + case Stmt::ObjCIvarRefExprClass: { > + const ObjCIvarRefExpr *IE = cast<ObjCIvarRefExpr>(E); > + Base.setPointer(IE->getDecl()); > + if (isSelfExpr(IE->getBase())) > + Base.setInt(true); > + break; > + } > + case Stmt::PseudoObjectExprClass: { > + const PseudoObjectExpr *POE = cast<PseudoObjectExpr>(E); > + const ObjCPropertyRefExpr *BaseProp = > + dyn_cast<ObjCPropertyRefExpr>(POE->getSyntacticForm()); > + if (BaseProp) { > + Base.setPointer(getBestPropertyDecl(BaseProp)); > + > + const Expr *DoubleBase = BaseProp->getBase(); > + if (const OpaqueValueExpr *OVE = > dyn_cast<OpaqueValueExpr>(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<PseudoObjectExpr>(E)) { > + markSafeWeakUse(POE->getSyntacticForm()); > + return; > + } > + > + if (const ConditionalOperator *Cond = dyn_cast<ConditionalOperator>(E)) { > + markSafeWeakUse(Cond->getTrueExpr()); > + markSafeWeakUse(Cond->getFalseExpr()); > + return; > + } > + > + if (const BinaryConditionalOperator *Cond = > + dyn_cast<BinaryConditionalOperator>(E)) { > + markSafeWeakUse(Cond->getCommon()); > + markSafeWeakUse(Cond->getFalseExpr()); > + return; > + } > + > + if (const ObjCPropertyRefExpr *RefExpr = dyn_cast<ObjCPropertyRefExpr>(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() { } > > Modified: cfe/trunk/lib/Sema/SemaDecl.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=164854&r1=164853&r2=164854&view=diff > ============================================================================== > --- cfe/trunk/lib/Sema/SemaDecl.cpp (original) > +++ cfe/trunk/lib/Sema/SemaDecl.cpp Fri Sep 28 17:21:30 2012 > @@ -6588,6 +6588,21 @@ > > if (VDecl->hasAttr<BlocksAttr>()) > 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); > > Modified: cfe/trunk/lib/Sema/SemaExpr.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=164854&r1=164853&r2=164854&view=diff > ============================================================================== > --- cfe/trunk/lib/Sema/SemaExpr.cpp (original) > +++ cfe/trunk/lib/Sema/SemaExpr.cpp Fri Sep 28 17:21:30 2012 > @@ -7726,6 +7726,19 @@ > if (!DRE || DRE->getDecl()->hasAttr<BlocksAttr>()) > 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()); > } > > Modified: cfe/trunk/lib/Sema/SemaPseudoObject.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaPseudoObject.cpp?rev=164854&r1=164853&r2=164854&view=diff > ============================================================================== > --- cfe/trunk/lib/Sema/SemaPseudoObject.cpp (original) > +++ cfe/trunk/lib/Sema/SemaPseudoObject.cpp Fri Sep 28 17:21:30 2012 > @@ -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 @@ > 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 @@ > 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 @@ > 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 @@ > 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. > // > > > Added: cfe/trunk/test/SemaObjC/arc-repeated-weak.mm > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaObjC/arc-repeated-weak.mm?rev=164854&view=auto > ============================================================================== > --- cfe/trunk/test/SemaObjC/arc-repeated-weak.mm (added) > +++ cfe/trunk/test/SemaObjC/arc-repeated-weak.mm Fri Sep 28 17:21:30 2012 > @@ -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 > +} > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
