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. +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
