[analyzer] Introduce correct lifetime extension behavior in simple cases.

This patch uses the reference to MaterializeTemporaryExpr stored in the
construction context since r326014 in order to model that expression correctly.

When modeling MaterializeTemporaryExpr, instead of copying the raw memory
contents from the sub-expression's rvalue to a completely new temporary region,
that we conjure up for the lack of better options, we now have the better
option to recall the region into which the object was originally constructed
and declare that region to be the value of the expression, which is semantically
correct.

This only works when the construction context is available, which is worked on
independently.

The temporary region's liveness (in the sense of removeDeadBindings) is extended
until the MaterializeTemporaryExpr is resolved, in order to keep the store
bindings around, because it wouldn't be referenced from anywhere else in the
program state.

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

llvm-svn: 326236

2 files changed, 143 insertions, 41 deletions

diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index 8808e95aedd..9f71b2fc03a 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -65,6 +65,17 @@ typedef llvm::ImmutableMap<std::pair<const CXXBindTemporaryExpr *,
 REGISTER_TRAIT_WITH_PROGRAMSTATE(InitializedTemporaries,
                                  InitializedTemporariesMap)
 
+typedef llvm::ImmutableMap<std::pair<const MaterializeTemporaryExpr *,
+                                     const StackFrameContext *>,
+                           const CXXTempObjectRegion *>
+        TemporaryMaterializationMap;
+
+// Keeps track of temporaries that will need to be materialized later.
+// The StackFrameContext assures that nested calls due to inlined recursive
+// functions do not interfere.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(TemporaryMaterializations,
+                                 TemporaryMaterializationMap)
+
 typedef llvm::ImmutableMap<std::pair<const CXXNewExpr *,
                                      const LocationContext *>,
                            SVal>
@@ -255,17 +266,35 @@ ExprEngine::createTemporaryRegionIfNeeded(ProgramStateRef State,
   const Expr *Init = InitWithAdjustments->skipRValueSubobjectAdjustments(
       CommaLHSs, Adjustments);
 
+  // Take the region for Init, i.e. for the whole object. If we do not remember
+  // the region in which the object originally was constructed, come up with
+  // a new temporary region out of thin air and copy the contents of the object
+  // (which are currently present in the Environment, because Init is an rvalue)
+  // into that region. This is not correct, but it is better than nothing.
+  bool FoundOriginalMaterializationRegion = false;
   const TypedValueRegion *TR = nullptr;
   if (const MaterializeTemporaryExpr *MT =
           dyn_cast<MaterializeTemporaryExpr>(Result)) {
-    StorageDuration SD = MT->getStorageDuration();
-    // If this object is bound to a reference with static storage duration, we
-    // put it in a different region to prevent "address leakage" warnings.
-    if (SD == SD_Static || SD == SD_Thread)
-      TR = MRMgr.getCXXStaticTempObjectRegion(Init);
-  }
-  if (!TR)
+    auto Key = std::make_pair(MT, LC->getCurrentStackFrame());
+    if (const CXXTempObjectRegion *const *TRPtr =
+            State->get<TemporaryMaterializations>(Key)) {
+      FoundOriginalMaterializationRegion = true;
+      TR = *TRPtr;
+      assert(TR);
+      State = State->remove<TemporaryMaterializations>(Key);
+    } else {
+      StorageDuration SD = MT->getStorageDuration();
+      // If this object is bound to a reference with static storage duration, we
+      // put it in a different region to prevent "address leakage" warnings.
+      if (SD == SD_Static || SD == SD_Thread) {
+        TR = MRMgr.getCXXStaticTempObjectRegion(Init);
+      } else {
+        TR = MRMgr.getCXXTempObjectRegion(Init, LC);
+      }
+    }
+  } else {
     TR = MRMgr.getCXXTempObjectRegion(Init, LC);
+  }
 
   SVal Reg = loc::MemRegionVal(TR);
   SVal BaseReg = Reg;
@@ -287,32 +316,35 @@ ExprEngine::createTemporaryRegionIfNeeded(ProgramStateRef State,
     }
   }
 
-  // What remains is to copy the value of the object to the new region.
-  // FIXME: In other words, what we should always do is copy value of the
-  // Init expression (which corresponds to the bigger object) to the whole
-  // temporary region TR. However, this value is often no longer present
-  // in the Environment. If it has disappeared, we instead invalidate TR.
-  // Still, what we can do is assign the value of expression Ex (which
-  // corresponds to the sub-object) to the TR's sub-region Reg. At least,
-  // values inside Reg would be correct.
-  SVal InitVal = State->getSVal(Init, LC);
-  if (InitVal.isUnknown()) {
-    InitVal = getSValBuilder().conjureSymbolVal(Result, LC, Init->getType(),
-                                                currBldrCtx->blockCount());
-    State = State->bindLoc(BaseReg.castAs<Loc>(), InitVal, LC, false);
-
-    // Then we'd need to take the value that certainly exists and bind it over.
-    if (InitValWithAdjustments.isUnknown()) {
-      // Try to recover some path sensitivity in case we couldn't
-      // compute the value.
-      InitValWithAdjustments = getSValBuilder().conjureSymbolVal(
-          Result, LC, InitWithAdjustments->getType(),
-          currBldrCtx->blockCount());
+  if (!FoundOriginalMaterializationRegion) {
+    // What remains is to copy the value of the object to the new region.
+    // FIXME: In other words, what we should always do is copy value of the
+    // Init expression (which corresponds to the bigger object) to the whole
+    // temporary region TR. However, this value is often no longer present
+    // in the Environment. If it has disappeared, we instead invalidate TR.
+    // Still, what we can do is assign the value of expression Ex (which
+    // corresponds to the sub-object) to the TR's sub-region Reg. At least,
+    // values inside Reg would be correct.
+    SVal InitVal = State->getSVal(Init, LC);
+    if (InitVal.isUnknown()) {
+      InitVal = getSValBuilder().conjureSymbolVal(Result, LC, Init->getType(),
+                                                  currBldrCtx->blockCount());
+      State = State->bindLoc(BaseReg.castAs<Loc>(), InitVal, LC, false);
+
+      // Then we'd need to take the value that certainly exists and bind it
+      // over.
+      if (InitValWithAdjustments.isUnknown()) {
+        // Try to recover some path sensitivity in case we couldn't
+        // compute the value.
+        InitValWithAdjustments = getSValBuilder().conjureSymbolVal(
+            Result, LC, InitWithAdjustments->getType(),
+            currBldrCtx->blockCount());
+      }
+      State =
+          State->bindLoc(Reg.castAs<Loc>(), InitValWithAdjustments, LC, false);
+    } else {
+      State = State->bindLoc(BaseReg.castAs<Loc>(), InitVal, LC, false);
     }
-    State =
-        State->bindLoc(Reg.castAs<Loc>(), InitValWithAdjustments, LC, false);
-  } else {
-    State = State->bindLoc(BaseReg.castAs<Loc>(), InitVal, LC, false);
   }
 
   // The result expression would now point to the correct sub-region of the
@@ -320,8 +352,10 @@ ExprEngine::createTemporaryRegionIfNeeded(ProgramStateRef State,
   // correctly in case (Result == Init).
   State = State->BindExpr(Result, LC, Reg);
 
-  // Notify checkers once for two bindLoc()s.
-  State = processRegionChange(State, TR, LC);
+  if (!FoundOriginalMaterializationRegion) {
+    // Notify checkers once for two bindLoc()s.
+    State = processRegionChange(State, TR, LC);
+  }
 
   return State;
 }
@@ -356,6 +390,29 @@ bool ExprEngine::areInitializedTemporariesClear(ProgramStateRef State,
   return true;
 }
 
+ProgramStateRef ExprEngine::addTemporaryMaterialization(
+    ProgramStateRef State, const MaterializeTemporaryExpr *MTE,
+    const LocationContext *LC, const CXXTempObjectRegion *R) {
+  const auto &Key = std::make_pair(MTE, LC->getCurrentStackFrame());
+  assert(!State->contains<TemporaryMaterializations>(Key));
+  return State->set<TemporaryMaterializations>(Key, R);
+}
+
+bool ExprEngine::areTemporaryMaterializationsClear(
+    ProgramStateRef State, const LocationContext *FromLC,
+    const LocationContext *ToLC) {
+  const LocationContext *LC = FromLC;
+  while (LC != ToLC) {
+    assert(LC && "ToLC must be a parent of FromLC!");
+    for (auto I : State->get<TemporaryMaterializations>())
+      if (I.first.second == LC)
+        return false;
+
+    LC = LC->getParent();
+  }
+  return true;
+}
+
 ProgramStateRef
 ExprEngine::setCXXNewAllocatorValue(ProgramStateRef State,
                                     const CXXNewExpr *CNE,
@@ -437,6 +494,24 @@ static void printInitializedTemporariesForContext(raw_ostream &Out,
   }
 }
 
+static void printTemporaryMaterializationsForContext(
+    raw_ostream &Out, ProgramStateRef State, const char *NL, const char *Sep,
+    const LocationContext *LC) {
+  PrintingPolicy PP =
+      LC->getAnalysisDeclContext()->getASTContext().getPrintingPolicy();
+  for (auto I : State->get<TemporaryMaterializations>()) {
+    std::pair<const MaterializeTemporaryExpr *, const LocationContext *> Key =
+        I.first;
+    const MemRegion *Value = I.second;
+    if (Key.second != LC)
+      continue;
+    Out << '(' << Key.second << ',' << Key.first << ") ";
+    Key.first->printPretty(Out, nullptr, PP);
+    assert(Value);
+    Out << " : " << Value << NL;
+  }
+}
+
 static void printCXXNewAllocatorValuesForContext(raw_ostream &Out,
                                                  ProgramStateRef State,
                                                  const char *NL,
@@ -468,6 +543,14 @@ void ExprEngine::printState(raw_ostream &Out, ProgramStateRef State,
       });
     }
 
+    if (!State->get<TemporaryMaterializations>().isEmpty()) {
+      Out << Sep << "Temporaries to be materialized:" << NL;
+
+      LCtx->dumpStack(Out, "", NL, Sep, [&](const LocationContext *LC) {
+        printTemporaryMaterializationsForContext(Out, State, NL, Sep, LC);
+      });
+    }
+
     if (!State->get<CXXNewAllocatorValues>().isEmpty()) {
       Out << Sep << "operator new() allocator return values:" << NL;
 
@@ -578,6 +661,10 @@ void ExprEngine::removeDead(ExplodedNode *Pred, ExplodedNodeSet &Out,
     if (I.second)
       SymReaper.markLive(I.second);
 
+  for (auto I : CleanedState->get<TemporaryMaterializations>())
+    if (I.second)
+      SymReaper.markLive(I.second);
+
   for (auto I : CleanedState->get<CXXNewAllocatorValues>()) {
     if (SymbolRef Sym = I.second.getAsSymbol())
       SymReaper.markLive(Sym);
@@ -2108,11 +2195,12 @@ void ExprEngine::processEndOfFunction(NodeBuilderContext& BC,
                                        Pred->getStackFrame()->getParent()));
 
   // FIXME: We currently assert that temporaries are clear, as lifetime extended
-  // temporaries are not modelled correctly. 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.
+  // 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);
@@ -2137,6 +2225,9 @@ void ExprEngine::processEndOfFunction(NodeBuilderContext& BC,
   assert(areInitializedTemporariesClear(Pred->getState(),
                                         Pred->getLocationContext(),
                                         Pred->getStackFrame()->getParent()));
+  assert(areTemporaryMaterializationsClear(Pred->getState(),
+                                           Pred->getLocationContext(),
+                                           Pred->getStackFrame()->getParent()));
 
   PrettyStackTraceLocationContext CrashInfo(Pred->getLocationContext());
   StateMgr.EndPath(Pred->getState());
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
index 9c95507ba57..2c1e858da25 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -230,6 +230,7 @@ void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE,
   const ConstructionContext *CC = C ? C->getConstructionContext() : nullptr;
 
   const CXXBindTemporaryExpr *BTE = nullptr;
+  const MaterializeTemporaryExpr *MTE = nullptr;
 
   switch (CE->getConstructionKind()) {
   case CXXConstructExpr::CK_Complete: {
@@ -237,8 +238,13 @@ void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE,
     if (CC && AMgr.getAnalyzerOptions().includeTemporaryDtorsInCFG() &&
         !CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion &&
         CallOpts.IsTemporaryCtorOrDtor) {
-      // May as well be a ReturnStmt.
-      BTE = dyn_cast<CXXBindTemporaryExpr>(CC->getTriggerStmt());
+      MTE = CC->getMaterializedTemporary();
+      if (!MTE || MTE->getStorageDuration() == SD_FullExpression) {
+        // If the temporary is lifetime-extended, don't save the BTE,
+        // because we don't need a temporary destructor, but an automatic
+        // destructor. The cast may fail because it may as well be a ReturnStmt.
+        BTE = dyn_cast<CXXBindTemporaryExpr>(CC->getTriggerStmt());
+      }
     }
     break;
   }
@@ -339,6 +345,11 @@ void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE,
                                         cast<CXXTempObjectRegion>(Target));
       }
 
+      if (MTE) {
+        State = addTemporaryMaterialization(State, MTE, LCtx,
+                                            cast<CXXTempObjectRegion>(Target));
+      }
+
       Bldr.generateNode(CE, *I, State, /*tag=*/nullptr,
                         ProgramPoint::PreStmtKind);
     }