llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra Author: None (pizzud) <details> <summary>Changes</summary> 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. --- Full diff: https://github.com/llvm/llvm-project/pull/67467.diff 8 Files Affected: - (modified) clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp (+3) - (modified) clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt (+1) - (added) clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp (+145) - (added) clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h (+42) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+8-3) - (added) clang-tools-extra/docs/clang-tidy/checks/bugprone/move-shared-pointer-contents.rst (+19) - (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+2-1) - (added) clang-tools-extra/test/clang-tidy/checkers/bugprone/move-shared-pointer-contents.cpp (+138) ``````````diff diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index 435cb1e3fbcff3..4e4092b90d4cf5 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" @@ -155,6 +156,8 @@ class BugproneModule : public ClangTidyModule { "bugprone-misplaced-widening-cast"); CheckFactories.registerCheck<MoveForwardingReferenceCheck>( "bugprone-move-forwarding-reference"); + CheckFactories.registerCheck<MoveSharedPointerContentsCheck>( + "bugprone-move-shared-pointer-contents"); CheckFactories.registerCheck<MultiLevelImplicitPointerConversionCheck>( "bugprone-multi-level-implicit-pointer-conversion"); CheckFactories.registerCheck<MultipleNewInOneExpressionCheck>( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index 70e7fbc7ec0c14..62bba10ad94dc6 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -37,6 +37,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 00000000000000..9ff357b58f9778 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp @@ -0,0 +1,145 @@ +//===--- 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 <signal.h> +#include <unistd.h> + +#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 { +namespace { + +// Reports whether the QualType matches the inner matcher, which is expected to be +// matchesAnyListedName. The QualType is expected to either point to a RecordDecl +// (for concrete types) or an ElaboratedType (for dependent ones). +AST_MATCHER_P(QualType, isSharedPointer, + clang::ast_matchers::internal::Matcher<NamedDecl>, InnerMatcher) { + if (const auto *RD = Node.getTypePtr()->getAsCXXRecordDecl(); RD != nullptr) { + return InnerMatcher.matches(*RD, Finder, Builder); + } else if (const auto *ED = Node.getTypePtr()->getAs<ElaboratedType>(); + ED != nullptr) { + if (const auto *TS = ED->getNamedType() + .getTypePtr() + ->getAs<TemplateSpecializationType>(); + TS != nullptr) { + return InnerMatcher.matches(*TS->getTemplateName().getAsTemplateDecl(), + Finder, Builder); + } + } + + return false; +} + +} // namespace + +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"))); + + auto resolvedType = callExpr(anyOf( + // Resolved type, direct move. + callExpr( + isStdMove, + hasArgument( + 0, + cxxOperatorCallExpr( + hasOverloadedOperatorName("*"), + hasDescendant(declRefExpr(hasType(qualType(isSharedPointer( + matchers::matchesAnyListedName(SharedPointerClasses)))))), + callee(cxxMethodDecl())))) + .bind("call"), + // Resolved type, move out of get(). + callExpr( + isStdMove, + hasArgument( + 0, unaryOperator( + hasOperatorName("*"), + hasUnaryOperand(allOf( + hasDescendant(declRefExpr(hasType(qualType( + isSharedPointer(matchers::matchesAnyListedName( + SharedPointerClasses)))))), + cxxMemberCallExpr( + callee(cxxMethodDecl(hasName("get"))))))))) + .bind("get_call"))); + + Finder->addMatcher(resolvedType, this); + + auto isStdMoveUnresolved = callee(unresolvedLookupExpr( + hasAnyDeclaration(namedDecl(hasUnderlyingDecl(hasName("::std::move")))))); + + auto unresolvedType = callExpr(anyOf( + // Unresolved type, direct move. + callExpr( + isStdMoveUnresolved, + hasArgument(0, unaryOperator( + hasOperatorName("*"), + hasUnaryOperand(declRefExpr(hasType(qualType( + isSharedPointer(matchers::matchesAnyListedName( + SharedPointerClasses))))))))) + .bind("unresolved_call"), + + // 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. + callExpr(isStdMoveUnresolved, + hasArgument( + 0, unaryOperator( + hasOperatorName("*"), + hasDescendant(cxxDependentScopeMemberExpr( + hasMemberName("get"))), + hasDescendant(declRefExpr(to(varDecl(hasType(qualType( + isSharedPointer(matchers::matchesAnyListedName( + SharedPointerClasses))))))))))) + .bind("unresolved_get_call"))); + + Finder->addMatcher(unresolvedType, this); +} + +void MoveSharedPointerContentsCheck::check( + const MatchFinder::MatchResult &Result) { + clang::SourceLocation Loc; + if (const auto *UnresolvedCall = + Result.Nodes.getNodeAs<CallExpr>("unresolved_call"); + UnresolvedCall != nullptr) { + Loc = UnresolvedCall->getBeginLoc(); + } else if (const auto *UnresolvedGetCall = + Result.Nodes.getNodeAs<CallExpr>("unresolved_get_call"); + UnresolvedGetCall != nullptr) { + Loc = UnresolvedGetCall->getBeginLoc(); + } else if (const auto *GetCall = Result.Nodes.getNodeAs<CallExpr>("get_call"); + GetCall != nullptr) { + Loc = GetCall->getBeginLoc(); + } else if (const auto *Call = Result.Nodes.getNodeAs<CallExpr>("call"); + Call != nullptr) { + Loc = Call->getBeginLoc(); + } else { + return; + } + + 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 00000000000000..d2a42c49ddf679 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h @@ -0,0 +1,42 @@ +//===--- 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: + 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 b4d87e0ed2a67a..1238ef8336239c 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. @@ -368,8 +374,7 @@ Changes in existing checks <clang-tidy/checks/misc/const-correctness>` check to avoid false positive when using pointer to member function. Additionally, the check no longer emits a diagnostic when a variable that is not type-dependent is an operand of a - type-dependent binary operator. Improved performance of the check through - optimizations. + type-dependent binary operator. - Improved :doc:`misc-include-cleaner <clang-tidy/checks/misc/include-cleaner>` check by adding option @@ -383,7 +388,7 @@ Changes in existing checks - Improved :doc:`misc-unused-using-decls <clang-tidy/checks/misc/unused-using-decls>` check to avoid false positive when - using in elaborated type and only check cpp files. + using in elaborated type. - Improved :doc:`modernize-avoid-bind <clang-tidy/checks/modernize/avoid-bind>` check to 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 00000000000000..ea4c1fb6df708c --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/move-shared-pointer-contents.rst @@ -0,0 +1,19 @@ +.. 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 2f86121ad87299..b7689889dd2838 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -117,7 +117,8 @@ Clang-Tidy Checks :doc:`bugprone-parent-virtual-call <bugprone/parent-virtual-call>`, "Yes" :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-reserved-identifier <bugprone/reserved-identifier>`, "Yes", + :doc:`bugprone-shared-pointer-contents-move <bugprone/shared-pointer-contents-move>`, :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 00000000000000..8a741581c30929 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/move-shared-pointer-contents.cpp @@ -0,0 +1,138 @@ +// 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); +} + +struct HasASharedPtrField { + std::shared_ptr<Nontrivial> x; +}; + +HasASharedPtrField returnsStruct() { + HasASharedPtrField h; + return h; +} + +void subfield() { + int x = std::move(returnsStruct().x->x); +} `````````` </details> https://github.com/llvm/llvm-project/pull/67467 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
