llvmbot wrote:

<!--LLVM PR SUMMARY COMMENT-->

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

Author: None (yronglin)

<details>
<summary>Changes</summary>

Depends on https://github.com/llvm/llvm-project/pull/87933

Clang now support the following:
- Extending lifetime of object bound to reference members of aggregates, that 
are created from default member initializer. 
- Rebuild `CXXDefaultArgExpr` and `CXXDefaultInitExpr` as needed where called 
or constructed.

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

This PR add `CXXDefaultArgExpr` and `CXXDefaultInitExpr` into CFG, and correct 
handle these expressions in ExprEngine

---
Full diff: https://github.com/llvm/llvm-project/pull/91879.diff


6 Files Affected:

- (modified) clang/lib/AST/ParentMap.cpp (+16) 
- (modified) clang/lib/Analysis/CFG.cpp (+41-9) 
- (modified) clang/lib/Sema/SemaExpr.cpp (+3-1) 
- (modified) clang/lib/StaticAnalyzer/Core/ExprEngine.cpp (+40-24) 
- (modified) clang/test/Analysis/cxx-uninitialized-object.cpp (+7-6) 
- (modified) clang/test/Analysis/lifetime-extended-regions.cpp (+3-4) 


``````````diff
diff --git a/clang/lib/AST/ParentMap.cpp b/clang/lib/AST/ParentMap.cpp
index 3d6a1cc84c7b1..534793b837bbb 100644
--- a/clang/lib/AST/ParentMap.cpp
+++ b/clang/lib/AST/ParentMap.cpp
@@ -97,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->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 64e6155de090c..02317257c2740 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);
@@ -2254,16 +2258,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);
@@ -2433,6 +2431,40 @@ 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 bb4b116fd73ca..eed328c716382 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -5813,6 +5813,7 @@ ExprResult Sema::BuildCXXDefaultInitExpr(SourceLocation 
Loc, FieldDecl *Field) {
     InitializationContext.emplace(Loc, Field, CurContext);
 
   Expr *Init = nullptr;
+  bool HasRewrittenInit = false;
 
   bool NestedDefaultChecking = isCheckingDefaultArgumentOrInitializer();
   bool InLifetimeExtendingContext = isInLifetimeExtendingContext();
@@ -5862,6 +5863,7 @@ 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 =
@@ -5905,7 +5907,7 @@ ExprResult Sema::BuildCXXDefaultInitExpr(SourceLocation 
Loc, FieldDecl *Field) {
 
     return CXXDefaultInitExpr::Create(Context, InitializationContext->Loc,
                                       Field, InitializationContext->Context,
-                                      Init);
+                                      HasRewrittenInit ? Init : nullptr);
   }
 
   // DR1351:
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp 
b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index 0b1edf3e5c96b..525239101471e 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1964,39 +1964,55 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode 
*Pred,
     case Stmt::CXXDefaultArgExprClass:
     case Stmt::CXXDefaultInitExprClass: {
       Bldr.takeNodes(Pred);
-      ExplodedNodeSet PreVisit;
-      getCheckerManager().runCheckersForPreStmt(PreVisit, Pred, S, *this);
+      ExplodedNodeSet CheckedSet;
+      getCheckerManager().runCheckersForPreStmt(CheckedSet, Pred, S, *this);
 
       ExplodedNodeSet Tmp;
-      StmtNodeBuilder Bldr2(PreVisit, Tmp, *currBldrCtx);
+      StmtNodeBuilder Bldr2(CheckedSet, Tmp, *currBldrCtx);
 
-      const Expr *ArgE;
-      if (const auto *DefE = dyn_cast<CXXDefaultArgExpr>(S))
+      bool HasRewrittenInit = false;
+      const Expr *ArgE = nullptr;
+      if (const auto *DefE = dyn_cast<CXXDefaultArgExpr>(S)) {
         ArgE = DefE->getExpr();
-      else if (const auto *DefE = dyn_cast<CXXDefaultInitExpr>(S))
+        HasRewrittenInit = DefE->hasRewrittenInit();
+      } else if (const auto *DefE = dyn_cast<CXXDefaultInitExpr>(S)) {
         ArgE = DefE->getExpr();
-      else
+        HasRewrittenInit = DefE->hasRewrittenInit();
+      } else
         llvm_unreachable("unknown constant wrapper kind");
 
-      bool IsTemporary = false;
-      if (const auto *MTE = dyn_cast<MaterializeTemporaryExpr>(ArgE)) {
-        ArgE = MTE->getSubExpr();
-        IsTemporary = true;
-      }
+      if (HasRewrittenInit) {
+        for (auto *I : CheckedSet) {
+          ProgramStateRef state = (*I).getState();
+          const LocationContext *LCtx = (*I).getLocationContext();
+          SVal Val = state->getSVal(ArgE, LCtx);
+          state = state->BindExpr(S, LCtx, Val);
+          Bldr2.generateNode(S, I, 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);
+        if (!ConstantVal)
+          ConstantVal = UnknownVal();
 
-      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),
+        const LocationContext *LCtx = Pred->getLocationContext();
+        for (auto *I : CheckedSet) {
+          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);
+
+          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 e3fa8ae8d7f29..c62fd1c312056 100644
--- a/clang/test/Analysis/cxx-uninitialized-object.cpp
+++ b/clang/test/Analysis/cxx-uninitialized-object.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -std=c++14 -verify  %s \
+// RUN: %clang_analyze_cc1 -std=c++14 -verify=expected,pedantic  %s \
 // RUN:   -analyzer-checker=core \
 // RUN:   -analyzer-checker=optin.cplusplus.UninitializedObject \
 // RUN:   -analyzer-config optin.cplusplus.UninitializedObject:Pedantic=true 
-DPEDANTIC \
@@ -1114,17 +1114,16 @@ void fCXX11MemberInitTest1() {
   CXX11MemberInitTest1();
 }
 
+#ifdef PEDANTIC
 struct CXX11MemberInitTest2 {
   struct RecordType {
-    // 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
+    int a; // expected-note {{uninitialized field 'this->a'}}
+    int b; // expected-note {{uninitialized field 'this->b'}}
 
     RecordType(int) {}
   };
 
-  RecordType rec = RecordType(int());
+  RecordType rec = RecordType(int()); // expected-warning {{2 uninitialized 
fields at the end of the constructor call}}
   int dontGetFilteredByNonPedanticMode = 0;
 
   CXX11MemberInitTest2() {}
@@ -1135,6 +1134,8 @@ void fCXX11MemberInitTest2() {
   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 4458ad294af7c..524f4e0c400d1 100644
--- a/clang/test/Analysis/lifetime-extended-regions.cpp
+++ b/clang/test/Analysis/lifetime-extended-regions.cpp
@@ -121,11 +121,10 @@ 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]+}}} }}
   
-  // 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]+}}} }}
+  // The lifetime lifetime of object bound to reference members of aggregates,
+  // that are created from default member initializer was extended.
   RefAggregate defaultInitExtended{i};
-  clang_analyzer_dump(defaultInitExtended.ry); // expected-warning {{Unknown }}
+  clang_analyzer_dump(defaultInitExtended.ry); // expected-warning-re 
{{&lifetime_extended_object{Composite, defaultInitExtended, S{{[0-9]+}}} }}
 }
 
 void lambda() {

``````````

</details>


https://github.com/llvm/llvm-project/pull/91879
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to