Revert "Reapply "[Analyzer][CFG] Correctly handle rebuilt default arg and default init expression" (#127338)" (#128205)

yronglin · web-flow · commit 7c7fb9442f9f · 2025-02-22T01:19:01.000+08:00
This reverts commit d235b72.
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
@@ -284,10 +284,6 @@ Code Completion
 Static Analyzer
 ---------------
 
-- Clang currently support extending lifetime of object bound to
-  reference members of aggregates in CFG and ExprEngine, that are
-  created from default member initializer.
-
 New features
 ^^^^^^^^^^^^
 
diff --git a/clang/lib/AST/ParentMap.cpp b/clang/lib/AST/ParentMap.cpp
@@ -13,7 +13,6 @@
 #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"
 
@@ -104,22 +103,6 @@ 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
@@ -556,10 +556,6 @@ 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);
@@ -2267,10 +2263,16 @@ CFGBlock *CFGBuilder::Visit(Stmt * S, AddStmtChoice asc,
                                    asc, ExternallyDestructed);
 
     case Stmt::CXXDefaultArgExprClass:
-      return VisitCXXDefaultArgExpr(cast<CXXDefaultArgExpr>(S), asc);
-
     case Stmt::CXXDefaultInitExprClass:
-      return VisitCXXDefaultInitExpr(cast<CXXDefaultInitExpr>(S), asc);
+      // 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);
 
     case Stmt::CXXBindTemporaryExprClass:
       return VisitCXXBindTemporaryExpr(cast<CXXBindTemporaryExpr>(S), asc);
@@ -2440,44 +2442,6 @@ CFGBlock *CFGBuilder::VisitChildren(Stmt *S) {
   return B;
 }
 
-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 stripped off the top level FullExpr and
-    // ConstantExpr, CXXDefaultInitExpr::getExpr does not do this, so we don't
-    // need to ignore ParenExprs, because the top level will not be a ParenExpr.
-    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
@@ -454,12 +454,11 @@ bool DeadCodeScan::isDeadCodeRoot(const clang::CFGBlock *Block) {
   return isDeadRoot;
 }
 
-// Check if the given `DeadStmt` is one of target statements or is a sub-stmt of
-// them. `Block` is the CFGBlock containing the `DeadStmt`.
-template <class... Ts>
-static bool isDeadStmtInOneOf(const Stmt *DeadStmt, const CFGBlock *Block) {
+// 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 *TargetStmt = nullptr;
+  const Stmt *CoroStmt = 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;
@@ -468,27 +467,32 @@ static bool isDeadStmtInOneOf(const Stmt *DeadStmt, const CFGBlock *Block) {
       const Stmt *S = CS->getStmt();
       if (S == DeadStmt)
         AfterDeadStmt = true;
-      if (AfterDeadStmt && llvm::isa<Ts...>(S)) {
-        TargetStmt = S;
+      if (AfterDeadStmt &&
+          // For simplicity, we only check simple coroutine statements.
+          (llvm::isa<CoreturnStmt>(S) || llvm::isa<CoroutineSuspendExpr>(S))) {
+        CoroStmt = S;
         break;
       }
     }
-  if (!TargetStmt)
+  if (!CoroStmt)
     return false;
   struct Checker : DynamicRecursiveASTVisitor {
     const Stmt *DeadStmt;
-    bool IsSubStmtOfTargetStmt = false;
-    Checker(const Stmt *S) : DeadStmt(S) { ShouldVisitImplicitCode = true; }
+    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)
-        IsSubStmtOfTargetStmt = true;
+        CoroutineSubStmt = true;
       return true;
     }
   };
   Checker checker(DeadStmt);
-  checker.TraverseStmt(const_cast<Stmt *>(TargetStmt));
-  return checker.IsSubStmtOfTargetStmt;
+  checker.TraverseStmt(const_cast<Stmt *>(CoroStmt));
+  return checker.CoroutineSubStmt;
 }
 
 static bool isValidDeadStmt(const Stmt *S, const clang::CFGBlock *Block) {
@@ -499,12 +503,7 @@ 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.
-  //
-  // 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 !isDeadStmtInOneOf<CoreturnStmt, CoroutineSuspendExpr,
-                            CXXDefaultArgExpr, CXXDefaultInitExpr>(S, Block);
+  return !isInCoroutineStmt(S, Block);
 }
 
 const Stmt *DeadCodeScan::findDeadCode(const clang::CFGBlock *Block) {
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
@@ -5571,10 +5571,8 @@ ExprResult Sema::BuildCXXDefaultArgExpr(SourceLocation CallLoc,
           /*SkipImmediateInvocations=*/NestedDefaultChecking))
     return ExprError();
 
-  Expr *RewrittenExpr = (Init == Param->getDefaultArg() ? nullptr : Init);
   return CXXDefaultArgExpr::Create(Context, InitializationContext->Loc, Param,
-                                   RewrittenExpr,
-                                   InitializationContext->Context);
+                                   Init, InitializationContext->Context);
 }
 
 static FieldDecl *FindFieldDeclInstantiationPattern(const ASTContext &Ctx,
@@ -5692,11 +5690,10 @@ ExprResult Sema::BuildCXXDefaultInitExpr(SourceLocation Loc, FieldDecl *Field) {
       return ExprError();
     }
     Init = Res.get();
-    Expr *RewrittenInit =
-        (Init == Field->getInClassInitializer() ? nullptr : Init);
+
     return CXXDefaultInitExpr::Create(Context, InitializationContext->Loc,
                                       Field, InitializationContext->Context,
-                                      RewrittenInit);
+                                      Init);
   }
 
   // DR1351:
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1990,45 +1990,33 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred,
       ExplodedNodeSet Tmp;
       StmtNodeBuilder Bldr2(PreVisit, Tmp, *currBldrCtx);
 
-      bool HasRebuiltInit = false;
-      const Expr *ArgE = nullptr;
-      if (const auto *DefE = dyn_cast<CXXDefaultArgExpr>(S)) {
+      const Expr *ArgE;
+      if (const auto *DefE = dyn_cast<CXXDefaultArgExpr>(S))
         ArgE = DefE->getExpr();
-        HasRebuiltInit = DefE->hasRewrittenInit();
-      } else if (const auto *DefE = dyn_cast<CXXDefaultInitExpr>(S)) {
+      else if (const auto *DefE = dyn_cast<CXXDefaultInitExpr>(S))
         ArgE = DefE->getExpr();
-        HasRebuiltInit = DefE->hasRewrittenInit();
-      } else
+      else
         llvm_unreachable("unknown constant wrapper kind");
 
-      if (HasRebuiltInit) {
-        for (auto *N : PreVisit) {
-          ProgramStateRef state = N->getState();
-          const LocationContext *LCtx = N->getLocationContext();
-          state = state->BindExpr(S, LCtx, state->getSVal(ArgE, LCtx));
-          Bldr2.generateNode(S, N, state);
-        }
-      } else {
-        // If it's not rewritten, the contents of these expressions are not
-        // actually part of the current function, so we fall back to constant
-        // evaluation.
-        bool IsTemporary = false;
-        if (const auto *MTE = dyn_cast<MaterializeTemporaryExpr>(ArgE)) {
-          ArgE = MTE->getSubExpr();
-          IsTemporary = true;
-        }
-
-        std::optional<SVal> ConstantVal = svalBuilder.getConstantVal(ArgE);
-        const LocationContext *LCtx = Pred->getLocationContext();
-        for (auto *I : PreVisit) {
-          ProgramStateRef State = I->getState();
-          State = State->BindExpr(S, LCtx, ConstantVal.value_or(UnknownVal()));
-          if (IsTemporary)
-            State = createTemporaryRegionIfNeeded(State, LCtx, cast<Expr>(S),
-                                                  cast<Expr>(S));
+      bool IsTemporary = false;
+      if (const auto *MTE = dyn_cast<MaterializeTemporaryExpr>(ArgE)) {
+        ArgE = MTE->getSubExpr();
+        IsTemporary = true;
+      }
 
-          Bldr2.generateNode(S, I, State);
-        }
+      std::optional<SVal> ConstantVal = svalBuilder.getConstantVal(ArgE);
+      if (!ConstantVal)
+        ConstantVal = UnknownVal();
+
+      const LocationContext *LCtx = Pred->getLocationContext();
+      for (const auto I : PreVisit) {
+        ProgramStateRef State = I->getState();
+        State = State->BindExpr(S, LCtx, *ConstantVal);
+        if (IsTemporary)
+          State = createTemporaryRegionIfNeeded(State, LCtx,
+                                                cast<Expr>(S),
+                                                cast<Expr>(S));
+        Bldr2.generateNode(S, I, State);
       }
 
       getCheckerManager().runCheckersForPostStmt(Dst, Tmp, S, *this);
diff --git a/clang/test/AST/ast-dump-recovery.cpp b/clang/test/AST/ast-dump-recovery.cpp
@@ -507,7 +507,7 @@ union U {
 // CHECK-NEXT:      `-DeclStmt {{.*}}
 // CHECK-NEXT:        `-VarDecl {{.*}} g 'U':'GH112560::U' listinit
 // CHECK-NEXT:          `-InitListExpr {{.*}} 'U':'GH112560::U' contains-errors field Field {{.*}} 'f' 'int'
-// CHECK-NEXT:            `-CXXDefaultInitExpr {{.*}} 'int' contains-errors
+// CHECK-NEXT:            `-CXXDefaultInitExpr {{.*}} 'int' contains-errors has rewritten init
 // CHECK-NEXT:              `-RecoveryExpr {{.*}} 'int' contains-errors
 // DISABLED-NOT: -RecoveryExpr {{.*}} contains-errors
 void foo() {
diff --git a/clang/test/Analysis/lifetime-extended-regions.cpp b/clang/test/Analysis/lifetime-extended-regions.cpp
@@ -121,10 +121,11 @@ void aggregateWithReferences() {
   clang_analyzer_dump(viaReference.rx); // expected-warning-re {{&lifetime_extended_object{int, viaReference, S{{[0-9]+}}} }}
   clang_analyzer_dump(viaReference.ry); // expected-warning-re {{&lifetime_extended_object{Composite, viaReference, S{{[0-9]+}}} }}
   
-  // The lifetime of object bound to reference members of aggregates,
-  // that are created from default member initializer was extended.
+  // FIXME: clang currently support extending lifetime of object bound to reference members of aggregates,
+  // that are created from default member initializer. But CFG and ExprEngine need to be updated to address this change.
+  // The following expect warning: {{&lifetime_extended_object{Composite, defaultInitExtended, S{{[0-9]+}}} }}
   RefAggregate defaultInitExtended{i};
-  clang_analyzer_dump(defaultInitExtended.ry); // expected-warning-re {{&lifetime_extended_object{Composite, defaultInitExtended, S{{[0-9]+}}} }}
+  clang_analyzer_dump(defaultInitExtended.ry); // expected-warning {{Unknown }}
 }
 
 void lambda() {
diff --git a/clang/test/SemaCXX/cxx2c-placeholder-vars.cpp b/clang/test/SemaCXX/cxx2c-placeholder-vars.cpp
@@ -274,16 +274,16 @@ void f() {
 // CHECK: ClassTemplateSpecializationDecl {{.*}} struct A definition
 // CHECK: CXXConstructorDecl {{.*}} implicit used constexpr A 'void () noexcept'
 // CHECK-NEXT: CXXCtorInitializer Field {{.*}} '_' 'int'
-// CHECK-NEXT: CXXDefaultInitExpr {{.*}} 'int'
+// CHECK-NEXT: CXXDefaultInitExpr {{.*}} 'int' has rewritten init
 // CHECK-NEXT: IntegerLiteral {{.*}} 'int' 1
 // CHECK-NEXT: CXXCtorInitializer Field {{.*}} '_' 'int'
-// CHECK-NEXT: CXXDefaultInitExpr {{.*}} 'int'
+// CHECK-NEXT: CXXDefaultInitExpr {{.*}} 'int' has rewritten init
 // CHECK-NEXT: IntegerLiteral {{.*}} 'int' 2
 // CHECK-NEXT: CXXCtorInitializer Field {{.*}} 'a' 'int'
-// CHECK-NEXT: CXXDefaultInitExpr {{.*}} 'int'
+// CHECK-NEXT: CXXDefaultInitExpr {{.*}} 'int' has rewritten init
 // CHECK-NEXT: IntegerLiteral {{.*}} 'int' 3
 // CHECK-NEXT: CXXCtorInitializer Field {{.*}} '_' 'int'
-// CHECK-NEXT: CXXDefaultInitExpr {{.*}} 'int'
+// CHECK-NEXT: CXXDefaultInitExpr {{.*}} 'int' has rewritten init
 // CHECK-NEXT: IntegerLiteral {{.*}} 'int' 4
 // CHECK-NEXT: CompoundStmt {{.*}}
 
diff --git a/clang/test/SemaCXX/warn-unreachable.cpp b/clang/test/SemaCXX/warn-unreachable.cpp
