[analyzer] Better detect when C++ object was constructed into existing region.

When the analyzer evaluates a CXXConstructExpr, it looks ahead in the CFG for
the current block to detect what region the object should be constructed into.
If the constructor was directly constructed into a local variable or field
region then there is no need to explicitly bind the constructed value to
the local or field when analyzing the DeclStmt or CXXCtorInitializer that
called the constructor.

Unfortunately, there were situations in which the CXXConstructExpr was
constructed into a temporary region but when evaluating the corresponding
DeclStmt or CXXCtorInitializer the analyzer assumed the object was constructed
into the local or field. This led to spurious warnings about uninitialized
values (PR25777).

To avoid these false positives, this commit factors out the logic for
determining when a CXXConstructExpr will be directly constructed into existing
storage, adds the inverse logic to detect when the corresponding later bind can
be safely skipped, and adds assertions to make sure these two checks are in
sync.

rdar://problem/21947725

llvm-svn: 255859

3 files changed, 101 insertions, 38 deletions

diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index 7ef91d65385..662b0a2dd79 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -476,8 +476,12 @@ void ExprEngine::ProcessInitializer(const CFGInitializer Init,
   if (BMI->isAnyMemberInitializer()) {
     // Constructors build the object directly in the field,
     // but non-objects must be copied in from the initializer.
-    const Expr *Init = BMI->getInit()->IgnoreImplicit();
-    if (!isa<CXXConstructExpr>(Init)) {
+    if (auto *CtorExpr = findDirectConstructorForCurrentCFGElement()) {
+      assert(BMI->getInit()->IgnoreImplicit() == CtorExpr);
+      (void)CtorExpr;
+      // The field was directly constructed, so there is no need to bind.
+    } else {
+      const Expr *Init = BMI->getInit()->IgnoreImplicit();
       const ValueDecl *Field;
       if (BMI->isIndirectMemberInitializer()) {
         Field = BMI->getIndirectMember();
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
index 35af3055243..a5b58710b21 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -492,7 +492,10 @@ void ExprEngine::VisitDeclStmt(const DeclStmt *DS, ExplodedNode *Pred,
       ExplodedNode *UpdatedN = N;
       SVal InitVal = state->getSVal(InitEx, LC);
 
-      if (isa<CXXConstructExpr>(InitEx->IgnoreImplicit())) {
+      assert(DS->isSingleDecl());
+      if (auto *CtorExpr = findDirectConstructorForCurrentCFGElement()) {
+        assert(InitEx->IgnoreImplicit() == CtorExpr);
+        (void)CtorExpr;
         // We constructed the object directly in the variable.
         // No need to bind anything.
         B.generateNode(DS, UpdatedN, state);
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
index 07ae41027d3..556e2239abf 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -103,49 +103,32 @@ static SVal makeZeroElementRegion(ProgramStateRef State, SVal LValue,
 }
 
 
-static const MemRegion *getRegionForConstructedObject(
-    const CXXConstructExpr *CE, ExplodedNode *Pred, ExprEngine &Eng,
-    unsigned int CurrStmtIdx) {
+const MemRegion *
+ExprEngine::getRegionForConstructedObject(const CXXConstructExpr *CE,
+                                          ExplodedNode *Pred) {
   const LocationContext *LCtx = Pred->getLocationContext();
   ProgramStateRef State = Pred->getState();
-  const NodeBuilderContext &CurrBldrCtx = Eng.getBuilderContext();
 
   // See if we're constructing an existing region by looking at the next
   // element in the CFG.
-  const CFGBlock *B = CurrBldrCtx.getBlock();
-  unsigned int NextStmtIdx = CurrStmtIdx + 1;
-  if (NextStmtIdx < B->size()) {
-    CFGElement Next = (*B)[NextStmtIdx];
-
-    // Is this a destructor? If so, we might be in the middle of an assignment
-    // to a local or member: look ahead one more element to see what we find.
-    while (Next.getAs<CFGImplicitDtor>() && NextStmtIdx + 1 < B->size()) {
-      ++NextStmtIdx;
-      Next = (*B)[NextStmtIdx];
-    }
 
-    // Is this a constructor for a local variable?
-    if (Optional<CFGStmt> StmtElem = Next.getAs<CFGStmt>()) {
-      if (const DeclStmt *DS = dyn_cast<DeclStmt>(StmtElem->getStmt())) {
-        if (const VarDecl *Var = dyn_cast<VarDecl>(DS->getSingleDecl())) {
-          if (Var->getInit() && Var->getInit()->IgnoreImplicit() == CE) {
-            SVal LValue = State->getLValue(Var, LCtx);
-            QualType Ty = Var->getType();
-            LValue = makeZeroElementRegion(State, LValue, Ty);
-            return LValue.getAsRegion();
-          }
+  if (auto Elem = findElementDirectlyInitializedByCurrentConstructor()) {
+    if (Optional<CFGStmt> StmtElem = Elem->getAs<CFGStmt>()) {
+      auto *DS = cast<DeclStmt>(StmtElem->getStmt());
+      if (const auto *Var = dyn_cast<VarDecl>(DS->getSingleDecl())) {
+        if (Var->getInit() && Var->getInit()->IgnoreImplicit() == CE) {
+          SVal LValue = State->getLValue(Var, LCtx);
+          QualType Ty = Var->getType();
+          LValue = makeZeroElementRegion(State, LValue, Ty);
+          return LValue.getAsRegion();
         }
       }
-    }
-
-    // Is this a constructor for a member?
-    if (Optional<CFGInitializer> InitElem = Next.getAs<CFGInitializer>()) {
+    } else if (Optional<CFGInitializer> InitElem = Elem->getAs<CFGInitializer>()) {
       const CXXCtorInitializer *Init = InitElem->getInitializer();
       assert(Init->isAnyMemberInitializer());
-
       const CXXMethodDecl *CurCtor = cast<CXXMethodDecl>(LCtx->getDecl());
-      Loc ThisPtr = Eng.getSValBuilder().getCXXThis(CurCtor,
-          LCtx->getCurrentStackFrame());
+      Loc ThisPtr =
+      getSValBuilder().getCXXThis(CurCtor, LCtx->getCurrentStackFrame());
       SVal ThisVal = State->getSVal(ThisPtr);
 
       const ValueDecl *Field;
@@ -167,13 +150,86 @@ static const MemRegion *getRegionForConstructedObject(
     // Don't forget to update the pre-constructor initialization code in
     // ExprEngine::VisitCXXConstructExpr.
   }
-
   // If we couldn't find an existing region to construct into, assume we're
   // constructing a temporary.
-  MemRegionManager &MRMgr = Eng.getSValBuilder().getRegionManager();
+  MemRegionManager &MRMgr = getSValBuilder().getRegionManager();
   return MRMgr.getCXXTempObjectRegion(CE, LCtx);
 }
 
+/// Returns true if the initializer for \Elem can be a direct
+/// constructor.
+static bool canHaveDirectConstructor(CFGElement Elem){
+  // DeclStmts and CXXCtorInitializers for fields can be directly constructed.
+
+  if (Optional<CFGStmt> StmtElem = Elem.getAs<CFGStmt>()) {
+    if (isa<DeclStmt>(StmtElem->getStmt())) {
+      return true;
+    }
+  }
+
+  if (Elem.getKind() == CFGElement::Initializer) {
+    return true;
+  }
+
+  return false;
+}
+
+Optional<CFGElement>
+ExprEngine::findElementDirectlyInitializedByCurrentConstructor() {
+  const NodeBuilderContext &CurrBldrCtx = getBuilderContext();
+  // See if we're constructing an existing region by looking at the next
+  // element in the CFG.
+  const CFGBlock *B = CurrBldrCtx.getBlock();
+  assert(isa<CXXConstructExpr>(((*B)[currStmtIdx]).castAs<CFGStmt>().getStmt()));
+  unsigned int NextStmtIdx = currStmtIdx + 1;
+  if (NextStmtIdx >= B->size())
+    return None;
+
+  CFGElement Next = (*B)[NextStmtIdx];
+
+  // Is this a destructor? If so, we might be in the middle of an assignment
+  // to a local or member: look ahead one more element to see what we find.
+  while (Next.getAs<CFGImplicitDtor>() && NextStmtIdx + 1 < B->size()) {
+    ++NextStmtIdx;
+    Next = (*B)[NextStmtIdx];
+  }
+
+  if (canHaveDirectConstructor(Next))
+    return Next;
+
+  return None;
+}
+
+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
+  // findElementDirectlyInitializedByCurrentConstructor().
+  if (currStmtIdx == 0)
+    return nullptr;
+
+  const CFGBlock *B = getBuilderContext().getBlock();
+  assert(canHaveDirectConstructor((*B)[currStmtIdx]));
+
+  unsigned int PreviousStmtIdx = currStmtIdx - 1;
+  CFGElement Previous = (*B)[PreviousStmtIdx];
+
+  while (Previous.getAs<CFGImplicitDtor>() && PreviousStmtIdx > 0) {
+    --PreviousStmtIdx;
+    Previous = (*B)[PreviousStmtIdx];
+  }
+
+  if (Optional<CFGStmt> PrevStmtElem = Previous.getAs<CFGStmt>()) {
+    if (auto *CtorExpr = dyn_cast<CXXConstructExpr>(PrevStmtElem->getStmt())) {
+      return CtorExpr;
+    }
+  }
+
+  return nullptr;
+}
+
 void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE,
                                        ExplodedNode *Pred,
                                        ExplodedNodeSet &destNodes) {
@@ -188,7 +244,7 @@ void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE,
 
   switch (CE->getConstructionKind()) {
   case CXXConstructExpr::CK_Complete: {
-    Target = getRegionForConstructedObject(CE, Pred, *this, currStmtIdx);
+    Target = getRegionForConstructedObject(CE, Pred);
     break;
   }
   case CXXConstructExpr::CK_VirtualBase: