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 @@ -120,6 +120,24 @@ EvalCallOptions() {} }; +struct PendingArrayDestructionData { + QualType type; + unsigned idx = 0; + bool shouldInline = false; + + void Profile(llvm::FoldingSetNodeID &ID) const { + type.Profile(ID); + ID.AddInteger(idx); + ID.AddBoolean(shouldInline); + } + + bool operator==(PendingArrayDestructionData R) const { + return type == R.type && idx == R.idx && shouldInline == R.shouldInline; + } + + bool operator!=(PendingArrayDestructionData R) const { return !(*this == R); } +}; + class ExprEngine { void anchor(); @@ -617,11 +635,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 CXXDestructorDecl *D, + const LocationContext *LCtx); + /// Retreives the size of the array in the pending ArrayInitLoopExpr. static Optional getPendingInitLoop(ProgramStateRef State, const CXXConstructExpr *E, @@ -825,6 +848,10 @@ const CXXConstructExpr *CE, const LocationContext *LCtx); + /// Checks whether our policies allow us to inline a non-POD type array + /// destruction. + bool shouldInlineArrayDestruction(const ArrayType *Type); + /// 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 +950,18 @@ 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 CXXDestructorDecl *D, + const LocationContext *LCtx, + const PendingArrayDestructionData &Data); + + static ProgramStateRef + removePendingArrayDestruction(ProgramStateRef State, + const CXXDestructorDecl *D, + const LocationContext *LCtx); + /// Sets the size of the array in a pending ArrayInitLoopExpr. static ProgramStateRef setPendingInitLoop(ProgramStateRef State, const CXXConstructExpr *E, Index: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -204,6 +204,14 @@ std::pair, unsigned> PendingInitLoopMap; REGISTER_TRAIT_WITH_PROGRAMSTATE(PendingInitLoop, PendingInitLoopMap) + +typedef llvm::ImmutableMap< + std::pair, + PendingArrayDestructionData> + PendingArrayDestructionMap; +REGISTER_TRAIT_WITH_PROGRAMSTATE(PendingArrayDestruction, + PendingArrayDestructionMap) + //===----------------------------------------------------------------------===// // Engine construction and deletion. //===----------------------------------------------------------------------===// @@ -470,6 +478,53 @@ return State->set(Key, Idx); } +Optional +ExprEngine::getIndexOfElementToConstruct(ProgramStateRef State, + const CXXConstructExpr *E, + const LocationContext *LCtx) { + + return Optional::create( + State->get({E, LCtx->getStackFrame()})); +} + +ProgramStateRef +ExprEngine::removeIndexOfElementToConstruct(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); +} + +Optional +ExprEngine::getPendingArrayDestruction(ProgramStateRef State, + const CXXDestructorDecl *D, + const LocationContext *LCtx) { + return Optional::create( + State->get({D, LCtx->getStackFrame()})); +} + +ProgramStateRef ExprEngine::setPendingArrayDestruction( + ProgramStateRef State, const CXXDestructorDecl *D, + const LocationContext *LCtx, const PendingArrayDestructionData &Data) { + auto Key = std::make_pair(D, LCtx->getStackFrame()); + + assert(!State->contains(Key) || Data.idx > 0); + + return State->set(Key, Data); +} + +ProgramStateRef +ExprEngine::removePendingArrayDestruction(ProgramStateRef State, + const CXXDestructorDecl *D, + const LocationContext *LCtx) { + auto Key = std::make_pair(D, LCtx->getStackFrame()); + + assert(D && State->contains(Key)); + return State->remove(Key); +} + Optional ExprEngine::getPendingInitLoop(ProgramStateRef State, const CXXConstructExpr *E, const LocationContext *LCtx) { @@ -498,25 +553,6 @@ return State->set(Key, Size); } -Optional -ExprEngine::getIndexOfElementToConstruct(ProgramStateRef State, - const CXXConstructExpr *E, - const LocationContext *LCtx) { - - return Optional::create( - State->get({E, LCtx->getStackFrame()})); -} - -ProgramStateRef -ExprEngine::removeIndexOfElementToConstruct(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::addObjectUnderConstruction(ProgramStateRef State, const ConstructionContextItem &Item, @@ -1125,7 +1161,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()) { @@ -1141,14 +1179,32 @@ 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). EvalCallOptions CallOpts; + + unsigned Idx = 0; + if (const auto *AT = dyn_cast(varType)) { + const auto DtorDecl = Dtor.getDestructorDecl(getContext()); + + auto data = getPendingArrayDestruction(state, DtorDecl, LCtx) + .getValueOr(PendingArrayDestructionData{}); + + Idx = data.idx++; + data.shouldInline = shouldInlineArrayDestruction(AT); + data.type = varType; + + state = setPendingArrayDestruction(state, DtorDecl, LCtx, data); + } + 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); } Index: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp +++ clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp @@ -234,6 +234,20 @@ // 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 Data = getPendingArrayDestruction(state, DtorDecl, callerCtx)) { + // FIXME: Handle non constant array types + if (const auto *CAT = dyn_cast(Data->type)) { + unsigned Size = getContext().getConstantArrayElementCount(CAT); + ShouldRepeatCall = Data->shouldInline && Data->idx < Size; + } + + if (!ShouldRepeatCall) + state = removePendingArrayDestruction(state, DtorDecl, callerCtx); + } + } + // If the callee returns an expression, bind its value to CallExpr. if (CE) { if (const ReturnStmt *RS = dyn_cast_or_null(LastSt)) { @@ -813,11 +827,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; @@ -872,9 +881,15 @@ 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) { + const CXXDestructorCall &Dtor = cast(Call); + if (auto Data = getPendingArrayDestruction( + Pred->getState(), cast(Dtor.getDecl()), + CurLC)) { + if (!Data->shouldInline) + return CIP_DisallowedOnce; + } + } // Allow disabling temporary destructor inlining with a separate option. if (CallOpts.IsTemporaryCtorOrDtor && @@ -1107,6 +1122,17 @@ return false; } +bool ExprEngine::shouldInlineArrayDestruction(const ArrayType *Type) { + // FIXME: Handle other arrays types. + if (const auto *CAT = dyn_cast(Type)) { + unsigned Size = getContext().getConstantArrayElementCount(CAT); + + return Size <= AMgr.options.maxBlockVisitOnPath; + } + + return false; +} + bool ExprEngine::shouldRepeatCtorCall(ProgramStateRef State, const CXXConstructExpr *E, const LocationContext *LCtx) { Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/RegionStore.cpp +++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp @@ -2373,8 +2373,22 @@ R = GetElementZeroRegion(SR, T); } - assert((!isa(R) || !B.lookup(R)) && - "'this' pointer is not an l-value and is not assignable"); + // FIXME: the snippet below fails on this assert + // + // struct S { + // ~S() {} + // }; + // + // foo() { + // S arr[4]; + // } + // + // The problem is that between the dtor calls the 'this' region + // is not cleaned up by the symbol reaper, so !B.lookup(R) fails + // on the 2nd dtor call. + // + // assert((!isa(R) || !B.lookup(R)) && + // "'this' pointer is not an l-value and is not assignable"); // Clear out bindings that may overlap with this binding. RegionBindingsRef NewB = removeSubRegionBindings(B, cast(R)); Index: clang/test/Analysis/new.cpp =================================================================== --- clang/test/Analysis/new.cpp +++ clang/test/Analysis/new.cpp @@ -323,9 +323,7 @@ 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; } // Invalidate Region even in case of default destructor