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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Ugh, i really hate that inline comments aren't submitted upon uploading the new diff, it catches me off-guard each time...



================
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");
----------------
aaron.ballman wrote:
> Is `misc` really the right place for this? If this is related to optimizer behavior, perhaps it should live in `performance`?
Yeah, i'm not sure. `performance` does seem like a marginally better place for this, so let's go with that.


================
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");
+}
----------------
aaron.ballman wrote:
> 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?
A cast from literal is a good point, we should not warn on those,
especially because those are most likely some predefined hardcoded hw registers.

> 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?

Do you mean
```
uintptr_t x = <>;
void *y = (void*)(x)
```
or
```
void *x = <>;
void *y = (void*)((uintptr_t)x)
```

The former is literally the case this check should warn on.
As for the latter, also not really, because like i have already stated,
such cast pair is used to be treated as opaque (and dropped by the optimizer),
but that is changing.

TLDR: no


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/misc-no-inttoptr.rst:6
+
+Diagnoses every integer to pointer cast.
+
----------------
aaron.ballman wrote:
> ```
> 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?
I'm open to suggestions as to how to title it more precisely given the idea underneath.

To recap, the check should warn on all the casts that may involve an unintentional,
subtle, unobvious cast from an integer to a pointer.
That example doesn't strike me as potentially unintentional,
while `(void*)((uintptr_t)x & -16)` does.

The obvious cast i guess i'd be fine with would be `synthesize_pointer_from_integer_cast<>()`,
but there isn't one now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91055