[clang] e39ba04 - [C++20] [Coroutines] Warning for always_inline coroutine

[Chuanqi Xu via cfe-commits]

Author: Chuanqi Xu
Date: 2022-02-08T11:43:42+08:00
New Revision: e39ba0461757339f7172f6bc3882f41116bb7c13

URL: https://github.com/llvm/llvm-project/commit/e39ba0461757339f7172f6bc3882f41116bb7c13
DIFF: https://github.com/llvm/llvm-project/commit/e39ba0461757339f7172f6bc3882f41116bb7c13.diff

LOG: [C++20] [Coroutines] Warning for always_inline coroutine

See the discussion in https://reviews.llvm.org/D100282. The coroutine
marked always inline might not be inlined properly in current compiler
support. Since the coroutine would be splitted into pieces. And the call
to resume() and destroy() functions might be indirect call. Also the
ramp function wouldn't get inlined under O0 due to pipeline ordering
problems. It might be different to what users expects to. Emit a warning
to tell it.

This is what GCC does too: https://godbolt.org/z/7eajb1Gf8

Reviewed By: Quuxplusone

Differential Revision: https://reviews.llvm.org/D115867

Added: 
    

Modified: 
    clang/include/clang/Basic/AttrDocs.td
    clang/include/clang/Basic/DiagnosticGroups.td
    clang/include/clang/Basic/DiagnosticSemaKinds.td
    clang/lib/Sema/SemaCoroutine.cpp
    clang/test/SemaCXX/coroutines.cpp

Removed: 
    


################################################################################
diff  --git a/clang/include/clang/Basic/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td
index e12a0b259cc2e..1682a71eb992e 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -6205,6 +6205,10 @@ optimization level.
 
 Does not guarantee that inline substitution actually occurs.
 
+<ins>Note: applying this attribute to a coroutine at the `-O0` optimization level
+has no effect; other optimization levels may only partially inline and result in a
+diagnostic.</ins>
+
 See also `the Microsoft Docs on Inline Functions`_, `the GCC Common Function
 Attribute docs`_, and `the GCC Inline docs`_.
 

diff  --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 608e16147b1cd..3f14fa32597d9 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -58,7 +58,9 @@ def DeprecatedExperimentalCoroutine :
   DiagGroup<"deprecated-experimental-coroutine">;
 def DeprecatedCoroutine :
   DiagGroup<"deprecated-coroutine", [DeprecatedExperimentalCoroutine]>;
-def Coroutine : DiagGroup<"coroutine", [CoroutineMissingUnhandledException, DeprecatedCoroutine]>;
+def AlwaysInlineCoroutine :
+  DiagGroup<"always-inline-coroutine">;
+def Coroutine : DiagGroup<"coroutine", [CoroutineMissingUnhandledException, DeprecatedCoroutine, AlwaysInlineCoroutine]>;
 def ObjCBoolConstantConversion : DiagGroup<"objc-bool-constant-conversion">;
 def ConstantConversion : DiagGroup<"constant-conversion",
                                    [BitFieldConstantConversion,

diff  --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index f9bfd343ac8d1..08cd8c8991ff9 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -11122,6 +11122,10 @@ def err_coroutine_promise_final_suspend_requires_nothrow : Error<
 def note_coroutine_function_declare_noexcept : Note<
   "must be declared with 'noexcept'"
 >;
+def warn_always_inline_coroutine : Warning<
+  "this coroutine may be split into pieces; not every piece is guaranteed to be inlined"
+  >,
+  InGroup<AlwaysInlineCoroutine>;
 } // end of coroutines issue category
 
 let CategoryName = "Documentation Issue" in {

diff  --git a/clang/lib/Sema/SemaCoroutine.cpp b/clang/lib/Sema/SemaCoroutine.cpp
index cd3ae62ebbe26..aae90c403f0f2 100644
--- a/clang/lib/Sema/SemaCoroutine.cpp
+++ b/clang/lib/Sema/SemaCoroutine.cpp
@@ -1081,6 +1081,14 @@ void Sema::CheckCompletedCoroutineBody(FunctionDecl *FD, Stmt *&Body) {
     return;
   }
 
+  // The always_inline attribute doesn't reliably apply to a coroutine,
+  // because the coroutine will be split into pieces and some pieces
+  // might be called indirectly, as in a virtual call. Even the ramp
+  // function cannot be inlined at -O0, due to pipeline ordering
+  // problems (see https://llvm.org/PR53413). Tell the user about it.
+  if (FD->hasAttr<AlwaysInlineAttr>())
+    Diag(FD->getLocation(), diag::warn_always_inline_coroutine);
+
   // [stmt.return.coroutine]p1:
   //   A coroutine shall not enclose a return statement ([stmt.return]).
   if (Fn->FirstReturnLoc.isValid()) {

diff  --git a/clang/test/SemaCXX/coroutines.cpp b/clang/test/SemaCXX/coroutines.cpp
index b461c881355b5..c81385bcc0595 100644
--- a/clang/test/SemaCXX/coroutines.cpp
+++ b/clang/test/SemaCXX/coroutines.cpp
@@ -1460,3 +1460,13 @@ void test_missing_awaitable_members() {
   co_await missing_await_suspend{}; // expected-error {{no member named 'await_suspend' in 'missing_await_suspend'}}
   co_await missing_await_resume{}; // expected-error {{no member named 'await_resume' in 'missing_await_resume'}}
 }
+
+__attribute__((__always_inline__))
+void warn_always_inline() { // expected-warning {{this coroutine may be split into pieces; not every piece is guaranteed to be inlined}}
+  co_await suspend_always{};
+}
+
+[[gnu::always_inline]]
+void warn_gnu_always_inline() { // expected-warning {{this coroutine may be split into pieces; not every piece is guaranteed to be inlined}}
+  co_await suspend_always{};
+}