[PATCH] D104616: [analyzer] Model comparision methods of std::unique_ptr

[Deep Majumder via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG48688257c52d: [analyzer] Model comparision methods of 
std::unique_ptr (authored by RedDocMD).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104616

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

Index: clang/test/Analysis/smart-ptr.cpp
===
--- clang/test/Analysis/smart-ptr.cpp
+++ clang/test/Analysis/smart-ptr.cpp
@@ -457,3 +457,52 @@
 P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   }
 }
+
+// The following is a silly function,
+// but serves to test if we are picking out
+// standard comparision functions from custom ones.
+template 
+bool operator<(std::unique_ptr , double d);
+
+void uniquePtrComparision(std::unique_ptr unknownPtr) {
+  auto ptr = std::unique_ptr(new int(13));
+  auto nullPtr = std::unique_ptr();
+  auto otherPtr = std::unique_ptr(new int(29));
+
+  clang_analyzer_eval(ptr == ptr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr > ptr);  // expected-warning{{FALSE}}
+  clang_analyzer_eval(ptr <= ptr); // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(nullPtr <= unknownPtr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(unknownPtr >= nullPtr); // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(ptr != otherPtr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr > nullPtr);   // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(ptr != nullptr);// expected-warning{{TRUE}}
+  clang_analyzer_eval(nullPtr != nullptr);// expected-warning{{FALSE}}
+  clang_analyzer_eval(nullptr <= unknownPtr); // expected-warning{{TRUE}}
+}
+
+void uniquePtrComparisionStateSplitting(std::unique_ptr unknownPtr) {
+  auto ptr = std::unique_ptr(new int(13));
+
+  clang_analyzer_eval(ptr > unknownPtr); // expected-warning{{TRUE}}
+  // expected-warning@-1{{FALSE}}
+}
+
+void uniquePtrComparisionDifferingTypes(std::unique_ptr unknownPtr) {
+  auto ptr = std::unique_ptr(new int(13));
+  auto nullPtr = std::unique_ptr();
+  auto otherPtr = std::unique_ptr(new double(3.14));
+
+  clang_analyzer_eval(nullPtr <= unknownPtr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(unknownPtr >= nullPtr); // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(ptr != otherPtr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr > nullPtr);   // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(ptr != nullptr);// expected-warning{{TRUE}}
+  clang_analyzer_eval(nullPtr != nullptr);// expected-warning{{FALSE}}
+  clang_analyzer_eval(nullptr <= unknownPtr); // expected-warning{{TRUE}}
+}
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,61 @@
 void swap(unique_ptr , unique_ptr ) noexcept {
   x.swap(y);
 }
+
+template 
+bool operator==(const unique_ptr , const unique_ptr );
+
+template 
+bool operator!=(const unique_ptr , const unique_ptr );
+
+template 
+bool operator<(const unique_ptr , const unique_ptr );
+
+template 
+bool operator>(const unique_ptr , const unique_ptr );
+
+template 
+bool operator<=(const unique_ptr , const unique_ptr );
+
+template 
+bool operator>=(const unique_ptr , const unique_ptr );
+
+template 
+bool operator==(const unique_ptr , nullptr_t y);
+
+template 
+bool operator!=(const unique_ptr , nullptr_t y);
+
+template 
+bool operator<(const unique_ptr , nullptr_t y);
+
+template 
+bool operator>(const unique_ptr , nullptr_t y);
+
+template 
+bool operator<=(const unique_ptr , nullptr_t y);
+
+template 
+bool operator>=(const unique_ptr , nullptr_t y);
+
+template 
+bool operator==(nullptr_t x, const unique_ptr );
+
+template 
+bool operator!=(nullptr_t x, const unique_ptr );
+
+template 
+bool operator>(nullptr_t x, const unique_ptr );
+
+template 
+bool operator<(nullptr_t x, const unique_ptr );
+
+template 
+bool operator>=(nullptr_t x, const unique_ptr );
+
+template 
+bool operator<=(nullptr_t x, const unique_ptr );
+
 } // namespace std
 #endif
 
Index: clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
===
--- clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
+++ clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
@@ -148,5 +148,39 @@
   return IntValue.getSExtValue();
 }
 
+OperatorKind operationKindFromOverloadedOperator(OverloadedOperatorKind OOK,
+ bool 

[PATCH] D104616: [analyzer] Model comparision methods of std::unique_ptr

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

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

The latest version looks good to me, let's land this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104616

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

[PATCH] D104616: [analyzer] Model comparision methods of std::unique_ptr

[Deep Majumder via Phabricator via cfe-commits]

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

Major bug fix


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104616

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

Index: clang/test/Analysis/smart-ptr.cpp
===
--- clang/test/Analysis/smart-ptr.cpp
+++ clang/test/Analysis/smart-ptr.cpp
@@ -457,3 +457,52 @@
 P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   }
 }
+
+// The following is a silly function,
+// but serves to test if we are picking out
+// standard comparision functions from custom ones.
+template 
+bool operator<(std::unique_ptr , double d);
+
+void uniquePtrComparision(std::unique_ptr unknownPtr) {
+  auto ptr = std::unique_ptr(new int(13));
+  auto nullPtr = std::unique_ptr();
+  auto otherPtr = std::unique_ptr(new int(29));
+
+  clang_analyzer_eval(ptr == ptr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr > ptr);  // expected-warning{{FALSE}}
+  clang_analyzer_eval(ptr <= ptr); // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(nullPtr <= unknownPtr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(unknownPtr >= nullPtr); // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(ptr != otherPtr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr > nullPtr);   // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(ptr != nullptr);// expected-warning{{TRUE}}
+  clang_analyzer_eval(nullPtr != nullptr);// expected-warning{{FALSE}}
+  clang_analyzer_eval(nullptr <= unknownPtr); // expected-warning{{TRUE}}
+}
+
+void uniquePtrComparisionStateSplitting(std::unique_ptr unknownPtr) {
+  auto ptr = std::unique_ptr(new int(13));
+
+  clang_analyzer_eval(ptr > unknownPtr); // expected-warning{{TRUE}}
+  // expected-warning@-1{{FALSE}}
+}
+
+void uniquePtrComparisionDifferingTypes(std::unique_ptr unknownPtr) {
+  auto ptr = std::unique_ptr(new int(13));
+  auto nullPtr = std::unique_ptr();
+  auto otherPtr = std::unique_ptr(new double(3.14));
+
+  clang_analyzer_eval(nullPtr <= unknownPtr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(unknownPtr >= nullPtr); // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(ptr != otherPtr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr > nullPtr);   // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(ptr != nullptr);// expected-warning{{TRUE}}
+  clang_analyzer_eval(nullPtr != nullptr);// expected-warning{{FALSE}}
+  clang_analyzer_eval(nullptr <= unknownPtr); // expected-warning{{TRUE}}
+}
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,61 @@
 void swap(unique_ptr , unique_ptr ) noexcept {
   x.swap(y);
 }
+
+template 
+bool operator==(const unique_ptr , const unique_ptr );
+
+template 
+bool operator!=(const unique_ptr , const unique_ptr );
+
+template 
+bool operator<(const unique_ptr , const unique_ptr );
+
+template 
+bool operator>(const unique_ptr , const unique_ptr );
+
+template 
+bool operator<=(const unique_ptr , const unique_ptr );
+
+template 
+bool operator>=(const unique_ptr , const unique_ptr );
+
+template 
+bool operator==(const unique_ptr , nullptr_t y);
+
+template 
+bool operator!=(const unique_ptr , nullptr_t y);
+
+template 
+bool operator<(const unique_ptr , nullptr_t y);
+
+template 
+bool operator>(const unique_ptr , nullptr_t y);
+
+template 
+bool operator<=(const unique_ptr , nullptr_t y);
+
+template 
+bool operator>=(const unique_ptr , nullptr_t y);
+
+template 
+bool operator==(nullptr_t x, const unique_ptr );
+
+template 
+bool operator!=(nullptr_t x, const unique_ptr );
+
+template 
+bool operator>(nullptr_t x, const unique_ptr );
+
+template 
+bool operator<(nullptr_t x, const unique_ptr );
+
+template 
+bool operator>=(nullptr_t x, const unique_ptr );
+
+template 
+bool operator<=(nullptr_t x, const unique_ptr );
+
 } // namespace std
 #endif
 
Index: clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
===
--- clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
+++ clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
@@ -148,5 +148,39 @@
   return IntValue.getSExtValue();
 }
 
+OperatorKind operationKindFromOverloadedOperator(OverloadedOperatorKind OOK,
+ bool IsBinary) {
+  llvm::StringMap BinOps{
+#define BINARY_OPERATION(Name, Spelling) {Spelling, 

[PATCH] D104616: [analyzer] Model comparision methods of std::unique_ptr

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

xazax.hun requested changes to this revision.
xazax.hun added inline comments.
This revision now requires changes to proceed.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:350
+QualType Type = getInnerPointerType(C, E->getType()->getAsCXXRecordDecl());
+std::tie(Val, NewState) = retrieveOrConjureInnerPtrVal(Reg, E, Type, C);
+return {Val, NewState};

Nit: couldn't you just ` return retrieveOrConjureInnerPtrVal(Reg, E, Type, C);` 
instead of all the ceremony with `std::tie`?



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:365
+  std::tie(FirstPtrVal, State) = makeSValFor(FirstExpr, First);
+  std::tie(SecondPtrVal, State) = makeSValFor(SecondExpr, Second);
+  BinaryOperatorKind BOK =

I think we might end up losing some information here. Imagine both 
`makeSValFor` conjuring a new symbol. Only one of the symbols will be stored in 
the state since capturing happend once, when you created the lambda. Maybe it 
is better to ask for a state in `makeSValFor` as a parameter instead of doing a 
lambda capture. 


Moreover, `retrieveOrConjureInnerPtrVal` will not even look at the state 
captured by `makeSValFor`. It will ask the `CheckerContext` to get the state, 
but that state is the state from the beginning of the function that does not 
have any of the modifications you made to it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104616

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

[PATCH] D104616: [analyzer] Model comparision methods of std::unique_ptr

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

If everybody's happy let's commit :)




Comment at: clang/test/Analysis/smart-ptr.cpp:467
+
+void uniquePtrComparision(std::unique_ptr unknownPtr) {
+  auto ptr = std::unique_ptr(new int(13));





Comment at: clang/test/Analysis/smart-ptr.cpp:487
+
+void uniquePtrComparisionStateSplitting(std::unique_ptr unknownPtr) {
+  auto ptr = std::unique_ptr(new int(13));





Comment at: clang/test/Analysis/smart-ptr.cpp:494
+
+void uniquePtrComparisionDifferingTypes(std::unique_ptr unknownPtr) {
+  auto ptr = std::unique_ptr(new int(13));




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104616

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

[PATCH] D104616: [analyzer] Model comparision methods of std::unique_ptr

[Deep Majumder via Phabricator via cfe-commits]

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

Little refactors


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104616

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

Index: clang/test/Analysis/smart-ptr.cpp
===
--- clang/test/Analysis/smart-ptr.cpp
+++ clang/test/Analysis/smart-ptr.cpp
@@ -457,3 +457,52 @@
 P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   }
 }
+
+// The following is a silly function,
+// but serves to test if we are picking out
+// standard comparision functions from custom ones.
+template 
+bool operator<(std::unique_ptr , double d);
+
+void uniquePtrComparision(std::unique_ptr unknownPtr) {
+  auto ptr = std::unique_ptr(new int(13));
+  auto nullPtr = std::unique_ptr();
+  auto otherPtr = std::unique_ptr(new int(29));
+
+  clang_analyzer_eval(ptr == ptr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr > ptr);  // expected-warning{{FALSE}}
+  clang_analyzer_eval(ptr <= ptr); // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(nullPtr <= unknownPtr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(unknownPtr >= nullPtr); // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(ptr != otherPtr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr > nullPtr);   // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(ptr != nullptr);// expected-warning{{TRUE}}
+  clang_analyzer_eval(nullPtr != nullptr);// expected-warning{{FALSE}}
+  clang_analyzer_eval(nullptr <= unknownPtr); // expected-warning{{TRUE}}
+}
+
+void uniquePtrComparisionStateSplitting(std::unique_ptr unknownPtr) {
+  auto ptr = std::unique_ptr(new int(13));
+
+  clang_analyzer_eval(ptr > unknownPtr); // expected-warning{{TRUE}}
+  // expected-warning@-1{{FALSE}}
+}
+
+void uniquePtrComparisionDifferingTypes(std::unique_ptr unknownPtr) {
+  auto ptr = std::unique_ptr(new int(13));
+  auto nullPtr = std::unique_ptr();
+  auto otherPtr = std::unique_ptr(new double(3.14));
+
+  clang_analyzer_eval(nullPtr <= unknownPtr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(unknownPtr >= nullPtr); // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(ptr != otherPtr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr > nullPtr);   // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(ptr != nullptr);// expected-warning{{TRUE}}
+  clang_analyzer_eval(nullPtr != nullptr);// expected-warning{{FALSE}}
+  clang_analyzer_eval(nullptr <= unknownPtr); // expected-warning{{TRUE}}
+}
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,61 @@
 void swap(unique_ptr , unique_ptr ) noexcept {
   x.swap(y);
 }
+
+template 
+bool operator==(const unique_ptr , const unique_ptr );
+
+template 
+bool operator!=(const unique_ptr , const unique_ptr );
+
+template 
+bool operator<(const unique_ptr , const unique_ptr );
+
+template 
+bool operator>(const unique_ptr , const unique_ptr );
+
+template 
+bool operator<=(const unique_ptr , const unique_ptr );
+
+template 
+bool operator>=(const unique_ptr , const unique_ptr );
+
+template 
+bool operator==(const unique_ptr , nullptr_t y);
+
+template 
+bool operator!=(const unique_ptr , nullptr_t y);
+
+template 
+bool operator<(const unique_ptr , nullptr_t y);
+
+template 
+bool operator>(const unique_ptr , nullptr_t y);
+
+template 
+bool operator<=(const unique_ptr , nullptr_t y);
+
+template 
+bool operator>=(const unique_ptr , nullptr_t y);
+
+template 
+bool operator==(nullptr_t x, const unique_ptr );
+
+template 
+bool operator!=(nullptr_t x, const unique_ptr );
+
+template 
+bool operator>(nullptr_t x, const unique_ptr );
+
+template 
+bool operator<(nullptr_t x, const unique_ptr );
+
+template 
+bool operator>=(nullptr_t x, const unique_ptr );
+
+template 
+bool operator<=(nullptr_t x, const unique_ptr );
+
 } // namespace std
 #endif
 
Index: clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
===
--- clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
+++ clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
@@ -148,5 +148,39 @@
   return IntValue.getSExtValue();
 }
 
+OperatorKind operationKindFromOverloadedOperator(OverloadedOperatorKind OOK,
+ bool IsBinary) {
+  llvm::StringMap BinOps{
+#define BINARY_OPERATION(Name, Spelling) {Spelling, 

[PATCH] D104616: [analyzer] Model comparision methods of std::unique_ptr

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD marked 4 inline comments as done.
RedDocMD added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:316
+
+class OperatorKind {
+  union {

NoQ wrote:
> One good place to put this may be `CheckerHelpers.h`. This is where we dump 
> all the stuff that's probably useful in multiple checkers but not in other 
> places.
> 
> I also wonder if you plan to support unary operators. The interesting part 
> about them is that they are sometimes ambiguous to binary operators in their 
> string representation, eg. `-`.
I guess passing a parameter to chose operator arity should work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104616

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

[PATCH] D104616: [analyzer] Model comparision methods of std::unique_ptr

[Artem Dergachev via Phabricator via cfe-commits]

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

F17741480: 704.jpg 

I only have a few nits left.




Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:35
 #include 
+#include 
 

I think you're not using this anymore.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:316
+
+class OperatorKind {
+  union {

One good place to put this may be `CheckerHelpers.h`. This is where we dump all 
the stuff that's probably useful in multiple checkers but not in other places.

I also wonder if you plan to support unary operators. The interesting part 
about them is that they are sometimes ambiguous to binary operators in their 
string representation, eg. `-`.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:435
+  operationKindFromOverloadedOperator(OOK).GetBinaryOpUnsafe();
+  auto RetVal = C.getSValBuilder().evalBinOp(
+  State, BOK, FirstPtrVal, SecondPtrVal, Call.getResultType());

`C.getSValBuilder()` is called three times now, you might want to stash it in a 
reference. There's probably not much performance benefit but the code should be 
easier to read this way.



Comment at: clang/test/Analysis/Inputs/system-header-simulator-cxx.h:983
+template 
+bool operator==(const unique_ptr , const unique_ptr );
+

I think it's a good idea to test these cross-type comparison operators as well. 
IIUC they're handled exactly the same as their same-type counterparts but it 
may uncover occasional assertion failures.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104616

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

[PATCH] D104616: [analyzer] Model comparision methods of std::unique_ptr

[Deep Majumder via Phabricator via cfe-commits]

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

Simplify SVal on state split, other refactors


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104616

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

Index: clang/test/Analysis/smart-ptr.cpp
===
--- clang/test/Analysis/smart-ptr.cpp
+++ clang/test/Analysis/smart-ptr.cpp
@@ -457,3 +457,36 @@
 P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   }
 }
+
+// The following is a silly function,
+// but serves to test if we are picking out
+// standard comparision functions from custom ones.
+template 
+bool operator<(std::unique_ptr , double d);
+
+void uniquePtrComparision(std::unique_ptr unknownPtr) {
+  auto ptr = std::unique_ptr(new int(13));
+  auto nullPtr = std::unique_ptr();
+  auto otherPtr = std::unique_ptr(new int(29));
+
+  clang_analyzer_eval(ptr == ptr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr > ptr);  // expected-warning{{FALSE}}
+  clang_analyzer_eval(ptr <= ptr); // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(nullPtr <= unknownPtr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(unknownPtr >= nullPtr); // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(ptr != otherPtr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr > nullPtr);   // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(ptr != nullptr);// expected-warning{{TRUE}}
+  clang_analyzer_eval(nullPtr != nullptr);// expected-warning{{FALSE}}
+  clang_analyzer_eval(nullptr <= unknownPtr); // expected-warning{{TRUE}}
+}
+
+void uniquePtrComparisionStateSplitting(std::unique_ptr unknownPtr) {
+  auto ptr = std::unique_ptr(new int(13));
+
+  clang_analyzer_eval(ptr > unknownPtr); // expected-warning{{TRUE}}
+  // expected-warning@-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,61 @@
 void swap(unique_ptr , unique_ptr ) noexcept {
   x.swap(y);
 }
+
+template 
+bool operator==(const unique_ptr , const unique_ptr );
+
+template 
+bool operator!=(const unique_ptr , const unique_ptr );
+
+template 
+bool operator<(const unique_ptr , const unique_ptr );
+
+template 
+bool operator>(const unique_ptr , const unique_ptr );
+
+template 
+bool operator<=(const unique_ptr , const unique_ptr );
+
+template 
+bool operator>=(const unique_ptr , const unique_ptr );
+
+template 
+bool operator==(const unique_ptr , nullptr_t y);
+
+template 
+bool operator!=(const unique_ptr , nullptr_t y);
+
+template 
+bool operator<(const unique_ptr , nullptr_t y);
+
+template 
+bool operator>(const unique_ptr , nullptr_t y);
+
+template 
+bool operator<=(const unique_ptr , nullptr_t y);
+
+template 
+bool operator>=(const unique_ptr , nullptr_t y);
+
+template 
+bool operator==(nullptr_t x, const unique_ptr );
+
+template 
+bool operator!=(nullptr_t x, const unique_ptr );
+
+template 
+bool operator>(nullptr_t x, const unique_ptr );
+
+template 
+bool operator<(nullptr_t x, const unique_ptr );
+
+template 
+bool operator>=(nullptr_t x, const unique_ptr );
+
+template 
+bool operator<=(nullptr_t x, const unique_ptr );
+
 } // namespace std
 #endif
 
Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
@@ -29,7 +29,10 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
+#include "llvm/ADT/StringMap.h"
+#include "llvm/Support/ErrorHandling.h"
 #include 
+#include 
 
 using namespace clang;
 using namespace ento;
@@ -68,6 +71,10 @@
   bool updateMovedSmartPointers(CheckerContext , const MemRegion *ThisRegion,
 const MemRegion *OtherSmartPtrRegion) const;
   void handleBoolConversion(const CallEvent , CheckerContext ) const;
+  bool handleComparisionOp(const CallEvent , CheckerContext ) const;
+  std::pair
+  retrieveOrConjureInnerPtrVal(const MemRegion *ThisRegion, const Expr *E,
+   QualType Type, CheckerContext ) const;
 
   using SmartPtrMethodHandlerFn =
   void (SmartPtrModeling::*)(const CallEvent , CheckerContext &) const;
@@ -89,18 +96,24 @@
   const auto *MethodDecl = dyn_cast_or_null(Call.getDecl());
   if (!MethodDecl || !MethodDecl->getParent())
 return false;
+  

[PATCH] D104616: [analyzer] Model comparision methods of std::unique_ptr

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD marked 3 inline comments as done.
RedDocMD added inline comments.



Comment at: clang/test/Analysis/smart-ptr.cpp:484
+  clang_analyzer_eval(nullPtr != nullptr);// expected-warning{{FALSE}}
+  clang_analyzer_eval(nullptr <= unknownPtr); // expected-warning{{TRUE}}
+}

xazax.hun wrote:
> I do not see test cases where the path is actually split. Would you add some?
Oops! I removed two tests emitting UNKNOWN and forgot to put them back in.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104616

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

[PATCH] D104616: [analyzer] Model comparision methods of std::unique_ptr

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:431
+  State, BOK, FirstPtrVal, SecondPtrVal, Call.getResultType());
+  State = State->BindExpr(Call.getOriginExpr(), C.getLocationContext(), 
RetVal);
+

Now that we've made a state split, it makes sense to bind a concrete true value 
or a concrete false value (depending on the state) instead of a symbolic value. 
This produces simpler symbolic equations for our constraint solver to handle.

I.e., something like
```lang=c++
std::tie(TrueState, FalseState) = State->assume(...);
if (TrueState)
  C.addTransition(TrueState->BindExpr(E, LCtx, SVB.makeTruthVal(true));
if (FalseState)
  C.addTransition(FalseState->BindExpr(E, LCtx, SVB.makeTruthVal(false));
```



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:435
+ProgramStateRef TrueState, FalseState;
+std::tie(TrueState, FalseState) =
+State->assume(*RetVal.getAs());

xazax.hun wrote:
> `assume` might not return a state (when the analyzer is smart enough to 
> figure out one path is infeasible). While your code would probably work as 
> is, as far as I remember we usually try to not call addTransition with `null` 
> states. 
Yeah I have a lot of anxiety about passing nulls there as well, have to look up 
the source code of these functions every time :/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104616

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

[PATCH] D104616: [analyzer] Model comparision methods of std::unique_ptr

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

xazax.hun added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:408
+  auto makeSValFor = [, , this](const Expr *E, SVal S) -> SVal {
+if (S.isZeroConstant()) {
+  return C.getSValBuilder().makeZeroVal(E->getType());

Do we want to create a new SVal in this case? Why returning `S` is insufficient?



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:416
+QualType Type = getInnerPointerType(C, E->getType()->getAsCXXRecordDecl());
+std::tie(Val, State) = retrieveOrConjureInnerPtrVal(Reg, E, Type, C);
+return Val;

While this is smart (overwriting the state here), I think this is hard to 
follow. I'd prefer this lambda returning a pair.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:435
+ProgramStateRef TrueState, FalseState;
+std::tie(TrueState, FalseState) =
+State->assume(*RetVal.getAs());

`assume` might not return a state (when the analyzer is smart enough to figure 
out one path is infeasible). While your code would probably work as is, as far 
as I remember we usually try to not call addTransition with `null` states. 



Comment at: clang/test/Analysis/smart-ptr.cpp:484
+  clang_analyzer_eval(nullPtr != nullptr);// expected-warning{{FALSE}}
+  clang_analyzer_eval(nullptr <= unknownPtr); // expected-warning{{TRUE}}
+}

I do not see test cases where the path is actually split. Would you add some?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104616

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

[PATCH] D104616: [analyzer] Model comparision methods of std::unique_ptr

[Deep Majumder via Phabricator via cfe-commits]

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

Performing state split on normal comparision ops


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104616

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

Index: clang/test/Analysis/smart-ptr.cpp
===
--- clang/test/Analysis/smart-ptr.cpp
+++ clang/test/Analysis/smart-ptr.cpp
@@ -457,3 +457,29 @@
 P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   }
 }
+
+// The following is a silly function,
+// but serves to test if we are picking out
+// standard comparision functions from custom ones.
+template 
+bool operator<(std::unique_ptr , double d);
+
+void uniquePtrComparision(std::unique_ptr unknownPtr) {
+  auto ptr = std::unique_ptr(new int(13));
+  auto nullPtr = std::unique_ptr();
+  auto otherPtr = std::unique_ptr(new int(29));
+
+  clang_analyzer_eval(ptr == ptr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr > ptr);  // expected-warning{{FALSE}}
+  clang_analyzer_eval(ptr <= ptr); // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(nullPtr <= unknownPtr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(unknownPtr >= nullPtr); // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(ptr != otherPtr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr > nullPtr);   // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(ptr != nullptr);// expected-warning{{TRUE}}
+  clang_analyzer_eval(nullPtr != nullptr);// expected-warning{{FALSE}}
+  clang_analyzer_eval(nullptr <= unknownPtr); // expected-warning{{TRUE}}
+}
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,61 @@
 void swap(unique_ptr , unique_ptr ) noexcept {
   x.swap(y);
 }
+
+template 
+bool operator==(const unique_ptr , const unique_ptr );
+
+template 
+bool operator!=(const unique_ptr , const unique_ptr );
+
+template 
+bool operator<(const unique_ptr , const unique_ptr );
+
+template 
+bool operator>(const unique_ptr , const unique_ptr );
+
+template 
+bool operator<=(const unique_ptr , const unique_ptr );
+
+template 
+bool operator>=(const unique_ptr , const unique_ptr );
+
+template 
+bool operator==(const unique_ptr , nullptr_t y);
+
+template 
+bool operator!=(const unique_ptr , nullptr_t y);
+
+template 
+bool operator<(const unique_ptr , nullptr_t y);
+
+template 
+bool operator>(const unique_ptr , nullptr_t y);
+
+template 
+bool operator<=(const unique_ptr , nullptr_t y);
+
+template 
+bool operator>=(const unique_ptr , nullptr_t y);
+
+template 
+bool operator==(nullptr_t x, const unique_ptr );
+
+template 
+bool operator!=(nullptr_t x, const unique_ptr );
+
+template 
+bool operator>(nullptr_t x, const unique_ptr );
+
+template 
+bool operator<(nullptr_t x, const unique_ptr );
+
+template 
+bool operator>=(nullptr_t x, const unique_ptr );
+
+template 
+bool operator<=(nullptr_t x, const unique_ptr );
+
 } // namespace std
 #endif
 
Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
@@ -29,7 +29,10 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
+#include "llvm/ADT/StringMap.h"
+#include "llvm/Support/ErrorHandling.h"
 #include 
+#include 
 
 using namespace clang;
 using namespace ento;
@@ -68,6 +71,10 @@
   bool updateMovedSmartPointers(CheckerContext , const MemRegion *ThisRegion,
 const MemRegion *OtherSmartPtrRegion) const;
   void handleBoolConversion(const CallEvent , CheckerContext ) const;
+  bool handleComparisionOp(const CallEvent , CheckerContext ) const;
+  std::pair
+  retrieveOrConjureInnerPtrVal(const MemRegion *ThisRegion, const Expr *E,
+   QualType Type, CheckerContext ) const;
 
   using SmartPtrMethodHandlerFn =
   void (SmartPtrModeling::*)(const CallEvent , CheckerContext &) const;
@@ -89,18 +96,24 @@
   const auto *MethodDecl = dyn_cast_or_null(Call.getDecl());
   if (!MethodDecl || !MethodDecl->getParent())
 return false;
+  return isStdSmartPtr(MethodDecl->getParent());
+}
 
-  const auto *RecordDecl = MethodDecl->getParent();
-  if (!RecordDecl || !RecordDecl->getDeclContext()->isStdNamespace())
+bool isStdSmartPtr(const CXXRecordDecl *RD) {
+  

[PATCH] D104616: [analyzer] Model comparision methods of std::unique_ptr

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:443-446
+  auto RetVal = C.getSValBuilder().evalBinOp(
+  State, BOK, FirstPtrVal, SecondPtrVal, Call.getResultType());
+  State = State->BindExpr(Call.getOriginExpr(), C.getLocationContext(), 
RetVal);
+  C.addTransition(State);

RedDocMD wrote:
> NoQ wrote:
> > Because these operators are pure boolean functions, a state split would be 
> > justified. It's pointless to call the operator unless both return values 
> > are feasible. I think you should do an `assume()` and transition into both 
> > states.
> > 
> > It might also make sense to consult `-analyzer-config eagerly-assume=` 
> > before doing it but it sounds like this option will stays true forever and 
> > we might as well remove it.
> > 
> > The spaceship operator is obviously an exceptional case. Invocation of the 
> > spaceship operator isn't a good indication that all three return values are 
> > feasible, might be only two.
> I think this comment has got displaced from its original location from 
> subsequent updates. Could you please clarify?
You can always go back in time by clicking the `|<<` button in the top-left 
corner of the comment ;)

I'm trying to say that you should introduce a state split while evaluating the 
operators, not wait for the control flow operators to do that for you, exactly 
like the static analyzer currently does for the raw comparison operators.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104616

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

[PATCH] D104616: [analyzer] Model comparision methods of std::unique_ptr

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:443-446
+  auto RetVal = C.getSValBuilder().evalBinOp(
+  State, BOK, FirstPtrVal, SecondPtrVal, Call.getResultType());
+  State = State->BindExpr(Call.getOriginExpr(), C.getLocationContext(), 
RetVal);
+  C.addTransition(State);

NoQ wrote:
> Because these operators are pure boolean functions, a state split would be 
> justified. It's pointless to call the operator unless both return values are 
> feasible. I think you should do an `assume()` and transition into both states.
> 
> It might also make sense to consult `-analyzer-config eagerly-assume=` before 
> doing it but it sounds like this option will stays true forever and we might 
> as well remove it.
> 
> The spaceship operator is obviously an exceptional case. Invocation of the 
> spaceship operator isn't a good indication that all three return values are 
> feasible, might be only two.
I think this comment has got displaced from its original location from 
subsequent updates. Could you please clarify?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104616

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

[PATCH] D104616: [analyzer] Model comparision methods of std::unique_ptr

[Deep Majumder via Phabricator via cfe-commits]

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

Refactored out common block


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104616

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

Index: clang/test/Analysis/smart-ptr.cpp
===
--- clang/test/Analysis/smart-ptr.cpp
+++ clang/test/Analysis/smart-ptr.cpp
@@ -457,3 +457,32 @@
 P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   }
 }
+
+// The following is a silly function,
+// but serves to test if we are picking out
+// standard comparision functions from custom ones.
+template 
+bool operator<(std::unique_ptr , double d);
+
+void uniquePtrComparision(std::unique_ptr unknownPtr) {
+  auto ptr = std::unique_ptr(new int(13));
+  auto nullPtr = std::unique_ptr();
+  auto otherPtr = std::unique_ptr(new int(29));
+
+  clang_analyzer_eval(ptr == ptr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr > ptr);  // expected-warning{{FALSE}}
+  clang_analyzer_eval(ptr <= ptr); // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(nullPtr <= unknownPtr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(unknownPtr >= nullPtr); // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(ptr != otherPtr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr > nullPtr);   // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(ptr != nullptr);// expected-warning{{TRUE}}
+  clang_analyzer_eval(nullPtr != nullptr);// expected-warning{{FALSE}}
+  clang_analyzer_eval(nullptr <= unknownPtr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(nullptr < unknownPtr);  // expected-warning{{UNKNOWN}}
+
+  clang_analyzer_eval(ptr < 2.0); // expected-warning{{UNKNOWN}}
+}
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,61 @@
 void swap(unique_ptr , unique_ptr ) noexcept {
   x.swap(y);
 }
+
+template 
+bool operator==(const unique_ptr , const unique_ptr );
+
+template 
+bool operator!=(const unique_ptr , const unique_ptr );
+
+template 
+bool operator<(const unique_ptr , const unique_ptr );
+
+template 
+bool operator>(const unique_ptr , const unique_ptr );
+
+template 
+bool operator<=(const unique_ptr , const unique_ptr );
+
+template 
+bool operator>=(const unique_ptr , const unique_ptr );
+
+template 
+bool operator==(const unique_ptr , nullptr_t y);
+
+template 
+bool operator!=(const unique_ptr , nullptr_t y);
+
+template 
+bool operator<(const unique_ptr , nullptr_t y);
+
+template 
+bool operator>(const unique_ptr , nullptr_t y);
+
+template 
+bool operator<=(const unique_ptr , nullptr_t y);
+
+template 
+bool operator>=(const unique_ptr , nullptr_t y);
+
+template 
+bool operator==(nullptr_t x, const unique_ptr );
+
+template 
+bool operator!=(nullptr_t x, const unique_ptr );
+
+template 
+bool operator>(nullptr_t x, const unique_ptr );
+
+template 
+bool operator<(nullptr_t x, const unique_ptr );
+
+template 
+bool operator>=(nullptr_t x, const unique_ptr );
+
+template 
+bool operator<=(nullptr_t x, const unique_ptr );
+
 } // namespace std
 #endif
 
Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
@@ -29,7 +29,10 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
+#include "llvm/ADT/StringMap.h"
+#include "llvm/Support/ErrorHandling.h"
 #include 
+#include 
 
 using namespace clang;
 using namespace ento;
@@ -68,6 +71,10 @@
   bool updateMovedSmartPointers(CheckerContext , const MemRegion *ThisRegion,
 const MemRegion *OtherSmartPtrRegion) const;
   void handleBoolConversion(const CallEvent , CheckerContext ) const;
+  bool handleComparisionOp(const CallEvent , CheckerContext ) const;
+  std::pair
+  retrieveOrConjureInnerPtrVal(const MemRegion *ThisRegion, const Expr *E,
+   QualType Type, CheckerContext ) const;
 
   using SmartPtrMethodHandlerFn =
   void (SmartPtrModeling::*)(const CallEvent , CheckerContext &) const;
@@ -89,18 +96,24 @@
   const auto *MethodDecl = dyn_cast_or_null(Call.getDecl());
   if (!MethodDecl || !MethodDecl->getParent())
 return false;
+  return isStdSmartPtr(MethodDecl->getParent());
+}
 
-  const auto *RecordDecl = 

[PATCH] D104616: [analyzer] Model comparision methods of std::unique_ptr

[Deep Majumder via Phabricator via cfe-commits]

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

Fixed bug in enum conversion


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104616

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

Index: clang/test/Analysis/smart-ptr.cpp
===
--- clang/test/Analysis/smart-ptr.cpp
+++ clang/test/Analysis/smart-ptr.cpp
@@ -457,3 +457,32 @@
 P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   }
 }
+
+// The following is a silly function,
+// but serves to test if we are picking out
+// standard comparision functions from custom ones.
+template 
+bool operator<(std::unique_ptr , double d);
+
+void uniquePtrComparision(std::unique_ptr unknownPtr) {
+  auto ptr = std::unique_ptr(new int(13));
+  auto nullPtr = std::unique_ptr();
+  auto otherPtr = std::unique_ptr(new int(29));
+
+  clang_analyzer_eval(ptr == ptr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr > ptr);  // expected-warning{{FALSE}}
+  clang_analyzer_eval(ptr <= ptr); // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(nullPtr <= unknownPtr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(unknownPtr >= nullPtr); // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(ptr != otherPtr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr > nullPtr);   // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(ptr != nullptr);// expected-warning{{TRUE}}
+  clang_analyzer_eval(nullPtr != nullptr);// expected-warning{{FALSE}}
+  clang_analyzer_eval(nullptr <= unknownPtr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(nullptr < unknownPtr);  // expected-warning{{UNKNOWN}}
+
+  clang_analyzer_eval(ptr < 2.0); // expected-warning{{UNKNOWN}}
+}
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,61 @@
 void swap(unique_ptr , unique_ptr ) noexcept {
   x.swap(y);
 }
+
+template 
+bool operator==(const unique_ptr , const unique_ptr );
+
+template 
+bool operator!=(const unique_ptr , const unique_ptr );
+
+template 
+bool operator<(const unique_ptr , const unique_ptr );
+
+template 
+bool operator>(const unique_ptr , const unique_ptr );
+
+template 
+bool operator<=(const unique_ptr , const unique_ptr );
+
+template 
+bool operator>=(const unique_ptr , const unique_ptr );
+
+template 
+bool operator==(const unique_ptr , nullptr_t y);
+
+template 
+bool operator!=(const unique_ptr , nullptr_t y);
+
+template 
+bool operator<(const unique_ptr , nullptr_t y);
+
+template 
+bool operator>(const unique_ptr , nullptr_t y);
+
+template 
+bool operator<=(const unique_ptr , nullptr_t y);
+
+template 
+bool operator>=(const unique_ptr , nullptr_t y);
+
+template 
+bool operator==(nullptr_t x, const unique_ptr );
+
+template 
+bool operator!=(nullptr_t x, const unique_ptr );
+
+template 
+bool operator>(nullptr_t x, const unique_ptr );
+
+template 
+bool operator<(nullptr_t x, const unique_ptr );
+
+template 
+bool operator>=(nullptr_t x, const unique_ptr );
+
+template 
+bool operator<=(nullptr_t x, const unique_ptr );
+
 } // namespace std
 #endif
 
Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
@@ -29,7 +29,10 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
+#include "llvm/ADT/StringMap.h"
+#include "llvm/Support/ErrorHandling.h"
 #include 
+#include 
 
 using namespace clang;
 using namespace ento;
@@ -68,6 +71,7 @@
   bool updateMovedSmartPointers(CheckerContext , const MemRegion *ThisRegion,
 const MemRegion *OtherSmartPtrRegion) const;
   void handleBoolConversion(const CallEvent , CheckerContext ) const;
+  bool handleComparisionOp(const CallEvent , CheckerContext ) const;
 
   using SmartPtrMethodHandlerFn =
   void (SmartPtrModeling::*)(const CallEvent , CheckerContext &) const;
@@ -89,18 +93,24 @@
   const auto *MethodDecl = dyn_cast_or_null(Call.getDecl());
   if (!MethodDecl || !MethodDecl->getParent())
 return false;
+  return isStdSmartPtr(MethodDecl->getParent());
+}
 
-  const auto *RecordDecl = MethodDecl->getParent();
-  if (!RecordDecl || !RecordDecl->getDeclContext()->isStdNamespace())
+bool isStdSmartPtr(const CXXRecordDecl *RD) {
+  if (!RD || 

[PATCH] D104616: [analyzer] Model comparision methods of std::unique_ptr

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I suspect that you might want to include `OperationKinds.def` instead of 
`OperationKinds.h`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104616

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

[PATCH] D104616: [analyzer] Model comparision methods of std::unique_ptr

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

Sorry for not updating. Was down with fever.
This patch does *not* work now. `operationKindFromOverloadedOperator` is broken 
because the maps don't get populated. I am not entirely sure why this is 
happening.
Will try to fix tomorrow. @NoQ, @vsavchenko, @xazax.hun, @teemperor do you have 
a hunch as to why this may be happening?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104616

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