From 2fdd95c1c823ebafc9286f7b730339c38636c5a8 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Fri, 31 May 2019 00:45:09 +0000 Subject: [PATCH] Defer capture initialization for blocks until after we've left the function scope. This removes one of the last few cases where we build expressions in the wrong function scope context. No functionality change intended. llvm-svn: 362178 --- clang/include/clang/Sema/AnalysisBasedWarnings.h | 2 +- clang/include/clang/Sema/ScopeInfo.h | 2 + clang/include/clang/Sema/Sema.h | 20 +++- clang/lib/Sema/AnalysisBasedWarnings.cpp | 9 +- clang/lib/Sema/Sema.cpp | 47 ++++---- clang/lib/Sema/SemaExpr.cpp | 130 +++++++++++++---------- clang/test/Analysis/blocks.mm | 7 +- 7 files changed, 130 insertions(+), 87 deletions(-) diff --git a/clang/include/clang/Sema/AnalysisBasedWarnings.h b/clang/include/clang/Sema/AnalysisBasedWarnings.h index d5df536..e13fe95 100644 --- a/clang/include/clang/Sema/AnalysisBasedWarnings.h +++ b/clang/include/clang/Sema/AnalysisBasedWarnings.h @@ -90,7 +90,7 @@ public: AnalysisBasedWarnings(Sema &s); void IssueWarnings(Policy P, FunctionScopeInfo *fscope, - const Decl *D, const BlockExpr *blkExpr); + const Decl *D, QualType BlockType); Policy getDefaultPolicy() { return DefaultPolicy; } diff --git a/clang/include/clang/Sema/ScopeInfo.h b/clang/include/clang/Sema/ScopeInfo.h index 375d931..7b6e011 100644 --- a/clang/include/clang/Sema/ScopeInfo.h +++ b/clang/include/clang/Sema/ScopeInfo.h @@ -487,6 +487,8 @@ public: /// Clear out the information in this function scope, making it /// suitable for reuse. void Clear(); + + bool isPlainFunction() const { return Kind == SK_Function; } }; class Capture { diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index d7486ec..7ec9f47 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -595,7 +595,7 @@ public: using MaybeODRUseExprSet = llvm::SmallPtrSet; MaybeODRUseExprSet MaybeODRUseExprs; - std::unique_ptr PreallocatedFunctionScope; + std::unique_ptr CachedFunctionScope; /// Stack containing information about each of the nested /// function, block, and method scopes that are currently active. @@ -1408,10 +1408,24 @@ public: void PushCapturedRegionScope(Scope *RegionScope, CapturedDecl *CD, RecordDecl *RD, CapturedRegionKind K); - void + + /// Custom deleter to allow FunctionScopeInfos to be kept alive for a short + /// time after they've been popped. + class PoppedFunctionScopeDeleter { + Sema *Self; + + public: + explicit PoppedFunctionScopeDeleter(Sema *Self) : Self(Self) {} + void operator()(sema::FunctionScopeInfo *Scope) const; + }; + + using PoppedFunctionScopePtr = + std::unique_ptr; + + PoppedFunctionScopePtr PopFunctionScopeInfo(const sema::AnalysisBasedWarnings::Policy *WP = nullptr, const Decl *D = nullptr, - const BlockExpr *blkExpr = nullptr); + QualType BlockType = QualType()); sema::FunctionScopeInfo *getCurFunction() const { return FunctionScopes.empty() ? nullptr : FunctionScopes.back(); diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index bac407b..ce01909 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -620,7 +620,7 @@ struct CheckFallThroughDiagnostics { /// of a noreturn function. We assume that functions and blocks not marked /// noreturn will return. static void CheckFallThroughForBody(Sema &S, const Decl *D, const Stmt *Body, - const BlockExpr *blkExpr, + QualType BlockType, const CheckFallThroughDiagnostics &CD, AnalysisDeclContext &AC, sema::FunctionScopeInfo *FSI) { @@ -641,9 +641,8 @@ static void CheckFallThroughForBody(Sema &S, const Decl *D, const Stmt *Body, HasNoReturn = MD->hasAttr(); } else if (isa(D)) { - QualType BlockTy = blkExpr->getType(); if (const FunctionType *FT = - BlockTy->getPointeeType()->getAs()) { + BlockType->getPointeeType()->getAs()) { if (FT->getReturnType()->isVoidType()) ReturnsVoid = true; if (FT->getNoReturnAttr()) @@ -2012,7 +2011,7 @@ static void flushDiagnostics(Sema &S, const sema::FunctionScopeInfo *fscope) { void clang::sema:: AnalysisBasedWarnings::IssueWarnings(sema::AnalysisBasedWarnings::Policy P, sema::FunctionScopeInfo *fscope, - const Decl *D, const BlockExpr *blkExpr) { + const Decl *D, QualType BlockType) { // We avoid doing analysis-based warnings when there are errors for // two reasons: @@ -2138,7 +2137,7 @@ AnalysisBasedWarnings::IssueWarnings(sema::AnalysisBasedWarnings::Policy P, : (fscope->isCoroutine() ? CheckFallThroughDiagnostics::MakeForCoroutine(D) : CheckFallThroughDiagnostics::MakeForFunction(D))); - CheckFallThroughForBody(S, D, Body, blkExpr, CD, AC, fscope); + CheckFallThroughForBody(S, D, Body, BlockType, CD, AC, fscope); } // Warning: check for unreachable code diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp index 71b2f47..1a8948b 100644 --- a/clang/lib/Sema/Sema.cpp +++ b/clang/lib/Sema/Sema.cpp @@ -176,8 +176,6 @@ Sema::Sema(Preprocessor &pp, ASTContext &ctxt, ASTConsumer &consumer, ExpressionEvaluationContext::PotentiallyEvaluated, 0, CleanupInfo{}, nullptr, ExpressionEvaluationContextRecord::EK_Other); - PreallocatedFunctionScope.reset(new FunctionScopeInfo(Diags)); - // Initialization of data sharing attributes stack for OpenMP InitDataSharingAttributesStack(); @@ -354,8 +352,7 @@ Sema::~Sema() { // Kill all the active scopes. for (sema::FunctionScopeInfo *FSI : FunctionScopes) - if (FSI != PreallocatedFunctionScope.get()) - delete FSI; + delete FSI; // Tell the SemaConsumer to forget about us; we're going out of scope. if (SemaConsumer *SC = dyn_cast(&Consumer)) @@ -1596,10 +1593,10 @@ Scope *Sema::getScopeForContext(DeclContext *Ctx) { /// Enter a new function scope void Sema::PushFunctionScope() { - if (FunctionScopes.empty()) { - // Use PreallocatedFunctionScope to avoid allocating memory when possible. - PreallocatedFunctionScope->Clear(); - FunctionScopes.push_back(PreallocatedFunctionScope.get()); + if (FunctionScopes.empty() && CachedFunctionScope) { + // Use CachedFunctionScope to avoid allocating memory when possible. + CachedFunctionScope->Clear(); + FunctionScopes.push_back(CachedFunctionScope.release()); } else { FunctionScopes.push_back(new FunctionScopeInfo(getDiagnostics())); } @@ -1680,30 +1677,42 @@ static void markEscapingByrefs(const FunctionScopeInfo &FSI, Sema &S) { } } -void Sema::PopFunctionScopeInfo(const AnalysisBasedWarnings::Policy *WP, - const Decl *D, const BlockExpr *blkExpr) { +/// Pop a function (or block or lambda or captured region) scope from the stack. +/// +/// \param WP The warning policy to use for CFG-based warnings, or null if such +/// warnings should not be produced. +/// \param D The declaration corresponding to this function scope, if producing +/// CFG-based warnings. +/// \param BlockType The type of the block expression, if D is a BlockDecl. +Sema::PoppedFunctionScopePtr +Sema::PopFunctionScopeInfo(const AnalysisBasedWarnings::Policy *WP, + const Decl *D, QualType BlockType) { assert(!FunctionScopes.empty() && "mismatched push/pop!"); - // This function shouldn't be called after popping the current function scope. - // markEscapingByrefs calls PerformMoveOrCopyInitialization, which can call - // PushFunctionScope, which can cause clearing out PreallocatedFunctionScope - // when FunctionScopes is empty. markEscapingByrefs(*FunctionScopes.back(), *this); - FunctionScopeInfo *Scope = FunctionScopes.pop_back_val(); + PoppedFunctionScopePtr Scope(FunctionScopes.pop_back_val(), + PoppedFunctionScopeDeleter(this)); if (LangOpts.OpenMP) - popOpenMPFunctionRegion(Scope); + popOpenMPFunctionRegion(Scope.get()); // Issue any analysis-based warnings. if (WP && D) - AnalysisWarnings.IssueWarnings(*WP, Scope, D, blkExpr); + AnalysisWarnings.IssueWarnings(*WP, Scope.get(), D, BlockType); else for (const auto &PUD : Scope->PossiblyUnreachableDiags) Diag(PUD.Loc, PUD.PD); - // Delete the scope unless its our preallocated scope. - if (Scope != PreallocatedFunctionScope.get()) + return Scope; +} + +void Sema::PoppedFunctionScopeDeleter:: +operator()(sema::FunctionScopeInfo *Scope) const { + // Stash the function scope for later reuse if it's for a normal function. + if (Scope->isPlainFunction() && !Self->CachedFunctionScope) + Self->CachedFunctionScope.reset(Scope); + else delete Scope; } diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp index 762ab67..7a86e88 100644 --- a/clang/lib/Sema/SemaExpr.cpp +++ b/clang/lib/Sema/SemaExpr.cpp @@ -13823,8 +13823,6 @@ ExprResult Sema::ActOnBlockStmtExpr(SourceLocation CaretLoc, if (BSI->HasImplicitReturnType) deduceClosureReturnType(*BSI); - PopDeclContext(); - QualType RetTy = Context.VoidTy; if (!BSI->ReturnType.isNull()) RetTy = BSI->ReturnType; @@ -13832,17 +13830,6 @@ ExprResult Sema::ActOnBlockStmtExpr(SourceLocation CaretLoc, bool NoReturn = BD->hasAttr(); QualType BlockTy; - // Set the captured variables on the block. - SmallVector Captures; - for (Capture &Cap : BSI->Captures) { - if (Cap.isInvalid() || Cap.isThisCapture()) - continue; - BlockDecl::Capture NewCap(Cap.getVariable(), Cap.isBlockCapture(), - Cap.isNested(), Cap.getInitExpr()); - Captures.push_back(NewCap); - } - BD->setCaptures(Context, Captures, BSI->CXXThisCaptureIndex != 0); - // If the user wrote a function type in some form, try to use that. if (!BSI->FunctionType.isNull()) { const FunctionType *FTy = BSI->FunctionType->getAs(); @@ -13898,9 +13885,80 @@ ExprResult Sema::ActOnBlockStmtExpr(SourceLocation CaretLoc, !BD->isDependentContext()) computeNRVO(Body, BSI); - BlockExpr *Result = new (Context) BlockExpr(BD, BlockTy); + PopDeclContext(); + + // Pop the block scope now but keep it alive to the end of this function. AnalysisBasedWarnings::Policy WP = AnalysisWarnings.getDefaultPolicy(); - PopFunctionScopeInfo(&WP, Result->getBlockDecl(), Result); + PoppedFunctionScopePtr ScopeRAII = PopFunctionScopeInfo(&WP, BD, BlockTy); + + // Set the captured variables on the block. + SmallVector Captures; + for (Capture &Cap : BSI->Captures) { + if (Cap.isInvalid() || Cap.isThisCapture()) + continue; + + VarDecl *Var = Cap.getVariable(); + Expr *CopyExpr = nullptr; + if (getLangOpts().CPlusPlus && Cap.isCopyCapture()) { + if (const RecordType *Record = + Cap.getCaptureType()->getAs()) { + // The capture logic needs the destructor, so make sure we mark it. + // Usually this is unnecessary because most local variables have + // their destructors marked at declaration time, but parameters are + // an exception because it's technically only the call site that + // actually requires the destructor. + if (isa(Var)) + FinalizeVarWithDestructor(Var, Record); + + // Enter a separate potentially-evaluated context while building block + // initializers to isolate their cleanups from those of the block + // itself. + // FIXME: Is this appropriate even when the block itself occurs in an + // unevaluated operand? + EnterExpressionEvaluationContext EvalContext( + *this, ExpressionEvaluationContext::PotentiallyEvaluated); + + SourceLocation Loc = Cap.getLocation(); + + ExprResult Result = BuildDeclarationNameExpr( + CXXScopeSpec(), DeclarationNameInfo(Var->getDeclName(), Loc), Var); + + // According to the blocks spec, the capture of a variable from + // the stack requires a const copy constructor. This is not true + // of the copy/move done to move a __block variable to the heap. + if (!Result.isInvalid() && + !Result.get()->getType().isConstQualified()) { + Result = ImpCastExprToType(Result.get(), + Result.get()->getType().withConst(), + CK_NoOp, VK_LValue); + } + + if (!Result.isInvalid()) { + Result = PerformCopyInitialization( + InitializedEntity::InitializeBlock(Var->getLocation(), + Cap.getCaptureType(), false), + Loc, Result.get()); + } + + // Build a full-expression copy expression if initialization + // succeeded and used a non-trivial constructor. Recover from + // errors by pretending that the copy isn't necessary. + if (!Result.isInvalid() && + !cast(Result.get())->getConstructor() + ->isTrivial()) { + Result = MaybeCreateExprWithCleanups(Result); + CopyExpr = Result.get(); + } + } + } + + BlockDecl::Capture NewCap(Var, Cap.isBlockCapture(), Cap.isNested(), + CopyExpr); + Captures.push_back(NewCap); + } + BD->setCaptures(Context, Captures, BSI->CXXThisCaptureIndex != 0); + + BlockExpr *Result = new (Context) BlockExpr(BD, BlockTy); // If the block isn't obviously global, i.e. it captures anything at // all, then we need to do a few things in the surrounding context: @@ -15192,7 +15250,6 @@ static bool captureInBlock(BlockScopeInfo *BSI, VarDecl *Var, QualType &DeclRefType, const bool Nested, Sema &S, bool Invalid) { - Expr *CopyExpr = nullptr; bool ByRef = false; // Blocks are not allowed to capture arrays, excepting OpenCL. @@ -15264,51 +15321,12 @@ static bool captureInBlock(BlockScopeInfo *BSI, VarDecl *Var, // Block capture by copy introduces 'const'. CaptureType = CaptureType.getNonReferenceType().withConst(); DeclRefType = CaptureType; - - if (S.getLangOpts().CPlusPlus && BuildAndDiagnose) { - if (const RecordType *Record = DeclRefType->getAs()) { - // The capture logic needs the destructor, so make sure we mark it. - // Usually this is unnecessary because most local variables have - // their destructors marked at declaration time, but parameters are - // an exception because it's technically only the call site that - // actually requires the destructor. - if (isa(Var)) - S.FinalizeVarWithDestructor(Var, Record); - - // Enter a new evaluation context to insulate the copy - // full-expression. - EnterExpressionEvaluationContext scope( - S, Sema::ExpressionEvaluationContext::PotentiallyEvaluated); - - // According to the blocks spec, the capture of a variable from - // the stack requires a const copy constructor. This is not true - // of the copy/move done to move a __block variable to the heap. - Expr *DeclRef = new (S.Context) DeclRefExpr( - S.Context, Var, Nested, DeclRefType.withConst(), VK_LValue, Loc); - - ExprResult Result - = S.PerformCopyInitialization( - InitializedEntity::InitializeBlock(Var->getLocation(), - CaptureType, false), - Loc, DeclRef); - - // Build a full-expression copy expression if initialization - // succeeded and used a non-trivial constructor. Recover from - // errors by pretending that the copy isn't necessary. - if (!Result.isInvalid() && - !cast(Result.get())->getConstructor() - ->isTrivial()) { - Result = S.MaybeCreateExprWithCleanups(Result); - CopyExpr = Result.get(); - } - } - } } // Actually capture the variable. if (BuildAndDiagnose) BSI->addCapture(Var, HasBlocksAttr, ByRef, Nested, Loc, SourceLocation(), - CaptureType, CopyExpr, Invalid); + CaptureType, nullptr, Invalid); return !Invalid; } diff --git a/clang/test/Analysis/blocks.mm b/clang/test/Analysis/blocks.mm index 8a3f170..97c531e 100644 --- a/clang/test/Analysis/blocks.mm +++ b/clang/test/Analysis/blocks.mm @@ -52,9 +52,10 @@ void testBlockWithCopyExpression(StructWithCopyConstructor s) { // CHECK: [B1] // CHECK-NEXT: 1: s -// CHECK-NEXT: 2: [B1.1] (CXXConstructExpr, const struct StructWithCopyConstructor) -// CHECK-NEXT: 3: ^{ } -// CHECK-NEXT: 4: (void)([B1.3]) (CStyleCastExpr, ToVoid, void) +// CHECK-NEXT: 2: [B1.1] (ImplicitCastExpr, NoOp, const struct StructWithCopyConstructor) +// CHECK-NEXT: 3: [B1.2] (CXXConstructExpr, const struct StructWithCopyConstructor) +// CHECK-NEXT: 4: ^{ } +// CHECK-NEXT: 5: (void)([B1.4]) (CStyleCastExpr, ToVoid, void) // CHECK-NEXT: Preds (1): B2 // CHECK-NEXT: Succs (1): B0 -- 2.7.4