[PATCH] D91055: [clang-tidy] Introduce misc No Integer To Pointer Cast check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D91055#2410447 <https://reviews.llvm.org/D91055#2410447>, @lebedev.ri wrote:

> Ping.
> If there's no pushback within next 24h i will commit this and wait for post-commit review (if any).
> Thanks.

FWIW, that's not the way we usually do things, so please don't push and hope for post-commit review on something people have expressed questions/concerns over.



================
Comment at: clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp:40
         "misc-new-delete-overloads");
+    CheckFactories.registerCheck<NoIntToPtrCheck>("misc-no-inttoptr");
     CheckFactories.registerCheck<NoRecursionCheck>("misc-no-recursion");
----------------
Is `misc` really the right place for this? If this is related to optimizer behavior, perhaps it should live in `performance`?


================
Comment at: clang-tools-extra/clang-tidy/misc/NoIntToPtrCheck.cpp:26
+  const auto *MatchedCast = Result.Nodes.getNodeAs<CastExpr>("x");
+  diag(MatchedCast->getBeginLoc(), "avoid integer to pointer casts");
+}
----------------
This doesn't really tell the user what's wrong with their code or how to fix it. There are valid use cases for casting an integer into a pointer, after all. For instance, if I'm doing this cast because I'm trying to read a register bank:
```
volatile int *registers = (volatile int *)0xFEEDFACE;
```
what is wrong with my code and what should I do to silence this diagnostic?

Intuitively, one would think that use of `intptr_t` or `uintptr_t` would have some impact on this given the standard's requirements for that type in C17 7.20.1.4p1. Could there be a certain cast path that can be used to silence the diagnostic because it's safe enough for the optimizer's needs?


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/misc-no-inttoptr.rst:6
+
+Diagnoses every integer to pointer cast.
+
----------------
```
char buffer[16];
sprintf(buffer, "%p", ptr);

intptr_t val;
sscanf(buffer, "%" SCNxPTR, &val);
```
Do you think this check should catch such constructs under the same reasoning as other conversions that hide provenance?


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/misc-no-inttoptr.rst:12
+if you got that integral value via a pointer-to-integer cast originally,
+the new pointer will lack the provenance information from the original pointer.
+
----------------
Does this run afoul of the C++ standard's requirements for pointer traceability: http://eel.is/c++draft/basic.stc.dynamic.safety ?


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/misc-no-inttoptr.cpp:3
+
+// can not implicitly cast int to a pointer.
+// can not use static_cast<>() to cast integer to a pointer.
----------------
can not -> cannot
(same below)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91055/new/

https://reviews.llvm.org/D91055