]> granicus.if.org Git - clang/commitdiff
[analyzer] Destroy and lifetime-extend inlined function return values properly.
authorArtem Dergachev <artem.dergachev@gmail.com>
Mon, 12 Mar 2018 23:22:35 +0000 (23:22 +0000)
committerArtem Dergachev <artem.dergachev@gmail.com>
Mon, 12 Mar 2018 23:22:35 +0000 (23:22 +0000)
This patch uses the newly added CFGCXXRecordTypedCall element at the call site
of the caller to construct the return value within the callee directly into the
caller's stack frame. This way it is also capable of populating the temporary
destructor and lifetime extension maps for the temporary, which allows
temporary destructors and lifetime extension to work correctly.

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

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

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

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
test/Analysis/lifetime-extension.cpp

index f4aa56f2750dec4c5a857b915a002a20ac12f9af..398bb978fc541461af965b9e82cc323d14b252d4 100644 (file)
@@ -197,16 +197,19 @@ ExprEngine::getRegionForConstructedObject(const CXXConstructExpr *CE,
       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);
     }
     }
   }
@@ -262,6 +265,7 @@ void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE,
   assert(C || getCurrentCFGElement().getAs<CFGStmt>());
   const ConstructionContext *CC = C ? C->getConstructionContext() : nullptr;
 
+  bool IsReturnedIntoParentStackFrame = false;
   const CXXBindTemporaryExpr *BTE = nullptr;
   const MaterializeTemporaryExpr *MTE = nullptr;
 
@@ -269,25 +273,44 @@ void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE,
   case CXXConstructExpr::CK_Complete: {
     Target = getRegionForConstructedObject(CE, Pred, CC, CallOpts);
 
-    // In case of temporary object construction, extract data necessary for
-    // destruction and lifetime extension.
-    if (const auto *TCC =
-            dyn_cast_or_null<TemporaryObjectConstructionContext>(CC)) {
-      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 (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 (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 (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 (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;
+          }
         }
       }
     }
@@ -385,13 +408,19 @@ void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE,
         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));
       }
 
index 53c8edbb2a4989d023b44ee3e545dd74d41b6804..52d5f9afc420f2457b3371be43af97f89b3ba396 100644 (file)
@@ -1,7 +1,10 @@
 // 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;
@@ -63,6 +66,8 @@ public:
   ~C() { if (after) *after = this; }
 
   operator bool() const { return x; }
+
+  static C make(C **after, C **before) { return C(false, after, before); }
 };
 
 void f1() {
@@ -180,3 +185,154 @@ void f2() {
   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