Revert "[Analyzer][CFG] Correctly handle rebuilt default arg and default 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 44aa618ef67d302f5ab77cc591fb3434fe967a2e.

2 files changed, 27 insertions, 60 deletions

diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index 6bba0e3..304bbb2 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -556,10 +556,6 @@ public:
 
 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);
@@ -2265,10 +2261,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);
@@ -2438,40 +2440,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(), 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);
-    }
-    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 3b1f716..dd81c8e 100644
--- 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) {