[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

161

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

170

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?

771

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

123

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

123

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

 //===----------------------------------------------------------------------===//
 
 // When modeling a C++ constructor, for a variety of reasons we need to track
 // the location of the object for the duration of its ConstructionContext.
 // ObjectsUnderConstruction maps statements within the construction context
 // to the object's location, so that on every such statement the location
 // could have been retrieved.
 
-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 {
+  typedef std::pair<
+      llvm::PointerUnion<const Stmt *, const CXXCtorInitializer *>,
+      const LocationContext *> ConstructedObjectKeyImpl;
+
+  ConstructedObjectKeyImpl Impl;
+
+  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)
+      : Impl(P, 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 Impl.first.dyn_cast<const Stmt *>();
+  }
+
+  const CXXCtorInitializer *getCXXCtorInitializer() const {
+    return Impl.first.dyn_cast<const CXXCtorInitializer *>();
+  }
+
+  const LocationContext *getLocationContext() const {
+    return Impl.second;
+  }
+
+  void print(llvm::raw_ostream &OS, PrinterHelper *Helper, PrintingPolicy &PP) {
+    OS << '(' << getLocationContext() << ',' << getAnyASTNodePtr() << ") ";
+    if (const Stmt *S = getStmt()) {
+      S->printPretty(OS, Helper, PP);
+    } else {
+      const CXXCtorInitializer *I = getCXXCtorInitializer();
+      OS << I->getAnyMember()->getNameAsString();
+    }
+  }
+
+  void Profile(llvm::FoldingSetNodeID &ID) const {
+    ID.AddPointer(Impl.first.getOpaqueValue());
+    ID.AddPointer(Impl.second);
+  }
+
+  bool operator==(const ConstructedObjectKey &RHS) const {
+    return Impl == RHS.Impl;
+  }
+
+  bool operator<(const ConstructedObjectKey &RHS) const {
+    return Impl < RHS.Impl;
+  }
+};
+
 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());
   // FIXME: Currently the state might already contain the marker due to
   // incorrect handling of temporaries bound to default parameters.
   assert(!State->get<ObjectsUnderConstruction>(Key) ||
-         isa<CXXBindTemporaryExpr>(S));
+         isa<CXXBindTemporaryExpr>(Key.getStmt()));
   return State->set<ObjectsUnderConstruction>(Key, V);
 }
 
-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());
   assert(State->contains<ObjectsUnderConstruction>(Key));
   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) {
if(AMgr.options.shouldUnrollLoops())
     NewState = processLoopEnd(S, NewState);
 
   LoopExit PP(S, Pred->getLocationContext());
   Bldr.generateNode(PP, NewState, Pred);
   // Enqueue the new nodes onto the work list.
   Engine.enqueue(Dst, currBldrCtx->getBlock(), currStmtIdx);
 }
 
-void ExprEngine::ProcessInitializer(const CFGInitializer Init,
+void ExprEngine::ProcessInitializer(const CFGInitializer CFGInit,
                                     ExplodedNode *Pred) {
-  const CXXCtorInitializer *BMI = Init.getInitializer();
+  const CXXCtorInitializer *BMI = CFGInit.getInitializer();
+  const Expr *Init = BMI->getInit()->IgnoreImplicit();
+  const LocationContext *LC = Pred->getLocationContext();
 
   PrettyStackTraceLoc CrashInfo(getContext().getSourceManager(),
                                 BMI->getSourceLocation(),
                                 "Error evaluating initializer");
 
   // We don't clean up dead bindings here.
   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);
+  ExplodedNodeSet Tmp;
   SVal FieldLoc;
 
   // 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, LC)) {
       // The field was directly constructed, so there is no need to bind.
+      // But we still need to stop tracking the object under construction.
+      State = finishObjectConstruction(State, BMI, LC);
+      NodeBuilder Bldr(Pred, Tmp, *currBldrCtx);
+      PostStore PS(Init, LC, /*Loc*/ nullptr, /*tag*/ nullptr);
+      Bldr.generateNode(PS, State, Pred);
     } 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();
         FieldLoc = State->getLValue(BMI->getMember(), thisVal);
       }
if (getObjectUnderConstruction(State, BMI, LC)) {
           InitVal = SVB.conjureSymbolVal(BMI->getInit(), stackFrame,
                                          Field->getType(),
                                          currBldrCtx->blockCount());
         }
       } else {
         InitVal = State->getSVal(BMI->getInit(), stackFrame);
       }
 
-      assert(Tmp.size() == 1 && "have not generated any new nodes yet");
-      assert(*Tmp.begin() == Pred && "have not generated any new nodes yet");
-      Tmp.clear();
-
       PostInitializer PP(BMI, FieldLoc.getAsRegion(), stackFrame);
       evalBind(Tmp, Init, Pred, FieldLoc, InitVal, /*isInit=*/true, &PP);
     }
   } else {
     assert(BMI->isBaseInitializer() || BMI->isDelegatingInitializer());
+    Tmp.insert(Pred);
     // 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();
+    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;
+    // FIXME: Should be TRUE in C++11 as well.
+    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) {}