[PATCH] D82904: [clang-tidy] Warn pointer captured in async block

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-no-escape.rst:13
+
+  .. code-block:: objc
+void foo(__attribute__((noescape)) int *p) {

ellis wrote:
> It looks like I triggered a build failure for the clang-tools docs 
> http://lab.llvm.org:8011/builders/clang-tools-sphinx-docs/builds/62418
> 
> I think the fix is to add a new line here, but I haven't been able to test it 
> locally.
I pushed that fix up before getting your email. I'll keep an eye on the bot to 
ensure the fix worked. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82904



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

[PATCH] D82904: [clang-tidy] Warn pointer captured in async block

[Ellis Hoag via Phabricator via cfe-commits]

ellis marked an inline comment as done.
ellis added inline comments.



Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-no-escape.rst:13
+
+  .. code-block:: objc
+void foo(__attribute__((noescape)) int *p) {

It looks like I triggered a build failure for the clang-tools docs 
http://lab.llvm.org:8011/builders/clang-tools-sphinx-docs/builds/62418

I think the fix is to add a new line here, but I haven't been able to test it 
locally.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82904



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

[PATCH] D82904: [clang-tidy] Warn pointer captured in async block

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman added a comment.

In D82904#2136686 , @ellis wrote:

> Hey @aaron.ballman, thanks for accepting my diff! Would you mind landing my 
> diff since I don't have commit access yet?


Gladly -- I've committed on your behalf in 
dfa0db79d0e37d5cf24a63d1e2b7ba5f40617574 
, thank 
you for the patch!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82904



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

[PATCH] D82904: [clang-tidy] Warn pointer captured in async block

[Ellis Hoag via Phabricator via cfe-commits]

ellis added a comment.

Hey @aaron.ballman, thanks for accepting my diff! Would you mind landing my 
diff since I don't have commit access yet?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82904



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

[PATCH] D82904: [clang-tidy] Warn pointer captured in async block

[Ellis Hoag via Phabricator via cfe-commits]

ellis updated this revision to Diff 276118.
ellis added a comment.

Update commit message


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82904

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-no-escape.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m
@@ -0,0 +1,28 @@
+// RUN: %check_clang_tidy %s bugprone-no-escape %t
+// RUN: %check_clang_tidy %s -assume-filename=bugprone-no-escape.c bugprone-no-escape %t -- -- -fblocks
+
+typedef struct dispatch_queue_s *dispatch_queue_t;
+typedef struct dispatch_time_s *dispatch_time_t;
+typedef void (^dispatch_block_t)(void);
+void dispatch_async(dispatch_queue_t queue, dispatch_block_t block);
+void dispatch_after(dispatch_time_t when, dispatch_queue_t queue, dispatch_block_t block);
+
+extern dispatch_queue_t queue;
+
+void test_noescape_attribute(__attribute__((noescape)) int *p, int *q) {
+  dispatch_async(queue, ^{
+*p = 123;
+// CHECK-MESSAGES: :[[@LINE-2]]:25: warning: pointer 'p' with attribute 'noescape' is captured by an asynchronously-executed block [bugprone-no-escape]
+// CHECK-MESSAGES: :[[@LINE-4]]:30: note: the 'noescape' attribute is declared here.
+  });
+
+  dispatch_after(456, queue, ^{
+*p = 789;
+// CHECK-MESSAGES: :[[@LINE-2]]:30: warning: pointer 'p' with attribute 'noescape' is captured by an asynchronously-executed block [bugprone-no-escape]
+  });
+
+  dispatch_async(queue, ^{
+*q = 0;
+// CHECK-MESSAGES-NOT: :[[@LINE-2]]:25: warning: pointer 'q' with attribute 'noescape' is captured by an asynchronously-executed block
+  });
+}
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
@@ -70,6 +70,7 @@
`bugprone-misplaced-widening-cast `_,
`bugprone-move-forwarding-reference `_, "Yes"
`bugprone-multiple-statement-macro `_,
+   `bugprone-no-escape `_, "Yes"
`bugprone-not-null-terminated-result `_, "Yes"
`bugprone-parent-virtual-call `_, "Yes"
`bugprone-posix-return `_, "Yes"
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-no-escape.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-no-escape.rst
@@ -0,0 +1,18 @@
+.. title:: clang-tidy - bugprone-no-escape
+
+bugprone-no-escape
+==
+
+Finds pointers with the ``noescape`` attribute that are captured by an
+asynchronously-executed block. The block arguments in ``dispatch_async()`` and
+``dispatch_after()`` are guaranteed to escape, so it is an error if a pointer with the
+``noescape`` attribute is captured by one of these blocks.
+
+The following is an example of an invalid use of the ``noescape`` attribute.
+
+  .. code-block:: objc
+void foo(__attribute__((noescape)) int *p) {
+  dispatch_async(queue, ^{
+*p = 123;
+  });
+});
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -94,6 +94,12 @@
   result of a memory allocation function (``malloc()``, ``calloc()``,
   ``realloc()``, ``alloca()``) instead of its argument.
 
+- New :doc:`bugprone-no-escape
+  ` check.
+
+  Finds pointers with the ``noescape`` attribute that are captured by an
+  asynchronously-executed block.
+
 - New :doc:`bugprone-spuriously-wake-up-functions
   ` check.
 
@@ -201,14 +207,14 @@
   Now checks ``std::basic_string_view`` by default.
 
 - Improved :doc:`readability-else-after-return
-  ` check now supports a 
+  ` check now supports a
   `WarnOnConditionVariables` option to control whether to refactor condition
   variables where possible.
 
 - Improved :doc:`readability-identifier-naming
   ` check.
 
-  Now able to rename member references in class template definitions with 
+  Now able to rename member references in class template definitions with
   explicit access.
 
 - Improved :doc:`readability-qualified-auto
Index: clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.h
@@ -0,0 +1,39 @@
+//===--- 

[PATCH] D82904: [clang-tidy] Warn pointer captured in async block

[Ellis Hoag via Phabricator via cfe-commits]

ellis updated this revision to Diff 276095.
ellis added a comment.

Add RUN line to test in C


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82904

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-no-escape.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m
@@ -0,0 +1,28 @@
+// RUN: %check_clang_tidy %s bugprone-no-escape %t
+// RUN: %check_clang_tidy %s -assume-filename=bugprone-no-escape.c bugprone-no-escape %t -- -- -fblocks
+
+typedef struct dispatch_queue_s *dispatch_queue_t;
+typedef struct dispatch_time_s *dispatch_time_t;
+typedef void (^dispatch_block_t)(void);
+void dispatch_async(dispatch_queue_t queue, dispatch_block_t block);
+void dispatch_after(dispatch_time_t when, dispatch_queue_t queue, dispatch_block_t block);
+
+extern dispatch_queue_t queue;
+
+void test_noescape_attribute(__attribute__((noescape)) int *p, int *q) {
+  dispatch_async(queue, ^{
+*p = 123;
+// CHECK-MESSAGES: :[[@LINE-2]]:25: warning: pointer 'p' with attribute 'noescape' is captured by an asynchronously-executed block [bugprone-no-escape]
+// CHECK-MESSAGES: :[[@LINE-4]]:30: note: the 'noescape' attribute is declared here.
+  });
+
+  dispatch_after(456, queue, ^{
+*p = 789;
+// CHECK-MESSAGES: :[[@LINE-2]]:30: warning: pointer 'p' with attribute 'noescape' is captured by an asynchronously-executed block [bugprone-no-escape]
+  });
+
+  dispatch_async(queue, ^{
+*q = 0;
+// CHECK-MESSAGES-NOT: :[[@LINE-2]]:25: warning: pointer 'q' with attribute 'noescape' is captured by an asynchronously-executed block
+  });
+}
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
@@ -70,6 +70,7 @@
`bugprone-misplaced-widening-cast `_,
`bugprone-move-forwarding-reference `_, "Yes"
`bugprone-multiple-statement-macro `_,
+   `bugprone-no-escape `_, "Yes"
`bugprone-not-null-terminated-result `_, "Yes"
`bugprone-parent-virtual-call `_, "Yes"
`bugprone-posix-return `_, "Yes"
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-no-escape.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-no-escape.rst
@@ -0,0 +1,18 @@
+.. title:: clang-tidy - bugprone-no-escape
+
+bugprone-no-escape
+==
+
+Finds pointers with the ``noescape`` attribute that are captured by an
+asynchronously-executed block. The block arguments in ``dispatch_async()`` and
+``dispatch_after()`` are guaranteed to escape, so it is an error if a pointer with the
+``noescape`` attribute is captured by one of these blocks.
+
+The following is an example of an invalid use of the ``noescape`` attribute.
+
+  .. code-block:: objc
+void foo(__attribute__((noescape)) int *p) {
+  dispatch_async(queue, ^{
+*p = 123;
+  });
+});
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -94,6 +94,12 @@
   result of a memory allocation function (``malloc()``, ``calloc()``,
   ``realloc()``, ``alloca()``) instead of its argument.
 
+- New :doc:`bugprone-no-escape
+  ` check.
+
+  Finds pointers with the ``noescape`` attribute that are captured by an
+  asynchronously-executed block.
+
 - New :doc:`bugprone-spuriously-wake-up-functions
   ` check.
 
@@ -201,14 +207,14 @@
   Now checks ``std::basic_string_view`` by default.
 
 - Improved :doc:`readability-else-after-return
-  ` check now supports a 
+  ` check now supports a
   `WarnOnConditionVariables` option to control whether to refactor condition
   variables where possible.
 
 - Improved :doc:`readability-identifier-naming
   ` check.
 
-  Now able to rename member references in class template definitions with 
+  Now able to rename member references in class template definitions with
   explicit access.
 
 - Improved :doc:`readability-qualified-auto
Index: clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.h
@@ -0,0 +1,39 @@
+//===--- 

[PATCH] D82904: [clang-tidy] Warn pointer captured in async block

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m:1
+// RUN: %check_clang_tidy %s bugprone-no-escape %t
+

aaron.ballman wrote:
> Can you add an additional RUN line so we get coverage of the blocks-enabled 
> behavior? Something like:
> ```
> // RUN: %check_clang_tidy %s bugprone-no-escape %t -- -- -fblocks -x c
> ```
> I'm not 100% certain I have that syntax right, but the idea is to run the 
> test as though it were a C compilation unit with blocks explicitly enabled.
check_clang_tidy will add `-fobjc-abi-version=2`, `-fobjc-arc` and `-fblocks` 
if the file extension is `.m` or `.mm`.
You can trick it with `assume-filename` to stop that happening
```
// RUN: %check_clang_tidy %s -assume-filename=bugprone-no-escape.c 
bugprone-no-escape %t -- -- -fblocks
```

Not 100% certain that is the right syntax but that feels like the designed way 
to run the test as a C compilation unit


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82904



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

[PATCH] D82904: [clang-tidy] Warn pointer captured in async block

[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 with a testing request.




Comment at: clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.cpp:34
+  const BlockDecl *EscapingBlockDecl = MatchedEscapingBlock->getBlockDecl();
+  for (const BlockDecl::Capture  : EscapingBlockDecl->captures()) {
+const VarDecl *Var = CapturedVar.getVariable();

ellis wrote:
> aaron.ballman wrote:
> > This makes me think we should extend the `hasAnyCaptures()` AST matcher so 
> > it works with blocks as well as lambdas. It would be nice to do all of this 
> > from the matcher interface.
> Should I add a TODO for this?
Naw, I don't think it's that critical. I'm mostly just surprised we haven't 
done that before now given how similar blocks and lambdas are.



Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m:1
+// RUN: %check_clang_tidy %s bugprone-no-escape %t
+

Can you add an additional RUN line so we get coverage of the blocks-enabled 
behavior? Something like:
```
// RUN: %check_clang_tidy %s bugprone-no-escape %t -- -- -fblocks -x c
```
I'm not 100% certain I have that syntax right, but the idea is to run the test 
as though it were a C compilation unit with blocks explicitly enabled.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82904



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

[PATCH] D82904: [clang-tidy] Warn pointer captured in async block

[Ellis Hoag via Phabricator via cfe-commits]

ellis added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.cpp:34
+  const BlockDecl *EscapingBlockDecl = MatchedEscapingBlock->getBlockDecl();
+  for (const BlockDecl::Capture  : EscapingBlockDecl->captures()) {
+const VarDecl *Var = CapturedVar.getVariable();

aaron.ballman wrote:
> This makes me think we should extend the `hasAnyCaptures()` AST matcher so it 
> works with blocks as well as lambdas. It would be nice to do all of this from 
> the matcher interface.
Should I add a TODO for this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82904



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

[PATCH] D82904: [clang-tidy] Warn pointer captured in async block

[Ellis Hoag via Phabricator via cfe-commits]

ellis updated this revision to Diff 275826.
ellis marked 9 inline comments as done.
ellis added a comment.

Use `LangOpts.Blocks` instead of `LangOpts.ObjC` and add FIXME about grabbing 
the use location of a `CapturedVar`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82904

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-no-escape.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m
@@ -0,0 +1,27 @@
+// RUN: %check_clang_tidy %s bugprone-no-escape %t
+
+typedef struct dispatch_queue_s *dispatch_queue_t;
+typedef struct dispatch_time_s *dispatch_time_t;
+typedef void (^dispatch_block_t)(void);
+void dispatch_async(dispatch_queue_t queue, dispatch_block_t block);
+void dispatch_after(dispatch_time_t when, dispatch_queue_t queue, dispatch_block_t block);
+
+extern dispatch_queue_t queue;
+
+void test_noescape_attribute(__attribute__((noescape)) int *p, int *q) {
+  dispatch_async(queue, ^{
+*p = 123;
+// CHECK-MESSAGES: :[[@LINE-2]]:25: warning: pointer 'p' with attribute 'noescape' is captured by an asynchronously-executed block [bugprone-no-escape]
+// CHECK-MESSAGES: :[[@LINE-4]]:30: note: the 'noescape' attribute is declared here.
+  });
+
+  dispatch_after(456, queue, ^{
+*p = 789;
+// CHECK-MESSAGES: :[[@LINE-2]]:30: warning: pointer 'p' with attribute 'noescape' is captured by an asynchronously-executed block [bugprone-no-escape]
+  });
+
+  dispatch_async(queue, ^{
+*q = 0;
+// CHECK-MESSAGES-NOT: :[[@LINE-2]]:25: warning: pointer 'q' with attribute 'noescape' is captured by an asynchronously-executed block
+  });
+}
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
@@ -70,6 +70,7 @@
`bugprone-misplaced-widening-cast `_,
`bugprone-move-forwarding-reference `_, "Yes"
`bugprone-multiple-statement-macro `_,
+   `bugprone-no-escape `_, "Yes"
`bugprone-not-null-terminated-result `_, "Yes"
`bugprone-parent-virtual-call `_, "Yes"
`bugprone-posix-return `_, "Yes"
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-no-escape.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-no-escape.rst
@@ -0,0 +1,18 @@
+.. title:: clang-tidy - bugprone-no-escape
+
+bugprone-no-escape
+==
+
+Finds pointers with the ``noescape`` attribute that are captured by an
+asynchronously-executed block. The block arguments in ``dispatch_async()`` and
+``dispatch_after()`` are guaranteed to escape, so it is an error if a pointer with the
+``noescape`` attribute is captured by one of these blocks.
+
+The following is an example of an invalid use of the ``noescape`` attribute.
+
+  .. code-block:: objc
+void foo(__attribute__((noescape)) int *p) {
+  dispatch_async(queue, ^{
+*p = 123;
+  });
+});
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -94,6 +94,12 @@
   result of a memory allocation function (``malloc()``, ``calloc()``,
   ``realloc()``, ``alloca()``) instead of its argument.
 
+- New :doc:`bugprone-no-escape
+  ` check.
+
+  Finds pointers with the ``noescape`` attribute that are captured by an
+  asynchronously-executed block.
+
 - New :doc:`bugprone-spuriously-wake-up-functions
   ` check.
 
@@ -201,14 +207,14 @@
   Now checks ``std::basic_string_view`` by default.
 
 - Improved :doc:`readability-else-after-return
-  ` check now supports a 
+  ` check now supports a
   `WarnOnConditionVariables` option to control whether to refactor condition
   variables where possible.
 
 - Improved :doc:`readability-identifier-naming
   ` check.
 
-  Now able to rename member references in class template definitions with 
+  Now able to rename member references in class template definitions with
   explicit access.
 
 - Improved :doc:`readability-qualified-auto
Index: clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.h
@@ -0,0 

[PATCH] D82904: [clang-tidy] Warn pointer captured in async block

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp:82
 "bugprone-bool-pointer-implicit-conversion");
-CheckFactories.registerCheck(
-"bugprone-branch-clone");
+CheckFactories.registerCheck("bugprone-branch-clone");
 CheckFactories.registerCheck(

It looks like unrelated formatting changes may have snuck in?



Comment at: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp:121
 "bugprone-narrowing-conversions");
+CheckFactories.registerCheck("bugprone-no-escape");
 CheckFactories.registerCheck(

Given that this is limited to Objective-C, why register this under `bugprone` 
as opposed to the `objc` module? Alternatively, why limit to Objective-C when 
blocks can be used in other modes like C or C++ with `-fblocks`?



Comment at: clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.cpp:34
+  const BlockDecl *EscapingBlockDecl = MatchedEscapingBlock->getBlockDecl();
+  for (const BlockDecl::Capture  : EscapingBlockDecl->captures()) {
+const VarDecl *Var = CapturedVar.getVariable();

This makes me think we should extend the `hasAnyCaptures()` AST matcher so it 
works with blocks as well as lambdas. It would be nice to do all of this from 
the matcher interface.



Comment at: clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.cpp:37
+if (Var && Var->hasAttr()) {
+  diag(MatchedEscapingBlock->getBeginLoc(),
+   "pointer %0 with attribute 'noescape' is captured by an "

Given that the capture is the issue (not the block), why not point to the use 
of the captured variable?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82904



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

[PATCH] D82904: [clang-tidy] Warn pointer captured in async block

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp:82
 "bugprone-bool-pointer-implicit-conversion");
-CheckFactories.registerCheck(
-"bugprone-branch-clone");
+CheckFactories.registerCheck("bugprone-branch-clone");
 CheckFactories.registerCheck(

ellis wrote:
> aaron.ballman wrote:
> > It looks like unrelated formatting changes may have snuck in?
> The script `add_new_check.py` doesn't format the code that it generated so a 
> few lines aren't linted. Would you like me to undo these extra formatted 
> lines?
Yes, please. The way I usually do this, btw, is to run the patch through 
clang-format-diff so that only the lines I've touched get formatted: 
https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting



Comment at: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp:121
 "bugprone-narrowing-conversions");
+CheckFactories.registerCheck("bugprone-no-escape");
 CheckFactories.registerCheck(

ellis wrote:
> njames93 wrote:
> > aaron.ballman wrote:
> > > Given that this is limited to Objective-C, why register this under 
> > > `bugprone` as opposed to the `objc` module? Alternatively, why limit to 
> > > Objective-C when blocks can be used in other modes like C or C++ with 
> > > `-fblocks`?
> > Thats a good point, maybe restrict this to `LangOptions::ObjC || 
> > LangOptions::Blocks` Then it can be left in bugprone.
> Ok, I'll include `LangOptions::Blocks`.
I think the only option you need is `LangOptions::Blocks` (it should be enabled 
automatically in ObjC language mode).



Comment at: clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.cpp:37
+if (Var && Var->hasAttr()) {
+  diag(MatchedEscapingBlock->getBeginLoc(),
+   "pointer %0 with attribute 'noescape' is captured by an "

ellis wrote:
> aaron.ballman wrote:
> > Given that the capture is the issue (not the block), why not point to the 
> > use of the captured variable?
> I actually agree that pointing to the use of the captured variable would be 
> easier to read, but honestly I couldn't figure out how to grab the location 
> of that use. All I could get was the definition of the variable.
Ah, yeah, I just looked at the class and it looks like we don't track that 
information yet and it would be somewhat involved to get it. Can you add a 
FIXME comment above the call to `diag()` mentioning that we'd prefer to 
diagnose the use of the capture rather than the block?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82904



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

[PATCH] D82904: [clang-tidy] Warn pointer captured in async block

[Ellis Hoag via Phabricator via cfe-commits]

ellis marked an inline comment as done.
ellis added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp:82
 "bugprone-bool-pointer-implicit-conversion");
-CheckFactories.registerCheck(
-"bugprone-branch-clone");
+CheckFactories.registerCheck("bugprone-branch-clone");
 CheckFactories.registerCheck(

aaron.ballman wrote:
> It looks like unrelated formatting changes may have snuck in?
The script `add_new_check.py` doesn't format the code that it generated so a 
few lines aren't linted. Would you like me to undo these extra formatted lines?



Comment at: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp:121
 "bugprone-narrowing-conversions");
+CheckFactories.registerCheck("bugprone-no-escape");
 CheckFactories.registerCheck(

njames93 wrote:
> aaron.ballman wrote:
> > Given that this is limited to Objective-C, why register this under 
> > `bugprone` as opposed to the `objc` module? Alternatively, why limit to 
> > Objective-C when blocks can be used in other modes like C or C++ with 
> > `-fblocks`?
> Thats a good point, maybe restrict this to `LangOptions::ObjC || 
> LangOptions::Blocks` Then it can be left in bugprone.
Ok, I'll include `LangOptions::Blocks`.



Comment at: clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.cpp:37
+if (Var && Var->hasAttr()) {
+  diag(MatchedEscapingBlock->getBeginLoc(),
+   "pointer %0 with attribute 'noescape' is captured by an "

aaron.ballman wrote:
> Given that the capture is the issue (not the block), why not point to the use 
> of the captured variable?
I actually agree that pointing to the use of the captured variable would be 
easier to read, but honestly I couldn't figure out how to grab the location of 
that use. All I could get was the definition of the variable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82904



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

[PATCH] D82904: [clang-tidy] Warn pointer captured in async block

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp:121
 "bugprone-narrowing-conversions");
+CheckFactories.registerCheck("bugprone-no-escape");
 CheckFactories.registerCheck(

aaron.ballman wrote:
> Given that this is limited to Objective-C, why register this under `bugprone` 
> as opposed to the `objc` module? Alternatively, why limit to Objective-C when 
> blocks can be used in other modes like C or C++ with `-fblocks`?
Thats a good point, maybe restrict this to `LangOptions::ObjC || 
LangOptions::Blocks` Then it can be left in bugprone.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82904



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

[PATCH] D82904: [clang-tidy] Warn pointer captured in async block

[Ellis Hoag via Phabricator via cfe-commits]

ellis updated this revision to Diff 274855.
ellis added a comment.

Add `isLanguageVersionSupported` for Objective-C


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82904

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-no-escape.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m
@@ -0,0 +1,27 @@
+// RUN: %check_clang_tidy %s bugprone-no-escape %t
+
+typedef struct dispatch_queue_s *dispatch_queue_t;
+typedef struct dispatch_time_s *dispatch_time_t;
+typedef void (^dispatch_block_t)(void);
+void dispatch_async(dispatch_queue_t queue, dispatch_block_t block);
+void dispatch_after(dispatch_time_t when, dispatch_queue_t queue, dispatch_block_t block);
+
+extern dispatch_queue_t queue;
+
+void test_noescape_attribute(__attribute__((noescape)) int *p, int *q) {
+  dispatch_async(queue, ^{
+*p = 123;
+// CHECK-MESSAGES: :[[@LINE-2]]:25: warning: pointer 'p' with attribute 'noescape' is captured by an asynchronously-executed block [bugprone-no-escape]
+// CHECK-MESSAGES: :[[@LINE-4]]:30: note: the 'noescape' attribute is declared here.
+  });
+
+  dispatch_after(456, queue, ^{
+*p = 789;
+// CHECK-MESSAGES: :[[@LINE-2]]:30: warning: pointer 'p' with attribute 'noescape' is captured by an asynchronously-executed block [bugprone-no-escape]
+  });
+
+  dispatch_async(queue, ^{
+*q = 0;
+// CHECK-MESSAGES-NOT: :[[@LINE-2]]:25: warning: pointer 'q' with attribute 'noescape' is captured by an asynchronously-executed block
+  });
+}
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
@@ -70,6 +70,7 @@
`bugprone-misplaced-widening-cast `_,
`bugprone-move-forwarding-reference `_, "Yes"
`bugprone-multiple-statement-macro `_,
+   `bugprone-no-escape `_, "Yes"
`bugprone-not-null-terminated-result `_, "Yes"
`bugprone-parent-virtual-call `_, "Yes"
`bugprone-posix-return `_, "Yes"
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-no-escape.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-no-escape.rst
@@ -0,0 +1,18 @@
+.. title:: clang-tidy - bugprone-no-escape
+
+bugprone-no-escape
+==
+
+Finds pointers with the ``noescape`` attribute that are captured by an
+asynchronously-executed block. The block arguments in ``dispatch_async()`` and
+``dispatch_after()`` are guaranteed to escape, so it is an error if a pointer with the
+``noescape`` attribute is captured by one of these blocks.
+
+The following is an example of an invalid use of the ``noescape`` attribute.
+
+  .. code-block:: objc
+void foo(__attribute__((noescape)) int *p) {
+  dispatch_async(queue, ^{
+*p = 123;
+  });
+});
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -94,6 +94,12 @@
   result of a memory allocation function (``malloc()``, ``calloc()``,
   ``realloc()``, ``alloca()``) instead of its argument.
 
+- New :doc:`bugprone-no-escape
+  ` check.
+
+  Finds pointers with the ``noescape`` attribute that are captured by an
+  asynchronously-executed block.
+
 - New :doc:`bugprone-spuriously-wake-up-functions
   ` check.
 
@@ -201,14 +207,14 @@
   Now checks ``std::basic_string_view`` by default.
 
 - Improved :doc:`readability-else-after-return
-  ` check now supports a 
+  ` check now supports a
   `WarnOnConditionVariables` option to control whether to refactor condition
   variables where possible.
 
 - Improved :doc:`readability-identifier-naming
   ` check.
 
-  Now able to rename member references in class template definitions with 
+  Now able to rename member references in class template definitions with
   explicit access.
 
 - Improved :doc:`readability-qualified-auto
Index: clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.h
@@ -0,0 +1,39 @@
+//===--- NoEscapeCheck.h - clang-tidy ---*- C++ -*-===//
+//
+// Part of the 

[PATCH] D82904: [clang-tidy] Warn pointer captured in async block

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp:125
+CheckFactories.registerCheck(
+"bugprone-no-escape");
 CheckFactories.registerCheck(

ellis wrote:
> Not sure if we want to lint this line because it was generated by 
> `clang-tidy/add_new_check.py` and every other `registerCheck` line is 
> formatted in the same way.
They should be formatted, but we don't pipe the changes through clang-format in 
the `add_new_check.py` script. only 8/21 clang-tidy module files are 
incorrectly formatted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82904



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

[PATCH] D82904: [clang-tidy] Warn pointer captured in async block

[Ellis Hoag via Phabricator via cfe-commits]

ellis marked an inline comment as done.
ellis added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp:125
+CheckFactories.registerCheck(
+"bugprone-no-escape");
 CheckFactories.registerCheck(

Not sure if we want to lint this line because it was generated by 
`clang-tidy/add_new_check.py` and every other `registerCheck` line is formatted 
in the same way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82904



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

[PATCH] D82904: [clang-tidy] Warn pointer captured in async block

[Ellis Hoag via Phabricator via cfe-commits]

ellis updated this revision to Diff 274622.
ellis added a comment.

Add double backtick to doc


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82904

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-no-escape.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m
@@ -0,0 +1,27 @@
+// RUN: %check_clang_tidy %s bugprone-no-escape %t
+
+typedef struct dispatch_queue_s *dispatch_queue_t;
+typedef struct dispatch_time_s *dispatch_time_t;
+typedef void (^dispatch_block_t)(void);
+void dispatch_async(dispatch_queue_t queue, dispatch_block_t block);
+void dispatch_after(dispatch_time_t when, dispatch_queue_t queue, dispatch_block_t block);
+
+extern dispatch_queue_t queue;
+
+void test_noescape_attribute(__attribute__((noescape)) int *p, int *q) {
+  dispatch_async(queue, ^{
+*p = 123;
+// CHECK-MESSAGES: :[[@LINE-2]]:25: warning: pointer 'p' with attribute 'noescape' is captured by an asynchronously-executed block [bugprone-no-escape]
+// CHECK-MESSAGES: :[[@LINE-4]]:30: note: the 'noescape' attribute is declared here.
+  });
+
+  dispatch_after(456, queue, ^{
+*p = 789;
+// CHECK-MESSAGES: :[[@LINE-2]]:30: warning: pointer 'p' with attribute 'noescape' is captured by an asynchronously-executed block [bugprone-no-escape]
+  });
+
+  dispatch_async(queue, ^{
+*q = 0;
+// CHECK-MESSAGES-NOT: :[[@LINE-2]]:25: warning: pointer 'q' with attribute 'noescape' is captured by an asynchronously-executed block
+  });
+}
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
@@ -70,6 +70,7 @@
`bugprone-misplaced-widening-cast `_,
`bugprone-move-forwarding-reference `_, "Yes"
`bugprone-multiple-statement-macro `_,
+   `bugprone-no-escape `_, "Yes"
`bugprone-not-null-terminated-result `_, "Yes"
`bugprone-parent-virtual-call `_, "Yes"
`bugprone-posix-return `_, "Yes"
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-no-escape.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-no-escape.rst
@@ -0,0 +1,18 @@
+.. title:: clang-tidy - bugprone-no-escape
+
+bugprone-no-escape
+==
+
+Finds pointers with the ``noescape`` attribute that are captured by an
+asynchronously-executed block. The block arguments in ``dispatch_async()`` and
+``dispatch_after()`` are guaranteed to escape, so it is an error if a pointer with the
+``noescape`` attribute is captured by one of these blocks.
+
+The following is an example of an invalid use of the ``noescape`` attribute.
+
+  .. code-block:: objc
+void foo(__attribute__((noescape)) int *p) {
+  dispatch_async(queue, ^{
+*p = 123;
+  });
+});
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -94,6 +94,12 @@
   result of a memory allocation function (``malloc()``, ``calloc()``,
   ``realloc()``, ``alloca()``) instead of its argument.
 
+- New :doc:`bugprone-no-escape
+  ` check.
+
+  Finds pointers with the ``noescape`` attribute that are captured by an
+  asynchronously-executed block.
+
 - New :doc:`bugprone-spuriously-wake-up-functions
   ` check.
 
@@ -201,14 +207,14 @@
   Now checks ``std::basic_string_view`` by default.
 
 - Improved :doc:`readability-else-after-return
-  ` check now supports a 
+  ` check now supports a
   `WarnOnConditionVariables` option to control whether to refactor condition
   variables where possible.
 
 - Improved :doc:`readability-identifier-naming
   ` check.
 
-  Now able to rename member references in class template definitions with 
+  Now able to rename member references in class template definitions with
   explicit access.
 
 - Improved :doc:`readability-qualified-auto
Index: clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.h
@@ -0,0 +1,36 @@
+//===--- NoEscapeCheck.h - clang-tidy ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under 

[PATCH] D82904: [clang-tidy] Warn pointer captured in async block

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:100
+
+  Finds pointers with the `noescape` attribute that are captured by an
+  asynchronously-executed block.

Please use double back-ticks for language constructs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82904



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

[PATCH] D82904: [clang-tidy] Warn pointer captured in async block

[Ellis Hoag via Phabricator via cfe-commits]

ellis updated this revision to Diff 274618.
ellis added a comment.

Applied changes suggested in comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82904

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-no-escape.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m
@@ -0,0 +1,27 @@
+// RUN: %check_clang_tidy %s bugprone-no-escape %t
+
+typedef struct dispatch_queue_s *dispatch_queue_t;
+typedef struct dispatch_time_s *dispatch_time_t;
+typedef void (^dispatch_block_t)(void);
+void dispatch_async(dispatch_queue_t queue, dispatch_block_t block);
+void dispatch_after(dispatch_time_t when, dispatch_queue_t queue, dispatch_block_t block);
+
+extern dispatch_queue_t queue;
+
+void test_noescape_attribute(__attribute__((noescape)) int *p, int *q) {
+  dispatch_async(queue, ^{
+*p = 123;
+// CHECK-MESSAGES: :[[@LINE-2]]:25: warning: pointer 'p' with attribute 'noescape' is captured by an asynchronously-executed block [bugprone-no-escape]
+// CHECK-MESSAGES: :[[@LINE-4]]:30: note: the 'noescape' attribute is declared here.
+  });
+
+  dispatch_after(456, queue, ^{
+*p = 789;
+// CHECK-MESSAGES: :[[@LINE-2]]:30: warning: pointer 'p' with attribute 'noescape' is captured by an asynchronously-executed block [bugprone-no-escape]
+  });
+
+  dispatch_async(queue, ^{
+*q = 0;
+// CHECK-MESSAGES-NOT: :[[@LINE-2]]:25: warning: pointer 'q' with attribute 'noescape' is captured by an asynchronously-executed block
+  });
+}
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
@@ -70,6 +70,7 @@
`bugprone-misplaced-widening-cast `_,
`bugprone-move-forwarding-reference `_, "Yes"
`bugprone-multiple-statement-macro `_,
+   `bugprone-no-escape `_, "Yes"
`bugprone-not-null-terminated-result `_, "Yes"
`bugprone-parent-virtual-call `_, "Yes"
`bugprone-posix-return `_, "Yes"
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-no-escape.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-no-escape.rst
@@ -0,0 +1,18 @@
+.. title:: clang-tidy - bugprone-no-escape
+
+bugprone-no-escape
+==
+
+Finds pointers with the ``noescape`` attribute that are captured by an
+asynchronously-executed block. The block arguments in ``dispatch_async()`` and
+``dispatch_after()`` are guaranteed to escape, so it is an error if a pointer with the
+``noescape`` attribute is captured by one of these blocks.
+
+The following is an example of an invalid use of the ``noescape`` attribute.
+
+  .. code-block:: objc
+void foo(__attribute__((noescape)) int *p) {
+  dispatch_async(queue, ^{
+*p = 123;
+  });
+});
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -94,6 +94,12 @@
   result of a memory allocation function (``malloc()``, ``calloc()``,
   ``realloc()``, ``alloca()``) instead of its argument.
 
+- New :doc:`bugprone-no-escape
+  ` check.
+
+  Finds pointers with the `noescape` attribute that are captured by an
+  asynchronously-executed block.
+
 - New :doc:`bugprone-spuriously-wake-up-functions
   ` check.
 
Index: clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.h
@@ -0,0 +1,36 @@
+//===--- NoEscapeCheck.h - clang-tidy ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_NOESCAPECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_NOESCAPECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Block arguments in `dispatch_async()` and `dispatch_after()` are 

[PATCH] D82904: [clang-tidy] Warn pointer captured in async block

[Nathan James via Phabricator via cfe-commits]

njames93 added subscribers: Eugene.Zelenko, njames93.
njames93 added a comment.

Please override `isLanguageVersionSupported` to restrict this check to just 
running on ObjectiveC translation units.




Comment at: clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.cpp:33
+  Result.Nodes.getNodeAs("arg-block");
+  const auto *EscapingBlockDecl = MatchedEscapingBlock->getBlockDecl();
+  for (const auto CapturedVar : EscapingBlockDecl->captures()) {

Don't use `auto` here as type isn't spelled out explicitly in the initializer.



Comment at: clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.cpp:34
+  const auto *EscapingBlockDecl = MatchedEscapingBlock->getBlockDecl();
+  for (const auto CapturedVar : EscapingBlockDecl->captures()) {
+const VarDecl *Var = CapturedVar.getVariable();

Ditto, also could this be a reference to avoid an unnecessary copy?



Comment at: clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.cpp:38
+  diag(MatchedEscapingBlock->getBeginLoc(),
+   "Pointer '%0' with attribute 'noescape' is captured by an "
+   "asynchronously-executed block")

Convention is warnings start with a lower case letter, same goes for the note 
below.



Comment at: clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.cpp:40
+   "asynchronously-executed block")
+  << Var->getName();
+  diag(Var->getBeginLoc(), "The 'noescape' attribute is declared here.",

You can get rid of `->getName()` and just pass `Var`, The diagnostics builder 
has a special case for `const NamedDecl*`



Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m:25
+*q = 0;
+// CHECK-MESSAGES-NOT: ::[@LINE-2]:{{[0-9]*}}: warning: {{.*}} 
[bugprone-no-escape]
+  });

Be explicit about the column number and warning message. You don't need to 
include the diagnostic name though.
Also the macro should be `[[@LINE-2]]`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82904



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

[PATCH] D82904: [clang-tidy] Warn pointer captured in async block

[Ellis Hoag via Phabricator via cfe-commits]

ellis created this revision.
Herald added subscribers: cfe-commits, xazax.hun, mgorny.
Herald added a project: clang.
ellis added a reviewer: clang-tools-extra.

The block arguments in `dispatch_async()` and `dispatch_after()` are
guaranteed to escape. If those blocks capture any pointers with the
`noescape` attribute then it is an error.

Test Plan:
ninja check-clang-tools


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D82904

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-no-escape.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m
@@ -0,0 +1,27 @@
+// RUN: %check_clang_tidy %s bugprone-no-escape %t
+
+typedef struct dispatch_queue_s *dispatch_queue_t;
+typedef struct dispatch_time_s *dispatch_time_t;
+typedef void (^dispatch_block_t)(void);
+void dispatch_async(dispatch_queue_t queue, dispatch_block_t block);
+void dispatch_after(dispatch_time_t when, dispatch_queue_t queue, dispatch_block_t block);
+
+extern dispatch_queue_t queue;
+
+void test_noescape_attribute(__attribute__((noescape)) int *p, int *q) {
+  dispatch_async(queue, ^{
+*p = 123;
+// CHECK-MESSAGES: :[[@LINE-2]]:25: warning: Pointer 'p' with attribute 'noescape' is captured by an asynchronously-executed block [bugprone-no-escape]
+// CHECK-MESSAGES: :[[@LINE-4]]:30: note: The 'noescape' attribute is declared here.
+  });
+
+  dispatch_after(456, queue, ^{
+*p = 789;
+// CHECK-MESSAGES: :[[@LINE-2]]:30: warning: Pointer 'p' with attribute 'noescape' is captured by an asynchronously-executed block [bugprone-no-escape]
+  });
+
+  dispatch_async(queue, ^{
+*q = 0;
+// CHECK-MESSAGES-NOT: ::[@LINE-2]:{{[0-9]*}}: warning: {{.*}} [bugprone-no-escape]
+  });
+}
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
@@ -70,6 +70,7 @@
`bugprone-misplaced-widening-cast `_,
`bugprone-move-forwarding-reference `_, "Yes"
`bugprone-multiple-statement-macro `_,
+   `bugprone-no-escape `_, "Yes"
`bugprone-not-null-terminated-result `_, "Yes"
`bugprone-parent-virtual-call `_, "Yes"
`bugprone-posix-return `_, "Yes"
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-no-escape.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-no-escape.rst
@@ -0,0 +1,18 @@
+.. title:: clang-tidy - bugprone-no-escape
+
+bugprone-no-escape
+==
+
+Finds pointers with the ``noescape`` attribute that are captured by an
+asynchronously-executed block. The block arguments in ``dispatch_async()`` and
+``dispatch_after()`` are guaranteed to escape, so it is an error if a pointer with the
+``noescape`` attribute is captured by one of these blocks.
+
+The following is an example of an invalid use of the ``noescape`` attribute.
+
+  .. code-block:: objc
+void foo(__attribute__((noescape)) int *p) {
+  dispatch_async(queue, ^{
+*p = 123;
+  });
+});
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -94,6 +94,12 @@
   result of a memory allocation function (``malloc()``, ``calloc()``,
   ``realloc()``, ``alloca()``) instead of its argument.
 
+- New :doc:`bugprone-no-escape
+  ` check.
+
+  Finds pointers with the `noescape` attribute that are captured by an
+  asynchronously-executed block.
+
 - New :doc:`bugprone-spuriously-wake-up-functions
   ` check.
 
Index: clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.h
@@ -0,0 +1,36 @@
+//===--- NoEscapeCheck.h - clang-tidy ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_NOESCAPECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_NOESCAPECHECK_H
+
+#include