llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy Author: Utkarsh Saxena (usx95) <details> <summary>Changes</summary> 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**. ```cpp // Call some async API while holding a lock. const my::MutexLock l(&mu_); // Oops! The async Bar function may finish on a different // thread from the one that created the MutexLock object and therefore called // Mutex::Lock -- now Mutex::Unlock will be called on the wrong thread. co_await Bar(); ``` --- Full diff: https://github.com/llvm/llvm-project/pull/68738.diff 8 Files Affected: - (modified) clang-tools-extra/clang-tidy/misc/CMakeLists.txt (+1) - (added) clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp (+82) - (added) clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h (+44) - (modified) clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp (+3) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+6) - (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1) - (added) clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-hostile-raii.rst (+48) - (added) clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-hostile-raii.cpp (+131) ``````````diff diff --git a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt index 2e88e68a5447825..d9ec268650c0532 100644 --- a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt @@ -18,6 +18,7 @@ add_custom_target(genconfusable DEPENDS Confusables.inc) add_clang_library(clangTidyMiscModule ConstCorrectnessCheck.cpp + CoroutineHostileRAIICheck.cpp DefinitionsInHeadersCheck.cpp ConfusableIdentifierCheck.cpp HeaderIncludeCycleCheck.cpp diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp new file mode 100644 index 000000000000000..7b084cd993c35ee --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp @@ -0,0 +1,82 @@ +//===--- CoroutineSuspensionHostileCheck.cpp - clang-tidy --------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "CoroutineHostileRAIICheck.h" +#include "../utils/OptionsUtils.h" +#include "clang/AST/Attrs.inc" +#include "clang/AST/Decl.h" +#include "clang/AST/ExprCXX.h" +#include "clang/AST/Stmt.h" +#include "clang/AST/Type.h" +#include "clang/ASTMatchers/ASTMatchers.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::misc { +CoroutineHostileRAIICheck::CoroutineHostileRAIICheck(StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) { + for (StringRef Denied : + utils::options::parseStringList(Options.get("RAIIDenyList", ""))) { + Denied.consume_front("::"); + RAIIDenyList.push_back(Denied); + } +} + +void CoroutineHostileRAIICheck::registerMatchers(MatchFinder *Finder) { + // A suspension happens with co_await or co_yield. + Finder->addMatcher(coawaitExpr().bind("suspension"), this); + Finder->addMatcher(coyieldExpr().bind("suspension"), this); +} + +void CoroutineHostileRAIICheck::checkVarDecl(VarDecl *VD) { + RecordDecl *RD = VD->getType().getCanonicalType()->getAsRecordDecl(); + if (RD->hasAttr<clang::ScopedLockableAttr>()) { + diag(VD->getLocation(), + "%0 holds a lock across a suspension point of coroutine and could be " + "unlocked by a different thread") + << VD; + } + if (std::find(RAIIDenyList.begin(), RAIIDenyList.end(), + RD->getQualifiedNameAsString()) != RAIIDenyList.end()) { + diag(VD->getLocation(), + "%0 persists across a suspension point of coroutine") + << VD; + } +} + +void CoroutineHostileRAIICheck::check(const MatchFinder::MatchResult &Result) { + const auto *Suspension = Result.Nodes.getNodeAs<Stmt>("suspension"); + DynTypedNode P; + for (const Stmt *Child = Suspension; Child; Child = P.get<Stmt>()) { + auto Parents = Result.Context->getParents(*Child); + if (Parents.empty()) + break; + P = *Parents.begin(); + auto *PCS = P.get<CompoundStmt>(); + if (!PCS) + continue; + for (auto Sibling = PCS->child_begin(); + *Sibling != Child && Sibling != PCS->child_end(); ++Sibling) { + if (auto *DS = dyn_cast<DeclStmt>(*Sibling)) { + for (Decl *D : DS->decls()) { + if (VarDecl *VD = dyn_cast<VarDecl>(D)) { + checkVarDecl(VD); + } + } + } + } + } +} + +void CoroutineHostileRAIICheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "RAIIDenyList", + utils::options::serializeStringList(RAIIDenyList)); +} +} // namespace clang::tidy::misc diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h new file mode 100644 index 000000000000000..84cb401508f4d40 --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h @@ -0,0 +1,44 @@ +//===--- CoroutineHostileRAIICheck.h - clang-tidy ---------------*- C++-*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_COROUTINESHOSTILERAIICHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_COROUTINESHOSTILERAIICHECK_H + +#include "../ClangTidyCheck.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "llvm/ADT/StringRef.h" +#include <vector> + +namespace clang::tidy::misc { + +/// Check detects objects which should not to persist across suspension points +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/misc/coroutine-hostile-raii.html +class CoroutineHostileRAIICheck : public ClangTidyCheck { +public: + CoroutineHostileRAIICheck(llvm::StringRef Name, ClangTidyContext *Context); + + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus20; + } + + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + void checkVarDecl(VarDecl *VD); + // List of fully qualified types which should not persist across a suspension + // point in a coroutine. + std::vector<StringRef> RAIIDenyList; +}; + +} // namespace clang::tidy::misc + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_COROUTINESHOSTILERAIICHECK_H diff --git a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp index 92590506e1ec1e6..d8a88324ee63e08 100644 --- a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp @@ -11,6 +11,7 @@ #include "../ClangTidyModuleRegistry.h" #include "ConfusableIdentifierCheck.h" #include "ConstCorrectnessCheck.h" +#include "CoroutineHostileRAIICheck.h" #include "DefinitionsInHeadersCheck.h" #include "HeaderIncludeCycleCheck.h" #include "IncludeCleanerCheck.h" @@ -41,6 +42,8 @@ class MiscModule : public ClangTidyModule { "misc-confusable-identifiers"); CheckFactories.registerCheck<ConstCorrectnessCheck>( "misc-const-correctness"); + CheckFactories.registerCheck<CoroutineHostileRAIICheck>( + "misc-coroutine-hostile-raii"); CheckFactories.registerCheck<DefinitionsInHeadersCheck>( "misc-definitions-in-headers"); CheckFactories.registerCheck<HeaderIncludeCycleCheck>( diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 03e5dc6f164af2a..31c861f83807b9c 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -180,6 +180,12 @@ New checks Detects C++ code where a reference variable is used to extend the lifetime of a temporary object that has just been constructed. +- New :doc:`misc-coroutine-hostile-raii + <clang-tidy/checks/misc/coroutine-hostile-raii>` check. + + Detects when objects of certain hostile RAII types persists across suspension points in a coroutine. + Such hostile types include scoped-lockable types and types belonging to a configurable denylist. + New check aliases ^^^^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 1baabceea06ef48..771245f083fe488 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -241,6 +241,7 @@ Clang-Tidy Checks :doc:`llvmlibc-restrict-system-libc-headers <llvmlibc/restrict-system-libc-headers>`, "Yes" :doc:`misc-confusable-identifiers <misc/confusable-identifiers>`, :doc:`misc-const-correctness <misc/const-correctness>`, "Yes" + :doc:`misc-coroutine-hostile-raii <misc/coroutine-hostile-raii.html>`_, "Yes" :doc:`misc-definitions-in-headers <misc/definitions-in-headers>`, "Yes" :doc:`misc-header-include-cycle <misc/header-include-cycle>`, :doc:`misc-include-cleaner <misc/include-cleaner>`, "Yes" diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-hostile-raii.rst b/clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-hostile-raii.rst new file mode 100644 index 000000000000000..602d08fd7fe873a --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-hostile-raii.rst @@ -0,0 +1,48 @@ +.. title:: clang-tidy - misc-coroutine-hostile-raii + +misc-coroutine-hostile-raii +==================== + +This check detects hostile-RAII objects which should not persist across a +suspension point in a coroutine. + +Some objects require that they be destroyed on the same thread that created them. +Traditionally this requirement was often phrased as "must be a local variable", +under the assumption that local variables always work this way. However this is +incorrect with C++20 coroutines, since an intervening `co_await` may cause the +coroutine to suspend and later be resumed on another thread. + +The lifetime of an object that requires being destroyed on the same thread must +not encompass a `co_await` or `co_yield` point. If you create/destroy an object, +you must do so without allowing the coroutine to suspend in the meantime. + +The check considers the following type as hostile: + + - Scoped-lockable types: A scoped-lockable object persisting across a suspension + point is problematic as the lock held by this object could be unlocked by a + different thread. This would be undefined behaviour. + + - Types belonging to a configurable denylist. + +.. code-block:: c++ + + // Call some async API while holding a lock. + { + const my::MutexLock l(&mu_); + + // Oops! The async Bar function may finish on a different + // thread from the one that created the MutexLock object and therefore called + // Mutex::Lock -- now Mutex::Unlock will be called on the wrong thread. + co_await Bar(); + } + + +Options +------- + +.. option:: RAIIDenyList + + A semicolon-separated list of qualified types which should not be allowed to + persist across suspension points. + Do not include `::` in the beginning of the qualified name. + Eg: `my::lockable; a::b; ::my::other::lockable;` \ No newline at end of file diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-hostile-raii.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-hostile-raii.cpp new file mode 100644 index 000000000000000..d0c2bbb21db7e1a --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-hostile-raii.cpp @@ -0,0 +1,131 @@ +// RUN: %check_clang_tidy -std=c++20 %s misc-coroutine-hostile-raii %t \ +// RUN: -config="{CheckOptions: \ +// RUN: {misc-coroutine-hostile-raii.RAIIDenyList: \ +// RUN: 'my::Mutex; ::my::other::Mutex'}}" + +namespace std { + +template <typename R, typename...> struct coroutine_traits { + using promise_type = typename R::promise_type; +}; + +template <typename Promise = void> struct coroutine_handle; + +template <> struct coroutine_handle<void> { + static coroutine_handle from_address(void *addr) noexcept { + coroutine_handle me; + me.ptr = addr; + return me; + } + void operator()() { resume(); } + void *address() const noexcept { return ptr; } + void resume() const { } + void destroy() const { } + bool done() const { return true; } + coroutine_handle &operator=(decltype(nullptr)) { + ptr = nullptr; + return *this; + } + coroutine_handle(decltype(nullptr)) : ptr(nullptr) {} + coroutine_handle() : ptr(nullptr) {} + // void reset() { ptr = nullptr; } // add to P0057? + explicit operator bool() const { return ptr; } + +protected: + void *ptr; +}; + +template <typename Promise> struct coroutine_handle : coroutine_handle<> { + using coroutine_handle<>::operator=; + + static coroutine_handle from_address(void *addr) noexcept { + coroutine_handle me; + me.ptr = addr; + return me; + } + + Promise &promise() const { + return *reinterpret_cast<Promise *>( + __builtin_coro_promise(ptr, alignof(Promise), false)); + } + static coroutine_handle from_promise(Promise &promise) { + coroutine_handle p; + p.ptr = __builtin_coro_promise(&promise, alignof(Promise), true); + return p; + } +}; + +struct suspend_always { + bool await_ready() noexcept { return false; } + void await_suspend(std::coroutine_handle<>) noexcept {} + void await_resume() noexcept {} +}; +} // namespace std + +struct ReturnObject { + struct promise_type { + ReturnObject get_return_object() { return {}; } + std::suspend_always initial_suspend() { return {}; } + std::suspend_always final_suspend() noexcept { return {}; } + void unhandled_exception() {} + std::suspend_always yield_value(int value) { return {}; } + }; +}; + + +#define SCOPED_LOCKABLE __attribute__ ((scoped_lockable)) + +namespace absl { +class SCOPED_LOCKABLE Mutex {}; +using Mutex2 = Mutex; +} // namespace absl + + +ReturnObject scopedLockableTest() { + absl::Mutex a; + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: 'a' holds a lock across a suspension point of coroutine and could be unlocked by a different thread [misc-coroutine-hostile-raii] + absl::Mutex2 b; + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'b' holds a lock across a suspension point of coroutine and could be unlocked by a different thread [misc-coroutine-hostile-raii] + { + absl::Mutex no_warning_1; + { absl::Mutex no_warning_2; } + } + + co_yield 1; + absl::Mutex c; + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: 'c' holds a lock across a suspension point of coroutine and could be unlocked by a different thread [misc-coroutine-hostile-raii] + co_await std::suspend_always{}; + for(int i=1;i<=10;++i ) { + absl::Mutex d; + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 'd' holds a lock across a suspension point of coroutine and could be unlocked by a different thread [misc-coroutine-hostile-raii] + co_await std::suspend_always{}; + co_yield 1; + absl::Mutex no_warning_3; + } + if (true) { + absl::Mutex e; + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 'e' holds a lock across a suspension point of coroutine and could be unlocked by a different thread [misc-coroutine-hostile-raii] + co_yield 1; + absl::Mutex no_warning_4; + } + absl::Mutex no_warning_5; +} +namespace my { +class Mutex{}; + +namespace other { +class Mutex{}; +} // namespace other + +using Mutex2 = Mutex; +} // namespace my + +ReturnObject denyListTest() { + my::Mutex a; + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'a' persists across a suspension point of coroutine [misc-coroutine-hostile-raii] + my::other::Mutex b; + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: 'b' persists across a suspension point of coroutine [misc-coroutine-hostile-raii] + my::Mutex2 c; + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'c' persists across a suspension point of coroutine [misc-coroutine-hostile-raii] + co_yield 1; +} \ No newline at end of file `````````` </details> https://github.com/llvm/llvm-project/pull/68738 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
