[analyzer] Track class member initializer constructors path-sensitively within their construction context.

[NoQ]

Summary

Same as D47305 but for CXXCtorInitializer-based constructors, based on the discussion in http://lists.llvm.org/pipermail/cfe-dev/2018-May/058055.html

Because these don't suffer from liveness bugs (member variables are born much earlier than their constructors are called and live much longer than stack locals), this is mostly a refactoring pass. It has minor observable side effects, but it will have way more visible effects when we enable C++17 construction contexts because finding the direct constructor would be much harder.

Currently the observable effect is that we can preserve direct bindings to the object more often and we need to fall back to binding the lazy compound value of the initializer expression less often. Direct bindings are modeled better by the store. In the provided test case the default binding produced by trivial-copying s to t.s would overwrite the existing default binding to t. But if we instead preserve direct bindings, only bindings that correspond to t.s will be overwritten and the binding for t.w will remain untouched. This only works for C++17 because in C++11 the member variable is still modeled as a trivial copy from the temporary object, because there semantically *is* a temporary object, while in C++17 it is elided.

So we finally get rid of findDirectConstructorForCurrentCFGElement() which is a CFG look-behind hack. We pay the price of making our trait's key more complex; we should have probably just added state trait specializations for PointerUnion instead.

[NoQ]

Add a forgotten FIXME to the test.

[george.karpenkov]

requesting changes due to minor nits inline

lib/StaticAnalyzer/Core/ExprEngine.cpp

152

newline after the brace. here and elsewhere within this class.

161

would it be faster to store a pair in the first place, instead of constructing it on each comparison? Is this operator required to be in a GDM?

756

gotta love state variables used 70 lines later on. Could we add a comment here? What is the semantics of the cleanup action? Running destructors? Maybe a better name would make this more readable?

Also would it be worse to change finishObjectConstruction function to not do anything when there's no object being constructed, and then we could simply call it on all code paths?

[NoQ]

Refactor everything to address review comments.

[george.karpenkov]

LGTM, but I would really prefer if you could split that inheritance back into composition.

lib/StaticAnalyzer/Core/ExprEngine.cpp

114

sorry I find that very confusing, and I think the previous version was better (prefer composition over inheritance)

[NoQ]

This comment has been deleted.

lib/StaticAnalyzer/Core/ExprEngine.cpp

114

Ok, so, i mean, i don't think it would be faster, i really hope the compiler would know how to inline std::pair methods. I think converting to a std::pair or std::tuple is a fairly standard trick to avoid x < rhs.x || (x == rhs.x && y < rhs.y) kind of code. But with inheritance we at least have boilerplate operators sorted out for free.

May i leave the actual previous version around without fixing it?^^

[NoQ]

Hmm, actually composition looks very pretty if you use the magic word "impl".

[george.karpenkov]

👍

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

private:
   /// 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 ProgramStateRef addObjectUnderConstruction(
-      ProgramStateRef State, const Stmt *S,
+      ProgramStateRef State,
+      llvm::PointerUnion<const Stmt *, const CXXCtorInitializer *> P,
       const LocationContext *LC, SVal V);
 
   /// Mark the object sa fully constructed, cleaning up the state trait
   /// that tracks objects under construction.
-  static ProgramStateRef finishObjectConstruction(ProgramStateRef State,
-                                                  const Stmt *S,
-                                                  const LocationContext *LC);
+  static ProgramStateRef finishObjectConstruction(
+      ProgramStateRef State,
+      llvm::PointerUnion<const Stmt *, const CXXCtorInitializer *> P,
 
   /// If the given statement corresponds to an object under construction,
   /// being part of its construciton context, retrieve that object's location.
-  static Optional<SVal> getObjectUnderConstruction(ProgramStateRef State,
-                                                   const Stmt *S,
-                                                   const LocationContext *LC);
+  static Optional<SVal> getObjectUnderConstruction(
+      ProgramStateRef State,
+      llvm::PointerUnion<const Stmt *, const CXXCtorInitializer *> P,
 
   /// 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);
 };

lib/StaticAnalyzer/Core/ExprEngine.cpp

 STATISTIC(NumMaxBlockCountReachedInInlined,
             "The # of aborted paths due to reaching the maximum block count in "
             "an inlined function");
 STATISTIC(NumTimesRetriedWithoutInlining,
             "The # of times we re-evaluated a call without inlining");
 
 // Keeps track of whether objects corresponding to the statement have been
 // fully constructed.
-typedef std::pair<const Stmt *, const LocationContext *> ConstructedObjectKey;
+
+/// ConstructedObjectKey is used for being able to find the path-sensitive
+/// memory region of a freshly constructed object while modeling the AST node
+/// that syntactically represents the object that is being constructed.
+/// Semantics of such nodes may sometimes require access to the region that's
+/// not otherwise present in the program state, or to the very fact that
+/// the construction context was present and contained references to these
+/// AST nodes.
+class ConstructedObjectKey {
+  llvm::PointerUnion<const Stmt *, const CXXCtorInitializer *> P;
+  const LocationContext *LC;
+
+  const void *getAnyASTNodePtr() const {
+    if (const Stmt *S = getStmt())
+      return S;
+    else
+      return getCXXCtorInitializer();
+  }
+
+public:
+  ConstructedObjectKey(
+      llvm::PointerUnion<const Stmt *, const CXXCtorInitializer *> P,
+      const LocationContext *LC)
+      : P(P), LC(LC) {
+    // This is the full list of statements that require additional actions when
+    // encountered. This list may be expanded when new actions are implemented.
+    assert(getCXXCtorInitializer() || isa<DeclStmt>(getStmt()) ||
+           isa<CXXNewExpr>(getStmt()) || isa<CXXBindTemporaryExpr>(getStmt()) ||
+           isa<MaterializeTemporaryExpr>(getStmt()));
+  }
+
+  const Stmt *getStmt() const {
+    return P.dyn_cast<const Stmt *>();
+  }
+  const CXXCtorInitializer *getCXXCtorInitializer() const {
+    return P.dyn_cast<const CXXCtorInitializer *>();
+  }
+  const LocationContext *getLocationContext() const {
+    return LC;
+  }
+
+  void print(llvm::raw_ostream &OS, PrinterHelper *Helper, PrintingPolicy &PP) {
+    OS << '(' << LC << ',' << getAnyASTNodePtr() << ") ";
+    if (const Stmt *S = P.dyn_cast<const Stmt *>()) {
+      S->printPretty(OS, Helper, PP);
+    } else {
+      const CXXCtorInitializer *I = P.get<const CXXCtorInitializer *>();
+      OS << I->getAnyMember()->getNameAsString();
+    }
+  }
+  void Profile(llvm::FoldingSetNodeID &ID) const {
+    ID.AddPointer(P.getOpaqueValue());
+    ID.AddPointer(LC);
+  }
+  bool operator==(const ConstructedObjectKey &Other) const {
+    return P == Other.P && LC == Other.LC;
+  }
+  bool operator<(const ConstructedObjectKey &Other) const {
+    return std::make_pair(P, LC) < std::make_pair(Other.P, Other.LC);
+  }
+};
+
 typedef llvm::ImmutableMap<ConstructedObjectKey, SVal>
     ObjectsUnderConstructionMap;
 REGISTER_TRAIT_WITH_PROGRAMSTATE(ObjectsUnderConstruction,
                                  ObjectsUnderConstructionMap)
 
 
 //===----------------------------------------------------------------------===//
 // Engine construction and deletion.
ExprEngine::createTemporaryRegionIfNeeded(ProgramStateRef State,
   if (!FoundOriginalMaterializationRegion) {
     // Notify checkers once for two bindLoc()s.
     State = processRegionChange(State, TR, LC);
   }
 
   return State;
 }
 
-ProgramStateRef
-ExprEngine::addObjectUnderConstruction(ProgramStateRef State, const Stmt *S,
-                                       const LocationContext *LC, SVal V) {
-  ConstructedObjectKey Key(S, LC->getCurrentStackFrame());
+ProgramStateRef ExprEngine::addObjectUnderConstruction(
+    ProgramStateRef State,
+    llvm::PointerUnion<const Stmt *, const CXXCtorInitializer *> P,
+  ConstructedObjectKey Key(P, LC->getCurrentStackFrame());
   if (!State->contains<ObjectsUnderConstruction>(Key)) {
     return State->set<ObjectsUnderConstruction>(Key, V);
   }
 
   // FIXME: Currently the state might already contain the marker due to
   // incorrect handling of temporaries bound to default parameters; for
   // those, we currently skip the CXXBindTemporaryExpr but rely on adding
   // temporary destructor nodes. Otherwise, this branch should be unreachable.
   return State;
 }
 
-Optional<SVal> ExprEngine::getObjectUnderConstruction(ProgramStateRef State,
-    const Stmt *S, const LocationContext *LC) {
-  ConstructedObjectKey Key(S, LC->getCurrentStackFrame());
+Optional<SVal> ExprEngine::getObjectUnderConstruction(
+    ProgramStateRef State,
+    llvm::PointerUnion<const Stmt *, const CXXCtorInitializer *> P,
+    const LocationContext *LC) {
+  ConstructedObjectKey Key(P, LC->getCurrentStackFrame());
   return Optional<SVal>::create(State->get<ObjectsUnderConstruction>(Key));
 }
 
-ProgramStateRef ExprEngine::finishObjectConstruction(ProgramStateRef State,
-    const Stmt *S, const LocationContext *LC) {
-  ConstructedObjectKey Key(S, LC->getCurrentStackFrame());
+ProgramStateRef ExprEngine::finishObjectConstruction(
+    ProgramStateRef State,
+    llvm::PointerUnion<const Stmt *, const CXXCtorInitializer *> P,
+    const LocationContext *LC) {
+  ConstructedObjectKey Key(P, LC->getCurrentStackFrame());
   return State->remove<ObjectsUnderConstruction>(Key);
 }
 
 bool ExprEngine::areAllObjectsFullyConstructed(ProgramStateRef State,
                                                const LocationContext *FromLC,
                                                const LocationContext *ToLC) {
   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.second == LC)
+      if (I.first.getLocationContext() == LC)
         return false;
 
     LC = LC->getParent();
   }
   return true;
 }
 
 
static void printObjectsUnderConstructionForContext(raw_ostream &Out,
                                                     const char *NL,
                                                     const char *Sep,
                                                     const LocationContext *LC) {
   PrintingPolicy PP =
       LC->getAnalysisDeclContext()->getASTContext().getPrintingPolicy();
   for (auto I : State->get<ObjectsUnderConstruction>()) {
     ConstructedObjectKey Key = I.first;
     SVal Value = I.second;
-    if (Key.second != LC)
+    if (Key.getLocationContext() != LC)
       continue;
-    Out << '(' << Key.second << ',' << Key.first << ") ";
-    Key.first->printPretty(Out, nullptr, PP);
+    Key.print(Out, nullptr, PP);
     Out << " : " << Value << NL;
   }
 }
 
 void ExprEngine::printState(raw_ostream &Out, ProgramStateRef State,
                             const char *NL, const char *Sep,
                             const LocationContext *LCtx) {
   if (LCtx) {
void ExprEngine::ProcessInitializer(const CFGInitializer Init,
   const auto *stackFrame = cast<StackFrameContext>(Pred->getLocationContext());
   const auto *decl = cast<CXXConstructorDecl>(stackFrame->getDecl());
 
   ProgramStateRef State = Pred->getState();
   SVal thisVal = State->getSVal(svalBuilder.getCXXThis(decl, stackFrame));
 
   ExplodedNodeSet Tmp(Pred);
   SVal FieldLoc;
+  bool ObjectUnderConstructionNeedsCleanup = false;
 
   // Evaluate the initializer, if necessary
   if (BMI->isAnyMemberInitializer()) {
     // Constructors build the object directly in the field,
     // but non-objects must be copied in from the initializer.
-    if (const auto *CtorExpr = findDirectConstructorForCurrentCFGElement()) {
-      assert(BMI->getInit()->IgnoreImplicit() == CtorExpr);
-      (void)CtorExpr;
+    if (getObjectUnderConstruction(State, BMI, Pred->getLocationContext())) {
       // The field was directly constructed, so there is no need to bind.
+      // But we still need to cleanup objects under construction.
+      ObjectUnderConstructionNeedsCleanup = true;
     } else {
       const Expr *Init = BMI->getInit()->IgnoreImplicit();
       const ValueDecl *Field;
       if (BMI->isIndirectMemberInitializer()) {
         Field = BMI->getIndirectMember();
         FieldLoc = State->getLValue(BMI->getIndirectMember(), thisVal);
       } else {
         Field = BMI->getMember();
if (BMI->isAnyMemberInitializer()) {
     // We already did all the work when visiting the CXXConstructExpr.
   }
 
   // Construct PostInitializer nodes whether the state changed or not,
   // so that the diagnostics don't get confused.
   PostInitializer PP(BMI, FieldLoc.getAsRegion(), stackFrame);
   ExplodedNodeSet Dst;
   NodeBuilder Bldr(Tmp, Dst, *currBldrCtx);
-  for (const auto I : Tmp)
-    Bldr.generateNode(PP, I->getState(), I);
+  for (const auto I : Tmp) {
+    ProgramStateRef State = I->getState();
+    if (ObjectUnderConstructionNeedsCleanup)
+      State = finishObjectConstruction(State, BMI, Pred->getLocationContext());
+    Bldr.generateNode(PP, State, I);
+  }
 
   // Enqueue the new nodes onto the work list.
   Engine.enqueue(Dst, currBldrCtx->getBlock(), currStmtIdx);
 }
 
 void ExprEngine::ProcessImplicitDtor(const CFGImplicitDtor D,
                                      ExplodedNode *Pred) {
   ExplodedNodeSet Dst;
// should go away, but the assertion should remain.
     NodeBuilder Bldr(Pred, CleanUpObjects, BC);
     ProgramStateRef State = Pred->getState();
     const LocationContext *FromLC = Pred->getLocationContext();
     const LocationContext *ToLC = FromLC->getCurrentStackFrame()->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.second == LC) {
+        if (I.first.getLocationContext() == LC) {
           // The comment above only pardons us for not cleaning up a
           // CXXBindTemporaryExpr. If any other statements are found here,
           // it must be a separate problem.
-          assert(isa<CXXBindTemporaryExpr>(I.first.first));
+          assert(isa<CXXBindTemporaryExpr>(I.first.getStmt()));
           State = State->remove<ObjectsUnderConstruction>(I.first);
         }
 
       LC = LC->getParent();
     }
     if (State != Pred->getState()) {
       Pred = Bldr.generateNode(Pred->getLocation(), State, Pred);
       if (!Pred) {

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

case ConstructionContext::SimpleConstructorInitializerKind: {
       } else {
         Field = Init->getMember();
         FieldVal = State->getLValue(Init->getMember(), ThisVal);
       }
 
       QualType Ty = Field->getType();
       FieldVal = makeZeroElementRegion(State, FieldVal, Ty,
                                        CallOpts.IsArrayCtorOrDtor);
+      State = addObjectUnderConstruction(State, Init, LCtx, 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 =
std::pair<ProgramStateRef, SVal> ExprEngine::prepareForObjectConstruction(
   }
   // 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)));
 }
 
-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();
-
-  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) {
   const LocationContext *LCtx = Pred->getLocationContext();
   ProgramStateRef State = Pred->getState();
 
   SVal Target = UnknownVal();
 

test/Analysis/cxx17-mandatory-elision.cpp

 struct B {
   A a;
   B(): a(A(0)) {}
 };
 
 } // namespace variable_functional_cast_crash
 
 
+namespace ctor_initializer {
+
+struct S {
+  int x, y, z;
+};
+
+struct T {
+  S s;
+  int w;
+  T(int w): s(), w(w) {}
+};
+
+class C {
+  T t;
+public:
+  C() : t(T(4)) {
+    S s = {1, 2, 3};
+    t.s = s;
+    clang_analyzer_eval(t.w == 4);
+#if __cplusplus >= 201703L
+    // expected-warning@-2{{TRUE}}
+#else
+    // expected-warning@-4{{UNKNOWN}}
+#endif
+  }
+};
+
+} // namespace ctor_initializer
+
+
 namespace address_vector_tests {
 
 template <typename T> struct AddressVector {
   T *buf[10];
   int len;
 
   AddressVector() : len(0) {}