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>, DefaultIgnore; +def warn_blocks_capturing_this : Warning<"block implicitly captures 'this'">, + InGroup>, + DefaultIgnore; +def warn_blocks_capturing_reference + : Warning<"block implicitly captures a C++ reference">, + InGroup>, + DefaultIgnore; +def warn_blocks_capturing_raw_pointer + : Warning<"block implicitly captures a raw pointer">, + InGroup>, + 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, 1> - ImplicitlyRetainedSelfLocs; + llvm::SmallVector 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 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 &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(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(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 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