llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-static-analyzer-1 Author: Yihan Wang (yronglin) <details> <summary>Changes</summary> A default member initializer used by a constructor is a separate full-expression, while one used during aggregate initialization belongs to the full-expression containing the aggregate initialization. This patch split the two building paths so aggregate initialization rebuilds the initializer in the surrounding evaluation context. Fixes https://github.com/llvm/llvm-project/issues/85601. --- Patch is 54.30 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/219288.diff 21 Files Affected: - (modified) clang/docs/ReleaseNotes.md (+4) - (modified) clang/include/clang/AST/ParentMap.h (+31) - (modified) clang/include/clang/Sema/Sema.h (+22-1) - (modified) clang/lib/AST/ByteCode/Compiler.cpp (+62-2) - (modified) clang/lib/AST/ParentMap.cpp (+17) - (modified) clang/lib/Analysis/CFG.cpp (+46-9) - (modified) clang/lib/Analysis/ReachableCode.cpp (+18-49) - (modified) clang/lib/Sema/SemaDeclCXX.cpp (+8-2) - (modified) clang/lib/Sema/SemaExpr.cpp (+131-79) - (modified) clang/lib/Sema/SemaInit.cpp (+7-21) - (modified) clang/lib/Sema/TreeTransform.h (+1-1) - (modified) clang/lib/StaticAnalyzer/Core/ExprEngine.cpp (+36-21) - (modified) clang/test/AST/ByteCode/records.cpp (+21-2) - (modified) clang/test/AST/ast-dump-default-init.cpp (+29-38) - (modified) clang/test/AST/ast-dump-recovery.cpp (+1-1) - (modified) clang/test/Analysis/lifetime-extended-regions.cpp (+3-4) - (added) clang/test/CodeGenCXX/aggregate-default-member-initializers.cpp (+42) - (added) clang/test/SemaCXX/aggregate-default-member-initializers.cpp (+103) - (modified) clang/test/SemaCXX/cxx2c-placeholder-vars.cpp (+4-4) - (modified) clang/test/SemaCXX/warn-unreachable.cpp (+104) - (modified) clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp (+18) ``````````diff diff --git a/clang/docs/ReleaseNotes.md b/clang/docs/ReleaseNotes.md index 35aae605d8476..50cfa0e75188b 100644 --- a/clang/docs/ReleaseNotes.md +++ b/clang/docs/ReleaseNotes.md @@ -468,6 +468,10 @@ features cannot lower the translation-unit ABI level; #### Bug Fixes to C++ Support +- Fixed the destruction timing of temporaries created by default member + initializers during aggregate initialization. Such an initializer is part of + the full-expression containing the aggregate initialization. (#GH85601) + - Fixed an issue where `__typeof__` incorrectly rejected cv-qualified function types. - Fixed a bug where top-level CV qualifiers (such as ``const``) were dropped from pointers modified by Microsoft pointer attributes (like ``__ptr32`` and ``__ptr64``) and WebAssembly's ``__funcref``. diff --git a/clang/include/clang/AST/ParentMap.h b/clang/include/clang/AST/ParentMap.h index 86e2f048a3445..5853a0be0a483 100644 --- a/clang/include/clang/AST/ParentMap.h +++ b/clang/include/clang/AST/ParentMap.h @@ -13,6 +13,8 @@ #ifndef LLVM_CLANG_AST_PARENTMAP_H #define LLVM_CLANG_AST_PARENTMAP_H +#include "llvm/Support/Casting.h" + namespace clang { class Stmt; class Expr; @@ -39,6 +41,25 @@ class ParentMap { Stmt *getParentIgnoreParenImpCasts(Stmt *) const; Stmt *getOuterParenParent(Stmt *) const; + template <typename... Ts> Stmt *getOuterMostAncestor(Stmt *S) const { + Stmt *Res = nullptr; + while (S) { + if (llvm::isa<Ts...>(S)) + Res = S; + S = getParent(S); + } + return Res; + } + + template <typename... Ts> Stmt *getInnerMostAncestor(Stmt *S) const { + while (S) { + if (llvm::isa<Ts...>(S)) + return S; + S = getParent(S); + } + return nullptr; + } + const Stmt *getParent(const Stmt* S) const { return getParent(const_cast<Stmt*>(S)); } @@ -51,6 +72,16 @@ class ParentMap { return getParentIgnoreParenCasts(const_cast<Stmt*>(S)); } + template <typename... Ts> + const Stmt *getOuterMostAncestor(const Stmt *S) const { + return getOuterMostAncestor<Ts...>(const_cast<Stmt *>(S)); + } + + template <typename... Ts> + const Stmt *getInnerMostAncestor(const Stmt *S) const { + return getInnerMostAncestor<Ts...>(const_cast<Stmt *>(S)); + } + bool hasParent(const Stmt *S) const { return getParent(S) != nullptr; } bool isConsumedExpr(Expr *E) const; diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index dcf112fd8eaa4..10a764a403e37 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -5543,6 +5543,10 @@ class Sema final : public SemaBase { ExprResult ConvertMemberDefaultInitExpression(FieldDecl *FD, Expr *InitExpr, SourceLocation InitLoc); + ExprResult ConvertMemberDefaultInitExpression(FieldDecl *FD, + const InitializedEntity &Entity, + Expr *InitExpr, + SourceLocation InitLoc); /// FinalizeVarWithDestructor - Prepare for calling destructor on the /// constructed variable. @@ -7712,7 +7716,24 @@ class Sema final : public SemaBase { /// Emit a warning for all pending noderef expressions that we recorded. void WarnOnPendingNoDerefs(ExpressionEvaluationContextRecord &Rec); - ExprResult BuildCXXDefaultInitExpr(SourceLocation Loc, FieldDecl *Field); +private: + /// Shared logic for building default member initializer which used in a + /// constructor or an aggregate initialization. + /// + /// + /// The caller enters that evaluation context and decides whether the result + /// is finished as a full-expression. \p NestedDefaultChecking and + /// \p NeedRebuild have to be sampled before entering it. + ExprResult BuildCXXDefaultInitInternal(SourceLocation Loc, FieldDecl *Field, + const InitializedEntity &Entity, + bool NestedDefaultChecking, + bool NeedRebuild); + +public: + ExprResult BuildCXXCtorDefaultInitExpr(SourceLocation Loc, FieldDecl *Field); + ExprResult + BuildCXXAggregateDefaultInitExpr(SourceLocation Loc, FieldDecl *Field, + const InitializedEntity &MemberEntity); /// Instantiate or parse a C++ default argument expression as necessary. /// Return true on error. diff --git a/clang/lib/AST/ByteCode/Compiler.cpp b/clang/lib/AST/ByteCode/Compiler.cpp index c182639ea07f8..6b02a57a0deda 100644 --- a/clang/lib/AST/ByteCode/Compiler.cpp +++ b/clang/lib/AST/ByteCode/Compiler.cpp @@ -50,6 +50,22 @@ static bool isSideEffectFree(const Expr *E) { return false; } +static bool containsDefaultInitExpr(const Expr *E) { + class Finder final : public ConstDynamicRecursiveASTVisitor { + public: + Finder() { ShouldVisitImplicitCode = true; } + + bool VisitCXXDefaultInitExpr(const CXXDefaultInitExpr *) override { + Found = true; + return true; + } + + bool Found = false; + } F; + F.TraverseStmt(E); + return F.Found; +} + /// Scope chain managing the variable lifetimes. template <class Emitter> class VariableScope { public: @@ -265,7 +281,9 @@ template <class Emitter> class InitStackScope final { public: InitStackScope(Compiler<Emitter> *Ctx, bool Active) : Ctx(Ctx), OldValue(Ctx->InitStackActive), Active(Active) { - Ctx->InitStackActive = Active; + // An explicit initializer nested in a default member initializer still + // needs the surrounding default initializer's `this` reconstruction. + Ctx->InitStackActive = OldValue || Active; if (Active) Ctx->InitStack.push_back(InitLink::DIE()); } @@ -3475,6 +3493,18 @@ bool Compiler<Emitter>::VisitExprWithCleanups(const ExprWithCleanups *E) { LocalScope<Emitter> ES(this, ScopeKind::FullExpression); const Expr *SubExpr = E->getSubExpr(); + if (DiscardResult && !SubExpr->isGLValue() && + !canClassify(SubExpr->getType()) && containsDefaultInitExpr(SubExpr)) { + UnsignedOrNone LocalIndex = + allocateLocal(SubExpr, QualType(), ScopeKind::FullExpression); + if (!LocalIndex) + return false; + InitLinkScope<Emitter> ILS(this, InitLink::Temp(*LocalIndex)); + if (!this->emitGetPtrLocal(*LocalIndex, E)) + return false; + return this->visitInitializerPop(SubExpr) && ES.destroyLocals(E); + } + return this->delegate(SubExpr) && ES.destroyLocals(E); } @@ -3541,8 +3571,12 @@ bool Compiler<Emitter>::VisitMaterializeTemporaryExpr( // Non-primitive values. if (!this->emitGetPtrGlobal(*GlobalIndex, E)) return false; + if (!this->emitStartInit(E)) + return false; if (!this->visitInitializer(Inner)) return false; + if (!this->emitEndInit(E)) + return false; if (IsStatic) { assert(TempDecl); return this->emitInitGlobalTempComp(TempDecl, E); @@ -3585,7 +3619,11 @@ bool Compiler<Emitter>::VisitMaterializeTemporaryExpr( if (!this->emitGetPtrLocal(*LocalIndex, E)) return false; - return this->visitInitializer(Inner); + if (!this->emitStartInit(E)) + return false; + if (!this->visitInitializer(Inner)) + return false; + return this->emitEndInit(E); } return false; } @@ -4888,6 +4926,22 @@ bool Compiler<Emitter>::VisitStmtExpr(const StmtExpr *E) { } template <class Emitter> bool Compiler<Emitter>::discard(const Expr *E) { + // A discarded composite prvalue still needs a result object when a default + // member initializer refers to previously initialized subobjects. Let an + // ExprWithCleanups establish its full-expression scope before allocating + // that object. + if (!isa<ExprWithCleanups>(E) && !E->isGLValue() && + !canClassify(E->getType()) && containsDefaultInitExpr(E)) { + UnsignedOrNone LocalIndex = + allocateLocal(E, QualType(), ScopeKind::FullExpression); + if (!LocalIndex) + return false; + InitLinkScope<Emitter> ILS(this, InitLink::Temp(*LocalIndex)); + if (!this->emitGetPtrLocal(*LocalIndex, E)) + return false; + return this->visitInitializerPop(E); + } + OptionScope<Emitter> Scope(this, /*NewDiscardResult=*/true, /*NewInitializing=*/false, /*ToLValue=*/false); return this->Visit(E); @@ -5451,8 +5505,14 @@ bool Compiler<Emitter>::visitExpr(const Expr *E, bool DestroyToplevelScope) { if (!this->emitGetPtrLocal(*LocalOffset, E)) return false; + // A const-qualified result object is writable while it is being + // initialized, just like an object evaluated through visitVarDecl(). + if (!this->emitStartInit(E)) + return false; if (!visitInitializer(E)) return false; + if (!this->emitEndInit(E)) + return false; // We are destroying the locals AFTER the Ret op. // The Ret op needs to copy the (alive) values, but the // destructors may still turn the entire expression invalid. diff --git a/clang/lib/AST/ParentMap.cpp b/clang/lib/AST/ParentMap.cpp index e62e71bf5a514..580613b2618fb 100644 --- a/clang/lib/AST/ParentMap.cpp +++ b/clang/lib/AST/ParentMap.cpp @@ -13,6 +13,7 @@ #include "clang/AST/ParentMap.h" #include "clang/AST/Decl.h" #include "clang/AST/Expr.h" +#include "clang/AST/ExprCXX.h" #include "clang/AST/StmtObjC.h" #include "llvm/ADT/DenseMap.h" @@ -103,6 +104,22 @@ static void BuildParentMap(MapTy& M, Stmt* S, BuildParentMap(M, SubStmt, OVMode); } break; + case Stmt::CXXDefaultArgExprClass: + if (auto *Arg = dyn_cast<CXXDefaultArgExpr>(S)) { + if (Arg->hasRewrittenInit()) { + M[Arg->getExpr()] = S; + BuildParentMap(M, Arg->getExpr(), OVMode); + } + } + break; + case Stmt::CXXDefaultInitExprClass: + if (auto *Init = dyn_cast<CXXDefaultInitExpr>(S)) { + if (Init->hasRewrittenInit()) { + M[Init->getExpr()] = S; + BuildParentMap(M, Init->getExpr(), OVMode); + } + } + break; default: for (Stmt *SubStmt : S->children()) { if (SubStmt) { diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp index 5263114ebca28..f92e6f3dcbf47 100644 --- a/clang/lib/Analysis/CFG.cpp +++ b/clang/lib/Analysis/CFG.cpp @@ -581,6 +581,10 @@ class CFGBuilder { private: // Visitors to walk an AST and construct the CFG. + CFGBlock *VisitCXXDefaultArgExpr(CXXDefaultArgExpr *Default, + AddStmtChoice asc); + CFGBlock *VisitCXXDefaultInitExpr(CXXDefaultInitExpr *Default, + AddStmtChoice asc); CFGBlock *VisitInitListExpr(InitListExpr *ILE, AddStmtChoice asc); CFGBlock *VisitAddrLabelExpr(AddrLabelExpr *A, AddStmtChoice asc); CFGBlock *VisitAttributedStmt(AttributedStmt *A, AddStmtChoice asc); @@ -2405,16 +2409,10 @@ CFGBlock *CFGBuilder::Visit(Stmt * S, AddStmtChoice asc, asc, ExternallyDestructed); case Stmt::CXXDefaultArgExprClass: + return VisitCXXDefaultArgExpr(cast<CXXDefaultArgExpr>(S), asc); + case Stmt::CXXDefaultInitExprClass: - // FIXME: The expression inside a CXXDefaultArgExpr is owned by the - // called function's declaration, not by the caller. If we simply add - // this expression to the CFG, we could end up with the same Expr - // appearing multiple times (PR13385). - // - // It's likewise possible for multiple CXXDefaultInitExprs for the same - // expression to be used in the same function (through aggregate - // initialization). - return VisitStmt(S, asc); + return VisitCXXDefaultInitExpr(cast<CXXDefaultInitExpr>(S), asc); case Stmt::CXXBindTemporaryExprClass: return VisitCXXBindTemporaryExpr(cast<CXXBindTemporaryExpr>(S), asc); @@ -2597,6 +2595,45 @@ CFGBlock *CFGBuilder::VisitCallExprChildren(CallExpr *C) { return VisitChildren(C); } +CFGBlock *CFGBuilder::VisitCXXDefaultArgExpr(CXXDefaultArgExpr *Arg, + AddStmtChoice asc) { + if (Arg->hasRewrittenInit()) { + if (asc.alwaysAdd(*this, Arg)) { + autoCreateBlock(); + appendStmt(Block, Arg); + } + return VisitStmt(Arg->getExpr()->IgnoreParens(), asc); + } + + // We can't add the default argument if it's not rewritten because the + // expression inside a CXXDefaultArgExpr is owned by the called function's + // declaration, not by the caller. We could end up with the same expression + // appearing multiple times. + return VisitStmt(Arg, asc); +} + +CFGBlock *CFGBuilder::VisitCXXDefaultInitExpr(CXXDefaultInitExpr *Init, + AddStmtChoice asc) { + if (Init->hasRewrittenInit()) { + if (asc.alwaysAdd(*this, Init)) { + autoCreateBlock(); + appendStmt(Block, Init); + } + + // Unlike CXXDefaultArgExpr::getExpr, which strips off the top-level + // FullExpr and ConstantExpr, CXXDefaultInitExpr::getExpr does not do this, + // so the top level cannot be a ParenExpr. Use Visit rather than VisitStmt + // so that control flow inside the initializer (a conditional operator, for + // instance) is decomposed into blocks instead of being laid out linearly. + return Visit(Init->getExpr(), asc); + } + + // We can't add the default initializer if it's not rewritten because multiple + // CXXDefaultInitExprs can refer to the same subexpression in the same + // function (through aggregate initialization). + return VisitStmt(Init, asc); +} + CFGBlock *CFGBuilder::VisitInitListExpr(InitListExpr *ILE, AddStmtChoice asc) { if (asc.alwaysAdd(*this, ILE)) { autoCreateBlock(); diff --git a/clang/lib/Analysis/ReachableCode.cpp b/clang/lib/Analysis/ReachableCode.cpp index 4a9ab5d9f0f73..7d17a0cb4cfd4 100644 --- a/clang/lib/Analysis/ReachableCode.cpp +++ b/clang/lib/Analysis/ReachableCode.cpp @@ -25,6 +25,7 @@ #include "clang/Basic/SourceManager.h" #include "clang/Lex/Preprocessor.h" #include "llvm/ADT/BitVector.h" +#include <cstddef> #include <optional> using namespace clang; @@ -396,6 +397,7 @@ namespace { SmallVector<const CFGBlock *, 10> WorkList; Preprocessor &PP; ASTContext &C; + AnalysisDeclContext &AC; typedef SmallVector<std::pair<const CFGBlock *, const Stmt *>, 12> DeferredLocsTy; @@ -403,10 +405,10 @@ namespace { DeferredLocsTy DeferredLocs; public: - DeadCodeScan(llvm::BitVector &reachable, Preprocessor &PP, ASTContext &C) - : Visited(reachable.size()), - Reachable(reachable), - PP(PP), C(C) {} + DeadCodeScan(llvm::BitVector &reachable, Preprocessor &PP, + AnalysisDeclContext &AC) + : Visited(reachable.size()), Reachable(reachable), PP(PP), + C(AC.getASTContext()), AC(AC) {} void enqueue(const CFGBlock *block); unsigned scanBackwards(const CFGBlock *Start, @@ -453,47 +455,8 @@ bool DeadCodeScan::isDeadCodeRoot(const clang::CFGBlock *Block) { return isDeadRoot; } -// Check if the given `DeadStmt` is a coroutine statement and is a substmt of -// the coroutine statement. `Block` is the CFGBlock containing the `DeadStmt`. -static bool isInCoroutineStmt(const Stmt *DeadStmt, const CFGBlock *Block) { - // The coroutine statement, co_return, co_await, or co_yield. - const Stmt *CoroStmt = nullptr; - // Find the first coroutine statement after the DeadStmt in the block. - bool AfterDeadStmt = false; - for (const CFGElement &Elem : *Block) - if (std::optional<CFGStmt> CS = Elem.getAs<CFGStmt>()) { - const Stmt *S = CS->getStmt(); - if (S == DeadStmt) - AfterDeadStmt = true; - if (AfterDeadStmt && - // For simplicity, we only check simple coroutine statements. - (llvm::isa<CoreturnStmt>(S) || llvm::isa<CoroutineSuspendExpr>(S))) { - CoroStmt = S; - break; - } - } - if (!CoroStmt) - return false; - struct Checker : DynamicRecursiveASTVisitor { - const Stmt *DeadStmt; - bool CoroutineSubStmt = false; - Checker(const Stmt *S) : DeadStmt(S) { - // Statements captured in the CFG can be implicit. - ShouldVisitImplicitCode = true; - } - - bool VisitStmt(Stmt *S) override { - if (S == DeadStmt) - CoroutineSubStmt = true; - return true; - } - }; - Checker checker(DeadStmt); - checker.TraverseStmt(const_cast<Stmt *>(CoroStmt)); - return checker.CoroutineSubStmt; -} - -static bool isValidDeadStmt(const Stmt *S, const clang::CFGBlock *Block) { +static bool isValidDeadStmt(ParentMap &PM, const Stmt *S, + const clang::CFGBlock *) { if (S->getBeginLoc().isInvalid()) return false; if (const BinaryOperator *BO = dyn_cast<BinaryOperator>(S)) @@ -501,21 +464,27 @@ static bool isValidDeadStmt(const Stmt *S, const clang::CFGBlock *Block) { // Coroutine statements are never considered dead statements, because removing // them may change the function semantic if it is the only coroutine statement // of the coroutine. - return !isInCoroutineStmt(S, Block); + return !PM.getInnerMostAncestor<CoreturnStmt, CoroutineSuspendExpr>(S); } const Stmt *DeadCodeScan::findDeadCode(const clang::CFGBlock *Block) { + auto &PM = AC.getParentMap(); + for (CFGBlock::const_iterator I = Block->begin(), E = Block->end(); I!=E; ++I) if (std::optional<CFGStmt> CS = I->getAs<CFGStmt>()) { const Stmt *S = CS->getStmt(); - if (isValidDeadStmt(S, Block)) + auto *RewrittenParent = + PM.getOuterMostAncestor<CXXDefaultArgExpr, CXXDefaultInitExpr>(S); + if (RewrittenParent) + S = RewrittenParent; + if (isValidDeadStmt(AC.getParentMap(), S, Block)) return S; } CFGTerminator T = Block->getTerminator(); if (T.isStmtBranch()) { const Stmt *S = T.getStmt(); - if (S && isValidDeadStmt(S, Block)) + if (S && isValidDeadStmt(AC.getParentMap(), S, Block)) return S; } @@ -761,7 +730,7 @@ void FindUnreachableCode(AnalysisDeclContext &AC, Preprocessor &PP, if (reachable[block->getBlockID()]) continue; - DeadCodeScan DS(reachable, PP, AC.getASTContext()); + DeadCodeScan DS(reachable, PP, AC); numReachable += DS.scanBackwards(block, CB); if (numReachable == cfg->getNumBlockIDs()) diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp index 4e90c496de342..f03d615bdce5b 100644 --- a/clang/lib/Sema/SemaDeclCXX.cpp +++ b/clang/lib/Sema/SemaDeclCXX.cpp @@ -4242,6 +4242,12 @@ ExprResult Sema::ConvertMemberDefaultInitExpression(FieldDecl *FD, SourceLocation InitLoc) { InitializedEntity Entity = InitializedEntity::InitializeMemberFromDefaultMemberInitializer(FD); + return ConvertMemberDefaultInitExpression(FD, Entity, InitExpr, InitLoc); +} + +ExprResult Sema::ConvertMemberDefaultInitExpression( + FieldDecl *FD, const InitializedEntity &Entity, Expr *InitExpr, + SourceLocation InitLoc) { InitializationKind Kind = FD->getInClassInitStyle() == ICIS_ListInit ? InitializationKind::CreateDirectList(InitExpr->getBeginLoc(), @@ -5342,7 +5348,7 @@ static bool CollectFieldInitializer(Sema &SemaRef, BaseAndFieldInfo &Info, if (Field->hasInClassInitializer() && !Info.isImplicitCopyOrMove()) { ExprResult DIE = - SemaRef.BuildCXXDefaultInitExpr(Info.Ctor->getLocation(), Field); + SemaRef.BuildCXXCtorDefaultInitExpr(Info.Ctor->getLocation(), Field); if (DIE.isInvalid()) return true; @@ -14129,7 +14135,7 @@ bool SpecialMemberExceptionSpecInfo::visitField(FieldDecl *FD) { // FIXME: We should have a single context note pointing at Loc, and // this location should be MD->getLocation() instead, since that's // the location where we actually use the default init expression. - E = S.BuildCXXDefaultInitExpr(Loc, FD).get(); + E = S.BuildCXXCtorDefaultInitExpr(Loc, FD).get(); if (E) ExceptSpec.CalledExpr(E); } else if (auto *RD = S.Context.getBaseElementType(FD->getType()) diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp index 2b524a956ecc4..31b308ad4b52b 100644 --- a/clang/lib/Sema/SemaExpr.cpp +++ b/clang/lib/Sema/SemaExpr.cpp @@ -5876,39 +5876,51 @@ static FieldDecl *FindFieldDeclInstantiationPattern(const ASTContext &Ctx, return cast<FieldDecl>(*... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/219288 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
