Skip to content

[Analyzer][CFG] Correctly handle rebuilt default arg and default init expression #117437

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 1, 2025

Conversation

yronglin
Copy link
Contributor

@yronglin yronglin commented Nov 23, 2024

Clang currently support extending lifetime of object bound to reference members of aggregates, that are created from default member initializer. This PR address this change and updaye CFG and ExprEngine.

This PR reapply #91879.
Fixes #93725.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules clang:static analyzer clang:analysis labels Nov 23, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 23, 2024

@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang-modules

Author: None (yronglin)

Changes

Clang now support the following:

  • Extending lifetime of object bound to reference members of aggregates, that are created from default member initializer.
  • Rebuild all sub-expressions in CXXDefaultArgExpr and CXXDefaultInitExpr as needed.

But CFG and ExprEngine need to be updated to address this change.

This PR reapply #91879.


Patch is 30.04 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/117437.diff

14 Files Affected:

  • (modified) clang/include/clang/AST/ExprCXX.h (+21-9)
  • (modified) clang/include/clang/AST/Stmt.h (+11-2)
  • (modified) clang/lib/AST/ASTImporter.cpp (+4-3)
  • (modified) clang/lib/AST/ExprCXX.cpp (+24-19)
  • (modified) clang/lib/AST/ParentMap.cpp (+17)
  • (modified) clang/lib/Analysis/CFG.cpp (+41-9)
  • (modified) clang/lib/Analysis/ReachableCode.cpp (+16-15)
  • (modified) clang/lib/Sema/SemaExpr.cpp (+7-2)
  • (modified) clang/lib/Sema/TreeTransform.h (+4-3)
  • (modified) clang/lib/Serialization/ASTReaderStmt.cpp (+6-2)
  • (modified) clang/lib/Serialization/ASTWriterStmt.cpp (+2)
  • (modified) clang/lib/StaticAnalyzer/Core/ExprEngine.cpp (+34-22)
  • (modified) clang/test/Analysis/lifetime-extended-regions.cpp (+3-4)
  • (modified) clang/test/SemaCXX/warn-unreachable.cpp (+39)
diff --git a/clang/include/clang/AST/ExprCXX.h b/clang/include/clang/AST/ExprCXX.h
index 696a574833dad2..99680537a3f777 100644
--- a/clang/include/clang/AST/ExprCXX.h
+++ b/clang/include/clang/AST/ExprCXX.h
@@ -1277,7 +1277,8 @@ class CXXDefaultArgExpr final
   DeclContext *UsedContext;
 
   CXXDefaultArgExpr(StmtClass SC, SourceLocation Loc, ParmVarDecl *Param,
-                    Expr *RewrittenExpr, DeclContext *UsedContext)
+                    DeclContext *UsedContext, Expr *RewrittenExpr,
+                    bool HasRebuiltInit)
       : Expr(SC,
              Param->hasUnparsedDefaultArg()
                  ? Param->getType().getNonReferenceType()
@@ -1287,25 +1288,27 @@ class CXXDefaultArgExpr final
         Param(Param), UsedContext(UsedContext) {
     CXXDefaultArgExprBits.Loc = Loc;
     CXXDefaultArgExprBits.HasRewrittenInit = RewrittenExpr != nullptr;
+    CXXDefaultArgExprBits.HasRebuiltInit = HasRebuiltInit;
     if (RewrittenExpr)
       *getTrailingObjects<Expr *>() = RewrittenExpr;
     setDependence(computeDependence(this));
   }
 
-  CXXDefaultArgExpr(EmptyShell Empty, bool HasRewrittenInit)
+  CXXDefaultArgExpr(EmptyShell Empty, bool HasRewrittenInit, bool HasRebuiltInit)
       : Expr(CXXDefaultArgExprClass, Empty) {
     CXXDefaultArgExprBits.HasRewrittenInit = HasRewrittenInit;
+    CXXDefaultArgExprBits.HasRebuiltInit = HasRebuiltInit;
   }
 
 public:
   static CXXDefaultArgExpr *CreateEmpty(const ASTContext &C,
-                                        bool HasRewrittenInit);
+                                        bool HasRewrittenInit, bool HasRebuiltInit);
 
   // \p Param is the parameter whose default argument is used by this
   // expression.
   static CXXDefaultArgExpr *Create(const ASTContext &C, SourceLocation Loc,
-                                   ParmVarDecl *Param, Expr *RewrittenExpr,
-                                   DeclContext *UsedContext);
+                                   ParmVarDecl *Param, DeclContext *UsedContext,
+                                   Expr *RewrittenExpr, bool HasRebuiltInit);
   // Retrieve the parameter that the argument was created from.
   const ParmVarDecl *getParam() const { return Param; }
   ParmVarDecl *getParam() { return Param; }
@@ -1314,6 +1317,10 @@ class CXXDefaultArgExpr final
     return CXXDefaultArgExprBits.HasRewrittenInit;
   }
 
+  bool hasRebuiltInit() const {
+    return CXXDefaultArgExprBits.HasRebuiltInit;
+  }
+
   // Retrieve the argument to the function call.
   Expr *getExpr();
   const Expr *getExpr() const {
@@ -1385,26 +1392,31 @@ class CXXDefaultInitExpr final
 
   CXXDefaultInitExpr(const ASTContext &Ctx, SourceLocation Loc,
                      FieldDecl *Field, QualType Ty, DeclContext *UsedContext,
-                     Expr *RewrittenInitExpr);
+                     Expr *RewrittenInitExpr, bool HasRebuiltInit);
 
-  CXXDefaultInitExpr(EmptyShell Empty, bool HasRewrittenInit)
+  CXXDefaultInitExpr(EmptyShell Empty, bool HasRewrittenInit, bool HasRebuiltInit)
       : Expr(CXXDefaultInitExprClass, Empty) {
     CXXDefaultInitExprBits.HasRewrittenInit = HasRewrittenInit;
+    CXXDefaultInitExprBits.HasRebuiltInit = HasRebuiltInit;
   }
 
 public:
   static CXXDefaultInitExpr *CreateEmpty(const ASTContext &C,
-                                         bool HasRewrittenInit);
+                                         bool HasRewrittenInit, bool HasRebuiltInit);
   /// \p Field is the non-static data member whose default initializer is used
   /// by this expression.
   static CXXDefaultInitExpr *Create(const ASTContext &Ctx, SourceLocation Loc,
                                     FieldDecl *Field, DeclContext *UsedContext,
-                                    Expr *RewrittenInitExpr);
+                                    Expr *RewrittenInitExpr, bool HasRebuiltInit);
 
   bool hasRewrittenInit() const {
     return CXXDefaultInitExprBits.HasRewrittenInit;
   }
 
+  bool hasRebuiltInit() const {
+    return CXXDefaultInitExprBits.HasRebuiltInit;
+  }
+
   /// Get the field whose initializer will be used.
   FieldDecl *getField() { return Field; }
   const FieldDecl *getField() const { return Field; }
diff --git a/clang/include/clang/AST/Stmt.h b/clang/include/clang/AST/Stmt.h
index 83fafbabb1d460..1b2dbcbaa09219 100644
--- a/clang/include/clang/AST/Stmt.h
+++ b/clang/include/clang/AST/Stmt.h
@@ -839,6 +839,10 @@ class alignas(void *) Stmt {
     LLVM_PREFERRED_TYPE(bool)
     unsigned HasRewrittenInit : 1;
 
+    /// Whether this CXXDefaultArgExpr rebuild its argument and stores a copy.
+    LLVM_PREFERRED_TYPE(bool)
+    unsigned HasRebuiltInit : 1;
+
     /// The location where the default argument expression was used.
     SourceLocation Loc;
   };
@@ -850,11 +854,16 @@ class alignas(void *) Stmt {
     LLVM_PREFERRED_TYPE(ExprBitfields)
     unsigned : NumExprBits;
 
-    /// Whether this CXXDefaultInitExprBitfields rewrote its argument and stores
-    /// a copy.
+    /// Whether this CXXDefaultInitExpr rewrote its argument and stores
+    /// a copy, unlike HasRebuiltInit, when this flag is true, the argument may
+    /// be partial rebuilt.
     LLVM_PREFERRED_TYPE(bool)
     unsigned HasRewrittenInit : 1;
 
+    /// Whether this CXXDefaultInitExpr fully rebuild its argument and stores a copy.
+    LLVM_PREFERRED_TYPE(bool)
+    unsigned HasRebuiltInit : 1;
+
     /// The location where the default initializer expression was used.
     SourceLocation Loc;
   };
diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index baed1416635432..e2126f5fd29a47 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -8154,8 +8154,8 @@ ExpectedStmt ASTNodeImporter::VisitCXXDefaultArgExpr(CXXDefaultArgExpr *E) {
     RewrittenInit = ExprOrErr.get();
   }
   return CXXDefaultArgExpr::Create(Importer.getToContext(), *ToUsedLocOrErr,
-                                   *ToParamOrErr, RewrittenInit,
-                                   *UsedContextOrErr);
+                                   *ToParamOrErr, *UsedContextOrErr,
+                                   RewrittenInit, E->hasRebuiltInit());
 }
 
 ExpectedStmt
@@ -8863,7 +8863,8 @@ ExpectedStmt ASTNodeImporter::VisitCXXDefaultInitExpr(CXXDefaultInitExpr *E) {
   }
 
   return CXXDefaultInitExpr::Create(Importer.getToContext(), *ToBeginLocOrErr,
-                                    ToField, *UsedContextOrErr, RewrittenInit);
+                                    ToField, *UsedContextOrErr, RewrittenInit,
+                                    E->hasRebuiltInit());
 }
 
 ExpectedStmt ASTNodeImporter::VisitCXXNamedCastExpr(CXXNamedCastExpr *E) {
diff --git a/clang/lib/AST/ExprCXX.cpp b/clang/lib/AST/ExprCXX.cpp
index 0ce129de85f03f..6b6507d4b5ebeb 100644
--- a/clang/lib/AST/ExprCXX.cpp
+++ b/clang/lib/AST/ExprCXX.cpp
@@ -1009,21 +1009,23 @@ const IdentifierInfo *UserDefinedLiteral::getUDSuffix() const {
 }
 
 CXXDefaultArgExpr *CXXDefaultArgExpr::CreateEmpty(const ASTContext &C,
-                                                  bool HasRewrittenInit) {
+                                                  bool HasRewrittenInit,
+                                                  bool HasRebuiltInit) {
   size_t Size = totalSizeToAlloc<Expr *>(HasRewrittenInit);
   auto *Mem = C.Allocate(Size, alignof(CXXDefaultArgExpr));
-  return new (Mem) CXXDefaultArgExpr(EmptyShell(), HasRewrittenInit);
+  return new (Mem)
+      CXXDefaultArgExpr(EmptyShell(), HasRewrittenInit, HasRebuiltInit);
 }
 
-CXXDefaultArgExpr *CXXDefaultArgExpr::Create(const ASTContext &C,
-                                             SourceLocation Loc,
-                                             ParmVarDecl *Param,
-                                             Expr *RewrittenExpr,
-                                             DeclContext *UsedContext) {
+CXXDefaultArgExpr *
+CXXDefaultArgExpr::Create(const ASTContext &C, SourceLocation Loc,
+                          ParmVarDecl *Param, DeclContext *UsedContext,
+                          Expr *RewrittenExpr, bool HasRebuiltInit) {
   size_t Size = totalSizeToAlloc<Expr *>(RewrittenExpr != nullptr);
   auto *Mem = C.Allocate(Size, alignof(CXXDefaultArgExpr));
-  return new (Mem) CXXDefaultArgExpr(CXXDefaultArgExprClass, Loc, Param,
-                                     RewrittenExpr, UsedContext);
+  return new (Mem)
+      CXXDefaultArgExpr(CXXDefaultArgExprClass, Loc, Param, UsedContext,
+                        RewrittenExpr, HasRebuiltInit);
 }
 
 Expr *CXXDefaultArgExpr::getExpr() {
@@ -1044,7 +1046,7 @@ Expr *CXXDefaultArgExpr::getAdjustedRewrittenExpr() {
 CXXDefaultInitExpr::CXXDefaultInitExpr(const ASTContext &Ctx,
                                        SourceLocation Loc, FieldDecl *Field,
                                        QualType Ty, DeclContext *UsedContext,
-                                       Expr *RewrittenInitExpr)
+                                       Expr *RewrittenInitExpr, bool hasRebuiltInit)
     : Expr(CXXDefaultInitExprClass, Ty.getNonLValueExprType(Ctx),
            Ty->isLValueReferenceType()   ? VK_LValue
            : Ty->isRValueReferenceType() ? VK_XValue
@@ -1053,6 +1055,7 @@ CXXDefaultInitExpr::CXXDefaultInitExpr(const ASTContext &Ctx,
       Field(Field), UsedContext(UsedContext) {
   CXXDefaultInitExprBits.Loc = Loc;
   CXXDefaultInitExprBits.HasRewrittenInit = RewrittenInitExpr != nullptr;
+  CXXDefaultInitExprBits.HasRebuiltInit = hasRebuiltInit;
 
   if (CXXDefaultInitExprBits.HasRewrittenInit)
     *getTrailingObjects<Expr *>() = RewrittenInitExpr;
@@ -1063,22 +1066,24 @@ CXXDefaultInitExpr::CXXDefaultInitExpr(const ASTContext &Ctx,
 }
 
 CXXDefaultInitExpr *CXXDefaultInitExpr::CreateEmpty(const ASTContext &C,
-                                                    bool HasRewrittenInit) {
+                                                    bool HasRewrittenInit,
+                                                    bool HasRebuiltInit) {
   size_t Size = totalSizeToAlloc<Expr *>(HasRewrittenInit);
   auto *Mem = C.Allocate(Size, alignof(CXXDefaultInitExpr));
-  return new (Mem) CXXDefaultInitExpr(EmptyShell(), HasRewrittenInit);
+  return new (Mem)
+      CXXDefaultInitExpr(EmptyShell(), HasRewrittenInit, HasRebuiltInit);
 }
 
-CXXDefaultInitExpr *CXXDefaultInitExpr::Create(const ASTContext &Ctx,
-                                               SourceLocation Loc,
-                                               FieldDecl *Field,
-                                               DeclContext *UsedContext,
-                                               Expr *RewrittenInitExpr) {
+CXXDefaultInitExpr *
+CXXDefaultInitExpr::Create(const ASTContext &Ctx, SourceLocation Loc,
+                           FieldDecl *Field, DeclContext *UsedContext,
+                           Expr *RewrittenInitExpr, bool HasRebuiltInit) {
 
   size_t Size = totalSizeToAlloc<Expr *>(RewrittenInitExpr != nullptr);
   auto *Mem = Ctx.Allocate(Size, alignof(CXXDefaultInitExpr));
-  return new (Mem) CXXDefaultInitExpr(Ctx, Loc, Field, Field->getType(),
-                                      UsedContext, RewrittenInitExpr);
+  return new (Mem)
+      CXXDefaultInitExpr(Ctx, Loc, Field, Field->getType(), UsedContext,
+                         RewrittenInitExpr, HasRebuiltInit);
 }
 
 Expr *CXXDefaultInitExpr::getExpr() {
diff --git a/clang/lib/AST/ParentMap.cpp b/clang/lib/AST/ParentMap.cpp
index fd749b02b758c9..61eb2ec0e1cb94 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"
 
@@ -96,6 +97,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->hasRebuiltInit()) {
+        M[Arg->getExpr()] = S;
+        BuildParentMap(M, Arg->getExpr(), OVMode);
+      }
+    }
+    break;
+  case Stmt::CXXDefaultInitExprClass:
+    if (auto *Init = dyn_cast<CXXDefaultInitExpr>(S)) {
+      if (Init->hasRebuiltInit()) {
+        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 304bbb2b422c61..82c283335b0ff4 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -556,6 +556,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);
@@ -2261,16 +2265,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);
@@ -2440,6 +2438,40 @@ CFGBlock *CFGBuilder::VisitChildren(Stmt *S) {
   return B;
 }
 
+CFGBlock *CFGBuilder::VisitCXXDefaultArgExpr(CXXDefaultArgExpr *Arg,
+                                             AddStmtChoice asc) {
+  if (Arg->hasRebuiltInit()) {
+    if (asc.alwaysAdd(*this, Arg)) {
+      autoCreateBlock();
+      appendStmt(Block, Arg);
+    }
+    return VisitStmt(Arg->getExpr(), 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->hasRebuiltInit()) {
+    if (asc.alwaysAdd(*this, Init)) {
+      autoCreateBlock();
+      appendStmt(Block, Init);
+    }
+    return VisitStmt(Init->getExpr(), asc);
+  }
+
+  // We can't add the default initializer if it's not rewritten because multiple
+  // CXXDefaultInitExprs for the same sub-expression to be used in the same
+  // function (through aggregate initialization). we could end up with the same
+  // expression appearing multiple times.
+  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 dd81c8e0a3d543..9f0527317eca95 100644
--- a/clang/lib/Analysis/ReachableCode.cpp
+++ b/clang/lib/Analysis/ReachableCode.cpp
@@ -454,11 +454,10 @@ 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) {
+template <class... Ts>
+static bool isDeadStmtIn(const Stmt *DeadStmt, const CFGBlock *Block) {
   // The coroutine statement, co_return, co_await, or co_yield.
-  const Stmt *CoroStmt = nullptr;
+  const Stmt *TargetStmt = nullptr;
   // Find the first coroutine statement after the DeadStmt in the block.
   bool AfterDeadStmt = false;
   for (CFGBlock::const_iterator I = Block->begin(), E = Block->end(); I != E;
@@ -467,32 +466,29 @@ static bool isInCoroutineStmt(const Stmt *DeadStmt, const CFGBlock *Block) {
       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;
+      if (AfterDeadStmt && llvm::isa<Ts...>(S)) {
+        TargetStmt = S;
         break;
       }
     }
-  if (!CoroStmt)
+  if (!TargetStmt)
     return false;
   struct Checker : DynamicRecursiveASTVisitor {
     const Stmt *DeadStmt;
-    bool CoroutineSubStmt = false;
+    bool IsSubStmtOfTargetStmt = 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;
+        IsSubStmtOfTargetStmt = true;
       return true;
     }
   };
   Checker checker(DeadStmt);
-  checker.TraverseStmt(const_cast<Stmt *>(CoroStmt));
-  return checker.CoroutineSubStmt;
+  checker.TraverseStmt(const_cast<Stmt *>(TargetStmt));
+  return checker.IsSubStmtOfTargetStmt;
 }
 
 static bool isValidDeadStmt(const Stmt *S, const clang::CFGBlock *Block) {
@@ -503,7 +499,12 @@ 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);
+  //
+  // If the dead stmt is a sub-stmt of CXXDefaultInitExpr and CXXDefaultArgExpr,
+  // we would rather expect to find CXXDefaultInitExpr and CXXDefaultArgExpr as
+  // a valid dead stmt.
+  return !isDeadStmtIn<CoreturnStmt, CoroutineSuspendExpr, CXXDefaultArgExpr,
+                       CXXDefaultInitExpr>(S, Block);
 }
 
 const Stmt *DeadCodeScan::findDeadCode(const clang::CFGBlock *Block) {
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 6c7472ce92703b..baea4ffef1799e 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -5489,6 +5489,7 @@ ExprResult Sema::BuildCXXDefaultArgExpr(SourceLocation CallLoc,
 
   bool NestedDefaultChecking = isCheckingDefaultArgumentOrInitializer();
   bool NeedRebuild = needsRebuildOfDefaultArgOrInit();
+  bool HasRebuiltInit = false;
   std::optional<ExpressionEvaluationContextRecord::InitializationContext>
       InitializationContext =
           OutermostDeclarationWithDelayedImmediateInvocations();
@@ -5546,6 +5547,7 @@ ExprResult Sema::BuildCXXDefaultArgExpr(SourceLocation CallLoc,
       if (Res.isInvalid())
         return ExprError();
       Init = Res.get();
+      HasRebuiltInit = true;
     }
   }
 
@@ -5555,7 +5557,8 @@ ExprResult Sema::BuildCXXDefaultArgExpr(SourceLocation CallLoc,
     return ExprError();
 
   return CXXDefaultArgExpr::Create(Context, InitializationContext->Loc, Param,
-                                   Init, InitializationContext->Context);
+                                   InitializationContext->Context, Init,
+                                   HasRebuiltInit);
 }
 
 static FieldDecl *FindFieldDeclInstantiationPattern(const ASTContext &Ctx,
@@ -5597,6 +5600,7 @@ ExprResult Sema::BuildCXXDefaultInitExpr(SourceLocation Loc, FieldDecl *Field) {
 
   bool NestedDefaultChecking = isCheckingDefaultArgumentOrInitializer();
   bool NeedRebuild = needsRebuildOfDefaultArgOrInit();
+  bool HasRebuiltInit = false;
   EnterExpressionEvaluationContext EvalContext(
       *this, ExpressionEvaluationContext::PotentiallyEvaluated, Field);
 
@@ -5658,6 +5662,7 @@ ExprResult Sema::BuildCXXDefaultInitExpr(SourceLocat...
[truncated]

@llvmbot
Copy link
Member

llvmbot commented Nov 23, 2024

@llvm/pr-subscribers-clang

Author: None (yronglin)

Changes

Clang now support the following:

  • Extending lifetime of object bound to reference members of aggregates, that are created from default member initializer.
  • Rebuild all sub-expressions in CXXDefaultArgExpr and CXXDefaultInitExpr as needed.

But CFG and ExprEngine need to be updated to address this change.

This PR reapply #91879.


Patch is 30.04 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/117437.diff

14 Files Affected:

  • (modified) clang/include/clang/AST/ExprCXX.h (+21-9)
  • (modified) clang/include/clang/AST/Stmt.h (+11-2)
  • (modified) clang/lib/AST/ASTImporter.cpp (+4-3)
  • (modified) clang/lib/AST/ExprCXX.cpp (+24-19)
  • (modified) clang/lib/AST/ParentMap.cpp (+17)
  • (modified) clang/lib/Analysis/CFG.cpp (+41-9)
  • (modified) clang/lib/Analysis/ReachableCode.cpp (+16-15)
  • (modified) clang/lib/Sema/SemaExpr.cpp (+7-2)
  • (modified) clang/lib/Sema/TreeTransform.h (+4-3)
  • (modified) clang/lib/Serialization/ASTReaderStmt.cpp (+6-2)
  • (modified) clang/lib/Serialization/ASTWriterStmt.cpp (+2)
  • (modified) clang/lib/StaticAnalyzer/Core/ExprEngine.cpp (+34-22)
  • (modified) clang/test/Analysis/lifetime-extended-regions.cpp (+3-4)
  • (modified) clang/test/SemaCXX/warn-unreachable.cpp (+39)
diff --git a/clang/include/clang/AST/ExprCXX.h b/clang/include/clang/AST/ExprCXX.h
index 696a574833dad2..99680537a3f777 100644
--- a/clang/include/clang/AST/ExprCXX.h
+++ b/clang/include/clang/AST/ExprCXX.h
@@ -1277,7 +1277,8 @@ class CXXDefaultArgExpr final
   DeclContext *UsedContext;
 
   CXXDefaultArgExpr(StmtClass SC, SourceLocation Loc, ParmVarDecl *Param,
-                    Expr *RewrittenExpr, DeclContext *UsedContext)
+                    DeclContext *UsedContext, Expr *RewrittenExpr,
+                    bool HasRebuiltInit)
       : Expr(SC,
              Param->hasUnparsedDefaultArg()
                  ? Param->getType().getNonReferenceType()
@@ -1287,25 +1288,27 @@ class CXXDefaultArgExpr final
         Param(Param), UsedContext(UsedContext) {
     CXXDefaultArgExprBits.Loc = Loc;
     CXXDefaultArgExprBits.HasRewrittenInit = RewrittenExpr != nullptr;
+    CXXDefaultArgExprBits.HasRebuiltInit = HasRebuiltInit;
     if (RewrittenExpr)
       *getTrailingObjects<Expr *>() = RewrittenExpr;
     setDependence(computeDependence(this));
   }
 
-  CXXDefaultArgExpr(EmptyShell Empty, bool HasRewrittenInit)
+  CXXDefaultArgExpr(EmptyShell Empty, bool HasRewrittenInit, bool HasRebuiltInit)
       : Expr(CXXDefaultArgExprClass, Empty) {
     CXXDefaultArgExprBits.HasRewrittenInit = HasRewrittenInit;
+    CXXDefaultArgExprBits.HasRebuiltInit = HasRebuiltInit;
   }
 
 public:
   static CXXDefaultArgExpr *CreateEmpty(const ASTContext &C,
-                                        bool HasRewrittenInit);
+                                        bool HasRewrittenInit, bool HasRebuiltInit);
 
   // \p Param is the parameter whose default argument is used by this
   // expression.
   static CXXDefaultArgExpr *Create(const ASTContext &C, SourceLocation Loc,
-                                   ParmVarDecl *Param, Expr *RewrittenExpr,
-                                   DeclContext *UsedContext);
+                                   ParmVarDecl *Param, DeclContext *UsedContext,
+                                   Expr *RewrittenExpr, bool HasRebuiltInit);
   // Retrieve the parameter that the argument was created from.
   const ParmVarDecl *getParam() const { return Param; }
   ParmVarDecl *getParam() { return Param; }
@@ -1314,6 +1317,10 @@ class CXXDefaultArgExpr final
     return CXXDefaultArgExprBits.HasRewrittenInit;
   }
 
+  bool hasRebuiltInit() const {
+    return CXXDefaultArgExprBits.HasRebuiltInit;
+  }
+
   // Retrieve the argument to the function call.
   Expr *getExpr();
   const Expr *getExpr() const {
@@ -1385,26 +1392,31 @@ class CXXDefaultInitExpr final
 
   CXXDefaultInitExpr(const ASTContext &Ctx, SourceLocation Loc,
                      FieldDecl *Field, QualType Ty, DeclContext *UsedContext,
-                     Expr *RewrittenInitExpr);
+                     Expr *RewrittenInitExpr, bool HasRebuiltInit);
 
-  CXXDefaultInitExpr(EmptyShell Empty, bool HasRewrittenInit)
+  CXXDefaultInitExpr(EmptyShell Empty, bool HasRewrittenInit, bool HasRebuiltInit)
       : Expr(CXXDefaultInitExprClass, Empty) {
     CXXDefaultInitExprBits.HasRewrittenInit = HasRewrittenInit;
+    CXXDefaultInitExprBits.HasRebuiltInit = HasRebuiltInit;
   }
 
 public:
   static CXXDefaultInitExpr *CreateEmpty(const ASTContext &C,
-                                         bool HasRewrittenInit);
+                                         bool HasRewrittenInit, bool HasRebuiltInit);
   /// \p Field is the non-static data member whose default initializer is used
   /// by this expression.
   static CXXDefaultInitExpr *Create(const ASTContext &Ctx, SourceLocation Loc,
                                     FieldDecl *Field, DeclContext *UsedContext,
-                                    Expr *RewrittenInitExpr);
+                                    Expr *RewrittenInitExpr, bool HasRebuiltInit);
 
   bool hasRewrittenInit() const {
     return CXXDefaultInitExprBits.HasRewrittenInit;
   }
 
+  bool hasRebuiltInit() const {
+    return CXXDefaultInitExprBits.HasRebuiltInit;
+  }
+
   /// Get the field whose initializer will be used.
   FieldDecl *getField() { return Field; }
   const FieldDecl *getField() const { return Field; }
diff --git a/clang/include/clang/AST/Stmt.h b/clang/include/clang/AST/Stmt.h
index 83fafbabb1d460..1b2dbcbaa09219 100644
--- a/clang/include/clang/AST/Stmt.h
+++ b/clang/include/clang/AST/Stmt.h
@@ -839,6 +839,10 @@ class alignas(void *) Stmt {
     LLVM_PREFERRED_TYPE(bool)
     unsigned HasRewrittenInit : 1;
 
+    /// Whether this CXXDefaultArgExpr rebuild its argument and stores a copy.
+    LLVM_PREFERRED_TYPE(bool)
+    unsigned HasRebuiltInit : 1;
+
     /// The location where the default argument expression was used.
     SourceLocation Loc;
   };
@@ -850,11 +854,16 @@ class alignas(void *) Stmt {
     LLVM_PREFERRED_TYPE(ExprBitfields)
     unsigned : NumExprBits;
 
-    /// Whether this CXXDefaultInitExprBitfields rewrote its argument and stores
-    /// a copy.
+    /// Whether this CXXDefaultInitExpr rewrote its argument and stores
+    /// a copy, unlike HasRebuiltInit, when this flag is true, the argument may
+    /// be partial rebuilt.
     LLVM_PREFERRED_TYPE(bool)
     unsigned HasRewrittenInit : 1;
 
+    /// Whether this CXXDefaultInitExpr fully rebuild its argument and stores a copy.
+    LLVM_PREFERRED_TYPE(bool)
+    unsigned HasRebuiltInit : 1;
+
     /// The location where the default initializer expression was used.
     SourceLocation Loc;
   };
diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index baed1416635432..e2126f5fd29a47 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -8154,8 +8154,8 @@ ExpectedStmt ASTNodeImporter::VisitCXXDefaultArgExpr(CXXDefaultArgExpr *E) {
     RewrittenInit = ExprOrErr.get();
   }
   return CXXDefaultArgExpr::Create(Importer.getToContext(), *ToUsedLocOrErr,
-                                   *ToParamOrErr, RewrittenInit,
-                                   *UsedContextOrErr);
+                                   *ToParamOrErr, *UsedContextOrErr,
+                                   RewrittenInit, E->hasRebuiltInit());
 }
 
 ExpectedStmt
@@ -8863,7 +8863,8 @@ ExpectedStmt ASTNodeImporter::VisitCXXDefaultInitExpr(CXXDefaultInitExpr *E) {
   }
 
   return CXXDefaultInitExpr::Create(Importer.getToContext(), *ToBeginLocOrErr,
-                                    ToField, *UsedContextOrErr, RewrittenInit);
+                                    ToField, *UsedContextOrErr, RewrittenInit,
+                                    E->hasRebuiltInit());
 }
 
 ExpectedStmt ASTNodeImporter::VisitCXXNamedCastExpr(CXXNamedCastExpr *E) {
diff --git a/clang/lib/AST/ExprCXX.cpp b/clang/lib/AST/ExprCXX.cpp
index 0ce129de85f03f..6b6507d4b5ebeb 100644
--- a/clang/lib/AST/ExprCXX.cpp
+++ b/clang/lib/AST/ExprCXX.cpp
@@ -1009,21 +1009,23 @@ const IdentifierInfo *UserDefinedLiteral::getUDSuffix() const {
 }
 
 CXXDefaultArgExpr *CXXDefaultArgExpr::CreateEmpty(const ASTContext &C,
-                                                  bool HasRewrittenInit) {
+                                                  bool HasRewrittenInit,
+                                                  bool HasRebuiltInit) {
   size_t Size = totalSizeToAlloc<Expr *>(HasRewrittenInit);
   auto *Mem = C.Allocate(Size, alignof(CXXDefaultArgExpr));
-  return new (Mem) CXXDefaultArgExpr(EmptyShell(), HasRewrittenInit);
+  return new (Mem)
+      CXXDefaultArgExpr(EmptyShell(), HasRewrittenInit, HasRebuiltInit);
 }
 
-CXXDefaultArgExpr *CXXDefaultArgExpr::Create(const ASTContext &C,
-                                             SourceLocation Loc,
-                                             ParmVarDecl *Param,
-                                             Expr *RewrittenExpr,
-                                             DeclContext *UsedContext) {
+CXXDefaultArgExpr *
+CXXDefaultArgExpr::Create(const ASTContext &C, SourceLocation Loc,
+                          ParmVarDecl *Param, DeclContext *UsedContext,
+                          Expr *RewrittenExpr, bool HasRebuiltInit) {
   size_t Size = totalSizeToAlloc<Expr *>(RewrittenExpr != nullptr);
   auto *Mem = C.Allocate(Size, alignof(CXXDefaultArgExpr));
-  return new (Mem) CXXDefaultArgExpr(CXXDefaultArgExprClass, Loc, Param,
-                                     RewrittenExpr, UsedContext);
+  return new (Mem)
+      CXXDefaultArgExpr(CXXDefaultArgExprClass, Loc, Param, UsedContext,
+                        RewrittenExpr, HasRebuiltInit);
 }
 
 Expr *CXXDefaultArgExpr::getExpr() {
@@ -1044,7 +1046,7 @@ Expr *CXXDefaultArgExpr::getAdjustedRewrittenExpr() {
 CXXDefaultInitExpr::CXXDefaultInitExpr(const ASTContext &Ctx,
                                        SourceLocation Loc, FieldDecl *Field,
                                        QualType Ty, DeclContext *UsedContext,
-                                       Expr *RewrittenInitExpr)
+                                       Expr *RewrittenInitExpr, bool hasRebuiltInit)
     : Expr(CXXDefaultInitExprClass, Ty.getNonLValueExprType(Ctx),
            Ty->isLValueReferenceType()   ? VK_LValue
            : Ty->isRValueReferenceType() ? VK_XValue
@@ -1053,6 +1055,7 @@ CXXDefaultInitExpr::CXXDefaultInitExpr(const ASTContext &Ctx,
       Field(Field), UsedContext(UsedContext) {
   CXXDefaultInitExprBits.Loc = Loc;
   CXXDefaultInitExprBits.HasRewrittenInit = RewrittenInitExpr != nullptr;
+  CXXDefaultInitExprBits.HasRebuiltInit = hasRebuiltInit;
 
   if (CXXDefaultInitExprBits.HasRewrittenInit)
     *getTrailingObjects<Expr *>() = RewrittenInitExpr;
@@ -1063,22 +1066,24 @@ CXXDefaultInitExpr::CXXDefaultInitExpr(const ASTContext &Ctx,
 }
 
 CXXDefaultInitExpr *CXXDefaultInitExpr::CreateEmpty(const ASTContext &C,
-                                                    bool HasRewrittenInit) {
+                                                    bool HasRewrittenInit,
+                                                    bool HasRebuiltInit) {
   size_t Size = totalSizeToAlloc<Expr *>(HasRewrittenInit);
   auto *Mem = C.Allocate(Size, alignof(CXXDefaultInitExpr));
-  return new (Mem) CXXDefaultInitExpr(EmptyShell(), HasRewrittenInit);
+  return new (Mem)
+      CXXDefaultInitExpr(EmptyShell(), HasRewrittenInit, HasRebuiltInit);
 }
 
-CXXDefaultInitExpr *CXXDefaultInitExpr::Create(const ASTContext &Ctx,
-                                               SourceLocation Loc,
-                                               FieldDecl *Field,
-                                               DeclContext *UsedContext,
-                                               Expr *RewrittenInitExpr) {
+CXXDefaultInitExpr *
+CXXDefaultInitExpr::Create(const ASTContext &Ctx, SourceLocation Loc,
+                           FieldDecl *Field, DeclContext *UsedContext,
+                           Expr *RewrittenInitExpr, bool HasRebuiltInit) {
 
   size_t Size = totalSizeToAlloc<Expr *>(RewrittenInitExpr != nullptr);
   auto *Mem = Ctx.Allocate(Size, alignof(CXXDefaultInitExpr));
-  return new (Mem) CXXDefaultInitExpr(Ctx, Loc, Field, Field->getType(),
-                                      UsedContext, RewrittenInitExpr);
+  return new (Mem)
+      CXXDefaultInitExpr(Ctx, Loc, Field, Field->getType(), UsedContext,
+                         RewrittenInitExpr, HasRebuiltInit);
 }
 
 Expr *CXXDefaultInitExpr::getExpr() {
diff --git a/clang/lib/AST/ParentMap.cpp b/clang/lib/AST/ParentMap.cpp
index fd749b02b758c9..61eb2ec0e1cb94 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"
 
@@ -96,6 +97,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->hasRebuiltInit()) {
+        M[Arg->getExpr()] = S;
+        BuildParentMap(M, Arg->getExpr(), OVMode);
+      }
+    }
+    break;
+  case Stmt::CXXDefaultInitExprClass:
+    if (auto *Init = dyn_cast<CXXDefaultInitExpr>(S)) {
+      if (Init->hasRebuiltInit()) {
+        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 304bbb2b422c61..82c283335b0ff4 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -556,6 +556,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);
@@ -2261,16 +2265,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);
@@ -2440,6 +2438,40 @@ CFGBlock *CFGBuilder::VisitChildren(Stmt *S) {
   return B;
 }
 
+CFGBlock *CFGBuilder::VisitCXXDefaultArgExpr(CXXDefaultArgExpr *Arg,
+                                             AddStmtChoice asc) {
+  if (Arg->hasRebuiltInit()) {
+    if (asc.alwaysAdd(*this, Arg)) {
+      autoCreateBlock();
+      appendStmt(Block, Arg);
+    }
+    return VisitStmt(Arg->getExpr(), 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->hasRebuiltInit()) {
+    if (asc.alwaysAdd(*this, Init)) {
+      autoCreateBlock();
+      appendStmt(Block, Init);
+    }
+    return VisitStmt(Init->getExpr(), asc);
+  }
+
+  // We can't add the default initializer if it's not rewritten because multiple
+  // CXXDefaultInitExprs for the same sub-expression to be used in the same
+  // function (through aggregate initialization). we could end up with the same
+  // expression appearing multiple times.
+  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 dd81c8e0a3d543..9f0527317eca95 100644
--- a/clang/lib/Analysis/ReachableCode.cpp
+++ b/clang/lib/Analysis/ReachableCode.cpp
@@ -454,11 +454,10 @@ 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) {
+template <class... Ts>
+static bool isDeadStmtIn(const Stmt *DeadStmt, const CFGBlock *Block) {
   // The coroutine statement, co_return, co_await, or co_yield.
-  const Stmt *CoroStmt = nullptr;
+  const Stmt *TargetStmt = nullptr;
   // Find the first coroutine statement after the DeadStmt in the block.
   bool AfterDeadStmt = false;
   for (CFGBlock::const_iterator I = Block->begin(), E = Block->end(); I != E;
@@ -467,32 +466,29 @@ static bool isInCoroutineStmt(const Stmt *DeadStmt, const CFGBlock *Block) {
       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;
+      if (AfterDeadStmt && llvm::isa<Ts...>(S)) {
+        TargetStmt = S;
         break;
       }
     }
-  if (!CoroStmt)
+  if (!TargetStmt)
     return false;
   struct Checker : DynamicRecursiveASTVisitor {
     const Stmt *DeadStmt;
-    bool CoroutineSubStmt = false;
+    bool IsSubStmtOfTargetStmt = 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;
+        IsSubStmtOfTargetStmt = true;
       return true;
     }
   };
   Checker checker(DeadStmt);
-  checker.TraverseStmt(const_cast<Stmt *>(CoroStmt));
-  return checker.CoroutineSubStmt;
+  checker.TraverseStmt(const_cast<Stmt *>(TargetStmt));
+  return checker.IsSubStmtOfTargetStmt;
 }
 
 static bool isValidDeadStmt(const Stmt *S, const clang::CFGBlock *Block) {
@@ -503,7 +499,12 @@ 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);
+  //
+  // If the dead stmt is a sub-stmt of CXXDefaultInitExpr and CXXDefaultArgExpr,
+  // we would rather expect to find CXXDefaultInitExpr and CXXDefaultArgExpr as
+  // a valid dead stmt.
+  return !isDeadStmtIn<CoreturnStmt, CoroutineSuspendExpr, CXXDefaultArgExpr,
+                       CXXDefaultInitExpr>(S, Block);
 }
 
 const Stmt *DeadCodeScan::findDeadCode(const clang::CFGBlock *Block) {
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 6c7472ce92703b..baea4ffef1799e 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -5489,6 +5489,7 @@ ExprResult Sema::BuildCXXDefaultArgExpr(SourceLocation CallLoc,
 
   bool NestedDefaultChecking = isCheckingDefaultArgumentOrInitializer();
   bool NeedRebuild = needsRebuildOfDefaultArgOrInit();
+  bool HasRebuiltInit = false;
   std::optional<ExpressionEvaluationContextRecord::InitializationContext>
       InitializationContext =
           OutermostDeclarationWithDelayedImmediateInvocations();
@@ -5546,6 +5547,7 @@ ExprResult Sema::BuildCXXDefaultArgExpr(SourceLocation CallLoc,
       if (Res.isInvalid())
         return ExprError();
       Init = Res.get();
+      HasRebuiltInit = true;
     }
   }
 
@@ -5555,7 +5557,8 @@ ExprResult Sema::BuildCXXDefaultArgExpr(SourceLocation CallLoc,
     return ExprError();
 
   return CXXDefaultArgExpr::Create(Context, InitializationContext->Loc, Param,
-                                   Init, InitializationContext->Context);
+                                   InitializationContext->Context, Init,
+                                   HasRebuiltInit);
 }
 
 static FieldDecl *FindFieldDeclInstantiationPattern(const ASTContext &Ctx,
@@ -5597,6 +5600,7 @@ ExprResult Sema::BuildCXXDefaultInitExpr(SourceLocation Loc, FieldDecl *Field) {
 
   bool NestedDefaultChecking = isCheckingDefaultArgumentOrInitializer();
   bool NeedRebuild = needsRebuildOfDefaultArgOrInit();
+  bool HasRebuiltInit = false;
   EnterExpressionEvaluationContext EvalContext(
       *this, ExpressionEvaluationContext::PotentiallyEvaluated, Field);
 
@@ -5658,6 +5662,7 @@ ExprResult Sema::BuildCXXDefaultInitExpr(SourceLocat...
[truncated]

Copy link

github-actions bot commented Nov 23, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Contributor

@steakhal steakhal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@yronglin
Copy link
Contributor Author

Thanks for the review! @cor3ntin Do you have any comments on the changes of CXXDefaultArgExpr and CXXDefaultInitExpr?

Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like more time to think about this change and discuss it with @AaronBallman / @erichkeane

@erichkeane
Copy link
Collaborator

I'd like more time to think about this change and discuss it with @AaronBallman / @erichkeane

I'm having some doubts too. This seems like the sort of thing we usually trace by knowing the original template, and seeing if we aren't that. So find the thing that owns this expression, and walk over to see if the primary template matches this one (though that isn't a perfect simile here).

@yronglin
Copy link
Contributor Author

When the it was used, is it possible for us to rebuild all default-arg and default-init?

@yronglin
Copy link
Contributor Author

yronglin commented Dec 4, 2024

friendly ping~

2 similar comments
@yronglin
Copy link
Contributor Author

friendly ping~

@yronglin
Copy link
Contributor Author

friendly ping~

@yronglin yronglin force-pushed the check_default_init_in_cfg branch from 6af171c to 392001e Compare December 29, 2024 15:12
@yronglin
Copy link
Contributor Author

friendly ping~

@erichkeane
Copy link
Collaborator

I don't have a great solution for this unfortuantely, but HasRebuiltInit doesn't seem right to me, it looks like we're just trying to smuggle a bit around for a few . First, it doesn't seem clear to me why the 'rebuilt-ness' of an initializer should have anything to do with anything.

Second, whether or not it was rebuilt should be a property of that expression perhaps? The init itself should know whether it needs to be built again or not.

I honestly don't think I understand the requirement well enough, but I DO think this looks/smells incorrect here. It seems to me the static analyzer should be responsible for this perhaps, and not the AST. But again, it isn't clear to me that is the case.

@cor3ntin
Copy link
Contributor

cor3ntin commented Jan 30, 2025

I think HasRebuiltInit is just CXXDefaultArgExpr::getExpr() != CXXDefaultArgExpr::getParam()->getDefaultArg()

@yronglin Can you modify hasRebuiltInit() as follow and see if anything breaks? (and if it works we don't need the bit)

bool hasRebuiltInit() const { return getExpr() != getParam()->getDefaultArg(); }`

@yronglin
Copy link
Contributor Author

@cor3ntin @erichkeane Many thanks for the review!

@yronglin Can you modify hasRebuiltInit() as follow and see if anything breaks? (and if it works we don't need the bit)

Sure, let me give a try.

It seems to me the static analyzer should be responsible for this perhaps, and not the AST.

Yeah, this function was only used in static analyzer now, maybe we can move it into static analyzer's code.

@yronglin yronglin force-pushed the check_default_init_in_cfg branch from 392001e to b5117ef Compare January 31, 2025 16:36
@yronglin
Copy link
Contributor Author

yronglin commented Jan 31, 2025

I've made changes in Sema, if the rewritten-expr same as Field->getInClassInitializer()/Param->getDefaultArg(), this PR will set {CXXDefaultArgExprBits, CXXDefaultInitExprBits}.HasRewrittenInit false. WDYT?

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm WAY happier with this. I don't have the ability to review the SA stuff well enough to give an approval, but the rest of the stuff is good. 1 Nit, else LGTM.

@yronglin
Copy link
Contributor Author

yronglin commented Feb 1, 2025

I'm WAY happier with this. I don't have the ability to review the SA stuff well enough to give an approval, but the rest of the stuff is good. 1 Nit, else LGTM.

Thanks for the review! There are no functional changes in Static Analyzer after @steakhal's +1 (just renamed a function).

Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better!
Thanks a lot for working on this

@yronglin yronglin merged commit 44aa618 into llvm:main Feb 1, 2025
9 checks passed
@zmodem
Copy link
Collaborator

zmodem commented Feb 3, 2025

We're hitting an assert after this:

clang/lib/Analysis/CFG.cpp:822: void (anonymous namespace)::CFGBuilder::appendStmt(CFGBlock *, const Stmt *): Assertion `!isa<Expr>(S) || cast<Expr>(S)->IgnoreParens() == S' failed.

See https://crbug.com/394015869 for stack trace and reproducer. I'll revert for now to unbreak the build, and start a creduce to see if we can get a smaller repro.

zmodem added a commit that referenced this pull request Feb 3, 2025
…ult init expression (#117437)"

This caused assertion failures:

  clang/lib/Analysis/CFG.cpp:822:
  void (anonymous namespace)::CFGBuilder::appendStmt(CFGBlock *, const Stmt *):
  Assertion `!isa<Expr>(S) || cast<Expr>(S)->IgnoreParens() == S' failed.

See comment on the PR.

This reverts commit 44aa618.
@zmodem
Copy link
Collaborator

zmodem commented Feb 3, 2025

creduce'd reproducer:

$ cat /tmp/a.cc
class Location {
public:
  static Location Current(int = __builtin_LINE());
};
class DOMMatrix;
class BasicMember {
public:
  BasicMember(DOMMatrix *);
};
template <typename> using Member = BasicMember;
class ExceptionState {
public:
  ExceptionState &ReturnThis();
  ExceptionState(Location);
};
class NonThrowableExceptionState : public ExceptionState {
public:
  NonThrowableExceptionState(Location location = Location::Current())
      : ExceptionState(location) {}
};
class DOMMatrix {
public:
  static DOMMatrix *
  Create(int *, ExceptionState & = (NonThrowableExceptionState().ReturnThis()));
};
class CSSMatrixComponent {
  int CSSMatrixComponent_matrix;
  CSSMatrixComponent()
      : matrix_(DOMMatrix::Create(&CSSMatrixComponent_matrix)) {}
  Member<DOMMatrix> matrix_;
};
$ clang -c -Wunreachable-code-aggressive /tmp/a.cc
clang: /work/llvm-project/clang/lib/Analysis/CFG.cpp:822: void (anonymous namespace)::CFGBuilder::appendStmt(CFGBlock *, const Stmt *): Assertion `!isa<Expr>(S) || cast<Expr>(S)->IgnoreParens() == S' failed.

@yronglin
Copy link
Contributor Author

Thanks for reporting this bug, I'll take a look.

Icohedron pushed a commit to Icohedron/llvm-project that referenced this pull request Feb 11, 2025
…ult init expression (llvm#117437)"

This caused assertion failures:

  clang/lib/Analysis/CFG.cpp:822:
  void (anonymous namespace)::CFGBuilder::appendStmt(CFGBlock *, const Stmt *):
  Assertion `!isa<Expr>(S) || cast<Expr>(S)->IgnoreParens() == S' failed.

See comment on the PR.

This reverts commit 44aa618.
yronglin added a commit that referenced this pull request Feb 16, 2025
…ault init expression" (#127338)

This PR reapply #117437.
The issue has been fixed by the 2nd commit, we need to ignore parens in
CXXDefaultArgExpr when build CFG, because CXXDefaultArgExpr::getExpr
stripped off the top level FullExpr and ConstantExpr, ParenExpr may
occurres in the top level.

---------

Signed-off-by: yronglin <[email protected]>
github-actions bot pushed a commit to arm/arm-toolchain that referenced this pull request Feb 16, 2025
…arg and default init expression" (#127338)

This PR reapply llvm/llvm-project#117437.
The issue has been fixed by the 2nd commit, we need to ignore parens in
CXXDefaultArgExpr when build CFG, because CXXDefaultArgExpr::getExpr
stripped off the top level FullExpr and ConstantExpr, ParenExpr may
occurres in the top level.

---------

Signed-off-by: yronglin <[email protected]>
sivan-shani pushed a commit to sivan-shani/llvm-project that referenced this pull request Feb 24, 2025
…ault init expression" (llvm#127338)

This PR reapply llvm#117437.
The issue has been fixed by the 2nd commit, we need to ignore parens in
CXXDefaultArgExpr when build CFG, because CXXDefaultArgExpr::getExpr
stripped off the top level FullExpr and ConstantExpr, ParenExpr may
occurres in the top level.

---------

Signed-off-by: yronglin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:analysis clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules clang:static analyzer clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[clang][diagnostics] -Wunreachable-code prints incorrect source range if a constructor has a default value set to a builtin
6 participants