clang-tidy: new bugprone-pointer-cast-widening

[jankratochvil]

Summary

Check for cast of a pointer to wider (even unsigned) integer. This will sign-extend the pointer which happens on 32-bit hosts for 64-bit integers:

void *p=(void *)0x80000000;
printf(  "%p"        "\n",                        p ); // 0x80000000
printf("0x%" PRIxPTR "\n",             (uintptr_t)p ); // 0x80000000
printf("0x%" PRIx64  "\n",
                       reinterpret_cast<uint64_t>(p)); // 0xffffffff80000000
printf("0x%" PRIx64  "\n",             (uint64_t) p ); // 0xffffffff80000000
printf("0x%" PRIx64  "\n",   (uint64_t)(uintptr_t)p ); // 0x80000000

It has caught a bug on ARM32: D71498 (and D71514)

[jankratochvil]

Example output:

PATH="$PWD/bin:$PATH" ~/redhat/llvm-monorepo/clang-tools-extra/clang-tidy/tool/run-clang-tidy.py -checks='-*,bugprone-pointer-cast-widening' /home/jkratoch/redhat/llvm-monorepo/lldb/source/

Enabled checks:
    bugprone-pointer-cast-widening

/home/jkratoch/redhat/llvm-monorepo/lldb/source/Core/PluginManager.cpp:89:35: warning: do not use cast of a pointer 'void *' to non-uintptr_t 'intptr_t' (aka 'long') which may sign-extend [bugprone-pointer-cast-widening]
  return reinterpret_cast<FPtrTy>(reinterpret_cast<intptr_t>(VPtr));
                                  ^
 - What is the use of intptr_t?
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Expression/IRMemoryMap.cpp:333:29: warning: do not use cast of a pointer 'std::__shared_ptr<lldb_private::Process, __gnu_cxx::_S_atomic>::element_type *' (aka 'lldb_private::Process *') to non-uintptr_t 'lldb::addr_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
              __FUNCTION__, (lldb::addr_t)process_sp.get(),
                            ^
 - A bug, submitted as: D71514
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Expression/IRMemoryMap.cpp:582:42: warning: do not use cast of a pointer 'const uint8_t *' (aka 'const unsigned char *') to non-uintptr_t 'uint64_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
              (uint64_t)process_address, (uint64_t)bytes, (uint64_t)size,
                                         ^
 - A bug, not yet submitted.
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Expression/IRMemoryMap.cpp:713:42: warning: do not use cast of a pointer 'uint8_t *' (aka 'unsigned char *') to non-uintptr_t 'uint64_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
              (uint64_t)process_address, (uint64_t)bytes, (uint64_t)size,
                                         ^
 - A bug, not yet submitted.
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Host/common/HostInfoBase.cpp:252:32: warning: do not use cast of a pointer 'bool (*)(lldb_private::FileSpec &)' to non-uintptr_t 'intptr_t' (aka 'long') which may sign-extend [bugprone-pointer-cast-widening]
      reinterpret_cast<void *>(reinterpret_cast<intptr_t>(
                               ^
 - What is the use of intptr_t?
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Host/posix/DomainSocket.cpp:48:20: warning: do not use cast of a pointer 'char *' to non-uintptr_t 'size_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
    saddr_un_len = SUN_LEN(saddr_un);
                   ^
/usr/include/sys/un.h:40:24: note: expanded from macro 'SUN_LEN'
# define SUN_LEN(ptr) ((size_t) (((struct sockaddr_un *) 0)->sun_path)        \
                       ^
 - False positive?
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Expression/IRExecutionUnit.cpp:351:53: warning: do not use cast of a pointer 'void *' to non-uintptr_t 'lldb::addr_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
        function.getName().str().c_str(), external, (lldb::addr_t)fun_ptr));
                                                    ^
 - A bug, submitted as: D71498
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Plugins/ExpressionParser/Clang/IRForTarget.cpp:358:24: warning: do not use cast of a pointer 'clang::NamedDecl *' to non-uintptr_t 'uint64_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
                       reinterpret_cast<uint64_t>(result_decl), false);
                       ^
 - A bug, not yet submitted.
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Plugins/Process/POSIX/CrashReason.cpp:139:23: warning: do not use cast of a pointer 'void *' to non-uintptr_t 'lldb::addr_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
    AppendBounds(str, reinterpret_cast<lldb::addr_t>(info.si_lower),
                      ^
 - A bug, submitted as: D71514
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Plugins/Process/POSIX/CrashReason.cpp:140:18: warning: do not use cast of a pointer 'void *' to non-uintptr_t 'lldb::addr_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
                 reinterpret_cast<lldb::addr_t>(info.si_upper),
                 ^
 - A bug, submitted as: D71514
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Plugins/Process/POSIX/CrashReason.cpp:141:18: warning: do not use cast of a pointer 'void *' to non-uintptr_t 'lldb::addr_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
                 reinterpret_cast<lldb::addr_t>(info.si_addr));
                 ^
 - A bug, submitted as: D71514
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Plugins/Process/POSIX/CrashReason.cpp:147:31: warning: do not use cast of a pointer 'void *' to non-uintptr_t 'lldb::addr_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
                              reinterpret_cast<lldb::addr_t>(info.si_addr));
                              ^
 - A bug, submitted as: D71514
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Utility/Stream.cpp:159:65: warning: do not use cast of a pointer 'const void *' to non-uintptr_t 'ptrdiff_t' (aka 'long') which may sign-extend [bugprone-pointer-cast-widening]
  Printf("0x%.*tx", static_cast<int>(sizeof(const void *)) * 2, (ptrdiff_t)p);
                                                                ^
 - False positive?

Output filtered by my script.

[labath]

Though this *was* my idea, I don't really feel qualified to review code here.

However, some things to consider:

• disallowing casts to intptr_t seems too restrictive -- I doubt many people are doing that, but I guess this type exists for a reason, and since the type (and it's signedness) is spelled out in the source, it shouldn't be too surprising that sign-extension can happen later
• requiring a literal uintptr_t (or a typedef of it) may be also problematic -- the user could obtain an integer type of the same bit width through some other means (e.g. #ifdef). OTOH, without that (and just checking the bit width for instance), one would have to actually compile for a 32-bit target to get this warning. I don't know what's the practice for this in clang-tidy...

[Eugene.Zelenko]

Please add documentation and mention new check in Release Notes.

[jankratochvil]

In D71707#1791280, @labath wrote:

• disallowing casts to intptr_t seems too restrictive -- I doubt many people are doing that, but I guess this type exists for a reason, and since the type (and it's signedness) is spelled out in the source, it shouldn't be too surprising that sign-extension can happen later

I was trying to find what is intptr_t good for and I haven't found any valid reason. It seems to me nobody knows that either. Which is why I find correct to report it. This checker has many false positives (or "not really a bug") anyway.

• requiring a literal uintptr_t (or a typedef of it) may be also problematic -- the user could obtain an integer type of the same bit width through some other means (e.g. #ifdef). OTOH, without that (and just checking the bit width for instance), one would have to actually compile for a 32-bit target to get this warning. I don't know what's the practice for this in clang-tidy...

Yes, I wanted first to check the widths but then I realized user would need a 32-bit host for that which is too difficult to (1) get nowadays and (2) primarily to build there LLVM.

[jankratochvil]

Wrote the documentation and Release Notes entry, thanks for the review.

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

100 ↗

(On Diff #234772)

Checks. One sentence should be enough.

clang-tools-extra/test/clang-tidy/checkers/bugprone-pointer-cast-widening.cpp

6

void is unnecessary.

[jankratochvil]

clang-tools-extra/docs/ReleaseNotes.rst

100 ↗

(On Diff #234772)

Not sure if the wording is OK in such a long sentence (non-native English here).

[aaron.ballman]

In D71707#1791394, @jankratochvil wrote:

In D71707#1791280, @labath wrote:

• disallowing casts to intptr_t seems too restrictive -- I doubt many people are doing that, but I guess this type exists for a reason, and since the type (and it's signedness) is spelled out in the source, it shouldn't be too surprising that sign-extension can happen later

I was trying to find what is intptr_t good for and I haven't found any valid reason. It seems to me nobody knows that either. Which is why I find correct to report it. This checker has many false positives (or "not really a bug") anyway.

I agree that restricting casts to intptr_t is too restrictive. intptr_t is good for all the same things as uintptr_t -- it depends on why you want to use the pointer value as an integer value as to what it is good for.

I'm also concerned about the number of false positives found by this checker, but I think fixing this would fix some of the more egregious false positives.

• requiring a literal uintptr_t (or a typedef of it) may be also problematic -- the user could obtain an integer type of the same bit width through some other means (e.g. #ifdef). OTOH, without that (and just checking the bit width for instance), one would have to actually compile for a 32-bit target to get this warning. I don't know what's the practice for this in clang-tidy...

Yes, I wanted first to check the widths but then I realized user would need a 32-bit host for that which is too difficult to (1) get nowadays and (2) primarily to build there LLVM.

Have you considered language extensions like __ptr32, __ptr64, __sptr, and __uptr (https://docs.microsoft.com/en-us/cpp/cpp/ptr32-ptr64?view=vs-2019) which we also support? I think those probably should factor into this new check as well.

clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.cpp

22–23

Why limiting this to C++ and >= C99? You can get pointer widening casts through extensions in C89, for instance.

25

I think you should use hasCastKind() in this matcher as well.

37

How well does this work with something like std::uinptr_t from <cstdint>?

clang-tools-extra/test/clang-tidy/checkers/bugprone-pointer-cast-widening.cpp

4

You should replicate the contents of the system header rather than including it for tests -- this ensures you are testing the same thing on all platforms.

12

This should not diagnose.

18

I'd also like to see tests for implicit casts as well as pointer width and sign extension language extensions.

[jankratochvil]

In D71707#1796671, @aaron.ballman wrote:

I agree that restricting casts to intptr_t is too restrictive.

Permitted intptr_t. But then implemented also checking a cast of intptr_t to wider integral types.

Have you considered language extensions like __ptr32, __ptr64, __sptr, and __uptr (https://docs.microsoft.com/en-us/cpp/cpp/ptr32-ptr64?view=vs-2019) which we also support?

I think only __uptr support would matter to prevent some false positives. I am not sure it is used anywhere. Should I really implement it?

clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.cpp

22–23

Removed the check completely.

37

It works fine because its

using::uintptr_t;

is recogized as a typedef in the loop above from namespace-less `uintptr_t'. Sure thanks for notifying me to check that.

clang-tools-extra/test/clang-tidy/checkers/bugprone-pointer-cast-widening.cpp

18

Done implicit cases. But I do not understand what you mean by the rest - could you provide an example? Thanks.

[aaron.ballman]

In D71707#1821903, @jankratochvil wrote:

In D71707#1796671, @aaron.ballman wrote:

Have you considered language extensions like __ptr32, __ptr64, __sptr, and __uptr (https://docs.microsoft.com/en-us/cpp/cpp/ptr32-ptr64?view=vs-2019) which we also support?

I think only __uptr support would matter to prevent some false positives. I am not sure it is used anywhere. Should I really implement it?

It's not super common so if it's a problem, you can punt on it. It is used within the Win32 SDK though (I found at least a few uses of it). If you don't want to add support for them right now, you can just throw a FIXME into the code so we remember to add support for it at some point.

clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.cpp

33

str should be Str.

47

static llvm::StringSet<> IntPtrs{"uintptr_t", "intptr_t"};

68

Similar here as above.

clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.h

29

This function doesn't need to be a member function, it doesn't rely on the class state whatsoever. I think the function should not take the container but instead be a templated function accepting iterators.

29–32

These should be findType, checkPointer, and checkIntegral by our usual naming conventions. Also, strset should be StrSet per conventions (same in the definition).

clang-tools-extra/docs/clang-tidy/checks/bugprone-pointer-cast-widening.rst

24 ↗

(On Diff #238260)

This seems outdated?

clang-tools-extra/test/clang-tidy/checkers/bugprone-pointer-cast-widening.cpp

18

I was talking about test cases involving __ptr32 or __uptr, if we wanted to support them.

22

Remove commented out code.

25

Remove commented out code.

33

Can you also show that C-style casts to intptr_t are not diagnosed? e.g., intptr_t ip2 = (inptr_t)p;

38

Remove commented-out code.

[lebedev.ri]

This review seems to be stuck/dead, consider abandoning if no longer relevant.

[jankratochvil]

Thanks for the reminder, I have todolisted it now for myself but for now I am abandoning it as I cannot promise anything.

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MisplacedOperatorInStrlenInAllocCheck.h"
 #include "MisplacedWideningCastCheck.h"
 #include "MoveForwardingReferenceCheck.h"
 #include "MultipleStatementMacroCheck.h"
 #include "NotNullTerminatedResultCheck.h"
 #include "ParentVirtualCallCheck.h"
+#include "PointerCastWideningCheck.h"
 #include "PosixReturnCheck.h"
 #include "SizeofContainerCheck.h"
 #include "SizeofExpressionCheck.h"
 #include "StringConstructorCheck.h"
 #include "StringIntegerAssignmentCheck.h"
 #include "StringLiteralWithEmbeddedNulCheck.h"
 #include "SuspiciousEnumUsageCheck.h"
 #include "SuspiciousMemsetUsageCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<MultipleStatementMacroCheck>(
         "bugprone-multiple-statement-macro");
     CheckFactories.registerCheck<cppcoreguidelines::NarrowingConversionsCheck>(
         "bugprone-narrowing-conversions");
     CheckFactories.registerCheck<NotNullTerminatedResultCheck>(
         "bugprone-not-null-terminated-result");
     CheckFactories.registerCheck<ParentVirtualCallCheck>(
         "bugprone-parent-virtual-call");
+    CheckFactories.registerCheck<PointerCastWideningCheck>(
+        "bugprone-pointer-cast-widening");
     CheckFactories.registerCheck<PosixReturnCheck>(
         "bugprone-posix-return");
     CheckFactories.registerCheck<SizeofContainerCheck>(
         "bugprone-sizeof-container");
     CheckFactories.registerCheck<SizeofExpressionCheck>(
         "bugprone-sizeof-expression");
     CheckFactories.registerCheck<StringConstructorCheck>(
         "bugprone-string-constructor");

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

add_clang_library(clangTidyBugproneModule
   MacroParenthesesCheck.cpp
   MacroRepeatedSideEffectsCheck.cpp
   MisplacedOperatorInStrlenInAllocCheck.cpp
   MisplacedWideningCastCheck.cpp
   MoveForwardingReferenceCheck.cpp
   MultipleStatementMacroCheck.cpp
   NotNullTerminatedResultCheck.cpp
   ParentVirtualCallCheck.cpp
+  PointerCastWideningCheck.cpp
   PosixReturnCheck.cpp
   SizeofContainerCheck.cpp
   SizeofExpressionCheck.cpp
   StringConstructorCheck.cpp
   StringIntegerAssignmentCheck.cpp
   StringLiteralWithEmbeddedNulCheck.cpp
   SuspiciousEnumUsageCheck.cpp
   SuspiciousMemsetUsageCheck.cpp

clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.h

+//===--- PointerCastWideningCheck.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_POINTER_CAST_WIDENING_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_POINTER_CAST_WIDENING_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Check for cast of a pointer to wider (even unsigned) integer. This will
+/// sign-extend the pointer which happens on 32-bit hosts for 64-bit integers.
+///
+/// \code
+/// void *p=(void *)0x80000000;
+/// printf(  "%p"        "\n",                        p ); // 0x80000000
+/// printf("0x%" PRIxPTR "\n",             (uintptr_t)p ); // 0x80000000
+/// printf("0x%" PRIx64  "\n",
+///                        reinterpret_cast<uint64_t>(p)); // 0xffffffff80000000
+/// printf("0x%" PRIx64  "\n",             (uint64_t) p ); // 0xffffffff80000000
+/// printf("0x%" PRIx64  "\n",   (uint64_t)(uintptr_t)p ); // 0x80000000
+/// \endcode
+class PointerCastWideningCheck : public ClangTidyCheck {
+public:
+  PointerCastWideningCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_POINTER_CAST_WIDENING_H

clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.cpp

+//===--- PointerCastWideningCheck.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 "PointerCastWideningCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+void PointerCastWideningCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus && !getLangOpts().C99)
+    return;
+
+  Finder->addMatcher(castExpr(unless(isInTemplateInstantiation())).bind("cast"),
+                     this);
+}
+
+void PointerCastWideningCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedCast = Result.Nodes.getNodeAs<CastExpr>("cast");
+
+  if (MatchedCast->getCastKind() != CK_PointerToIntegral)
+    return;
+
+  QualType DestType = MatchedCast->getType();
+  for (QualType DestTypeCheck = DestType;;) {
+    if (DestTypeCheck.getAsString() == "uintptr_t")
+      return;
+    QualType DestTypeNext =
+        DestTypeCheck.getSingleStepDesugaredType(*Result.Context);
+    if (DestTypeNext == DestTypeCheck)
+      break;
+    DestTypeCheck = DestTypeNext;
+  }
+
+  QualType SourceType = MatchedCast->getSubExpr()->getType();
+
+  diag(MatchedCast->getBeginLoc(), "do not use cast of a pointer %0 to "
+                                   "non-uintptr_t %1 which may sign-extend")
+      << SourceType << DestType;
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

clang-tools-extra/test/clang-tidy/checkers/bugprone-pointer-cast-widening.cpp

+// RUN: %check_clang_tidy %s bugprone-pointer-cast-widening %t
+
+#include <stdint.h>
+
+void test(void) {
+  void *p = 0;
+  uint64_t u64 = (uint64_t)p;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: do not use cast of a pointer 'void *' to non-uintptr_t 'uint64_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
+  uint64_t u64r = reinterpret_cast<uint64_t>(p);
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: do not use cast of a pointer 'void *' to non-uintptr_t 'uint64_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
+  intptr_t ip = reinterpret_cast<intptr_t>(p);
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: do not use cast of a pointer 'void *' to non-uintptr_t 'intptr_t' (aka 'long') which may sign-extend [bugprone-pointer-cast-widening]
+  uintptr_t up = (uintptr_t)p;
+  typedef uintptr_t t1;
+  typedef t1 t2;
+  t2 t = (t2)p;
+}