[clang-tools-extra] 72d4d4e - [clang-tidy]add new check `bugprone-compare-pointer-to-member-virtual-function` (#66055)
via cfe-commits
cfe-commits at lists.llvm.org
Fri Sep 15 05:59:17 PDT 2023
Author: Congcong Cai
Date: 2023-09-15T20:59:12+08:00
New Revision: 72d4d4e3b9f6b54582358ac5c1006e295b9ed58e
URL: https://github.com/llvm/llvm-project/commit/72d4d4e3b9f6b54582358ac5c1006e295b9ed58e
DIFF: https://github.com/llvm/llvm-project/commit/72d4d4e3b9f6b54582358ac5c1006e295b9ed58e.diff
LOG: [clang-tidy]add new check `bugprone-compare-pointer-to-member-virtual-function` (#66055)
Added:
clang-tools-extra/clang-tidy/bugprone/ComparePointerToMemberVirtualFunctionCheck.cpp
clang-tools-extra/clang-tidy/bugprone/ComparePointerToMemberVirtualFunctionCheck.h
clang-tools-extra/docs/clang-tidy/checks/bugprone/compare-pointer-to-member-virtual-function.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/compare-pointer-to-member-virtual-function.cpp
Modified:
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
index a67a91eedd10482..543c522899d7a52 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -16,6 +16,7 @@
#include "BadSignalToKillThreadCheck.h"
#include "BoolPointerImplicitConversionCheck.h"
#include "BranchCloneCheck.h"
+#include "ComparePointerToMemberVirtualFunctionCheck.h"
#include "CopyConstructorInitCheck.h"
#include "DanglingHandleCheck.h"
#include "DynamicStaticInitializersCheck.h"
@@ -103,6 +104,8 @@ class BugproneModule : public ClangTidyModule {
CheckFactories.registerCheck<BoolPointerImplicitConversionCheck>(
"bugprone-bool-pointer-implicit-conversion");
CheckFactories.registerCheck<BranchCloneCheck>("bugprone-branch-clone");
+ CheckFactories.registerCheck<ComparePointerToMemberVirtualFunctionCheck>(
+ "bugprone-compare-pointer-to-member-virtual-function");
CheckFactories.registerCheck<CopyConstructorInitCheck>(
"bugprone-copy-constructor-init");
CheckFactories.registerCheck<DanglingHandleCheck>(
diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
index 3c768021feb1502..0df9e439b715e5a 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
@@ -11,6 +11,7 @@ add_clang_library(clangTidyBugproneModule
BoolPointerImplicitConversionCheck.cpp
BranchCloneCheck.cpp
BugproneTidyModule.cpp
+ ComparePointerToMemberVirtualFunctionCheck.cpp
CopyConstructorInitCheck.cpp
DanglingHandleCheck.cpp
DynamicStaticInitializersCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/ComparePointerToMemberVirtualFunctionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ComparePointerToMemberVirtualFunctionCheck.cpp
new file mode 100644
index 000000000000000..9d1d92b989bf1f0
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/ComparePointerToMemberVirtualFunctionCheck.cpp
@@ -0,0 +1,106 @@
+//===--- ComparePointerToMemberVirtualFunctionCheck.cpp - clang-tidy ------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "ComparePointerToMemberVirtualFunctionCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/ASTTypeTraits.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/OperationKinds.h"
+#include "clang/AST/Type.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchersMacros.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticIDs.h"
+#include "llvm/ADT/SmallVector.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+namespace {
+
+AST_MATCHER(CXXMethodDecl, isVirtual) { return Node.isVirtual(); }
+
+static const char *const ErrorMsg =
+ "comparing a pointer to member virtual function with other pointer is "
+ "unspecified behavior, only compare it with a null-pointer constant for "
+ "equality.";
+
+} // namespace
+
+void ComparePointerToMemberVirtualFunctionCheck::registerMatchers(
+ MatchFinder *Finder) {
+
+ auto DirectMemberVirtualFunctionPointer = unaryOperator(
+ allOf(hasOperatorName("&"),
+ hasUnaryOperand(declRefExpr(to(cxxMethodDecl(isVirtual()))))));
+ auto IndirectMemberPointer =
+ ignoringImpCasts(declRefExpr().bind("indirect_member_pointer"));
+
+ Finder->addMatcher(
+ binaryOperator(
+ allOf(hasAnyOperatorName("==", "!="),
+ hasEitherOperand(
+ hasType(memberPointerType(pointee(functionType())))),
+ anyOf(hasEitherOperand(DirectMemberVirtualFunctionPointer),
+ hasEitherOperand(IndirectMemberPointer)),
+ unless(hasEitherOperand(
+ castExpr(hasCastKind(CK_NullToMemberPointer))))))
+ .bind("binary_operator"),
+ this);
+}
+
+void ComparePointerToMemberVirtualFunctionCheck::check(
+ const MatchFinder::MatchResult &Result) {
+ const auto *BO = Result.Nodes.getNodeAs<BinaryOperator>("binary_operator");
+ const auto *DRE =
+ Result.Nodes.getNodeAs<DeclRefExpr>("indirect_member_pointer");
+
+ if (DRE == nullptr) {
+ // compare with pointer to member virtual function.
+ diag(BO->getOperatorLoc(), ErrorMsg);
+ return;
+ }
+ // compare with variable which type is pointer to member function.
+ llvm::SmallVector<SourceLocation, 12U> SameSignatureVirtualMethods{};
+ const auto *MPT = cast<MemberPointerType>(DRE->getType().getCanonicalType());
+ const Type *T = MPT->getClass();
+ if (T == nullptr)
+ return;
+ const CXXRecordDecl *RD = T->getAsCXXRecordDecl();
+ if (RD == nullptr)
+ return;
+
+ constexpr bool StopVisit = false;
+
+ auto VisitSameSignatureVirtualMethods =
+ [&](const CXXRecordDecl *CurrentRecordDecl) -> bool {
+ bool Ret = !StopVisit;
+ for (const auto *MD : CurrentRecordDecl->methods()) {
+ if (MD->isVirtual() && MD->getType() == MPT->getPointeeType()) {
+ SameSignatureVirtualMethods.push_back(MD->getBeginLoc());
+ Ret = StopVisit;
+ }
+ }
+ return Ret;
+ };
+
+ if (StopVisit != VisitSameSignatureVirtualMethods(RD)) {
+ RD->forallBases(VisitSameSignatureVirtualMethods);
+ }
+
+ if (!SameSignatureVirtualMethods.empty()) {
+ diag(BO->getOperatorLoc(), ErrorMsg);
+ for (const auto Loc : SameSignatureVirtualMethods)
+ diag(Loc, "potential member virtual function is declared here.",
+ DiagnosticIDs::Note);
+ }
+}
+
+} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/clang-tidy/bugprone/ComparePointerToMemberVirtualFunctionCheck.h b/clang-tools-extra/clang-tidy/bugprone/ComparePointerToMemberVirtualFunctionCheck.h
new file mode 100644
index 000000000000000..15561e068a6709d
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/ComparePointerToMemberVirtualFunctionCheck.h
@@ -0,0 +1,36 @@
+//=--- ComparePointerToMemberVirtualFunctionCheck.h - clang-tidy --*- C++ -*-=//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_COMPAREPOINTERTOMEMBERVIRTUALFUNCTIONCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_COMPAREPOINTERTOMEMBERVIRTUALFUNCTIONCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::bugprone {
+
+/// Detects unspecified behavior about equality comparison between pointer to
+/// member virtual function and anything other than null-pointer-constant.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/compare-pointer-to-member-virtual-function.html
+class ComparePointerToMemberVirtualFunctionCheck : public ClangTidyCheck {
+public:
+ ComparePointerToMemberVirtualFunctionCheck(StringRef Name,
+ ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+
+ bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+ return LangOpts.CPlusPlus;
+ }
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_COMPAREPOINTERTOMEMBERVIRTUALFUNCTIONCHECK_H
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 19c977977f9044c..6d6f51998a01e57 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -125,6 +125,12 @@ Improvements to clang-tidy
New checks
^^^^^^^^^^
+- New :doc:`bugprone-compare-pointer-to-member-virtual-function
+ <clang-tidy/checks/bugprone/compare-pointer-to-member-virtual-function>` check.
+
+ Detects equality comparison between pointer to member virtual function and
+ anything other than null-pointer-constant.
+
- New :doc:`bugprone-inc-dec-in-conditions
<clang-tidy/checks/bugprone/inc-dec-in-conditions>` check.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/compare-pointer-to-member-virtual-function.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/compare-pointer-to-member-virtual-function.rst
new file mode 100644
index 000000000000000..8b6749938957e86
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/compare-pointer-to-member-virtual-function.rst
@@ -0,0 +1,63 @@
+.. title:: clang-tidy - bugprone-compare-pointer-to-member-virtual-function
+
+bugprone-compare-pointer-to-member-virtual-function
+===================================================
+
+Detects unspecified behavior about equality comparison between pointer to member
+virtual function and anything other than null-pointer-constant.
+
+.. code-block:: c++
+ struct A {
+ void f1();
+ void f2();
+ virtual void f3();
+ virtual void f4();
+
+ void g1(int);
+ };
+
+ void fn() {
+ bool r1 = (&A::f1 == &A::f2); // ok
+ bool r2 = (&A::f1 == &A::f3); // bugprone
+ bool r3 = (&A::f1 != &A::f3); // bugprone
+ bool r4 = (&A::f3 == nullptr); // ok
+ bool r5 = (&A::f3 == &A::f4); // bugprone
+
+ void (A::*v1)() = &A::f3;
+ bool r6 = (v1 == &A::f1); // bugprone
+ bool r6 = (v1 == nullptr); // ok
+
+ void (A::*v2)() = &A::f2;
+ bool r7 = (v2 == &A::f1); // false positive, but potential risk if assigning other value to v2.
+
+ void (A::*v3)(int) = &A::g1;
+ bool r8 = (v3 == &A::g1); // ok, no virtual function match void(A::*)(int) signature.
+ }
+
+Provide warnings on equality comparisons involve pointers to member virtual
+function or variables which is potential pointer to member virtual function and
+any entity other than a null-pointer constant.
+
+In certain compilers, virtual function addresses are not conventional pointers
+but instead consist of offsets and indexes within a virtual function table
+(vtable). Consequently, these pointers may vary between base and derived
+classes, leading to unpredictable behavior when compared directly. This issue
+becomes particularly challenging when dealing with pointers to pure virtual
+functions, as they may not even have a valid address, further complicating
+comparisons.
+
+Instead, it is recommended to utilize the ``typeid`` operator or other appropriate
+mechanisms for comparing objects to ensure robust and predictable behavior in
+your codebase. By heeding this detection and adopting a more reliable comparison
+method, you can mitigate potential issues related to unspecified behavior,
+especially when dealing with pointers to member virtual functions or pure
+virtual functions, thereby improving the overall stability and maintainability
+of your code. In scenarios involving pointers to member virtual functions, it's
+only advisable to employ ``nullptr`` for comparisons.
+
+Limitations
+-----------
+
+Does not analyze values stored in a variable. For variable, only analyze all virtual
+methods in the same ``class`` or ``struct`` and diagnose when assigning a pointer
+to member virtual function to this variable is possible.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 75a86431d264412..1baabceea06ef48 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -82,6 +82,7 @@ Clang-Tidy Checks
:doc:`bugprone-bad-signal-to-kill-thread <bugprone/bad-signal-to-kill-thread>`,
:doc:`bugprone-bool-pointer-implicit-conversion <bugprone/bool-pointer-implicit-conversion>`, "Yes"
:doc:`bugprone-branch-clone <bugprone/branch-clone>`,
+ :doc:`bugprone-compare-pointer-to-member-virtual-function <bugprone/compare-pointer-to-member-virtual-function>`, "Yes"
:doc:`bugprone-copy-constructor-init <bugprone/copy-constructor-init>`, "Yes"
:doc:`bugprone-dangling-handle <bugprone/dangling-handle>`,
:doc:`bugprone-dynamic-static-initializers <bugprone/dynamic-static-initializers>`,
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/compare-pointer-to-member-virtual-function.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/compare-pointer-to-member-virtual-function.cpp
new file mode 100644
index 000000000000000..af737f1db81072d
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/compare-pointer-to-member-virtual-function.cpp
@@ -0,0 +1,86 @@
+// RUN: %check_clang_tidy %s bugprone-compare-pointer-to-member-virtual-function %t
+
+struct A {
+ virtual ~A();
+ void f1();
+ void f2();
+ virtual void f3();
+ virtual void f4();
+
+ void g1(int);
+};
+
+bool Result;
+
+void base() {
+ Result = (&A::f1 == &A::f2);
+
+ Result = (&A::f1 == &A::f3);
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: comparing a pointer to member virtual function with other pointer is unspecified behavior, only compare it with a null-pointer constant for equality.
+
+ Result = (&A::f1 != &A::f3);
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: comparing a pointer to member virtual function with other pointer is unspecified behavior, only compare it with a null-pointer constant for equality.
+
+ Result = (&A::f3 == nullptr);
+
+ Result = (&A::f3 == &A::f4);
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: comparing a pointer to member virtual function with other pointer is unspecified behavior, only compare it with a null-pointer constant for equality.
+
+ void (A::*V1)() = &A::f3;
+ Result = (V1 == &A::f1);
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: comparing a pointer to member virtual function with other pointer is unspecified behavior, only compare it with a null-pointer constant for equality.
+ // CHECK-MESSAGES: :7:3: note: potential member virtual function is declared here.
+ // CHECK-MESSAGES: :8:3: note: potential member virtual function is declared here.
+ Result = (&A::f1 == V1);
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: comparing a pointer to member virtual function with other pointer is unspecified behavior, only compare it with a null-pointer constant for equality.
+ // CHECK-MESSAGES: :7:3: note: potential member virtual function is declared here.
+ // CHECK-MESSAGES: :8:3: note: potential member virtual function is declared here.
+
+ auto& V2 = V1;
+ Result = (&A::f1 == V2);
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: comparing a pointer to member virtual function with other pointer is unspecified behavior, only compare it with a null-pointer constant for equality.
+ // CHECK-MESSAGES: :7:3: note: potential member virtual function is declared here.
+ // CHECK-MESSAGES: :8:3: note: potential member virtual function is declared here.
+
+ void (A::*V3)(int) = &A::g1;
+ Result = (V3 == &A::g1);
+ Result = (&A::g1 == V3);
+}
+
+using B = A;
+void usingRecordName() {
+ Result = (&B::f1 == &B::f3);
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: comparing a pointer to member virtual function with other pointer is unspecified behavior, only compare it with a null-pointer constant for equality.
+
+ void (B::*V1)() = &B::f1;
+ Result = (V1 == &B::f1);
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: comparing a pointer to member virtual function with other pointer is unspecified behavior, only compare it with a null-pointer constant for equality.
+ // CHECK-MESSAGES: :7:3: note: potential member virtual function is declared here.
+ // CHECK-MESSAGES: :8:3: note: potential member virtual function is declared here.
+}
+
+typedef A C;
+void typedefRecordName() {
+ Result = (&C::f1 == &C::f3);
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: comparing a pointer to member virtual function with other pointer is unspecified behavior, only compare it with a null-pointer constant for equality.
+
+ void (C::*V1)() = &C::f1;
+ Result = (V1 == &C::f1);
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: comparing a pointer to member virtual function with other pointer is unspecified behavior, only compare it with a null-pointer constant for equality.
+ // CHECK-MESSAGES: :7:3: note: potential member virtual function is declared here.
+ // CHECK-MESSAGES: :8:3: note: potential member virtual function is declared here.
+}
+
+struct A1 : public A {
+};
+
+void inheritClass() {
+ Result = (&A1::f1 == &A1::f3);
+ // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: comparing a pointer to member virtual function with other pointer is unspecified behavior, only compare it with a null-pointer constant for equality.
+
+ void (A1::*V1)() = &A1::f1;
+ Result = (V1 == &A1::f1);
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: comparing a pointer to member virtual function with other pointer is unspecified behavior, only compare it with a null-pointer constant for equality.
+ // CHECK-MESSAGES: :7:3: note: potential member virtual function is declared here.
+ // CHECK-MESSAGES: :8:3: note: potential member virtual function is declared here.
+}
More information about the cfe-commits
mailing list