[analyzer] Suppress more C++17-related crashes.

[NoQ]

Summary

This is assertion removal. The assertion was saying that a function's return value is always a temporary object - which is not true when copy elision is happening, C++17 mandatory copy elision in particular (we don't support other forms of copy elision yet, but we have no choice but to support this one).

[george.karpenkov]

lib/StaticAnalyzer/Core/ExprEngine.cpp

480

I'm a bit confused as to what is going on here; maybe a short comment would be helpful?
What I don't understand is that we go through that branch when construction context is supplied only to completely ignore it [..because we know better?..]

[NoQ]

Add a comment for the confusing part.

lib/StaticAnalyzer/Core/ExprEngine.cpp

bool ExprEngine::areTemporaryMaterializationsClear(
   return true;
 }
 
 ProgramStateRef ExprEngine::addAllNecessaryTemporaryInfo(
     ProgramStateRef State, const ConstructionContext *CC,
     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;
+      }
+      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;
+      CC = dyn_cast<TemporaryObjectConstructionContext>(
+          RTCElem->getConstructionContext());
+      if (!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();
         if (!BTE) {
           // FIXME: Lifetime extension for temporaries without destructors
           // is not implemented yet.
           MTE = nullptr;
         }
         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.
           BTE = nullptr;
         }
       }
     }
 
     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));
     }
   }
 
   return State;
 }
 
 ProgramStateRef

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

if (CC) {
     }
     case ConstructionContext::SimpleReturnedValueKind: {
       // The temporary is to be managed by the parent stack frame.
       // So build it in the parent stack frame if we're not in the
       // top frame of the analysis.
       // 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()) {
         TempLCtx = CallerLCtx;
       }
       CallOpts.IsTemporaryCtorOrDtor = true;
       return MRMgr.getCXXTempObjectRegion(CE, TempLCtx);
     }

test/Analysis/lifetime-extension.cpp

 // RUN: %clang_analyze_cc1 -Wno-unused -std=c++11 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify %s
 // RUN: %clang_analyze_cc1 -Wno-unused -std=c++11 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true -DTEMPORARIES -verify %s
+// RUN: %clang_analyze_cc1 -Wno-unused -std=c++17 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true -DTEMPORARIES %s
 // RUN: %clang_analyze_cc1 -Wno-unused -std=c++11 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -DMOVES -verify %s
 // RUN: %clang_analyze_cc1 -Wno-unused -std=c++11 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true -DTEMPORARIES -DMOVES -verify %s
+// RUN: %clang_analyze_cc1 -Wno-unused -std=c++17 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true -DTEMPORARIES -DMOVES %s
+
+// Note: The C++17 run-lines don't -verify yet - it is a no-crash test.
 
 void clang_analyzer_eval(bool);
 void clang_analyzer_checkInlined(bool);
 
 namespace pr17001_call_wrong_destructor {
 bool x;
 struct A {
   int *a;

test/Analysis/temporaries.cpp

 // RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++03 %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++11 %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -DTEMPORARY_DTORS -verify -w -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true %s -std=c++11
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -DTEMPORARY_DTORS -w -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true %s -std=c++17
+
+// Note: The C++17 run-line doesn't -verify yet - it is a no-crash test.
 
 extern bool clang_analyzer_eval(bool);
 extern bool clang_analyzer_warnIfReached();
 void clang_analyzer_checkInlined(bool);
 
 #include "Inputs/system-header-simulator-cxx.h";
 
 struct Trivial {