[libcxx] [coroutine] Conform coroutine implementation

[ChuanqiXu]

Summary

This revision is based on @Quuxplusone's suggestion in D108697.
This patch aims to conform the coroutine implementation to C++20 standard.

The reference for this patch is: https://eel.is/c++draft/support.coroutine

The main change of this patch includes:

• Require _LIBCPP_STD_VER > 11. This keeps consistency with tests and gcc.
• Replace _LIBCPP_CONSTEXPR and _NOEXCEPT with constexpr and noexcept.
• Add missing const and noexcept qualifier for functions.
• Break the inheritance relationship of coroutine_handle<Promise>, coroutine_handle<void> and noop_coroutine_handle based on the specification.
• Remove redundant overloads for from_address.
• Use operator<=> to replace <, >, =>, <=.
• Use stable names throughout like` [coroutine.handle.compare] to replace something like 18.11.2.7 comparison operators`.
• Use _LIBCPP_HIDE_FROM_ABI to replace _LIBCPP_INLINE_VISIBILITY.

Test Plan: check-all

[ChuanqiXu]

Run generate_private_header_tests.py and adding the inclusion check for <experimental/coroutine>.

[ChuanqiXu]

Fix test ''libcxx/utils/ci/run-buildbot check-generated-output". I also checked generic-cxx2b and generic-cxx20 locally. Hope it could pass CI this time.
Noticed that "libcxx/utils/ci/run-buildbot check-format" would still fail. But I remembered that we shouldn't apply clang-check on libcxx. @Quuxplusone so could we ignore the failed test from libcxx/utils/ci/run-buildbot check-format?

[• Quuxplusone]

In D109433#3086525, @ChuanqiXu wrote:

Fix test ''libcxx/utils/ci/run-buildbot check-generated-output". I also checked generic-cxx2b and generic-cxx20 locally. Hope it could pass CI this time.
Noticed that "libcxx/utils/ci/run-buildbot check-format" would still fail. But I remembered that we shouldn't apply clang-check on libcxx. @Quuxplusone so could we ignore the failed test from libcxx/utils/ci/run-buildbot check-format?

Right. At the moment CI says "libcxx CI (Format, C++2b, ...) failed". You must fix the "C++2b, ..." parts, but you can leave the "(Format)" part failing. It's sort of like warnings and errors: As long as the CI is red (because of "C++2b, ..." failures), it will also mention that "Format" failed. But, if the only thing that is failing is "Format", then the CI will be green. "Format" is just a warning, not an error.

[ChuanqiXu]

In D109433#3090494, @Quuxplusone wrote:

In D109433#3086525, @ChuanqiXu wrote:

Fix test ''libcxx/utils/ci/run-buildbot check-generated-output". I also checked generic-cxx2b and generic-cxx20 locally. Hope it could pass CI this time.
Noticed that "libcxx/utils/ci/run-buildbot check-format" would still fail. But I remembered that we shouldn't apply clang-check on libcxx. @Quuxplusone so could we ignore the failed test from libcxx/utils/ci/run-buildbot check-format?

Right. At the moment CI says "libcxx CI (Format, C++2b, ...) failed". You must fix the "C++2b, ..." parts, but you can leave the "(Format)" part failing. It's sort of like warnings and errors: As long as the CI is red (because of "C++2b, ..." failures), it will also mention that "Format" failed. But, if the only thing that is failing is "Format", then the CI will be green. "Format" is just a warning, not an error.

From what I see in the CI system, the reason why C++2b and C++2a failed is that the compiler wouldn't search in std:: namespace. In another word, the CI system would become green after D108696 checked in.

[• Quuxplusone]

In D109433#3092213, @ChuanqiXu wrote:

From what I see in the CI system, the reason why C++2b and C++2a failed is that the compiler wouldn't search in std:: namespace. In another word, the CI system would become green after D108696 checked in.

D108696 is already correctly listed as a "Parent Revision" of D109433, so D109433's CI is already running on the-combination-of-both-of-them. And it appears that it's still failing. I believe part of the reason is identified in my comments above. I recommend fixing that, also fixing anything that needs fixing in D108696, then doing "Update Diff" on D108696, waiting an hour, doing "Update Diff" on D109433, and then waiting for the CI to complete on both. If that continues to fail CI, then that continues to be a problem (and I would by default assume it's a problem with the patches, not a problem with the CI setup).

libcxx/include/__config

1275–1276

I believe this diff is the reason your tests are failing.
Based on a quick skim, I think the deal is:

• __cpp_coroutines is set when the compiler supports <experimental/coroutine>
• __cpp_impl_coroutine is set when the compiler supports <coroutine>

So we might want something here like

#if !(defined(__cpp_coroutines) && __cpp_coroutines >= 201703L)
#define _LIBCPP_HAS_NO_EXPERIMENTAL_COROUTINES
#endif

#if !(defined(__cpp_impl_coroutine) && __cpp_impl_coroutine >= 201902L)
#define _LIBCPP_HAS_NO_CXX20_COROUTINES
#endif

and then check the appropriate macro in each coroutine header.

This will also need to be reflected in generate_header_tests.py.

libcxx/include/__coroutine/coroutine_handle.h

11

Please rename the include guard to _LIBCPP___COROUTINE_COROUTINE_HANDLE_H.

libcxx/include/__coroutine/coroutine_noop.h

10 ↗

(On Diff #382207)

Please rename the file to __coroutine/noop_coroutine_handle.h, and the include guard to _LIBCPP___COROUTINE_NOOP_COROUTINE_HANDLE_H.

libcxx/include/__coroutine/coroutine_traits.h

11

Please rename the include guard to _LIBCPP___COROUTINE_COROUTINE_TRAITS_H.

[ChuanqiXu]

• Rename sub header name and macro as suggested.
• Move the change part in clang into previous revision D108696.

I suspect the reason why it is failing is that the original version contains the change in clang too! So the CI didn't compile it. Let's see if it could work.

[ChuanqiXu]

In D109433#3092534, @Quuxplusone wrote:

In D109433#3092213, @ChuanqiXu wrote:

From what I see in the CI system, the reason why C++2b and C++2a failed is that the compiler wouldn't search in std:: namespace. In another word, the CI system would become green after D108696 checked in.

D108696 is already correctly listed as a "Parent Revision" of D109433, so D109433's CI is already running on the-combination-of-both-of-them. And it appears that it's still failing. I believe part of the reason is identified in my comments above. I recommend fixing that, also fixing anything that needs fixing in D108696, then doing "Update Diff" on D108696, waiting an hour, doing "Update Diff" on D109433, and then waiting for the CI to complete on both. If that continues to fail CI, then that continues to be a problem (and I would by default assume it's a problem with the patches, not a problem with the CI setup).

I guess the reason is that this patch included the change in clang part and it doesn't tag clang. So the change isn't compiled in CI. Now I move the change in clang part into the parent revision. Let's see if it could works : )

libcxx/include/__config

1275–1276

_LIBCPP_HAS_NO_EXPERIMENTAL_COROUTINES is defined in experimental/__config now. I think the place is better.
And I remain the name _LIBCPP_HAS_NO_COROUTINES instead of _LIBCPP_HAS_NO_CXX20_COROUTINES since it looks a little bit redundancy.

[ChuanqiXu]

Update generated test

[• Quuxplusone]

libcxx/include/__config

1275–1276

FWIW, I'm okay with "keeping" the name _LIBCPP_HAS_NO_COROUTINES; but I do still think it's better to change all uses of _LIBCPP_HAS_NO_COROUTINES to _LIBCPP_HAS_NO_{CXX20,EXPERIMENTAL}_COROUTINES. Both for programming reasons (so that the future reader can unambiguously see that a certain codepath is related to C++20 or experimental or both, based on the name(s) of the macros involved), and for engineering reasons (so that the patch-writer is forced to touch every instance of _LIBCPP_HAS_NO_COROUTINES in the entire codebase, and any _LIBCPP_HAS_NO_COROUTINES that remains in the codebase can be instantly identified as an oversight, not just something that coincidentally didn't need to be changed).

Anyway, I assume you do intend that one of these macros relates to experimental coroutines and the other one relates specifically to the non-experimental C++20 coroutines. Right?:

#if defined(_LIBCPP_HAS_NO_{CXX20_,}COROUTINES) && defined(_LIBCPP_HAS_NO_EXPERIMENTAL_COROUTINES)
// we have no coroutine support at all
#else
// we have _some_ kind of coroutines (maybe C++20, maybe experimental)
#endif

[ChuanqiXu]

@Quuxplusone for the still failing CI system, I believe it didn't combine D108696. Here is my reason:

• First, goto the link that show the CI failed: https://buildkite.com/llvm-project/libcxx-ci/builds/6275
• Second, search for the commit id 8fc667f and jump into the link: https://github.com/llvm-premerge-tests/llvm-project/commit/8fc667f4430d8cd05a9c8ed85c01f21e5cad7056.
• Then in the history we could find: https://github.com/llvm-premerge-tests/llvm-project/commits/8fc667f4430d8cd05a9c8ed85c01f21e5cad7056

So it didn't contain the codes in D108696. So if we care the CI result, we could wait after D108696 merged in.

libcxx/include/__config

1275–1276

Yeah, I am not a defender on the names. Since I am not a native speaker, so I trust your sensitivity on names should be right. I would rename it later.

[ChuanqiXu]

Rename _LIBCPP_HAS_NO_COROUTINES to _LIBCPP_HAS_NO_CXX20_COROUTINES.

[ChuanqiXu]

Oh, it is weird that the CI keeps failing after landing D108696. I have tested many many times locally. I am wondering if the compiler used in the CI system didn't get updated in time. Let's see if it could be green tomorrow.

[ChuanqiXu]

I had an experiments in https://reviews.llvm.org/D113170. The diff there is the same with here except that diff would check if the compiler introduces __cpp_impl_coroutine which should be introduced in previous commit: https://github.com/llvm/llvm-project/commit/ec117158a390a0ebf64377caa5abd0c976df8f7a. And the results there shows that the compiler in the CI system didn't contain macro __cpp_impl_coroutine so that's the reason for failing.
I guess it would be updated in days. Let's wait some days.

[• Quuxplusone]

libcxx/test/std/language.support/support.coroutines/coroutine.handle/void_handle.pass.cpp

10

libc++ needs to work on Clang 13.x as well as trunk/14.x. Clang 13.x doesn't define __cpp_impl_coroutine. Therefore, to fix the buildkite, I think you need to introduce a new feature flag so that you can say UNSUPPORTED: libcpp-no-coroutines on all of these new tests. Grep for libcpp-no-concepts and see if the process is self-explanatory... or ask @ldionne what's the right way to do it. :)

[ChuanqiXu]

Disable newly added tests for 13.x by adding libcpp-no-coroutines feature. I have tested 13.x locally. Hope CI could get green this time : (

[ChuanqiXu]

re-check check-generated-output

[ChuanqiXu]

• Include <version> in coroutine
• Export compare for coroutine in module map.

[• Quuxplusone]

I'm not sure I'm reading the test results right, but if I am, then it looks like we're still skipping all your new tests — the test runner thinks that Clang 14.x doesn't support __cpp_impl_coroutine.
https://buildkite.com/llvm-project/libcxx-ci/builds/6457
But I have verified locally that Clang trunk (14.x) clang++ -std=c++20 -dM -E x.cpp does output #define __cpp_impl_coroutine 201902L, so I think this is just due to buildkite using an old build... I'm not sure. We still shouldn't land this until we can prove that buildkite will run (and pass) these new tests.

libcxx/utils/generate_header_tests.py

58–60

I think we can now change these lines to

"experimental/coroutine": ["ifndef _LIBCPP_HAS_NO_EXPERIMENTAL_COROUTINES"],
"coroutine": ["ifndef _LIBCPP_HAS_NO_CXX20_COROUTINES"],

for better consistency with all the other features in this list.

[Mordante]

The build system uses https://apt.llvm.org/ for its packages. This isn't updated nightly so it might be the build used an old version. The current timestamp of the build is 20211106103030. @ChuanqiXu can you rebase this patch to trigger the CI again?

[ChuanqiXu]

Address comments.

[ChuanqiXu]

In D109433#3114239, @Mordante wrote:

The build system uses https://apt.llvm.org/ for its packages. This isn't updated nightly so it might be the build used an old version. The current timestamp of the build is 20211106103030. @ChuanqiXu can you rebase this patch to trigger the CI again?

@Mordante Due to my system is not Debian/Ubuntu, I couldn't test the package in the link locally. And I sent another diff in: https://reviews.llvm.org/D113170. It is the same with this one except that it would check the macro 'cpp_impl_coroutine'. And the result shows that the compiler in the CI still didn't emit macro 'cpp_impl_coroutine'. Could you help to check the compiler in the CI system or tell me how to test the timestamp of the compiler in the CI system? Thanks.

[Mordante]

In D109433#3114657, @ChuanqiXu wrote:

In D109433#3114239, @Mordante wrote:

The build system uses https://apt.llvm.org/ for its packages. This isn't updated nightly so it might be the build used an old version. The current timestamp of the build is 20211106103030. @ChuanqiXu can you rebase this patch to trigger the CI again?

@Mordante Due to my system is not Debian/Ubuntu, I couldn't test the package in the link locally. And I sent another diff in: https://reviews.llvm.org/D113170. It is the same with this one except that it would check the macro 'cpp_impl_coroutine'. And the result shows that the compiler in the CI still didn't emit macro 'cpp_impl_coroutine'. Could you help to check the compiler in the CI system or tell me how to test the timestamp of the compiler in the CI system? Thanks.

I run Debian so I tested locally which version was available in apt.llvm.org. I just did a quick test with the Docker image we run on our CI. There the version installed is of the 27th of October. When I manually update in Docker I get a binary of the 5th of November. (Not sure why Ubuntu has a one day older build than Debian). So it seems our CI uses a version of Clang before D108696 landed. I have some suspicions, but @ldionne knows more about our CI.

[ldionne]

In D109433#3082788, @lewissbaker wrote:

General comment regarding stdlib/compiler compatibility.

I notice that you are keeping the std::experimental::coroutine_handle types but leaving them as separate types rather than making them type-aliases of the std::coroutine_handle types.

As it has been explained by @ChuanqiXu , the plan is to first add support for C++20 coroutines while keeping around experimental coroutines for two releases. After two releases, we remove the experimental ones and any user that hasn't migrated yet will be broken. This is the policy we have for experimental features in libc++, which tries to balance keeping the library sane without being too eager to break users.

Is the intention to just require everyone to move to a clang compiler that supports std::coroutine_handle and abandon support for std::experimental types?

Yes, but after two releases. During this two-release time frame, both experimental and non-experimental coroutines should keep working. We probably don't want to allow mixing the two for sanity, though. My understanding is that this is what D108696 achieves.

Once the clang compiler has been updated to reference the new types, is there any point in continuing to keep the std::experimental types around?

Just to avoid breaking users too eagerly. We want to give them a bit of time to migrate, but then we should nuke std::experimental as soon as we can to avoid user confusion and reduce our maintenance costs.

In D109433#3116176, @Mordante wrote:

I run Debian so I tested locally which version was available in apt.llvm.org. I just did a quick test with the Docker image we run on our CI. There the version installed is of the 27th of October. When I manually update in Docker I get a binary of the 5th of November. (Not sure why Ubuntu has a one day older build than Debian). So it seems our CI uses a version of Clang before D108696 landed. I have some suspicions, but @ldionne knows more about our CI.

Long story short, our CI doesn't get an updated version of the compiler on apt.llvm.org automatically. They cache the Docker image and don't rebuild it unless the Dockerfile changes AFAICT. This isn't great -- we should always use the Dockerfile in the repo to run the build, but there are hurdles to setting that up. I'll manually change the Dockerfile to trigger an image rebuild -- we should pick up the latest apt.llvm.org compiler, which I think should include D108696.

By the way, thanks a lot to everyone for chiming in and reviewing this. I've been MIA because I'm swamped by other work, but please keep pinging me on Discord for specific questions.

[ChuanqiXu]

Rebase with trunk.

[ChuanqiXu]

In D109433#3116176, @Mordante wrote:

In D109433#3114657, @ChuanqiXu wrote:

In D109433#3114239, @Mordante wrote:

The build system uses https://apt.llvm.org/ for its packages. This isn't updated nightly so it might be the build used an old version. The current timestamp of the build is 20211106103030. @ChuanqiXu can you rebase this patch to trigger the CI again?

@Mordante Due to my system is not Debian/Ubuntu, I couldn't test the package in the link locally. And I sent another diff in: https://reviews.llvm.org/D113170. It is the same with this one except that it would check the macro 'cpp_impl_coroutine'. And the result shows that the compiler in the CI still didn't emit macro 'cpp_impl_coroutine'. Could you help to check the compiler in the CI system or tell me how to test the timestamp of the compiler in the CI system? Thanks.

I run Debian so I tested locally which version was available in apt.llvm.org. I just did a quick test with the Docker image we run on our CI. There the version installed is of the 27th of October. When I manually update in Docker I get a binary of the 5th of November. (Not sure why Ubuntu has a one day older build than Debian). So it seems our CI uses a version of Clang before D108696 landed. I have some suspicions, but @ldionne knows more about our CI.

Got it. Thanks.

In D109433#3116311, @ldionne wrote:

In D109433#3116176, @Mordante wrote:

I run Debian so I tested locally which version was available in apt.llvm.org. I just did a quick test with the Docker image we run on our CI. There the version installed is of the 27th of October. When I manually update in Docker I get a binary of the 5th of November. (Not sure why Ubuntu has a one day older build than Debian). So it seems our CI uses a version of Clang before D108696 landed. I have some suspicions, but @ldionne knows more about our CI.

Long story short, our CI doesn't get an updated version of the compiler on apt.llvm.org automatically. They cache the Docker image and don't rebuild it unless the Dockerfile changes AFAICT. This isn't great -- we should always use the Dockerfile in the repo to run the build, but there are hurdles to setting that up. I'll manually change the Dockerfile to trigger an image rebuild -- we should pick up the latest apt.llvm.org compiler, which I think should include D108696.

It looks like that the CI didn't update the compiler yet. May I ask when would it be OK to use a later version?

[ChuanqiXu]

Now it shows that the compiler should be updated. And there are two tests failing:

• libcxx/utils/ci/run-buildbot generic-modules
• libcxx/utils/ci/run-buildbot generic-32bit
  • libcxx/gdb/gdb_pretty_printer_test.sh.cpp

I don't find the reason why generic-modules fails now. And it is weird that libcxx/gdb/gdb_pretty_printer_test.sh.cpp is failing since it is irrelevant with coroutine. I would test them locally.

[ChuanqiXu]

Fix the test failure for generic modules.
I finally found the reason why it would fail for generic modules. Since the tests didn't contain headers which is included in the __coroutine/* headers even if they are already included! It is surprising to people who didn't use clang module before. Then the only failure is for libcxx/gdb/gdb_pretty_printer_test.sh.cpp. It looks not relevant with coroutines. But I would try to look into it in case it would fail still.

[ChuanqiXu]

When I look into the error message, I find the following error message:

FAIL: /home/libcxx-builder/.buildkite-agent/builds/344e6285b245-1/llvm-project/libcxx-ci/libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp:247
GDB printed:
   'std::bitset<258u>'
Value should match:
   'std::bitset<258(ul)?>'
FAIL: /home/libcxx-builder/.buildkite-agent/builds/344e6285b245-1/llvm-project/libcxx-ci/libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp:250
GDB printed:
   'std::bitset<0u>'
Value should match:
   'std::bitset<0(ul)?>'
FAIL: /home/libcxx-builder/.buildkite-agent/builds/344e6285b245-1/llvm-project/libcxx-ci/libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp:253
GDB printed:
   'std::bitset<15u> = {[2] = 1, [3] = 1, [4] = 1, [5] = 1, [6] = 1, [7] = 1, [8] = 1, [9] = 1}'
Value should match:
   'std::bitset<15(ul)?> = {\\[2\\] = 1, \\[3\\] = 1, \\[4\\] = 1, \\[5\\] = 1, \\[6\\] = 1, \\[7\\] = 1, \\[8\\] = 1, \\[9\\] = 1}'
FAIL: /home/libcxx-builder/.buildkite-agent/builds/344e6285b245-1/llvm-project/libcxx-ci/libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp:261
GDB printed:
   'std::bitset<258u> = {[0] = 1, [129] = 1, [132] = 1}'
Value should match:
   'std::bitset<258(ul)?> = {\\[0\\] = 1, \\[129\\] = 1, \\[132\\] = 1}'

It looks like this is introduced in https://reviews.llvm.org/D113112. @ldionne hi, could you help to look into this? @Quuxplusone Now I think this test failure should be irrelevant with this diff and all other tests are passed. Do you think we should land this after the bug fixed or we could land this now?

[• Quuxplusone]

In D109433#3123703, @ChuanqiXu wrote:

[gdb_pretty_printer_test.sh.cpp is failing]

It looks like this is introduced in https://reviews.llvm.org/D113112. @ldionne hi, could you help to look into this? @Quuxplusone Now I think this test failure should be irrelevant with this diff and all other tests are passed. Do you think we should land this after the bug fixed or we could land this now?

• I'm 100% sure that the GDB-pretty-printer test failure is not your fault.
• I think @ldionne should look into it (in fact, my impression is that he already is). Maybe the solution will just be to UNSUPPORTED it temporarily, but let's actually get it green.
• IMO this coroutines stuff is big and important enough that you should wait until that pretty-printer test has been fixed, and get a really 100% green CI run, before you land it. It's been a few months; waiting another few days won't hurt anything. And green CI runs for big features are nice to have. So I don't think you should land it until CI is green.

[ChuanqiXu]

In D109433#3124512, @Quuxplusone wrote:

In D109433#3123703, @ChuanqiXu wrote:

[gdb_pretty_printer_test.sh.cpp is failing]

It looks like this is introduced in https://reviews.llvm.org/D113112. @ldionne hi, could you help to look into this? @Quuxplusone Now I think this test failure should be irrelevant with this diff and all other tests are passed. Do you think we should land this after the bug fixed or we could land this now?

• I'm 100% sure that the GDB-pretty-printer test failure is not your fault.
• I think @ldionne should look into it (in fact, my impression is that he already is). Maybe the solution will just be to UNSUPPORTED it temporarily, but let's actually get it green.
• IMO this coroutines stuff is big and important enough that you should wait until that pretty-printer test has been fixed, and get a really 100% green CI run, before you land it. It's been a few months; waiting another few days won't hurt anything. And green CI runs for big features are nice to have. So I don't think you should land it until CI is green.

Got it. Thanks for the explanation!

[ChuanqiXu]

@Quuxplusone the CI is finally green now!

[ldionne]

LGTM with some suggestions. I am assuming that you copy-pasted the code and tests from experimental/coroutine and performed minor modifications as explained in the description.

Thanks!

libcxx/include/CMakeLists.txt

134–137

Please sort!

libcxx/include/__coroutine/coroutine_handle.h

98

I *think* this block can be removed entirely, but let's not block this on D113938.

libcxx/include/__coroutine/noop_coroutine_handle.h

24

Can we get rid of this (and similarly in the corresponding test file)?

[• Quuxplusone]

libcxx/include/CMakeLists.txt

134–137

And before sorting, please rename coroutine_trivial_awaitables to just trivial_awaitables (and update the modulemap, and the header guard, and the generated tests). I just noticed this. :P

[ChuanqiXu]

Rename coroutine_trivial_awaitables to trivial_awaitables and sort CMakeLists, modulemap .
(Didn't change the header guard since I found it is consistency now)

[ChuanqiXu]

libcxx/include/__coroutine/noop_coroutine_handle.h

24

Only Clang and MSVC implements __builtin_coro_noop. GCC uses other tricks. So if we move get rid of this guard, it couldn't compiled by GCC. According to our previous discussion, we decide to support this in later patches.

[ChuanqiXu]

Rename correctly.

[ChuanqiXu]

@Quuxplusone @ldionne many thanks for your helps! I couldn't make this without your helps.