https://github.com/usx95 updated https://github.com/llvm/llvm-project/pull/72954
>From c863646669d0b2b54e1c1c353b063a8209730528 Mon Sep 17 00:00:00 2001 From: Utkarsh Saxena <u...@google.com> Date: Tue, 21 Nov 2023 06:51:48 +0100 Subject: [PATCH 1/6] [clangtidy]Allow safe suspensions in coroutine-hostile-raii check --- .../misc/CoroutineHostileRAIICheck.cpp | 17 ++++++++++++++++- .../checkers/misc/coroutine-hostile-raii.cpp | 11 +++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp index e820cd39d83d21b..1c39a6624a1da69 100644 --- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp +++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp @@ -52,6 +52,19 @@ AST_MATCHER_P(Stmt, forEachPrevStmt, ast_matchers::internal::Matcher<Stmt>, } return IsHostile; } + +AST_MATCHER_P(CoawaitExpr, awaiatable, ast_matchers::internal::Matcher<Expr>, + InnerMatcher) { + return Node.getCommonExpr() && + InnerMatcher.matches(*Node.getCommonExpr(), Finder, Builder); +} + +AST_MATCHER(Decl, isRAIISafeAwaitable) { +for (const auto &Attr : Node.specific_attrs<clang::AnnotateAttr>()) + if (Attr->getAnnotation() == "coro_raii_safe_suspend") + return true; + return false; +} } // namespace CoroutineHostileRAIICheck::CoroutineHostileRAIICheck(StringRef Name, @@ -68,7 +81,9 @@ void CoroutineHostileRAIICheck::registerMatchers(MatchFinder *Finder) { auto OtherRAII = varDecl(hasType(hasCanonicalType(hasDeclaration( namedDecl(hasAnyName(RAIITypesList)))))) .bind("raii"); - Finder->addMatcher(expr(anyOf(coawaitExpr(), coyieldExpr()), + auto Allowed = awaiatable( + hasType(hasCanonicalType(hasDeclaration(isRAIISafeAwaitable())))); + Finder->addMatcher(expr(anyOf(coawaitExpr(unless(Allowed)), coyieldExpr()), forEachPrevStmt(declStmt(forEach( varDecl(anyOf(ScopedLockable, OtherRAII)))))) .bind("suspension"), 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..4a1dc61b34ded18 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 @@ -62,6 +62,12 @@ struct suspend_always { }; } // namespace std +struct [[clang::annotate("coro_raii_safe_suspend")]] raii_safe_suspend { + bool await_ready() noexcept { return false; } + void await_suspend(std::coroutine_handle<>) noexcept {} + void await_resume() noexcept {} +}; + struct ReturnObject { struct promise_type { ReturnObject get_return_object() { return {}; } @@ -135,6 +141,11 @@ ReturnObject scopedLockableTest() { absl::Mutex no_warning_5; } +ReturnObject RAIISafeSuspendTest() { + absl::Mutex a; + co_await raii_safe_suspend{}; +} + void lambda() { absl::Mutex no_warning; auto lambda = []() -> ReturnObject { >From b35e1fece9b35f5faf1a28a5fa744ed29ddbf250 Mon Sep 17 00:00:00 2001 From: Utkarsh Saxena <u...@google.com> Date: Tue, 21 Nov 2023 07:03:16 +0100 Subject: [PATCH 2/6] fix format --- clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp index 1c39a6624a1da69..235bb584be14754 100644 --- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp +++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp @@ -60,7 +60,7 @@ AST_MATCHER_P(CoawaitExpr, awaiatable, ast_matchers::internal::Matcher<Expr>, } AST_MATCHER(Decl, isRAIISafeAwaitable) { -for (const auto &Attr : Node.specific_attrs<clang::AnnotateAttr>()) + for (const auto &Attr : Node.specific_attrs<clang::AnnotateAttr>()) if (Attr->getAnnotation() == "coro_raii_safe_suspend") return true; return false; >From 9d1c6d90e3fd7b4eb673bd67ccbf48c1d24ce329 Mon Sep 17 00:00:00 2001 From: Utkarsh Saxena <u...@google.com> Date: Tue, 21 Nov 2023 07:06:51 +0100 Subject: [PATCH 3/6] rename var --- .../clang-tidy/misc/CoroutineHostileRAIICheck.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp index 235bb584be14754..6b438b6c44d855c 100644 --- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp +++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp @@ -81,13 +81,14 @@ void CoroutineHostileRAIICheck::registerMatchers(MatchFinder *Finder) { auto OtherRAII = varDecl(hasType(hasCanonicalType(hasDeclaration( namedDecl(hasAnyName(RAIITypesList)))))) .bind("raii"); - auto Allowed = awaiatable( + auto SafeSuspend = awaiatable( hasType(hasCanonicalType(hasDeclaration(isRAIISafeAwaitable())))); - Finder->addMatcher(expr(anyOf(coawaitExpr(unless(Allowed)), coyieldExpr()), - forEachPrevStmt(declStmt(forEach( - varDecl(anyOf(ScopedLockable, OtherRAII)))))) - .bind("suspension"), - this); + Finder->addMatcher( + expr(anyOf(coawaitExpr(unless(SafeSuspend)), coyieldExpr()), + forEachPrevStmt( + declStmt(forEach(varDecl(anyOf(ScopedLockable, OtherRAII)))))) + .bind("suspension"), + this); } void CoroutineHostileRAIICheck::check(const MatchFinder::MatchResult &Result) { >From 37f412080ee64d98527aeb1a06429846d935de81 Mon Sep 17 00:00:00 2001 From: Utkarsh Saxena <u...@google.com> Date: Tue, 21 Nov 2023 07:40:13 +0100 Subject: [PATCH 4/6] more work --- .../clang-tidy/misc/CoroutineHostileRAIICheck.cpp | 6 +++--- .../checkers/misc/coroutine-hostile-raii.cpp | 11 +++++------ 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp index 6b438b6c44d855c..ee7565d5c714d4d 100644 --- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp +++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp @@ -59,7 +59,7 @@ AST_MATCHER_P(CoawaitExpr, awaiatable, ast_matchers::internal::Matcher<Expr>, InnerMatcher.matches(*Node.getCommonExpr(), Finder, Builder); } -AST_MATCHER(Decl, isRAIISafeAwaitable) { +AST_MATCHER(Decl, isRAIISafe) { for (const auto &Attr : Node.specific_attrs<clang::AnnotateAttr>()) if (Attr->getAnnotation() == "coro_raii_safe_suspend") return true; @@ -81,8 +81,8 @@ void CoroutineHostileRAIICheck::registerMatchers(MatchFinder *Finder) { auto OtherRAII = varDecl(hasType(hasCanonicalType(hasDeclaration( namedDecl(hasAnyName(RAIITypesList)))))) .bind("raii"); - auto SafeSuspend = awaiatable( - hasType(hasCanonicalType(hasDeclaration(isRAIISafeAwaitable())))); + auto SafeSuspend = + awaiatable(hasType(hasCanonicalType(hasDeclaration(isRAIISafe())))); Finder->addMatcher( expr(anyOf(coawaitExpr(unless(SafeSuspend)), coyieldExpr()), forEachPrevStmt( 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 608f9aec6ba0a71..6fd649bb021b414 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 @@ -62,12 +62,6 @@ struct suspend_always { }; } // namespace std -struct [[clang::annotate("coro_raii_safe_suspend")]] raii_safe_suspend { - bool await_ready() noexcept { return false; } - void await_suspend(std::coroutine_handle<>) noexcept {} - void await_resume() noexcept {} -}; - struct ReturnObject { struct promise_type { ReturnObject get_return_object() { return {}; } @@ -141,6 +135,11 @@ ReturnObject scopedLockableTest() { absl::Mutex no_warning_5; } +struct [[clang::annotate("coro_raii_safe_suspend")]] raii_safe_suspend { + bool await_ready() noexcept { return false; } + void await_suspend(std::coroutine_handle<>) noexcept {} + void await_resume() noexcept {} +}; ReturnObject RAIISafeSuspendTest() { absl::Mutex a; co_await raii_safe_suspend{}; >From e438500e8775887f72f234395a781da66277c803 Mon Sep 17 00:00:00 2001 From: Utkarsh Saxena <u...@google.com> Date: Tue, 21 Nov 2023 08:02:46 +0100 Subject: [PATCH 5/6] add docs --- .../checks/misc/coroutine-hostile-raii.rst | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) 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..b36bded6866641b 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,15 +29,28 @@ 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 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. + // 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(); } +.. code-block:: c++ + + struct [[clang::annotate("coro_raii_safe_suspend")]] safe_awaitable { + bool await_ready() noexcept { return false; } + void await_suspend(std::coroutine_handle<>) noexcept {} + void await_resume() noexcept {} + }; + + task coro() { + const std::lock_guard l(&mu_); + co_await safe_awaitable{}; + } + Options ------- >From 5a3af35ee3f17b384dd6314e5ea1918160f19856 Mon Sep 17 00:00:00 2001 From: Utkarsh Saxena <u...@google.com> Date: Tue, 21 Nov 2023 09:00:39 +0100 Subject: [PATCH 6/6] update docs (cherry picked from commit f2092a62148dca3ee6cb3ea7a80fc7c9ad640f50) --- .../checks/misc/coroutine-hostile-raii.rst | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) 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 b36bded6866641b..93e9158d9f3fd16 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 @@ -38,6 +38,22 @@ Following types are considered as hostile: co_await Bar(); } +Exclusions +------- +It is possible to make the check treat certain suspensions as safe. +``co_await``-ing an expression of ``awaitable`` type is considered +safe if the ``awaitable`` type is annotated with +``[[clang::annotate("coro_raii_safe_suspend")]]``. +RAII objects persisting across such a ``co_await`` expression are +considered safe and hence are not flagged. + +This annotation can be used to mark ``awaitable`` types which can be safely +awaited while having hostile RAII objects in scope. For example, such safe +``awaitable`` could ensure resumption on the same thread or even unlock the mutex +on suspension and reacquire on resumption. + +Example usage: + .. code-block:: c++ struct [[clang::annotate("coro_raii_safe_suspend")]] safe_awaitable { @@ -51,6 +67,13 @@ Following types are considered as hostile: co_await safe_awaitable{}; } + auto wait() { return safe_awaitable{}; } + + task coro() { + const std::lock_guard l(&mu_); // No warning. + co_await safe_awaitable{}; + co_await wait(); + } Options ------- _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits