[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

218–219

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

218–219

Does this work?

[NoQ]

Yes, I think this totally works now!

Can you also add some tests?

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

85

This should be static right?

98

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?

221–222

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

86

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

111

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.

194

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

98–99

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.

192

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 ↗

(On Diff #357422)

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 ↗

(On Diff #357422)

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

[RedDocMD]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

192

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

86

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

102

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.

192

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/SmartPtr.h

 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.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);
+bool isStdSmartPtr(const Expr *E);
 
 /// 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

CallDescriptionMap<SmartPtrMethodHandlerFn> SmartPtrMethodHandlers{
       {{"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
+bool hasStdClassWithName(const CXXRecordDecl *RD,
+                         const SmallVectorImpl<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;
+}
+
 // 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;
+  return isStdSmartPtr(MethodDecl->getParent());
+}
 
-  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;
+bool isStdSmartPtr(const CXXRecordDecl *RD) {
+  return hasStdClassWithName(
+      RD, SmallVector<StringRef, 3>{"shared_ptr", "unique_ptr", "weak_ptr"});
+
+bool isStdSmartPtr(const Expr *E) {
+  return isStdSmartPtr(E->getType()->getAsCXXRecordDecl());
 }
 
 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
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();
 }
 
+bool isStdBasicOstream(const Expr *E) {
+  const auto *RD = E->getType()->getAsCXXRecordDecl();
+  return hasStdClassWithName(RD, SmallVector<StringRef, 1>{"basic_ostream"});
+}
+
+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 smartptr::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 true;
+
   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.