[analyzer] Cleanup some artifacts from non-POD array evaluation

[isuckatcs]

Summary

Most of the state traits used for non-POD array evaluation were
only cleaned up if the ctors/dtors were inlined, since the cleanup
happened in ExprEngine::processCallExit(). This patch makes sure
they are removed even if said functions are not inlined.

[isuckatcs]

Updated

[NoQ]

Ohh great thanks!!

I wish we had asserts to catch this but that's not urgent I suppose.

[isuckatcs]

I wish we had asserts to catch this but that's not urgent I suppose.

You mean asserts that check whether the states are empty before they are cleaned up or some specific asserts regarding this patch?

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

static ProgramStateRef setPendingInitLoop(ProgramStateRef State,
                                             const CXXConstructExpr *E,
                                             const LocationContext *LCtx,
                                             unsigned Idx);
 
   static ProgramStateRef removePendingInitLoop(ProgramStateRef State,
                                                const CXXConstructExpr *E,
                                                const LocationContext *LCtx);
 
+  static ProgramStateRef
+  removeStateTraitsUsedForArrayEvaluation(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
   /// to be "under construction" between CXXConstructExpr and DeclStmt.
   /// This allows, among other things, to keep bindings to variable's fields
   /// made within the constructor alive until its declaration actually
   /// goes into scope.
   static ProgramStateRef

clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

if (!ElementCount.isConstant())
       return 0;
 
     return ElementCount.getAsInteger()->getLimitedValue();
   }
 
   return 0;
 }
 
+ProgramStateRef ExprEngine::removeStateTraitsUsedForArrayEvaluation(
+    ProgramStateRef State, const CXXConstructExpr *E,
+    const LocationContext *LCtx) {
+
+  assert(LCtx && "Location context must be provided!");
+
+  if (E) {
+    if (getPendingInitLoop(State, E, LCtx))
+      State = removePendingInitLoop(State, E, LCtx);
+
+    if (getIndexOfElementToConstruct(State, E, LCtx))
+      State = removeIndexOfElementToConstruct(State, E, LCtx);
+  }
+
+  if (getPendingArrayDestruction(State, LCtx))
+    State = removePendingArrayDestruction(State, LCtx);
+
+  return State;
+}
+
 /// 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:
 /// 1. CallExitBegin (triggers the start of call exit sequence)
 /// 2. Bind the return value
 /// 3. Run Remove dead bindings to clean up the dead symbols from the callee.
 /// 4. CallExitEnd (switch to the caller context)
 /// 5. PostStmt<CallExpr>
void ExprEngine::processCallExit(ExplodedNode *CEBNode) {
 
   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)) {
       const LocationContext *LCtx = CEBNode->getLocationContext();
       SVal V = state->getSVal(RS, LCtx);
if (CE) {
     if (const CXXConstructExpr *CCE = dyn_cast<CXXConstructExpr>(CE)) {
       loc::MemRegionVal This =
         svalBuilder.getCXXThis(CCE->getConstructor()->getParent(), calleeCtx);
       SVal ThisV = state->getSVal(This);
       ThisV = state->getSVal(ThisV.castAs<Loc>());
       state = state->BindExpr(CCE, callerCtx, ThisV);
 
       ShouldRepeatCall = shouldRepeatCtorCall(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<CXXNewExpr>(CE)) {
       // We are currently evaluating a CXXNewAllocator CFGElement. It takes a
       // while to reach the actual CXXNewExpr element from here, so keep the
       // region for later use.
       // Additionally cast the return value of the inlined operator new
       // (which is of type 'void *') to the correct object type.
       SVal AllocV = state->getSVal(CNE, callerCtx);
       AllocV = svalBuilder.evalCast(
           AllocV, CNE->getType(),
           getContext().getPointerType(getContext().VoidTy));
 
       state = addObjectUnderConstruction(state, CNE, calleeCtx->getParent(),
                                          AllocV);
     }
   }
 
+  if (!ShouldRepeatCall) {
+    state = removeStateTraitsUsedForArrayEvaluation(
+        state, dyn_cast_or_null<CXXConstructExpr>(CE), callerCtx);
+  }
+
   // Step 3: BindedRetNode -> CleanedNodes
   // If we can find a statement and a block in the inlined function, run remove
   // dead bindings before returning from the call. This is important to ensure
   // that we report the issues such as leaks in the stack contexts in which
   // they occurred.
   ExplodedNodeSet CleanedNodes;
   if (LastSt && Blk && AMgr.options.AnalysisPurgeOpt != PurgeNone) {
     static SimpleProgramPointTag retValBind("ExprEngine", "Bind Return Value");
if (const auto *CAT = dyn_cast<ConstantArrayType>(CE->getType())) {
     // 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.
   if (auto Size = getPendingInitLoop(State, CE, LCtx))
-    return *Size <= AMgr.options.maxBlockVisitOnPath;
+    return shouldInlineArrayDestruction(*Size);
 
   return false;
 }
 
 bool ExprEngine::shouldInlineArrayDestruction(uint64_t Size) {
 
   uint64_t maxAllowedSize = AMgr.options.maxBlockVisitOnPath;
 
if (shouldInlineCall(*Call, D, Pred, CallOpts)) {
           return;
         }
       }
       ctuBifurcate(*Call, D, Bldr, Pred, State);
       return;
     }
   }
 
-  // If we can't inline it, handle the return value and invalidate the regions.
+  // If we can't inline it, clean up the state traits used only if the function
+  // is inlined.
+  State = removeStateTraitsUsedForArrayEvaluation(
+      State, dyn_cast_or_null<CXXConstructExpr>(E), Call->getLocationContext());
+
+  // Also handle the return value and invalidate the regions.
   conservativeEvalCall(*Call, Bldr, Pred, State);
 }
 
 void ExprEngine::BifurcateCall(const MemRegion *BifurReg,
                                const CallEvent &Call, const Decl *D,
                                NodeBuilder &Bldr, ExplodedNode *Pred) {
   assert(BifurReg);
   BifurReg = BifurReg->StripCasts();