Skip to content

Commit f41dc70

Browse files
sdefresnesdefresne
committed
Add warning for blocks capturing {'this', raw pointers, references}
Add three new warning that reports when blocks captures 'this', a raw pointer (i.e. not a pointer to a reference counted object) or a reference. Those warnings can be used in Objective-C++ to detect blocks that create potentially unsafe capture (as the captured pointer can be dangling by the time the block is captured). To reduce the false positive, no warning is emitted if the block is detected as not escaping. The three warnings are: - -Wblocks-capturing-this - -Wblocks-capturing-reference - -Wblocks-capturing-raw-pointer Fixes issue #143924.
1 parent dfb14b6 commit f41dc70

File tree

7 files changed

+184
-14
lines changed

7 files changed

+184
-14
lines changed

‎clang/include/clang/Basic/DiagnosticSemaKinds.td‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1641,6 +1641,17 @@ def warn_implicitly_retains_self : Warning <
16411641
"block implicitly retains 'self'; explicitly mention 'self' to indicate "
16421642
"this is intended behavior">,
16431643
InGroup<DiagGroup<"implicit-retain-self">>, DefaultIgnore;
1644+
def warn_blocks_capturing_this : Warning<"block implicitly captures 'this'">,
1645+
InGroup<DiagGroup<"blocks-capturing-this">>,
1646+
DefaultIgnore;
1647+
def warn_blocks_capturing_reference
1648+
: Warning<"block implicitly captures a C++ reference">,
1649+
InGroup<DiagGroup<"blocks-capturing-reference">>,
1650+
DefaultIgnore;
1651+
def warn_blocks_capturing_raw_pointer
1652+
: Warning<"block implicitly captures a raw pointer">,
1653+
InGroup<DiagGroup<"blocks-capturing-raw-pointer">>,
1654+
DefaultIgnore;
16441655
def warn_arc_possible_repeated_use_of_weak : Warning <
16451656
"weak %select{variable|property|implicit property|instance variable}0 %1 may "
16461657
"be accessed multiple times in this %select{function|method|block|lambda}2 "

‎clang/include/clang/Sema/Sema.h‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8227,6 +8227,22 @@ class Sema final : public SemaBase {
82278227
}
82288228
};
82298229

8230+
/// Store information about a diagnosable block catpure.
8231+
struct BlockCapture {
8232+
/// Enumeration representing types of block captures that may be
8233+
/// diagnosable because they could be problematic.
8234+
enum CaptureType {
8235+
Self,
8236+
This,
8237+
Reference,
8238+
RawPointer,
8239+
};
8240+
8241+
SourceLocation Loc;
8242+
const BlockDecl *BD;
8243+
CaptureType Type;
8244+
};
8245+
82308246
/// Check an argument list for placeholders that we won't try to
82318247
/// handle later.
82328248
bool CheckArgsForPlaceholders(MultiExprArg args);
@@ -8243,8 +8259,7 @@ class Sema final : public SemaBase {
82438259

82448260
/// List of SourceLocations where 'self' is implicitly retained inside a
82458261
/// block.
8246-
llvm::SmallVector<std::pair<SourceLocation, const BlockDecl *>, 1>
8247-
ImplicitlyRetainedSelfLocs;
8262+
llvm::SmallVector<BlockCapture, 1> DiagnosableBlockCaptures;
82488263

82498264
/// Do an explicit extend of the given block pointer if we're in ARC.
82508265
void maybeExtendBlockObject(ExprResult &E);

‎clang/lib/Sema/SemaDecl.cpp‎

Lines changed: 35 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -16149,7 +16149,7 @@ class ExitFunctionBodyRAII {
1614916149
bool IsLambda = false;
1615016150
};
1615116151

16152-
static void diagnoseImplicitlyRetainedSelf(Sema &S) {
16152+
static void diagnoseBlockCaptures(Sema &S) {
1615316153
llvm::DenseMap<const BlockDecl *, bool> EscapeInfo;
1615416154

1615516155
auto IsOrNestedInEscapingBlock = [&](const BlockDecl *BD) {
@@ -16170,13 +16170,35 @@ static void diagnoseImplicitlyRetainedSelf(Sema &S) {
1617016170
return It->second = R;
1617116171
};
1617216172

16173-
// If the location where 'self' is implicitly retained is inside a escaping
16174-
// block, emit a diagnostic.
16175-
for (const std::pair<SourceLocation, const BlockDecl *> &P :
16176-
S.ImplicitlyRetainedSelfLocs)
16177-
if (IsOrNestedInEscapingBlock(P.second))
16178-
S.Diag(P.first, diag::warn_implicitly_retains_self)
16179-
<< FixItHint::CreateInsertion(P.first, "self->");
16173+
for (const auto &C : S.DiagnosableBlockCaptures) {
16174+
// Do not emit a diagnostic if the location is invalid.
16175+
if (!C.Loc.isValid())
16176+
continue;
16177+
16178+
// Do not emit a diagnostic if the block is not escaping.
16179+
if (!IsOrNestedInEscapingBlock(C.BD))
16180+
continue;
16181+
16182+
// Emit the correct warning depending on the type of capture.
16183+
switch (C.Type) {
16184+
case Sema::BlockCapture::Self:
16185+
S.Diag(C.Loc, diag::warn_implicitly_retains_self)
16186+
<< FixItHint::CreateInsertion(C.Loc, "self->");
16187+
break;
16188+
16189+
case Sema::BlockCapture::This:
16190+
S.Diag(C.Loc, diag::warn_blocks_capturing_this);
16191+
break;
16192+
16193+
case Sema::BlockCapture::Reference:
16194+
S.Diag(C.Loc, diag::warn_blocks_capturing_reference);
16195+
break;
16196+
16197+
case Sema::BlockCapture::RawPointer:
16198+
S.Diag(C.Loc, diag::warn_blocks_capturing_raw_pointer);
16199+
break;
16200+
}
16201+
}
1618016202
}
1618116203

1618216204
static bool methodHasName(const FunctionDecl *FD, StringRef Name) {
@@ -16511,6 +16533,10 @@ Decl *Sema::ActOnFinishFunctionBody(Decl *dcl, Stmt *Body,
1651116533
}
1651216534
}
1651316535

16536+
// Warn about block captures, unless this is a lambda function.
16537+
if (!isLambdaCallOperator(FD))
16538+
diagnoseBlockCaptures(*this);
16539+
1651416540
assert((FD == getCurFunctionDecl(/*AllowLambdas=*/true)) &&
1651516541
"Function parsing confused");
1651616542
} else if (ObjCMethodDecl *MD = dyn_cast_or_null<ObjCMethodDecl>(dcl)) {
@@ -16563,7 +16589,7 @@ Decl *Sema::ActOnFinishFunctionBody(Decl *dcl, Stmt *Body,
1656316589
FSI->ObjCWarnForNoInitDelegation = false;
1656416590
}
1656516591

16566-
diagnoseImplicitlyRetainedSelf(*this);
16592+
diagnoseBlockCaptures(*this);
1656716593
} else {
1656816594
// Parsing the function declaration failed in some way. Pop the fake scope
1656916595
// we pushed on.

‎clang/lib/Sema/SemaDeclObjC.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -367,7 +367,7 @@ HasExplicitOwnershipAttr(Sema &S, ParmVarDecl *Param) {
367367
/// and user declared, in the method definition's AST.
368368
void SemaObjC::ActOnStartOfObjCMethodDef(Scope *FnBodyScope, Decl *D) {
369369
ASTContext &Context = getASTContext();
370-
SemaRef.ImplicitlyRetainedSelfLocs.clear();
370+
SemaRef.DiagnosableBlockCaptures.clear();
371371
assert((SemaRef.getCurMethodDecl() == nullptr) && "Methodparsing confused");
372372
ObjCMethodDecl *MDecl = dyn_cast_or_null<ObjCMethodDecl>(D);
373373

‎clang/lib/Sema/SemaExpr.cpp‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16536,6 +16536,11 @@ ExprResult Sema::ActOnBlockStmtExpr(SourceLocation CaretLoc,
1653616536
// Set the captured variables on the block.
1653716537
SmallVector<BlockDecl::Capture, 4> Captures;
1653816538
for (Capture &Cap : BSI->Captures) {
16539+
if (Cap.isThisCapture()) {
16540+
DiagnosableBlockCaptures.push_back(
16541+
Sema::BlockCapture{Cap.getLocation(), BD, Sema::BlockCapture::This});
16542+
}
16543+
1653916544
if (Cap.isInvalid() || Cap.isThisCapture())
1654016545
continue;
1654116546
// Cap.getVariable() is always a VarDecl because
@@ -16595,6 +16600,14 @@ ExprResult Sema::ActOnBlockStmtExpr(SourceLocation CaretLoc,
1659516600
}
1659616601
}
1659716602

16603+
QualType Type = Cap.getCaptureType();
16604+
if (Type->isPointerOrReferenceType()) {
16605+
DiagnosableBlockCaptures.push_back(Sema::BlockCapture{
16606+
Cap.getLocation(), BD,
16607+
Type->isReferenceType() ? Sema::BlockCapture::Reference
16608+
: Sema::BlockCapture::RawPointer});
16609+
}
16610+
1659816611
BlockDecl::Capture NewCap(Var, Cap.isBlockCapture(), Cap.isNested(),
1659916612
CopyExpr);
1660016613
Captures.push_back(NewCap);

‎clang/lib/Sema/SemaExprObjC.cpp‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4913,8 +4913,10 @@ ExprResult SemaObjC::BuildIvarRefExpr(Scope *S, SourceLocation Loc,
49134913
SemaRef.getCurFunction()->recordUseOfWeak(Result);
49144914
}
49154915
if (getLangOpts().ObjCAutoRefCount && !SemaRef.isUnevaluatedContext())
4916-
if (const BlockDecl *BD = SemaRef.CurContext->getInnermostBlockDecl())
4917-
SemaRef.ImplicitlyRetainedSelfLocs.push_back({Loc, BD});
4916+
if (const BlockDecl *BD = SemaRef.CurContext->getInnermostBlockDecl()) {
4917+
SemaRef.DiagnosableBlockCaptures.push_back(
4918+
Sema::BlockCapture{Loc, BD, Sema::BlockCapture::Self});
4919+
}
49184920

49194921
return Result;
49204922
}

‎clang/test/SemaObjCXX/warn-blocks-capturing-pointers.mm‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
// RUN: %clang_cc1 -x objective-c++ -std=c++11 -fobjc-arc -fblocks -Wblocks-capturing-this -Wblocks-capturing-reference -Wblocks-capturing-raw-pointer -Wimplicit-retain-self -verify %s
2+
3+
typedef void (^BlockTy)();
4+
5+
void noescapeFunc(__attribute__((noescape)) BlockTy);
6+
void escapeFunc(BlockTy);
7+
8+
@interface Root @end
9+
10+
@interface I : Root
11+
- (void)foo;
12+
@end
13+
14+
class F {
15+
public:
16+
void Foo() const;
17+
void FooAsync(F* p, F& r, I* i) {
18+
escapeFunc(
19+
^{
20+
Foo(); // expected-warning {{block implicitly captures 'this'}}
21+
p->Foo(); // expected-warning {{block implicitly captures a raw pointer}}
22+
r.Foo(); // expected-warning {{block implicitly captures a C++ reference}}
23+
[i foo];
24+
});
25+
26+
([=, &r]() {
27+
escapeFunc(
28+
^{
29+
Foo(); // expected-warning {{block implicitly captures 'this'}}
30+
p->Foo(); // expected-warning {{block implicitly captures a raw pointer}}
31+
r.Foo(); // expected-warning {{block implicitly captures a C++ reference}}
32+
[i foo];
33+
});
34+
})();
35+
36+
escapeFunc(
37+
^{
38+
([=]() {
39+
Foo(); // expected-warning {{block implicitly captures 'this'}}
40+
p->Foo(); // expected-warning {{block implicitly captures a raw pointer}}
41+
r.Foo(); // expected-warning {{block implicitly captures a C++ reference}}
42+
[i foo];
43+
})();
44+
});
45+
46+
noescapeFunc(
47+
^{
48+
Foo();
49+
p->Foo();
50+
r.Foo();
51+
[i foo];
52+
});
53+
}
54+
};
55+
56+
@implementation I {
57+
int _bar;
58+
}
59+
60+
- (void)doSomethingWithPtr:(F*)p
61+
ref:(F&)r
62+
obj:(I*)i {
63+
escapeFunc(
64+
^{
65+
(void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
66+
p->Foo(); // expected-warning {{block implicitly captures a raw pointer}}
67+
r.Foo(); // expected-warning {{block implicitly captures a C++ reference}}
68+
[i foo];
69+
});
70+
71+
([=, &r]() {
72+
escapeFunc(
73+
^{
74+
(void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
75+
p->Foo(); // expected-warning {{block implicitly captures a raw pointer}}
76+
r.Foo(); // expected-warning {{block implicitly captures a C++ reference}}
77+
[i foo];
78+
});
79+
})();
80+
81+
escapeFunc(
82+
^{
83+
([=]() {
84+
(void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
85+
p->Foo(); // expected-warning {{block implicitly captures a raw pointer}}
86+
r.Foo(); // expected-warning {{block implicitly captures a C++ reference}}
87+
[i foo];
88+
})();
89+
});
90+
91+
noescapeFunc(
92+
^{
93+
(void)_bar;
94+
p->Foo();
95+
r.Foo();
96+
[i foo];
97+
});
98+
}
99+
100+
- (void)foo {
101+
}
102+
103+
@end

0 commit comments

Comments
 (0)