ahatanak created this revision.
ahatanak added reviewers: rjmccall, erik.pilkington, arphaman.
ahatanak added a project: clang.
Herald added subscribers: dexonsmith, jkorous.
If the block implicitly referencing `self` doesn't escape, there is no risk of
creating retain cycles, so clang shouldn't diagnose it and force users to add
`self->` to silence the diagnostic.
Also, fix a bug where clang was failing to diagnose `self` referenced inside a
block that was nested inside a c++ lambda.
rdar://problem/25059955
Repository:
rC Clang
https://reviews.llvm.org/D60736
Files:
include/clang/Sema/ScopeInfo.h
lib/Sema/Sema.cpp
lib/Sema/SemaDeclObjC.cpp
lib/Sema/SemaExpr.cpp
test/SemaObjC/warn-implicit-self-in-block.m
test/SemaObjCXX/warn-implicit-self-in-block.mm
Index: test/SemaObjCXX/warn-implicit-self-in-block.mm
===================================================================
--- /dev/null
+++ test/SemaObjCXX/warn-implicit-self-in-block.mm
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 -x objective-c++ -std=c++11 -fobjc-arc -fblocks -Wimplicit-retain-self -verify %s
+// rdar://11194874
+
+typedef void (^BlockTy)();
+
+void noescapeFunc(__attribute__((noescape)) BlockTy);
+void escapeFunc(BlockTy);
+
+@interface Root @end
+
+@interface I : Root
+{
+ int _bar;
+}
+@end
+
+@implementation I
+ - (void)foo{
+ ^{
+ _bar = 3; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+ }();
+ }
+
+ - (void)testNested{
+ noescapeFunc(^{
+ (void)_bar;
+ escapeFunc(^{
+ (void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+ noescapeFunc(^{
+ (void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+ });
+ (void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+ });
+ (void)_bar;
+ });
+ }
+
+ - (void)testLambdaInBlock{
+ noescapeFunc(^{ [&](){ (void)_bar; }(); });
+ escapeFunc(^{ [&](){ (void)_bar; }(); }); // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+ }
+@end
Index: test/SemaObjC/warn-implicit-self-in-block.m
===================================================================
--- test/SemaObjC/warn-implicit-self-in-block.m
+++ /dev/null
@@ -1,18 +0,0 @@
-// RUN: %clang_cc1 -x objective-c -fobjc-arc -fblocks -Wimplicit-retain-self -verify %s
-// rdar://11194874
-
-@interface Root @end
-
-@interface I : Root
-{
- int _bar;
-}
-@end
-
-@implementation I
- - (void)foo{
- ^{
- _bar = 3; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
- }();
- }
-@end
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -2575,11 +2575,6 @@
!Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, Loc))
getCurFunction()->recordUseOfWeak(Result);
}
- if (getLangOpts().ObjCAutoRefCount) {
- if (CurContext->isClosure())
- Diag(Loc, diag::warn_implicitly_retains_self)
- << FixItHint::CreateInsertion(Loc, "self->");
- }
return Result;
}
@@ -13652,6 +13647,9 @@
}
}
+ if (getCurFunction())
+ getCurFunction()->addBlock(Block, /*IsIntroducedInCurrentScope*/true);
+
PushBlockScope(CurScope, Block);
CurContext->addDecl(Block);
if (CurScope)
@@ -13913,9 +13911,6 @@
}
}
- if (getCurFunction())
- getCurFunction()->addBlock(BD);
-
return Result;
}
Index: lib/Sema/SemaDeclObjC.cpp
===================================================================
--- lib/Sema/SemaDeclObjC.cpp
+++ lib/Sema/SemaDeclObjC.cpp
@@ -378,6 +378,7 @@
// Allow all of Sema to see that we are entering a method definition.
PushDeclContext(FnBodyScope, MDecl);
PushFunctionScope();
+ getCurFunction()->setIsObjCMethod();
// Create Decl objects for each parameter, entrring them in the scope for
// binding to their use.
Index: lib/Sema/Sema.cpp
===================================================================
--- lib/Sema/Sema.cpp
+++ lib/Sema/Sema.cpp
@@ -20,6 +20,7 @@
#include "clang/AST/ExprCXX.h"
#include "clang/AST/PrettyDeclStackTrace.h"
#include "clang/AST/StmtCXX.h"
+#include "clang/AST/StmtVisitor.h"
#include "clang/Basic/DiagnosticOptions.h"
#include "clang/Basic/PartialDiagnostic.h"
#include "clang/Basic/TargetInfo.h"
@@ -1636,10 +1637,16 @@
static void markEscapingByrefs(const FunctionScopeInfo &FSI, Sema &S) {
// Set the EscapingByref flag of __block variables captured by
// escaping blocks.
- for (const BlockDecl *BD : FSI.Blocks) {
- if (BD->doesNotEscape())
+ for (const std::pair<const BlockDecl *, bool> &P : FSI.Blocks) {
+ // Skip blocks that aren't directly introduced in this function. We've
+ // already seen them.
+ if (!P.second)
continue;
- for (const BlockDecl::Capture &BC : BD->captures()) {
+
+ if (P.first->doesNotEscape())
+ continue;
+
+ for (const BlockDecl::Capture &BC : P.first->captures()) {
VarDecl *VD = BC.getVariable();
if (VD->hasAttr<BlocksAttr>())
VD->setEscapingByref();
@@ -1659,6 +1666,54 @@
}
}
+namespace {
+
+class DiagImplicitRetainedSelf
+ : public ConstStmtVisitor<DiagImplicitRetainedSelf> {
+ Sema &S;
+
+ // A flag indicating whether we are visiting a Stmt that is inside a block
+ // that can possibly escape.
+ bool VisitingEscapingBlock = false;
+
+public:
+ DiagImplicitRetainedSelf(Sema &S) : S(S) {}
+
+ void VisitBlockDecl(const BlockDecl *BD) {
+ bool OldVisitingEscapingBlock = VisitingEscapingBlock;
+ VisitingEscapingBlock = VisitingEscapingBlock || !BD->doesNotEscape();
+ Visit(BD->getBody());
+ VisitingEscapingBlock = OldVisitingEscapingBlock;
+ }
+
+ void VisitBlockExpr(const BlockExpr *BE) {
+ VisitBlockDecl(BE->getBlockDecl());
+ }
+
+ void VisitObjCIvarRefExpr(const ObjCIvarRefExpr *ORE) {
+ if (!VisitingEscapingBlock || !ORE->isFreeIvar())
+ return;
+ SourceLocation Loc = ORE->getExprLoc();
+ S.Diag(Loc, diag::warn_implicitly_retains_self)
+ << FixItHint::CreateInsertion(Loc, "self->");
+ }
+
+ void VisitStmt(const Stmt *S) {
+ for (const Stmt *SubStmt : S->children())
+ if (SubStmt)
+ Visit(SubStmt);
+ }
+};
+
+} // namespace
+
+static void diagnoseImplicitlyRetainedSelf(const FunctionScopeInfo &FSI,
+ Sema &S) {
+ DiagImplicitRetainedSelf D(S);
+ for (const std::pair<const BlockDecl *, bool> &P : FSI.Blocks)
+ D.VisitBlockDecl(P.first);
+}
+
void Sema::PopFunctionScopeInfo(const AnalysisBasedWarnings::Policy *WP,
const Decl *D, const BlockExpr *blkExpr) {
assert(!FunctionScopes.empty() && "mismatched push/pop!");
@@ -1671,6 +1726,16 @@
FunctionScopeInfo *Scope = FunctionScopes.pop_back_val();
+ if (getLangOpts().ObjCAutoRefCount && Scope->isObjCMethod())
+ diagnoseImplicitlyRetainedSelf(*Scope, *this);
+
+ // If the current scope isn't a block, add the blocks in the list to the
+ // enclosing function's block list.
+ if (!Scope->isBlockScope() && !FunctionScopes.empty())
+ for (std::pair<const BlockDecl *, bool> &p : Scope->Blocks)
+ FunctionScopes.back()->addBlock(
+ p.first, /*IsIntroducedInCurrentScope*/false);
+
if (LangOpts.OpenMP)
popOpenMPFunctionRegion(Scope);
Index: include/clang/Sema/ScopeInfo.h
===================================================================
--- include/clang/Sema/ScopeInfo.h
+++ include/clang/Sema/ScopeInfo.h
@@ -151,6 +151,9 @@
/// false if there is an invocation of an initializer on 'self'.
bool ObjCWarnForNoInitDelegation : 1;
+ /// A flag indicating the scope is an ObjC method definition.
+ bool IsObjCMethod : 1;
+
/// True only when this function has not already built, or attempted
/// to build, the initial and final coroutine suspend points
bool NeedsCoroutineSuspends : 1;
@@ -202,8 +205,23 @@
/// function.
SmallVector<CompoundScopeInfo, 4> CompoundScopes;
- /// The set of blocks that are introduced in this function.
- llvm::SmallPtrSet<const BlockDecl *, 1> Blocks;
+ /// The list of blocks that are directly or indirectly introduced in this
+ /// function and aren't enclosed in another block. The boolean flag indicates
+ /// whether the block is introduced in the current function scope. For
+ /// example, when function 'foo1' is parsed in the code below, the flag is
+ /// false for the first block and is true for the second block. The block that
+ /// calls 'foo2(2)' isn't added to the list since it is nested inside another
+ /// block.
+ ///
+ /// void foo1() {
+ /// [](){ ^{ foo2(0); }; }();
+ /// ^{ foo2(1); }();
+ /// ^{ ^{ foo2(2); }(); }();
+ /// }
+ ///
+ /// The blocks are stored to the list in the order they appear in the
+ /// function.
+ llvm::SmallVector<std::pair<const BlockDecl *, bool>, 1> Blocks;
/// The set of __block variables that are introduced in this function.
llvm::TinyPtrVector<VarDecl *> ByrefBlockVars;
@@ -432,9 +450,17 @@
(HasBranchProtectedScope && HasBranchIntoScope));
}
- // Add a block introduced in this function.
- void addBlock(const BlockDecl *BD) {
- Blocks.insert(BD);
+ bool isObjCMethod() const {
+ return IsObjCMethod;
+ }
+
+ void setIsObjCMethod() {
+ IsObjCMethod = true;
+ }
+
+ // Add a block to the list.
+ void addBlock(const BlockDecl *BD, bool IsIntroducedInCurrentScope) {
+ Blocks.push_back({BD, IsIntroducedInCurrentScope});
}
// Add a __block variable introduced in this function.
@@ -442,6 +468,8 @@
ByrefBlockVars.push_back(VD);
}
+ bool isBlockScope() const { return Kind == SK_Block; }
+
bool isCoroutine() const { return !FirstCoroutineStmtLoc.isInvalid(); }
void setFirstCoroutineStmt(SourceLocation Loc, StringRef Keyword) {
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
