[Analyzer] Add checkRegionChanges for SmartPtrModeling

[vrnithinkumar]

Summary

Implementing checkRegionChanges for SmartPtrModeling.
To improve the accuracy, when a smart pointer is passed by a non-const reference into a function, removed the tracked region data. Since it is not sure what happens to the smart pointer inside the function.

[vrnithinkumar]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

182

I am using the same code from the MoveChecker since the same logic applies here too.
But I am not completely sure whether this is the clean approach.

clang/test/Analysis/Inputs/system-header-simulator-cxx.h

962

added this to support use case like Q = std::move(P)

clang/test/Analysis/smart-ptr.cpp

145

@xazax.hun , Is this the type of test you were suggesting?

[NoQ]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

191–192

The whole reason why we are making this patch first is because we *don't* fully handle methods :)

Also, unlike the move checker, once we handle them, they'll probably stop changing regions, so no exceptional code path would be necessary in most cases.

So i'd rather start with fully wiping all the data on any sneeze and later see if we can relax it.

[xazax.hun]

clang/test/Analysis/Inputs/system-header-simulator-cxx.h

962

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

clang/test/Analysis/smart-ptr.cpp

145

Yes, looks good to me, thanks! :)

[vrnithinkumar]

Untrack all changing regions in checkRegionChanges

[vrnithinkumar]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

191–192

I completely missed that point and jumped ahead.
Made changes to untrack all changing regions in checkRegionChanges

[vsavchenko]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

97

nit: *need to be removed

98

Maybe Subregions would fit better in this name then?

188

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<TrackedRegionMap>()), which still have similar problem but to a lesser degree. Additionally, this get<MAP_TYPE> 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.

clang/test/Analysis/smart-ptr.cpp

190

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.

[vsavchenko]

Additionally, I would prefer commit message to be imperative. It is sorta like a rule of a good commit message.

[vrnithinkumar]

• Addressing review comments
• Enabling commented out tests

[vrnithinkumar]

• Adding a missed todo

[vrnithinkumar]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

97

fixed

98

Changed to Subregions

188

Updated removeTrackedSubregions for passing TrackedRegionMap

clang/test/Analysis/Inputs/system-header-simulator-cxx.h

962

Added noexcept for all applicable methods

clang/test/Analysis/smart-ptr.cpp

190

Enabled the test and added the todo.

[NoQ]

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

[NoQ]

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

[vrnithinkumar]

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)

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

 // SmartPtrModeling.cpp - Model behavior of C++ smart pointers - C++ ------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //

 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h"
 
 using namespace clang;
 using namespace ento;
 
 namespace {
-class SmartPtrModeling : public Checker<eval::Call, check::DeadSymbols> {
+class SmartPtrModeling
+    : public Checker<eval::Call, check::DeadSymbols, check::RegionChanges> {
 
   bool isNullAfterMoveMethod(const CallEvent &Call) const;
 
 public:
   // Whether the checker should model for null dereferences of smart pointers.
   DefaultBool ModelSmartPtrDereference;
   bool evalCall(const CallEvent &Call, CheckerContext &C) const;
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
   void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
+  ProgramStateRef
+  checkRegionChanges(ProgramStateRef State,
+                     const InvalidatedSymbols *Invalidated,
+                     ArrayRef<const MemRegion *> ExplicitRegions,
+                     ArrayRef<const MemRegion *> Regions,
+                     const LocationContext *LCtx, const CallEvent *Call) const;
 
 private:
   ProgramStateRef updateTrackedRegion(const CallEvent &Call, CheckerContext &C,
                                       const MemRegion *ThisValRegion) const;
   void handleReset(const CallEvent &Call, CheckerContext &C) const;
   void handleRelease(const CallEvent &Call, CheckerContext &C) const;
   void handleSwap(const CallEvent &Call, CheckerContext &C) const;
 

 bool isNullSmartPtr(const ProgramStateRef State, const MemRegion *ThisRegion) {
   const auto *InnerPointVal = State->get<TrackedRegionMap>(ThisRegion);
   return InnerPointVal && InnerPointVal->isZeroConstant();
 }
 } // namespace smartptr
 } // namespace ento
 } // namespace clang
 
+// If a region is removed all of the subregions need to be removed too.
+static TrackedRegionMapTy
+removeTrackedSubregions(TrackedRegionMapTy RegionMap,
+                        TrackedRegionMapTy::Factory &RegionMapFactory,
+                        const MemRegion *Region) {
+  if (!Region)
+    return RegionMap;
+  for (const auto &E : RegionMap) {
+    if (E.first->isSubRegionOf(Region))
+      RegionMap = RegionMapFactory.remove(RegionMap, E.first);
+  }
+  return RegionMap;
+}
+
 bool SmartPtrModeling::isNullAfterMoveMethod(const CallEvent &Call) const {
   // TODO: Update CallDescription to support anonymous calls?
   // TODO: Handle other methods, such as .get() or .release().
   // But once we do, we'd need a visitor to explain null dereferences
   // that are found via such modeling.
   const auto *CD = dyn_cast_or_null<CXXConversionDecl>(Call.getDecl());
   return CD && CD->getConversionType()->isBooleanType();
 }
for (auto E : TrackedRegions) {
     bool IsRegDead = !SymReaper.isLiveRegion(Region);
 
     if (IsRegDead)
       State = State->remove<TrackedRegionMap>(Region);
   }
   C.addTransition(State);
 }
 
+ProgramStateRef SmartPtrModeling::checkRegionChanges(
+    ProgramStateRef State, const InvalidatedSymbols *Invalidated,
+    ArrayRef<const MemRegion *> ExplicitRegions,
+    ArrayRef<const MemRegion *> Regions, const LocationContext *LCtx,
+    const CallEvent *Call) const {
+  TrackedRegionMapTy RegionMap = State->get<TrackedRegionMap>();
+  TrackedRegionMapTy::Factory &RegionMapFactory =
+      State->get_context<TrackedRegionMap>();
+  for (const auto *Region : Regions)
+    RegionMap = removeTrackedSubregions(RegionMap, RegionMapFactory,
+                                        Region->getBaseRegion());
+  return State->set<TrackedRegionMap>(RegionMap);
+}
+
 void SmartPtrModeling::handleReset(const CallEvent &Call,
                                    CheckerContext &C) const {
   const auto *IC = dyn_cast<CXXInstanceCall>(&Call);
   if (!IC)
     return;
 
   const MemRegion *ThisValRegion = IC->getCXXThisVal().getAsRegion();
   if (!ThisValRegion)

clang/test/Analysis/Inputs/system-header-simulator-cxx.h

 // Like the compiler, the static analyzer treats some functions differently if
 // they come from a system header -- for example, it is assumed that system
 // functions do not arbitrarily free() their parameters, and that some bugs
 // found in system headers cannot be fixed by the user and should be
 // suppressed.
 #pragma clang system_header
 
 typedef unsigned char uint8_t;
#endif
   template <class InputIterator, class OutputIterator>
   OutputIterator copy(InputIterator first, InputIterator last,
                       OutputIterator result);
 
 }
 
 #if __cplusplus >= 201103L
 namespace std {
-  template <typename T> // TODO: Implement the stub for deleter.
-  class unique_ptr {
-  public:
-    unique_ptr() {}
-    unique_ptr(T *) {}
-    unique_ptr(const unique_ptr &) = delete;
-    unique_ptr(unique_ptr &&);
-
-    T *get() const;
-    T *release() const;
-    void reset(T *p = nullptr) const;
-    void swap(unique_ptr<T> &p) const;
+  unique_ptr() noexcept {}
+  unique_ptr(T *) noexcept {}
+  unique_ptr(const unique_ptr &) noexcept = delete;
+  unique_ptr(unique_ptr &&) noexcept;
+
+  T *get() const noexcept;
+  T *release() noexcept;
+  void reset(T *p = nullptr) noexcept;
+  void swap(unique_ptr<T> &p) noexcept;
 
-    typename std::add_lvalue_reference<T>::type operator*() const;
-    T *operator->() const;
-    operator bool() const;
-  };
-}
+  T *operator->() const noexcept;
+  operator bool() const noexcept;
+  unique_ptr<T> &operator=(unique_ptr<T> &&p) noexcept;
+} // namespace std
 #endif
 
 #ifdef TEST_INLINABLE_ALLOCATORS
 namespace std {
   void *malloc(size_t);
   void free(void *);
 }
 void* operator new(std::size_t size, const std::nothrow_t&) throw() { return std::malloc(size); }

clang/test/Analysis/smart-ptr.cpp

 // RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection\
 // RUN:   -analyzer-checker cplusplus.Move,alpha.cplusplus.SmartPtr\
 // RUN:   -analyzer-config cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
 // RUN:   -std=c++11 -verify %s
 
 #include "Inputs/system-header-simulator-cxx.h"
 
 void clang_analyzer_warnIfReached();
void derefAfterCtrWithNullReturnMethod() {
   std::unique_ptr<A> P(return_null());
   P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterRelease() {
   std::unique_ptr<A> 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<A> 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() {
   std::unique_ptr<A> P(new A());
   P.reset(nullptr);
   P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
 }
 
void derefAfterReleaseAndResetWithNonNull() {
   P->foo(); // No warning.
 }
 
 void derefOnReleasedNullRawPtr() {
   std::unique_ptr<A> P;
   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<A> &a);
+void pass_smart_ptr_by_const_ref(const std::unique_ptr<A> &a);
+void pass_smart_ptr_by_rvalue_ref(std::unique_ptr<A> &&a);
+void pass_smart_ptr_by_const_rvalue_ref(const std::unique_ptr<A> &&a);
+void pass_smart_ptr_by_ptr(std::unique_ptr<A> *a);
+void pass_smart_ptr_by_const_ptr(const std::unique_ptr<A> *a);
+
+void regioninvalidationTest() {
+  {
+    std::unique_ptr<A> P;
+    pass_smart_ptr_by_ref(P);
+    P->foo(); // no-warning
+  }
+  {
+    std::unique_ptr<A> P;
+    pass_smart_ptr_by_const_ref(P);
+    P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  }
+  {
+    std::unique_ptr<A> P;
+    pass_smart_ptr_by_rvalue_ref(std::move(P));
+    P->foo(); // no-warning
+  }
+  {
+    std::unique_ptr<A> 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<A> P;
+    pass_smart_ptr_by_ptr(&P);
+    P->foo();
+  }
+  {
+    std::unique_ptr<A> P;
+    pass_smart_ptr_by_const_ptr(&P);
+    P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  }
+}
+
+struct StructWithSmartPtr {
+  std::unique_ptr<A> P;
+};
+
+void pass_struct_with_smart_ptr_by_ref(StructWithSmartPtr &a);
+void pass_struct_with_smart_ptr_by_const_ref(const StructWithSmartPtr &a);
+void pass_struct_with_smart_ptr_by_rvalue_ref(StructWithSmartPtr &&a);
+void pass_struct_with_smart_ptr_by_const_rvalue_ref(const StructWithSmartPtr &&a);
+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);
+    S.P->foo();
+  }
+  {
+    StructWithSmartPtr S;
+    pass_struct_with_smart_ptr_by_const_ptr(&S);
+    S.P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  }
+}
+
+void derefAfterAssignment() {
+  {
+    std::unique_ptr<A> P(new A());
+    std::unique_ptr<A> Q;
+    Q = std::move(P);
+    Q->foo(); // no-warning
+  }
+  {
+    std::unique_ptr<A> P;
+    std::unique_ptr<A> Q;
+    Q = std::move(P);
+    // TODO: Fix test with expecting warning after '=' operator overloading modeling.
+    Q->foo(); // no-warning
+  }
+}