c++/coroutines: fix actor cases not being added to the current switch [PR109867]

Tested on x86_64-pc-linux-gnu, no regression.

OK for trunk?

TIA, have a lovely day.
---------- >8 ----------
Previously, we were building and inserting case_labels manually, which
lead to them not being added into the currently running switch via
c_add_case_label.  This lead to false diagnostics that the user could
not act on.

The use of temp_override on current_function_decl is a temporary hack,
as we intend to factor out generating the outlined functions (including
the actor) to happen later in finish_function, when we can create a new
function using the standard routines for that, similar to
finish_function_contracts.

PR c++/109867 - -Wswitch-default reports missing default in coroutine

gcc/cp/ChangeLog:

	PR c++/109867
	* coroutines.cc (expand_one_await_expression): Replace uses of
	build_case_label with finish_case_label.
	(build_actor_fn): Ditto.  Also adjust current_function_decl.
	(create_anon_label_with_ctx): Remove now-unused function.

gcc/testsuite/ChangeLog:

	PR c++/109867
	* g++.dg/coroutines/torture/pr109867.C: New test.
---
 gcc/cp/coroutines.cc                          | 57 +++++--------------
 .../g++.dg/coroutines/torture/pr109867.C      | 23 ++++++++
 2 files changed, 38 insertions(+), 42 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/torture/pr109867.C

On 8/1/24 12:48 PM, Arsen Arsenović wrote:
> Tested on x86_64-pc-linux-gnu, no regression.
> 
> OK for trunk?
> 
> TIA, have a lovely day.
> ---------- >8 ----------
> Previously, we were building and inserting case_labels manually, which
> lead to them not being added into the currently running switch via

"led"

> c_add_case_label.  This lead to false diagnostics that the user could
> not act on.

The case changes are OK.

> The use of temp_override on current_function_decl is a temporary hack,

We can do better than this.

> as we intend to factor out generating the outlined functions (including
> the actor) to happen later in finish_function, when we can create a new
> function using the standard routines for that, similar to
> finish_function_contracts.

If you want to do that, sure, but it doesn't seem necessary; it's quite 
common to push into defining a function in the middle of defining 
another one.  In maybe_add_lambda_conv_op, for instance: basically just 
push_function_context before start_preparsed_function.

Jason

Jason Merrill <jason@redhat.com> writes:

> On 8/1/24 12:48 PM, Arsen Arsenović wrote:
>> Tested on x86_64-pc-linux-gnu, no regression.
>> OK for trunk?
>> TIA, have a lovely day.
>> ---------- >8 ----------
>> Previously, we were building and inserting case_labels manually, which
>> lead to them not being added into the currently running switch via
>
> "led"

Ah, whoops, thanks.  Will amend.

>> c_add_case_label.  This lead to false diagnostics that the user could
>> not act on.
>
> The case changes are OK.

Thanks.

>> The use of temp_override on current_function_decl is a temporary hack,
>
> We can do better than this.
>
>> as we intend to factor out generating the outlined functions (including
>> the actor) to happen later in finish_function, when we can create a new
>> function using the standard routines for that, similar to
>> finish_function_contracts.
>
> If you want to do that, sure, but it doesn't seem necessary; it's quite common
> to push into defining a function in the middle of defining another one.  In
> maybe_add_lambda_conv_op, for instance: basically just push_function_context
> before start_preparsed_function.

Okay, thanks, I wasn't aware of push_function_context - I have a WIP
that does that, I'll coordinate with Iain on what exactly to do, though
(as he was planning to rework parts of the code that the aforementioned
WIP also did), so a v2 might take us a bit longer.

Thanks for the review.

Have a lovely evening.

Jason Merrill <jason@redhat.com> writes:

> On 8/1/24 12:48 PM, Arsen Arsenović wrote:
>> Tested on x86_64-pc-linux-gnu, no regression.
>> OK for trunk?
>> TIA, have a lovely day.
>> ---------- >8 ----------
>> Previously, we were building and inserting case_labels manually, which
>> lead to them not being added into the currently running switch via
>
> "led"
>
>> c_add_case_label.  This lead to false diagnostics that the user could
>> not act on.
>
> The case changes are OK.

Applying the following now that we don't need the hack anymore.
Regstrapped on x86_64-pc-linux-gnu.

Thanks, have a lovely evening.
---------- >8 ----------
Previously, we were building and inserting case_labels manually, which
led to them not being added into the currently running switch via
c_add_case_label.  This led to false diagnostics that the user could not
act on.

	PR c++/109867

gcc/cp/ChangeLog:

	* coroutines.cc (expand_one_await_expression): Replace uses of
	build_case_label with finish_case_label.
	(build_actor_fn): Ditto.
	(create_anon_label_with_ctx): Remove now-unused function.

gcc/testsuite/ChangeLog:

	* g++.dg/coroutines/torture/pr109867.C: New test.

Reviewed-by: Iain Sandoe <iain@sandoe.co.uk>
---
 gcc/cp/coroutines.cc                          | 52 ++++---------------
 .../g++.dg/coroutines/torture/pr109867.C      | 23 ++++++++
 2 files changed, 34 insertions(+), 41 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/torture/pr109867.C

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index 31dc39afeee2..f243fe9adae2 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -1708,20 +1708,6 @@ coro_build_artificial_var (location_t loc, const char *name, tree type,
 				    type, ctx, init);
 }
 
-/* Helpers for label creation:
-   1. Create a named label in the specified context.  */
-
-static tree
-create_anon_label_with_ctx (location_t loc, tree ctx)
-{
-  tree lab = build_decl (loc, LABEL_DECL, NULL_TREE, void_type_node);
-
-  DECL_CONTEXT (lab) = ctx;
-  DECL_ARTIFICIAL (lab) = true;
-  DECL_IGNORED_P (lab) = true;
-  TREE_USED (lab) = true;
-  return lab;
-}
 
 /*  2. Create a named label in the specified context.  */
 
@@ -1935,22 +1921,16 @@ expand_one_await_expression (tree *stmt, tree *await_expr, void *d)
 				    data->coro_fp);
   r = cp_build_init_expr (cond, r);
   finish_switch_cond (r, sw);
-  r = build_case_label (integer_zero_node, NULL_TREE,
-			create_anon_label_with_ctx (loc, actor));
-  add_stmt (r); /* case 0: */
+  finish_case_label (loc, integer_zero_node, NULL_TREE); /*  case 0: */
   /* Implement the suspend, a scope exit without clean ups.  */
   r = build_call_expr_internal_loc (loc, IFN_CO_SUSPN, void_type_node, 1,
 				    is_cont ? cont : susp);
   r = coro_build_cvt_void_expr_stmt (r, loc);
   add_stmt (r); /*   goto ret;  */
-  r = build_case_label (integer_one_node, NULL_TREE,
-			create_anon_label_with_ctx (loc, actor));
-  add_stmt (r); /* case 1:  */
+  finish_case_label (loc, integer_one_node, NULL_TREE); /*  case 1:  */
   r = build1_loc (loc, GOTO_EXPR, void_type_node, resume_label);
   add_stmt (r); /*  goto resume;  */
-  r = build_case_label (NULL_TREE, NULL_TREE,
-			create_anon_label_with_ctx (loc, actor));
-  add_stmt (r); /* default:;  */
+  finish_case_label (loc, NULL_TREE, NULL_TREE); /* default:;  */
   r = build1_loc (loc, GOTO_EXPR, void_type_node, destroy_label);
   add_stmt (r); /* goto destroy;  */
 
@@ -2291,9 +2271,7 @@ build_actor_fn (location_t loc, tree coro_frame_type, tree actor, tree fnbody,
 
   tree destroy_dispatcher = begin_switch_stmt ();
   finish_switch_cond (rat, destroy_dispatcher);
-  tree ddeflab = build_case_label (NULL_TREE, NULL_TREE,
-				   create_anon_label_with_ctx (loc, actor));
-  add_stmt (ddeflab);
+  tree ddeflab = finish_case_label (loc, NULL_TREE, NULL_TREE);
   tree b = build_call_expr_loc (loc, builtin_decl_explicit (BUILT_IN_TRAP), 0);
   b = coro_build_cvt_void_expr_stmt (b, loc);
   add_stmt (b);
@@ -2304,18 +2282,15 @@ build_actor_fn (location_t loc, tree coro_frame_type, tree actor, tree fnbody,
      frame itself.  */
   tree del_promise_label
     = create_named_label_with_ctx (loc, "coro.delete.promise", actor);
-  b = build_case_label (build_int_cst (short_unsigned_type_node, 1), NULL_TREE,
-			create_anon_label_with_ctx (loc, actor));
-  add_stmt (b);
+  finish_case_label (loc, build_int_cst (short_unsigned_type_node, 1),
+		     NULL_TREE);
   add_stmt (build_stmt (loc, GOTO_EXPR, del_promise_label));
 
   short unsigned lab_num = 3;
   for (unsigned destr_pt = 0; destr_pt < body_count; destr_pt++)
     {
       tree l_num = build_int_cst (short_unsigned_type_node, lab_num);
-      b = build_case_label (l_num, NULL_TREE,
-			    create_anon_label_with_ctx (loc, actor));
-      add_stmt (b);
+      finish_case_label (loc, l_num, NULL_TREE);
       b = build_call_expr_internal_loc (loc, IFN_CO_ACTOR, void_type_node, 1,
 					l_num);
       b = coro_build_cvt_void_expr_stmt (b, loc);
@@ -2333,15 +2308,12 @@ build_actor_fn (location_t loc, tree coro_frame_type, tree actor, tree fnbody,
 
   tree dispatcher = begin_switch_stmt ();
   finish_switch_cond (rat, dispatcher);
-  b = build_case_label (build_int_cst (short_unsigned_type_node, 0), NULL_TREE,
-			create_anon_label_with_ctx (loc, actor));
-  add_stmt (b);
+  finish_case_label (loc, build_int_cst (short_unsigned_type_node, 0),
+		     NULL_TREE);
   b = build1 (GOTO_EXPR, void_type_node, actor_begin_label);
   add_stmt (b);
 
-  tree rdeflab = build_case_label (NULL_TREE, NULL_TREE,
-				   create_anon_label_with_ctx (loc, actor));
-  add_stmt (rdeflab);
+  tree rdeflab = finish_case_label (loc, NULL_TREE, NULL_TREE);
   b = build_call_expr_loc (loc, builtin_decl_explicit (BUILT_IN_TRAP), 0);
   b = coro_build_cvt_void_expr_stmt (b, loc);
   add_stmt (b);
@@ -2353,9 +2325,7 @@ build_actor_fn (location_t loc, tree coro_frame_type, tree actor, tree fnbody,
   for (unsigned resu_pt = 0; resu_pt < body_count; resu_pt++)
     {
       tree l_num = build_int_cst (short_unsigned_type_node, lab_num);
-      b = build_case_label (l_num, NULL_TREE,
-			    create_anon_label_with_ctx (loc, actor));
-      add_stmt (b);
+      finish_case_label (loc, l_num, NULL_TREE);
       b = build_call_expr_internal_loc (loc, IFN_CO_ACTOR, void_type_node, 1,
 					l_num);
       b = coro_build_cvt_void_expr_stmt (b, loc);
diff --git a/gcc/testsuite/g++.dg/coroutines/torture/pr109867.C b/gcc/testsuite/g++.dg/coroutines/torture/pr109867.C
new file mode 100644
index 000000000000..d4663771ea40
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/torture/pr109867.C
@@ -0,0 +1,23 @@
+// https://gcc.gnu.org/PR109867
+// { dg-additional-options "-Werror=switch-default" }
+#include <coroutine>
+
+struct task
+{
+    struct promise_type
+    {
+        std::suspend_always initial_suspend();
+        std::suspend_always final_suspend() noexcept;
+        void unhandled_exception();
+        task get_return_object();
+        void return_value(int);
+    };
+};
+
+int main()
+{
+    auto t = []() -> task
+    {
+        co_return 2;
+    }();
+}