NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet.
Herald added subscribers: cfe-commits, rnkovacs.
- If we are unable to determine a good target region for a C++ constructor
call, we come up with a temporary region and construct into that.
- If we suddenly notice that our constructor is part of an C++ array
construction (operator new[] or an array-typed local variable), we come up with
the element-zero region and construct into that, same for destructors.
Both are bad not only because we fail to model stuff correctly, but also
because we use region structure to communicate the fact that we failed to
`ExprEngine::evalCall`, which would later decide if we should inline the call
or not by looking at the region structure. The former is particularly bad
because this prevents us from supporting the actual construction into
temporaries - even if we produce a correct temporary region, we won't inline
the constructor because temporary region is an (incorrect) indication of
failure.
In order to unblock the progress in this area, this patch adds an auxiliary
flag for `evalCall`'s clients to communicate potential problems with the call.
Then `evalCall` would decide if it should still inline the call. Apparently,
the current decision process is non-trivial in this case: for instance, we may
still inline the constructor even if the target region is incorrect aka
temporary, when the class's destructor is trivial. In any case, it's good to
have all the when-to-inline heuristics in one place.
For now `ExprEngine::evalCall` itself doesn't have a flag - only
`ExprEngine::defaultEvalCall` does. Because for now it's only relevant to
constructor/destructor calls, but checker-side `evalCall` only works for simple
call expressions - that's a separate issue. Ideally the flag should go there as
well, and `defaultEvalCall` would only be called from `ExprEngine::evalCall`.
Functional change here is accidental - by communicating array destructor
situation properly, we're able to fix an old FIXME.
Repository:
rC Clang
https://reviews.llvm.org/D42457
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
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,8 +311,7 @@
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}}
+ clang_analyzer_eval(true); // no-warning
}
// Invalidate Region even in case of default destructor
Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -624,7 +624,8 @@
static CallInlinePolicy mayInlineCallKind(const CallEvent &Call,
const ExplodedNode *Pred,
- AnalyzerOptions &Opts) {
+ AnalyzerOptions &Opts,
+ ExprEngine::EvalCallFlags Flags) {
const LocationContext *CurLC = Pred->getLocationContext();
const StackFrameContext *CallerSFC = CurLC->getCurrentStackFrame();
switch (Call.getKind()) {
@@ -658,18 +659,8 @@
// initializers for array fields in default move/copy constructors.
// We still allow construction into ElementRegion targets when they don't
// represent array elements.
- const MemRegion *Target = Ctor.getCXXThisVal().getAsRegion();
- if (Target && isa<ElementRegion>(Target)) {
- if (ParentExpr)
- if (const CXXNewExpr *NewExpr = dyn_cast<CXXNewExpr>(ParentExpr))
- if (NewExpr->isArray())
- return CIP_DisallowedOnce;
-
- if (const TypedValueRegion *TR = dyn_cast<TypedValueRegion>(
- cast<SubRegion>(Target)->getSuperRegion()))
- if (TR->getValueType()->isArrayType())
- return CIP_DisallowedOnce;
- }
+ if (Flags.IsArrayConstructorOrDestructor)
+ return CIP_DisallowedOnce;
// Inlining constructors requires including initializers in the CFG.
const AnalysisDeclContext *ADC = CallerSFC->getAnalysisDeclContext();
@@ -688,7 +679,7 @@
// FIXME: This is a hack. We don't handle temporary destructors
// right now, so we shouldn't inline their constructors.
if (CtorExpr->getConstructionKind() == CXXConstructExpr::CK_Complete)
- if (!Target || isa<CXXTempObjectRegion>(Target))
+ if (Flags.IsConstructorWithImproperlyModeledTargetRegion)
return CIP_DisallowedOnce;
break;
@@ -702,11 +693,8 @@
assert(ADC->getCFGBuildOptions().AddImplicitDtors && "No CFG destructors");
(void)ADC;
- const CXXDestructorCall &Dtor = cast<CXXDestructorCall>(Call);
-
// FIXME: We don't handle constructors or destructors for arrays properly.
- const MemRegion *Target = Dtor.getCXXThisVal().getAsRegion();
- if (Target && isa<ElementRegion>(Target))
+ if (Flags.IsArrayConstructorOrDestructor)
return CIP_DisallowedOnce;
break;
@@ -847,7 +835,8 @@
}
bool ExprEngine::shouldInlineCall(const CallEvent &Call, const Decl *D,
- const ExplodedNode *Pred) {
+ const ExplodedNode *Pred,
+ EvalCallFlags Flags) {
if (!D)
return false;
@@ -894,7 +883,7 @@
// FIXME: this checks both static and dynamic properties of the call, which
// means we're redoing a bit of work that could be cached in the function
// summary.
- CallInlinePolicy CIP = mayInlineCallKind(Call, Pred, Opts);
+ CallInlinePolicy CIP = mayInlineCallKind(Call, Pred, Opts, Flags);
if (CIP != CIP_Allowed) {
if (CIP == CIP_DisallowedAlways) {
assert(!MayInline.hasValue() || MayInline.getValue());
@@ -946,7 +935,8 @@
}
void ExprEngine::defaultEvalCall(NodeBuilder &Bldr, ExplodedNode *Pred,
- const CallEvent &CallTemplate) {
+ const CallEvent &CallTemplate,
+ EvalCallFlags Flags) {
// Make sure we have the most recent state attached to the call.
ProgramStateRef State = Pred->getState();
CallEventRef<> Call = CallTemplate.cloneWithState(State);
@@ -969,7 +959,7 @@
} else {
RuntimeDefinition RD = Call->getRuntimeDefinition();
const Decl *D = RD.getDecl();
- if (shouldInlineCall(*Call, D, Pred)) {
+ if (shouldInlineCall(*Call, D, Pred, Flags)) {
if (RD.mayHaveOtherDefinitions()) {
AnalyzerOptions &Options = getAnalysisManager().options;
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -91,22 +91,24 @@
///
/// If the type is not an array type at all, the original value is returned.
static SVal makeZeroElementRegion(ProgramStateRef State, SVal LValue,
- QualType &Ty) {
+ QualType &Ty, bool &IsIndeedAnArray) {
SValBuilder &SVB = State->getStateManager().getSValBuilder();
ASTContext &Ctx = SVB.getContext();
while (const ArrayType *AT = Ctx.getAsArrayType(Ty)) {
Ty = AT->getElementType();
LValue = State->getLValue(Ty, SVB.makeZeroArrayIndex(), LValue);
+ IsIndeedAnArray = true;
}
return LValue;
}
const MemRegion *
ExprEngine::getRegionForConstructedObject(const CXXConstructExpr *CE,
- ExplodedNode *Pred) {
+ ExplodedNode *Pred,
+ EvalCallFlags &Flags) {
const LocationContext *LCtx = Pred->getLocationContext();
ProgramStateRef State = Pred->getState();
@@ -122,9 +124,9 @@
if (const SubRegion *MR = dyn_cast_or_null<SubRegion>(
getCXXNewAllocatorValue(State, CNE, LCtx).getAsRegion())) {
if (CNE->isArray()) {
- // TODO: This code exists only to trigger the suppression for
- // array constructors. In fact, we need to call the constructor
- // for every allocated element, not just the first one!
+ // TODO: In fact, we need to call the constructor for every
+ // allocated element, not just the first one!
+ Flags.IsArrayConstructorOrDestructor = true;
return getStoreManager().GetElementZeroRegion(
MR, CNE->getType()->getPointeeType());
}
@@ -136,7 +138,10 @@
if (Var->getInit() && Var->getInit()->IgnoreImplicit() == CE) {
SVal LValue = State->getLValue(Var, LCtx);
QualType Ty = Var->getType();
- LValue = makeZeroElementRegion(State, LValue, Ty);
+ bool IsArray = false;
+ LValue = makeZeroElementRegion(State, LValue, Ty, IsArray);
+ if (IsArray)
+ Flags.IsArrayConstructorOrDestructor = true;
return LValue.getAsRegion();
}
}
@@ -162,16 +167,20 @@
}
QualType Ty = Field->getType();
- FieldVal = makeZeroElementRegion(State, FieldVal, Ty);
+ bool IsArray = false;
+ FieldVal = makeZeroElementRegion(State, FieldVal, Ty, IsArray);
+ if (IsArray)
+ Flags.IsArrayConstructorOrDestructor = true;
return FieldVal.getAsRegion();
}
// FIXME: This will eventually need to handle new-expressions as well.
// Don't forget to update the pre-constructor initialization code in
// ExprEngine::VisitCXXConstructExpr.
}
// If we couldn't find an existing region to construct into, assume we're
- // constructing a temporary.
+ // constructing a temporary. Notify the caller of our failure.
+ Flags.IsConstructorWithImproperlyModeledTargetRegion = true;
MemRegionManager &MRMgr = getSValBuilder().getRegionManager();
return MRMgr.getCXXTempObjectRegion(CE, LCtx);
}
@@ -265,9 +274,11 @@
// For now, we just run the first constructor (which should still invalidate
// the entire array).
+ EvalCallFlags Flags{};
+
switch (CE->getConstructionKind()) {
case CXXConstructExpr::CK_Complete: {
- Target = getRegionForConstructedObject(CE, Pred);
+ Target = getRegionForConstructedObject(CE, Pred, Flags);
break;
}
case CXXConstructExpr::CK_VirtualBase:
@@ -304,6 +315,7 @@
if (dyn_cast_or_null<InitListExpr>(LCtx->getParentMap().getParent(CE))) {
MemRegionManager &MRMgr = getSValBuilder().getRegionManager();
Target = MRMgr.getCXXTempObjectRegion(CE, LCtx);
+ Flags.IsConstructorWithImproperlyModeledTargetRegion = true;
break;
}
// FALLTHROUGH
@@ -371,19 +383,18 @@
ExplodedNodeSet DstEvaluated;
StmtNodeBuilder Bldr(DstPreCall, DstEvaluated, *currBldrCtx);
- bool IsArray = isa<ElementRegion>(Target);
if (CE->getConstructor()->isTrivial() &&
CE->getConstructor()->isCopyOrMoveConstructor() &&
- !IsArray) {
+ !Flags.IsArrayConstructorOrDestructor) {
// FIXME: Handle other kinds of trivial constructors as well.
for (ExplodedNodeSet::iterator I = DstPreCall.begin(), E = DstPreCall.end();
I != E; ++I)
performTrivialCopy(Bldr, *I, *Call);
} else {
for (ExplodedNodeSet::iterator I = DstPreCall.begin(), E = DstPreCall.end();
I != E; ++I)
- defaultEvalCall(Bldr, *I, *Call);
+ defaultEvalCall(Bldr, *I, *Call, Flags);
}
// If the CFG was contructed without elements for temporary destructors
@@ -431,7 +442,14 @@
SVal DestVal = UnknownVal();
if (Dest)
DestVal = loc::MemRegionVal(Dest);
- DestVal = makeZeroElementRegion(State, DestVal, ObjectType);
+
+ EvalCallFlags Flags{};
+
+ bool IsArray = false;
+ DestVal = makeZeroElementRegion(State, DestVal, ObjectType, IsArray);
+ if (IsArray)
+ Flags.IsArrayConstructorOrDestructor = true;
+
Dest = DestVal.getAsRegion();
const CXXRecordDecl *RecordDecl = ObjectType->getAsCXXRecordDecl();
@@ -454,7 +472,7 @@
StmtNodeBuilder Bldr(DstPreCall, DstInvalidated, *currBldrCtx);
for (ExplodedNodeSet::iterator I = DstPreCall.begin(), E = DstPreCall.end();
I != E; ++I)
- defaultEvalCall(Bldr, *I, *Call);
+ defaultEvalCall(Bldr, *I, *Call, Flags);
ExplodedNodeSet DstPostCall;
getCheckerManager().runCheckersForPostCall(Dst, DstInvalidated,
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===================================================================
--- include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -567,14 +567,21 @@
const LocationContext *LCtx,
ProgramStateRef State);
+ struct EvalCallFlags {
+ bool IsConstructorWithImproperlyModeledTargetRegion : 1;
+ bool IsArrayConstructorOrDestructor : 1;
+ };
+
/// 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,
const CallEvent &Call);
/// \brief Default implementation of call evaluation.
void defaultEvalCall(NodeBuilder &B, ExplodedNode *Pred,
- const CallEvent &Call);
+ const CallEvent &Call,
+ EvalCallFlags Flags = {});
+
private:
void evalLoadCommon(ExplodedNodeSet &Dst,
const Expr *NodeEx, /* Eventually will be a CFGStmt */
@@ -600,7 +607,7 @@
/// Checks our policies and decides weither the given call should be inlined.
bool shouldInlineCall(const CallEvent &Call, const Decl *D,
- const ExplodedNode *Pred);
+ const ExplodedNode *Pred, EvalCallFlags Flags);
bool inlineCall(const CallEvent &Call, const Decl *D, NodeBuilder &Bldr,
ExplodedNode *Pred, ProgramStateRef State);
@@ -650,11 +657,11 @@
/// For a given constructor, look forward in the current CFG block to
/// determine the region into which an object will be constructed by \p CE.
- /// Returns either a field or local variable region if the object will be
- /// directly constructed in an existing region or a temporary object region
- /// if not.
+ /// When the lookahead fails, a temporary region is returned, and a flag is
+ /// set in \p Flags.
const MemRegion *getRegionForConstructedObject(const CXXConstructExpr *CE,
- ExplodedNode *Pred);
+ ExplodedNode *Pred,
+ EvalCallFlags &Flags);
/// Store the region returned by operator new() so that the constructor
/// that follows it knew what location to initialize. The value should be
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
