[analyzer] NFC: Merge the functions for obtaining constructed object location and storing this location for later use.

[NoQ]

Summary

getLocationForConstructedObject() looks at the construction context and figures out what region should represent the object.

markStatementsCorrespondingToConstructedObject() looks at the construction context and figures out what statements will need to retrieve that region directly later.

These functions are coupled and code is duplicated between them. They should be merged. The resulting function is large, so it'd probably later need to be split in a different manner (i.e. by construction context kinds). It'll also soon become recursive as we add better support for copy elision at return sites. I really hope we don't end up coding any sort of ConstructionContextVisitor.

No functional change intended here; this is a refactoring pass.

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

private:
 
   /// For a DeclStmt or CXXInitCtorInitializer, walk backward in the current CFG
   /// block to find the constructor expression that directly constructed into
   /// the storage for this statement. Returns null if the constructor for this
   /// statement created a temporary object region rather than directly
   /// constructing into an existing region.
   const CXXConstructExpr *findDirectConstructorForCurrentCFGElement();
 
-  /// For a given constructor, look forward in the current CFG block to
-  /// determine the region into which an object will be constructed by \p CE.
-  /// When the lookahead fails, a temporary region is returned, and the
+  /// Update the program state with all the path-sensitive information
+  /// that's necessary to perform construction of an object with a given
+  /// syntactic construction context. If the construction context is unavailable
+  /// or unusable for any reason, a dummy temporary region is returned, and the
   /// IsConstructorWithImproperlyModeledTargetRegion flag is set in \p CallOpts.
-  SVal getLocationForConstructedObject(const CXXConstructExpr *CE,
-                                       ExplodedNode *Pred,
-                                       const ConstructionContext *CC,
-                                       EvalCallOptions &CallOpts);
+  /// Returns the updated program state and the new object's this-region.
+  std::pair<ProgramStateRef, SVal> prepareForObjectConstruction(
+      const Expr *E, ProgramStateRef State, const LocationContext *LCtx,
+      const ConstructionContext *CC, EvalCallOptions &CallOpts);
 
   /// Store the location of a C++ object corresponding to a statement
   /// until the statement is actually encountered. For example, if a DeclStmt
   /// has CXXConstructExpr as its initializer, the object would be considered
   /// to be "under construction" between CXXConstructExpr and DeclStmt.
   /// This allows, among other things, to keep bindings to variable's fields
   /// made within the constructor alive until its declaration actually
   /// goes into scope.
static Optional<SVal> getObjectUnderConstruction(ProgramStateRef State,
                                                    const LocationContext *LC);
 
   /// Check if all objects under construction have been fully constructed
   /// for the given context range (including FromLC, not including ToLC).
   /// This is useful for assertions.
   static bool areAllObjectsFullyConstructed(ProgramStateRef State,
                                             const LocationContext *FromLC,
                                             const LocationContext *ToLC);
-
-  /// Adds an initialized temporary and/or a materialization, whichever is
-  /// necessary, by looking at the whole construction context. Handles
-  /// function return values, which need the construction context of the parent
-  /// stack frame, automagically.
-  ProgramStateRef markStatementsCorrespondingToConstructedObject(
-      ProgramStateRef State, const ConstructionContext *CC,
-      const LocationContext *LC, SVal V);
 };
 
 /// Traits for storing the call processing policy inside GDM.
 /// The GDM stores the corresponding CallExpr pointer.
 // FIXME: This does not use the nice trait macros because it must be accessible
 // from multiple translation units.
 struct ReplayWithoutInlining{};
 template <>

lib/StaticAnalyzer/Core/ExprEngine.cpp

for (auto I : State->get<ObjectsUnderConstruction>())
       if (I.first.second == LC)
         return false;
 
     LC = LC->getParent();
   }
   return true;
 }
 
-ProgramStateRef ExprEngine::markStatementsCorrespondingToConstructedObject(
-    ProgramStateRef State, const ConstructionContext *CC,
-    const LocationContext *LC, SVal V) {
-  if (CC) {
-    // If the temporary is being returned from the function, it will be
-    // destroyed or lifetime-extended in the caller stack frame.
-    if (isa<ReturnedValueConstructionContext>(CC)) {
-      const StackFrameContext *SFC = LC->getCurrentStackFrame();
-      assert(SFC);
-      LC = SFC->getParent();
-      if (!LC) {
-        // We are on the top frame. We won't ever need any info
-        // for this temporary, so don't set anything.
-        return State;
-      }
-      const CFGElement &CallElem =
-          (*SFC->getCallSiteBlock())[SFC->getIndex()];
-      auto RTCElem = CallElem.getAs<CFGCXXRecordTypedCall>();
-      if (!RTCElem) {
-        // We have a parent stack frame, but no construction context for the
-        // return value. Give up until we provide the construction context
-        // at the call site.
-        return State;
-      }
-      // We use the ReturnedValueConstructionContext as an indication that we
-      // need to look for the actual construction context on the parent stack
-      // frame. This purpose has been fulfilled, so now we replace CC with the
-      // actual construction context.
-      CC = RTCElem->getConstructionContext();
-      if (!isa<TemporaryObjectConstructionContext>(CC)) {
-        // TODO: We are not returning an object into a temporary. There must
-        // be copy elision happening at the call site. We still need to
-        // explicitly support the situation when the return value is put
-        // into another return statement, i.e.
-        // ReturnedValueConstructionContexts are chained through multiple
-        // stack frames before finally settling in a temporary.
-        // We don't seem to need to explicitly support construction into
-        // a variable after a return.
-        return State;
-      }
-      // Proceed to deal with the temporary we've found on the parent
-      // stack frame.
-    }
-    // In case of temporary object construction, extract data necessary for
-    // destruction and lifetime extension.
-    if (const auto *TCC = dyn_cast<TemporaryObjectConstructionContext>(CC)) {
-      if (AMgr.getAnalyzerOptions().includeTemporaryDtorsInCFG()) {
-        const CXXBindTemporaryExpr *BTE =
-            TCC->getCXXBindTemporaryExpr();
-        const MaterializeTemporaryExpr *MTE =
-            TCC->getMaterializedTemporaryExpr();
-        if (!BTE) {
-          // FIXME: Lifetime extension for temporaries without destructors
-          // is not implemented yet.
-          MTE = nullptr;
-        }
-        if (MTE && MTE->getStorageDuration() != SD_FullExpression) {
-          // If the temporary is lifetime-extended, don't save the BTE,
-          // because we don't need a temporary destructor, but an automatic
-          // destructor.
-          BTE = nullptr;
-        }
-
-        if (BTE)
-          State = addObjectUnderConstruction(State, BTE, LC, V);
-
-        if (MTE)
-          State = addObjectUnderConstruction(State, MTE, LC, V);
-      }
-    }
-  }
-
-  return State;
-}
-
 
 //===----------------------------------------------------------------------===//
 // Top-level transfer function logic (Dispatcher).
 //===----------------------------------------------------------------------===//
 
 /// evalAssume - Called by ConstraintManager. Used to call checker-specific
 ///  logic for handling assumptions on symbolic values.
 ProgramStateRef ExprEngine::processAssume(ProgramStateRef state,

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

while (const ArrayType *AT = Ctx.getAsArrayType(Ty)) {
     Ty = AT->getElementType();
     LValue = State->getLValue(Ty, SVB.makeZeroArrayIndex(), LValue);
     IsArray = true;
   }
 
   return LValue;
 }
 
-
-SVal ExprEngine::getLocationForConstructedObject(const CXXConstructExpr *CE,
-                                                 ExplodedNode *Pred,
-                                                 const ConstructionContext *CC,
-                                                 EvalCallOptions &CallOpts) {
-  const LocationContext *LCtx = Pred->getLocationContext();
-  ProgramStateRef State = Pred->getState();
+std::pair<ProgramStateRef, SVal> ExprEngine::prepareForObjectConstruction(
+    const Expr *E, ProgramStateRef State, const LocationContext *LCtx,
+    const ConstructionContext *CC, EvalCallOptions &CallOpts) {
   MemRegionManager &MRMgr = getSValBuilder().getRegionManager();
 
   // See if we're constructing an existing region by looking at the
   // current construction context.
   if (CC) {
     switch (CC->getKind()) {
     case ConstructionContext::SimpleVariableKind: {
       const auto *DSCC = cast<SimpleVariableConstructionContext>(CC);
       const auto *DS = DSCC->getDeclStmt();
       const auto *Var = cast<VarDecl>(DS->getSingleDecl());
       SVal LValue = State->getLValue(Var, LCtx);
       QualType Ty = Var->getType();
-      return makeZeroElementRegion(State, LValue, Ty,
-                                   CallOpts.IsArrayCtorOrDtor);
+      return std::make_pair(
+          State,
+          makeZeroElementRegion(State, LValue, Ty, CallOpts.IsArrayCtorOrDtor));
     }
     case ConstructionContext::SimpleConstructorInitializerKind: {
       const auto *ICC = cast<ConstructorInitializerConstructionContext>(CC);
       const auto *Init = ICC->getCXXCtorInitializer();
       assert(Init->isAnyMemberInitializer());
       const CXXMethodDecl *CurCtor = cast<CXXMethodDecl>(LCtx->getDecl());
       Loc ThisPtr =
       getSValBuilder().getCXXThis(CurCtor, LCtx->getCurrentStackFrame());
       SVal ThisVal = State->getSVal(ThisPtr);
 
       const ValueDecl *Field;
       SVal FieldVal;
       if (Init->isIndirectMemberInitializer()) {
         Field = Init->getIndirectMember();
         FieldVal = State->getLValue(Init->getIndirectMember(), ThisVal);
       } else {
         Field = Init->getMember();
         FieldVal = State->getLValue(Init->getMember(), ThisVal);
       }
 
       QualType Ty = Field->getType();
       FieldVal = makeZeroElementRegion(State, FieldVal, Ty,
                                        CallOpts.IsArrayCtorOrDtor);
-      return FieldVal;
+      return std::make_pair(State, FieldVal);
     }
     case ConstructionContext::NewAllocatedObjectKind: {
       if (AMgr.getAnalyzerOptions().mayInlineCXXAllocator()) {
         const auto *NECC = cast<NewAllocatedObjectConstructionContext>(CC);
         const auto *NE = NECC->getCXXNewExpr();
         SVal V = *getObjectUnderConstruction(State, NE, LCtx);
         if (const SubRegion *MR =
                 dyn_cast_or_null<SubRegion>(V.getAsRegion())) {
           if (NE->isArray()) {
             // TODO: In fact, we need to call the constructor for every
             // allocated element, not just the first one!
             CallOpts.IsArrayCtorOrDtor = true;
-            return loc::MemRegionVal(getStoreManager().GetElementZeroRegion(
-                MR, NE->getType()->getPointeeType()));
+            return std::make_pair(
+                State, loc::MemRegionVal(getStoreManager().GetElementZeroRegion(
+                           MR, NE->getType()->getPointeeType())));
           }
-          return V;
+          return std::make_pair(State, V);
         }
         // TODO: Detect when the allocator returns a null pointer.
         // Constructor shall not be called in this case.
       }
       break;
     }
-    case ConstructionContext::TemporaryObjectKind: {
-      const auto *TOCC = cast<TemporaryObjectConstructionContext>(CC);
-      if (const auto *MTE = TOCC->getMaterializedTemporaryExpr()) {
-        if (const ValueDecl *VD = MTE->getExtendingDecl()) {
-          assert(MTE->getStorageDuration() != SD_FullExpression);
-          if (!VD->getType()->isReferenceType()) {
-            // We're lifetime-extended by a surrounding aggregate.
-            // Automatic destructors aren't quite working in this case
-            // on the CFG side. We should warn the caller about that.
-            // FIXME: Is there a better way to retrieve this information from
-            // the MaterializeTemporaryExpr?
-            CallOpts.IsTemporaryLifetimeExtendedViaAggregate = true;
-          }
-        }
-      }
-      // TODO: Support temporaries lifetime-extended via static references.
-      // They'd need a getCXXStaticTempObjectRegion().
-      CallOpts.IsTemporaryCtorOrDtor = true;
-      return loc::MemRegionVal(MRMgr.getCXXTempObjectRegion(CE, LCtx));
-    }
     case ConstructionContext::SimpleReturnedValueKind: {
       // The temporary is to be managed by the parent stack frame.
       // So build it in the parent stack frame if we're not in the
       // top frame of the analysis.
-      // TODO: What exactly happens when we are? Does the temporary object live
-      // long enough in the region store in this case? Would checkers think
-      // that this object immediately goes out of scope?
-      const LocationContext *TempLCtx = LCtx;
       const StackFrameContext *SFC = LCtx->getCurrentStackFrame();
       if (const LocationContext *CallerLCtx = SFC->getParent()) {
         auto RTC = (*SFC->getCallSiteBlock())[SFC->getIndex()]
                        .getAs<CFGCXXRecordTypedCall>();
         if (!RTC) {
           // We were unable to find the correct construction context for the
           // call in the parent stack frame. This is equivalent to not being
           // able to find construction context at all.
-          CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion = true;
+          break;
         } else if (!isa<TemporaryObjectConstructionContext>(
                        RTC->getConstructionContext())) {
           // FIXME: The return value is constructed directly into a
           // non-temporary due to C++17 mandatory copy elision. This is not
           // implemented yet.
           assert(getContext().getLangOpts().CPlusPlus17);
-          CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion = true;
+          break;
+        }
+        CC = RTC->getConstructionContext();
+        LCtx = CallerLCtx;
+      } else {
+        // We are on the top frame of the analysis.
+        // TODO: What exactly happens when we are? Does the temporary object
+        // live long enough in the region store in this case? Would checkers
+        // think that this object immediately goes out of scope?
+        CallOpts.IsTemporaryCtorOrDtor = true;
+        SVal V = loc::MemRegionVal(MRMgr.getCXXTempObjectRegion(E, LCtx));
+        return std::make_pair(State, V);
+      }
+
+      // Continue as if we have a temporary with a different location context.
+      // FALLTHROUGH.
+    }
+    case ConstructionContext::TemporaryObjectKind: {
+      const auto *TCC = cast<TemporaryObjectConstructionContext>(CC);
+      const CXXBindTemporaryExpr *BTE = TCC->getCXXBindTemporaryExpr();
+      const MaterializeTemporaryExpr *MTE = TCC->getMaterializedTemporaryExpr();
+
+      if (!BTE) {
+        // FIXME: Lifetime extension for temporaries without destructors
+        // is not implemented yet.
+        MTE = nullptr;
+      }
+
+      if (MTE) {
+        if (const ValueDecl *VD = MTE->getExtendingDecl()) {
+          assert(MTE->getStorageDuration() != SD_FullExpression);
+          if (!VD->getType()->isReferenceType()) {
+            // We're lifetime-extended by a surrounding aggregate.
+            // Automatic destructors aren't quite working in this case
+            // on the CFG side. We should warn the caller about that.
+            // FIXME: Is there a better way to retrieve this information from
+            // the MaterializeTemporaryExpr?
+            CallOpts.IsTemporaryLifetimeExtendedViaAggregate = true;
           }
-        TempLCtx = CallerLCtx;
         }
+      }
+
+      if (MTE && MTE->getStorageDuration() != SD_FullExpression) {
+        // If the temporary is lifetime-extended, don't save the BTE,
+        // because we don't need a temporary destructor, but an automatic
+        // destructor.
+        BTE = nullptr;
+      }
+
+      // FIXME: Support temporaries lifetime-extended via static references.
+      // They'd need a getCXXStaticTempObjectRegion().
+      SVal V = loc::MemRegionVal(MRMgr.getCXXTempObjectRegion(E, LCtx));
+
+      if (BTE)
+        State = addObjectUnderConstruction(State, BTE, LCtx, V);
+
+      if (MTE)
+        State = addObjectUnderConstruction(State, MTE, LCtx, V);
+
       CallOpts.IsTemporaryCtorOrDtor = true;
-      return loc::MemRegionVal(MRMgr.getCXXTempObjectRegion(CE, TempLCtx));
+      return std::make_pair(State, V);
     }
     case ConstructionContext::CXX17ElidedCopyVariableKind:
     case ConstructionContext::CXX17ElidedCopyReturnedValueKind:
     case ConstructionContext::CXX17ElidedCopyConstructorInitializerKind:
       // Not implemented yet.
       break;
     }
   }
   // 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 loc::MemRegionVal(MRMgr.getCXXTempObjectRegion(CE, LCtx));
+  return std::make_pair(
+      State, loc::MemRegionVal(MRMgr.getCXXTempObjectRegion(E, LCtx)));
 }
 
 const CXXConstructExpr *
 ExprEngine::findDirectConstructorForCurrentCFGElement() {
   // Go backward in the CFG to see if the previous element (ignoring
   // destructors) was a CXXConstructExpr. If so, that constructor
   // was constructed directly into an existing region.
   // This process is essentially the inverse of that performed in
void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE,
 
   EvalCallOptions CallOpts;
   auto C = getCurrentCFGElement().getAs<CFGConstructor>();
   assert(C || getCurrentCFGElement().getAs<CFGStmt>());
   const ConstructionContext *CC = C ? C->getConstructionContext() : nullptr;
 
   switch (CE->getConstructionKind()) {
   case CXXConstructExpr::CK_Complete: {
-    Target = getLocationForConstructedObject(CE, Pred, CC, CallOpts);
+    std::tie(State, Target) =
+        prepareForObjectConstruction(CE, State, LCtx, CC, CallOpts);
     break;
   }
   case CXXConstructExpr::CK_VirtualBase:
     // Make sure we are not calling virtual base class initializers twice.
     // Only the most-derived object should initialize virtual base classes.
     if (const Stmt *Outer = LCtx->getCurrentStackFrame()->getCallSite()) {
       const CXXConstructExpr *OuterCtor = dyn_cast<CXXConstructExpr>(Outer);
       if (OuterCtor) {
if (CE->getConstructionKind() == CXXConstructExpr::CK_Delegating) {
       SVal BaseVal = getStoreManager().evalDerivedToBase(ThisVal, CE->getType(),
                                                          IsVirtual);
       Target = BaseVal;
     }
     break;
   }
   }
 
+  if (State != Pred->getState()) {
+    static SimpleProgramPointTag T("ExprEngine",
+                                   "Prepare for object construction");
+    ExplodedNodeSet DstPrepare;
+    StmtNodeBuilder BldrPrepare(Pred, DstPrepare, *currBldrCtx);
+    BldrPrepare.generateNode(CE, Pred, State, &T, ProgramPoint::PreStmtKind);
+    assert(DstPrepare.size() <= 1);
+    if (DstPrepare.size() == 0)
+      return;
+    Pred = *BldrPrepare.begin();
+  }
+
   CallEventManager &CEMgr = getStateManager().getCallEventManager();
   CallEventRef<CXXConstructorCall> Call =
     CEMgr.getCXXConstructorCall(CE, Target.getAsRegion(), State, LCtx);
 
   ExplodedNodeSet DstPreVisit;
   getCheckerManager().runCheckersForPreStmt(DstPreVisit, Pred, CE, *this);
 
   // FIXME: Is it possible and/or useful to do this before PreStmt?
for (ExplodedNodeSet::iterator I = DstPreVisit.begin(),
         // initialize the entire array, not just the first element -- but our
         // handling of arrays everywhere else is weak as well, so this shouldn't
         // actually make things worse. Placement new makes this tricky as well,
         // since it's then possible to be initializing one part of a multi-
         // dimensional array.
         State = State->bindDefaultZero(Target, LCtx);
       }
 
-      State = markStatementsCorrespondingToConstructedObject(State, CC, LCtx,
-                                                             Target);
-
       Bldr.generateNode(CE, *I, State, /*tag=*/nullptr,
                         ProgramPoint::PreStmtKind);
     }
   }
 
   ExplodedNodeSet DstPreCall;
   getCheckerManager().runCheckersForPreCall(DstPreCall, PreInitialized,
                                             *Call, *this);

lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

if (const ObjCMethodCall *Msg = dyn_cast<ObjCMethodCall>(&Call)) {
     return State->BindExpr(E, LCtx, ThisV);
   }
 
   SVal R;
   QualType ResultTy = Call.getResultType();
   unsigned Count = currBldrCtx->blockCount();
   if (auto RTC = getCurrentCFGElement().getAs<CFGCXXRecordTypedCall>()) {
     // Conjure a temporary if the function returns an object by value.
-    MemRegionManager &MRMgr = svalBuilder.getRegionManager();
-    const CXXTempObjectRegion *TR = MRMgr.getCXXTempObjectRegion(E, LCtx);
-    State = markStatementsCorrespondingToConstructedObject(
-        State, RTC->getConstructionContext(), LCtx, loc::MemRegionVal(TR));
-
+    SVal Target;
+    assert(RTC->getStmt() == Call.getOriginExpr());
+    EvalCallOptions CallOpts; // FIXME: We won't really need those.
+    std::tie(State, Target) =
+        prepareForObjectConstruction(Call.getOriginExpr(), State, LCtx,
+                                     RTC->getConstructionContext(), CallOpts);
+    assert(Target.getAsRegion());
     // Invalidate the region so that it didn't look uninitialized. Don't notify
     // the checkers.
-    State = State->invalidateRegions(TR, E, Count, LCtx,
+    State = State->invalidateRegions(Target.getAsRegion(), E, Count, LCtx,
                                      /* CausedByPointerEscape=*/false, nullptr,
                                      &Call, nullptr);
 
-    R = State->getSVal(TR, E->getType());
+    R = State->getSVal(Target.castAs<Loc>(), E->getType());
   } else {
     // Conjure a symbol if the return value is unknown.
 
     // See if we need to conjure a heap pointer instead of
     // a regular unknown pointer.
     bool IsHeapPointer = false;
     if (const auto *CNE = dyn_cast<CXXNewExpr>(E))
       if (CNE->getOperatorNew()->isReplaceableGlobalAllocationFunction()) {