[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

309

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?

337

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

309

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.

337

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

309

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

337

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

297

first two if's could be joined with &&

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);

lib/StaticAnalyzer/Core/CallEvent.cpp

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,

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;

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,

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 I = 0, N = Call.parameters().size(); I != N; ++I) {
+    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)

lib/StaticAnalyzer/Core/ExprEngineObjC.cpp

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);
 }

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

test/Analysis/explain-svals.cpp

void test_5(int i) {
     clang_analyzer_explain(this); // expected-warning-re{{{{^pointer to 'this' object$}}}}
     clang_analyzer_explain(&x[i]); // expected-warning-re{{{{^pointer to element of type 'int' with index 'argument 'i'' of field 'x' of 'this' object$}}}}
     clang_analyzer_explain(__builtin_alloca(i)); // expected-warning-re{{{{^pointer to region allocated by '__builtin_alloca\(i\)'$}}}}
   }
 };
 } // end of anonymous namespace
 
 void test_6() {
-  clang_analyzer_explain(conjure_S()); // expected-warning-re{{{{^lazily frozen compound value of temporary object constructed at statement 'conjure_S\(\)'$}}}}
+  clang_analyzer_explain(conjure_S()); // expected-warning-re{{{{^lazily frozen compound value of parameter ''}}}}
   clang_analyzer_explain(conjure_S().z); // expected-warning-re{{{{^value derived from \(symbol of type 'int' conjured at statement 'conjure_S\(\)'\) for field 'z' of temporary object constructed at statement 'conjure_S\(\)'$}}}}
 }

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;