[clang-tidy] Warn pointer captured in async block

[ellis]

Summary

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

[njames93]

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

clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.cpp

34

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

35

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

39

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

41

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

clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m

26

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

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

100

Please use double back-ticks for language constructs.

[ellis]

Applied changes suggested in comments

[ellis]

Add double backtick to doc

[ellis]

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

125

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.

[njames93]

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

125

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.

[ellis]

Add isLanguageVersionSupported for Objective-C

[aaron.ballman]

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

83

It looks like unrelated formatting changes may have snuck in?

124

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?

clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.cpp

35

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.

38

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

[njames93]

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

124

Thats a good point, maybe restrict this to LangOptions::ObjC || LangOptions::Blocks Then it can be left in bugprone.

[ellis]

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

83

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?

124

Ok, I'll include LangOptions::Blocks.

clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.cpp

38

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.

[aaron.ballman]

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

83

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

124

I think the only option you need is LangOptions::Blocks (it should be enabled automatically in ObjC language mode).

clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.cpp

38

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?

[ellis]

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

[ellis]

clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.cpp

35

Should I add a TODO for this?

[aaron.ballman]

LGTM with a testing request.

clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.cpp

35

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.

clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m

2

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.

[njames93]

clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m

2

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

[ellis]

Add RUN line to test in C

[ellis]

Update commit message

[ellis]

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

[aaron.ballman]

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!

[ellis]

clang-tools-extra/docs/clang-tidy/checks/bugprone-no-escape.rst

13

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.

[aaron.ballman]

clang-tools-extra/docs/clang-tidy/checks/bugprone-no-escape.rst

13

I pushed that fix up before getting your email. I'll keep an eye on the bot to ensure the fix worked. Thanks!

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

 #include "LambdaFunctionNameCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MisplacedOperatorInStrlenInAllocCheck.h"
 #include "MisplacedPointerArithmeticInAllocCheck.h"
 #include "MisplacedWideningCastCheck.h"
 #include "MoveForwardingReferenceCheck.h"
 #include "MultipleStatementMacroCheck.h"
+#include "NoEscapeCheck.h"
 #include "NotNullTerminatedResultCheck.h"
 #include "ParentVirtualCallCheck.h"
 #include "PosixReturnCheck.h"
 #include "ReservedIdentifierCheck.h"
 #include "SignedCharMisuseCheck.h"
 #include "SizeofContainerCheck.h"
 #include "SizeofExpressionCheck.h"
 #include "SpuriouslyWakeUpFunctionsCheck.h"
CheckFactories.registerCheck<ArgumentCommentCheck>(
         "bugprone-argument-comment");
     CheckFactories.registerCheck<AssertSideEffectCheck>(
         "bugprone-assert-side-effect");
     CheckFactories.registerCheck<BadSignalToKillThreadCheck>(
         "bugprone-bad-signal-to-kill-thread");
     CheckFactories.registerCheck<BoolPointerImplicitConversionCheck>(
         "bugprone-bool-pointer-implicit-conversion");
     CheckFactories.registerCheck<BranchCloneCheck>(
         "bugprone-branch-clone");
     CheckFactories.registerCheck<CopyConstructorInitCheck>(
         "bugprone-copy-constructor-init");
     CheckFactories.registerCheck<DanglingHandleCheck>(
         "bugprone-dangling-handle");
     CheckFactories.registerCheck<DynamicStaticInitializersCheck>(
         "bugprone-dynamic-static-initializers");
     CheckFactories.registerCheck<ExceptionEscapeCheck>(
         "bugprone-exception-escape");
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<MisplacedWideningCastCheck>(
         "bugprone-misplaced-widening-cast");
     CheckFactories.registerCheck<MoveForwardingReferenceCheck>(
         "bugprone-move-forwarding-reference");
     CheckFactories.registerCheck<MultipleStatementMacroCheck>(
         "bugprone-multiple-statement-macro");
     CheckFactories.registerCheck<cppcoreguidelines::NarrowingConversionsCheck>(
         "bugprone-narrowing-conversions");
+    CheckFactories.registerCheck<NoEscapeCheck>("bugprone-no-escape");
     CheckFactories.registerCheck<NotNullTerminatedResultCheck>(
         "bugprone-not-null-terminated-result");
     CheckFactories.registerCheck<ParentVirtualCallCheck>(
         "bugprone-parent-virtual-call");
     CheckFactories.registerCheck<PosixReturnCheck>(
         "bugprone-posix-return");
     CheckFactories.registerCheck<ReservedIdentifierCheck>(
         "bugprone-reserved-identifier");
     CheckFactories.registerCheck<SignedCharMisuseCheck>(

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

add_clang_library(clangTidyBugproneModule
   LambdaFunctionNameCheck.cpp
   MacroParenthesesCheck.cpp
   MacroRepeatedSideEffectsCheck.cpp
   MisplacedOperatorInStrlenInAllocCheck.cpp
   MisplacedPointerArithmeticInAllocCheck.cpp
   MisplacedWideningCastCheck.cpp
   MoveForwardingReferenceCheck.cpp
   MultipleStatementMacroCheck.cpp
+  NoEscapeCheck.cpp
   NotNullTerminatedResultCheck.cpp
   ParentVirtualCallCheck.cpp
   PosixReturnCheck.cpp
   ReservedIdentifierCheck.cpp
   SignedCharMisuseCheck.cpp
   SizeofContainerCheck.cpp
   SizeofExpressionCheck.cpp
   SpuriouslyWakeUpFunctionsCheck.cpp

clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.h

+//===--- 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 guaranteed
+/// to escape. If those blocks capture any pointers with the `noescape`
+/// attribute, then we warn the user of their error.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-no-escape.html
+class NoEscapeCheck : public ClangTidyCheck {
+public:
+  NoEscapeCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.Blocks;
+  }
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_NOESCAPECHECK_H

clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.cpp

+//===--- NoEscapeCheck.cpp - clang-tidy -----------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "NoEscapeCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+void NoEscapeCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(callExpr(callee(functionDecl(hasName("::dispatch_async"))),
+                              argumentCountIs(2),
+                              hasArgument(1, blockExpr().bind("arg-block"))),
+                     this);
+  Finder->addMatcher(callExpr(callee(functionDecl(hasName("::dispatch_after"))),
+                              argumentCountIs(3),
+                              hasArgument(2, blockExpr().bind("arg-block"))),
+                     this);
+}
+
+void NoEscapeCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedEscapingBlock =
+      Result.Nodes.getNodeAs<BlockExpr>("arg-block");
+  const BlockDecl *EscapingBlockDecl = MatchedEscapingBlock->getBlockDecl();
+  for (const BlockDecl::Capture &CapturedVar : EscapingBlockDecl->captures()) {
+    const VarDecl *Var = CapturedVar.getVariable();
+    if (Var && Var->hasAttr<NoEscapeAttr>()) {
+      // FIXME: Add a method to get the location of the use of a CapturedVar so
+      // that we can diagnose the use of the pointer instead of the block.
+      diag(MatchedEscapingBlock->getBeginLoc(),
+           "pointer %0 with attribute 'noescape' is captured by an "
+           "asynchronously-executed block")
+          << Var;
+      diag(Var->getBeginLoc(), "the 'noescape' attribute is declared here.",
+           DiagnosticIDs::Note);
+    }
+  }
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

 
 - New :doc:`bugprone-misplaced-pointer-arithmetic-in-alloc
   <clang-tidy/checks/bugprone-misplaced-pointer-arithmetic-in-alloc>` check.
 
   Finds cases where an integer expression is added to or subtracted from the
   result of a memory allocation function (``malloc()``, ``calloc()``,
   ``realloc()``, ``alloca()``) instead of its argument.
 
+- New :doc:`bugprone-no-escape
+  <clang-tidy/checks/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
   <clang-tidy/checks/bugprone-spuriously-wake-up-functions>` check.
 
   Finds ``cnd_wait``, ``cnd_timedwait``, ``wait``, ``wait_for``, or
   ``wait_until`` function calls when the function is not invoked from a loop
   that checks whether a condition predicate holds or the function has a
   condition parameter.
 

 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 - Improved :doc:`performance-faster-string-find
   <clang-tidy/checks/performance-faster-string-find>` check.
 
   Now checks ``std::basic_string_view`` by default.
 
 - Improved :doc:`readability-else-after-return
-  <clang-tidy/checks/readability-else-after-return>` check now supports a 
+  <clang-tidy/checks/readability-else-after-return>` check now supports a
   `WarnOnConditionVariables` option to control whether to refactor condition
   variables where possible.
 
 - Improved :doc:`readability-identifier-naming
   <clang-tidy/checks/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
   <clang-tidy/checks/readability-qualified-auto>` check now supports a
   `AddConstToQualified` to enable adding ``const`` qualifiers to variables
   typed with ``auto *`` and ``auto &``.
 
 - Improved :doc:`readability-redundant-string-init

clang-tools-extra/docs/clang-tidy/checks/bugprone-no-escape.rst

+.. 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;
+      });
+    });

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `bugprone-lambda-function-name <bugprone-lambda-function-name.html>`_,
    `bugprone-macro-parentheses <bugprone-macro-parentheses.html>`_, "Yes"
    `bugprone-macro-repeated-side-effects <bugprone-macro-repeated-side-effects.html>`_,
    `bugprone-misplaced-operator-in-strlen-in-alloc <bugprone-misplaced-operator-in-strlen-in-alloc.html>`_, "Yes"
    `bugprone-misplaced-pointer-arithmetic-in-alloc <bugprone-misplaced-pointer-arithmetic-in-alloc.html>`_, "Yes"
    `bugprone-misplaced-widening-cast <bugprone-misplaced-widening-cast.html>`_,
    `bugprone-move-forwarding-reference <bugprone-move-forwarding-reference.html>`_, "Yes"
    `bugprone-multiple-statement-macro <bugprone-multiple-statement-macro.html>`_,
+   `bugprone-no-escape <bugprone-no-escape.html>`_, "Yes"
    `bugprone-not-null-terminated-result <bugprone-not-null-terminated-result.html>`_, "Yes"
    `bugprone-parent-virtual-call <bugprone-parent-virtual-call.html>`_, "Yes"
    `bugprone-posix-return <bugprone-posix-return.html>`_, "Yes"
    `bugprone-reserved-identifier <bugprone-reserved-identifier.html>`_, "Yes"
    `bugprone-signed-char-misuse <bugprone-signed-char-misuse.html>`_,
    `bugprone-sizeof-container <bugprone-sizeof-container.html>`_,
    `bugprone-sizeof-expression <bugprone-sizeof-expression.html>`_,
    `bugprone-spuriously-wake-up-functions <bugprone-spuriously-wake-up-functions.html>`_,

clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m

+// 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
+  });
+}