[analyzer] Track temporaries without construction contexts for destruction.

Sometimes it is not known at compile time which temporary objects will be
constructed, eg. 'x ? A() : B()' or 'C() || D()'. In this case we track which
temporary was constructed to know how to properly call the destructor.

Once the construction context for temporaries was introduced, we moved the
tracking code to the code that investigates the construction context.

Bring back the old mechanism because construction contexts are not always
available yet - eg. in the case where a temporary is constructed without a
constructor expression, eg. returned from a function by value. The mechanism
should still go away eventually.

Additionally, fix a bug in the temporary cleanup code for the case when
construction contexts are not available, which could lead to temporaries
staying in the program state and increasing memory consumption.

Differential Revision: https://reviews.llvm.org/D43666

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@326246 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h b/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
index f332132a0ed1ca29e9f81b6e2d71c15561a36715..a552118eb617aa895ec07fe6570f76ada9f1d4a0 100644 (file)
--- a/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ b/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -452,6 +452,10 @@ public:
                                        ExplodedNode *Pred,
                                        ExplodedNodeSet &Dst);
 
+  void VisitCXXBindTemporaryExpr(const CXXBindTemporaryExpr *BTE,
+                                 ExplodedNodeSet &PreVisit,
+                                 ExplodedNodeSet &Dst);
+
   void VisitCXXCatchStmt(const CXXCatchStmt *CS, ExplodedNode *Pred,
                          ExplodedNodeSet &Dst);
 

diff --git a/lib/StaticAnalyzer/Core/ExprEngine.cpp b/lib/StaticAnalyzer/Core/ExprEngine.cpp
index 9f71b2fc03a5bc91c3cc22b4d71069ab5f45b029..406837fa3d2a818a62d01378001d2c27cd94de73 100644 (file)
--- a/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1089,6 +1089,34 @@ void ExprEngine::processCleanupTemporaryBranch(const CXXBindTemporaryExpr *BTE,
   }
 }
 
+void ExprEngine::VisitCXXBindTemporaryExpr(const CXXBindTemporaryExpr *BTE,
+                                           ExplodedNodeSet &PreVisit,
+                                           ExplodedNodeSet &Dst) {
+  // This is a fallback solution in case we didn't have a construction
+  // context when we were constructing the temporary. Otherwise the map should
+  // have been populated there.
+  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();
+               const auto &Key = std::make_pair(BTE, Node->getStackFrame());
+
+    if (!State->contains<InitializedTemporaries>(Key)) {
+      // FIXME: Currently the state might also 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->set<InitializedTemporaries>(Key, nullptr);
+    }
+    StmtBldr.generateNode(BTE, Node, State);
+  }
+}
+
 namespace {
 class CollectReachableSymbolsCallback final : public SymbolVisitor {
   InvalidatedSymbols Symbols;
@@ -1251,7 +1279,9 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred,
       Bldr.takeNodes(Pred);
       ExplodedNodeSet PreVisit;
       getCheckerManager().runCheckersForPreStmt(PreVisit, Pred, S, *this);
-      getCheckerManager().runCheckersForPostStmt(Dst, PreVisit, S, *this);
+      ExplodedNodeSet Next;
+      VisitCXXBindTemporaryExpr(cast<CXXBindTemporaryExpr>(S), PreVisit, Next);
+      getCheckerManager().runCheckersForPostStmt(Dst, Next, S, *this);
       Bldr.addNodes(Dst);
       break;
     }
@@ -2194,13 +2224,13 @@ void ExprEngine::processEndOfFunction(NodeBuilderContext& BC,
                                        Pred->getLocationContext(),
                                        Pred->getStackFrame()->getParent()));
 
-  // FIXME: We currently assert that temporaries are clear, as lifetime extended
-  // temporaries are not always modelled correctly. In some cases when we
-  // materialize the temporary, we do createTemporaryRegionIfNeeded(), and
-  // the region changes, and also the respective destructor becomes automatic
-  // from temporary. So for now clean up the state manually before asserting.
-  // Ideally, the code above the assertion should go away, but the assertion
-  // should remain.
+  // FIXME: We currently cannot assert that temporaries are clear, because
+  // lifetime extended temporaries are not always modelled correctly. In some
+  // cases when we materialize the temporary, we do
+  // createTemporaryRegionIfNeeded(), and the region changes, and also the
+  // respective destructor becomes automatic from temporary. So for now clean up
+  // the state manually before asserting. Ideally, the code above the assertion
+  // should go away, but the assertion should remain.
   {
     ExplodedNodeSet CleanUpTemporaries;
     NodeBuilder Bldr(Pred, CleanUpTemporaries, BC);
@@ -2217,9 +2247,12 @@ void ExprEngine::processEndOfFunction(NodeBuilderContext& BC,
       LC = LC->getParent();
     }
     if (State != Pred->getState()) {
-      Bldr.generateNode(Pred->getLocation(), State, Pred);
-      assert(CleanUpTemporaries.size() <= 1);
-      Pred = CleanUpTemporaries.empty() ? Pred : *CleanUpTemporaries.begin();
+      Pred = Bldr.generateNode(Pred->getLocation(), State, Pred);
+      if (!Pred) {
+        // The node with clean temporaries already exists. We might have reached
+        // it on a path on which we initialize different temporaries.
+        return;
+      }
     }
   }
   assert(areInitializedTemporariesClear(Pred->getState(),

diff --git a/test/Analysis/temporaries.cpp b/test/Analysis/temporaries.cpp
index 48a1c5751d85b4fa9c813833a4a58146c1e1332d..367f53d0a9e36aa06ae61c064d7bf36c9d57eee2 100644 (file)
--- a/test/Analysis/temporaries.cpp
+++ b/test/Analysis/temporaries.cpp
@@ -900,7 +900,13 @@ int glob;
 
 class C {
 public:
-  ~C() { glob = 1; }
+  ~C() {
+    glob = 1;
+    clang_analyzer_checkInlined(true);
+#ifdef TEMPORARY_DTORS
+    // expected-warning@-2{{TRUE}}
+#endif
+  }
 };
 
 C get();
@@ -913,12 +919,11 @@ void test(int coin) {
   // temporaries: the return value of get() and the elidable copy constructor
   // of that return value into is(). According to the CFG, we need to cleanup
   // both of them depending on whether the temporary corresponding to the
-  // return value of get() was initialized. However, for now we don't track
-  // temporaries returned from functions, so we take the wrong branch.
+  // return value of get() was initialized. However, we didn't track
+  // temporaries returned from functions, so we took the wrong branch.
   coin && is(get()); // no-crash
-  // FIXME: We should have called the destructor, i.e. should be TRUE,
-  // at least when we inline temporary destructors.
-  clang_analyzer_eval(glob == 1); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be true once we inline all destructors.
+  clang_analyzer_eval(glob); // expected-warning{{UNKNOWN}}
 }
 } // namespace dont_forget_destructor_around_logical_op