[PATCH] D41250: [analyzer] Model implied cast around operator new().

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC322777: [analyzer] operator new: Model the cast of returned 
pointer into object type. (authored by dergachev, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D41250

Files:
  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/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 =
-  getCXXNewAllocatorValue(State, CNE, LCtx).getAsRegion())
+  if (const SubRegion *MR = dyn_cast_or_null(
+  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!
+  return getStoreManager().GetElementZeroRegion(
+  MR, CNE->getType()->getPointeeType());
+}
 return MR;
+  }
 }
   } else if (auto *DS = dyn_cast(StmtElem->getStmt())) {
 if (const auto *Var = dyn_cast(DS->getSingleDecl())) {
@@ -473,12 +481,22 @@
   StmtNodeBuilder CallBldr(DstPreCall, DstPostCall, *currBldrCtx);
   for (auto I : DstPreCall)
 defaultEvalCall(CallBldr, I, *Call);
+  // If the call is inlined, DstPostCall will be empty and we bail out now.
 
   // Store return value of operator new() for future use, until the actual
   // CXXNewExpr gets processed.
   ExplodedNodeSet DstPostValue;
   StmtNodeBuilder ValueBldr(DstPostCall, DstPostValue, *currBldrCtx);
   for (auto I : DstPostCall) {
+// FIXME: Because CNE serves as the "call site" for the allocator (due to
+// lack of a better expression in the AST), the conjured return value symbol
+// is going to be of the same type (C++ object pointer type). Technically
+// this is not correct because the operator new's prototype always says that
+// it returns a 'void *'. So we should change the type of the symbol,
+// and then evaluate the cast over the symbolic pointer from 'void *' to
+// the object pointer type. But without changing the symbol's type it
+// is breaking too much to evaluate the no-op symbolic cast over it, so we
+// skip it for now.
 ProgramStateRef State = I->getState();
 ValueBldr.generateNode(
 CNE, I,
@@ -564,25 +582,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().getRegionAs();
-QualType ObjTy = CNE->getType()->getAs()->getPointeeType();
-const ElementRegion *EleReg =
-  getStoreManager().GetElementZeroRegion(NewReg, ObjTy);
-State = State->BindExpr(CNE, Pred->getLocationContext(),
-

[PATCH] D41250: [analyzer] Model implied cast around operator new().

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin accepted this revision.
dcoughlin added a comment.

This looks good to me, as well.


https://reviews.llvm.org/D41250



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41250: [analyzer] Model implied cast around operator new().

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 129365.
NoQ added a comment.

Rebase.

Add a FIXME to bring back the cast in the conservative case.


https://reviews.llvm.org/D41250

Files:
  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,19 @@
   state = state->BindExpr(CCE, callerCtx, ThisV);
 }
 
-if (const CXXNewExpr *CNE = dyn_cast(CE)) {
+if (const auto *CNE = dyn_cast(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 = setCXXNewAllocatorValue(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 =
+  setCXXNewAllocatorValue(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 =
-  getCXXNewAllocatorValue(State, CNE, LCtx).getAsRegion())
+  if (const SubRegion *MR = dyn_cast_or_null(
+  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!
+  return getStoreManager().GetElementZeroRegion(
+  MR, CNE->getType()->getPointeeType());
+}
 return MR;
+  }
 }
   } else if (auto *DS = dyn_cast(StmtElem->getStmt())) {
 if (const auto *Var = dyn_cast(DS->getSingleDecl())) {
@@ -473,12 +481,22 @@
   StmtNodeBuilder CallBldr(DstPreCall, DstPostCall, *currBldrCtx);
   for (auto I : DstPreCall)
 defaultEvalCall(CallBldr, I, *Call);
+  // If the call is inlined, DstPostCall will be empty and we bail out now.
 
   // Store return value of operator new() for future use, until the actual
   // CXXNewExpr gets processed.
   ExplodedNodeSet DstPostValue;
   StmtNodeBuilder ValueBldr(DstPostCall, DstPostValue, *currBldrCtx);
   for (auto I : DstPostCall) {
+// FIXME: Because CNE serves as the "call site" for the allocator (due to
+// lack of a better expression in the AST), the conjured return value symbol
+// is going to be of the same type (C++ object pointer type). 

[PATCH] D41250: [analyzer] Model implied cast around operator new().

[Artem Dergachev via Phabricator via cfe-commits]

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 `{$x}` into `{T, 
> > 0S32b, SymRegion{$x}}` even when `$x` is already of type `T *`. The form 
> > `{$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

> `{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 `{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(CE)) {
+if (const auto *CNE = dyn_cast(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 

[PATCH] D41250: [analyzer] Model implied cast around operator new().

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 127549.
NoQ added a comment.

Rebase.

> I also noticed that `evalCast` from `void *` to `T *` is uncomfortable to use 
> because sometimes it transforms `{$x}` into `{T, 0S32b, 
> SymRegion{$x}}` even when `$x` is already of type `T *`. The form 
> `{$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.


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
  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/ExprEngineCallAndReturn.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -277,11 +277,12 @@
   state = state->BindExpr(CCE, callerCtx, ThisV);
 }
 
-if (isa(CE)) {
+if (const auto *CNE = dyn_cast(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
@@ -463,7 +463,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(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 

[PATCH] D41250: [analyzer] Model implied cast around operator new().

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added a comment.
This revision is now accepted and ready to land.

The code looks good to me. But the best way to verify these kinds of changes to 
see how the results change on large projects after applying the patch.


https://reviews.llvm.org/D41250



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41250: [analyzer] Model implied cast around operator new().

[Artem Dergachev via Phabricator via cfe-commits]

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(R)) {
   QualType ObjTy = Ctx.getCanonicalType(TR->getValueType());
   if (CanonPointeeTy == ObjTy)
 return R;
 }
 
+if (const SymbolicRegion *SR = dyn_cast(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(CE)) {
+if (const auto *CNE = dyn_cast(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 

[PATCH] D41250: [analyzer] Model implied cast around operator new().

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet.
Herald added subscribers: cfe-commits, rnkovacs, eraman.

C++ overridable `operator new()` has the following prototype:

  void *operator new(size_t size, user-defined arguments...);

The return value is `void *`. However, before passing it to constructor, we 
need to make a cast to the respective object pointer type. Hence an implicit 
cast is present here, which is not represented in the current AST or CFG. 
Modeling this cast is straightforward though. This is the change i mentioned in 
https://reviews.llvm.org/D40939.

I also noticed that `evalCast` from `void *` to `T *` is uncomfortable to use 
because sometimes it transforms `{$x}` into `{T, 0S32b, 
SymRegion{$x}}` even when `$x` is already of type `T *`. The form 
`{$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.

The problem of how to represent memregion value casts better still stands - it 
wouldn't add much to the analyzer's quality, but we just keep running into it 
over and over again.


Repository:
  rC Clang

https://reviews.llvm.org/D41250

Files:
  lib/StaticAnalyzer/Core/ExprEngineCXX.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,12 @@
   // 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}}
+}
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(R)) {
   QualType ObjTy = Ctx.getCanonicalType(TR->getValueType());
   if (CanonPointeeTy == ObjTy)
 return R;
 }
 
+if (const SymbolicRegion *SR = dyn_cast(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/ExprEngineCXX.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -571,6 +571,10 @@
 SVal PlacementLoc = State->getSVal(CNE->getPlacementArg(0), LCtx);
 Result = svalBuilder.evalCast(PlacementLoc, CNE->getType(),
   CNE->getPlacementArg(0)->getType());
+  } else {
+Result =
+svalBuilder.evalCast(Result, CNE->getType(),
+ getContext().getPointerType(getContext().VoidTy));
   }
 
   // Bind the address of the object, then check to see if we cached out.


Index: test/Analysis/new-ctor-inlined.cpp
===
--- test/Analysis/new-ctor-inlined.cpp
+++ test/Analysis/new-ctor-inlined.cpp
@@ -27,3 +27,12 @@
   // 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}}
+}
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;