From d4ba9bd25d746870e5a5e5d09a4f24cc3d72ac30 Mon Sep 17 00:00:00 2001 From: Devin Coughlin Date: Thu, 17 Dec 2015 00:28:33 +0000 Subject: [PATCH] [analyzer] Better detect when C++ object was constructed into existing region. When the analyzer evaluates a CXXConstructExpr, it looks ahead in the CFG for the current block to detect what region the object should be constructed into. If the constructor was directly constructed into a local variable or field region then there is no need to explicitly bind the constructed value to the local or field when analyzing the DeclStmt or CXXCtorInitializer that called the constructor. Unfortunately, there were situations in which the CXXConstructExpr was constructed into a temporary region but when evaluating the corresponding DeclStmt or CXXCtorInitializer the analyzer assumed the object was constructed into the local or field. This led to spurious warnings about uninitialized values (PR25777). To avoid these false positives, this commit factors out the logic for determining when a CXXConstructExpr will be directly constructed into existing storage, adds the inverse logic to detect when the corresponding later bind can be safely skipped, and adds assertions to make sure these two checks are in sync. rdar://problem/21947725 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@255859 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../Core/PathSensitive/ExprEngine.h | 22 +++ lib/StaticAnalyzer/Core/ExprEngine.cpp | 8 +- lib/StaticAnalyzer/Core/ExprEngineC.cpp | 5 +- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp | 126 +++++++++++++----- test/Analysis/initializer.cpp | 54 ++++++++ 5 files changed, 177 insertions(+), 38 deletions(-) diff --git a/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h b/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h index 8337f770d4..99083c9d61 100644 --- a/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h +++ b/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h @@ -604,6 +604,28 @@ private: const LocationContext *LC, const Expr *E, const Expr *ResultE = nullptr); + + /// For a DeclStmt or CXXInitCtorInitializer, walk backward in the current CFG + /// block to find the constructor expression that directly constructed into + /// the storage for this statement. Returns null if the constructor for this + /// statement created a temporary object region rather than directly + /// constructing into an existing region. + const CXXConstructExpr *findDirectConstructorForCurrentCFGElement(); + + /// For a CXXConstructExpr, walk forward in the current CFG block to find the + /// CFGElement for the DeclStmt or CXXInitCtorInitializer for which is + /// directly constructed by this constructor. Returns None if the current + /// constructor expression did not directly construct into an existing + /// region. + Optional findElementDirectlyInitializedByCurrentConstructor(); + + /// For a given constructor, look forward in the current CFG block to + /// determine the region into which an object will be constructed by \p CE. + /// Returns either a field or local variable region if the object will be + /// directly constructed in an existing region or a temporary object region + /// if not. + const MemRegion *getRegionForConstructedObject(const CXXConstructExpr *CE, + ExplodedNode *Pred); }; /// Traits for storing the call processing policy inside GDM. diff --git a/lib/StaticAnalyzer/Core/ExprEngine.cpp b/lib/StaticAnalyzer/Core/ExprEngine.cpp index 7ef91d6538..662b0a2dd7 100644 --- a/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -476,8 +476,12 @@ void ExprEngine::ProcessInitializer(const CFGInitializer Init, if (BMI->isAnyMemberInitializer()) { // Constructors build the object directly in the field, // but non-objects must be copied in from the initializer. - const Expr *Init = BMI->getInit()->IgnoreImplicit(); - if (!isa(Init)) { + if (auto *CtorExpr = findDirectConstructorForCurrentCFGElement()) { + assert(BMI->getInit()->IgnoreImplicit() == CtorExpr); + (void)CtorExpr; + // The field was directly constructed, so there is no need to bind. + } else { + const Expr *Init = BMI->getInit()->IgnoreImplicit(); const ValueDecl *Field; if (BMI->isIndirectMemberInitializer()) { Field = BMI->getIndirectMember(); diff --git a/lib/StaticAnalyzer/Core/ExprEngineC.cpp b/lib/StaticAnalyzer/Core/ExprEngineC.cpp index 35af305524..a5b58710b2 100644 --- a/lib/StaticAnalyzer/Core/ExprEngineC.cpp +++ b/lib/StaticAnalyzer/Core/ExprEngineC.cpp @@ -492,7 +492,10 @@ void ExprEngine::VisitDeclStmt(const DeclStmt *DS, ExplodedNode *Pred, ExplodedNode *UpdatedN = N; SVal InitVal = state->getSVal(InitEx, LC); - if (isa(InitEx->IgnoreImplicit())) { + assert(DS->isSingleDecl()); + if (auto *CtorExpr = findDirectConstructorForCurrentCFGElement()) { + assert(InitEx->IgnoreImplicit() == CtorExpr); + (void)CtorExpr; // We constructed the object directly in the variable. // No need to bind anything. B.generateNode(DS, UpdatedN, state); diff --git a/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp index 07ae41027d..556e2239ab 100644 --- a/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp +++ b/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp @@ -103,49 +103,32 @@ static SVal makeZeroElementRegion(ProgramStateRef State, SVal LValue, } -static const MemRegion *getRegionForConstructedObject( - const CXXConstructExpr *CE, ExplodedNode *Pred, ExprEngine &Eng, - unsigned int CurrStmtIdx) { +const MemRegion * +ExprEngine::getRegionForConstructedObject(const CXXConstructExpr *CE, + ExplodedNode *Pred) { const LocationContext *LCtx = Pred->getLocationContext(); ProgramStateRef State = Pred->getState(); - const NodeBuilderContext &CurrBldrCtx = Eng.getBuilderContext(); // See if we're constructing an existing region by looking at the next // element in the CFG. - const CFGBlock *B = CurrBldrCtx.getBlock(); - unsigned int NextStmtIdx = CurrStmtIdx + 1; - if (NextStmtIdx < B->size()) { - CFGElement Next = (*B)[NextStmtIdx]; - - // Is this a destructor? If so, we might be in the middle of an assignment - // to a local or member: look ahead one more element to see what we find. - while (Next.getAs() && NextStmtIdx + 1 < B->size()) { - ++NextStmtIdx; - Next = (*B)[NextStmtIdx]; - } - // Is this a constructor for a local variable? - if (Optional StmtElem = Next.getAs()) { - if (const DeclStmt *DS = dyn_cast(StmtElem->getStmt())) { - if (const VarDecl *Var = dyn_cast(DS->getSingleDecl())) { - if (Var->getInit() && Var->getInit()->IgnoreImplicit() == CE) { - SVal LValue = State->getLValue(Var, LCtx); - QualType Ty = Var->getType(); - LValue = makeZeroElementRegion(State, LValue, Ty); - return LValue.getAsRegion(); - } + if (auto Elem = findElementDirectlyInitializedByCurrentConstructor()) { + if (Optional StmtElem = Elem->getAs()) { + auto *DS = cast(StmtElem->getStmt()); + if (const auto *Var = dyn_cast(DS->getSingleDecl())) { + if (Var->getInit() && Var->getInit()->IgnoreImplicit() == CE) { + SVal LValue = State->getLValue(Var, LCtx); + QualType Ty = Var->getType(); + LValue = makeZeroElementRegion(State, LValue, Ty); + return LValue.getAsRegion(); } } - } - - // Is this a constructor for a member? - if (Optional InitElem = Next.getAs()) { + } else if (Optional InitElem = Elem->getAs()) { const CXXCtorInitializer *Init = InitElem->getInitializer(); assert(Init->isAnyMemberInitializer()); - const CXXMethodDecl *CurCtor = cast(LCtx->getDecl()); - Loc ThisPtr = Eng.getSValBuilder().getCXXThis(CurCtor, - LCtx->getCurrentStackFrame()); + Loc ThisPtr = + getSValBuilder().getCXXThis(CurCtor, LCtx->getCurrentStackFrame()); SVal ThisVal = State->getSVal(ThisPtr); const ValueDecl *Field; @@ -167,13 +150,86 @@ static const MemRegion *getRegionForConstructedObject( // Don't forget to update the pre-constructor initialization code in // ExprEngine::VisitCXXConstructExpr. } - // If we couldn't find an existing region to construct into, assume we're // constructing a temporary. - MemRegionManager &MRMgr = Eng.getSValBuilder().getRegionManager(); + MemRegionManager &MRMgr = getSValBuilder().getRegionManager(); return MRMgr.getCXXTempObjectRegion(CE, LCtx); } +/// Returns true if the initializer for \Elem can be a direct +/// constructor. +static bool canHaveDirectConstructor(CFGElement Elem){ + // DeclStmts and CXXCtorInitializers for fields can be directly constructed. + + if (Optional StmtElem = Elem.getAs()) { + if (isa(StmtElem->getStmt())) { + return true; + } + } + + if (Elem.getKind() == CFGElement::Initializer) { + return true; + } + + return false; +} + +Optional +ExprEngine::findElementDirectlyInitializedByCurrentConstructor() { + const NodeBuilderContext &CurrBldrCtx = getBuilderContext(); + // See if we're constructing an existing region by looking at the next + // element in the CFG. + const CFGBlock *B = CurrBldrCtx.getBlock(); + assert(isa(((*B)[currStmtIdx]).castAs().getStmt())); + unsigned int NextStmtIdx = currStmtIdx + 1; + if (NextStmtIdx >= B->size()) + return None; + + CFGElement Next = (*B)[NextStmtIdx]; + + // Is this a destructor? If so, we might be in the middle of an assignment + // to a local or member: look ahead one more element to see what we find. + while (Next.getAs() && NextStmtIdx + 1 < B->size()) { + ++NextStmtIdx; + Next = (*B)[NextStmtIdx]; + } + + if (canHaveDirectConstructor(Next)) + return Next; + + return None; +} + +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(); + assert(canHaveDirectConstructor((*B)[currStmtIdx])); + + unsigned int PreviousStmtIdx = currStmtIdx - 1; + CFGElement Previous = (*B)[PreviousStmtIdx]; + + while (Previous.getAs() && PreviousStmtIdx > 0) { + --PreviousStmtIdx; + Previous = (*B)[PreviousStmtIdx]; + } + + if (Optional PrevStmtElem = Previous.getAs()) { + if (auto *CtorExpr = dyn_cast(PrevStmtElem->getStmt())) { + return CtorExpr; + } + } + + return nullptr; +} + void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE, ExplodedNode *Pred, ExplodedNodeSet &destNodes) { @@ -188,7 +244,7 @@ void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE, switch (CE->getConstructionKind()) { case CXXConstructExpr::CK_Complete: { - Target = getRegionForConstructedObject(CE, Pred, *this, currStmtIdx); + Target = getRegionForConstructedObject(CE, Pred); break; } case CXXConstructExpr::CK_VirtualBase: diff --git a/test/Analysis/initializer.cpp b/test/Analysis/initializer.cpp index a71e35dc63..b31c315ba5 100644 --- a/test/Analysis/initializer.cpp +++ b/test/Analysis/initializer.cpp @@ -143,3 +143,57 @@ namespace DefaultMemberInitializers { clang_analyzer_eval(w.p[1] == 'y'); // expected-warning{{TRUE}} } } + +namespace ReferenceInitialization { + struct OtherStruct { + OtherStruct(int i); + ~OtherStruct(); + }; + + struct MyStruct { + MyStruct(int i); + MyStruct(OtherStruct os); + + void method() const; + }; + + void referenceInitializeLocal() { + const MyStruct &myStruct(5); + myStruct.method(); // no-warning + } + + void referenceInitializeMultipleLocals() { + const MyStruct &myStruct1(5), myStruct2(5), &myStruct3(5); + myStruct1.method(); // no-warning + myStruct2.method(); // no-warning + myStruct3.method(); // no-warning + } + + void referenceInitializeLocalWithCleanup() { + const MyStruct &myStruct(OtherStruct(5)); + myStruct.method(); // no-warning + } + + struct HasMyStruct { + const MyStruct &ms; // expected-note {{reference member declared here}} + const MyStruct &msWithCleanups; // expected-note {{reference member declared here}} + + // clang's Sema issues a warning when binding a reference member to a + // temporary value. + HasMyStruct() : ms(5), msWithCleanups(OtherStruct(5)) { + // expected-warning@-1 {{binding reference member 'ms' to a temporary value}} + // expected-warning@-2 {{binding reference member 'msWithCleanups' to a temporary value}} + + // At this point the members are not garbage so we should not expect an + // analyzer warning here even though binding a reference member + // to a member is a terrible idea. + ms.method(); // no-warning + msWithCleanups.method(); // no-warning + } + }; + + void referenceInitializeField() { + HasMyStruct hms; + } + +}; -- 2.40.0