[clang-tools-extra] c944443 - [clangtidy] Allow safe suspensions in coroutine-hostile-raii check (#72954)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Nov 27 07:21:12 PST 2023
Author: Utkarsh Saxena
Date: 2023-11-27T16:21:07+01:00
New Revision: c94444390f4c8f4d898ac8a8444bc4bf3394609f
URL: https://github.com/llvm/llvm-project/commit/c94444390f4c8f4d898ac8a8444bc4bf3394609f
DIFF: https://github.com/llvm/llvm-project/commit/c94444390f4c8f4d898ac8a8444bc4bf3394609f.diff
LOG: [clangtidy] Allow safe suspensions in coroutine-hostile-raii check (#72954)
Certain `awaitable` types could be safe to `co_await` on even when we
have suspension-hostile RAII objects in scope.
This PR adds a way for users to mark such safe `awaitable` and silence
false positive warnings in `co_await` expressions involving such an
`awaitable`.
`co_await`-ing an expression of `awaitable` type is considered safe if
the type is part of `SafeAwaiatablesList` check option. RAII objects
persisting across such a `co_await` expression are
considered safe and hence are not flagged.
Added:
Modified:
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
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
index e820cd39d83d21b..a0e8700b0522bc5 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
@@ -52,27 +52,42 @@ AST_MATCHER_P(Stmt, forEachPrevStmt, ast_matchers::internal::Matcher<Stmt>,
}
return IsHostile;
}
+
+// Matches the expression awaited by the `co_await`.
+AST_MATCHER_P(CoawaitExpr, awaitable, ast_matchers::internal::Matcher<Expr>,
+ InnerMatcher) {
+ if (Expr *E = Node.getCommonExpr())
+ return InnerMatcher.matches(*E, Finder, Builder);
+ return false;
+}
+
+auto typeWithNameIn(const std::vector<StringRef> &Names) {
+ return hasType(
+ hasCanonicalType(hasDeclaration(namedDecl(hasAnyName(Names)))));
+}
} // namespace
CoroutineHostileRAIICheck::CoroutineHostileRAIICheck(StringRef Name,
ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
RAIITypesList(utils::options::parseStringList(
- Options.get("RAIITypesList", "std::lock_guard;std::scoped_lock"))) {}
+ Options.get("RAIITypesList", "std::lock_guard;std::scoped_lock"))),
+ AllowedAwaitablesList(utils::options::parseStringList(
+ Options.get("AllowedAwaitablesList", ""))) {}
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);
+ auto OtherRAII = varDecl(typeWithNameIn(RAIITypesList)).bind("raii");
+ auto AllowedSuspend = awaitable(typeWithNameIn(AllowedAwaitablesList));
+ Finder->addMatcher(
+ expr(anyOf(coawaitExpr(unless(AllowedSuspend)), coyieldExpr()),
+ forEachPrevStmt(
+ declStmt(forEach(varDecl(anyOf(ScopedLockable, OtherRAII))))))
+ .bind("suspension"),
+ this);
}
void CoroutineHostileRAIICheck::check(const MatchFinder::MatchResult &Result) {
@@ -94,5 +109,7 @@ void CoroutineHostileRAIICheck::storeOptions(
ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "RAIITypesList",
utils::options::serializeStringList(RAIITypesList));
+ Options.store(Opts, "SafeAwaitableList",
+ utils::options::serializeStringList(AllowedAwaitablesList));
}
} // namespace clang::tidy::misc
diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
index a5e9cb89ef67695..be925097692a485 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
@@ -43,6 +43,9 @@ class CoroutineHostileRAIICheck : public ClangTidyCheck {
// List of fully qualified types which should not persist across a suspension
// point in a coroutine.
std::vector<StringRef> RAIITypesList;
+ // List of fully qualified awaitable types which are considered safe to
+ // co_await.
+ std::vector<StringRef> AllowedAwaitablesList;
};
} // namespace clang::tidy::misc
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
index b8698ba3de85300..a39c1853b313c49 100644
--- 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
@@ -29,16 +29,15 @@ Following types are considered as hostile:
.. code-block:: c++
// Call some async API while holding a lock.
- {
- const my::MutexLock l(&mu_);
+ task coro() {
+ const std::lock_guard 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.
+ // thread from the one that created the lock_guard (and called
+ // Mutex::Lock). After suspension, Mutex::Unlock will be called on the wrong thread.
co_await Bar();
}
-
Options
-------
@@ -48,3 +47,37 @@ Options
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"`.
+
+.. option:: AllowedAwaitablesList
+
+ A semicolon-separated list of qualified types of awaitables types which can
+ be safely awaited while having hostile RAII objects in scope.
+
+ ``co_await``-ing an expression of ``awaitable`` type is considered
+ safe if the ``awaitable`` type is part of this list.
+ RAII objects persisting across such a ``co_await`` expression are
+ considered safe and hence are not flagged.
+
+ Example usage:
+
+ .. code-block:: c++
+
+ // Consider option AllowedAwaitablesList = "safe_awaitable"
+ struct safe_awaitable {
+ bool await_ready() noexcept { return false; }
+ void await_suspend(std::coroutine_handle<>) noexcept {}
+ void await_resume() noexcept {}
+ };
+ auto wait() { return safe_awaitable{}; }
+
+ task coro() {
+ // This persists across both the co_await's but is not flagged
+ // because the awaitable is considered safe to await on.
+ const std::lock_guard l(&mu_);
+ co_await safe_awaitable{};
+ co_await wait();
+ }
+
+ Eg: ``my::safe::awaitable;other::awaitable``
+ The default value of this option is empty string `""`.
+
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
index 2d022e21c85d566..55a7e4b8f2954aa 100644
--- 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
@@ -1,7 +1,8 @@
// 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'}}"
+// RUN: -config="{CheckOptions: {\
+// RUN: misc-coroutine-hostile-raii.RAIITypesList: 'my::Mutex; ::my::other::Mutex', \
+// RUN: misc-coroutine-hostile-raii.AllowedAwaitablesList: 'safe::awaitable; ::my::other::awaitable' \
+// RUN: }}"
namespace std {
@@ -135,6 +136,20 @@ ReturnObject scopedLockableTest() {
absl::Mutex no_warning_5;
}
+namespace safe {
+ struct awaitable {
+ bool await_ready() noexcept { return false; }
+ void await_suspend(std::coroutine_handle<>) noexcept {}
+ void await_resume() noexcept {}
+};
+} // namespace safe
+ReturnObject RAIISafeSuspendTest() {
+ absl::Mutex a;
+ co_await safe::awaitable{};
+ using other = safe::awaitable;
+ co_await other{};
+}
+
void lambda() {
absl::Mutex no_warning;
auto lambda = []() -> ReturnObject {
More information about the cfe-commits
mailing list