[analyzer] Conversion to CheckerFamily: DereferenceChecker

[NagyDonat]

This commit converts the class DereferenceChecker to the checker family framework and simplifies some parts of the implementation.

This commit is almost NFC, the only technically "functional" change is that it removes the hidden modeling checker DereferenceModeling which was only relevant as an implementation detail of the old checker registration procedure.

[llvmbot]

@llvm/pr-subscribers-clang

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

Author: Donát Nagy (NagyDonat)

Changes

This commit converts the class DereferenceChecker to the checker family framework and simplifies some parts of the implementation.

This commit is almost NFC, the only technically "functional" change is that it removes the hidden modeling checker DereferenceModeling which was only relevant as an implementation detail of the old checker registration procedure.

---

Full diff: https://github.com/llvm/llvm-project/pull/150442.diff

4 Files Affected:

• (modified) clang/include/clang/StaticAnalyzer/Checkers/Checkers.td (+5-11)
• (modified) clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp (+74-112)
• (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 38584c98fcc91..6225977e980cc 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -206,21 +206,15 @@ def CallAndMessageChecker : Checker<"CallAndMessage">,
   Documentation<HasDocumentation>,
   Dependencies<[CallAndMessageModeling]>;
 
-def DereferenceModeling : Checker<"DereferenceModeling">,
-  HelpText<"General support for dereference related checkers">,
-  Documentation<NotDocumented>,
-  Hidden;
-
 def FixedAddressDereferenceChecker
     : Checker<"FixedAddressDereference">,
       HelpText<"Check for dereferences of fixed addresses">,
-      Documentation<HasDocumentation>,
-      Dependencies<[DereferenceModeling]>;
+      Documentation<HasDocumentation>;
 
-def NullDereferenceChecker : Checker<"NullDereference">,
-  HelpText<"Check for dereferences of null pointers">,
-  Documentation<HasDocumentation>,
-  Dependencies<[DereferenceModeling]>;
+def NullDereferenceChecker
+    : Checker<"NullDereference">,
+      HelpText<"Check for dereferences of null pointers">,
+      Documentation<HasDocumentation>;
 
 def NonNullParamChecker : Checker<"NonNullParamChecker">,
   HelpText<"Check for null pointers passed as arguments to a function whose "
diff --git a/clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
index d7eea7e0b0db7..d572c4aba70a7 100644
--- a/clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
@@ -25,18 +25,22 @@ using namespace clang;
 using namespace ento;
 
 namespace {
+
+class DerefBugType : public BugType {
+  StringRef ArrayMsg, FieldMsg;
+
+public:
+  DerefBugType(CheckerFrontend *FE, StringRef Desc, const char *RMsg,
+               const char *FMsg = nullptr)
+      : BugType(FE, Desc), ArrayMsg(RMsg), FieldMsg(FMsg ? FMsg : RMsg) {}
+  StringRef getArrayMsg() const { return ArrayMsg; }
+  StringRef getFieldMsg() const { return FieldMsg; }
+};
+
 class DereferenceChecker
-    : public Checker< check::Location,
-                      check::Bind,
-                      EventDispatcher<ImplicitNullDerefEvent> > {
-  enum DerefKind {
-    NullPointer,
-    UndefinedPointerValue,
-    AddressOfLabel,
-    FixedAddress,
-  };
-
-  void reportBug(DerefKind K, ProgramStateRef State, const Stmt *S,
+    : public CheckerFamily<check::Location, check::Bind,
+                           EventDispatcher<ImplicitNullDerefEvent>> {
+  void reportBug(const DerefBugType &BT, ProgramStateRef State, const Stmt *S,
                  CheckerContext &C) const;
 
   bool suppressReport(CheckerContext &C, const Expr *E) const;
@@ -52,13 +56,23 @@ class DereferenceChecker
                              const LocationContext *LCtx,
                              bool loadedFrom = false);
 
-  bool CheckNullDereference = false;
-  bool CheckFixedDereference = false;
-
-  std::unique_ptr<BugType> BT_Null;
-  std::unique_ptr<BugType> BT_Undef;
-  std::unique_ptr<BugType> BT_Label;
-  std::unique_ptr<BugType> BT_FixedAddress;
+  CheckerFrontend NullDerefChecker, FixedDerefChecker;
+  const DerefBugType NullBug{&NullDerefChecker, "Dereference of null pointer",
+                             "a null pointer dereference",
+                             "a dereference of a null pointer"};
+  const DerefBugType UndefBug{&NullDerefChecker,
+                              "Dereference of undefined pointer value",
+                              "an undefined pointer dereference",
+                              "a dereference of an undefined pointer value"};
+  const DerefBugType LabelBug{&NullDerefChecker,
+                              "Dereference of the address of a label",
+                              "an undefined pointer dereference",
+                              "a dereference of an address of a label"};
+  const DerefBugType FixedAddressBug{&FixedDerefChecker,
+                                     "Dereference of a fixed address",
+                                     "a dereference of a fixed address"};
+
+  StringRef getDebugTag() const override { return "DereferenceChecker"; }
 };
 } // end anonymous namespace
 
@@ -158,115 +172,85 @@ static bool isDeclRefExprToReference(const Expr *E) {
   return false;
 }
 
-void DereferenceChecker::reportBug(DerefKind K, ProgramStateRef State,
-                                   const Stmt *S, CheckerContext &C) const {
-  const BugType *BT = nullptr;
-  llvm::StringRef DerefStr1;
-  llvm::StringRef DerefStr2;
-  switch (K) {
-  case DerefKind::NullPointer:
-    if (!CheckNullDereference) {
-      C.addSink();
-      return;
-    }
-    BT = BT_Null.get();
-    DerefStr1 = " results in a null pointer dereference";
-    DerefStr2 = " results in a dereference of a null pointer";
-    break;
-  case DerefKind::UndefinedPointerValue:
-    if (!CheckNullDereference) {
-      C.addSink();
+void DereferenceChecker::reportBug(const DerefBugType &BT,
+                                   ProgramStateRef State, const Stmt *S,
+                                   CheckerContext &C) const {
+  if (&BT == &FixedAddressBug) {
+    if (!FixedDerefChecker.isEnabled())
       return;
-    }
-    BT = BT_Undef.get();
-    DerefStr1 = " results in an undefined pointer dereference";
-    DerefStr2 = " results in a dereference of an undefined pointer value";
-    break;
-  case DerefKind::AddressOfLabel:
-    if (!CheckNullDereference) {
+  } else {
+    if (!NullDerefChecker.isEnabled()) {
       C.addSink();
       return;
     }
-    BT = BT_Label.get();
-    DerefStr1 = " results in an undefined pointer dereference";
-    DerefStr2 = " results in a dereference of an address of a label";
-    break;
-  case DerefKind::FixedAddress:
-    // Deliberately don't add a sink node if check is disabled.
-    // This situation may be valid in special cases.
-    if (!CheckFixedDereference)
-      return;
-
-    BT = BT_FixedAddress.get();
-    DerefStr1 = " results in a dereference of a fixed address";
-    DerefStr2 = " results in a dereference of a fixed address";
-    break;
-  };
+  }
 
   // Generate an error node.
   ExplodedNode *N = C.generateErrorNode(State);
   if (!N)
     return;
 
-  SmallString<100> buf;
-  llvm::raw_svector_ostream os(buf);
+  SmallString<100> Buf;
+  llvm::raw_svector_ostream Out(Buf);
 
   SmallVector<SourceRange, 2> Ranges;
 
   switch (S->getStmtClass()) {
   case Stmt::ArraySubscriptExprClass: {
-    os << "Array access";
+    Out << "Array access";
     const ArraySubscriptExpr *AE = cast<ArraySubscriptExpr>(S);
-    AddDerefSource(os, Ranges, AE->getBase()->IgnoreParenCasts(),
-                   State.get(), N->getLocationContext());
-    os << DerefStr1;
+    AddDerefSource(Out, Ranges, AE->getBase()->IgnoreParenCasts(), State.get(),
+                   N->getLocationContext());
+    Out << " results in " << BT.getArrayMsg();
     break;
   }
   case Stmt::ArraySectionExprClass: {
-    os << "Array access";
+    Out << "Array access";
     const ArraySectionExpr *AE = cast<ArraySectionExpr>(S);
-    AddDerefSource(os, Ranges, AE->getBase()->IgnoreParenCasts(),
-                   State.get(), N->getLocationContext());
-    os << DerefStr1;
+    AddDerefSource(Out, Ranges, AE->getBase()->IgnoreParenCasts(), State.get(),
+                   N->getLocationContext());
+    Out << " results in " << BT.getArrayMsg();
     break;
   }
   case Stmt::UnaryOperatorClass: {
-    os << BT->getDescription();
+    Out << BT.getDescription();
     const UnaryOperator *U = cast<UnaryOperator>(S);
-    AddDerefSource(os, Ranges, U->getSubExpr()->IgnoreParens(),
-                   State.get(), N->getLocationContext(), true);
+    AddDerefSource(Out, Ranges, U->getSubExpr()->IgnoreParens(), State.get(),
+                   N->getLocationContext(), true);
     break;
   }
   case Stmt::MemberExprClass: {
     const MemberExpr *M = cast<MemberExpr>(S);
     if (M->isArrow() || isDeclRefExprToReference(M->getBase())) {
-      os << "Access to field '" << M->getMemberNameInfo() << "'" << DerefStr2;
-      AddDerefSource(os, Ranges, M->getBase()->IgnoreParenCasts(),
-                     State.get(), N->getLocationContext(), true);
+      Out << "Access to field '" << M->getMemberNameInfo() << "' results in "
+          << BT.getFieldMsg();
+      AddDerefSource(Out, Ranges, M->getBase()->IgnoreParenCasts(), State.get(),
+                     N->getLocationContext(), true);
     }
     break;
   }
   case Stmt::ObjCIvarRefExprClass: {
     const ObjCIvarRefExpr *IV = cast<ObjCIvarRefExpr>(S);
-    os << "Access to instance variable '" << *IV->getDecl() << "'" << DerefStr2;
-    AddDerefSource(os, Ranges, IV->getBase()->IgnoreParenCasts(),
-                   State.get(), N->getLocationContext(), true);
+    Out << "Access to instance variable '" << *IV->getDecl() << "' results in "
+        << BT.getFieldMsg();
+    AddDerefSource(Out, Ranges, IV->getBase()->IgnoreParenCasts(), State.get(),
+                   N->getLocationContext(), true);
     break;
   }
   default:
     break;
   }
 
-  auto report = std::make_unique<PathSensitiveBugReport>(
-      *BT, buf.empty() ? BT->getDescription() : buf.str(), N);
+  auto BR = std::make_unique<PathSensitiveBugReport>(
+      BT, Buf.empty() ? BT.getDescription() : Buf.str(), N);
 
-  bugreporter::trackExpressionValue(N, bugreporter::getDerefExpr(S), *report);
+  bugreporter::trackExpressionValue(N, bugreporter::getDerefExpr(S), *BR);
 
   for (SmallVectorImpl<SourceRange>::iterator
        I = Ranges.begin(), E = Ranges.end(); I!=E; ++I)
-    report->addRange(*I);
+    BR->addRange(*I);
 
-  C.emitReport(std::move(report));
+  C.emitReport(std::move(BR));
 }
 
 void DereferenceChecker::checkLocation(SVal l, bool isLoad, const Stmt* S,
@@ -275,7 +259,7 @@ void DereferenceChecker::checkLocation(SVal l, bool isLoad, const Stmt* S,
   if (l.isUndef()) {
     const Expr *DerefExpr = getDereferenceExpr(S);
     if (!suppressReport(C, DerefExpr))
-      reportBug(DerefKind::UndefinedPointerValue, C.getState(), DerefExpr, C);
+      reportBug(UndefBug, C.getState(), DerefExpr, C);
     return;
   }
 
@@ -296,7 +280,7 @@ void DereferenceChecker::checkLocation(SVal l, bool isLoad, const Stmt* S,
       // we call an "explicit" null dereference.
       const Expr *expr = getDereferenceExpr(S);
       if (!suppressReport(C, expr)) {
-        reportBug(DerefKind::NullPointer, nullState, expr, C);
+        reportBug(NullBug, nullState, expr, C);
         return;
       }
     }
@@ -314,7 +298,7 @@ void DereferenceChecker::checkLocation(SVal l, bool isLoad, const Stmt* S,
   if (location.isConstant()) {
     const Expr *DerefExpr = getDereferenceExpr(S, isLoad);
     if (!suppressReport(C, DerefExpr))
-      reportBug(DerefKind::FixedAddress, notNullState, DerefExpr, C);
+      reportBug(FixedAddressBug, notNullState, DerefExpr, C);
     return;
   }
 
@@ -330,7 +314,7 @@ void DereferenceChecker::checkBind(SVal L, SVal V, const Stmt *S,
 
   // One should never write to label addresses.
   if (auto Label = L.getAs<loc::GotoLabel>()) {
-    reportBug(DerefKind::AddressOfLabel, C.getState(), S, C);
+    reportBug(LabelBug, C.getState(), S, C);
     return;
   }
 
@@ -351,7 +335,7 @@ void DereferenceChecker::checkBind(SVal L, SVal V, const Stmt *S,
     if (!StNonNull) {
       const Expr *expr = getDereferenceExpr(S, /*IsBind=*/true);
       if (!suppressReport(C, expr)) {
-        reportBug(DerefKind::NullPointer, StNull, expr, C);
+        reportBug(NullBug, StNull, expr, C);
         return;
       }
     }
@@ -369,7 +353,7 @@ void DereferenceChecker::checkBind(SVal L, SVal V, const Stmt *S,
   if (V.isConstant()) {
     const Expr *DerefExpr = getDereferenceExpr(S, true);
     if (!suppressReport(C, DerefExpr))
-      reportBug(DerefKind::FixedAddress, State, DerefExpr, C);
+      reportBug(FixedAddressBug, State, DerefExpr, C);
     return;
   }
 
@@ -392,26 +376,8 @@ void DereferenceChecker::checkBind(SVal L, SVal V, const Stmt *S,
   C.addTransition(State, this);
 }
 
-void ento::registerDereferenceModeling(CheckerManager &Mgr) {
-  Mgr.registerChecker<DereferenceChecker>();
-}
-
-bool ento::shouldRegisterDereferenceModeling(const CheckerManager &) {
-  return true;
-}
-
 void ento::registerNullDereferenceChecker(CheckerManager &Mgr) {
-  auto *Chk = Mgr.getChecker<DereferenceChecker>();
-  Chk->CheckNullDereference = true;
-  Chk->BT_Null.reset(new BugType(Mgr.getCurrentCheckerName(),
-                                 "Dereference of null pointer",
-                                 categories::LogicError));
-  Chk->BT_Undef.reset(new BugType(Mgr.getCurrentCheckerName(),
-                                  "Dereference of undefined pointer value",
-                                  categories::LogicError));
-  Chk->BT_Label.reset(new BugType(Mgr.getCurrentCheckerName(),
-                                  "Dereference of the address of a label",
-                                  categories::LogicError));
+  Mgr.getChecker<DereferenceChecker>()->NullDerefChecker.enable(Mgr);
 }
 
 bool ento::shouldRegisterNullDereferenceChecker(const CheckerManager &) {
@@ -419,11 +385,7 @@ bool ento::shouldRegisterNullDereferenceChecker(const CheckerManager &) {
 }
 
 void ento::registerFixedAddressDereferenceChecker(CheckerManager &Mgr) {
-  auto *Chk = Mgr.getChecker<DereferenceChecker>();
-  Chk->CheckFixedDereference = true;
-  Chk->BT_FixedAddress.reset(new BugType(Mgr.getCurrentCheckerName(),
-                                         "Dereference of a fixed address",
-                                         categories::LogicError));
+  Mgr.getChecker<DereferenceChecker>()->FixedDerefChecker.enable(Mgr);
 }
 
 bool ento::shouldRegisterFixedAddressDereferenceChecker(
diff --git a/clang/test/Analysis/analyzer-enabled-checkers.c b/clang/test/Analysis/analyzer-enabled-checkers.c
index 78ee00deea18d..a632b70fa843a 100644
--- a/clang/test/Analysis/analyzer-enabled-checkers.c
+++ b/clang/test/Analysis/analyzer-enabled-checkers.c
@@ -14,7 +14,6 @@
 // CHECK-NEXT: core.BitwiseShift
 // CHECK-NEXT: core.CallAndMessageModeling
 // CHECK-NEXT: core.CallAndMessage
-// CHECK-NEXT: core.DereferenceModeling
 // CHECK-NEXT: core.DivideZero
 // CHECK-NEXT: core.DynamicTypePropagation
 // CHECK-NEXT: core.FixedAddressDereference
diff --git a/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c b/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
index 7f9c9ff4c9fd7..b388c312ba957 100644
--- a/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
+++ b/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
@@ -22,7 +22,6 @@
 // CHECK-NEXT: core.BitwiseShift
 // CHECK-NEXT: core.CallAndMessageModeling
 // CHECK-NEXT: core.CallAndMessage
-// CHECK-NEXT: core.DereferenceModeling
 // CHECK-NEXT: core.DivideZero
 // CHECK-NEXT: core.DynamicTypePropagation
 // CHECK-NEXT: core.FixedAddressDereference

[steakhal]

I've only skimmed this through. It looks appealing though.
You can merge this if someone else also approves it.

[gamesh411]

Great simplification.