Skip to content

[Clang] Preserve coroutine parameter referenced state #70973

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 2, 2023
Merged

[Clang] Preserve coroutine parameter referenced state #70973

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions clang/lib/Sema/SemaCoroutine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1965,9 +1965,15 @@ bool Sema::buildCoroutineParameterMoves(SourceLocation Loc) {
if (PD->getType()->isDependentType())
continue;

// Preserve the referenced state for unused parameter diagnostics.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

bool DeclReferenced = PD->isReferenced();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@yuxuanchen1997 yuxuanchen1997 Nov 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool.


ExprResult PDRefExpr =
BuildDeclRefExpr(PD, PD->getType().getNonReferenceType(),
ExprValueKind::VK_LValue, Loc); // FIXME: scope?

PD->setReferenced(DeclReferenced);

if (PDRefExpr.isInvalid())
return false;

Expand Down
34 changes: 34 additions & 0 deletions clang/test/SemaCXX/warn-unused-parameters-coroutine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
};
}