[analyzer] Handle std::swap for std::unique_ptr

[RedDocMD]

Summary

This patch handles the std::swap function specialization
for std::unique_ptr. Implemented to be very similar to
how swap method is handled

[RedDocMD]

I am not entirely satisfied with the note that is being emitted currently from the std::swap handling (its ripped off from the note emitted for std::unique_ptr::swap).
Any suggestions?

[vsavchenko]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

96–106

That's great!

190–228

Maybe a separate method then?

219

Wait, and what if the second argument is interesting?

clang/test/Analysis/smart-ptr-text-output.cpp

80

I know that this case existed before, but can we initialize P with non-null, so that the warning is a bit more life-like?

[vsavchenko]

It looks like new functionality is VERY much like handleSwap, and we should definitely merge common parts.

Also, I think that commit title should be more specific than "Handle std::swap". It should at least mention the checker.

[RedDocMD]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

219

I know. That's why I said that the note is crappy.
The question I have in mind is - what if we swap null with null? Do we want a note in that case?
IMO, we should have a note for every case of swapping a confirmed null with something that is not confirmed to be null.

[RedDocMD]

The current implementation of how notes are emitted in handleSwap is broken. Consider the following code:

#include <memory>

void foo() {
  auto ptr1 = std::unique_ptr<int>(new int(10));
  auto ptr2 = std::unique_ptr<int>(new int(13));
  ptr1.swap(ptr2);
  ptr1.reset();
  *ptr1;
}

This yields the following warning and notes:

swap-and-reset.cpp:8:3: warning: Dereference of null smart pointer 'ptr1' [alpha.cplusplus.SmartPtr]
  *ptr1;
  ^~~~~
swap-and-reset.cpp:4:36: note: Assigning 10
  auto ptr1 = std::unique_ptr<int>(new int(10));
                                   ^~~~~~~~~~~
swap-and-reset.cpp:4:15: note: Smart pointer 'ptr1' is constructed
  auto ptr1 = std::unique_ptr<int>(new int(10));
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
swap-and-reset.cpp:5:36: note: Assigning 13
  auto ptr2 = std::unique_ptr<int>(new int(13));
                                   ^~~~~~~~~~~
swap-and-reset.cpp:5:15: note: Smart pointer 'ptr2' is constructed
  auto ptr2 = std::unique_ptr<int>(new int(13));
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
swap-and-reset.cpp:6:3: note: Swapped null smart pointer 'ptr2' with smart pointer 'ptr1'
  ptr1.swap(ptr2);
  ^~~~~~~~~~~~~~~
swap-and-reset.cpp:7:3: note: Smart pointer 'ptr1' reset using a null value
  ptr1.reset();
  ^~~~~~~~~~~~
swap-and-reset.cpp:8:3: note: Dereference of null smart pointer 'ptr1'
  *ptr1;
  ^~~~~

But clearly, ptr2 is not null.

[RedDocMD]

Refactored common code, removed note emission

[RedDocMD]

I have gotten rid of notes for the time being.
The trouble is that, we were not figuring out whether the null-pointer de-reference was occurring due to the null value being swapped with the current pointer.
We just assumed that. So I am guessing we would need a visitor to figure that out? (Hooray for Valeriy!)
Or is there a simpler solution?

[NoQ]

I think notes are fairly straightforward: if we're tracking one smart pointer below the swap, we should stop tracking it and start tracking the other smart pointer above the swap. If both were tracked, keep tracking both (but in swapped manners, assuming we're tracking them in different manners). It doesn't matter whether any of them is null, it only matters which ones were tracked. Though I guess we could mention if any of them are null or do other cosmetic improvements depending on how they participated in the bug. Trackedness is equivalent to interestingness because we're dealing with regions.

[RedDocMD]

Marking and un-marking interestingness

[vsavchenko]

I agree with @NoQ that notes are pretty much straightforward and we shouldn't abandon them altogether. Refinements about what is null or non-null pointer are purely cosmetic and we definitely can tweak this messaging.

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

196–202

I guess handleSwap can take SVals instead of MemRegion and we can mostly cut on this boilerplate as well.

return handleSwap(State, Call.getArgSVal(0), Call.getArgSVal(1), C);

and

return handleSwap(State, IC->getCXXThisVal(), Call.getArgSVal(0), C);

458–467

nit: these two pieces of code are very much the same

[xazax.hun]

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h

435 ↗

(On Diff #352627)

Bikeshedding: I wonder if we prefer Uninteresting to NotInteresting. Or alternatively, if we want to emphasize this is only the lack of interestingness, we could name it removeInterestingness. I do not have strong feelings about any of the options, I was just wondering if any of you has a preference.

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

191

I wonder about the value of this assertion. Shouldn`t Call.isCalled(StdSwapCall) already validate the number of arguments?

193

Nit: we usually omit braces for simple if statements.

458–467

I guess we could make markInteresting take an optional bool and swap the interestingness unconditionally.

[NoQ]

I think this patch is good to go as long as other folks are happy. I'm glad that uninterestingness is becoming a thing!

[RedDocMD]

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h

435 ↗

(On Diff #352627)

I was actually planning to use something like unmarkInteresting, but that is grammatically incorrect. removeInterestingness is fair enough, but a mouthful I think. As for Uninteresting, I think we are better off avoiding clever uses of English and stick with simple, programmer jargon.

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

191

About as valuable as any assertion is ;)
My reasoning is that since we are calling Call.getArgSval(0) and Call.getArgSVal(1), we had better assert that there are two args.
As a side-note, I had a bug in my CallDescriptor, which was caught by this assertion

196–202

Yup, okay.

458–467

Then we would need to rename it to something like setInterestingness.
And renaming such a widely used function would be a real pain.

458–467

Yes, but I don't think refactoring them into a lambda (within a lambda) would be that nice either.

[RedDocMD]

Some more refactoring

[balazske]

This change is accepted already. Is it good to commit? I would need the change for symbol "uninterestingness" in D104925.

[RedDocMD]

Post rebase cleanup

clang/lib/StaticAnalyzer/Checkers/SmartPtr.h

 
 namespace clang {
 namespace ento {
 namespace smartptr {
 
 /// Returns true if the event call is on smart pointer.
 bool isStdSmartPtrCall(const CallEvent &Call);
 
+bool isStdSmartPtr(const CXXRecordDecl *RD);
+
 /// Returns whether the smart pointer is null or not.
 bool isNullSmartPtr(const ProgramStateRef State, const MemRegion *ThisRegion);
 
 const BugType *getNullDereferenceBugType();
 
 } // namespace smartptr
 } // namespace ento
 } // namespace clang
 
 #endif // LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_SMARTPTR_H

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

checkRegionChanges(ProgramStateRef State,
                      const LocationContext *LCtx, const CallEvent *Call) const;
   void printState(raw_ostream &Out, ProgramStateRef State, const char *NL,
                   const char *Sep) const override;
   void checkLiveSymbols(ProgramStateRef State, SymbolReaper &SR) const;
 
 private:
   void handleReset(const CallEvent &Call, CheckerContext &C) const;
   void handleRelease(const CallEvent &Call, CheckerContext &C) const;
-  void handleSwap(const CallEvent &Call, CheckerContext &C) const;
+  void handleSwapMethod(const CallEvent &Call, CheckerContext &C) const;
   void handleGet(const CallEvent &Call, CheckerContext &C) const;
   bool handleAssignOp(const CallEvent &Call, CheckerContext &C) const;
   bool handleMoveCtr(const CallEvent &Call, CheckerContext &C,
                      const MemRegion *ThisRegion) const;
   bool updateMovedSmartPointers(CheckerContext &C, const MemRegion *ThisRegion,
                                 const MemRegion *OtherSmartPtrRegion) const;
   void handleBoolConversion(const CallEvent &Call, CheckerContext &C) const;
+  bool handleSwap(ProgramStateRef State, const MemRegion *FirstRegion,
+                  const MemRegion *SecondRegion, CheckerContext &C) const;
 
   using SmartPtrMethodHandlerFn =
       void (SmartPtrModeling::*)(const CallEvent &Call, CheckerContext &) const;
   CallDescriptionMap<SmartPtrMethodHandlerFn> SmartPtrMethodHandlers{
       {{"reset"}, &SmartPtrModeling::handleReset},
       {{"release"}, &SmartPtrModeling::handleRelease},
-      {{"swap", 1}, &SmartPtrModeling::handleSwap},
+      {{"swap", 1}, &SmartPtrModeling::handleSwapMethod},
       {{"get"}, &SmartPtrModeling::handleGet}};
+  const CallDescription StdSwapCall{{"std", "swap"}, 2};
 };
 } // end of anonymous namespace
 
 REGISTER_MAP_WITH_PROGRAMSTATE(TrackedRegionMap, const MemRegion *, SVal)
 
 // Define the inter-checker API.
 namespace clang {
 namespace ento {
 namespace smartptr {
 bool isStdSmartPtrCall(const CallEvent &Call) {
   const auto *MethodDecl = dyn_cast_or_null<CXXMethodDecl>(Call.getDecl());
   if (!MethodDecl || !MethodDecl->getParent())
     return false;
 
   const auto *RecordDecl = MethodDecl->getParent();
-  if (!RecordDecl || !RecordDecl->getDeclContext()->isStdNamespace())
+  return isStdSmartPtr(RecordDecl);
+}
+
+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 isNullSmartPtr(const ProgramStateRef State, const MemRegion *ThisRegion) {
   const auto *InnerPointVal = State->get<TrackedRegionMap>(ThisRegion);
   return InnerPointVal &&
          !State->assume(InnerPointVal->castAs<DefinedOrUnknownSVal>(), true);
bool SmartPtrModeling::isBoolConversionMethod(const CallEvent &Call) const {
   // that are found via such modeling.
   const auto *CD = dyn_cast_or_null<CXXConversionDecl>(Call.getDecl());
   return CD && CD->getConversionType()->isBooleanType();
 }
 
 bool SmartPtrModeling::evalCall(const CallEvent &Call,
                                 CheckerContext &C) const {
   ProgramStateRef State = C.getState();
+
+  if (Call.isCalled(StdSwapCall)) {
+    // Check the first arg, if it is of std::unique_ptr type.
+    assert(Call.getNumArgs() == 2 && "std::swap should have two arguments");
+    const Expr *FirstArg = Call.getArgExpr(0);
+    if (!smartptr::isStdSmartPtr(FirstArg->getType()->getAsCXXRecordDecl())) {
+      return false;
+    }
+    const MemRegion *FirstArgThisRegion = Call.getArgSVal(0).getAsRegion();
+    if (!FirstArgThisRegion)
+      return false;
+    const MemRegion *SecondArgThisRegion = Call.getArgSVal(1).getAsRegion();
+    if (!SecondArgThisRegion)
+      return false;
+
+    return handleSwap(State, FirstArgThisRegion, SecondArgThisRegion, C);
+  }
+
   if (!smartptr::isStdSmartPtrCall(Call))
     return false;
 
   if (isBoolConversionMethod(Call)) {
     const MemRegion *ThisR =
         cast<CXXInstanceCall>(&Call)->getCXXThisVal().getAsRegion();
 
     if (ModelSmartPtrDereference) {
       // The check for the region is moved is duplicated in handleBoolOperation
       // method.
       // FIXME: Once we model std::move for smart pointers clean up this and use
       // that modeling.
       handleBoolConversion(Call, C);
       return true;
     } else {
       if (!move::isMovedFrom(State, ThisR)) {
         // TODO: Model this case as well. At least, avoid invalidation of
         // globals.
         return false;
       }
 
       // TODO: Add a note to bug reports describing this decision.
       C.addTransition(State->BindExpr(
           Call.getOriginExpr(), C.getLocationContext(),
           C.getSValBuilder().makeZeroVal(Call.getResultType())));
 
       return true;
     }
   }
 
   if (!ModelSmartPtrDereference)
C.addTransition(State, C.getNoteTag([ThisRegion](PathSensitiveBugReport &BR,
     OS << "Smart pointer";
     checkAndPrettyPrintRegion(OS, ThisRegion);
     OS << " is released and set to null";
   }));
   // TODO: Add support to enable MallocChecker to start tracking the raw
   // pointer.
 }
 
-void SmartPtrModeling::handleSwap(const CallEvent &Call,
+void SmartPtrModeling::handleSwapMethod(const CallEvent &Call,
                                         CheckerContext &C) const {
   // To model unique_ptr::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.
-  State = updateSwappedRegion(State, ThisRegion, ArgRegionInnerPointerVal);
-  State = updateSwappedRegion(State, ArgRegion, ThisRegionInnerPointerVal);
+  handleSwap(State, ThisRegion, ArgRegion, C);
+}
 
-  C.addTransition(
-      State, C.getNoteTag([ThisRegion, ArgRegion](PathSensitiveBugReport &BR,
-                                                  llvm::raw_ostream &OS) {
-        if (&BR.getBugType() != smartptr::getNullDereferenceBugType() ||
-            !BR.isInteresting(ThisRegion))
-          return;
-        BR.markInteresting(ArgRegion);
-        OS << "Swapped null smart pointer";
-        checkAndPrettyPrintRegion(OS, ArgRegion);
-        OS << " with smart pointer";
-        checkAndPrettyPrintRegion(OS, ThisRegion);
-      }));
+bool SmartPtrModeling::handleSwap(ProgramStateRef State,
+                                  const MemRegion *FirstThisRegion,
+                                  const MemRegion *SecondThisRegion,
+                                  CheckerContext &C) const {
+  const auto *FirstInnerPtrVal = State->get<TrackedRegionMap>(FirstThisRegion);
+  const auto *SecondInnerPtrVal =
+      State->get<TrackedRegionMap>(SecondThisRegion);
+
+  State = updateSwappedRegion(State, FirstThisRegion, SecondInnerPtrVal);
+  State = updateSwappedRegion(State, SecondThisRegion, FirstInnerPtrVal);
+
+  // TODO: We need to emit some note here probably!!
+  C.addTransition(State);
+
+  return true;
 }
 
 void SmartPtrModeling::handleGet(const CallEvent &Call,
                                  CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   const auto *IC = dyn_cast<CXXInstanceCall>(&Call);
   if (!IC)
     return;
 
   const MemRegion *ThisRegion = IC->getCXXThisVal().getAsRegion();
   if (!ThisRegion)
     return;
 
   SVal InnerPointerVal;
   if (const auto *InnerValPtr = State->get<TrackedRegionMap>(ThisRegion)) {
     InnerPointerVal = *InnerValPtr;
   } else {
     const auto *CallExpr = Call.getOriginExpr();
     InnerPointerVal = C.getSValBuilder().conjureSymbolVal(

clang/test/Analysis/smart-ptr-text-output.cpp

void derefOnReleasedNullRawPtr() {
   std::unique_ptr<A> P; // FIXME: add note "Default constructed smart pointer 'P' is null"
   A *AP = P.release(); // expected-note {{'AP' initialized to a null pointer value}}
   AP->foo(); // expected-warning {{Called C++ object pointer is null [core.CallAndMessage]}}
   // expected-note@-1{{Called C++ object pointer is null}}
 }
 
 void derefOnSwappedNullPtr() {
   std::unique_ptr<A> P(new A()); // expected-note {{Smart pointer 'P' is constructed}}
-  std::unique_ptr<A> PNull; // expected-note {{Default constructed smart pointer 'PNull' is null}}
-  P.swap(PNull); // expected-note {{Swapped null smart pointer 'PNull' with smart pointer 'P'}}
+  std::unique_ptr<A> PNull;
+  P.swap(PNull);
   PNull->foo(); // No warning.
   (*P).foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   // expected-note@-1{{Dereference of null smart pointer 'P'}}
 }
 
-// FIXME: Fix this test when "std::swap" is modeled seperately.
 void derefOnStdSwappedNullPtr() {
   std::unique_ptr<A> P; // expected-note {{Default constructed smart pointer 'P' is null}}
-  std::unique_ptr<A> PNull; // expected-note {{Default constructed smart pointer 'PNull' is null}}
-  std::swap(P, PNull); // expected-note@Inputs/system-header-simulator-cxx.h:979 {{Swapped null smart pointer 'PNull' with smart pointer 'P'}}
-  // expected-note@-1 {{Calling 'swap<A>'}}
-  // expected-note@-2 {{Returning from 'swap<A>'}}
+  std::unique_ptr<A> PNull;
+  std::swap(P, PNull);
   P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   // expected-note@-1{{Dereference of null smart pointer 'P'}}
 }
 
 struct StructWithSmartPtr { // expected-note {{Default constructed smart pointer 'S.P' is null}}
   std::unique_ptr<A> P;
 };