[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
Dmitry Polukhin via cfe-commits
cfe-commits at lists.llvm.org
Mon Jun 9 06:38:32 PDT 2025
https://github.com/dmpolukhin updated https://github.com/llvm/llvm-project/pull/140912
>From f277796a98f81bc3f0c6949adff3ad43eda51d5f Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin <dmitry.polukhin at gmail.com>
Date: Wed, 21 May 2025 07:54:02 -0700
Subject: [PATCH 1/4] [clang-tidy][performance-unnecessary-value-param] Avoid
in coroutines
Summary:
Replacing by-value parameters with passing by-reference is not safe for coroutines
because the caller may be executed in parallel with the callee, which increases
the chances of resulting in dangling references and hard-to-find crashes.
Test Plan: check-clang-tools
---
.../UnnecessaryValueParamCheck.cpp | 1 +
.../unnecessary-value-param-coroutine.cpp | 81 +++++++++++++++++++
2 files changed, 82 insertions(+)
create mode 100644 clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-coroutine.cpp
diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
index a877f9a7ee912..52d4c70a83f65 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -65,6 +65,7 @@ void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) {
TK_AsIs,
functionDecl(hasBody(stmt()), isDefinition(), unless(isImplicit()),
unless(cxxMethodDecl(anyOf(isOverride(), isFinal()))),
+ unless(hasBody(coroutineBodyStmt())),
has(typeLoc(forEach(ExpensiveValueParamDecl))),
decl().bind("functionDecl"))),
this);
diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-coroutine.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-coroutine.cpp
new file mode 100644
index 0000000000000..f2fb477143578
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-coroutine.cpp
@@ -0,0 +1,81 @@
+// RUN: %check_clang_tidy -std=c++20-or-later %s performance-unnecessary-value-param %t -- -fix-errors
+
+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 {}; }
+ };
+};
+
+struct A {
+ A(const A&);
+};
+
+ReturnObject evaluateModels(const A timeMachineId) {
+// No change for non-coroutine function expected because it is not safe.
+// CHECK-FIXES: ReturnObject evaluateModels(const A timeMachineId) {
+ co_return;
+}
>From 011624d536cd5641be54260c3eca2980d4094b45 Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin <dmitry.polukhin at gmail.com>
Date: Wed, 21 May 2025 09:19:56 -0700
Subject: [PATCH 2/4] Add documentation and simplify test
---
clang-tools-extra/docs/ReleaseNotes.rst | 2 +
.../performance/unnecessary-value-param.rst | 4 ++
.../unnecessary-value-param-coroutine.cpp | 63 ++++++-------------
3 files changed, 24 insertions(+), 45 deletions(-)
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 19ccd1790e757..f1a8819e21788 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -265,6 +265,8 @@ Changes in existing checks
<clang-tidy/checks/performance/unnecessary-value-param>` check performance by
tolerating fix-it breaking compilation when functions is used as pointers
to avoid matching usage of functions within the current compilation unit.
+ Also suppressed this check for coroutine functions because it may not be safe
+ and suggested fixes may result in hard-to-find bugs and crashes.
- Improved :doc:`readability-convert-member-functions-to-static
<clang-tidy/checks/readability/convert-member-functions-to-static>` check by
diff --git a/clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-value-param.rst b/clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-value-param.rst
index dc86530b95f13..665e0a6360723 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-value-param.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-value-param.rst
@@ -18,6 +18,10 @@ the following heuristic is employed:
on it, or it is used as const reference or value argument in constructors or
function calls.
+Note: This check does not suggest passing parameters by reference in coroutines
+because, after a coroutine suspend point, references could be dangling and no
+longer valid, so suggested changes may result in hard-to-find bugs and crashes.
+
Example:
.. code-block:: c++
diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-coroutine.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-coroutine.cpp
index f2fb477143578..b6e9bd0a60d44 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-coroutine.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-coroutine.cpp
@@ -2,59 +2,32 @@
namespace std {
-template <typename R, typename...> struct coroutine_traits {
- using promise_type = typename R::promise_type;
+template <class Ret, typename... T> struct coroutine_traits {
+ using promise_type = typename Ret::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 <class Promise = void> struct coroutine_handle {
+ static coroutine_handle from_address(void *) noexcept;
+ static coroutine_handle from_promise(Promise &promise);
+ constexpr void *address() const noexcept;
};
-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;
- }
+template <> struct coroutine_handle<void> {
+ template <class PromiseType>
+ coroutine_handle(coroutine_handle<PromiseType>) noexcept;
+ static coroutine_handle from_address(void *);
+ constexpr void *address() const noexcept;
};
struct suspend_always {
bool await_ready() noexcept { return false; }
- void await_suspend(std::coroutine_handle<>) noexcept {}
+ void await_suspend(coroutine_handle<>) noexcept {}
+ void await_resume() noexcept {}
+};
+
+struct suspend_never {
+ bool await_ready() noexcept { return true; }
+ void await_suspend(coroutine_handle<>) noexcept {}
void await_resume() noexcept {}
};
>From 6f53984f854310522d37b8d0315e33443bbbc7eb Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin <dmitry.polukhin at gmail.com>
Date: Wed, 21 May 2025 09:54:34 -0700
Subject: [PATCH 3/4] Move note to more appropiate place
---
.../checks/performance/unnecessary-value-param.rst | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-value-param.rst b/clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-value-param.rst
index 665e0a6360723..fce3bec00504e 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-value-param.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-value-param.rst
@@ -18,10 +18,6 @@ the following heuristic is employed:
on it, or it is used as const reference or value argument in constructors or
function calls.
-Note: This check does not suggest passing parameters by reference in coroutines
-because, after a coroutine suspend point, references could be dangling and no
-longer valid, so suggested changes may result in hard-to-find bugs and crashes.
-
Example:
.. code-block:: c++
@@ -60,7 +56,11 @@ Will become:
Because the fix-it needs to change the signature of the function, it may break
builds if the function is used in multiple translation units or some codes
-depends on funcion signatures.
+depends on function signatures.
+
+Note: This check does not suggest passing parameters by reference in coroutines
+because, after a coroutine suspend point, references could be dangling and no
+longer valid, so suggested changes may result in hard-to-find bugs and crashes.
Options
-------
>From 47640912484b2e32cbe9fa51b83c18552e7cb6dd Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin <dmitry.polukhin at gmail.com>
Date: Mon, 9 Jun 2025 06:19:43 -0700
Subject: [PATCH 4/4] Added option IsAllowedInCoroutines
---
.../performance/UnnecessaryValueParamCheck.cpp | 9 +++++++--
.../performance/UnnecessaryValueParamCheck.h | 1 +
clang-tools-extra/docs/ReleaseNotes.rst | 4 ++--
.../checks/performance/unnecessary-value-param.rst | 10 ++++++----
.../performance/unnecessary-value-param-coroutine.cpp | 4 ++++
5 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
index 52d4c70a83f65..b3e5910e113b5 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -50,7 +50,8 @@ UnnecessaryValueParamCheck::UnnecessaryValueParamCheck(
utils::IncludeSorter::IS_LLVM),
areDiagsSelfContained()),
AllowedTypes(
- utils::options::parseStringList(Options.get("AllowedTypes", ""))) {}
+ utils::options::parseStringList(Options.get("AllowedTypes", ""))),
+ IsAllowedInCoroutines(Options.get("IsAllowedInCoroutines", false)) {}
void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) {
const auto ExpensiveValueParamDecl = parmVarDecl(
@@ -65,7 +66,6 @@ void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) {
TK_AsIs,
functionDecl(hasBody(stmt()), isDefinition(), unless(isImplicit()),
unless(cxxMethodDecl(anyOf(isOverride(), isFinal()))),
- unless(hasBody(coroutineBodyStmt())),
has(typeLoc(forEach(ExpensiveValueParamDecl))),
decl().bind("functionDecl"))),
this);
@@ -74,6 +74,10 @@ void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) {
void UnnecessaryValueParamCheck::check(const MatchFinder::MatchResult &Result) {
const auto *Param = Result.Nodes.getNodeAs<ParmVarDecl>("param");
const auto *Function = Result.Nodes.getNodeAs<FunctionDecl>("functionDecl");
+ if (!IsAllowedInCoroutines) {
+ if (auto Body = Function->getBody(); Body && isa<CoroutineBodyStmt>(Body))
+ return;
+ }
TraversalKindScope RAII(*Result.Context, TK_AsIs);
@@ -124,6 +128,7 @@ void UnnecessaryValueParamCheck::storeOptions(
Options.store(Opts, "IncludeStyle", Inserter.getStyle());
Options.store(Opts, "AllowedTypes",
utils::options::serializeStringList(AllowedTypes));
+ Options.store(Opts, "IsAllowedInCoroutines", IsAllowedInCoroutines);
}
void UnnecessaryValueParamCheck::onEndOfTranslationUnit() {
diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.h b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.h
index 8bfd814d16357..8101bf6e8306d 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.h
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.h
@@ -46,6 +46,7 @@ class UnnecessaryValueParamCheck : public ClangTidyCheck {
ExprMutationAnalyzer::Memoized MutationAnalyzerCache;
utils::IncludeInserter Inserter;
const std::vector<StringRef> AllowedTypes;
+ bool IsAllowedInCoroutines;
};
} // namespace clang::tidy::performance
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index f1a8819e21788..761a466b43e2a 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -265,8 +265,8 @@ Changes in existing checks
<clang-tidy/checks/performance/unnecessary-value-param>` check performance by
tolerating fix-it breaking compilation when functions is used as pointers
to avoid matching usage of functions within the current compilation unit.
- Also suppressed this check for coroutine functions because it may not be safe
- and suggested fixes may result in hard-to-find bugs and crashes.
+ Added an option `IsAllowedInCoroutines` with the default value ``false`` to
+ suppress this check for coroutines where passing by reference may be unsafe.
- Improved :doc:`readability-convert-member-functions-to-static
<clang-tidy/checks/readability/convert-member-functions-to-static>` check by
diff --git a/clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-value-param.rst b/clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-value-param.rst
index fce3bec00504e..459042784345d 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-value-param.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-value-param.rst
@@ -58,10 +58,6 @@ Because the fix-it needs to change the signature of the function, it may break
builds if the function is used in multiple translation units or some codes
depends on function signatures.
-Note: This check does not suggest passing parameters by reference in coroutines
-because, after a coroutine suspend point, references could be dangling and no
-longer valid, so suggested changes may result in hard-to-find bugs and crashes.
-
Options
-------
@@ -78,3 +74,9 @@ Options
default is empty. If a name in the list contains the sequence `::`, it is
matched against the qualified type name (i.e. ``namespace::Type``),
otherwise it is matched against only the type name (i.e. ``Type``).
+
+.. option:: IsAllowedInCoroutines
+
+ A boolean specifying whether the check should suggest passing parameters by
+ reference in coroutines. The default is `false`. Passing parameters by reference
+ in coroutines may not be safe.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-coroutine.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-coroutine.cpp
index b6e9bd0a60d44..52e86f8d32af4 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-coroutine.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-coroutine.cpp
@@ -1,4 +1,8 @@
// RUN: %check_clang_tidy -std=c++20-or-later %s performance-unnecessary-value-param %t -- -fix-errors
+// RUN: %check_clang_tidy -std=c++20-or-later %s performance-unnecessary-value-param %t -- \
+// RUN: -config='{CheckOptions: {performance-unnecessary-value-param.IsAllowedInCoroutines: false}}' -fix-errors
+// RUN: not %check_clang_tidy -std=c++20-or-later %s performance-unnecessary-value-param %t -- \
+// RUN: -config='{CheckOptions: {performance-unnecessary-value-param.IsAllowedInCoroutines: true}}' -fix-errors
namespace std {
More information about the cfe-commits
mailing list