[CFG] [analyzer] pr41300: Add a branch to skip virtual base initializers when they are handled by the superclass.

[NoQ]

Summary

A copy-paste from https://bugs.llvm.org/show_bug.cgi?id=41300#c4:

A cleaner repro:

#include <iostream>

struct S {
  S() { std::cout << "  Calling S()" << std::endl; }
};

struct A {
  A() { std::cout << "  Calling A()" << std::endl; }
  A(S s) { std::cout << "  Calling A(S)" << std::endl; }
};

struct B : virtual A {
  B() : A(S()) { std::cout << "  Calling B()" << std::endl; }
};

struct C : B {
  C() { std::cout << "  Calling C()" << std::endl; }
};

int main() {
  std::cout << "Constructing 'b':" << std::endl;
  B b;

  std::cout << "Constructing 'c':" << std::endl;
  C c;
  return 0;
}

The output of this program if you run it:

Constructing 'b':

Calling S()
Calling A(S)
Calling B()

Constructing 'c':

Calling A()
Calling B()
Calling C()

So, like, when calling the constructor for class B from class C, we silently (even under -Weverything) ignore the initializer A(S()). That is, we don't even compute S(), we just ignore it the whole initializer. But when we invoke B() directly, we do compute S().

This behavior implemented by compiling two versions of B(): one with the initializer for A and one without it, as can be seen in https://godbolt.org/z/8Sc-Re

However in the AST there's only one constructor for B(), so we should probably implement a runtime branch while modeling a CXXCtorInitializer in an inlined constructor call.

This patch adds the run-time CFG branch that would skip initialization of virtual base classes depending on whether the constructor is called from a superclass constructor or not. Previously the Static Analyzer was already skipping virtual base-class initializers in such constructors, but it wasn't skipping its arguments and their potential side effects, which was causing a crash (and was generally incorrect). The previous skipping behavior is replaced with a hard assertion that we're not even getting there due to how our CFG works.

The new CFG element is under a CFG build option so that not to break other consumers of the CFG by this change. Static Analyzer support for this change is implemented.

I've no idea how to write CFGBuilder code correctly because it's confusing because everything depends on the global mutable state of Block and Succ in the CFGBuilder. But i hope that initializers-cfg-output.cpp, together with the newly added test, provides some basic test coverage for my code.

Note that a similar functionality is necessary for destructors of virtual bases, but it remains to be done for now. We should be able to re-use the same terminator kind.

[NoQ]

Note that a similar functionality is necessary for destructors of virtual bases, but it remains to be done for now. We should be able to re-use the same terminator kind.

It's not really that important because virtual base destructors don't have sub-expressions that might cause side effects. So we can simply skip them in ExprEngine.

[NoQ]

Rebase.

[Szelethus]

Exquisite detective work as always! I read a bit of the standard, your summary, the bugreport and the new test cases, and I feel confident that I have a good understanding of the problem and your solution, though I didn't dig through the details just yet. At a first glance this looks great, and I'll be sure to revisit this patch and accept it formally.

The expression-list or braced-init-list in a mem-initializer is used to initialize the designated subobject (or, in the case of a delegating constructor, the complete class object) according to the initialization rules of 11.6 for direct-initialization. [ Example:

struct B1 { B1(int) ; /* . . . */ };
struct B2 { B2(int) ; /* . . . */ };
struct D : B1, B2 {
  D(int) ;
  B1 b;
  const int c;
};
D::D(int a) : B2(a+1) , B1(a+2) , c(a+3) , b(a+4) { /* . . . */ }
D d(10) ;

— end example ] [ Note: The initialization performed by each mem-initializer constitutes a full-expression (4.6) . Any expression in a mem-initializer is evaluated as part of the full-expression that performs the initialization. — end note ] A mem-initializer where the mem-initializer-id denotes a virtual base class is ignored during execution of a constructor of any class that is not the most derived class.

I think this part of the standard isn't well known (due mostly the the scarcity of virtual bases), and I'd like to see it referenced, maybe even quoted.

clang/include/clang/Analysis/CFG.h

567 ↗

(On Diff #199532)

In the context of this patch, I understand what you mean, but without that, this might not be a good description for a class this important.

How about

///     vbase inits   |  initialization handled by superclass;
///                   |  initialization not handled by superclass

?

[Charusso]

I do not like to have a variable storing/mention one stuff named in plural. It is your decision as it is just my personal feeling.

clang/include/clang/Analysis/AnalysisDeclContext.h

462 ↗

(On Diff #199532)

Bases -> Base

clang/include/clang/Analysis/CFG.h

510 ↗

(On Diff #199532)

The most derived class instead. Bases -> Base

541 ↗

(On Diff #199532)

Bases -> Base

1050 ↗

(On Diff #199532)

Bases -> Base

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

120 ↗

(On Diff #199532)

Bases -> Base

clang/lib/Analysis/AnalysisDeclContext.cpp

73 ↗

(On Diff #199532)

Bases -> Base

clang/lib/Analysis/CFG.cpp

1435 ↗

(On Diff #199532)

The most derived class instead.

1794 ↗

(On Diff #199532)

Bases -> Base

clang/lib/StaticAnalyzer/Core/AnalysisManager.cpp

40 ↗

(On Diff #199532)

Bases -> Base

clang/lib/StaticAnalyzer/Core/CoreEngine.cpp

447 ↗

(On Diff #199532)

Bases -> Base

451 ↗

(On Diff #199532)

May it is worth to handle the call outside to see whether it is non-null.

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

435 ↗

(On Diff #199532)

May it is worth to handle the call outside to see whether it is non-null.

440 ↗

(On Diff #199532)

The most derived class?

[Szelethus]

Hmmm, how about this?

#include <iostream>

struct VBase1 {
  VBase1() { std::cout << "VBase1()\n"; }
  VBase1(int) { std::cout << "VBase1(int)\n"; }
};

struct VBase2 {
  VBase2() { std::cout << "VBase2()\n"; }
  VBase2(std::string) { std::cout << "VBase2(std::string)\n"; }
};

struct B : public virtual VBase1 {
  B() : VBase1(0) {}
};

struct C : public B, public virtual VBase2 {
  C() : VBase2("sajt") {}
};

int main() {
  C c;
}

Output:

VBase1()
VBase2(std::string)

You are right that all virtual bases as initialized before everything else, but not all ctor delegations are skipped. If not this specific one, can something similar screw us over?

edit:

Twin of the abomination above:

#include <iostream>

struct VBase1 {
  VBase1() { std::cout << "VBase1()\n"; }
  VBase1(int) { std::cout << "VBase1(int)\n"; }
};

struct VBase2 {
  VBase2() { std::cout << "VBase2()\n"; }
  VBase2(std::string) { std::cout << "VBase2(std::string)\n"; }
};

struct B : public virtual VBase1, public virtual VBase2 { // Note that VBase2 is here.
  B() : VBase1(0) {}
};

struct C : public B {
  C() : VBase2("sajt") {}
};

int main() {
  C c;
}

Output:

VBase1()
VBase2(std::string)

edit2: What if the order of inheritance changes?

clang/lib/Analysis/CFG.cpp

1434–1438 ↗

(On Diff #199532)

I find this a little too vague. The standard states:

First, and only for the constructor of the most derived class (4.5) , virtual base classes are initialized in the order they appear on a depth-first left-to-right traversal of the directed acyclic graph of base classes, where “left-to-right” is the order of appearance of the base classes in the derived class base-specifier-list.

Then, direct base classes are initialized in declaration order as they appear in the (regardless of the order of the mem-initializer s).

Then, non-static data members are initialized in the order they were declared in the class definition (again regardless of the order of the mem-initializer s).

Finally, the base-specifier-list compound-statement of the constructor body is executed.

This would be an overkill here, but how does this sound like:

Constructor delegations are ignored to virtual bases unless the object is of the most derived class.

class VBase { VBase() = default; VBase(int) {} };
class A : virtual public VBase { A() : VBase(0) {} };
class B : public A {};

B b; // Constructor calls in order: VBase(), A(), B().

// VBase(int) is ignored, B isn't the most derived class that
// delegates to the virtual base.

This may result in the virtual base(s) being already initialized at this point, in which case we should jump right onto non-virtual bases and fields. To handle this, make a CFG branch. We only need to do this once, since the standard states that all virtual bases shall be initialized before non-virtual bases and direct data members.

Also, the comment of mine complementing this inline raises a concern about "doing this only once", can you specify?

[Szelethus]

clang/lib/Analysis/CFG.cpp

1434–1438 ↗

(On Diff #199532)

Mind you, this is the order of initialization for non-delegating constructors!

[Szelethus]

The more I think about this, the more I think it would be great to see this tested :)

[NoQ]

In D61816#1504590, @Szelethus wrote:

Hmmm, how about this?
...
You are right that all virtual bases as initialized before everything else, but not all ctor delegations are skipped. If not this specific one, can something similar screw us over?
edit: Twin of the abomination above:
...
edit2: What if the order of inheritance changes?

VBase1(int) is skipped because B is not the most derived class, VBase2(std::string) is not skipped because C is the most derived class. That's the whole point of the branch; i don't see a problem. AST handles the initialization order for us, all we need is to branch around it correctly. Added these tests^^

clang/include/clang/Analysis/CFG.h

567 ↗

(On Diff #199532)

Because this is a doxygen table syntax, i'm trying to make it look reasonable both in comments and in doxygen. In this sense it's hard to make line breaks in cells.

Hmm, btw, doxygen also fails to handle || because it thinks that it's a table cell delimiter. Let me fix that as well:

clang/lib/StaticAnalyzer/Core/CoreEngine.cpp

451 ↗

(On Diff #199532)

Mmm, what do you mean?

[Charusso]

clang/lib/StaticAnalyzer/Core/CoreEngine.cpp

451 ↗

(On Diff #199532)

It was that which is null-proof:

if (const Stmt *Outer = LCtx->getStackFrame()->getCallSite()) {
  const CXXConstructExpr *OuterCtor = dyn_cast<CXXConstructExpr>(Outer);
  if (OuterCtor) {
    // ...
  }
}

[NoQ]

clang/lib/StaticAnalyzer/Core/CoreEngine.cpp

451 ↗

(On Diff #199532)

That's exactly what _or_null stands for in dyn_cast_or_null.

[Charusso]

clang/lib/StaticAnalyzer/Core/CoreEngine.cpp

451 ↗

(On Diff #199532)

Hm, I believe I had problems with that nullability a year ago, but it was long ago. Thanks for the clarification.

[Szelethus]

LGTM! Nicely done!

cfe/trunk/include/clang/Analysis/AnalysisDeclContext.h

AnalysisDeclContextManager(ASTContext &ASTCtx, bool useUnoptimizedCFG = false,
                              bool addLifetime = false,
                              bool addLoopExit = false,
                              bool addScopes = false,
                              bool synthesizeBodies = false,
                              bool addStaticInitBranches = false,
                              bool addCXXNewAllocator = true,
                              bool addRichCXXConstructors = true,
                              bool markElidedCXXConstructors = true,
+                             bool addVirtualBaseBranches = true,
                              CodeInjector *injector = nullptr);
 
   AnalysisDeclContext *getContext(const Decl *D);
 
   bool getUseUnoptimizedCFG() const {
     return !cfgBuildOptions.PruneTriviallyFalseEdges;
   }
 

cfe/trunk/include/clang/Analysis/CFG.h

public:
   enum Kind {
     /// A branch that corresponds to a statement in the code,
     /// such as an if-statement.
     StmtBranch,
     /// A branch in control flow of destructors of temporaries. In this case
     /// terminator statement is the same statement that branches control flow
     /// in evaluation of matching full expression.
     TemporaryDtorsBranch,
+    /// A shortcut around virtual base initializers. It gets taken when
+    /// virtual base classes have already been initialized by the constructor
+    /// of the most derived class while we're in the base class.
+    VirtualBaseBranch,
 
     /// Number of different kinds, for sanity checks. We subtract 1 so that
     /// to keep receiving compiler warnings when we don't cover all enum values
     /// in a switch.
-    NumKindsMinusOne = TemporaryDtorsBranch
+    NumKindsMinusOne = VirtualBaseBranch
   };
 
 private:
-  static constexpr int KindBits = 1;
+  static constexpr int KindBits = 2;
   static_assert((1 << KindBits) > NumKindsMinusOne,
                 "Not enough room for kind!");
   llvm::PointerIntPair<Stmt *, KindBits> Data;
 
 public:
   CFGTerminator() { assert(!isValid()); }
   CFGTerminator(Stmt *S, Kind K = StmtBranch) : Data(S, K) {}
 
   bool isValid() const { return Data.getOpaqueValue() != nullptr; }
   Stmt *getStmt() { return Data.getPointer(); }
   const Stmt *getStmt() const { return Data.getPointer(); }
   Kind getKind() const { return static_cast<Kind>(Data.getInt()); }
 
   bool isStmtBranch() const {
     return getKind() == StmtBranch;
   }
   bool isTemporaryDtorsBranch() const {
     return getKind() == TemporaryDtorsBranch;
   }
+  bool isVirtualBaseBranch() const {
+    return getKind() == VirtualBaseBranch;
+  }
 };
 
 /// Represents a single basic block in a source-level CFG.
 ///  It consists of:
 ///
 ///  (1) A set of statements/expressions (which may contain subexpressions).
 ///  (2) A "terminator" statement (not in the set of statements).
 ///  (3) A list of successors and predecessors.
 ///
 /// Terminator: The terminator represents the type of control-flow that occurs
 /// at the end of the basic block.  The terminator is a Stmt* referring to an
 /// AST node that has control-flow: if-statements, breaks, loops, etc.
 /// If the control-flow is conditional, the condition expression will appear
 /// within the set of statements in the block (usually the last statement).
 ///
 /// Predecessors: the order in the set of predecessors is arbitrary.
 ///
 /// Successors: the order in the set of successors is NOT arbitrary.  We
 ///  currently have the following orderings based on the terminator:
 ///
-///     Terminator       Successor Ordering
-///  -----------------------------------------------------
-///       if            Then Block;  Else Block
-///     ? operator      LHS expression;  RHS expression
-///     &&, ||          expression that uses result of && or ||, RHS
+///     Terminator     |   Successor Ordering
+///  ------------------|------------------------------------
+///       if           |  Then Block;  Else Block
+///     ? operator     |  LHS expression;  RHS expression
+///     logical and/or |  expression that consumes the op, RHS
+///     vbase inits    |  already handled by the most derived class; not yet
 ///
 /// But note that any of that may be NULL in case of optimized-out edges.
 class CFGBlock {
   class ElementList {
     using ImplTy = BumpVector<CFGElement>;
 
     ImplTy Impl;
 
public:
     bool AddLoopExit = false;
     bool AddTemporaryDtors = false;
     bool AddScopes = false;
     bool AddStaticInitBranches = false;
     bool AddCXXNewAllocator = false;
     bool AddCXXDefaultInitExprInCtors = false;
     bool AddRichCXXConstructors = false;
     bool MarkElidedCXXConstructors = false;
+    bool AddVirtualBaseBranches = false;
 
     BuildOptions() = default;
 
     bool alwaysAdd(const Stmt *stmt) const {
       return alwaysAddMask[stmt->getStmtClass()];
     }
 
     BuildOptions &setAlwaysAdd(Stmt::StmtClass stmtClass, bool val = true) {

cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h

void HandleBranch(const Stmt *Cond, const Stmt *Term, const CFGBlock *B,
                     ExplodedNode *Pred);
   void HandleCleanupTemporaryBranch(const CXXBindTemporaryExpr *BTE,
                                     const CFGBlock *B, ExplodedNode *Pred);
 
   /// Handle conditional logic for running static initializers.
   void HandleStaticInit(const DeclStmt *DS, const CFGBlock *B,
                         ExplodedNode *Pred);
 
+  void HandleVirtualBaseBranch(const CFGBlock *B, ExplodedNode *Pred);
+
 private:
   ExplodedNode *generateCallExitBeginNode(ExplodedNode *N,
                                           const ReturnStmt *RS);
 
 public:
   /// Construct a CoreEngine object to analyze the provided CFG.
   CoreEngine(SubEngine &subengine,
              FunctionSummariesTy *FS,

cfe/trunk/lib/Analysis/AnalysisDeclContext.cpp

 }
 
 AnalysisDeclContextManager::AnalysisDeclContextManager(
     ASTContext &ASTCtx, bool useUnoptimizedCFG, bool addImplicitDtors,
     bool addInitializers, bool addTemporaryDtors, bool addLifetime,
     bool addLoopExit, bool addScopes, bool synthesizeBodies,
     bool addStaticInitBranch, bool addCXXNewAllocator,
     bool addRichCXXConstructors, bool markElidedCXXConstructors,
-    CodeInjector *injector)
+    bool addVirtualBaseBranches, CodeInjector *injector)
     : Injector(injector), FunctionBodyFarm(ASTCtx, injector),
       SynthesizeBodies(synthesizeBodies) {
   cfgBuildOptions.PruneTriviallyFalseEdges = !useUnoptimizedCFG;
   cfgBuildOptions.AddImplicitDtors = addImplicitDtors;
   cfgBuildOptions.AddInitializers = addInitializers;
   cfgBuildOptions.AddTemporaryDtors = addTemporaryDtors;
   cfgBuildOptions.AddLifetime = addLifetime;
   cfgBuildOptions.AddLoopExit = addLoopExit;
   cfgBuildOptions.AddScopes = addScopes;
   cfgBuildOptions.AddStaticInitBranches = addStaticInitBranch;
   cfgBuildOptions.AddCXXNewAllocator = addCXXNewAllocator;
   cfgBuildOptions.AddRichCXXConstructors = addRichCXXConstructors;
   cfgBuildOptions.MarkElidedCXXConstructors = markElidedCXXConstructors;
+  cfgBuildOptions.AddVirtualBaseBranches = addVirtualBaseBranches;
 }
 
 void AnalysisDeclContextManager::clear() { Contexts.clear(); }
 
 Stmt *AnalysisDeclContext::getBody(bool &IsAutosynthesized) const {
   IsAutosynthesized = false;
   if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
     Stmt *Body = FD->getBody();

cfe/trunk/lib/Analysis/CFG.cpp

if (const CXXDestructorDecl *DD = dyn_cast_or_null<CXXDestructorDecl>(D))
       addImplicitDtorsForDestructor(DD);
 
   // Visit the statements and create the CFG.
   CFGBlock *B = addStmt(Statement);
 
   if (badCFG)
     return nullptr;
 
-  // For C++ constructor add initializers to CFG.
-  if (const CXXConstructorDecl *CD = dyn_cast_or_null<CXXConstructorDecl>(D)) {
+  // For C++ constructor add initializers to CFG. Constructors of virtual bases
+  // are ignored unless the object is of the most derived class.
+  //   class VBase { VBase() = default; VBase(int) {} };
+  //   class A : virtual public VBase { A() : VBase(0) {} };
+  //   class B : public A {};
+  //   B b; // Constructor calls in order: VBase(), A(), B().
+  //        // VBase(0) is ignored because A isn't the most derived class.
+  // This may result in the virtual base(s) being already initialized at this
+  // point, in which case we should jump right onto non-virtual bases and
+  // fields. To handle this, make a CFG branch. We only need to add one such
+  // branch per constructor, since the Standard states that all virtual bases
+  // shall be initialized before non-virtual bases and direct data members.
+  if (const auto *CD = dyn_cast_or_null<CXXConstructorDecl>(D)) {
+    CFGBlock *VBaseSucc = nullptr;
     for (auto *I : llvm::reverse(CD->inits())) {
+      if (BuildOpts.AddVirtualBaseBranches && !VBaseSucc &&
+          I->isBaseInitializer() && I->isBaseVirtual()) {
+        // We've reached the first virtual base init while iterating in reverse
+        // order. Make a new block for virtual base initializers so that we
+        // could skip them.
+        VBaseSucc = Succ = B ? B : &cfg->getExit();
+        Block = createBlock();
+      }
       B = addInitializer(I);
       if (badCFG)
         return nullptr;
     }
+    if (VBaseSucc) {
+      // Make a branch block for potentially skipping virtual base initializers.
+      Succ = VBaseSucc;
+      B = createBlock();
+      B->setTerminator(
+          CFGTerminator(nullptr, CFGTerminator::VirtualBaseBranch));
+      addSuccessor(B, Block, true);
+    }
   }
 
   if (B)
     Succ = B;
 
   // Backpatch the gotos whose label -> block mappings we didn't know when we
   // encountered them.
   for (BackpatchBlocksTy::iterator I = BackpatchBlocks.begin(),

 /// base and member objects in destructor.
 void CFGBuilder::addImplicitDtorsForDestructor(const CXXDestructorDecl *DD) {
   assert(BuildOpts.AddImplicitDtors &&
          "Can be called only when dtors should be added");
   const CXXRecordDecl *RD = DD->getParent();
 
   // At the end destroy virtual base objects.
   for (const auto &VI : RD->vbases()) {
+    // TODO: Add a VirtualBaseBranch to see if the most derived class
+    // (which is different from the current class) is responsible for
+    // destroying them.
     const CXXRecordDecl *CD = VI.getType()->getAsCXXRecordDecl();
     if (!CD->hasTrivialDestructor()) {
       autoCreateBlock();
       appendBaseDtor(Block, &VI);
     }
   }
 
   // Before virtual bases destroy direct base objects.
void print(CFGTerminator T) {
     switch (T.getKind()) {
     case CFGTerminator::StmtBranch:
       Visit(T.getStmt());
       break;
     case CFGTerminator::TemporaryDtorsBranch:
       OS << "(Temp Dtor) ";
       Visit(T.getStmt());
       break;
+    case CFGTerminator::VirtualBaseBranch:
+      OS << "(See if most derived ctor has already initialized vbases)";
+      break;
     }
   }
 };
 
 } // namespace
 
 static void print_initializer(raw_ostream &OS, StmtPrinterHelper &Helper,
                               const CXXCtorInitializer *I) {

cfe/trunk/lib/StaticAnalyzer/Core/AnalysisManager.cpp

: AnaCtxMgr(
           // unrolling.
           Options.ShouldIncludeLoopExitInCFG ||
             Options.ShouldUnrollLoops,
           Options.ShouldIncludeScopesInCFG,
           Options.ShouldSynthesizeBodies,
           Options.ShouldConditionalizeStaticInitializers,
           /*addCXXNewAllocator=*/true,
           Options.ShouldIncludeRichConstructorsInCFG,
-          Options.ShouldElideConstructors, injector),
+          Options.ShouldElideConstructors,
+          /*addVirtualBaseBranches=*/true,
+          injector),
       Ctx(ASTCtx), Diags(diags), LangOpts(ASTCtx.getLangOpts()),
       PathConsumers(PDC), CreateStoreMgr(storemgr),
       CreateConstraintMgr(constraintmgr), CheckerMgr(checkerMgr),
       options(Options) {
   AnaCtxMgr.getCFGBuildOptions().setAllAlwaysAdd();
 }
 
 AnalysisManager::~AnalysisManager() {

cfe/trunk/lib/StaticAnalyzer/Core/CoreEngine.cpp

switch (Term->getStmtClass()) {
       }
 
       case Stmt::WhileStmtClass:
         HandleBranch(cast<WhileStmt>(Term)->getCond(), Term, B, Pred);
         return;
     }
   }
 
+  if (B->getTerminator().isVirtualBaseBranch()) {
+    HandleVirtualBaseBranch(B, Pred);
+    return;
+  }
+
   assert(B->succ_size() == 1 &&
          "Blocks with no terminator should have at most 1 successor.");
 
   generateNode(BlockEdge(B, *(B->succ_begin()), Pred->getLocationContext()),
                Pred->State, Pred);
 }
 
 void CoreEngine::HandleCallEnter(const CallEnter &CE, ExplodedNode *Pred) {
void CoreEngine::HandlePostStmt(const CFGBlock *B, unsigned StmtIdx,
   if (StmtIdx == B->size())
     HandleBlockExit(B, Pred);
   else {
     NodeBuilderContext Ctx(*this, B, Pred);
     SubEng.processCFGElement((*B)[StmtIdx], Pred, StmtIdx, &Ctx);
   }
 }
 
+void CoreEngine::HandleVirtualBaseBranch(const CFGBlock *B,
+                                         ExplodedNode *Pred) {
+  const LocationContext *LCtx = Pred->getLocationContext();
+  if (const auto *CallerCtor = dyn_cast_or_null<CXXConstructExpr>(
+          LCtx->getStackFrame()->getCallSite())) {
+    switch (CallerCtor->getConstructionKind()) {
+    case CXXConstructExpr::CK_NonVirtualBase:
+    case CXXConstructExpr::CK_VirtualBase: {
+      BlockEdge Loc(B, *B->succ_begin(), LCtx);
+      HandleBlockEdge(Loc, Pred);
+      return;
+    }
+    default:
+      break;
+    }
+  }
+
+  // We either don't see a parent stack frame because we're in the top frame,
+  // or the parent stack frame doesn't initialize our virtual bases.
+  BlockEdge Loc(B, *(B->succ_begin() + 1), LCtx);
+  HandleBlockEdge(Loc, Pred);
+}
+
 /// generateNode - Utility method to generate nodes, hook up successors,
 ///  and add nodes to the worklist.
 void CoreEngine::generateNode(const ProgramPoint &Loc,
                               ProgramStateRef State,
                               ExplodedNode *Pred) {
   bool IsNew;
   ExplodedNode *Node = G.getNode(Loc, State, false, &IsNew);
 

cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE,
   const ConstructionContext *CC = C ? C->getConstructionContext() : nullptr;
 
   switch (CE->getConstructionKind()) {
   case CXXConstructExpr::CK_Complete: {
     std::tie(State, Target) =
         prepareForObjectConstruction(CE, State, LCtx, CC, CallOpts);
     break;
   }
-  case CXXConstructExpr::CK_VirtualBase:
+  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->getStackFrame()->getCallSite()) {
-      const CXXConstructExpr *OuterCtor = dyn_cast<CXXConstructExpr>(Outer);
-      if (OuterCtor) {
-        switch (OuterCtor->getConstructionKind()) {
-        case CXXConstructExpr::CK_NonVirtualBase:
-        case CXXConstructExpr::CK_VirtualBase:
-          // Bail out!
-          destNodes.Add(Pred);
-          return;
-        case CXXConstructExpr::CK_Complete:
-        case CXXConstructExpr::CK_Delegating:
-          break;
-        }
-      }
-    }
+    const auto *OuterCtor = dyn_cast_or_null<CXXConstructExpr>(
+        LCtx->getStackFrame()->getCallSite());
+    assert(
+        (!OuterCtor ||
+         OuterCtor->getConstructionKind() == CXXConstructExpr::CK_Complete ||
+         OuterCtor->getConstructionKind() == CXXConstructExpr::CK_Delegating) &&
+        ("This virtual base should have already been initialized by "
+         "the most derived class!"));
+    (void)OuterCtor;
     LLVM_FALLTHROUGH;
+  }
   case CXXConstructExpr::CK_NonVirtualBase:
     // In C++17, classes with non-virtual bases may be aggregates, so they would
     // be initialized as aggregates without a constructor call, so we may have
     // a base class constructed directly into an initializer list without
     // having the derived-class constructor call on the previous stack frame.
     // Initializer lists may be nested into more initializer lists that
     // correspond to surrounding aggregate initializations.
     // FIXME: For now this code essentially bails out. We need to find the

cfe/trunk/test/Analysis/initializer.cpp

 void foo2() {
   C c { coo() }; // no-crash
 }
 
 B foo_recursive() {
   B b { foo_recursive() };
 }
 } // namespace CXX17_transparent_init_list_exprs
+
+namespace skip_vbase_initializer_side_effects {
+int glob;
+struct S {
+  S() { ++glob; }
+};
+
+struct A {
+  A() {}
+  A(S s) {}
+};
+
+struct B : virtual A {
+  B() : A(S()) {}
+};
+
+struct C : B {
+  C() {}
+};
+
+void foo() {
+  glob = 0;
+  B b;
+  clang_analyzer_eval(glob == 1); // expected-warning{{TRUE}}
+  C c; // no-crash
+  clang_analyzer_eval(glob == 1); // expected-warning{{TRUE}}
+}
+} // namespace skip_vbase_initializer_side_effects
+
+namespace dont_skip_vbase_initializers_in_most_derived_class {
+struct A {
+  static int a;
+  A() { a = 0; }
+  A(int x) { a = x; }
+};
+
+struct B {
+  static int b;
+  B() { b = 0; }
+  B(int y) { b = y; }
+};
+
+struct C : virtual A {
+  C() : A(1) {}
+};
+struct D : C, virtual B {
+  D() : B(2) {}
+};
+
+void testD() {
+  D d;
+  clang_analyzer_eval(A::a == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(B::b == 2); // expected-warning{{TRUE}}
+}
+
+struct E : virtual B, C {
+  E() : B(2) {}
+};
+
+void testE() {
+  E e;
+  clang_analyzer_eval(A::a == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(B::b == 2); // expected-warning{{TRUE}}
+}
+
+struct F : virtual A, virtual B {
+  F() : A(1) {}
+};
+struct G : F {
+  G(): B(2) {}
+};
+
+void testG() {
+  G g;
+  clang_analyzer_eval(A::a == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(B::b == 2); // expected-warning{{TRUE}}
+}
+
+struct H : virtual B, virtual A {
+  H(): A(1) {}
+};
+struct I : H {
+  I(): B(2) {}
+};
+
+void testI() {
+  I i;
+  clang_analyzer_eval(A::a == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(B::b == 2); // expected-warning{{TRUE}}
+}
+} // namespace dont_skip_vbase_initializers_in_most_derived_class

cfe/trunk/test/Analysis/initializers-cfg-output.cpp

public:
   // CHECK:        [B0 (EXIT)]
   // CHECK-NEXT:     Preds (1): B1
   A(int i) {}
 };
 
 class B : public virtual A {
 public:
   // CHECK:       B()
-  // CHECK:        [B2 (ENTRY)]
-  // CHECK-NEXT:     Succs (1): B1
+  // CHECK:        [B3 (ENTRY)]
+  // CHECK-NEXT:     Succs (1): B2
   // CHECK:        [B1]
   // WARNINGS-NEXT:     1:  (CXXConstructExpr, class A)
   // ANALYZER-NEXT:     1:  (CXXConstructExpr, A() (Base initializer), class A)
   // CHECK-NEXT:     2: A([B1.1]) (Base initializer)
   // CHECK-NEXT:     Preds (1): B2
   // CHECK-NEXT:     Succs (1): B0
+  // CHECK:        [B2]
+  // CHECK-NEXT:     T: (See if most derived ctor has already initialized vbases)
+  // CHECK-NEXT:     Preds (1): B3
+  // CHECK-NEXT:     Succs (2): B0 B1
   // CHECK:        [B0 (EXIT)]
-  // CHECK-NEXT:     Preds (1): B1
+  // CHECK-NEXT:     Preds (2): B1 B2
   B() {}
 
   // CHECK:       B(int i)
-  // CHECK:        [B2 (ENTRY)]
-  // CHECK-NEXT:     Succs (1): B1
+  // CHECK:        [B3 (ENTRY)]
+  // CHECK-NEXT:     Succs (1): B2
   // CHECK:        [B1]
   // CHECK-NEXT:     1: i
   // CHECK-NEXT:     2: [B1.1] (ImplicitCastExpr, LValueToRValue, int)
   // WARNINGS-NEXT:     3: [B1.2] (CXXConstructExpr, class A)
   // ANALYZER-NEXT:     3: [B1.2] (CXXConstructExpr, A([B1.2]) (Base initializer), class A)
   // CHECK-NEXT:     4: A([B1.3]) (Base initializer)
   // CHECK-NEXT:     Preds (1): B2
   // CHECK-NEXT:     Succs (1): B0
+  // CHECK:        [B2]
+  // CHECK-NEXT:     T: (See if most derived ctor has already initialized vbases)
+  // CHECK-NEXT:     Preds (1): B3
+  // CHECK-NEXT:     Succs (2): B0 B1
   // CHECK:        [B0 (EXIT)]
-  // CHECK-NEXT:     Preds (1): B1
+  // CHECK-NEXT:     Preds (2): B1 B2
   B(int i) : A(i) {}
 };
 
 class C : public virtual A {
 public:
   // CHECK:       C()
-  // CHECK:        [B2 (ENTRY)]
-  // CHECK-NEXT:     Succs (1): B1
+  // CHECK:        [B3 (ENTRY)]
+  // CHECK-NEXT:     Succs (1): B2
   // CHECK:        [B1]
   // WARNINGS-NEXT:     1:  (CXXConstructExpr, class A)
   // ANALYZER-NEXT:     1:  (CXXConstructExpr, A() (Base initializer), class A)
   // CHECK-NEXT:     2: A([B1.1]) (Base initializer)
   // CHECK-NEXT:     Preds (1): B2
   // CHECK-NEXT:     Succs (1): B0
+  // CHECK:        [B2]
+  // CHECK-NEXT:     T: (See if most derived ctor has already initialized vbases)
+  // CHECK-NEXT:     Preds (1): B3
+  // CHECK-NEXT:     Succs (2): B0 B1
   // CHECK:        [B0 (EXIT)]
-  // CHECK-NEXT:     Preds (1): B1
+  // CHECK-NEXT:     Preds (2): B1 B2
   C() {}
 
   // CHECK:       C(int i)
-  // CHECK:        [B2 (ENTRY)]
-  // CHECK-NEXT:     Succs (1): B1
+  // CHECK:        [B3 (ENTRY)]
+  // CHECK-NEXT:     Succs (1): B2
   // CHECK:        [B1]
   // CHECK-NEXT:     1: i
   // CHECK-NEXT:     2: [B1.1] (ImplicitCastExpr, LValueToRValue, int)
   // WARNINGS-NEXT:     3: [B1.2] (CXXConstructExpr, class A)
   // ANALYZER-NEXT:     3: [B1.2] (CXXConstructExpr, A([B1.2]) (Base initializer), class A)
   // CHECK-NEXT:     4: A([B1.3]) (Base initializer)
   // CHECK-NEXT:     Preds (1): B2
   // CHECK-NEXT:     Succs (1): B0
+  // CHECK:        [B2]
+  // CHECK-NEXT:     T: (See if most derived ctor has already initialized vbases)
+  // CHECK-NEXT:     Preds (1): B3
+  // CHECK-NEXT:     Succs (2): B0 B1
   // CHECK:        [B0 (EXIT)]
-  // CHECK-NEXT:     Preds (1): B1
+  // CHECK-NEXT:     Preds (2): B1 B2
   C(int i) : A(i) {}
 };
 
 
 class TestOrder : public C, public B, public A {
   int i;
   int& r;
 public:
   TestOrder();
 };
 
 // CHECK:       TestOrder::TestOrder()
-// CHECK:        [B2 (ENTRY)]
-// CHECK-NEXT:     Succs (1): B1
+// CHECK:        [B4 (ENTRY)]
+// CHECK-NEXT:     Succs (1): B3
 // CHECK:        [B1]
+// WARNINGS-NEXT:     1:  (CXXConstructExpr, class C)
+// ANALYZER-NEXT:     1:  (CXXConstructExpr, C() (Base initializer), class C)
+// CHECK-NEXT:     2: C([B1.1]) (Base initializer)
+// WARNINGS-NEXT:     3:  (CXXConstructExpr, class B)
+// ANALYZER-NEXT:     3:  (CXXConstructExpr, B() (Base initializer), class B)
+// CHECK-NEXT:     4: B([B1.3]) (Base initializer)
+// WARNINGS-NEXT:     5:  (CXXConstructExpr, class A)
+// ANALYZER-NEXT:     5:  (CXXConstructExpr, A() (Base initializer), class A)
+// CHECK-NEXT:     6: A([B1.5]) (Base initializer)
+// CHECK-NEXT:     7: /*implicit*/(int)0
+// CHECK-NEXT:     8: i([B1.7]) (Member initializer)
+// CHECK-NEXT:     9: this
+// CHECK-NEXT:    10: [B1.9]->i
+// CHECK-NEXT:    11: r([B1.10]) (Member initializer)
+// WARNINGS-NEXT:    12:  (CXXConstructExpr, class A)
+// ANALYZER-NEXT:    12:  (CXXConstructExpr, [B1.13], class A)
+// CHECK-NEXT:    13: A a;
+// CHECK-NEXT:     Preds (2): B2 B3
+// CHECK-NEXT:     Succs (1): B0
+// CHECK:        [B2]
 // WARNINGS-NEXT:     1:  (CXXConstructExpr, class A)
 // ANALYZER-NEXT:     1:  (CXXConstructExpr, A() (Base initializer), class A)
-// CHECK-NEXT:     2: A([B1.1]) (Base initializer)
-// WARNINGS-NEXT:     3:  (CXXConstructExpr, class C)
-// ANALYZER-NEXT:     3:  (CXXConstructExpr, C() (Base initializer), class C)
-// CHECK-NEXT:     4: C([B1.3]) (Base initializer)
-// WARNINGS-NEXT:     5:  (CXXConstructExpr, class B)
-// ANALYZER-NEXT:     5:  (CXXConstructExpr, B() (Base initializer), class B)
-// CHECK-NEXT:     6: B([B1.5]) (Base initializer)
-// WARNINGS-NEXT:     7:  (CXXConstructExpr, class A)
-// ANALYZER-NEXT:     7:  (CXXConstructExpr, A() (Base initializer), class A)
-// CHECK-NEXT:     8: A([B1.7]) (Base initializer)
-// CHECK-NEXT:     9: /*implicit*/(int)0
-// CHECK-NEXT:    10: i([B1.9]) (Member initializer)
-// CHECK-NEXT:    11: this
-// CHECK-NEXT:    12: [B1.11]->i
-// CHECK-NEXT:    13: r([B1.12]) (Member initializer)
-// WARNINGS-NEXT:    14:  (CXXConstructExpr, class A)
-// ANALYZER-NEXT:    14:  (CXXConstructExpr, [B1.15], class A)
-// CHECK-NEXT:    15: A a;
-// CHECK-NEXT:     Preds (1): B2
-// CHECK-NEXT:     Succs (1): B0
+// CHECK-NEXT:     2: A([B2.1]) (Base initializer)
+// CHECK-NEXT:     Preds (1): B3
+// CHECK-NEXT:     Succs (1): B1
+// CHECK:        [B3]
+// CHECK-NEXT:     T: (See if most derived ctor has already initialized vbases)
+// CHECK-NEXT:     Preds (1): B4
+// CHECK-NEXT:     Succs (2): B1 B2
 // CHECK:        [B0 (EXIT)]
 // CHECK-NEXT:     Preds (1): B1
 TestOrder::TestOrder()
   : r(i), B(), i(), C() {
   A a;
 }
 
 class TestControlFlow {
public:
   // CHECK-NEXT:     5: [B1.4] (ImplicitCastExpr, LValueToRValue, int)
   // CHECK-NEXT:     6: z([B1.5]) (Member initializer)
   // CHECK-NEXT:     Preds (1): B2
   // CHECK-NEXT:     Succs (1): B0
   // CHECK:        [B0 (EXIT)]
   // CHECK-NEXT:     Preds (1): B1
   TestDelegating(int x, int z) : x(x), z(z) {}
 };
+
+class TestMoreControlFlow : public virtual A {
+  A a;
+
+public:
+  TestMoreControlFlow(bool coin);
+};
+
+// CHECK:       TestMoreControlFlow::TestMoreControlFlow(bool coin)
+// CHECK:        [B10 (ENTRY)]
+// CHECK-NEXT:     Succs (1): B9
+// CHECK:        [B1]
+// CHECK-NEXT:     1: [B4.2] ? [B2.1] : [B3.1]
+// WARNINGS-NEXT:     2: [B1.1] (CXXConstructExpr, class A)
+// ANALYZER-NEXT:     2: [B1.1] (CXXConstructExpr, a([B1.1]) (Member initializer), class A)
+// CHECK-NEXT:     3: a([B1.2]) (Member initializer)
+// CHECK-NEXT:     Preds (2): B2 B3
+// CHECK-NEXT:     Succs (1): B0
+// CHECK:        [B2]
+// CHECK-NEXT:     1: 3
+// CHECK-NEXT:     Preds (1): B4
+// CHECK-NEXT:     Succs (1): B1
+// CHECK:        [B3]
+// CHECK-NEXT:     1: 4
+// CHECK-NEXT:     Preds (1): B4
+// CHECK-NEXT:     Succs (1): B1
+// CHECK:        [B4]
+// CHECK-NEXT:     1: coin
+// CHECK-NEXT:     2: [B4.1] (ImplicitCastExpr, LValueToRValue, _Bool)
+// CHECK-NEXT:     T: [B4.2] ? ... : ...
+// CHECK-NEXT:     Preds (2): B5 B9
+// CHECK-NEXT:     Succs (2): B2 B3
+// CHECK:        [B5]
+// CHECK-NEXT:     1: [B8.2] ? [B6.1] : [B7.1]
+// WARNINGS-NEXT:     2: [B5.1] (CXXConstructExpr, class A)
+// ANALYZER-NEXT:     2: [B5.1] (CXXConstructExpr, A([B5.1]) (Base initializer), class A)
+// CHECK-NEXT:     3: A([B5.2]) (Base initializer)
+// CHECK-NEXT:     Preds (2): B6 B7
+// CHECK-NEXT:     Succs (1): B4
+// CHECK:        [B6]
+// CHECK-NEXT:     1: 1
+// CHECK-NEXT:     Preds (1): B8
+// CHECK-NEXT:     Succs (1): B5
+// CHECK:        [B7]
+// CHECK-NEXT:     1: 2
+// CHECK-NEXT:     Preds (1): B8
+// CHECK-NEXT:     Succs (1): B5
+// CHECK:        [B8]
+// CHECK-NEXT:     1: coin
+// CHECK-NEXT:     2: [B8.1] (ImplicitCastExpr, LValueToRValue, _Bool)
+// CHECK-NEXT:     T: [B8.2] ? ... : ...
+// CHECK-NEXT:     Preds (1): B9
+// CHECK-NEXT:     Succs (2): B6 B7
+// CHECK:        [B9]
+// CHECK-NEXT:     T: (See if most derived ctor has already initialized vbases)
+// CHECK-NEXT:     Preds (1): B10
+// CHECK-NEXT:     Succs (2): B4 B8
+// CHECK:        [B0 (EXIT)]
+// CHECK-NEXT:     Preds (1): B1
+TestMoreControlFlow::TestMoreControlFlow(bool coin)
+    : A(coin ? 1 : 2), a(coin ? 3 : 4) {}