[clang][analyzer] Merge apiModeling.StdCLibraryFunctions and StdCLibraryFunctionArgs checkers into one.

Main reason for this change is that these checkers were implemented in the same class
but had different dependency ordering. (NonNullParamChecker should run before StdCLibraryFunctionArgs
to get more special warning about null arguments, but the apiModeling.StdCLibraryFunctions was a modeling
checker that should run before other non-modeling checkers. The modeling checker changes state in a way
that makes it impossible to detect a null argument by NonNullParamChecker.)
To make it more simple, the modeling part is removed as separate checker and can be only used if
checker StdCLibraryFunctions is turned on, that produces the warnings too. Modeling the functions
without bug detection (for invalid argument) is not possible. The modeling of standard functions
does not happen by default from this change on.

Reviewed By: Szelethus

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

Author: balazske

clang/docs/analyzer/checkers.rst

@@ -2421,10 +2421,10 @@ For a more detailed description of configuration options, please see the :doc:`u
 alpha.unix
 ^^^^^^^^^^^
 
-.. _alpha-unix-StdCLibraryFunctionArgs:
+.. _alpha-unix-StdCLibraryFunctions:
 
-alpha.unix.StdCLibraryFunctionArgs (C)
-""""""""""""""""""""""""""""""""""""""
+alpha.unix.StdCLibraryFunctions (C)
+"""""""""""""""""""""""""""""""""""
 Check for calls of standard library functions that violate predefined argument
 constraints. For example, it is stated in the C standard that for the ``int
 isalnum(int ch)`` function the behavior is undefined if the value of ``ch`` is
@@ -2457,6 +2457,12 @@ on standard library functions. Preconditions are checked, and when they are
 violated, a warning is emitted. Post conditions are added to the analysis, e.g.
 that the return value must be no greater than 255.
 
+For example if an argument to a function must be in between 0 and 255, but the
+value of the argument is unknown, the analyzer will conservatively assume that
+it is in this interval. Similarly, if a function mustn't be called with a null
+pointer and the null value of the argument can not be proven, the analyzer will
+assume that it is non-null.
+
 These are the possible checks on the values passed as function arguments:
  - The argument has an allowed range (or multiple ranges) of values. The checker
    can detect if a passed value is outside of the allowed range and show the
@@ -2471,16 +2477,6 @@ These are the possible checks on the values passed as function arguments:
    checker can detect if the buffer size is too small and in optimal case show
    the size of the buffer and the values of the corresponding arguments.
 
-If the user disables the checker then the argument violation warning is
-suppressed. However, the assumption about the argument is still modeled.
-For instance, if the argument to a function must be in between 0 and 255,
-but the value of the argument is unknown, the analyzer will conservatively
-assume that it is in this interval, even if warnings for this checker are
-disabled. Similarly, if a function mustn't be called with a null pointer but it
-is, analysis will stop on that execution path (similarly to a division by zero),
-with or without a warning. If the null value of the argument can not be proven,
-the analyzer will assume that it is non-null.
-
 .. code-block:: c
 
   int test_alnum_symbolic(int x) {
@@ -2493,6 +2489,13 @@ the analyzer will assume that it is non-null.
     return ret;
   }
 
+Additionally to the argument and return value conditions, this checker also adds
+state of the value ``errno`` if applicable to the analysis. Many system
+functions set the ``errno`` value only if an error occurs (together with a
+specific return value of the function), otherwise it becomes undefined. This
+checker changes the analysis state to contain such information. This data is
+used by other checkers, for example :ref:`alpha-unix-Errno`.
+
 **Limitations**
 
 The checker can not always provide notes about the values of the arguments.
@@ -2508,12 +2511,9 @@ range of the argument.
 **Parameters**
 
 The checker models functions (and emits diagnostics) from the C standard by
-default. The ``apiModeling.StdCLibraryFunctions:ModelPOSIX`` option enables
-modeling (and emit diagnostics) of additional functions that are defined in the
-POSIX standard. This option is disabled by default. Note that this option
-belongs to a separate built-in checker ``apiModeling.StdCLibraryFunctions`` and
-can have effect on other checkers because it toggles modeling of the functions
-in various aspects.
+default. The ``ModelPOSIX`` option enables modeling (and emit diagnostics) of
+additional functions that are defined in the POSIX standard. This option is
+disabled by default.
 
 .. _alpha-unix-BlockInCriticalSection:
 
@@ -2582,9 +2582,10 @@ pages of the functions and in the `POSIX standard <https://pubs.opengroup.org/on
    return 1;
  }
 
-The supported functions are the same that are modeled by checker
-:ref:`alpha-unix-StdCLibraryFunctionArgs`.
-The ``ModelPOSIX`` option of that checker affects the set of checked functions.
+The checker :ref:`alpha-unix-StdCLibraryFunctions` must be turned on to get the
+warnings from this checker. The supported functions are the same as by
+:ref:`alpha-unix-StdCLibraryFunctions`. The ``ModelPOSIX`` option of that
+checker affects the set of checked functions.
 
 **Parameters**
 

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

@@ -359,29 +359,6 @@ def ErrnoModeling : Checker<"Errno">,
   HelpText<"Make the special value 'errno' available to other checkers.">,
   Documentation<NotDocumented>;
 
-def StdCLibraryFunctionsChecker : Checker<"StdCLibraryFunctions">,
-  HelpText<"Improve modeling of the C standard library functions">,
-  // Uninitialized value check is a mandatory dependency. This Checker asserts
-  // that arguments are always initialized.
-  Dependencies<[CallAndMessageModeling]>,
-  CheckerOptions<[
-    CmdLineOption<Boolean,
-                  "DisplayLoadedSummaries",
-                  "If set to true, the checker displays the found summaries "
-                  "for the given translation unit.",
-                  "false",
-                  Released,
-                  Hide>,
-    CmdLineOption<Boolean,
-                  "ModelPOSIX",
-                  "If set to true, the checker models functions from the "
-                  "POSIX standard.",
-                  "false",
-                  InAlpha>
-  ]>,
-  Documentation<NotDocumented>,
-  Hidden;
-
 def TrustNonnullChecker : Checker<"TrustNonnull">,
   HelpText<"Trust that returns from framework methods annotated with _Nonnull "
            "are not null">,
@@ -583,11 +560,24 @@ def BlockInCriticalSectionChecker : Checker<"BlockInCriticalSection">,
   HelpText<"Check for calls to blocking functions inside a critical section">,
   Documentation<HasDocumentation>;
 
-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]>,
+def StdCLibraryFunctionsChecker : Checker<"StdCLibraryFunctions">,
+  HelpText<"Check for invalid arguments of C standard library functions, "
+           "and apply relations between arguments and return value">,
+  CheckerOptions<[
+    CmdLineOption<Boolean,
+                  "DisplayLoadedSummaries",
+                  "If set to true, the checker displays the found summaries "
+                  "for the given translation unit.",
+                  "false",
+                  Released,
+                  Hide>,
+    CmdLineOption<Boolean,
+                  "ModelPOSIX",
+                  "If set to true, the checker models additional functions "
+                  "from the POSIX standard.",
+                  "false",
+                  InAlpha>
+  ]>,
   WeakDependencies<[CallAndMessageChecker, NonNullParamChecker]>,
   Documentation<HasDocumentation>;
 
@@ -1618,7 +1608,6 @@ def DebugIteratorModeling : Checker<"DebugIteratorModeling">,
 def StdCLibraryFunctionsTesterChecker : Checker<"StdCLibraryFunctionsTester">,
   HelpText<"Add test functions to the summary map, so testing of individual "
            "summary constituents becomes possible.">,
-  Dependencies<[StdCLibraryFunctionsChecker]>,
   Documentation<NotDocumented>;
 
 } // end "debug"

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

@@ -790,13 +790,8 @@ class StdLibraryFunctionsChecker
   void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
   bool evalCall(const CallEvent &Call, CheckerContext &C) const;
 
-  enum CheckKind {
-    CK_StdCLibraryFunctionArgsChecker,
-    CK_StdCLibraryFunctionsTesterChecker,
-    CK_NumCheckKinds
-  };
-  bool ChecksEnabled[CK_NumCheckKinds] = {false};
-  CheckerNameRef CheckNames[CK_NumCheckKinds];
+  CheckerNameRef CheckName;
+  bool AddTestFunctions = false;
 
   bool DisplayLoadedSummaries = false;
   bool ModelPOSIX = false;
@@ -813,8 +808,6 @@ class StdLibraryFunctionsChecker
   void reportBug(const CallEvent &Call, ExplodedNode *N,
                  const ValueConstraint *VC, const ValueConstraint *NegatedVC,
                  const Summary &Summary, CheckerContext &C) const {
-    if (!ChecksEnabled[CK_StdCLibraryFunctionArgsChecker])
-      return;
     assert(Call.getDecl() &&
            "Function found in summary must have a declaration available");
     SmallString<256> Msg;
@@ -834,8 +827,8 @@ class StdLibraryFunctionsChecker
     Msg[0] = toupper(Msg[0]);
     if (!BT_InvalidArg)
       BT_InvalidArg = std::make_unique<BugType>(
-          CheckNames[CK_StdCLibraryFunctionArgsChecker],
-          "Function call with invalid argument", categories::LogicError);
+          CheckName, "Function call with invalid argument",
+          categories::LogicError);
     auto R = std::make_unique<PathSensitiveBugReport>(*BT_InvalidArg, Msg, N);
 
     for (ArgNo ArgN : VC->getArgsToTrack()) {
@@ -1423,6 +1416,7 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
     CheckerContext &C) const {
   if (SummariesInitialized)
     return;
+  SummariesInitialized = true;
 
   SValBuilder &SVB = C.getSValBuilder();
   BasicValueFactory &BVF = SVB.getBasicValueFactory();
@@ -3370,7 +3364,7 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
   }
 
   // Functions for testing.
-  if (ChecksEnabled[CK_StdCLibraryFunctionsTesterChecker]) {
+  if (AddTestFunctions) {
     const RangeInt IntMin = BVF.getMinValue(IntTy).getLimitedValue();
 
     addToFunctionSummaryMap(
@@ -3594,12 +3588,11 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
                    ReturnValueCondition(WithinRange, SingleValue(4))},
                   ErrnoIrrelevant));
   }
-
-  SummariesInitialized = true;
 }
 
 void ento::registerStdCLibraryFunctionsChecker(CheckerManager &mgr) {
   auto *Checker = mgr.registerChecker<StdLibraryFunctionsChecker>();
+  Checker->CheckName = mgr.getCurrentCheckerName();
   const AnalyzerOptions &Opts = mgr.getAnalyzerOptions();
   Checker->DisplayLoadedSummaries =
       Opts.getCheckerBooleanOption(Checker, "DisplayLoadedSummaries");
@@ -3613,16 +3606,12 @@ bool ento::shouldRegisterStdCLibraryFunctionsChecker(
   return true;
 }
 
-#define REGISTER_CHECKER(name)                                                 \
-  void ento::register##name(CheckerManager &mgr) {                             \
-    StdLibraryFunctionsChecker *checker =                                      \
-        mgr.getChecker<StdLibraryFunctionsChecker>();                          \
-    checker->ChecksEnabled[StdLibraryFunctionsChecker::CK_##name] = true;      \
-    checker->CheckNames[StdLibraryFunctionsChecker::CK_##name] =               \
-        mgr.getCurrentCheckerName();                                           \
-  }                                                                            \
-                                                                               \
-  bool ento::shouldRegister##name(const CheckerManager &mgr) { return true; }
-
-REGISTER_CHECKER(StdCLibraryFunctionArgsChecker)
-REGISTER_CHECKER(StdCLibraryFunctionsTesterChecker)
+void ento::registerStdCLibraryFunctionsTesterChecker(CheckerManager &mgr) {
+  auto *Checker = mgr.getChecker<StdLibraryFunctionsChecker>();
+  Checker->AddTestFunctions = true;
+}
+
+bool ento::shouldRegisterStdCLibraryFunctionsTesterChecker(
+    const CheckerManager &mgr) {
+  return true;
+}

clang/test/Analysis/PR49642.c

@@ -1,6 +1,6 @@
 // RUN: %clang_analyze_cc1 -Wno-implicit-function-declaration -Wno-implicit-int -w -verify %s \
 // RUN:   -analyzer-checker=core \
-// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions
+// RUN:   -analyzer-checker=alpha.unix.StdCLibraryFunctions
 
 // expected-no-diagnostics
 

clang/test/Analysis/analyzer-config.c

@@ -13,8 +13,8 @@
 // CHECK-NEXT: alpha.security.MmapWriteExec:MmapProtRead = 0x01
 // CHECK-NEXT: alpha.security.taint.TaintPropagation:Config = ""
 // CHECK-NEXT: alpha.unix.Errno:AllowErrnoReadOutsideConditionExpressions = true
-// CHECK-NEXT: apiModeling.StdCLibraryFunctions:DisplayLoadedSummaries = false
-// CHECK-NEXT: apiModeling.StdCLibraryFunctions:ModelPOSIX = false
+// CHECK-NEXT: alpha.unix.StdCLibraryFunctions:DisplayLoadedSummaries = false
+// CHECK-NEXT: alpha.unix.StdCLibraryFunctions:ModelPOSIX = false
 // CHECK-NEXT: apply-fixits = false
 // CHECK-NEXT: assume-controlled-environment = false
 // CHECK-NEXT: avoid-suppressing-null-argument-paths = false

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

@@ -7,12 +7,11 @@
 // CHECK:      OVERVIEW: Clang Static Analyzer Enabled Checkers List
 // CHECK-EMPTY:
 // CHECK-NEXT: apiModeling.Errno
-// 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.CallAndMessageModeling
 // CHECK-NEXT: core.CallAndMessage
 // CHECK-NEXT: core.DivideZero
 // CHECK-NEXT: core.DynamicTypePropagation

clang/test/Analysis/conversion.c

@@ -1,6 +1,6 @@
 // RUN: %clang_analyze_cc1 %s \
 // RUN:   -Wno-conversion -Wno-tautological-constant-compare \
-// RUN:   -analyzer-checker=core,apiModeling,alpha.core.Conversion \
+// RUN:   -analyzer-checker=core,apiModeling,alpha.unix.StdCLibraryFunctions,alpha.core.Conversion \
 // RUN:   -verify
 
 unsigned char U8;
@@ -187,7 +187,7 @@ char dontwarn10(long long x) {
 }
 
 
-// C library functions, handled via apiModeling.StdCLibraryFunctions
+// C library functions, handled via alpha.unix.StdCLibraryFunctions
 
 int isascii(int c);
 void libraryFunction1(void) {

clang/test/Analysis/errno-stdlibraryfunctions-notes.c

@@ -1,10 +1,10 @@
 // RUN: %clang_analyze_cc1 -verify -analyzer-output text %s \
 // RUN:   -analyzer-checker=core \
 // RUN:   -analyzer-checker=debug.ExprInspection \
-// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=alpha.unix.StdCLibraryFunctions \
 // RUN:   -analyzer-checker=apiModeling.Errno \
 // RUN:   -analyzer-checker=alpha.unix.Errno \
-// RUN:   -analyzer-config apiModeling.StdCLibraryFunctions:ModelPOSIX=true
+// RUN:   -analyzer-config alpha.unix.StdCLibraryFunctions:ModelPOSIX=true
 
 #include "Inputs/errno_var.h"
 

clang/test/Analysis/errno-stdlibraryfunctions.c

@@ -1,10 +1,10 @@
 // RUN: %clang_analyze_cc1 -verify %s \
 // RUN:   -analyzer-checker=core \
 // RUN:   -analyzer-checker=debug.ExprInspection \
-// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=alpha.unix.StdCLibraryFunctions \
 // RUN:   -analyzer-checker=apiModeling.Errno \
 // RUN:   -analyzer-checker=alpha.unix.Errno \
-// RUN:   -analyzer-config apiModeling.StdCLibraryFunctions:ModelPOSIX=true
+// RUN:   -analyzer-config alpha.unix.StdCLibraryFunctions:ModelPOSIX=true
 
 #include "Inputs/errno_var.h"
 

clang/test/Analysis/std-c-library-functions-POSIX-lookup.c

@@ -1,8 +1,8 @@
 // RUN: %clang_analyze_cc1 %s \
 // RUN:   -analyzer-checker=core \
-// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
-// RUN:   -analyzer-config apiModeling.StdCLibraryFunctions:ModelPOSIX=true \
-// RUN:   -analyzer-config apiModeling.StdCLibraryFunctions:DisplayLoadedSummaries=true \
+// RUN:   -analyzer-checker=alpha.unix.StdCLibraryFunctions \
+// RUN:   -analyzer-config alpha.unix.StdCLibraryFunctions:ModelPOSIX=true \
+// RUN:   -analyzer-config alpha.unix.StdCLibraryFunctions:DisplayLoadedSummaries=true \
 // RUN:   -analyzer-checker=debug.ExprInspection \
 // RUN:   -analyzer-config eagerly-assume=false \
 // RUN:   -triple i686-unknown-linux 2>&1 | FileCheck %s --allow-empty