[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

1129

Corotuines -> Coroutines

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

6

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

- Clang has an internal limit of 2GB of preprocessed source code per
   ``#pragma clang __debug sloc_usage`` can also be used to request this report.
 - Clang no longer permits the keyword 'bool' in a concept declaration as a
   concepts-ts compatibility extension.
 - Clang now diagnoses overflow undefined behavior in a constant expression while
   evaluating a compound assignment with remainder as operand.
 - Add ``-Wreturn-local-addr``, a GCC alias for ``-Wreturn-stack-address``.
 - Clang now suppresses ``-Wlogical-op-parentheses`` on ``(x && a || b)`` and ``(a || b && x)``
   only when ``x`` is a string literal.
+- 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
 -------------------------------------------------
 - It's now possible to set the crash diagnostics directory through
   the environment variable ``CLANG_CRASH_DIAGNOSTICS_DIR``.
   The ``-fcrash-diagnostics-dir`` flag takes precedence.
 - When using header modules, inclusion of a private header and violations of
   the `use-declaration rules

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def warn_non_aligned_allocation_function : Warning <
   "under -fcoro-aligned-allocation, the non-aligned allocation function "
   "for the promise type %0 has higher precedence than the global aligned "
   "allocation function">,
   InGroup<CoroNonAlignedAllocationFunction>;
 def err_conflicting_aligned_options : Error <
   "conflicting option '-fcoro-aligned-allocation' and '-fno-aligned-allocation'"
 >;
+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();
   }
+
+  // Coroutines 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,
                       IsAfterAmp);
 }
 
 /// 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 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;
+}