[analyzer] Clean up bug types in CallAndMessageChecker

[NagyDonat]

In CallAndMessageChecker the initialization of bug types was highly obfuscated (even compared to other mutable std::unique_ptr hacks). This commit cleans up this situation and removes a totally superfluous hidded 'modeling' sub-checker that did not have any role apart from obstructing the normal initialization of bug types.

(Note that if we need to reintroduce CallAndMessageModeling in the future, we can do it cleanly within the CheckerFamily framework, so we wouldn't need to re-obfuscate the bug type initialization.)

This change is mostly non-functional, the only visible change is the removal of the hidden modeling checker.

[llvmbot]

@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang

Author: Donát Nagy (NagyDonat)

Changes

In CallAndMessageChecker the initialization of bug types was highly obfuscated (even compared to other mutable std::unique_ptr hacks). This commit cleans up this situation and removes a totally superfluous hidded 'modeling' sub-checker that did not have any role apart from obstructing the normal initialization of bug types.

(Note that if we need to reintroduce CallAndMessageModeling in the future, we can do it cleanly within the CheckerFamily framework, so we wouldn't need to re-obfuscate the bug type initialization.)

This change is mostly non-functional, the only visible change is the removal of the hidden modeling checker.

---

Patch is 22.64 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/156073.diff

4 Files Affected:

• (modified) clang/include/clang/StaticAnalyzer/Checkers/Checkers.td (+43-62)
• (modified) clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp (+59-117)
• (modified) clang/test/Analysis/analyzer-enabled-checkers.c (-1)
• (modified) clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c (-1)

diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 73f702de581d9..3849a02bfc7c5 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -143,68 +143,49 @@ def BitwiseShiftChecker : Checker<"BitwiseShift">,
   ]>,
   Documentation<HasDocumentation>;
 
-def CallAndMessageModeling : Checker<"CallAndMessageModeling">,
-  HelpText<"Responsible for essential modeling and assumptions after a "
-           "function/method call. For instance, if we can't reason about the "
-           "nullability of the implicit this parameter after a method call, "
-           "this checker conservatively assumes it to be non-null">,
-  Documentation<HasDocumentation>,
-  Hidden;
-
-def CallAndMessageChecker : Checker<"CallAndMessage">,
-  HelpText<"Check for logical errors for function calls and Objective-C "
-           "message expressions (e.g., uninitialized arguments, null function "
-           "pointers)">,
-  CheckerOptions<[
-    CmdLineOption<Boolean,
-                  "FunctionPointer",
-                  "Check whether a called function pointer is null or "
-                  "undefined",
-                  "true",
-                  Released>,
-    CmdLineOption<Boolean,
-                  "ParameterCount",
-                  "Check whether a function was called with the appropriate "
-                  "number of arguments",
-                  "true",
-                  Released>,
-    CmdLineOption<Boolean,
-                  "CXXThisMethodCall",
-                  "Check whether the implicit this parameter is null or "
-                  "undefined upon a method call",
-                  "true",
-                  Released>,
-    CmdLineOption<Boolean,
-                  "CXXDeallocationArg",
-                  "Check whether the argument of operator delete is undefined",
-                  "true",
-                  Released>,
-    CmdLineOption<Boolean,
-                  "ArgInitializedness",
-                  "Check whether any of the pass-by-value parameters is "
-                  "undefined",
-                  "true",
-                  Released>,
-    CmdLineOption<Boolean,
-                  "ArgPointeeInitializedness",
-                  "Check whether the pointee of a pass-by-reference or "
-                  "pass-by-pointer is undefined",
-                  "false",
-                  InAlpha>,
-    CmdLineOption<Boolean,
-                  "NilReceiver",
-                  "Check whether the reciever in the message expression is nil",
-                  "true",
-                  Released>,
-    CmdLineOption<Boolean,
-                  "UndefReceiver",
-                  "Check whether the reciever in the message expression is "
-                  "undefined",
-                  "true",
-                  Released>,
-  ]>,
-  Documentation<HasDocumentation>,
-  Dependencies<[CallAndMessageModeling]>;
+def CallAndMessageChecker
+    : Checker<"CallAndMessage">,
+      HelpText<
+          "Check for logical errors for function calls and Objective-C "
+          "message expressions (e.g., uninitialized arguments, null function "
+          "pointers)">,
+      CheckerOptions<
+          [CmdLineOption<Boolean, "FunctionPointer",
+                         "Check whether a called function pointer is null or "
+                         "undefined",
+                         "true", Released>,
+           CmdLineOption<
+               Boolean, "ParameterCount",
+               "Check whether a function was called with the appropriate "
+               "number of arguments",
+               "true", Released>,
+           CmdLineOption<Boolean, "CXXThisMethodCall",
+                         "Check whether the implicit this parameter is null or "
+                         "undefined upon a method call",
+                         "true", Released>,
+           CmdLineOption<
+               Boolean, "CXXDeallocationArg",
+               "Check whether the argument of operator delete is undefined",
+               "true", Released>,
+           CmdLineOption<Boolean, "ArgInitializedness",
+                         "Check whether any of the pass-by-value parameters is "
+                         "undefined",
+                         "true", Released>,
+           CmdLineOption<Boolean, "ArgPointeeInitializedness",
+                         "Check whether the pointee of a pass-by-reference or "
+                         "pass-by-pointer is undefined",
+                         "false", InAlpha>,
+           CmdLineOption<
+               Boolean, "NilReceiver",
+               "Check whether the reciever in the message expression is nil",
+               "true", Released>,
+           CmdLineOption<
+               Boolean, "UndefReceiver",
+               "Check whether the reciever in the message expression is "
+               "undefined",
+               "true", Released>,
+]>,
+      Documentation<HasDocumentation>;
 
 def FixedAddressDereferenceChecker
     : Checker<"FixedAddressDereference">,
diff --git a/clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
index e98710aadacf2..6d39dc862c97c 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
@@ -31,34 +31,38 @@ namespace {
 class CallAndMessageChecker
     : public Checker<check::PreObjCMessage, check::ObjCMessageNil,
                      check::PreCall> {
-  mutable std::unique_ptr<BugType> BT_call_null;
-  mutable std::unique_ptr<BugType> BT_call_undef;
-  mutable std::unique_ptr<BugType> BT_cxx_call_null;
-  mutable std::unique_ptr<BugType> BT_cxx_call_undef;
-  mutable std::unique_ptr<BugType> BT_call_arg;
-  mutable std::unique_ptr<BugType> BT_cxx_delete_undef;
-  mutable std::unique_ptr<BugType> BT_msg_undef;
-  mutable std::unique_ptr<BugType> BT_objc_prop_undef;
-  mutable std::unique_ptr<BugType> BT_objc_subscript_undef;
-  mutable std::unique_ptr<BugType> BT_msg_arg;
-  mutable std::unique_ptr<BugType> BT_msg_ret;
-  mutable std::unique_ptr<BugType> BT_call_few_args;
+  const BugType BT_call_null{
+      this, "Called function pointer is null (null dereference)"};
+  const BugType BT_call_undef{
+      this, "Called function pointer is an uninitialized pointer value"};
+  const BugType BT_cxx_call_null{this, "Called C++ object pointer is null"};
+  const BugType BT_cxx_call_undef{this,
+                                  "Called C++ object pointer is uninitialized"};
+  const BugType BT_call_arg{this, "Uninitialized argument value"};
+  const BugType BT_cxx_delete_undef{this, "Uninitialized argument value"};
+  const BugType BT_msg_undef{
+      this, "Receiver in message expression is an uninitialized value"};
+  const BugType BT_objc_prop_undef{
+      this, "Property access on an uninitialized object pointer"};
+  const BugType BT_objc_subscript_undef{
+      this, "Subscript access on an uninitialized object pointer"};
+  const BugType BT_msg_arg{this, "Uninitialized argument value"};
+  const BugType BT_msg_ret{this, "Receiver in message expression is 'nil'"};
+  const BugType BT_call_few_args{this, "Function call with too few arguments"};
 
 public:
-  // These correspond with the checker options. Looking at other checkers such
-  // as MallocChecker and CStringChecker, this is similar as to how they pull
-  // off having a modeling class, but emitting diagnostics under a smaller
-  // checker's name that can be safely disabled without disturbing the
-  // underlaying modeling engine.
-  // The reason behind having *checker options* rather then actual *checkers*
-  // here is that CallAndMessage is among the oldest checkers out there, and can
+  // Like a checker family, CallAndMessageChecker can produce many kinds of
+  // warnings which can be separately enabled or disabled. However, for
+  // historical reasons these warning kinds are represented by checker options
+  // (and not separate checkcer frontends with their own names) because
+  // CallAndMessage is among the oldest checkers out there, and can
   // be responsible for the majority of the reports on any given project. This
   // is obviously not ideal, but changing checker name has the consequence of
   // changing the issue hashes associated with the reports, and databases
   // relying on this (CodeChecker, for instance) would suffer greatly.
   // If we ever end up making changes to the issue hash generation algorithm, or
   // the warning messages here, we should totally jump on the opportunity to
-  // convert these to actual checkers.
+  // convert these to actual checker frontends.
   enum CheckKind {
     CK_FunctionPointer,
     CK_ParameterCount,
@@ -108,10 +112,11 @@ class CallAndMessageChecker
   bool PreVisitProcessArg(CheckerContext &C, SVal V, SourceRange ArgRange,
                           const Expr *ArgEx, int ArgumentNumber,
                           bool CheckUninitFields, const CallEvent &Call,
-                          std::unique_ptr<BugType> &BT,
+                          const BugType &BT,
                           const ParmVarDecl *ParamDecl) const;
 
-  static void emitBadCall(BugType *BT, CheckerContext &C, const Expr *BadE);
+  static void emitBadCall(const BugType *BT, CheckerContext &C,
+                          const Expr *BadE);
   void emitNilReceiverBug(CheckerContext &C, const ObjCMethodCall &msg,
                           ExplodedNode *N) const;
 
@@ -119,18 +124,14 @@ class CallAndMessageChecker
                          ProgramStateRef state,
                          const ObjCMethodCall &msg) const;
 
-  void LazyInit_BT(const char *desc, std::unique_ptr<BugType> &BT) const {
-    if (!BT)
-      BT.reset(new BugType(OriginalName, desc));
-  }
   bool uninitRefOrPointer(CheckerContext &C, SVal V, SourceRange ArgRange,
-                          const Expr *ArgEx, std::unique_ptr<BugType> &BT,
-                          const ParmVarDecl *ParamDecl, const char *BD,
+                          const Expr *ArgEx, const BugType &BT,
+                          const ParmVarDecl *ParamDecl,
                           int ArgumentNumber) const;
 };
 } // end anonymous namespace
 
-void CallAndMessageChecker::emitBadCall(BugType *BT, CheckerContext &C,
+void CallAndMessageChecker::emitBadCall(const BugType *BT, CheckerContext &C,
                                         const Expr *BadE) {
   ExplodedNode *N = C.generateErrorNode();
   if (!N)
@@ -183,8 +184,7 @@ static void describeUninitializedArgumentInCall(const CallEvent &Call,
 
 bool CallAndMessageChecker::uninitRefOrPointer(
     CheckerContext &C, SVal V, SourceRange ArgRange, const Expr *ArgEx,
-    std::unique_ptr<BugType> &BT, const ParmVarDecl *ParamDecl, const char *BD,
-    int ArgumentNumber) const {
+    const BugType &BT, const ParmVarDecl *ParamDecl, int ArgumentNumber) const {
 
   // The pointee being uninitialized is a sign of code smell, not a bug, no need
   // to sink here.
@@ -218,8 +218,7 @@ bool CallAndMessageChecker::uninitRefOrPointer(
     const SVal PSV = State->getSVal(SValMemRegion, C.getASTContext().CharTy);
     if (PSV.isUndef()) {
       if (ExplodedNode *N = C.generateErrorNode()) {
-        LazyInit_BT(BD, BT);
-        auto R = std::make_unique<PathSensitiveBugReport>(*BT, Os.str(), N);
+        auto R = std::make_unique<PathSensitiveBugReport>(BT, Os.str(), N);
         R->addRange(ArgRange);
         if (ArgEx)
           bugreporter::trackExpressionValue(N, ArgEx, *R);
@@ -275,20 +274,11 @@ class FindUninitializedField {
 };
 } // namespace
 
-bool CallAndMessageChecker::PreVisitProcessArg(CheckerContext &C,
-                                               SVal V,
-                                               SourceRange ArgRange,
-                                               const Expr *ArgEx,
-                                               int ArgumentNumber,
-                                               bool CheckUninitFields,
-                                               const CallEvent &Call,
-                                               std::unique_ptr<BugType> &BT,
-                                               const ParmVarDecl *ParamDecl
-                                               ) const {
-  const char *BD = "Uninitialized argument value";
-
-  if (uninitRefOrPointer(C, V, ArgRange, ArgEx, BT, ParamDecl, BD,
-                         ArgumentNumber))
+bool CallAndMessageChecker::PreVisitProcessArg(
+    CheckerContext &C, SVal V, SourceRange ArgRange, const Expr *ArgEx,
+    int ArgumentNumber, bool CheckUninitFields, const CallEvent &Call,
+    const BugType &BT, const ParmVarDecl *ParamDecl) const {
+  if (uninitRefOrPointer(C, V, ArgRange, ArgEx, BT, ParamDecl, ArgumentNumber))
     return true;
 
   if (V.isUndef()) {
@@ -297,12 +287,11 @@ bool CallAndMessageChecker::PreVisitProcessArg(CheckerContext &C,
       return true;
     }
     if (ExplodedNode *N = C.generateErrorNode()) {
-      LazyInit_BT(BD, BT);
       // Generate a report for this bug.
       SmallString<200> Buf;
       llvm::raw_svector_ostream Os(Buf);
       describeUninitializedArgumentInCall(Call, ArgumentNumber, Os);
-      auto R = std::make_unique<PathSensitiveBugReport>(*BT, Os.str(), N);
+      auto R = std::make_unique<PathSensitiveBugReport>(BT, Os.str(), N);
 
       R->addRange(ArgRange);
       if (ArgEx)
@@ -327,7 +316,6 @@ bool CallAndMessageChecker::PreVisitProcessArg(CheckerContext &C,
         return true;
       }
       if (ExplodedNode *N = C.generateErrorNode()) {
-        LazyInit_BT(BD, BT);
         SmallString<512> Str;
         llvm::raw_svector_ostream os(Str);
         os << "Passed-by-value struct argument contains uninitialized data";
@@ -349,7 +337,7 @@ bool CallAndMessageChecker::PreVisitProcessArg(CheckerContext &C,
         }
 
         // Generate a report for this bug.
-        auto R = std::make_unique<PathSensitiveBugReport>(*BT, os.str(), N);
+        auto R = std::make_unique<PathSensitiveBugReport>(BT, os.str(), N);
         R->addRange(ArgRange);
 
         if (ArgEx)
@@ -377,11 +365,7 @@ ProgramStateRef CallAndMessageChecker::checkFunctionPointerCall(
       C.addSink(State);
       return nullptr;
     }
-    if (!BT_call_undef)
-      BT_call_undef.reset(new BugType(
-          OriginalName,
-          "Called function pointer is an uninitialized pointer value"));
-    emitBadCall(BT_call_undef.get(), C, Callee);
+    emitBadCall(&BT_call_undef, C, Callee);
     return nullptr;
   }
 
@@ -393,10 +377,7 @@ ProgramStateRef CallAndMessageChecker::checkFunctionPointerCall(
       C.addSink(StNull);
       return nullptr;
     }
-    if (!BT_call_null)
-      BT_call_null.reset(new BugType(
-          OriginalName, "Called function pointer is null (null dereference)"));
-    emitBadCall(BT_call_null.get(), C, Callee);
+    emitBadCall(&BT_call_null, C, Callee);
     return nullptr;
   }
 
@@ -421,8 +402,6 @@ ProgramStateRef CallAndMessageChecker::checkParameterCount(
   if (!N)
     return nullptr;
 
-  LazyInit_BT("Function call with too few arguments", BT_call_few_args);
-
   SmallString<512> Str;
   llvm::raw_svector_ostream os(Str);
   if (isa<AnyFunctionCall>(Call)) {
@@ -435,7 +414,7 @@ ProgramStateRef CallAndMessageChecker::checkParameterCount(
      << " is called with fewer (" << Call.getNumArgs() << ")";
 
   C.emitReport(
-      std::make_unique<PathSensitiveBugReport>(*BT_call_few_args, os.str(), N));
+      std::make_unique<PathSensitiveBugReport>(BT_call_few_args, os.str(), N));
   return nullptr;
 }
 
@@ -448,10 +427,7 @@ ProgramStateRef CallAndMessageChecker::checkCXXMethodCall(
       C.addSink(State);
       return nullptr;
     }
-    if (!BT_cxx_call_undef)
-      BT_cxx_call_undef.reset(new BugType(
-          OriginalName, "Called C++ object pointer is uninitialized"));
-    emitBadCall(BT_cxx_call_undef.get(), C, CC->getCXXThisExpr());
+    emitBadCall(&BT_cxx_call_undef, C, CC->getCXXThisExpr());
     return nullptr;
   }
 
@@ -463,10 +439,7 @@ ProgramStateRef CallAndMessageChecker::checkCXXMethodCall(
       C.addSink(StNull);
       return nullptr;
     }
-    if (!BT_cxx_call_null)
-      BT_cxx_call_null.reset(
-          new BugType(OriginalName, "Called C++ object pointer is null"));
-    emitBadCall(BT_cxx_call_null.get(), C, CC->getCXXThisExpr());
+    emitBadCall(&BT_cxx_call_null, C, CC->getCXXThisExpr());
     return nullptr;
   }
 
@@ -492,15 +465,12 @@ CallAndMessageChecker::checkCXXDeallocation(const CXXDeallocatorCall *DC,
   ExplodedNode *N = C.generateErrorNode();
   if (!N)
     return nullptr;
-  if (!BT_cxx_delete_undef)
-    BT_cxx_delete_undef.reset(
-        new BugType(OriginalName, "Uninitialized argument value"));
   if (DE->isArrayFormAsWritten())
     Desc = "Argument to 'delete[]' is uninitialized";
   else
     Desc = "Argument to 'delete' is uninitialized";
   auto R =
-      std::make_unique<PathSensitiveBugReport>(*BT_cxx_delete_undef, Desc, N);
+      std::make_unique<PathSensitiveBugReport>(BT_cxx_delete_undef, Desc, N);
   bugreporter::trackExpressionValue(N, DE, *R);
   C.emitReport(std::move(R));
   return nullptr;
@@ -518,11 +488,7 @@ ProgramStateRef CallAndMessageChecker::checkArgInitializedness(
   const bool checkUninitFields =
       !(C.getAnalysisManager().shouldInlineCall() && (D && D->getBody()));
 
-  std::unique_ptr<BugType> *BT;
-  if (isa<ObjCMethodCall>(Call))
-    BT = &BT_msg_arg;
-  else
-    BT = &BT_call_arg;
+  const BugType &BT = isa<ObjCMethodCall>(Call) ? BT_msg_arg : BT_call_arg;
 
   const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(D);
   for (unsigned i = 0, e = Call.getNumArgs(); i != e; ++i) {
@@ -530,7 +496,7 @@ ProgramStateRef CallAndMessageChecker::checkArgInitializedness(
     if (FD && i < FD->getNumParams())
       ParamDecl = FD->getParamDecl(i);
     if (PreVisitProcessArg(C, Call.getArgSVal(i), Call.getArgSourceRange(i),
-                           Call.getArgExpr(i), i, checkUninitFields, Call, *BT,
+                           Call.getArgExpr(i), i, checkUninitFields, Call, BT,
                            ParamDecl))
       return nullptr;
   }
@@ -580,28 +546,16 @@ void CallAndMessageChecker::checkPreObjCMessage(const ObjCMethodCall &msg,
       return;
     }
     if (ExplodedNode *N = C.generateErrorNode()) {
-      BugType *BT = nullptr;
+      const BugType *BT = nullptr;
       switch (msg.getMessageKind()) {
       case OCM_Message:
-        if (!BT_msg_undef)
-          BT_msg_undef.reset(new BugType(OriginalName,
-                                         "Receiver in message expression "
-                                         "is an uninitialized value"));
-        BT = BT_msg_undef.get();
+        BT = &BT_msg_undef;
         break;
       case OCM_PropertyAccess:
-        if (!BT_objc_prop_undef)
-          BT_objc_prop_undef.reset(new BugType(
-              OriginalName,
-              "Property access on an uninitialized object pointer"));
-        BT = BT_objc_prop_undef.get();
+        BT = &BT_objc_prop_undef;
         break;
       case OCM_Subscript:
-        if (!BT_objc_subscript_undef)
-          BT_objc_subscript_undef.reset(new BugType(
-              OriginalName,
-              "Subscript access on an uninitialized object pointer"));
-        BT = BT_objc_subscript_undef.get();
+        BT = &BT_objc_subscript_undef;
         break;
       }
       assert(BT && "Unknown message kind.");
@@ -632,10 +586,6 @@ void CallAndMessageChecker::emitNilReceiverBug(CheckerContext &C,
     return;
   }
 
-  if (!BT_msg_ret)
-    BT_msg_ret.reset(
-        new BugType(OriginalName, "Receiver in message expression is 'nil'"));
-
   const ObjCMessageExpr *ME = msg.getOriginExpr();
 
   QualType ResTy = msg.getResultType();
@@ -654,7 +604,7 @@ void CallAndMessageChecker::emitNilReceiverBug(CheckerContext &C,
   }
 
   auto report =
-      std::make_unique<PathSensitiveBugReport>(*BT_msg_ret, os.str(), N);
+      std::make_unique<PathSensitiveBugReport>(BT_msg_ret, os.str(), N);
   report->addRange(ME->getReceiverRange());
   // FIXME: This won't track "self" in messages to super.
   if (const Expr *receiver = ME->getInstanceReceiver()) {
@@ -728,23 +678,15 @@ void CallAndMessageChecker::HandleNilReceiver(CheckerContext &C,
   C.addTransition(state);...
[truncated]

[NagyDonat]

This block is just reformatted by git-clang-format. It really doesn't like the customary layout of Checkers.td... 🙄