https://github.com/usx95 created https://github.com/llvm/llvm-project/pull/68738
None >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] [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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
