[C++20] [Coroutines] Disable to take the address of labels in coroutines

[ChuanqiXu]

Summary

Closing https://github.com/llvm/llvm-project/issues/56436

We can't support the GNU address of label extension in coroutines well in current architecture. Since the coroutines are going to split into pieces in the middle end so the address of labels are ambiguous that time.

To avoid any further misunderstanding, we try to emit an error here.

[ChuanqiXu]

Add ReleaseNotes.

[avogelsgesang]

looks good to me on a high-level, but I don't know clang well enough to confidently approve this change

clang/lib/Sema/SemaCoroutine.cpp

1107

Corotuines -> Coroutines

clang/test/SemaCXX/addr-label-in-coroutines.cpp

5

unused

[ChuanqiXu]

Address comments.

[ychen]

Since the coroutines are going to split into pieces in the middle end so the address of labels are ambiguous that time.

Do we split in the middle or copy the whole computed goto CFG? I'd imagine the branch on a dynamic block address is like n-way branch which is not feasible to split. Does/Would it fix the problem by letting the ramp/resume function each have its own dispatch table? I skimmed through the blockaddress handling during function cloning which looks insufficient to handle this issue: https://github.com/llvm/llvm-project/blob/e20d210eef92f3952de0e89ef2f01a146480a13b/llvm/lib/Transforms/Utils/CloneFunction.cpp#L177-L182, it says "It is only legal to clone a function if a block address within that function is never referenced outside of the function." . This is not true when the dispatch table is a global variable.

[ChuanqiXu]

In D131938#3727954, @ychen wrote:

Since the coroutines are going to split into pieces in the middle end so the address of labels are ambiguous that time.

Do we split in the middle or copy the whole computed goto CFG?

Yes.

I'd imagine the branch on a dynamic block address is like n-way branch which is not feasible to split. Does/Would it fix the problem by letting the ramp/resume function each have its own dispatch table? I skimmed through the blockaddress handling during function cloning which looks insufficient to handle this issue: https://github.com/llvm/llvm-project/blob/e20d210eef92f3952de0e89ef2f01a146480a13b/llvm/lib/Transforms/Utils/CloneFunction.cpp#L177-L182, it says "It is only legal to clone a function if a block address within that function is never referenced outside of the function." . This is not true when the dispatch table is a global variable.

The key problem here is that the compiler don't know the idea 'dispatch table'. The dispatch table is just a global variable. And coroutines should just leave the global variable there just like other global variables.

"It is only legal to clone a function if a block address within that function is never referenced outside of the function."

This also shows the problem. The coroutines are multiple functions indeed.

[avogelsgesang]

Personally, I am fine with this change as in review right now.

@ychen are you still planning to look further into this issue as part of your alternative patch https://reviews.llvm.org/D132084 ? Otherwise, I would propose that we merge @ChuanqiXu 's change for the time being. Later on, we can still re-enable this functionality, as soon as we figured out the LLVM coroutine semantics of this

[ChuanqiXu]

Oh, sorry, I forgot this. Given D132084 is not updated recently, I think we can merge this first since it is innocent. And it should be fine to remove this one after patches like D132084 works properly.

clang/docs/ReleaseNotes.rst

- ``-Wbitfield-constant-conversion`` now diagnoses implicit truncation when 1 is
   `Issue 53253 <https://github.com/llvm/llvm-project/issues/53253>`_.
 - ``-Wincompatible-function-pointer-types`` now defaults to an error in all C
   language modes. It may be downgraded to a warning with
   ``-Wno-error=incompatible-function-pointer-types`` or disabled entirely with
   ``-Wno-implicit-function-pointer-types``.
 - Clang will now print more information about failed static assertions. In
   particular, simple static assertion expressions are evaluated to their
   compile-time value and printed out if the assertion fails.
+- Clang will now reject the GNU extension address of label in coroutines explicitly.
+  This fixes `Issue 56436 <https://github.com/llvm/llvm-project/issues/56436>`_.
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 
 New Compiler Flags
 ------------------
 
 Deprecated Compiler Flags

clang/include/clang/Basic/DiagnosticSemaKinds.td

 >;
 def warn_always_inline_coroutine : Warning<
   "this coroutine may be split into pieces; not every piece is guaranteed to be inlined"
   >,
   InGroup<AlwaysInlineCoroutine>;
 def err_coroutine_unusable_new : Error<
   "'operator new' provided by %0 is not usable with the function signature of %1"
 >;
+def err_coro_invalid_addr_of_label : Error<
+  "the GNU address of label extension is not allowed in coroutines."
+>;
 } // end of coroutines issue category
 
 let CategoryName = "Documentation Issue" in {
 def warn_not_a_doxygen_trailing_member_comment : Warning<
   "not a Doxygen trailing comment">, InGroup<Documentation>, DefaultIgnore;
 } // end of documentation issue category
 
 let CategoryName = "Nullability Issue" in {

clang/include/clang/Sema/ScopeInfo.h

public:
   /// current function scope.  These diagnostics are vetted for reachability
   /// prior to being emitted.
   SmallVector<PossiblyUnreachableDiag, 4> PossiblyUnreachableDiags;
 
   /// A list of parameters which have the nonnull attribute and are
   /// modified in the function.
   llvm::SmallPtrSet<const ParmVarDecl *, 8> ModifiedNonNullParams;
 
+  /// The set of GNU address of label extension "&&label".
+  llvm::SmallVector<AddrLabelExpr *, 4> AddrLabels;
+
 public:
   /// Represents a simple identification of a weak object.
   ///
   /// Part of the implementation of -Wrepeated-use-of-weak.
   ///
   /// This is used to determine if two weak accesses refer to the same object.
   /// Here are some examples of how various accesses are "profiled":
   ///

clang/lib/Sema/ScopeInfo.cpp

void FunctionScopeInfo::Clear() {
   SwitchStack.clear();
   Returns.clear();
   ErrorTrap.reset();
   PossiblyUnreachableDiags.clear();
   WeakObjectUses.clear();
   ModifiedNonNullParams.clear();
   Blocks.clear();
   ByrefBlockVars.clear();
+  AddrLabels.clear();
 }
 
 static const NamedDecl *getBestPropertyDecl(const ObjCPropertyRefExpr *PropE) {
   if (PropE->isExplicitProperty())
     return PropE->getExplicitProperty();
 
   return PropE->getImplicitPropertyGetter();
 }

clang/lib/Sema/SemaCoroutine.cpp

void Sema::CheckCompletedCoroutineBody(FunctionDecl *FD, Stmt *&Body) {
   //   A coroutine shall not enclose a return statement ([stmt.return]).
   if (Fn->FirstReturnLoc.isValid()) {
     assert(Fn->FirstCoroutineStmtLoc.isValid() &&
                    "first coroutine location not set");
     Diag(Fn->FirstReturnLoc, diag::err_return_in_coroutine);
     Diag(Fn->FirstCoroutineStmtLoc, diag::note_declared_coroutine_here)
             << Fn->getFirstCoroutineStmtKeyword();
   }
+
+  // Corotuines will get splitted into pieces. The GNU address of label
+  // extension wouldn't be meaningful in coroutines.
+  for (AddrLabelExpr *ALE : Fn->AddrLabels)
+    Diag(ALE->getBeginLoc(), diag::err_coro_invalid_addr_of_label);
+
   CoroutineStmtBuilder Builder(*this, *FD, *Fn, Body);
   if (Builder.isInvalid() || !Builder.buildStatements())
     return FD->setInvalidDecl();
 
   // Build body for the coroutine wrapper statement.
   Body = CoroutineBodyStmt::Create(Context, Builder);
 }
 

clang/lib/Sema/SemaExpr.cpp

   return BuildUnaryOp(S, OpLoc, ConvertTokenKindToUnaryOpcode(Op), Input);
 }
 
 /// ActOnAddrLabel - Parse the GNU address of label extension: "&&foo".
 ExprResult Sema::ActOnAddrLabel(SourceLocation OpLoc, SourceLocation LabLoc,
                                 LabelDecl *TheDecl) {
   TheDecl->markUsed(Context);
   // Create the AST node.  The address of a label always has type 'void*'.
-  return new (Context) AddrLabelExpr(OpLoc, LabLoc, TheDecl,
-                                     Context.getPointerType(Context.VoidTy));
+  auto *Res = new (Context) AddrLabelExpr(
+      OpLoc, LabLoc, TheDecl, Context.getPointerType(Context.VoidTy));
+
+  if (getCurFunction())
+    getCurFunction()->AddrLabels.push_back(Res);
+
+  return Res;
 }
 
 void Sema::ActOnStartStmtExpr() {
   PushExpressionEvaluationContext(ExprEvalContexts.back().Context);
 }
 
 void Sema::ActOnStmtExprError() {
   // Note that function is also called by TreeTransform when leaving a

clang/test/SemaCXX/addr-label-in-coroutines.cpp

+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify %s
+
+#include "Inputs/std-coroutine.h"
+
+struct Allocator {};
+
+struct resumable {
+  struct promise_type {
+    resumable get_return_object() { return {}; }
+    auto initial_suspend() { return std::suspend_always(); }
+    auto final_suspend() noexcept { return std::suspend_always(); }
+    void unhandled_exception() {}
+    void return_void(){};
+  };
+};
+
+resumable f1(int &out, int *inst) {
+    static void* dispatch_table[] = {&&inc,      // expected-error {{the GNU address of label extension is not allowed in coroutines.}}
+                                     &&suspend,  // expected-error {{the GNU address of label extension is not allowed in coroutines.}}
+                                     &&stop};    // expected-error {{the GNU address of label extension is not allowed in coroutines.}}
+    #define DISPATCH() goto *dispatch_table[*inst++]
+inc:
+    out++;
+    DISPATCH();
+
+suspend:
+    co_await std::suspend_always{};
+    DISPATCH();
+
+stop:
+    co_return;
+}
+
+resumable f2(int &out, int *inst) {
+    void* dispatch_table[] = {nullptr, nullptr, nullptr};
+    dispatch_table[0] = &&inc;      // expected-error {{the GNU address of label extension is not allowed in coroutines.}}
+    dispatch_table[1] = &&suspend;  // expected-error {{the GNU address of label extension is not allowed in coroutines.}}
+    dispatch_table[2] = &&stop;     // expected-error {{the GNU address of label extension is not allowed in coroutines.}}
+    #define DISPATCH() goto *dispatch_table[*inst++]
+inc:
+    out++;
+    DISPATCH();
+
+suspend:
+    co_await std::suspend_always{};
+    DISPATCH();
+
+stop:
+    co_return;
+}
+
+resumable f3(int &out, int *inst) {
+    void* dispatch_table[] = {nullptr, nullptr, nullptr};
+    [&]() -> resumable {
+        dispatch_table[0] = &&inc;      // expected-error {{the GNU address of label extension is not allowed in coroutines.}}
+        dispatch_table[1] = &&suspend;  // expected-error {{the GNU address of label extension is not allowed in coroutines.}}
+        dispatch_table[2] = &&stop;     // expected-error {{the GNU address of label extension is not allowed in coroutines.}}
+        #define DISPATCH() goto *dispatch_table[*inst++]
+    inc:
+        out++;
+        DISPATCH();
+
+    suspend:
+        co_await std::suspend_always{};
+        DISPATCH();
+
+    stop:
+        co_return;
+    }();
+
+    co_return;
+}