This is an archive of the discontinued LLVM Phabricator instance.

[C++20] [Coroutines] Allow promise_type to not define return_void or return_value
ClosedPublic

Authored by ChuanqiXu on Dec 22 2021, 10:23 PM.

Details

Summary

According to [[dcl.fct.def.coroutine]p6](http://eel.is/c++draft/dcl.fct.def.coroutine#6), the promise_type is allowed to not define return_void or return_value:

If searches for the names return_­void and return_­value in the scope of the promise type each find any declarations, the program is ill-formed.
[Note 1: If return_­void is found, flowing off the end of a coroutine is equivalent to a co_­return with no operand. Otherwise, flowing off the end of a coroutine results in
undefined behavior ([stmt.return.coroutine]). — end note]

Diff Detail

Unit TestsFailed

Event Timeline

ChuanqiXu requested review of this revision.Dec 22 2021, 10:23 PM
ChuanqiXu created this revision.
urnathan added inline comments.Dec 23 2021, 4:04 AM
clang/lib/Sema/SemaCoroutine.cpp
1434–1435

there's some repetition in the comments. Perhaps a bloc comment before the if-else sequence?

1435–1436

Your testcase doesnt show such a warning. This seems unhelpful.

ChuanqiXu marked 2 inline comments as done.Dec 23 2021, 4:09 AM
ChuanqiXu added inline comments.
clang/lib/Sema/SemaCoroutine.cpp
1434–1435

Yeah, would do.

1435–1436

Oh, I need emphasize in the comment that the warning is not correct. I want to say that if we don't set FallThrough, the compiler would emit incorrect warning. And in this revision, we set FallThrough so that we could avoid the warning. The edited test case could address this.

ChuanqiXu updated this revision to Diff 396009.Dec 23 2021, 4:38 AM
ChuanqiXu marked 2 inline comments as done.

Address comments.

urnathan accepted this revision.Dec 23 2021, 7:30 AM

looks good! Yay, Halting Problem :)

This revision is now accepted and ready to land.Dec 23 2021, 7:30 AM
clang/test/SemaCXX/coroutines.cpp
987

It's not UB; it's potential UB.
My understanding is that such a coroutine could still be used:

  • if it exits by throwing an exception instead of co_return'ing
  • if it suspends and then its caller destroys it instead of resumeing it

I believe it would be useful to have some actual tests for these scenarios, that would actually run and check that the runtime behavior is correct, or at least check the IR that was generated. Is that possible?

ChuanqiXu updated this revision to Diff 396126.Dec 23 2021, 5:49 PM

Address comments

ChuanqiXu added inline comments.Dec 23 2021, 6:03 PM
clang/test/SemaCXX/coroutines.cpp
987

Yes, such a coroutine could still be used. AFAIK, we couldn't make tests which would run actually. All the test cases I know is about pattern match. So we couldn't make tests in clang to run actually for this. I have tested it locally. And I think it is unnecessary to check the IR was generated since this patch didn't add new statements except a null statement as marker.

BTW, it shows another big issue for coroutines. We lack test-suite and benchmarks now. Now I tested the correctness of the compiler by test it in our internal projects. But we need an open source test suites absolutely. This is true for benchmarks. I thought to make the two things... but I don't find time for it now...

This revision was landed with ongoing or failed builds.Dec 23 2021, 9:39 PM
This revision was automatically updated to reflect the committed changes.