[analyzer] Add modeling of Eq operator in smart ptr

[vrnithinkumar]

Summary

Support for std::unique_ptr>::operator=

[vrnithinkumar]

• Add assignment to nullptr case

[NoQ]

This looks outright correct to me. I have random suggestions on note text here and there.

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

132

I suggest: Null pointer value move-assigned to 'P'.

139

I suggest: Smart pointer 'PToMove' is null; previous value moved to 'P'. Or maybe instead keep the note that the move-checker currently emits?

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

351

I'd prefer to call this AssignOp to avoid confusion with ==. While your naming is correct, I always found this nomenclature confusing.

384

I also find the names of the variables confusing.

Instead of ArgRegion what about OtherSmartPtrRegion?
Instead of ArgRegionVal what about OtherInnerPtr?

391

Adding return after every addTransition is a good practive that we should follow.

419

Isn't this the same as the beginning of the note tag above?

I wonder if there is a way to deduplicate this code. Not a huge priority though as I do not have an immediate idea for doing this in a clean way.

[vsavchenko]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

351

IMO it's not a question of preference with this one, EqOp is misleading.

399

The grammar seems off in this one. I think it should be smith like "after being moved to" or "after it was moved to".

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

132

+1

[vrnithinkumar]

• Addressing review comments

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

351

Changed to AssignOp

384

Changed to OtherSmartPtrRegion and OtherInnerPtr

391

Added return

399

Changed to "after being moved to"

419

Yes.
The check for bug type is duplicated across all the note tags.

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

132

Updated to Null pointer value move-assigned to 'P'

139

Going with first option "Smart pointer 'PToMove' is null; previous value moved to 'P'"

[NoQ]

Looks amazing now.

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
 //
 //===----------------------------------------------------------------------===//
 //
checkRegionChanges(ProgramStateRef State,
                      ArrayRef<const MemRegion *> ExplicitRegions,
                      ArrayRef<const MemRegion *> Regions,
                      const LocationContext *LCtx, const CallEvent *Call) 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;
+  bool handleEqOp(const CallEvent &Call, 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}};
 };
bool SmartPtrModeling::isNullAfterMoveMethod(const CallEvent &Call) const {
   // 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();
 }
 
 bool SmartPtrModeling::evalCall(const CallEvent &Call,
                                 CheckerContext &C) const {
-
   ProgramStateRef State = C.getState();
   if (!smartptr::isStdSmartPtrCall(Call))
     return false;
 
   if (isNullAfterMoveMethod(Call)) {
     const MemRegion *ThisR =
         cast<CXXInstanceCall>(&Call)->getCXXThisVal().getAsRegion();
 
if (Call.getNumArgs() == 0) {
           OS << " is constructed using a null value";
         else
           OS << " is constructed";
       }));
     }
     return true;
   }
 
+  if (handleEqOp(Call, C))
+    return true;
+
   const SmartPtrMethodHandlerFn *Handler = SmartPtrMethodHandlers.lookup(Call);
   if (!Handler)
     return false;
   (this->**Handler)(Call, C);
 
   return C.isDifferent();
 }
 
C.addTransition(
         BR.markInteresting(ArgRegion);
         OS << "Swapped null smart pointer ";
         ArgRegion->printPretty(OS);
         OS << " with smart pointer ";
         ThisRegion->printPretty(OS);
       }));
 }
 
+bool SmartPtrModeling::handleEqOp(const CallEvent &Call,
+                                  CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
+  const auto *OC = dyn_cast<CXXMemberOperatorCall>(&Call);
+  if (!OC)
+    return false;
+  OverloadedOperatorKind OOK = OC->getOverloadedOperator();
+  if (OOK != OO_Equal)
+    return false;
+  const MemRegion *ThisRegion = OC->getCXXThisVal().getAsRegion();
+  if (!ThisRegion)
+    return false;
+  const MemRegion *ArgRegion = OC->getArgSVal(0).getAsRegion();
+  if (!ArgRegion)
+    return false;
+
+  const auto *ArgRegionVal = State->get<TrackedRegionMap>(ArgRegion);
+  if (ArgRegionVal) {
+    State = State->set<TrackedRegionMap>(ThisRegion, *ArgRegionVal);
+    auto NullVal = C.getSValBuilder().makeNull();
+    State = State->set<TrackedRegionMap>(ArgRegion, NullVal);
+    bool IsArgValNull = ArgRegionVal->isZeroConstant();
+
+    C.addTransition(State, C.getNoteTag([ThisRegion, ArgRegion, IsArgValNull](
+                                            PathSensitiveBugReport &BR,
+                                            llvm::raw_ostream &OS) {
+      if (&BR.getBugType() != smartptr::getNullDereferenceBugType())
+        return;
+      if (BR.isInteresting(ArgRegion)) {
+        OS << "Smart pointer ";
+        ArgRegion->printPretty(OS);
+        OS << " is null after moved to ";
+        ThisRegion->printPretty(OS);
+      }
+      if (BR.isInteresting(ThisRegion) && IsArgValNull) {
+        OS << "Smart pointer ";
+        ThisRegion->printPretty(OS);
+        OS << " is null after a null value moved from ";
+        ArgRegion->printPretty(OS);
+        BR.markInteresting(ArgRegion);
+      }
+    }));
+  } else {
+    // In case we dont know anything about value we are moving from
+    // remove the entry from map for which smart pointer got moved to.
+    auto NullVal = C.getSValBuilder().makeNull();
+    State = State->remove<TrackedRegionMap>(ThisRegion);
+    State = State->set<TrackedRegionMap>(ArgRegion, NullVal);
+    C.addTransition(State);
+  }
+  return true;
+}
+
 void ento::registerSmartPtrModeling(CheckerManager &Mgr) {
   auto *Checker = Mgr.registerChecker<SmartPtrModeling>();
   Checker->ModelSmartPtrDereference =
       Mgr.getAnalyzerOptions().getCheckerBooleanOption(
           Checker, "ModelSmartPtrDereference");
 }
 
 bool ento::shouldRegisterSmartPtrModeling(const CheckerManager &mgr) {
   const LangOptions &LO = mgr.getLangOpts();
   return LO.CPlusPlus;
 }

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

void noNoteTagsForNonInterestingRegion() {
   std::unique_ptr<A> P2; // No note.
   P1.release(); // No note.
   P1.reset(); // No note.
   P1.swap(P2); // No note.
   P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   // expected-note@-1{{Dereference of null smart pointer 'P'}}
 }
 
-void noNoteTagsForNonMatchingBugType() {
-  std::unique_ptr<A> P; // No note.
-  std::unique_ptr<A> P1; // No note.
-  P1 = std::move(P); // expected-note {{Smart pointer 'P' of type 'std::unique_ptr' is reset to null when moved from}}
-  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' of type 'std::unique_ptr' [cplusplus.Move]}}
-  // expected-note@-1 {{Dereference of null smart pointer 'P' of type 'std::unique_ptr'}}
+void drefOnMovedFromValidPtr() {
+  std::unique_ptr<A> PToMove(new A());  // expected-note {{Smart pointer 'PToMove' is constructed}}
+  // FIXME: above note should go away once we fix marking region not interested. 
+  std::unique_ptr<A> P;
+  P = std::move(PToMove); // expected-note {{Smart pointer 'PToMove' is null after moved to 'P'}}
+  PToMove->foo(); // expected-warning {{Dereference of null smart pointer 'PToMove' [alpha.cplusplus.SmartPtr]}}
+  // expected-note@-1 {{Dereference of null smart pointer 'PToMove'}}
+}
+
+void drefOnMovedToNullPtr() {
+  std::unique_ptr<A> PToMove(new A());
+  std::unique_ptr<A> P;
+  P = std::move(PToMove); // No note.
+  P->foo(); // No warning.
+}
+
+void derefOnNullPtrGotMovedFromValidPtr() {
+  std::unique_ptr<A> P(new A()); // expected-note {{Smart pointer 'P' is constructed}}
+  // FIXME: above note should go away once we fix marking region not interested. 
+  std::unique_ptr<A> PToMove; // expected-note {{Default constructed smart pointer 'PToMove' is null}}
+  P = std::move(PToMove); // expected-note {{Smart pointer 'P' is null after a null value moved from 'PToMove'}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+  // expected-note@-1 {{Dereference of null smart pointer 'P'}}
+}
+
+void drefOnMovedUnknownPtr(std::unique_ptr<A> PToMove) {
+  std::unique_ptr<A> P;
+  P = std::move(PToMove);
+  PToMove->foo(); // expected-warning {{Dereference of null smart pointer 'PToMove' [alpha.cplusplus.SmartPtr]}}
+  // expected-note@-1 {{Dereference of null smart pointer 'PToMove'}}
 }

clang/test/Analysis/smart-ptr.cpp

void derefAfterAssignment() {
     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
+    Q->foo(); // expected-warning {{Dereference of null smart pointer 'Q' [alpha.cplusplus.SmartPtr]}}
   }
 }
 
 void derefOnSwappedNullPtr() {
   std::unique_ptr<A> P(new A());
   std::unique_ptr<A> PNull;
   P.swap(PNull);
   PNull->foo(); // No warning.
void derefOnSwappedValidPtr() {
   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 drefOnMovedFromValidPtr() {
+  std::unique_ptr<A> PToMove(new A());
+  std::unique_ptr<A> P;
+  P = std::move(PToMove);
+  PToMove->foo(); // expected-warning {{Dereference of null smart pointer 'PToMove' [alpha.cplusplus.SmartPtr]}}
+}
+
+void drefOnMovedToNullPtr() {
+  std::unique_ptr<A> PToMove(new A());
+  std::unique_ptr<A> P;
+  P = std::move(PToMove); // No note.
+  P->foo(); // No warning.
+}
+
+void derefOnNullPtrGotMovedFromValidPtr() {
+  std::unique_ptr<A> P(new A());
+  std::unique_ptr<A> PToMove;
+  P = std::move(PToMove);
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+}
+
+void drefOnMovedFromUnknownPtr(std::unique_ptr<A> PToMove) {
+  std::unique_ptr<A> P;
+  P = std::move(PToMove);
+  P->foo(); // No warning.
+}
+
+void drefOnMovedUnknownPtr(std::unique_ptr<A> PToMove) {
+  std::unique_ptr<A> P;
+  P = std::move(PToMove);
+  PToMove->foo(); // expected-warning {{Dereference of null smart pointer 'PToMove' [alpha.cplusplus.SmartPtr]}}
+}