diff --git a/clang/lib/StaticAnalyzer/Checkers/UndefinedArraySubscriptChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/UndefinedArraySubscriptChecker.cpp index be3a34f3ea17..176ee480826c 100644 --- a/clang/lib/StaticAnalyzer/Checkers/UndefinedArraySubscriptChecker.cpp +++ b/clang/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")); + const Expr *Index = A->getIdx(); + if (!C.getSVal(Index).isUndef()) + return; - // 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); - } - } + // 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<CXXConstructorDecl>(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/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index 789abb7d863f..ffc382fcf077 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/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<CXXConstructExpr>(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<ArraySubscriptExpr>(Init))) + Init = ASE->getBase()->IgnoreImplicit(); + SVal LValue = State->getSVal(Init, stackFrame); + if (Optional<Loc> LValueLoc = LValue.getAs<Loc>()) + 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/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp index b0f2579cd9b7..ed90dc589181 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp +++ b/clang/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<DeclStmt>(StmtElem->getStmt())) { if (const VarDecl *Var = dyn_cast<VarDecl>(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<ElementRegion>(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/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp index ee603d728e4e..f01e4e764014 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp @@ -605,6 +605,8 @@ static CallInlinePolicy mayInlineCallKind(const CallEvent &Call, const CXXConstructorCall &Ctor = cast<CXXConstructorCall>(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<ElementRegion>(Target)) return CIP_DisallowedOnce; diff --git a/clang/test/Analysis/ctor-inlining.mm b/clang/test/Analysis/ctor-inlining.mm index fd8caf324aa3..8cdb005968c3 100644 --- a/clang/test/Analysis/ctor-inlining.mm +++ b/clang/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}} + } +};