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

[Roman Lebedev via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG39431e479ffd: [clang-tidy] Introduce misc No Integer To 
Pointer Cast check (authored by lebedev.ri).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91055

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/NoIntToPtrCheck.cpp
  clang-tools-extra/clang-tidy/performance/NoIntToPtrCheck.h
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/performance-no-int-to-ptr.rst
  clang-tools-extra/test/clang-tidy/checkers/performance-no-int-to-ptr.c
  clang-tools-extra/test/clang-tidy/checkers/performance-no-int-to-ptr.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance-no-int-to-ptr.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance-no-int-to-ptr.cpp
@@ -0,0 +1,22 @@
+// RUN: %check_clang_tidy %s performance-no-int-to-ptr %t
+
+// can't implicitly cast int to a pointer.
+// can't use static_cast<>() to cast integer to a pointer.
+// can't use dynamic_cast<>() to cast integer to a pointer.
+// can't use const_cast<>() to cast integer to a pointer.
+
+void *t0(long long int x) {
+  return (void *)x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+
+void *t1(int x) {
+  return reinterpret_cast(x);
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+
+// Don't diagnose casts from integer literals.
+// It's a widely-used technique in embedded/microcontroller/hardware interfacing.
+void *t3(long long int x) {
+  return (void *)0xFEEDFACE;
+}
Index: clang-tools-extra/test/clang-tidy/checkers/performance-no-int-to-ptr.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance-no-int-to-ptr.c
@@ -0,0 +1,66 @@
+// RUN: %check_clang_tidy %s performance-no-int-to-ptr %t
+
+void *t0(char x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+void *t1(signed char x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+void *t2(unsigned char x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+
+void *t3(short x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+void *t4(unsigned short x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+void *t5(signed short x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+
+void *t6(int x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+void *t7(unsigned int x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+void *t8(signed int x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+
+void *t9(long x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+void *t10(unsigned long x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+void *t11(signed long x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+
+void *t12(long long x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+void *t13(unsigned long long x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+void *t14(signed long long x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: 

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D91055#2440344 , @aaron.ballman 
wrote:

> LGTM!

Thank you for the review!


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM!




Comment at: clang-tools-extra/clang-tidy/performance/NoIntToPtrCheck.cpp:21
+  Finder->addMatcher(castExpr(hasCastKind(CK_IntegralToPointer),
+  unless(hasSourceExpression(integerLiteral(
+ .bind("x"),

aaron.ballman wrote:
> Do we also want to exclude the case where the destination is a `volatile` 
> pointer on the assumption that something out of the ordinary is going on. 
> e.g.,
> ```
> intptr_t addr;
> if (something) {
>   addr = SOME_CONSTANT;
> } else {
>   addr = SOME_OTHER_CONSTANT;
> }
> volatile int *register_bank = (volatile int *)addr;
> ```
On reflection, I think we can make an adjustment here if there's user feedback 
that it's a frequent false positive.


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

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 309976.
lebedev.ri marked an inline comment as done.
lebedev.ri added a comment.

@aaron.ballman thank you for taking a look!
Addressing review notes:

- s/inttoptr/int-to-ptr/
- Adjust diagnostic to be more descriptive of the problem


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91055

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/NoIntToPtrCheck.cpp
  clang-tools-extra/clang-tidy/performance/NoIntToPtrCheck.h
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/performance-no-int-to-ptr.rst
  clang-tools-extra/test/clang-tidy/checkers/performance-no-int-to-ptr.c
  clang-tools-extra/test/clang-tidy/checkers/performance-no-int-to-ptr.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance-no-int-to-ptr.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance-no-int-to-ptr.cpp
@@ -0,0 +1,22 @@
+// RUN: %check_clang_tidy %s performance-no-int-to-ptr %t
+
+// can't implicitly cast int to a pointer.
+// can't use static_cast<>() to cast integer to a pointer.
+// can't use dynamic_cast<>() to cast integer to a pointer.
+// can't use const_cast<>() to cast integer to a pointer.
+
+void *t0(long long int x) {
+  return (void *)x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+
+void *t1(int x) {
+  return reinterpret_cast(x);
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+
+// Don't diagnose casts from integer literals.
+// It's a widely-used technique in embedded/microcontroller/hardware interfacing.
+void *t3(long long int x) {
+  return (void *)0xFEEDFACE;
+}
Index: clang-tools-extra/test/clang-tidy/checkers/performance-no-int-to-ptr.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance-no-int-to-ptr.c
@@ -0,0 +1,66 @@
+// RUN: %check_clang_tidy %s performance-no-int-to-ptr %t
+
+void *t0(char x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+void *t1(signed char x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+void *t2(unsigned char x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+
+void *t3(short x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+void *t4(unsigned short x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+void *t5(signed short x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+
+void *t6(int x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+void *t7(unsigned int x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+void *t8(signed int x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+
+void *t9(long x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+void *t10(unsigned long x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+void *t11(signed long x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+
+void *t12(long long x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+void *t13(unsigned long long x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
+}
+void *t14(signed long long x) {
+  return x;
+  // 

[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("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, );
> > ```
> > 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

[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("misc-no-inttoptr");
 CheckFactories.registerCheck("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("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, );
> ```
> 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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

There are a couple of questions from the previous iteration of the review that 
aren't answered yet:

- the diagnostic wording doesn't actually say what's wrong with the code or how 
to fix it; how should users silence the diagnostic if they've found a case 
where it's a false positive for some reason?
- do you expect this check to catch other hidden provenance gotchas like 
grabbing values through printf/scanf, fread, etc)? (Not for this patch 
iteration, just wondering if you expect to extend the check in the future.)




Comment at: clang-tools-extra/clang-tidy/performance/NoIntToPtrCheck.cpp:21
+  Finder->addMatcher(castExpr(hasCastKind(CK_IntegralToPointer),
+  unless(hasSourceExpression(integerLiteral(
+ .bind("x"),

Do we also want to exclude the case where the destination is a `volatile` 
pointer on the assumption that something out of the ordinary is going on. e.g.,
```
intptr_t addr;
if (something) {
  addr = SOME_CONSTANT;
} else {
  addr = SOME_OTHER_CONSTANT;
}
volatile int *register_bank = (volatile int *)addr;
```



Comment at: 
clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp:53
 "performance-no-automatic-move");
+CheckFactories.registerCheck("performance-no-inttoptr");
 CheckFactories.registerCheck(

Does anyone else have opinions on `inttoptr` vs `int-to-ptr`? I feel like the 
latter is easier to read than the former, but I'm wondering if I'm just strange.



Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:15
 
-   `abseil-duration-addition `_, "Yes"
-   `abseil-duration-comparison `_, "Yes"
-   `abseil-duration-conversion-cast `_, 
"Yes"
-   `abseil-duration-division `_, "Yes"
-   `abseil-duration-factory-float `_, "Yes"
-   `abseil-duration-factory-scale `_, "Yes"
-   `abseil-duration-subtraction `_, "Yes"
-   `abseil-duration-unnecessary-conversion 
`_, "Yes"
-   `abseil-faster-strsplit-delimiter 
`_, "Yes"
+   `abseil-duration-addition `_,
+   `abseil-duration-comparison `_,

It looks like there's a whole bunch of unrelated changes happening here?


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

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 308683.
lebedev.ri marked 6 inline comments as done.
lebedev.ri added a comment.
Herald added a subscriber: phosek.

@aaron.ballman @aqjune thank you for taking a look!

Rebased, mostly addressed review notes:

- Moved into `performance` module
- Don't diagnose casts of integer literals


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91055

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/NoIntToPtrCheck.cpp
  clang-tools-extra/clang-tidy/performance/NoIntToPtrCheck.h
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/performance-no-inttoptr.rst
  clang-tools-extra/test/clang-tidy/checkers/performance-no-inttoptr.c
  clang-tools-extra/test/clang-tidy/checkers/performance-no-inttoptr.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance-no-inttoptr.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance-no-inttoptr.cpp
@@ -0,0 +1,22 @@
+// RUN: %check_clang_tidy %s performance-no-inttoptr %t
+
+// can't implicitly cast int to a pointer.
+// can't use static_cast<>() to cast integer to a pointer.
+// can't use dynamic_cast<>() to cast integer to a pointer.
+// can't use const_cast<>() to cast integer to a pointer.
+
+void *t0(long long int x) {
+  return (void *)x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: avoid integer to pointer casts [performance-no-inttoptr]
+}
+
+void *t1(int x) {
+  return reinterpret_cast(x);
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: avoid integer to pointer casts [performance-no-inttoptr]
+}
+
+// Don't diagnose casts from integer literals.
+// It's a widely-used technique in embedded/microcontroller/hardware interfacing.
+void *t3(long long int x) {
+  return (void *)0xFEEDFACE;
+}
Index: clang-tools-extra/test/clang-tidy/checkers/performance-no-inttoptr.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance-no-inttoptr.c
@@ -0,0 +1,66 @@
+// RUN: %check_clang_tidy %s performance-no-inttoptr %t
+
+void *t0(char x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: avoid integer to pointer casts [performance-no-inttoptr]
+}
+void *t1(signed char x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: avoid integer to pointer casts [performance-no-inttoptr]
+}
+void *t2(unsigned char x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: avoid integer to pointer casts [performance-no-inttoptr]
+}
+
+void *t3(short x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: avoid integer to pointer casts [performance-no-inttoptr]
+}
+void *t4(unsigned short x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: avoid integer to pointer casts [performance-no-inttoptr]
+}
+void *t5(signed short x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: avoid integer to pointer casts [performance-no-inttoptr]
+}
+
+void *t6(int x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: avoid integer to pointer casts [performance-no-inttoptr]
+}
+void *t7(unsigned int x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: avoid integer to pointer casts [performance-no-inttoptr]
+}
+void *t8(signed int x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: avoid integer to pointer casts [performance-no-inttoptr]
+}
+
+void *t9(long x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: avoid integer to pointer casts [performance-no-inttoptr]
+}
+void *t10(unsigned long x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: avoid integer to pointer casts [performance-no-inttoptr]
+}
+void *t11(signed long x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: avoid integer to pointer casts [performance-no-inttoptr]
+}
+
+void *t12(long long x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: avoid integer to pointer casts [performance-no-inttoptr]
+}
+void *t13(unsigned long long x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: avoid integer to pointer casts [performance-no-inttoptr]
+}
+void *t14(signed long long x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: avoid integer to pointer casts [performance-no-inttoptr]
+}
Index: clang-tools-extra/docs/clang-tidy/checks/performance-no-inttoptr.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/performance-no-inttoptr.rst
@@ -0,0 +1,45 @@
+.. title:: clang-tidy - performance-no-inttoptr
+
+performance-no-inttoptr
+===
+
+Diagnoses every integer to pointer cast.
+

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

[Juneyoung Lee via Phabricator via cfe-commits]

aqjune added inline comments.



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.
+

aaron.ballman wrote:
> Does this run afoul of the C++ standard's requirements for pointer 
> traceability: http://eel.is/c++draft/basic.stc.dynamic.safety ?
This is fine because the compiled IR is allowed to be more defined than the C++ 
source.

In C++, accessing a pointer that is derived from an integer is UB if it is 
out-of-bounds location of the object pointed by the integer.
In LLVM IR (I mean in the suggested memory model), it is relaxed to have 
well-defined behavior if the pointer is pointing to a different, but in-bounds 
location of an object.
This loses the precision of points-to analysis if inttoptr is involved, but 
makes more arithmetic optimizations on integers valid.


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

[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 , @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("misc-no-inttoptr");
 CheckFactories.registerCheck("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("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, );
```
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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

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


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

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

ping


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added subscribers: jeroen.dobbelaere, jdoerfert, hfinkel.
aaron.ballman added a comment.

In D91055#2382356 , @lebedev.ri wrote:

> CC'ing @rsmith regarding the suggestion/question of also having a clang 
> diagnostic for this.

I'm not Richard, but I have some thoughts just the same. :-) The C committee is 
currently exploring questions of pointer provenance that may have impact in 
this area. You can see the proposed text for a TS on pointer provenance here: 
http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2577.pdf The paper goes into a 
lot of detail about various provenance choices and how those choices impact 
existing code, implementations, optimization opportunities, etc. and my concern 
with adding a provenance check to the Clang frontend at this time is that LLVM 
may want to implement changes from that TS which may cause unfortunate code 
churn for users who enable the warning in Clang. I think keeping this check in 
clang-tidy gives us good utility for catching bugs today, but also gives us a 
safer place to react to changes in the C standard. At some point, I expect the 
question of how to track provenance to settle down within the committee and our 
implementation, and that would be a good time to consider lifting the analysis 
into Clang. CCing @nlopes @hfinkel @jeroen.dobbelaere and @jdoerfert to raise 
awareness on the provenance TS in case it wasn't yet on their radar.


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

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

[Thorsten via Phabricator via cfe-commits]

tschuett added a comment.

I did not consider false positives, but it provides better visibility. It is 
always on.


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

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a subscriber: rsmith.
lebedev.ri added a comment.

CC'ing @rsmith regarding the suggestion/question of also having a clang 
diagnostic for this.


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

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

[Alexander Richardson via Phabricator via cfe-commits]

arichardson added a comment.

Nice! I wonder if this could be an off-by-default (maybe enabled by -Wpedantic) 
warning in clang instead?

Diagnostics like this could be very useful for CHERI since our capabilities are 
in some way tracking provenance  in hardware: we have bounds, permissions and a 
validity bit for all pointers.
All pointers must be derived from another valid pointer (CHERI capability) 
otherwise the validity bit is cleared.
Casting from an integer to a pointer always results in a value without 
provenance (lacking a tag bit in hardware) and will result in traps when 
dereferenced.
We do however treat uintptr_t as carrying provenance (casts retain the same 
permissions+bounds+validity bit in hardware as the original pointer).

Out compiler has some diagnostics to flag int -> pointer casts, so we warn 
about the example code when using long as the intermediate value (since that 
will cause run-time crashes if the value is dereferenced), but we do not warn 
for uintptr_t:
CHERI compiler explorer 


I just checked whether we emit diagnotics for the testscases here and 
discovered that we only diagnose explicit casts from  (non-uintptr_t) int -> 
pointer as lacking provenance, but are missing warnings for the 
-Wint-conversion case. 


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

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D91055#2382112 , @nlopes wrote:

> Nice!

Why thank you.

> BTW, another popular idiom is to store data in the last few bits of the 
> pointer (e.g., LLVM's own PointerIntPair). I guess that one can also be 
> implement by casting the ptr to char* and doing operations over that.

Yep, there is a number of such patterns.
My basic idea behind this patch is that it is very much non-obvious whether

1. such a cast was actually intended to be there
2. the provenance obscurement is intentional

so let's just diagnose everything, and the user can either silence it, or 
rewrite it without the cast.


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

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

[Thorsten via Phabricator via cfe-commits]

tschuett added a comment.

Out of curiosity: why is this in clang-tidy and not in clang?


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

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

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

Nice!
BTW, another popular idiom is to store data in the last few bits of the pointer 
(e.g., LLVM's own PointerIntPair). I guess that one can also be implement by 
casting the ptr to char* and doing operations over that.


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

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri created this revision.
lebedev.ri added reviewers: aaron.ballman, njames93, JonasToth.
lebedev.ri added a project: LLVM.
Herald added subscribers: cfe-commits, xazax.hun, mgorny.
Herald added a project: clang.
lebedev.ri requested review of this revision.

While casting an (integral) pointer to an integer is obvious - you just get
the integral value of the pointer, casting an integer to an (integral) pointer
is deceivingly different. While you will get a pointer with that integral value,
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.

So while (integral) pointer to integer casts are effectively no-ops,
and are transparent to the optimizer, integer to (integral) pointer casts
are *NOT* transparent, and may conceal information from optimizer.

While that may be the intention, it is not always so. For example,
let's take a look at a routine to align the pointer up to the multiple of 16:
The obvious, naive implementation for that is:

  char* src(char* maybe_underbiased_ptr) {
uintptr_t maybe_underbiased_intptr = (uintptr_t)maybe_underbiased_ptr;
uintptr_t aligned_biased_intptr = maybe_underbiased_intptr + 15;
uintptr_t aligned_intptr = aligned_biased_intptr & (~15);
return (char*)aligned_intptr; // warning: avoid integer to pointer casts 
[misc-no-inttoptr]
  }

The check will rightfully diagnose that cast.

But when provenance concealment is not the goal of the code, but an accident,
this example can be rewritten as follows, without using integer to pointer cast:

  char*
  tgt(char* maybe_underbiased_ptr) {
  uintptr_t maybe_underbiased_intptr = (uintptr_t)maybe_underbiased_ptr;
  uintptr_t aligned_biased_intptr = maybe_underbiased_intptr + 15;
  uintptr_t aligned_intptr = aligned_biased_intptr & (~15);
  uintptr_t bias = aligned_intptr - maybe_underbiased_intptr;
  return maybe_underbiased_ptr + bias;
  }

See also:

- D71499 
- Juneyoung Lee, Chung-Kil Hur, Ralf Jung, Zhengyang Liu, John Regehr, and Nuno 
P. Lopes. 2018. Reconciling High-Level Optimizations and Low-Level Code in 
LLVM. Proc. ACM Program. Lang. 2, OOPSLA, Article 125 (November 2018), 28 
pages. 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91055

Files:
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/clang-tidy/misc/NoIntToPtrCheck.cpp
  clang-tools-extra/clang-tidy/misc/NoIntToPtrCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/misc-no-inttoptr.rst
  clang-tools-extra/test/clang-tidy/checkers/misc-no-inttoptr.c
  clang-tools-extra/test/clang-tidy/checkers/misc-no-inttoptr.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc-no-inttoptr.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc-no-inttoptr.cpp
@@ -0,0 +1,16 @@
+// RUN: %check_clang_tidy %s misc-no-inttoptr %t
+
+// can not implicitly cast int to a pointer.
+// can not use static_cast<>() to cast integer to a pointer.
+// can not use dynamic_cast<>() to cast integer to a pointer.
+// can not use const_cast<>() to cast integer to a pointer.
+
+void *t0(long long int x) {
+  return (void *)x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: avoid integer to pointer casts [misc-no-inttoptr]
+}
+
+void *t1(int x) {
+  return reinterpret_cast(x);
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: avoid integer to pointer casts [misc-no-inttoptr]
+}
Index: clang-tools-extra/test/clang-tidy/checkers/misc-no-inttoptr.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc-no-inttoptr.c
@@ -0,0 +1,66 @@
+// RUN: %check_clang_tidy %s misc-no-inttoptr %t
+
+void *t0(char x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: avoid integer to pointer casts [misc-no-inttoptr]
+}
+void *t1(signed char x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: avoid integer to pointer casts [misc-no-inttoptr]
+}
+void *t2(unsigned char x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: avoid integer to pointer casts [misc-no-inttoptr]
+}
+
+void *t3(short x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: avoid integer to pointer casts [misc-no-inttoptr]
+}
+void *t4(unsigned short x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: avoid integer to pointer casts [misc-no-inttoptr]
+}
+void *t5(signed short x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: avoid integer to pointer casts [misc-no-inttoptr]
+}
+
+void *t6(int x) {
+  return x;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: avoid integer to pointer casts [misc-no-inttoptr]
+}