[analyzer] Fix for faulty namespace test in SmartPtrModelling

[RedDocMD]

Summary

This patch:

• Fixes how the std-namespace test is written in SmartPtrModelling

(now accounts for functions with no Decl available)

• Adds the smart pointer checker flag check where it was missing

[RedDocMD]

Reformatted patch

[xazax.hun]

Commented some nits, but overall looks good to me.

However, could you include some tests? We usually do not commit any changes without tests unless it is really hard to create one. But I suspect that this is not the case here.

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

274

Can we model a function call without a declaration? I wonder if we should make this check more eagerly in evalCall.

285–289

I'd prefer not repeating the ModelSmartPtrDereference check.

[RedDocMD]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

274

I think it is not that there is no Decl, but it is more likely the Decl is not available at that time.

void foo(void (*bar)(bool, bool)) {
    bar();
}

If just foo is analyzed, then there is no Decl.
If a call to foo is analyzed, then there is a Decl.
That said, I think the check can be done in evalCall itself. I don't think it will cause a problem.

285–289

I don't think we can do that, since the isBoolConversion() check has a branch which is executed when ModelSmartPtrDereference is false.

[RedDocMD]

Added a simple test

[RedDocMD]

Would this test do?

[vsavchenko]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

258–261

I think you can have a separate function:

bool isStdFunctionCall(const CallEvent &Call) {
  return Call.getDecl() && Call.getDecl()->getDeclContext()->isStdNamespace();
}

and then use it like this:

if (Call.getNumArgs() != 2 || !isStdFunctionCall(Call))
  return false;

[vsavchenko]

Also, I tested this fix on a set of open-source projects and I don't see any problems.

[RedDocMD]

Refactored out check

[vsavchenko]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

252–255

Can we use a one-liner that I suggested?

[RedDocMD]

More refactor

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

252–255

Sure

[vsavchenko]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

252

nit: there's an extra space here.

[RedDocMD]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

252

Wow, I didn't know this is even valid syntax 😂

[RedDocMD]

Removed unnecessary white space

[vsavchenko]

Great, LGTM!
But let's wait for @xazax.hun anyways

[xazax.hun]

LGTM!

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

285–289

That function is way below. I was just thinking about merging the first two check.

[vsavchenko]

Can we please land the fix?

[RedDocMD]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

285–289

Looks like I missed this. Sorry!

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

 
 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);
 }
 
+static bool isStdFunctionCall(const CallEvent &Call) {
+  return Call.getDecl() && Call.getDecl()->getDeclContext()->isStdNamespace();
+}
+
 bool isStdOstreamOperatorCall(const CallEvent &Call) {
-  if (Call.getNumArgs() != 2 ||
-      !Call.getDecl()->getDeclContext()->isStdNamespace())
+  if (Call.getNumArgs() != 2 || !isStdFunctionCall(Call))
     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));
 }
 
+static bool isPotentiallyComparisionOpCall(const CallEvent &Call) {
+  if (Call.getNumArgs() != 2 || !isStdFunctionCall(Call))
+    return false;
+  return smartptr::isStdSmartPtr(Call.getArgExpr(0)) ||
+         smartptr::isStdSmartPtr(Call.getArgExpr(1));
+}
+
 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 (ModelSmartPtrDereference && isPotentiallyComparisionOpCall(Call))
 
-  if (isStdOstreamOperatorCall(Call))
+  if (ModelSmartPtrDereference && 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;

clang/test/Analysis/smart-ptr.cpp

 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
+
+// The following test isn't really a "smart-ptr" test
+// It came up during a bug fix (D106296)
+void testCheckForFunctionsWithNoDecl(void (*bar)(bool, bool)) {
+  // This should NOT crash.
+  bar(true, false);
+}