[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

131–138

That's great!

254–292

Maybe a separate method then?

283

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

283

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

260–266

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

628–637

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

255

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

257

Nit: we usually omit braces for simple if statements.

628–637

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

255

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

260–266

Yup, okay.

628–637

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

628–637

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 ento {
 namespace smartptr {
 
 /// Returns true if the event call is on smart pointer.
 bool isStdSmartPtrCall(const CallEvent &Call);
 bool isStdSmartPtr(const CXXRecordDecl *RD);
 bool isStdSmartPtr(const Expr *E);
 
+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 handleComparisionOp(const CallEvent &Call, CheckerContext &C) const;
   bool handleOstreamOperator(const CallEvent &Call, CheckerContext &C) const;
+  bool handleSwap(ProgramStateRef State, SVal First, SVal Second,
+                  CheckerContext &C) const;
   std::pair<SVal, ProgramStateRef>
   retrieveOrConjureInnerPtrVal(ProgramStateRef State,
                                const MemRegion *ThisRegion, const Expr *E,
                                QualType Type, 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)
 
 // Checks if RD has name in Names and is in std namespace
 static bool hasStdClassWithName(const CXXRecordDecl *RD,
                                 ArrayRef<llvm::StringLiteral> Names) {

 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;
   return isStdSmartPtr(MethodDecl->getParent());
 }
 
 bool isStdSmartPtr(const CXXRecordDecl *RD) {
   if (!RD || !RD->getDeclContext()->isStdNamespace())
     return false;
 
   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());
 }
 
return isStdSmartPtr(Call.getArgExpr(1)) &&
          isStdBasicOstream(Call.getArgExpr(0));
 }
 
 bool SmartPtrModeling::evalCall(const CallEvent &Call,
                                 CheckerContext &C) 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 &&
       Call.getDecl()->getDeclContext()->isStdNamespace())
     if (smartptr::isStdSmartPtr(Call.getArgExpr(0)) ||
         smartptr::isStdSmartPtr(Call.getArgExpr(1)))
       if (handleComparisionOp(Call, C))
         return true;
 
   if (isStdOstreamOperatorCall(Call))
     return handleOstreamOperator(Call, C);
 
+  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;
+    return handleSwap(State, Call.getArgSVal(0), Call.getArgSVal(1), 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;
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;
+  auto State = C.getState();
+  handleSwap(State, IC->getCXXThisVal(), Call.getArgSVal(0), C);
+}
 
-  const auto *ArgRegion = Call.getArgSVal(0).getAsRegion();
-  if (!ArgRegion)
-    return;
+bool SmartPtrModeling::handleSwap(ProgramStateRef State, SVal First,
+                                  SVal Second, CheckerContext &C) const {
+  const MemRegion *FirstThisRegion = First.getAsRegion();
+  if (!FirstThisRegion)
+    return false;
+  const MemRegion *SecondThisRegion = Second.getAsRegion();
+  if (!SecondThisRegion)
+    return false;
 
-  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);
+  const auto *FirstInnerPtrVal = State->get<TrackedRegionMap>(FirstThisRegion);
+  const auto *SecondInnerPtrVal =
+      State->get<TrackedRegionMap>(SecondThisRegion);
 
-  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);
-      }));
+  State = updateSwappedRegion(State, FirstThisRegion, SecondInnerPtrVal);
+  State = updateSwappedRegion(State, SecondThisRegion, FirstInnerPtrVal);
+
+  C.addTransition(State, C.getNoteTag([FirstThisRegion, SecondThisRegion](
+                                          PathSensitiveBugReport &BR,
+    if (&BR.getBugType() != smartptr::getNullDereferenceBugType())
+    if (BR.isInteresting(FirstThisRegion) &&
+        !BR.isInteresting(SecondThisRegion)) {
+      BR.markInteresting(SecondThisRegion);
+      BR.markNotInteresting(FirstThisRegion);
+    }
+    if (BR.isInteresting(SecondThisRegion) &&
+        !BR.isInteresting(FirstThisRegion)) {
+      BR.markInteresting(FirstThisRegion);
+      BR.markNotInteresting(SecondThisRegion);
+    }
+    // TODO: We need to emit some note here probably!!
+
+  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;

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

 void PathSensitiveBugReport::markNotInteresting(SymbolRef sym) {
   if (!sym)
     return;
   InterestingSymbols.erase(sym);
 
   // The metadata part of markInteresting is not reversed here.
   // Just making the same region not interesting is incorrect
   // in specific cases.
+  if (const auto *meta = dyn_cast<SymbolMetadata>(sym))
+    markNotInteresting(meta->getRegion());
 }
 
 void PathSensitiveBugReport::markInteresting(const MemRegion *R,
                                              bugreporter::TrackingKind TKind) {
   if (!R)
     return;
 
   R = R->getBaseRegion();

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