[analyzer] Re-enable lifetime extension for temporaries without destructors and bring back static temporaries.

[NoQ]

Summary

I don't remember why lifetime extension for temporaries without destructors was disabled. It's clearly less important and is a different syntax to support, so i guess i simply didn't want to pretend that it works before i tested it, but it generally seems to be pretty much the same.

Added a simple test to test this.

One thing i noticed when i re-enabled such lifetime extension was that we had tests for "static temporaries" (i.e. temporaries lifetime-extended by static or global variables) which started to fail, so i had to fix them as well, which wasn't hard.

[NoQ]

Fix comments in the callback test. The test itself is pretty pointless now, because the behavior that it was supposed to test was a workaround for lack of lifetime extension support.

[NoQ]

Remove the outdated FIXME comment.

[MTC]

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

241

An unrelated question. I want to know, what considerations are you based on not continue to use short circuit style : )?

[NoQ]

Fxd.

[NoQ]

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

241

Whoops accidental^^

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

case ConstructionContext::ElidedTemporaryObjectKind:
       assert(AMgr.getAnalyzerOptions().shouldElideConstructors());
       // FALL-THROUGH
     case ConstructionContext::SimpleTemporaryObjectKind: {
       // TODO: Copy elision implementation goes here.
       const auto *TCC = cast<TemporaryObjectConstructionContext>(CC);
       const CXXBindTemporaryExpr *BTE = TCC->getCXXBindTemporaryExpr();
       const MaterializeTemporaryExpr *MTE = TCC->getMaterializedTemporaryExpr();
 
-      if (!BTE) {
-        // FIXME: Lifetime extension for temporaries without destructors
-        // is not implemented yet.
-        MTE = nullptr;
-      }
-
       if (MTE) {
         if (const ValueDecl *VD = MTE->getExtendingDecl()) {
           assert(MTE->getStorageDuration() != SD_FullExpression);
           if (!VD->getType()->isReferenceType()) {
             // We're lifetime-extended by a surrounding aggregate.
             // Automatic destructors aren't quite working in this case
             // on the CFG side. We should warn the caller about that.
             // FIXME: Is there a better way to retrieve this information from
             // the MaterializeTemporaryExpr?
             CallOpts.IsTemporaryLifetimeExtendedViaAggregate = true;
           }
         }
       }
 
+      SVal V = UnknownVal();
       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 (MTE->getStorageDuration() == SD_Static ||
+            MTE->getStorageDuration() == SD_Thread)
+          V = loc::MemRegionVal(MRMgr.getCXXStaticTempObjectRegion(E));
       }
 
-      // FIXME: Support temporaries lifetime-extended via static references.
-      // They'd need a getCXXStaticTempObjectRegion().
-      SVal V = loc::MemRegionVal(MRMgr.getCXXTempObjectRegion(E, LCtx));
+      if (V.isUnknown())
+        V = loc::MemRegionVal(MRMgr.getCXXTempObjectRegion(E, LCtx));
 
       if (BTE)
         State = addObjectUnderConstruction(State, BTE, LCtx, V);
 
       if (MTE)
         State = addObjectUnderConstruction(State, MTE, LCtx, V);
 
       CallOpts.IsTemporaryCtorOrDtor = true;

test/Analysis/lifetime-extension.cpp

void f2() {
   // &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];
+template <typename T> struct AddressVector {
+  const T *buf[10];
   int len;
 
   AddressVector() : len(0) {}
 
-  void push(C *c) {
-    buf[len] = c;
+  void push(const T *t) {
+    buf[len] = t;
     ++len;
   }
 };
 
 class C {
-  AddressVector &v;
+  AddressVector<C> &v;
 
 public:
-  C(AddressVector &v) : v(v) { v.push(this); }
+  C(AddressVector<C> &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); }
+  static C make(AddressVector<C> &v) { return C(v); }
 };
 
 void f1() {
-  AddressVector v;
+  AddressVector<C> 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;
+  AddressVector<C> 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;
+  AddressVector<C> 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) {
+C doubleMake(AddressVector<C> &v) {
   return C::make(v);
 }
 
 void f4() {
-  AddressVector v;
+  AddressVector<C> 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(),

 #else
   // expected-warning@-12{{UNKNOWN}}
   // expected-warning@-12{{UNKNOWN}}
   // expected-warning@-12{{UNKNOWN}}
   // expected-warning@-12{{UNKNOWN}}
   // expected-warning@-12{{UNKNOWN}}
 #endif
 }
+
+class NoDtor {
+  AddressVector<NoDtor> &v;
+
+public:
+  NoDtor(AddressVector<NoDtor> &v) : v(v) { v.push(this); }
+};
+
+void f5() {
+  AddressVector<NoDtor> v;
+  const NoDtor &N = NoDtor(v);
+  clang_analyzer_eval(v.buf[0] == &N); // expected-warning{{TRUE}}
+}
+
 } // end namespace maintain_address_of_copies

test/Analysis/temporaries-callback-order.cpp

 // RUN: %clang_cc1 -analyze -analyzer-checker=debug.AnalysisOrder -analyzer-config debug.AnalysisOrder:Bind=true -analyzer-config debug.AnalysisOrder:RegionChanges=true %s 2>&1 | FileCheck %s
 
 struct Super {
   virtual void m();
 };
 struct Sub : Super {
   virtual void m() {}
 };
 
 void testTemporaries() {
-  // This triggers RegionChanges twice:
-  // - Once for zero-initialization of the structure.
-  // - Once for creating a temporary region and copying the structure there.
-  // FIXME: This code shouldn't really produce the extra temporary, however
-  // that's how we behave for now.
+  // This triggers RegionChanges once for zero-initialization of the structure.
   Sub().m();
 }
 
 void seeIfCheckBindWorks() {
   // This should trigger checkBind. The rest of the code shouldn't.
   // This also triggers checkRegionChanges after that.
   // Note that this function is analyzed first, so the messages would be on top.
   int x = 1;
 }
 
 // seeIfCheckBindWorks():
 // CHECK: Bind
 // CHECK-NEXT: RegionChanges
 
 // testTemporaries():
 // CHECK-NEXT: RegionChanges
-// CHECK-NEXT: RegionChanges
 
 // Make sure there's no further output.
 // CHECK-NOT: Bind
 // CHECK-NOT: RegionChanges