[analyzer] Handle << operator for std::unique_ptr

[RedDocMD]

Summary

This patch handles the << operator defined for std::unique_ptr in
the std namespace (ignores custom overloads of the operator).

[NoQ]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

223–224

When you're doing evalCall you're responsible for modeling *all* aspects of the call. One does not simply say that they modeled all aspects of the call when they didn't even set the return value :)

Similarly to how make_unique delegates work to the constructor of the managed object and ~unique_ptr delegates work to the destructor of the managed object, I suspect this code could delegate work to basic_ostream::operator<<(T *). We don't yet have any facilities to implement such logic yet (i.e., entering function call from a checker callback).

Given that we don't do much modeling of basic_ostream yet, I think it's perfectly fine to invalidate the stream entirely (which would be as precise as whatever the default evaluation gave us) (see ProgramState::invalidateRegions) and return the reference to the stream (which is already better than what the default evaluation gave us); additionally, we get extra precision because we don't invalidate the rest of the heap. That's the bare minimum of what we have to do here if we are to do anything at all.

This also gives some ideas of how to write tests for this patch.

That said, I suspect that this patch is not critical to enabling the checker by default, because we probably already know that this method doesn't change the inner pointer value (simply through not doing anything special) (and accepting the smart pointer by const reference, so even if we implement invalidation it will probably still "just work") (?).

[RedDocMD]

Invalidating regions

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

223–224

Does this work?

[NoQ]

Yes, I think this totally works now!

Can you also add some tests?

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

86

This should be static right?

110

I think the new functions that you've added aren't yet used in an inter-checker manner. Maybe keep them as static too until we have an actual use case?

226–227

Might also be an UnknownVal which doesn't have a region. It's hard to test because there doesn't exist a test in which UnknownVal appears for a good reason (they only appear for bad reasons) but I'd still rather bail out.

[vsavchenko]

Good job, great to see how you are going through the whole list of methods!

As for the patch, some tests and COMMENTS will be nice. Also, I think that for a checker that models quite a lot of functions and methods, we need to follow the pattern:

if (isMethodA(...)) {
  return handleMethodA(...);
}
if (isMethodB(...)) {
  return handleMethodB(...);
}
...

however small implementations for those methods are. It will give fast insight into which methods are actually supported.

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

87

When you change those vectors of names to global arrays, we can change it to ArrayRef to be more idiomatic LLVM code.

119

This is a compile-time constant, I don't think we should construct it every time in runtime. Global array of three names is totally fine.

197

Same goes here

[RedDocMD]

Little refactors

[RedDocMD]

I will be figuring out some tests tomorrow morning.

[RedDocMD]

Tests implemented

[vsavchenko]

Great, thanks for addressing my comments! I still have a couple of minor suggestions though.

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

99–100

If it's a compile-time constant, let's make sure it is.
nit: I prefer global scope constants to be CAPITALIZED, so it's easier to spot them in the code.

195

Same here + don't call it "Name" (singular). It is a) an array and b) in the future, we might add more things to it, so we shouldn't need to rename it everywhere.

[NoQ]

Great, thanks!

clang/test/Analysis/smart-ptr.cpp

472–474

Another thing to test could be that global variables are not invalidated. Eg.:

int glob;
void foo(std::unique_ptr<int> P) {
  int x = glob;
  std::cout << P;
  int y = glob;
  clang_analyzer_eval(x == y); // expected-warning{{TRUE}}
}

476

This deserves a FIXME: because the expected result is still TRUE.

[RedDocMD]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

195

Well, we won't have to rename but we will still have to change the array length everywhere if more names are added.

[RedDocMD]

Little refactors, one more test

[vsavchenko]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

87

ArrayRef is already a ref (and it says it in its name), so you should remove it there.

103

ArrayRef has an implicit constructor from C-arrays, putting size here is unnecessary.
And the reason why you were not able to do this is type mismatch. STD_PTR_NAMES is array of llvm::StringLiteral, but you chose ArrayRef<StringRef> as the type of the argument.

195

No, we don't have to do that. See the reason in the other comment.
Whenever you need to explicitly hardcode size for a stack-allocated array, you are doing something wrong. Stack-allocated everything (VLAs aside) is required to have a fixed size.

[RedDocMD]

Removed stupid mistakes

[vsavchenko]

In D105421#2873458, @RedDocMD wrote:

Removed stupid mistakes

No need for self deprecation here! Thanks for addressing these!

[RedDocMD]

Post rebase cleanup

[RedDocMD]

For some reason this revision did not get automatically closed after commit, manually merging it.

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

private:
   void handleSwap(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 handleOstreamOperator(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},
       {{"get"}, &SmartPtrModeling::handleGet}};
 };
 } // 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,
+                                const ArrayRef<StringRef> &Names) {
+  if (!RD || !RD->getDeclContext()->isStdNamespace())
+    return false;
+  if (RD->getDeclName().isIdentifier()) {
+    StringRef Name = RD->getName();
+    return llvm::any_of(Names, [&Name](StringRef GivenName) -> bool {
+      return Name == GivenName;
+    });
+  }
+  return false;
+}
+
+constexpr llvm::StringLiteral STD_PTR_NAMES[] = {"shared_ptr", "unique_ptr",
+                                                 "weak_ptr"};
+
+static bool isStdSmartPtr(const CXXRecordDecl *RD) {
+  return hasStdClassWithName(RD, {STD_PTR_NAMES, 3});
+}
+
+static bool isStdSmartPtr(const Expr *E) {
+  return isStdSmartPtr(E->getType()->getAsCXXRecordDecl());
+}
+
 // 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 false;
-
-  if (RecordDecl->getDeclName().isIdentifier()) {
-    StringRef Name = RecordDecl->getName();
-    return Name == "shared_ptr" || Name == "unique_ptr" || Name == "weak_ptr";
-  }
-  return false;
+  return isStdSmartPtr(MethodDecl->getParent());
 }
 
 bool isNullSmartPtr(const ProgramStateRef State, const MemRegion *ThisRegion) {
   const auto *InnerPointVal = State->get<TrackedRegionMap>(ThisRegion);
   return InnerPointVal &&
          !State->assume(InnerPointVal->castAs<DefinedOrUnknownSVal>(), true);
 }
 } // namespace smartptr
 } // namespace ento
bool SmartPtrModeling::isBoolConversionMethod(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();
 }
 
+constexpr llvm::StringLiteral BASIC_OSTREAM_NAMES[] = {"basic_ostream"};
+
+bool isStdBasicOstream(const Expr *E) {
+  const auto *RD = E->getType()->getAsCXXRecordDecl();
+  return hasStdClassWithName(RD, {BASIC_OSTREAM_NAMES, 1});
+}
+
+bool isStdOstreamOperatorCall(const CallEvent &Call) {
+  if (Call.getNumArgs() != 2 ||
+      !Call.getDecl()->getDeclContext()->isStdNamespace())
+    return false;
+  const auto *FC = dyn_cast<SimpleFunctionCall>(&Call);
+  if (!FC)
+    return false;
+  const FunctionDecl *FD = FC->getDecl();
+  if (!FD->isOverloadedOperator())
+    return false;
+  const OverloadedOperatorKind OOK = FD->getOverloadedOperator();
+  if (OOK != clang::OO_LessLess)
+    return false;
+  return isStdSmartPtr(Call.getArgExpr(1)) &&
+         isStdBasicOstream(Call.getArgExpr(0));
+}
+
 bool SmartPtrModeling::evalCall(const CallEvent &Call,
                                 CheckerContext &C) const {
   ProgramStateRef State = C.getState();
+
+  if (isStdOstreamOperatorCall(Call))
+    return handleOstreamOperator(Call, 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.
bool SmartPtrModeling::evalCall(const CallEvent &Call,
   const SmartPtrMethodHandlerFn *Handler = SmartPtrMethodHandlers.lookup(Call);
   if (!Handler)
     return false;
   (this->**Handler)(Call, C);
 
   return C.isDifferent();
 }
 
+bool SmartPtrModeling::handleOstreamOperator(const CallEvent &Call,
+                                             CheckerContext &C) const {
+  // operator<< does not modify the smart pointer.
+  // And we don't really have much of modelling of basic_ostream.
+  // So, we are better off:
+  // 1) Invalidating the mem-region of the ostream object at hand.
+  // 2) Setting the SVal of the basic_ostream as the return value.
+  // Not very satisfying, but it gets the job done, and is better
+  // than the default handling. :)
+
+  ProgramStateRef State = C.getState();
+  const auto StreamVal = Call.getArgSVal(0);
+  const MemRegion *StreamThisRegion = StreamVal.getAsRegion();
+  if (!StreamThisRegion)
+    return false;
+  State =
+      State->invalidateRegions({StreamThisRegion}, Call.getOriginExpr(),
+                               C.blockCount(), C.getLocationContext(), false);
+  State =
+      State->BindExpr(Call.getOriginExpr(), C.getLocationContext(), StreamVal);
+  C.addTransition(State);
+  return true;
+}
+
 void SmartPtrModeling::checkDeadSymbols(SymbolReaper &SymReaper,
                                         CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   // Clean up dead regions from the region map.
   TrackedRegionMapTy TrackedRegions = State->get<TrackedRegionMap>();
   for (auto E : TrackedRegions) {
     const MemRegion *Region = E.first;
     bool IsRegDead = !SymReaper.isLiveRegion(Region);

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

 // TODO :: Once the deleter parameter is added update with additional template parameter.
 template <typename T>
 void swap(unique_ptr<T> &x, unique_ptr<T> &y) noexcept {
   x.swap(y);
 }
 } // namespace std
 #endif
 
+namespace std {
+template <class CharT>
+class basic_ostream;
+
+using ostream = basic_ostream<char>;
+
+extern std::ostream cout;
+
+ostream &operator<<(ostream &, const string &);
+
+#if __cplusplus >= 202002L
+template <class T>
+ostream &operator<<(ostream &, const std::unique_ptr<T> &);
+#endif
+} // namespace std
+
 #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); }
 void* operator new[](std::size_t size, const std::nothrow_t&) throw() { return std::malloc(size); }
 void operator delete(void* ptr, const std::nothrow_t&) throw() { std::free(ptr); }

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
 
+// 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++20 -verify %s
+
 #include "Inputs/system-header-simulator-cxx.h"
 
 void clang_analyzer_warnIfReached();
 void clang_analyzer_numTimesReached();
 void clang_analyzer_eval(bool);
 void clang_analyzer_warnOnDeadSymbol(int *);
 
 void derefAfterMove(std::unique_ptr<int> P) {

 }
 
 void derefAfterBranchingOnUnknownInnerPtr(std::unique_ptr<A> P) {
   A *RP = P.get();
   if (!RP) {
     P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   }
 }
+
+#if __cplusplus >= 202002L
+
+void testOstreamOverload(std::unique_ptr<int> P) {
+  auto &Cout = std::cout;
+  auto &PtrCout = std::cout << P;
+  auto &StringCout = std::cout << "hello";
+  // We are testing the fact that in our modelling of
+  // operator<<(basic_ostream<T1> &, const unique_ptr<T2> &)
+  // we set the return SVal to the SVal of the ostream arg.
+  clang_analyzer_eval(&Cout == &PtrCout);    // expected-warning {{TRUE}}
+  // FIXME: Technically, they should be equal,
+  // that hasn't been modelled yet.
+  clang_analyzer_eval(&Cout == &StringCout); // expected-warning {{UNKNOWN}}
+}
+
+int glob;
+void testOstreamDoesntInvalidateGlobals(std::unique_ptr<int> P) {
+  int x = glob;
+  std::cout << P;
+  int y = glob;
+  clang_analyzer_eval(x == y); // expected-warning {{TRUE}}
+}
+
+#endif