From cafd9121cb5c0d6c6ca8b69de5cdc5420f357911 Mon Sep 17 00:00:00 2001 From: Brian Kelley Date: Wed, 29 Mar 2017 17:55:11 +0000 Subject: [PATCH] [Objective-C] Fix "repeated use of weak" warning with -fobjc-weak Summary: -Warc-repeated-use-of-weak should produce the same warnings with -fobjc-weak as it does with -objc-arc. Also check for ObjCWeak along with ObjCAutoRefCount when recording the use of an evaluated weak variable. Add a -fobjc-weak run to the existing arc-repeated-weak test case and adapt it slightly to work in both modes. Reviewers: rsmith, doug.gregor, jordan_rose, rjmccall Reviewed By: rjmccall Subscribers: arphaman, rjmccall, cfe-commits Differential Revision: https://reviews.llvm.org/D31005 llvm-svn: 299011 --- clang/include/clang/AST/Type.h | 3 +++ clang/lib/AST/Type.cpp | 6 +++++- clang/lib/Sema/SemaDecl.cpp | 3 ++- clang/lib/Sema/SemaExpr.cpp | 18 +++++++++++------- clang/lib/Sema/SemaExprMember.cpp | 10 ++++------ clang/lib/Sema/SemaExprObjC.cpp | 2 ++ clang/lib/Sema/SemaPseudoObject.cpp | 16 +++++++--------- clang/test/SemaObjC/arc-repeated-weak.mm | 10 +++++++--- 8 files changed, 41 insertions(+), 27 deletions(-) diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h index 9c1d110..848f538 100644 --- a/clang/include/clang/AST/Type.h +++ b/clang/include/clang/AST/Type.h @@ -1020,6 +1020,9 @@ public: return getQualifiers().hasStrongOrWeakObjCLifetime(); } + // true when Type is objc's weak and weak is enabled but ARC isn't. + bool isNonWeakInMRRWithObjCWeak(const ASTContext &Context) const; + enum DestructionKind { DK_none, DK_cxx_destructor, diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp index 0ebdd4b..df26233 100644 --- a/clang/lib/AST/Type.cpp +++ b/clang/lib/AST/Type.cpp @@ -2148,7 +2148,11 @@ bool QualType::isTriviallyCopyableType(const ASTContext &Context) const { return false; } - +bool QualType::isNonWeakInMRRWithObjCWeak(const ASTContext &Context) const { + return !Context.getLangOpts().ObjCAutoRefCount && + Context.getLangOpts().ObjCWeak && + getObjCLifetime() != Qualifiers::OCL_Weak; +} bool Type::isLiteralType(const ASTContext &Ctx) const { if (isDependentType()) diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index bb630b2..f964d05 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -10167,7 +10167,8 @@ void Sema::AddInitializerToDecl(Decl *RealDecl, Expr *Init, bool DirectInit) { // 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 && + if ((VDecl->getType().getObjCLifetime() == Qualifiers::OCL_Strong || + VDecl->getType().isNonWeakInMRRWithObjCWeak(Context)) && !Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, Init->getLocStart())) getCurFunction()->markSafeWeakUse(Init); diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp index 410c8b3..1abeeb4 100644 --- a/clang/lib/Sema/SemaExpr.cpp +++ b/clang/lib/Sema/SemaExpr.cpp @@ -704,8 +704,7 @@ ExprResult Sema::DefaultLvalueConversion(Expr *E) { // Loading a __weak object implicitly retains the value, so we need a cleanup to // balance that. - if (getLangOpts().ObjCAutoRefCount && - E->getType().getObjCLifetime() == Qualifiers::OCL_Weak) + if (E->getType().getObjCLifetime() == Qualifiers::OCL_Weak) Cleanup.setExprNeedsCleanups(true); ExprResult Res = ImplicitCastExpr::Create(Context, T, CK_LValueToRValue, E, @@ -2509,11 +2508,11 @@ Sema::LookupInObjCMethod(LookupResult &Lookup, Scope *S, ObjCIvarRefExpr(IV, IV->getUsageType(SelfExpr.get()->getType()), Loc, IV->getLocation(), SelfExpr.get(), true, true); + if (IV->getType().getObjCLifetime() == Qualifiers::OCL_Weak) { + if (!Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, Loc)) + recordUseOfEvaluatedWeak(Result); + } if (getLangOpts().ObjCAutoRefCount) { - if (IV->getType().getObjCLifetime() == Qualifiers::OCL_Weak) { - if (!Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, Loc)) - recordUseOfEvaluatedWeak(Result); - } if (CurContext->isClosure()) Diag(Loc, diag::warn_implicitly_retains_self) << FixItHint::CreateInsertion(Loc, "self->"); @@ -10317,7 +10316,10 @@ QualType Sema::CheckAssignmentOperands(Expr *LHSExpr, ExprResult &RHS, const DeclRefExpr *DRE = dyn_cast(InnerLHS); if (!DRE || DRE->getDecl()->hasAttr()) checkRetainCycles(LHSExpr, RHS.get()); + } + if (LHSType.getObjCLifetime() == Qualifiers::OCL_Strong || + LHSType.isNonWeakInMRRWithObjCWeak(Context)) { // It is safe to assign a weak reference into a strong variable. // Although this code can still have problems: // id x = self.weakProp; @@ -10325,11 +10327,13 @@ QualType Sema::CheckAssignmentOperands(Expr *LHSExpr, ExprResult &RHS, // 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. + // For ObjCWeak only, we do not warn if the assign is to a non-weak + // variable, which will be valid for the current autorelease scope. if (!Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, RHS.get()->getLocStart())) getCurFunction()->markSafeWeakUse(RHS.get()); - } else if (getLangOpts().ObjCAutoRefCount) { + } else if (getLangOpts().ObjCAutoRefCount || getLangOpts().ObjCWeak) { checkUnsafeExprAssigns(Loc, LHSExpr, RHS.get()); } } diff --git a/clang/lib/Sema/SemaExprMember.cpp b/clang/lib/Sema/SemaExprMember.cpp index 542f10b..66f346e 100644 --- a/clang/lib/Sema/SemaExprMember.cpp +++ b/clang/lib/Sema/SemaExprMember.cpp @@ -1496,7 +1496,7 @@ static ExprResult LookupMemberExpr(Sema &S, LookupResult &R, } } bool warn = true; - if (S.getLangOpts().ObjCAutoRefCount) { + if (S.getLangOpts().ObjCWeak) { Expr *BaseExp = BaseExpr.get()->IgnoreParenImpCasts(); if (UnaryOperator *UO = dyn_cast(BaseExp)) if (UO->getOpcode() == UO_Deref) @@ -1523,11 +1523,9 @@ static ExprResult LookupMemberExpr(Sema &S, LookupResult &R, IV, IV->getUsageType(BaseType), MemberLoc, OpLoc, BaseExpr.get(), IsArrow); - if (S.getLangOpts().ObjCAutoRefCount) { - if (IV->getType().getObjCLifetime() == Qualifiers::OCL_Weak) { - if (!S.Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, MemberLoc)) - S.recordUseOfEvaluatedWeak(Result); - } + if (IV->getType().getObjCLifetime() == Qualifiers::OCL_Weak) { + if (!S.Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, MemberLoc)) + S.recordUseOfEvaluatedWeak(Result); } return Result; diff --git a/clang/lib/Sema/SemaExprObjC.cpp b/clang/lib/Sema/SemaExprObjC.cpp index fba0653..6b159a6 100644 --- a/clang/lib/Sema/SemaExprObjC.cpp +++ b/clang/lib/Sema/SemaExprObjC.cpp @@ -3100,7 +3100,9 @@ ExprResult Sema::BuildInstanceMessage(Expr *Receiver, // In ARC, check for message sends which are likely to introduce // retain cycles. checkRetainCycles(Result); + } + if (getLangOpts().ObjCWeak) { if (!isImplicit && Method) { if (const ObjCPropertyDecl *Prop = Method->findPropertyDecl()) { bool IsWeak = diff --git a/clang/lib/Sema/SemaPseudoObject.cpp b/clang/lib/Sema/SemaPseudoObject.cpp index b740540..05c593b 100644 --- a/clang/lib/Sema/SemaPseudoObject.cpp +++ b/clang/lib/Sema/SemaPseudoObject.cpp @@ -842,12 +842,10 @@ ExprResult ObjCPropertyOpBuilder::buildRValueOperation(Expr *op) { result = S.ImpCastExprToType(result.get(), propType, CK_BitCast); } } - if (S.getLangOpts().ObjCAutoRefCount) { - Qualifiers::ObjCLifetime LT = propType.getObjCLifetime(); - if (LT == Qualifiers::OCL_Weak) - if (!S.Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, RefExpr->getLocation())) - S.getCurFunction()->markSafeWeakUse(RefExpr); - } + if (propType.getObjCLifetime() == Qualifiers::OCL_Weak && + !S.Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, + RefExpr->getLocation())) + S.getCurFunction()->markSafeWeakUse(RefExpr); } return result; @@ -963,11 +961,11 @@ ObjCPropertyOpBuilder::buildIncDecOperation(Scope *Sc, SourceLocation opcLoc, } ExprResult ObjCPropertyOpBuilder::complete(Expr *SyntacticForm) { - if (S.getLangOpts().ObjCAutoRefCount && isWeakProperty() && + if (isWeakProperty() && !S.Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, SyntacticForm->getLocStart())) - S.recordUseOfEvaluatedWeak(SyntacticRefExpr, - SyntacticRefExpr->isMessagingGetter()); + S.recordUseOfEvaluatedWeak(SyntacticRefExpr, + SyntacticRefExpr->isMessagingGetter()); return PseudoOpBuilder::complete(SyntacticForm); } diff --git a/clang/test/SemaObjC/arc-repeated-weak.mm b/clang/test/SemaObjC/arc-repeated-weak.mm index 8e2828fca..37b2123 100644 --- a/clang/test/SemaObjC/arc-repeated-weak.mm +++ b/clang/test/SemaObjC/arc-repeated-weak.mm @@ -1,4 +1,5 @@ // RUN: %clang_cc1 -fsyntax-only -fobjc-runtime-has-weak -fobjc-arc -fblocks -Wno-objc-root-class -std=c++11 -Warc-repeated-use-of-weak -verify %s +// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime-has-weak -fobjc-weak -fblocks -Wno-objc-root-class -std=c++11 -Warc-repeated-use-of-weak -verify %s @interface Test { @public @@ -445,8 +446,8 @@ void doubleLevelAccessIvar(Test *a, Test *b) { @class NSString; @interface NSBundle +(NSBundle *)foo; -@property (class) NSBundle *foo2; -@property NSString *prop; +@property (class, strong) NSBundle *foo2; +@property (strong) NSString *prop; @property(weak) NSString *weakProp; @end @@ -473,5 +474,8 @@ enum E { }; void foo1() { - INTFPtrTy tmp = (INTFPtrTy)e1; // expected-error{{cast of 'E' to 'INTFPtrTy' (aka 'INTF *') is disallowed with ARC}} + INTFPtrTy tmp = (INTFPtrTy)e1; +#if __has_feature(objc_arc) +// expected-error@-2{{cast of 'E' to 'INTFPtrTy' (aka 'INTF *') is disallowed with ARC}} +#endif } -- 2.7.4