Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[analyzer][NFC] Make CheckerDocumentation checker in-sync with actual checker callbacks #83973

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

steakhal
Copy link
Contributor

@steakhal steakhal commented Mar 5, 2024

In PR #83677 I was surprised to see that outdated checker callback signatures are a problem. It turns out, we need the registerChecker... function to invoke the Mgr.registerChecker<>() which would instantiate the _register calls, that would take the address of the defined checker callbacks. Consequently, if the expected signatures mismatch, it won't compile from now on, so we have static guarantee that this issue never pops up again.

Given we need the register call, at this point we could just hook this checker into the debug package and make it never registered. It shouldn't hurt anyone :)

… checker callbacks

In PR llvm#83677 I was surprised to see that outdated checker callback
signatures are a problem. It turns out, we need the `registerChecker...`
function to invoke the `Mgr.registerChecker<>()` which would instantiate
the `_register` calls, that would take the address of the defined
checker callbacks. Consequently, if the expected signatires mismatch, it
won't complie from now on, so we have static guarantee that this issue
never pops up again.

Given we need the `register` call, at this point we could just hook this
checker into the `debug` package and make it never registered.
It shouldn't hurt anyone :)
@steakhal steakhal requested review from mzyKi and NagyDonat March 5, 2024 08:35
@llvmbot llvmbot added the clang Clang issues not falling into any other category label Mar 5, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 5, 2024

@llvm/pr-subscribers-clang

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

Author: Balazs Benics (steakhal)

Changes

In PR #83677 I was surprised to see that outdated checker callback signatures are a problem. It turns out, we need the registerChecker... function to invoke the Mgr.registerChecker&lt;&gt;() which would instantiate the _register calls, that would take the address of the defined checker callbacks. Consequently, if the expected signatires mismatch, it won't complie from now on, so we have static guarantee that this issue never pops up again.

Given we need the register call, at this point we could just hook this checker into the debug package and make it never registered. It shouldn't hurt anyone :)


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

2 Files Affected:

  • (modified) clang/include/clang/StaticAnalyzer/Checkers/Checkers.td (+4)
  • (modified) clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp (+9-4)
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index a224b81c33a624..686e5e99f4a62c 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1654,6 +1654,10 @@ def StdCLibraryFunctionsTesterChecker : Checker<"StdCLibraryFunctionsTester">,
   WeakDependencies<[StdCLibraryFunctionsChecker]>,
   Documentation<NotDocumented>;
 
+def CheckerDocumentationChecker : Checker<"CheckerDocumentation">,
+  HelpText<"Defines an empty checker callback for all possible handlers.">,
+  Documentation<NotDocumented>;
+
 } // end "debug"
 
 
diff --git a/clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp b/clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp
index 0ca0c487b64550..01e0bed54cc6ed 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp
@@ -137,10 +137,7 @@ class CheckerDocumentation : public Checker< check::PreStmt<ReturnStmt>,
   /// (2) and (3). Post-call for the allocator is called after step (1).
   /// Pre-statement for the new-expression is called on step (4) when the value
   /// of the expression is evaluated.
-  /// \param NE     The C++ new-expression that triggered the allocation.
-  /// \param Target The allocated region, casted to the class type.
-  void checkNewAllocator(const CXXNewExpr *NE, SVal Target,
-                         CheckerContext &) const {}
+  void checkNewAllocator(const CXXAllocatorCall &, CheckerContext &) const {}
 
   /// Called on a load from and a store to a location.
   ///
@@ -330,5 +327,13 @@ void CheckerDocumentation::checkPostStmt(const DeclStmt *DS,
                                          CheckerContext &C) const {
 }
 
+void registerCheckerDocumentationChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<CheckerDocumentation>();
+}
+
+bool shouldRegisterCheckerDocumentationChecker(const CheckerManager &) {
+  return false;
+}
+
 } // end namespace ento
 } // end namespace clang

Copy link
Contributor

@NagyDonat NagyDonat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Minor remark: "signatires" is misspelled in the commit message.

@steakhal
Copy link
Contributor Author

steakhal commented Mar 5, 2024

LGTM.

Minor remark: "signatires" is misspelled in the commit message.

Fixed the typos. Thanks!

@steakhal steakhal merged commit 0d8e16a into llvm:main Mar 5, 2024
7 checks passed
@steakhal steakhal deleted the checker-documentation branch March 5, 2024 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:static analyzer clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants