NoQ updated this revision to Diff 127202.
NoQ added a comment.
`VisitCXXNewExpr` is too late. We need to perform cast before calling the
constructor. Otherwise bad things happen, for instance `performTrivialCopy`
would construct another void region :)
Move the cast to `pushCXXNewAllocatorValue()`. This way we perform the cast
before putting this-value into our temporary storage (the top of
`CXXNewAllocatorValueStack`, or `_this` in terms of
http://lists.llvm.org/pipermail/cfe-dev/2017-December/056314.html), which seems
correct. And this affects all two code paths on which we exit the allocator
call - both the `conservativeEvalCall` path and the `processCallExit` path (and
ideally the future `evalCall` path).
https://reviews.llvm.org/D41250
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
lib/StaticAnalyzer/Core/ExprEngine.cpp
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
lib/StaticAnalyzer/Core/Store.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
@@ -27,3 +27,18 @@
// Check that bindings are correct (and also not dying).
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 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,12 @@
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}}
+}
Index: lib/StaticAnalyzer/Core/Store.cpp
===================================================================
--- lib/StaticAnalyzer/Core/Store.cpp
+++ lib/StaticAnalyzer/Core/Store.cpp
@@ -91,13 +91,21 @@
return R;
// Handle casts from compatible types.
- if (R->isBoundable())
+ if (R->isBoundable()) {
if (const TypedValueRegion *TR = dyn_cast<TypedValueRegion>(R)) {
QualType ObjTy = Ctx.getCanonicalType(TR->getValueType());
if (CanonPointeeTy == ObjTy)
return R;
}
+ if (const SymbolicRegion *SR = dyn_cast<SymbolicRegion>(R)) {
+ QualType SymTy =
+ Ctx.getCanonicalType(SR->getSymbol()->getType()->getPointeeType());
+ if (CanonPointeeTy == SymTy)
+ return R;
+ }
+ }
+
// Process region cast according to the kind of the region being cast.
switch (R->getKind()) {
case MemRegion::CXXThisRegionKind:
Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -277,11 +277,12 @@
state = state->BindExpr(CCE, callerCtx, ThisV);
}
- if (isa<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, state->getSVal(CE, callerCtx));
+ state = pushCXXNewAllocatorValue(state, state->getSVal(CE, callerCtx),
+ CNE->getType());
}
}
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -468,7 +468,8 @@
for (auto I: DstPostCall) {
ProgramStateRef State = I->getState();
ValueBldr.generateNode(
- CNE, I, pushCXXNewAllocatorValue(State, State->getSVal(CNE, LCtx)));
+ CNE, I, pushCXXNewAllocatorValue(State, State->getSVal(CNE, LCtx),
+ CNE->getType()));
}
getCheckerManager().runCheckersForPostCall(Dst, DstPostValue,
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -314,7 +314,12 @@
}
ProgramStateRef ExprEngine::pushCXXNewAllocatorValue(ProgramStateRef State,
- SVal V) {
+ SVal V, QualType T) {
+ // Because operator new returns void *, we need to perform the cast here,
+ // which is implied by the semantics of operator new.
+ V = svalBuilder.evalCast(V, T,
+ getContext().getPointerType(getContext().VoidTy));
+
return State->add<CXXNewAllocatorValueStack>(V);
}
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===================================================================
--- include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -659,8 +659,10 @@
/// 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,
- SVal V);
+ SVal V, QualType T);
/// Retrieve the location returned by the current operator new().
SVal peekCXXNewAllocatorValue(ProgramStateRef State);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
