Add warning for blocks capturing {'this', raw pointers, references} #144388
Conversation
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 llvm#143924.
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Sylvain Defresne (sdefresne) ChangesAdd 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:
Fixes issue #143924. Full diff: https://github.com/llvm/llvm-project/pull/144388.diff 7 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 979ff60b73b75..4b33c40658028 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -1641,6 +1641,17 @@ def warn_implicitly_retains_self : Warning <
"block implicitly retains 'self'; explicitly mention 'self' to indicate "
"this is intended behavior">,
InGroup<DiagGroup<"implicit-retain-self">>, DefaultIgnore;
+def warn_blocks_capturing_this : Warning<"block implicitly captures 'this'">,
+ InGroup<DiagGroup<"blocks-capturing-this">>,
+ DefaultIgnore;
+def warn_blocks_capturing_reference
+ : Warning<"block implicitly captures a C++ reference">,
+ InGroup<DiagGroup<"blocks-capturing-reference">>,
+ DefaultIgnore;
+def warn_blocks_capturing_raw_pointer
+ : Warning<"block implicitly captures a raw pointer">,
+ InGroup<DiagGroup<"blocks-capturing-raw-pointer">>,
+ DefaultIgnore;
def warn_arc_possible_repeated_use_of_weak : Warning <
"weak %select{variable|property|implicit property|instance variable}0 %1 may "
"be accessed multiple times in this %select{function|method|block|lambda}2 "
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 29452bb37260d..7a6ec7c7a3f2c 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -8227,6 +8227,22 @@ class Sema final : public SemaBase {
}
};
+ /// Store information about a diagnosable block catpure.
+ struct BlockCapture {
+ /// Enumeration representing types of block captures that may be
+ /// diagnosable because they could be problematic.
+ enum CaptureType {
+ Self,
+ This,
+ Reference,
+ RawPointer,
+ };
+
+ SourceLocation Loc;
+ const BlockDecl *BD;
+ CaptureType Type;
+ };
+
/// Check an argument list for placeholders that we won't try to
/// handle later.
bool CheckArgsForPlaceholders(MultiExprArg args);
@@ -8243,8 +8259,7 @@ class Sema final : public SemaBase {
/// List of SourceLocations where 'self' is implicitly retained inside a
/// block.
- llvm::SmallVector<std::pair<SourceLocation, const BlockDecl *>, 1>
- ImplicitlyRetainedSelfLocs;
+ llvm::SmallVector<BlockCapture, 1> DiagnosableBlockCaptures;
/// Do an explicit extend of the given block pointer if we're in ARC.
void maybeExtendBlockObject(ExprResult &E);
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 5cffd82e3372e..713cbf9e3ef2c 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -16149,7 +16149,7 @@ class ExitFunctionBodyRAII {
bool IsLambda = false;
};
-static void diagnoseImplicitlyRetainedSelf(Sema &S) {
+static void diagnoseBlockCaptures(Sema &S) {
llvm::DenseMap<const BlockDecl *, bool> EscapeInfo;
auto IsOrNestedInEscapingBlock = [&](const BlockDecl *BD) {
@@ -16170,13 +16170,35 @@ static void diagnoseImplicitlyRetainedSelf(Sema &S) {
return It->second = R;
};
- // If the location where 'self' is implicitly retained is inside a escaping
- // block, emit a diagnostic.
- for (const std::pair<SourceLocation, const BlockDecl *> &P :
- S.ImplicitlyRetainedSelfLocs)
- if (IsOrNestedInEscapingBlock(P.second))
- S.Diag(P.first, diag::warn_implicitly_retains_self)
- << FixItHint::CreateInsertion(P.first, "self->");
+ for (const auto &C : S.DiagnosableBlockCaptures) {
+ // Do not emit a diagnostic if the location is invalid.
+ if (!C.Loc.isValid())
+ continue;
+
+ // Do not emit a diagnostic if the block is not escaping.
+ if (!IsOrNestedInEscapingBlock(C.BD))
+ continue;
+
+ // Emit the correct warning depending on the type of capture.
+ switch (C.Type) {
+ case Sema::BlockCapture::Self:
+ S.Diag(C.Loc, diag::warn_implicitly_retains_self)
+ << FixItHint::CreateInsertion(C.Loc, "self->");
+ break;
+
+ case Sema::BlockCapture::This:
+ S.Diag(C.Loc, diag::warn_blocks_capturing_this);
+ break;
+
+ case Sema::BlockCapture::Reference:
+ S.Diag(C.Loc, diag::warn_blocks_capturing_reference);
+ break;
+
+ case Sema::BlockCapture::RawPointer:
+ S.Diag(C.Loc, diag::warn_blocks_capturing_raw_pointer);
+ break;
+ }
+ }
}
static bool methodHasName(const FunctionDecl *FD, StringRef Name) {
@@ -16511,6 +16533,10 @@ Decl *Sema::ActOnFinishFunctionBody(Decl *dcl, Stmt *Body,
}
}
+ // Warn about block captures, unless this is a lambda function.
+ if (!isLambdaCallOperator(FD))
+ diagnoseBlockCaptures(*this);
+
assert((FD == getCurFunctionDecl(/*AllowLambdas=*/true)) &&
"Function parsing confused");
} else if (ObjCMethodDecl *MD = dyn_cast_or_null<ObjCMethodDecl>(dcl)) {
@@ -16563,7 +16589,7 @@ Decl *Sema::ActOnFinishFunctionBody(Decl *dcl, Stmt *Body,
FSI->ObjCWarnForNoInitDelegation = false;
}
- diagnoseImplicitlyRetainedSelf(*this);
+ diagnoseBlockCaptures(*this);
} else {
// Parsing the function declaration failed in some way. Pop the fake scope
// we pushed on.
diff --git a/clang/lib/Sema/SemaDeclObjC.cpp b/clang/lib/Sema/SemaDeclObjC.cpp
index 9dbc3efbca405..b6191bbe9aebf 100644
--- a/clang/lib/Sema/SemaDeclObjC.cpp
+++ b/clang/lib/Sema/SemaDeclObjC.cpp
@@ -367,7 +367,7 @@ HasExplicitOwnershipAttr(Sema &S, ParmVarDecl *Param) {
/// and user declared, in the method definition's AST.
void SemaObjC::ActOnStartOfObjCMethodDef(Scope *FnBodyScope, Decl *D) {
ASTContext &Context = getASTContext();
- SemaRef.ImplicitlyRetainedSelfLocs.clear();
+ SemaRef.DiagnosableBlockCaptures.clear();
assert((SemaRef.getCurMethodDecl() == nullptr) && "Methodparsing confused");
ObjCMethodDecl *MDecl = dyn_cast_or_null<ObjCMethodDecl>(D);
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 413eff4aa294a..80200fbc00b19 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -16536,6 +16536,11 @@ ExprResult Sema::ActOnBlockStmtExpr(SourceLocation CaretLoc,
// Set the captured variables on the block.
SmallVector<BlockDecl::Capture, 4> Captures;
for (Capture &Cap : BSI->Captures) {
+ if (Cap.isThisCapture()) {
+ DiagnosableBlockCaptures.push_back(
+ Sema::BlockCapture{Cap.getLocation(), BD, Sema::BlockCapture::This});
+ }
+
if (Cap.isInvalid() || Cap.isThisCapture())
continue;
// Cap.getVariable() is always a VarDecl because
@@ -16595,6 +16600,14 @@ ExprResult Sema::ActOnBlockStmtExpr(SourceLocation CaretLoc,
}
}
+ QualType Type = Cap.getCaptureType();
+ if (Type->isPointerOrReferenceType()) {
+ DiagnosableBlockCaptures.push_back(Sema::BlockCapture{
+ Cap.getLocation(), BD,
+ Type->isReferenceType() ? Sema::BlockCapture::Reference
+ : Sema::BlockCapture::RawPointer});
+ }
+
BlockDecl::Capture NewCap(Var, Cap.isBlockCapture(), Cap.isNested(),
CopyExpr);
Captures.push_back(NewCap);
diff --git a/clang/lib/Sema/SemaExprObjC.cpp b/clang/lib/Sema/SemaExprObjC.cpp
index 3505d9f38d23c..8c8ccbefa46ed 100644
--- a/clang/lib/Sema/SemaExprObjC.cpp
+++ b/clang/lib/Sema/SemaExprObjC.cpp
@@ -4913,8 +4913,10 @@ ExprResult SemaObjC::BuildIvarRefExpr(Scope *S, SourceLocation Loc,
SemaRef.getCurFunction()->recordUseOfWeak(Result);
}
if (getLangOpts().ObjCAutoRefCount && !SemaRef.isUnevaluatedContext())
- if (const BlockDecl *BD = SemaRef.CurContext->getInnermostBlockDecl())
- SemaRef.ImplicitlyRetainedSelfLocs.push_back({Loc, BD});
+ if (const BlockDecl *BD = SemaRef.CurContext->getInnermostBlockDecl()) {
+ SemaRef.DiagnosableBlockCaptures.push_back(
+ Sema::BlockCapture{Loc, BD, Sema::BlockCapture::Self});
+ }
return Result;
}
diff --git a/clang/test/SemaObjCXX/warn-blocks-capturing-pointers.mm b/clang/test/SemaObjCXX/warn-blocks-capturing-pointers.mm
new file mode 100644
index 0000000000000..2bf1ea5261196
--- /dev/null
+++ b/clang/test/SemaObjCXX/warn-blocks-capturing-pointers.mm
@@ -0,0 +1,103 @@
+// 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
+
+typedef void (^BlockTy)();
+
+void noescapeFunc(__attribute__((noescape)) BlockTy);
+void escapeFunc(BlockTy);
+
+@interface Root @end
+
+@interface I : Root
+- (void)foo;
+@end
+
+class F {
+ public:
+ void Foo() const;
+ void FooAsync(F* p, F& r, I* i) {
+ escapeFunc(
+ ^{
+ Foo(); // expected-warning {{block implicitly captures 'this'}}
+ p->Foo(); // expected-warning {{block implicitly captures a raw pointer}}
+ r.Foo(); // expected-warning {{block implicitly captures a C++ reference}}
+ [i foo];
+ });
+
+ ([=, &r]() {
+ escapeFunc(
+ ^{
+ Foo(); // expected-warning {{block implicitly captures 'this'}}
+ p->Foo(); // expected-warning {{block implicitly captures a raw pointer}}
+ r.Foo(); // expected-warning {{block implicitly captures a C++ reference}}
+ [i foo];
+ });
+ })();
+
+ escapeFunc(
+ ^{
+ ([=]() {
+ Foo(); // expected-warning {{block implicitly captures 'this'}}
+ p->Foo(); // expected-warning {{block implicitly captures a raw pointer}}
+ r.Foo(); // expected-warning {{block implicitly captures a C++ reference}}
+ [i foo];
+ })();
+ });
+
+ noescapeFunc(
+ ^{
+ Foo();
+ p->Foo();
+ r.Foo();
+ [i foo];
+ });
+ }
+};
+
+@implementation I {
+ int _bar;
+}
+
+- (void)doSomethingWithPtr:(F*)p
+ ref:(F&)r
+ obj:(I*)i {
+ escapeFunc(
+ ^{
+ (void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+ p->Foo(); // expected-warning {{block implicitly captures a raw pointer}}
+ r.Foo(); // expected-warning {{block implicitly captures a C++ reference}}
+ [i foo];
+ });
+
+ ([=, &r]() {
+ escapeFunc(
+ ^{
+ (void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+ p->Foo(); // expected-warning {{block implicitly captures a raw pointer}}
+ r.Foo(); // expected-warning {{block implicitly captures a C++ reference}}
+ [i foo];
+ });
+ })();
+
+ escapeFunc(
+ ^{
+ ([=]() {
+ (void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+ p->Foo(); // expected-warning {{block implicitly captures a raw pointer}}
+ r.Foo(); // expected-warning {{block implicitly captures a C++ reference}}
+ [i foo];
+ })();
+ });
+
+ noescapeFunc(
+ ^{
+ (void)_bar;
+ p->Foo();
+ r.Foo();
+ [i foo];
+ });
+}
+
+- (void)foo {
+}
+
+@end
|
| def warn_blocks_capturing_raw_pointer | ||
| : Warning<"block implicitly captures a raw pointer">, | ||
| InGroup<DiagGroup<"blocks-capturing-raw-pointer">>, | ||
| DefaultIgnore; |
There was a problem hiding this comment.
There's an existing block-capture-autoreleasing warning group that's probably the right pattern for these group names.
We can also create a diagnostic group that includes all of these warnings. I'm not sure if that's a good idea because of my concerns above, but I state it for completeness.
| InGroup<DiagGroup<"blocks-capturing-reference">>, | ||
| DefaultIgnore; | ||
| def warn_blocks_capturing_raw_pointer | ||
| : Warning<"block implicitly captures a raw pointer">, |
There was a problem hiding this comment.
I don't think "raw pointer" is a term we use anywhere else in Clang diagnostics. You might say a "C pointer" or "non-Objective-C pointer".
The more I think about it, though, the more I find the idea of warning about pointer captures in all escaping blocks problematic. For references, sure, I think there's a very reasonable argument that they're specifically likely to be temporary. Even for this, though, that really depends on the kind of programming you're doing, although I suppose there's an argument that an escaping block might need to capture a smart pointer to this. (That's definitely not reliably true, though, e.g. if someone was doing manual refcounting.) For pointers in general... unfortunately, we just can't know the ownership semantics that the user intends; I think this would be very noisy. Maybe that's okay in an opt-in warning, but it's unfortunate, because I think there's potential for the reference-capture warning to be turned on by default eventually.
| @@ -16149,7 +16149,7 @@ class ExitFunctionBodyRAII { | |||
| bool IsLambda = false; | |||
| }; | |||
|
|
|||
| static void diagnoseImplicitlyRetainedSelf(Sema &S) { | |||
| static void diagnoseBlockCaptures(Sema &S) { | |||
There was a problem hiding this comment.
Maybe diagnoseEscapingBlockCaptures?
| @@ -8243,8 +8259,7 @@ class Sema final : public SemaBase { | |||
|
|
|||
| /// List of SourceLocations where 'self' is implicitly retained inside a | |||
| /// block. | |||
| llvm::SmallVector<std::pair<SourceLocation, const BlockDecl *>, 1> | |||
| ImplicitlyRetainedSelfLocs; | |||
| llvm::SmallVector<BlockCapture, 1> DiagnosableBlockCaptures; | |||
There was a problem hiding this comment.
Hmm. I am concerned about this being global on Sema rather than tied to something like a specific function scope info. We might end up diagnosing the same thing in multiple places when we have nested functions after the block capture, e.g. because of lambdas or local classes — anything that causes us to end a function body while the outer function body is still being processed. Maybe we just never saw this with the narrow case of implicit self captures that this was used with before.
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:
Fixes issue #143924.