diff --git a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt index 2e88e68a54478..d9ec268650c05 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 0000000000000..e820cd39d83d2 --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp @@ -0,0 +1,98 @@ +//===--- 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. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "CoroutineHostileRAIICheck.h" +#include "../utils/OptionsUtils.h" +#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 { +namespace { +using clang::ast_matchers::internal::BoundNodesTreeBuilder; + +AST_MATCHER_P(Stmt, forEachPrevStmt, ast_matchers::internal::Matcher, + InnerMatcher) { + DynTypedNode P; + bool IsHostile = false; + for (const Stmt *Child = &Node; Child; Child = P.get()) { + auto Parents = Finder->getASTContext().getParents(*Child); + if (Parents.empty()) + break; + P = *Parents.begin(); + auto *PCS = P.get(); + if (!PCS) + continue; + for (const auto &Sibling : PCS->children()) { + // Child contains suspension. Siblings after Child do not persist across + // this suspension. + if (Sibling == Child) + break; + // 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; +} +} // 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. + 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) { + if (const auto *VD = Result.Nodes.getNodeAs("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("raii")) + diag(VD->getLocation(), + "%0 persists across a suspension point of coroutine") + << VD; + if (const auto *Suspension = Result.Nodes.getNodeAs("suspension")) + diag(Suspension->getBeginLoc(), "suspension point is here", + DiagnosticIDs::Note); +} + +void CoroutineHostileRAIICheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + 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 new file mode 100644 index 0000000000000..a5e9cb89ef676 --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h @@ -0,0 +1,50 @@ +//===--- 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/AST/ASTTypeTraits.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "llvm/ADT/StringRef.h" +#include + +namespace clang::tidy::misc { + +/// 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 +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; + + std::optional getCheckTraversalKind() const override { + return TK_AsIs; + } + +private: + // List of fully qualified types which should not persist across a suspension + // point in a coroutine. + std::vector RAIITypesList; +}; + +} // 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 92590506e1ec1..d8a88324ee63e 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( "misc-const-correctness"); + CheckFactories.registerCheck( + "misc-coroutine-hostile-raii"); CheckFactories.registerCheck( "misc-definitions-in-headers"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index af164d0462d52..3e1fbe091c9ff 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -163,6 +163,13 @@ New checks Flags coroutines that suspend while a lock guard is in scope at the suspension point. +- New :doc:`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 ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 2125ebd7a213c..819e3974e3f13 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 `, "Yes" :doc:`misc-confusable-identifiers `, :doc:`misc-const-correctness `, "Yes" + :doc:`misc-coroutine-hostile-raii `_, :doc:`misc-definitions-in-headers `, "Yes" :doc:`misc-header-include-cycle `, :doc:`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 0000000000000..dcb9f399774cb --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-hostile-raii.rst @@ -0,0 +1,50 @@ +.. title:: clang-tidy - misc-coroutine-hostile-raii + +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. + +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. + +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 + different thread. This would be undefined behaviour. + This includes all types annotated with the ``scoped_lockable`` attribute. + + - 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:: RAIITypesList + + 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 new file mode 100644 index 0000000000000..2d022e21c85d5 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-hostile-raii.cpp @@ -0,0 +1,192 @@ +// RUN: %check_clang_tidy -std=c++20 %s misc-coroutine-hostile-raii %t \ +// RUN: -config="{CheckOptions: \ +// RUN: {misc-coroutine-hostile-raii.RAIITypesList: \ +// RUN: 'my::Mutex; ::my::other::Mutex'}}" + +namespace std { + +template struct coroutine_traits { + using promise_type = typename R::promise_type; +}; + +template struct coroutine_handle; + +template <> struct coroutine_handle { + 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 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( + __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 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; + // 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; + // 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{}; + // 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; + } + 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; + // 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 +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(); } + +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; + // 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; +}