llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-tidy Author: Daniil Dudkin (unterumarmung) <details> <summary>Changes</summary> Fixes #<!-- -->189499 Assisted by Codex. --- Patch is 28.41 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/189962.diff 8 Files Affected: - (modified) clang-tools-extra/clang-tidy/modernize/CMakeLists.txt (+1) - (modified) clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp (+2) - (added) clang-tools-extra/clang-tidy/modernize/UseBitCastCheck.cpp (+308) - (added) clang-tools-extra/clang-tidy/modernize/UseBitCastCheck.h (+44) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+6) - (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1) - (added) clang-tools-extra/docs/clang-tidy/checks/modernize/use-bit-cast.rst (+62) - (added) clang-tools-extra/test/clang-tidy/checkers/modernize/use-bit-cast.cpp (+291) ``````````diff diff --git a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt index 2c5c44db587fe..728a0b21613fd 100644 --- a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt @@ -32,6 +32,7 @@ add_clang_library(clangTidyModernizeModule STATIC TypeTraitsCheck.cpp UnaryStaticAssertCheck.cpp UseAutoCheck.cpp + UseBitCastCheck.cpp UseBoolLiteralsCheck.cpp UseConstraintsCheck.cpp UseDefaultMemberInitCheck.cpp diff --git a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp index cc13da7535bcb..6e823a558c299 100644 --- a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp @@ -32,6 +32,7 @@ #include "TypeTraitsCheck.h" #include "UnaryStaticAssertCheck.h" #include "UseAutoCheck.h" +#include "UseBitCastCheck.h" #include "UseBoolLiteralsCheck.h" #include "UseConstraintsCheck.h" #include "UseDefaultMemberInitCheck.h" @@ -88,6 +89,7 @@ class ModernizeModule : public ClangTidyModule { CheckFactories.registerCheck<MinMaxUseInitializerListCheck>( "modernize-min-max-use-initializer-list"); CheckFactories.registerCheck<PassByValueCheck>("modernize-pass-by-value"); + CheckFactories.registerCheck<UseBitCastCheck>("modernize-use-bit-cast"); CheckFactories.registerCheck<UseDesignatedInitializersCheck>( "modernize-use-designated-initializers"); CheckFactories.registerCheck<UseIntegerSignComparisonCheck>( diff --git a/clang-tools-extra/clang-tidy/modernize/UseBitCastCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseBitCastCheck.cpp new file mode 100644 index 0000000000000..bc91f9fee1775 --- /dev/null +++ b/clang-tools-extra/clang-tidy/modernize/UseBitCastCheck.cpp @@ -0,0 +1,308 @@ +//===----------------------------------------------------------------------===// +// +// 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 "UseBitCastCheck.h" +#include "../utils/Matchers.h" +#include "clang/AST/ASTContext.h" +#include "clang/AST/Expr.h" +#include "clang/AST/ExprCXX.h" +#include "clang/AST/Type.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Lex/Lexer.h" +#include "llvm/ADT/STLExtras.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::modernize { + +static const Expr *stripMemcpyArgument(const Expr *ExprNode) { + ExprNode = ExprNode->IgnoreParenImpCasts(); + while (const auto *Cast = dyn_cast<ExplicitCastExpr>(ExprNode)) + ExprNode = Cast->getSubExpr()->IgnoreParenImpCasts(); + return ExprNode; +} + +static bool isSupportedMemcpyObjectExpr(const Expr *ExprNode) { + ExprNode = ExprNode->IgnoreParenImpCasts(); + + if (isa<DeclRefExpr>(ExprNode)) + return true; + + const auto *Member = dyn_cast<MemberExpr>(ExprNode); + if (!Member || !isa<FieldDecl>(Member->getMemberDecl())) + if (const auto *MemberPointer = dyn_cast<BinaryOperator>(ExprNode)) + if (MemberPointer->getOpcode() == BO_PtrMemD || + MemberPointer->getOpcode() == BO_PtrMemI) + return isSupportedMemcpyObjectExpr(MemberPointer->getLHS()); + + if (!Member) + return false; + + return isSupportedMemcpyObjectExpr(Member->getBase()); +} + +static const Expr *extractMemcpyObjectExpr(const Expr *ExprNode) { + ExprNode = stripMemcpyArgument(ExprNode); + const auto *AddressOf = dyn_cast<UnaryOperator>(ExprNode); + if (!AddressOf || AddressOf->getOpcode() != UO_AddrOf) + return nullptr; + + const Expr *ObjectExpr = AddressOf->getSubExpr()->IgnoreParenImpCasts(); + return isSupportedMemcpyObjectExpr(ObjectExpr) ? ObjectExpr : nullptr; +} + +static bool isSupportedMemcpyArgType(QualType Type, const ASTContext &Context, + bool RequireMutable) { + if (Type.isNull()) + return false; + + const QualType CanonicalType = Type.getCanonicalType().getNonReferenceType(); + if (CanonicalType.isNull() || CanonicalType->isDependentType() || + CanonicalType->isIncompleteType() || + CanonicalType.isVolatileQualified() || + CanonicalType->isAnyPointerType() || CanonicalType->isArrayType() || + CanonicalType->isFunctionType()) + return false; + + if (RequireMutable) { + if (CanonicalType.isConstQualified()) + return false; + + if (const auto *Record = CanonicalType->getAsCXXRecordDecl()) + if (!Record->hasSimpleCopyAssignment() && + !Record->hasSimpleMoveAssignment()) + return false; + } + + return Type.getNonReferenceType().isTriviallyCopyableType(Context); +} + +static bool isSameUnqualifiedCanonicalType(QualType LHS, QualType RHS) { + return LHS.getCanonicalType().getUnqualifiedType() == + RHS.getCanonicalType().getUnqualifiedType(); +} + +static bool isMatchingSizeOfExpression(const Expr *SizeExpr, QualType SrcType, + QualType DstType, + const ASTContext &Context) { + const auto *UnaryExpr = + dyn_cast<UnaryExprOrTypeTraitExpr>(SizeExpr->IgnoreParenImpCasts()); + if (!UnaryExpr || UnaryExpr->getKind() != UETT_SizeOf || + SizeExpr->getBeginLoc().isMacroID()) + return false; + + const QualType SizeType = UnaryExpr->getTypeOfArgument(); + if (SizeType.isNull()) + return false; + + const QualType SizeCanonical = + SizeType.getCanonicalType().getUnqualifiedType(); + const QualType SrcCanonical = SrcType.getCanonicalType().getUnqualifiedType(); + const QualType DstCanonical = DstType.getCanonicalType().getUnqualifiedType(); + if (SizeCanonical != SrcCanonical && SizeCanonical != DstCanonical) + return false; + + return Context.getTypeSizeInChars(SrcCanonical) == + Context.getTypeSizeInChars(DstCanonical); +} + +static bool isStatementBody(const Stmt *Current, const Stmt *Parent) { + if (const auto *Block = dyn_cast<CompoundStmt>(Parent)) + return llvm::is_contained(Block->body(), Current); + + if (const auto *If = dyn_cast<IfStmt>(Parent)) + return If->getThen() == Current || If->getElse() == Current; + if (const auto *While = dyn_cast<WhileStmt>(Parent)) + return While->getBody() == Current; + if (const auto *Do = dyn_cast<DoStmt>(Parent)) + return Do->getBody() == Current; + if (const auto *For = dyn_cast<ForStmt>(Parent)) + return For->getBody() == Current; + if (const auto *RangeFor = dyn_cast<CXXForRangeStmt>(Parent)) + return RangeFor->getBody() == Current; + if (const auto *Label = dyn_cast<LabelStmt>(Parent)) + return Label->getSubStmt() == Current; + if (const auto *Case = dyn_cast<SwitchCase>(Parent)) + return Case->getSubStmt() == Current; + if (const auto *Attributed = dyn_cast<AttributedStmt>(Parent)) + return Attributed->getSubStmt() == Current; + + return false; +} + +namespace { + +// These states describe how to spell the replacement when only the memcpy call +// is replaced. An existing `(void)` cast is preserved by parenthesizing the +// assignment, while comma/discarded subexpressions need an injected `(void)`. +enum class MemcpyReplacementForm { + None, + StatementBody, + PreserveOuterVoidCast, + InjectVoidCast, +}; + +} // namespace + +static MemcpyReplacementForm getMemcpyReplacementForm(const Expr *ExprNode, + ASTContext &Context) { + const Stmt *Current = ExprNode; + MemcpyReplacementForm Kind = MemcpyReplacementForm::StatementBody; + + while (true) { + auto Parents = Context.getParents(*Current); + if (Parents.size() != 1) + return MemcpyReplacementForm::None; + + if (const auto *ParentExpr = Parents[0].get<Expr>()) { + if (isa<ExprWithCleanups, ImplicitCastExpr, MaterializeTemporaryExpr, + CXXBindTemporaryExpr, ParenExpr>(ParentExpr)) { + Current = ParentExpr; + continue; + } + + if (const auto *Cast = dyn_cast<CastExpr>(ParentExpr)) + if (Cast->getCastKind() == CK_ToVoid) + return MemcpyReplacementForm::PreserveOuterVoidCast; + + if (const auto *Comma = dyn_cast<BinaryOperator>(ParentExpr)) { + if (Comma->getOpcode() != BO_Comma) + return MemcpyReplacementForm::None; + if (Comma->getLHS() == Current) + return MemcpyReplacementForm::InjectVoidCast; + if (Comma->getRHS() == Current) { + Current = Comma; + Kind = MemcpyReplacementForm::InjectVoidCast; + continue; + } + } + + return MemcpyReplacementForm::None; + } + + const auto *ParentStmt = Parents[0].get<Stmt>(); + if (!ParentStmt || !isStatementBody(Current, ParentStmt)) + return MemcpyReplacementForm::None; + return Kind; + } +} + +namespace { + +AST_MATCHER(CallExpr, isDiscardedValueContext) { + return getMemcpyReplacementForm(&Node, Finder->getASTContext()) != + MemcpyReplacementForm::None; +} + +AST_MATCHER(CallExpr, isBitCastMemcpyCandidate) { + if (Node.getNumArgs() != 3 || Node.getBeginLoc().isMacroID()) + return false; + + const auto *DstExpr = extractMemcpyObjectExpr(Node.getArg(0)); + const auto *SrcExpr = extractMemcpyObjectExpr(Node.getArg(1)); + if (!DstExpr || !SrcExpr || DstExpr->getBeginLoc().isMacroID() || + SrcExpr->getBeginLoc().isMacroID()) + return false; + + const auto &Context = Finder->getASTContext(); + const QualType DstType = DstExpr->getType().getNonReferenceType(); + const QualType SrcType = SrcExpr->getType().getNonReferenceType(); + + return isSupportedMemcpyArgType(DstType, Context, /*RequireMutable=*/true) && + isSupportedMemcpyArgType(SrcType, Context, + /*RequireMutable=*/false) && + !isSameUnqualifiedCanonicalType(SrcType, DstType) && + isMatchingSizeOfExpression(Node.getArg(2), SrcType, DstType, Context); +} + +} // namespace + +static StringRef getSourceText(const Expr *ExprNode, const SourceManager &SM, + const LangOptions &LangOpts) { + return Lexer::getSourceText( + CharSourceRange::getTokenRange(ExprNode->getSourceRange()), SM, LangOpts); +} + +UseBitCastCheck::UseBitCastCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + IncludeInserter(Options.getLocalOrGlobal("IncludeStyle", + utils::IncludeSorter::IS_LLVM), + areDiagsSelfContained()) {} + +void UseBitCastCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "IncludeStyle", IncludeInserter.getStyle()); +} + +void UseBitCastCheck::registerPPCallbacks(const SourceManager &SM, + Preprocessor *PP, + Preprocessor *ModuleExpanderPP) { + IncludeInserter.registerPreprocessor(PP); +} + +void UseBitCastCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + callExpr(callee(functionDecl(hasName("::memcpy"))), + isDiscardedValueContext(), unless(isInTemplateInstantiation()), + unless(hasAncestor(expr(matchers::hasUnevaluatedContext()))), + isBitCastMemcpyCandidate()) + .bind("memcpy"), + this); +} + +void UseBitCastCheck::check(const MatchFinder::MatchResult &Result) { + const auto *MemcpyCall = Result.Nodes.getNodeAs<CallExpr>("memcpy"); + if (!MemcpyCall) + return; + + const auto *DstExpr = extractMemcpyObjectExpr(MemcpyCall->getArg(0)); + const auto *SrcExpr = extractMemcpyObjectExpr(MemcpyCall->getArg(1)); + if (!DstExpr || !SrcExpr) + return; + + const SourceManager &SM = *Result.SourceManager; + const LangOptions &LangOpts = getLangOpts(); + StringRef DstText = getSourceText(DstExpr, SM, LangOpts); + StringRef SrcText = getSourceText(SrcExpr, SM, LangOpts); + if (DstText.empty() || SrcText.empty()) + return; + + const MemcpyReplacementForm ReplacementForm = + getMemcpyReplacementForm(MemcpyCall, *Result.Context); + if (ReplacementForm == MemcpyReplacementForm::None) + return; + + const PrintingPolicy Policy(LangOpts); + const QualType DstType = + DstExpr->getType().getNonReferenceType().getUnqualifiedType(); + const std::string Assignment = std::string(DstText) + " = std::bit_cast<" + + DstType.getAsString(Policy) + ">(" + + std::string(SrcText) + ")"; + std::string Replacement = Assignment; + switch (ReplacementForm) { + case MemcpyReplacementForm::StatementBody: + break; + case MemcpyReplacementForm::PreserveOuterVoidCast: + Replacement = "(" + Assignment + ")"; + break; + case MemcpyReplacementForm::InjectVoidCast: + Replacement = "(void)(" + Assignment + ")"; + break; + case MemcpyReplacementForm::None: + return; + } + + const DiagnosticBuilder Diag = + diag(MemcpyCall->getBeginLoc(), + "use 'std::bit_cast' instead of 'memcpy' for type punning"); + Diag << FixItHint::CreateReplacement(MemcpyCall->getSourceRange(), + Replacement); + Diag << IncludeInserter.createIncludeInsertion( + SM.getFileID(MemcpyCall->getBeginLoc()), "<bit>"); +} + +} // namespace clang::tidy::modernize diff --git a/clang-tools-extra/clang-tidy/modernize/UseBitCastCheck.h b/clang-tools-extra/clang-tidy/modernize/UseBitCastCheck.h new file mode 100644 index 0000000000000..c4672a7321d36 --- /dev/null +++ b/clang-tools-extra/clang-tidy/modernize/UseBitCastCheck.h @@ -0,0 +1,44 @@ +//===----------------------------------------------------------------------===// +// +// 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_MODERNIZE_USEBITCASTCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USEBITCASTCHECK_H + +#include "../ClangTidyCheck.h" +#include "../utils/IncludeInserter.h" + +namespace clang::tidy::modernize { + +/// Finds conservative object-to-object ``memcpy`` type punning that can be +/// expressed as ``std::bit_cast``. +/// +/// For the user-facing documentation see: +/// https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-bit-cast.html +class UseBitCastCheck : public ClangTidyCheck { +public: + UseBitCastCheck(StringRef Name, ClangTidyContext *Context); + + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus20; + } + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP, + Preprocessor *ModuleExpanderPP) override; + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + std::optional<TraversalKind> getCheckTraversalKind() const override { + return TK_IgnoreUnlessSpelledInSource; + } + +private: + utils::IncludeInserter IncludeInserter; +}; + +} // namespace clang::tidy::modernize + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USEBITCASTCHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 69dc5b9633398..0cf3c6fdfe6b8 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -139,6 +139,12 @@ New checks Finds common idioms which can be replaced by standard functions from the ``<bit>`` C++20 header. +- New :doc:`modernize-use-bit-cast + <clang-tidy/checks/modernize/use-bit-cast>` check. + + Finds conservative object-to-object ``memcpy`` type punning that can be + rewritten as ``std::bit_cast`` in C++20 and later. + - New :doc:`modernize-use-string-view <clang-tidy/checks/modernize/use-string-view>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 2b5be931271ec..6cb962d1c2705 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -311,6 +311,7 @@ Clang-Tidy Checks :doc:`modernize-type-traits <modernize/type-traits>`, "Yes" :doc:`modernize-unary-static-assert <modernize/unary-static-assert>`, "Yes" :doc:`modernize-use-auto <modernize/use-auto>`, "Yes" + :doc:`modernize-use-bit-cast <modernize/use-bit-cast>`, "Yes" :doc:`modernize-use-bool-literals <modernize/use-bool-literals>`, "Yes" :doc:`modernize-use-constraints <modernize/use-constraints>`, "Yes" :doc:`modernize-use-default-member-init <modernize/use-default-member-init>`, "Yes" diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-bit-cast.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-bit-cast.rst new file mode 100644 index 0000000000000..03f78faf396d6 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-bit-cast.rst @@ -0,0 +1,62 @@ +.. title:: clang-tidy - modernize-use-bit-cast + +modernize-use-bit-cast +====================== + +Finds conservative object-to-object ``memcpy`` type punning that can be +rewritten as ``std::bit_cast`` in C++20 and later. + +The check targets the common pattern of copying the full object representation +of one trivially copyable object into another trivially copyable object of a +different type: + +.. code-block:: c++ + + float src = 1.0f; + unsigned int dst; + std::memcpy(&dst, &src, sizeof(src)); + +This is rewritten to: + +.. code-block:: c++ + + float src = 1.0f; + unsigned int dst; + dst = std::bit_cast<unsigned int>(src); + +The fix intentionally replaces only the ``memcpy`` call. It does not fold a +preceding declaration into ``auto dst = ...`` because doing so can change the +construction behavior of the destination object. + +It only matches direct named source and destination objects, or direct field +subobjects accessed through ``.``, ``->``, ``.*``, or ``->*``, and only when: + +* both object types are trivially copyable, +* neither object type is a pointer, array, function type, or volatile-qualified, +* the source and destination types differ, +* the copy size is expressed as ``sizeof(...)`` for either copied type, and +* the ``memcpy`` call appears in a discarded-value context, such as a statement + body, the operand of an explicit ``(void)`` cast, or a comma subexpression + whose value is discarded. + +The check intentionally does not diagnose: + +* pointer punning, +* array or buffer manipulation, +* macro expansions, +* dependent template cases, +* unevaluated contexts such as ``sizeof(memcpy(...))``, +* larger expressions where the ``memcpy`` value affects the enclosing + expression, such as conditions or operands of unrelated operators, +* calls where the return value of ``memcpy`` is used, or +* unrelated overloads such as a user-defined ``memcpy``. + +If needed, the fix also inserts ``#include <bit>``. + +Options +------- + +.. option:: IncludeStyle + + A string specifying which include-style is used, ``llvm`` or ``google``. + Default is ``llvm``. diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-bit-cast.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-bit-cast.cpp new file mode 100644 index 0000000000000..9820bf6e7987c --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-bit-cast.cpp @@ -0,0 +1,291 @@ +// RUN: %check_clang_tidy -std=c++20-or-later %s modernize-use-bit-cast %t + +// CHECK-FIXES: #include <bit> + +void *memcpy(void *To, const void *From, unsigned long long Size); + +namespace std { +using ::memcpy; +} + +template <typename T> +struct identity { + using type = T; +}; + +struct NonTrivial { + NonTrivial(); + NonTrivial(const NonTrivial &); + int Value; +}; + +extern unsigned long long n; + +void basic_case() { + float src = 1.0f; + unsigned int dst; + std::memcpy(&dst, &src, sizeof(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'std::bit_cast' instead of 'memcpy' for type punning [modernize-use-... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/189962 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
