[analyzer] Do not report CFError null dereference for nonnull params.

We want to trust user type annotations and stop assuming pointers declared
as nonnull still can be null. This functionality is implemented as part
of NonNullParamChecker because it already checks parameter attributes.
Whenever we start analyzing a new function, we assume that all parameters
with 'nonnull' attribute are indeed non-null.

Patch by Valeriy Savchenko!

Differential Revision: https://reviews.llvm.org/D77806

clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp

@@ -14,8 +14,9 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/AST/Attr.h"
+#include "clang/Analysis/AnyCall.h"
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
@@ -28,44 +29,82 @@ using namespace ento;
 
 namespace {
 class NonNullParamChecker
-  : public Checker< check::PreCall, EventDispatcher<ImplicitNullDerefEvent> > {
+    : public Checker<check::PreCall, check::BeginFunction,
+                     EventDispatcher<ImplicitNullDerefEvent>> {
   mutable std::unique_ptr<BugType> BTAttrNonNull;
   mutable std::unique_ptr<BugType> BTNullRefArg;
 
 public:
-
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
+  void checkBeginFunction(CheckerContext &C) const;
 
   std::unique_ptr<PathSensitiveBugReport>
-  genReportNullAttrNonNull(const ExplodedNode *ErrorN,
-                           const Expr *ArgE,
+  genReportNullAttrNonNull(const ExplodedNode *ErrorN, const Expr *ArgE,
                            unsigned IdxOfArg) const;
   std::unique_ptr<PathSensitiveBugReport>
   genReportReferenceToNullPointer(const ExplodedNode *ErrorN,
                                   const Expr *ArgE) const;
 };
-} // end anonymous namespace
 
-/// \return Bitvector marking non-null attributes.
-static llvm::SmallBitVector getNonNullAttrs(const CallEvent &Call) {
+template <class CallType>
+void setBitsAccordingToFunctionAttributes(const CallType &Call,
+                                          llvm::SmallBitVector &AttrNonNull) {
   const Decl *FD = Call.getDecl();
-  unsigned NumArgs = Call.getNumArgs();
-  llvm::SmallBitVector AttrNonNull(NumArgs);
+
   for (const auto *NonNull : FD->specific_attrs<NonNullAttr>()) {
     if (!NonNull->args_size()) {
-      AttrNonNull.set(0, NumArgs);
+      // Lack of attribute parameters means that all of the parameters are
+      // implicitly marked as non-null.
+      AttrNonNull.set();
       break;
     }
+
     for (const ParamIdx &Idx : NonNull->args()) {
+      // 'nonnull' attribute's parameters are 1-based and should be adjusted to
+      // match actual AST parameter/argument indices.
       unsigned IdxAST = Idx.getASTIndex();
-      if (IdxAST >= NumArgs)
+      if (IdxAST >= AttrNonNull.size())
         continue;
       AttrNonNull.set(IdxAST);
     }
   }
+}
+
+template <class CallType>
+void setBitsAccordingToParameterAttributes(const CallType &Call,
+                                           llvm::SmallBitVector &AttrNonNull) {
+  for (const ParmVarDecl *Parameter : Call.parameters()) {
+    unsigned ParameterIndex = Parameter->getFunctionScopeIndex();
+    if (ParameterIndex == AttrNonNull.size())
+      break;
+
+    if (Parameter->hasAttr<NonNullAttr>())
+      AttrNonNull.set(ParameterIndex);
+  }
+}
+
+template <class CallType>
+llvm::SmallBitVector getNonNullAttrsImpl(const CallType &Call,
+                                         unsigned ExpectedSize) {
+  llvm::SmallBitVector AttrNonNull(ExpectedSize);
+
+  setBitsAccordingToFunctionAttributes(Call, AttrNonNull);
+  setBitsAccordingToParameterAttributes(Call, AttrNonNull);
+
   return AttrNonNull;
 }
 
+/// \return Bitvector marking non-null attributes.
+llvm::SmallBitVector getNonNullAttrs(const CallEvent &Call) {
+  return getNonNullAttrsImpl(Call, Call.getNumArgs());
+}
+
+/// \return Bitvector marking non-null attributes.
+llvm::SmallBitVector getNonNullAttrs(const AnyCall &Call) {
+  return getNonNullAttrsImpl(Call, Call.param_size());
+}
+} // end anonymous namespace
+
 void NonNullParamChecker::checkPreCall(const CallEvent &Call,
                                        CheckerContext &C) const {
   if (!Call.getDecl())
@@ -75,7 +114,7 @@ void NonNullParamChecker::checkPreCall(const CallEvent &Call,
   unsigned NumArgs = Call.getNumArgs();
 
   ProgramStateRef state = C.getState();
-  ArrayRef<ParmVarDecl*> parms = Call.parameters();
+  ArrayRef<ParmVarDecl *> parms = Call.parameters();
 
   for (unsigned idx = 0; idx < NumArgs; ++idx) {
     // For vararg functions, a corresponding parameter decl may not exist.
@@ -83,15 +122,11 @@ void NonNullParamChecker::checkPreCall(const CallEvent &Call,
 
     // Check if the parameter is a reference. We want to report when reference
     // to a null pointer is passed as a parameter.
-    bool haveRefTypeParam =
+    bool HasRefTypeParam =
         HasParam ? parms[idx]->getType()->isReferenceType() : false;
-    bool haveAttrNonNull = AttrNonNull[idx];
+    bool ExpectedToBeNonNull = AttrNonNull.test(idx);
 
-    // Check if the parameter is also marked 'nonnull'.
-    if (!haveAttrNonNull && HasParam)
-      haveAttrNonNull = parms[idx]->hasAttr<NonNullAttr>();
-
-    if (!haveAttrNonNull && !haveRefTypeParam)
+    if (!ExpectedToBeNonNull && !HasRefTypeParam)
       continue;
 
     // If the value is unknown or undefined, we can't perform this check.
@@ -101,10 +136,10 @@ void NonNullParamChecker::checkPreCall(const CallEvent &Call,
     if (!DV)
       continue;
 
-    assert(!haveRefTypeParam || DV->getAs<Loc>());
+    assert(!HasRefTypeParam || DV->getAs<Loc>());
 
     // Process the case when the argument is not a location.
-    if (haveAttrNonNull && !DV->getAs<Loc>()) {
+    if (ExpectedToBeNonNull && !DV->getAs<Loc>()) {
       // If the argument is a union type, we want to handle a potential
       // transparent_union GCC extension.
       if (!ArgE)
@@ -145,9 +180,9 @@ void NonNullParamChecker::checkPreCall(const CallEvent &Call,
       if (ExplodedNode *errorNode = C.generateErrorNode(stateNull)) {
 
         std::unique_ptr<BugReport> R;
-        if (haveAttrNonNull)
+        if (ExpectedToBeNonNull)
           R = genReportNullAttrNonNull(errorNode, ArgE, idx + 1);
-        else if (haveRefTypeParam)
+        else if (HasRefTypeParam)
           R = genReportReferenceToNullPointer(errorNode, ArgE);
 
         // Highlight the range of the argument that was null.
@@ -164,8 +199,8 @@ void NonNullParamChecker::checkPreCall(const CallEvent &Call,
     if (stateNull) {
       if (ExplodedNode *N = C.generateSink(stateNull, C.getPredecessor())) {
         ImplicitNullDerefEvent event = {
-          V, false, N, &C.getBugReporter(),
-          /*IsDirectDereference=*/haveRefTypeParam};
+            V, false, N, &C.getBugReporter(),
+            /*IsDirectDereference=*/HasRefTypeParam};
         dispatchEvent(event);
       }
     }
@@ -180,6 +215,59 @@ void NonNullParamChecker::checkPreCall(const CallEvent &Call,
   C.addTransition(state);
 }
 
+/// We want to trust developer annotations and consider all 'nonnull' parameters
+/// as non-null indeed. Each marked parameter will get a corresponding
+/// constraint.
+///
+/// This approach will not only help us to get rid of some false positives, but
+/// remove duplicates and shorten warning traces as well.
+///
+/// \code
+///   void foo(int *x) [[gnu::nonnull]] {
+///     // . . .
+///     *x = 42;    // we don't want to consider this as an error...
+///     // . . .
+///   }
+///
+///   foo(nullptr); // ...and report here instead
+/// \endcode
+void NonNullParamChecker::checkBeginFunction(CheckerContext &Context) const {
+  // Planned assumption makes sense only for top-level functions.
+  // Inlined functions will get similar constraints as part of 'checkPreCall'.
+  if (!Context.inTopFrame())
+    return;
+
+  const LocationContext *LocContext = Context.getLocationContext();
+
+  const Decl *FD = LocContext->getDecl();
+  // AnyCall helps us here to avoid checking for FunctionDecl and ObjCMethodDecl
+  // separately and aggregates interfaces of these classes.
+  auto AbstractCall = AnyCall::forDecl(FD);
+  if (!AbstractCall)
+    return;
+
+  ProgramStateRef State = Context.getState();
+  llvm::SmallBitVector ParameterNonNullMarks = getNonNullAttrs(*AbstractCall);
+
+  for (const ParmVarDecl *Parameter : AbstractCall->parameters()) {
+    // 1. Check parameter if it is annotated as non-null
+    if (!ParameterNonNullMarks.test(Parameter->getFunctionScopeIndex()))
+      continue;
+
+    Loc ParameterLoc = State->getLValue(Parameter, LocContext);
+    // We never consider top-level function parameters undefined.
+    auto StoredVal =
+        State->getSVal(ParameterLoc).castAs<DefinedOrUnknownSVal>();
+
+    // 2. Assume that it is indeed non-null
+    if (ProgramStateRef NewState = State->assume(StoredVal, true)) {
+      State = NewState;
+    }
+  }
+
+  Context.addTransition(State);
+}
+
 std::unique_ptr<PathSensitiveBugReport>
 NonNullParamChecker::genReportNullAttrNonNull(const ExplodedNode *ErrorNode,
                                               const Expr *ArgE,

clang/test/Analysis/CheckNSError.m

@@ -22,6 +22,7 @@ extern NSString * const NSXMLParserErrorDomain ;
 - (void)myMethodWhichMayFail:(NSError **)error;
 - (BOOL)myMethodWhichMayFail2:(NSError **)error;
 - (BOOL)myMethodWhichMayFail3:(NSError **_Nonnull)error;
+- (BOOL)myMethodWhichMayFail4:(NSError **)error __attribute__((nonnull));
 @end
 
 @implementation A
@@ -38,6 +39,11 @@ extern NSString * const NSXMLParserErrorDomain ;
   *error = [NSError errorWithDomain:@"domain" code:1 userInfo:0]; // no-warning
   return 0;
 }
+
+- (BOOL)myMethodWhichMayFail4:(NSError **)error {                 // no-warning
+  *error = [NSError errorWithDomain:@"domain" code:1 userInfo:0]; // no-warning
+  return 0;
+}
 @end
 
 struct __CFError {};
@@ -62,4 +68,17 @@ int f3(CFErrorRef* error) {
   return 0;
 }
 
+int __attribute__((nonnull)) f4(CFErrorRef *error) {
+  *error = 0; // no-warning
+  return 0;
+}
 
+int __attribute__((nonnull(1))) f5(int *x, CFErrorRef *error) {
+  *error = 0; // expected-warning {{Potential null dereference}}
+  return 0;
+}
+
+int __attribute__((nonnull(2))) f6(int *x, CFErrorRef *error) {
+  *error = 0; // no-warning
+  return 0;
+}

clang/test/Analysis/UserNullabilityAnnotations.m

@@ -0,0 +1,36 @@
+// RUN: %clang_analyze_cc1 -verify -Wno-objc-root-class %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=nullability \
+// RUN:   -analyzer-checker=debug.ExprInspection
+
+void clang_analyzer_eval(int);
+
+@interface TestFunctionLevelAnnotations
+- (void)method1:(int *_Nonnull)x;
+- (void)method2:(int *)x __attribute__((nonnull));
+@end
+
+@implementation TestFunctionLevelAnnotations
+- (void)method1:(int *_Nonnull)x {
+  clang_analyzer_eval(x != 0); // expected-warning{{TRUE}}
+}
+
+- (void)method2:(int *)x {
+  clang_analyzer_eval(x != 0); // expected-warning{{TRUE}}
+}
+@end
+
+typedef struct NestedNonnullMember {
+  struct NestedNonnullMember *Child;
+  int *_Nonnull Value;
+} NestedNonnullMember;
+
+NestedNonnullMember *foo();
+
+void f1(NestedNonnullMember *Root) {
+  NestedNonnullMember *Grandson = Root->Child->Child;
+
+  clang_analyzer_eval(Root->Value != 0);         // expected-warning{{TRUE}}
+  clang_analyzer_eval(Grandson->Value != 0);     // expected-warning{{TRUE}}
+  clang_analyzer_eval(foo()->Child->Value != 0); // expected-warning{{TRUE}}
+}

clang/test/Analysis/nonnull.cpp

@@ -0,0 +1,34 @@
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core -verify %s
+
+void nonnull [[gnu::nonnull]] (int *q);
+
+void f1(int *p) {
+  if (p)
+    return;
+  nonnull(p); //expected-warning{{nonnull}}
+}
+
+void f2(int *p) {
+  if (p)
+    return;
+  auto lambda = [](int *q) __attribute__((nonnull)){};
+  lambda(p); //expected-warning{{nonnull}}
+}
+
+template <class... ARGS>
+void variadicNonnull(ARGS... args) __attribute__((nonnull));
+
+void f3(int a, float b, int *p) {
+  if (p)
+    return;
+  variadicNonnull(a, b, p); //expected-warning{{nonnull}}
+}
+
+int globalVar = 15;
+void moreParamsThanArgs [[gnu::nonnull(2, 4)]] (int a, int *p, int b = 42, int *q = &globalVar);
+
+void f4(int a, int *p) {
+  if (p)
+    return;
+  moreParamsThanArgs(a, p); //expected-warning{{nonnull}}
+}