[PATCH] D71001: [clang-tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Balogh, Ádám via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGfccd0da5ee6f: [clang-tidy] New check: 
bugprone-misplaced-pointer-arithmetic-in-alloc (authored by baloghadamsoftware).

Changed prior to commit:
  https://reviews.llvm.org/D71001?vs=239103=239290#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71001

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  
clang-tools-extra/clang-tidy/bugprone/MisplacedPointerArithmeticInAllocCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/MisplacedPointerArithmeticInAllocCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-misplaced-pointer-arithmetic-in-alloc.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.c
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.cpp
@@ -0,0 +1,53 @@
+// RUN: %check_clang_tidy %s bugprone-misplaced-pointer-arithmetic-in-alloc %t
+
+class C {
+  int num;
+public:
+  explicit C(int n) : num(n) {}
+};
+
+void bad_new(int n, int m) {
+  C *p = new C(n) + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: arithmetic operation is applied to the result of operator new() instead of its size-like argument
+  // CHECK-FIXES: C *p = new C(n + 10);
+
+  p = new C(n) - 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new() instead of its size-like argument
+  // CHECK-FIXES: p = new C(n - 10);
+
+  p = new C(n) + m;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new() instead of its size-like argument
+  // CHECK-FIXES: p = new C(n + m);
+
+  p = new C(n) - (m + 10);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new() instead of its size-like argument
+  // CHECK-FIXES: p = new C(n - (m + 10));
+
+  p = new C(n) - m + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new() instead of its size-like argument
+  // CHECK-FIXES: p = new C(n - m) + 10;
+  // FIXME: Should be p = new C(n - m + 10);
+}
+
+void bad_new_array(int n, int m) {
+  char *p = new char[n] + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: arithmetic operation is applied to the result of operator new[]() instead of its size-like argument
+  // CHECK-FIXES: char *p = new char[n + 10];
+
+  p = new char[n] - 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new[]() instead of its size-like argument
+  // CHECK-FIXES: p = new char[n - 10];
+
+  p = new char[n] + m;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new[]() instead of its size-like argument
+  // CHECK-FIXES: p = new char[n + m];
+
+  p = new char[n] - (m + 10);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new[]() instead of its size-like argument
+  // CHECK-FIXES: p = new char[n - (m + 10)];
+
+  p = new char[n] - m + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new[]() instead of its size-like argument
+  // CHECK-FIXES: p = new char[n - m] + 10;
+  // FIXME: should be p = new char[n - m + 10];
+}
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.c
@@ -0,0 +1,56 @@
+// RUN: %check_clang_tidy %s bugprone-misplaced-pointer-arithmetic-in-alloc %t
+
+typedef __typeof(sizeof(int)) size_t;
+void *malloc(size_t);
+void *alloca(size_t);
+void *calloc(size_t, size_t);
+void *realloc(void *, size_t);
+
+void bad_malloc(int n) {
+  char *p = (char *)malloc(n) + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: arithmetic operation is applied to the result of malloc() instead of its size-like argument
+  // CHECK-FIXES: char *p = (char *)malloc(n + 10);
+
+  p = (char *)malloc(n) - 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of malloc() instead of its size-like argument
+  // CHECK-FIXES: p = (char *)malloc(n - 10);
+
+  p = (char *)malloc(n) + n;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied 

[PATCH] D71001: [clang-tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

In D71001#1830792 , @gribozavr2 wrote:

> Let me know if you want me to commit this change for you.


Thank you! I will commit it today.


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

https://reviews.llvm.org/D71001



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

[PATCH] D71001: [clang-tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added a comment.

Let me know if you want me to commit this change for you.


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

https://reviews.llvm.org/D71001



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

[PATCH] D71001: [clang-tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 239103.
baloghadamsoftware added a comment.

Updated according to the comments.


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

https://reviews.llvm.org/D71001

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  
clang-tools-extra/clang-tidy/bugprone/MisplacedPointerArithmeticInAllocCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/MisplacedPointerArithmeticInAllocCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-misplaced-pointer-arithmetic-in-alloc.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.c
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.cpp
@@ -0,0 +1,53 @@
+// RUN: %check_clang_tidy %s bugprone-misplaced-pointer-arithmetic-in-alloc %t
+
+class C {
+  int num;
+public:
+  explicit C(int n) : num(n) {}
+};
+
+void bad_new(int n, int m) {
+  C *p = new C(n) + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: arithmetic operation is applied to the result of operator new() instead of its size-like argument
+  // CHECK-FIXES: C *p = new C(n + 10);
+
+  p = new C(n) - 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new() instead of its size-like argument
+  // CHECK-FIXES: p = new C(n - 10);
+
+  p = new C(n) + m;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new() instead of its size-like argument
+  // CHECK-FIXES: p = new C(n + m);
+
+  p = new C(n) - (m + 10);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new() instead of its size-like argument
+  // CHECK-FIXES: p = new C(n - (m + 10));
+
+  p = new C(n) - m + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new() instead of its size-like argument
+  // CHECK-FIXES: p = new C(n - m) + 10;
+  // FIXME: Should be p = new C(n - m + 10);
+}
+
+void bad_new_array(int n, int m) {
+  char *p = new char[n] + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: arithmetic operation is applied to the result of operator new[]() instead of its size-like argument
+  // CHECK-FIXES: char *p = new char[n + 10];
+
+  p = new char[n] - 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new[]() instead of its size-like argument
+  // CHECK-FIXES: p = new char[n - 10];
+
+  p = new char[n] + m;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new[]() instead of its size-like argument
+  // CHECK-FIXES: p = new char[n + m];
+
+  p = new char[n] - (m + 10);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new[]() instead of its size-like argument
+  // CHECK-FIXES: p = new char[n - (m + 10)];
+
+  p = new char[n] - m + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new[]() instead of its size-like argument
+  // CHECK-FIXES: p = new char[n - m] + 10;
+  // FIXME: should be p = new char[n - m + 10];
+}
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.c
@@ -0,0 +1,56 @@
+// RUN: %check_clang_tidy %s bugprone-misplaced-pointer-arithmetic-in-alloc %t
+
+typedef __typeof(sizeof(int)) size_t;
+void *malloc(size_t);
+void *alloca(size_t);
+void *calloc(size_t, size_t);
+void *realloc(void *, size_t);
+
+void bad_malloc(int n) {
+  char *p = (char *)malloc(n) + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: arithmetic operation is applied to the result of malloc() instead of its size-like argument
+  // CHECK-FIXES: char *p = (char *)malloc(n + 10);
+
+  p = (char *)malloc(n) - 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of malloc() instead of its size-like argument
+  // CHECK-FIXES: p = (char *)malloc(n - 10);
+
+  p = (char *)malloc(n) + n;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of malloc() instead of its size-like argument
+  // CHECK-FIXES: p = (char *)malloc(n + n);
+
+  p = (char *)malloc(n) - (n + 10);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic 

[PATCH] D71001: [clang-tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked 5 inline comments as done.
baloghadamsoftware added a comment.

Hello,

Thank you for your useful comments. I think I fixed them all now.


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

https://reviews.llvm.org/D71001



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

[PATCH] D71001: [clang-tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added a comment.

Since I seem to be in the minority about thinking that this check does not pull 
its weight, I reviewed the code, and will LGTM and push once the few small 
issues are fixed.




Comment at: 
clang-tools-extra/clang-tidy/bugprone/MisplacedPointerArithmeticInAllocCheck.cpp:36
+
+  return FixItHint::CreateReplacement(
+  Outer->getSourceRange(),

We usually try to make the replacement as small as possible. In the case of 
this checker, the replacement should move the closing parenthesis or the 
closing bracket after the RHS of the binary expression.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/MisplacedPointerArithmeticInAllocCheck.cpp:73
+  const auto CXXConstructorWithSingleIntArg =
+cxxConstructExpr(argumentCountIs(1), hasArgument(0, IntExpr));
+

It does not have to be a single argument, the same typo can happen with the 
last argument of the constructor:

```
A *ptr = new A(a, b, c) + 1; // as written
A *ptr = new A(a, b, c + 1); // should have been
```



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.c:12
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: arithmetic operation is applied 
to the result of malloc() instead of its size-like argument
+  // CHECK-FIXES: {{^  char \*p = \(char \*\)malloc\(n}} + 10{{\);$}}
+

I don't think the check lines in this file need regexes. You can match literal 
text and avoid having to escape special characters:

CHECK-FIXES: char *p = ...;


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

https://reviews.llvm.org/D71001



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

[PATCH] D71001: [clang-tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 238528.
baloghadamsoftware added a comment.

Indentation of the code fixed in the documentation.


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

https://reviews.llvm.org/D71001

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  
clang-tools-extra/clang-tidy/bugprone/MisplacedPointerArithmeticInAllocCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/MisplacedPointerArithmeticInAllocCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-misplaced-pointer-arithmetic-in-alloc.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.c
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.cpp
@@ -0,0 +1,53 @@
+// RUN: %check_clang_tidy %s bugprone-misplaced-pointer-arithmetic-in-alloc %t
+
+class C {
+  int num;
+public:
+  explicit C(int n) : num(n) {}
+};
+
+void bad_new(int n, int m) {
+  C *p = new C(n) + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: arithmetic operation is applied to the result of operator new() instead of its size-like argument
+  // CHECK-FIXES: {{^  C \*p = new C\(n}} + 10{{\);$}}
+
+  p = new C(n) - 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new() instead of its size-like argument
+  // CHECK-FIXES: {{^  p = new C\(n}} - 10{{\);$}}
+
+  p = new C(n) + m;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new() instead of its size-like argument
+  // CHECK-FIXES: {{^  p = new C\(n}} + m{{\);$}}
+
+  p = new C(n) - (m + 10);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new() instead of its size-like argument
+  // CHECK-FIXES: {{^  p = new C\(n}} - (m + 10){{\);$}}
+
+  p = new C(n) - m + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new() instead of its size-like argument
+  // CHECK-FIXES: {{^  p = new C\(n}} - m{{\) \+ 10;$}}
+  // FIXME: Should be {{^  p = new C\(n}} - m + 10{{\);$}}
+}
+
+void bad_new_array(int n, int m) {
+  char *p = new char[n] + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: arithmetic operation is applied to the result of operator new[]() instead of its size-like argument
+  // CHECK-FIXES: {{^  char \*p = new char\[n}} + 10{{\];$}}
+
+  p = new char[n] - 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new[]() instead of its size-like argument
+  // CHECK-FIXES: {{^  p = new char\[n}} - 10{{\];$}}
+
+  p = new char[n] + m;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new[]() instead of its size-like argument
+  // CHECK-FIXES: {{^  p = new char\[n}} + m{{\];$}}
+
+  p = new char[n] - (m + 10);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new[]() instead of its size-like argument
+  // CHECK-FIXES: {{^  p = new char\[n}} - (m + 10){{\];$}}
+
+  p = new char[n] - m + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new[]() instead of its size-like argument
+  // CHECK-FIXES: {{^  p = new char\[n}} - m{{\] \+ 10;$}}
+  // FIXME: should be {{^  p = new char\[n}} - m + 10{{\];$}}
+}
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.c
@@ -0,0 +1,56 @@
+// RUN: %check_clang_tidy %s bugprone-misplaced-pointer-arithmetic-in-alloc %t
+
+typedef __typeof(sizeof(int)) size_t;
+void *malloc(size_t);
+void *alloca(size_t);
+void *calloc(size_t, size_t);
+void *realloc(void *, size_t);
+
+void bad_malloc(int n) {
+  char *p = (char *)malloc(n) + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: arithmetic operation is applied to the result of malloc() instead of its size-like argument
+  // CHECK-FIXES: {{^  char \*p = \(char \*\)malloc\(n}} + 10{{\);$}}
+
+  p = (char *)malloc(n) - 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of malloc() instead of its size-like argument
+  // CHECK-FIXES: {{^  p = \(char \*\)malloc\(n}} - 10{{\);$}}
+
+  p = (char *)malloc(n) + n;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic 

[PATCH] D71001: [clang-tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-misplaced-pointer-arithmetic-in-alloc.rst:15
+
+void bad_malloc(int n) {
+  char *p = (char*) malloc(n) + 10;

Identification. I think 2 character should be enough.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-misplaced-pointer-arithmetic-in-alloc.rst:25
+
+  char *p = (char*) malloc(n + 10);

Identification. I think 2 character should be enough.


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

https://reviews.llvm.org/D71001



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

[PATCH] D71001: [clang-tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 238451.
baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added a comment.

Rebased.


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

https://reviews.llvm.org/D71001

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  
clang-tools-extra/clang-tidy/bugprone/MisplacedPointerArithmeticInAllocCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/MisplacedPointerArithmeticInAllocCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-misplaced-pointer-arithmetic-in-alloc.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.c
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.cpp
@@ -0,0 +1,53 @@
+// RUN: %check_clang_tidy %s bugprone-misplaced-pointer-arithmetic-in-alloc %t
+
+class C {
+  int num;
+public:
+  explicit C(int n) : num(n) {}
+};
+
+void bad_new(int n, int m) {
+  C *p = new C(n) + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: arithmetic operation is applied to the result of operator new() instead of its size-like argument
+  // CHECK-FIXES: {{^  C \*p = new C\(n}} + 10{{\);$}}
+
+  p = new C(n) - 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new() instead of its size-like argument
+  // CHECK-FIXES: {{^  p = new C\(n}} - 10{{\);$}}
+
+  p = new C(n) + m;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new() instead of its size-like argument
+  // CHECK-FIXES: {{^  p = new C\(n}} + m{{\);$}}
+
+  p = new C(n) - (m + 10);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new() instead of its size-like argument
+  // CHECK-FIXES: {{^  p = new C\(n}} - (m + 10){{\);$}}
+
+  p = new C(n) - m + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new() instead of its size-like argument
+  // CHECK-FIXES: {{^  p = new C\(n}} - m{{\) \+ 10;$}}
+  // FIXME: Should be {{^  p = new C\(n}} - m + 10{{\);$}}
+}
+
+void bad_new_array(int n, int m) {
+  char *p = new char[n] + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: arithmetic operation is applied to the result of operator new[]() instead of its size-like argument
+  // CHECK-FIXES: {{^  char \*p = new char\[n}} + 10{{\];$}}
+
+  p = new char[n] - 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new[]() instead of its size-like argument
+  // CHECK-FIXES: {{^  p = new char\[n}} - 10{{\];$}}
+
+  p = new char[n] + m;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new[]() instead of its size-like argument
+  // CHECK-FIXES: {{^  p = new char\[n}} + m{{\];$}}
+
+  p = new char[n] - (m + 10);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new[]() instead of its size-like argument
+  // CHECK-FIXES: {{^  p = new char\[n}} - (m + 10){{\];$}}
+
+  p = new char[n] - m + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new[]() instead of its size-like argument
+  // CHECK-FIXES: {{^  p = new char\[n}} - m{{\] \+ 10;$}}
+  // FIXME: should be {{^  p = new char\[n}} - m + 10{{\];$}}
+}
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.c
@@ -0,0 +1,56 @@
+// RUN: %check_clang_tidy %s bugprone-misplaced-pointer-arithmetic-in-alloc %t
+
+typedef __typeof(sizeof(int)) size_t;
+void *malloc(size_t);
+void *alloca(size_t);
+void *calloc(size_t, size_t);
+void *realloc(void *, size_t);
+
+void bad_malloc(int n) {
+  char *p = (char *)malloc(n) + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: arithmetic operation is applied to the result of malloc() instead of its size-like argument
+  // CHECK-FIXES: {{^  char \*p = \(char \*\)malloc\(n}} + 10{{\);$}}
+
+  p = (char *)malloc(n) - 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of malloc() instead of its size-like argument
+  // CHECK-FIXES: {{^  p = \(char \*\)malloc\(n}} - 10{{\);$}}
+
+  p = (char *)malloc(n) + n;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: 

[PATCH] D71001: [clang-tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/MisplacedPointerArithmeticInAllocCheck.cpp:81-82
+  diag(PtrArith->getBeginLoc(),
+   "pointer arithmetic is applied to the result of %0() instead of its "
+   "argument")
+  << Func->getName() << Hint;

baloghadamsoftware wrote:
> whisperity wrote:
> > If I put the `+ b` on `X` as in `malloc(X + b)` instead of `malloc(X) + b`, 
> > then it's not //pointer// arithmetic anymore, but (hopefully unsigned) 
> > arithmetic. Should the warning message really start with "pointer 
> > arithmetic"?
> > 
> > Maybe you could consider the check saying
> > 
> > arithmetic operation applied to pointer result of ...() instead of 
> > size-like argument
> > 
> > optionally, I'd clarify it further by putting at the end:
> > 
> > resulting in ignoring a prefix of the buffer.
> > 
> > considering you specifically match on the std(-like) allocations. (At least 
> > for now.)
> "resulting in ignoring a prefix of the buffer" <- this is only true for 
> addition. What should we write for subtraction?
You are right about subtraction. I think this message is concise as is.


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

https://reviews.llvm.org/D71001



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

[PATCH] D71001: [clang-tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked 5 inline comments as done.
baloghadamsoftware added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/MisplacedPointerArithmeticInAllocCheck.cpp:81-82
+  diag(PtrArith->getBeginLoc(),
+   "pointer arithmetic is applied to the result of %0() instead of its "
+   "argument")
+  << Func->getName() << Hint;

whisperity wrote:
> If I put the `+ b` on `X` as in `malloc(X + b)` instead of `malloc(X) + b`, 
> then it's not //pointer// arithmetic anymore, but (hopefully unsigned) 
> arithmetic. Should the warning message really start with "pointer arithmetic"?
> 
> Maybe you could consider the check saying
> 
> arithmetic operation applied to pointer result of ...() instead of 
> size-like argument
> 
> optionally, I'd clarify it further by putting at the end:
> 
> resulting in ignoring a prefix of the buffer.
> 
> considering you specifically match on the std(-like) allocations. (At least 
> for now.)
"resulting in ignoring a prefix of the buffer" <- this is only true for 
addition. What should we write for subtraction?


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

https://reviews.llvm.org/D71001



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

[PATCH] D71001: [clang-tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 232573.
baloghadamsoftware added a comment.

Updated according to the comments.


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

https://reviews.llvm.org/D71001

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  
clang-tools-extra/clang-tidy/bugprone/MisplacedPointerArithmeticInAllocCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/MisplacedPointerArithmeticInAllocCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-misplaced-pointer-arithmetic-in-alloc.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.c
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.cpp
@@ -0,0 +1,53 @@
+// RUN: %check_clang_tidy %s bugprone-misplaced-pointer-arithmetic-in-alloc %t
+
+class C {
+  int num;
+public:
+  explicit C(int n) : num(n) {}
+};
+
+void bad_new(int n, int m) {
+  C *p = new C(n) + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: arithmetic operation is applied to the result of operator new() instead of its size-like argument
+  // CHECK-FIXES: {{^  C \*p = new C\(n}} + 10{{\);$}}
+
+  p = new C(n) - 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new() instead of its size-like argument
+  // CHECK-FIXES: {{^  p = new C\(n}} - 10{{\);$}}
+
+  p = new C(n) + m;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new() instead of its size-like argument
+  // CHECK-FIXES: {{^  p = new C\(n}} + m{{\);$}}
+
+  p = new C(n) - (m + 10);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new() instead of its size-like argument
+  // CHECK-FIXES: {{^  p = new C\(n}} - (m + 10){{\);$}}
+
+  p = new C(n) - m + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new() instead of its size-like argument
+  // CHECK-FIXES: {{^  p = new C\(n}} - m{{\) \+ 10;$}}
+  // FIXME: Should be {{^  p = new C\(n}} - m + 10{{\);$}}
+}
+
+void bad_new_array(int n, int m) {
+  char *p = new char[n] + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: arithmetic operation is applied to the result of operator new[]() instead of its size-like argument
+  // CHECK-FIXES: {{^  char \*p = new char\[n}} + 10{{\];$}}
+
+  p = new char[n] - 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new[]() instead of its size-like argument
+  // CHECK-FIXES: {{^  p = new char\[n}} - 10{{\];$}}
+
+  p = new char[n] + m;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new[]() instead of its size-like argument
+  // CHECK-FIXES: {{^  p = new char\[n}} + m{{\];$}}
+
+  p = new char[n] - (m + 10);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new[]() instead of its size-like argument
+  // CHECK-FIXES: {{^  p = new char\[n}} - (m + 10){{\];$}}
+
+  p = new char[n] - m + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of operator new[]() instead of its size-like argument
+  // CHECK-FIXES: {{^  p = new char\[n}} - m{{\] \+ 10;$}}
+  // FIXME: should be {{^  p = new char\[n}} - m + 10{{\];$}}
+}
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.c
@@ -0,0 +1,56 @@
+// RUN: %check_clang_tidy %s bugprone-misplaced-pointer-arithmetic-in-alloc %t
+
+typedef __typeof(sizeof(int)) size_t;
+void *malloc(size_t);
+void *alloca(size_t);
+void *calloc(size_t, size_t);
+void *realloc(void *, size_t);
+
+void bad_malloc(int n) {
+  char *p = (char *)malloc(n) + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: arithmetic operation is applied to the result of malloc() instead of its size-like argument
+  // CHECK-FIXES: {{^  char \*p = \(char \*\)malloc\(n}} + 10{{\);$}}
+
+  p = (char *)malloc(n) - 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is applied to the result of malloc() instead of its size-like argument
+  // CHECK-FIXES: {{^  p = \(char \*\)malloc\(n}} - 10{{\);$}}
+
+  p = (char *)malloc(n) + n;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: arithmetic operation is 

[PATCH] D71001: [clang-tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

While I can't pitch in with actual findings of this check, @MyDeveloperDay, 
you're right in many aspects, including those specific examples //not// firing. 
But an example that actually fires this check indicates a very specific 
**undefined behaviour** case. So if such bogus code (that would trigger on this 
check) did not cause run-time issues so far, it's because they never `free()` 
their memory allocations (really bad?), or their platform is particular enough 
that it allows calling `free` into the buffer, not only for the **start** of 
the buffer.

You must (only at most once) free each and every pointer as they had returned 
from a function of the `*alloc` family. `free(malloc(X))` is okay, but 
`free(malloc(X) + b)` is, as per the rules of the language, clearly undefined 
behaviour. Which of course may just as well include the OS/runtime going "Oh 
the stupid `free`d into this buffer, let me find the beginning of the 
allocation..." until one day it doesn't anymore.

(Snarky "self"-advertisement: Did you check CodeChecker to be that bug 
database?  )




Comment at: 
clang-tools-extra/clang-tidy/bugprone/MisplacedPointerArithmeticInAllocCheck.cpp:23
+  anyOf(hasName("::malloc"), hasName("std::malloc"), hasName("::alloca"),
+hasName("std::alloca"), hasName("::calloc"), 
hasName("std::calloc"),
+hasName("::realloc"), hasName("std::realloc")));

`alloca` is a linux/posix-specific thing, and as such, I don't think it 
//could// be part of the `std` namespace.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/MisplacedPointerArithmeticInAllocCheck.cpp:81-82
+  diag(PtrArith->getBeginLoc(),
+   "pointer arithmetic is applied to the result of %0() instead of its "
+   "argument")
+  << Func->getName() << Hint;

If I put the `+ b` on `X` as in `malloc(X + b)` instead of `malloc(X) + b`, 
then it's not //pointer// arithmetic anymore, but (hopefully unsigned) 
arithmetic. Should the warning message really start with "pointer arithmetic"?

Maybe you could consider the check saying

arithmetic operation applied to pointer result of ...() instead of 
size-like argument

optionally, I'd clarify it further by putting at the end:

resulting in ignoring a prefix of the buffer.

considering you specifically match on the std(-like) allocations. (At least for 
now.)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D71001



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

[PATCH] D71001: [clang-tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Drive by comment: (I have no vested interested other than seeing cool new 
checkers)

FWIW, I work on a system with a multi million line legacy 25+ years old C++ 
codebase, where we now have clang-tidy integrated into a CI system and we have 
> 100,000 firings of clang-tidy warnings (with most non platform specific 
checks turned on),  these get collated into a database based on checker type so 
help us triage them.

I find myself focusing on those checks which have < 10 firings more than those 
that report 1000's, I like very specific checkers, unlike the checkers like 
`readability-convert-member-function-to-static` that tends to report hundreds 
of warnings but the false positive rate is very high. IMHO The more general the 
checker the more false positives there seems to be (just personal observation, 
not backed by data)

When I see new checkers being sent for review I like eyeball (normally with 
grep) some of my core libraries to see what chance could I be hit by the 
checker, in order to assess its usefulness (to me at least)

Just to give you an example, I pulled out 3 malloc's from one directory.

`::malloc(a + b+ sizeof(void *));`
`(malloc((a+ 1) * sizeof(char *)));`
`malloc(sizeof(a) + (unsigned)b)`

Now I don't think any of these mallocs would fire this checkers, but I think 
I'm just a fat finger away from it! Especially if this is the kind of code 
which exists elsewhere in my code base.

We all know writing code like this today isn't the best way, but clang-tidy is 
being used to check code which is hidden deep inside our repos, in places 
people fear to tread or rarely visit or more to the point are too scared to 
visit let alone touch.

I don't think we should measure the usefulness of a checker based just on it 
checking new modern c++ code.

I also don't think we should measure a checker by the number of firings it 
gives. It might only take one bug to cause a pacemaker to stop or a car to 
crash, or to let a hacker in.

If it can happen, it does happen or it has happened, I like the idea of having 
a checker to prevent it.

I'm not a fan of the "maintenance" is a burden argument to prevent new stuff 
coming in. No matter how true it might be. Maintenance of software has been my 
life for 30 years, in fact, I quite enjoy it. And aren't a lot of us here for 
fun and giving our timing freely to explicitly help with that burden?

As we just changed some of our communication systems to encourage new people 
in. Thats not going to work if we beat them back at the door with "its not 
coming in".


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D71001



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

[PATCH] D71001: [Clang-Tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

This might be a strange argument, but I did see this happen several times at 
different companies.

When a company tries to decide which lint tools to use they are doing an 
evaluation. Usually, people doing the evaluation are not compiler/static 
analysis devs. Given the information they have they often end up comparing the 
checks different tools have and do the checkbox game. So if their current tool 
does support some checks that the other tool does not they might have the fear 
of missing out and do not switch the new one.

This methodology is, of course, flawed. They do not know the utility of each 
check, but they do not have the resources to do a proper comparison and they do 
not have the resources to support multiple tools. So one additional 
consideration might be, should clang tidy try to have appeal when those 
comparisons are made?
We could argue on both sides. More users the better, since we can get more bug 
reports, contributors etc. But it might come with additional maintenance costs. 
If a check is very unlikely to have false positives, this cost might be low 
enough to worth it.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D71001



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

[PATCH] D71001: [Clang-Tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D71001#1769048 , @gribozavr2 wrote:

> In D71001#1769035 , @Eugene.Zelenko 
> wrote:
>
> > > ASan can help debug this issue, and more.
> >
> > This is dynamic analysis, and detection of problem depends on test case. 
> > Detection of such problem during static analysis makes sense.
>
>
> As is, this check targets a very narrow pattern, and is very easy to fool 
> with the same code split across multiple statements. I don't think it pulls 
> its weight.


It's not uncommon to ask the author to run the check over several large corpora 
of code to see what the false positive and true positive rates are. Not finding 
any true positives may mean that the check needs a bit more justification (like 
publications talking about the issue, etc).


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D71001



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

[PATCH] D71001: [Clang-Tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In D71001#1769429 , @gribozavr2 wrote:

> In D71001#1769394 , @Eugene.Zelenko 
> wrote:
>
> > With such logic, Clang-tidy is maintenance burden: 368 unaddressed request 
> > in Bugzilla is very telling.
>
>
> Doesn't that just prove the point that we already have a problem with too 
> many bugs in existing checkers, and adding more checkers is only going to 
> make the situation worse?


I don't think that adding new check will hurt Clang-tidy. After all author may 
observe such coding patterns in some code bases. Indeed, it'll be reasonable to 
run this check on LLVM and other big open source projects.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D71001



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

[PATCH] D71001: [Clang-Tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added a comment.

In D71001#1769394 , @Eugene.Zelenko 
wrote:

> With such logic, Clang-tidy is maintenance burden: 368 unaddressed request in 
> Bugzilla is very telling.


Doesn't that just prove the point that we already have a problem with too many 
bugs in existing checkers, and adding more checkers is only going to make the 
situation worse?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D71001



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

[PATCH] D71001: [Clang-Tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In D71001#1769382 , @gribozavr2 wrote:

> In D71001#1769071 , 
> @baloghadamsoftware wrote:
>
> > In D71001#1769018 , @gribozavr2 
> > wrote:
> >
> > > ASan can help debug this issue, and more.
> >
> >
> > ASan is too heavyweight for this simple problem. It does not point out the 
> > source of the issue as quickly as this simple check which also provides a 
> > fix. ASan is meant for the less trivial cases. Is this really such a 
> > performance hit? Clang-Tidy already contains lots of checks which target a 
> > very narrow pattern.
>
>
> It is not just a performance hit. Adding a new checker is primarily a 
> maintenance burden.


With such logic, Clang-tidy is maintenance burden: 368 unaddressed request in 
Bugzilla is very telling.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D71001



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

[PATCH] D71001: [Clang-Tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added a comment.

In D71001#1769071 , 
@baloghadamsoftware wrote:

> In D71001#1769018 , @gribozavr2 
> wrote:
>
> > ASan can help debug this issue, and more.
>
>
> ASan is too heavyweight for this simple problem. It does not point out the 
> source of the issue as quickly as this simple check which also provides a 
> fix. ASan is meant for the less trivial cases. Is this really such a 
> performance hit? Clang-Tidy already contains lots of checks which target a 
> very narrow pattern.


It is not just a performance hit. Adding a new checker is primarily a 
maintenance burden.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D71001



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

[PATCH] D71001: [Clang-Tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

In D71001#1769018 , @gribozavr2 wrote:

> ASan can help debug this issue, and more.


ASan is too heavyweight for this simple problem. It does not point out the 
source of the issue as quickly as this simple check which also provides a fix. 
ASan is meant for the less trivial cases. Is this really such a performance 
hit? Clang-Tidy already contains lots of checks which target a very narrow 
pattern.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D71001



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

[PATCH] D71001: [Clang-Tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

In D71001#1769018 , @gribozavr2 wrote:

> This check is quite limited. For example, if the addition is done in a 
> separate statement, this check wouldn't catch the problem. ASan would.


That is not a simple and common mistake like putting the addition or 
subtraction to the wrong side of the closing parenthesis.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D71001



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

[PATCH] D71001: [Clang-Tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added a comment.

In D71001#1769035 , @Eugene.Zelenko 
wrote:

> > ASan can help debug this issue, and more.
>
> This is dynamic analysis, and detection of problem depends on test case. 
> Detection of such problem during static analysis makes sense.


As is, this check targets a very narrow pattern, and is very easy to fool with 
the same code split across multiple statements. I don't think it pulls its 
weight.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D71001



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

[PATCH] D71001: [Clang-Tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In D71001#1768964 , 
@baloghadamsoftware wrote:

> In D71001#1768704 , @Eugene.Zelenko 
> wrote:
>
> > What about new operator? May be check should have option for custom 
> > allocators?
>
>
> Do you mean `new[]`. Thus e.g. `const *carr = new C[n] + 10;` instead of 
> `const *carr = new C[n + 10];`?


I meant both new and new[].


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D71001



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

[PATCH] D71001: [Clang-Tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

> ASan can help debug this issue, and more.

This is dynamic analysis, and detection of problem depends on test case. 
Detection of such problem during static analysis makes sense.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D71001



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

[PATCH] D71001: [Clang-Tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added a comment.

In D71001#1768963 , 
@baloghadamsoftware wrote:

> In D71001#1768880 , @gribozavr2 
> wrote:
>
> > Is this a common problem? There's a lot of silly code we could try to find, 
> > but if people don't actually write it, then we get all downsides of 
> > maintenance without the benefits of the checker.
>
>
> Oh yes, all our checkers are developed upon user request. They only request 
> it if they find out their developers write such silly code.


But how often is it? Is it just one case?

> And this kind of bug is nasty to debug. (In case of addition less memory is 
> available behind the pointer while the memory ahead of it is lost. In case of 
> subtraction data before the pointer gets overwritten.)

ASan can help debug this issue, and more.

This check is quite limited. For example, if the addition is done in a separate 
statement, this check wouldn't catch the problem. ASan would.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D71001



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

[PATCH] D71001: [Clang-Tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

In D71001#1768704 , @Eugene.Zelenko 
wrote:

> What about new operator? May be check should have option for custom 
> allocators?


Do you mean `new[]`. Thus e.g. `const *carr = new C[n] + 10;` instead of `const 
*carr = new C[n + 10];`?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D71001



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

[PATCH] D71001: [Clang-Tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

In D71001#1768880 , @gribozavr2 wrote:

> Is this a common problem? There's a lot of silly code we could try to find, 
> but if people don't actually write it, then we get all downsides of 
> maintenance without the benefits of the checker.


Oh yes, all our checkers are developed upon user request. They only request it 
if they find out their developers write such silly code. And this kind of bug 
is nasty to debug. (In case of addition less memory is available behind the 
pointer while the memory ahead of it is lost. In case of subtraction data 
before the pointer gets overwritten.)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D71001



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

[PATCH] D71001: [Clang-Tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added a comment.

Is this a common problem? There's a lot of silly code we could try to find, but 
if people don't actually write it, then we get all downsides of maintenance 
without the benefits of the checker.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D71001



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

[PATCH] D71001: [Clang-Tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

What about new operator? May be check should have option for custom allocators?




Comment at: clang-tools-extra/docs/ReleaseNotes.rst:207
 
+- New :doc:`bugprone-misplaced-pointer-arithmetic-in-alloc
+  ` check.

Please move to new checks list in alphabetical order.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:210
+
+  Finds cases where an integer expression is added to the result of a memory
+  allocation function instead of its argument.

Please synchronize with first statement in documentation.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D71001



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

[PATCH] D71001: [Clang-Tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware created this revision.
baloghadamsoftware added reviewers: alexfh, aaron.ballman, gribozavr.
baloghadamsoftware added a project: clang-tools-extra.
Herald added subscribers: mgehre, Szelethus, rnkovacs, xazax.hun, whisperity, 
mgorny.
Herald added a project: clang.

Finds cases where an integer expression is added to the result of a memory 
allocation function instead of its argument.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D71001

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  
clang-tools-extra/clang-tidy/bugprone/MisplacedPointerArithmeticInAllocCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/MisplacedPointerArithmeticInAllocCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-misplaced-pointer-arithmetic-in-alloc.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.c

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-misplaced-pointer-arithmetic-in-alloc.c
@@ -0,0 +1,55 @@
+// RUN: %check_clang_tidy %s bugprone-misplaced-pointer-arithmetic-in-alloc %t
+
+typedef __typeof(sizeof(int)) size_t;
+void *malloc(size_t);
+void *alloca(size_t);
+void *calloc(size_t, size_t);
+void *realloc(void *, size_t);
+
+void bad_malloc(int n) {
+  char *p = (char *)malloc(n) + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: pointer arithmetic is applied to the result of malloc() instead of its argument
+  // CHECK-FIXES: {{^  char \*p = \(char \*\)malloc\(n}} + 10{{\);$}}
+
+  p = (char *)malloc(n) - 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer arithmetic is applied to the result of malloc() instead of its argument
+  // CHECK-FIXES: {{^  p = \(char \*\)malloc\(n}} - 10{{\);$}}
+
+  p = (char *)malloc(n) + n;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer arithmetic is applied to the result of malloc() instead of its argument
+  // CHECK-FIXES: {{^  p = \(char \*\)malloc\(n}} + n{{\);$}}
+
+  p = (char *)malloc(n) - (n + 10);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer arithmetic is applied to the result of malloc() instead of its argument
+  // CHECK-FIXES: {{^  p = \(char \*\)malloc\(n}} - (n + 10){{\);$}}
+
+  p = (char *)malloc(n) - n + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer arithmetic is applied to the result of malloc() instead of its argument
+  // CHECK-FIXES: {{^  p = \(char \*\)malloc\(n}} - n{{\) \+ 10;$}}
+}
+
+void bad_alloca(int n) {
+  char *p = (char *)alloca(n) + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: pointer arithmetic is applied to the result of alloca() instead of its argument
+  // CHECK-FIXES: {{^  char \*p = \(char \*\)alloca\(n}} + 10{{\);$}}
+}
+
+void bad_realloc(char *s, int n) {
+  char *p = (char *)realloc(s, n) + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: pointer arithmetic is applied to the result of realloc() instead of its argument
+  // CHECK-FIXES: {{^  char \*p = \(char \*\)realloc\(s, n}} + 10{{\);$}}
+}
+
+void bad_calloc(int n, int m) {
+  char *p = (char *)calloc(m, n) + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: pointer arithmetic is applied to the result of calloc() instead of its argument
+  // CHECK-FIXES: {{^  char \*p = \(char \*\)calloc\(m, n}} + 10{{\);$}}
+}
+
+void (*(*const alloc_ptr)(size_t)) = malloc;
+
+void bad_indirect_alloc(int n) {
+  char *p = (char *)alloc_ptr(n) + 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: pointer arithmetic is applied to the result of alloc_ptr() instead of its argument
+  // CHECK-FIXES: {{^  char \*p = \(char \*\)alloc_ptr\(n}} + 10{{\);$}}
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -57,6 +57,7 @@
bugprone-macro-parentheses
bugprone-macro-repeated-side-effects
bugprone-misplaced-operator-in-strlen-in-alloc
+   bugprone-misplaced-pointer-arithmetic-in-alloc
bugprone-misplaced-widening-cast
bugprone-move-forwarding-reference
bugprone-multiple-statement-macro
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-misplaced-pointer-arithmetic-in-alloc.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-misplaced-pointer-arithmetic-in-alloc.rst
@@ -0,0 +1,25 @@
+.. title:: clang-tidy - bugprone-misplaced-pointer-arithmetic-in-alloc
+
+bugprone-misplaced-pointer-artithmetic-in-alloc
+===
+
+Finds cases where an integer expression is added to the result