https://github.com/dmpolukhin updated
https://github.com/llvm/llvm-project/pull/140912
>From e3ce9bb18f165649b00db14b2282649315b28883 Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin <dmitry.poluk...@gmail.com>
Date: Wed, 21 May 2025 07:54:02 -0700
Subject: [PATCH 1/3] [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 470bf837a1a958c75f4661c0c13dc7e48513d12d Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin <dmitry.poluk...@gmail.com>
Date: Wed, 21 May 2025 09:19:56 -0700
Subject: [PATCH 2/3] Add documentation and simplify test
---
clang-tools-extra/docs/ReleaseNotes.rst | 4 +-
.../performance/unnecessary-value-param.rst | 4 ++
.../unnecessary-value-param-coroutine.cpp | 63 ++++++-------------
3 files changed, 25 insertions(+), 46 deletions(-)
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst
b/clang-tools-extra/docs/ReleaseNotes.rst
index 761c1d3a80359..9522b5ac8d6ca 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -183,7 +183,7 @@ Changes in existing checks
explicit casting of built-in types within member list initialization.
- Improved :doc:`modernize-use-ranges
- <clang-tidy/checks/modernize/use-ranges>` check by updating suppress
+ <clang-tidy/checks/modernize/use-ranges>` check by updating suppress
warnings logic for ``nullptr`` in ``std::find``.
- Improved :doc:`modernize-use-starts-ends-with
@@ -203,6 +203,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.
Removed checks
^^^^^^^^^^^^^^
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 e8635e02964aa8a38c11b9741dc76f89c2093159 Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin <dmitry.poluk...@gmail.com>
Date: Wed, 21 May 2025 09:54:34 -0700
Subject: [PATCH 3/3] 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
-------
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
