[analyzer] Adds TrustReturnsNonnullChecker

[t-rasmud]

Summary

This checker adds nullability-related assumptions to methods annotated with returns_nonnull attribute.

[NoQ]

Lovely! I have some nits.

@NoQ is my actual account, sorry for the mess >.<

clang/lib/StaticAnalyzer/Checkers/TrustReturnsNonnullChecker.cpp

1–2

This line broke.

42

Parameter C appears to be unused.

47

Call.getDecl() has to be checked for null. Calls with null decls appear when the function being called is unknown, eg. during calls through unknown/symbolic pointer-to-function:

void test(void *(*f)(void)) {
  // will probably crash the compiler, worth a test case
  f();
}

48

if (x) {
  return true;
}
return false;

can usually be shortened to

return x;

.

[t-rasmud]

This diff addresses review comments.

[NoQ]

Great, thanks!

The phabricator expects complete patches reuploaded (i.e., diffs against main branch rather than diffs against the previous upload); but it knows how to produce patches-on-patches from complete patches through the History tab. Another reason to upload complete patches is to unconfuse pre-merge buildbots (the "Build Status: patch application failed" part).

So please re-upload and that'd be a ✅

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

114–115 ↗

(On Diff #404737)

As this checker mimics that other checker, the question inevitably arises whether the two can be merged. The other checker is currently stuffed with extra code to handle operator == which is possibly no longer necessary since D82445 provides a more general solution. I guess this could be an interesting refactoring pass.

clang/test/Analysis/test-decl-crash.cpp

6

It makes sense to put the new test into the same file given that they share the RUN: line.

[t-rasmud]

This diff merges relevant test cases into one file.

[NoQ]

Awesome! I'll commit.

clang/lib/StaticAnalyzer/Checkers/TrustReturnsNonnullChecker.cpp

-//== TrustReturnsNonnullChecker.cpp --------- API nullability modeling -*- C++
-//-*--==//
+//== TrustReturnsNonnullChecker.cpp --------- API nullability modeling -*- 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
 //
 //===----------------------------------------------------------------------===//
 //
 // This checker adds nullability-related assumptions to methods annotated with
 // returns_nonnull attribute.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/AST/Attr.h"
+#include "clang/AST/Decl.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 
 using namespace clang;
 using namespace ento;
 
 namespace {
 
 class TrustReturnsNonnullChecker : public Checker<check::PostCall> {
 
 public:
   TrustReturnsNonnullChecker(ASTContext &Ctx) {}
 
   void checkPostCall(const CallEvent &Call, CheckerContext &C) const {
     ProgramStateRef State = C.getState();
 
-    if (isNonNullPtr(Call, C))
+    if (isNonNullPtr(Call))
       if (auto L = Call.getReturnValue().getAs<Loc>())
         State = State->assume(*L, /*assumption=*/true);
 
     C.addTransition(State);
   }
 
 private:
   /// \returns Whether the method declaration has the attribute returns_nonnull.
-  bool isNonNullPtr(const CallEvent &Call, CheckerContext &C) const {
+  bool isNonNullPtr(const CallEvent &Call) const {
     QualType ExprRetType = Call.getResultType();
-    if (!ExprRetType->isAnyPointerType())
+    const Decl *CallDeclaration =  Call.getDecl();
+    if (!ExprRetType->isAnyPointerType() || !CallDeclaration)
       return false;
 
-    if (Call.getDecl()->hasAttr<ReturnsNonNullAttr>()) {
-      return true;
-    }
-
-    return false;
+    return CallDeclaration->hasAttr<ReturnsNonNullAttr>();
   }
 };
 
 } // namespace
 
 void ento::registerTrustReturnsNonnullChecker(CheckerManager &Mgr) {
   Mgr.registerChecker<TrustReturnsNonnullChecker>(Mgr.getASTContext());
 }
 
 bool ento::shouldRegisterTrustReturnsNonnullChecker(const CheckerManager &mgr) {
   return true;
 }

clang/test/Analysis/test-decl-crash.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,apiModeling.TrustReturnsNonnull -verify %s
+
+// expected-no-diagnostics
+
+void test(void *(*f)(void)) {
+  // will probably crash the compiler, worth a test case
+  f();
+}
 No newline at end of file