[analyzer] Remove an assertion that doesn't hold in C++17.

Function return values can be constructed directly in variables or passed
directly into return statements, without even an elidable copy in between.
This is how the C++17 mandatory copy elision AST behaves. The behavior we'll
have in such cases is the "old" behavior that we've had before we've
implemented destructor inlining and proper lifetime extension support.

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

llvm-svn: 328253

2 files changed, 42 insertions, 16 deletions

diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index e6e5be0d137..d22cf82963b 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -455,29 +455,51 @@ ProgramStateRef ExprEngine::addAllNecessaryTemporaryInfo(
     const LocationContext *LC, const MemRegion *R) {
   const CXXBindTemporaryExpr *BTE = nullptr;
   const MaterializeTemporaryExpr *MTE = nullptr;
-  const LocationContext *TempLC = LC;
 
   if (CC) {
-    // In case of temporary object construction, extract data necessary for
-    // destruction and lifetime extension.
-    const auto *TCC = dyn_cast<TemporaryObjectConstructionContext>(CC);
-
     // If the temporary is being returned from the function, it will be
     // destroyed or lifetime-extended in the caller stack frame.
     if (isa<ReturnedValueConstructionContext>(CC)) {
       const StackFrameContext *SFC = LC->getCurrentStackFrame();
       assert(SFC);
-      if (SFC->getParent()) {
-        TempLC = SFC->getParent();
-        const CFGElement &CallElem =
-            (*SFC->getCallSiteBlock())[SFC->getIndex()];
-        if (auto RTCElem = CallElem.getAs<CFGCXXRecordTypedCall>()) {
-          TCC = cast<TemporaryObjectConstructionContext>(
-              RTCElem->getConstructionContext());
-        }
+      LC = SFC->getParent();
+      if (!LC) {
+        // We are on the top frame. We won't ever need any info
+        // for this temporary, so don't set anything.
+        return State;
+      }
+      const CFGElement &CallElem =
+          (*SFC->getCallSiteBlock())[SFC->getIndex()];
+      auto RTCElem = CallElem.getAs<CFGCXXRecordTypedCall>();
+      if (!RTCElem) {
+        // We have a parent stack frame, but no construction context for the
+        // return value. Give up until we provide the construction context
+        // at the call site.
+        return State;
       }
+      // We use the ReturnedValueConstructionContext as an indication that we
+      // need to look for the actual construction context on the parent stack
+      // frame. This purpose has been fulfilled, so now we replace CC with the
+      // actual construction context.
+      CC = RTCElem->getConstructionContext();
+      if (!isa<TemporaryObjectConstructionContext>(CC)) {
+        // TODO: We are not returning an object into a temporary. There must
+        // be copy elision happening at the call site. We still need to
+        // explicitly support the situation when the return value is put
+        // into another return statement, i.e.
+        // ReturnedValueConstructionContexts are chained through multiple
+        // stack frames before finally settling in a temporary.
+        // We don't seem to need to explicitly support construction into
+        // a variable after a return.
+        return State;
+      }
+      // Proceed to deal with the temporary we've found on the parent
+      // stack frame.
     }
-    if (TCC) {
+
+    // In case of temporary object construction, extract data necessary for
+    // destruction and lifetime extension.
+    if (const auto *TCC = dyn_cast<TemporaryObjectConstructionContext>(CC)) {
       if (AMgr.getAnalyzerOptions().includeTemporaryDtorsInCFG()) {
         BTE = TCC->getCXXBindTemporaryExpr();
         MTE = TCC->getMaterializedTemporaryExpr();
@@ -496,12 +518,12 @@ ProgramStateRef ExprEngine::addAllNecessaryTemporaryInfo(
     }
 
     if (BTE) {
-      State = addInitializedTemporary(State, BTE, TempLC,
+      State = addInitializedTemporary(State, BTE, LC,
                                       cast<CXXTempObjectRegion>(R));
     }
 
     if (MTE) {
-      State = addTemporaryMaterialization(State, MTE, TempLC,
+      State = addTemporaryMaterialization(State, MTE, LC,
                                           cast<CXXTempObjectRegion>(R));
     }
   }
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
index 07515b36470..ea1d1d30c39 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -203,6 +203,10 @@ ExprEngine::getRegionForConstructedObject(const CXXConstructExpr *CE,
       // TODO: What exactly happens when we are? Does the temporary object live
       // long enough in the region store in this case? Would checkers think
       // that this object immediately goes out of scope?
+      // TODO: We assume that the call site has a temporary object construction
+      // context. This is no longer true in C++17 or when copy elision is
+      // performed. We may need to unwrap multiple stack frames here and we
+      // won't necessarily end up with a temporary at the end.
       const LocationContext *TempLCtx = LCtx;
       if (const LocationContext *CallerLCtx =
               LCtx->getCurrentStackFrame()->getParent()) {