[CFG] [analyzer] Add stubs for argument construction contexts for arguments of C++ constructors and Objective-C messages.

[NoQ]

Summary

This is similar to D45650. Constructors of pass-by-value arguments aren't similar to other temporary constructors, we should treat them differently. For now we don't handle them, so we shouldn't produce temporary-like construction contexts for them. D45650 fixed this bug for call-expressions, but construct-expressions and Objective-C message expressions both aren't call-expressions, so they need a separate fix.

[NoQ]

Whoops, that was an old patch.

[george.karpenkov]

lib/Analysis/CFG.cpp

2414

Would appreciate a top-level comment that this is specifically for constructors passing arguments by value.

4287

I think we can do better then duplicating the same code block three times with an exact comment as well. I do understand that it's painful due to call expressions not having a common superclass, but maybe it would be sufficient to just have a function iterating over arguments?

[NoQ]

Code re-use!

[NoQ]

Actually, yeah, add the comment.

[george.karpenkov]

Minor nit: request for code reuse.

lib/Analysis/CFG.cpp

694

Who needs inheritance if one has templates.
This is pretty bad though, I wish we could add another interface to those three, if anything just to have arguments()

lib/Analysis/ConstructionContext.cpp

115

Could we refactor the check into isCallable or whatever would be the appropriate name here?

[NoQ]

Committed in rC338425 but i accidentally screwed up the revision link.

lib/Analysis/CFG.cpp

CFGBlock *VisitMaterializeTemporaryExpr(MaterializeTemporaryExpr *MTE,
                                           AddStmtChoice asc);
   CFGBlock *VisitMemberExpr(MemberExpr *M, AddStmtChoice asc);
   CFGBlock *VisitObjCAtCatchStmt(ObjCAtCatchStmt *S);
   CFGBlock *VisitObjCAtSynchronizedStmt(ObjCAtSynchronizedStmt *S);
   CFGBlock *VisitObjCAtThrowStmt(ObjCAtThrowStmt *S);
   CFGBlock *VisitObjCAtTryStmt(ObjCAtTryStmt *S);
   CFGBlock *VisitObjCAutoreleasePoolStmt(ObjCAutoreleasePoolStmt *S);
   CFGBlock *VisitObjCForCollectionStmt(ObjCForCollectionStmt *S);
+  CFGBlock *VisitObjCMessageExpr(ObjCMessageExpr *E, AddStmtChoice asc);
   CFGBlock *VisitPseudoObjectExpr(PseudoObjectExpr *E);
   CFGBlock *VisitReturnStmt(ReturnStmt *R);
   CFGBlock *VisitSEHExceptStmt(SEHExceptStmt *S);
   CFGBlock *VisitSEHFinallyStmt(SEHFinallyStmt *S);
   CFGBlock *VisitSEHLeaveStmt(SEHLeaveStmt *S);
   CFGBlock *VisitSEHTryStmt(SEHTryStmt *S);
   CFGBlock *VisitStmtExpr(StmtExpr *S, AddStmtChoice asc);
   CFGBlock *VisitSwitchStmt(SwitchStmt *S);
private:
   // Scan \p Child statement to find constructors in it, while keeping in mind
   // that its parent statement is providing a partial construction context
   // described by \p Layer. If a constructor is found, it would be assigned
   // the context based on the layer. If an additional construction context layer
   // is found, the function recurses into that.
   void findConstructionContexts(const ConstructionContextLayer *Layer,
                                 Stmt *Child);
 
+  // Scan all arguments of a call expression for a construction context.
+  // These sorts of call expressions don't have a common superclass,
+  // hence strict duck-typing.
+  template <typename CallLikeExpr,
+            typename = typename std::enable_if<
+                std::is_same<CallLikeExpr, CallExpr>::value ||
+                std::is_same<CallLikeExpr, CXXConstructExpr>::value ||
+                std::is_same<CallLikeExpr, ObjCMessageExpr>::value>>
+  void findConstructionContextsForArguments(CallLikeExpr *E) {
+    // A stub for the code that'll eventually be used for finding construction
+    // contexts for constructors of C++ object-type arguments passed into
+    // call-like expression E.
+    // FIXME: Once actually implemented, this construction context layer should
+    // include the index of the argument as well.
+    for (auto Arg : E->arguments())
+      if (Arg->getType()->getAsCXXRecordDecl() && !Arg->isGLValue())
+        findConstructionContexts(
+            ConstructionContextLayer::create(cfg->getBumpVectorContext(), E),
+            Arg);
+  }
+
   // Unset the construction context after consuming it. This is done immediately
   // after adding the CFGConstructor or CFGCXXRecordTypedCall element, so
   // there's no need to do this manually in every Visit... function.
   void cleanupConstructionContext(Expr *E);
 
   void autoCreateBlock() { if (!Block) Block = createBlock(); }
   CFGBlock *createBlock(bool add_successor = true);
   CFGBlock *createNoReturnBlock();
case Stmt::ObjCAtThrowStmtClass:
       return VisitObjCAtThrowStmt(cast<ObjCAtThrowStmt>(S));
 
     case Stmt::ObjCAtTryStmtClass:
       return VisitObjCAtTryStmt(cast<ObjCAtTryStmt>(S));
 
     case Stmt::ObjCForCollectionStmtClass:
       return VisitObjCForCollectionStmt(cast<ObjCForCollectionStmt>(S));
 
+    case Stmt::ObjCMessageExprClass:
+      return VisitObjCMessageExpr(cast<ObjCMessageExpr>(S), asc);
+
     case Stmt::OpaqueValueExprClass:
       return Block;
 
     case Stmt::PseudoObjectExprClass:
       return VisitPseudoObjectExpr(cast<PseudoObjectExpr>(S));
 
     case Stmt::ReturnStmtClass:
       return VisitReturnStmt(cast<ReturnStmt>(S));
CFGBlock *CFGBuilder::VisitCallExpr(CallExpr *C, AddStmtChoice asc) {
   if (calleeType == Context->BoundMemberTy) {
     QualType boundType = Expr::findBoundMemberType(C->getCallee());
 
     // We should only get a null bound type if processing a dependent
     // CFG.  Recover by assuming nothing.
     if (!boundType.isNull()) calleeType = boundType;
   }
 
-  // FIXME: Once actually implemented, this construction context layer should
-  // include the number of the argument as well.
-  for (auto Arg: C->arguments()) {
-    findConstructionContexts(
-        ConstructionContextLayer::create(cfg->getBumpVectorContext(), C), Arg);
-  }
+  findConstructionContextsForArguments(C);
 
   // If this is a call to a no-return function, this stops the block here.
   bool NoReturn = getFunctionExtInfo(*calleeType).getNoReturn();
 
   bool AddEHEdge = false;
 
   // Languages without exceptions are assumed to not throw.
   if (Context->getLangOpts().Exceptions) {
     if (BuildOpts.AddEHEdges)
       AddEHEdge = true;
CFGBlock *CFGBuilder::VisitObjCAtThrowStmt(ObjCAtThrowStmt *S) {
   // The Exit block is the only successor.
   addSuccessor(Block, &cfg->getExit());
 
   // Add the statement to the block.  This may create new blocks if S contains
   // control-flow (short-circuit operations).
   return VisitStmt(S, AddStmtChoice::AlwaysAdd);
 }
 
+CFGBlock *CFGBuilder::VisitObjCMessageExpr(ObjCMessageExpr *ME,
+                                           AddStmtChoice asc) {
+  findConstructionContextsForArguments(ME);
+
+  autoCreateBlock();
+  appendStmt(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;
 
   // Create the new block.
   Block = createBlock(false);
 
if (asc.alwaysAdd(*this, E)) {
     // We do not want to propagate the AlwaysAdd property.
     asc = asc.withAlwaysAdd(false);
   }
   return Visit(E->getSubExpr(), asc);
 }
 
 CFGBlock *CFGBuilder::VisitCXXConstructExpr(CXXConstructExpr *C,
                                             AddStmtChoice asc) {
+  // If the constructor takes objects as arguments by value, we need to properly
+  // construct these objects. Construction contexts we find here aren't for the
+  // constructor C, they're for its arguments only.
+  findConstructionContextsForArguments(C);
+
   autoCreateBlock();
   appendConstructor(Block, C);
 
   return VisitChildren(C);
 }
 
 CFGBlock *CFGBuilder::VisitCXXNewExpr(CXXNewExpr *NE,
                                       AddStmtChoice asc) {
   autoCreateBlock();
   appendStmt(Block, NE);
 
   findConstructionContexts(
       ConstructionContextLayer::create(cfg->getBumpVectorContext(), NE),

lib/Analysis/ConstructionContext.cpp

 // This file defines the ConstructionContext class and its sub-classes,
 // which represent various different ways of constructing C++ objects
 // with the additional information the users may want to know about
 // the constructor.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Analysis/ConstructionContext.h"
+#include "clang/AST/ExprObjC.h"
 
 using namespace clang;
 
 const ConstructionContextLayer *
 ConstructionContextLayer::create(BumpVectorContext &C, TriggerTy Trigger,
                                  const ConstructionContextLayer *Parent) {
   ConstructionContextLayer *CC =
       C.getAllocator().Allocate<ConstructionContextLayer>();
if (const auto *BTE = dyn_cast<CXXBindTemporaryExpr>(S)) {
             }
           }
           assert(ParentLayer->isLast());
           return create<SimpleTemporaryObjectConstructionContext>(C, BTE, MTE);
         }
         assert(ParentLayer->isLast());
 
         // This is a constructor into a function argument. Not implemented yet.
-        if (isa<CallExpr>(ParentLayer->getTriggerStmt()))
+        if (isa<CallExpr>(ParentLayer->getTriggerStmt()) ||
+            isa<CXXConstructExpr>(ParentLayer->getTriggerStmt()) ||
+            isa<ObjCMessageExpr>(ParentLayer->getTriggerStmt()))
           return nullptr;
         // This is C++17 copy-elided construction into return statement.
         if (auto *RS = dyn_cast<ReturnStmt>(ParentLayer->getTriggerStmt())) {
           assert(!RS->getRetValue()->getType().getCanonicalType()
                     ->getAsCXXRecordDecl()->hasTrivialDestructor());
           return create<CXX17ElidedCopyReturnedValueConstructionContext>(C,
                                                                        RS, BTE);
         }
if (const auto *MTE = dyn_cast<MaterializeTemporaryExpr>(S)) {
       assert(TopLayer->isLast());
       return create<SimpleTemporaryObjectConstructionContext>(C, nullptr, MTE);
     }
     if (const auto *RS = dyn_cast<ReturnStmt>(S)) {
       assert(TopLayer->isLast());
       return create<SimpleReturnedValueConstructionContext>(C, RS);
     }
     // This is a constructor into a function argument. Not implemented yet.
-    if (isa<CallExpr>(TopLayer->getTriggerStmt()))
+    if (isa<CallExpr>(TopLayer->getTriggerStmt()) ||
+        isa<CXXConstructExpr>(TopLayer->getTriggerStmt()) ||
+        isa<ObjCMessageExpr>(TopLayer->getTriggerStmt()))
       return nullptr;
     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/cfg-rich-constructors.cpp

 
 namespace argument_constructors {
 class D {
 public:
   D();
   ~D();
 };
 
+class E {
+public:
+  E(D d);
+};
+
 void useC(C c);
 void useCByReference(const C &c);
 void useD(D d);
 void useDByReference(const D &d);
 
 // FIXME: Find construction context for the argument.
 // CHECK: void passArgument()
 // CHECK:          1: useC

 // CHECK-NEXT:     4: [B1.3] (BindTemporary)
 // CHECK-NEXT:     5: [B1.4] (ImplicitCastExpr, NoOp, const class argument_constructors::D)
 // CHECK-NEXT:     6: [B1.5]
 // CHECK-NEXT:     7: [B1.2]([B1.6])
 // CHECK-NEXT:     8: ~argument_constructors::D() (Temporary object destructor)
 void passArgumentWithDestructorByReference() {
   useDByReference(D());
 }
+
+// FIXME: Find construction context for the argument.
+// CHECK: void passArgumentIntoAnotherConstructor()
+// CXX11:          1: argument_constructors::D() (CXXConstructExpr, [B1.2], [B1.4], class argument_constructors::D)
+// CXX11-NEXT:     2: [B1.1] (BindTemporary)
+// CXX11-NEXT:     3: [B1.2] (ImplicitCastExpr, NoOp, const class argument_constructors::D)
+// CXX11-NEXT:     4: [B1.3]
+// CXX11-NEXT:     5: [B1.4] (CXXConstructExpr, class argument_constructors::D)
+// CXX11-NEXT:     6: [B1.5] (BindTemporary)
+// CXX11-ELIDE-NEXT:     7: [B1.6] (CXXConstructExpr, [B1.9], [B1.10], class argument_constructors::E)
+// CXX11-NOELIDE-NEXT:     7: [B1.6] (CXXConstructExpr, [B1.9], class argument_constructors::E)
+// CXX11-NEXT:     8: argument_constructors::E([B1.7]) (CXXFunctionalCastExpr, ConstructorConversion, class argument_constructors::E)
+// CXX11-NEXT:     9: [B1.8]
+// CXX11-NEXT:    10: [B1.9] (CXXConstructExpr, [B1.11], class argument_constructors::E)
+// CXX11-NEXT:    11: argument_constructors::E e = argument_constructors::E(argument_constructors::D());
+// CXX11-NEXT:    12: ~argument_constructors::D() (Temporary object destructor)
+// CXX11-NEXT:    13: ~argument_constructors::D() (Temporary object destructor)
+// CXX17:          1: argument_constructors::D() (CXXConstructExpr, class argument_constructors::D)
+// CXX17-NEXT:     2: [B1.1] (BindTemporary)
+// CXX17-NEXT:     3: [B1.2] (CXXConstructExpr, [B1.5], class argument_constructors::E)
+// CXX17-NEXT:     4: argument_constructors::E([B1.3]) (CXXFunctionalCastExpr, ConstructorConversion, class argument_constructors::E)
+// CXX17-NEXT:     5: argument_constructors::E e = argument_constructors::E(argument_constructors::D());
+// CXX17-NEXT:     6: ~argument_constructors::D() (Temporary object destructor)
+void passArgumentIntoAnotherConstructor() {
+  E e = E(D());
+}
 } // end namespace argument_constructors

test/Analysis/cfg-rich-constructors.mm

+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.DumpCFG -triple x86_64-apple-darwin12 -std=c++11 -w %s > %t 2>&1
+// RUN: FileCheck --input-file=%t -check-prefixes=CHECK,CXX11,ELIDE,CXX11-ELIDE %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.DumpCFG -triple x86_64-apple-darwin12 -std=c++17 -w %s > %t 2>&1
+// RUN: FileCheck --input-file=%t -check-prefixes=CHECK,CXX17,ELIDE,CXX17-ELIDE %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.DumpCFG -triple x86_64-apple-darwin12 -std=c++11 -w -analyzer-config elide-constructors=false %s > %t 2>&1
+// RUN: FileCheck --input-file=%t -check-prefixes=CHECK,CXX11,NOELIDE,CXX11-NOELIDE %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.DumpCFG -triple x86_64-apple-darwin12 -std=c++17 -w -analyzer-config elide-constructors=false %s > %t 2>&1
+// RUN: FileCheck --input-file=%t -check-prefixes=CHECK,CXX17,NOELIDE,CXX17-NOELIDE %s
+
+class D {
+public:
+  D();
+  ~D();
+};
+
+@interface E {}
+-(void) foo: (D) d;
+@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()];
+}

test/Analysis/temporaries.cpp

-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++03 %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++11 %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -DTEMPORARY_DTORS -verify -w -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true %s -std=c++11
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -DTEMPORARY_DTORS -w -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true %s -std=c++17
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++03 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++11 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus,debug.ExprInspection -DTEMPORARY_DTORS -verify -w -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true %s -std=c++11
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus,debug.ExprInspection -DTEMPORARY_DTORS -w -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true %s -std=c++17
 
 // Note: The C++17 run-line doesn't -verify yet - it is a no-crash test.
 
 extern bool clang_analyzer_eval(bool);
 extern bool clang_analyzer_warnIfReached();
 void clang_analyzer_checkInlined(bool);
 
 #include "Inputs/system-header-simulator-cxx.h";

 const C &foo1();
 C &&foo2();
 
 // In these examples the foo() expression has record type, not reference type.
 // Don't try to figure out how to perform construction of the record here.
 const C &bar1() { return foo1(); } // no-crash
 C &&bar2() { return foo2(); } // no-crash
 } // end namespace pass_references_through
+
+
+namespace ctor_argument {
+// Stripped down unique_ptr<int>
+struct IntPtr {
+  IntPtr(): i(new int) {}
+  IntPtr(IntPtr &&o): i(o.i) { o.i = 0; }
+  ~IntPtr() { delete i; }
+
+  int *i;
+};
+
+struct Foo {
+  Foo(IntPtr);
+  void bar();
+
+  IntPtr i;
+};
+
+void bar() {
+  IntPtr ptr;
+  int *i = ptr.i;
+  Foo f(static_cast<IntPtr &&>(ptr));
+  *i = 99; // no-warning
+}
+} // namespace ctor_argument

test/Analysis/temporaries.mm

+// RUN: %clang_analyze_cc1 -analyzer-checker core,cplusplus -verify %s
+
+// expected-no-diagnostics
+
+// Stripped down unique_ptr<int>
+struct IntPtr {
+  IntPtr(): i(new int) {}
+  IntPtr(IntPtr &&o): i(o.i) { o.i = nullptr; }
+  ~IntPtr() { delete i; }
+
+  int *i;
+};
+
+@interface Foo {}
+  -(void) foo: (IntPtr)arg;
+@end
+
+void bar(Foo *f) {
+  IntPtr ptr;
+  int *i = ptr.i;
+  [f foo: static_cast<IntPtr &&>(ptr)];
+  *i = 99; // no-warning
+}