[clang] [clang-tools-extra] [clang][NFC] Fix example code for memberPointerType() AST matcher (PR #109403)

via cfe-commits cfe-commits at lists.llvm.org
Fri Sep 20 04:33:34 PDT 2024


Carlos =?utf-8?q?Gálvez?= <carlos.galvez at zenseact.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/109403 at github.com>


llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang-tools-extra

Author: Carlos Galvez (carlosgalvezp)

<details>
<summary>Changes</summary>

The example code doesn't compile otherwise.


---
Full diff: https://github.com/llvm/llvm-project/pull/109403.diff


9 Files Affected:

- (added) clang-tools-extra/clang-tidy/bugprone/BitCastPointersCheck.cpp (+30) 
- (added) clang-tools-extra/clang-tidy/bugprone/BitCastPointersCheck.h (+34) 
- (modified) clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp (+3) 
- (modified) clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt (+1) 
- (modified) clang-tools-extra/docs/ReleaseNotes.rst (+6) 
- (added) clang-tools-extra/docs/clang-tidy/checks/bugprone/bit-cast-pointers.rst (+39) 
- (added) clang-tools-extra/test/clang-tidy/checkers/bugprone/bit-cast-pointers.cpp (+38) 
- (modified) clang/docs/LibASTMatchersReference.html (+2-4) 
- (modified) clang/include/clang/ASTMatchers/ASTMatchers.h (+2-2) 


``````````diff
diff --git a/clang-tools-extra/clang-tidy/bugprone/BitCastPointersCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/BitCastPointersCheck.cpp
new file mode 100644
index 00000000000000..4589a35a567552
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/BitCastPointersCheck.cpp
@@ -0,0 +1,30 @@
+//===--- BitCastPointersCheck.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 "BitCastPointersCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+void BitCastPointersCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      callExpr(callee(functionDecl(allOf(hasName("::std::bit_cast"),
+                                         hasAnyTemplateArgument(refersToType(
+                                             qualType(isAnyPointer())))))))
+          .bind("x"),
+      this);
+}
+
+void BitCastPointersCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *MatchedDecl = Result.Nodes.getNodeAs<CallExpr>("x"))
+    diag(MatchedDecl->getBeginLoc(), "do not use std::bit_cast on pointers; use reinterpret_cast instead");
+}
+
+} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/clang-tidy/bugprone/BitCastPointersCheck.h b/clang-tools-extra/clang-tidy/bugprone/BitCastPointersCheck.h
new file mode 100644
index 00000000000000..2083979e527fd3
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/BitCastPointersCheck.h
@@ -0,0 +1,34 @@
+//===--- BitCastPointersCheck.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_BITCASTPOINTERSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_BITCASTPOINTERSCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::bugprone {
+
+/// Warns about usage of ``std::bit_cast`` when either the input or output types
+/// are pointers.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/bit-cast-pointers.html
+class BitCastPointersCheck : public ClangTidyCheck {
+public:
+  BitCastPointersCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus20;
+  }
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_BITCASTPOINTERSCHECK_H
diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
index 689eb92a3d8d17..931624224d784b 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -14,6 +14,7 @@
 #include "AssertSideEffectCheck.h"
 #include "AssignmentInIfConditionCheck.h"
 #include "BadSignalToKillThreadCheck.h"
+#include "BitCastPointersCheck.h"
 #include "BoolPointerImplicitConversionCheck.h"
 #include "BranchCloneCheck.h"
 #include "CastingThroughVoidCheck.h"
@@ -108,6 +109,8 @@ class BugproneModule : public ClangTidyModule {
         "bugprone-assignment-in-if-condition");
     CheckFactories.registerCheck<BadSignalToKillThreadCheck>(
         "bugprone-bad-signal-to-kill-thread");
+    CheckFactories.registerCheck<BitCastPointersCheck>(
+        "bugprone-bit-cast-pointers");
     CheckFactories.registerCheck<BoolPointerImplicitConversionCheck>(
         "bugprone-bool-pointer-implicit-conversion");
     CheckFactories.registerCheck<BranchCloneCheck>("bugprone-branch-clone");
diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
index cb0d8ae98bac58..55766b3d9bf91c 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
@@ -8,6 +8,7 @@ add_clang_library(clangTidyBugproneModule
   AssertSideEffectCheck.cpp
   AssignmentInIfConditionCheck.cpp
   BadSignalToKillThreadCheck.cpp
+  BitCastPointersCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
   BranchCloneCheck.cpp
   BugproneTidyModule.cpp
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 8d028f8863cb7a..61c3b0fba61c99 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -98,6 +98,12 @@ Improvements to clang-tidy
 New checks
 ^^^^^^^^^^
 
+- New :doc:`bugprone-bit-cast-pointers
+  <clang-tidy/checks/bugprone/bit-cast-pointers>` check.
+
+  Warns about usage of ``std::bit_cast`` when either the input or output types
+  are pointers.
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/bit-cast-pointers.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/bit-cast-pointers.rst
new file mode 100644
index 00000000000000..93b2caf224d6f9
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/bit-cast-pointers.rst
@@ -0,0 +1,39 @@
+.. title:: clang-tidy - bugprone-bit-cast-pointers
+
+bugprone-bit-cast-pointers
+==========================
+
+Warns about usage of ``std::bit_cast`` when either the input or output types
+are pointers.
+
+The motivation is that ``std::bit_cast`` is advertised as the safe alternative
+to type punning via ``reinterpret_cast`` in modern C++. However, one should not
+blindly replace ``reinterpret_cast`` with ``std::bit_cast``, as follows:
+
+.. code-block:: c++
+
+    int x{};
+    -float y = *reinterpret_cast<float*>(&x);
+    +float y = *std::bit_cast<float*>(&x);
+
+The drop-in replacement behaves exactly the same as ``reinterpret_cast``, and
+Undefined Behavior is still invoked. ``std::bit_cast`` is copying the bytes of
+the input pointer, not the pointee, into an output pointer of a different type,
+which violates the strict aliasing rules. However, simply looking at the code,
+it looks "safe", because it uses ``std::bit_cast`` which is advertised as safe.
+
+The solution to safe type punning is to apply ``std::bit_cast`` on value types,
+not on pointer types:
+
+.. code-block:: c++
+
+    int x{};
+    float y = std::bit_cast<float>(x);
+
+This way, the bytes of the input object are copied into the output object, which
+is safe from Undefined Behavior. Compilers are able to optimize this copy and
+generate identical assembly to the original ``reinterpret_cast`` version.
+
+Alternatively, if a cast between pointers is truly wanted, ``reinterpret_cast``
+should be used, to clearly convey intent and enable warnings from compilers and
+linters, which should be addressed accordingly.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/bit-cast-pointers.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/bit-cast-pointers.cpp
new file mode 100644
index 00000000000000..284149633049fb
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/bit-cast-pointers.cpp
@@ -0,0 +1,38 @@
+// RUN: %check_clang_tidy -std=c++20-or-later %s bugprone-bit-cast-pointers %t
+
+namespace std
+{
+template <typename To, typename From>
+To bit_cast(From from)
+{
+  // Dummy implementation for the purpose of the check.
+  // We don't want to include <cstring> to get std::memcpy.
+  To to{};
+  return to;
+}
+}
+
+void pointer2pointer()
+{
+  int x{};
+  float bad = *std::bit_cast<float*>(&x);
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: do not use std::bit_cast on pointers; use reinterpret_cast instead [bugprone-bit-cast-pointers]
+  float good = *reinterpret_cast<float*>(&x);
+  float good2 = std::bit_cast<float>(x);
+}
+
+void int2pointer()
+{
+  unsigned long long addr{};
+  float* bad = std::bit_cast<float*>(addr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: do not use std::bit_cast on pointers; use reinterpret_cast instead [bugprone-bit-cast-pointers]
+  float* good = reinterpret_cast<float*>(addr);
+}
+
+void pointer2int()
+{
+  int x{};
+  auto bad = std::bit_cast<unsigned long long>(&x);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: do not use std::bit_cast on pointers; use reinterpret_cast instead [bugprone-bit-cast-pointers]
+  auto good = reinterpret_cast<unsigned long long>(&x);
+}
diff --git a/clang/docs/LibASTMatchersReference.html b/clang/docs/LibASTMatchersReference.html
index a16b9c44ef0eab..30695efc3e304e 100644
--- a/clang/docs/LibASTMatchersReference.html
+++ b/clang/docs/LibASTMatchersReference.html
@@ -2675,8 +2675,8 @@ <h2 id="decl-matchers">Node Matchers</h2>
 <tr><td>Matcher<<a href="https://clang.llvm.org/doxygen/classclang_1_1Type.html">Type</a>></td><td class="name" onclick="toggle('memberPointerType0')"><a name="memberPointerType0Anchor">memberPointerType</a></td><td>Matcher<<a href="https://clang.llvm.org/doxygen/classclang_1_1MemberPointerType.html">MemberPointerType</a>>...</td></tr>
 <tr><td colspan="4" class="doc" id="memberPointerType0"><pre>Matches member pointer types.
 Given
-  struct A { int i; }
-  A::* ptr = A::i;
+  struct A { int i; };
+  int A::* ptr = &A::i;
 memberPointerType()
   matches "A::* ptr"
 </pre></td></tr>
@@ -10659,5 +10659,3 @@ <h2 id="traversal-matchers">AST Traversal Matchers</h2>
 </div>
 </body>
 </html>
-
-
diff --git a/clang/include/clang/ASTMatchers/ASTMatchers.h b/clang/include/clang/ASTMatchers/ASTMatchers.h
index f1c72efc238784..61877c435208fa 100644
--- a/clang/include/clang/ASTMatchers/ASTMatchers.h
+++ b/clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -7327,8 +7327,8 @@ extern const AstTypeMatcher<BlockPointerType> blockPointerType;
 /// Matches member pointer types.
 /// Given
 /// \code
-///   struct A { int i; }
-///   A::* ptr = A::i;
+///   struct A { int i; };
+///   int A::* ptr = &A::i;
 /// \endcode
 /// memberPointerType()
 ///   matches "A::* ptr"

``````````

</details>


https://github.com/llvm/llvm-project/pull/109403


More information about the cfe-commits mailing list