NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet.
Herald added subscribers: cfe-commits, rnkovacs.
In https://reviews.llvm.org/D42457 we added the `EvalCallOptions` structure to
notify `evalCall()` when some other code believes that there's something fishy
about the call, so that this other code didn't make decisions about whether the
call needs to be inlined, but instead `evalCall()` itself had all the
information presented in a clear manner and could make a well-thought decision.
This was initially used for constructors, to notify `evalCall()` that we didn't
manage to find the correct region for construction.
Do the same for destructors. The code here is structured differently - we don't
have single "`getRegionForDestructor()`" function within `VisitCXXDestructor`
(but there is a small part of it - the `makeZeroElementRegion()` part), but
we're doing it separately for different CFGElement kinds and only then call
`VisitCXXDestructor()` with a ready-made region. I didn't try to change that,
apart from moving the `makeZeroElementRegion()` hack out to the caller, so that
`EvalCallOptions` were initialized in a single place and then passed by const
reference.
Renamed `EvalCallOptions` flags to be more concise.
Accidentally, part of this patch has leaked into
https://reviews.llvm.org/D42457, which caused a functional change. This patch
reverts that accidental functional change. Apart from that, no functional
change is intended.
Repository:
rC Clang
https://reviews.llvm.org/D42991
Files:
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: test/Analysis/new.cpp
===================================================================
--- test/Analysis/new.cpp
+++ test/Analysis/new.cpp
@@ -311,7 +311,8 @@
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
Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -650,7 +650,7 @@
// 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.
@@ -670,14 +670,14 @@
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;
}
@@ -693,9 +693,14 @@
(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:
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -84,16 +84,8 @@
}
-/// 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();
@@ -128,7 +120,7 @@
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());
}
@@ -140,10 +132,10 @@
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)) {
- CallOpts.IsConstructorIntoTemporary = true;
+ CallOpts.IsTemporaryCtorOrDtor = true;
return MRMgr.getCXXTempObjectRegion(CE, LCtx);
}
// TODO: Consider other directly initialized elements.
@@ -166,7 +158,7 @@
QualType Ty = Field->getType();
FieldVal = makeZeroElementRegion(State, FieldVal, Ty,
- CallOpts.IsArrayConstructorOrDestructor);
+ CallOpts.IsArrayCtorOrDtor);
return FieldVal.getAsRegion();
}
@@ -176,7 +168,7 @@
}
// 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);
}
@@ -266,7 +258,7 @@
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
@@ -336,7 +328,7 @@
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)
@@ -391,23 +383,11 @@
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: lib/StaticAnalyzer/Core/ExprEngine.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -803,8 +803,15 @@
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,
@@ -814,12 +821,12 @@
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();
@@ -830,10 +837,19 @@
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,
@@ -852,12 +868,13 @@
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();
@@ -867,9 +884,16 @@
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,
@@ -893,10 +917,14 @@
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: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===================================================================
--- include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -55,6 +55,20 @@
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 @@
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,
@@ -574,14 +589,6 @@
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,
@@ -665,6 +672,18 @@
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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
