[PATCH] D41266: [analyzer] With c++-allocator-inlining, fix memory space for operator new pointers.

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC322780: [analyzer] operator new: Fix memory space for the 
returned region. (authored by dergachev, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D41266

Files:
  lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  test/Analysis/NewDelete-checker-test.cpp
  test/Analysis/new-ctor-null.cpp

Index: test/Analysis/NewDelete-checker-test.cpp
===
--- test/Analysis/NewDelete-checker-test.cpp
+++ test/Analysis/NewDelete-checker-test.cpp
@@ -1,5 +1,7 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete -std=c++11 -fblocks -verify %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDeleteLeaks -DLEAKS -std=c++11 -fblocks -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete -std=c++11 -fblocks -analyzer-config c++-allocator-inlining=true -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDeleteLeaks -DLEAKS -std=c++11 -fblocks -analyzer-config c++-allocator-inlining=true -verify %s
 #include "Inputs/system-header-simulator-cxx.h"
 
 typedef __typeof__(sizeof(int)) size_t;
Index: test/Analysis/new-ctor-null.cpp
===
--- test/Analysis/new-ctor-null.cpp
+++ test/Analysis/new-ctor-null.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+
+void clang_analyzer_eval(bool);
+
+typedef __typeof__(sizeof(int)) size_t;
+
+void *operator new(size_t size) throw() {
+  return nullptr;
+}
+void *operator new[](size_t size) throw() {
+  return nullptr;
+}
+
+struct S {
+  int x;
+  S() : x(1) {}
+  ~S() {}
+};
+
+void testArrays() {
+  S *s = new S[10]; // no-crash
+  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+}
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -479,8 +479,10 @@
 
   ExplodedNodeSet DstPostCall;
   StmtNodeBuilder CallBldr(DstPreCall, DstPostCall, *currBldrCtx);
-  for (auto I : DstPreCall)
+  for (auto I : DstPreCall) {
+// FIXME: Provide evalCall for checkers?
 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
@@ -507,7 +509,6 @@
  *Call, *this);
 }
 
-
 void ExprEngine::VisitCXXNewExpr(const CXXNewExpr *CNE, ExplodedNode *Pred,
ExplodedNodeSet ) {
   // FIXME: Much of this should eventually migrate to CXXAllocatorCall.
@@ -520,18 +521,8 @@
   SVal symVal = UnknownVal();
   FunctionDecl *FD = CNE->getOperatorNew();
 
-  bool IsStandardGlobalOpNewFunction = false;
-  if (FD && !isa(FD) && !FD->isVariadic()) {
-if (FD->getNumParams() == 2) {
-  QualType T = FD->getParamDecl(1)->getType();
-  if (const IdentifierInfo *II = T.getBaseTypeIdentifier())
-// NoThrow placement new behaves as a standard new.
-IsStandardGlobalOpNewFunction = II->getName().equals("nothrow_t");
-}
-else
-  // Placement forms are considered non-standard.
-  IsStandardGlobalOpNewFunction = (FD->getNumParams() == 1);
-  }
+  bool IsStandardGlobalOpNewFunction =
+  FD->isReplaceableGlobalAllocationFunction();
 
   ProgramStateRef State = Pred->getState();
 
@@ -587,9 +578,8 @@
   if (CNE->isArray()) {
 // FIXME: allocating an array requires simulating the constructors.
 // For now, just return a symbolicated region.
-if (!AMgr.getAnalyzerOptions().mayInlineCXXAllocator()) {
-  const SubRegion *NewReg =
-  symVal.castAs().getRegionAs();
+if (const SubRegion *NewReg =
+dyn_cast_or_null(symVal.getAsRegion())) {
   QualType ObjTy = CNE->getType()->getAs()->getPointeeType();
   const ElementRegion *EleReg =
   getStoreManager().GetElementZeroRegion(NewReg, ObjTy);
Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -573,7 +573,19 @@
   QualType ResultTy = Call.getResultType();
   SValBuilder  = getSValBuilder();
   unsigned Count = currBldrCtx->blockCount();
-  SVal R = SVB.conjureSymbolVal(nullptr, E, LCtx, ResultTy, Count);
+
+  // See if we need to conjure a heap pointer instead of
+  // a regular unknown pointer.
+  bool IsHeapPointer = false;
+  if (const auto *CNE = dyn_cast(E))
+if (CNE->getOperatorNew()->isReplaceableGlobalAllocationFunction()) {
+  // FIXME: 

[PATCH] D41266: [analyzer] With c++-allocator-inlining, fix memory space for operator new pointers.

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin accepted this revision.
dcoughlin added a comment.

LGTM as well.


https://reviews.llvm.org/D41266



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

[PATCH] D41266: [analyzer] With c++-allocator-inlining, fix memory space for operator new pointers.

[Artem Dergachev via Phabricator via cfe-commits]

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

Rebase (fix minor conflict).


https://reviews.llvm.org/D41266

Files:
  lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  test/Analysis/NewDelete-checker-test.cpp
  test/Analysis/new-ctor-null.cpp

Index: test/Analysis/new-ctor-null.cpp
===
--- /dev/null
+++ test/Analysis/new-ctor-null.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+
+void clang_analyzer_eval(bool);
+
+typedef __typeof__(sizeof(int)) size_t;
+
+void *operator new(size_t size) throw() {
+  return nullptr;
+}
+void *operator new[](size_t size) throw() {
+  return nullptr;
+}
+
+struct S {
+  int x;
+  S() : x(1) {}
+  ~S() {}
+};
+
+void testArrays() {
+  S *s = new S[10]; // no-crash
+  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+}
Index: test/Analysis/NewDelete-checker-test.cpp
===
--- test/Analysis/NewDelete-checker-test.cpp
+++ test/Analysis/NewDelete-checker-test.cpp
@@ -1,5 +1,7 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete -std=c++11 -fblocks -verify %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDeleteLeaks -DLEAKS -std=c++11 -fblocks -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete -std=c++11 -fblocks -analyzer-config c++-allocator-inlining=true -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDeleteLeaks -DLEAKS -std=c++11 -fblocks -analyzer-config c++-allocator-inlining=true -verify %s
 #include "Inputs/system-header-simulator-cxx.h"
 
 typedef __typeof__(sizeof(int)) size_t;
Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -569,7 +569,19 @@
   QualType ResultTy = Call.getResultType();
   SValBuilder  = getSValBuilder();
   unsigned Count = currBldrCtx->blockCount();
-  SVal R = SVB.conjureSymbolVal(nullptr, E, LCtx, ResultTy, Count);
+
+  // See if we need to conjure a heap pointer instead of
+  // a regular unknown pointer.
+  bool IsHeapPointer = false;
+  if (const auto *CNE = dyn_cast(E))
+if (CNE->getOperatorNew()->isReplaceableGlobalAllocationFunction()) {
+  // FIXME: Delegate this to evalCall in MallocChecker?
+  IsHeapPointer = true;
+}
+
+  SVal R = IsHeapPointer
+   ? SVB.getConjuredHeapSymbolVal(E, LCtx, Count)
+   : SVB.conjureSymbolVal(nullptr, E, LCtx, ResultTy, Count);
   return State->BindExpr(E, LCtx, R);
 }
 
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -479,8 +479,10 @@
 
   ExplodedNodeSet DstPostCall;
   StmtNodeBuilder CallBldr(DstPreCall, DstPostCall, *currBldrCtx);
-  for (auto I : DstPreCall)
+  for (auto I : DstPreCall) {
+// FIXME: Provide evalCall for checkers?
 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
@@ -507,7 +509,6 @@
  *Call, *this);
 }
 
-
 void ExprEngine::VisitCXXNewExpr(const CXXNewExpr *CNE, ExplodedNode *Pred,
ExplodedNodeSet ) {
   // FIXME: Much of this should eventually migrate to CXXAllocatorCall.
@@ -520,18 +521,8 @@
   SVal symVal = UnknownVal();
   FunctionDecl *FD = CNE->getOperatorNew();
 
-  bool IsStandardGlobalOpNewFunction = false;
-  if (FD && !isa(FD) && !FD->isVariadic()) {
-if (FD->getNumParams() == 2) {
-  QualType T = FD->getParamDecl(1)->getType();
-  if (const IdentifierInfo *II = T.getBaseTypeIdentifier())
-// NoThrow placement new behaves as a standard new.
-IsStandardGlobalOpNewFunction = II->getName().equals("nothrow_t");
-}
-else
-  // Placement forms are considered non-standard.
-  IsStandardGlobalOpNewFunction = (FD->getNumParams() == 1);
-  }
+  bool IsStandardGlobalOpNewFunction =
+  FD->isReplaceableGlobalAllocationFunction();
 
   ProgramStateRef State = Pred->getState();
 
@@ -587,9 +578,8 @@
   if (CNE->isArray()) {
 // FIXME: allocating an array requires simulating the constructors.
 // For now, just return a symbolicated region.
-if (!AMgr.getAnalyzerOptions().mayInlineCXXAllocator()) {
-  const SubRegion *NewReg =
-  symVal.castAs().getRegionAs();
+if (const SubRegion *NewReg =
+dyn_cast_or_null(symVal.getAsRegion())) {
   QualType ObjTy = 

[PATCH] D41266: [analyzer] With c++-allocator-inlining, fix memory space for operator new pointers.

[Artem Dergachev via Phabricator via cfe-commits]

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

Rebase. Address the comment.

In https://reviews.llvm.org/D41266#959763, @NoQ wrote:

> Remove the redundant cast that is done in `c++-allocator-inlining` mode when 
> modeling array new. After rebase it started causing two identical element 
> regions top appear on top of each other.


Remove this workaround because i reverted the change for which it is a 
workaround in https://reviews.llvm.org/D41250#971888: we no longer add the 
redundant `ElementRegion`, so we don't need to defend from double 
`ElementRegion`s here. I guess it all finally starts to make sense.


https://reviews.llvm.org/D41266

Files:
  lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  test/Analysis/NewDelete-checker-test.cpp
  test/Analysis/new-ctor-null.cpp

Index: test/Analysis/new-ctor-null.cpp
===
--- /dev/null
+++ test/Analysis/new-ctor-null.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+
+void clang_analyzer_eval(bool);
+
+typedef __typeof__(sizeof(int)) size_t;
+
+void *operator new(size_t size) throw() {
+  return nullptr;
+}
+void *operator new[](size_t size) throw() {
+  return nullptr;
+}
+
+struct S {
+  int x;
+  S() : x(1) {}
+  ~S() {}
+};
+
+void testArrays() {
+  S *s = new S[10]; // no-crash
+  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+}
Index: test/Analysis/NewDelete-checker-test.cpp
===
--- test/Analysis/NewDelete-checker-test.cpp
+++ test/Analysis/NewDelete-checker-test.cpp
@@ -1,5 +1,7 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete -std=c++11 -fblocks -verify %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDeleteLeaks -DLEAKS -std=c++11 -fblocks -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete -std=c++11 -fblocks -analyzer-config c++-allocator-inlining=true -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDeleteLeaks -DLEAKS -std=c++11 -fblocks -analyzer-config c++-allocator-inlining=true -verify %s
 #include "Inputs/system-header-simulator-cxx.h"
 
 typedef __typeof__(sizeof(int)) size_t;
Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -568,7 +568,19 @@
   QualType ResultTy = Call.getResultType();
   SValBuilder  = getSValBuilder();
   unsigned Count = currBldrCtx->blockCount();
-  SVal R = SVB.conjureSymbolVal(nullptr, E, LCtx, ResultTy, Count);
+
+  // See if we need to conjure a heap pointer instead of
+  // a regular unknown pointer.
+  bool IsHeapPointer = false;
+  if (const auto *CNE = dyn_cast(E))
+if (CNE->getOperatorNew()->isReplaceableGlobalAllocationFunction()) {
+  // FIXME: Delegate this to evalCall in MallocChecker?
+  IsHeapPointer = true;
+}
+
+  SVal R = IsHeapPointer
+   ? SVB.getConjuredHeapSymbolVal(E, LCtx, Count)
+   : SVB.conjureSymbolVal(nullptr, E, LCtx, ResultTy, Count);
   return State->BindExpr(E, LCtx, R);
 }
 
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -479,8 +479,10 @@
 
   ExplodedNodeSet DstPostCall;
   StmtNodeBuilder CallBldr(DstPreCall, DstPostCall, *currBldrCtx);
-  for (auto I : DstPreCall)
+  for (auto I : DstPreCall) {
+// FIXME: Provide evalCall for checkers?
 defaultEvalCall(CallBldr, I, *Call);
+  }
 
   // Store return value of operator new() for future use, until the actual
   // CXXNewExpr gets processed.
@@ -497,7 +499,6 @@
  *Call, *this);
 }
 
-
 void ExprEngine::VisitCXXNewExpr(const CXXNewExpr *CNE, ExplodedNode *Pred,
ExplodedNodeSet ) {
   // FIXME: Much of this should eventually migrate to CXXAllocatorCall.
@@ -510,18 +511,8 @@
   SVal symVal = UnknownVal();
   FunctionDecl *FD = CNE->getOperatorNew();
 
-  bool IsStandardGlobalOpNewFunction = false;
-  if (FD && !isa(FD) && !FD->isVariadic()) {
-if (FD->getNumParams() == 2) {
-  QualType T = FD->getParamDecl(1)->getType();
-  if (const IdentifierInfo *II = T.getBaseTypeIdentifier())
-// NoThrow placement new behaves as a standard new.
-IsStandardGlobalOpNewFunction = II->getName().equals("nothrow_t");
-}
-else
-  // Placement forms are considered non-standard.
-  IsStandardGlobalOpNewFunction = (FD->getNumParams() == 1);
-  }
+  bool IsStandardGlobalOpNewFunction =
+  

[PATCH] D41266: [analyzer] With c++-allocator-inlining, fix memory space for operator new pointers.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:477
+bool ExprEngine::isStandardGlobalOperatorNewFunction(const CXXNewExpr *CNE) {
+  const FunctionDecl *FD = CNE->getOperatorNew();
+  if (FD && !isa(FD) && !FD->isVariadic()) {

dcoughlin wrote:
> I realize this isn't your code, but could we use 
> `FunctionDecl::isReplaceableGlobalAllocationFunction() here?`
Hmm. Totally. That's much better.


https://reviews.llvm.org/D41266



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

[PATCH] D41266: [analyzer] With c++-allocator-inlining, fix memory space for operator new pointers.

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin added inline comments.



Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:477
+bool ExprEngine::isStandardGlobalOperatorNewFunction(const CXXNewExpr *CNE) {
+  const FunctionDecl *FD = CNE->getOperatorNew();
+  if (FD && !isa(FD) && !FD->isVariadic()) {

I realize this isn't your code, but could we use 
`FunctionDecl::isReplaceableGlobalAllocationFunction() here?`


https://reviews.llvm.org/D41266



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

[PATCH] D41266: [analyzer] With c++-allocator-inlining, fix memory space for operator new pointers.

[Artem Dergachev via Phabricator via cfe-commits]

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

Rebase.

Remove the redundant cast that is done in `c++-allocator-inlining` mode when 
modeling array new. After rebase it started causing two identical element 
regions top appear on top of each other.


https://reviews.llvm.org/D41266

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  test/Analysis/NewDelete-checker-test.cpp
  test/Analysis/new-ctor-conservative.cpp
  test/Analysis/new-ctor-null.cpp

Index: test/Analysis/new-ctor-null.cpp
===
--- /dev/null
+++ test/Analysis/new-ctor-null.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+
+void clang_analyzer_eval(bool);
+
+typedef __typeof__(sizeof(int)) size_t;
+
+void *operator new(size_t size) throw() {
+  return nullptr;
+}
+void *operator new[](size_t size) throw() {
+  return nullptr;
+}
+
+struct S {
+  int x;
+  S() : x(1) {}
+  ~S() {}
+};
+
+void testArrays() {
+  S *s = new S[10]; // no-crash
+  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+}
Index: test/Analysis/new-ctor-conservative.cpp
===
--- test/Analysis/new-ctor-conservative.cpp
+++ test/Analysis/new-ctor-conservative.cpp
@@ -21,3 +21,9 @@
   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: test/Analysis/NewDelete-checker-test.cpp
===
--- test/Analysis/NewDelete-checker-test.cpp
+++ test/Analysis/NewDelete-checker-test.cpp
@@ -1,5 +1,7 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete -std=c++11 -fblocks -verify %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDeleteLeaks -DLEAKS -std=c++11 -fblocks -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete -std=c++11 -fblocks -analyzer-config c++-allocator-inlining=true -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDeleteLeaks -DLEAKS -std=c++11 -fblocks -analyzer-config c++-allocator-inlining=true -verify %s
 #include "Inputs/system-header-simulator-cxx.h"
 
 typedef __typeof__(sizeof(int)) size_t;
Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -562,7 +562,19 @@
   QualType ResultTy = Call.getResultType();
   SValBuilder  = getSValBuilder();
   unsigned Count = currBldrCtx->blockCount();
-  SVal R = SVB.conjureSymbolVal(nullptr, E, LCtx, ResultTy, Count);
+
+  // See if we need to conjure a heap pointer instead of
+  // a regular unknown pointer.
+  bool IsHeapPointer = false;
+  if (const auto *CNE = dyn_cast(E))
+if (isStandardGlobalOperatorNewFunction(CNE)) {
+  // FIXME: Delegate this to evalCall in MallocChecker?
+  IsHeapPointer = true;
+}
+
+  SVal R = IsHeapPointer
+   ? SVB.getConjuredHeapSymbolVal(E, LCtx, Count)
+   : SVB.conjureSymbolVal(nullptr, E, LCtx, ResultTy, Count);
   return State->BindExpr(E, LCtx, R);
 }
 
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -453,8 +453,10 @@
 
   ExplodedNodeSet DstPostCall;
   StmtNodeBuilder CallBldr(DstPreCall, DstPostCall, *currBldrCtx);
-  for (auto I : DstPreCall)
+  for (auto I : DstPreCall) {
+// FIXME: Provide evalCall for checkers?
 defaultEvalCall(CallBldr, I, *Call);
+  }
 
   // Store return value of operator new() for future use, until the actual
   // CXXNewExpr gets processed.
@@ -471,6 +473,22 @@
  *Call, *this);
 }
 
+bool ExprEngine::isStandardGlobalOperatorNewFunction(const CXXNewExpr *CNE) {
+  const FunctionDecl *FD = CNE->getOperatorNew();
+  if (FD && !isa(FD) && !FD->isVariadic()) {
+if (FD->getNumParams() == 2) {
+  QualType T = FD->getParamDecl(1)->getType();
+  if (const IdentifierInfo *II = T.getBaseTypeIdentifier()) {
+// NoThrow placement new behaves as a standard new.
+return II->getName().equals("nothrow_t");
+  }
+} else {
+  // Placement forms are considered non-standard.
+  return FD->getNumParams() == 1;
+}
+  }
+  return false;
+}
 
 void ExprEngine::VisitCXXNewExpr(const CXXNewExpr *CNE, ExplodedNode *Pred,

[PATCH] D41266: [analyzer] With c++-allocator-inlining, fix memory space for operator new pointers.

[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.

In the future, we might want to model the standard placement new and return a 
symbol with the correct SpaceRegion (i.e.: the space region of the argument).




Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:491
+  // Placement forms are considered non-standard.
+  return (FD->getNumParams() == 1);
+}

The parens are redundant here.


Repository:
  rC Clang

https://reviews.llvm.org/D41266



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

[PATCH] D41266: [analyzer] With c++-allocator-inlining, fix memory space for operator new pointers.

[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.

Default global `operator new()`, like `malloc()`, should return heap pointers, 
which in the analyzer are represented by `SymbolicRegion`s with 
`HeapSpaceRegion` as their parent.

In the `-analyzer-config c++-allocator-inlining` mode, this was broken, and 
regular `SymbolicRegion`s were returned instead,  which have 
`UnknownSpaceRegion` as their parent.

This patch fixes this straightforwardly on `ExprEngine` side. We may want to 
delegate this job to the checkers though `evalCall`, but for now this mode 
doesn't support `evalCall` for `operator new()`, and i'm not sure if it'd be 
used much.

With this patch going on top of previous patches, enabling 
`c++-allocator-inlining` by default causes no regressions on tests (causes some 
improvements though). It doesn't mean it works (we still have callbacks broken, 
path diagnostic pieces unsupported, and i've just noticed one more void element 
region crash), just a psychological checkpoint.


Repository:
  rC Clang

https://reviews.llvm.org/D41266

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  test/Analysis/NewDelete-checker-test.cpp

Index: test/Analysis/NewDelete-checker-test.cpp
===
--- test/Analysis/NewDelete-checker-test.cpp
+++ test/Analysis/NewDelete-checker-test.cpp
@@ -1,5 +1,7 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete -std=c++11 -fblocks -verify %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDeleteLeaks -DLEAKS -std=c++11 -fblocks -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete -std=c++11 -fblocks -analyzer-config c++-allocator-inlining=true -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDeleteLeaks -DLEAKS -std=c++11 -fblocks -analyzer-config c++-allocator-inlining=true -verify %s
 #include "Inputs/system-header-simulator-cxx.h"
 
 typedef __typeof__(sizeof(int)) size_t;
Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -561,7 +561,19 @@
   QualType ResultTy = Call.getResultType();
   SValBuilder  = getSValBuilder();
   unsigned Count = currBldrCtx->blockCount();
-  SVal R = SVB.conjureSymbolVal(nullptr, E, LCtx, ResultTy, Count);
+
+  // See if we need to conjure a heap pointer instead of
+  // a regular unknown pointer.
+  bool IsHeapPointer = false;
+  if (const auto *CNE = dyn_cast(E))
+if (isStandardGlobalOperatorNewFunction(CNE)) {
+  // FIXME: Delegate this to evalCall in MallocChecker?
+  IsHeapPointer = true;
+}
+
+  SVal R = IsHeapPointer
+   ? SVB.getConjuredHeapSymbolVal(E, LCtx, Count)
+   : SVB.conjureSymbolVal(nullptr, E, LCtx, ResultTy, Count);
   return State->BindExpr(E, LCtx, R);
 }
 
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -458,8 +458,10 @@
 
   ExplodedNodeSet DstPostCall;
   StmtNodeBuilder CallBldr(DstPreCall, DstPostCall, *currBldrCtx);
-  for (auto I: DstPreCall)
+  for (auto I: DstPreCall) {
+// FIXME: Provide evalCall for checkers?
 defaultEvalCall(CallBldr, I, *Call);
+  }
 
   // Store return value of operator new() for future use, until the actual
   // CXXNewExpr gets processed.
@@ -475,6 +477,22 @@
  *Call, *this);
 }
 
+bool ExprEngine::isStandardGlobalOperatorNewFunction(const CXXNewExpr *CNE) {
+  const FunctionDecl *FD = CNE->getOperatorNew();
+  if (FD && !isa(FD) && !FD->isVariadic()) {
+if (FD->getNumParams() == 2) {
+  QualType T = FD->getParamDecl(1)->getType();
+  if (const IdentifierInfo *II = T.getBaseTypeIdentifier()) {
+// NoThrow placement new behaves as a standard new.
+return II->getName().equals("nothrow_t");
+  }
+} else {
+  // Placement forms are considered non-standard.
+  return (FD->getNumParams() == 1);
+}
+  }
+  return false;
+}
 
 void ExprEngine::VisitCXXNewExpr(const CXXNewExpr *CNE, ExplodedNode *Pred,
ExplodedNodeSet ) {
@@ -488,18 +506,7 @@
   SVal symVal = UnknownVal();
   FunctionDecl *FD = CNE->getOperatorNew();
 
-  bool IsStandardGlobalOpNewFunction = false;
-  if (FD && !isa(FD) && !FD->isVariadic()) {
-if (FD->getNumParams() == 2) {
-  QualType T = FD->getParamDecl(1)->getType();
-  if (const IdentifierInfo *II = T.getBaseTypeIdentifier())
-// NoThrow placement new behaves as