[CFG] [analyzer] Add construction contexts for returning C++ objects in ObjC++.

Like any normal funciton, Objective-C message can return a C++ object
in Objective-C++. Such object would require a construction context.

This patch, therefore, is an extension of r327343 onto Objective-C++.

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


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

include/clang/Analysis/CFG.h

@@ -15,9 +15,10 @@
 #ifndef LLVM_CLANG_ANALYSIS_CFG_H
 #define LLVM_CLANG_ANALYSIS_CFG_H
 
-#include "clang/AST/ExprCXX.h"
 #include "clang/Analysis/Support/BumpVector.h"
 #include "clang/Analysis/ConstructionContext.h"
+#include "clang/AST/ExprCXX.h"
+#include "clang/AST/ExprObjC.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/GraphTraits.h"
@@ -179,15 +180,18 @@ class CFGCXXRecordTypedCall : public CFGStmt {
 public:
   /// Returns true when call expression \p CE needs to be represented
   /// by CFGCXXRecordTypedCall, as opposed to a regular CFGStmt.
-  static bool isCXXRecordTypedCall(CallExpr *CE, const ASTContext &ACtx) {
+  static bool isCXXRecordTypedCall(Expr *E) {
-    return CE->getCallReturnType(ACtx).getCanonicalType()->getAsCXXRecordDecl();
+    assert(isa<CallExpr>(E) || isa<ObjCMessageExpr>(E));
+    // There is no such thing as reference-type expression. If the function
+    // returns a reference, it'll return the respective lvalue or xvalue
+    // instead, and we're only interested in objects.
+    return !E->isGLValue() &&
+           E->getType().getCanonicalType()->getAsCXXRecordDecl();
   }
 
-  explicit CFGCXXRecordTypedCall(CallExpr *CE, const ConstructionContext *C)
+  explicit CFGCXXRecordTypedCall(Expr *E, const ConstructionContext *C)
-      : CFGStmt(CE, CXXRecordTypedCall) {
+      : CFGStmt(E, CXXRecordTypedCall) {
-    // FIXME: This is not protected against squeezing a non-record-typed-call
+    assert(isCXXRecordTypedCall(E));
-    // into the constructor. An assertion would require passing an ASTContext
-    // which would mean paying for something we don't use.
     assert(C && (isa<TemporaryObjectConstructionContext>(C) ||
                  // These are possible in C++17 due to mandatory copy elision.
                  isa<ReturnedValueConstructionContext>(C) ||
@@ -874,10 +878,10 @@ public:
     Elements.push_back(CFGConstructor(CE, CC), C);
   }
 
-  void appendCXXRecordTypedCall(CallExpr *CE,
+  void appendCXXRecordTypedCall(Expr *E,
                                 const ConstructionContext *CC,
                                 BumpVectorContext &C) {
-    Elements.push_back(CFGCXXRecordTypedCall(CE, CC), C);
+    Elements.push_back(CFGCXXRecordTypedCall(E, CC), C);
   }
 
   void appendInitializer(CXXCtorInitializer *initializer,

lib/Analysis/CFG.cpp

@@ -743,6 +743,19 @@ private:
 
   void addLocalScopeAndDtors(Stmt *S);
 
+  const ConstructionContext *retrieveAndCleanupConstructionContext(Expr *E) {
+    if (!BuildOpts.AddRichCXXConstructors)
+      return nullptr;
+
+    const ConstructionContextLayer *Layer = ConstructionContextMap.lookup(E);
+    if (!Layer)
+      return nullptr;
+
+    cleanupConstructionContext(E);
+    return ConstructionContext::createFromLayers(cfg->getBumpVectorContext(),
+                                                 Layer);
+  }
+
   // Interface to CFGBlock - adding CFGElements.
 
   void appendStmt(CFGBlock *B, const Stmt *S) {
@@ -755,17 +768,11 @@ private:
   }
 
   void appendConstructor(CFGBlock *B, CXXConstructExpr *CE) {
-    if (BuildOpts.AddRichCXXConstructors) {
+    if (const ConstructionContext *CC =
-      if (const ConstructionContextLayer *Layer =
+            retrieveAndCleanupConstructionContext(CE)) {
-              ConstructionContextMap.lookup(CE)) {
-        cleanupConstructionContext(CE);
-        if (const auto *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());
@@ -775,19 +782,11 @@ private:
     if (alwaysAdd(CE) && cachedEntry)
       cachedEntry->second = B;
 
-    if (BuildOpts.AddRichCXXConstructors) {
+    if (const ConstructionContext *CC =
-      if (CFGCXXRecordTypedCall::isCXXRecordTypedCall(CE, *Context)) {
+            retrieveAndCleanupConstructionContext(CE)) {
-        if (const ConstructionContextLayer *Layer =
-                ConstructionContextMap.lookup(CE)) {
-          cleanupConstructionContext(CE);
-          if (const auto *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());
@@ -809,6 +808,20 @@ private:
     B->appendMemberDtor(FD, cfg->getBumpVectorContext());
   }
 
+  void appendObjCMessage(CFGBlock *B, ObjCMessageExpr *ME) {
+    if (alwaysAdd(ME) && cachedEntry)
+      cachedEntry->second = B;
+
+    if (const ConstructionContext *CC =
+            retrieveAndCleanupConstructionContext(ME)) {
+      B->appendCXXRecordTypedCall(ME, CC, cfg->getBumpVectorContext());
+      return;
+    }
+
+    B->appendStmt(const_cast<ObjCMessageExpr *>(ME),
+                  cfg->getBumpVectorContext());
+  }
+
   void appendTemporaryDtor(CFGBlock *B, CXXBindTemporaryExpr *E) {
     B->appendTemporaryDtor(E, cfg->getBumpVectorContext());
   }
@@ -1254,6 +1267,8 @@ static const VariableArrayType *FindVA(const Type *t) {
 
 void CFGBuilder::consumeConstructionContext(
     const ConstructionContextLayer *Layer, Expr *E) {
+  assert((isa<CXXConstructExpr>(E) || isa<CallExpr>(E) ||
+          isa<ObjCMessageExpr>(E)) && "Expression cannot construct an object!");
   if (const ConstructionContextLayer *PreviouslyStoredLayer =
           ConstructionContextMap.lookup(E)) {
     (void)PreviouslyStoredLayer;
@@ -1297,10 +1312,11 @@ void CFGBuilder::findConstructionContexts(
   case Stmt::CallExprClass:
   case Stmt::CXXMemberCallExprClass:
   case Stmt::CXXOperatorCallExprClass:
-  case Stmt::UserDefinedLiteralClass: {
+  case Stmt::UserDefinedLiteralClass:
-    auto *CE = cast<CallExpr>(Child);
+  case Stmt::ObjCMessageExprClass: {
-    if (CFGCXXRecordTypedCall::isCXXRecordTypedCall(CE, *Context))
+    auto *E = cast<Expr>(Child);
-      consumeConstructionContext(Layer, CE);
+    if (CFGCXXRecordTypedCall::isCXXRecordTypedCall(E))
+      consumeConstructionContext(Layer, E);
     break;
   }
   case Stmt::ExprWithCleanupsClass: {
@@ -3605,7 +3621,7 @@ CFGBlock *CFGBuilder::VisitObjCMessageExpr(ObjCMessageExpr *ME,
   findConstructionContextsForArguments(ME);
 
   autoCreateBlock();
-  appendStmt(Block, ME);
+  appendObjCMessage(Block, ME);
 
   return VisitChildren(ME);
 }

test/Analysis/cfg-rich-constructors.mm

@@ -15,6 +15,7 @@ public:
 
 @interface E {}
 -(void) foo: (D) d;
+-(D) bar;
 @end
 
 // FIXME: Find construction context for the argument.
@@ -39,3 +40,27 @@ public:
 void passArgumentIntoMessage(E *e) {
   [e foo: D()];
 }
+
+// CHECK: void returnObjectFromMessage(E *e)
+// CHECK:        [B1]
+// CHECK-NEXT:     1: e
+// CHECK-NEXT:     2: [B1.1] (ImplicitCastExpr, LValueToRValue, E *)
+// Double brackets trigger FileCheck variables, escape.
+// CXX11-ELIDE-NEXT:     3: {{\[}}[B1.2] bar] (CXXRecordTypedCall, [B1.4], [B1.6], [B1.7])
+// CXX11-NOELIDE-NEXT:     3: {{\[}}[B1.2] bar] (CXXRecordTypedCall, [B1.4], [B1.6])
+// CXX11-NEXT:     4: [B1.3] (BindTemporary)
+// CXX11-NEXT:     5: [B1.4] (ImplicitCastExpr, NoOp, const class D)
+// CXX11-NEXT:     6: [B1.5]
+// CXX11-NEXT:     7: [B1.6] (CXXConstructExpr, [B1.8], class D)
+// CXX11-NEXT:     8: D d = [e bar];
+// CXX11-NEXT:     9: ~D() (Temporary object destructor)
+// CXX11-NEXT:    10: [B1.8].~D() (Implicit destructor)
+// Double brackets trigger FileCheck variables, escape.
+// CXX17-NEXT:     3: {{\[}}[B1.2] bar] (CXXRecordTypedCall, [B1.5], [B1.4])
+// CXX17-NEXT:     4: [B1.3] (BindTemporary)
+// CXX17-NEXT:     5: D d = [e bar];
+// CXX17-NEXT:     6: ~D() (Temporary object destructor)
+// CXX17-NEXT:     7: [B1.5].~D() (Implicit destructor)
+void returnObjectFromMessage(E *e) {
+  D d = [e bar];
+}

test/Analysis/lifetime-extension.mm

@@ -0,0 +1,64 @@
+// RUN: %clang_analyze_cc1 -Wno-unused -std=c++11 -analyzer-checker=core,debug.ExprInspection -verify %s
+// RUN: %clang_analyze_cc1 -Wno-unused -std=c++17 -analyzer-checker=core,debug.ExprInspection -verify %s
+// RUN: %clang_analyze_cc1 -Wno-unused -std=c++11 -analyzer-checker=core,debug.ExprInspection -DMOVES -verify %s
+// RUN: %clang_analyze_cc1 -Wno-unused -std=c++17 -analyzer-checker=core,debug.ExprInspection -DMOVES -verify %s
+
+void clang_analyzer_eval(bool);
+void clang_analyzer_checkInlined(bool);
+
+template <typename T> struct AddressVector {
+  T *buf[10];
+  int len;
+
+  AddressVector() : len(0) {}
+
+  void push(T *t) {
+    buf[len] = t;
+    ++len;
+  }
+};
+
+class C {
+  AddressVector<C> &v;
+
+public:
+  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
+};
+
+@interface NSObject {}
+@end;
+@interface Foo: NSObject {}
+  -(C) make: (AddressVector<C> &)v;
+@end
+
+@implementation Foo
+-(C) make: (AddressVector<C> &)v {
+  return C(v);
+}
+@end
+
+void testReturnByValueFromMessage(Foo *foo) {
+  AddressVector<C> v;
+  {
+    const C &c = [foo make: v];
+  }
+  // 0. Construct the return value of -make (copy/move elided) and
+  //    lifetime-extend it directly via reference 'c',
+  // 1. Destroy the temporary lifetime-extended by 'c'.
+  clang_analyzer_eval(v.len == 2); // expected-warning{{TRUE}}
+  clang_analyzer_eval(v.buf[0] == v.buf[1]); // expected-warning{{TRUE}}
+}