[analyzer] Support function argument constructors.

[NoQ]

Summary

I've got a simple example working, but there's quite a bit of work remaining on this patch for now.

Argument constructor is being called with a target region that is a parameter variable region that corresponds to the stack frame of the future function call. The region is tracked within the program state until we evaluate the call itself in order to ensure that bindings are not garbage-collected.

The annoying part is, of course, to make sure we guess the future stack frame correctly. I need to re-use some code and add some assertions to make sure i'm doing it right, and additionally it requires mapping an expression back to CFG, which i currently do via a CFGStmtMap instead of passing it directly (i'm not sure i'll be committing into finding the beautiful solution here, because there are other bigger issues for now).

The other annoying part is to match parameters to arguments, because operator calls have one of these shifted by 1, and also there are C-style variadic functions. I need to make a reusable code to make this easier.

Support for constructors of placement arguments of operator new() is still pending, even in CFG. Not hard, but also annoying.

Last but not least, once we inline the constructors, we need to invalidate the parameter region itself (not just regions reachable from it!) when the call is evaluated conservatively because https://bugs.llvm.org/show_bug.cgi?id=37459 (D48249). I marked these tests as FIXME for now but i plan to fix them before committing. That's also blocked on matching arguments to parameters.

[jordan_rose]

Cool stuff! I didn't look at things line-by-line, but here's a few comments anyway.

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

310 ↗

(On Diff #155939)

Remember that this might be null, for a function pointer call, or may not match up with the actual executed body, for a virtual call (cf. CallEvent::getRuntimeDefinition). What happens in those cases?

338 ↗

(On Diff #155939)

Are you sure this isn't just a member vs. non-member function problem? Or static vs. non-static member?

[NoQ]

Np, i only poked you because you were particularly curious about this case^^

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

310 ↗

(On Diff #155939)

Yup, that's one of the dirty parts. It seems that getDecl()'s overrides are very smart (i.e., for function pointer calls they lookup the value of the function pointer sub-expression in the Environment), but i'm not sure it works correctly in the general case. For now it's untested.

338 ↗

(On Diff #155939)

Yeah, i guess we should just solve these problems in a general case: "How do i obtain the function that's actually called", "How do i obtain its parameter that corresponds to a certain argument", etc.

This one seems to have failed only for operator calls, and they are indeed specifically squishy sometimes, but i didn't actually check it carefully.

[NoQ]

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

310 ↗

(On Diff #155939)

Ok, so facts here are as follows. getRuntimeDefinition().getDecl() returns a definition that can be inlined. If there's no such definition, it returns a null decl. getDecl(), on the other hand, always succeeds, but doesn't always return a definition, it may return a forward declaration instead.

So if we are going to inline a function, getRuntimeDefinition() is necessary. If not, getDecl() used to be sufficient because previously we only cared about number of arguments and their types, which is enough for invalidation purposes. That is, when we aren't inlining, we don't care if we're using the right override; they're all the same to us.

But for the purposes of this patch we do actually care because we want to eventually obtain the exact ParmVarDecl that corresponds to the exact function declaration that is going to be "called". Then, again, it wouldn't immediately matter because that decl isn't used for any sort of evaluation. Once we start invalidating the argument storage, as per aforementioned pr37459, we'll need to make sure that we're using the same decl here and during invalidation, but not necessarily the correct one. Finally, when we start writing checkers that check specific overrides of virtual functions, we'll need to make sure that we're in fact using the correct decl, so that when a checker gets the correct ParmVarDecl it finds the bindings it expects to see within it.

So for now Call.getRuntimeDefinition().getDecl() || Call.getDecl() is going to work, but i might end up trying to come up with a getRuntimeDeclaration() method that also takes everything into account but isn't forced to return a definition, so that we were also correct.

[NoQ]

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

338 ↗

(On Diff #155939)

Call.parameters() is pretty good, but here we have an AST problem: operator declaration is a CXXMethodDecl and it accepts this as, well, this, while CXXOperatorCallExpr is not a sub-class of CXXMemberCallExpr, so it accepts this as its first argument instead. So there's AST inconsistency that's annoying to work around.

[NoQ]

Handle the decl problem (somehow) and the operator problem (more or less, with the help of D49627), add respective tests.

Passing a non-POD object into a variadic function is implementation-defined with no requirement to work whatsoever. For the test case with bar5(7, S(7)), the AST is roughly as follows:

|     |-ExprWithCleanups 0x7f9d1b013830 <line:1041:3, col:15> 'void'
|     | `-CallExpr 0x7f9d1b012da0 <col:3, col:15> 'void'
|     |   |-ImplicitCastExpr 0x7f9d1b012d88 <col:3> 'void (*)(int, ...)' <FunctionToPointerDecay>
|     |   | `-DeclRefExpr 0x7f9d1b012d30 <col:3> 'void (int, ...)' lvalue Function 0x7f9d1b0112c0 'bar5' 'void (int, ...)'
|     |   |-IntegerLiteral 0x7f9d1b012c58 <col:8> 'int' 7
|     |   `-BinaryOperator 0x7f9d1b013808 <col:11, col:14> 'arguments::S' ','
|     |     |-CallExpr 0x7f9d1b012fe0 <col:11, col:14> 'void'
|     |     | `-ImplicitCastExpr 0x7f9d1b012fc8 <col:11> 'void (*)() __attribute__((noreturn)) throw()' <BuiltinFnToFnPtr>
|     |     |   `-DeclRefExpr 0x7f9d1b012f48 <col:11> '<builtin fn type>' Function 0x7f9d1b012e60 '__builtin_trap' 'void () __attribute__((noreturn)) throw()'
|     |     `-CXXFunctionalCastExpr 0x7f9d1b012d08 <col:11, col:14> 'arguments::S' functional cast to struct arguments::S <ConstructorConversion>
|     |       `-CXXBindTemporaryExpr 0x7f9d1b012ce8 <col:11, col:14> 'arguments::S' (CXXTemporary 0x7f9d1b012ce0)
|     |         `-CXXConstructExpr 0x7f9d1b012ca8 <col:11, col:14> 'arguments::S' 'void (int)'
|     |           `-IntegerLiteral 0x7f9d1b012c88 <col:13> 'int' 7

(notice the __builtin_trap). So we won't analyze much here. We still need to support non-variadic arguments of variadic functions, but i think this can wait.

[NoQ]

Move future stack frame lookup to CallEvent and split it out into D49715.

[NoQ]

Fix the regression in temporaries.cpp/temporaries.mm with IntPtr by invalidating parameter variable regions, as intended. Delay cleanup for that purpose. Next i'll add some lifetime extension address tests and move it out of WIP.

[NoQ]

Add tests for making sure that the address of the object doesn't jump around.

I'm removing the WIP: tag. The patch should be ready for review.

[NoQ]

Rebase. Fix a bug in isArgumentConstructedDirectly() that accidentally slipped through D49715.

[NoQ]

Stop re-binding arguments to parameter regions in enterStackFrame()->addParameterValuesToBindings() when these are constructed directly in the parameter region. This was mostly harmless because it's just re-binding the same value, but because that becomes a lazy binding, and RegionStore is known to be bad with handling partial/overlapping lazy bindings, *some* functional change is observed, as demonstrated by the test.

[NoQ]

Apparently, there are not 2 but 3 different ways of enumerating arguments/parameters: CallEvent uses its own, which keeps arguments and parameters in sync, but is incompatible with argument numbering in AST expressions.

Fix finishArgumentConstruction() to convert CallEvent index to AST index using a newly introduced helper function, getASTArgumentIndex(). Add a test.

[NoQ]

Finish argument construction when Objective-C message has a nil receiver (there's a separate code path to model this situation). Tested as testNilReceiverCleanup(). While debugging this, found a potentially hazardous piece of code in NilArgChecker and documented it.

[NoQ]

Fix getCalleeAnalysisDeclContext() to work correctly when a virtual function's definition is located below its call site and is being called on a non-region. Because definition lookup. Logic in this function is becoming really weird. I hope to get to it some day for a proper refactoring.

[NoQ]

I guess i should have skipped argument constructor support completely when the call doesn't have a runtime definition. It's not very useful anyway. But too late.

[NoQ]

Found more crashes and decided to give up on argument constructors of functions without definitions, at least for now. Because the function would not be inlined anyway, loss of precision shouldn't be that bad (unless a checker tries to model the function, in this case we'd rather have constructors working correctly). It hasn't been too hard to give up on this.

Also make sure to use the definition's parameters() instead of Call.getDecl()'s parameters(), to avoid conflicting ParmVarDecls.

I see no more known crashes and the patch seems to be working as intended.

[george.karpenkov]

Looks reasonable in general.
For all the FIXME cases would it make sense to add logging there and explicitly test for those?

lib/StaticAnalyzer/Core/CallEvent.cpp

323 ↗

(On Diff #159843)

first two if's could be joined with &&

cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

public:
   /// calling a C++ constructor over it. This is an internal detail of the
   /// analysis which doesn't necessarily represent the program semantics:
   /// if we are supposed to construct an argument directly, we may still
   /// not do that because we don't know how (i.e., construction context is
   /// unavailable in the CFG or not supported by the analyzer).
   bool isArgumentConstructedDirectly(unsigned Index) const {
     // This assumes that the object was not yet removed from the state.
     return ExprEngine::getObjectUnderConstruction(
-        getState(), {getOriginExpr(), Index}, getCalleeStackFrame()).hasValue();
+        getState(), {getOriginExpr(), Index}, getLocationContext()).hasValue();
   }
 
   /// Some calls have parameter numbering mismatched from argument numbering.
   /// This function converts an argument index to the corresponding
   /// parameter index. Returns None is the argument doesn't correspond
   /// to any parameter variable.
-  Optional<unsigned> getAdjustedParameterIndex(unsigned ArgumentIndex) const {
-    if (dyn_cast_or_null<CXXOperatorCallExpr>(getOriginExpr()) &&
-        dyn_cast_or_null<CXXMethodDecl>(getDecl())) {
-      // For member operator calls argument 0 on the expression corresponds
-      // to implicit this-parameter on the declaration.
-      return (ArgumentIndex > 0) ? Optional<unsigned>(ArgumentIndex - 1) : None;
-    }
-    return ArgumentIndex;
+  virtual Optional<unsigned>
+  getAdjustedParameterIndex(unsigned ASTArgumentIndex) const {
+    return ASTArgumentIndex;
+
+  /// Some call event sub-classes conveniently adjust mismatching AST indices
+  /// to match parameter indices. This function converts an argument index
+  /// as understood by CallEvent to the argument index as understood by the AST.
+  virtual unsigned getASTArgumentIndex(unsigned CallArgumentIndex) const {
+    return CallArgumentIndex;
   }
 
   // Iterator access to formal parameters and their types.
 private:
   struct GetTypeFn {
     QualType operator()(ParmVarDecl *PD) const { return PD->getType(); }
   };
 
public:
 
   const Expr *getCXXThisExpr() const override;
 
   Kind getKind() const override { return CE_CXXMemberOperator; }
 
   static bool classof(const CallEvent *CA) {
     return CA->getKind() == CE_CXXMemberOperator;
   }
+
+  Optional<unsigned>
+  getAdjustedParameterIndex(unsigned ASTArgumentIndex) const override {
+    // For member operator calls argument 0 on the expression corresponds
+    // to implicit this-parameter on the declaration.
+    return (ASTArgumentIndex > 0) ? Optional<unsigned>(ASTArgumentIndex - 1)
+                                  : None;
+  }
+
+  unsigned getASTArgumentIndex(unsigned CallArgumentIndex) const override {
+    // For member operator calls argument 0 on the expression corresponds
+    // to implicit this-parameter on the declaration.
+    return CallArgumentIndex + 1;
+  }
 };
 
 /// Represents an implicit call to a C++ destructor.
 ///
 /// This can occur at the end of a scope (for automatic objects), at the end
 /// of a full-expression (for temporaries), or as part of a delete.
 class CXXDestructorCall : public CXXInstanceCall {
   friend class CallEventManager;

cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h

void evalCall(ExplodedNodeSet &Dst, ExplodedNode *Pred,
                 const CallEvent &Call);
 
   /// Default implementation of call evaluation.
   void defaultEvalCall(NodeBuilder &B, ExplodedNode *Pred,
                        const CallEvent &Call,
                        const EvalCallOptions &CallOpts = {});
 
 private:
+  ProgramStateRef finishArgumentConstruction(ProgramStateRef State,
+                                             const CallEvent &Call);
+  void finishArgumentConstruction(ExplodedNodeSet &Dst, ExplodedNode *Pred,
+                                  const CallEvent &Call);
+
   void evalLoadCommon(ExplodedNodeSet &Dst,
                       const Expr *NodeEx,  /* Eventually will be a CFGStmt */
                       const Expr *BoundEx,
                       ExplodedNode *Pred,
                       ProgramStateRef St,
                       SVal location,
                       const ProgramPointTag *tag,
                       QualType LoadTy);

cfe/trunk/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp

void NilArgChecker::warnIfNilArg(CheckerContext &C,
                                  unsigned int Arg,
                                  FoundationClass Class,
                                  bool CanBeSubscript) const {
   // Check if the argument is nil.
   ProgramStateRef State = C.getState();
   if (!State->isNull(msg.getArgSVal(Arg)).isConstrainedTrue())
       return;
 
+  // NOTE: We cannot throw non-fatal errors from warnIfNilExpr,
+  // because it's called multiple times from some callers, so it'd cause
+  // an unwanted state split if two or more non-fatal errors are thrown
+  // within the same checker callback. For now we don't want to, but
+  // it'll need to be fixed if we ever want to.
   if (ExplodedNode *N = C.generateErrorNode()) {
     SmallString<128> sbuf;
     llvm::raw_svector_ostream os(sbuf);
 
     if (CanBeSubscript && msg.getMessageKind() == OCM_Subscript) {
 
       if (Class == FC_NSArray) {
         os << "Array element cannot be nil";

cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp

bool CallEvent::isGlobalCFunction(StringRef FunctionName) const {
   if (!FD)
     return false;
 
   return CheckerContext::isCLibraryFunction(FD, FunctionName);
 }
 
 AnalysisDeclContext *CallEvent::getCalleeAnalysisDeclContext() const {
   const Decl *D = getDecl();
-
-  // If the callee is completely unknown, we cannot construct the stack frame.
   if (!D)
     return nullptr;
 
-  // FIXME: Skip virtual functions for now. There's no easy procedure to foresee
-  // the exact decl that should be used, especially when it's not a definition.
-  if (const Decl *RD = getRuntimeDefinition().getDecl())
-    if (RD != D)
-      return nullptr;
+  // TODO: For now we skip functions without definitions, even if we have
+  // our own getDecl(), because it's hard to find out which re-declaration
+  // is going to be used, and usually clients don't really care about this
+  // situation because there's a loss of precision anyway because we cannot
+  // inline the call.
+  RuntimeDefinition RD = getRuntimeDefinition();
+  if (!RD.getDecl())
 
-  return LCtx->getAnalysisDeclContext()->getManager()->getContext(D);
+  AnalysisDeclContext *ADC =
+      LCtx->getAnalysisDeclContext()->getManager()->getContext(D);
+
+  // TODO: For now we skip virtual functions, because this also rises
+  // the problem of which decl to use, but now it's across different classes.
+  if (RD.mayHaveOtherDefinitions() || RD.getDecl() != ADC->getDecl())
+    return nullptr;
+
+  return ADC;
 }
 
 const StackFrameContext *CallEvent::getCalleeStackFrame() const {
   AnalysisDeclContext *ADC = getCalleeAnalysisDeclContext();
   if (!ADC)
     return nullptr;
 
   const Expr *E = getOriginExpr();

 }
 
 const VarRegion *CallEvent::getParameterLocation(unsigned Index) const {
   const StackFrameContext *SFC = getCalleeStackFrame();
   // We cannot construct a VarRegion without a stack frame.
   if (!SFC)
     return nullptr;
 
-  const ParmVarDecl *PVD = parameters()[Index];
+  // Retrieve parameters of the definition, which are different from
+  // CallEvent's parameters() because getDecl() isn't necessarily
+  // the definition. SFC contains the definition that would be used
+  // during analysis.
+  const Decl *D = SFC->getDecl();
+
+  // TODO: Refactor into a virtual method of CallEvent, like parameters().
+  const ParmVarDecl *PVD = nullptr;
+  if (const auto *FD = dyn_cast<FunctionDecl>(D))
+    PVD = FD->parameters()[Index];
+  else if (const auto *BD = dyn_cast<BlockDecl>(D))
+    PVD = BD->parameters()[Index];
+  else if (const auto *MD = dyn_cast<ObjCMethodDecl>(D))
+    PVD = MD->parameters()[Index];
+  else if (const auto *CD = dyn_cast<CXXConstructorDecl>(D))
+    PVD = CD->parameters()[Index];
+  assert(PVD && "Unexpected Decl kind!");
+
   const VarRegion *VR =
       State->getStateManager().getRegionManager().getVarRegion(PVD, SFC);
 
   // This sanity check would fail if our parameter declaration doesn't
   // correspond to the stack frame's function declaration.
   assert(VR->getStackFrame() == SFC);
 
   return VR;
for (unsigned Idx = 0, Count = getNumArgs(); Idx != Count; ++Idx) {
     // below for efficiency.
     if (PreserveArgs.count(Idx))
       if (const MemRegion *MR = getArgSVal(Idx).getAsRegion())
         ETraits.setTrait(MR->getBaseRegion(),
                         RegionAndSymbolInvalidationTraits::TK_PreserveContents);
         // TODO: Factor this out + handle the lower level const pointers.
 
     ValuesToInvalidate.push_back(getArgSVal(Idx));
+
+    // If a function accepts an object by argument (which would of course be a
+    // temporary that isn't lifetime-extended), invalidate the object itself,
+    // not only other objects reachable from it. This is necessary because the
+    // destructor has access to the temporary object after the call.
+    // TODO: Support placement arguments once we start
+    // constructing them directly.
+    // TODO: This is unnecessary when there's no destructor, but that's
+    // currently hard to figure out.
+    if (getKind() != CE_CXXAllocator)
+      if (isArgumentConstructedDirectly(Idx))
+        if (auto AdjIdx = getAdjustedParameterIndex(Idx))
+          if (const VarRegion *VR = getParameterLocation(*AdjIdx))
+            ValuesToInvalidate.push_back(loc::MemRegionVal(VR));
   }
 
   // Invalidate designated regions using the batch invalidation API.
   // NOTE: Even if RegionsToInvalidate is empty, we may still invalidate
   //  global variables.
   return Result->invalidateRegions(ValuesToInvalidate, getOriginExpr(),
                                    BlockCount, getLocationContext(),
                                    /*CausedByPointerEscape*/ true,
static void addParameterValuesToBindings(const StackFrameContext *CalleeCtx,
   // do not bind any values to them.
   unsigned NumArgs = Call.getNumArgs();
   unsigned Idx = 0;
   ArrayRef<ParmVarDecl*>::iterator I = parameters.begin(), E = parameters.end();
   for (; I != E && Idx < NumArgs; ++I, ++Idx) {
     const ParmVarDecl *ParamDecl = *I;
     assert(ParamDecl && "Formal parameter has no decl?");
 
+    if (Call.getKind() != CE_CXXAllocator)
+      if (Call.isArgumentConstructedDirectly(Idx))
+        continue;
+
     SVal ArgVal = Call.getArgSVal(Idx);
     if (!ArgVal.isUnknown()) {
       Loc ParamLoc = SVB.makeLoc(MRMgr.getVarRegion(ParamDecl, CalleeCtx));
       Bindings.push_back(std::make_pair(ParamLoc, ArgVal));
     }
   }
 
   // FIXME: Variadic arguments are not handled at all right now.

cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp

void ExprEngine::processBeginOfFunction(NodeBuilderContext &BC,
   getCheckerManager().runCheckersForBeginFunction(Dst, L, Pred, *this);
 }
 
 /// ProcessEndPath - Called by CoreEngine.  Used to generate end-of-path
 ///  nodes when the control reaches the end of a function.
 void ExprEngine::processEndOfFunction(NodeBuilderContext& BC,
                                       ExplodedNode *Pred,
                                       const ReturnStmt *RS) {
+  ProgramStateRef State = Pred->getState();
+
+  if (!Pred->getStackFrame()->inTopFrame())
+    State = finishArgumentConstruction(
+        State, *getStateManager().getCallEventManager().getCaller(
+                   Pred->getStackFrame(), Pred->getState()));
+
   // FIXME: We currently cannot assert that temporaries are clear, because
   // lifetime extended temporaries are not always modelled correctly. In some
   // cases when we materialize the temporary, we do
   // createTemporaryRegionIfNeeded(), and the region changes, and also the
   // respective destructor becomes automatic from temporary. So for now clean up
-  // the state manually before asserting. Ideally, the code above the assertion
-  // should go away, but the assertion should remain.
+  // the state manually before asserting. Ideally, this braced block of code
+  // should go away.
   {
-    ExplodedNodeSet CleanUpObjects;
-    NodeBuilder Bldr(Pred, CleanUpObjects, BC);
-    ProgramStateRef State = Pred->getState();
     const LocationContext *FromLC = Pred->getLocationContext();
     const LocationContext *ToLC = FromLC->getStackFrame()->getParent();
     const LocationContext *LC = FromLC;
     while (LC != ToLC) {
       assert(LC && "ToLC must be a parent of FromLC!");
       for (auto I : State->get<ObjectsUnderConstruction>())
         if (I.first.getLocationContext() == LC) {
           // The comment above only pardons us for not cleaning up a
           // temporary destructor. If any other statements are found here,
           // it must be a separate problem.
           assert(I.first.getItem().getKind() ==
                      ConstructionContextItem::TemporaryDestructorKind ||
                  I.first.getItem().getKind() ==
                      ConstructionContextItem::ElidedDestructorKind);
           State = State->remove<ObjectsUnderConstruction>(I.first);
         }
       LC = LC->getParent();
     }
-    if (State != Pred->getState()) {
-      Pred = Bldr.generateNode(Pred->getLocation(), State, Pred);
-      if (!Pred) {
-        // The node with clean temporaries already exists. We might have reached
-        // it on a path on which we initialize different temporaries.
-        return;
-      }
-    }
-  }
+  }
+
+  // Perform the transition with cleanups.
+    ExplodedNodeSet PostCleanup;
+    NodeBuilder Bldr(Pred, PostCleanup, BC);
+
   assert(areAllObjectsFullyConstructed(Pred->getState(),
                                        Pred->getLocationContext(),
                                        Pred->getStackFrame()->getParent()));
 
   PrettyStackTraceLocationContext CrashInfo(Pred->getLocationContext());
   StateMgr.EndPath(Pred->getState());
 
   ExplodedNodeSet Dst;

cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

case ConstructionContext::SimpleTemporaryObjectKind: {
 
       if (MTE)
         State = addObjectUnderConstruction(State, MTE, LCtx, V);
 
       CallOpts.IsTemporaryCtorOrDtor = true;
       return std::make_pair(State, V);
     }
     case ConstructionContext::ArgumentKind: {
-      // Function argument constructors. Not implemented yet.
+      // Arguments are technically temporaries.
+      CallOpts.IsTemporaryCtorOrDtor = true;
+
+      const auto *ACC = cast<ArgumentConstructionContext>(CC);
+      const Expr *E = ACC->getCallLikeExpr();
+      unsigned Idx = ACC->getIndex();
+      const CXXBindTemporaryExpr *BTE = ACC->getCXXBindTemporaryExpr();
+
+      CallEventManager &CEMgr = getStateManager().getCallEventManager();
+      SVal V = UnknownVal();
+      auto getArgLoc = [&](CallEventRef<> Caller) -> Optional<SVal> {
+        const LocationContext *FutureSFC = Caller->getCalleeStackFrame();
+        // Return early if we are unable to reliably foresee
+        // the future stack frame.
+        if (!FutureSFC)
+          return None;
+
+        // This should be equivalent to Caller->getDecl() for now, but
+        // FutureSFC->getDecl() is likely to support better stuff (like
+        // virtual functions) earlier.
+        const Decl *CalleeD = FutureSFC->getDecl();
+
+        // FIXME: Support for variadic arguments is not implemented here yet.
+        if (CallEvent::isVariadic(CalleeD))
+          return None;
+
+        // Operator arguments do not correspond to operator parameters
+        // because this-argument is implemented as a normal argument in
+        // operator call expressions but not in operator declarations.
+        const VarRegion *VR = Caller->getParameterLocation(
+            *Caller->getAdjustedParameterIndex(Idx));
+        if (!VR)
+          return None;
+
+        return loc::MemRegionVal(VR);
+      };
+
+      if (const auto *CE = dyn_cast<CallExpr>(E)) {
+        CallEventRef<> Caller = CEMgr.getSimpleCall(CE, State, LCtx);
+        if (auto OptV = getArgLoc(Caller))
+          V = *OptV;
+        else
+          break;
+        State = addObjectUnderConstruction(State, {CE, Idx}, LCtx, V);
+      } else if (const auto *CCE = dyn_cast<CXXConstructExpr>(E)) {
+        // Don't bother figuring out the target region for the future
+        // constructor because we won't need it.
+        CallEventRef<> Caller =
+            CEMgr.getCXXConstructorCall(CCE, /*Target=*/nullptr, State, LCtx);
+        if (auto OptV = getArgLoc(Caller))
+          V = *OptV;
+        else
+          break;
+        State = addObjectUnderConstruction(State, {CCE, Idx}, LCtx, V);
+      } else if (const auto *ME = dyn_cast<ObjCMessageExpr>(E)) {
+        CallEventRef<> Caller = CEMgr.getObjCMethodCall(ME, State, LCtx);
+        if (auto OptV = getArgLoc(Caller))
+          V = *OptV;
+        else
           break;
+        State = addObjectUnderConstruction(State, {ME, Idx}, LCtx, V);
+      }
+
+      assert(!V.isUnknown());
+
+      if (BTE)
+        State = addObjectUnderConstruction(State, BTE, LCtx, V);
+
+      return std::make_pair(State, V);
     }
     }
   }
   // If we couldn't find an existing region to construct into, assume we're
   // constructing a temporary. Notify the caller of our failure.
   CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion = true;
   return std::make_pair(
       State, loc::MemRegionVal(MRMgr.getCXXTempObjectRegion(E, LCtx)));
if (Target && isa<CXXTempObjectRegion>(Target) &&
 
       // There is no need to run the PostCall and PostStmt checker
       // callbacks because we just generated sinks on all nodes in th
       // frontier.
       return;
     }
   }
 
+  ExplodedNodeSet DstPostArgumentCleanup;
+  for (auto I : DstEvaluated)
+    finishArgumentConstruction(DstPostArgumentCleanup, I, *Call);
+
+  // If there were other constructors called for object-type arguments
+  // of this constructor, clean them up.
   ExplodedNodeSet DstPostCall;
-  getCheckerManager().runCheckersForPostCall(DstPostCall, DstEvaluated,
+  getCheckerManager().runCheckersForPostCall(DstPostCall,
+                                             DstPostArgumentCleanup,
                                              *Call, *this);
   getCheckerManager().runCheckersForPostStmt(destNodes, DstPostCall, CE, *this);
 }
 
 void ExprEngine::VisitCXXDestructor(QualType ObjectType,
                                     const MemRegion *Dest,
                                     const Stmt *S,
                                     bool IsBaseDtor,

cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

void ExprEngine::VisitCallExpr(const CallExpr *CE, ExplodedNode *Pred,
   // Finally, perform the post-condition check of the CallExpr and store
   // the created nodes in 'Dst'.
   // Note that if the call was inlined, dstCallEvaluated will be empty.
   // The post-CallExpr check will occur in processCallExit.
   getCheckerManager().runCheckersForPostStmt(dst, dstCallEvaluated, CE,
                                              *this);
 }
 
+ProgramStateRef ExprEngine::finishArgumentConstruction(ProgramStateRef State,
+                                                       const CallEvent &Call) {
+  const Expr *E = Call.getOriginExpr();
+  // FIXME: Constructors to placement arguments of operator new
+  // are not supported yet.
+  if (!E || isa<CXXNewExpr>(E))
+    return State;
+
+  const LocationContext *LC = Call.getLocationContext();
+  for (unsigned CallI = 0, CallN = Call.getNumArgs(); CallI != CallN; ++CallI) {
+    unsigned I = Call.getASTArgumentIndex(CallI);
+    if (Optional<SVal> V =
+            getObjectUnderConstruction(State, {E, I}, LC)) {
+      SVal VV = *V;
+      assert(cast<VarRegion>(VV.castAs<loc::MemRegionVal>().getRegion())
+                 ->getStackFrame()->getParent()
+                 ->getStackFrame() == LC->getStackFrame());
+      State = finishObjectConstruction(State, {E, I}, LC);
+    }
+  }
+
+  return State;
+}
+
+void ExprEngine::finishArgumentConstruction(ExplodedNodeSet &Dst,
+                                            ExplodedNode *Pred,
+                                            const CallEvent &Call) {
+  ProgramStateRef State = Pred->getState();
+  ProgramStateRef CleanedState = finishArgumentConstruction(State, Call);
+  if (CleanedState == State) {
+    Dst.insert(Pred);
+    return;
+  }
+
+  const Expr *E = Call.getOriginExpr();
+  const LocationContext *LC = Call.getLocationContext();
+  NodeBuilder B(Pred, Dst, *currBldrCtx);
+  static SimpleProgramPointTag Tag("ExprEngine",
+                                   "Finish argument construction");
+  PreStmt PP(E, LC, &Tag);
+  B.generateNode(PP, CleanedState, Pred);
+}
+
 void ExprEngine::evalCall(ExplodedNodeSet &Dst, ExplodedNode *Pred,
                           const CallEvent &Call) {
   // WARNING: At this time, the state attached to 'Call' may be older than the
   // state in 'Pred'. This is a minor optimization since CheckerManager will
   // use an updated CallEvent instance when calling checkers, but if 'Call' is
   // ever used directly in this function all callers should be updated to pass
   // the most recent state. (It is probably not worth doing the work here since
   // for some callers this will not be necessary.)
 
   // Run any pre-call checks using the generic call interface.
   ExplodedNodeSet dstPreVisit;
-  getCheckerManager().runCheckersForPreCall(dstPreVisit, Pred, Call, *this);
+  getCheckerManager().runCheckersForPreCall(dstPreVisit, Pred,
+                                            Call, *this);
 
   // Actually evaluate the function call.  We try each of the checkers
   // to see if the can evaluate the function call, and get a callback at
   // defaultEvalCall if all of them fail.
   ExplodedNodeSet dstCallEvaluated;
   getCheckerManager().runCheckersForEvalCall(dstCallEvaluated, dstPreVisit,
                                              Call, *this);
 
+  // If there were other constructors called for object-type arguments
+  // of this call, clean them up.
+  ExplodedNodeSet dstArgumentCleanup;
+  for (auto I : dstCallEvaluated)
+    finishArgumentConstruction(dstArgumentCleanup, I, Call);
+
   // Finally, run any post-call checks.
-  getCheckerManager().runCheckersForPostCall(Dst, dstCallEvaluated,
+  getCheckerManager().runCheckersForPostCall(Dst, dstArgumentCleanup,
                                              Call, *this);
 }
 
 ProgramStateRef ExprEngine::bindReturnValue(const CallEvent &Call,
                                             const LocationContext *LCtx,
                                             ProgramStateRef State) {
   const Expr *E = Call.getOriginExpr();
   if (!E)

cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp

if (!recVal.isUndef()) {
           recVal.castAs<DefinedOrUnknownSVal>();
       ProgramStateRef State = Pred->getState();
 
       ProgramStateRef notNilState, nilState;
       std::tie(notNilState, nilState) = State->assume(receiverVal);
 
       // Receiver is definitely nil, so run ObjCMessageNil callbacks and return.
       if (nilState && !notNilState) {
-        StmtNodeBuilder Bldr(Pred, Dst, *currBldrCtx);
+        ExplodedNodeSet dstNil;
+        StmtNodeBuilder Bldr(Pred, dstNil, *currBldrCtx);
         bool HasTag = Pred->getLocation().getTag();
         Pred = Bldr.generateNode(ME, Pred, nilState, nullptr,
                                  ProgramPoint::PreStmtKind);
         assert((Pred || HasTag) && "Should have cached out already!");
         (void)HasTag;
         if (!Pred)
           return;
-        getCheckerManager().runCheckersForObjCMessageNil(Dst, Pred,
+
+        ExplodedNodeSet dstPostCheckers;
+        getCheckerManager().runCheckersForObjCMessageNil(dstPostCheckers, Pred,
                                                          *Msg, *this);
+        for (auto I : dstPostCheckers)
+          finishArgumentConstruction(Dst, I, *Msg);
         return;
       }
 
       ExplodedNodeSet dstNonNil;
       StmtNodeBuilder Bldr(Pred, dstNonNil, *currBldrCtx);
       // Generate a transition to the non-nil state, dropping any potential
       // nil flow.
       if (notNilState != State) {
if (UpdatedMsg->isInstanceMessage()) {
         Bldr.generateSink(ME, Pred, Pred->getState());
         continue;
       }
     }
 
     defaultEvalCall(Bldr, Pred, *UpdatedMsg);
   }
 
+  // If there were constructors called for object-type arguments, clean them up.
+  ExplodedNodeSet dstArgCleanup;
+  for (auto I : dstEval)
+    finishArgumentConstruction(dstArgCleanup, I, *Msg);
+
   ExplodedNodeSet dstPostvisit;
-  getCheckerManager().runCheckersForPostCall(dstPostvisit, dstEval,
+  getCheckerManager().runCheckersForPostCall(dstPostvisit, dstArgCleanup,
                                              *Msg, *this);
 
   // Finally, perform the post-condition check of the ObjCMessageExpr and store
   // the created nodes in 'Dst'.
   getCheckerManager().runCheckersForPostObjCMessage(Dst, dstPostvisit,
                                                     *Msg, *this);
 }

cfe/trunk/test/Analysis/copy-elision.cpp

void foo(int coin) {
   }
 }
 } // namespace elision_on_ternary_op_branches
 
 
 namespace address_vector_tests {
 
 template <typename T> struct AddressVector {
-  T *buf[10];
+  T *buf[20];
   int len;
 
   AddressVector() : len(0) {}
 
   void push(T *t) {
     buf[len] = t;
     ++len;
   }
 };
 
 class ClassWithoutDestructor {
   AddressVector<ClassWithoutDestructor> &v;
 
 public:
   ClassWithoutDestructor(AddressVector<ClassWithoutDestructor> &v) : v(v) {
-    v.push(this);
+    push();
   }
 
-  ClassWithoutDestructor(ClassWithoutDestructor &&c) : v(c.v) { v.push(this); }
-  ClassWithoutDestructor(const ClassWithoutDestructor &c) : v(c.v) {
-    v.push(this);
-  }
+  ClassWithoutDestructor(ClassWithoutDestructor &&c) : v(c.v) { push(); }
+  ClassWithoutDestructor(const ClassWithoutDestructor &c) : v(c.v) { push(); }
+
+  void push() { v.push(this); }
 };
 
 ClassWithoutDestructor make1(AddressVector<ClassWithoutDestructor> &v) {
   return ClassWithoutDestructor(v);
 }
 ClassWithoutDestructor make2(AddressVector<ClassWithoutDestructor> &v) {
   return make1(v);
 }
#else
   clang_analyzer_eval(v.buf[0] != v.buf[1]); // expected-warning{{TRUE}}
   clang_analyzer_eval(v.buf[1] != v.buf[2]); // expected-warning{{TRUE}}
   clang_analyzer_eval(v.buf[2] != v.buf[3]); // expected-warning{{TRUE}}
   clang_analyzer_eval(v.buf[3] != v.buf[4]); // expected-warning{{TRUE}}
   clang_analyzer_eval(v.buf[4] == &c); // expected-warning{{TRUE}}
 #endif
 }
 
+void consume(ClassWithoutDestructor c) {
+  c.push();
+}
+
+void testArgumentConstructorWithoutDestructor() {
+  AddressVector<ClassWithoutDestructor> v;
+
+  consume(make3(v));
+
+#if ELIDE
+  clang_analyzer_eval(v.len == 2); // expected-warning{{TRUE}}
+  clang_analyzer_eval(v.buf[0] == v.buf[1]); // expected-warning{{TRUE}}
+#else
+  clang_analyzer_eval(v.len == 6); // expected-warning{{TRUE}}
+  clang_analyzer_eval(v.buf[0] != v.buf[1]); // expected-warning{{TRUE}}
+  clang_analyzer_eval(v.buf[1] != v.buf[2]); // expected-warning{{TRUE}}
+  clang_analyzer_eval(v.buf[2] != v.buf[3]); // expected-warning{{TRUE}}
+  clang_analyzer_eval(v.buf[3] != v.buf[4]); // expected-warning{{TRUE}}
+  // We forced a push() in consume(), let's see if the address here matches
+  // the address during construction.
+  clang_analyzer_eval(v.buf[4] == v.buf[5]); // expected-warning{{TRUE}}
+#endif
+}
+
 class ClassWithDestructor {
   AddressVector<ClassWithDestructor> &v;
 
 public:
   ClassWithDestructor(AddressVector<ClassWithDestructor> &v) : v(v) {
-    v.push(this);
+    push();
   }
 
-  ClassWithDestructor(ClassWithDestructor &&c) : v(c.v) { v.push(this); }
-  ClassWithDestructor(const ClassWithDestructor &c) : v(c.v) {
-    v.push(this);
-  }
+  ClassWithDestructor(ClassWithDestructor &&c) : v(c.v) { push(); }
+  ClassWithDestructor(const ClassWithDestructor &c) : v(c.v) { push(); }
+
+  ~ClassWithDestructor() { push(); }
 
-  ~ClassWithDestructor() { v.push(this); }
+  void push() { v.push(this); }
 };
 
 void testVariable() {
   AddressVector<ClassWithDestructor> v;
   {
     ClassWithDestructor c = ClassWithDestructor(v);
     // Check if the last destructor is an automatic destructor.
     // A temporary destructor would have fired by now.
#else
   clang_analyzer_eval(v.len == 10); // expected-warning{{TRUE}}
   clang_analyzer_eval(v.buf[0] == v.buf[2]); // expected-warning{{TRUE}}
   clang_analyzer_eval(v.buf[1] == v.buf[4]); // expected-warning{{TRUE}}
   clang_analyzer_eval(v.buf[3] == v.buf[6]); // expected-warning{{TRUE}}
   clang_analyzer_eval(v.buf[5] == v.buf[8]); // expected-warning{{TRUE}}
   clang_analyzer_eval(v.buf[7] == v.buf[9]); // expected-warning{{TRUE}}
 #endif
 }
+
+void consume(ClassWithDestructor c) {
+  c.push();
+}
+
+void testArgumentConstructorWithDestructor() {
+  AddressVector<ClassWithDestructor> v;
+
+  consume(make3(v));
+
+#if ELIDE
+  // 0. Construct the argument.
+  // 1. Forced push() in consume().
+  // 2. Destroy the argument.
+  clang_analyzer_eval(v.len == 3); // expected-warning{{TRUE}}
+  clang_analyzer_eval(v.buf[0] == v.buf[1]); // expected-warning{{TRUE}}
+  clang_analyzer_eval(v.buf[1] == v.buf[2]); // expected-warning{{TRUE}}
+#else
+  // 0. Construct the temporary in make1().
+  // 1. Construct the temporary in make2().
+  // 2. Destroy the temporary in make1().
+  // 3. Construct the temporary in make3().
+  // 4. Destroy the temporary in make2().
+  // 5. Construct the temporary here.
+  // 6. Destroy the temporary in make3().
+  // 7. Construct the argument.
+  // 8. Forced push() in consume().
+  // 9. Destroy the argument. Notice the reverse order!
+  // 10. Destroy the temporary here.
+  clang_analyzer_eval(v.len == 11); // expected-warning{{TRUE}}
+  clang_analyzer_eval(v.buf[0] == v.buf[2]); // expected-warning{{TRUE}}
+  clang_analyzer_eval(v.buf[1] == v.buf[4]); // expected-warning{{TRUE}}
+  clang_analyzer_eval(v.buf[3] == v.buf[6]); // expected-warning{{TRUE}}
+  clang_analyzer_eval(v.buf[5] == v.buf[10]); // expected-warning{{TRUE}}
+  clang_analyzer_eval(v.buf[7] == v.buf[8]); // expected-warning{{TRUE}}
+  clang_analyzer_eval(v.buf[8] == v.buf[9]); // expected-warning{{TRUE}}
+#endif
+}
+
 } // namespace address_vector_tests

cfe/trunk/test/Analysis/temporaries.cpp

-// 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
+// RUN: %clang_analyze_cc1 -Wno-non-pod-varargs -analyzer-checker=core,cplusplus,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++03 %s
+// RUN: %clang_analyze_cc1 -Wno-non-pod-varargs -analyzer-checker=core,cplusplus,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++11 %s
+// RUN: %clang_analyze_cc1 -Wno-non-pod-varargs -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 -Wno-non-pod-varargs -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";

 
 // 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 arguments {
+int glob;
+
+struct S {
+  int x;
+  S(int x): x(x) {}
+  S(const S &s) : x(s.x) {}
+  ~S() {}
+
+  S &operator+(S s) {
+    glob = s.x;
+    x += s.x;
+    return *this;
+  }
+};
+
+class C {
+public:
+  virtual void bar3(S s) {}
+};
+
+class D: public C {
+public:
+  D() {}
+  virtual void bar3(S s) override { glob = s.x; }
+};
+
+void bar1(S s) {
+  glob = s.x;
+}
+
+// Record-typed calls are a different CFGStmt, let's see if we handle that
+// as well.
+S bar2(S s) {
+  glob = s.x;
+  return S(3);
+}
+
+void bar5(int, ...);
+
+void foo(void (*bar4)(S)) {
+  bar1(S(1));
+  clang_analyzer_eval(glob == 1);
+#ifdef TEMPORARY_DTORS
+  // expected-warning@-2{{TRUE}}
+#else
+  // expected-warning@-4{{UNKNOWN}}
+#endif
+
+  bar2(S(2));
+  clang_analyzer_eval(glob == 2);
+#ifdef TEMPORARY_DTORS
+  // expected-warning@-2{{TRUE}}
+#else
+  // expected-warning@-4{{UNKNOWN}}
+#endif
+
+  C *c = new D();
+  c->bar3(S(3));
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob == 3); // expected-warning{{UNKNOWN}}
+  delete c;
+
+  // What if we've no idea what we're calling?
+  bar4(S(4)); // no-crash
+
+  S(5) + S(6);
+  clang_analyzer_eval(glob == 6);
+#ifdef TEMPORARY_DTORS
+  // expected-warning@-2{{TRUE}}
+#else
+  // expected-warning@-4{{UNKNOWN}}
+#endif
+
+  // Variadic functions. This will __builtin_trap() because you cannot pass
+  // an object as a variadic argument.
+  bar5(7, S(7)); // no-crash
+  clang_analyzer_warnIfReached(); // no-warning
+}
+} // namespace arguments
+
 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;
if (S(false)) {
     clang_analyzer_warnIfReached(); // no-warning
   }
   if (S(true)) {
     clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
   }
 }
 } // namespace operator_implicit_argument
 
+
+#if __cplusplus >= 201103L
+namespace argument_lazy_bindings {
+int glob;
+
+struct S {
+  int x, y, z;
+};
+
+struct T {
+  S s;
+  int w;
+  T(int w): s{5, 6, 7}, w(w) {}
+};
+
+void foo(T t) {
+  t.s = {1, 2, 3};
+  glob = t.w;
+}
+
+void bar() {
+  foo(T(4));
+  clang_analyzer_eval(glob == 4); // expected-warning{{TRUE}}
+}
+} // namespace argument_lazy_bindings
+#endif
+
+namespace operator_argument_cleanup {
+struct S {
+  S();
+};
+
+class C {
+public:
+  void operator=(S);
+};
+
+void foo() {
+  C c;
+  c = S(); // no-crash
+}
+} // namespace operator_argument_cleanup
+
+namespace argument_decl_lookup {
+class C {};
+int foo(C);
+int bar(C c) { foo(c); }
+int foo(C c) {}
+} // namespace argument_decl_lookup
+
+namespace argument_virtual_decl_lookup {
+class C {};
+
+struct T  {
+  virtual void foo(C);
+};
+
+void run() {
+  T *t;
+  t->foo(C()); // no-crash // expected-warning{{Called C++ object pointer is uninitialized}}
+}
+
+// This is after run() because the test is about picking the correct decl
+// for the parameter region, which should belong to the correct function decl,
+// and the non-definition decl should be found by direct lookup.
+void T::foo(C) {}
+} // namespace argument_virtual_decl_lookup

cfe/trunk/test/Analysis/temporaries.mm

 // RUN: %clang_analyze_cc1 -analyzer-checker core,cplusplus -verify %s
 
 // expected-no-diagnostics
 
+#define nil ((id)0)
+
 // 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) {
+void testArgumentRegionInvalidation(Foo *f) {
   IntPtr ptr;
   int *i = ptr.i;
   [f foo: static_cast<IntPtr &&>(ptr)];
   *i = 99; // no-warning
 }
+
+void testNilReceiverCleanup() {
+  [nil foo: IntPtr()];
+}