[PATCH] D83836: [Analyzer] Add checkRegionChanges for SmartPtrModeling

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar marked an inline comment as done.
vrnithinkumar added a comment.

In D83836#2189636 , @NoQ wrote:

> These patches look like they're done, maybe let's land them?

This one also committed. 
(https://github.com/llvm/llvm-project/commit/a5609102117d2384fb73a14f37d24a0c844e3864)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83836

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

[PATCH] D83836: [Analyzer] Add checkRegionChanges for SmartPtrModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

These patches look like they're done, maybe let's land them?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83836

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

[PATCH] D83836: [Analyzer] Add checkRegionChanges for SmartPtrModeling

[Artem Dergachev via Phabricator via cfe-commits]

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

I think this looks great!

P.S. I unforgot why we won't be able to remove the option after all. That's 
because when we partially evalCall and partially inline, inlining breaks. Like, 
once we evalCalled a single method call, we can no longer trust RegionStore to 
have the right data inside of it on the current path, so further inlining 
cannot work correctly. We could forbid inlining and resort to conservative 
evaluation but that'd still be a regression compared to inlining. So i'd rather 
keep the option for as long as possible :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83836



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

[PATCH] D83836: [Analyzer] Add checkRegionChanges for SmartPtrModeling

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar marked 2 inline comments as done.
vrnithinkumar added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:97
 
+// If a region is removed all of the subregions needs to be removed too.
+static ProgramStateRef removeTrackedRegions(ProgramStateRef State,

vsavchenko wrote:
> nit: *need to be removed
fixed



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:98
+// If a region is removed all of the subregions needs to be removed too.
+static ProgramStateRef removeTrackedRegions(ProgramStateRef State,
+const MemRegion *Region) {

vsavchenko wrote:
> Maybe `Subregions` would fit better in this name then?
Changed to Subregions



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:186
+  for (const auto *Region : Regions)
+State = removeTrackedRegions(State, Region->getBaseRegion());
+  return State;

vsavchenko wrote:
> It is not critical, but potentially we can allocate/deallocate a whole bunch 
> of states here.  We can do the same sort of operation with the map itself 
> (`State->get()`), which still have similar problem but to a 
> lesser degree.  Additionally, this `get` method is not 
> compile-time, it searches for the corresponding map in runtime (also in a 
> map), so you repeat that as many times as you have `Regions`.
> 
> And super-duper over-optimization note on my part: making the loop over 
> `Regions` to be the inner-most is more cache-friendly.  It is not really 
> critical here, but it is overall good to have an eye for things like that.
Updated `removeTrackedSubregions` for passing `TrackedRegionMap`



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

xazax.hun wrote:
> vrnithinkumar wrote:
> > added this to support  use case like `Q = std::move(P)`
> This operation should be `noexcept`: 
> https://en.cppreference.com/w/cpp/memory/unique_ptr/operator%3D
> 
> While it makes little difference for the analyzer at this point we should try 
> to be as close to the standard as possible. If you have some time feel free 
> to add `noexcept` to other methods that miss it :)
Added `noexcept` for all applicable methods



Comment at: clang/test/Analysis/smart-ptr.cpp:190
+/*
+// TODO: Enable this test after '=' operator overloading modeling.
+void derefAfterAssignment() {

vsavchenko wrote:
> Usually we simply add the test with expectations matching current state of 
> things, but add a TODO over those particular lines. This way when you fix the 
> issue the test will start failing and you won't forget to uncomment it.
Enabled the test and added the todo.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83836



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

[PATCH] D83836: [Analyzer] Add checkRegionChanges for SmartPtrModeling

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar updated this revision to Diff 278628.
vrnithinkumar marked 8 inline comments as done.
vrnithinkumar added a comment.

- Adding a missed todo


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83836

Files:
  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
@@ -67,14 +67,14 @@
   std::unique_ptr P(new A());
   P.release();
   clang_analyzer_numTimesReached(); // expected-warning {{1}}
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterReset() {
   std::unique_ptr P(new A());
   P.reset();
   clang_analyzer_numTimesReached(); // expected-warning {{1}}
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterResetWithNull() {
@@ -101,3 +101,103 @@
   A *AP = P.release();
   AP->foo(); // expected-warning {{Called C++ object pointer is null [core.CallAndMessage]}}
 }
+
+void pass_smart_ptr_by_ref(std::unique_ptr );
+void pass_smart_ptr_by_const_ref(const std::unique_ptr );
+void pass_smart_ptr_by_rvalue_ref(std::unique_ptr &);
+void pass_smart_ptr_by_const_rvalue_ref(const std::unique_ptr &);
+void pass_smart_ptr_by_ptr(std::unique_ptr *a);
+void pass_smart_ptr_by_const_ptr(const std::unique_ptr *a);
+
+void regioninvalidationTest() {
+  {
+std::unique_ptr P;
+pass_smart_ptr_by_ref(P);
+P->foo(); // no-warning
+  }
+  {
+std::unique_ptr P;
+pass_smart_ptr_by_const_ref(P);
+P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  }
+  {
+std::unique_ptr P;
+pass_smart_ptr_by_rvalue_ref(std::move(P));
+P->foo(); // no-warning
+  }
+  {
+std::unique_ptr P;
+pass_smart_ptr_by_const_rvalue_ref(std::move(P));
+P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  }
+  {
+std::unique_ptr P;
+pass_smart_ptr_by_ptr();
+P->foo();
+  }
+  {
+std::unique_ptr P;
+pass_smart_ptr_by_const_ptr();
+P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  }
+}
+
+struct StructWithSmartPtr {
+  std::unique_ptr P;
+};
+
+void pass_struct_with_smart_ptr_by_ref(StructWithSmartPtr );
+void pass_struct_with_smart_ptr_by_const_ref(const StructWithSmartPtr );
+void pass_struct_with_smart_ptr_by_rvalue_ref(StructWithSmartPtr &);
+void pass_struct_with_smart_ptr_by_const_rvalue_ref(const StructWithSmartPtr &);
+void pass_struct_with_smart_ptr_by_ptr(StructWithSmartPtr *a);
+void pass_struct_with_smart_ptr_by_const_ptr(const StructWithSmartPtr *a);
+
+void regioninvalidationTestWithinStruct() {
+  {
+StructWithSmartPtr S;
+pass_struct_with_smart_ptr_by_ref(S);
+S.P->foo(); // no-warning
+  }
+  {
+StructWithSmartPtr S;
+pass_struct_with_smart_ptr_by_const_ref(S);
+S.P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  }
+  {
+StructWithSmartPtr S;
+pass_struct_with_smart_ptr_by_rvalue_ref(std::move(S));
+S.P->foo(); // no-warning
+  }
+  {
+StructWithSmartPtr S;
+pass_struct_with_smart_ptr_by_const_rvalue_ref(std::move(S));
+S.P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  }
+  {
+StructWithSmartPtr S;
+pass_struct_with_smart_ptr_by_ptr();
+S.P->foo();
+  }
+  {
+StructWithSmartPtr S;
+pass_struct_with_smart_ptr_by_const_ptr();
+S.P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  }
+}
+
+void derefAfterAssignment() {
+  {
+std::unique_ptr P(new A());
+std::unique_ptr Q;
+Q = std::move(P);
+Q->foo(); // no-warning
+  }
+  {
+std::unique_ptr P;
+std::unique_ptr Q;
+Q = std::move(P);
+// TODO: Fix test with expecting warning after '=' operator overloading modeling.
+Q->foo(); // no-warning
+  }
+}
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
@@ -943,24 +943,25 @@
 
 #if __cplusplus >= 201103L
 namespace std {
-  template  // TODO: Implement the stub for deleter.
-  class unique_ptr {
-  public:
-unique_ptr() {}
-unique_ptr(T *) {}
-

[PATCH] D83836: [Analyzer] Add checkRegionChanges for SmartPtrModeling

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar updated this revision to Diff 278622.
vrnithinkumar marked an inline comment as done.
vrnithinkumar added a comment.

- Addressing review comments
- Enabling commented out tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83836

Files:
  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
@@ -67,14 +67,14 @@
   std::unique_ptr P(new A());
   P.release();
   clang_analyzer_numTimesReached(); // expected-warning {{1}}
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterReset() {
   std::unique_ptr P(new A());
   P.reset();
   clang_analyzer_numTimesReached(); // expected-warning {{1}}
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterResetWithNull() {
@@ -101,3 +101,102 @@
   A *AP = P.release();
   AP->foo(); // expected-warning {{Called C++ object pointer is null [core.CallAndMessage]}}
 }
+
+void pass_smart_ptr_by_ref(std::unique_ptr );
+void pass_smart_ptr_by_const_ref(const std::unique_ptr );
+void pass_smart_ptr_by_rvalue_ref(std::unique_ptr &);
+void pass_smart_ptr_by_const_rvalue_ref(const std::unique_ptr &);
+void pass_smart_ptr_by_ptr(std::unique_ptr *a);
+void pass_smart_ptr_by_const_ptr(const std::unique_ptr *a);
+
+void regioninvalidationTest() {
+  {
+std::unique_ptr P;
+pass_smart_ptr_by_ref(P);
+P->foo(); // no-warning
+  }
+  {
+std::unique_ptr P;
+pass_smart_ptr_by_const_ref(P);
+P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  }
+  {
+std::unique_ptr P;
+pass_smart_ptr_by_rvalue_ref(std::move(P));
+P->foo(); // no-warning
+  }
+  {
+std::unique_ptr P;
+pass_smart_ptr_by_const_rvalue_ref(std::move(P));
+P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  }
+  {
+std::unique_ptr P;
+pass_smart_ptr_by_ptr();
+P->foo();
+  }
+  {
+std::unique_ptr P;
+pass_smart_ptr_by_const_ptr();
+P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  }
+}
+
+struct StructWithSmartPtr {
+  std::unique_ptr P;
+};
+
+void pass_struct_with_smart_ptr_by_ref(StructWithSmartPtr );
+void pass_struct_with_smart_ptr_by_const_ref(const StructWithSmartPtr );
+void pass_struct_with_smart_ptr_by_rvalue_ref(StructWithSmartPtr &);
+void pass_struct_with_smart_ptr_by_const_rvalue_ref(const StructWithSmartPtr &);
+void pass_struct_with_smart_ptr_by_ptr(StructWithSmartPtr *a);
+void pass_struct_with_smart_ptr_by_const_ptr(const StructWithSmartPtr *a);
+
+void regioninvalidationTestWithinStruct() {
+  {
+StructWithSmartPtr S;
+pass_struct_with_smart_ptr_by_ref(S);
+S.P->foo(); // no-warning
+  }
+  {
+StructWithSmartPtr S;
+pass_struct_with_smart_ptr_by_const_ref(S);
+S.P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  }
+  {
+StructWithSmartPtr S;
+pass_struct_with_smart_ptr_by_rvalue_ref(std::move(S));
+S.P->foo(); // no-warning
+  }
+  {
+StructWithSmartPtr S;
+pass_struct_with_smart_ptr_by_const_rvalue_ref(std::move(S));
+S.P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  }
+  {
+StructWithSmartPtr S;
+pass_struct_with_smart_ptr_by_ptr();
+S.P->foo();
+  }
+  {
+StructWithSmartPtr S;
+pass_struct_with_smart_ptr_by_const_ptr();
+S.P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  }
+}
+
+void derefAfterAssignment() {
+  {
+std::unique_ptr P(new A());
+std::unique_ptr Q;
+Q = std::move(P);
+Q->foo(); // no-warning
+  }
+  {
+std::unique_ptr P;
+std::unique_ptr Q;
+Q = std::move(P);
+Q->foo(); // no-warning
+  }
+}
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
@@ -943,24 +943,25 @@
 
 #if __cplusplus >= 201103L
 namespace std {
-  template  // TODO: Implement the stub for deleter.
-  class unique_ptr {
-  public:
-unique_ptr() {}
-unique_ptr(T *) {}
-unique_ptr(const unique_ptr &) = delete;
-