[CFG] Add branch to skip vbase inits when they're handled by superclass.

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 their arguments and their potential side effects, which was causing
pr41300 (and was generally incorrect). The previous skipping behavior is
now 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.

Differential Revision: https://reviews.llvm.org/D61816

llvm-svn: 361681

clang/include/clang/Analysis/AnalysisDeclContext.h

@@ -459,6 +459,7 @@ class AnalysisDeclContextManager {
                              bool addCXXNewAllocator = true,
                              bool addRichCXXConstructors = true,
                              bool markElidedCXXConstructors = true,
+                             bool addVirtualBaseBranches = true,
                              CodeInjector *injector = nullptr);
 
   AnalysisDeclContext *getContext(const Decl *D);

clang/include/clang/Analysis/CFG.h

@@ -504,15 +504,19 @@ class CFGTerminator {
     /// 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;
@@ -532,6 +536,9 @@ class CFGTerminator {
   bool isTemporaryDtorsBranch() const {
     return getKind() == TemporaryDtorsBranch;
   }
+  bool isVirtualBaseBranch() const {
+    return getKind() == VirtualBaseBranch;
+  }
 };
 
 /// Represents a single basic block in a source-level CFG.
@@ -552,11 +559,12 @@ class CFGTerminator {
 /// 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 {
@@ -1039,6 +1047,7 @@ class CFG {
     bool AddCXXDefaultInitExprInCtors = false;
     bool AddRichCXXConstructors = false;
     bool MarkElidedCXXConstructors = false;
+    bool AddVirtualBaseBranches = false;
 
     BuildOptions() = default;
 

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

@@ -116,6 +116,8 @@ class CoreEngine {
   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);

clang/lib/Analysis/AnalysisDeclContext.cpp

@@ -70,7 +70,7 @@ AnalysisDeclContextManager::AnalysisDeclContextManager(
     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;
@@ -84,6 +84,7 @@ AnalysisDeclContextManager::AnalysisDeclContextManager(
   cfgBuildOptions.AddCXXNewAllocator = addCXXNewAllocator;
   cfgBuildOptions.AddRichCXXConstructors = addRichCXXConstructors;
   cfgBuildOptions.MarkElidedCXXConstructors = markElidedCXXConstructors;
+  cfgBuildOptions.AddVirtualBaseBranches = addVirtualBaseBranches;
 }
 
 void AnalysisDeclContextManager::clear() { Contexts.clear(); }

clang/lib/Analysis/CFG.cpp

@@ -1431,13 +1431,41 @@ std::unique_ptr<CFG> CFGBuilder::buildCFG(const Decl *D, Stmt *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)
@@ -1769,6 +1797,9 @@ void CFGBuilder::addImplicitDtorsForDestructor(const CXXDestructorDecl *DD) {
 
   // 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();
@@ -5066,6 +5097,9 @@ class CFGBlockTerminatorPrint
       OS << "(Temp Dtor) ";
       Visit(T.getStmt());
       break;
+    case CFGTerminator::VirtualBaseBranch:
+      OS << "(See if most derived ctor has already initialized vbases)";
+      break;
     }
   }
 };

clang/lib/StaticAnalyzer/Core/AnalysisManager.cpp

@@ -35,7 +35,9 @@ AnalysisManager::AnalysisManager(ASTContext &ASTCtx, DiagnosticsEngine &diags,
           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),

clang/lib/StaticAnalyzer/Core/CoreEngine.cpp

@@ -380,6 +380,11 @@ void CoreEngine::HandleBlockExit(const CFGBlock * B, ExplodedNode *Pred) {
     }
   }
 
+  if (B->getTerminator().isVirtualBaseBranch()) {
+    HandleVirtualBaseBranch(B, Pred);
+    return;
+  }
+
   assert(B->succ_size() == 1 &&
          "Blocks with no terminator should have at most 1 successor.");
 
@@ -439,6 +444,29 @@ void CoreEngine::HandlePostStmt(const CFGBlock *B, unsigned StmtIdx,
   }
 }
 
+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,

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

@@ -428,25 +428,20 @@ void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE,
         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

clang/test/Analysis/initializer.cpp

@@ -275,3 +275,94 @@ 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