Page MenuHomePhabricator

[libc++] Remove <experimental/coroutine>
Needs ReviewPublic

Authored by ldionne on Aug 25 2021, 5:37 AM.

Details

Reviewers
rjmccall
lxfind
junparser
GorNishanov
Quuxplusone
ChuanqiXu
Group Reviewers
Restricted Project
Summary

We've been shipping <coroutine> since LLVM 14, so LLVM 16 won't ship
the <experimental/coroutine> header per our policy for removing TSes.

Diff Detail

Unit TestsFailed

TimeTest
66,940 mslibcxx CI C++03 > llvm-libc++-shared-cfg-in.libcxx::modules_include.sh.cpp
Script: -- : 'RUN: at line 44'; rm -rf /home/libcxx-builder/.buildkite-agent/builds/915fd8a54c41-1/llvm-project/libcxx-ci/build/generic-cxx03/test/libcxx/Output/modules_include.sh.cpp.dir/t.tmp
86,420 mslibcxx CI C++11 > llvm-libc++-shared-cfg-in.libcxx::modules_include.sh.cpp
Script: -- : 'RUN: at line 44'; rm -rf /home/libcxx-builder/.buildkite-agent/builds/86529a11c733-1/llvm-project/libcxx-ci/build/generic-cxx11/test/libcxx/Output/modules_include.sh.cpp.dir/t.tmp

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ChuanqiXu requested review of this revision.Aug 25 2021, 5:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2021, 5:37 AM

My guess is you might need to keep coro in both std and std::experimental for a while, to allow smooth transition.

My guess is you might need to keep coro in both std and std::experimental for a while, to allow smooth transition.

In D108696, the clang would lookup in std and std::experimental namespace. And this patch would only be checked in after D108696 get checked in. So I think it wouldn't affect users much.

In D108696, the clang would lookup in std and std::experimental namespace. And this patch would only be checked in after D108696 get checked in. So I think it wouldn't affect users much.

Well, but if user code has "#include <experimental/coroutine>", would it still compile?

ChuanqiXu edited the summary of this revision. (Show Details)
  • Remain <experimental/coroutine>
  • Emit a warning told the user to include <coroutine> if user included <experimental/coroutine>

In D108696, the clang would lookup in std and std::experimental namespace. And this patch would only be checked in after D108696 get checked in. So I think it wouldn't affect users much.

Well, but if user code has "#include <experimental/coroutine>", would it still compile?

Yes, it wouldn't compile. So in this revision, I remained the <experimental/coroutine> and add a warning directive to notice the user to include <coroutine>.
Now the user codes should be able to compile all the way after the user update either clang or libcxx. The only exception is that the user uses -Werror. It would the codes couldn't compile after updating. But I think we can't do nothing better.

The warning seems redundant to the one in D108696?

The warning seems redundant to the one in D108696?

Yes. I considered that clang and libcxx are two different projects. So the user might update them separately. e.g., update clang only or update libcxx only. So I choose to put warning in both clang part and libcxx part. It may produce redundant warnings if the user update clang and libcxx together. But I thought that it might not be a problem.

Quuxplusone requested changes to this revision.Sep 7 2021, 3:39 PM
Quuxplusone added a subscriber: Quuxplusone.

First pass. This is still presumably a step in the right direction, but it's currently far from being "C++20 conforming" and IMHO it would probably be nice to get it conforming (or close to conforming) before it lands.

libcxx/include/coroutine
47–48 ↗(On Diff #368816)

Now that we're doing granular headers, please prefer to #include just the smallest helper headers possible (e.g. in this case I think all you need is <__functional/hash.h> for hash<T*>).

58–60 ↗(On Diff #368816)

Remove experimental/

86 ↗(On Diff #368816)

(1) I'd prefer to see __handle_ get a default member initializer on its declaration, so we don't have to member-initialize it here nor on line 89.

(2) If this codepath were limited to _LIBCPP_STD_VER > 11, then we could use constexpr throughout instead of _LIBCPP_CONSTEXPR, and noexcept instead of _NOEXCEPT. Can we do that? Should we?

107 ↗(On Diff #368816)

void resume() const.
Also void operator()() const on line 104.
(yes, this smells wrong, but it's how it was standardized, according to https://eel.is/c++draft/coroutine.handle#general )

108 ↗(On Diff #368816)

Grammar nit: resume() can be called only on suspended coroutines

114 ↗(On Diff #368816)

void destroy() const

127 ↗(On Diff #368816)

This should be marked constexpr.

133–135 ↗(On Diff #368816)

Yes, it must be allowed, at least according to https://eel.is/c++draft/coroutine.handle.export.import ; but you shouldn't have this overload. Lines 126-131 above are already doing the right thing for both null and non-null pointers.

137–141 ↗(On Diff #368816)

This overload looks wrong; remove it. And add a test; something like this:

std::coroutine_handle<> h1 = [...];
int *p = (int*)h1.to_address();
std::coroutine_handle<> h2 = std::coroutine_handle<>::from_address(p);
154 ↗(On Diff #368816)

Use stablenames throughout: e.g., // [coroutine.handle.compare]

155–160 ↗(On Diff #368816)

Question for experts: Are we allowed to make this operator a hidden friend? If we're allowed to, then I strongly think we should make it a hidden friend.
In C++20 operator!= is not needed. (And the C++20 standard doesn't provide it.)

Ditto, C++20 specifies that operator<=> exists (and I'd like it to be a hidden friend), not the four < <= > >= operators.

179–185 ↗(On Diff #368816)

Here we could just do

_LIBCPP_HIDE_FROM_ABI coroutine_handle() noexcept = default;
_LIBCPP_HIDE_FROM_ABI coroutine_handle(nullptr_t) noexcept {}

because default-constructing a _Base already has the right behavior. Right?
But actually, whoa, C++20 definitely does not permit us to make an inheritance relationship between coroutine_handle<> and coroutine_handle<Promise>! These need to be two unrelated class types.
https://eel.is/c++draft/coroutine.handle#general

205–222 ↗(On Diff #368816)

Remove all of this.

238 ↗(On Diff #368816)

This inheritance relationship shouldn't be here.
https://eel.is/c++draft/support.coroutine#coroutine.noop

285–290 ↗(On Diff #368816)

Nits:

template <class _Tp>
struct hash<coroutine_handle<_Tp> > {
  _LIBCPP_HIDE_FROM_ABI
  size_t operator()(const coroutine_handle<_Tp>& __v) const noexcept { return hash<void*>()(__v.address()); }
};
296 ↗(On Diff #368816)

Nit: // not /* (compare to some other header for consistency)

libcxx/test/libcxx/experimental/language.support/support.coroutines/version.pass.cpp
20

This file is autogenerated; you shouldn't change its whitespace.
(In general, you should ignore clang-format's wibbling and just follow existing style.)

libcxx/test/std/experimental/language.support/support.coroutines/coroutine.handle/coroutine.handle.compare/less_comp.pass.cpp
55

Undo this whitespace change.

libcxx/test/std/experimental/language.support/support.coroutines/coroutine.handle/coroutine.handle.export/from_address.fail.cpp
30–33

This test will have to change, since all of these are conforming C++20 and shouldn't be erroring.

libcxx/test/std/experimental/language.support/support.coroutines/coroutine.handle/coroutine.handle.hash/hash.pass.cpp
51–52

You added some extra spaces here; remove them. (Ditto in several other test files.)

libcxx/utils/generate_header_tests.py
9–10

a-z plz

llvm/utils/gn/secondary/libcxx/include/BUILD.gn
362 ↗(On Diff #368816)

a-z plz; but also, what is this file? I've never seen it before and we (libc++) definitely haven't been maintaining it. For example, it's missing headers like "concepts" and "compare" and "ranges". But it has "semaphore", so it's not just C++17 stuff... What is this?

This revision now requires changes to proceed.Sep 7 2021, 3:39 PM

First pass. This is still presumably a step in the right direction, but it's currently far from being "C++20 conforming" and IMHO it would probably be nice to get it conforming (or close to conforming) before it lands.

Many thanks for the detailed review! It looks like we need to refactor the implementation for <coroutine> from your comments. I would try to send another patch if possible.
Then it looks like that there is a phase called 'confirming' for c++ features, right? What's that and what does that mean? Is there any documentation?

Then it looks like that there is a phase called 'confirming' for c++ features, right?

No, I said "conforming," as in "make sure our implementation is conforming to the ISO C++20 Standard."
The documentation/specification of what <coroutine> should look like is primarily the standard:
http://eel.is/c++draft/support.coroutine
and secondarily (but sometimes more user-friendly-ly) cppreference:
https://en.cppreference.com/w/cpp/header/coroutine

As for the steps or phases of the political process here, there's only one step: get @ldionne to mark this PR as "accepted." :)

Then it looks like that there is a phase called 'confirming' for c++ features, right?

No, I said "conforming," as in "make sure our implementation is conforming to the ISO C++20 Standard."
The documentation/specification of what <coroutine> should look like is primarily the standard:
http://eel.is/c++draft/support.coroutine
and secondarily (but sometimes more user-friendly-ly) cppreference:
https://en.cppreference.com/w/cpp/header/coroutine

As for the steps or phases of the political process here, there's only one step: get @ldionne to mark this PR as "accepted." :)

Got it. Although I didn't touch libcxx before, I am willing to refactor this. Thanks : )

ldionne requested changes to this revision.Sep 20 2021, 2:13 PM

As I explained in D109433, we don't need to "move" the header. What we normally do is introduce the <coroutine> header that contains the C++20 conforming implementation without touching <experimental/coroutine>. Then, after 2 releases (LLVM 16), we remove <experimental/coroutine> altogether. I suggest that

  1. D109433 is used to check-in a conforming <coroutine> implementation, and
  2. This patch is repurposed to remove <experimental/coroutine> altogether. Once that's the case, this patch can sleep for two releases and I'll rebase + merge it as soon as LLVM 15 is shipped (to make sure it makes it into LLVM 16).
ChuanqiXu retitled this revision from [Coroutines] [libcxx] Move coroutine components out from std::experimental namespace to [libcxx] [coroutine] Remove <experimental/coroutine>.
ChuanqiXu edited the summary of this revision. (Show Details)

Address @ldionne 's comments.

ChuanqiXu planned changes to this revision.Sep 21 2021, 10:55 PM

According @ldionne's plan, this patch would sleep 2 releases.

ldionne commandeered this revision.Sep 23 2021, 7:19 AM
ldionne edited reviewers, added: ChuanqiXu; removed: ldionne.

Thanks for repurposing the patch. I'm going to commandeer it to make a few tweaks and I'll keep an eye on this to ship it once we've shipped <coroutine> for two releases.

ldionne updated this revision to Diff 374544.Sep 23 2021, 7:45 AM
ldionne retitled this revision from [libcxx] [coroutine] Remove <experimental/coroutine> to [libc++] Remove <experimental/coroutine>.
ldionne edited the summary of this revision. (Show Details)

Things I've done to your patch (just so you know for the future):

  1. Run libcxx-generate-files. More generally, please look at https://libcxx.llvm.org/Contributing.html.
  2. Remove <experimental/coroutine> from the module map.
  3. Don't mention <coroutine> in libcxx/utils/generate_header_tests.py -- that belongs to the patch that will add <coroutine> proper.
Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2021, 7:45 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne edited the summary of this revision. (Show Details)Sep 23 2021, 7:46 AM

Thanks for taking an eye on this!

@ldionne hi, @Quuxplusone mentioned that <coroutine> would only work under C++20, while there are people who use C++17/C++14 and -fcoroutines-ts combination in practice now. It implies that they have to upgrade to c++20 if they update the libcxx with <experimental/coroutine> removed. I am wondering if this is OK? Or do we need to remove <experimental/coroutine> later? (I guess it would be really long time until the primary user update to C++20.)

@ldionne hi, @Quuxplusone mentioned that <coroutine> would only work under C++20, while there are people who use C++17/C++14 and -fcoroutines-ts combination in practice now. It implies that they have to upgrade to c++20 if they update the libcxx with <experimental/coroutine> removed. I am wondering if this is OK? Or do we need to remove <experimental/coroutine> later? (I guess it would be really long time until the primary user update to C++20.)

We have a policy of removing TSes two releases after we ship the "real" feature. If someone has been writing production code against something in std::experimental, they need to be ready to upgrade. So, generally speaking, my stance here would be that these users indeed need to move to C++20 an adopt the real feature. In practice, we can try to accommodate some users by keeping it around for a bit longer, but I wouldn't do that unless someone explicitly requests it and can provide a justification + timeline for moving off. At the end of the day, keeping the experimental version around is not good for anyone, it just delays work that needs to happen nonetheless.

Also note that one thing we should look into is guarding uses of any experimental feature with some compiler flag like -fexperimental, which would be documented to make it clear that production code is not to be written with that flag enabled. That would make the contract between us and our users more clear.

@ldionne hi, @Quuxplusone mentioned that <coroutine> would only work under C++20, while there are people who use C++17/C++14 and -fcoroutines-ts combination in practice now. It implies that they have to upgrade to c++20 if they update the libcxx with <experimental/coroutine> removed. I am wondering if this is OK? Or do we need to remove <experimental/coroutine> later? (I guess it would be really long time until the primary user update to C++20.)

We have a policy of removing TSes two releases after we ship the "real" feature. If someone has been writing production code against something in std::experimental, they need to be ready to upgrade. So, generally speaking, my stance here would be that these users indeed need to move to C++20 an adopt the real feature. In practice, we can try to accommodate some users by keeping it around for a bit longer, but I wouldn't do that unless someone explicitly requests it and can provide a justification + timeline for moving off. At the end of the day, keeping the experimental version around is not good for anyone, it just delays work that needs to happen nonetheless.

Also note that one thing we should look into is guarding uses of any experimental feature with some compiler flag like -fexperimental, which would be documented to make it clear that production code is not to be written with that flag enabled. That would make the contract between us and our users more clear.

Got it. If there is already a policy about this, I have no problem.

ldionne updated this revision to Diff 432308.May 26 2022, 9:18 AM

Rebase onto main

Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2022, 9:18 AM
ldionne updated this revision to Diff 432309.May 26 2022, 9:19 AM

Remove the -fcoroutines-ts Lit feature

ldionne updated this revision to Diff 432351.May 26 2022, 12:04 PM

Remove parent revision and rebase to trigger CI properly.

Maybe we need to land this soon since the release/15.x branch would be created in July 26: https://discourse.llvm.org/t/llvm-15-0-0-release-schedule/63495

Maybe we need to land this soon since the release/15.x branch would be created in July 26: https://discourse.llvm.org/t/llvm-15-0-0-release-schedule/63495

You're one release early. Your coroutine patch was part of LLVM 14, so <experimental/coroutine> stays for 14 and 15. It will be removed during the LLVM 16 timeframe.

Maybe we need to land this soon since the release/15.x branch would be created in July 26: https://discourse.llvm.org/t/llvm-15-0-0-release-schedule/63495

You're one release early. Your coroutine patch was part of LLVM 14, so <experimental/coroutine> stays for 14 and 15. It will be removed during the LLVM 16 timeframe.

Oh, I made a mistake. My bad. Thanks for clarifying.

ldionne updated this revision to Diff 449506.Aug 2 2022, 6:46 PM
ldionne edited the summary of this revision. (Show Details)

Rebase onto main.

ldionne updated this revision to Diff 449507.Aug 2 2022, 6:49 PM

Add release note.