[coroutines] Support "coroutines" feature in module map requires clause

[EricWF]

Summary

In order for libc++ to add <experimental/coroutine> to its module map, there has to be a feature that can be used to detect if coroutines support is enabled in Clang.

[EricWF]

• Alphabetize newly added switch case.

[EricWF]

test/Modules/requires-coroutines.mm

1

Should this test be called requires-coroutines.cpp or is using Obj-C++ a correct thing to do here?

[rsmith]

Do we need to conditionalize this part of libc++? Nothing in the <coroutines> header appears to need compiler support.

[rsmith]

In D33538#765045, @rsmith wrote:

Do we need to conditionalize this part of libc++? Nothing in the <coroutines> header appears to need compiler support.

Oh wait, I see what's going on. You're not testing for whether coroutines is enabled, you're testing for whether the __builtin_coro_* builtins exist. Are we sufficiently confident that those aren't going to change that we're prepared to make libc++ rely on this? (If we change the signature of those builtins in the future, then new versions of clang would stop being able to build old versions of the libc++ module.)

If we're not confident of that, how about calling the new feature something ugly like experimental_coroutines_builtins_20170525 or similar? That way, future versions of Clang can stop advertising that feature if we change the design of the builtins, and we can add a feature 'coroutines' later in a non-disruptive way if/when we decide we're happy with them as-is.

test/Modules/requires-coroutines.mm

1

You can use a .cpp extension and the Modules TS import keyword if you prefer (add -fmodules-ts to the clang arguments), or use a .cpp extension and #pragma clang module import if you don't want to depend on a second TS in this test.

[rsmith]

In D33538#765062, @rsmith wrote:

In D33538#765045, @rsmith wrote:

Do we need to conditionalize this part of libc++? Nothing in the <coroutines> header appears to need compiler support.

Oh wait, I see what's going on. You're not testing for whether coroutines is enabled, you're testing for whether the __builtin_coro_* builtins exist. Are we sufficiently confident that those aren't going to change that we're prepared to make libc++ rely on this? (If we change the signature of those builtins in the future, then new versions of clang would stop being able to build old versions of the libc++ module.)

On reflection, I think this is fine. If the signatures of the builtins change, and the user builds with -fmodules -fcoroutines-ts and libc++, and there's version skew between libc++ and clang, they'll get a compile error. That's mostly the expected outcome; the issue is that we'd produce this compile error *even if* they never #include <experimental/coroutines>, because building the complete libc++ module would fail in that situation.

If we're worried about that, we could split the coroutines header out of the main libc++ module into its own top-level module, but I don't think we need to worry too much about rejecting code that uses -fcoroutines-ts but never actually uses a coroutine.

[EricWF]

In D33538#765045, @rsmith wrote:

Do we need to conditionalize this part of libc++? Nothing in the <coroutines> header appears to need compiler support.

That's correct. I was mistaken as to why this was needed. I mistook a bug in libc++ for the reason this was needed.
So I have no need for this patch anymore.

Do you still want to land this for the reasons you mentioned?

[EricWF]

Also see r303936, which re-adds <experimental/coroutine> to the module map and fixes the bug.

[rsmith]

In D33538#765146, @EricWF wrote:

In D33538#765045, @rsmith wrote:

Do we need to conditionalize this part of libc++? Nothing in the <coroutines> header appears to need compiler support.

That's correct. I was mistaken as to why this was needed. I mistook a bug in libc++ for the reason this was needed.
So I have no need for this patch anymore.

Do you still want to land this for the reasons you mentioned?

r303936 will break the libc++ modules build if used with an older version of clang that doesn't have the coroutines builtins. If you're OK with that, then we don't need this. But if you intend to support older versions of Clang, then I think you need either this or a different approach (such as splitting out a separate top-level module for the coroutines header) to avoid that problem.

[EricWF]

In D33538#765225, @rsmith wrote:

In D33538#765146, @EricWF wrote:

In D33538#765045, @rsmith wrote:

Do we need to conditionalize this part of libc++? Nothing in the <coroutines> header appears to need compiler support.

That's correct. I was mistaken as to why this was needed. I mistook a bug in libc++ for the reason this was needed.
So I have no need for this patch anymore.

Do you still want to land this for the reasons you mentioned?

r303936 will break the libc++ modules build if used with an older version of clang that doesn't have the coroutines builtins. If you're OK with that, then we don't need this. But if you intend to support older versions of Clang, then I think you need either this or a different approach (such as splitting out a separate top-level module for the coroutines header) to avoid that problem.

I'll have to investigate this further. I had assumed the builtins were added at the same time as the __cpp_coroutines macro, but if that's not the case libc++ could still guard the header correctly using either __has_builtin or the newly updated value of __cpp_coroutines; but a complete library solution seems possible.

For other users of Clang module maps, though, I see the convince of being able to do this within the module map.

[EricWF]

Even after fixing the libc++ guards, the header still emits a #warning when it's processed when coroutines are unavailable.

It seems like a useful feature test to have available. I'll commit shortly.

[EricWF]

This patch was initially reverted due to a failing test it caused elsewhere; which has been address in this version.

docs/Modules.rst

 The following features are defined:
 
 altivec
   The target supports AltiVec.
 
 blocks
   The "blocks" language feature is available.
 
+coroutines
+  Support for the coroutines TS is available.
+
 cplusplus
   C++ support is available.
 
 cplusplus11
   C++11 support is available.
 
 freestanding
   A freestanding environment is available.

lib/Basic/Module.cpp

 
 /// \brief Determine whether a translation unit built using the current
 /// language options has the given feature.
 static bool hasFeature(StringRef Feature, const LangOptions &LangOpts,
                        const TargetInfo &Target) {
   bool HasFeature = llvm::StringSwitch<bool>(Feature)
                         .Case("altivec", LangOpts.AltiVec)
                         .Case("blocks", LangOpts.Blocks)
+                        .Case("coroutines", LangOpts.CoroutinesTS)
                         .Case("cplusplus", LangOpts.CPlusPlus)
                         .Case("cplusplus11", LangOpts.CPlusPlus11)
                         .Case("freestanding", LangOpts.Freestanding)
                         .Case("gnuinlineasm", LangOpts.GNUAsm)
                         .Case("objc", LangOpts.ObjC1)
                         .Case("objc_arc", LangOpts.ObjCAutoRefCount)
                         .Case("opencl", LangOpts.OpenCL)
                         .Case("tls", Target.isTLSSupported())

test/Modules/Inputs/DependsOnModule.framework/Headers/coroutines.h

+#ifndef __cpp_coroutines
+#error coroutines must be enabled
+#endif

test/Modules/Inputs/DependsOnModule.framework/Headers/not_coroutines.h

+#ifdef __cpp_coroutines
+#error coroutines must NOT be enabled
+#endif

test/Modules/Inputs/DependsOnModule.framework/module.map

explicit module NotObjC {
     header "not_objc.h"
   }
   explicit module CustomReq1 {
     requires custom_req1
   }
   explicit module CustomReq2 {
     requires custom_req2
   }
-
+  explicit module Coroutines {
+    requires coroutines
+    header "coroutines.h"
+  }
+  explicit module NotCoroutines {
+    requires !coroutines
+    header "not_coroutines.h"
+  }
   explicit framework module SubFramework {
     umbrella header "SubFramework.h"
 
     module * {
       export *
     }
   }
 }

test/Modules/requires-coroutines.mm

+// RUN: rm -rf %t
+// RUN: %clang_cc1 -Wauto-import -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -F %S/Inputs %s -verify
+// RUN: %clang_cc1 -Wauto-import -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -F %S/Inputs %s -verify -fcoroutines-ts -DCOROUTINES
+
+
+#ifdef COROUTINES
+@import DependsOnModule.Coroutines;
+@import DependsOnModule.NotCoroutines; // expected-error {{module 'DependsOnModule.NotCoroutines' is incompatible with feature 'coroutines'}}
+#else
+@import DependsOnModule.NotCoroutines;
+@import DependsOnModule.Coroutines; // expected-error {{module 'DependsOnModule.Coroutines' requires feature 'coroutines'}}
+#endif