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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
