[Analyzer] Handle unique_ptr::swap() in SmartPtrModeling

[vrnithinkumar]

Summary

Enabling the SmartPtrModeling to handle swap method for unique_ptr

[vrnithinkumar]

clang/test/Analysis/smart-ptr.cpp

131

I will fix this.

[xazax.hun]

LGTM, thanks!

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

965

Maybe it is worth to add a TODO to introduce an additional template parameter once the deleter is added above.

https://en.cppreference.com/w/cpp/memory/unique_ptr/swap2

clang/test/Analysis/smart-ptr.cpp

124

This test case is very similar to the first one. Maybe you could move them closer. And you could make both pointers invalid to make them more distinct.

[NoQ]

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

964–965

You seem to be relying on the fact that global std::swap is implemented in terms of the member std::swap. That's an implementation detail of the standard library; i'm not sure that this is always the case. Ideally we should model the global std::swap separately.

[xazax.hun]

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

964–965

I am not sure how reliable cppreference is, but I think this overload might actually be guaranteed by the standard: https://en.cppreference.com/w/cpp/memory/unique_ptr/swap2

[vsavchenko]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

198

I think it would be good to add some comments in the body of the function to be more explicit about the situation you are handling.

224–228

These two if's are clearly duplicates and it seems like a good candidate for a separate function.

clang/test/Analysis/smart-ptr.cpp

124

+1

[vsavchenko]

The same thing with the commit message here, it would be better as just "Change"

[vrnithinkumar]

• Changes from review comments

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

198

Added comments

224–228

Thanks!
created a separate function updateSwappedRegion()

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

964–965

I also made the assumption based on this (https://en.cppreference.com/w/cpp/memory/unique_ptr/swap2).

965

Added the TODO

clang/test/Analysis/smart-ptr.cpp

124

Moved and made both pointers invalid in the test

131

Fixed

[NoQ]

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

964–965

Aha, interesting. We still probably can't rely on it being always inlined though, so we'll still probably have to model it separately.

[NoQ]

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

[vrnithinkumar]

In D83877#2189637, @NoQ wrote:

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

I have already committed this changes.
https://github.com/llvm/llvm-project/commit/76c0577763505ea3db1017a9aab579c1c2f135d0

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

964–965

Okay. We will add the separate modeling for std::swap

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
 //
 //===----------------------------------------------------------------------===//
 //
void SmartPtrModeling::handleReset(const CallEvent &Call,
   if (!IC)
     return;
 
   const MemRegion *ThisValRegion = IC->getCXXThisVal().getAsRegion();
   if (!ThisValRegion)
     return;
   auto State = updateTrackedRegion(Call, C, ThisValRegion);
   C.addTransition(State);
-  // TODO: Make sure to ivalidate the the region in the Store if we don't have
+  // TODO: Make sure to ivalidate the region in the Store if we don't have
   // time to model all methods.
 }
 
 void SmartPtrModeling::handleRelease(const CallEvent &Call,
                                      CheckerContext &C) const {
   const auto *IC = dyn_cast<CXXInstanceCall>(&Call);
   if (!IC)
     return;
if (InnerPointVal) {
     State = State->BindExpr(Call.getOriginExpr(), C.getLocationContext(),
                             *InnerPointVal);
   }
   C.addTransition(State);
   // TODO: Add support to enable MallocChecker to start tracking the raw
   // pointer.
 }
 
 void SmartPtrModeling::handleSwap(const CallEvent &Call,
                                   CheckerContext &C) const {
-  // TODO: Add support to handle swap method.
+  const auto *IC = dyn_cast<CXXInstanceCall>(&Call);
+  if (!IC)
+    return;
+
+  const MemRegion *ThisRegion = IC->getCXXThisVal().getAsRegion();
+  if (!ThisRegion)
+    return;
+
+  const auto *ArgRegion = Call.getArgSVal(0).getAsRegion();
+  if (!ArgRegion)
+    return;
+
+  auto State = C.getState();
+  const auto *ThisRegionInnerPointerVal =
+      State->get<TrackedRegionMap>(ThisRegion);
+  const auto *ArgRegionInnerPointerVal =
+      State->get<TrackedRegionMap>(ArgRegion);
+
+  // Swap the tracked region values.
+  if (ArgRegionInnerPointerVal) {
+    State = State->set<TrackedRegionMap>(ThisRegion, *ArgRegionInnerPointerVal);
+  } else {
+    State = State->remove<TrackedRegionMap>(ThisRegion);
+  }
+  if (ThisRegionInnerPointerVal) {
+    State = State->set<TrackedRegionMap>(ArgRegion, *ThisRegionInnerPointerVal);
+  } else {
+    State = State->remove<TrackedRegionMap>(ArgRegion);
+  }
+
+  C.addTransition(State);
 }
 
 ProgramStateRef
 SmartPtrModeling::updateTrackedRegion(const CallEvent &Call, CheckerContext &C,
                                       const MemRegion *ThisValRegion) const {
   // TODO: Refactor and clean up handling too many things.
   ProgramStateRef State = C.getState();
   auto NumArgs = Call.getNumArgs();

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;
public:
     T *release() const;
     void reset(T *p = nullptr) const;
     void swap(unique_ptr<T> &p) const;
 
     typename std::add_lvalue_reference<T>::type operator*() const;
     T *operator->() const;
     operator bool() const;
   };
+
+  template <typename T>
+  void swap(unique_ptr<T> &x, unique_ptr<T> &y) noexcept {
+    x.swap(y);
   }
+  } // 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 derefOnSwappedNullPtr() {
+  std::unique_ptr<A> P(new A());
+  std::unique_ptr<A> PNull;
+  P.swap(PNull);
+  PNull->foo(); // No warning.
+  (*P).foo();   // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+}
+
+void derefOnSwappedValidPtr() {
+  std::unique_ptr<A> P(new A());
+  std::unique_ptr<A> PValid(new A());
+  P.swap(PValid);
+  (*P).foo();    // No warning.
+  PValid->foo(); // No warning.
+  std::swap(P, PValid);
+  P->foo();      // No warning.
+  PValid->foo(); // No warning.
+}
+
+void derefOnStdSwappedNullPtr() {
+  std::unique_ptr<A> P(new A());
+  std::unique_ptr<A> PNull;
+  std::swap(P, PNull);
+  PNull->foo(); // No warning.
+  P->foo();     // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+}
 No newline at end of file