[clang] 86295dc - Revert "[Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (#91879)" (#94597)

via cfe-commits cfe-commits at lists.llvm.org
Thu Jun 6 02:46:37 PDT 2024


Author: bgra8
Date: 2024-06-06T11:46:33+02:00
New Revision: 86295dc197db2f08f4eb582ed1026a8f74ac3338

URL: https://github.com/llvm/llvm-project/commit/86295dc197db2f08f4eb582ed1026a8f74ac3338
DIFF: https://github.com/llvm/llvm-project/commit/86295dc197db2f08f4eb582ed1026a8f74ac3338.diff

LOG: Revert "[Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (#91879)" (#94597)

This depends on https://github.com/llvm/llvm-project/pull/92527 which
needs to be reverted due to
https://github.com/llvm/llvm-project/pull/92527#issuecomment-2149120420.

This reverts commit 905b402a5d8f1490d668f40942390ebd6e87aa8f.

Co-authored-by: Bogdan Graur <bgraur at google.com>

Added: 
    

Modified: 
    clang/lib/AST/ParentMap.cpp
    clang/lib/Analysis/CFG.cpp
    clang/lib/Sema/SemaExpr.cpp
    clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
    clang/test/Analysis/cxx-uninitialized-object.cpp
    clang/test/Analysis/lifetime-extended-regions.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/AST/ParentMap.cpp b/clang/lib/AST/ParentMap.cpp
index 534793b837bbb..3d6a1cc84c7b1 100644
--- a/clang/lib/AST/ParentMap.cpp
+++ b/clang/lib/AST/ParentMap.cpp
@@ -97,22 +97,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
index 02317257c2740..64e6155de090c 100644
--- 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);
@@ -2258,10 +2254,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);
@@ -2431,40 +2433,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/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index fb5ca199b3fc6..d8c77e3e0a5cd 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -5608,7 +5608,6 @@ ExprResult Sema::BuildCXXDefaultInitExpr(SourceLocation Loc, FieldDecl *Field) {
     InitializationContext.emplace(Loc, Field, CurContext);
 
   Expr *Init = nullptr;
-  bool HasRewrittenInit = false;
 
   bool NestedDefaultChecking = isCheckingDefaultArgumentOrInitializer();
   bool InLifetimeExtendingContext = isInLifetimeExtendingContext();
@@ -5658,7 +5657,6 @@ ExprResult Sema::BuildCXXDefaultInitExpr(SourceLocation Loc, FieldDecl *Field) {
       isa_and_present<ExprWithCleanups>(Field->getInClassInitializer());
   if (V.HasImmediateCalls || InLifetimeExtendingContext ||
       ContainsAnyTemporaries) {
-    HasRewrittenInit = true;
     ExprEvalContexts.back().DelayedDefaultInitializationContext = {Loc, Field,
                                                                    CurContext};
     ExprEvalContexts.back().IsCurrentlyCheckingDefaultArgumentOrInitializer =
@@ -5702,7 +5700,7 @@ ExprResult Sema::BuildCXXDefaultInitExpr(SourceLocation Loc, FieldDecl *Field) {
 
     return CXXDefaultInitExpr::Create(Context, InitializationContext->Loc,
                                       Field, InitializationContext->Context,
-                                      HasRewrittenInit ? Init : nullptr);
+                                      Init);
   }
 
   // DR1351:

diff  --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index 290d96611d466..197d673107285 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1971,45 +1971,33 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred,
       ExplodedNodeSet Tmp;
       StmtNodeBuilder Bldr2(PreVisit, Tmp, *currBldrCtx);
 
-      bool HasRewrittenInit = 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();
-        HasRewrittenInit = DefE->hasRewrittenInit();
-      } else if (const auto *DefE = dyn_cast<CXXDefaultInitExpr>(S)) {
+      else if (const auto *DefE = dyn_cast<CXXDefaultInitExpr>(S))
         ArgE = DefE->getExpr();
-        HasRewrittenInit = DefE->hasRewrittenInit();
-      } else
+      else
         llvm_unreachable("unknown constant wrapper kind");
 
-      if (HasRewrittenInit) {
-        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/Analysis/cxx-uninitialized-object.cpp b/clang/test/Analysis/cxx-uninitialized-object.cpp
index aee0dae15fbfa..e3fa8ae8d7f29 100644
--- a/clang/test/Analysis/cxx-uninitialized-object.cpp
+++ b/clang/test/Analysis/cxx-uninitialized-object.cpp
@@ -1114,27 +1114,27 @@ void fCXX11MemberInitTest1() {
   CXX11MemberInitTest1();
 }
 
-#ifdef PEDANTIC
 struct CXX11MemberInitTest2 {
   struct RecordType {
-    int a; // expected-note {{uninitialized field 'this->a'}}
-    int b; // expected-note {{uninitialized field 'this->b'}}
+    // TODO: we'd expect the note: {{uninitialized field 'this->rec.a'}}
+    int a; // no-note
+    // TODO: we'd expect the note: {{uninitialized field 'this->rec.b'}}
+    int b; // no-note
 
     RecordType(int) {}
   };
 
-  RecordType rec = RecordType(int()); // expected-warning {{2 uninitialized fields}}
+  RecordType rec = RecordType(int());
   int dontGetFilteredByNonPedanticMode = 0;
 
   CXX11MemberInitTest2() {}
 };
 
 void fCXX11MemberInitTest2() {
+  // TODO: we'd expect the warning: {{2 uninitializeds field}}
   CXX11MemberInitTest2(); // no-warning
 }
 
-#endif // PEDANTIC
-
 //===----------------------------------------------------------------------===//
 // "Esoteric" primitive type tests.
 //===----------------------------------------------------------------------===//

diff  --git a/clang/test/Analysis/lifetime-extended-regions.cpp b/clang/test/Analysis/lifetime-extended-regions.cpp
index 524f4e0c400d1..4458ad294af7c 100644
--- 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 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() {


        


More information about the cfe-commits mailing list