[analyzer] Process non-POD array element destructors

The constructors of non-POD array elements are evaluated under
certain conditions. This patch makes sure that in such cases
we also evaluate the destructors.

Differential Revision: https://reviews.llvm.org/D130737

clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h

@@ -617,11 +617,16 @@ public:
     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<unsigned>
   getIndexOfElementToConstruct(ProgramStateRef State, const CXXConstructExpr *E,
                                const LocationContext *LCtx);
 
+  /// Retreives which element is being destructed in a non-POD type array.
+  static Optional<unsigned>
+  getPendingArrayDestruction(ProgramStateRef State,
+                             const LocationContext *LCtx);
+
   /// Retreives the size of the array in the pending ArrayInitLoopExpr.
   static Optional<unsigned> getPendingInitLoop(ProgramStateRef State,
                                                const CXXConstructExpr *E,
@@ -825,6 +830,27 @@ private:
                                      const CXXConstructExpr *CE,
                                      const LocationContext *LCtx);
 
+  /// Checks whether our policies allow us to inline a non-POD type array
+  /// destruction.
+  /// \param Size The size of the array.
+  bool shouldInlineArrayDestruction(uint64_t Size);
+
+  /// Prepares the program state for array destruction. If no error happens
+  /// the function binds a 'PendingArrayDestruction' entry to the state, which
+  /// it returns along with the index. If any error happens (we fail to read
+  /// the size, the index would be -1, etc.) the function will return the
+  /// original state along with an index of 0. The actual element count of the
+  /// array can be accessed by the optional 'ElementCountVal' parameter. \param
+  /// State The program state. \param Region The memory region where the array
+  /// is stored. \param ElementTy The type an element in the array. \param LCty
+  /// The location context. \param ElementCountVal A pointer to an optional
+  /// SVal. If specified, the size of the array will be returned in it. It can
+  /// be Unknown.
+  std::pair<ProgramStateRef, uint64_t> prepareStateForArrayDestruction(
+      const ProgramStateRef State, const MemRegion *Region,
+      const QualType &ElementTy, const LocationContext *LCtx,
+      SVal *ElementCountVal = nullptr);
+
   /// 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,
@@ -924,6 +950,16 @@ private:
                                   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,

clang/lib/Analysis/CFG.cpp

@@ -1970,9 +1970,10 @@ void CFGBuilder::addImplicitDtorsForDestructor(const CXXDestructorDecl *DD) {
   for (auto *FI : RD->fields()) {
     // Check for constant size array. Set type to array element type.
     QualType QT = FI->getType();
-    if (const ConstantArrayType *AT = Context->getAsConstantArrayType(QT)) {
+    // It may be a multidimensional array.
+    while (const ConstantArrayType *AT = Context->getAsConstantArrayType(QT)) {
       if (AT->getSize() == 0)
-        continue;
+        break;
       QT = AT->getElementType();
     }
 
@@ -5333,8 +5334,19 @@ CFGImplicitDtor::getDestructorDecl(ASTContext &astContext) const {
       const CXXTemporary *temp = bindExpr->getTemporary();
       return temp->getDestructor();
     }
+    case CFGElement::MemberDtor: {
+      const FieldDecl *field = castAs<CFGMemberDtor>().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;
   }

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 @@ typedef llvm::ImmutableMap<
     std::pair<const CXXConstructExpr *, const LocationContext *>, unsigned>
     PendingInitLoopMap;
 REGISTER_TRAIT_WITH_PROGRAMSTATE(PendingInitLoop, PendingInitLoopMap)
+
+typedef llvm::ImmutableMap<const LocationContext *, unsigned>
+    PendingArrayDestructionMap;
+REGISTER_TRAIT_WITH_PROGRAMSTATE(PendingArrayDestruction,
+                                 PendingArrayDestructionMap)
+
 //===----------------------------------------------------------------------===//
 // Engine construction and deletion.
 //===----------------------------------------------------------------------===//
@@ -517,6 +524,35 @@ ExprEngine::removeIndexOfElementToConstruct(ProgramStateRef State,
   return State->remove<IndexOfElementToConstruct>(Key);
 }
 
+Optional<unsigned>
+ExprEngine::getPendingArrayDestruction(ProgramStateRef State,
+                                       const LocationContext *LCtx) {
+  assert(LCtx && "LocationContext shouldn't be null!");
+
+  return Optional<unsigned>::create(
+      State->get<PendingArrayDestruction>(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<PendingArrayDestruction>(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<PendingArrayDestruction>(Key));
+  return State->remove<PendingArrayDestruction>(Key);
+}
+
 ProgramStateRef
 ExprEngine::addObjectUnderConstruction(ProgramStateRef State,
                                        const ConstructionContextItem &Item,
@@ -1072,6 +1108,42 @@ void ExprEngine::ProcessInitializer(const CFGInitializer CFGInit,
   Engine.enqueue(Dst, currBldrCtx->getBlock(), currStmtIdx);
 }
 
+std::pair<ProgramStateRef, uint64_t>
+ExprEngine::prepareStateForArrayDestruction(const ProgramStateRef State,
+                                            const MemRegion *Region,
+                                            const QualType &ElementTy,
+                                            const LocationContext *LCtx,
+                                            SVal *ElementCountVal) {
+
+  QualType Ty = ElementTy.getDesugaredType(getContext());
+  while (const auto *NTy = dyn_cast<ArrayType>(Ty))
+    Ty = NTy->getElementType().getDesugaredType(getContext());
+
+  auto ElementCount = getDynamicElementCount(State, Region, svalBuilder, Ty);
+
+  if (ElementCountVal)
+    *ElementCountVal = ElementCount;
+
+  // Note: the destructors are called in reverse order.
+  unsigned Idx = 0;
+  if (auto OptionalIdx = getPendingArrayDestruction(State, LCtx)) {
+    Idx = *OptionalIdx;
+  } else {
+    // The element count is either unknown, or an SVal that's not an integer.
+    if (!ElementCount.isConstant())
+      return {State, 0};
+
+    Idx = ElementCount.getAsInteger()->getLimitedValue();
+  }
+
+  if (Idx == 0)
+    return {State, 0};
+
+  --Idx;
+
+  return {setPendingArrayDestruction(State, LCtx, Idx), Idx};
+}
+
 void ExprEngine::ProcessImplicitDtor(const CFGImplicitDtor D,
                                      ExplodedNode *Pred) {
   ExplodedNodeSet Dst;
@@ -1121,11 +1193,14 @@ void ExprEngine::ProcessNewAllocator(const CXXNewExpr *NE,
 void ExprEngine::ProcessAutomaticObjDtor(const CFGAutomaticObjDtor Dtor,
                                          ExplodedNode *Pred,
                                          ExplodedNodeSet &Dst) {
+  const auto *DtorDecl = Dtor.getDestructorDecl(getContext());
   const VarDecl *varDecl = Dtor.getVarDecl();
   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<loc::MemRegionVal>().getRegion();
 
   if (varType->isReferenceType()) {
@@ -1141,14 +1216,46 @@ void ExprEngine::ProcessAutomaticObjDtor(const CFGAutomaticObjDtor Dtor,
     varType = cast<TypedValueRegion>(Region)->getValueType();
   }
 
-  // FIXME: We need to run the same destructor on every element of the array.
+  unsigned Idx = 0;
-  // This workaround will just run the first destructor (which will still
+  if (const auto *AT = dyn_cast<ArrayType>(varType)) {
-  // invalidate the entire array).
+    SVal ElementCount;
+    std::tie(state, Idx) = prepareStateForArrayDestruction(
+        state, Region, varType, LCtx, &ElementCount);
+
+    if (ElementCount.isConstant()) {
+      uint64_t ArrayLength = ElementCount.getAsInteger()->getLimitedValue();
+      assert(ArrayLength &&
+             "An automatic dtor for a 0 length array shouldn't be triggered!");
+
+      // Still handle this case if we don't have assertions enabled.
+      if (!ArrayLength) {
+        static SimpleProgramPointTag PT(
+            "ExprEngine", "Skipping automatic 0 length array destruction, "
+                          "which shouldn't be in the CFG.");
+        PostImplicitCall PP(DtorDecl, varDecl->getLocation(), LCtx, &PT);
+        NodeBuilder Bldr(Pred, Dst, *currBldrCtx);
+        Bldr.generateSink(PP, Pred->getState(), Pred);
+        return;
+      }
+    }
+  }
+
   EvalCallOptions CallOpts;
   Region = makeElementRegion(state, loc::MemRegionVal(Region), varType,
-                             CallOpts.IsArrayCtorOrDtor)
+                             CallOpts.IsArrayCtorOrDtor, Idx)
                .getAsRegion();
 
+  NodeBuilder Bldr(Pred, Dst, getBuilderContext());
+
+  static SimpleProgramPointTag PT("ExprEngine",
+                                  "Prepare for object destruction");
+  PreImplicitCall PP(DtorDecl, varDecl->getLocation(), LCtx, &PT);
+  Pred = Bldr.generateNode(PP, state, Pred);
+  Bldr.takeNodes(Pred);
+
+  if (!Pred)
+    return;
+
   VisitCXXDestructor(varType, Region, Dtor.getTriggerStmt(),
                      /*IsBase=*/false, Pred, Dst, CallOpts);
 }
@@ -1176,20 +1283,53 @@ void ExprEngine::ProcessDeleteDtor(const CFGDeleteDtor Dtor,
     return;
   }
 
+  auto getDtorDecl = [](const QualType &DTy) {
+    const CXXRecordDecl *RD = DTy->getAsCXXRecordDecl();
+    return RD->getDestructor();
+  };
+
+  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.
+    SVal ElementCount;
-    // This workaround will just run the first destructor (which will still
+    std::tie(State, Idx) =
-    // invalidate the entire array).
+        prepareStateForArrayDestruction(State, ArgR, DTy, LCtx, &ElementCount);
+
     CallOpts.IsArrayCtorOrDtor = true;
     // Yes, it may even be a multi-dimensional array.
     while (const auto *AT = getContext().getAsArrayType(DTy))
       DTy = AT->getElementType();
+
+    // If we're about to destruct a 0 length array, don't run any of the
+    // destructors.
+    if (ElementCount.isConstant() &&
+        ElementCount.getAsInteger()->getLimitedValue() == 0) {
+
+      static SimpleProgramPointTag PT(
+          "ExprEngine", "Skipping 0 length array delete destruction");
+      PostImplicitCall PP(getDtorDecl(DTy), DE->getBeginLoc(), LCtx, &PT);
+      NodeBuilder Bldr(Pred, Dst, *currBldrCtx);
+      Bldr.generateNode(PP, Pred->getState(), Pred);
+      return;
+    }
+
     if (ArgR)
-      ArgR = getStoreManager().GetElementZeroRegion(cast<SubRegion>(ArgR), DTy);
+      ArgR = State->getLValue(DTy, svalBuilder.makeArrayIndex(Idx), ArgVal)
+                 .getAsRegion();
   }
 
+  NodeBuilder Bldr(Pred, Dst, getBuilderContext());
+  static SimpleProgramPointTag PT("ExprEngine",
+                                  "Prepare for object destruction");
+  PreImplicitCall PP(getDtorDecl(DTy), DE->getBeginLoc(), LCtx, &PT);
+  Pred = Bldr.generateNode(PP, State, Pred);
+  Bldr.takeNodes(Pred);
+
+  if (!Pred)
+    return;
+
   VisitCXXDestructor(DTy, ArgR, DE, /*IsBase=*/false, Pred, Dst, CallOpts);
 }
 
@@ -1215,6 +1355,7 @@ void ExprEngine::ProcessBaseDtor(const CFGBaseDtor D,
 
 void ExprEngine::ProcessMemberDtor(const CFGMemberDtor D,
                                    ExplodedNode *Pred, ExplodedNodeSet &Dst) {
+  const auto *DtorDecl = D.getDestructorDecl(getContext());
   const FieldDecl *Member = D.getFieldDecl();
   QualType T = Member->getType();
   ProgramStateRef State = Pred->getState();
@@ -1226,11 +1367,44 @@ void ExprEngine::ProcessMemberDtor(const CFGMemberDtor D,
   Loc ThisLoc = State->getSVal(ThisStorageLoc).castAs<Loc>();
   SVal FieldVal = State->getLValue(Member, ThisLoc);
 
-  // FIXME: We need to run the same destructor on every element of the array.
+  unsigned Idx = 0;
-  // This workaround will just run the first destructor (which will still
+  if (const auto *AT = dyn_cast<ArrayType>(T)) {
-  // invalidate the entire array).
+    SVal ElementCount;
+    std::tie(State, Idx) = prepareStateForArrayDestruction(
+        State, FieldVal.getAsRegion(), T, LCtx, &ElementCount);
+
+    if (ElementCount.isConstant()) {
+      uint64_t ArrayLength = ElementCount.getAsInteger()->getLimitedValue();
+      assert(ArrayLength &&
+             "A member dtor for a 0 length array shouldn't be triggered!");
+
+      // Still handle this case if we don't have assertions enabled.
+      if (!ArrayLength) {
+        static SimpleProgramPointTag PT(
+            "ExprEngine", "Skipping member 0 length array destruction, which "
+                          "shouldn't be in the CFG.");
+        PostImplicitCall PP(DtorDecl, Member->getLocation(), LCtx, &PT);
+        NodeBuilder Bldr(Pred, Dst, *currBldrCtx);
+        Bldr.generateSink(PP, Pred->getState(), Pred);
+        return;
+      }
+    }
+  }
+
   EvalCallOptions CallOpts;
-  FieldVal = makeElementRegion(State, FieldVal, T, CallOpts.IsArrayCtorOrDtor);
+  FieldVal =
+      makeElementRegion(State, FieldVal, T, CallOpts.IsArrayCtorOrDtor, Idx);
+
+  NodeBuilder Bldr(Pred, Dst, getBuilderContext());
+
+  static SimpleProgramPointTag PT("ExprEngine",
+                                  "Prepare for object destruction");
+  PreImplicitCall PP(DtorDecl, Member->getLocation(), LCtx, &PT);
+  Pred = Bldr.generateNode(PP, State, Pred);
+  Bldr.takeNodes(Pred);
+
+  if (!Pred)
+    return;
 
   VisitCXXDestructor(T, FieldVal.getAsRegion(), CurDtor->getBody(),
                      /*IsBase=*/false, Pred, Dst, CallOpts);
@@ -1281,15 +1455,31 @@ void ExprEngine::ProcessTemporaryDtor(const CFGTemporaryDtor D,
   EvalCallOptions CallOpts;
   CallOpts.IsTemporaryCtorOrDtor = true;
   if (!MR) {
-    // If we have no MR, we still need to unwrap the array to avoid destroying
+    // FIXME: If we have no MR, we still need to unwrap the array to avoid
-    // the whole array at once. Regardless, we'd eventually need to model array
+    // destroying the whole array at once.
-    // destructors properly, element-by-element.
+    //
+    // For this case there is no universal solution as there is no way to
+    // directly create an array of temporary objects. There are some expressions
+    // however which can create temporary objects and have an array type.
+    //
+    // E.g.: std::initializer_list<S>{S(), S()};
+    //
+    // The expression above has a type of 'const struct S[2]' but it's a single
+    // 'std::initializer_list<>'. The destructors of the 2 temporary 'S()'
+    // objects will be called anyway, because they are 2 separate objects in 2
+    // separate clusters, i.e.: not an array.
+    //
+    // Now the 'std::initializer_list<>' is not an array either even though it
+    // has the type of an array. The point is, we only want to invoke the
+    // destructor for the initializer list once not twice or so.
     while (const ArrayType *AT = getContext().getAsArrayType(T)) {
       T = AT->getElementType();
-      CallOpts.IsArrayCtorOrDtor = true;
+
+      // FIXME: Enable this flag once we handle this case properly.
+      // 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.
   }

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

@@ -171,13 +171,14 @@ SVal ExprEngine::computeObjectUnderConstruction(
         if (const SubRegion *MR =
                 dyn_cast_or_null<SubRegion>(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(),
+            auto Ty = NE->getType()->getPointeeType();
-                                            svalBuilder.makeArrayIndex(Idx), MR,
+            while (const auto *AT = getContext().getAsArrayType(Ty))
-                                            SVB.getContext());
+              Ty = AT->getElementType();
+
+            auto R = MRMgr.getElementRegion(Ty, svalBuilder.makeArrayIndex(Idx),
+                                            MR, SVB.getContext());
 
             return loc::MemRegionVal(R);
           }

clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

@@ -195,6 +195,33 @@ static bool wasDifferentDeclUsedForInlining(CallEventRef<> Call,
   return RuntimeCallee->getCanonicalDecl() != StaticDecl->getCanonicalDecl();
 }
 
+// Returns the number of elements in the array currently being destructed.
+// If the element count is not found 0 will be returned.
+static unsigned getElementCountOfArrayBeingDestructed(
+    const CallEvent &Call, const ProgramStateRef State, SValBuilder &SVB) {
+  assert(isa<CXXDestructorCall>(Call) &&
+         "The call event is not a destructor call!");
+
+  const auto &DtorCall = cast<CXXDestructorCall>(Call);
+
+  auto ThisVal = DtorCall.getCXXThisVal();
+
+  if (auto ThisElementRegion = dyn_cast<ElementRegion>(ThisVal.getAsRegion())) {
+    auto ArrayRegion = ThisElementRegion->getAsArrayOffset().getRegion();
+    auto ElementType = ThisElementRegion->getElementType();
+
+    auto ElementCount =
+        getDynamicElementCount(State, ArrayRegion, SVB, ElementType);
+
+    if (!ElementCount.isConstant())
+      return 0;
+
+    return ElementCount.getAsInteger()->getLimitedValue();
+  }
+
+  return 0;
+}
+
 /// 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 +261,19 @@ void ExprEngine::processCallExit(ExplodedNode *CEBNode) {
   // 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<CXXDestructorDecl>(Call->getDecl())) {
+    if (auto Idx = getPendingArrayDestruction(state, callerCtx)) {
+      ShouldRepeatCall = *Idx > 0;
+
+      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<ReturnStmt>(LastSt)) {
@@ -818,11 +858,6 @@ ExprEngine::mayInlineCallKind(const CallEvent &Call, const ExplodedNode *Pred,
         !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 +912,12 @@ ExprEngine::mayInlineCallKind(const CallEvent &Call, const ExplodedNode *Pred,
     assert(ADC->getCFGBuildOptions().AddImplicitDtors && "No CFG destructors");
     (void)ADC;
 
-    // FIXME: We don't handle destructors for arrays properly.
+    if (CallOpts.IsArrayCtorOrDtor) {
-    if (CallOpts.IsArrayCtorOrDtor)
+      if (!shouldInlineArrayDestruction(getElementCountOfArrayBeingDestructed(
-      return CIP_DisallowedOnce;
+              Call, Pred->getState(), svalBuilder))) {
+        return CIP_DisallowedOnce;
+      }
+    }
 
     // Allow disabling temporary destructor inlining with a separate option.
     if (CallOpts.IsTemporaryCtorOrDtor &&
@@ -1096,13 +1134,19 @@ bool ExprEngine::shouldInlineArrayConstruction(const ProgramStateRef State,
   if (!CE)
     return false;
 
-  auto Type = CE->getType();
-
   // FIXME: Handle other arrays types.
-  if (const auto *CAT = dyn_cast<ConstantArrayType>(Type)) {
+  if (const auto *CAT = dyn_cast<ConstantArrayType>(CE->getType())) {
-    unsigned Size = getContext().getConstantArrayElementCount(CAT);
+    unsigned ArrSize = getContext().getConstantArrayElementCount(CAT);
 
-    return Size <= AMgr.options.maxBlockVisitOnPath;
+    // 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.
+    return shouldInlineArrayDestruction(ArrSize);
   }
 
   // Check if we're inside an ArrayInitLoopExpr, and it's sufficiently small.
@@ -1112,6 +1156,14 @@ bool ExprEngine::shouldInlineArrayConstruction(const ProgramStateRef State,
   return false;
 }
 
+bool ExprEngine::shouldInlineArrayDestruction(uint64_t Size) {
+
+  uint64_t maxAllowedSize = AMgr.options.maxBlockVisitOnPath;
+
+  // Declaring a 0 element array is also possible.
+  return Size <= maxAllowedSize && Size > 0;
+}
+
 bool ExprEngine::shouldRepeatCtorCall(ProgramStateRef State,
                                       const CXXConstructExpr *E,
                                       const LocationContext *LCtx) {

clang/lib/StaticAnalyzer/Core/ProgramState.cpp

@@ -216,8 +216,6 @@ ProgramState::invalidateRegionsImpl(ValueList Values,
 }
 
 ProgramStateRef ProgramState::killBinding(Loc LV) const {
-  assert(!isa<loc::MemRegionVal>(LV) && "Use invalidateRegion instead.");
-
   Store OldStore = getStore();
   const StoreRef &newStore =
     getStateManager().StoreMgr->killBinding(OldStore, LV);

clang/test/Analysis/dtor-array.cpp

@@ -0,0 +1,347 @@
+// 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
+
+using size_t =  __typeof(sizeof(int));
+
+void clang_analyzer_eval(bool);
+void clang_analyzer_checkInlined(bool);
+void clang_analyzer_warnIfReached();
+void clang_analyzer_explain(int);
+
+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}}
+}
+
+// This snippet used to crash.
+namespace crash2
+{
+  template <class _Tp> class unique_ptr {
+  typedef _Tp *pointer;
+  pointer __ptr_;
+
+public:
+  unique_ptr(pointer __p) : __ptr_(__p) {}
+  ~unique_ptr() { reset(); }
+  pointer get() { return __ptr_;}
+  void reset() {}
+};
+
+struct S;
+
+S *makeS();
+int bar(S *x, S *y);
+
+void foo() {
+  unique_ptr<S> x(makeS()), y(makeS());
+  bar(x.get(), y.get());
+}
+
+void bar() {
+  foo();
+  clang_analyzer_warnIfReached();  // expected-warning{{REACHABLE}}
+}
+
+} // namespace crash2
+
+// This snippet used to crash.
+namespace crash3
+{
+struct InlineDtor {
+  ~InlineDtor() {}
+};
+struct MultipleMemberDtor
+{
+  InlineDtor arr[4];
+  InlineDtor arr2[4];
+};
+
+void foo(){
+  auto *arr = new MultipleMemberDtor[4];
+  delete[] arr;
+  clang_analyzer_warnIfReached();  // expected-warning{{REACHABLE}}
+}
+} // namespace crash3
+
+namespace crash4 {
+struct a {
+  a *b;
+};
+struct c {
+  a d;
+  c();
+  ~c() {
+    for (a e = d;; e = *e.b)
+      ;
+  }
+};
+void f() { 
+  c g; 
+  clang_analyzer_warnIfReached();  // expected-warning{{REACHABLE}}
+}
+
+} // namespace crash4
+
+namespace crash5 {
+namespace std {
+template <class _Tp> class unique_ptr {
+  _Tp *__ptr_;
+public:
+  unique_ptr(_Tp *__p) : __ptr_(__p) {}
+  ~unique_ptr() {}
+};
+} // namespace std
+
+int SSL_use_certificate(int *arg) {
+  std::unique_ptr<int> free_x509(arg);
+  {
+    if (SSL_use_certificate(arg)) {
+      return 0;
+    }
+  }
+  clang_analyzer_warnIfReached();  // expected-warning{{REACHABLE}}
+  return 1;
+}
+
+} // namespace crash5
+
+void zeroLength(){
+  InlineDtor::dtorCalled = 0;
+
+  auto *arr = new InlineDtor[0];
+  delete[] arr;
+
+  auto *arr2 = new InlineDtor[2][0][2];
+  delete[] arr2;
+
+  auto *arr3 = new InlineDtor[0][2][2];
+  delete[] arr3;
+
+  auto *arr4 = new InlineDtor[2][2][0];
+  delete[] arr4;
+
+  // FIXME: Should be TRUE once the constructors are handled properly.
+  clang_analyzer_eval(InlineDtor::dtorCalled == 0); // expected-warning {{TRUE}} expected-warning {{FALSE}}
+}
+
+
+void evalOrderPrep() {
+  EvalOrderArr[0] = 0;
+  EvalOrderArr[1] = 0;
+  EvalOrderArr[2] = 0;
+  EvalOrderArr[3] = 0;
+
+  EvalOrder::ctorCalled = 0;
+  EvalOrder::dtorCalled = 0;
+}
+
+void multidimensionalPrep(){
+  EvalOrder::ctorCalled = 0;
+  EvalOrder::dtorCalled = 0;
+
+  EvalOrder arr[2][2];
+}
+
+void multidimensional(){
+  evalOrderPrep();
+  multidimensionalPrep();
+  
+  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}}
+}
+
+void multidimensionalHeap() {
+  evalOrderPrep();
+
+  auto* eptr = new EvalOrder[2][2];
+  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 MultiWrapper{
+  EvalOrder arr[2][2];
+};
+
+void multidimensionalMember(){
+  evalOrderPrep();
+  
+  auto* mptr = new MultiWrapper;
+  delete mptr;
+
+  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}}
+}
+
+void *memset(void *, int, size_t);
+void clang_analyzer_dumpElementCount(InlineDtor *);
+
+void nonConstantRegionExtent(){
+
+  InlineDtor::dtorCalled = 0;
+
+  int x = 3;
+  memset(&x, 1, sizeof(x));
+
+  InlineDtor *arr = new InlineDtor[x];
+  clang_analyzer_dumpElementCount(arr); // expected-warning {{conj_$0}}
+  delete [] arr;
+
+  //FIXME: This should be TRUE but memset also sets this
+  // region to a conjured symbol.
+  clang_analyzer_eval(InlineDtor::dtorCalled == 0); // expected-warning {{TRUE}} expected-warning {{FALSE}}
+}

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 testArrayNull() {
 
 void testArrayDestr() {
   NoReturnDtor *p = new NoReturnDtor[2];
-  delete[] p;                // Calls the base destructor which aborts, checked below
+  delete[] p;
-                             // TODO: clang_analyzer_eval should not be called
+  clang_analyzer_warnIfReached(); // no-warning
-  clang_analyzer_eval(true); // expected-warning{{TRUE}}
 }
 
 // Invalidate Region even in case of default destructor