[clang-tidy] Add check to diagnose coroutine-hostile RAII objects

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,82 @@
+//===--- CoroutineSuspensionHostileCheck.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/Attrs.inc"
+#include "clang/AST/Decl.h"
+#include "clang/AST/ExprCXX.h"
+#include "clang/AST/Stmt.h"
+#include "clang/AST/Type.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::misc {
+CoroutineHostileRAIICheck::CoroutineHostileRAIICheck(StringRef Name,
+                                                     ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context) {
+  for (StringRef Denied :
+       utils::options::parseStringList(Options.get("RAIIDenyList", ""))) {
+    Denied.consume_front("::");
+    RAIIDenyList.push_back(Denied);
+  }
+}
+
+void CoroutineHostileRAIICheck::registerMatchers(MatchFinder *Finder) {
+  // A suspension happens with co_await or co_yield.
+  Finder->addMatcher(coawaitExpr().bind("suspension"), this);
+  Finder->addMatcher(coyieldExpr().bind("suspension"), this);
+}
+
+void CoroutineHostileRAIICheck::checkVarDecl(VarDecl *VD) {
+  RecordDecl *RD = VD->getType().getCanonicalType()->getAsRecordDecl();
+  if (RD->hasAttr<clang::ScopedLockableAttr>()) {
+    diag(VD->getLocation(),
+         "%0 holds a lock across a suspension point of coroutine and could be "
+         "unlocked by a different thread")
+        << VD;
+  }
+  if (std::find(RAIIDenyList.begin(), RAIIDenyList.end(),
+                RD->getQualifiedNameAsString()) != RAIIDenyList.end()) {
+    diag(VD->getLocation(),
+         "%0 persists across a suspension point of coroutine")
+        << VD;
+  }
+}
+
+void CoroutineHostileRAIICheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Suspension = Result.Nodes.getNodeAs<Stmt>("suspension");
+  DynTypedNode P;
+  for (const Stmt *Child = Suspension; Child; Child = P.get<Stmt>()) {
+    auto Parents = Result.Context->getParents(*Child);
+    if (Parents.empty())
+      break;
+    P = *Parents.begin();
+    auto *PCS = P.get<CompoundStmt>();
+    if (!PCS)
+      continue;
+    for (auto Sibling = PCS->child_begin();
+         *Sibling != Child && Sibling != PCS->child_end(); ++Sibling) {
+      if (auto *DS = dyn_cast<DeclStmt>(*Sibling)) {
+        for (Decl *D : DS->decls()) {
+          if (VarDecl *VD = dyn_cast<VarDecl>(D)) {
+            checkVarDecl(VD);
+          }
+        }
+      }
+    }
+  }
+}
+
+void CoroutineHostileRAIICheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "RAIIDenyList",
+                utils::options::serializeStringList(RAIIDenyList));
+}
+} // namespace clang::tidy::misc

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

@@ -0,0 +1,44 @@
+//===--- 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/ASTMatchers/ASTMatchFinder.h"
+#include "llvm/ADT/StringRef.h"
+#include <vector>
+
+namespace clang::tidy::misc {
+
+/// Check detects objects which should not to persist across suspension points
+///
+/// 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;
+
+private:
+  void checkVarDecl(VarDecl *VD);
+  // List of fully qualified types which should not persist across a suspension
+  // point in a coroutine.
+  std::vector<StringRef> RAIIDenyList;
+};
+
+} // 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

@@ -180,6 +180,12 @@ New checks
   Detects C++ code where a reference variable is used to extend the lifetime
   of a temporary object that has just been constructed.
 
+- 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 check aliases
 ^^^^^^^^^^^^^^^^^
 

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>`_, "Yes"
    :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,48 @@
+.. title:: clang-tidy - misc-coroutine-hostile-raii
+
+misc-coroutine-hostile-raii
+====================
+
+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.
+
+.. 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:: RAIIDenyList
+
+   A semicolon-separated list of qualified types which should not be allowed to 
+   persist across suspension points.
+   Do not include `::` in the beginning of the qualified name.
+   Eg: `my::lockable; a::b; ::my::other::lockable;`

clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-hostile-raii.cpp

@@ -0,0 +1,131 @@
+// RUN: %check_clang_tidy -std=c++20 %s misc-coroutine-hostile-raii %t \
+// RUN:   -config="{CheckOptions: \
+// RUN:             {misc-coroutine-hostile-raii.RAIIDenyList: \
+// RUN:               'my::Mutex; ::my::other::Mutex'}}"
+
+namespace std {
+
+template <typename R, typename...> struct coroutine_traits {
+  using promise_type = typename R::promise_type;
+};
+
+template <typename Promise = void> struct coroutine_handle;
+
+template <> struct coroutine_handle<void> {
+  static coroutine_handle from_address(void *addr) noexcept {
+    coroutine_handle me;
+    me.ptr = addr;
+    return me;
+  }
+  void operator()() { resume(); }
+  void *address() const noexcept { return ptr; }
+  void resume() const {  }
+  void destroy() const { }
+  bool done() const { return true; }
+  coroutine_handle &operator=(decltype(nullptr)) {
+    ptr = nullptr;
+    return *this;
+  }
+  coroutine_handle(decltype(nullptr)) : ptr(nullptr) {}
+  coroutine_handle() : ptr(nullptr) {}
+  //  void reset() { ptr = nullptr; } // add to P0057?
+  explicit operator bool() const { return ptr; }
+
+protected:
+  void *ptr;
+};
+
+template <typename Promise> struct coroutine_handle : coroutine_handle<> {
+  using coroutine_handle<>::operator=;
+
+  static coroutine_handle from_address(void *addr) noexcept {
+    coroutine_handle me;
+    me.ptr = addr;
+    return me;
+  }
+
+  Promise &promise() const {
+    return *reinterpret_cast<Promise *>(
+        __builtin_coro_promise(ptr, alignof(Promise), false));
+  }
+  static coroutine_handle from_promise(Promise &promise) {
+    coroutine_handle p;
+    p.ptr = __builtin_coro_promise(&promise, alignof(Promise), true);
+    return p;
+  }
+};
+
+struct suspend_always {
+  bool await_ready() noexcept { return false; }
+  void await_suspend(std::coroutine_handle<>) noexcept {}
+  void await_resume() noexcept {}
+};
+} // namespace std
+
+struct ReturnObject {
+    struct promise_type {
+        ReturnObject get_return_object() { return {}; }
+        std::suspend_always initial_suspend() { return {}; }
+        std::suspend_always final_suspend() noexcept { return {}; }
+        void unhandled_exception() {}
+        std::suspend_always yield_value(int value) { return {}; }
+    };
+};
+
+
+#define SCOPED_LOCKABLE __attribute__ ((scoped_lockable))
+
+namespace absl {
+class SCOPED_LOCKABLE Mutex {};
+using Mutex2 = Mutex;
+} // namespace absl
+
+
+ReturnObject scopedLockableTest() {
+    absl::Mutex a;
+    // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: 'a' holds a lock across a suspension point of coroutine and could be unlocked by a different thread [misc-coroutine-hostile-raii]
+    absl::Mutex2 b;
+    // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'b' holds a lock across a suspension point of coroutine and could be unlocked by a different thread [misc-coroutine-hostile-raii]
+    {
+        absl::Mutex no_warning_1;
+        { absl::Mutex no_warning_2; }
+    }
+
+    co_yield 1;
+    absl::Mutex c;
+    // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: 'c' holds a lock across a suspension point of coroutine and could be unlocked by a different thread [misc-coroutine-hostile-raii]
+    co_await std::suspend_always{};
+    for(int i=1;i<=10;++i ) {
+      absl::Mutex d;
+      // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 'd' holds a lock across a suspension point of coroutine and could be unlocked by a different thread [misc-coroutine-hostile-raii]
+      co_await std::suspend_always{};
+      co_yield 1;
+      absl::Mutex no_warning_3;
+    }
+    if (true) {
+      absl::Mutex e;
+      // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 'e' holds a lock across a suspension point of coroutine and could be unlocked by a different thread [misc-coroutine-hostile-raii]
+      co_yield 1;
+      absl::Mutex no_warning_4;
+    }
+    absl::Mutex no_warning_5;
+}
+namespace my {
+class Mutex{};
+
+namespace other {
+class Mutex{};
+} // namespace other
+
+using Mutex2 = Mutex;
+} // namespace my
+
+ReturnObject denyListTest() {
+    my::Mutex a;
+    // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'a' persists across a suspension point of coroutine [misc-coroutine-hostile-raii]
+    my::other::Mutex b;
+    // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: 'b' persists across a suspension point of coroutine [misc-coroutine-hostile-raii]
+    my::Mutex2 c;
+    // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'c' persists across a suspension point of coroutine [misc-coroutine-hostile-raii]
+    co_yield 1;
+}