[CFG] [analyzer] Work around a disappearing CXXBindTemporaryExpr.

[NoQ]

Summary

This is a CFG-side patch for the weird AST i encountered in http://lists.llvm.org/pipermail/cfe-dev/2018-March/057398.html . The omission of the CXXBindTemporaryExpr causes a crash because it produces an unexpected construction context for a temporary object that requires non-trivial destruction.

I removed the crashing assertion for now, together with the ill-formed construction context. Temporary destructor is still missing from the CFG in this case, so it was important to remove the construction context, so that not to try modeling the constructor when the destructor is completely missing.

CodeGen works fine because they defensively destroy everything they ever wanted to destroy at ExprWithCleanups.

[george.karpenkov]

LGTM with a nit, provided a proper fix can not be landed in a timely enough matter.

lib/Analysis/CFG.cpp

740

auto

[NoQ]

Use auto.

include/clang/Analysis/ConstructionContext.h

private:
   template <typename T, typename... ArgTypes>
   static T *create(BumpVectorContext &C, ArgTypes... Args) {
     auto *CC = C.getAllocator().Allocate<T>();
     return new (CC) T(Args...);
   }
 
 public:
   /// Consume the construction context layer, together with its parent layers,
-  /// and wrap it up into a complete construction context.
+  /// and wrap it up into a complete construction context. May return null
+  /// if layers do not form any supported construction context.
   static const ConstructionContext *
   createFromLayers(BumpVectorContext &C,
                    const ConstructionContextLayer *TopLayer);
 
   Kind getKind() const { return K; }
 };
 
 /// An abstract base class for local variable constructors.

lib/Analysis/CFG.cpp

void appendStmt(CFGBlock *B, const Stmt *S) {
     assert(!isa<Expr>(S) || cast<Expr>(S)->IgnoreParens() == S);
     B->appendStmt(const_cast<Stmt*>(S), cfg->getBumpVectorContext());
   }
 
   void appendConstructor(CFGBlock *B, CXXConstructExpr *CE) {
     if (BuildOpts.AddRichCXXConstructors) {
       if (const ConstructionContextLayer *Layer =
               ConstructionContextMap.lookup(CE)) {
-        const ConstructionContext *CC =
-            ConstructionContext::createFromLayers(cfg->getBumpVectorContext(),
-                                                  Layer);
-        B->appendConstructor(CE, CC, cfg->getBumpVectorContext());
         cleanupConstructionContext(CE);
+        if (const ConstructionContext *CC =
+                ConstructionContext::createFromLayers(
+                    cfg->getBumpVectorContext(), Layer)) {
+          B->appendConstructor(CE, CC, cfg->getBumpVectorContext());
           return;
         }
       }
+    }
 
     // No valid construction context found. Fall back to statement.
     B->appendStmt(CE, cfg->getBumpVectorContext());
   }
 
   void appendCall(CFGBlock *B, CallExpr *CE) {
     if (alwaysAdd(CE) && cachedEntry)
       cachedEntry->second = B;
 
     if (BuildOpts.AddRichCXXConstructors) {
       if (CFGCXXRecordTypedCall::isCXXRecordTypedCall(CE, *Context)) {
         if (const ConstructionContextLayer *Layer =
                 ConstructionContextMap.lookup(CE)) {
-          const ConstructionContext *CC =
-              ConstructionContext::createFromLayers(cfg->getBumpVectorContext(),
-                                                    Layer);
-          B->appendCXXRecordTypedCall(CE, CC, cfg->getBumpVectorContext());
           cleanupConstructionContext(CE);
+          if (const ConstructionContext *CC =
+                  ConstructionContext::createFromLayers(
+                      cfg->getBumpVectorContext(), Layer)) {
+            B->appendCXXRecordTypedCall(CE, CC, cfg->getBumpVectorContext());
             return;
           }
         }
       }
+    }
 
     // No valid construction context found. Fall back to statement.
     B->appendStmt(CE, cfg->getBumpVectorContext());
   }
 
   void appendInitializer(CFGBlock *B, CXXCtorInitializer *I) {
     B->appendInitializer(I, cfg->getBumpVectorContext());
   }

lib/Analysis/ConstructionContext.cpp

if (const auto *BTE = dyn_cast<CXXBindTemporaryExpr>(S)) {
         llvm_unreachable("Unexpected construction context with destructor!");
       }
       // A temporary object that doesn't require materialization.
       return create<TemporaryObjectConstructionContext>(C, BTE, /*MTE=*/nullptr);
     }
     if (const auto *MTE = dyn_cast<MaterializeTemporaryExpr>(S)) {
       // If the object requires destruction and is not lifetime-extended,
       // then it must have a BTE within its MTE.
-      assert(MTE->getType().getCanonicalType()
+      // FIXME: This should be an assertion.
+      if (!(MTE->getType().getCanonicalType()
                 ->getAsCXXRecordDecl()->hasTrivialDestructor() ||
-             MTE->getStorageDuration() != SD_FullExpression);
+             MTE->getStorageDuration() != SD_FullExpression))
+        return nullptr;
+
       assert(TopLayer->isLast());
       return create<TemporaryObjectConstructionContext>(C, nullptr, MTE);
     }
     if (const auto *RS = dyn_cast<ReturnStmt>(S)) {
       assert(TopLayer->isLast());
       return create<SimpleReturnedValueConstructionContext>(C, RS);
     }
     llvm_unreachable("Unexpected construction context with statement!");
   } else if (const CXXCtorInitializer *I = TopLayer->getTriggerInit()) {
     assert(TopLayer->isLast());
     return create<SimpleConstructorInitializerConstructionContext>(C, I);
   }
   llvm_unreachable("Unexpected construction context!");
 }

test/Analysis/missing-bind-temporary.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.DumpCFG %s > %t 2>&1
+// RUN: FileCheck --input-file=%t %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++14 -verify %s
+
+void clang_analyzer_eval(bool);
+
+int global;
+
+namespace variant_0 {
+// This variant of the code works correctly. Function foo() is not a template
+// function. Note that there are two destructors within foo().
+
+class A {
+public:
+  ~A() { ++global; }
+};
+
+class B {
+  A a;
+};
+
+// CHECK: void foo(int)
+// CHECK:       [B1]
+// CHECK-NEXT:    1:  (CXXConstructExpr, [B1.2], class variant_0::B)
+// CHECK-NEXT:    2: variant_0::B i;
+// CHECK-NEXT:    3: operator=
+// CHECK-NEXT:    4: [B1.3] (ImplicitCastExpr, FunctionToPointerDecay, class variant_0::B &(*)(class variant_0::B &&) noexcept)
+// CHECK-NEXT:    5: i
+// CHECK-NEXT:    6: {} (CXXConstructExpr, [B1.7], [B1.8], class variant_0::B)
+// CHECK-NEXT:    7: [B1.6] (BindTemporary)
+// CHECK-NEXT:    8: [B1.7]
+// CHECK-NEXT:    9: [B1.5] = [B1.8] (OperatorCall)
+// CHECK-NEXT:   10: ~variant_0::B() (Temporary object destructor)
+// CHECK-NEXT:   11: [B1.2].~B() (Implicit destructor)
+void foo(int) {
+  B i;
+  i = {};
+}
+
+void bar() {
+  global = 0;
+  foo(1);
+  clang_analyzer_eval(global == 2); // expected-warning{{TRUE}}
+}
+
+} // end namespace variant_0
+
+namespace variant_1 {
+// Suddenly, if we turn foo() into a template, we are missing a
+// CXXBindTemporaryExpr in the AST, and therefore we're missing a
+// temporary destructor in the CFG.
+
+class A {
+public:
+  ~A() { ++global; }
+};
+
+class B {
+  A a;
+};
+
+// FIXME: Find the construction context for {} and enforce the temporary
+// destructor.
+// CHECK: template<> void foo<int>(int)
+// CHECK:       [B1]
+// CHECK-NEXT:    1:  (CXXConstructExpr, [B1.2], class variant_1::B)
+// CHECK-NEXT:    2: variant_1::B i;
+// CHECK-NEXT:    3: operator=
+// CHECK-NEXT:    4: [B1.3] (ImplicitCastExpr, FunctionToPointerDecay, class variant_1::B &(*)(class variant_1::B &&) noexcept)
+// CHECK-NEXT:    5: i
+// CHECK-NEXT:    6: {} (CXXConstructExpr, class variant_1::B)
+// CHECK-NEXT:    7: [B1.6]
+// CHECK-NEXT:    8: [B1.5] = [B1.7] (OperatorCall)
+// CHECK-NEXT:    9: [B1.2].~B() (Implicit destructor)
+template <typename T> void foo(T) {
+  B i;
+  i = {};
+}
+
+void bar() {
+  global = 0;
+  foo(1);
+  // FIXME: Should be TRUE, i.e. we should call (and inline) two destructors.
+  clang_analyzer_eval(global == 2); // expected-warning{{UNKNOWN}}
+}
+
+} // end namespace variant_1
+
+namespace variant_2 {
+// Making field 'a' in class 'B' public turns the class into an aggregate.
+// In this case there is no constructor at {} - only an aggregate
+// initialization. Aggregate initialization is unsupported for now.
+
+class A {
+public:
+  ~A() { ++global; }
+};
+
+class B {
+public:
+  A a;
+};
+
+// CHECK: template<> void foo<int>(int)
+// CHECK:       [B1]
+// CHECK-NEXT:    1:  (CXXConstructExpr, [B1.2], class variant_2::B)
+// CHECK-NEXT:    2: variant_2::B i;
+// CHECK-NEXT:    3: operator=
+// CHECK-NEXT:    4: [B1.3] (ImplicitCastExpr, FunctionToPointerDecay, class variant_2::B &(*)(class variant_2::B &&) noexcept)
+// CHECK-NEXT:    5: i
+// CHECK-NEXT:    6: {}
+// CHECK-NEXT:    7: {}
+// CHECK-NEXT:    8: [B1.7] (BindTemporary)
+// CHECK-NEXT:    9: [B1.8]
+// CHECK-NEXT:   10: [B1.5] = [B1.9] (OperatorCall)
+// CHECK-NEXT:   11: ~variant_2::B() (Temporary object destructor)
+// CHECK-NEXT:   12: [B1.2].~B() (Implicit destructor)
+template <typename T> void foo(T) {
+  B i;
+  i = {};
+}
+
+void bar() {
+  global = 0;
+  foo(1);
+  // FIXME: Should be TRUE, i.e. we should call (and inline) two destructors.
+  clang_analyzer_eval(global == 2); // expected-warning{{UNKNOWN}}
+}
+
+} // end namespace variant_2