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,19 @@ 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); + + static Optional + getHeapRegionInitializer(ProgramStateRef State, const MemRegion *MR); + /// Retreives the size of the array in the pending ArrayInitLoopExpr. static Optional getPendingInitLoop(ProgramStateRef State, const CXXConstructExpr *E, @@ -825,6 +851,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 +953,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, @@ -933,6 +975,12 @@ const CXXConstructExpr *E, const LocationContext *LCtx); + static ProgramStateRef setHeapRegionInitializer(ProgramStateRef State, + const MemRegion *MR, + const CXXNewExpr *); + static ProgramStateRef removeHeapRegionInitializer(ProgramStateRef State, + const MemRegion *MR); + /// 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 Index: clang/lib/Analysis/CFG.cpp =================================================================== --- clang/lib/Analysis/CFG.cpp +++ clang/lib/Analysis/CFG.cpp @@ -5251,8 +5251,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 @@ -204,6 +204,19 @@ std::pair, unsigned> PendingInitLoopMap; REGISTER_TRAIT_WITH_PROGRAMSTATE(PendingInitLoop, PendingInitLoopMap) + +typedef llvm::ImmutableMap< + std::pair, + PendingArrayDestructionData> + PendingArrayDestructionMap; +REGISTER_TRAIT_WITH_PROGRAMSTATE(PendingArrayDestruction, + PendingArrayDestructionMap) + +// This trait stores the initializer of a heap region. +// This is useful when we need to evaluate delete[], which +// has no information about the size of the region. +REGISTER_MAP_WITH_PROGRAMSTATE(HeapRegionInitializer, const MemRegion *, + const CXXNewExpr *) //===----------------------------------------------------------------------===// // Engine construction and deletion. //===----------------------------------------------------------------------===// @@ -470,6 +483,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,23 +558,29 @@ return State->set(Key, Size); } -Optional -ExprEngine::getIndexOfElementToConstruct(ProgramStateRef State, - const CXXConstructExpr *E, - const LocationContext *LCtx) { +Optional +ExprEngine::getHeapRegionInitializer(ProgramStateRef State, + const MemRegion *MR) { + return Optional::create( + State->get(MR)); +} - return Optional::create( - State->get({E, LCtx->getStackFrame()})); +ProgramStateRef ExprEngine::setHeapRegionInitializer(ProgramStateRef State, + const MemRegion *MR, + const CXXNewExpr *Init) { + assert((!State->contains(MR) || + Init->getNumPlacementArgs() > 0) && + "The state already contains the initializer for the heap region!"); + + return State->set(MR, Init); } -ProgramStateRef -ExprEngine::removeIndexOfElementToConstruct(ProgramStateRef State, - const CXXConstructExpr *E, - const LocationContext *LCtx) { - auto Key = std::make_pair(E, LCtx->getStackFrame()); +ProgramStateRef ExprEngine::removeHeapRegionInitializer(ProgramStateRef State, + const MemRegion *MR) { + assert(State->contains(MR) && + "The state doesn't contain the initializer for the heap region!"); - assert(E && State->contains(Key)); - return State->remove(Key); + return State->remove(MR); } ProgramStateRef @@ -1125,7 +1191,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 +1209,31 @@ 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)) { + 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); + } + 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); } @@ -1176,18 +1261,52 @@ 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). + const auto DtorDecl = Dtor.getDestructorDecl(getContext()); + + auto data = getPendingArrayDestruction(State, DtorDecl, LCtx) + .getValueOr(PendingArrayDestructionData{}); + + Idx = data.idx++; + auto NewExpr = getHeapRegionInitializer(State, ArgR).getValueOr(nullptr); + + // If we fail to get the new expression, like when the region is allocated + // using + // ::operator new[], we fall back to conservative evaluation. + if (NewExpr) { + auto Ctor = NewExpr->getInitializer(); + auto Ty = Ctor->getType(); + + data.shouldInline = shouldInlineArrayDestruction(cast(Ty)); + data.type = Ty; + + // Try to do some cleanup, though in case of memory leak the initializer + // won't be removed. + if (!data.shouldInline) + State = removeHeapRegionInitializer(State, ArgR); + } else { + data.shouldInline = false; + data.type = DTy; + } + + State = setPendingArrayDestruction(State, DtorDecl, LCtx, data); + 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); @@ -1226,11 +1345,29 @@ 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)) { + const auto DtorDecl = D.getDestructorDecl(getContext()); + + auto data = getPendingArrayDestruction(State, DtorDecl, LCtx) + .getValueOr(PendingArrayDestructionData{}); + + Idx = data.idx++; + data.shouldInline = shouldInlineArrayDestruction(AT); + data.type = T; + + State = setPendingArrayDestruction(State, DtorDecl, LCtx, data); + } + EvalCallOptions CallOpts; - FieldVal = makeElementRegion(State, FieldVal, T, CallOpts.IsArrayCtorOrDtor); + FieldVal = + makeElementRegion(State, FieldVal, T, CallOpts.IsArrayCtorOrDtor, Idx); + + NodeBuilder Bldr(Pred, Dst, getBuilderContext()); + + EpsilonPoint EP(LCtx, nullptr); + Pred = Bldr.generateNode(EP, State, Pred); + Bldr.takeNodes(Pred); VisitCXXDestructor(T, FieldVal.getAsRegion(), CurDtor->getBody(), /*IsBase=*/false, Pred, Dst, CallOpts); 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(), @@ -1030,7 +1028,9 @@ } } - State = State->BindExpr(CNE, Pred->getLocationContext(), Result); + auto LCtx = Pred->getLocationContext(); + State = setHeapRegionInitializer(State, Result.getAsRegion(), CNE); + State = State->BindExpr(CNE, LCtx, Result); Bldr.generateNode(CNE, Pred, State); return; } @@ -1047,6 +1047,7 @@ } // Bind the address of the object, then check to see if we cached out. + State = setHeapRegionInitializer(State, Result.getAsRegion(), CNE); State = State->BindExpr(CNE, LCtx, Result); ExplodedNode *NewN = Bldr.generateNode(CNE, Pred, State); if (!NewN) 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 && @@ -1091,8 +1106,25 @@ 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) { // FIXME: Handle other arrays types. if (const auto *CAT = dyn_cast(Type)) { unsigned Size = getContext().getConstantArrayElementCount(CAT); @@ -1100,10 +1132,6 @@ 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; } 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/dtor-array.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/dtor-array.cpp @@ -0,0 +1,77 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-inlining=destructors -verify %s + +void clang_analyzer_eval(bool); +void clang_analyzer_checkInlined(bool); + +int a, b, c, d; + +struct InlineDtor { + static int cnt; + static int dtorCalled; + ~InlineDtor() { + switch (dtorCalled) { + 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}} +} 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