From a84374dc0e4e756a92191dd55efca43165e3977d Mon Sep 17 00:00:00 2001 From: Artem Dergachev Date: Thu, 14 Jun 2018 01:40:49 +0000 Subject: [PATCH] [analyzer] Track class member initializer constructors path-sensitively. The reasoning behind this change is similar to the previous commit, r334681. Because members are already in scope when construction occurs, we are not suffering from liveness problems, but we still want to figure out if the object was constructed with construction context, because in this case we'll be able to avoid trivial copy, which we don't always model perfectly. It'd also have more importance when copy elision is implemented. This also gets rid of the old CFG look-behind mechanism. Differential Revision: https://reviews.llvm.org/D47350 llvm-svn: 334682 --- .../Core/PathSensitive/ExprEngine.h | 17 ++- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 137 ++++++++++++++---- .../lib/StaticAnalyzer/Core/ExprEngineCXX.cpp | 30 +--- .../test/Analysis/cxx17-mandatory-elision.cpp | 31 ++++ 4 files changed, 148 insertions(+), 67 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h index b5e210232417..e469df0d15e4 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h @@ -762,20 +762,23 @@ private: /// made within the constructor alive until its declaration actually /// goes into scope. static ProgramStateRef addObjectUnderConstruction( - ProgramStateRef State, const Stmt *S, + ProgramStateRef State, + llvm::PointerUnion 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 P, + const LocationContext *LC); /// If the given statement corresponds to an object under construction, /// being part of its construciton context, retrieve that object's location. - static Optional getObjectUnderConstruction(ProgramStateRef State, - const Stmt *S, - const LocationContext *LC); + static Optional getObjectUnderConstruction( + ProgramStateRef State, + llvm::PointerUnion P, + const LocationContext *LC); /// Check if all objects under construction have been fully constructed /// for the given context range (including FromLC, not including ToLC). diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index 3fed9c088b85..939b21944d94 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -109,7 +109,75 @@ STATISTIC(NumTimesRetriedWithoutInlining, // to the object's location, so that on every such statement the location // could have been retrieved. -typedef std::pair 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 LocationContext *> ConstructedObjectKeyImpl; + + ConstructedObjectKeyImpl Impl; + + const void *getAnyASTNodePtr() const { + if (const Stmt *S = getStmt()) + return S; + else + return getCXXCtorInitializer(); + } + +public: + ConstructedObjectKey( + llvm::PointerUnion 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(getStmt()) || + isa(getStmt()) || isa(getStmt()) || + isa(getStmt())); + } + + const Stmt *getStmt() const { + return Impl.first.dyn_cast(); + } + + const CXXCtorInitializer *getCXXCtorInitializer() const { + return Impl.first.dyn_cast(); + } + + 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 ObjectsUnderConstructionMap; REGISTER_TRAIT_WITH_PROGRAMSTATE(ObjectsUnderConstruction, @@ -378,26 +446,31 @@ ExprEngine::createTemporaryRegionIfNeeded(ProgramStateRef State, 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 P, + const LocationContext *LC, SVal V) { + 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(Key) || - isa(S)); + isa(Key.getStmt())); return State->set(Key, V); } -Optional ExprEngine::getObjectUnderConstruction(ProgramStateRef State, - const Stmt *S, const LocationContext *LC) { - ConstructedObjectKey Key(S, LC->getCurrentStackFrame()); +Optional ExprEngine::getObjectUnderConstruction( + ProgramStateRef State, + llvm::PointerUnion P, + const LocationContext *LC) { + ConstructedObjectKey Key(P, LC->getCurrentStackFrame()); return Optional::create(State->get(Key)); } -ProgramStateRef ExprEngine::finishObjectConstruction(ProgramStateRef State, - const Stmt *S, const LocationContext *LC) { - ConstructedObjectKey Key(S, LC->getCurrentStackFrame()); +ProgramStateRef ExprEngine::finishObjectConstruction( + ProgramStateRef State, + llvm::PointerUnion P, + const LocationContext *LC) { + ConstructedObjectKey Key(P, LC->getCurrentStackFrame()); assert(State->contains(Key)); return State->remove(Key); } @@ -409,7 +482,7 @@ bool ExprEngine::areAllObjectsFullyConstructed(ProgramStateRef State, while (LC != ToLC) { assert(LC && "ToLC must be a parent of FromLC!"); for (auto I : State->get()) - if (I.first.second == LC) + if (I.first.getLocationContext() == LC) return false; LC = LC->getParent(); @@ -451,10 +524,9 @@ static void printObjectsUnderConstructionForContext(raw_ostream &Out, for (auto I : State->get()) { 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; } } @@ -677,9 +749,11 @@ void ExprEngine::ProcessLoopExit(const Stmt* S, ExplodedNode *Pred) { 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(), @@ -692,19 +766,21 @@ void ExprEngine::ProcessInitializer(const CFGInitializer Init, 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(); @@ -738,15 +814,12 @@ void ExprEngine::ProcessInitializer(const CFGInitializer Init, 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. } @@ -755,8 +828,10 @@ void ExprEngine::ProcessInitializer(const CFGInitializer Init, 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); @@ -2119,11 +2194,11 @@ void ExprEngine::processEndOfFunction(NodeBuilderContext& BC, while (LC != ToLC) { assert(LC && "ToLC must be a parent of FromLC!"); for (auto I : State->get()) - 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(I.first.first)); + assert(isa(I.first.getStmt())); State = State->remove(I.first); } diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp index dd4660fff99a..ada99e1369d9 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp @@ -153,6 +153,7 @@ std::pair ExprEngine::prepareForObjectConstruction( 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: { @@ -272,35 +273,6 @@ std::pair ExprEngine::prepareForObjectConstruction( 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() && 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) { diff --git a/clang/test/Analysis/cxx17-mandatory-elision.cpp b/clang/test/Analysis/cxx17-mandatory-elision.cpp index a1645a7efe11..6a88bbaad9b9 100644 --- a/clang/test/Analysis/cxx17-mandatory-elision.cpp +++ b/clang/test/Analysis/cxx17-mandatory-elision.cpp @@ -21,6 +21,37 @@ struct B { } // 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 struct AddressVector {