Index: clang/include/clang/Analysis/AnalysisDeclContext.h =================================================================== --- clang/include/clang/Analysis/AnalysisDeclContext.h +++ clang/include/clang/Analysis/AnalysisDeclContext.h @@ -459,6 +459,7 @@ bool addCXXNewAllocator = true, bool addRichCXXConstructors = true, bool markElidedCXXConstructors = true, + bool addVirtualBaseBranches = true, CodeInjector *injector = nullptr); AnalysisDeclContext *getContext(const Decl *D); Index: clang/include/clang/Analysis/CFG.h =================================================================== --- clang/include/clang/Analysis/CFG.h +++ clang/include/clang/Analysis/CFG.h @@ -504,15 +504,19 @@ /// 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 Data; @@ -532,6 +536,9 @@ 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 @@ /// 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 @@ bool AddCXXDefaultInitExprInCtors = false; bool AddRichCXXConstructors = false; bool MarkElidedCXXConstructors = false; + bool AddVirtualBaseBranches = false; BuildOptions() = default; Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h =================================================================== --- clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h +++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h @@ -116,6 +116,8 @@ 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); Index: clang/lib/Analysis/AnalysisDeclContext.cpp =================================================================== --- clang/lib/Analysis/AnalysisDeclContext.cpp +++ clang/lib/Analysis/AnalysisDeclContext.cpp @@ -70,7 +70,7 @@ 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 @@ cfgBuildOptions.AddCXXNewAllocator = addCXXNewAllocator; cfgBuildOptions.AddRichCXXConstructors = addRichCXXConstructors; cfgBuildOptions.MarkElidedCXXConstructors = markElidedCXXConstructors; + cfgBuildOptions.AddVirtualBaseBranches = addVirtualBaseBranches; } void AnalysisDeclContextManager::clear() { Contexts.clear(); } Index: clang/lib/Analysis/CFG.cpp =================================================================== --- clang/lib/Analysis/CFG.cpp +++ clang/lib/Analysis/CFG.cpp @@ -1431,13 +1431,41 @@ if (badCFG) return nullptr; - // For C++ constructor add initializers to CFG. - if (const CXXConstructorDecl *CD = dyn_cast_or_null(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(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 @@ // 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 @@ OS << "(Temp Dtor) "; Visit(T.getStmt()); break; + case CFGTerminator::VirtualBaseBranch: + OS << "(See if most derived ctor has already initialized vbases)"; + break; } } }; Index: clang/lib/StaticAnalyzer/Core/AnalysisManager.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/AnalysisManager.cpp +++ clang/lib/StaticAnalyzer/Core/AnalysisManager.cpp @@ -35,7 +35,9 @@ 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), Index: clang/lib/StaticAnalyzer/Core/CoreEngine.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/CoreEngine.cpp +++ clang/lib/StaticAnalyzer/Core/CoreEngine.cpp @@ -380,6 +380,11 @@ } } + 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::HandleVirtualBaseBranch(const CFGBlock *B, + ExplodedNode *Pred) { + const LocationContext *LCtx = Pred->getLocationContext(); + if (const auto *CallerCtor = dyn_cast_or_null( + 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, Index: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp +++ clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp @@ -428,25 +428,20 @@ 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(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( + 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 Index: clang/test/Analysis/initializer.cpp =================================================================== --- clang/test/Analysis/initializer.cpp +++ clang/test/Analysis/initializer.cpp @@ -275,3 +275,94 @@ 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 Index: clang/test/Analysis/initializers-cfg-output.cpp =================================================================== --- clang/test/Analysis/initializers-cfg-output.cpp +++ clang/test/Analysis/initializers-cfg-output.cpp @@ -30,21 +30,25 @@ 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) @@ -53,29 +57,37 @@ // 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) @@ -84,8 +96,12 @@ // 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) {} }; @@ -98,31 +114,38 @@ }; // 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() @@ -209,3 +232,64 @@ // 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) {}