NoQ updated this revision to Diff 129212.
NoQ added a comment.
In https://reviews.llvm.org/D41250#959755, @NoQ wrote:
> > I also noticed that `evalCast` from `void *` to `T *` is uncomfortable to
> > use because sometimes it transforms `&SymRegion{$x}` into `&element{T,
> > 0S32b, SymRegion{$x}}` even when `$x` is already of type `T *`. The form
> > `&SymRegion{$x}` seems to be the canonical form of this symbolic pointer
> > value in the rest of the analyzer, so i decided to change `evalCast` to
> > preserve it.
>
> Suddenly it turns out that this is not needed anymore. I'm struggling quite a
> bit to get the casts right, and still failing to identify the actual system
> we're trying to follow when representing pointer casts. I'd love to get to
> the bottom of it eventually.
Model the cast only around allocators that were inlined. Additionally, produce
array element for array `new[]` allocator calls. This completely reverts the
rather-accidental-than-intended change in behavior in the conservative case
which i described above: we no longer get this
> `&element{T, 0S32b, SymRegion{$x}}` even when `$x` is already of type `T *`
thing in the conservative case, while still behaving reasonably in the inlined
case, without touching any other behavior of the analyzer (i.e. not touching
the whole `evalCast` thing). So i believe this is the right thing to do, at
least for this patch.
Eventually it might be better to return `&element{T, 0S32b, SymRegion{$x}}`
where `$x` is a conjured `void` pointer - this would express the semantics and
the origins of that `SVal` better. However, we cannot do that, because we
cannot conjure a symbol without a void-pointer-type expression, and we don't
have the expression that represents the call site for the allocator call.
https://reviews.llvm.org/D41250
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
test/Analysis/new-ctor-conservative.cpp
test/Analysis/new-ctor-inlined.cpp
Index: test/Analysis/new-ctor-inlined.cpp
===================================================================
--- test/Analysis/new-ctor-inlined.cpp
+++ test/Analysis/new-ctor-inlined.cpp
@@ -38,3 +38,18 @@
Sp *p = new Sp(new Sp(0));
clang_analyzer_eval(p->p->p == 0); // expected-warning{{TRUE}}
}
+
+void checkNewPOD() {
+ int *i = new int;
+ clang_analyzer_eval(*i == 0); // expected-warning{{UNKNOWN}}
+ int *j = new int();
+ clang_analyzer_eval(*j == 0); // expected-warning{{TRUE}}
+ int *k = new int(5);
+ clang_analyzer_eval(*k == 5); // expected-warning{{TRUE}}
+}
+
+void checkTrivialCopy() {
+ S s;
+ S *t = new S(s); // no-crash
+ clang_analyzer_eval(t->x == 1); // expected-warning{{TRUE}}
+}
Index: test/Analysis/new-ctor-conservative.cpp
===================================================================
--- test/Analysis/new-ctor-conservative.cpp
+++ test/Analysis/new-ctor-conservative.cpp
@@ -12,3 +12,18 @@
S *s = new S;
clang_analyzer_eval(s->x == 1); // expected-warning{{TRUE}}
}
+
+void checkNewPOD() {
+ int *i = new int;
+ clang_analyzer_eval(*i == 0); // expected-warning{{UNKNOWN}}
+ int *j = new int();
+ clang_analyzer_eval(*j == 0); // expected-warning{{TRUE}}
+ int *k = new int(5);
+ clang_analyzer_eval(*k == 5); // expected-warning{{TRUE}}
+}
+
+void checkNewArray() {
+ S *s = new S[10];
+ // FIXME: Should be true once we inline array constructors.
+ clang_analyzer_eval(s[0].x == 1); // expected-warning{{UNKNOWN}}
+}
Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -277,12 +277,18 @@
state = state->BindExpr(CCE, callerCtx, ThisV);
}
- if (const CXXNewExpr *CNE = dyn_cast<CXXNewExpr>(CE)) {
+ 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.
- state = pushCXXNewAllocatorValue(state, CNE, calleeCtx->getParent(),
- state->getSVal(CE, callerCtx));
+ // 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 = pushCXXNewAllocatorValue(state, CNE, calleeCtx->getParent(), AllocV);
}
}
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -119,9 +119,17 @@
if (AMgr.getAnalyzerOptions().mayInlineCXXAllocator()) {
// TODO: Detect when the allocator returns a null pointer.
// Constructor shall not be called in this case.
- if (const MemRegion *MR =
- peekCXXNewAllocatorValue(State, CNE, LCtx).getAsRegion())
+ if (const SubRegion *MR = dyn_cast_or_null<SubRegion>(
+ peekCXXNewAllocatorValue(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!
+ return getStoreManager().GetElementZeroRegion(
+ MR, CNE->getType()->getPointeeType());
+ }
return MR;
+ }
}
} else if (auto *DS = dyn_cast<DeclStmt>(StmtElem->getStmt())) {
if (const auto *Var = dyn_cast<VarDecl>(DS->getSingleDecl())) {
@@ -564,25 +572,28 @@
StmtNodeBuilder Bldr(Pred, Dst, *currBldrCtx);
+ SVal Result = symVal;
+
if (CNE->isArray()) {
// FIXME: allocating an array requires simulating the constructors.
// For now, just return a symbolicated region.
- const SubRegion *NewReg =
- symVal.castAs<loc::MemRegionVal>().getRegionAs<SubRegion>();
- QualType ObjTy = CNE->getType()->getAs<PointerType>()->getPointeeType();
- const ElementRegion *EleReg =
- getStoreManager().GetElementZeroRegion(NewReg, ObjTy);
- State = State->BindExpr(CNE, Pred->getLocationContext(),
- loc::MemRegionVal(EleReg));
+ if (!AMgr.getAnalyzerOptions().mayInlineCXXAllocator()) {
+ const SubRegion *NewReg =
+ symVal.castAs<loc::MemRegionVal>().getRegionAs<SubRegion>();
+ QualType ObjTy = CNE->getType()->getAs<PointerType>()->getPointeeType();
+ const ElementRegion *EleReg =
+ getStoreManager().GetElementZeroRegion(NewReg, ObjTy);
+ Result = loc::MemRegionVal(EleReg);
+ }
+ State = State->BindExpr(CNE, Pred->getLocationContext(), Result);
Bldr.generateNode(CNE, Pred, State);
return;
}
// FIXME: Once we have proper support for CXXConstructExprs inside
// CXXNewExpr, we need to make sure that the constructed object is not
// immediately invalidated here. (The placement call should happen before
// the constructor call anyway.)
- SVal Result = symVal;
if (FD && FD->isReservedGlobalPlacementOperator()) {
// Non-array placement new should always return the placement location.
SVal PlacementLoc = State->getSVal(CNE->getPlacementArg(0), LCtx);
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===================================================================
--- include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -659,6 +659,8 @@
/// Store the region returned by operator new() so that the constructor
/// that follows it knew what location to initialize. The value should be
/// cleared once the respective CXXNewExpr CFGStmt element is processed.
+ /// \p T is the type of the newly allocated object, which is not necessarily
+ /// obvious from \p V because the return type of operator new is void *.
ProgramStateRef pushCXXNewAllocatorValue(ProgramStateRef State,
const CXXNewExpr *CNE,
const LocationContext *CallerLC,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
