[analyzer] Make NonNullParamChecker as dependency for StdCLibraryFunctionsChecker

[martong]

Summary

If a given parameter in a FunctionDecl has a nonull attribute then the NonNull
constraint in StdCLibraryFunctionsChecker has the same effect as
NonNullParamChecker. I think it is better to emit diagnostics from the simpler
checker. By making NonNullParamChecker as a dependency, in these cases it will
be the first to emit a diagnostic and to stop the analysis on that path.

[Szelethus]

I'm a bit unsure about the naming, because it's not technically true that NonNullParamChecker is a dependency of StdCLibraryFunctionsChecker. The actual relationship is different, because we only want to change the evaluation order, but then again, you can argue that this is in fact a dependency. So its like whatever, LGTM :^). Could you just drop a // NOTE: though about this?

[xazax.hun]

If a given parameter in a FunctionDecl has a nonull attribute then the NonNull constraint in StdCLibraryFunctionsChecker has the same effect as NonNullParamChecker.

Wait, where the diagnostic is coming from? My point is, the user should be able to turn apiModeling.StdCLibraryFunctions on, while suppressing all the null related diagnostics. Is this possible now? Is this possible after this change?
As long as it is possible to separately turn on modeling independently form the diagnostics, it looks good to me.

[martong]

In D79420#2022237, @xazax.hun wrote:

If a given parameter in a FunctionDecl has a nonull attribute then the NonNull constraint in StdCLibraryFunctionsChecker has the same effect as NonNullParamChecker.

Wait, where the diagnostic is coming from?

StdCLibraryFunctionArgs are responsible for emitting diags. This is a subchecker of StdCLibraryFunctions, and can be disabled separately.

My point is, the user should be able to turn apiModeling.StdCLibraryFunctions on, while suppressing all the null related diagnostics. Is this possible now? Is this possible after this change?

Yes, in both cases (before and after this change). By enabling StdCLibraryFunctions and disabling StdCLibraryFunctionArgs and also disabling NonNullParamChecker. (But the last one is in core, so the user should not do that.) This patch changes only the order evaluation order, from now on NonNullParamChecker's callbacks will be called earlier.

As long as it is possible to separately turn on modeling independently form the diagnostics, it looks good to me.

Yes, that is possible, so I consider this as an LGTM :)

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

 //===----------------------------------------------------------------------===//
 // APIModeling.
 //===----------------------------------------------------------------------===//
 
 let ParentPackage = APIModeling in {
 
 def StdCLibraryFunctionsChecker : Checker<"StdCLibraryFunctions">,
   HelpText<"Improve modeling of the C standard library functions">,
+  Dependencies<[NonNullParamChecker, CallAndMessageChecker]>,
   Documentation<NotDocumented>;
 
 def StdCLibraryFunctionArgsChecker : Checker<"StdCLibraryFunctionArgs">,
   HelpText<"Check constraints of arguments of C standard library functions, "
            "such as whether the parameter of isalpha is in the range [0, 255] "
            "or is EOF.">,
-  Dependencies<[StdCLibraryFunctionsChecker, CallAndMessageChecker]>,
+  Dependencies<[StdCLibraryFunctionsChecker]>,
   Documentation<NotDocumented>;
 
 def TrustNonnullChecker : Checker<"TrustNonnull">,
   HelpText<"Trust that returns from framework methods annotated with _Nonnull "
            "are not null">,
   Documentation<NotDocumented>;
 
 } // end "apiModeling"

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: apiModeling.StdCLibraryFunctions
+// CHECK-NEXT: core.NonNullParamChecker
 // CHECK-NEXT: core.CallAndMessage
+// CHECK-NEXT: apiModeling.StdCLibraryFunctions
 // CHECK-NEXT: apiModeling.StdCLibraryFunctionArgs
 // CHECK-NEXT: apiModeling.TrustNonnull
 // CHECK-NEXT: apiModeling.llvm.CastValue
 // CHECK-NEXT: apiModeling.llvm.ReturnValue
 // CHECK-NEXT: core.DivideZero
 // CHECK-NEXT: core.DynamicTypePropagation
-// CHECK-NEXT: core.NonNullParamChecker
 // CHECK-NEXT: core.NonnilStringConstants
 // CHECK-NEXT: core.NullDereference
 // CHECK-NEXT: core.StackAddrEscapeBase
 // CHECK-NEXT: core.StackAddressEscape
 // CHECK-NEXT: core.UndefinedBinaryOperatorResult
 // CHECK-NEXT: core.VLASize
 // CHECK-NEXT: core.builtin.BuiltinFunctions
 // CHECK-NEXT: core.builtin.NoReturnFunctions