[Clang] Add a warning on uses of coroutine keywords

[ilya-biryukov]

Summary

Clang 17 will land with unaddressed coroutine bugs, see

• https://github.com/llvm/llvm-project/issues/58459
• https://github.com/llvm/llvm-project/issues/56301
• https://github.com/llvm/llvm-project/issues/57638
• https://github.com/llvm/llvm-project/issues/63818

It would be useful to have a way to use other C++20, but guarantee
coroutines are not used. This patch achieves it with a warning that
fires on co_await, co_return and co_yield.

The new warning is -Wpre-c++20-compat-coroutines, which is also
enabled with -Wpre-c++20-compat-pedantic.

[ilya-biryukov]

I know this is a little late and we should have thought about it before cutting Clang 17 release.
The plan is to cherry-pick this change into Clang 17 after it lands.

[cor3ntin]

We usually reserve these warnings for feature backported to older c++ standards, and I'm not sure "you are using a standard feature" is in general a useful warning.
however, if there are important unresolved issues, why did we set __cpp_impl_coroutine ?

[aaron.ballman]

clang/include/clang/Basic/DiagnosticGroups.td

308 ↗

(On Diff #544009)

We don't typically give these their own warning group (it's also not clear why this would be a pedantic warning warn_cxx17_compat_designated_init is incorrect).

[ChuanqiXu]

however, if there are important unresolved issues, why did we set __cpp_impl_coroutine ?

The discussion is here: https://discourse.llvm.org/t/rfc-could-we-mark-coroutines-as-unreleased-now/69220. My short summary may be:
(1) It is not necessary to claim a feature is completed only after there is no related open issues. For example, GCC claims coroutines (and many other features) as completed in the early stage and there are still many following bug reports then.
(2) Coroutines are used widely and deeply around the C++ world. People from Meta (Meta should be, at least, one of the most heavy user of coroutines in the past years) said they would love to approve to the idea even if it is not bug free.

[ilya-biryukov]

Thanks for the quick feedback!
I understand that this goes against common practice in Clang, but the reason we want this warning is very practical.

At Google, we would like to postpone adoption of coroutines until the aforementioned bugs are fixed, but we feel that Clang 17 would be a good candidate to enable other C++20 features (sans Modules).
We could have landed a local patch with a similar warning in our main toolchain that we control tightly. However, we want to have it for other toolchains (e.g. Apple Clang) that are based on upstream and we do not control.

Some code is compiled both by the main toolchain and Apple Clang and we want to have a way to enforce in the compiler that this code does not use coroutines, but is still compiled as C++20.
We are taking a very cautious approach here (the bugs above are rare), but we feel it is warranted given the amount of C++ users that we have.
I do not propose to have flags like this for every single feature, but for pragmatic reasons I think it makes sense to have it for coroutines in Clang 17.

In D156247#4532478, @cor3ntin wrote:

We usually reserve these warnings for feature backported to older c++ standards, and I'm not sure "you are using a standard feature" is in general a useful warning.

I agree that is weird, but it's not unprecedented: designated initializers are exactly like that.
Maybe there is a different warning wording and flag that would suit this better?

In D156247#4533848, @ChuanqiXu wrote:

however, if there are important unresolved issues, why did we set __cpp_impl_coroutine ?

The discussion is here: https://discourse.llvm.org/t/rfc-could-we-mark-coroutines-as-unreleased-now/69220. My short summary may be:
(1) It is not necessary to claim a feature is completed only after there is no related open issues. For example, GCC claims coroutines (and many other features) as completed in the early stage and there are still many following bug reports then.
(2) Coroutines are used widely and deeply around the C++ world. People from Meta (Meta should be, at least, one of the most heavy user of coroutines in the past years) said they would love to approve to the idea even if it is not bug free.

I also feel this is the right call, coroutines are very much usable and we have been experimenting with them at Google for a more than a year too.
However, scale matters. Our stance is that bugs above are not blockers for one or two projects with experts that can deal with compiler bugs, but they are blockers for allowing all our C++ users to use coroutines.

A few questions for the reviewers to understand how to move forward:

• Does the use-case mentioned above make sense (allow C++20, disallow coroutines for code using Clang 17)?
• Is the warning on coroutines a proper way to solve this in Clang or are there alternative approaches we should consider?

clang/include/clang/Basic/DiagnosticGroups.td

308 ↗

(On Diff #544009)

The reason for a separate warning group is to have a way to detect and prevent (via -Werror) the use of coroutines, but not other features.
Wpre-c++20-compat-coroutines might be a totally wrong name.

[cor3ntin]

In D156247#4534645, @ilya-biryukov wrote:

Thanks for the quick feedback!
I understand that this goes against common practice in Clang, but the reason we want this warning is very practical.

At Google, we would like to postpone adoption of coroutines until the aforementioned bugs are fixed, but we feel that Clang 17 would be a good candidate to enable other C++20 features (sans Modules).
We could have landed a local patch with a similar warning in our main toolchain that we control tightly. However, we want to have it for other toolchains (e.g. Apple Clang) that are based on upstream and we do not control.

Some code is compiled both by the main toolchain and Apple Clang and we want to have a way to enforce in the compiler that this code does not use coroutines, but is still compiled as C++20.
We are taking a very cautious approach here (the bugs above are rare), but we feel it is warranted given the amount of C++ users that we have.
I do not propose to have flags like this for every single feature, but for pragmatic reasons I think it makes sense to have it for coroutines in Clang 17.

In D156247#4532478, @cor3ntin wrote:

We usually reserve these warnings for feature backported to older c++ standards, and I'm not sure "you are using a standard feature" is in general a useful warning.

I agree that is weird, but it's not unprecedented: designated initializers are exactly like that.
Maybe there is a different warning wording and flag that would suit this better?

Designated initializers have this warning because they have been backported to older language mode, and they have
a different warning in these modes https://gcc.godbolt.org/z/9acefPW1n (which is important for pedantic conformance), and i guess the c++20 warning helps identify portability issues when trying newer standards on code bases that use these extensions.

But that does not apply to coroutines that are not available in older language modes.

Which for me brings a few questions. Why is it an issue for coroutines specifically? Concepts are also rough on the edges (but we do not set up the feature test macro), and consteval has (or had until very recently)
some bugs too (including code gen bugs, ironically!)

So the only warning that would make sense to me is "Support for coroutines is still experimental." I think that's what you want to express here.
Which again begs the question of why we claim conformance for something we think is too unstable to use in production.
Three of the four bugs above are miss compilation of code that does not look particularly unusual, so I'm not sure the number of users matters. Whoever encounters that code will have an unpleasant time.
And as you say. few people can work their way around such bugs

But if we think coroutines are good enough for one groups of users but not another... i'm not sure the warning belongs upstream.
Because the logical conclusion of that is that we would have to add warnings for any features added to the language.... which i don't think would benefit the ecosystem.

Having a separate warning group (ie pre-c++20-compat-coroutines) also makes me wonder about future clang versions:

• Either we remove the warning group and break build scripts
• We keep the warning group forever
• We keep the group but stop emitting the warning, which is also not great.

Sadly, i do not have the expertise to look at codegen bugs but maybe we should prioritize them higher in the hope of fixing some of them in the next few weeks.
If we could identify the problematic patterns, i think emitting an error from codegen until we fix them would also be reasonable.

[ChuanqiXu]

There are several topics now. Let's discuss them separately to make things clear:

(1) Should we add such warning?

Does the use-case mentioned above make sense (allow C++20, disallow coroutines for code using Clang 17)?

This looks like a coding guide to me. So I feel it may be better to follow what other coding guides does. As far as I know, it is rare to insert warnings to the compilers for a specific coding guide.

(2) Is it OK to claim coroutines as released?

The reason why we marked it is in the above link. And I think the key question here is that how stable coroutines is in production? And as far as I know, all the industry users (Facebook, Alibaba, Seastar, I am not sure if Google has used it in production) don't feel bad with the current status. Also there a lot of relatively smaller projects which have been using coroutines for a long time. So personally, I feel it is OK to mark it as completed.

Also my intention to mark it as released is primarily for smaller groups. Since they may lack the confidence to use it in serious situations. But I feel (and always tell) they can. Since we already tried it for a long time.

(3) What's the practical affects for these bugs?

https://github.com/llvm/llvm-project/issues/57638: it is about the performance at the O0 level. So I feel the priority is relatively low. Since generally we don't care about the performance within O0. (I know there are some against opinions)

https://github.com/llvm/llvm-project/issues/63818: it looks like only related to statement expressions. Maybe it is a good idea to emit warnings for using coroutines in statement expressions. Maybe we can fix this in the frontend as @rsmith said.

https://github.com/llvm/llvm-project/issues/58459: while we need to look into the reasons, from the reproducer, it looks like it only appears if the return type of the await_resume() function is std::nullptr_t, which is rare. Also I feel this is a frontend issue.

https://github.com/llvm/llvm-project/issues/56301: this is the most severe issue among these reports. This issue may only be possible with a special await_suspend implementation. And ecosystem of coroutines is that some people write coroutine library and other people use these coroutine libraries. Then it won't be a problem for the wide users if the library don't use a special await_suspend implementation. And this is why the existing users of coroutines don't meet the issue.

(4) How should we handle the issue?

I have an idea about https://github.com/llvm/llvm-project/issues/56301 and it requires us to implement a new alias analysis pass for coroutines. And I can image there will be a lot of works and I don't think I can make it in the recent 2~3 months. (This may be the reason that I always delay it...). I can try to promote its priority but I can't promise when can I fix it...

For https://github.com/llvm/llvm-project/issues/57638, I feel it is hard to fix and the practical affects of it doesn't matter really. So I think we don't need to discuss it now.

I don't have insights about https://github.com/llvm/llvm-project/issues/63818 and https://github.com/llvm/llvm-project/issues/58459. I feel https://github.com/llvm/llvm-project/issues/58459 may be an oversight somewhere but I didn't look into it.

Hope this helps.

[ilya-biryukov]

The list of bugs I shared was meant to illustrate the problem, it's not meant to be exhaustive. I am not sure there is a need to rush and fix them before the release, it is just a signal that makes us worried about the feature.
The bugs are niche, and likely not a problem for most people who want to use coroutines today. As I mentioned before, we tend to be overly cautious when it comes to supporting features for a large population of our users.

So the only warning that would make sense to me is "Support for coroutines is still experimental." I think that's what you want to express here.
Which again begs the question of why we claim conformance for something we think is too unstable to use in production.

I am not sure if there is a process for ensuring conformance in Clang in general. We should have one, but how do we know if the feature is buggy until users try it?
Not enabling coroutines in Clang or not defining the feature macros would likely lead to people not using coroutines and, consequently not discovering those bugs.

"Support for coroutines is experimental" is temporary, we might need to remove it in the future (this seems fine, I suspect there were precedents before).
Having something like -Wcoroutines-used saying "you use coroutines and requested to flag it by a compiler warning" is likely to be hold up for the years to come.

I agree that doing it for all features is an overkill, I also think this warning is very niche.
As mentioned before, we want it for purely pragmatic reasons, I acknowledge this does not align well with Clang's warnings nor generalizes well to all features.

This looks like a coding guide to me. So I feel it may be better to follow what other coding guides does. As far as I know, it is rare to insert warnings to the compilers for a specific coding guide.

It is ultimately a style guide enforcement, yes. Using a compiler means we can reliably capture all violations.
In a typical situation, not following a style guide results in "worse" code (in terms of the style guide itself), so enforcing via linters works well.
Here, it can lead to miscompiles, so enforcing at compiler level and catching all violations is preferable. Again, we are overly cautious about miscompiles when shipping to a large number of users.

I am not sure if Google has used it in production

We do have it in production, but for a very limited experiment and not widely deployed.

[tahonermann]

At first, it sounded to me like the option that would best suit Google's needs is -fno-coroutines. However:

• I see that Google does have some limited use of coroutines; perhaps that could be enabled on a per-TU or project basis by the build system? However...
• The -fno-coroutines option doesn't seem to work; https://godbolt.org/z/Phhqjd9so (Nor does -fcoroutines work to enable standard library coroutine features in pre-C++20 language modes).

[ilya-biryukov]

In D156247#4535664, @tahonermann wrote:

At first, it sounded to me like the option that would best suit Google's needs is -fno-coroutines. However:

• I see that Google does have some limited use of coroutines; perhaps that could be enabled on a per-TU or project basis by the build system? However...
• The -fno-coroutines option doesn't seem to work; https://godbolt.org/z/Phhqjd9so (Nor does -fcoroutines work to enable standard library coroutine features in pre-C++20 language modes).

You are absolutely right, -fno-coroutines would totally work for us, had it been available.

[cor3ntin]

If there is consensus we need to do _something_, I would prefer that solution as well

[tahonermann]

You are absolutely right, -fno-coroutines would totally work for us, had it been available.

Good. Gcc handles -fcoroutines and -fno-coroutines as I would expect (https://godbolt.org/z/7zEMd7cdW), so matching gcc behavior makes sense in any case. I would expect implementation to be quite straight forward, so I recommend we head in that direction.

[aaron.ballman]

In D156247#4536034, @tahonermann wrote:

You are absolutely right, -fno-coroutines would totally work for us, had it been available.

Good. Gcc handles -fcoroutines and -fno-coroutines as I would expect (https://godbolt.org/z/7zEMd7cdW), so matching gcc behavior makes sense in any case. I would expect implementation to be quite straight forward, so I recommend we head in that direction.

I think we generally want to avoid "congratulatory" diagnostics unless the language feature is truly surprising to users in practice. e.g., we have -Wvla as a diagnostic to tell users when they're using a VLA because of just how often people think const int i = 12; int array[i]; forms a constant-sized array when it doesn't (and VLAs can have security implications). There's nothing about coroutines that seems like it would surprise users such that they need a diagnostic to tell them they're using the feature. So I don't think this meets the bar for a diagnostic in Clang.

That said, I think -fno-coroutines should work.

[shafik]

In D156247#4536034, @tahonermann wrote:

You are absolutely right, -fno-coroutines would totally work for us, had it been available.

Good. Gcc handles -fcoroutines and -fno-coroutines as I would expect (https://godbolt.org/z/7zEMd7cdW), so matching gcc behavior makes sense in any case. I would expect implementation to be quite straight forward, so I recommend we head in that direction.

This feels like a much cleaner solution to me as well.

[ChuanqiXu]

While I am not against the approach, do you think we need similar semantics for -fno-concepts, -fno-modules, etc... ?

[ChuanqiXu]

FYI, for https://github.com/llvm/llvm-project/issues/56301, I've posted https://discourse.llvm.org/t/rfc-a-new-aapass-for-coroutines-or-a-simple-workaround/72336 to make some initial progress.

[aaron.ballman]

In D156247#4537351, @ChuanqiXu wrote:

While I am not against the approach, do you think we need similar semantics for -fno-concepts, -fno-modules, etc... ?

I think we need to take it on a case by case basis. In general, we only let users disable language features when the feature has hidden costs that cannot be avoided (e.g., exceptions and RTTI in C++), which isn't the case with coroutines. However, because GCC allows you to disable coroutines and we try to keep our flags reasonably compatible, and because we have users with a concrete need, it is perhaps reasonable in this case. I think the answer is different for something like concepts because those are used by the STL, so disabling them has greater impact on what code is accepted by the compiler. If GCC didn't support -fno-coroutines, I'd likely be opposed to adding it to Clang because coroutines impose no hidden costs unless you actually use the feature and the use case is unmotivated (clang-tidy is the tool for enforcing coding style guides).

We definitely do not want a complicated testing matrix of "what if this set of features is enabled and that set of features is disabled but the rest follows the given language standard". One thing that worries me in this case is the <generator> header in C++23; without coroutines, that can't compile.

Personally, I think we should err on the side of being conservative with language dialect flags unless there's a clear need: don't reflexively enable features as extensions in older language modes and don't allow users to disable features in newer language modes.

[aaron.ballman]

I would like explicit buy-in from the libc++ folks on the idea of adding -fno-coroutines as they're going to be impacted by Clang supporting such an option (and they may have additional testing requirements to ensure that libc++ works reasonably well when coroutines are disabled).

[ChuanqiXu]

Thanks for the explanation. I am strongly in favor with you : )

[cor3ntin]

In D156247#4534645, @ilya-biryukov wrote:

At Google, we would like to postpone adoption of coroutines until the aforementioned bugs are fixed, but we feel that Clang 17 would be a good candidate to enable other C++20 features (sans Modules).
We could have landed a local patch with a similar warning in our main toolchain that we control tightly. However, we want to have it for other toolchains (e.g. Apple Clang) that are based on upstream and we do not control.

Given that adoption by Apple Clang is one of the motivation to make this change upstream, do we have a reasonable expectation they would pick this change soon? Historically Apple follows a release schedule that is not synchronized
with the LLVM release process.

[ilya-biryukov]

Thanks, disabling the language feature definitely makes more sense.
-fno-coroutines is trickier to implement, but this cannot be too hard.

While we are waiting for libc++ folks to come back with feedback, what would be the desired behavior. GCC seems to

1. errors out on #include <coroutine> (undefines feature test macros, so STL shows an error),
2. treats coroutine keywords as identifiers.

While (1) makes total sense, I'm not sure about (2). Should we instead show errors, but still treat co_await and friends as keywords?
I think it would be a better choice because it would prevent writing non-standard-compliant code.
On the other hand, given the co_ prefix, the clashes should be really rare anyway and we might want to match GCC behavior.

Thoughts?

In D156247#4538647, @cor3ntin wrote:

In D156247#4534645, @ilya-biryukov wrote:

At Google, we would like to postpone adoption of coroutines until the aforementioned bugs are fixed, but we feel that Clang 17 would be a good candidate to enable other C++20 features (sans Modules).
We could have landed a local patch with a similar warning in our main toolchain that we control tightly. However, we want to have it for other toolchains (e.g. Apple Clang) that are based on upstream and we do not control.
Given that adoption by Apple Clang is one of the motivation to make this change upstream, do we have a reasonable expectation they would pick this change soon? Historically Apple follows a release schedule that is not synchronized

with the LLVM release process.

[ilya-biryukov]

In D156247#4538647, @cor3ntin wrote:

Given that adoption by Apple Clang is one of the motivation to make this change upstream, do we have a reasonable expectation they would pick this change soon? Historically Apple follows a release schedule that is not synchronized
with the LLVM release process.

I thought Apple releases are based on major Clang releases, but may be wrong here.
Maybe @ldionne or someone else from Apple could clarify this?

[philnik]

In D156247#4538626, @aaron.ballman wrote:

I would like explicit buy-in from the libc++ folks on the idea of adding -fno-coroutines as they're going to be impacted by Clang supporting such an option (and they may have additional testing requirements to ensure that libc++ works reasonably well when coroutines are disabled).

I don't think we want to support having another flag for a dialect, since that will result in more problems down the road. We already have a problem with the number of configurations we support and adding style-guide configurations won't help that. IMO this should just be a clang-tidy check. They can be enforced just as well as compiler warnings/errors, and don't cause problems for other people.

[ilya-biryukov]

• Expose -fno-coroutine and add tests

Current implementation exposes:

• -fno-coroutines to disable Coroutines in C++20 and beyond,
• -fcoroutines to enable Coroutines in prior standards.

TODO: decide if we want to allow -fcoroutines in C++17. If so, add tests for codegen. If not, add a warning.
TOOD: release notes.

[ilya-biryukov]

In D156247#4538962, @philnik wrote:

I don't think we want to support having another flag for a dialect, since that will result in more problems down the road. We already have a problem with the number of configurations we support and adding style-guide configurations won't help that. IMO this should just be a clang-tidy check. They can be enforced just as well as compiler warnings/errors, and don't cause problems for other people.

I don't want to go too much into detail, but unfortunately we cannot enforce this with a tidy check.
We do not have reliable ways to mark all shared code that is compiled by multiple toolchains (e.g. Apple Clang and stable Clang). Consequently we cannot guarantee that tidy runs on that code.
Moreover, linters like clang-tidy can be accidentally disabled or ignored and we want strict enforcement for this particular change.

Could we find a compromise here? E.g.

• We would be fine with an "unsupported" configuration. We can fix any issues with it ourselves.
• Could we keep -fno-coroutines as an internal -cc1 option and not advertise it?

clang/test/SemaCXX/coroutines.cpp

5

Note to self: this is wrong and should be removed.

NOTE: Clang-Format Team Automated Review Comment

It looks like your clang-format review does not contain any unit tests, please try to ensure all code changes have a unit test (unless this is an NFC or refactoring, adding documentation etc..)

Add your unit tests in clang/unittests/Format and you can build with ninja FormatTests. We recommend using the verifyFormat(xxx) format of unit tests rather than EXPECT_EQ as this will ensure you change is tolerant to random whitespace changes (see FormatTest.cpp as an example)

For situations where your change is altering the TokenAnnotator.cpp which can happen if you are trying to improve the annotation phase to ensure we are correctly identifying the type of a token, please add a token annotator test in TokenAnnotatorTest.cpp

[aaron.ballman]

In D156247#4538664, @ilya-biryukov wrote:

Thanks, disabling the language feature definitely makes more sense.
-fno-coroutines is trickier to implement, but this cannot be too hard.

While we are waiting for libc++ folks to come back with feedback, what would be the desired behavior. GCC seems to

1. errors out on #include <coroutine> (undefines feature test macros, so STL shows an error),
2. treats coroutine keywords as identifiers.

While (1) makes total sense, I'm not sure about (2). Should we instead show errors, but still treat co_await and friends as keywords?
I think it would be a better choice because it would prevent writing non-standard-compliant code.
On the other hand, given the co_ prefix, the clashes should be really rare anyway and we might want to match GCC behavior.

Thoughts?

If we allow the feature to be disabled, it should be fully disabled. I doubt people are going to need to disable the feature because they have co_yield as an identifier in their program, but just the same, it'd be pretty strange to have a feature only partially disabled.

In D156247#4539048, @ilya-biryukov wrote:

In D156247#4538962, @philnik wrote:

I don't think we want to support having another flag for a dialect, since that will result in more problems down the road. We already have a problem with the number of configurations we support and adding style-guide configurations won't help that. IMO this should just be a clang-tidy check. They can be enforced just as well as compiler warnings/errors, and don't cause problems for other people.

I don't want to go too much into detail, but unfortunately we cannot enforce this with a tidy check.
We do not have reliable ways to mark all shared code that is compiled by multiple toolchains (e.g. Apple Clang and stable Clang). Consequently we cannot guarantee that tidy runs on that code.
Moreover, linters like clang-tidy can be accidentally disabled or ignored and we want strict enforcement for this particular change.

I'm not certain this is particularly motivating rationale for why Clang should allow people to turn off arbitrary standards features.

Could we find a compromise here? E.g.

• We would be fine with an "unsupported" configuration. We can fix any issues with it ourselves.

Anything we do that's user facing is something we have to support. I don't think it makes sense to add a language dialect mode and tell users "if you use this, we won't support it."

• Could we keep -fno-coroutines as an internal -cc1 option and not advertise it?

That would be an approach to ensuring this is not a supported configuration and that any fallout from using the flag is on the user rather than the community.

One interesting thing to note is that GCC doesn't seem to advertise -fno-coroutines as a supported option (https://gcc.gnu.org/onlinedocs/gcc-13.2.0/gcc/C_002b_002b-Dialect-Options.html#index-fcoroutines). They document other -ffoo/-fno-foo options, so the lack of documentation for -fno-coroutines could perhaps be indicative that this isn't intended to be a supported command line flag for GCC. We should reach out to some GCC folks to see if this is an oversight in their documentation or not. If there's not a GCC compatibility story here, that may change opinions on how/if to proceed.

[Mordante]

In D156247#4538626, @aaron.ballman wrote:

I would like explicit buy-in from the libc++ folks on the idea of adding -fno-coroutines as they're going to be impacted by Clang supporting such an option (and they may have additional testing requirements to ensure that libc++ works reasonably well when coroutines are disabled).

Thanks for the adding libc++ to the review @aaron.ballman!

If some concerns with this patch

1. I share @philnik's concern that it adds a new configuration we need to test.
2. My biggest concern is the timing of this patch. The date of the branching is know upfront and well communicated by the release managers. This patch was created after the LLVM-17 branch was created. For libc++ this patch would be a new feature which has no patch at the moment. This means we're asked to add a new feature after RC1. In libc++ we normally avoid adding new features in the release branch.
3. I don't see a consensus from the Clang developers to add this option. I also see no consensus coroutines are "broken". I agree if we were to do something the -fno-coroutines would be the way to go.

I do not speak for Apple, but I know the beta's for the new XCode are available since June and I have not heard from @ldionne (who works at Apple) there were concerns with coroutines. @ldionne is out of the office at the moment. So even when we add this option now it will not be available in the next XCode. (Unless Apple backports it.)

@ilya-biryukov if you feel strongly about pursuing this patch I would like to see:

• a consensus of the Clang developers this is really needed,
• a pre-approval by the release managers to apply these changes to the release branch,
• a libc++ patch that works with -fno-coroutines, which I would be happy to review.

@ilya-biryukov the next time it would be really great to have such intrusive changes up for review well before the release branch is created. That gives the LLVM community time review this without the need to rush it. For me it would have been a lot easier to accept such a patch a month ago than it's now.

[ChuanqiXu]

We should reach out to some GCC folks to see if this is an oversight in their documentation or not.

I've sent a mail: https://gcc.gnu.org/pipermail/gcc/2023-July/242159.html

[ilya-biryukov]

@aaron.ballman the internal -cc1 flag is good enough for us and should aleviate most of the concerns in this thread. We could also live without a libc++ change.
I suggest to move forward with -cc1 flag. Would that be okay or do you feel this does not address some of the concerns mentioned in the thread?

@thieta, @tstellar would you be willing to cherry-pick a flag to disable coroutines to a release branch?
There are two potential forms, in case the answers differ:

• user-facing clang -fno-coroutines flag,
• internal clang -cc1 -fno-coroutines.

In D156247#4540228, @Mordante wrote:

2. My biggest concern is the timing of this patch. The date of the branching is know upfront and well communicated by the release managers. This patch was created after the LLVM-17 branch was created. For libc++ this patch would be a new feature which has no patch at the moment. This means we're asked to add a new feature after RC1. In libc++ we normally avoid adding new features in the release branch.

I am sorry for sending the patch late, we have not thought about it before. I have already mentioned that before in the thread.
We could have foreseen the need for this change before, but we did not and there is nothing I can do to fix it now.

@ilya-biryukov if you feel strongly about pursuing this patch I would like to see:

As mentioned before, we feel this would give us substantial benefits when rolling out C++20.
I acknowledge that coroutines are good enough for most people and those bugs may not be a problem. We are overly cautious, our bar is higher and we would not to ship coroutines with those bugs (at the very least we'd like to have an option to bail out).

• a consensus of the Clang developers this is really needed,
• a pre-approval by the release managers to apply these changes to the release branch,

I have asked the release managers above.

• a libc++ patch that works with -fno-coroutines, which I would be happy to review.

I will check what needs to be done for libc++ here, I suspect adding #error inside <coroutine> if the feature macro is not defined should be enough.
It might not be necessary if we use a -cc1 flag.

In D156247#4541003, @ChuanqiXu wrote:

We should reach out to some GCC folks to see if this is an oversight in their documentation or not.

I've sent a mail: https://gcc.gnu.org/pipermail/gcc/2023-July/242159.html

Thanks, there is now a response in the thread saying that documentation intentionally leaves out most -fno-foo flags and GCC supports -fno-coroutines.

[cor3ntin]

How does a -cc1 flag alleviate the burden concerns for libc++? It would still be something they would have to support, unless we plan to remove the flag before c++26 and you are happy with the inclusion of <coroutine> and <generator> producing subpar errors.
However, in the C++26 cycle, <execution> (P2300) is going to have a dependency on <coroutine>, and it does not seem reasonable for that to lead to errors.

[Endill]

In D156247#4541756, @ilya-biryukov wrote:

@aaron.ballman the internal -cc1 flag

• internal clang -cc1 -fno-coroutines.

I'm sorry, but I find this wording misleading. -cc1 -fno-coroutines is not internal in the same sense as e.g. -verify is. Both are hidden from user-facing drivers, but unlike -verify, potential -cc1 -fno-coroutines is not just intended, but focused on users outside of monorepo. So I'd say it's going to be hidden but user-facing feature, rather than internal one. And the one that we should document and test if not for users, then for ourselves to make sure we maintain its semantics and don't break users. At this point I'd say if we go for it, we should go for proper driver option, rather than frontend one.

I also wonder how much complexity this language dialect would introduce while interacting with other language dialect flags. Is it really worth it?

[ilya-biryukov]

In D156247#4541826, @cor3ntin wrote:

How does a -cc1 flag alleviate the burden concerns for libc++? It would still be something they would have to support, unless we plan to remove the flag before c++26 and you are happy with the inclusion of <coroutine> and <generator> producing subpar errors.
However, in the C++26 cycle, <execution> (P2300) is going to have a dependency on <coroutine>, and it does not seem reasonable for that to lead to errors.

We would be okay with <coroutine> and <generator> producing subpar errors. Because it's on users of those flags (i.e. us) to handle those errors, the users (i.e. us) will have to stop using it in C++26 or whenever libc++ starts to depend on coroutine in other headers.
If we are the only ones who need this configuration (and this is my impression right now), it makes sense that we are the ones who need to figure out who to deal with issues like this.

In D156247#4541880, @Endill wrote:

In D156247#4541756, @ilya-biryukov wrote:

@aaron.ballman the internal -cc1 flag

• internal clang -cc1 -fno-coroutines.

I'm sorry, but I find this wording misleading. -cc1 -fno-coroutines is not internal in the same sense as e.g. -verify is. Both are hidden from user-facing drivers, but unlike -verify, potential -cc1 -fno-coroutines is not just intended, but focused on users outside of monorepo. So I'd say it's going to be hidden but user-facing feature, rather than internal one. And the one that we should document and test if not for users, then for ourselves to make sure we maintain its semantics and don't break users. At this point I'd say if we go for it, we should go for proper driver option, rather than frontend one.

The -cc1 flags can change between compiler releases and do not need to provide the same level of guarantees as user-facing features.
I do not see why we would want to put a high bar on documenting, testing -cc1 -fno-coroutines and making sure that it works.
E.g. Clang has -cc1 -fcoroutines, but it does not enable coroutines in -std=c++17 like GCC does. We don't test it, we intend to ship Clang 17 with this behavior and I don't see why that's a problem.

I also wonder how much complexity this language dialect would introduce while interacting with other language dialect flags. Is it really worth it?

The complexity on the compiler side seems very low (this patch or something not much harder). If we do not require libc++ to support this configuration and allow it to break going forward (that is my proposal), this also should not create problems for libc++.

[aaron.ballman]

Given the views expressed on this thread, I think this requires a wider community discussion to determine whether we want to support this idea or not, regardless of -cc1 or driver-level option. We should not be adding a novel language dialect as we're getting ready to cut a release unless the need and benefits are *very* compelling, and I don't think that's the case here. We need the time to think about the long-term effects of such a mode, so I don't think Clang 17 is an appropriate ship vehicle for this change, especially because Google can carry this patch in your downstream easily enough.

My recommendation is to start an RFC on Discourse. If the community quickly and decisively agrees this should land in Clang 17 in the next few days, it might still be reasonable to make the rc2 cutoff, but I'd leave that decision to the release managers and it would be contingent on strong libc++ maintainer agreement to whatever direction we go (because they'll be the most impacted by this decision).

[Mordante]

In D156247#4541756, @ilya-biryukov wrote:

In D156247#4540228, @Mordante wrote:

2. My biggest concern is the timing of this patch. The date of the branching is know upfront and well communicated by the release managers. This patch was created after the LLVM-17 branch was created. For libc++ this patch would be a new feature which has no patch at the moment. This means we're asked to add a new feature after RC1. In libc++ we normally avoid adding new features in the release branch.

I am sorry for sending the patch late, we have not thought about it before. I have already mentioned that before in the thread.

I've read that, before my reply. That does not change the fact I feel it is a concern.

We could have foreseen the need for this change before, but we did not and there is nothing I can do to fix it now.

I understand.

• a pre-approval by the release managers to apply these changes to the release branch,

I have asked the release managers above.

Thanks.

• a libc++ patch that works with -fno-coroutines, which I would be happy to review.

I will check what needs to be done for libc++ here, I suspect adding #error inside <coroutine> if the feature macro is not defined should be enough.

This is not enough. There is quite a bit of work involved on the libc++ side. Here is a quick list of the things I directly can think of. First the patch in Clang needs to be available in our CI:

1. The patch needs to land in LLVM
2. We need to wait for the patch to be available in a build on apt.llvm.org (typically this takes one day, but may be longer if the build fails.)
3. We need to update and test our Docker image (this takes a few hours)
4. We need to upload the Docker image to the CI runners and wait for it to propagate (takes about 8 hours)

So it will take at least two days to get this working. The CI parts needs a libc++ developer who wants to do the work and have time in their schedule to work on this. When nobody has time, it will take longer to get the CI working.

Then the patch itself needs to do the following things (the work can be done directly, testing requires the CI)

1. Add #error inside <coroutine>
2. Add a LIBCPP feature flag and propagate that to lit
3. Adjust the tests that would fail with -fno-coroutine
4. Add a CI job so we can validate the new feature works

As @cor3ntin pointed out we need to be wary of regressions since other C++ features depend on coroutines. So we need a CI job to test whether the feature works correctly and do not regress when we add features depending on coroutines.

Maybe that helps a bit more to explain why this is a concern and not something we can do quickly on the libc++ side.

In D156247#4542155, @aaron.ballman wrote:

Given the views expressed on this thread, I think this requires a wider community discussion to determine whether we want to support this idea or not, regardless of -cc1 or driver-level option. We should not be adding a novel language dialect as we're getting ready to cut a release unless the need and benefits are *very* compelling, and I don't think that's the case here. We need the time to think about the long-term effects of such a mode, so I don't think Clang 17 is an appropriate ship vehicle for this change, especially because Google can carry this patch in your downstream easily enough.

My recommendation is to start an RFC on Discourse. If the community quickly and decisively agrees this should land in Clang 17 in the next few days, it might still be reasonable to make the rc2 cutoff, but I'd leave that decision to the release managers and it would be contingent on strong libc++ maintainer agreement to whatever direction we go (because they'll be the most impacted by this decision).

+1

[ilya-biryukov]

In D156247#4542155, @aaron.ballman wrote:

Given the views expressed on this thread, I think this requires a wider community discussion to determine whether we want to support this idea or not, regardless of -cc1 or driver-level option. We should not be adding a novel language dialect as we're getting ready to cut a release unless the need and benefits are *very* compelling, and I don't think that's the case here. We need the time to think about the long-term effects of such a mode, so I don't think Clang 17 is an appropriate ship vehicle for this change, especially because Google can carry this patch in your downstream easily enough.

Google cannot do this with a downstream patch as we need this for toolchains that we do not build from the LLVM sources we integrate.
If we could do with a local patch, we would have definitely done it.

My recommendation is to start an RFC on Discourse. If the community quickly and decisively agrees this should land in Clang 17 in the next few days, it might still be reasonable to make the rc2 cutoff, but I'd leave that decision to the release managers and it would be contingent on strong libc++ maintainer agreement to whatever direction we go (because they'll be the most impacted by this decision).

https://discourse.llvm.org/t/rfc-add-a-flag-to-clang-17-to-disable-coroutines-in-c-20

@aaron.ballman could you comment on having a -cc1 flag without guarantees from libc++ folks that this works (so it imposes no support costs on them)?

[aaron.ballman]

In D156247#4542769, @ilya-biryukov wrote:

In D156247#4542155, @aaron.ballman wrote:

Given the views expressed on this thread, I think this requires a wider community discussion to determine whether we want to support this idea or not, regardless of -cc1 or driver-level option. We should not be adding a novel language dialect as we're getting ready to cut a release unless the need and benefits are *very* compelling, and I don't think that's the case here. We need the time to think about the long-term effects of such a mode, so I don't think Clang 17 is an appropriate ship vehicle for this change, especially because Google can carry this patch in your downstream easily enough.

Google cannot do this with a downstream patch as we need this for toolchains that we do not build from the LLVM sources we integrate.
If we could do with a local patch, we would have definitely done it.

Good to know! How certain are you that the other toolchains will have been cut such that they support this feature given how late in our cycle it is? @Mordante had some evidence that Apple Clang is from June, for example.

My recommendation is to start an RFC on Discourse. If the community quickly and decisively agrees this should land in Clang 17 in the next few days, it might still be reasonable to make the rc2 cutoff, but I'd leave that decision to the release managers and it would be contingent on strong libc++ maintainer agreement to whatever direction we go (because they'll be the most impacted by this decision).

https://discourse.llvm.org/t/rfc-add-a-flag-to-clang-17-to-disable-coroutines-in-c-20

@aaron.ballman could you comment on having a -cc1 flag without guarantees from libc++ folks that this works (so it imposes no support costs on them)?

Personally (not a code owner decision statement)...
I think it's a bad precedent to add a -cc1 flag knowing we won't support it and that only exists to help enforce a coding style decision. I think it's especially bad precedent to add features to a release branch without having any bake time in the main tree. I don't think we should add any -fno- flags for standardized language features unless the feature imposes extra cost even when not in use. e.g., exceptions and RTTI are very reasonable to disable, but allowing arbitrary mixing and matching of language features to create your own mutant C++ version seems like a support nightmare in general. I know that clang-tidy isn't a great solution for you, but that's where I think this functionality belongs just the same even if it isn't ideal.

[jyknight]

As I posted on the RFC thread, I think we have a viable alternative solution to address the original motiving use-case.

One might potentially still make a case for implementing the -fno-coroutines flag for GCC compatibility, but given the concerns raised -- and that the use-case is gone -- it may be better to leave it unimplemented.

[ilya-biryukov]

Marking the revision as abandoned to reflect the status of the RFC.
Thanks everyone for your feedback and sorry about the timing of this change again.

clang/include/clang/Driver/Options.td

 // languages and accept other values such as CPU/GPU architectures,
 // offload kinds and target aliases.
 def offload_EQ : CommaJoined<["--"], "offload=">, Flags<[NoXarchOption]>,
   HelpText<"Specify comma-separated list of offloading target triples (CUDA and HIP only)">;
 
 // C++ Coroutines
 defm coroutines : BoolFOption<"coroutines",
   LangOpts<"Coroutines">, Default<cpp20.KeyPath>,
-  PosFlag<SetTrue, [CC1Option], "Enable support for the C++ Coroutines">,
-  NegFlag<SetFalse>>;
+  PosFlag<SetTrue, [CoreOption, CC1Option], "Enable support for the C++ Coroutines">,
+  NegFlag<SetFalse, [CoreOption, CC1Option], "Disable support for the C++ Coroutines">>;
 
 defm coro_aligned_allocation : BoolFOption<"coro-aligned-allocation",
   LangOpts<"CoroAlignedAllocation">, DefaultFalse,
   PosFlag<SetTrue, [CC1Option], "Prefer aligned allocation for C++ Coroutines">,
   NegFlag<SetFalse>>;
 
 defm experimental_library : BoolFOption<"experimental-library",
   LangOpts<"ExperimentalLibrary">, DefaultFalse,

clang/lib/Basic/IdentifierTable.cpp

static KeywordStatus getKeywordStatus(const LangOptions &LangOpts,
   // KEYALL means always enabled, so special case this one.
   if (Flags == KEYALL) return KS_Enabled;
   // These are tests that need to 'always win', as they are special in that they
   // disable based on certain conditions.
   if (LangOpts.OpenCL && (Flags & KEYNOOPENCL)) return KS_Disabled;
   if (LangOpts.MSVCCompat && (Flags & KEYNOMS18) &&
       !LangOpts.isCompatibleWithMSVC(LangOptions::MSVC2015))
     return KS_Disabled;
+  if (LangOpts.CPlusPlus20 && !LangOpts.Coroutines && (Flags & KEYCOROUTINES))
+    return KS_Disabled;
 
   KeywordStatus CurStatus = KS_Unknown;
 
   while (Flags != 0) {
     unsigned CurFlag = Flags & ~(Flags - 1);
     Flags = Flags & ~CurFlag;
     CurStatus = std::max(
         CurStatus,

clang/lib/Format/Format.cpp

LangOptions getFormattingLangOpts(const FormatStyle &Style) {
   LangOpts.CPlusPlus14 = LexingStd >= FormatStyle::LS_Cpp14;
   LangOpts.CPlusPlus17 = LexingStd >= FormatStyle::LS_Cpp17;
   LangOpts.CPlusPlus20 = LexingStd >= FormatStyle::LS_Cpp20;
   LangOpts.Char8 = LexingStd >= FormatStyle::LS_Cpp20;
   // Turning on digraphs in standards before C++0x is error-prone, because e.g.
   // the sequence "<::" will be unconditionally treated as "[:".
   // Cf. Lexer::LexTokenInternal.
   LangOpts.Digraphs = LexingStd >= FormatStyle::LS_Cpp11;
+  // Do not miss coroutine keywords.
+  LangOpts.Coroutines =  LexingStd >= FormatStyle::LS_Cpp20;
 
   LangOpts.LineComment = 1;
   bool AlternativeOperators = Style.isCpp();
   LangOpts.CXXOperatorNames = AlternativeOperators ? 1 : 0;
   LangOpts.Bool = 1;
   LangOpts.ObjC = 1;
   LangOpts.MicrosoftExt = 1;    // To get kw___try, kw___finally.
   LangOpts.DeclSpecKeyword = 1; // To get __declspec.

clang/lib/Frontend/InitPreprocessor.cpp

if (LangOpts.CPlusPlus20) {
     // P0848 is implemented, but we're still waiting for other concepts
     // issues to be addressed before bumping __cpp_concepts up to 202002L.
     // Refer to the discussion of this at https://reviews.llvm.org/D128619.
     Builder.defineMacro("__cpp_concepts", "201907L");
     Builder.defineMacro("__cpp_conditional_explicit", "201806L");
     Builder.defineMacro("__cpp_consteval", "202211L");
     Builder.defineMacro("__cpp_constexpr_dynamic_alloc", "201907L");
     Builder.defineMacro("__cpp_constinit", "201907L");
-    Builder.defineMacro("__cpp_impl_coroutine", "201902L");
+    // __cpp_impl_coroutine is below as it can be enabled with `-fcoroutines`.
     Builder.defineMacro("__cpp_designated_initializers", "201707L");
     Builder.defineMacro("__cpp_impl_three_way_comparison", "201907L");
     //Builder.defineMacro("__cpp_modules", "201907L");
     Builder.defineMacro("__cpp_using_enum", "201907L");
   }
+  if (LangOpts.Coroutines)
+    Builder.defineMacro("__cpp_impl_coroutine", "201902L");
   // C++23 features.
   if (LangOpts.CPlusPlus23) {
     Builder.defineMacro("__cpp_implicit_move", "202011L");
     Builder.defineMacro("__cpp_size_t_suffix", "202011L");
     Builder.defineMacro("__cpp_if_consteval", "202106L");
     Builder.defineMacro("__cpp_multidimensional_subscript", "202211L");
   }
 

clang/test/SemaCXX/coroutines.cpp

 // This file contains references to sections of the Coroutines TS, which can be
 // found at http://wg21.link/coroutines.
 
 // RUN: %clang_cc1 -std=c++23 -fsyntax-only -verify=expected,cxx20_23,cxx23    %s -fcxx-exceptions -fexceptions -Wunused-result
-// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,cxx14_20,cxx20_23 %s -fcxx-exceptions -fexceptions -Wunused-result
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,cxx14_20,cxx20,cxx20_23 %s -fcxx-exceptions -fexceptions -Wunused-result
+// RUN: %clang_cc1 -std=c++17 -fcoroutines -fsyntax-only -verify=expected,cxx14_20 %s -fcxx-exceptions -fexceptions -Wunused-result
 
 void no_coroutine_traits_bad_arg_await() {
   co_await a; // expected-error {{include <coroutine>}}
   // expected-error@-1 {{use of undeclared identifier 'a'}}
 }
 
 void no_coroutine_traits_bad_arg_yield() {
   co_yield a; // expected-error {{include <coroutine>}}

clang/test/SemaCXX/cxx20-disabled-coroutines.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++20 -fno-coroutines %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++23 -fno-coroutines %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++26 -fno-coroutines %s
+
+#if __cpp_impl_coroutine
+#else
+// expected-error@+1 {{coroutines disabled}}
+#error "coroutines disabled" 
+#endif
+
+int co_await;
+int co_return;
+int co_yield;
 No newline at end of file