Page MenuHomePhabricator

[Coroutines] [Frontend] Lookup in std namespace first
Needs ReviewPublic

Authored by ChuanqiXu on Aug 25 2021, 5:33 AM.

Details

Summary

Now in libcxx and clang, all the coroutine components are defined in std::experimental namespace.
And now the coroutine TS is merged into C++20. So in the working draft like N4892, we could find the coroutine components is defined in std namespace instead of std::experimental namespace.
And the coroutine support in clang seems to be relatively stable. So I think it may be suitable to move the coroutine component into the experiment namespace now.

But move the coroutine component into the std namespace may be an break change. So I planned to split this change into two patch. One in clang and other in libcxx.

This patch would make clang lookup coroutine_traits in std namespace first. For the compatibility consideration, clang would lookup in std::experimental namespace if it can't find definitions in std namespace. So the existing codes wouldn't be break after update compiler.

Test Plan: check-clang, check-libcxx

Diff Detail

Event Timeline

ChuanqiXu created this revision.Aug 25 2021, 5:33 AM
ChuanqiXu requested review of this revision.Aug 25 2021, 5:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2021, 5:33 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ChuanqiXu edited the summary of this revision. (Show Details)
  • Emit warning if the compiler finds the user are using std::experimental namespace:

warning message is:

coroutine_traits defined in std::experimental namespace is deprecated. Considering update libcxx and include <coroutine> instead of <experimental/coroutine>.

@rjmccall @lxfind @junparser hi, do you feel comfortable with this?

avogelsgesang added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
11015

Considering update -> Consider updating

ChuanqiXu updated this revision to Diff 370220.Sep 2 2021, 3:40 AM

Address comments.

clang/include/clang/Basic/DiagnosticSemaKinds.td
11015

Done. Thanks for suggestion!

lxfind accepted this revision.Sep 2 2021, 2:34 PM

LGTM. Thanks!

This revision is now accepted and ready to land.Sep 2 2021, 2:34 PM
This revision was landed with ongoing or failed builds.Sep 2 2021, 7:24 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 2 2021, 7:24 PM
uabelho added a subscriber: uabelho.Sep 3 2021, 2:29 AM

This patch makes several libcxx tests fail due to the new warning. See e.g.
http://lab.llvm.org:8011/#/builders/60/builds/4632

gulfem added a subscriber: gulfem.Sep 3 2021, 10:32 AM

We started seeing the following test failures in our Fuchsia builds after this patch:

Failed Tests (10):
  libc++ :: libcxx/experimental/language.support/support.coroutines/dialect_support.pass.cpp
  libc++ :: std/experimental/language.support/support.coroutines/coroutine.handle/coroutine.handle.prom/promise.pass.cpp
  libc++ :: std/experimental/language.support/support.coroutines/end.to.end/await_result.pass.cpp
  libc++ :: std/experimental/language.support/support.coroutines/end.to.end/bool_await_suspend.pass.cpp
  libc++ :: std/experimental/language.support/support.coroutines/end.to.end/expected.pass.cpp
  libc++ :: std/experimental/language.support/support.coroutines/end.to.end/fullexpr-dtor.pass.cpp
  libc++ :: std/experimental/language.support/support.coroutines/end.to.end/generator.pass.cpp
  libc++ :: std/experimental/language.support/support.coroutines/end.to.end/go.pass.cpp
  libc++ :: std/experimental/language.support/support.coroutines/end.to.end/multishot_func.pass.cpp
  libc++ :: std/experimental/language.support/support.coroutines/end.to.end/oneshot_func.pass.cpp

https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket/8837144494402864417/+/u/clang/test/stdout

Do you have any idea what might be going wrong?

ldionne reopened this revision.Sep 3 2021, 1:00 PM
ldionne added a subscriber: ldionne.

Based on the commit description, I don't understand this change at all. Why do we want to tweak the name lookup just for std::coroutine? Yes, we do have an action item to finish coroutines in libc++ already, and I'd love to see a patch that does that, but I don't think that mandates changing Clang.

The rollout plan for coroutines should be:

  1. Make sure we implement coroutines fully
  2. Duplicate it all into namespace std
  3. In two LLVM releases, remove all the coroutines stuff in std::experimental.

I'm going to revert this for now on the basis that it breaks libc++ CI. Let's have a discussion about the above if you think I'm mistaken or if I'm misunderstanding what this patch does.

This revision is now accepted and ready to land.Sep 3 2021, 1:00 PM
ldionne reopened this revision.Sep 3 2021, 1:02 PM

(woops, this closed the rev when I committed the revert)

This revision is now accepted and ready to land.Sep 3 2021, 1:02 PM
ldionne requested changes to this revision.Sep 3 2021, 1:03 PM
This revision now requires changes to proceed.Sep 3 2021, 1:03 PM
ChuanqiXu added a comment.EditedSep 5 2021, 7:07 PM

Based on the commit description, I don't understand this change at all. Why do we want to tweak the name lookup just for std::coroutine? Yes, we do have an action item to finish coroutines in libc++ already, and I'd love to see a patch that does that, but I don't think that mandates changing Clang.

The rollout plan for coroutines should be:

  1. Make sure we implement coroutines fully
  2. Duplicate it all into namespace std
  3. In two LLVM releases, remove all the coroutines stuff in std::experimental.

I'm going to revert this for now on the basis that it breaks libc++ CI. Let's have a discussion about the above if you think I'm mistaken or if I'm misunderstanding what this patch does.

Many thanks for reverting this!

Why do we want to tweak the name lookup just for std::coroutine? Yes, we do have an action item to finish coroutines in libc++ already

This is due to a practice that:

  • People want to use clang but they prefer to use libstdc++ instead of libc++.
  • So that they would implement a coroutine.h by coping from libc++. So that they could depend on libstdc++ still.

This is a reason that I introduce the warning in the compiler side instead of libc++. But if this warning matters, I am Ok to emit the warning in the compiler side.

Then let's talk about the plans to move coroutine components into std namespace:

In D108697, the successor of this patch, we had duplicated all the components into namespace std. (And I add an warning in std::experimental by #warning directive, I am not sure if this warning may break the CI)
Then here is the problem, we must commit this before D108697, otherwise the compiler wouldn't search coroutine components in std namespace.
The solution I got now is that I should remove the warning message in this patch. Then the compiler would still try to lookup in std::experimental namespace without warning if it can't find things in std namespace.
Do you @ldionne @Quuxplusone think this solution workable?

ChuanqiXu updated this revision to Diff 370838.Sep 5 2021, 8:29 PM
ChuanqiXu edited the summary of this revision. (Show Details)

Remove warning if the compiler found std::experimental::coroutine_traits to avoid libcxx CI problems.


I still think this warning is beneficial. We need to note the user who update clang only that they need to update libcxx too.
So I think we should add this warning after D108697 merged in.

ChuanqiXu planned changes to this revision.Sep 7 2021, 6:52 PM

We should proceed after D108697 accepted.

@ldionne @Quuxplusone Now the compiler wouldn't emit warning for <experimental/coroutine> any more. So I think this is good. Do you feel good with this? This is necessary to conform the implementation of coroutine since the compiler would search coroutine components in corresponding namespace (std::experimental before and both std and std::experimental after this patch).

ChuanqiXu requested review of this revision.Sep 21 2021, 7:01 PM

This sounds more reasonable to me, however we need to ship <coroutine> in libc++ before we enable this, or else we're going to start suggesting that users include <coroutine> when we don't have it.

clang/lib/Sema/SemaCoroutine.cpp
1668
1677

Add a warning about what?

clang/test/SemaCXX/coroutines-exp-namespace.cpp
3
ChuanqiXu updated this revision to Diff 374709.Thu, Sep 23, 6:59 PM

Address comments.

This sounds more reasonable to me, however we need to ship <coroutine> in libc++ before we enable this, or else we're going to start suggesting that users include <coroutine> when we don't have it.

We couldn't ship '<coroutine>' before we enable this. Otherwise the compiler couldn't find coroutine components defined in std namespace.
The patch before tries to emit warning for user who including <experimental/coroutine> but it broke the CI system of libcxx. That's the reason that the first commit get reverted.
I think the right order may be:

  • Check in this.
  • Then check in D109433.
  • Add a warning to tell people that they should include <coroutine> instead of <experimental/coroutine>. This patch should be simple enough to check in quickly.

How do you think about this order?

ChuanqiXu updated this revision to Diff 380306.Mon, Oct 18, 1:36 AM

Rebase with trunk.
Hi @Quuxplusone. This patch is necessary for D109433. This patch would search the std namespace first and search std::experimental namespace if the search in std namespace failed. The reason why it got reverted before was that this original patch would emit warning when find components in std::experimental namespace. And now it wouldn't emit any warning. Would you be happy with this? I want to check-in this to avoid extra rebase work.

Hi @ChuanqiXu

sorry for what might be naive questions, but just to make sure I understand the context of this patch correctly:

This patch fixes the look up of the coroutine_traits, so that clang now search both the std and the std::experimental namespace. As such, it must land before D109433 since otherwise the new std::coroutine_traits wouldn't be found by the compiler, correct?

Also, does this patch enable me to use clang-cl to build programs which use the coroutine header from Microsoft's STL? Or are there still other incompatibilities between clang and the Microsoft STL with regards to coroutines?

Hi @ChuanqiXu

sorry for what might be naive questions, but just to make sure I understand the context of this patch correctly:

Hi, you are welcome : )

This patch fixes the look up of the coroutine_traits, so that clang now search both the std and the std::experimental namespace. As such, it must land before D109433 since otherwise the new std::coroutine_traits wouldn't be found by the compiler, correct?

Yes, correct.

Also, does this patch enable me to use clang-cl to build programs which use the coroutine header from Microsoft's STL? Or are there still other incompatibilities between clang and the Microsoft STL with regards to coroutines?

After this patch, it should be able to compile <coroutine> MS STL. Otherwise, there must be a bug in either the clang part or in the MS STL part.