[clang-tools-extra] 3151281 - [clang-tidy] Add check to diagnose coroutine-hostile RAII objects (#68738)
via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 17 11:46:06 PDT 2023
Author: Utkarsh Saxena
Date: 2023-10-17T20:46:01+02:00
New Revision: 31512811b8c0f8fd328fba585640992c39218f1e
URL: https://github.com/llvm/llvm-project/commit/31512811b8c0f8fd328fba585640992c39218f1e
DIFF: https://github.com/llvm/llvm-project/commit/31512811b8c0f8fd328fba585640992c39218f1e.diff
LOG: [clang-tidy] Add check to diagnose coroutine-hostile RAII objects (#68738)
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();
```
Added:
clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-hostile-raii.rst
clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-hostile-raii.cpp
Modified:
clang-tools-extra/clang-tidy/misc/CMakeLists.txt
clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt
index 2e88e68a5447825..d9ec268650c0532 100644
--- a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt
+++ b/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
diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
new file mode 100644
index 000000000000000..e820cd39d83d21b
--- /dev/null
+++ b/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
diff erent 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
diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
new file mode 100644
index 000000000000000..a5e9cb89ef67695
--- /dev/null
+++ b/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
diff --git a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
index 92590506e1ec1e6..d8a88324ee63e08 100644
--- a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
+++ b/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>(
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index af164d0462d52c1..3e1fbe091c9ff6a 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/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.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 2125ebd7a213c15..819e3974e3f1330 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/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"
diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-hostile-raii.rst b/clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-hostile-raii.rst
new file mode 100644
index 000000000000000..dcb9f399774cba9
--- /dev/null
+++ b/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
+
diff erent 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
diff erent
+ // 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"`.
\ No newline at end of file
diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-hostile-raii.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-hostile-raii.cpp
new file mode 100644
index 000000000000000..2d022e21c85d566
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-hostile-raii.cpp
@@ -0,0 +1,192 @@
+// RUN: %check_clang_tidy -std=c++20 %s misc-coroutine-hostile-raii %t \
+// RUN: -config="{CheckOptions: \
+// RUN: {misc-coroutine-hostile-raii.RAIITypesList: \
+// 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 BasicWarning() {
+ absl::Mutex mtx;
+ // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'mtx' holds a lock across a suspension point of coroutine and could be unlocked by a
diff erent thread [misc-coroutine-hostile-raii]
+ int no_warning;
+ {
+ co_yield 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: note: suspension point is here
+ }
+}
+
+ReturnObject BasicNoWarning() {
+ co_yield 1;
+ { absl::Mutex no_warning; }
+ int no_warning;
+ {
+ co_yield 1;
+ absl::Mutex no_warning;
+ }
+ co_yield 1;
+}
+
+ReturnObject scopedLockableTest() {
+ co_yield 0;
+ 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
diff erent 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
diff erent thread [misc-coroutine-hostile-raii]
+ {
+ absl::Mutex no_warning_1;
+ { absl::Mutex no_warning_2; }
+ }
+
+ co_yield 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: note: suspension point is here
+ 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
diff erent thread [misc-coroutine-hostile-raii]
+ co_await std::suspend_always{};
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: note: suspension point is here
+ 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
diff erent thread [misc-coroutine-hostile-raii]
+ co_await std::suspend_always{};
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: note: suspension point is here
+ 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
diff erent thread [misc-coroutine-hostile-raii]
+ co_yield 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: note: suspension point is here
+ absl::Mutex no_warning_4;
+ }
+ absl::Mutex no_warning_5;
+}
+
+void lambda() {
+ absl::Mutex no_warning;
+ auto lambda = []() -> ReturnObject {
+ co_await std::suspend_always{};
+ 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
diff erent thread [misc-coroutine-hostile-raii]
+ co_yield 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: note: suspension point is here
+ co_await std::suspend_always{};
+ co_yield 1;
+ };
+ absl::Mutex no_warning_2;
+}
+
+template<class T>
+ReturnObject raii_in_template(){
+ T a;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'a' holds a lock across a suspension point of coroutine and could be unlocked by a
diff erent thread [misc-coroutine-hostile-raii]
+ co_yield 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: note: suspension point is here
+}
+void foo_template() { raii_in_template<absl::Mutex>(); }
+
+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;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: note: suspension point is here
+}
+
+ReturnObject referenceTest(my::Mutex& ref) {
+ my::Mutex& a = ref;
+ co_yield 1;
+}
+ReturnObject pointerTest(my::Mutex* ref) {
+ my::Mutex* a = ref;
+ co_yield 1;
+}
+
+ReturnObject functionArgTest(my::Mutex ref) {
+ co_yield 1;
+}
More information about the cfe-commits
mailing list