[clang-tidy] Add check to diagnose coroutine-hostile RAII objects #68738
+402
−0
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
usx95
changed the title
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
@llvm/pr-subscribers-coroutines @llvm/pr-subscribers-clang-tidy Author: Utkarsh Saxena (usx95) ChangesThis 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 The lifetime of an object that requires being destroyed on the same thread must not encompass a The check considers the following type as hostile:
// 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();Full diff: https://github.com/llvm/llvm-project/pull/68738.diff 8 Files Affected:
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..7b084cd993c35ee
--- /dev/null
+++ b/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
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..84cb401508f4d40
--- /dev/null
+++ b/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
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 03e5dc6f164af2a..31c861f83807b9c 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/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
^^^^^^^^^^^^^^^^^
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 1baabceea06ef48..771245f083fe488 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>`_, "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"
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..602d08fd7fe873a
--- /dev/null
+++ b/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;`
\ 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..d0c2bbb21db7e1a
--- /dev/null
+++ b/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;
+}
\ No newline at end of file
|
PiotrZSL
reviewed
Oct 11, 2023
clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-hostile-raii.rst
Outdated
Show resolved
Hide resolved
PiotrZSL
reviewed
Oct 11, 2023
clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
Outdated
Show resolved
Hide resolved
PiotrZSL
reviewed
Oct 11, 2023
clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
Outdated
Show resolved
Hide resolved
PiotrZSL
reviewed
Oct 11, 2023
clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
Outdated
Show resolved
Hide resolved
PiotrZSL
reviewed
Oct 11, 2023
clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
Outdated
Show resolved
Hide resolved
PiotrZSL
reviewed
Oct 11, 2023
clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
Outdated
Show resolved
Hide resolved
PiotrZSL
reviewed
Oct 11, 2023
clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
Outdated
Show resolved
Hide resolved
PiotrZSL
reviewed
Oct 11, 2023
clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-hostile-raii.cpp
Outdated
Show resolved
Hide resolved
PiotrZSL
reviewed
Oct 11, 2023
clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-hostile-raii.rst
Show resolved
Hide resolved
PiotrZSL
reviewed
Oct 11, 2023
clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-hostile-raii.rst
Outdated
Show resolved
Hide resolved
PiotrZSL
reviewed
Oct 11, 2023
clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-hostile-raii.rst
Outdated
Show resolved
Hide resolved
PiotrZSL
reviewed
Oct 11, 2023
clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-hostile-raii.rst
Outdated
Show resolved
Hide resolved
PiotrZSL
reviewed
Oct 11, 2023
PiotrZSL
reviewed
Oct 11, 2023
clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
Outdated
Show resolved
Hide resolved
PiotrZSL
reviewed
Oct 11, 2023
clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-hostile-raii.cpp
Outdated
Show resolved
Hide resolved
PiotrZSL
reviewed
Oct 11, 2023
clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-hostile-raii.cpp
Outdated
Show resolved
Hide resolved
PiotrZSL
reviewed
Oct 11, 2023
clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-hostile-raii.cpp
Outdated
Show resolved
Hide resolved
PiotrZSL
requested changes
Oct 11, 2023
clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-hostile-raii.rst
Outdated
Show resolved
Hide resolved
PiotrZSL
reviewed
Oct 17, 2023
clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
Outdated
Show resolved
Hide resolved
PiotrZSL
reviewed
Oct 17, 2023
clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
Outdated
Show resolved
Hide resolved
PiotrZSL
reviewed
Oct 17, 2023
clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
Outdated
Show resolved
Hide resolved
PiotrZSL
reviewed
Oct 17, 2023
clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
Outdated
Show resolved
Hide resolved
PiotrZSL
reviewed
Oct 17, 2023
clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-hostile-raii.rst
Outdated
Show resolved
Hide resolved
PiotrZSL
reviewed
Oct 17, 2023
clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-hostile-raii.rst
Outdated
Show resolved
Hide resolved
PiotrZSL
reviewed
Oct 17, 2023
clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-hostile-raii.rst
Outdated
Show resolved
Hide resolved
PiotrZSL
reviewed
Oct 17, 2023
PiotrZSL
reviewed
Oct 17, 2023
PiotrZSL
approved these changes
Oct 17, 2023
clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-hostile-raii.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-hostile-raii.rst
Show resolved
Hide resolved
|
And run clang-format/git-clang-format on this (check code, not tests).... |
sam-mccall
reviewed
Oct 17, 2023
clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-hostile-raii.rst
Show resolved
Hide resolved
| - 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. |
There was a problem hiding this comment.
it's kind of unfortunate that do this based on lock_guard when it's mutex that's the problem. I see instances of lock_guard being used with spinlocks, which seem like they should be fine in coroutines.
But in practice this seems like a really useful heuristic, and I don't have a better suggestion. Maybe it'll need refinement in future.
| - 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. |
There was a problem hiding this comment.
is it worth mentioning that/why unique_lock is ignored?
| namespace { | ||
| using clang::ast_matchers::internal::BoundNodesTreeBuilder; | ||
|
|
||
| AST_MATCHER_P(Stmt, forEachPrevStmt, ast_matchers::internal::Matcher<Stmt>, |
There was a problem hiding this comment.
even for an locally-defined matcher, it would be helpful to have a comment on what this matcher does (the definition of "before" is non-obvious).
I also don't think its contract makes much sense: the definition of before excludes this:
{
; // this EmptyStmt is surely "before"...
}
; // this one
This is not just an implementation limitation but important to correctness, because we're really talking about variable scope (initialized before, but destroyed after).
But if that's the case, the matcher should also be named differently, and it should yield only VarDecls, because what it does doesn't make sense for anything except variables.
| namedDecl(hasAnyName(RAIITypesList)))))) | ||
| .bind("raii"); | ||
| Finder->addMatcher(expr(anyOf(coawaitExpr(), coyieldExpr()), | ||
| forEachPrevStmt(declStmt(forEach( |
There was a problem hiding this comment.
this implies a quadratic algorithm: every suspend point is going to walk every prior statement. (the matcher framework has memoization but it won't apply here).
It may well be fine in practice, and I don't really have a better suggestion - leaning heavily on matchers is idiomatic in clang-tidy, and is rarely efficient :-) No action needed here.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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_awaitmay 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_awaitorco_yieldpoint. 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.