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 <u...@google.com> Date: Tue, 10 Oct 2023 21:53:37 +0200 Subject: [PATCH 1/5] [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 <u...@google.com> Date: Wed, 11 Oct 2023 14:08:48 +0200 Subject: [PATCH 2/5] 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 <u...@google.com> Date: Wed, 11 Oct 2023 14:41:48 +0200 Subject: [PATCH 3/5] 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 <u...@google.com> Date: Wed, 11 Oct 2023 14:43:47 +0200 Subject: [PATCH 4/5] 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 <u...@google.com> Date: Wed, 11 Oct 2023 15:13:10 +0200 Subject: [PATCH 5/5] 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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
