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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
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");
+}
----------------
lebedev.ri wrote:
> 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
Okay, thank you for the explanation. I think it's fine to not have a way to silence the diagnostic (users can use // NOLINT, pragmas, etc), but I think the diagnostic should be made a bit more verbose so it's clear why. How about: `integer to pointer cast pessimizes optimization opportunities`?


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/misc-no-inttoptr.rst:6
+
+Diagnoses every integer to pointer cast.
+
----------------
lebedev.ri wrote:
> 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.
Okay, I think the check is fine as-is then. This isn't a "catch all the ways users can lose provenance information in a way that matters" check, it's a "catch the most common ways users do that" check. If we need a stronger variant of the check, we can add it to the static analyzer at that point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91055