From ecee1651c100342366a9417c85c6e50399039930 Mon Sep 17 00:00:00 2001 From: Jordan Rose Date: Wed, 3 Apr 2013 01:39:08 +0000 Subject: [PATCH] [analyzer] Better model for copying of array fields in implicit copy ctors. - Find the correct region to represent the first array element when constructing a CXXConstructorCall. - If the array is trivial, model the copy with a primitive load/store. - Don't warn about the "uninitialized" subscript in the AST -- we don't use the helper variable that Sema provides. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@178602 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../UndefinedArraySubscriptChecker.cpp | 34 +-- lib/StaticAnalyzer/Core/ExprEngine.cpp | 33 ++- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp | 64 +++--- .../Core/ExprEngineCallAndReturn.cpp | 2 + test/Analysis/ctor-inlining.mm | 193 ++++++++++++++++++ 5 files changed, 288 insertions(+), 38 deletions(-) diff --git a/lib/StaticAnalyzer/Checkers/UndefinedArraySubscriptChecker.cpp b/lib/StaticAnalyzer/Checkers/UndefinedArraySubscriptChecker.cpp index be3a34f3ea..176ee48082 100644 --- a/lib/StaticAnalyzer/Checkers/UndefinedArraySubscriptChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/UndefinedArraySubscriptChecker.cpp @@ -34,18 +34,28 @@ public: void UndefinedArraySubscriptChecker::checkPreStmt(const ArraySubscriptExpr *A, CheckerContext &C) const { - if (C.getState()->getSVal(A->getIdx(), C.getLocationContext()).isUndef()) { - if (ExplodedNode *N = C.generateSink()) { - if (!BT) - BT.reset(new BuiltinBug("Array subscript is undefined")); - - // Generate a report for this bug. - BugReport *R = new BugReport(*BT, BT->getName(), N); - R->addRange(A->getIdx()->getSourceRange()); - bugreporter::trackNullOrUndefValue(N, A->getIdx(), *R); - C.emitReport(R); - } - } + const Expr *Index = A->getIdx(); + if (!C.getSVal(Index).isUndef()) + return; + + // Sema generates anonymous array variables for copying array struct fields. + // Don't warn if we're in an implicitly-generated constructor. + const Decl *D = C.getLocationContext()->getDecl(); + if (const CXXConstructorDecl *Ctor = dyn_cast(D)) + if (Ctor->isImplicitlyDefined()) + return; + + ExplodedNode *N = C.generateSink(); + if (!N) + return; + if (!BT) + BT.reset(new BuiltinBug("Array subscript is undefined")); + + // Generate a report for this bug. + BugReport *R = new BugReport(*BT, BT->getName(), N); + R->addRange(A->getIdx()->getSourceRange()); + bugreporter::trackNullOrUndefValue(N, A->getIdx(), *R); + C.emitReport(R); } void ento::registerUndefinedArraySubscriptChecker(CheckerManager &mgr) { diff --git a/lib/StaticAnalyzer/Core/ExprEngine.cpp b/lib/StaticAnalyzer/Core/ExprEngine.cpp index 789abb7d86..ffc382fcf0 100644 --- a/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -432,14 +432,41 @@ void ExprEngine::ProcessInitializer(const CFGInitializer Init, // but non-objects must be copied in from the initializer. const Expr *Init = BMI->getInit()->IgnoreImplicit(); if (!isa(Init)) { + const ValueDecl *Field; SVal FieldLoc; - if (BMI->isIndirectMemberInitializer()) + if (BMI->isIndirectMemberInitializer()) { + Field = BMI->getIndirectMember(); FieldLoc = State->getLValue(BMI->getIndirectMember(), thisVal); - else + } else { + Field = BMI->getMember(); FieldLoc = State->getLValue(BMI->getMember(), thisVal); + } - SVal InitVal = State->getSVal(BMI->getInit(), stackFrame); + SVal InitVal; + if (BMI->getNumArrayIndices() > 0) { + // Handle arrays of trivial type. We can represent this with a + // primitive load/copy from the base array region. + const ArraySubscriptExpr *ASE; + while ((ASE = dyn_cast(Init))) + Init = ASE->getBase()->IgnoreImplicit(); + + SVal LValue = State->getSVal(Init, stackFrame); + if (Optional LValueLoc = LValue.getAs()) + InitVal = State->getSVal(*LValueLoc); + + // If we fail to get the value for some reason, use a symbolic value. + if (InitVal.isUnknownOrUndef()) { + SValBuilder &SVB = getSValBuilder(); + InitVal = SVB.conjureSymbolVal(BMI->getInit(), stackFrame, + Field->getType(), + currBldrCtx->blockCount()); + } + } else { + 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(); evalBind(Tmp, Init, Pred, FieldLoc, InitVal, /*isInit=*/true, &PP); } diff --git a/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp index b0f2579cd9..ed90dc5891 100644 --- a/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp +++ b/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp @@ -96,6 +96,26 @@ void ExprEngine::performTrivialCopy(NodeBuilder &Bldr, ExplodedNode *Pred, } } + +/// Returns a region representing the first element of a (possibly +/// multi-dimensional) array. +/// +/// On return, \p Ty will be set to the base type of the array. +/// +/// If the type is not an array type at all, the original value is returned. +static SVal makeZeroElementRegion(ProgramStateRef State, SVal LValue, + QualType &Ty) { + SValBuilder &SVB = State->getStateManager().getSValBuilder(); + ASTContext &Ctx = SVB.getContext(); + + while (const ArrayType *AT = Ctx.getAsArrayType(Ty)) { + Ty = AT->getElementType(); + LValue = State->getLValue(Ty, SVB.makeZeroArrayIndex(), LValue); + } + + return LValue; +} + void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE, ExplodedNode *Pred, ExplodedNodeSet &destNodes) { @@ -103,7 +123,10 @@ void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE, ProgramStateRef State = Pred->getState(); const MemRegion *Target = 0; - bool IsArray = false; + + // FIXME: Handle arrays, which run the same constructor for every element. + // For now, we just run the first constructor (which should still invalidate + // the entire array). switch (CE->getConstructionKind()) { case CXXConstructExpr::CK_Complete: { @@ -118,19 +141,10 @@ void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE, if (const DeclStmt *DS = dyn_cast(StmtElem->getStmt())) { if (const VarDecl *Var = dyn_cast(DS->getSingleDecl())) { if (Var->getInit()->IgnoreImplicit() == CE) { + SVal LValue = State->getLValue(Var, LCtx); QualType Ty = Var->getType(); - if (const ArrayType *AT = getContext().getAsArrayType(Ty)) { - // FIXME: Handle arrays, which run the same constructor for - // every element. This workaround will just run the first - // constructor (which should still invalidate the entire array). - SVal Base = State->getLValue(Var, LCtx); - Target = State->getLValue(AT->getElementType(), - getSValBuilder().makeZeroArrayIndex(), - Base).getAsRegion(); - IsArray = true; - } else { - Target = State->getLValue(Var, LCtx).getAsRegion(); - } + LValue = makeZeroElementRegion(State, LValue, Ty); + Target = LValue.getAsRegion(); } } } @@ -146,13 +160,19 @@ void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE, LCtx->getCurrentStackFrame()); SVal ThisVal = State->getSVal(ThisPtr); + const ValueDecl *Field; + SVal FieldVal; if (Init->isIndirectMemberInitializer()) { - SVal Field = State->getLValue(Init->getIndirectMember(), ThisVal); - Target = Field.getAsRegion(); + Field = Init->getIndirectMember(); + FieldVal = State->getLValue(Init->getIndirectMember(), ThisVal); } else { - SVal Field = State->getLValue(Init->getMember(), ThisVal); - Target = Field.getAsRegion(); + Field = Init->getMember(); + FieldVal = State->getLValue(Init->getMember(), ThisVal); } + + QualType Ty = Field->getType(); + FieldVal = makeZeroElementRegion(State, FieldVal, Ty); + Target = FieldVal.getAsRegion(); } // FIXME: This will eventually need to handle new-expressions as well. @@ -202,6 +222,7 @@ void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE, ExplodedNodeSet DstEvaluated; StmtNodeBuilder Bldr(DstPreCall, DstEvaluated, *currBldrCtx); + bool IsArray = isa(Target); if (CE->getConstructor()->isTrivial() && CE->getConstructor()->isCopyOrMoveConstructor() && !IsArray) { @@ -234,12 +255,9 @@ void ExprEngine::VisitCXXDestructor(QualType ObjectType, // FIXME: We need to run the same destructor on every element of the array. // This workaround will just run the first destructor (which will still // invalidate the entire array). - // This is a loop because of multidimensional arrays. - while (const ArrayType *AT = getContext().getAsArrayType(ObjectType)) { - ObjectType = AT->getElementType(); - Dest = State->getLValue(ObjectType, getSValBuilder().makeZeroArrayIndex(), - loc::MemRegionVal(Dest)).getAsRegion(); - } + SVal DestVal = loc::MemRegionVal(Dest); + DestVal = makeZeroElementRegion(State, DestVal, ObjectType); + Dest = DestVal.getAsRegion(); const CXXRecordDecl *RecordDecl = ObjectType->getAsCXXRecordDecl(); assert(RecordDecl && "Only CXXRecordDecls should have destructors"); diff --git a/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp b/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp index ee603d728e..f01e4e7640 100644 --- a/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp +++ b/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp @@ -605,6 +605,8 @@ static CallInlinePolicy mayInlineCallKind(const CallEvent &Call, const CXXConstructorCall &Ctor = cast(Call); // FIXME: We don't handle constructors or destructors for arrays properly. + // Even once we do, we still need to be careful about implicitly-generated + // initializers for array fields in default move/copy constructors. const MemRegion *Target = Ctor.getCXXThisVal().getAsRegion(); if (Target && isa(Target)) return CIP_DisallowedOnce; diff --git a/test/Analysis/ctor-inlining.mm b/test/Analysis/ctor-inlining.mm index fd8caf324a..8cdb005968 100644 --- a/test/Analysis/ctor-inlining.mm +++ b/test/Analysis/ctor-inlining.mm @@ -307,3 +307,196 @@ namespace PODUninitialized { } } } + +namespace ArrayMembers { + struct Primitive { + int values[3]; + }; + + void testPrimitive() { + Primitive a = { { 1, 2, 3 } }; + + clang_analyzer_eval(a.values[0] == 1); // expected-warning{{TRUE}} + clang_analyzer_eval(a.values[1] == 2); // expected-warning{{TRUE}} + clang_analyzer_eval(a.values[2] == 3); // expected-warning{{TRUE}} + + Primitive b = a; + + clang_analyzer_eval(b.values[0] == 1); // expected-warning{{TRUE}} + clang_analyzer_eval(b.values[1] == 2); // expected-warning{{TRUE}} + clang_analyzer_eval(b.values[2] == 3); // expected-warning{{TRUE}} + + Primitive c; + c = b; + + clang_analyzer_eval(c.values[0] == 1); // expected-warning{{TRUE}} + clang_analyzer_eval(c.values[1] == 2); // expected-warning{{TRUE}} + clang_analyzer_eval(c.values[2] == 3); // expected-warning{{TRUE}} + } + + struct NestedPrimitive { + int values[2][3]; + }; + + void testNestedPrimitive() { + NestedPrimitive a = { { { 0, 0, 0 }, { 1, 2, 3 } } }; + + clang_analyzer_eval(a.values[1][0] == 1); // expected-warning{{TRUE}} + clang_analyzer_eval(a.values[1][1] == 2); // expected-warning{{TRUE}} + clang_analyzer_eval(a.values[1][2] == 3); // expected-warning{{TRUE}} + + NestedPrimitive b = a; + + clang_analyzer_eval(b.values[1][0] == 1); // expected-warning{{TRUE}} + clang_analyzer_eval(b.values[1][1] == 2); // expected-warning{{TRUE}} + clang_analyzer_eval(b.values[1][2] == 3); // expected-warning{{TRUE}} + + NestedPrimitive c; + c = b; + + clang_analyzer_eval(c.values[1][0] == 1); // expected-warning{{TRUE}} + clang_analyzer_eval(c.values[1][1] == 2); // expected-warning{{TRUE}} + clang_analyzer_eval(c.values[1][2] == 3); // expected-warning{{TRUE}} + } + + struct POD { + IntWrapper values[3]; + }; + + void testPOD() { + POD a = { { { 1 }, { 2 }, { 3 } } }; + + clang_analyzer_eval(a.values[0].x == 1); // expected-warning{{TRUE}} + clang_analyzer_eval(a.values[1].x == 2); // expected-warning{{TRUE}} + clang_analyzer_eval(a.values[2].x == 3); // expected-warning{{TRUE}} + + POD b = a; + + clang_analyzer_eval(b.values[0].x == 1); // expected-warning{{TRUE}} + clang_analyzer_eval(b.values[1].x == 2); // expected-warning{{TRUE}} + clang_analyzer_eval(b.values[2].x == 3); // expected-warning{{TRUE}} + + POD c; + c = b; + + clang_analyzer_eval(c.values[0].x == 1); // expected-warning{{TRUE}} + clang_analyzer_eval(c.values[1].x == 2); // expected-warning{{TRUE}} + clang_analyzer_eval(c.values[2].x == 3); // expected-warning{{TRUE}} + } + + struct NestedPOD { + IntWrapper values[2][3]; + }; + + void testNestedPOD() { + NestedPOD a = { { { { 0 }, { 0 }, { 0 } }, { { 1 }, { 2 }, { 3 } } } }; + + clang_analyzer_eval(a.values[1][0].x == 1); // expected-warning{{TRUE}} + clang_analyzer_eval(a.values[1][1].x == 2); // expected-warning{{TRUE}} + clang_analyzer_eval(a.values[1][2].x == 3); // expected-warning{{TRUE}} + + NestedPOD b = a; + + clang_analyzer_eval(b.values[1][0].x == 1); // expected-warning{{TRUE}} + clang_analyzer_eval(b.values[1][1].x == 2); // expected-warning{{TRUE}} + clang_analyzer_eval(b.values[1][2].x == 3); // expected-warning{{TRUE}} + + NestedPOD c; + c = b; + + clang_analyzer_eval(c.values[1][0].x == 1); // expected-warning{{TRUE}} + clang_analyzer_eval(c.values[1][1].x == 2); // expected-warning{{TRUE}} + clang_analyzer_eval(c.values[1][2].x == 3); // expected-warning{{TRUE}} + } + + struct NonPOD { + NonPODIntWrapper values[3]; + }; + + void testNonPOD() { + NonPOD a; + a.values[0].x = 1; + a.values[1].x = 2; + a.values[2].x = 3; + + clang_analyzer_eval(a.values[0].x == 1); // expected-warning{{TRUE}} + clang_analyzer_eval(a.values[1].x == 2); // expected-warning{{TRUE}} + clang_analyzer_eval(a.values[2].x == 3); // expected-warning{{TRUE}} + + NonPOD b = a; + + clang_analyzer_eval(b.values[0].x == 1); // expected-warning{{UNKNOWN}} + clang_analyzer_eval(b.values[1].x == 2); // expected-warning{{UNKNOWN}} + clang_analyzer_eval(b.values[2].x == 3); // expected-warning{{UNKNOWN}} + + NonPOD c; + c = b; + + clang_analyzer_eval(c.values[0].x == 1); // expected-warning{{UNKNOWN}} + clang_analyzer_eval(c.values[1].x == 2); // expected-warning{{UNKNOWN}} + clang_analyzer_eval(c.values[2].x == 3); // expected-warning{{UNKNOWN}} + } + + struct NestedNonPOD { + NonPODIntWrapper values[2][3]; + }; + + void testNestedNonPOD() { + NestedNonPOD a; + a.values[0][0].x = 0; + a.values[0][1].x = 0; + a.values[0][2].x = 0; + a.values[1][0].x = 1; + a.values[1][1].x = 2; + a.values[1][2].x = 3; + + clang_analyzer_eval(a.values[1][0].x == 1); // expected-warning{{TRUE}} + clang_analyzer_eval(a.values[1][1].x == 2); // expected-warning{{TRUE}} + clang_analyzer_eval(a.values[1][2].x == 3); // expected-warning{{TRUE}} + + NestedNonPOD b = a; + + clang_analyzer_eval(b.values[1][0].x == 1); // expected-warning{{UNKNOWN}} + clang_analyzer_eval(b.values[1][1].x == 2); // expected-warning{{UNKNOWN}} + clang_analyzer_eval(b.values[1][2].x == 3); // expected-warning{{UNKNOWN}} + + NestedNonPOD c; + c = b; + + clang_analyzer_eval(c.values[1][0].x == 1); // expected-warning{{UNKNOWN}} + clang_analyzer_eval(c.values[1][1].x == 2); // expected-warning{{UNKNOWN}} + clang_analyzer_eval(c.values[1][2].x == 3); // expected-warning{{UNKNOWN}} + } + + struct NonPODDefaulted { + NonPODIntWrapper values[3]; + + NonPODDefaulted() = default; + NonPODDefaulted(const NonPODDefaulted &) = default; + NonPODDefaulted &operator=(const NonPODDefaulted &) = default; + }; + + void testNonPODDefaulted() { + NonPODDefaulted a; + a.values[0].x = 1; + a.values[1].x = 2; + a.values[2].x = 3; + + clang_analyzer_eval(a.values[0].x == 1); // expected-warning{{TRUE}} + clang_analyzer_eval(a.values[1].x == 2); // expected-warning{{TRUE}} + clang_analyzer_eval(a.values[2].x == 3); // expected-warning{{TRUE}} + + NonPODDefaulted b = a; + + clang_analyzer_eval(b.values[0].x == 1); // expected-warning{{UNKNOWN}} + clang_analyzer_eval(b.values[1].x == 2); // expected-warning{{UNKNOWN}} + clang_analyzer_eval(b.values[2].x == 3); // expected-warning{{UNKNOWN}} + + NonPODDefaulted c; + c = b; + + clang_analyzer_eval(c.values[0].x == 1); // expected-warning{{UNKNOWN}} + clang_analyzer_eval(c.values[1].x == 2); // expected-warning{{UNKNOWN}} + clang_analyzer_eval(c.values[2].x == 3); // expected-warning{{UNKNOWN}} + } +}; -- 2.49.0