[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

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 ↗

(On Diff #405030)

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/include/clang/StaticAnalyzer/Checkers/Checkers.td

def StdCLibraryFunctionsChecker : Checker<"StdCLibraryFunctions">,
   Documentation<NotDocumented>,
   Hidden;
 
 def TrustNonnullChecker : Checker<"TrustNonnull">,
   HelpText<"Trust that returns from framework methods annotated with _Nonnull "
            "are not null">,
   Documentation<NotDocumented>;
 
+def TrustReturnsNonnullChecker : Checker<"TrustReturnsNonnull">,
+  HelpText<"Trust that returns from methods annotated with returns_nonnull "
+           "are not null">,
+  Documentation<NotDocumented>;
+
 } // end "apiModeling"
 
 //===----------------------------------------------------------------------===//
 // Evaluate "builtin" functions.
 //===----------------------------------------------------------------------===//
 
 let ParentPackage = CoreBuiltin in {
 

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   StdLibraryFunctionsChecker.cpp
   STLAlgorithmModeling.cpp
   StreamChecker.cpp
   StringChecker.cpp
   Taint.cpp
   TaintTesterChecker.cpp
   TestAfterDivZeroChecker.cpp
   TraversalChecker.cpp
   TrustNonnullChecker.cpp
+  TrustReturnsNonnullChecker.cpp
   UndefBranchChecker.cpp
   UndefCapturedBlockVarChecker.cpp
   UndefResultChecker.cpp
   UndefinedArraySubscriptChecker.cpp
   UndefinedAssignmentChecker.cpp
   UninitializedObject/UninitializedObjectChecker.cpp
   UninitializedObject/UninitializedPointee.cpp
   UnixAPIChecker.cpp

clang/lib/StaticAnalyzer/Checkers/TrustReturnsNonnullChecker.cpp

+//== 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/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 (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 {
+    QualType ExprRetType = Call.getResultType();
+    if (!ExprRetType->isAnyPointerType())
+      return false;
+
+    if (Call.getDecl()->hasAttr<ReturnsNonNullAttr>()) {
+      return true;
+    }
+
+    return false;
+  }
+};
+
+} // namespace
+
+void ento::registerTrustReturnsNonnullChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<TrustReturnsNonnullChecker>(Mgr.getASTContext());
+}
+
+bool ento::shouldRegisterTrustReturnsNonnullChecker(const CheckerManager &mgr) {
+  return true;
+}

clang/test/Analysis/analyzer-enabled-checkers.c

 // RUN: %clang --analyze %s --target=x86_64-pc-linux-gnu \
 // RUN:   -Xclang -analyzer-list-enabled-checkers \
 // RUN:   -Xclang -analyzer-display-progress \
 // RUN:   2>&1 | FileCheck %s --implicit-check-not=ANALYZE \
 // RUN:                       --implicit-check-not=\.
 
 // CHECK:      OVERVIEW: Clang Static Analyzer Enabled Checkers List
 // CHECK-EMPTY:
 // CHECK-NEXT: core.CallAndMessageModeling
 // CHECK-NEXT: apiModeling.StdCLibraryFunctions
 // CHECK-NEXT: apiModeling.TrustNonnull
+// CHECK-NEXT: apiModeling.TrustReturnsNonnull
 // CHECK-NEXT: apiModeling.llvm.CastValue
 // CHECK-NEXT: apiModeling.llvm.ReturnValue
 // CHECK-NEXT: core.CallAndMessage
 // CHECK-NEXT: core.DivideZero
 // CHECK-NEXT: core.DynamicTypePropagation
 // CHECK-NEXT: core.NonNullParamChecker
 // CHECK-NEXT: core.NonnilStringConstants
 // CHECK-NEXT: core.NullDereference

clang/test/Analysis/returns_nonnull-attribute.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,apiModeling.TrustReturnsNonnull -verify %s
+
+int *foo() __attribute__((returns_nonnull));
+
+int *foo_no_attribute();
+
+int test_foo() {
+  int *x = foo();
+  if (x) {}
+  return *x; // no-warning
+}
+
+int test_foo_no_attribute() {
+  int *x = foo_no_attribute();
+  if (x) {}
+  return *x;  // expected-warning{{Dereference of null pointer}}
+}

clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c

 // CHECK-EMPTY:
 // CHECK-NEXT: core.CallAndMessageModeling
 // CHECK-NEXT: core.CallAndMessage
 // CHECK-NEXT: core.NonNullParamChecker
 // CHECK-NEXT: alpha.unix.Stream
 // CHECK-NEXT: apiModeling.StdCLibraryFunctions
 // CHECK-NEXT: alpha.unix.StdCLibraryFunctionArgs
 // CHECK-NEXT: apiModeling.TrustNonnull
+// CHECK-NEXT: apiModeling.TrustReturnsNonnull
 // CHECK-NEXT: apiModeling.llvm.CastValue
 // CHECK-NEXT: apiModeling.llvm.ReturnValue
 // CHECK-NEXT: core.DivideZero
 // CHECK-NEXT: core.DynamicTypePropagation
 // CHECK-NEXT: core.NonnilStringConstants
 // CHECK-NEXT: core.NullDereference
 // CHECK-NEXT: core.StackAddrEscapeBase
 // CHECK-NEXT: core.StackAddressEscape

llvm/utils/gn/secondary/clang/lib/StaticAnalyzer/Checkers/BUILD.gn

sources = [
     "StdLibraryFunctionsChecker.cpp",
     "StreamChecker.cpp",
     "StringChecker.cpp",
     "Taint.cpp",
     "TaintTesterChecker.cpp",
     "TestAfterDivZeroChecker.cpp",
     "TraversalChecker.cpp",
     "TrustNonnullChecker.cpp",
+    "TrustReturnsNonnullChecker.cpp",
     "UndefBranchChecker.cpp",
     "UndefCapturedBlockVarChecker.cpp",
     "UndefResultChecker.cpp",
     "UndefinedArraySubscriptChecker.cpp",
     "UndefinedAssignmentChecker.cpp",
     "UninitializedObject/UninitializedObjectChecker.cpp",
     "UninitializedObject/UninitializedPointee.cpp",
     "UnixAPIChecker.cpp",