diff --git a/clang/include/clang/Analysis/ConstructionContext.h b/clang/include/clang/Analysis/ConstructionContext.h --- a/clang/include/clang/Analysis/ConstructionContext.h +++ b/clang/include/clang/Analysis/ConstructionContext.h @@ -298,6 +298,11 @@ const ConstructionContextLayer *TopLayer); Kind getKind() const { return K; } + + virtual const ArrayInitLoopExpr *getArrayInitLoop() const { return nullptr; } + + // Only declared to silence -Wnon-virtual-dtor warnings. + virtual ~ConstructionContext() = default; }; /// An abstract base class for local variable constructors. @@ -314,6 +319,12 @@ public: const DeclStmt *getDeclStmt() const { return DS; } + const ArrayInitLoopExpr *getArrayInitLoop() const override { + const auto *Var = cast(DS->getSingleDecl()); + + return dyn_cast(Var->getInit()); + } + static bool classof(const ConstructionContext *CC) { return CC->getKind() >= VARIABLE_BEGIN && CC->getKind() <= VARIABLE_END; @@ -381,6 +392,10 @@ public: const CXXCtorInitializer *getCXXCtorInitializer() const { return I; } + const ArrayInitLoopExpr *getArrayInitLoop() const override { + return dyn_cast(I->getInit()); + } + static bool classof(const ConstructionContext *CC) { return CC->getKind() >= INITIALIZER_BEGIN && CC->getKind() <= INITIALIZER_END; diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h @@ -622,6 +622,11 @@ getIndexOfElementToConstruct(ProgramStateRef State, const CXXConstructExpr *E, const LocationContext *LCtx); + /// Retreives the size of the array in the pending ArrayInitLoopExpr. + static Optional getPendingInitLoop(ProgramStateRef State, + const CXXConstructExpr *E, + const LocationContext *LCtx); + /// By looking at a certain item that may be potentially part of an object's /// ConstructionContext, retrieve such object's location. A particular /// statement can be transparently passed as \p Item in most cases. @@ -816,7 +821,9 @@ /// Checks whether our policies allow us to inline a non-POD type array /// construction. - bool shouldInlineArrayConstruction(const ArrayType *Type); + bool shouldInlineArrayConstruction(const ProgramStateRef State, + const CXXConstructExpr *CE, + const LocationContext *LCtx); /// Checks whether we construct an array of non-POD type, and decides if the /// constructor should be inkoved once again. @@ -916,6 +923,16 @@ const CXXConstructExpr *E, const LocationContext *LCtx); + /// Sets the size of the array in a pending ArrayInitLoopExpr. + static ProgramStateRef setPendingInitLoop(ProgramStateRef State, + const CXXConstructExpr *E, + const LocationContext *LCtx, + unsigned Idx); + + static ProgramStateRef removePendingInitLoop(ProgramStateRef State, + const CXXConstructExpr *E, + const LocationContext *LCtx); + /// Store the location of a C++ object corresponding to a statement /// until the statement is actually encountered. For example, if a DeclStmt /// has CXXConstructExpr as its initializer, the object would be considered diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp --- a/clang/lib/Analysis/CFG.cpp +++ b/clang/lib/Analysis/CFG.cpp @@ -1659,9 +1659,13 @@ appendInitializer(Block, I); if (Init) { + // If the initializer is an ArrayInitLoopExpr, we want to extract the + // initializer, that's used for each element. + const auto *AILE = dyn_cast_or_null(Init); + findConstructionContexts( ConstructionContextLayer::create(cfg->getBumpVectorContext(), I), - Init); + AILE ? AILE->getSubExpr() : Init); if (HasTemporaries) { // For expression with temporaries go directly to subexpression to omit @@ -2931,9 +2935,13 @@ autoCreateBlock(); appendStmt(Block, DS); + // If the initializer is an ArrayInitLoopExpr, we want to extract the + // initializer, that's used for each element. + const auto *AILE = dyn_cast_or_null(Init); + findConstructionContexts( ConstructionContextLayer::create(cfg->getBumpVectorContext(), DS), - Init); + AILE ? AILE->getSubExpr() : Init); // Keep track of the last non-null block, as 'Block' can be nulled out // if the initializer expression is something like a 'while' in a diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -196,6 +196,14 @@ IndexOfElementToConstructMap; REGISTER_TRAIT_WITH_PROGRAMSTATE(IndexOfElementToConstruct, IndexOfElementToConstructMap) + +// This trait is responsible for holding our pending ArrayInitLoopExprs. +// It pairs the LocationContext and the initializer CXXConstructExpr with +// the size of the array that's being copy initialized. +typedef llvm::ImmutableMap< + std::pair, unsigned> + PendingInitLoopMap; +REGISTER_TRAIT_WITH_PROGRAMSTATE(PendingInitLoop, PendingInitLoopMap) //===----------------------------------------------------------------------===// // Engine construction and deletion. //===----------------------------------------------------------------------===// @@ -462,6 +470,34 @@ return State->set(Key, Idx); } +Optional ExprEngine::getPendingInitLoop(ProgramStateRef State, + const CXXConstructExpr *E, + const LocationContext *LCtx) { + + return Optional::create( + State->get({E, LCtx->getStackFrame()})); +} + +ProgramStateRef ExprEngine::removePendingInitLoop(ProgramStateRef State, + const CXXConstructExpr *E, + const LocationContext *LCtx) { + auto Key = std::make_pair(E, LCtx->getStackFrame()); + + assert(E && State->contains(Key)); + return State->remove(Key); +} + +ProgramStateRef ExprEngine::setPendingInitLoop(ProgramStateRef State, + const CXXConstructExpr *E, + const LocationContext *LCtx, + unsigned Size) { + auto Key = std::make_pair(E, LCtx->getStackFrame()); + + assert(!State->contains(Key) && Size > 0); + + return State->set(Key, Size); +} + Optional ExprEngine::getIndexOfElementToConstruct(ProgramStateRef State, const CXXConstructExpr *E, @@ -487,17 +523,22 @@ const LocationContext *LC, SVal V) { ConstructedObjectKey Key(Item, LC->getStackFrame()); - const CXXConstructExpr *E = nullptr; + const Expr *Init = nullptr; if (auto DS = dyn_cast_or_null(Item.getStmtOrNull())) { if (auto VD = dyn_cast_or_null(DS->getSingleDecl())) - E = dyn_cast(VD->getInit()); + Init = VD->getInit(); } - if (!E && !Item.getStmtOrNull()) { - auto CtorInit = Item.getCXXCtorInitializer(); - E = dyn_cast(CtorInit->getInit()); - } + if (!Init && !Item.getStmtOrNull()) + Init = Item.getCXXCtorInitializer()->getInit(); + + // In an ArrayInitLoopExpr the real initializer is returned by + // getSubExpr(). + if (const auto *AILE = dyn_cast_or_null(Init)) + Init = AILE->getSubExpr(); + + const auto *E = dyn_cast_or_null(Init); // FIXME: Currently the state might already contain the marker due to // incorrect handling of temporaries bound to default parameters. @@ -2797,6 +2838,11 @@ const Expr *Arr = Ex->getCommonExpr()->getSourceExpr(); for (auto *Node : CheckerPreStmt) { + + // The constructor visitior has already taken care of everything. + if (auto *CE = dyn_cast(Ex->getSubExpr())) + break; + const LocationContext *LCtx = Node->getLocationContext(); ProgramStateRef state = Node->getState(); diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp --- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp @@ -462,6 +462,59 @@ llvm_unreachable("Unhandled construction context!"); } +static ProgramStateRef +bindRequiredArrayElementToEnvironment(ProgramStateRef State, + const ArrayInitLoopExpr *AILE, + const LocationContext *LCtx, SVal Idx) { + // The ctor in this case is guaranteed to be a copy ctor, otherwise we hit a + // compile time error. + // + // -ArrayInitLoopExpr <-- we're here + // |-OpaqueValueExpr + // | `-DeclRefExpr <-- match this + // `-CXXConstructExpr + // `-ImplicitCastExpr + // `-ArraySubscriptExpr + // |-ImplicitCastExpr + // | `-OpaqueValueExpr + // | `-DeclRefExpr + // `-ArrayInitIndexExpr + // + // The resulting expression might look like the one below in an implicit + // copy/move ctor. + // + // ArrayInitLoopExpr <-- we're here + // |-OpaqueValueExpr + // | `-MemberExpr <-- match this + // | (`-CXXStaticCastExpr) <-- move ctor only + // | `-DeclRefExpr + // `-CXXConstructExpr + // `-ArraySubscriptExpr + // |-ImplicitCastExpr + // | `-OpaqueValueExpr + // | `-MemberExpr + // | `-DeclRefExpr + // `-ArrayInitIndexExpr + // + // HACK: There is no way we can put the index of the array element into the + // CFG unless we unroll the loop, so we manually select and bind the required + // parameter to the environment. + const auto *CE = cast(AILE->getSubExpr()); + const auto *OVESrc = AILE->getCommonExpr()->getSourceExpr(); + + SVal Base = UnknownVal(); + if (const auto *ME = dyn_cast(OVESrc)) + Base = State->getSVal(ME, LCtx); + else if (const auto *DRE = cast(OVESrc)) + Base = State->getLValue(cast(DRE->getDecl()), LCtx); + else + llvm_unreachable("ArrayInitLoopExpr contains unexpected source expression"); + + SVal NthElem = State->getLValue(CE->getType(), Idx, Base); + + return State->BindExpr(CE->getArg(0), LCtx, NthElem); +} + void ExprEngine::handleConstructor(const Expr *E, ExplodedNode *Pred, ExplodedNodeSet &destNodes) { @@ -502,12 +555,26 @@ // Inherited constructors are always base class constructors. assert(CE && !CIE && "A complete constructor is inherited?!"); + // If the ctor is part of an ArrayInitLoopExpr, we want to handle it + // differently. + auto *AILE = CC ? CC->getArrayInitLoop() : nullptr; + unsigned Idx = 0; - if (CE->getType()->isArrayType()) { - Idx = getIndexOfElementToConstruct(State, CE, LCtx).value_or(0u); + if (CE->getType()->isArrayType() || AILE) { + Idx = getIndexOfElementToConstruct(State, CE, LCtx).getValueOr(0u); State = setIndexOfElementToConstruct(State, CE, LCtx, Idx + 1); } + if (AILE) { + // Only set this once even though we loop through it multiple times. + if (!getPendingInitLoop(State, CE, LCtx)) + State = setPendingInitLoop(State, CE, LCtx, + AILE->getArraySize().getLimitedValue()); + + State = bindRequiredArrayElementToEnvironment( + State, AILE, LCtx, svalBuilder.makeArrayIndex(Idx)); + } + // The target region is found from construction context. std::tie(State, Target) = handleConstructionContext(CE, State, LCtx, CC, CallOpts, Idx); diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp --- a/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp @@ -265,9 +265,13 @@ ShouldRepeatCall = shouldRepeatCtorCall(state, CCE, callerCtx); - if (!ShouldRepeatCall && - getIndexOfElementToConstruct(state, CCE, callerCtx)) - state = removeIndexOfElementToConstruct(state, CCE, callerCtx); + if (!ShouldRepeatCall) { + if (getIndexOfElementToConstruct(state, CCE, callerCtx)) + state = removeIndexOfElementToConstruct(state, CCE, callerCtx); + + if (getPendingInitLoop(state, CCE, callerCtx)) + state = removePendingInitLoop(state, CCE, callerCtx); + } } if (const auto *CNE = dyn_cast(CE)) { @@ -815,8 +819,7 @@ // We still allow construction into ElementRegion targets when they don't // represent array elements. if (CallOpts.IsArrayCtorOrDtor) { - if (!shouldInlineArrayConstruction( - dyn_cast(CtorExpr->getType()))) + if (!shouldInlineArrayConstruction(Pred->getState(), CtorExpr, CurLC)) return CIP_DisallowedOnce; } @@ -1082,10 +1085,14 @@ return true; } -bool ExprEngine::shouldInlineArrayConstruction(const ArrayType *Type) { - if (!Type) +bool ExprEngine::shouldInlineArrayConstruction(const ProgramStateRef State, + const CXXConstructExpr *CE, + const LocationContext *LCtx) { + if (!CE) return false; + auto Type = CE->getType(); + // FIXME: Handle other arrays types. if (const auto *CAT = dyn_cast(Type)) { unsigned Size = getContext().getConstantArrayElementCount(CAT); @@ -1093,6 +1100,10 @@ return Size <= AMgr.options.maxBlockVisitOnPath; } + // Check if we're inside an ArrayInitLoopExpr, and it's sufficiently small. + if (auto Size = getPendingInitLoop(State, CE, LCtx)) + return *Size <= AMgr.options.maxBlockVisitOnPath; + return false; } @@ -1111,6 +1122,9 @@ return Size > getIndexOfElementToConstruct(State, E, LCtx); } + if (auto Size = getPendingInitLoop(State, E, LCtx)) + return Size > getIndexOfElementToConstruct(State, E, LCtx); + return false; } diff --git a/clang/test/Analysis/array-init-loop.cpp b/clang/test/Analysis/array-init-loop.cpp --- a/clang/test/Analysis/array-init-loop.cpp +++ b/clang/test/Analysis/array-init-loop.cpp @@ -125,3 +125,97 @@ clang_analyzer_eval(moved.arr[3]); // expected-warning{{UNKNOWN}} clang_analyzer_eval(moved.arr[4]); // expected-warning{{UNKNOWN}} } + +// The struct has a user defined copy and move ctor, which allow us to +// track the values more precisely when an array of this struct is being +// copy/move initialized by ArrayInitLoopExpr. +struct S2 { + inline static int c = 0; + int i; + + S2() : i(++c) {} + + S2(const S2 ©) { + i = copy.i + 1; + } + + S2(S2 &&move) { + i = move.i + 2; + } +}; + +void array_init_non_pod() { + S2::c = 0; + S2 arr[4]; + + auto [a, b, c, d] = arr; + + clang_analyzer_eval(a.i == 2); // expected-warning{{TRUE}} + clang_analyzer_eval(b.i == 3); // expected-warning{{TRUE}} + clang_analyzer_eval(c.i == 4); // expected-warning{{TRUE}} + clang_analyzer_eval(d.i == 5); // expected-warning{{TRUE}} +} + +struct S3 { + int i; +}; + +void array_uninit_non_pod() { + S3 arr[1]; + + auto [a] = arr; // expected-warning@159{{ in implicit constructor is garbage or undefined }} +} + +void lambda_init_non_pod() { + S2::c = 0; + S2 arr[4]; + + // FIXME: These should be TRUE, but we fail to capture the array properly. + auto l = [arr] { return arr[0].i; }(); + clang_analyzer_eval(l == 2); // expected-warning{{TRUE}} // expected-warning{{FALSE}} + + l = [arr] { return arr[1].i; }(); + clang_analyzer_eval(l == 3); // expected-warning{{TRUE}} // expected-warning{{FALSE}} + + l = [arr] { return arr[2].i; }(); + clang_analyzer_eval(l == 4); // expected-warning{{TRUE}} // expected-warning{{FALSE}} + + l = [arr] { return arr[3].i; }(); + clang_analyzer_eval(l == 5); // expected-warning{{TRUE}} // expected-warning{{FALSE}} +} + +void lambda_uninit_non_pod() { + S3 arr[4]; + + int l = [arr] { return arr[3].i; }(); +} + +// If this struct is being copy/move constructed by the implicit ctors, ArrayInitLoopExpr +// is responsible for the initialization of 'arr' by copy/move constructing each of the +// elements. +struct S5 { + S2 arr[4]; +}; + +void copy_ctor_init_non_pod() { + S2::c = 0; + S5 orig; + + S5 copy = orig; + clang_analyzer_eval(copy.arr[0].i == 2); // expected-warning{{TRUE}} + clang_analyzer_eval(copy.arr[1].i == 3); // expected-warning{{TRUE}} + clang_analyzer_eval(copy.arr[2].i == 4); // expected-warning{{TRUE}} + clang_analyzer_eval(copy.arr[3].i == 5); // expected-warning{{TRUE}} +} + +void move_ctor_init_non_pod() { + S2::c = 0; + S5 orig; + + S5 moved = (S5 &&) orig; + + clang_analyzer_eval(moved.arr[0].i == 3); // expected-warning{{TRUE}} + clang_analyzer_eval(moved.arr[1].i == 4); // expected-warning{{TRUE}} + clang_analyzer_eval(moved.arr[2].i == 5); // expected-warning{{TRUE}} + clang_analyzer_eval(moved.arr[3].i == 6); // expected-warning{{TRUE}} +} diff --git a/clang/test/Analysis/ctor.mm b/clang/test/Analysis/ctor.mm --- a/clang/test/Analysis/ctor.mm +++ b/clang/test/Analysis/ctor.mm @@ -439,16 +439,16 @@ 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}} + 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}} 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}} + 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 NestedNonPOD { @@ -502,16 +502,16 @@ 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}} + 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}} 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}} + 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}} } }; diff --git a/clang/test/Analysis/uninit-structured-binding-array.cpp b/clang/test/Analysis/uninit-structured-binding-array.cpp --- a/clang/test/Analysis/uninit-structured-binding-array.cpp +++ b/clang/test/Analysis/uninit-structured-binding-array.cpp @@ -292,3 +292,97 @@ clang_analyzer_eval(e == 5); // expected-warning{{UNKNOWN}} clang_analyzer_eval(f == 6); // expected-warning{{UNKNOWN}} } + +struct S { + int a = 1; + int b = 2; +}; + +void non_pod_val(void) { + S arr[2]; + + auto [x, y] = arr; + + clang_analyzer_eval(x.a == 1); // expected-warning{{TRUE}} + clang_analyzer_eval(x.b == 2); // expected-warning{{TRUE}} + + clang_analyzer_eval(y.a == 1); // expected-warning{{TRUE}} + clang_analyzer_eval(y.b == 2); // expected-warning{{TRUE}} +} + +void non_pod_val_syntax_2(void) { + S arr[2]; + + auto [x, y](arr); + + clang_analyzer_eval(x.a == 1); // expected-warning{{TRUE}} + clang_analyzer_eval(x.b == 2); // expected-warning{{TRUE}} + + clang_analyzer_eval(y.a == 1); // expected-warning{{TRUE}} + clang_analyzer_eval(y.b == 2); // expected-warning{{TRUE}} +} + +void non_pod_lref(void) { + S arr[2]; + + auto &[x, y] = arr; + + clang_analyzer_eval(x.a == 1); // expected-warning{{TRUE}} + clang_analyzer_eval(x.b == 2); // expected-warning{{TRUE}} + + clang_analyzer_eval(y.a == 1); // expected-warning{{TRUE}} + clang_analyzer_eval(y.b == 2); // expected-warning{{TRUE}} +} + +void non_pod_rref(void) { + S arr[2]; + + auto &&[x, y] = arr; + + clang_analyzer_eval(x.a == 1); // expected-warning{{TRUE}} + clang_analyzer_eval(x.b == 2); // expected-warning{{TRUE}} + + clang_analyzer_eval(y.a == 1); // expected-warning{{TRUE}} + clang_analyzer_eval(y.b == 2); // expected-warning{{TRUE}} +} + +struct SUD { + inline static int c = 0; + + int a = 1; + int b = 2; + + SUD() { ++c; }; + + SUD(const SUD ©) { + a = copy.a + 1; + b = copy.b + 1; + } +}; + +void non_pod_user_defined_val(void) { + SUD arr[2]; + + auto [x, y] = arr; + + clang_analyzer_eval(x.a == 2); // expected-warning{{TRUE}} + clang_analyzer_eval(x.b == 3); // expected-warning{{TRUE}} + + clang_analyzer_eval(y.a == 2); // expected-warning{{TRUE}} + clang_analyzer_eval(y.b == 3); // expected-warning{{TRUE}} +} + +void non_pod_user_defined_val_syntax_2(void) { + SUD::c = 0; + SUD arr[2]; + + auto [x, y](arr); + + clang_analyzer_eval(SUD::c == 2); // expected-warning{{TRUE}} + + clang_analyzer_eval(x.a == 2); // expected-warning{{TRUE}} + clang_analyzer_eval(x.b == 3); // expected-warning{{TRUE}} + + clang_analyzer_eval(y.a == 2); // expected-warning{{TRUE}} + clang_analyzer_eval(y.b == 3); // expected-warning{{TRUE}} +}