[analyzer] Support destruction and lifetime-extension of inlined function return values.

[NoQ]

Summary

With D44120 we can find the CXXBindTemporaryExpr and MaterializeTemporaryExpr that correspond to the temporary object that is being returned from a function into its caller. These expressions are on the caller side because it is the caller that's responsible for managing the lifetime of such temporary.

In order to find those expressions, we look at the call site's CFG element, notice that it is a CFGValueTypedCall that was added in D44120, and take the relevant construction context information from there.

This patch does not yet affect temporaries that were returned from conservatively evaluated functions.

The idea was initially described in the last part of http://lists.llvm.org/pipermail/cfe-dev/2018-February/056898.html and addresses most of the concerns from D42876.

[george.karpenkov]

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

316

5 closing braces is a lot. What about moving the entire block under CK_Complete into a static function?

[NoQ]

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

316

I'm already on it for the next patch^^

[dcoughlin]

LGTM!

test/Analysis/lifetime-extension.cpp

301

It might be easier to follow if you started from '0' so that steps here match offsets into buf.

[NoQ]

Fix test comments and make them start from 0 :) Also rebase.

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

case ConstructionContext::TemporaryObjectKind: {
         }
       }
       // TODO: Support temporaries lifetime-extended via static references.
       // They'd need a getCXXStaticTempObjectRegion().
       CallOpts.IsTemporaryCtorOrDtor = true;
       return MRMgr.getCXXTempObjectRegion(CE, LCtx);
     }
     case ConstructionContext::ReturnedValueKind: {
-      // TODO: We should construct into a CXXBindTemporaryExpr or a
-      // MaterializeTemporaryExpr around the call-expression on the previous
-      // stack frame. Currently we re-bind the temporary to the correct region
-      // later, but that's not semantically correct. This of course does not
-      // apply when we're in the top frame. But if we are in an inlined
-      // function, we should be able to take the call-site CFG element,
-      // and it should contain (but right now it wouldn't) some sort of
-      // construction context that'd give us the right temporary expression.
+      // 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?
+      const LocationContext *TempLCtx = LCtx;
+      if (const LocationContext *CallerLCtx =
+              LCtx->getCurrentStackFrame()->getParent()) {
+        TempLCtx = CallerLCtx;
+      }
       CallOpts.IsTemporaryCtorOrDtor = true;
-      return MRMgr.getCXXTempObjectRegion(CE, LCtx);
+      return MRMgr.getCXXTempObjectRegion(CE, TempLCtx);
     }
     }
   }
   // If we couldn't find an existing region to construct into, assume we're
   // constructing a temporary. Notify the caller of our failure.
   CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion = true;
   return MRMgr.getCXXTempObjectRegion(CE, LCtx);
 }
void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE,
   // For now, we just run the first constructor (which should still invalidate
   // the entire array).
 
   EvalCallOptions CallOpts;
   auto C = getCurrentCFGElement().getAs<CFGConstructor>();
   assert(C || getCurrentCFGElement().getAs<CFGStmt>());
   const ConstructionContext *CC = C ? C->getConstructionContext() : nullptr;
 
+  bool IsReturnedIntoParentStackFrame = false;
   const CXXBindTemporaryExpr *BTE = nullptr;
   const MaterializeTemporaryExpr *MTE = nullptr;
 
   switch (CE->getConstructionKind()) {
   case CXXConstructExpr::CK_Complete: {
     Target = getRegionForConstructedObject(CE, Pred, CC, CallOpts);
 
+    if (CC) {
       // In case of temporary object construction, extract data necessary for
       // destruction and lifetime extension.
-    if (const auto *TCC =
-            dyn_cast_or_null<TemporaryObjectConstructionContext>(CC)) {
+      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 (const auto *RCC = dyn_cast<ReturnedValueConstructionContext>(CC)) {
+        const StackFrameContext *SFC = LCtx->getCurrentStackFrame();
+        assert(SFC);
+        if (SFC->getParent()) {
+          IsReturnedIntoParentStackFrame = true;
+          const CFGElement &CallElem =
+              (*SFC->getCallSiteBlock())[SFC->getIndex()];
+          if (auto RTCElem = CallElem.getAs<CFGCXXRecordTypedCall>()) {
+            TCC = cast<TemporaryObjectConstructionContext>(
+                RTCElem->getConstructionContext());
+          }
+        }
+      }
+
+      if (TCC) {
         assert(CallOpts.IsTemporaryCtorOrDtor);
         assert(!CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion);
         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;
           }
         }
       }
+    }
     break;
   }
   case CXXConstructExpr::CK_VirtualBase:
     // Make sure we are not calling virtual base class initializers twice.
     // Only the most-derived object should initialize virtual base classes.
     if (const Stmt *Outer = LCtx->getCurrentStackFrame()->getCallSite()) {
       const CXXConstructExpr *OuterCtor = dyn_cast<CXXConstructExpr>(Outer);
       if (OuterCtor) {
for (ExplodedNodeSet::iterator I = DstPreVisit.begin(),
         // initialize the entire array, not just the first element -- but our
         // handling of arrays everywhere else is weak as well, so this shouldn't
         // actually make things worse. Placement new makes this tricky as well,
         // since it's then possible to be initializing one part of a multi-
         // dimensional array.
         State = State->bindDefault(loc::MemRegionVal(Target), ZeroVal, LCtx);
       }
 
+      // Set up destruction and lifetime extension information.
+      const LocationContext *TempLCtx =
+          IsReturnedIntoParentStackFrame
+              ? LCtx->getCurrentStackFrame()->getParent()
+              : LCtx;
+
       if (BTE) {
-        State = addInitializedTemporary(State, BTE, LCtx,
+        State = addInitializedTemporary(State, BTE, TempLCtx,
                                         cast<CXXTempObjectRegion>(Target));
       }
 
       if (MTE) {
-        State = addTemporaryMaterialization(State, MTE, LCtx,
+        State = addTemporaryMaterialization(State, MTE, TempLCtx,
                                             cast<CXXTempObjectRegion>(Target));
       }
 
       Bldr.generateNode(CE, *I, State, /*tag=*/nullptr,
                         ProgramPoint::PreStmtKind);
     }
   }
 

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++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
 
 void clang_analyzer_eval(bool);
+void clang_analyzer_checkInlined(bool);
 
 namespace pr17001_call_wrong_destructor {
 bool x;
 struct A {
   int *a;
   A() {}
   ~A() {}
 };
public:
   }
 
   // Don't track copies in our tests.
   C(const C &c) : x(c.x), after(nullptr), before(nullptr) {}
 
   ~C() { if (after) *after = this; }
 
   operator bool() const { return x; }
+
+  static C make(C **after, C **before) { return C(false, after, before); }
 };
 
 void f1() {
   C *after, *before;
   {
     const C &c = C(true, &after, &before);
   }
   clang_analyzer_eval(after == before);
void f2() {
   int x = 1;
   C c;
   // &x is replaced with nullptr in move-assignment before the temporary dies.
   c = C(&x);
   // Hence x was not set to 0 yet.
   1 / x; // no-warning
 }
 } // end namespace maintain_original_object_address_on_move
+
+namespace maintain_address_of_copies {
+class C;
+
+struct AddressVector {
+  C *buf[10];
+  int len;
+
+  AddressVector() : len(0) {}
+
+  void push(C *c) {
+    buf[len] = c;
+    ++len;
+  }
+};
+
+class C {
+  AddressVector &v;
+
+public:
+  C(AddressVector &v) : v(v) { v.push(this); }
+  ~C() { v.push(this); }
+
+#ifdef MOVES
+  C(C &&c) : v(c.v) { v.push(this); }
+#endif
+
+  // Note how return-statements prefer move-constructors when available.
+  C(const C &c) : v(c.v) {
+#ifdef MOVES
+    clang_analyzer_checkInlined(false); // no-warning
+#else
+    v.push(this);
+#endif
+  } // no-warning
+
+  static C make(AddressVector &v) { return C(v); }
+};
+
+void f1() {
+  AddressVector v;
+  {
+    C c = C(v);
+  }
+  // 0. Create the original temporary and lifetime-extend it into variable 'c'
+  //    construction argument.
+  // 1. Construct variable 'c' (elidable copy/move).
+  // 2. Destroy the temporary.
+  // 3. Destroy variable 'c'.
+  clang_analyzer_eval(v.len == 4);
+  clang_analyzer_eval(v.buf[0] == v.buf[2]);
+  clang_analyzer_eval(v.buf[1] == v.buf[3]);
+#ifdef TEMPORARIES
+  // expected-warning@-4{{TRUE}}
+  // expected-warning@-4{{TRUE}}
+  // expected-warning@-4{{TRUE}}
+#else
+  // expected-warning@-8{{UNKNOWN}}
+  // expected-warning@-8{{UNKNOWN}}
+  // expected-warning@-8{{UNKNOWN}}
+#endif
+}
+
+void f2() {
+  AddressVector v;
+  {
+    const C &c = C::make(v);
+  }
+  // 0. Construct the original temporary within make(),
+  // 1. Construct the return value of make() (elidable copy/move) and
+  //    lifetime-extend it via reference 'c',
+  // 2. Destroy the temporary within make(),
+  // 3. Destroy the temporary lifetime-extended by 'c'.
+  clang_analyzer_eval(v.len == 4);
+  clang_analyzer_eval(v.buf[0] == v.buf[2]);
+  clang_analyzer_eval(v.buf[1] == v.buf[3]);
+#ifdef TEMPORARIES
+  // expected-warning@-4{{TRUE}}
+  // expected-warning@-4{{TRUE}}
+  // expected-warning@-4{{TRUE}}
+#else
+  // expected-warning@-8{{UNKNOWN}}
+  // expected-warning@-8{{UNKNOWN}}
+  // expected-warning@-8{{UNKNOWN}}
+#endif
+}
+
+void f3() {
+  AddressVector v;
+  {
+    C &&c = C::make(v);
+  }
+  // 0. Construct the original temporary within make(),
+  // 1. Construct the return value of make() (elidable copy/move) and
+  //    lifetime-extend it via reference 'c',
+  // 2. Destroy the temporary within make(),
+  // 3. Destroy the temporary lifetime-extended by 'c'.
+  clang_analyzer_eval(v.len == 4);
+  clang_analyzer_eval(v.buf[0] == v.buf[2]);
+  clang_analyzer_eval(v.buf[1] == v.buf[3]);
+#ifdef TEMPORARIES
+  // expected-warning@-4{{TRUE}}
+  // expected-warning@-4{{TRUE}}
+  // expected-warning@-4{{TRUE}}
+#else
+  // expected-warning@-8{{UNKNOWN}}
+  // expected-warning@-8{{UNKNOWN}}
+  // expected-warning@-8{{UNKNOWN}}
+#endif
+}
+
+C doubleMake(AddressVector &v) {
+  return C::make(v);
+}
+
+void f4() {
+  AddressVector v;
+  {
+    C c = doubleMake(v);
+  }
+  // 0. Construct the original temporary within make(),
+  // 1. Construct the return value of make() (elidable copy/move) and
+  //    lifetime-extend it into the return value constructor argument within
+  //    doubleMake(),
+  // 2. Destroy the temporary within make(),
+  // 3. Construct the return value of doubleMake() (elidable copy/move) and
+  //    lifetime-extend it into the variable 'c' constructor argument,
+  // 4. Destroy the return value of make(),
+  // 5. Construct variable 'c' (elidable copy/move),
+  // 6. Destroy the return value of doubleMake(),
+  // 7. Destroy variable 'c'.
+  clang_analyzer_eval(v.len == 8);
+  clang_analyzer_eval(v.buf[0] == v.buf[2]);
+  clang_analyzer_eval(v.buf[1] == v.buf[4]);
+  clang_analyzer_eval(v.buf[3] == v.buf[6]);
+  clang_analyzer_eval(v.buf[5] == v.buf[7]);
+#ifdef TEMPORARIES
+  // expected-warning@-6{{TRUE}}
+  // expected-warning@-6{{TRUE}}
+  // expected-warning@-6{{TRUE}}
+  // expected-warning@-6{{TRUE}}
+  // expected-warning@-6{{TRUE}}
+#else
+  // expected-warning@-12{{UNKNOWN}}
+  // expected-warning@-12{{UNKNOWN}}
+  // expected-warning@-12{{UNKNOWN}}
+  // expected-warning@-12{{UNKNOWN}}
+  // expected-warning@-12{{UNKNOWN}}
+#endif
+}
+} // end namespace maintain_address_of_copies