[clang-tidy] Add check to diagnose coroutine-hostile RAII objects (#6…

…8738)

This check detects **hostile-RAII** objects which should not **persist
across a suspension point in a coroutine**.

Some objects require that they be destroyed on the same thread that
created them. Traditionally this requirement was often phrased as "must
be a local variable", under the assumption that local variables always
work this way. However this is incorrect with **C++20 coroutines**,
since an intervening `co_await` may cause the coroutine to suspend and
later be resumed on another thread.

The lifetime of an object that requires being destroyed on the same
thread must not encompass a `co_await` or `co_yield` point. If you
create/destroy an object, you must do so without allowing the coroutine
to suspend in the meantime.

The check considers the following type as hostile:

- **Scoped-lockable types**: A scoped-lockable object persisting across
a suspension point is problematic as the lock held by this object could
be unlocked by a different thread. This would be undefined behaviour.

 - Types belonging to a configurable **denylist**.

```cpp
  // Call some async API while holding a lock.
const my::MutexLock l(&mu_);

// Oops! The async Bar function may finish on a different
// thread from the one that created the MutexLock object and therefore called
// Mutex::Lock -- now Mutex::Unlock will be called on the wrong thread.
co_await Bar();
```

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

@@ -18,6 +18,7 @@ add_custom_target(genconfusable DEPENDS Confusables.inc)
 
 add_clang_library(clangTidyMiscModule
   ConstCorrectnessCheck.cpp
+  CoroutineHostileRAIICheck.cpp
   DefinitionsInHeadersCheck.cpp
   ConfusableIdentifierCheck.cpp
   HeaderIncludeCycleCheck.cpp

clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp

@@ -0,0 +1,98 @@
+//===--- CoroutineHostileRAII.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 "CoroutineHostileRAIICheck.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/AST/Attr.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/ExprCXX.h"
+#include "clang/AST/Stmt.h"
+#include "clang/AST/Type.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchersInternal.h"
+#include "clang/Basic/AttrKinds.h"
+#include "clang/Basic/DiagnosticIDs.h"
+
+using namespace clang::ast_matchers;
+namespace clang::tidy::misc {
+namespace {
+using clang::ast_matchers::internal::BoundNodesTreeBuilder;
+
+AST_MATCHER_P(Stmt, forEachPrevStmt, ast_matchers::internal::Matcher<Stmt>,
+              InnerMatcher) {
+  DynTypedNode P;
+  bool IsHostile = false;
+  for (const Stmt *Child = &Node; Child; Child = P.get<Stmt>()) {
+    auto Parents = Finder->getASTContext().getParents(*Child);
+    if (Parents.empty())
+      break;
+    P = *Parents.begin();
+    auto *PCS = P.get<CompoundStmt>();
+    if (!PCS)
+      continue;
+    for (const auto &Sibling : PCS->children()) {
+      // Child contains suspension. Siblings after Child do not persist across
+      // this suspension.
+      if (Sibling == Child)
+        break;
+      // In case of a match, add the bindings as a separate match. Also don't
+      // clear the bindings if a match is not found (unlike Matcher::matches).
+      BoundNodesTreeBuilder SiblingBuilder;
+      if (InnerMatcher.matches(*Sibling, Finder, &SiblingBuilder)) {
+        Builder->addMatch(SiblingBuilder);
+        IsHostile = true;
+      }
+    }
+  }
+  return IsHostile;
+}
+} // namespace
+
+CoroutineHostileRAIICheck::CoroutineHostileRAIICheck(StringRef Name,
+                                                     ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      RAIITypesList(utils::options::parseStringList(
+          Options.get("RAIITypesList", "std::lock_guard;std::scoped_lock"))) {}
+
+void CoroutineHostileRAIICheck::registerMatchers(MatchFinder *Finder) {
+  // A suspension happens with co_await or co_yield.
+  auto ScopedLockable = varDecl(hasType(hasCanonicalType(hasDeclaration(
+                                    hasAttr(attr::Kind::ScopedLockable)))))
+                            .bind("scoped-lockable");
+  auto OtherRAII = varDecl(hasType(hasCanonicalType(hasDeclaration(
+                               namedDecl(hasAnyName(RAIITypesList))))))
+                       .bind("raii");
+  Finder->addMatcher(expr(anyOf(coawaitExpr(), coyieldExpr()),
+                          forEachPrevStmt(declStmt(forEach(
+                              varDecl(anyOf(ScopedLockable, OtherRAII))))))
+                         .bind("suspension"),
+                     this);
+}
+
+void CoroutineHostileRAIICheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *VD = Result.Nodes.getNodeAs<VarDecl>("scoped-lockable"))
+    diag(VD->getLocation(),
+         "%0 holds a lock across a suspension point of coroutine and could be "
+         "unlocked by a different thread")
+        << VD;
+  if (const auto *VD = Result.Nodes.getNodeAs<VarDecl>("raii"))
+    diag(VD->getLocation(),
+         "%0 persists across a suspension point of coroutine")
+        << VD;
+  if (const auto *Suspension = Result.Nodes.getNodeAs<Expr>("suspension"))
+    diag(Suspension->getBeginLoc(), "suspension point is here",
+         DiagnosticIDs::Note);
+}
+
+void CoroutineHostileRAIICheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "RAIITypesList",
+                utils::options::serializeStringList(RAIITypesList));
+}
+} // namespace clang::tidy::misc

clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h

@@ -0,0 +1,50 @@
+//===--- CoroutineHostileRAIICheck.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_MISC_COROUTINESHOSTILERAIICHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_COROUTINESHOSTILERAIICHECK_H
+
+#include "../ClangTidyCheck.h"
+#include "clang/AST/ASTTypeTraits.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "llvm/ADT/StringRef.h"
+#include <vector>
+
+namespace clang::tidy::misc {
+
+/// Detects when objects of certain hostile RAII types persists across
+/// suspension points in a coroutine. Such hostile types include scoped-lockable
+/// types and types belonging to a configurable denylist.
+///
+///  For the user-facing documentation see:
+///  http://clang.llvm.org/extra/clang-tidy/checks/misc/coroutine-hostile-raii.html
+class CoroutineHostileRAIICheck : public ClangTidyCheck {
+public:
+  CoroutineHostileRAIICheck(llvm::StringRef Name, ClangTidyContext *Context);
+
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus20;
+  }
+
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+  std::optional<TraversalKind> getCheckTraversalKind() const override {
+    return TK_AsIs;
+  }
+
+private:
+  // List of fully qualified types which should not persist across a suspension
+  // point in a coroutine.
+  std::vector<StringRef> RAIITypesList;
+};
+
+} // namespace clang::tidy::misc
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_COROUTINESHOSTILERAIICHECK_H

clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp

@@ -11,6 +11,7 @@
 #include "../ClangTidyModuleRegistry.h"
 #include "ConfusableIdentifierCheck.h"
 #include "ConstCorrectnessCheck.h"
+#include "CoroutineHostileRAIICheck.h"
 #include "DefinitionsInHeadersCheck.h"
 #include "HeaderIncludeCycleCheck.h"
 #include "IncludeCleanerCheck.h"
@@ -41,6 +42,8 @@ class MiscModule : public ClangTidyModule {
         "misc-confusable-identifiers");
     CheckFactories.registerCheck<ConstCorrectnessCheck>(
         "misc-const-correctness");
+    CheckFactories.registerCheck<CoroutineHostileRAIICheck>(
+        "misc-coroutine-hostile-raii");
     CheckFactories.registerCheck<DefinitionsInHeadersCheck>(
         "misc-definitions-in-headers");
     CheckFactories.registerCheck<HeaderIncludeCycleCheck>(

clang-tools-extra/docs/ReleaseNotes.rst

@@ -163,6 +163,13 @@ New checks
   Flags coroutines that suspend while a lock guard is in scope at the
   suspension point.
 
+- New :doc:`misc-coroutine-hostile-raii
+  <clang-tidy/checks/misc/coroutine-hostile-raii>` check.
+
+  Detects when objects of certain hostile RAII types persists across suspension
+  points in a coroutine. Such hostile types include scoped-lockable types and
+  types belonging to a configurable denylist.
+
 - New :doc:`modernize-use-constraints
   <clang-tidy/checks/modernize/use-constraints>` check.
 

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

@@ -241,6 +241,7 @@ Clang-Tidy Checks
    :doc:`llvmlibc-restrict-system-libc-headers <llvmlibc/restrict-system-libc-headers>`, "Yes"
    :doc:`misc-confusable-identifiers <misc/confusable-identifiers>`,
    :doc:`misc-const-correctness <misc/const-correctness>`, "Yes"
+   :doc:`misc-coroutine-hostile-raii <misc/coroutine-hostile-raii.html>`_,
    :doc:`misc-definitions-in-headers <misc/definitions-in-headers>`, "Yes"
    :doc:`misc-header-include-cycle <misc/header-include-cycle>`,
    :doc:`misc-include-cleaner <misc/include-cleaner>`, "Yes"

clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-hostile-raii.rst

@@ -0,0 +1,50 @@
+.. title:: clang-tidy - misc-coroutine-hostile-raii
+
+misc-coroutine-hostile-raii
+====================
+
+Detects when objects of certain hostile RAII types persists across suspension
+points in a coroutine. Such hostile types include scoped-lockable types and
+types belonging to a configurable denylist.
+
+Some objects require that they be destroyed on the same thread that created them. 
+Traditionally this requirement was often phrased as "must be a local variable",
+under the assumption that local variables always work this way. However this is
+incorrect with C++20 coroutines, since an intervening ``co_await`` may cause the
+coroutine to suspend and later be resumed on another thread.
+
+The lifetime of an object that requires being destroyed on the same thread must 
+not encompass a ``co_await`` or ``co_yield`` point. If you create/destroy an object,
+you must do so without allowing the coroutine to suspend in the meantime.
+
+Following types are considered as hostile:
+
+ - Scoped-lockable types: A scoped-lockable object persisting across a suspension
+ point is problematic as the lock held by this object could be unlocked by a 
+ different thread. This would be undefined behaviour.
+ This includes all types annotated with the ``scoped_lockable`` attribute.
+
+ - Types belonging to a configurable denylist.
+
+.. code-block:: c++
+
+  // Call some async API while holding a lock.
+  {
+    const my::MutexLock l(&mu_);
+
+    // Oops! The async Bar function may finish on a different
+    // thread from the one that created the MutexLock object and therefore called
+    // Mutex::Lock -- now Mutex::Unlock will be called on the wrong thread.
+    co_await Bar();
+  }
+
+
+Options
+-------
+
+.. option:: RAIITypesList
+
+    A semicolon-separated list of qualified types which should not be allowed to 
+    persist across suspension points.
+    Eg: ``my::lockable; a::b;::my::other::lockable;``
+    The default value of this option is `"std::lock_guard;std::scoped_lock"`.