[Clang] Preserve coroutine parameter referenced state

[yuxuanchen1997]

This PR is proposing a fix for #65971.

Previously, given a coroutine like this

task foo(int a) { co_return; } 

Parameter a is never used. However, because C++ coroutines move constructs the variable to a heap allocated coroutine activation frame, we considered all parameters referenced. When diagnosing unused parameters, we cannot distinguish if the variable reference was due to coroutine parameter moves.

Compiler Explorer shows that GCC warns against this case correctly, but clang does not: https://godbolt.org/z/Wo7dfqeaf

This patch addresses this issue by preserving the original ParmVarDecl's Referenced state.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-coroutines

Author: Yuxuan Chen (yuxuanchen1997)

Changes

This PR is proposing a fix for #65971.

Previously, given a coroutine like this

task foo(int a) { co_return; } 

Parameter a is never used. However, because C++ coroutines move constructs the variable to a heap allocated coroutine activation frame, we considered all parameters referenced. When diagnosing unused parameters, we cannot distinguish if the variable reference was due to coroutine parameter moves.

Compiler Explorer shows that GCC warns against this case correctly, but clang does not: https://godbolt.org/z/Wo7dfqeaf

This patch addresses this issue by preserving the original ParmVarDecl's Referenced state.

---

Full diff: https://github.com/llvm/llvm-project/pull/70973.diff

2 Files Affected:

• (modified) clang/lib/Sema/SemaCoroutine.cpp (+6)
• (added) clang/test/SemaCXX/warn-unused-parameters-coroutine.cpp (+34)

diff --git a/clang/lib/Sema/SemaCoroutine.cpp b/clang/lib/Sema/SemaCoroutine.cpp index 38ac406b14adadf..bee80db8d166a68 100644 --- a/clang/lib/Sema/SemaCoroutine.cpp +++ b/clang/lib/Sema/SemaCoroutine.cpp @@ -1965,9 +1965,15 @@ bool Sema::buildCoroutineParameterMoves(SourceLocation Loc) { if (PD->getType()->isDependentType()) continue; + // Preserve the referenced state for unused parameter diagnostics. + bool DeclReferenced = PD->isReferenced(); + ExprResult PDRefExpr = BuildDeclRefExpr(PD, PD->getType().getNonReferenceType(), ExprValueKind::VK_LValue, Loc); // FIXME: scope? + + PD->setReferenced(DeclReferenced); + if (PDRefExpr.isInvalid()) return false; diff --git a/clang/test/SemaCXX/warn-unused-parameters-coroutine.cpp b/clang/test/SemaCXX/warn-unused-parameters-coroutine.cpp new file mode 100644 index 000000000000000..b4c01550f9f7883 --- /dev/null +++ b/clang/test/SemaCXX/warn-unused-parameters-coroutine.cpp @@ -0,0 +1,34 @@ +// RUN: %clang_cc1 -fsyntax-only -Wunused-parameter -verify -std=c++20 %s + +#include "Inputs/std-coroutine.h" + +struct awaitable { + bool await_ready() noexcept; + void await_resume() noexcept; + void await_suspend(std::coroutine_handle<>) noexcept; +}; + +struct task : awaitable { + struct promise_type { + task get_return_object() noexcept; + awaitable initial_suspend() noexcept; + awaitable final_suspend() noexcept; + void unhandled_exception() noexcept; + void return_void() noexcept; + }; +}; + +task foo(int a) { // expected-warning{{unused parameter 'a'}} + co_return; +} + +task bar(int a, int b) { // expected-warning{{unused parameter 'b'}} + a = a + 1; + co_return; +} + +void create_closure() { + auto closure = [](int c) -> task { // expected-warning{{unused parameter 'c'}} + co_return; + }; +} 

[yuxuanchen1997]

@ChuanqiXu9 it was my mistake. This patch does work.

[bcardosolopes]

Thanks for improving this! I haven't looked at your previous review of this PR, but I like the simplicity of this one.

[bcardosolopes]

LGTM. @ChuanqiXu9 should give the final blessing though.

[ChuanqiXu9]

It looks much better now. Let's try to optimize it a bit more.

[ChuanqiXu9]

Readers can understand what is going on here. But they may be confused about the motivation. Let's add a comment to explain the reasons. I mean, something like, if we don't do so, we can't diagnose the unused parameter things.

[ChuanqiXu9]

Since we will construct the parameter moves at the very beginning of the coroutine function, I am wondering if it is really possible that PD->isReferenced() may be true now. And if it is not possible,
we should convert it to assert(!PD->isReferenced());. Please changing this after verifying this with some actual coroutine related workloads.

[yuxuanchen1997]

PD->isReferenced() may be true. The order Sema processes things is based on how Parser is sending AST nodes to it. I can try to explain based on my limited understanding and corroborate with a test case. So please feel free to correct me if I am wrong.

Let's take a look at this very simple coroutine. Suppose task is defined somewhere as a return object.

task foo(int a, int b) { a = a + 1; co_return; } 

Sema is first asked to compile foo like a normal function. Decl a and b are processed normally. When we use a in the a = a + 1 statement, we mark a as referenced.

When the parser encounters co_return, it calls Sema::ActOnCoreturn.

What Sema::ActOnCoreturn (and its siblings handling other co_ family of statements) does is to call Sema::ActOnCoroutineBodyStart. It checks if clang is already treating foo as a coroutine. It will find out that, no. We are not compiling foo like a coroutine yet, let's perform necessary checks and establish this context. buildCoroutineParameterMoves is called as a result. At that time a already has references we need to carry along.

Suppose that our function has subsequent co_* statements, Sema knows that we already established the context and doesn't build them again.

The aforementioned foo coroutine will cause clang ICE if we assert(!PD->isReferenced()) here.

[ChuanqiXu9]

Cool.

[ChuanqiXu9]

LGTM.

[ChuanqiXu9]

Cool.