Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h =================================================================== --- clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h +++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h @@ -617,11 +617,16 @@ return svalBuilder.evalBinOp(ST, Op, LHS, RHS, T); } - /// Retreives which element is being constructed in a non POD type array. + /// Retreives which element is being constructed in a non-POD type array. static Optional getIndexOfElementToConstruct(ProgramStateRef State, const CXXConstructExpr *E, const LocationContext *LCtx); + /// Retreives which element is being destructed in a non-POD type array. + static Optional + getPendingArrayDestruction(ProgramStateRef State, + const LocationContext *LCtx); + /// Retreives the size of the array in the pending ArrayInitLoopExpr. static Optional getPendingInitLoop(ProgramStateRef State, const CXXConstructExpr *E, @@ -825,6 +830,20 @@ const CXXConstructExpr *CE, const LocationContext *LCtx); + /// Checks whether our policies allow us to inline a non-POD type array + /// destruction. Either specify Type or Size, but never both. If Type is + /// specified it will be used to decide about inlining, and in this case + /// Size should never be specified. If Size is specified Type must be + /// a nullptr. + /// \param Type The type of the array. + /// \param Size The size of the array. + bool shouldInlineArrayDestruction(const ArrayType *Type, + const Optional Size = None); + + std::pair prepareStateForArrayDestruction( + const ProgramStateRef State, const MemRegion *Region, + const QualType &ElementTy, const LocationContext *LCtx); + /// Checks whether we construct an array of non-POD type, and decides if the /// constructor should be inkoved once again. bool shouldRepeatCtorCall(ProgramStateRef State, const CXXConstructExpr *E, @@ -923,6 +942,16 @@ const CXXConstructExpr *E, const LocationContext *LCtx); + /// Assuming we destruct an array of non-POD types, this method allows us + /// to store which element is to be destructed next. + static ProgramStateRef setPendingArrayDestruction(ProgramStateRef State, + const LocationContext *LCtx, + unsigned Idx); + + static ProgramStateRef + removePendingArrayDestruction(ProgramStateRef State, + const LocationContext *LCtx); + /// Sets the size of the array in a pending ArrayInitLoopExpr. static ProgramStateRef setPendingInitLoop(ProgramStateRef State, const CXXConstructExpr *E, Index: clang/lib/Analysis/CFG.cpp =================================================================== --- clang/lib/Analysis/CFG.cpp +++ clang/lib/Analysis/CFG.cpp @@ -5275,8 +5275,19 @@ const CXXTemporary *temp = bindExpr->getTemporary(); return temp->getDestructor(); } + case CFGElement::MemberDtor: { + const FieldDecl *field = castAs().getFieldDecl(); + QualType ty = field->getType(); + + while (const ArrayType *arrayType = astContext.getAsArrayType(ty)) { + ty = arrayType->getElementType(); + } + + const CXXRecordDecl *classDecl = ty->getAsCXXRecordDecl(); + assert(classDecl); + return classDecl->getDestructor(); + } case CFGElement::BaseDtor: - case CFGElement::MemberDtor: // Not yet supported. return nullptr; } Index: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -48,6 +48,7 @@ #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" #include "clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h" #include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h" #include "clang/StaticAnalyzer/Core/PathSensitive/LoopUnrolling.h" #include "clang/StaticAnalyzer/Core/PathSensitive/LoopWidening.h" @@ -204,6 +205,12 @@ std::pair, unsigned> PendingInitLoopMap; REGISTER_TRAIT_WITH_PROGRAMSTATE(PendingInitLoop, PendingInitLoopMap) + +typedef llvm::ImmutableMap + PendingArrayDestructionMap; +REGISTER_TRAIT_WITH_PROGRAMSTATE(PendingArrayDestruction, + PendingArrayDestructionMap) + //===----------------------------------------------------------------------===// // Engine construction and deletion. //===----------------------------------------------------------------------===// @@ -517,6 +524,35 @@ return State->remove(Key); } +Optional +ExprEngine::getPendingArrayDestruction(ProgramStateRef State, + const LocationContext *LCtx) { + assert(LCtx && "LocationContext shouldn't be null!"); + + return Optional::create( + State->get(LCtx->getStackFrame())); +} + +ProgramStateRef ExprEngine::setPendingArrayDestruction( + ProgramStateRef State, const LocationContext *LCtx, unsigned Idx) { + assert(LCtx && "LocationContext shouldn't be null!"); + + auto Key = LCtx->getStackFrame(); + + return State->set(Key, Idx); +} + +ProgramStateRef +ExprEngine::removePendingArrayDestruction(ProgramStateRef State, + const LocationContext *LCtx) { + assert(LCtx && "LocationContext shouldn't be null!"); + + auto Key = LCtx->getStackFrame(); + + assert(LCtx && State->contains(Key)); + return State->remove(Key); +} + ProgramStateRef ExprEngine::addObjectUnderConstruction(ProgramStateRef State, const ConstructionContextItem &Item, @@ -1071,6 +1107,22 @@ Engine.enqueue(Dst, currBldrCtx->getBlock(), currStmtIdx); } +std::pair +ExprEngine::prepareStateForArrayDestruction(const ProgramStateRef State, + const MemRegion *Region, + const QualType &ElementTy, + const LocationContext *LCtx) { + auto ElementCountVal = + getDynamicElementCount(State, Region, svalBuilder, ElementTy); + + auto Idx = + getPendingArrayDestruction(State, LCtx) + .value_or(ElementCountVal.getAsInteger()->getLimitedValue() - 1); + + // Note: the destructors are called in reverse order. + return {setPendingArrayDestruction(State, LCtx, Idx - 1), Idx}; +} + void ExprEngine::ProcessImplicitDtor(const CFGImplicitDtor D, ExplodedNode *Pred) { ExplodedNodeSet Dst; @@ -1124,7 +1176,9 @@ QualType varType = varDecl->getType(); ProgramStateRef state = Pred->getState(); - SVal dest = state->getLValue(varDecl, Pred->getLocationContext()); + const LocationContext *LCtx = Pred->getLocationContext(); + + SVal dest = state->getLValue(varDecl, LCtx); const MemRegion *Region = dest.castAs().getRegion(); if (varType->isReferenceType()) { @@ -1140,14 +1194,22 @@ varType = cast(Region)->getValueType(); } - // 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). + unsigned Idx = 0; + if (const auto *AT = dyn_cast(varType)) + std::tie(state, Idx) = prepareStateForArrayDestruction( + state, Region, AT->getElementType(), LCtx); + EvalCallOptions CallOpts; Region = makeElementRegion(state, loc::MemRegionVal(Region), varType, - CallOpts.IsArrayCtorOrDtor) + CallOpts.IsArrayCtorOrDtor, Idx) .getAsRegion(); + NodeBuilder Bldr(Pred, Dst, getBuilderContext()); + + EpsilonPoint EP(LCtx, nullptr); + Pred = Bldr.generateNode(EP, state, Pred); + Bldr.takeNodes(Pred); + VisitCXXDestructor(varType, Region, Dtor.getTriggerStmt(), /*IsBase=*/false, Pred, Dst, CallOpts); } @@ -1175,18 +1237,27 @@ return; } + unsigned Idx = 0; EvalCallOptions CallOpts; const MemRegion *ArgR = ArgVal.getAsRegion(); + if (DE->isArrayForm()) { - // 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). + std::tie(State, Idx) = + prepareStateForArrayDestruction(State, ArgR, DTy, LCtx); + CallOpts.IsArrayCtorOrDtor = true; // Yes, it may even be a multi-dimensional array. while (const auto *AT = getContext().getAsArrayType(DTy)) DTy = AT->getElementType(); if (ArgR) - ArgR = getStoreManager().GetElementZeroRegion(cast(ArgR), DTy); + ArgR = MRMgr.getElementRegion(DTy, svalBuilder.makeArrayIndex(Idx), + cast(ArgR), getContext()); + + NodeBuilder Bldr(Pred, Dst, getBuilderContext()); + + EpsilonPoint EP(LCtx, nullptr); + Pred = Bldr.generateNode(EP, State, Pred); + Bldr.takeNodes(Pred); } VisitCXXDestructor(DTy, ArgR, DE, /*IsBase=*/false, Pred, Dst, CallOpts); @@ -1225,11 +1296,33 @@ Loc ThisLoc = State->getSVal(ThisStorageLoc).castAs(); SVal FieldVal = State->getLValue(Member, ThisLoc); - // 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). + unsigned Idx = 0; + if (const auto *AT = dyn_cast(T)) + std::tie(State, Idx) = prepareStateForArrayDestruction( + State, FieldVal.getAsRegion(), AT->getElementType(), LCtx); + EvalCallOptions CallOpts; - FieldVal = makeElementRegion(State, FieldVal, T, CallOpts.IsArrayCtorOrDtor); + FieldVal = + makeElementRegion(State, FieldVal, T, CallOpts.IsArrayCtorOrDtor, Idx); + + NodeBuilder Bldr(Pred, Dst, getBuilderContext()); + + // Use the location of the member here, otherwhise if the record contains + // the same array twice we are going to get a node that already exists and + // a loop in the egraph will be created, which leads to a crash. + // + // E.g.: + // struct MultipleMemberDtor + // { + // InlineDtor arr[4]; + // InlineDtor arr2[4]; + // }; + // + // After we finish the destruction of 'arr' we are going to create the exact + // same node that was created before it. + PreImplicitCall EP(CurDtor, Member->getLocation(), LCtx); + Pred = Bldr.generateNode(EP, State, Pred); + Bldr.takeNodes(Pred); VisitCXXDestructor(T, FieldVal.getAsRegion(), CurDtor->getBody(), /*IsBase=*/false, Pred, Dst, CallOpts); @@ -1280,15 +1373,15 @@ EvalCallOptions CallOpts; CallOpts.IsTemporaryCtorOrDtor = true; if (!MR) { - // If we have no MR, we still need to unwrap the array to avoid destroying - // the whole array at once. Regardless, we'd eventually need to model array - // destructors properly, element-by-element. + // FIXME: If we have no MR, we still need to unwrap the array to avoid + // destroying the whole array at once. Regardless, we'd eventually need to + // model array destructors properly, element-by-element. while (const ArrayType *AT = getContext().getAsArrayType(T)) { T = AT->getElementType(); CallOpts.IsArrayCtorOrDtor = true; } } else { - // We'd eventually need to makeElementRegion() trick here, + // FIXME: We'd eventually need to makeElementRegion() trick here, // but for now we don't have the respective construction contexts, // so MR would always be null in this case. Do nothing for now. } Index: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp +++ clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp @@ -171,8 +171,6 @@ if (const SubRegion *MR = dyn_cast_or_null(V.getAsRegion())) { if (NE->isArray()) { - // TODO: In fact, we need to call the constructor for every - // allocated element, not just the first one! CallOpts.IsArrayCtorOrDtor = true; auto R = MRMgr.getElementRegion(NE->getType()->getPointeeType(), Index: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp +++ clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp @@ -195,6 +195,28 @@ return RuntimeCallee->getCanonicalDecl() != StaticDecl->getCanonicalDecl(); } +static unsigned getElementCountOfArrayBeingDestructed( + const CallEvent &Call, const ProgramStateRef State, SValBuilder &SVB) { + assert(isa(Call) && + "The call event is not a destructor call!"); + + const auto &DtorCall = cast(Call); + + auto ThisVal = DtorCall.getCXXThisVal(); + + if (auto ThisElementRegion = dyn_cast(ThisVal.getAsRegion())) { + auto ArrayRegion = ThisElementRegion->getAsArrayOffset().getRegion(); + auto ElementType = ThisElementRegion->getElementType(); + + auto ElementCount = + getDynamicElementCount(State, ArrayRegion, SVB, ElementType); + + return ElementCount.getAsInteger()->getLimitedValue(); + } + + return -1; +} + /// The call exit is simulated with a sequence of nodes, which occur between /// CallExitBegin and CallExitEnd. The following operations occur between the /// two program points: @@ -234,6 +256,22 @@ // but we want to evaluate it as many times as many elements the array has. bool ShouldRepeatCall = false; + if (const auto *DtorDecl = + dyn_cast_or_null(Call->getDecl())) { + if (auto Idx = getPendingArrayDestruction(state, callerCtx)) { + + auto Size = + getElementCountOfArrayBeingDestructed(*Call, state, svalBuilder); + ShouldRepeatCall = Idx < Size; + + auto ThisVal = svalBuilder.getCXXThis(DtorDecl->getParent(), calleeCtx); + state = state->killBinding(ThisVal); + + if (!ShouldRepeatCall) + state = removePendingArrayDestruction(state, callerCtx); + } + } + // If the callee returns an expression, bind its value to CallExpr. if (CE) { if (const ReturnStmt *RS = dyn_cast_or_null(LastSt)) { @@ -818,11 +856,6 @@ !Opts.MayInlineCXXAllocator) return CIP_DisallowedOnce; - // 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. - // We still allow construction into ElementRegion targets when they don't - // represent array elements. if (CallOpts.IsArrayCtorOrDtor) { if (!shouldInlineArrayConstruction(Pred->getState(), CtorExpr, CurLC)) return CIP_DisallowedOnce; @@ -877,9 +910,13 @@ assert(ADC->getCFGBuildOptions().AddImplicitDtors && "No CFG destructors"); (void)ADC; - // FIXME: We don't handle destructors for arrays properly. - if (CallOpts.IsArrayCtorOrDtor) - return CIP_DisallowedOnce; + if (CallOpts.IsArrayCtorOrDtor) { + if (!shouldInlineArrayDestruction( + nullptr, getElementCountOfArrayBeingDestructed( + Call, Pred->getState(), svalBuilder))) { + return CIP_DisallowedOnce; + } + } // Allow disabling temporary destructor inlining with a separate option. if (CallOpts.IsTemporaryCtorOrDtor && @@ -1096,19 +1133,44 @@ if (!CE) return false; - auto Type = CE->getType(); + // This might seem conter-intuitive at first glance, but the functions are + // closely related. Reasoning about destructors depends only on the type + // of the expression that initialized the memory region, which is the + // CXXConstructExpr. So to avoid code repetition, the work is delegated + // to the function that reasons about destructor inlining. Also note that + // if the constructors of the array elements are inlined, the destructors + // can also be inlined and if the destructors can be inline, it's safe to + // inline the constructors. + if (shouldInlineArrayDestruction(dyn_cast(CE->getType()))) + return true; + + // 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; +} + +bool ExprEngine::shouldInlineArrayDestruction(const ArrayType *Type, + const Optional Size) { + + uint64_t maxAllowedSize = AMgr.options.maxBlockVisitOnPath; + + if (!Type && !Size) + return false; + + if (Size) { + assert(!Type && "If Size is specified, Type must be a nullptr!"); + return *Size <= maxAllowedSize; + } // FIXME: Handle other arrays types. if (const auto *CAT = dyn_cast(Type)) { - unsigned Size = getContext().getConstantArrayElementCount(CAT); + unsigned ArrSize = getContext().getConstantArrayElementCount(CAT); - return Size <= AMgr.options.maxBlockVisitOnPath; + return ArrSize <= maxAllowedSize; } - // 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; } Index: clang/lib/StaticAnalyzer/Core/ProgramState.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/ProgramState.cpp +++ clang/lib/StaticAnalyzer/Core/ProgramState.cpp @@ -216,8 +216,6 @@ } ProgramStateRef ProgramState::killBinding(Loc LV) const { - assert(!isa(LV) && "Use invalidateRegion instead."); - Store OldStore = getStore(); const StoreRef &newStore = getStateManager().StoreMgr->killBinding(OldStore, LV); Index: clang/test/Analysis/dtor-array.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/dtor-array.cpp @@ -0,0 +1,149 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-inlining=destructors -verify -std=c++11 %s +// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-inlining=destructors -verify -std=c++17 %s + +void clang_analyzer_eval(bool); +void clang_analyzer_checkInlined(bool); +void clang_analyzer_warnIfReached(); + +int a, b, c, d; + +struct InlineDtor { + static int cnt; + static int dtorCalled; + ~InlineDtor() { + switch (dtorCalled % 4) { + case 0: + a = cnt++; + break; + case 1: + b = cnt++; + break; + case 2: + c = cnt++; + break; + case 3: + d = cnt++; + break; + } + + ++dtorCalled; + } +}; + +int InlineDtor::cnt = 0; +int InlineDtor::dtorCalled = 0; + +void foo() { + InlineDtor::cnt = 0; + InlineDtor::dtorCalled = 0; + InlineDtor arr[4]; +} + +void testAutoDtor() { + foo(); + + clang_analyzer_eval(a == 0); // expected-warning {{TRUE}} + clang_analyzer_eval(b == 1); // expected-warning {{TRUE}} + clang_analyzer_eval(c == 2); // expected-warning {{TRUE}} + clang_analyzer_eval(d == 3); // expected-warning {{TRUE}} +} + +void testDeleteDtor() { + InlineDtor::cnt = 10; + InlineDtor::dtorCalled = 0; + + InlineDtor *arr = new InlineDtor[4]; + delete[] arr; + + clang_analyzer_eval(a == 10); // expected-warning {{TRUE}} + clang_analyzer_eval(b == 11); // expected-warning {{TRUE}} + clang_analyzer_eval(c == 12); // expected-warning {{TRUE}} + clang_analyzer_eval(d == 13); // expected-warning {{TRUE}} +} + +struct MemberDtor { + InlineDtor arr[4]; +}; + +void testMemberDtor() { + InlineDtor::cnt = 5; + InlineDtor::dtorCalled = 0; + + MemberDtor *MD = new MemberDtor{}; + delete MD; + + clang_analyzer_eval(a == 5); // expected-warning {{TRUE}} + clang_analyzer_eval(b == 6); // expected-warning {{TRUE}} + clang_analyzer_eval(c == 7); // expected-warning {{TRUE}} + clang_analyzer_eval(d == 8); // expected-warning {{TRUE}} +} + +struct MultipleMemberDtor +{ + InlineDtor arr[4]; + InlineDtor arr2[4]; +}; + +void testMultipleMemberDtor() { + InlineDtor::cnt = 30; + InlineDtor::dtorCalled = 0; + + MultipleMemberDtor *MD = new MultipleMemberDtor{}; + delete MD; + + clang_analyzer_eval(a == 34); // expected-warning {{TRUE}} + clang_analyzer_eval(b == 35); // expected-warning {{TRUE}} + clang_analyzer_eval(c == 36); // expected-warning {{TRUE}} + clang_analyzer_eval(d == 37); // expected-warning {{TRUE}} +} + +int EvalOrderArr[4]; + +struct EvalOrder +{ + int ctor = 0; + static int dtorCalled; + static int ctorCalled; + + EvalOrder() { ctor = ctorCalled++; }; + + ~EvalOrder() { EvalOrderArr[ctor] = dtorCalled++; } +}; + +int EvalOrder::ctorCalled = 0; +int EvalOrder::dtorCalled = 0; + +void dtorEvaluationOrder() { + EvalOrder::ctorCalled = 0; + EvalOrder::dtorCalled = 0; + + EvalOrder* eptr = new EvalOrder[4]; + delete[] eptr; + + clang_analyzer_eval(EvalOrder::dtorCalled == 4); // expected-warning {{TRUE}} + clang_analyzer_eval(EvalOrder::dtorCalled == EvalOrder::ctorCalled); // expected-warning {{TRUE}} + + clang_analyzer_eval(EvalOrderArr[0] == 3); // expected-warning {{TRUE}} + clang_analyzer_eval(EvalOrderArr[1] == 2); // expected-warning {{TRUE}} + clang_analyzer_eval(EvalOrderArr[2] == 1); // expected-warning {{TRUE}} + clang_analyzer_eval(EvalOrderArr[3] == 0); // expected-warning {{TRUE}} +} + +struct EmptyDtor { + ~EmptyDtor(){}; +}; + +struct DefaultDtor { + ~DefaultDtor() = default; +}; + +// This function used to fail on an assertion. +void no_crash() { + EmptyDtor* eptr = new EmptyDtor[4]; + delete[] eptr; + clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}} + + DefaultDtor* dptr = new DefaultDtor[4]; + delete[] dptr; + clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}} +} Index: clang/test/Analysis/new.cpp =================================================================== --- clang/test/Analysis/new.cpp +++ clang/test/Analysis/new.cpp @@ -3,6 +3,7 @@ #include "Inputs/system-header-simulator-cxx.h" void clang_analyzer_eval(bool); +void clang_analyzer_warnIfReached(); typedef __typeof__(sizeof(int)) size_t; extern "C" void *malloc(size_t); @@ -323,9 +324,8 @@ void testArrayDestr() { NoReturnDtor *p = new NoReturnDtor[2]; - delete[] p; // Calls the base destructor which aborts, checked below - // TODO: clang_analyzer_eval should not be called - clang_analyzer_eval(true); // expected-warning{{TRUE}} + delete[] p; + clang_analyzer_warnIfReached(); // no-warning } // Invalidate Region even in case of default destructor