[clang-tools-extra] [clang-tidy] Add check to diagnose coroutine-hostile RAII objects (PR #68738)
Utkarsh Saxena via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 17 11:39:29 PDT 2023
https://github.com/usx95 updated https://github.com/llvm/llvm-project/pull/68738
>From f9e29053a7a8fd8222cfbdf579776fafd6564b89 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Tue, 10 Oct 2023 21:53:37 +0200
Subject: [PATCH 01/10] [clang-tidy] Add check to flag objects hostile to
suspension in a coroutine
---
.../clang-tidy/misc/CMakeLists.txt | 1 +
.../misc/CoroutineSuspensionHostileCheck.cpp | 79 +++++++++++
.../misc/CoroutineSuspensionHostileCheck.h | 45 ++++++
.../clang-tidy/misc/MiscTidyModule.cpp | 3 +
clang-tools-extra/docs/ReleaseNotes.rst | 6 +
.../docs/clang-tidy/checks/list.rst | 1 +
.../misc/coroutine-suspension-hostile.rst | 36 +++++
.../misc/coroutine-suspension-hostile.cpp | 131 ++++++++++++++++++
8 files changed, 302 insertions(+)
create mode 100644 clang-tools-extra/clang-tidy/misc/CoroutineSuspensionHostileCheck.cpp
create mode 100644 clang-tools-extra/clang-tidy/misc/CoroutineSuspensionHostileCheck.h
create mode 100644 clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-suspension-hostile.rst
create mode 100644 clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-suspension-hostile.cpp
diff --git a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt
index 2e88e68a5447825..c58a99d165dc34c 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
+ CoroutineSuspensionHostileCheck.cpp
DefinitionsInHeadersCheck.cpp
ConfusableIdentifierCheck.cpp
HeaderIncludeCycleCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineSuspensionHostileCheck.cpp b/clang-tools-extra/clang-tidy/misc/CoroutineSuspensionHostileCheck.cpp
new file mode 100644
index 000000000000000..d9fb34f5e92888a
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineSuspensionHostileCheck.cpp
@@ -0,0 +1,79 @@
+//===--- 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 "CoroutineSuspensionHostileCheck.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 {
+CoroutineSuspensionHostileCheck::CoroutineSuspensionHostileCheck(
+ StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ DenyTypeList(
+ utils::options::parseStringList(Options.get("DenyTypeList", ""))) {}
+
+void CoroutineSuspensionHostileCheck::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 CoroutineSuspensionHostileCheck::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(DenyTypeList.begin(), DenyTypeList.end(),
+ RD->getQualifiedNameAsString()) != DenyTypeList.end()) {
+ diag(VD->getLocation(),
+ "%0 persists across a suspension point of coroutine")
+ << VD;
+ }
+}
+
+void CoroutineSuspensionHostileCheck::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 CoroutineSuspensionHostileCheck::storeOptions(
+ ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "DenyTypeList",
+ utils::options::serializeStringList(DenyTypeList));
+}
+} // namespace clang::tidy::misc
diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineSuspensionHostileCheck.h b/clang-tools-extra/clang-tidy/misc/CoroutineSuspensionHostileCheck.h
new file mode 100644
index 000000000000000..1075507416c7c89
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineSuspensionHostileCheck.h
@@ -0,0 +1,45 @@
+//===--- HeaderIncludeCycleCheck.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_COROUTINESUSPENSSIONHOSTILECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_COROUTINESUSPENSSIONHOSTILECHECK_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-check.html
+class CoroutineSuspensionHostileCheck : public ClangTidyCheck {
+public:
+ CoroutineSuspensionHostileCheck(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.
+ const std::vector<StringRef> DenyTypeList;
+};
+
+} // namespace clang::tidy::misc
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_COROUTINESUSPENSSIONHOSTILECHECK_H
diff --git a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
index 92590506e1ec1e6..189fd1a4787a21b 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 "CoroutineSuspensionHostileCheck.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<CoroutineSuspensionHostileCheck>(
+ "misc-coroutine-suspension-hostile");
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..fd5d66f5c809eb1 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-suspension-hostile
+ <clang-tidy/checks/misc/coroutine-suspension-hostile>` check.
+
+ Detects when objects of certain hostile 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..5047f91e33d67cd 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-suspension-hostile <misc/coroutine-suspension-hostile.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-suspension-hostile.rst b/clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-suspension-hostile.rst
new file mode 100644
index 000000000000000..123d9a966a0a058
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-suspension-hostile.rst
@@ -0,0 +1,36 @@
+.. title:: clang-tidy - misc-coroutine-suspension-hostile
+
+misc-coroutine-suspension-hostile
+====================
+
+This check detects when objects of certain hostile types persists across suspension points in a coroutine.
+Such hostile types include scoped-lockable types and types belonging to a configurable denylist.
+
+A scoped-lockable object persisting across a suspension point in a coroutine is
+problematic as it is possible for the lock held by this object at the suspension
+point to be unlocked by a wrong thread if the coroutine resume on a different thread.
+This would be undefined behaviour.
+
+The check also diagnosis objects persisting across suspension points which belong 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:: DenyTypeList
+
+ A semicolon-separated list of qualified types which should not be allowed to persist across suspension points.
+ Do not include trailing `::` in the qualified name.
+ Eg: `my::lockable; my::other::lockable;`
\ No newline at end of file
diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-suspension-hostile.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-suspension-hostile.cpp
new file mode 100644
index 000000000000000..987339816b4bdc4
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-suspension-hostile.cpp
@@ -0,0 +1,131 @@
+// RUN: %check_clang_tidy -std=c++20 %s misc-coroutine-suspension-hostile %t \
+// RUN: -config="{CheckOptions: \
+// RUN: {misc-coroutine-suspension-hostile.DenyTypeList: \
+// 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-suspension-hostile]
+ 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-suspension-hostile]
+ {
+ 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-suspension-hostile]
+ 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-suspension-hostile]
+ 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-suspension-hostile]
+ 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-suspension-hostile]
+ my::other::Mutex b;
+ // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: 'b' persists across a suspension point of coroutine [misc-coroutine-suspension-hostile]
+ my::Mutex2 c;
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'c' persists across a suspension point of coroutine [misc-coroutine-suspension-hostile]
+ co_yield 1;
+}
\ No newline at end of file
>From 78e3102b83809486fd6b4f255a2e77b59bc4ad0e Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Wed, 11 Oct 2023 14:08:48 +0200
Subject: [PATCH 02/10] Renamed check to misc-coroutine-hostile-raii
---
.../clang-tidy/misc/CMakeLists.txt | 2 +-
...heck.cpp => CoroutineHostileRAIICheck.cpp} | 27 +++++++++----------
...ileCheck.h => CoroutineHostileRAIICheck.h} | 14 +++++-----
.../clang-tidy/misc/MiscTidyModule.cpp | 6 ++---
.../misc/coroutine-suspension-hostile.rst | 23 +++++++++-------
.../misc/coroutine-suspension-hostile.cpp | 20 +++++++-------
6 files changed, 47 insertions(+), 45 deletions(-)
rename clang-tools-extra/clang-tidy/misc/{CoroutineSuspensionHostileCheck.cpp => CoroutineHostileRAIICheck.cpp} (73%)
rename clang-tools-extra/clang-tidy/misc/{CoroutineSuspensionHostileCheck.h => CoroutineHostileRAIICheck.h} (71%)
diff --git a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt
index c58a99d165dc34c..d9ec268650c0532 100644
--- a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt
@@ -18,7 +18,7 @@ add_custom_target(genconfusable DEPENDS Confusables.inc)
add_clang_library(clangTidyMiscModule
ConstCorrectnessCheck.cpp
- CoroutineSuspensionHostileCheck.cpp
+ CoroutineHostileRAIICheck.cpp
DefinitionsInHeadersCheck.cpp
ConfusableIdentifierCheck.cpp
HeaderIncludeCycleCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineSuspensionHostileCheck.cpp b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
similarity index 73%
rename from clang-tools-extra/clang-tidy/misc/CoroutineSuspensionHostileCheck.cpp
rename to clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
index d9fb34f5e92888a..abfd2b76b0476dc 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineSuspensionHostileCheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
@@ -6,7 +6,7 @@
//
//===----------------------------------------------------------------------===//
-#include "CoroutineSuspensionHostileCheck.h"
+#include "CoroutineHostileRAIICheck.h"
#include "../utils/OptionsUtils.h"
#include "clang/AST/Attrs.inc"
#include "clang/AST/Decl.h"
@@ -18,19 +18,19 @@
using namespace clang::ast_matchers;
namespace clang::tidy::misc {
-CoroutineSuspensionHostileCheck::CoroutineSuspensionHostileCheck(
- StringRef Name, ClangTidyContext *Context)
+CoroutineHostileRAIICheck::CoroutineHostileRAIICheck(StringRef Name,
+ ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
- DenyTypeList(
- utils::options::parseStringList(Options.get("DenyTypeList", ""))) {}
+ RAIIDenyList(
+ utils::options::parseStringList(Options.get("RAIIDenyList", ""))) {}
-void CoroutineSuspensionHostileCheck::registerMatchers(MatchFinder *Finder) {
+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 CoroutineSuspensionHostileCheck::checkVarDecl(VarDecl *VD) {
+void CoroutineHostileRAIICheck::checkVarDecl(VarDecl *VD) {
RecordDecl *RD = VD->getType().getCanonicalType()->getAsRecordDecl();
if (RD->hasAttr<clang::ScopedLockableAttr>()) {
diag(VD->getLocation(),
@@ -38,16 +38,15 @@ void CoroutineSuspensionHostileCheck::checkVarDecl(VarDecl *VD) {
"unlocked by a different thread")
<< VD;
}
- if (std::find(DenyTypeList.begin(), DenyTypeList.end(),
- RD->getQualifiedNameAsString()) != DenyTypeList.end()) {
+ if (std::find(RAIIDenyList.begin(), RAIIDenyList.end(),
+ RD->getQualifiedNameAsString()) != RAIIDenyList.end()) {
diag(VD->getLocation(),
"%0 persists across a suspension point of coroutine")
<< VD;
}
}
-void CoroutineSuspensionHostileCheck::check(
- const MatchFinder::MatchResult &Result) {
+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>()) {
@@ -71,9 +70,9 @@ void CoroutineSuspensionHostileCheck::check(
}
}
-void CoroutineSuspensionHostileCheck::storeOptions(
+void CoroutineHostileRAIICheck::storeOptions(
ClangTidyOptions::OptionMap &Opts) {
- Options.store(Opts, "DenyTypeList",
- utils::options::serializeStringList(DenyTypeList));
+ Options.store(Opts, "RAIIDenyList",
+ utils::options::serializeStringList(RAIIDenyList));
}
} // namespace clang::tidy::misc
diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineSuspensionHostileCheck.h b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
similarity index 71%
rename from clang-tools-extra/clang-tidy/misc/CoroutineSuspensionHostileCheck.h
rename to clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
index 1075507416c7c89..cd61523b6561a5a 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineSuspensionHostileCheck.h
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
@@ -1,4 +1,4 @@
-//===--- HeaderIncludeCycleCheck.h - clang-tidy -----------------*- C++ -*-===//
+//===--- 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.
@@ -6,8 +6,8 @@
//
//===----------------------------------------------------------------------===//
-#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_COROUTINESUSPENSSIONHOSTILECHECK_H
-#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_COROUTINESUSPENSSIONHOSTILECHECK_H
+#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"
@@ -20,9 +20,9 @@ namespace clang::tidy::misc {
///
/// For the user-facing documentation see:
/// http://clang.llvm.org/extra/clang-tidy/checks/misc/coroutine-hostile-check.html
-class CoroutineSuspensionHostileCheck : public ClangTidyCheck {
+class CoroutineHostileRAIICheck : public ClangTidyCheck {
public:
- CoroutineSuspensionHostileCheck(llvm::StringRef Name,
+ CoroutineHostileRAIICheck(llvm::StringRef Name,
ClangTidyContext *Context);
bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
@@ -37,9 +37,9 @@ class CoroutineSuspensionHostileCheck : public ClangTidyCheck {
void checkVarDecl(VarDecl *VD);
// List of fully qualified types which should not persist across a suspension
// point in a coroutine.
- const std::vector<StringRef> DenyTypeList;
+ const std::vector<StringRef> RAIIDenyList;
};
} // namespace clang::tidy::misc
-#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_COROUTINESUSPENSSIONHOSTILECHECK_H
+#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 189fd1a4787a21b..d8a88324ee63e08 100644
--- a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
@@ -11,7 +11,7 @@
#include "../ClangTidyModuleRegistry.h"
#include "ConfusableIdentifierCheck.h"
#include "ConstCorrectnessCheck.h"
-#include "CoroutineSuspensionHostileCheck.h"
+#include "CoroutineHostileRAIICheck.h"
#include "DefinitionsInHeadersCheck.h"
#include "HeaderIncludeCycleCheck.h"
#include "IncludeCleanerCheck.h"
@@ -42,8 +42,8 @@ class MiscModule : public ClangTidyModule {
"misc-confusable-identifiers");
CheckFactories.registerCheck<ConstCorrectnessCheck>(
"misc-const-correctness");
- CheckFactories.registerCheck<CoroutineSuspensionHostileCheck>(
- "misc-coroutine-suspension-hostile");
+ CheckFactories.registerCheck<CoroutineHostileRAIICheck>(
+ "misc-coroutine-hostile-raii");
CheckFactories.registerCheck<DefinitionsInHeadersCheck>(
"misc-definitions-in-headers");
CheckFactories.registerCheck<HeaderIncludeCycleCheck>(
diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-suspension-hostile.rst b/clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-suspension-hostile.rst
index 123d9a966a0a058..262d7eedcd84fe9 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-suspension-hostile.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-suspension-hostile.rst
@@ -1,17 +1,20 @@
-.. title:: clang-tidy - misc-coroutine-suspension-hostile
+.. title:: clang-tidy - misc-coroutine-hostile-raii
-misc-coroutine-suspension-hostile
+misc-coroutine-hostile-raii
====================
-This check detects when objects of certain hostile types persists across suspension points in a coroutine.
-Such hostile types include scoped-lockable types and types belonging to a configurable denylist.
+This check detects hostile-RAII objects which should not persist across a suspension point in a coroutine.
+Since after a suspension a coroutine could potentially be resumed on a different thread,
+such RAII objects could be created by one thread but destroyed by another.
+Certain RAII types could be hostile to being destroyed by another thread.
-A scoped-lockable object persisting across a suspension point in a coroutine is
-problematic as it is possible for the lock held by this object at the suspension
-point to be unlocked by a wrong thread if the coroutine resume on a different thread.
-This would be undefined behaviour.
+The check considers the following type as hostile:
-The check also diagnosis objects persisting across suspension points which belong to a configurable denylist.
+ - Scoped-lockable types: A scoped-lockable object persisting across a suspension point in a coroutine is
+ problematic as it is possible for the lock held by this object at the suspension
+ point to be unlocked by a different thread. This would be undefined behaviour.
+
+ - Types belonging to a configurable denylist.
.. code-block:: c++
@@ -29,7 +32,7 @@ The check also diagnosis objects persisting across suspension points which belon
Options
-------
-.. option:: DenyTypeList
+.. option:: RAIIDenyList
A semicolon-separated list of qualified types which should not be allowed to persist across suspension points.
Do not include trailing `::` in the qualified name.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-suspension-hostile.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-suspension-hostile.cpp
index 987339816b4bdc4..e83ce771aaed38c 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-suspension-hostile.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-suspension-hostile.cpp
@@ -1,6 +1,6 @@
-// RUN: %check_clang_tidy -std=c++20 %s misc-coroutine-suspension-hostile %t \
+// RUN: %check_clang_tidy -std=c++20 %s misc-coroutine-hostile-raii %t \
// RUN: -config="{CheckOptions: \
-// RUN: {misc-coroutine-suspension-hostile.DenyTypeList: \
+// RUN: {misc-coroutine-hostile-raii.RAIIDenyList: \
// RUN: 'my::Mutex; my::other::Mutex'}}"
namespace std {
@@ -83,9 +83,9 @@ using Mutex2 = Mutex;
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-suspension-hostile]
+ // 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-suspension-hostile]
+ // 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; }
@@ -93,18 +93,18 @@ ReturnObject scopedLockableTest() {
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-suspension-hostile]
+ // 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-suspension-hostile]
+ // 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-suspension-hostile]
+ // 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;
}
@@ -122,10 +122,10 @@ using Mutex2 = Mutex;
ReturnObject denyListTest() {
my::Mutex a;
- // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'a' persists across a suspension point of coroutine [misc-coroutine-suspension-hostile]
+ // 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-suspension-hostile]
+ // 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-suspension-hostile]
+ // 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
>From 6880288a911cf912d71e114028c7a3aa27ab6d03 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Wed, 11 Oct 2023 14:41:48 +0200
Subject: [PATCH 03/10] Continued rename and improved denylist parsing.
---
.../misc/CoroutineHostileRAIICheck.cpp | 10 ++--
.../misc/CoroutineHostileRAIICheck.h | 4 +-
.../docs/clang-tidy/checks/list.rst | 2 +-
.../checks/misc/coroutine-hostile-raii.rst | 48 +++++++++++++++++++
.../misc/coroutine-suspension-hostile.rst | 39 ---------------
...hostile.cpp => coroutine-hostile-raii.cpp} | 2 +-
6 files changed, 59 insertions(+), 46 deletions(-)
create mode 100644 clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-hostile-raii.rst
delete mode 100644 clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-suspension-hostile.rst
rename clang-tools-extra/test/clang-tidy/checkers/misc/{coroutine-suspension-hostile.cpp => coroutine-hostile-raii.cpp} (98%)
diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
index abfd2b76b0476dc..7b084cd993c35ee 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
@@ -20,9 +20,13 @@ using namespace clang::ast_matchers;
namespace clang::tidy::misc {
CoroutineHostileRAIICheck::CoroutineHostileRAIICheck(StringRef Name,
ClangTidyContext *Context)
- : ClangTidyCheck(Name, Context),
- RAIIDenyList(
- utils::options::parseStringList(Options.get("RAIIDenyList", ""))) {}
+ : 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.
diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
index cd61523b6561a5a..27d152127dc3735 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
@@ -19,7 +19,7 @@ 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-check.html
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc/coroutine-hostile-raii.html
class CoroutineHostileRAIICheck : public ClangTidyCheck {
public:
CoroutineHostileRAIICheck(llvm::StringRef Name,
@@ -37,7 +37,7 @@ class CoroutineHostileRAIICheck : public ClangTidyCheck {
void checkVarDecl(VarDecl *VD);
// List of fully qualified types which should not persist across a suspension
// point in a coroutine.
- const std::vector<StringRef> RAIIDenyList;
+ std::vector<StringRef> RAIIDenyList;
};
} // namespace clang::tidy::misc
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 5047f91e33d67cd..771245f083fe488 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -241,7 +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-suspension-hostile <misc/coroutine-suspension-hostile.html>`_, "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/docs/clang-tidy/checks/misc/coroutine-suspension-hostile.rst b/clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-suspension-hostile.rst
deleted file mode 100644
index 262d7eedcd84fe9..000000000000000
--- a/clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-suspension-hostile.rst
+++ /dev/null
@@ -1,39 +0,0 @@
-.. 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.
-Since after a suspension a coroutine could potentially be resumed on a different thread,
-such RAII objects could be created by one thread but destroyed by another.
-Certain RAII types could be hostile to being destroyed by another thread.
-
-The check considers the following type as hostile:
-
- - Scoped-lockable types: A scoped-lockable object persisting across a suspension point in a coroutine is
- problematic as it is possible for the lock held by this object at the suspension
- point to 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 trailing `::` in the qualified name.
- Eg: `my::lockable; my::other::lockable;`
\ No newline at end of file
diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-suspension-hostile.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-hostile-raii.cpp
similarity index 98%
rename from clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-suspension-hostile.cpp
rename to clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-hostile-raii.cpp
index e83ce771aaed38c..d0c2bbb21db7e1a 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-suspension-hostile.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-hostile-raii.cpp
@@ -1,7 +1,7 @@
// 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'}}"
+// RUN: 'my::Mutex; ::my::other::Mutex'}}"
namespace std {
>From ab1f8231a3921812c9332fb6495ba1214728c3e3 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Wed, 11 Oct 2023 14:43:47 +0200
Subject: [PATCH 04/10] Fix formatting
---
.../clang-tidy/misc/CoroutineHostileRAIICheck.h | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
index 27d152127dc3735..84cb401508f4d40 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
@@ -1,4 +1,4 @@
-//===--- CoroutineHostileRAIICheck.h - clang-tidy -----------------*- C++ -*-===//
+//===--- 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.
@@ -22,8 +22,7 @@ namespace clang::tidy::misc {
/// http://clang.llvm.org/extra/clang-tidy/checks/misc/coroutine-hostile-raii.html
class CoroutineHostileRAIICheck : public ClangTidyCheck {
public:
- CoroutineHostileRAIICheck(llvm::StringRef Name,
- ClangTidyContext *Context);
+ CoroutineHostileRAIICheck(llvm::StringRef Name, ClangTidyContext *Context);
bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
return LangOpts.CPlusPlus20;
>From db7d9bc2a24bd7f0149b901a7c8cfa78a66e06f5 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Wed, 11 Oct 2023 15:13:10 +0200
Subject: [PATCH 05/10] Check rename (continued)
---
clang-tools-extra/docs/ReleaseNotes.rst | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index fd5d66f5c809eb1..31c861f83807b9c 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -180,10 +180,10 @@ 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-suspension-hostile
- <clang-tidy/checks/misc/coroutine-suspension-hostile>` check.
+- New :doc:`misc-coroutine-hostile-raii
+ <clang-tidy/checks/misc/coroutine-hostile-raii>` check.
- Detects when objects of certain hostile types persists across suspension points in a coroutine.
+ 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
>From 7197982c5a708592078dde32a4dab7f7865f8bc6 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Thu, 12 Oct 2023 19:30:31 +0200
Subject: [PATCH 06/10] Address comments
---
.../clang-tidy/misc/CoroutineHostileRAIICheck.cpp | 4 ++--
.../clang-tidy/misc/CoroutineHostileRAIICheck.h | 4 ++--
clang-tools-extra/docs/ReleaseNotes.rst | 12 ++++++------
.../checks/misc/coroutine-hostile-raii.rst | 8 ++++----
4 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
index 7b084cd993c35ee..e298f02968a58c9 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
@@ -1,4 +1,4 @@
-//===--- CoroutineSuspensionHostileCheck.cpp - clang-tidy --------------===//
+//===--- 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.
@@ -34,7 +34,7 @@ void CoroutineHostileRAIICheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(coyieldExpr().bind("suspension"), this);
}
-void CoroutineHostileRAIICheck::checkVarDecl(VarDecl *VD) {
+void CoroutineHostileRAIICheck::checkVarDecl(const VarDecl *VD) {
RecordDecl *RD = VD->getType().getCanonicalType()->getAsRecordDecl();
if (RD->hasAttr<clang::ScopedLockableAttr>()) {
diag(VD->getLocation(),
diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
index 84cb401508f4d40..41140497885a131 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
@@ -1,4 +1,4 @@
-//===--- CoroutineHostileRAIICheck.h - clang-tidy ---------------*- C++-*-===//
+//===--- 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.
@@ -33,7 +33,7 @@ class CoroutineHostileRAIICheck : public ClangTidyCheck {
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
private:
- void checkVarDecl(VarDecl *VD);
+ void checkVarDecl(const VarDecl *VD);
// List of fully qualified types which should not persist across a suspension
// point in a coroutine.
std::vector<StringRef> RAIIDenyList;
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 31c861f83807b9c..50a8c181c1bdd4c 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -163,6 +163,12 @@ 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.
@@ -180,12 +186,6 @@ 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/misc/coroutine-hostile-raii.rst b/clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-hostile-raii.rst
index 602d08fd7fe873a..b66ad86a9553def 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
@@ -9,11 +9,11 @@ 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
+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,
+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:
@@ -44,5 +44,5 @@ Options
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
+ Do not include ``::`` in the beginning of the qualified name.
+ Eg: ``my::lockable; a::b; ::my::other::lockable;``
\ No newline at end of file
>From 1565fae7e732f48943510f5ae567ab6ab54f11d6 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Mon, 16 Oct 2023 16:05:06 +0200
Subject: [PATCH 07/10] address comments
---
.../misc/CoroutineHostileRAIICheck.cpp | 29 +++++++---
.../misc/CoroutineHostileRAIICheck.h | 12 ++--
.../docs/clang-tidy/checks/list.rst | 2 +-
.../checks/misc/coroutine-hostile-raii.rst | 11 ++--
.../checkers/misc/coroutine-hostile-raii.cpp | 58 +++++++++++++++++--
5 files changed, 88 insertions(+), 24 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
index e298f02968a58c9..328a04f33efe67a 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
@@ -14,6 +14,7 @@
#include "clang/AST/Stmt.h"
#include "clang/AST/Type.h"
#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/DiagnosticIDs.h"
using namespace clang::ast_matchers;
@@ -21,10 +22,10 @@ namespace clang::tidy::misc {
CoroutineHostileRAIICheck::CoroutineHostileRAIICheck(StringRef Name,
ClangTidyContext *Context)
: ClangTidyCheck(Name, Context) {
- for (StringRef Denied :
- utils::options::parseStringList(Options.get("RAIIDenyList", ""))) {
+ for (StringRef Denied : utils::options::parseStringList(
+ Options.get("RAIITypesList", "std::lock_guard;std::scoped_lock"))) {
Denied.consume_front("::");
- RAIIDenyList.push_back(Denied);
+ RAIITypesList.push_back(Denied);
}
}
@@ -34,24 +35,34 @@ void CoroutineHostileRAIICheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(coyieldExpr().bind("suspension"), this);
}
-void CoroutineHostileRAIICheck::checkVarDecl(const VarDecl *VD) {
+void CoroutineHostileRAIICheck::checkVarDecl(const VarDecl *VD,
+ SourceLocation SuspensionLoc) {
RecordDecl *RD = VD->getType().getCanonicalType()->getAsRecordDecl();
+ if (!RD)
+ return;
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;
+ diag(SuspensionLoc, "suspension point is here", DiagnosticIDs::Note);
+ return;
}
- if (std::find(RAIIDenyList.begin(), RAIIDenyList.end(),
- RD->getQualifiedNameAsString()) != RAIIDenyList.end()) {
+ if (std::find(RAIITypesList.begin(), RAIITypesList.end(),
+ RD->getQualifiedNameAsString()) != RAIITypesList.end()) {
diag(VD->getLocation(),
"%0 persists across a suspension point of coroutine")
<< VD;
+ diag(SuspensionLoc, "suspension point is here", DiagnosticIDs::Note);
}
}
void CoroutineHostileRAIICheck::check(const MatchFinder::MatchResult &Result) {
const auto *Suspension = Result.Nodes.getNodeAs<Stmt>("suspension");
+ SourceLocation SuspensionLoc;
+ if(auto* E = dyn_cast<Expr>(Suspension)) {
+ SuspensionLoc = E->getExprLoc();
+ }
DynTypedNode P;
for (const Stmt *Child = Suspension; Child; Child = P.get<Stmt>()) {
auto Parents = Result.Context->getParents(*Child);
@@ -66,7 +77,7 @@ void CoroutineHostileRAIICheck::check(const MatchFinder::MatchResult &Result) {
if (auto *DS = dyn_cast<DeclStmt>(*Sibling)) {
for (Decl *D : DS->decls()) {
if (VarDecl *VD = dyn_cast<VarDecl>(D)) {
- checkVarDecl(VD);
+ checkVarDecl(VD, SuspensionLoc);
}
}
}
@@ -76,7 +87,7 @@ void CoroutineHostileRAIICheck::check(const MatchFinder::MatchResult &Result) {
void CoroutineHostileRAIICheck::storeOptions(
ClangTidyOptions::OptionMap &Opts) {
- Options.store(Opts, "RAIIDenyList",
- utils::options::serializeStringList(RAIIDenyList));
+ 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
index 41140497885a131..d53003ceae05dc3 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
@@ -11,15 +11,17 @@
#include "../ClangTidyCheck.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Basic/DiagnosticIDs.h"
#include "llvm/ADT/StringRef.h"
#include <vector>
namespace clang::tidy::misc {
-/// Check detects objects which should not to persist across suspension points
+/// This check detects hostile-RAII objects which should not persist across a
+/// suspension point in a coroutine.
///
-/// For the user-facing documentation see:
-/// http://clang.llvm.org/extra/clang-tidy/checks/misc/coroutine-hostile-raii.html
+/// 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);
@@ -33,10 +35,10 @@ class CoroutineHostileRAIICheck : public ClangTidyCheck {
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
private:
- void checkVarDecl(const VarDecl *VD);
+ void checkVarDecl(const VarDecl *VD, SourceLocation SuspensionLoc);
// List of fully qualified types which should not persist across a suspension
// point in a coroutine.
- std::vector<StringRef> RAIIDenyList;
+ std::vector<StringRef> RAIITypesList;
};
} // namespace clang::tidy::misc
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 771245f083fe488..633971615eb57f4 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -241,7 +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-coroutine-hostile-raii <misc/coroutine-hostile-raii.html>`_, "No"
: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
index b66ad86a9553def..660bab7175d1cf4 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
@@ -21,6 +21,7 @@ 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.
+ This includes all types annotated with the ``scoped_lockable`` attribute.
- Types belonging to a configurable denylist.
@@ -40,9 +41,9 @@ The check considers the following type as hostile:
Options
-------
-.. option:: RAIIDenyList
+.. option:: RAIITypesList
- 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
+ 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
index d0c2bbb21db7e1a..6c932238f3318f4 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,6 +1,6 @@
// RUN: %check_clang_tidy -std=c++20 %s misc-coroutine-hostile-raii %t \
// RUN: -config="{CheckOptions: \
-// RUN: {misc-coroutine-hostile-raii.RAIIDenyList: \
+// RUN: {misc-coroutine-hostile-raii.RAIITypesList: \
// RUN: 'my::Mutex; ::my::other::Mutex'}}"
namespace std {
@@ -80,8 +80,17 @@ class SCOPED_LOCKABLE Mutex {};
using Mutex2 = Mutex;
} // namespace absl
+ReturnObject scopedLockableBasic() {
+ 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 different thread [misc-coroutine-hostile-raii]
+ {
+ co_yield 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: note: suspension point is here
+ }
+}
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 different thread [misc-coroutine-hostile-raii]
absl::Mutex2 b;
@@ -92,13 +101,16 @@ ReturnObject scopedLockableTest() {
}
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 different thread [misc-coroutine-hostile-raii]
co_await std::suspend_always{};
- for(int i=1;i<=10;++i ) {
+ // 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 different 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;
}
@@ -106,13 +118,37 @@ ReturnObject scopedLockableTest() {
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;
+ // 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 different 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 different 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
@@ -128,4 +164,18 @@ ReturnObject denyListTest() {
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
+ // 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;
+}
>From 79d0109ad519dae8c8a32fe5bf8fd9857981b82a Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Tue, 17 Oct 2023 17:05:59 +0200
Subject: [PATCH 08/10] use ast matchers
---
.../misc/CoroutineHostileRAIICheck.cpp | 124 +++++++++++-------
.../misc/CoroutineHostileRAIICheck.h | 1 -
.../checkers/misc/coroutine-hostile-raii.cpp | 15 ++-
3 files changed, 87 insertions(+), 53 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
index 328a04f33efe67a..aa9a8b9a1019d8f 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
@@ -8,81 +8,105 @@
#include "CoroutineHostileRAIICheck.h"
#include "../utils/OptionsUtils.h"
-#include "clang/AST/Attrs.inc"
+#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 {
-CoroutineHostileRAIICheck::CoroutineHostileRAIICheck(StringRef Name,
- ClangTidyContext *Context)
- : ClangTidyCheck(Name, Context) {
- for (StringRef Denied : utils::options::parseStringList(
- Options.get("RAIITypesList", "std::lock_guard;std::scoped_lock"))) {
- Denied.consume_front("::");
- RAIITypesList.push_back(Denied);
+using ::clang::ast_matchers::internal::ASTMatchFinder;
+using clang::ast_matchers::internal::BoundNodesTreeBuilder;
+
+// 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).
+template <class Matcher, class Node>
+bool match(Matcher M, Node &N, ASTMatchFinder *Finder,
+ BoundNodesTreeBuilder *Builder) {
+ BoundNodesTreeBuilder LocalBuilder;
+ if (M.matches(N, Finder, &LocalBuilder)) {
+ Builder->addMatch(LocalBuilder);
+ return true;
}
+ return false;
}
-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);
+CoroutineHostileRAIICheck::CoroutineHostileRAIICheck(StringRef Name,
+ ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ RAIITypesList(utils::options::parseStringList(
+ Options.get("RAIITypesList", "std::lock_guard;std::scoped_lock"))) {}
+
+AST_MATCHER_P(VarDecl, isHostileRAII, std::vector<StringRef>, RAIITypesList) {
+ return match(varDecl(hasType(hasCanonicalType(hasDeclaration(
+ hasAttr(attr::Kind::ScopedLockable)))))
+ .bind("scoped-lockable"),
+ Node, Finder, Builder) ||
+ match(varDecl(hasType(hasCanonicalType(hasDeclaration(
+ namedDecl(hasAnyName(RAIITypesList))))))
+ .bind("raii"),
+ Node, Finder, Builder);
}
-void CoroutineHostileRAIICheck::checkVarDecl(const VarDecl *VD,
- SourceLocation SuspensionLoc) {
- RecordDecl *RD = VD->getType().getCanonicalType()->getAsRecordDecl();
- if (!RD)
- return;
- 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;
- diag(SuspensionLoc, "suspension point is here", DiagnosticIDs::Note);
- return;
- }
- if (std::find(RAIITypesList.begin(), RAIITypesList.end(),
- RD->getQualifiedNameAsString()) != RAIITypesList.end()) {
- diag(VD->getLocation(),
- "%0 persists across a suspension point of coroutine")
- << VD;
- diag(SuspensionLoc, "suspension point is here", DiagnosticIDs::Note);
- }
+AST_MATCHER_P(DeclStmt, hasHostileRAII, std::vector<StringRef>, RAIITypesList) {
+ bool IsHostile = false;
+ for (Decl *D : Node.decls())
+ IsHostile |=
+ match(varDecl(isHostileRAII(RAIITypesList)), *D, Finder, Builder);
+ return IsHostile;
}
-void CoroutineHostileRAIICheck::check(const MatchFinder::MatchResult &Result) {
- const auto *Suspension = Result.Nodes.getNodeAs<Stmt>("suspension");
- SourceLocation SuspensionLoc;
- if(auto* E = dyn_cast<Expr>(Suspension)) {
- SuspensionLoc = E->getExprLoc();
- }
+AST_MATCHER_P(Expr, isHostileSuspension, std::vector<StringRef>,
+ RAIITypesList) {
DynTypedNode P;
- for (const Stmt *Child = Suspension; Child; Child = P.get<Stmt>()) {
- auto Parents = Result.Context->getParents(*Child);
+ 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 (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, SuspensionLoc);
- }
- }
- }
+ for (const auto &Sibling : PCS->children()) {
+ // Child contains suspension. Siblings after Child do not persist across
+ // this suspension.
+ if (Sibling == Child)
+ break;
+ IsHostile |= match(declStmt(hasHostileRAII(RAIITypesList)), *Sibling,
+ Finder, Builder);
}
}
+ return IsHostile;
+}
+
+void CoroutineHostileRAIICheck::registerMatchers(MatchFinder *Finder) {
+ // A suspension happens with co_await or co_yield.
+ Finder->addMatcher(
+ coawaitExpr(isHostileSuspension(RAIITypesList)).bind("suspension"), this);
+ Finder->addMatcher(
+ coyieldExpr(isHostileSuspension(RAIITypesList)).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 different 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(
diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
index d53003ceae05dc3..02003456a784ad4 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
@@ -35,7 +35,6 @@ class CoroutineHostileRAIICheck : public ClangTidyCheck {
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
private:
- void checkVarDecl(const VarDecl *VD, SourceLocation SuspensionLoc);
// List of fully qualified types which should not persist across a suspension
// point in a coroutine.
std::vector<StringRef> RAIITypesList;
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 6c932238f3318f4..2d022e21c85d566 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
@@ -72,7 +72,6 @@ struct ReturnObject {
};
};
-
#define SCOPED_LOCKABLE __attribute__ ((scoped_lockable))
namespace absl {
@@ -80,15 +79,27 @@ class SCOPED_LOCKABLE Mutex {};
using Mutex2 = Mutex;
} // namespace absl
-ReturnObject scopedLockableBasic() {
+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 different 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;
>From bd3101fe24159c17e399155b76cc8b146b71f78e Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Tue, 17 Oct 2023 19:12:42 +0200
Subject: [PATCH 09/10] use more inner matchers
---
.../misc/CoroutineHostileRAIICheck.cpp | 61 ++++++-------------
.../misc/CoroutineHostileRAIICheck.h | 10 ++-
.../checks/misc/coroutine-hostile-raii.rst | 6 +-
3 files changed, 30 insertions(+), 47 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
index aa9a8b9a1019d8f..e4fd6945871a181 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
@@ -21,49 +21,16 @@
using namespace clang::ast_matchers;
namespace clang::tidy::misc {
-using ::clang::ast_matchers::internal::ASTMatchFinder;
using clang::ast_matchers::internal::BoundNodesTreeBuilder;
-// 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).
-template <class Matcher, class Node>
-bool match(Matcher M, Node &N, ASTMatchFinder *Finder,
- BoundNodesTreeBuilder *Builder) {
- BoundNodesTreeBuilder LocalBuilder;
- if (M.matches(N, Finder, &LocalBuilder)) {
- Builder->addMatch(LocalBuilder);
- return true;
- }
- return false;
-}
-
CoroutineHostileRAIICheck::CoroutineHostileRAIICheck(StringRef Name,
ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
RAIITypesList(utils::options::parseStringList(
Options.get("RAIITypesList", "std::lock_guard;std::scoped_lock"))) {}
-AST_MATCHER_P(VarDecl, isHostileRAII, std::vector<StringRef>, RAIITypesList) {
- return match(varDecl(hasType(hasCanonicalType(hasDeclaration(
- hasAttr(attr::Kind::ScopedLockable)))))
- .bind("scoped-lockable"),
- Node, Finder, Builder) ||
- match(varDecl(hasType(hasCanonicalType(hasDeclaration(
- namedDecl(hasAnyName(RAIITypesList))))))
- .bind("raii"),
- Node, Finder, Builder);
-}
-
-AST_MATCHER_P(DeclStmt, hasHostileRAII, std::vector<StringRef>, RAIITypesList) {
- bool IsHostile = false;
- for (Decl *D : Node.decls())
- IsHostile |=
- match(varDecl(isHostileRAII(RAIITypesList)), *D, Finder, Builder);
- return IsHostile;
-}
-
-AST_MATCHER_P(Expr, isHostileSuspension, std::vector<StringRef>,
- RAIITypesList) {
+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>()) {
@@ -79,8 +46,13 @@ AST_MATCHER_P(Expr, isHostileSuspension, std::vector<StringRef>,
// this suspension.
if (Sibling == Child)
break;
- IsHostile |= match(declStmt(hasHostileRAII(RAIITypesList)), *Sibling,
- Finder, Builder);
+ // 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;
@@ -88,10 +60,17 @@ AST_MATCHER_P(Expr, isHostileSuspension, std::vector<StringRef>,
void CoroutineHostileRAIICheck::registerMatchers(MatchFinder *Finder) {
// A suspension happens with co_await or co_yield.
- Finder->addMatcher(
- coawaitExpr(isHostileSuspension(RAIITypesList)).bind("suspension"), this);
- Finder->addMatcher(
- coyieldExpr(isHostileSuspension(RAIITypesList)).bind("suspension"), this);
+ 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) {
diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
index 02003456a784ad4..920894102ba953b 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
@@ -10,15 +10,16 @@
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_COROUTINESHOSTILERAIICHECK_H
#include "../ClangTidyCheck.h"
+#include "clang/AST/ASTTypeTraits.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
-#include "clang/Basic/DiagnosticIDs.h"
#include "llvm/ADT/StringRef.h"
#include <vector>
namespace clang::tidy::misc {
-/// This check detects hostile-RAII objects which should not persist across a
-/// suspension point in a coroutine.
+/// 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
@@ -34,6 +35,9 @@ class CoroutineHostileRAIICheck : public ClangTidyCheck {
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.
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 660bab7175d1cf4..501706b058c3bc4 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
@@ -3,8 +3,8 @@
misc-coroutine-hostile-raii
====================
-This check detects hostile-RAII objects which should not persist across a
-suspension point in a coroutine.
+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",
@@ -16,7 +16,7 @@ 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:
+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
>From a5a88c3d354e2f2ede23bfecb809f2efe846953f Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Tue, 17 Oct 2023 20:38:55 +0200
Subject: [PATCH 10/10] doc fixes and clang-format
---
.../clang-tidy/misc/CoroutineHostileRAIICheck.cpp | 14 ++++++++------
.../clang-tidy/misc/CoroutineHostileRAIICheck.h | 1 +
clang-tools-extra/docs/ReleaseNotes.rst | 5 +++--
clang-tools-extra/docs/clang-tidy/checks/list.rst | 2 +-
.../checks/misc/coroutine-hostile-raii.rst | 7 ++++---
5 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
index e4fd6945871a181..e820cd39d83d21b 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
@@ -21,14 +21,9 @@
using namespace clang::ast_matchers;
namespace clang::tidy::misc {
+namespace {
using clang::ast_matchers::internal::BoundNodesTreeBuilder;
-CoroutineHostileRAIICheck::CoroutineHostileRAIICheck(StringRef Name,
- ClangTidyContext *Context)
- : ClangTidyCheck(Name, Context),
- RAIITypesList(utils::options::parseStringList(
- Options.get("RAIITypesList", "std::lock_guard;std::scoped_lock"))) {}
-
AST_MATCHER_P(Stmt, forEachPrevStmt, ast_matchers::internal::Matcher<Stmt>,
InnerMatcher) {
DynTypedNode P;
@@ -57,6 +52,13 @@ AST_MATCHER_P(Stmt, forEachPrevStmt, ast_matchers::internal::Matcher<Stmt>,
}
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.
diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
index 920894102ba953b..a5e9cb89ef67695 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
@@ -38,6 +38,7 @@ class CoroutineHostileRAIICheck : public ClangTidyCheck {
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.
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 50a8c181c1bdd4c..c61c5b8fb474cb9 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -166,8 +166,9 @@ New checks
- 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.
+ 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 633971615eb57f4..c8e5707a8d117f7 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -241,7 +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>`_, "No"
+ :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
index 501706b058c3bc4..dcb9f399774cba9 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
@@ -3,8 +3,9 @@
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.
+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",
@@ -46,4 +47,4 @@ Options
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
+ The default value of this option is `"std::lock_guard;std::scoped_lock"`.
\ No newline at end of file
More information about the cfe-commits
mailing list