[CFG] [analyzer] Add construction contexts for C++ objects returned from Objective-C messages.

[NoQ]

Summary

This patch extends D44120 to Objective-C messages that can also sometimes return C++ objects (in Objective-C++), but aren't inheriting from CallExpr. We'll now be able to properly destroy temporaries returned from such messages and/or materialize them. The analyzer picks up the newly added context automatically, as demonstrated by the new test case.

I removed the getCallReturnType() check because most of its branches aren't useful in our case.

[george.karpenkov]

Minor nits: would like some more comments.

include/clang/Analysis/CFG.h

185

Can we have a comment on why are filtering out GLValues here?

lib/Analysis/CFG.cpp

815

It's tempting to factor this tower of if's into a function returning bool.
Then an appropriate name could be used to indicate semantics, which I'm currently not sure about.

819

Do we want to cleanup construction context here even if the condition below is not met?

[NoQ]

Code re-use!

And the comment.

[NoQ]

lib/Analysis/CFG.cpp

819

Yeah, most certainly. Failure to create construction context from existing layers indicates that we've encountered a valid set of layers that's currently not supported. We still need to inform the rest of the builder that these layers were taken into account rather than leaked.

include/clang/Analysis/CFG.h

 //  This file defines the CFG and CFGBuilder classes for representing and
 //  building Control-Flow Graphs (CFGs) from ASTs.
 //
 //===----------------------------------------------------------------------===//
 
 #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"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/PointerIntPair.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/Allocator.h"

 /// storage of the returned object . In C such tracking is not necessary because
 /// no additional effort is required for destroying the object or modeling copy
 /// elision. Like CFGConstructor, this element is for now only used by the
 /// analyzer's CFG.
 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) {
-    return CE->getCallReturnType(ACtx).getCanonicalType()->getAsCXXRecordDecl();
+  static bool isCXXRecordTypedCall(Expr *E) {
+    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)
-      : CFGStmt(CE, CXXRecordTypedCall) {
-    // FIXME: This is not protected against squeezing a non-record-typed-call
-    // into the constructor. An assertion would require passing an ASTContext
-    // which would mean paying for something we don't use.
+  explicit CFGCXXRecordTypedCall(Expr *E, const ConstructionContext *C)
+      : CFGStmt(E, CXXRecordTypedCall) {
+    assert(isCXXRecordTypedCall(E));
     assert(C && (isa<TemporaryObjectConstructionContext>(C) ||
                  // These are possible in C++17 due to mandatory copy elision.
                  isa<ReturnedValueConstructionContext>(C) ||
                  isa<VariableConstructionContext>(C) ||
                  isa<ConstructorInitializerConstructionContext>(C)));
     Data2.setPointer(const_cast<ConstructionContext *>(C));
   }
 
void appendStmt(Stmt *statement, BumpVectorContext &C) {
     Elements.push_back(CFGStmt(statement), C);
   }
 
   void appendConstructor(CXXConstructExpr *CE, const ConstructionContext *CC,
                          BumpVectorContext &C) {
     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,
                         BumpVectorContext &C) {
     Elements.push_back(CFGInitializer(initializer), C);
   }
 
   void appendNewAllocator(CXXNewExpr *NE,

lib/Analysis/CFG.cpp

private:
 
   void addLocalScopeForStmt(Stmt *S);
   LocalScope* addLocalScopeForDeclStmt(DeclStmt *DS,
                                        LocalScope* Scope = nullptr);
   LocalScope* addLocalScopeForVarDecl(VarDecl *VD, LocalScope* Scope = nullptr);
 
   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) {
     if (alwaysAdd(S) && cachedEntry)
       cachedEntry->second = B;
 
     // All block-level expressions should have already been IgnoreParens()ed.
     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)) {
-        cleanupConstructionContext(CE);
-        if (const auto *CC = ConstructionContext::createFromLayers(
-                cfg->getBumpVectorContext(), Layer)) {
-          B->appendConstructor(CE, CC, cfg->getBumpVectorContext());
-          return;
-        }
-      }
-    }
+    if (const ConstructionContext *CC =
+            retrieveAndCleanupConstructionContext(CE)) {
 
     // 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)) {
-          cleanupConstructionContext(CE);
-          if (const auto *CC = ConstructionContext::createFromLayers(
-                  cfg->getBumpVectorContext(), Layer)) {
-            B->appendCXXRecordTypedCall(CE, CC, cfg->getBumpVectorContext());
-            return;
-          }
-        }
-      }
-    }
+    if (const ConstructionContext *CC =
+            retrieveAndCleanupConstructionContext(CE)) {
 
     // 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());
   }
 
   void appendNewAllocator(CFGBlock *B, CXXNewExpr *NE) {
     B->appendNewAllocator(NE, cfg->getBumpVectorContext());
   }
 
   void appendBaseDtor(CFGBlock *B, const CXXBaseSpecifier *BS) {
     B->appendBaseDtor(BS, cfg->getBumpVectorContext());
   }
 
   void appendMemberDtor(CFGBlock *B, FieldDecl *FD) {
     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());
   }
 
   void appendAutomaticObjDtor(CFGBlock *B, VarDecl *VD, Stmt *S) {
     B->appendAutomaticObjDtor(VD, S, cfg->getBumpVectorContext());
   }
 
while (const ArrayType *vt = dyn_cast<ArrayType>(t)) {
     t = vt->getElementType().getTypePtr();
   }
 
   return nullptr;
 }
 
 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;
     // We might have visited this child when we were finding construction
     // contexts within its parents.
     assert(PreviouslyStoredLayer->isStrictlyMoreSpecificThan(Layer) &&
            "Already within a different construction context!");
   } else {
case Stmt::CXXTemporaryObjectExprClass: {
     break;
   }
   // FIXME: This, like the main visit, doesn't support CUDAKernelCallExpr.
   // FIXME: An isa<> would look much better but this whole switch is a
   // workaround for an internal compiler error in MSVC 2015 (see r326021).
   case Stmt::CallExprClass:
   case Stmt::CXXMemberCallExprClass:
   case Stmt::CXXOperatorCallExprClass:
-  case Stmt::UserDefinedLiteralClass: {
-    auto *CE = cast<CallExpr>(Child);
-    if (CFGCXXRecordTypedCall::isCXXRecordTypedCall(CE, *Context))
-      consumeConstructionContext(Layer, CE);
+  case Stmt::UserDefinedLiteralClass:
+  case Stmt::ObjCMessageExprClass: {
+    auto *E = cast<Expr>(Child);
+    if (CFGCXXRecordTypedCall::isCXXRecordTypedCall(E))
+      consumeConstructionContext(Layer, E);
     break;
   }
   case Stmt::ExprWithCleanupsClass: {
     auto *Cleanups = cast<ExprWithCleanups>(Child);
     findConstructionContexts(Layer, Cleanups->getSubExpr());
     break;
   }
   case Stmt::CXXFunctionalCastExprClass: {
CFGBlock *CFGBuilder::VisitObjCAtThrowStmt(ObjCAtThrowStmt *S) {
   return VisitStmt(S, AddStmtChoice::AlwaysAdd);
 }
 
 CFGBlock *CFGBuilder::VisitObjCMessageExpr(ObjCMessageExpr *ME,
                                            AddStmtChoice asc) {
   findConstructionContextsForArguments(ME);
 
   autoCreateBlock();
-  appendStmt(Block, ME);
+  appendObjCMessage(Block, ME);
 
   return VisitChildren(ME);
 }
 
 CFGBlock *CFGBuilder::VisitCXXThrowExpr(CXXThrowExpr *T) {
   // If we were in the middle of a block we stop processing that block.
   if (badCFG)
     return nullptr;

test/Analysis/cfg-rich-constructors.mm

 class D {
 public:
   D();
   ~D();
 };
 
 @interface E {}
 -(void) foo: (D) d;
+-(D) bar;
 @end
 
 // FIXME: Find construction context for the argument.
 // CHECK: void passArgumentIntoMessage(E *e)
 // CHECK:          1: e
 // CHECK-NEXT:     2: [B1.1] (ImplicitCastExpr, LValueToRValue, E *)
 // CXX11-NEXT:     3: D() (CXXConstructExpr, [B1.4], [B1.6], class D)
 // 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, class D)
 // CXX11-NEXT:     8: [B1.7] (BindTemporary)
 // Double brackets trigger FileCheck variables, escape.
 // CXX11-NEXT:     9: {{\[}}[B1.2] foo:[B1.8]]
 // CXX11-NEXT:    10: ~D() (Temporary object destructor)
 // CXX11-NEXT:    11: ~D() (Temporary object destructor)
 // CXX17-NEXT:     3: D() (CXXConstructExpr, class D)
 // CXX17-NEXT:     4: [B1.3] (BindTemporary)
 // Double brackets trigger FileCheck variables, escape.
 // CXX17-NEXT:     5: {{\[}}[B1.2] foo:[B1.4]]
 // CXX17-NEXT:     6: ~D() (Temporary object destructor)
 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

+// 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}}
+}