RFC: add a flag to Clang 17 to disable coroutines in C++20

[ilya]

Summary

We propose to add an internal -cc1 -fno-coroutines compiler flag for disabling coroutines in -std=c++20 for Clang 17.

I propose to not advertise this configuration to the users and keeping it a -cc1 option to have the ability to remove it in the future.

Motivation

At Google, we build code with a variety of toolchains. Most code is only built with the main toolchain (Clang) that is close to LLVM head. However, some code also builds with other toolchains based on LLVM releases (most notably, Apple Clang).

We want to have the ability to enable C++20 without coroutines in the Clang 17-based toolchains .
This is motivated by unaddressed compiler bugs in coroutines, there is a list in ⚙ D156247 [Clang] Add a warning on uses of coroutine keywords.
The bugs are niche and are not blockers to using coroutines for most users. However, because of the large number of affected C++ users that we support, we are overly cautious of any miscompiles and would like to have the ability to turn off the feature if something goes wrong.

Related work

GCC has -fno-coroutines available as a user option it works as expected, disabling the feature.

Costs

This adds a new language dialect to Clang (C++20 without coroutines). Implementation in the compiler is simple, see ⚙ D156247 [Clang] Add a warning on uses of coroutine keywords.

libc++ may choose to show an error via preprocessor if <coroutine> is included, but the coroutine implementation macro is not defined (this is probably the right thing anyway as C++17).
For future standards (C++23, C++26), more standard headers (e.g. <generator>) will start depending on <coroutine> and users of -cc1 -fno-coroutines will see failures on the parts of the STL that depend on <coroutine>.
However, libc++ may also choose to not add any extra error code, in which case the users will bear the consequences of this feature will have to deal with unclear error messages.

Users of the feature will need to workaround any problems in this configuration.

Complications

• This proposal is very late in the release timeline and should have been submitted earlier. I am sorry for not bringing this up a month ago, but the idea came up in the discussions early last week.
• Support costs, see above.

Alternatives considered

Local downstream patch

Because we do not control the source code of the toolchains in questions, we do not have the ability to make this a downstream patch.

Implement this as a linter check

We cannot enforce this as a linter check (through clang-tidy or other means) because we don’t have the capacity to identify all code compiled by the toolchains in questions (this is a limitation of our internal infrastructure).

Driver flag instead of compiler flag

User-visible flags (i.e. no -cc1) have a higher quality bar and impose more support costs. Given the niche nature of the problems this proposal tries to solve (specific to Google, specific to a single feature and to a single compiler release) and short timelines to the release, I believe we should not consider a public flag.

Compiler warning

Marking all uses of a standard feature does not seem like a good fit for a compiler warning.

[tobiashieta]

Since it was discussed in Phab, I’ll chime in here as the release manager for 17. I don’t mind this if:

1. we can land it before rc3/4 timeline. Giving us around a month to do this.
2. the libc++ devs are ok with it in that timeline.

Other than that I don’t have any objections.

[mordante]

@ilya

However, libc++ may also choose to not add any extra error code, in which case the users will bear the consequences of this feature will have to deal with unclear error messages.

I really dislike the idea of this. When people use it we will get bug reports and unhappy users. I don’t think it’s good for PR when Clang and libc++ do not work nicely together. This means adding the feature will have maintenance cost for libc++.

@ilya

Complications

• This proposal is very late in the release timeline and should have been submitted earlier. I am sorry for not bringing this up a month ago, but the idea came up in the discussions early last week.
• Support costs, see above.

I just wrote in the review the work needed to implement this properly. Especially the CI part needs a libc++ developer to have the time to work on it. Based on @tobiashieta’s reply this seems not as urgent as I initially expected.

We have been shipping coroutines as stable for multiple libc++ releases, so to me it looks odd to now give a signal they are not working correctly.

Personally I really dislike the idea of adding new features after branching the release. I think it sets a bad precedence. IMO the goal after branching is to stabilize the release and not add new features. In general in libc++ we’re very reluctant to add new features after branching. Typically we only do that when a feature just missed the deadline.

One of the things I like to know is this something only useful for Google or are there more users who want to disable coroutines and if so why do they want to disable it.

The other thing I like to know, why can’t we fix the coroutine bugs before LLVM 17 is released. That in my opinion would be the best solution for all users of coroutines.

[tahonermann]

I’m not opposed to such an option, but I have a slight preference to just commit to providing a public option that matches gcc behavior rather than a half way undocumented approach.

I have sympathy for @mordante’s position, but I think it is also pretty reasonable for libc++ to respect the C++ feature test macros and disable coroutine support if __cpp_impl_coroutine is not defined. I think it is also reasonable for libc++ to require both __cpp_impl_coroutine and C++20 in order for coroutine support to be enabled.

I also agree that the best solution would be to just fix the bugs if we have people with the right combination of time and talent available to do so.

[cjdb]

@mordante

One of the things I like to know is this something only useful for Google or are there more users who want to disable coroutines and if so why do they want to disable it.

I expect that Chromium, ChromiumOS, Android, and Fuchsia will need this support as well. I understand that folks typically conflate these with “Google code” (since they’re Google-owned), but they are disjoint projects to our internal code, and all maintain their own independent toolchains. At least three of the above projects also have downstream forks that will be impacted by this.

[pkasting]

Speaking for Chromium: We have a simple presubmit python script that just regexes for the co_ keywords and warns people if they try to commit a patch using them. That’s enough for our purposes; I don’t think we’d be interested in this particular warning.

[Endill]

@ilya

(most notably, Apple Clang).

According to Wikipedia, currently shipped Apple Clang is based on Clang 15, while beta version is based on Clang 16. Since this RFC targets 17, I assume you want to get this feature rather soon, but it’s still going to take long to get a stable Apple Clang with this option. Getting this patch into Apple Clang directly seems a much shorter way than going through upstream Clang.

@ilya

We want to have the ability to enable C++20 without coroutines in the Clang 17-based toolchains .
This is motivated by unaddressed compiler bugs in coroutines, there is a list in D156247 [Clang] Add a warning on uses of coroutine keywords .
The bugs are niche and are not blockers to using coroutines for most users. However, because of the large number of affected C++ users that we support, we are overly cautious of any miscompiles and would like to have the ability to turn off the feature if something goes wrong.

I don’t see this a good precedent of providing a flag to disable a major language feature that has been shipping as stable for a while. We are receiving a constant stream of bugs for every major feature of C++20. On top of that, concepts do not have stable mangling in Itanium ABI. While it may not be an issue for you, it still could be for other major C++ deployments out there. If they come to us proposing -fno-concepts because of that, how are we going to respond after this precedent?

@ilya

GCC has -fno-coroutines available as a user option it works as expected, disabling the feature.

It should be noted, though, that according to SourceGraph it’s not used in the wild at all.

@ilya

We cannot enforce this as a linter check (through clang-tidy or other means) because we don’t have the capacity to identify all code compiled by the toolchains in questions (this is a limitation of our internal infrastructure).

If Google has special needs, I find it fair that it should strengthen its internal infrastructure to meet them. Especially given that we already provide static analyzer designed to enforce this kind of coding guides.

@ilya

User-visible flags (i.e. no -cc1) have a higher quality bar and impose more support costs. Given the niche nature of the problems this proposal tries to solve (specific to Google, specific to a single feature and to a single compiler release) and short timelines to the release, I believe we should not consider a public flag.

This flag should be documented, otherwise I don’t see why Clang contributors would be concerned with keeping it working as intended. If we are going to match GCC, it should be done on driver level, which you argue against.

You also mentioned “single compiler release”. If you’re sure that your concerns regarding coroutines are going to be addressed in the next release, what’s the rush then? Not to mention that feature patches that target release branch specifically don’t seem typical to me.

---

While I’m sympathetic to miscompilation concerns that manifest in bugs that hard to debug, I strongly disagree with the direction of this RFC. I agree with @mordante that addressing these concerns in the next release is a path forward for the use case you have.

[jyknight]

The proposal initiated with a concern I had raised internally at Google. (I’ll just add that it was not mainly Apple Clang I was concerned with, but really the “long tail” of all the miscellaneous other toolchains people build parts of our internal codebase with, for varied purposes).

Given the high level of pushback on the review and here, Ilya has suggested an alternative to me: we can locally create a file named ‘coroutine’ and put it in the include path before the libc++ headers. I think that’ll be sufficient to address my concerns, and I wish we’d thought of this earlier, before triggering a huge debate.

So, I think we can call this proposal rejected/withdrawn. Sorry!

[AaronBallman]

That’s a creative and great solution, thank you for sharing it!

[mordante]

Great to hear there is a simple solution for you to address this issue.

[mordante]

I’ve seen this proposal has been withdrawn. I still want to give some feedback regarding the libc++ process in using “new” compiler features.

@tahonermann

I have sympathy for @mordante’s position, but I think it is also pretty reasonable for libc++ to respect the C++ feature test macros and disable coroutine support if __cpp_impl_coroutine is not defined. I think it is also reasonable for libc++ to require both __cpp_impl_coroutine and C++20 in order for coroutine support to be enabled.

When a new feature is added to the compiler we indeed conditionally enable this based on the feature-test macro for that feature. This is needed since clang-HEAD supports it, but clang-LATEST_RELEASE doesn’t. At some point when all compilers supported by libc++ support the feature we enable it unconditionally. For example ⚙ D135274 [libc++] Remove _LIBCPP_HAS_NO_CXX20_COROUTINES removed the conditionally coroutine support. Since coroutines are rather contained it would be not to hard to make it conditionally. (Still that would need CI validation, which we don’t do when there is no explicit compiler flag.)

@Endill

I don’t see this a good precedent of providing a flag to disable a major language feature that has been shipping as stable for a while. We are receiving a constant stream of bugs for every major feature of C++20. On top of that, concepts do not have stable mangling in Itanium ABI. While it may not be an issue for you, it still could be for other major C++ deployments out there. If they come to us proposing -fno-concepts because of that, how are we going to respond after this precedent?

Concepts on the other hand would be a huge issue. Places where the Standard uses the constraints clause we prefer to use concepts over enable_if. In the format library I prefer to use the range based algorithms over the iterator pair based algorithms. The range based algorithms require concepts. So even when the Standard does not mandate the usage of concepts they are used quite often in libc++.

Making all code in libc++ depend on compiler feature-test macros would hugely increase the number of permutations to test. Counting all possible values for the feature-test macros; there are currently over a 100 compiler related feature-test macros. I think that is not doable to support all of these features conditionally in libc++. So I think it’s not reasonable to expect to use feature-test macros for all compiler features.

Note that does not mean I’m against conditionally enabling features in the compiler general. My biggest issue with this option was the timing and the precedence it sets to make features retroactively optional. It’s easier to avoid using a feature when it’s known to be optional, than to undo its usage at a later time.