[PATCH] D104616: [analyzer][WIP] 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:351-352
+  case OO_##Name:  
\
+BinOpIt = BinOps.find(Spelling);   
\
+UnOpIt = UnOps.find(Spelling); 
\
+if (BinOpIt != BinOps.end())   
\

Optimization: if one of the lookups succeeds, the other is unnecessary. A bit 
premature but I think we shouldn't lose too much elegance over this.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:388-389
+
+  auto retrieveOrConjureInnerPtrVal = [, ](const Expr *E,
+   SVal S) -> SVal {
+if (S.isZeroConstant()) {

This sounds like a super common functionality. Doesn't `.get()` do the same as 
well? I think it should be de-duplicated into a top-level method.



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);

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.


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][WIP] Model comparision methods of std::unique_ptr

[Deep Majumder via Phabricator via cfe-commits]

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

First try at implementing conversion function from OverloadedOperatorKind to 
BinaryOperatorKind


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 

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added inline comments.



Comment at: clang/test/Analysis/smart-ptr.cpp:466
+
+  clang_analyzer_eval(ptr == ptr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr > ptr);  // expected-warning{{FALSE}}

xazax.hun wrote:
> RedDocMD wrote:
> > xazax.hun wrote:
> > > Putting tests like this on the same path can be risky. Tests might split 
> > > the execution path (maybe not now, but in the future). I think it might 
> > > be more future proof to have a large switch statement that switches on an 
> > > unknown value and put the tests in separate cases. 
> > I didn't quite get you.
> You remember this in the other patch:
> ```
> 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.
> ```
> 
> It looks like this does not happen for overloaded comparison operators at the 
> moment. But we might want to do that in the future (@NoW what do you think). 
> I was wondering, if we want to future proof these test cases for that 
> behavior. But looking at the test cases again, you only have two, where the 
> expected result is unknown, and they are at the very end. So feel free to 
> ignore this and leave the code as is.
Then perhaps I should leave a comment to indicate this possibility.


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][WIP] 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:336
+  BinaryOperatorKind BOK;
+  switch (OOK) {
+  case OO_EqualEqual:

Btw, if we do not have a helper yet to translate between these enums in the 
analyer, we should create one. Could you look for some other places in the 
analyzer code base to double check? 


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][WIP] 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:329
+  return *Ptr;
+return C.getSValBuilder().conjureSymbolVal(
+E, C.getLocationContext(),

In case we conjure a new symbol, we want this stored in the `TrackedRegionMap`. 
So the analyzer can correctly record the constraints between the two symbols. 



Comment at: clang/test/Analysis/smart-ptr.cpp:466
+
+  clang_analyzer_eval(ptr == ptr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr > ptr);  // expected-warning{{FALSE}}

RedDocMD wrote:
> xazax.hun wrote:
> > Putting tests like this on the same path can be risky. Tests might split 
> > the execution path (maybe not now, but in the future). I think it might be 
> > more future proof to have a large switch statement that switches on an 
> > unknown value and put the tests in separate cases. 
> I didn't quite get you.
You remember this in the other patch:
```
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.
```

It looks like this does not happen for overloaded comparison operators at the 
moment. But we might want to do that in the future (@NoW what do you think). I 
was wondering, if we want to future proof these test cases for that behavior. 
But looking at the test cases again, you only have two, where the expected 
result is unknown, and they are at the very end. So feel free to ignore this 
and leave the code as is.


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][WIP] Model comparision methods of std::unique_ptr

[Deep Majumder via Phabricator via cfe-commits]

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

Removed dump statement


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
@@ -68,6 +68,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 +90,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 || !RD->getDeclContext()->isStdNamespace())
 return false;
 
-  if (RecordDecl->getDeclName().isIdentifier()) {
-StringRef Name = RecordDecl->getName();
+  if (RD->getDeclName().isIdentifier()) {
+StringRef Name = RD->getName();
 return Name == "shared_ptr" || Name == "unique_ptr" || Name == "weak_ptr";
   }
   return false;
 }
 
+bool isStdSmartPtr(const Expr *E) {
+ 

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added inline comments.



Comment at: clang/test/Analysis/smart-ptr.cpp:466
+
+  clang_analyzer_eval(ptr == ptr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr > ptr);  // expected-warning{{FALSE}}

xazax.hun wrote:
> Putting tests like this on the same path can be risky. Tests might split the 
> execution path (maybe not now, but in the future). I think it might be more 
> future proof to have a large switch statement that switches on an unknown 
> value and put the tests in separate cases. 
I didn't quite get you.


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][WIP] Model comparision methods of std::unique_ptr

[Deep Majumder via Phabricator via cfe-commits]

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

Refactored code, removed duplications, fixed tests, added some more


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
@@ -68,6 +68,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 +90,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 || !RD->getDeclContext()->isStdNamespace())
 return false;
 
-  if (RecordDecl->getDeclName().isIdentifier()) {
-StringRef Name = RecordDecl->getName();
+  if (RD->getDeclName().isIdentifier()) {
+StringRef Name = RD->getName();
 return Name == "shared_ptr" || Name == "unique_ptr" || Name == "weak_ptr";
   }
   return 

[PATCH] D104616: [analyzer][WIP] 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:301
+  const OverloadedOperatorKind OOK = FD->getOverloadedOperator();
+  if (!(OOK == OO_Equal || OOK == OO_ExclaimEqual || OOK == OO_Less ||
+OOK == OO_LessEqual || OOK == OO_Greater || OOK == OO_GreaterEqual ||

xazax.hun wrote:
> So it looks like `operator<<` is the only operator we are not supporting. I 
> think it might be good to include some test code to ensure that its use does 
> not suppress warnings. (Also OK, if you decide to deal with this in a 
> follow-up PR.)
Yes that's the next patch. (and one more for `std::hash`).



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:341
+  case OO_Spaceship:
+// TODO: What would be a good thing to do here?
+  default:

xazax.hun wrote:
> Instead of marking this unreachable, I'd suggest to just return a conjured 
> symbol for now.  Usually, we should not use asserts to test user input.
Ah yes that's what is happening now. `RetVal` is initialized to a conjured 
value. If we can conclude no further, then that is what is returned - which is 
what happens here. In other cases, constraints are applied or it is replaced by 
the output from `evalBinOp`.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:354
+
+  if (FirstPtr && !SecondPtr &&
+  State->assume(FirstPtr->castAs(), false)) {

xazax.hun wrote:
> I am not sure if we actually need all this special casing. You could create 
> an `SVal` that represents a nullpointer constant and use `evalBinOp` with 
> that `SVal` when there is no symbol available.
Actually the naming is a bit misleading here, perhaps. `FirstPtr` is not the 
inner pointer itself but a pointer to the //SVal//. So the test `FirstPtr && 
!SecondPtr` checks that we know the SVal for the inner pointer of the first arg 
but we do not know that of the second arg. Using a null-pointer constant would 
not help.
However, we could use a conjured value to simplify some 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][WIP] 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:301
+  const OverloadedOperatorKind OOK = FD->getOverloadedOperator();
+  if (!(OOK == OO_Equal || OOK == OO_ExclaimEqual || OOK == OO_Less ||
+OOK == OO_LessEqual || OOK == OO_Greater || OOK == OO_GreaterEqual ||

So it looks like `operator<<` is the only operator we are not supporting. I 
think it might be good to include some test code to ensure that its use does 
not suppress warnings. (Also OK, if you decide to deal with this in a follow-up 
PR.)



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:334
+  case OO_LessEqual:
+State = State->assume(()->castAs(), true);
+break;

I think in cases where we know that the result is `true` or `false`, the 
`RetVal` should probably be a constant instead of a conjured symbol with an 
assumption. 



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:341
+  case OO_Spaceship:
+// TODO: What would be a good thing to do here?
+  default:

Instead of marking this unreachable, I'd suggest to just return a conjured 
symbol for now.  Usually, we should not use asserts to test user input.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:354
+
+  if (FirstPtr && !SecondPtr &&
+  State->assume(FirstPtr->castAs(), false)) {

I am not sure if we actually need all this special casing. You could create an 
`SVal` that represents a nullpointer constant and use `evalBinOp` with that 
`SVal` when there is no symbol available.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:394
+default:
+  llvm_unreachable("cannot reach here");
+}

`cannot reach here` is redundant information. That is already encoded in 
`llvm_unreachable`. I suggest including a message that tells the reader **why** 
is it unreachable. In this case it could be `"unexpected overloaded operator 
kind"`.



Comment at: clang/test/Analysis/smart-ptr.cpp:466
+
+  clang_analyzer_eval(ptr == ptr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr > ptr);  // expected-warning{{FALSE}}

Putting tests like this on the same path can be risky. Tests might split the 
execution path (maybe not now, but in the future). I think it might be more 
future proof to have a large switch statement that switches on an unknown value 
and put the tests in separate cases. 


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][WIP] Model comparision methods of std::unique_ptr

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

We have a failing test  here (test at line 473).
Which makes me wonder if the `handleComparision` function is at all called. 
This is something I need to check.


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][WIP] Model comparision methods of std::unique_ptr

[Deep Majumder via Phabricator via cfe-commits]

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

Removed re-invention, added tests


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/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,19 @@
 P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   }
 }
+
+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}}
+}
Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
@@ -29,6 +29,7 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
+#include "llvm/Support/ErrorHandling.h"
 #include 
 
 using namespace clang;
@@ -68,6 +69,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 +91,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 || !RD->getDeclContext()->isStdNamespace())
 return false;
 
-  if (RecordDecl->getDeclName().isIdentifier()) {
-StringRef Name = RecordDecl->getName();
+  if (RD->getDeclName().isIdentifier()) {
+StringRef Name = RD->getName();
 return Name == "shared_ptr" || Name == "unique_ptr" || Name == "weak_ptr";
   }
   return false;
 }
 
+bool isStdSmartPtr(const Expr *E) {
+  return isStdSmartPtr(E->getType()->getAsCXXRecordDecl());
+}
+
 bool isNullSmartPtr(const ProgramStateRef State, const MemRegion *ThisRegion) {
   const auto *InnerPointVal = State->get(ThisRegion);
   return InnerPointVal &&
@@ -178,6 +186,15 @@
 bool SmartPtrModeling::evalCall(const CallEvent ,
 CheckerContext ) const {
   ProgramStateRef State = C.getState();
+
+  // If any one of the arg is a unique_ptr, then
+  // we can try this function
+  if (Call.getNumArgs() == 2)
+if (smartptr::isStdSmartPtr(Call.getArgExpr(0)) ||
+smartptr::isStdSmartPtr(Call.getArgExpr(1)))
+  if (handleComparisionOp(Call, C))
+return true;
+
   if (!smartptr::isStdSmartPtrCall(Call))
 return false;
 
@@ -272,6 +289,123 @@
   return C.isDifferent();
 }
 
+bool SmartPtrModeling::handleComparisionOp(const CallEvent ,
+   CheckerContext ) const {
+  const auto *FC = dyn_cast();
+  if (!FC)
+return false;
+  const FunctionDecl *FD = FC->getDecl();
+  if (!FD->isOverloadedOperator())
+return false;
+  const OverloadedOperatorKind OOK = FD->getOverloadedOperator();
+  if (!(OOK == OO_Equal || OOK == OO_ExclaimEqual || OOK == OO_Less ||
+OOK == OO_LessEqual || OOK == OO_Greater || OOK == OO_GreaterEqual ||
+OOK == OO_Spaceship))
+return false;
+
+  SVal First = Call.getArgSVal(0);
+  SVal Second = Call.getArgSVal(1);
+
+  // There are some special cases about which we can infer about
+  // the resulting answer.
+  // For reference, there is a discussion at https://reviews.llvm.org/D104616.
+  // Also, the cppreference page is good to look at
+  // https://en.cppreference.com/w/cpp/memory/unique_ptr/operator_cmp.
+
+  ProgramStateRef State = C.getState();
+  SVal RetVal = C.getSValBuilder().conjureSymbolVal(
+  Call.getOriginExpr(), 

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

In D104616#2835061 , @xazax.hun wrote:

> In D104616#2835030 , @RedDocMD 
> wrote:
>
>> Looks like I have wasted a good deal of effort. :(
>
> Sorry about that! :( If we learned anything new in the process it was not 
> wasted effort though.

I know what I learnt - if I am writing too much code, there is probably a 
function to that. :)


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][WIP] Model comparision methods of std::unique_ptr

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

xazax.hun added a comment.

In D104616#2835030 , @RedDocMD wrote:

> Looks like I have wasted a good deal of effort. :(

Sorry about that! :( If we learned anything new in the process it was not 
wasted effort though.


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][WIP] Model comparision methods of std::unique_ptr

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

In D104616#2834770 , @xazax.hun wrote:

> In D104616#2834714 , @NoQ wrote:
>
>> Why not simply delegate this job to `assume(evalBinOp(...))` over raw 
>> pointer values, which already has all this logic written down nicely?
>
> This is what I had in mind, I just did not want to spoil it :)

Looks like I have wasted a good deal of effort. :(


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][WIP] Model comparision methods of std::unique_ptr

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

xazax.hun added a comment.

In D104616#2834714 , @NoQ wrote:

> Why not simply delegate this job to `assume(evalBinOp(...))` over raw pointer 
> values, which already has all this logic written down nicely?

This is what I had in mind, I just did not want to spoil 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][WIP] Model comparision methods of std::unique_ptr

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D104616#2830349 , @xazax.hun wrote:

> In D104616#2829705 , @RedDocMD 
> wrote:
>
>> If `(ptr1 == ptr2)` is false, we can't say anything really.
>
> Well, I think it depends. If one of the pointers is null, for some platforms, 
> we can. E.g. null < non-null is probably true on most architectures, and 
> similarly non-null < null is false. Also null <= anyptr is probably true on 
> the platforms we care about.
>
>> If they are not the same, the only way == can return true if the two smart 
>> pointers were initialized from the same raw pointer. This is of course a 
>> fatal bug in itself.
>
> Is it? E.g. two default constructed unique_ptrs both should have null as the 
> underlying pointer, they should be considered equal, are not the same object, 
> and this is not a fatal bug.

Why not simply delegate this job to `assume(evalBinOp(...))` over raw pointer 
values, which already has all this logic written down nicely?


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][WIP] Model comparision methods of std::unique_ptr

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Looks like a solid start!




Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:301
+
+  bool addTransition = false;
+  ProgramStateRef State = C.getState();

nit: variable names should be capitalized



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:321-323
+State =
+State->assume(llvm::dyn_cast(RetVal), true);
+addTransition = true;

Another idea here.
Instead of repeating:
```
State = State->assume(llvm::dyn_cast(RetVal), VALUE);
addTransition = true;
```
and having boolean `addTransition`, you can have `Optional CompResult` 
and do:
```
CompResult = VALUE;
```
And do the actual assumption at the bottom section when `CompResult.hasValue()`



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:334
+  default:
+assert(false && "shouldn't reach here");
+  }

nit: `llvm_unreachable` instead of `assert(false)`



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:345-364
+  if (FirstPtr && !SecondPtr &&
+  State->assume(llvm::dyn_cast(*FirstPtr),
+false)) {
+// FirstPtr is null, SecondPtr is unknown
+if (OOK == OO_LessEqual) {
+  State =
+  State->assume(llvm::dyn_cast(RetVal), 
true);

These are also symmetrical.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:373-391
+  switch (OOK) {
+  case OO_Equal:
+  case OO_GreaterEqual:
+  case OO_LessEqual:
+State = State->assume(llvm::dyn_cast(RetVal),
+  true);
+addTransition = true;

This switch exactly repeats the previous switch.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:394-436
+if (FirstNull && !SecondNull) {
+  switch (OOK) {
+  case OO_Less:
+  case OO_LessEqual:
+State = State->assume(llvm::dyn_cast(RetVal),
+  true);
+addTransition = true;

These are symmetrical, is there a way to implement it without this boilerplate?


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][WIP] Model comparision methods of std::unique_ptr

[Deep Majumder via Phabricator via cfe-commits]

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

Logic for handling special cases, when both are unique_ptr


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/SmartPtrModeling.cpp

Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
@@ -68,6 +68,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;
@@ -264,6 +265,11 @@
   if (handleAssignOp(Call, C))
 return true;
 
+  // FIXME: This won't work
+  // Check for one arg or the other being a smart-ptr.
+  if (handleComparisionOp(Call, C))
+return true;
+
   const SmartPtrMethodHandlerFn *Handler = SmartPtrMethodHandlers.lookup(Call);
   if (!Handler)
 return false;
@@ -272,6 +278,177 @@
   return C.isDifferent();
 }
 
+bool SmartPtrModeling::handleComparisionOp(const CallEvent ,
+   CheckerContext ) const {
+  const auto *MC = llvm::dyn_cast();
+  if (!MC)
+return false;
+  const OverloadedOperatorKind OOK = MC->getOperator();
+  if (!(OOK == OO_Equal || OOK == OO_ExclaimEqual || OOK == OO_Less ||
+OOK == OO_LessEqual || OOK == OO_Greater || OOK == OO_GreaterEqual ||
+OOK == OO_Spaceship))
+return false;
+
+  SVal First = Call.getArgSVal(0);
+  SVal Second = Call.getArgSVal(1);
+
+  // There are some special cases about which we can infer about
+  // the resulting answer.
+  // For reference, there is a discussion at https://reviews.llvm.org/D104616.
+  // Also, the cppreference page is good to look at
+  // https://en.cppreference.com/w/cpp/memory/unique_ptr/operator_cmp.
+
+  bool addTransition = false;
+  ProgramStateRef State = C.getState();
+  SVal RetVal = C.getSValBuilder().conjureSymbolVal(
+  Call.getOriginExpr(), C.getLocationContext(), Call.getResultType(),
+  C.blockCount());
+
+  if (!First.isZeroConstant() && !Second.isZeroConstant()) {
+// Neither are nullptr, so they are both std::unique_ptr. (whether the smart
+// pointers are null or not is an entire different question.)
+const MemRegion *FirstReg = First.getAsRegion();
+const MemRegion *SecondReg = Second.getAsRegion();
+if (!FirstReg || !SecondReg)
+  return false;
+
+// First and Second may refer to the same object
+if (FirstReg == SecondReg) {
+  switch (OOK) {
+  case OO_Equal:
+  case OO_GreaterEqual:
+  case OO_LessEqual:
+State =
+State->assume(llvm::dyn_cast(RetVal), true);
+addTransition = true;
+break;
+  case OO_Greater:
+  case OO_Less:
+State =
+State->assume(llvm::dyn_cast(RetVal), false);
+addTransition = true;
+break;
+  case OO_Spaceship:
+// TODO: What would be a good thing to do here?
+  default:
+assert(false && "shouldn't reach here");
+  }
+} else {
+  const auto *FirstPtr = State->get(FirstReg);
+  const auto *SecondPtr = State->get(SecondReg);
+
+  if (!FirstPtr && !SecondPtr)
+return false;
+
+  // Now, we know the inner pointer of at least one
+
+  if (FirstPtr && !SecondPtr &&
+  State->assume(llvm::dyn_cast(*FirstPtr),
+false)) {
+// FirstPtr is null, SecondPtr is unknown
+if (OOK == OO_LessEqual) {
+  State =
+  State->assume(llvm::dyn_cast(RetVal), true);
+  addTransition = true;
+}
+  }
+  if (SecondPtr && !FirstPtr &&
+  State->assume(llvm::dyn_cast(*SecondPtr),
+false)) {
+// SecondPtr is null, FirstPtr is unknown
+if (OOK == OO_GreaterEqual) {
+  State =
+  State->assume(llvm::dyn_cast(RetVal), true);
+  addTransition = true;
+}
+  }
+
+  if (FirstPtr && SecondPtr) {
+bool FirstNull{!State->assume(
+llvm::dyn_cast(*FirstPtr), true)};
+bool SecondNull{!State->assume(
+llvm::dyn_cast(*SecondPtr), true)};
+
+if (FirstNull && SecondNull) {
+  switch (OOK) {
+  case OO_Equal:
+  case OO_GreaterEqual:
+  case OO_LessEqual:
+State = State->assume(llvm::dyn_cast(RetVal),
+  true);
+addTransition = true;
+break;
+  case OO_Greater:
+  case OO_Less:
+

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

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

xazax.hun added a comment.

In D104616#2829705 , @RedDocMD wrote:

> If `(ptr1 == ptr2)` is false, we can't say anything really.

Well, I think it depends. If one of the pointers is null, for some platforms, 
we can. E.g. null < non-null is probably true on most architectures, and 
similarly non-null < null is false. Also null <= anyptr is probably true on the 
platforms we care about.

> If they are not the same, the only way == can return true if the two smart 
> pointers were initialized from the same raw pointer. This is of course a 
> fatal bug in itself.

Is it? E.g. two default constructed unique_ptrs both should have null as the 
underlying pointer, they should be considered equal, are not the same object, 
and this is not a fatal bug.


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][WIP] Model comparision methods of std::unique_ptr

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D104616#2829705 , @RedDocMD wrote:

> The only method that I think can be realistically modelled is `==` (and thus 
> `!=`). If both the operands refer to the same `unique_ptr`, we know `==` 
> returns true. If they are not the same, the only way `==` can return true if 
> the two smart pointers were initialized from the //same// raw pointer. This 
> is of course a fatal bug in itself. So perhaps we can ignore this case and 
> only consider the first case.
> The ordering operators I guess can't be handled because there is no way to 
> statically tell in general the address of some value. We have the following 
> deductions, nevertheless, mathematically:
> Let `ptr1` and `ptr2` be two `std::unique_ptr` objects.
> If `(ptr1 == ptr2)` is true:
>
> - `ptr1 < ptr2` is false
> - `ptr1 > ptr2` is false
> - `ptr1 <= ptr2` is true
> - `ptr1 >= ptr2` is true
>
> If `(ptr1 == ptr2)` is false, we can't say anything really.

Sounds good to me!


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][WIP] Model comparision methods of std::unique_ptr

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

The only method that I think can be realistically modelled is `==` (and thus 
`!=`). If both the operands refer to the same `unique_ptr`, we know `==` 
returns true. If they are not the same, the only way `==` can return true if 
the two smart pointers were initialized from the //same// raw pointer. This is 
of course a fatal bug in itself. So perhaps we can ignore this case and only 
consider the first case.
The ordering operators I guess can't be handled because there is no way to 
statically tell in general the address of some value. We have the following 
deductions, nevertheless, mathematically:
Let `ptr1` and `ptr2` be two `std::unique_ptr` objects.
If `(ptr1 == ptr2)` is true:

- `ptr1 < ptr2` is false
- `ptr1 > ptr2` is false
- `ptr1 <= ptr2` is true
- `ptr1 >= ptr2` is true

If `(ptr1 == ptr2)` is false, we can't say anything really.


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][WIP] Model comparision methods of std::unique_ptr

[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 handles all the comparision methods (defined via overloaded
operators) on std::unique_ptr. These operators compare the underlying
pointers, which makes it difficult to model the result.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D104616

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
@@ -68,6 +68,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;
@@ -264,6 +265,9 @@
   if (handleAssignOp(Call, C))
 return true;
 
+  if (handleComparisionOp(Call, C))
+return true;
+
   const SmartPtrMethodHandlerFn *Handler = SmartPtrMethodHandlers.lookup(Call);
   if (!Handler)
 return false;
@@ -272,6 +276,26 @@
   return C.isDifferent();
 }
 
+bool SmartPtrModeling::handleComparisionOp(const CallEvent ,
+   CheckerContext ) const {
+  const auto *MC = llvm::dyn_cast();
+  if (!MC)
+return false;
+  const OverloadedOperatorKind OOK = MC->getOverloadedOperator();
+  if (!(OOK == OO_Equal || OOK == OO_ExclaimEqual || OOK == OO_Less ||
+OOK == OO_LessEqual || OOK == OO_Greater || OOK == OO_GreaterEqual ||
+OOK == OO_Spaceship))
+return false;
+
+  // TODO: We had better put some useful modelling here.
+  // But the problem is that all these operators act on the raw
+  // inner pointer (there are special cases for comparision with nullptr).
+  // So, we are not really comparing the value pointed to by the pointer
+  // but the *pointer* itself, which is just an address, which is not
+  // something that symbolic evaluation is concerned about.
+  return true;
+}
+
 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
@@ -68,6 +68,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;
@@ -264,6 +265,9 @@
   if (handleAssignOp(Call, C))
 return true;
 
+  if (handleComparisionOp(Call, C))
+return true;
+
   const SmartPtrMethodHandlerFn *Handler = SmartPtrMethodHandlers.lookup(Call);
   if (!Handler)
 return false;
@@ -272,6 +276,26 @@
   return C.isDifferent();
 }
 
+bool SmartPtrModeling::handleComparisionOp(const CallEvent ,
+   CheckerContext ) const {
+  const auto *MC = llvm::dyn_cast();
+  if (!MC)
+return false;
+  const OverloadedOperatorKind OOK = MC->getOverloadedOperator();
+  if (!(OOK == OO_Equal || OOK == OO_ExclaimEqual || OOK == OO_Less ||
+OOK == OO_LessEqual || OOK == OO_Greater || OOK == OO_GreaterEqual ||
+OOK == OO_Spaceship))
+return false;
+
+  // TODO: We had better put some useful modelling here.
+  // But the problem is that all these operators act on the raw
+  // inner pointer (there are special cases for comparision with nullptr).
+  // So, we are not really comparing the value pointed to by the pointer
+  // but the *pointer* itself, which is just an address, which is not
+  // something that symbolic evaluation is concerned about.
+  return true;
+}
+
 void SmartPtrModeling::checkDeadSymbols(SymbolReaper ,
 CheckerContext ) const {
   ProgramStateRef State = C.getState();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits