NoQ updated this revision to Diff 133956.
NoQ added a comment.

- Test `const C &c = coin ? C(x, y) : C(z, w);`.
- Fix comments surrounding the assertion.


https://reviews.llvm.org/D43104

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  lib/StaticAnalyzer/Core/CallEvent.cpp
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  test/Analysis/temporaries.cpp

Index: test/Analysis/temporaries.cpp
===================================================================
--- test/Analysis/temporaries.cpp
+++ test/Analysis/temporaries.cpp
@@ -779,3 +779,116 @@
 }
 
 } // namespace test_temporary_object_expr
+
+namespace test_match_constructors_and_destructors {
+class C {
+public:
+  int &x, &y;
+  C(int &_x, int &_y) : x(_x), y(_y) { ++x; }
+  C(const C &c): x(c.x), y(c.y) { ++x; }
+  ~C() { ++y; }
+};
+
+void test_simple_temporary() {
+  int x = 0, y = 0;
+  {
+    const C &c = C(x, y);
+  }
+  // One constructor and one destructor.
+  clang_analyzer_eval(x == 1);
+  clang_analyzer_eval(y == 1);
+#ifdef TEMPORARY_DTORS
+  // expected-warning@-3{{TRUE}}
+  // expected-warning@-3{{TRUE}}
+#else
+  // expected-warning@-6{{UNKNOWN}}
+  // expected-warning@-6{{UNKNOWN}}
+#endif
+}
+
+void test_simple_temporary_with_copy() {
+  int x = 0, y = 0;
+  {
+    C c = C(x, y);
+  }
+  // Two constructors (temporary object expr and copy) and two destructors.
+  clang_analyzer_eval(x == 2);
+  clang_analyzer_eval(y == 2);
+#ifdef TEMPORARY_DTORS
+  // expected-warning@-3{{TRUE}}
+  // expected-warning@-3{{TRUE}}
+#else
+  // expected-warning@-6{{UNKNOWN}}
+  // expected-warning@-6{{UNKNOWN}}
+#endif
+}
+
+void test_ternary_temporary(int coin) {
+  int x = 0, y = 0, z = 0, w = 0;
+  {
+    const C &c = coin ? C(x, y) : C(z, w);
+  }
+  // This time each branch contains an additional elidable copy constructor.
+  if (coin) {
+    clang_analyzer_eval(x == 2);
+    clang_analyzer_eval(y == 2);
+#ifdef TEMPORARY_DTORS
+    // expected-warning@-3{{TRUE}}
+    // expected-warning@-3{{TRUE}}
+#else
+    // expected-warning@-6{{UNKNOWN}}
+    // expected-warning@-6{{UNKNOWN}}
+#endif
+    clang_analyzer_eval(z == 0); // expected-warning{{TRUE}}
+    clang_analyzer_eval(w == 0); // expected-warning{{TRUE}}
+
+  } else {
+    clang_analyzer_eval(x == 0); // expected-warning{{TRUE}}
+    clang_analyzer_eval(y == 0); // expected-warning{{TRUE}}
+    clang_analyzer_eval(z == 2);
+    clang_analyzer_eval(w == 2);
+#ifdef TEMPORARY_DTORS
+    // expected-warning@-3{{TRUE}}
+    // expected-warning@-3{{TRUE}}
+#else
+    // expected-warning@-6{{UNKNOWN}}
+    // expected-warning@-6{{UNKNOWN}}
+#endif
+  }
+}
+
+void test_ternary_temporary_with_copy(int coin) {
+  int x = 0, y = 0, z = 0, w = 0;
+  {
+    C c = coin ? C(x, y) : C(z, w);
+  }
+  // Temporary expression, elidable copy within branch,
+  // constructor for variable - 3 total.
+  if (coin) {
+    clang_analyzer_eval(x == 3);
+    clang_analyzer_eval(y == 3);
+#ifdef TEMPORARY_DTORS
+    // expected-warning@-3{{TRUE}}
+    // expected-warning@-3{{TRUE}}
+#else
+    // expected-warning@-6{{UNKNOWN}}
+    // expected-warning@-6{{UNKNOWN}}
+#endif
+    clang_analyzer_eval(z == 0); // expected-warning{{TRUE}}
+    clang_analyzer_eval(w == 0); // expected-warning{{TRUE}}
+
+  } else {
+    clang_analyzer_eval(x == 0); // expected-warning{{TRUE}}
+    clang_analyzer_eval(y == 0); // expected-warning{{TRUE}}
+    clang_analyzer_eval(z == 3);
+    clang_analyzer_eval(w == 3);
+#ifdef TEMPORARY_DTORS
+    // expected-warning@-3{{TRUE}}
+    // expected-warning@-3{{TRUE}}
+#else
+    // expected-warning@-6{{UNKNOWN}}
+    // expected-warning@-6{{UNKNOWN}}
+#endif
+  }
+}
+} // namespace test_match_constructors_and_destructors
Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -332,6 +332,8 @@
 
     // See if we have any stale C++ allocator values.
     assert(areCXXNewAllocatorValuesClear(CEEState, calleeCtx, callerCtx));
+    // See if we have any stale C++ temporary object values.
+    assert(areInitializedTemporariesClear(CEEState, calleeCtx, callerCtx));
 
     ExplodedNode *CEENode = G.getNode(Loc, CEEState, false, &isNew);
     CEENode->addPredecessor(*I, G);
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -102,14 +102,15 @@
 const MemRegion *
 ExprEngine::getRegionForConstructedObject(const CXXConstructExpr *CE,
                                           ExplodedNode *Pred,
+                                          const ConstructionContext *CC,
                                           EvalCallOptions &CallOpts) {
   const LocationContext *LCtx = Pred->getLocationContext();
   ProgramStateRef State = Pred->getState();
   MemRegionManager &MRMgr = getSValBuilder().getRegionManager();
 
   // See if we're constructing an existing region by looking at the
   // current construction context.
-  if (auto CC = getCurrentCFGElement().getAs<CFGConstructor>()) {
+  if (CC) {
     if (const Stmt *TriggerStmt = CC->getTriggerStmt()) {
       if (const CXXNewExpr *CNE = dyn_cast<CXXNewExpr>(TriggerStmt)) {
         if (AMgr.getAnalyzerOptions().mayInlineCXXAllocator()) {
@@ -221,10 +222,20 @@
   // the entire array).
 
   EvalCallOptions CallOpts;
+  auto C = getCurrentCFGElement().getAs<CFGConstructor>();
+  const ConstructionContext *CC = C ? C->getConstructionContext() : nullptr;
+
+  const CXXBindTemporaryExpr *BTE = nullptr;
 
   switch (CE->getConstructionKind()) {
   case CXXConstructExpr::CK_Complete: {
-    Target = getRegionForConstructedObject(CE, Pred, CallOpts);
+    Target = getRegionForConstructedObject(CE, Pred, CC, CallOpts);
+    if (CC && AMgr.getAnalyzerOptions().includeTemporaryDtorsInCFG() &&
+        !CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion &&
+        CallOpts.IsTemporaryCtorOrDtor) {
+      // May as well be a ReturnStmt.
+      BTE = dyn_cast<CXXBindTemporaryExpr>(CC->getTriggerStmt());
+    }
     break;
   }
   case CXXConstructExpr::CK_VirtualBase:
@@ -292,17 +303,18 @@
   ExplodedNodeSet DstPreVisit;
   getCheckerManager().runCheckersForPreStmt(DstPreVisit, Pred, CE, *this);
 
+  // FIXME: Is it possible and/or useful to do this before PreStmt?
   ExplodedNodeSet PreInitialized;
   {
     StmtNodeBuilder Bldr(DstPreVisit, PreInitialized, *currBldrCtx);
-    if (CE->requiresZeroInitialization()) {
-      // Type of the zero doesn't matter.
-      SVal ZeroVal = svalBuilder.makeZeroVal(getContext().CharTy);
-
-      for (ExplodedNodeSet::iterator I = DstPreVisit.begin(),
-                                     E = DstPreVisit.end();
-           I != E; ++I) {
-        ProgramStateRef State = (*I)->getState();
+    for (ExplodedNodeSet::iterator I = DstPreVisit.begin(),
+                                   E = DstPreVisit.end();
+         I != E; ++I) {
+      ProgramStateRef State = (*I)->getState();
+      if (CE->requiresZeroInitialization()) {
+        // Type of the zero doesn't matter.
+        SVal ZeroVal = svalBuilder.makeZeroVal(getContext().CharTy);
+
         // FIXME: Once we properly handle constructors in new-expressions, we'll
         // need to invalidate the region before setting a default value, to make
         // sure there aren't any lingering bindings around. This probably needs
@@ -316,9 +328,15 @@
         // since it's then possible to be initializing one part of a multi-
         // dimensional array.
         State = State->bindDefault(loc::MemRegionVal(Target), ZeroVal, LCtx);
-        Bldr.generateNode(CE, *I, State, /*tag=*/nullptr,
-                          ProgramPoint::PreStmtKind);
       }
+
+      if (BTE) {
+        State = addInitializedTemporary(State, BTE, LCtx,
+                                        cast<CXXTempObjectRegion>(Target));
+      }
+
+      Bldr.generateNode(CE, *I, State, /*tag=*/nullptr,
+                        ProgramPoint::PreStmtKind);
     }
   }
 
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -54,18 +54,21 @@
 STATISTIC(NumTimesRetriedWithoutInlining,
             "The # of times we re-evaluated a call without inlining");
 
-typedef std::pair<const CXXBindTemporaryExpr *, const StackFrameContext *>
-    CXXBindTemporaryContext;
+typedef llvm::ImmutableMap<std::pair<const CXXBindTemporaryExpr *,
+                                     const StackFrameContext *>,
+                           const CXXTempObjectRegion *>
+        InitializedTemporariesMap;
 
 // Keeps track of whether CXXBindTemporaryExpr nodes have been evaluated.
 // The StackFrameContext assures that nested calls due to inlined recursive
 // functions do not interfere.
-REGISTER_TRAIT_WITH_PROGRAMSTATE(InitializedTemporariesSet,
-                                 llvm::ImmutableSet<CXXBindTemporaryContext>)
+REGISTER_TRAIT_WITH_PROGRAMSTATE(InitializedTemporaries,
+                                 InitializedTemporariesMap)
 
 typedef llvm::ImmutableMap<std::pair<const CXXNewExpr *,
-                           const LocationContext *>, SVal>
-    CXXNewAllocatorValuesMap;
+                                     const LocationContext *>,
+                           SVal>
+        CXXNewAllocatorValuesMap;
 
 // Keeps track of return values of various operator new() calls between
 // evaluation of the inlined operator new(), through the constructor call,
@@ -323,6 +326,36 @@
   return State;
 }
 
+ProgramStateRef ExprEngine::addInitializedTemporary(
+    ProgramStateRef State, const CXXBindTemporaryExpr *BTE,
+    const LocationContext *LC, const CXXTempObjectRegion *R) {
+  const auto &Key = std::make_pair(BTE, LC->getCurrentStackFrame());
+  if (!State->contains<InitializedTemporaries>(Key)) {
+    return State->set<InitializedTemporaries>(Key, R);
+  }
+
+  // FIXME: Currently the state might already contain the marker due to
+  // incorrect handling of temporaries bound to default parameters; for
+  // those, we currently skip the CXXBindTemporaryExpr but rely on adding
+  // temporary destructor nodes. Otherwise, this branch should be unreachable.
+  return State;
+}
+
+bool ExprEngine::areInitializedTemporariesClear(ProgramStateRef State,
+                                                const LocationContext *FromLC,
+                                                const LocationContext *ToLC) {
+  const LocationContext *LC = FromLC;
+  do {
+    for (auto I : State->get<InitializedTemporaries>())
+      if (I.first.second == LC)
+        return false;
+
+    LC = LC->getParent();
+    assert(LC && "ToLC must be a parent of FromLC!");
+  } while (LC != ToLC);
+  return true;
+}
+
 ProgramStateRef
 ExprEngine::setCXXNewAllocatorValue(ProgramStateRef State,
                                     const CXXNewExpr *CNE,
@@ -390,11 +423,16 @@
                                                   const LocationContext *LC) {
   PrintingPolicy PP =
       LC->getAnalysisDeclContext()->getASTContext().getPrintingPolicy();
-  for (auto I : State->get<InitializedTemporariesSet>()) {
-    if (I.second != LC)
+  for (auto I : State->get<InitializedTemporaries>()) {
+    std::pair<const CXXBindTemporaryExpr *, const LocationContext *> Key =
+        I.first;
+    const MemRegion *Value = I.second;
+    if (Key.second != LC)
       continue;
-    Out << '(' << I.second << ',' << I.first << ") ";
-    I.first->printPretty(Out, nullptr, PP);
+    Out << '(' << Key.second << ',' << Key.first << ") ";
+    Key.first->printPretty(Out, nullptr, PP);
+    if (Value)
+      Out << " : " << Value;
     Out << NL;
   }
 }
@@ -422,7 +460,7 @@
                             const char *NL, const char *Sep,
                             const LocationContext *LCtx) {
   if (LCtx) {
-    if (!State->get<InitializedTemporariesSet>().isEmpty()) {
+    if (!State->get<InitializedTemporaries>().isEmpty()) {
       Out << Sep << "Initialized temporaries:" << NL;
 
       LCtx->dumpStack(Out, "", NL, Sep, [&](const LocationContext *LC) {
@@ -536,6 +574,10 @@
   const StackFrameContext *SFC = LC ? LC->getCurrentStackFrame() : nullptr;
   SymbolReaper SymReaper(SFC, ReferenceStmt, SymMgr, getStoreManager());
 
+  for (auto I : CleanedState->get<InitializedTemporaries>())
+    if (I.second)
+      SymReaper.markLive(I.second);
+
   for (auto I : CleanedState->get<CXXNewAllocatorValues>()) {
     if (SymbolRef Sym = I.second.getAsSymbol())
       SymReaper.markLive(Sym);
@@ -900,12 +942,18 @@
   ExplodedNodeSet CleanDtorState;
   StmtNodeBuilder StmtBldr(Pred, CleanDtorState, *currBldrCtx);
   ProgramStateRef State = Pred->getState();
-  if (State->contains<InitializedTemporariesSet>(
-      std::make_pair(D.getBindTemporaryExpr(), Pred->getStackFrame()))) {
+  const MemRegion *MR = nullptr;
+  if (const CXXTempObjectRegion *const *MRPtr =
+          State->get<InitializedTemporaries>(std::make_pair(
+              D.getBindTemporaryExpr(), Pred->getStackFrame()))) {
     // FIXME: Currently we insert temporary destructors for default parameters,
-    // but we don't insert the constructors.
-    State = State->remove<InitializedTemporariesSet>(
+    // but we don't insert the constructors, so the entry in
+    // InitializedTemporaries may be missing.
+    State = State->remove<InitializedTemporaries>(
         std::make_pair(D.getBindTemporaryExpr(), Pred->getStackFrame()));
+    // *MRPtr may still be null when the construction context for the temporary
+    // was not implemented.
+    MR = *MRPtr;
   }
   StmtBldr.generateNode(D.getBindTemporaryExpr(), Pred, State);
 
@@ -916,12 +964,11 @@
   ExplodedNode *CleanPred =
       CleanDtorState.empty() ? Pred : *CleanDtorState.begin();
 
-  // FIXME: Inlining of temporary destructors is not supported yet anyway, so
-  // we just put a NULL region for now. This will need to be changed later.
   EvalCallOptions CallOpts;
   CallOpts.IsTemporaryCtorOrDtor = true;
-  CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion = true;
-  VisitCXXDestructor(varType, nullptr, D.getBindTemporaryExpr(),
+  if (!MR)
+    CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion = true;
+  VisitCXXDestructor(varType, MR, D.getBindTemporaryExpr(),
                      /*IsBase=*/false, CleanPred, Dst, CallOpts);
 }
 
@@ -932,7 +979,7 @@
                                                const CFGBlock *DstT,
                                                const CFGBlock *DstF) {
   BranchNodeBuilder TempDtorBuilder(Pred, Dst, BldCtx, DstT, DstF);
-  if (Pred->getState()->contains<InitializedTemporariesSet>(
+  if (Pred->getState()->contains<InitializedTemporaries>(
           std::make_pair(BTE, Pred->getStackFrame()))) {
     TempDtorBuilder.markInfeasible(false);
     TempDtorBuilder.generateNode(Pred->getState(), true, Pred);
@@ -942,32 +989,6 @@
   }
 }
 
-void ExprEngine::VisitCXXBindTemporaryExpr(const CXXBindTemporaryExpr *BTE,
-                                           ExplodedNodeSet &PreVisit,
-                                           ExplodedNodeSet &Dst) {
-  if (!getAnalysisManager().options.includeTemporaryDtorsInCFG()) {
-    // In case we don't have temporary destructors in the CFG, do not mark
-    // the initialization - we would otherwise never clean it up.
-    Dst = PreVisit;
-    return;
-  }
-  StmtNodeBuilder StmtBldr(PreVisit, Dst, *currBldrCtx);
-  for (ExplodedNode *Node : PreVisit) {
-    ProgramStateRef State = Node->getState();
-
-    if (!State->contains<InitializedTemporariesSet>(
-            std::make_pair(BTE, Node->getStackFrame()))) {
-      // FIXME: Currently the state might already contain the marker due to
-      // incorrect handling of temporaries bound to default parameters; for
-      // those, we currently skip the CXXBindTemporaryExpr but rely on adding
-      // temporary destructor nodes.
-      State = State->add<InitializedTemporariesSet>(
-          std::make_pair(BTE, Node->getStackFrame()));
-    }
-    StmtBldr.generateNode(BTE, Node, State);
-  }
-}
-
 namespace {
 class CollectReachableSymbolsCallback final : public SymbolVisitor {
   InvalidatedSymbols Symbols;
@@ -1130,9 +1151,7 @@
       Bldr.takeNodes(Pred);
       ExplodedNodeSet PreVisit;
       getCheckerManager().runCheckersForPreStmt(PreVisit, Pred, S, *this);
-      ExplodedNodeSet Next;
-      VisitCXXBindTemporaryExpr(cast<CXXBindTemporaryExpr>(S), PreVisit, Next);
-      getCheckerManager().runCheckersForPostStmt(Dst, Next, S, *this);
+      getCheckerManager().runCheckersForPostStmt(Dst, PreVisit, S, *this);
       Bldr.addNodes(Dst);
       break;
     }
Index: lib/StaticAnalyzer/Core/CallEvent.cpp
===================================================================
--- lib/StaticAnalyzer/Core/CallEvent.cpp
+++ lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -1197,9 +1197,8 @@
   // destructors, though this could change in the future.
   const CFGBlock *B = CalleeCtx->getCallSiteBlock();
   CFGElement E = (*B)[CalleeCtx->getIndex()];
-  assert(E.getAs<CFGImplicitDtor>() &&
+  assert(E.getAs<CFGImplicitDtor>() || E.getAs<CFGTemporaryDtor>() &&
          "All other CFG elements should have exprs");
-  assert(!E.getAs<CFGTemporaryDtor>() && "We don't handle temporaries yet");
 
   SValBuilder &SVB = State->getStateManager().getSValBuilder();
   const CXXDestructorDecl *Dtor = cast<CXXDestructorDecl>(CalleeCtx->getDecl());
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===================================================================
--- include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -448,10 +448,6 @@
                                        ExplodedNode *Pred,
                                        ExplodedNodeSet &Dst);
 
-  void VisitCXXBindTemporaryExpr(const CXXBindTemporaryExpr *BTE,
-                                 ExplodedNodeSet &PreVisit,
-                                 ExplodedNodeSet &Dst);
-
   void VisitCXXCatchStmt(const CXXCatchStmt *CS, ExplodedNode *Pred,
                          ExplodedNodeSet &Dst);
 
@@ -696,8 +692,22 @@
   /// IsConstructorWithImproperlyModeledTargetRegion flag is set in \p CallOpts.
   const MemRegion *getRegionForConstructedObject(const CXXConstructExpr *CE,
                                                  ExplodedNode *Pred,
+                                                 const ConstructionContext *CC,
                                                  EvalCallOptions &CallOpts);
 
+  /// Store the region of a C++ temporary object corresponding to a
+  /// CXXBindTemporaryExpr for later destruction.
+  static ProgramStateRef addInitializedTemporary(
+      ProgramStateRef State, const CXXBindTemporaryExpr *BTE,
+      const LocationContext *LC, const CXXTempObjectRegion *R);
+
+  /// Check if all initialized temporary regions are clear for the given
+  /// context range (including FromLC, not including ToLC).
+  /// This is useful for assertions.
+  static bool areInitializedTemporariesClear(ProgramStateRef State,
+                                             const LocationContext *FromLC,
+                                             const LocationContext *ToLC);
+
   /// Store the region returned by operator new() so that the constructor
   /// that follows it knew what location to initialize. The value should be
   /// cleared once the respective CXXNewExpr CFGStmt element is processed.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to