https://github.com/pizzud updated https://github.com/llvm/llvm-project/pull/67467
>From 8e1216f1b0d5d44ed0644bf8ff33fc9fc9344efd Mon Sep 17 00:00:00 2001 From: David Pizzuto <piz...@google.com> Date: Tue, 26 Sep 2023 10:45:42 -0700 Subject: [PATCH] [clang-tidy] Add bugprone-move-shared-pointer-contents check. This check detects moves of the contents of a shared pointer rather than the pointer itself. Other code with a reference to the shared pointer is probably not expecting the move. The set of shared pointer classes is configurable via options to allow individual projects to cover additional types. --- .../bugprone/BugproneTidyModule.cpp | 3 + .../clang-tidy/bugprone/CMakeLists.txt | 2 + .../MoveSharedPointerContentsCheck.cpp | 157 ++++++++++++++++++ .../bugprone/MoveSharedPointerContentsCheck.h | 45 +++++ clang-tools-extra/docs/ReleaseNotes.rst | 6 + .../bugprone/move-shared-pointer-contents.rst | 20 +++ .../docs/clang-tidy/checks/list.rst | 2 + .../bugprone/move-shared-pointer-contents.cpp | 125 ++++++++++++++ 8 files changed, 360 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/move-shared-pointer-contents.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/move-shared-pointer-contents.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index 435cb1e3fbcff34..a973299f867af03 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -41,6 +41,7 @@ #include "MisplacedPointerArithmeticInAllocCheck.h" #include "MisplacedWideningCastCheck.h" #include "MoveForwardingReferenceCheck.h" +#include "MoveSharedPointerContentsCheck.h" #include "MultiLevelImplicitPointerConversionCheck.h" #include "MultipleNewInOneExpressionCheck.h" #include "MultipleStatementMacroCheck.h" @@ -132,6 +133,8 @@ class BugproneModule : public ClangTidyModule { "bugprone-inaccurate-erase"); CheckFactories.registerCheck<IncorrectEnableIfCheck>( "bugprone-incorrect-enable-if"); + CheckFactories.registerCheck<MoveSharedPointerContentsCheck>( + "bugprone-move-shared-pointer-contents"); CheckFactories.registerCheck<SwitchMissingDefaultCaseCheck>( "bugprone-switch-missing-default-case"); CheckFactories.registerCheck<IncDecInConditionsCheck>( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index 70e7fbc7ec0c144..464feb144322cd4 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -25,6 +25,7 @@ add_clang_library(clangTidyBugproneModule ImplicitWideningOfMultiplicationResultCheck.cpp InaccurateEraseCheck.cpp IncorrectEnableIfCheck.cpp + MoveSharedPointerContentsCheck.cpp SwitchMissingDefaultCaseCheck.cpp IncDecInConditionsCheck.cpp IncorrectRoundingsCheck.cpp @@ -37,6 +38,7 @@ add_clang_library(clangTidyBugproneModule MisplacedPointerArithmeticInAllocCheck.cpp MisplacedWideningCastCheck.cpp MoveForwardingReferenceCheck.cpp + MoveSharedPointerContentsCheck.cpp MultiLevelImplicitPointerConversionCheck.cpp MultipleNewInOneExpressionCheck.cpp MultipleStatementMacroCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp new file mode 100644 index 000000000000000..2901e5687b99330 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp @@ -0,0 +1,157 @@ +//===--- MoveSharedPointerContentsCheck.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 "MoveSharedPointerContentsCheck.h" +#include "../ClangTidyCheck.h" +#include "../utils/Matchers.h" +#include "../utils/OptionsUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::bugprone { + +MoveSharedPointerContentsCheck::MoveSharedPointerContentsCheck( + StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + SharedPointerClasses(utils::options::parseStringList( + Options.get("SharedPointerClasses", "::std::shared_ptr"))) {} + +void MoveSharedPointerContentsCheck::registerMatchers(MatchFinder *Finder) { + auto isStdMove = callee(functionDecl(hasName("::std::move"))); + + // Resolved type, direct move. + Finder->addMatcher( + callExpr(isStdMove, hasArgument(0, cxxOperatorCallExpr( + hasOverloadedOperatorName("*"), + callee(cxxMethodDecl(ofClass( + matchers::matchesAnyListedName( + SharedPointerClasses))))))) + .bind("call"), + this); + + // Resolved type, move out of get(). + Finder->addMatcher( + callExpr( + isStdMove, + hasArgument( + 0, unaryOperator( + hasOperatorName("*"), + hasUnaryOperand(cxxMemberCallExpr(callee(cxxMethodDecl( + hasName("get"), ofClass(matchers::matchesAnyListedName( + SharedPointerClasses))))))))) + .bind("get_call"), + this); + + auto isStdMoveUnresolved = callee(unresolvedLookupExpr( + hasAnyDeclaration(namedDecl(hasUnderlyingDecl(hasName("::std::move")))))); + + // Unresolved type, direct move. + Finder->addMatcher( + callExpr( + isStdMoveUnresolved, + hasArgument(0, unaryOperator(hasOperatorName("*"), + hasUnaryOperand(declRefExpr(hasType( + qualType().bind("unresolved_p"))))))) + .bind("unresolved_call"), + this); + // Annoyingly, the declRefExpr in the unresolved-move-of-get() case + // is of <dependent type> rather than shared_ptr<T>, so we have to + // just fetch the variable. This does leave a gap where a temporary + // shared_ptr wouldn't be caught, but moving out of a temporary + // shared pointer is a truly wild thing to do so it should be okay. + + // Unresolved type, move out of get(). + Finder->addMatcher( + callExpr(isStdMoveUnresolved, + hasArgument( + 0, unaryOperator(hasOperatorName("*"), + hasDescendant(cxxDependentScopeMemberExpr( + hasMemberName("get"))), + hasDescendant(declRefExpr(to( + varDecl().bind("unresolved_get_p"))))))) + .bind("unresolved_get_call"), + this); +} + +bool MoveSharedPointerContentsCheck::isSharedPointerClass( + const VarDecl *VD) const { + if (VD == nullptr) { + return false; + } + + const QualType QT = VD->getType(); + return isSharedPointerClass(&QT); +} + +bool MoveSharedPointerContentsCheck::isSharedPointerClass( + const QualType *QT) const { + if (QT == nullptr) { + return false; + } + // We want the qualified name without template parameters, + // const/volatile, or reference/pointer qualifiers so we can look + // it up in SharedPointerClasses. This is a bit messy, but gets us + // to the underlying type without template parameters (eg + // std::shared_ptr) or const/volatile qualifiers even in the face of + // typedefs. + + bool found = false; + const auto *Template = llvm::dyn_cast<TemplateSpecializationType>( + QT->getSplitDesugaredType().Ty); + if (Template != nullptr) { + const std::string TypeName = Template->getTemplateName() + .getAsTemplateDecl() + ->getQualifiedNameAsString(); + for (const llvm::StringRef SharedPointer : SharedPointerClasses) { + // SharedPointer entries may or may not have leading ::, but TypeName + // definitely won't. + if (SharedPointer == TypeName || SharedPointer.substr(2) == TypeName) { + found = true; + break; + } + } + } + + return found; +} + +void MoveSharedPointerContentsCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *Call = Result.Nodes.getNodeAs<CallExpr>("call"); + const auto *GetCall = Result.Nodes.getNodeAs<CallExpr>("get_call"); + const auto *UnresolvedCall = + Result.Nodes.getNodeAs<CallExpr>("unresolved_call"); + const auto *UnresolvedPtr = Result.Nodes.getNodeAs<QualType>("unresolved_p"); + const auto *UnresolvedGetCall = + Result.Nodes.getNodeAs<CallExpr>("unresolved_get_call"); + const auto *UnresolvedGetPtr = + Result.Nodes.getNodeAs<VarDecl>("unresolved_get_p"); + + const bool Unresolved = isSharedPointerClass(UnresolvedPtr); + const bool UnresolvedGet = isSharedPointerClass(UnresolvedGetPtr); + + clang::SourceLocation Loc; + if (Unresolved) { + Loc = UnresolvedCall->getBeginLoc(); + } else if (UnresolvedGet) { + Loc = UnresolvedGetCall->getBeginLoc(); + } else if (GetCall != nullptr) { + Loc = GetCall->getBeginLoc(); + } else if (Call != nullptr) { + Loc = Call->getBeginLoc(); + } + if (Loc.isValid()) { + diag(Loc, + "don't move the contents out of a shared pointer, as other accessors " + "expect them to remain in a determinate state"); + } +} + +} // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h new file mode 100644 index 000000000000000..ffdd94bfcb0e711 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h @@ -0,0 +1,45 @@ +//===--- MoveSharedPointerContentsCheck.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_MOVESHAREDPOINTERCONTENTSCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_MOVESHAREDPOINTERCONTENTSCHECK_H + +#include <vector> + +#include "../ClangTidyCheck.h" + +namespace clang::tidy::bugprone { + +/// Detects calls to move the contents out of a ``std::shared_ptr`` rather than +/// moving the pointer itself. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/move-shared-pointer-contents.html +class MoveSharedPointerContentsCheck : public ClangTidyCheck { +public: + MoveSharedPointerContentsCheck(StringRef Name, ClangTidyContext *Context); + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + bool + isLanguageVersionSupported(const LangOptions &LangOptions) const override { + return LangOptions.CPlusPlus11; + } + std::optional<TraversalKind> getCheckTraversalKind() const override { + return TK_IgnoreUnlessSpelledInSource; + } + +private: + // Returns whether the type or variable is one of the SharedPointerClasses. + bool isSharedPointerClass(const VarDecl *VD) const; + bool isSharedPointerClass(const QualType *QT) const; + const std::vector<StringRef> SharedPointerClasses; +}; + +} // namespace clang::tidy::bugprone + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_MOVESHAREDPOINTERCONTENTSCHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 1bd5a72126c10be..779b7d66f8d052c 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -162,6 +162,12 @@ New checks Detects incorrect usages of ``std::enable_if`` that don't name the nested ``type`` type. +- New :doc:`bugprone-move-shared-pointer-contents + <clang-tidy/checks/bugprone/move-shared-pointer-contents>` check. + + Detects calls to move the contents out of a ``std::shared_ptr`` rather than + moving the pointer itself. + - New :doc:`bugprone-multi-level-implicit-pointer-conversion <clang-tidy/checks/bugprone/multi-level-implicit-pointer-conversion>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/move-shared-pointer-contents.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/move-shared-pointer-contents.rst new file mode 100644 index 000000000000000..5c47550cceeff2e --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/move-shared-pointer-contents.rst @@ -0,0 +1,20 @@ +.. title:: clang-tidy - bugprone-move-shared-pointer-contents + +bugprone-move-shared-pointer-contents +===================================== + + +Detects calls to move the contents out of a ``std::shared_ptr`` rather +than moving the pointer itself. In other words, calling +``std::move(*p)`` or ``std::move(*p.get())``. Other reference holders +may not be expecting the move and suddenly getting empty or otherwise +indeterminate states can cause issues. + +Options +------- + +.. option :: SharedPointerClasses + + A semicolon-separated list of class names that should be treated as shared + pointers. Classes are resolved through aliases, so any alias to the defined + classes will be considered. Default is `std::shared_ptr`. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 2f86121ad872998..ad89b2de31bc948 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -107,6 +107,7 @@ Clang-Tidy Checks :doc:`bugprone-misplaced-pointer-arithmetic-in-alloc <bugprone/misplaced-pointer-arithmetic-in-alloc>`, "Yes" :doc:`bugprone-misplaced-widening-cast <bugprone/misplaced-widening-cast>`, :doc:`bugprone-move-forwarding-reference <bugprone/move-forwarding-reference>`, "Yes" + :doc:`bugprone-move-shared-pointer-contents <bugprone/move-shared-pointer-contents>`, "Yes" :doc:`bugprone-multi-level-implicit-pointer-conversion <bugprone/multi-level-implicit-pointer-conversion>`, :doc:`bugprone-multiple-new-in-one-expression <bugprone/multiple-new-in-one-expression>`, :doc:`bugprone-multiple-statement-macro <bugprone/multiple-statement-macro>`, @@ -118,6 +119,7 @@ Clang-Tidy Checks :doc:`bugprone-posix-return <bugprone/posix-return>`, "Yes" :doc:`bugprone-redundant-branch-condition <bugprone/redundant-branch-condition>`, "Yes" :doc:`bugprone-reserved-identifier <bugprone/reserved-identifier>`, "Yes" + :doc:`bugprone-shared-pointer-contents-move <bugprone/shared-pointer-contents-move>`, "Yes" :doc:`bugprone-shared-ptr-array-mismatch <bugprone/shared-ptr-array-mismatch>`, "Yes" :doc:`bugprone-signal-handler <bugprone/signal-handler>`, :doc:`bugprone-signed-char-misuse <bugprone/signed-char-misuse>`, diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/move-shared-pointer-contents.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/move-shared-pointer-contents.cpp new file mode 100644 index 000000000000000..ccc4400aa691c74 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/move-shared-pointer-contents.cpp @@ -0,0 +1,125 @@ +// RUN: %check_clang_tidy %s bugprone-move-shared-pointer-contents %t -- -config="{CheckOptions: {bugprone-move-shared-pointer-contents.SharedPointerClasses: '::std::shared_ptr;my::OtherSharedPtr;'}}" + +// Some dummy definitions we'll need. + +namespace std { + +using size_t = int; + +template <typename> struct remove_reference; +template <typename _Tp> struct remove_reference { typedef _Tp type; }; +template <typename _Tp> struct remove_reference<_Tp &> { typedef _Tp type; }; +template <typename _Tp> struct remove_reference<_Tp &&> { typedef _Tp type; }; + +template <typename _Tp> +constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t) { + return static_cast<typename std::remove_reference<_Tp>::type &&>(__t); +} + +template <typename T> +struct shared_ptr { + shared_ptr(); + T *get() const; + explicit operator bool() const; + void reset(T *ptr); + T &operator*() const; + T *operator->() const; +}; + +} // namespace std + +namespace my { +template <typename T> +using OtherSharedPtr = std::shared_ptr<T>; +// Not part of the config. +template <typename T> +using YetAnotherSharedPtr = T*; +} // namespace my + +struct Nontrivial { + int x; + Nontrivial() : x(2) {} + Nontrivial(Nontrivial& other) { x = other.x; } + Nontrivial(Nontrivial&& other) { x = std::move(other.x); } + Nontrivial& operator=(Nontrivial& other) { x = other.x; } + Nontrivial& operator=(Nontrivial&& other) { x = std::move(other.x); } +}; + +// Test cases begin here. + +void correct() { + std::shared_ptr<Nontrivial> p; + Nontrivial x = *std::move(p); +} + +void simpleFinding() { + std::shared_ptr<Nontrivial> p; + Nontrivial y = std::move(*p); +} +// CHECK-MESSAGES: :[[@LINE-2]]:18: warning: don't move the contents out of a shared pointer, as other accessors expect them to remain in a determinate state [bugprone-move-shared-pointer-contents] + +void aliasedType() { + using nontrivial_ptr = std::shared_ptr<Nontrivial>; + nontrivial_ptr p; + Nontrivial x = std::move(*p); +} +// CHECK-MESSAGES: :[[@LINE-2]]:18: warning: don't move the contents out of a shared pointer, as other accessors expect them to remain in a determinate state [bugprone-move-shared-pointer-contents] + +void configWorks() { + my::OtherSharedPtr<Nontrivial> p; + Nontrivial x = std::move(*p); +} +// CHECK-MESSAGES: :[[@LINE-2]]:18: warning: don't move the contents out of a shared pointer, as other accessors expect them to remain in a determinate state [bugprone-move-shared-pointer-contents] + +void sharedEsquePointerNotInConfig() { + my::YetAnotherSharedPtr<Nontrivial> p; + Nontrivial x = std::move(*p); +} + + +void multiStars() { + std::shared_ptr<Nontrivial> p; + int x = 2 * std::move(*p).x * 3; +} +// CHECK-MESSAGES: :[[@LINE-2]]:15: warning: don't move the contents out of a shared pointer, as other accessors expect them to remain in a determinate state [bugprone-move-shared-pointer-contents] + +template <typename T> +void unresolved() { + std::shared_ptr<T> p; + int x = 2 * std::move(*p).x * 3; +} +// CHECK-MESSAGES: :[[@LINE-2]]:15: warning: don't move the contents out of a shared pointer, as other accessors expect them to remain in a determinate state [bugprone-move-shared-pointer-contents] + +template <typename T> +void unresolvedAsAReference() { + std::shared_ptr<T> p; + std::shared_ptr<T>& q = p; + int x = 2 * std::move(*q).x * 3; +} +// CHECK-MESSAGES: :[[@LINE-2]]:15: warning: don't move the contents out of a shared pointer, as other accessors expect them to remain in a determinate state [bugprone-move-shared-pointer-contents] + +template <typename T> +void unresolvedAlias() { + my::OtherSharedPtr<T> p; + Nontrivial x = std::move(*p); +} +// CHECK-MESSAGES: :[[@LINE-2]]:18: warning: don't move the contents out of a shared pointer, as other accessors expect them to remain in a determinate state [bugprone-move-shared-pointer-contents] + +void dereferencedGet() { + std::shared_ptr<Nontrivial> p; + Nontrivial x = std::move(*p.get()); +} +// CHECK-MESSAGES: :[[@LINE-2]]:18: warning: don't move the contents out of a shared pointer, as other accessors expect them to remain in a determinate state [bugprone-move-shared-pointer-contents] + +template <typename T> +void unresolvedDereferencedGet() { + std::shared_ptr<T> p; + T x = std::move(*p.get()); +} +// CHECK-MESSAGES: :[[@LINE-2]]:9: warning: don't move the contents out of a shared pointer, as other accessors expect them to remain in a determinate state [bugprone-move-shared-pointer-contents] + +template <typename T> +void rawPointer() { + T* p; + T x = std::move(*p); +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits