[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

[Deep Majumder via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGd825309352b4: [analyzer] Handle std::make_unique (authored 
by RedDocMD).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
  clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
  clang/test/Analysis/Inputs/system-header-simulator-cxx.h
  clang/test/Analysis/smart-ptr-text-output.cpp

Index: clang/test/Analysis/smart-ptr-text-output.cpp
===
--- clang/test/Analysis/smart-ptr-text-output.cpp
+++ clang/test/Analysis/smart-ptr-text-output.cpp
@@ -1,10 +1,17 @@
 // RUN: %clang_analyze_cc1\
-// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr\
+// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr,debug.ExprInspection\
+// RUN:  -analyzer-config cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
+// RUN:  -analyzer-output=text -std=c++20 %s -verify=expected
+
+// RUN: %clang_analyze_cc1\
+// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr,debug.ExprInspection\
 // RUN:  -analyzer-config cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
 // RUN:  -analyzer-output=text -std=c++11 %s -verify=expected
 
 #include "Inputs/system-header-simulator-cxx.h"
 
+void clang_analyzer_eval(bool);
+
 class A {
 public:
   A(){};
@@ -310,3 +317,61 @@
 // expected-note@-1{{Dereference of null smart pointer 'P'}}
   }
 }
+
+void makeUniqueReturnsNonNullUniquePtr() {
+  auto P = std::make_unique();
+  if (!P) {   // expected-note {{Taking false branch}}
+P->foo(); // should have no warning here, path is impossible
+  }
+  P.reset(); // expected-note {{Smart pointer 'P' reset using a null value}}
+  // Now P is null
+  if (!P) {
+// expected-note@-1 {{Taking true branch}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+// expected-note@-1{{Dereference of null smart pointer 'P'}}
+  }
+}
+
+#if __cplusplus >= 202002L
+
+void makeUniqueForOverwriteReturnsNullUniquePtr() {
+  auto P = std::make_unique_for_overwrite();
+  if (!P) {   // expected-note {{Taking false branch}}
+P->foo(); // should have no warning here, path is impossible
+  }
+  P.reset(); // expected-note {{Smart pointer 'P' reset using a null value}}
+  // Now P is null
+  if (!P) {
+// expected-note@-1 {{Taking true branch}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+// expected-note@-1{{Dereference of null smart pointer 'P'}}
+  }
+}
+
+#endif
+
+struct G {
+  int *p;
+  G(int *p): p(p) {}
+  ~G() { *p = 0; }
+};
+
+void foo() {
+  int x = 1;
+  {
+auto P = std::make_unique();
+// FIXME: There should not be a state split here, it should take the true path.
+clang_analyzer_eval(*P->p == 1); // expected-warning {{TRUE}}
+// expected-warning@-1 {{FALSE}}
+// expected-note@-2 {{Assuming the condition is true}}
+// expected-note@-3 {{Assuming the condition is false}}
+// expected-note@-4 {{TRUE}}
+// expected-note@-5 {{FALSE}}
+// expected-note@-6 {{Assuming the condition is false}}
+  }
+  // FIXME: Should be fixed when unique_ptr desctructors are
+  // properly modelled. This includes modelling the call to
+  // the destructor of the inner pointer type.
+  clang_analyzer_eval(x == 0); // expected-warning {{FALSE}}
+  // expected-note@-1 {{FALSE}}
+}
Index: clang/test/Analysis/Inputs/system-header-simulator-cxx.h
===
--- clang/test/Analysis/Inputs/system-header-simulator-cxx.h
+++ clang/test/Analysis/Inputs/system-header-simulator-cxx.h
@@ -1033,6 +1033,16 @@
 template 
 bool operator<=(nullptr_t x, const unique_ptr );
 
+template 
+unique_ptr make_unique(Args &&...args);
+
+#if __cplusplus >= 202002L
+
+template 
+unique_ptr make_unique_for_overwrite();
+
+#endif
+
 } // namespace std
 #endif
 
Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -192,12 +192,19 @@
   const LocationContext *LCtx,
   unsigned VisitCount) {
   QualType T = E->getType();
-  assert(Loc::isLocType(T));
-  assert(SymbolManager::canSymbolicate(T));
-  if (T->isNullPtrType())
-return makeZeroVal(T);
+  return getConjuredHeapSymbolVal(E, LCtx, T, VisitCount);
+}
+
+DefinedOrUnknownSVal
+SValBuilder::getConjuredHeapSymbolVal(const Expr *E,
+  const LocationContext *LCtx,
+  

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD updated this revision to Diff 359627.
RedDocMD added a comment.

Marked test with FIXME notes


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
  clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
  clang/test/Analysis/Inputs/system-header-simulator-cxx.h
  clang/test/Analysis/smart-ptr-text-output.cpp

Index: clang/test/Analysis/smart-ptr-text-output.cpp
===
--- clang/test/Analysis/smart-ptr-text-output.cpp
+++ clang/test/Analysis/smart-ptr-text-output.cpp
@@ -1,10 +1,17 @@
 // RUN: %clang_analyze_cc1\
-// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr\
+// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr,debug.ExprInspection\
+// RUN:  -analyzer-config cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
+// RUN:  -analyzer-output=text -std=c++20 %s -verify=expected
+
+// RUN: %clang_analyze_cc1\
+// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr,debug.ExprInspection\
 // RUN:  -analyzer-config cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
 // RUN:  -analyzer-output=text -std=c++11 %s -verify=expected
 
 #include "Inputs/system-header-simulator-cxx.h"
 
+void clang_analyzer_eval(bool);
+
 class A {
 public:
   A(){};
@@ -310,3 +317,61 @@
 // expected-note@-1{{Dereference of null smart pointer 'P'}}
   }
 }
+
+void makeUniqueReturnsNonNullUniquePtr() {
+  auto P = std::make_unique();
+  if (!P) {   // expected-note {{Taking false branch}}
+P->foo(); // should have no warning here, path is impossible
+  }
+  P.reset(); // expected-note {{Smart pointer 'P' reset using a null value}}
+  // Now P is null
+  if (!P) {
+// expected-note@-1 {{Taking true branch}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+// expected-note@-1{{Dereference of null smart pointer 'P'}}
+  }
+}
+
+#if __cplusplus >= 202002L
+
+void makeUniqueForOverwriteReturnsNullUniquePtr() {
+  auto P = std::make_unique_for_overwrite();
+  if (!P) {   // expected-note {{Taking false branch}}
+P->foo(); // should have no warning here, path is impossible
+  }
+  P.reset(); // expected-note {{Smart pointer 'P' reset using a null value}}
+  // Now P is null
+  if (!P) {
+// expected-note@-1 {{Taking true branch}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+// expected-note@-1{{Dereference of null smart pointer 'P'}}
+  }
+}
+
+#endif
+
+struct G {
+  int *p;
+  G(int *p): p(p) {}
+  ~G() { *p = 0; }
+};
+
+void foo() {
+  int x = 1;
+  {
+auto P = std::make_unique();
+// FIXME: There should not be a state split here, it should take the true path.
+clang_analyzer_eval(*P->p == 1); // expected-warning {{TRUE}}
+// expected-warning@-1 {{FALSE}}
+// expected-note@-2 {{Assuming the condition is true}}
+// expected-note@-3 {{Assuming the condition is false}}
+// expected-note@-4 {{TRUE}}
+// expected-note@-5 {{FALSE}}
+// expected-note@-6 {{Assuming the condition is false}}
+  }
+  // FIXME: Should be fixed when unique_ptr desctructors are
+  // properly modelled. This includes modelling the call to
+  // the destructor of the inner pointer type.
+  clang_analyzer_eval(x == 0); // expected-warning {{FALSE}}
+  // expected-note@-1 {{FALSE}}
+}
Index: clang/test/Analysis/Inputs/system-header-simulator-cxx.h
===
--- clang/test/Analysis/Inputs/system-header-simulator-cxx.h
+++ clang/test/Analysis/Inputs/system-header-simulator-cxx.h
@@ -1033,6 +1033,16 @@
 template 
 bool operator<=(nullptr_t x, const unique_ptr );
 
+template 
+unique_ptr make_unique(Args &&...args);
+
+#if __cplusplus >= 202002L
+
+template 
+unique_ptr make_unique_for_overwrite();
+
+#endif
+
 } // namespace std
 #endif
 
Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -192,12 +192,19 @@
   const LocationContext *LCtx,
   unsigned VisitCount) {
   QualType T = E->getType();
-  assert(Loc::isLocType(T));
-  assert(SymbolManager::canSymbolicate(T));
-  if (T->isNullPtrType())
-return makeZeroVal(T);
+  return getConjuredHeapSymbolVal(E, LCtx, T, VisitCount);
+}
+
+DefinedOrUnknownSVal
+SValBuilder::getConjuredHeapSymbolVal(const Expr *E,
+  const LocationContext *LCtx,
+  QualType type, unsigned VisitCount) {
+  assert(Loc::isLocType(type));
+  

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD updated this revision to Diff 359625.
RedDocMD added a comment.

Fixed up tests


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
  clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
  clang/test/Analysis/Inputs/system-header-simulator-cxx.h
  clang/test/Analysis/smart-ptr-text-output.cpp

Index: clang/test/Analysis/smart-ptr-text-output.cpp
===
--- clang/test/Analysis/smart-ptr-text-output.cpp
+++ clang/test/Analysis/smart-ptr-text-output.cpp
@@ -1,10 +1,17 @@
 // RUN: %clang_analyze_cc1\
-// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr\
+// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr,debug.ExprInspection\
+// RUN:  -analyzer-config cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
+// RUN:  -analyzer-output=text -std=c++20 %s -verify=expected
+
+// RUN: %clang_analyze_cc1\
+// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr,debug.ExprInspection\
 // RUN:  -analyzer-config cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
 // RUN:  -analyzer-output=text -std=c++11 %s -verify=expected
 
 #include "Inputs/system-header-simulator-cxx.h"
 
+void clang_analyzer_eval(bool);
+
 class A {
 public:
   A(){};
@@ -310,3 +317,57 @@
 // expected-note@-1{{Dereference of null smart pointer 'P'}}
   }
 }
+
+void makeUniqueReturnsNonNullUniquePtr() {
+  auto P = std::make_unique();
+  if (!P) {   // expected-note {{Taking false branch}}
+P->foo(); // should have no warning here, path is impossible
+  }
+  P.reset(); // expected-note {{Smart pointer 'P' reset using a null value}}
+  // Now P is null
+  if (!P) {
+// expected-note@-1 {{Taking true branch}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+// expected-note@-1{{Dereference of null smart pointer 'P'}}
+  }
+}
+
+#if __cplusplus >= 202002L
+
+void makeUniqueForOverwriteReturnsNullUniquePtr() {
+  auto P = std::make_unique_for_overwrite();
+  if (!P) {   // expected-note {{Taking false branch}}
+P->foo(); // should have no warning here, path is impossible
+  }
+  P.reset(); // expected-note {{Smart pointer 'P' reset using a null value}}
+  // Now P is null
+  if (!P) {
+// expected-note@-1 {{Taking true branch}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+// expected-note@-1{{Dereference of null smart pointer 'P'}}
+  }
+}
+
+#endif
+
+struct G {
+  int *p;
+  G(int *p): p(p) {}
+  ~G() { *p = 0; }
+};
+
+void foo() {
+  int x = 1;
+  {
+auto P = std::make_unique();
+clang_analyzer_eval(*P->p == 1); // expected-warning {{TRUE}}
+// expected-warning@-1 {{FALSE}}
+// expected-note@-2 {{Assuming the condition is true}}
+// expected-note@-3 {{Assuming the condition is false}}
+// expected-note@-4 {{TRUE}}
+// expected-note@-5 {{FALSE}}
+// expected-note@-6 {{Assuming the condition is false}}
+  }
+  clang_analyzer_eval(x == 0); // expected-warning {{FALSE}}
+  // expected-note@-1 {{FALSE}}
+}
Index: clang/test/Analysis/Inputs/system-header-simulator-cxx.h
===
--- clang/test/Analysis/Inputs/system-header-simulator-cxx.h
+++ clang/test/Analysis/Inputs/system-header-simulator-cxx.h
@@ -1033,6 +1033,16 @@
 template 
 bool operator<=(nullptr_t x, const unique_ptr );
 
+template 
+unique_ptr make_unique(Args &&...args);
+
+#if __cplusplus >= 202002L
+
+template 
+unique_ptr make_unique_for_overwrite();
+
+#endif
+
 } // namespace std
 #endif
 
Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -192,12 +192,19 @@
   const LocationContext *LCtx,
   unsigned VisitCount) {
   QualType T = E->getType();
-  assert(Loc::isLocType(T));
-  assert(SymbolManager::canSymbolicate(T));
-  if (T->isNullPtrType())
-return makeZeroVal(T);
+  return getConjuredHeapSymbolVal(E, LCtx, T, VisitCount);
+}
+
+DefinedOrUnknownSVal
+SValBuilder::getConjuredHeapSymbolVal(const Expr *E,
+  const LocationContext *LCtx,
+  QualType type, unsigned VisitCount) {
+  assert(Loc::isLocType(type));
+  assert(SymbolManager::canSymbolicate(type));
+  if (type->isNullPtrType())
+return makeZeroVal(type);
 
-  SymbolRef sym = SymMgr.conjureSymbol(E, LCtx, T, VisitCount);
+  SymbolRef sym = SymMgr.conjureSymbol(E, LCtx, type, VisitCount);
   return 

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD updated this revision to Diff 359605.
RedDocMD added a comment.

Post-rebase cleanup


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
  clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
  clang/test/Analysis/Inputs/system-header-simulator-cxx.h
  clang/test/Analysis/smart-ptr-text-output.cpp

Index: clang/test/Analysis/smart-ptr-text-output.cpp
===
--- clang/test/Analysis/smart-ptr-text-output.cpp
+++ clang/test/Analysis/smart-ptr-text-output.cpp
@@ -1,10 +1,17 @@
 // RUN: %clang_analyze_cc1\
-// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr\
+// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr,debug.ExprInspection\
+// RUN:  -analyzer-config cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
+// RUN:  -analyzer-output=text -std=c++20 %s -verify=expected
+
+// RUN: %clang_analyze_cc1\
+// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr,debug.ExprInspection\
 // RUN:  -analyzer-config cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
 // RUN:  -analyzer-output=text -std=c++11 %s -verify=expected
 
 #include "Inputs/system-header-simulator-cxx.h"
 
+void clang_analyzer_eval(bool);
+
 class A {
 public:
   A(){};
@@ -310,3 +317,55 @@
 // expected-note@-1{{Dereference of null smart pointer 'P'}}
   }
 }
+
+void makeUniqueReturnsNonNullUniquePtr() {
+  auto P = std::make_unique();
+  if (!P) {   // expected-note {{Taking false branch}}
+P->foo(); // should have no warning here, path is impossible
+  }
+  P.reset(); // expected-note {{Smart pointer 'P' reset using a null value}}
+  // Now P is null
+  if (!P) {
+// expected-note@-1 {{Taking true branch}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+// expected-note@-1{{Dereference of null smart pointer 'P'}}
+  }
+}
+
+#if __cplusplus >= 202002L
+
+void makeUniqueForOverwriteReturnsNullUniquePtr() {
+  auto P = std::make_unique_for_overwrite();
+  if (!P) {   // expected-note {{Taking false branch}}
+P->foo(); // should have no warning here, path is impossible
+  }
+  P.reset(); // expected-note {{Smart pointer 'P' reset using a null value}}
+  // Now P is null
+  if (!P) {
+// expected-note@-1 {{Taking true branch}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+// expected-note@-1{{Dereference of null smart pointer 'P'}}
+  }
+}
+
+#endif
+
+struct G {
+  int *p;
+  G(int *p): p(p) {}
+  ~G() { *p = 0; }
+};
+
+void foo() {
+  int x = 1;
+  {
+auto P = std::make_unique();
+clang_analyzer_eval(*P->p == 1); // expected-warning {{TRUE}}
+// expected-note@-1 {{Assuming the condition is false}}
+// expected-note@-2 {{FALSE}}
+// expected-note@-3 {{Assuming the condition is true}}
+// expected-note@-4 {{TRUE}}
+  }
+  clang_analyzer_eval(x == 0); // expected-warning {{FALSE}}
+  // expected-note@-1 {{FALSE}}
+}
Index: clang/test/Analysis/Inputs/system-header-simulator-cxx.h
===
--- clang/test/Analysis/Inputs/system-header-simulator-cxx.h
+++ clang/test/Analysis/Inputs/system-header-simulator-cxx.h
@@ -1033,6 +1033,16 @@
 template 
 bool operator<=(nullptr_t x, const unique_ptr );
 
+template 
+unique_ptr make_unique(Args &&...args);
+
+#if __cplusplus >= 202002L
+
+template 
+unique_ptr make_unique_for_overwrite();
+
+#endif
+
 } // namespace std
 #endif
 
Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -192,12 +192,19 @@
   const LocationContext *LCtx,
   unsigned VisitCount) {
   QualType T = E->getType();
-  assert(Loc::isLocType(T));
-  assert(SymbolManager::canSymbolicate(T));
-  if (T->isNullPtrType())
-return makeZeroVal(T);
+  return getConjuredHeapSymbolVal(E, LCtx, T, VisitCount);
+}
+
+DefinedOrUnknownSVal
+SValBuilder::getConjuredHeapSymbolVal(const Expr *E,
+  const LocationContext *LCtx,
+  QualType type, unsigned VisitCount) {
+  assert(Loc::isLocType(type));
+  assert(SymbolManager::canSymbolicate(type));
+  if (type->isNullPtrType())
+return makeZeroVal(type);
 
-  SymbolRef sym = SymMgr.conjureSymbol(E, LCtx, T, VisitCount);
+  SymbolRef sym = SymMgr.conjureSymbol(E, LCtx, type, VisitCount);
   return loc::MemRegionVal(MemMgr.getSymbolicHeapRegion(sym));
 }
 
Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

Is the syntax of specifying expected notes and warnings documented somewhere? I 
could not find the note-specific syntax.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750

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

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD updated this revision to Diff 358267.
RedDocMD added a comment.

Fixing up tests


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
  clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
  clang/test/Analysis/Inputs/system-header-simulator-cxx.h
  clang/test/Analysis/smart-ptr-text-output.cpp

Index: clang/test/Analysis/smart-ptr-text-output.cpp
===
--- clang/test/Analysis/smart-ptr-text-output.cpp
+++ clang/test/Analysis/smart-ptr-text-output.cpp
@@ -1,10 +1,17 @@
 // RUN: %clang_analyze_cc1\
-// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr\
+// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr,debug.ExprInspection\
+// RUN:  -analyzer-config cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
+// RUN:  -analyzer-output=text -std=c++20 %s -verify=expected
+
+// RUN: %clang_analyze_cc1\
+// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr,debug.ExprInspection\
 // RUN:  -analyzer-config cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
 // RUN:  -analyzer-output=text -std=c++11 %s -verify=expected
 
 #include "Inputs/system-header-simulator-cxx.h"
 
+void clang_analyzer_eval(bool);
+
 class A {
 public:
   A(){};
@@ -313,3 +320,55 @@
 // expected-note@-1{{Dereference of null smart pointer 'P'}}
   }
 }
+
+void makeUniqueReturnsNonNullUniquePtr() {
+  auto P = std::make_unique();
+  if (!P) {   // expected-note {{Taking false branch}}
+P->foo(); // should have no warning here, path is impossible
+  }
+  P.reset(); // expected-note {{Smart pointer 'P' reset using a null value}}
+  // Now P is null
+  if (!P) {
+// expected-note@-1 {{Taking true branch}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+// expected-note@-1{{Dereference of null smart pointer 'P'}}
+  }
+}
+
+#if __cplusplus >= 202002L
+
+void makeUniqueForOverwriteReturnsNullUniquePtr() {
+  auto P = std::make_unique_for_overwrite();
+  if (!P) {   // expected-note {{Taking false branch}}
+P->foo(); // should have no warning here, path is impossible
+  }
+  P.reset(); // expected-note {{Smart pointer 'P' reset using a null value}}
+  // Now P is null
+  if (!P) {
+// expected-note@-1 {{Taking true branch}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+// expected-note@-1{{Dereference of null smart pointer 'P'}}
+  }
+}
+
+#endif
+
+struct G {
+  int *p;
+  G(int *p): p(p) {}
+  ~G() { *p = 0; }
+};
+
+void foo() {
+  int x = 1;
+  {
+auto P = std::make_unique();
+clang_analyzer_eval(*P->p == 1); // expected-warning {{TRUE}}
+// expected-note@-1 {{Assuming the condition is false}}
+// expected-note@-2 {{FALSE}}
+// expected-note@-3 {{Assuming the condition is true}}
+// expected-note@-4 {{TRUE}}
+  }
+  clang_analyzer_eval(x == 0); // expected-warning {{FALSE}}
+  // expected-note@-1 {{FALSE}}
+}
Index: clang/test/Analysis/Inputs/system-header-simulator-cxx.h
===
--- clang/test/Analysis/Inputs/system-header-simulator-cxx.h
+++ clang/test/Analysis/Inputs/system-header-simulator-cxx.h
@@ -978,6 +978,17 @@
 void swap(unique_ptr , unique_ptr ) noexcept {
   x.swap(y);
 }
+
+template 
+unique_ptr make_unique(Args &&...args);
+
+#if __cplusplus >= 202002L
+
+template 
+unique_ptr make_unique_for_overwrite();
+
+#endif
+
 } // namespace std
 #endif
 
Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -192,12 +192,19 @@
   const LocationContext *LCtx,
   unsigned VisitCount) {
   QualType T = E->getType();
-  assert(Loc::isLocType(T));
-  assert(SymbolManager::canSymbolicate(T));
-  if (T->isNullPtrType())
-return makeZeroVal(T);
+  return getConjuredHeapSymbolVal(E, LCtx, T, VisitCount);
+}
+
+DefinedOrUnknownSVal
+SValBuilder::getConjuredHeapSymbolVal(const Expr *E,
+  const LocationContext *LCtx,
+  QualType type, unsigned VisitCount) {
+  assert(Loc::isLocType(type));
+  assert(SymbolManager::canSymbolicate(type));
+  if (type->isNullPtrType())
+return makeZeroVal(type);
 
-  SymbolRef sym = SymMgr.conjureSymbol(E, LCtx, T, VisitCount);
+  SymbolRef sym = SymMgr.conjureSymbol(E, LCtx, type, VisitCount);
   return loc::MemRegionVal(MemMgr.getSymbolicHeapRegion(sym));
 }
 
Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Yes, @xazax.hun is correct.

It's incorrect to say that the static analyzer "doesn't seem to be able to make 
up its mind". The analyzer gives perfectly clear and consistent answers for 
each execution path it explores and it's not surprising that the results are 
different on different execution paths. The presence of the FALSE path 
indicates indicates that the test indeed doesn't pass: an impossible execution 
path is being explored.

Eagerly-assume is a thing because it produces easier constraints for the solver 
to solve. Also the state split is pretty justified on any boolean expression 
with no side effects.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750

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

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

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

xazax.hun added a comment.

In D103750#2828995 , @RedDocMD wrote:

> What about this?

I believe that you might see `analyzer-eagerly-assume` in action. Basically, 
instead of keeping a constraint unknown, the analyzer can split the path 
eagerly. So you have one path where the constraint is assumed to be true and 
one where it is assumed to be false. I think the rationale was that the user is 
likely to branch on certain conditions and doing it eagerly can make the 
analysis more precise in some cases but I do not really remember the details. 
Hopefully Artem has some more contexts.




Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:374
+}
\ No newline at end of file


Nit: we usually add new lines to the ends of the files.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750

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

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

In D103750#2827548 , @RedDocMD wrote:

> In D103750#2825342 , @NoQ wrote:
>
>> Do the above tests pass when your new `evalCall` modeling is enabled?
>
> The analyzer doesn't seem to be able to make up its mind.
>
>   member-constructor.cpp:15:5: warning: FALSE [debug.ExprInspection]
>   clang_analyzer_eval(*P->p == 0);
>   ^~~
>   member-constructor.cpp:15:25: note: Assuming the condition is false
>   clang_analyzer_eval(*P->p == 0);
>   ^~
>   member-constructor.cpp:15:5: note: FALSE
>   clang_analyzer_eval(*P->p == 0);
>   ^~~
>   member-constructor.cpp:15:5: warning: TRUE [debug.ExprInspection]
>   clang_analyzer_eval(*P->p == 0);
>   ^~~
>   member-constructor.cpp:15:25: note: Assuming the condition is true
>   clang_analyzer_eval(*P->p == 0);
>   ^~
>   member-constructor.cpp:15:5: note: TRUE
>   clang_analyzer_eval(*P->p == 0);
>   ^~~
>   2 warnings generated.

What about this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750

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

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D103750#2827566 , @RedDocMD wrote:

> Do you want the new failing test to be marked //expected to fail//?

The line of thinking here is that tests are just something that gives us a 
signal when the behavior changes. They don't necessarily demonstrate the 
expected behavior. If they don't there's still value in knowing that the 
observed behavior on them has changed.

For example, if you believe that your commit shouldn't change the observed 
behavior ("NFC commit", eg. refactoring), any signal that the observed behavior 
has changed should raise concerns and probably cause you to rethink the commit, 
even if the behavior has in fact improved.

Similarly, if the functional change is intended, you may still find it 
suspicious if it suddenly fixes a test that it wasn't supposed to fix. This may 
help you discover a bug in your commit that would cause the behavior to regress 
in other related cases.

You don't want the buggy test to be silenced, you want it to keep actively 
verifying that the bug is not yet fixed.

Then, separately, for every test there's a requirement to keep it 
understandable, so that the reader would understand what exactly is tested and 
what are the consequences of breaking the test. It's often valuable to provide 
comments indicating your level of confidence - "this test is extremely 
important to pass", "we don't really care about our behavior on this test but 
it's still nice to know when that behavior changes", etc. - and "we definitely 
don't want this test to pass but for now it does" is a possible answer as well 
and it doesn't make the test any less valuable.

So basically

  void foo() {
// FIXME: Should be FALSE.
clang_analyzer_eval(1 + 1 == 3); // expected-warning{{TRUE}}
// FIXME: Should be TRUE.
clang_analyzer_eval(1 + 1 == 2); // expected-warning{{FALSE}}
  }

is a great test to have.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750

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

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

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

xazax.hun added a comment.

In D103750#2827566 , @RedDocMD wrote:

> Do you want the new failing test to be marked //expected to fail//?

I usually just add the wrong expectation to make the test pass and add a TODO 
comment that explains why is this wrong and we should fix it in the future.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750

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

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

In D103750#2825823 , @xazax.hun wrote:

> I believe there are a couple of comments that are done but not marked 
> accordingly. 
> I agree with Artem, if we could craft code that fails due to not calling 
> ctors, we should probably include them in the tests, even if they don't 
> reflect the desired behavior they are a great source of documentation what is 
> missing.

Do you want the new failing test to be marked //expected to fail//?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750

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

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD updated this revision to Diff 353048.
RedDocMD added a comment.

Little changes, a failing test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
  clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
  clang/test/Analysis/Inputs/system-header-simulator-cxx.h
  clang/test/Analysis/smart-ptr-text-output.cpp

Index: clang/test/Analysis/smart-ptr-text-output.cpp
===
--- clang/test/Analysis/smart-ptr-text-output.cpp
+++ clang/test/Analysis/smart-ptr-text-output.cpp
@@ -1,10 +1,17 @@
 // RUN: %clang_analyze_cc1\
-// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr\
+// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr,debug.ExprInspection\
+// RUN:  -analyzer-config cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
+// RUN:  -analyzer-output=text -std=c++20 %s -verify=expected
+
+// RUN: %clang_analyze_cc1\
+// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr,debug.ExprInspection\
 // RUN:  -analyzer-config cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
 // RUN:  -analyzer-output=text -std=c++11 %s -verify=expected
 
 #include "Inputs/system-header-simulator-cxx.h"
 
+void clang_analyzer_eval(bool);
+
 class A {
 public:
   A(){};
@@ -313,3 +320,54 @@
 // expected-note@-1{{Dereference of null smart pointer 'P'}}
   }
 }
+
+void makeUniqueReturnsNonNullUniquePtr() {
+  auto P = std::make_unique();
+  if (!P) {   // expected-note {{Taking false branch}}
+P->foo(); // should have no warning here, path is impossible
+  }
+  P.reset(); // expected-note {{Smart pointer 'P' reset using a null value}}
+  // Now P is null
+  if (!P) {
+// expected-note@-1 {{Taking true branch}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+// expected-note@-1{{Dereference of null smart pointer 'P'}}
+  }
+}
+
+#if __cplusplus >= 202002L
+
+void makeUniqueForOverwriteReturnsNullUniquePtr() {
+  auto P = std::make_unique_for_overwrite();
+  if (!P) {   // expected-note {{Taking false branch}}
+P->foo(); // should have no warning here, path is impossible
+  }
+  P.reset(); // expected-note {{Smart pointer 'P' reset using a null value}}
+  // Now P is null
+  if (!P) {
+// expected-note@-1 {{Taking true branch}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+// expected-note@-1{{Dereference of null smart pointer 'P'}}
+  }
+}
+
+#endif
+
+struct G {
+  int *p;
+  G(int *p): p(p) {}
+  ~G() { *p = 0; }
+};
+
+void foo() {
+  int x = 1;
+  {
+auto P = std::make_unique();
+clang_analyzer_eval(*P->p == 1); // expected-warning {{TRUE [debug.ExprInspection]}}
+// expected-note@-1 {{TRUE}}
+// expected-note@-2 {{Assuming condition is true}}
+  }
+  clang_analyzer_eval(x == 0); // expected-warning {{TRUE [debug.ExprInspection]}}
+  // expected-note@-1 {{TRUE}}
+  // expected-note@-2 {{Assuming condition is true}}
+}
\ No newline at end of file
Index: clang/test/Analysis/Inputs/system-header-simulator-cxx.h
===
--- clang/test/Analysis/Inputs/system-header-simulator-cxx.h
+++ clang/test/Analysis/Inputs/system-header-simulator-cxx.h
@@ -978,6 +978,17 @@
 void swap(unique_ptr , unique_ptr ) noexcept {
   x.swap(y);
 }
+
+template 
+unique_ptr make_unique(Args &&...args);
+
+#if __cplusplus >= 202002L
+
+template 
+unique_ptr make_unique_for_overwrite();
+
+#endif
+
 } // namespace std
 #endif
 
Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -192,12 +192,19 @@
   const LocationContext *LCtx,
   unsigned VisitCount) {
   QualType T = E->getType();
-  assert(Loc::isLocType(T));
-  assert(SymbolManager::canSymbolicate(T));
-  if (T->isNullPtrType())
-return makeZeroVal(T);
+  return getConjuredHeapSymbolVal(E, LCtx, T, VisitCount);
+}
+
+DefinedOrUnknownSVal
+SValBuilder::getConjuredHeapSymbolVal(const Expr *E,
+  const LocationContext *LCtx,
+  QualType type, unsigned VisitCount) {
+  assert(Loc::isLocType(type));
+  assert(SymbolManager::canSymbolicate(type));
+  if (type->isNullPtrType())
+return makeZeroVal(type);
 
-  SymbolRef sym = SymMgr.conjureSymbol(E, LCtx, T, VisitCount);
+  SymbolRef sym = SymMgr.conjureSymbol(E, LCtx, type, VisitCount);
   return loc::MemRegionVal(MemMgr.getSymbolicHeapRegion(sym));
 }
 
Index: 

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

In D103750#2825342 , @NoQ wrote:

> In D103750#2823741 , @RedDocMD 
> wrote:
>
>> I would suppose that constructor calls are properly handled. (ie, member 
>> constructors are called properly).
>
> Do the above tests pass when your new `evalCall` modeling is enabled?

The analyzer doesn't seem to be able to make up its mind.

  member-constructor.cpp:15:5: warning: FALSE [debug.ExprInspection]
  clang_analyzer_eval(*P->p == 0);
  ^~~
  member-constructor.cpp:15:25: note: Assuming the condition is false
  clang_analyzer_eval(*P->p == 0);
  ^~
  member-constructor.cpp:15:5: note: FALSE
  clang_analyzer_eval(*P->p == 0);
  ^~~
  member-constructor.cpp:15:5: warning: TRUE [debug.ExprInspection]
  clang_analyzer_eval(*P->p == 0);
  ^~~
  member-constructor.cpp:15:25: note: Assuming the condition is true
  clang_analyzer_eval(*P->p == 0);
  ^~
  member-constructor.cpp:15:5: note: TRUE
  clang_analyzer_eval(*P->p == 0);
  ^~~
  2 warnings generated.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750

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

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD marked an inline comment as done.
RedDocMD added inline comments.



Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:3
+// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr\
+// RUN:  -analyzer-config 
cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
+// RUN:  -analyzer-output=text -std=c++20 %s -verify=expected

NoQ wrote:
> RedDocMD wrote:
> > NoQ wrote:
> > > Could you double-check that this flag correctly disables all the newly 
> > > introduced modeling so that it wasn't accidentally enabled for all users 
> > > before it's ready?
> > Yup. There are 133 notes and warnings (just search for "// expected-" and 
> > see the count). And on setting the flag to `false`, there are 133 errors.
> > So it works.
> I mean, specifically the modeling added by this patch? I see you wrote an 
> entire checker callback that doesn't seem to have an analyzer-config option 
> check anywhere in it. Simply having different results isn't sufficient to 
> verify this.
Oh I see.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750

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

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

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

xazax.hun added a comment.

I believe there are a couple of comments that are done but not marked 
accordingly. 
I agree with Artem, if we could craft code that fails due to not calling ctors, 
we should probably include them in the tests, even if they don't reflect the 
desired behavior they are a great source of documentation what is missing.




Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:151
+  const auto  = FD->getTemplateSpecializationArgs()->asArray();
+  if (TemplateArgs.size() == 0)
+return {};

Nit: consider `TemplateArgs.empty()`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750

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

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D103750#2823741 , @RedDocMD wrote:

> I would suppose that constructor calls are properly handled. (ie, member 
> constructors are called properly).

Do the above tests pass when your new `evalCall` modeling is enabled?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750

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

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:3
+// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr\
+// RUN:  -analyzer-config 
cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
+// RUN:  -analyzer-output=text -std=c++20 %s -verify=expected

RedDocMD wrote:
> NoQ wrote:
> > Could you double-check that this flag correctly disables all the newly 
> > introduced modeling so that it wasn't accidentally enabled for all users 
> > before it's ready?
> Yup. There are 133 notes and warnings (just search for "// expected-" and see 
> the count). And on setting the flag to `false`, there are 133 errors.
> So it works.
I mean, specifically the modeling added by this patch? I see you wrote an 
entire checker callback that doesn't seem to have an analyzer-config option 
check anywhere in it. Simply having different results isn't sufficient to 
verify this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750

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

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

I would suppose that constructor calls are properly handled. (ie, member 
constructors are called properly).
As for modelling destructors, there is a separate problem - since we don't have 
a `Stmt`, the `postCall` handler keeps on crashing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750

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

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD marked an inline comment as done.
RedDocMD added inline comments.



Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:3
+// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr\
+// RUN:  -analyzer-config 
cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
+// RUN:  -analyzer-output=text -std=c++20 %s -verify=expected

NoQ wrote:
> Could you double-check that this flag correctly disables all the newly 
> introduced modeling so that it wasn't accidentally enabled for all users 
> before it's ready?
Yup. There are 133 notes and warnings (just search for "// expected-" and see 
the count). And on setting the flag to `false`, there are 133 errors.
So it works.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750

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

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD updated this revision to Diff 352621.
RedDocMD added a comment.

Added more info to the TODO


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
  clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
  clang/test/Analysis/Inputs/system-header-simulator-cxx.h
  clang/test/Analysis/smart-ptr-text-output.cpp

Index: clang/test/Analysis/smart-ptr-text-output.cpp
===
--- clang/test/Analysis/smart-ptr-text-output.cpp
+++ clang/test/Analysis/smart-ptr-text-output.cpp
@@ -1,3 +1,8 @@
+// RUN: %clang_analyze_cc1\
+// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr\
+// RUN:  -analyzer-config cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
+// RUN:  -analyzer-output=text -std=c++20 %s -verify=expected
+
 // RUN: %clang_analyze_cc1\
 // RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr\
 // RUN:  -analyzer-config cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
@@ -313,3 +318,35 @@
 // expected-note@-1{{Dereference of null smart pointer 'P'}}
   }
 }
+
+void makeUniqueReturnsNonNullUniquePtr() {
+  auto P = std::make_unique();
+  if (!P) {   // expected-note {{Taking false branch}}
+P->foo(); // should have no warning here, path is impossible
+  }
+  P.reset(); // expected-note {{Smart pointer 'P' reset using a null value}}
+  // Now P is null
+  if (!P) {
+// expected-note@-1 {{Taking true branch}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+// expected-note@-1{{Dereference of null smart pointer 'P'}}
+  }
+}
+
+#if __cplusplus >= 202002L
+
+void makeUniqueForOverwriteReturnsNullUniquePtr() {
+  auto P = std::make_unique_for_overwrite();
+  if (!P) {   // expected-note {{Taking false branch}}
+P->foo(); // should have no warning here, path is impossible
+  }
+  P.reset(); // expected-note {{Smart pointer 'P' reset using a null value}}
+  // Now P is null
+  if (!P) {
+// expected-note@-1 {{Taking true branch}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+// expected-note@-1{{Dereference of null smart pointer 'P'}}
+  }
+}
+
+#endif
Index: clang/test/Analysis/Inputs/system-header-simulator-cxx.h
===
--- clang/test/Analysis/Inputs/system-header-simulator-cxx.h
+++ clang/test/Analysis/Inputs/system-header-simulator-cxx.h
@@ -978,6 +978,17 @@
 void swap(unique_ptr , unique_ptr ) noexcept {
   x.swap(y);
 }
+
+template 
+unique_ptr make_unique(Args &&...args);
+
+#if __cplusplus >= 202002L
+
+template 
+unique_ptr make_unique_for_overwrite();
+
+#endif
+
 } // namespace std
 #endif
 
Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -192,12 +192,19 @@
   const LocationContext *LCtx,
   unsigned VisitCount) {
   QualType T = E->getType();
-  assert(Loc::isLocType(T));
-  assert(SymbolManager::canSymbolicate(T));
-  if (T->isNullPtrType())
-return makeZeroVal(T);
+  return getConjuredHeapSymbolVal(E, LCtx, T, VisitCount);
+}
+
+DefinedOrUnknownSVal
+SValBuilder::getConjuredHeapSymbolVal(const Expr *E,
+  const LocationContext *LCtx,
+  QualType type, unsigned VisitCount) {
+  assert(Loc::isLocType(type));
+  assert(SymbolManager::canSymbolicate(type));
+  if (type->isNullPtrType())
+return makeZeroVal(type);
 
-  SymbolRef sym = SymMgr.conjureSymbol(E, LCtx, T, VisitCount);
+  SymbolRef sym = SymMgr.conjureSymbol(E, LCtx, type, VisitCount);
   return loc::MemRegionVal(MemMgr.getSymbolicHeapRegion(sym));
 }
 
Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
@@ -35,6 +35,7 @@
 using namespace ento;
 
 namespace {
+
 class SmartPtrModeling
 : public Checker {
@@ -76,6 +77,9 @@
   {{"release"}, ::handleRelease},
   {{"swap", 1}, ::handleSwap},
   {{"get"}, ::handleGet}};
+  const CallDescription StdMakeUniqueCall{{"std", "make_unique"}};
+  const CallDescription StdMakeUniqueForOverwriteCall{
+  {"std", "make_unique_for_overwrite"}};
 };
 } // end of anonymous namespace
 
@@ -135,6 +139,21 @@
   return State;
 }
 
+// This is for use with standalone-functions like std::make_unique,
+// std::make_unique_for_overwrite, etc. It reads the 

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

[Artem Dergachev via Phabricator via cfe-commits]

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

This looks great to me, I think the patch is good to go as long as Valeriy's 
comment is addressed :)

Speaking of specs, hold up, we're forgetting something very important.

`make_unique()` also has to call the //constructor// of the underlying value. 
We're supposed to model that.

The constructor may be inlined or evaluated conservatively.

`~unique_ptr()` is also supposed to call the //destructor// of the underlying 
value. We're supposed to model that.

F17422410: .jpg 

I.e., you could imagine a bunch of tests like this

  struct S {
int *p;
S(int *p): p(p) {}
~S() { *p = 0; }
  };
  
  void foo() {
int x = 1;
{
  auto P = std::make_unique();
  clang_analyzer_eval(*P->p == 1); // TRUE
}
clang_analyzer_eval(x == 0); // TRUE
  }

We could stick to conservative evaluation of such constructors and destructors; 
at the very least, we should actively invalidate Store at the destructor. This 
patch can remain unchanged because the default values inside freshly conjured 
regions are unknown. But that'd still be a regression because the above test 
(presumably) passes with inlining.

Is now a good time to legalize modeling function calls inside checker callbacks?




Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:3
+// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr\
+// RUN:  -analyzer-config 
cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
+// RUN:  -analyzer-output=text -std=c++20 %s -verify=expected

Could you double-check that this flag correctly disables all the newly 
introduced modeling so that it wasn't accidentally enabled for all users before 
it's ready?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750

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

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:216-220
+// TODO: ExprEngine should do this for us.
+auto  = State->getStateManager().getOwningEngine();
+State = Engine.updateObjectsUnderConstruction(
+*ThisRegionOpt, nullptr, State, C.getLocationContext(),
+Call.getConstructionContext(), {});

I think it's fine to have it here in this commit and change it in one of the 
future commits.  But I think we should add a bit more description of 1) what's 
the problem, 2) how is this a solution, 3) how should it look when moved to the 
engine.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750

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

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD updated this revision to Diff 352127.
RedDocMD added a comment.

Fixed up meaning of make_unique_for_overwrite, use `getConjuredHeapSymbolVal`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
  clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
  clang/test/Analysis/Inputs/system-header-simulator-cxx.h
  clang/test/Analysis/smart-ptr-text-output.cpp

Index: clang/test/Analysis/smart-ptr-text-output.cpp
===
--- clang/test/Analysis/smart-ptr-text-output.cpp
+++ clang/test/Analysis/smart-ptr-text-output.cpp
@@ -1,3 +1,8 @@
+// RUN: %clang_analyze_cc1\
+// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr\
+// RUN:  -analyzer-config cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
+// RUN:  -analyzer-output=text -std=c++20 %s -verify=expected
+
 // RUN: %clang_analyze_cc1\
 // RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr\
 // RUN:  -analyzer-config cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
@@ -313,3 +318,35 @@
 // expected-note@-1{{Dereference of null smart pointer 'P'}}
   }
 }
+
+void makeUniqueReturnsNonNullUniquePtr() {
+  auto P = std::make_unique();
+  if (!P) {   // expected-note {{Taking false branch}}
+P->foo(); // should have no warning here, path is impossible
+  }
+  P.reset(); // expected-note {{Smart pointer 'P' reset using a null value}}
+  // Now P is null
+  if (!P) {
+// expected-note@-1 {{Taking true branch}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+// expected-note@-1{{Dereference of null smart pointer 'P'}}
+  }
+}
+
+#if __cplusplus >= 202002L
+
+void makeUniqueForOverwriteReturnsNullUniquePtr() {
+  auto P = std::make_unique_for_overwrite();
+  if (!P) {   // expected-note {{Taking false branch}}
+P->foo(); // should have no warning here, path is impossible
+  }
+  P.reset(); // expected-note {{Smart pointer 'P' reset using a null value}}
+  // Now P is null
+  if (!P) {
+// expected-note@-1 {{Taking true branch}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+// expected-note@-1{{Dereference of null smart pointer 'P'}}
+  }
+}
+
+#endif
Index: clang/test/Analysis/Inputs/system-header-simulator-cxx.h
===
--- clang/test/Analysis/Inputs/system-header-simulator-cxx.h
+++ clang/test/Analysis/Inputs/system-header-simulator-cxx.h
@@ -978,6 +978,17 @@
 void swap(unique_ptr , unique_ptr ) noexcept {
   x.swap(y);
 }
+
+template 
+unique_ptr make_unique(Args &&...args);
+
+#if __cplusplus >= 202002L
+
+template 
+unique_ptr make_unique_for_overwrite();
+
+#endif
+
 } // namespace std
 #endif
 
Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -192,12 +192,19 @@
   const LocationContext *LCtx,
   unsigned VisitCount) {
   QualType T = E->getType();
-  assert(Loc::isLocType(T));
-  assert(SymbolManager::canSymbolicate(T));
-  if (T->isNullPtrType())
-return makeZeroVal(T);
+  return getConjuredHeapSymbolVal(E, LCtx, T, VisitCount);
+}
+
+DefinedOrUnknownSVal
+SValBuilder::getConjuredHeapSymbolVal(const Expr *E,
+  const LocationContext *LCtx,
+  QualType type, unsigned VisitCount) {
+  assert(Loc::isLocType(type));
+  assert(SymbolManager::canSymbolicate(type));
+  if (type->isNullPtrType())
+return makeZeroVal(type);
 
-  SymbolRef sym = SymMgr.conjureSymbol(E, LCtx, T, VisitCount);
+  SymbolRef sym = SymMgr.conjureSymbol(E, LCtx, type, VisitCount);
   return loc::MemRegionVal(MemMgr.getSymbolicHeapRegion(sym));
 }
 
Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
@@ -35,6 +35,7 @@
 using namespace ento;
 
 namespace {
+
 class SmartPtrModeling
 : public Checker {
@@ -76,6 +77,9 @@
   {{"release"}, ::handleRelease},
   {{"swap", 1}, ::handleSwap},
   {{"get"}, ::handleGet}};
+  const CallDescription StdMakeUniqueCall{{"std", "make_unique"}};
+  const CallDescription StdMakeUniqueForOverwriteCall{
+  {"std", "make_unique_for_overwrite"}};
 };
 } // end of anonymous namespace
 
@@ -135,6 +139,21 @@
   return State;
 }
 
+// This is for use with standalone-functions like std::make_unique,
+// 

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD marked an inline comment as done.
RedDocMD added inline comments.



Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:339
+void makeUniqueForOverwriteReturnsNullUniquePtr() {
+  auto P = std::make_unique_for_overwrite(); // expected-note 
{{std::unique_ptr 'P' constructed by std::make_unique_for_overwrite is null}}
+  *P;   // expected-warning 
{{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}

NoQ wrote:
> Mmm wait a sec, that doesn't look like what the spec says.
> 
> https://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique:
> > Same as (1), except that the object is default-initialized. This overload 
> > participates in overload resolution only if T is not an array type. The 
> > function is equivalent to `unique_ptr(new T)`
> 
> It zero-initializes the //pointee//, not the //pointer//.
> 
> The difference between `std::make_unique()` and 
> `std::make_unique_for_overwrite()` is the difference between 
> value-initialization (invoking the default constructor) and 
> zero-initialization (simply filling the buffer with `0`s).
Oops! Look like I completely misunderstood the spec.
So basically, the inner pointer is //not// null, and we have the same 
assumptions that apply for `std::make_unique`. Only problem is that, for 
primitive types, the value obtained on de-referencing is undefined.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750

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

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:207
+
+const auto PtrVal = C.getSValBuilder().conjureSymbolVal(
+Call.getOriginExpr(), C.getLocationContext(),

Can you do a `getConjuredHeapSymbolVal()` instead? That'd give us the right 
memory space as well as the extra bit of information that the new symbol 
doesn't alias with any previous symbols.

I get it that it doesn't accept the type but it's perfectly ok for you to teach 
it how to accept the type.



Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:339
+void makeUniqueForOverwriteReturnsNullUniquePtr() {
+  auto P = std::make_unique_for_overwrite(); // expected-note 
{{std::unique_ptr 'P' constructed by std::make_unique_for_overwrite is null}}
+  *P;   // expected-warning 
{{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}

Mmm wait a sec, that doesn't look like what the spec says.

https://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique:
> Same as (1), except that the object is default-initialized. This overload 
> participates in overload resolution only if T is not an array type. The 
> function is equivalent to `unique_ptr(new T)`

It zero-initializes the //pointee//, not the //pointer//.

The difference between `std::make_unique()` and 
`std::make_unique_for_overwrite()` is the difference between 
value-initialization (invoking the default constructor) and zero-initialization 
(simply filling the buffer with `0`s).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750

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

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD updated this revision to Diff 351628.
RedDocMD added a comment.

Fixed up technique used to find inner pointer type, put TODO's


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750

Files:
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
  clang/test/Analysis/Inputs/system-header-simulator-cxx.h
  clang/test/Analysis/smart-ptr-text-output.cpp

Index: clang/test/Analysis/smart-ptr-text-output.cpp
===
--- clang/test/Analysis/smart-ptr-text-output.cpp
+++ clang/test/Analysis/smart-ptr-text-output.cpp
@@ -1,3 +1,8 @@
+// RUN: %clang_analyze_cc1\
+// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr\
+// RUN:  -analyzer-config cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
+// RUN:  -analyzer-output=text -std=c++20 %s -verify=expected
+
 // RUN: %clang_analyze_cc1\
 // RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr\
 // RUN:  -analyzer-config cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
@@ -313,3 +318,27 @@
 // expected-note@-1{{Dereference of null smart pointer 'P'}}
   }
 }
+
+void makeUniqueReturnsNonNullUniquePtr() {
+  auto P = std::make_unique();
+  if (!P) {   // expected-note {{Taking false branch}}
+P->foo(); // should have no warning here, path is impossible
+  }
+  P.reset(); // expected-note {{Smart pointer 'P' reset using a null value}}
+  // Now P is null
+  if (!P) {
+// expected-note@-1 {{Taking true branch}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+// expected-note@-1{{Dereference of null smart pointer 'P'}}
+  }
+}
+
+#if __cplusplus >= 202002L
+
+void makeUniqueForOverwriteReturnsNullUniquePtr() {
+  auto P = std::make_unique_for_overwrite(); // expected-note {{std::unique_ptr 'P' constructed by std::make_unique_for_overwrite is null}}
+  *P;   // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+  // expected-note@-1{{Dereference of null smart pointer 'P'}}
+}
+
+#endif
Index: clang/test/Analysis/Inputs/system-header-simulator-cxx.h
===
--- clang/test/Analysis/Inputs/system-header-simulator-cxx.h
+++ clang/test/Analysis/Inputs/system-header-simulator-cxx.h
@@ -978,6 +978,17 @@
 void swap(unique_ptr , unique_ptr ) noexcept {
   x.swap(y);
 }
+
+template 
+unique_ptr make_unique(Args &&...args);
+
+#if __cplusplus >= 202002L
+
+template 
+unique_ptr make_unique_for_overwrite();
+
+#endif
+
 } // namespace std
 #endif
 
Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
@@ -35,6 +35,7 @@
 using namespace ento;
 
 namespace {
+
 class SmartPtrModeling
 : public Checker {
@@ -76,6 +77,9 @@
   {{"release"}, ::handleRelease},
   {{"swap", 1}, ::handleSwap},
   {{"get"}, ::handleGet}};
+  const CallDescription StdMakeUniqueCall{{"std", "make_unique"}};
+  const CallDescription StdMakeUniqueForOverwriteCall{
+  {"std", "make_unique_for_overwrite"}};
 };
 } // end of anonymous namespace
 
@@ -135,6 +139,21 @@
   return State;
 }
 
+// This is for use with standalone-functions like std::make_unique,
+// std::make_unique_for_overwrite, etc. It reads the template parameter and
+// returns the pointer type corresponding to it,
+static QualType getPointerTypeFromTemplateArg(const CallEvent ,
+  CheckerContext ) {
+  const auto *FD = dyn_cast_or_null(Call.getDecl());
+  if (!FD || !FD->isFunctionTemplateSpecialization())
+return {};
+  const auto  = FD->getTemplateSpecializationArgs()->asArray();
+  if (TemplateArgs.size() == 0)
+return {};
+  auto ValueType = TemplateArgs[0].getAsType();
+  return C.getASTContext().getPointerType(ValueType.getCanonicalType());
+}
+
 // Helper method to get the inner pointer type of specialized smart pointer
 // Returns empty type if not found valid inner pointer type.
 static QualType getInnerPointerType(const CallEvent , CheckerContext ) {
@@ -177,7 +196,60 @@
 
 bool SmartPtrModeling::evalCall(const CallEvent ,
 CheckerContext ) const {
+
   ProgramStateRef State = C.getState();
+
+  if (Call.isCalled(StdMakeUniqueCall)) {
+const Optional ThisRegionOpt = Call.getReturnValueUnderConstruction();
+if (!ThisRegionOpt)
+  return false;
+
+const auto PtrVal = C.getSValBuilder().conjureSymbolVal(
+Call.getOriginExpr(), C.getLocationContext(),
+getPointerTypeFromTemplateArg(Call, C), C.blockCount());
+
+const MemRegion *ThisRegion = ThisRegionOpt->getAsRegion();
+State = State->set(ThisRegion, 

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:201-202
+const TypedValueRegion *TVR = llvm::dyn_cast(ThisRegion);
+assert(TVR && "expected std::make_unique to return a std::unique_ptr "
+  "object (which is typed)");
+const QualType InnerPtrType =

Untyped region isn't a region without a type; everything has a type. Untyped 
region is when we //don't know// the type. A typical situation that produces 
untyped region is when the region comes in through a void pointer.

I vaguely remember that one way to trick your specific code may be to do
```lang=c++
std::unique_ptr foo() {
  return make_unique(123);
}
```
which will RVO into an unknown region. I also wouldn't rely on it being typed 
in all other cases.

A much safer way to access the inner pointer type would be to query the 
function's template parameter.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:214-217
+auto  = State->getStateManager().getOwningEngine();
+State = Engine.updateObjectsUnderConstruction(
+*ThisRegionOpt, nullptr, State, C.getLocationContext(),
+Call.getConstructionContext(), {});

I suggest a `TODO: ExprEngine should do this for us.`.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:219
+
+C.addTransition(State);
+return true;

Do we need a note here as well? I guess we don't because we'll never emit null 
dereference reports against a non-null pointer. But if we later emit more 
sophisticated bug reports, we might need one. Maybe leave a comment?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750

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

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD updated this revision to Diff 351485.
RedDocMD added a comment.

Removed un-necessary check


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750

Files:
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
  clang/test/Analysis/Inputs/system-header-simulator-cxx.h
  clang/test/Analysis/smart-ptr-text-output.cpp

Index: clang/test/Analysis/smart-ptr-text-output.cpp
===
--- clang/test/Analysis/smart-ptr-text-output.cpp
+++ clang/test/Analysis/smart-ptr-text-output.cpp
@@ -1,3 +1,8 @@
+// RUN: %clang_analyze_cc1\
+// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr\
+// RUN:  -analyzer-config cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
+// RUN:  -analyzer-output=text -std=c++20 %s -verify=expected
+
 // RUN: %clang_analyze_cc1\
 // RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr\
 // RUN:  -analyzer-config cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
@@ -313,3 +318,27 @@
 // expected-note@-1{{Dereference of null smart pointer 'P'}}
   }
 }
+
+void makeUniqueReturnsNonNullUniquePtr() {
+  auto P = std::make_unique();
+  if (!P) {   // expected-note {{Taking false branch}}
+P->foo(); // should have no warning here, path is impossible
+  }
+  P.reset(); // expected-note {{Smart pointer 'P' reset using a null value}}
+  // Now P is null
+  if (!P) {
+// expected-note@-1 {{Taking true branch}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+// expected-note@-1{{Dereference of null smart pointer 'P'}}
+  }
+}
+
+#if __cplusplus >= 202002L
+
+void makeUniqueForOverwriteReturnsNullUniquePtr() {
+  auto P = std::make_unique_for_overwrite(); // expected-note {{std::unique_ptr 'P' constructed by std::make_unique_for_overwrite is null}}
+  *P;   // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+  // expected-note@-1{{Dereference of null smart pointer 'P'}}
+}
+
+#endif
Index: clang/test/Analysis/Inputs/system-header-simulator-cxx.h
===
--- clang/test/Analysis/Inputs/system-header-simulator-cxx.h
+++ clang/test/Analysis/Inputs/system-header-simulator-cxx.h
@@ -978,6 +978,17 @@
 void swap(unique_ptr , unique_ptr ) noexcept {
   x.swap(y);
 }
+
+template 
+unique_ptr make_unique(Args &&...args);
+
+#if __cplusplus >= 202002L
+
+template 
+unique_ptr make_unique_for_overwrite();
+
+#endif
+
 } // namespace std
 #endif
 
Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
@@ -35,6 +35,7 @@
 using namespace ento;
 
 namespace {
+
 class SmartPtrModeling
 : public Checker {
@@ -76,6 +77,9 @@
   {{"release"}, ::handleRelease},
   {{"swap", 1}, ::handleSwap},
   {{"get"}, ::handleGet}};
+  const CallDescription StdMakeUniqueCall{{"std", "make_unique"}};
+  const CallDescription StdMakeUniqueForOverwriteCall{
+  {"std", "make_unique_for_overwrite"}};
 };
 } // end of anonymous namespace
 
@@ -137,12 +141,8 @@
 
 // Helper method to get the inner pointer type of specialized smart pointer
 // Returns empty type if not found valid inner pointer type.
-static QualType getInnerPointerType(const CallEvent , CheckerContext ) {
-  const auto *MethodDecl = dyn_cast_or_null(Call.getDecl());
-  if (!MethodDecl || !MethodDecl->getParent())
-return {};
-
-  const auto *RecordDecl = MethodDecl->getParent();
+static QualType getInnerPointerType(const CXXRecordDecl *RecordDecl,
+CheckerContext ) {
   if (!RecordDecl || !RecordDecl->isInStdNamespace())
 return {};
 
@@ -157,6 +157,17 @@
   return C.getASTContext().getPointerType(InnerValueType.getCanonicalType());
 }
 
+// Helper method to get the inner pointer type of specialized smart pointer
+// Returns empty type if not found valid inner pointer type.
+static QualType getInnerPointerType(const CallEvent , CheckerContext ) {
+  const auto *MethodDecl = dyn_cast_or_null(Call.getDecl());
+  if (!MethodDecl->getParent())
+return {};
+
+  const auto *RecordDecl = MethodDecl->getParent();
+  return getInnerPointerType(RecordDecl, C);
+}
+
 // Helper method to pretty print region and avoid extra spacing.
 static void checkAndPrettyPrintRegion(llvm::raw_ostream ,
   const MemRegion *Region) {
@@ -177,7 +188,63 @@
 
 bool SmartPtrModeling::evalCall(const CallEvent ,
 CheckerContext ) const {
+
   ProgramStateRef State = C.getState();
+
+  if (Call.isCalled(StdMakeUniqueCall)) {
+const Optional ThisRegionOpt = 

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:164
+  const auto *MethodDecl = dyn_cast_or_null(Call.getDecl());
+  if (!MethodDecl || !MethodDecl->getParent())
+return {};

`getParent()` will assert when the MethodDecl is not a in a Record (which is 
the only way this can return null) so you can remove that check.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750

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

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD created this revision.
RedDocMD added reviewers: NoQ, vsavchenko, xazax.hun, teemperor.
Herald added subscribers: manas, steakhal, ASDenysPetrov, martong, dkrupp, 
donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, 
baloghadamsoftware.
RedDocMD requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This patch models the std::make_unique method.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D103750

Files:
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp


Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
@@ -40,6 +40,7 @@
  check::LiveSymbols> {
 
   bool isBoolConversionMethod(const CallEvent ) const;
+  bool isStdMakeUniqueCall(const CallEvent ) const;
 
 public:
   // Whether the checker should model for null dereferences of smart pointers.
@@ -68,6 +69,7 @@
   bool updateMovedSmartPointers(CheckerContext , const MemRegion *ThisRegion,
 const MemRegion *OtherSmartPtrRegion) const;
   void handleBoolConversion(const CallEvent , CheckerContext ) const;
+  void handleMakeUnique(const CallEvent , CheckerContext ) const;
 
   using SmartPtrMethodHandlerFn =
   void (SmartPtrModeling::*)(const CallEvent , CheckerContext &) 
const;
@@ -175,8 +177,31 @@
   return CD && CD->getConversionType()->isBooleanType();
 }
 
+bool SmartPtrModeling::isStdMakeUniqueCall(const CallEvent ) const {
+  if (Call.getKind() != CallEventKind::CE_Function)
+return false;
+  const auto *D = Call.getDecl();
+  if (!D)
+return false;
+  const auto *FTD = llvm::dyn_cast(D);
+  if (!FTD)
+return false;
+  if (FTD->getDeclName().isIdentifier()) {
+StringRef Name = FTD->getName();
+if (Name == "make_unique")
+  return true;
+  }
+  return false;
+}
+
 bool SmartPtrModeling::evalCall(const CallEvent ,
 CheckerContext ) const {
+
+  if (isStdMakeUniqueCall(Call)) {
+handleMakeUnique(Call, C);
+return true;
+  }
+
   ProgramStateRef State = C.getState();
   if (!smartptr::isStdSmartPtrCall(Call))
 return false;
@@ -272,6 +297,21 @@
   return C.isDifferent();
 }
 
+void SmartPtrModeling::handleMakeUnique(const CallEvent ,
+CheckerContext ) const {
+  const auto *OriginExpr = Call.getOriginExpr();
+  const auto *LocCtx = C.getLocationContext();
+  auto ExprVal = C.getSValBuilder().conjureSymbolVal(
+  OriginExpr, LocCtx, Call.getResultType(), C.blockCount());
+  ProgramStateRef State = C.getState();
+  State = State->BindExpr(OriginExpr, LocCtx, ExprVal);
+
+  // With make unique it is not possible to make a null smart pointer.
+  // So we can set the SVal for Call.getOriginExpr() to be non-null
+  State = State->assume(ExprVal, true);
+  C.addTransition(State);
+}
+
 void SmartPtrModeling::checkDeadSymbols(SymbolReaper ,
 CheckerContext ) const {
   ProgramStateRef State = C.getState();


Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
@@ -40,6 +40,7 @@
  check::LiveSymbols> {
 
   bool isBoolConversionMethod(const CallEvent ) const;
+  bool isStdMakeUniqueCall(const CallEvent ) const;
 
 public:
   // Whether the checker should model for null dereferences of smart pointers.
@@ -68,6 +69,7 @@
   bool updateMovedSmartPointers(CheckerContext , const MemRegion *ThisRegion,
 const MemRegion *OtherSmartPtrRegion) const;
   void handleBoolConversion(const CallEvent , CheckerContext ) const;
+  void handleMakeUnique(const CallEvent , CheckerContext ) const;
 
   using SmartPtrMethodHandlerFn =
   void (SmartPtrModeling::*)(const CallEvent , CheckerContext &) const;
@@ -175,8 +177,31 @@
   return CD && CD->getConversionType()->isBooleanType();
 }
 
+bool SmartPtrModeling::isStdMakeUniqueCall(const CallEvent ) const {
+  if (Call.getKind() != CallEventKind::CE_Function)
+return false;
+  const auto *D = Call.getDecl();
+  if (!D)
+return false;
+  const auto *FTD = llvm::dyn_cast(D);
+  if (!FTD)
+return false;
+  if (FTD->getDeclName().isIdentifier()) {
+StringRef Name = FTD->getName();
+if (Name == "make_unique")
+  return true;
+  }
+  return false;
+}
+
 bool SmartPtrModeling::evalCall(const CallEvent ,
 CheckerContext ) const {
+
+  if (isStdMakeUniqueCall(Call)) {
+handleMakeUnique(Call, C);
+return true;
+  }
+
   ProgramStateRef State = C.getState();
   if (!smartptr::isStdSmartPtrCall(Call))
 return false;
@@ -272,6