]> granicus.if.org Git - clang/commitdiff
[analyzer] Decide on inlining destructors via EvalCallOptions.
authorArtem Dergachev <artem.dergachev@gmail.com>
Thu, 15 Feb 2018 02:51:58 +0000 (02:51 +0000)
committerArtem Dergachev <artem.dergachev@gmail.com>
Thu, 15 Feb 2018 02:51:58 +0000 (02:51 +0000)
EvalCallOptions were introduced in r324018 for allowing various parts of
ExprEngine to notify the inlining mechanism, while preparing for evaluating a
function call, of possible difficulties with evaluating the call that they
foresee. Then mayInlineCall() would still be a single place for making the
decision.

Use that mechanism for destructors as well - pass the necessary flags from the
CFG-element-specific destructor handlers.

Part of this patch accidentally leaked into r324018, which led into a change in
tests; this change is reverted now, because even though the change looked
correct, the underlying behavior wasn't. Both of these commits were not intended
to introduce any function changes otherwise.

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

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@325209 91177308-0d34-0410-b5e6-96231b3b80d8

include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
lib/StaticAnalyzer/Core/ExprEngine.cpp
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
test/Analysis/new.cpp

index 9864cf2936766985423cf64ba62008a23b19342d..a9423d4f029db99dac413505f13ff1cfa93dbc4e 100644 (file)
@@ -55,6 +55,20 @@ public:
     Inline_Minimal = 0x1
   };
 
+  /// Hints for figuring out of a call should be inlined during evalCall().
+  struct EvalCallOptions {
+    /// This call is a constructor or a destructor for which we do not currently
+    /// compute the this-region correctly.
+    bool IsCtorOrDtorWithImproperlyModeledTargetRegion = false;
+    /// This call is a constructor or a destructor for a single element within
+    /// an array, a part of array construction or destruction.
+    bool IsArrayCtorOrDtor = false;
+    /// This call is a constructor or a destructor of a temporary value.
+    bool IsTemporaryCtorOrDtor = false;
+
+    EvalCallOptions() {}
+  };
+
 private:
   AnalysisManager &AMgr;
   
@@ -449,7 +463,8 @@ public:
 
   void VisitCXXDestructor(QualType ObjectType, const MemRegion *Dest,
                           const Stmt *S, bool IsBaseDtor,
-                          ExplodedNode *Pred, ExplodedNodeSet &Dst);
+                          ExplodedNode *Pred, ExplodedNodeSet &Dst,
+                          const EvalCallOptions &Options);
 
   void VisitCXXNewAllocatorCall(const CXXNewExpr *CNE,
                                 ExplodedNode *Pred,
@@ -568,14 +583,6 @@ public:
                                   const LocationContext *LCtx,
                                   ProgramStateRef State);
 
-  struct EvalCallOptions {
-    bool IsConstructorWithImproperlyModeledTargetRegion = false;
-    bool IsArrayConstructorOrDestructor = false;
-    bool IsConstructorIntoTemporary = false;
-
-    EvalCallOptions() {}
-  };
-
   /// Evaluate a call, running pre- and post-call checks and allowing checkers
   /// to be responsible for handling the evaluation of the call itself.
   void evalCall(ExplodedNodeSet &Dst, ExplodedNode *Pred,
@@ -659,6 +666,17 @@ private:
                                                 const Expr *InitWithAdjustments,
                                                 const Expr *Result = nullptr);
 
+  /// Returns a region representing the first element of a (possibly
+  /// multi-dimensional) array, for the purposes of element construction or
+  /// destruction.
+  ///
+  /// On return, \p Ty will be set to the base type of the array.
+  ///
+  /// If the type is not an array type at all, the original value is returned.
+  /// Otherwise the "IsArray" flag is set.
+  static SVal makeZeroElementRegion(ProgramStateRef State, SVal LValue,
+                                    QualType &Ty, bool &IsArray);
+
   /// For a DeclStmt or CXXInitCtorInitializer, walk backward in the current CFG
   /// block to find the constructor expression that directly constructed into
   /// the storage for this statement. Returns null if the constructor for this
index 210cea6818e20770eddd78513c2fc8226cf3dd53..4c2aca27a4ab06b7504a4717fc5757bc5e7a27d4 100644 (file)
@@ -802,8 +802,15 @@ 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.
+  // This workaround will just run the first destructor (which will still
+  // invalidate the entire array).
+  EvalCallOptions CallOpts;
+  Region = makeZeroElementRegion(state, loc::MemRegionVal(Region), varType,
+                                 CallOpts.IsArrayCtorOrDtor).getAsRegion();
+
   VisitCXXDestructor(varType, Region, Dtor.getTriggerStmt(), /*IsBase=*/ false,
-                     Pred, Dst);
+                     Pred, Dst, CallOpts);
 }
 
 void ExprEngine::ProcessDeleteDtor(const CFGDeleteDtor Dtor,
@@ -813,12 +820,12 @@ void ExprEngine::ProcessDeleteDtor(const CFGDeleteDtor Dtor,
   const LocationContext *LCtx = Pred->getLocationContext();
   const CXXDeleteExpr *DE = Dtor.getDeleteExpr();
   const Stmt *Arg = DE->getArgument();
+  QualType DTy = DE->getDestroyedType();
   SVal ArgVal = State->getSVal(Arg, LCtx);
 
   // If the argument to delete is known to be a null value,
   // don't run destructor.
   if (State->isNull(ArgVal).isConstrainedTrue()) {
-    QualType DTy = DE->getDestroyedType();
     QualType BTy = getContext().getBaseElementType(DTy);
     const CXXRecordDecl *RD = BTy->getAsCXXRecordDecl();
     const CXXDestructorDecl *Dtor = RD->getDestructor();
@@ -829,10 +836,19 @@ void ExprEngine::ProcessDeleteDtor(const CFGDeleteDtor Dtor,
     return;
   }
 
-  VisitCXXDestructor(DE->getDestroyedType(),
-                     ArgVal.getAsRegion(),
-                     DE, /*IsBase=*/ false,
-                     Pred, Dst);
+  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).
+    CallOpts.IsArrayCtorOrDtor = true;
+    if (ArgR)
+      ArgR = getStoreManager().GetElementZeroRegion(cast<SubRegion>(ArgR), DTy);
+  }
+
+  VisitCXXDestructor(DE->getDestroyedType(), ArgR, DE, /*IsBase=*/false,
+                     Pred, Dst, CallOpts);
 }
 
 void ExprEngine::ProcessBaseDtor(const CFGBaseDtor D,
@@ -851,12 +867,13 @@ void ExprEngine::ProcessBaseDtor(const CFGBaseDtor D,
                                                      Base->isVirtual());
 
   VisitCXXDestructor(BaseTy, BaseVal.castAs<loc::MemRegionVal>().getRegion(),
-                     CurDtor->getBody(), /*IsBase=*/ true, Pred, Dst);
+                     CurDtor->getBody(), /*IsBase=*/ true, Pred, Dst, {});
 }
 
 void ExprEngine::ProcessMemberDtor(const CFGMemberDtor D,
                                    ExplodedNode *Pred, ExplodedNodeSet &Dst) {
   const FieldDecl *Member = D.getFieldDecl();
+  QualType T = Member->getType();
   ProgramStateRef State = Pred->getState();
   const LocationContext *LCtx = Pred->getLocationContext();
 
@@ -866,9 +883,15 @@ void ExprEngine::ProcessMemberDtor(const CFGMemberDtor D,
   SVal FieldVal =
       State->getLValue(Member, State->getSVal(ThisVal).castAs<Loc>());
 
-  VisitCXXDestructor(Member->getType(),
-                     FieldVal.castAs<loc::MemRegionVal>().getRegion(),
-                     CurDtor->getBody(), /*IsBase=*/false, Pred, Dst);
+  // 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;
+  FieldVal = makeZeroElementRegion(State, FieldVal, T,
+                                   CallOpts.IsArrayCtorOrDtor);
+
+  VisitCXXDestructor(T, FieldVal.castAs<loc::MemRegionVal>().getRegion(),
+                     CurDtor->getBody(), /*IsBase=*/false, Pred, Dst, CallOpts);
 }
 
 void ExprEngine::ProcessTemporaryDtor(const CFGTemporaryDtor D,
@@ -892,10 +915,14 @@ void ExprEngine::ProcessTemporaryDtor(const CFGTemporaryDtor D,
   assert(CleanDtorState.size() <= 1);
   ExplodedNode *CleanPred =
       CleanDtorState.empty() ? Pred : *CleanDtorState.begin();
+
   // FIXME: Inlining of temporary destructors is not supported yet anyway, so
   // we just put a NULL region for now. This will need to be changed later.
+  EvalCallOptions CallOpts;
+  CallOpts.IsTemporaryCtorOrDtor = true;
+  CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion = true;
   VisitCXXDestructor(varType, nullptr, D.getBindTemporaryExpr(),
-                     /*IsBase=*/false, CleanPred, Dst);
+                     /*IsBase=*/false, CleanPred, Dst, CallOpts);
 }
 
 void ExprEngine::processCleanupTemporaryBranch(const CXXBindTemporaryExpr *BTE,
index 5b1d81315835280f0ed47ebc93ffba26a374c810..fb906cb0951da7c4bb989fd4f4b8bc1fa757546c 100644 (file)
@@ -84,16 +84,8 @@ void ExprEngine::performTrivialCopy(NodeBuilder &Bldr, ExplodedNode *Pred,
 }
 
 
-/// Returns a region representing the first element of a (possibly
-/// multi-dimensional) array, for the purposes of element construction or
-/// destruction.
-///
-/// On return, \p Ty will be set to the base type of the array.
-///
-/// If the type is not an array type at all, the original value is returned.
-/// Otherwise the "IsArray" flag is set.
-static SVal makeZeroElementRegion(ProgramStateRef State, SVal LValue,
-                                  QualType &Ty, bool &IsArray) {
+SVal ExprEngine::makeZeroElementRegion(ProgramStateRef State, SVal LValue,
+                                       QualType &Ty, bool &IsArray) {
   SValBuilder &SVB = State->getStateManager().getSValBuilder();
   ASTContext &Ctx = SVB.getContext();
 
@@ -131,7 +123,7 @@ ExprEngine::getRegionForConstructedObject(const CXXConstructExpr *CE,
             if (CNE->isArray()) {
               // TODO: In fact, we need to call the constructor for every
               // allocated element, not just the first one!
-              CallOpts.IsArrayConstructorOrDestructor = true;
+              CallOpts.IsArrayCtorOrDtor = true;
               return getStoreManager().GetElementZeroRegion(
                   MR, CNE->getType()->getPointeeType());
             }
@@ -143,7 +135,7 @@ ExprEngine::getRegionForConstructedObject(const CXXConstructExpr *CE,
         SVal LValue = State->getLValue(Var, LCtx);
         QualType Ty = Var->getType();
         LValue = makeZeroElementRegion(State, LValue, Ty,
-                                       CallOpts.IsArrayConstructorOrDestructor);
+                                       CallOpts.IsArrayCtorOrDtor);
         return LValue.getAsRegion();
       } else if (auto *RS = dyn_cast<ReturnStmt>(TriggerStmt)) {
         // TODO: We should construct into a CXXBindTemporaryExpr or a
@@ -154,7 +146,7 @@ ExprEngine::getRegionForConstructedObject(const CXXConstructExpr *CE,
         // function, we should be able to take the call-site CFG element,
         // and it should contain (but right now it wouldn't) some sort of
         // construction context that'd give us the right temporary expression.
-        CallOpts.IsConstructorIntoTemporary = true;
+        CallOpts.IsTemporaryCtorOrDtor = true;
         return MRMgr.getCXXTempObjectRegion(CE, LCtx);
       }
       // TODO: Consider other directly initialized elements.
@@ -177,7 +169,7 @@ ExprEngine::getRegionForConstructedObject(const CXXConstructExpr *CE,
 
       QualType Ty = Field->getType();
       FieldVal = makeZeroElementRegion(State, FieldVal, Ty,
-                                       CallOpts.IsArrayConstructorOrDestructor);
+                                       CallOpts.IsArrayCtorOrDtor);
       return FieldVal.getAsRegion();
     }
 
@@ -187,7 +179,7 @@ ExprEngine::getRegionForConstructedObject(const CXXConstructExpr *CE,
   }
   // If we couldn't find an existing region to construct into, assume we're
   // constructing a temporary. Notify the caller of our failure.
-  CallOpts.IsConstructorWithImproperlyModeledTargetRegion = true;
+  CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion = true;
   return MRMgr.getCXXTempObjectRegion(CE, LCtx);
 }
 
@@ -273,7 +265,7 @@ void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE,
     if (dyn_cast_or_null<InitListExpr>(LCtx->getParentMap().getParent(CE))) {
       MemRegionManager &MRMgr = getSValBuilder().getRegionManager();
       Target = MRMgr.getCXXTempObjectRegion(CE, LCtx);
-      CallOpts.IsConstructorWithImproperlyModeledTargetRegion = true;
+      CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion = true;
       break;
     }
     // FALLTHROUGH
@@ -343,7 +335,7 @@ void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE,
 
   if (CE->getConstructor()->isTrivial() &&
       CE->getConstructor()->isCopyOrMoveConstructor() &&
-      !CallOpts.IsArrayConstructorOrDestructor) {
+      !CallOpts.IsArrayCtorOrDtor) {
     // FIXME: Handle other kinds of trivial constructors as well.
     for (ExplodedNodeSet::iterator I = DstPreCall.begin(), E = DstPreCall.end();
          I != E; ++I)
@@ -400,23 +392,11 @@ void ExprEngine::VisitCXXDestructor(QualType ObjectType,
                                     const Stmt *S,
                                     bool IsBaseDtor,
                                     ExplodedNode *Pred,
-                                    ExplodedNodeSet &Dst) {
+                                    ExplodedNodeSet &Dst,
+                                    const EvalCallOptions &CallOpts) {
   const LocationContext *LCtx = Pred->getLocationContext();
   ProgramStateRef State = Pred->getState();
 
-  // 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).
-  SVal DestVal = UnknownVal();
-  if (Dest)
-    DestVal = loc::MemRegionVal(Dest);
-
-  EvalCallOptions CallOpts;
-  DestVal = makeZeroElementRegion(State, DestVal, ObjectType,
-                                  CallOpts.IsArrayConstructorOrDestructor);
-
-  Dest = DestVal.getAsRegion();
-
   const CXXRecordDecl *RecordDecl = ObjectType->getAsCXXRecordDecl();
   assert(RecordDecl && "Only CXXRecordDecls should have destructors");
   const CXXDestructorDecl *DtorDecl = RecordDecl->getDestructor();
index e022196fe589ece0885abc1313d08173d376337f..9f61861ae4fb156c7a7ff3cfbb29eab5988b000a 100644 (file)
@@ -653,7 +653,7 @@ ExprEngine::mayInlineCallKind(const CallEvent &Call, const ExplodedNode *Pred,
     // 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.IsArrayConstructorOrDestructor)
+    if (CallOpts.IsArrayCtorOrDtor)
       return CIP_DisallowedOnce;
 
     // Inlining constructors requires including initializers in the CFG.
@@ -673,14 +673,14 @@ ExprEngine::mayInlineCallKind(const CallEvent &Call, const ExplodedNode *Pred,
     if (CtorExpr->getConstructionKind() == CXXConstructExpr::CK_Complete) {
       // If we don't handle temporary destructors, we shouldn't inline
       // their constructors.
-      if (CallOpts.IsConstructorIntoTemporary &&
+      if (CallOpts.IsTemporaryCtorOrDtor &&
           !Opts.includeTemporaryDtorsInCFG())
         return CIP_DisallowedOnce;
 
-      // If we did not construct the correct this-region, it would be pointless
+      // If we did not find the correct this-region, it would be pointless
       // to inline the constructor. Instead we will simply invalidate
       // the fake temporary target.
-      if (CallOpts.IsConstructorWithImproperlyModeledTargetRegion)
+      if (CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion)
         return CIP_DisallowedOnce;
     }
 
@@ -696,9 +696,14 @@ ExprEngine::mayInlineCallKind(const CallEvent &Call, const ExplodedNode *Pred,
     (void)ADC;
 
     // FIXME: We don't handle constructors or destructors for arrays properly.
-    if (CallOpts.IsArrayConstructorOrDestructor)
+    if (CallOpts.IsArrayCtorOrDtor)
       return CIP_DisallowedOnce;
 
+    // If we did not find the correct this-region, it would be pointless
+    // to inline the destructor. Instead we will simply invalidate
+    // the fake temporary target.
+    if (CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion)
+      return CIP_DisallowedOnce;
     break;
   }
   case CE_CXXAllocator:
@@ -847,14 +852,6 @@ bool ExprEngine::shouldInlineCall(const CallEvent &Call, const Decl *D,
   AnalysisDeclContextManager &ADCMgr = AMgr.getAnalysisDeclContextManager();
   AnalysisDeclContext *CalleeADC = ADCMgr.getContext(D);
 
-  // Temporary object destructor processing is currently broken, so we never
-  // inline them.
-  // FIXME: Remove this once temp destructors are working.
-  if (isa<CXXDestructorCall>(Call)) {
-    if ((*currBldrCtx->getBlock())[currStmtIdx].getAs<CFGTemporaryDtor>())
-      return false;
-  }
-
   // The auto-synthesized bodies are essential to inline as they are
   // usually small and commonly used. Note: we should do this check early on to
   // ensure we always inline these calls.
index 71b9d168eebfc3df27d2d65183aa6665e44e5969..0cd2d085f72c7dd11991fa40237347ee8e011047 100644 (file)
@@ -311,7 +311,8 @@ void testArrayNull() {
 void testArrayDestr() {
   NoReturnDtor *p = new NoReturnDtor[2];
   delete[] p; // Calls the base destructor which aborts, checked below
-  clang_analyzer_eval(true); // no-warning
+   //TODO: clang_analyzer_eval should not be called
+  clang_analyzer_eval(true); // expected-warning{{TRUE}}
 }
 
 // Invalidate Region even in case of default destructor