public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] c++, coroutines: Make sure our temporaries are in a bind expr [PR105287]
@ 2022-04-18 14:02 Iain Sandoe
  2022-04-20  2:14 ` Jason Merrill
  0 siblings, 1 reply; 4+ messages in thread
From: Iain Sandoe @ 2022-04-18 14:02 UTC (permalink / raw)
  To: gcc-patches

There are a few cases where we can generate a temporary that does not need
to be added to the coroutine frame (i.e. these are genuinely ephemeral).  The
intent was that unnamed temporaries should not be 'promoted' to coroutine
frame entries.  However there was a thinko and these were not actually ever
added to the bind expressions being generated for the expanded awaits.  This
meant that they were showing in the global namspace, leading to an empty
DECL_CONTEXT and the ICE reported.

tested on x86_64-darwin, OK for mainline? / Backports? (when?)
thanks,
Iain

Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>

	PR c++/105287

gcc/cp/ChangeLog:

	* coroutines.cc (maybe_promote_temps): Ensure generated temporaries
	are added to the bind expr.
	(add_var_to_bind): Fix local var naming to use portable punctuation.
	(register_local_var_uses): Do not add synthetic names to unnamed
	temporaries.

gcc/testsuite/ChangeLog:

	* g++.dg/coroutines/pr105287.C: New test.
---
 gcc/cp/coroutines.cc                       | 17 ++++----
 gcc/testsuite/g++.dg/coroutines/pr105287.C | 48 ++++++++++++++++++++++
 2 files changed, 56 insertions(+), 9 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/pr105287.C

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index a9ce6e050dd..dcc2284171b 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -3103,7 +3103,7 @@ maybe_promote_temps (tree *stmt, void *d)
 	 If the initializer is a conditional expression, we need to collect
 	 and declare any promoted variables nested within it.  DTORs for such
 	 variables must be run conditionally too.  */
-      if (t->var && DECL_NAME (t->var))
+      if (t->var)
 	{
 	  tree var = t->var;
 	  DECL_CHAIN (var) = vlist;
@@ -3304,7 +3304,7 @@ add_var_to_bind (tree& bind, tree var_type,
   tree b_vars = BIND_EXPR_VARS (bind);
   /* Build a variable to hold the condition, this will be included in the
      frame as a local var.  */
-  char *nam = xasprintf ("%s.%d", nam_root, nam_vers);
+  char *nam = xasprintf ("`__%s_%d", nam_root, nam_vers);
   tree newvar = build_lang_decl (VAR_DECL, get_identifier (nam), var_type);
   free (nam);
   DECL_CHAIN (newvar) = b_vars;
@@ -3949,7 +3949,7 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d)
 	     scopes with identically named locals and still be able to
 	     identify them in the coroutine frame.  */
 	  tree lvname = DECL_NAME (lvar);
-	  char *buf;
+	  char *buf = NULL;
 
 	  /* The outermost bind scope contains the artificial variables that
 	     we inject to implement the coro state machine.  We want to be able
@@ -3959,14 +3959,13 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d)
 	  else if (lvname != NULL_TREE)
 	    buf = xasprintf ("%s_%u_%u", IDENTIFIER_POINTER (lvname),
 			     lvd->nest_depth, lvd->bind_indx);
-	  else
-	    buf = xasprintf ("_D%u_%u_%u", DECL_UID (lvar), lvd->nest_depth,
-			     lvd->bind_indx);
 	  /* TODO: Figure out if we should build a local type that has any
 	     excess alignment or size from the original decl.  */
-	  local_var.field_id
-	    = coro_make_frame_entry (lvd->field_list, buf, lvtype, lvd->loc);
-	  free (buf);
+	  if (buf) {
+	    local_var.field_id
+	      = coro_make_frame_entry (lvd->field_list, buf, lvtype, lvd->loc);
+	    free (buf);
+	  }
 	  /* We don't walk any of the local var sub-trees, they won't contain
 	     any bind exprs.  */
 	}
diff --git a/gcc/testsuite/g++.dg/coroutines/pr105287.C b/gcc/testsuite/g++.dg/coroutines/pr105287.C
new file mode 100644
index 00000000000..9790945287d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/pr105287.C
@@ -0,0 +1,48 @@
+// { dg-additional-options "-fanalyzer" }
+// { dg-excess-errors "lots of analyzer output, but no ICE" }
+namespace std {
+template <typename _Result> struct coroutine_traits : _Result {};
+template <typename = void> struct coroutine_handle {
+  operator coroutine_handle<>();
+};
+}
+struct coro1 {
+  using handle_type = std::coroutine_handle<>;
+  coro1(handle_type);
+  struct suspend_always_prt {
+    bool await_ready() noexcept;
+    void await_suspend(handle_type) noexcept;
+    void await_resume() noexcept;
+  };
+  struct promise_type {
+    std::coroutine_handle<> ch_;
+    auto get_return_object() { return ch_; }
+    auto initial_suspend() { return suspend_always_prt{}; }
+    auto final_suspend() noexcept { return suspend_always_prt{}; }
+    void unhandled_exception();
+  };
+};
+struct BoolAwaiter {
+  BoolAwaiter(bool);
+  bool await_ready();
+  void await_suspend(std::coroutine_handle<>);
+  bool await_resume();
+};
+struct IntAwaiter {
+  IntAwaiter(int);
+  bool await_ready();
+  void await_suspend(std::coroutine_handle<>);
+  int await_resume();
+};
+coro1 my_coro() {
+ int a = 1;
+ if (a == 0) {
+   int b = 5;
+   
+ }
+ {
+   int c = 10;
+ }
+ co_await BoolAwaiter(true) && co_await IntAwaiter(a); 
+ 
+ }
-- 
2.24.3 (Apple Git-128)


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] c++, coroutines: Make sure our temporaries are in a bind expr [PR105287]
  2022-04-18 14:02 [PATCH] c++, coroutines: Make sure our temporaries are in a bind expr [PR105287] Iain Sandoe
@ 2022-04-20  2:14 ` Jason Merrill
  2022-04-28  8:28   ` Iain Sandoe
  0 siblings, 1 reply; 4+ messages in thread
From: Jason Merrill @ 2022-04-20  2:14 UTC (permalink / raw)
  To: iain, gcc-patches

On 4/18/22 10:02, Iain Sandoe wrote:
> There are a few cases where we can generate a temporary that does not need
> to be added to the coroutine frame (i.e. these are genuinely ephemeral).  The
> intent was that unnamed temporaries should not be 'promoted' to coroutine
> frame entries.  However there was a thinko and these were not actually ever
> added to the bind expressions being generated for the expanded awaits.  This
> meant that they were showing in the global namspace, leading to an empty
> DECL_CONTEXT and the ICE reported.
> 
> tested on x86_64-darwin, OK for mainline? / Backports? (when?)
> thanks,
> Iain
> 
> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
> 
> 	PR c++/105287
> 
> gcc/cp/ChangeLog:
> 
> 	* coroutines.cc (maybe_promote_temps): Ensure generated temporaries
> 	are added to the bind expr.
> 	(add_var_to_bind): Fix local var naming to use portable punctuation.
> 	(register_local_var_uses): Do not add synthetic names to unnamed
> 	temporaries.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/coroutines/pr105287.C: New test.
> ---
>   gcc/cp/coroutines.cc                       | 17 ++++----
>   gcc/testsuite/g++.dg/coroutines/pr105287.C | 48 ++++++++++++++++++++++
>   2 files changed, 56 insertions(+), 9 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/coroutines/pr105287.C
> 
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index a9ce6e050dd..dcc2284171b 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -3103,7 +3103,7 @@ maybe_promote_temps (tree *stmt, void *d)
>   	 If the initializer is a conditional expression, we need to collect
>   	 and declare any promoted variables nested within it.  DTORs for such
>   	 variables must be run conditionally too.  */
> -      if (t->var && DECL_NAME (t->var))
> +      if (t->var)
>   	{
>   	  tree var = t->var;
>   	  DECL_CHAIN (var) = vlist;
> @@ -3304,7 +3304,7 @@ add_var_to_bind (tree& bind, tree var_type,
>     tree b_vars = BIND_EXPR_VARS (bind);
>     /* Build a variable to hold the condition, this will be included in the
>        frame as a local var.  */
> -  char *nam = xasprintf ("%s.%d", nam_root, nam_vers);
> +  char *nam = xasprintf ("`__%s_%d", nam_root, nam_vers);

` is portable?

>     tree newvar = build_lang_decl (VAR_DECL, get_identifier (nam), var_type);
>     free (nam);
>     DECL_CHAIN (newvar) = b_vars;
> @@ -3949,7 +3949,7 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d)
>   	     scopes with identically named locals and still be able to
>   	     identify them in the coroutine frame.  */
>   	  tree lvname = DECL_NAME (lvar);
> -	  char *buf;
> +	  char *buf = NULL;
>   
>   	  /* The outermost bind scope contains the artificial variables that
>   	     we inject to implement the coro state machine.  We want to be able
> @@ -3959,14 +3959,13 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d)
>   	  else if (lvname != NULL_TREE)
>   	    buf = xasprintf ("%s_%u_%u", IDENTIFIER_POINTER (lvname),
>   			     lvd->nest_depth, lvd->bind_indx);
> -	  else
> -	    buf = xasprintf ("_D%u_%u_%u", DECL_UID (lvar), lvd->nest_depth,
> -			     lvd->bind_indx);
>   	  /* TODO: Figure out if we should build a local type that has any
>   	     excess alignment or size from the original decl.  */
> -	  local_var.field_id
> -	    = coro_make_frame_entry (lvd->field_list, buf, lvtype, lvd->loc);
> -	  free (buf);
> +	  if (buf) {

Brace should be on the next line.

> +	    local_var.field_id
> +	      = coro_make_frame_entry (lvd->field_list, buf, lvtype, lvd->loc);
> +	    free (buf);
> +	  }
>   	  /* We don't walk any of the local var sub-trees, they won't contain
>   	     any bind exprs.  */
>   	}
> diff --git a/gcc/testsuite/g++.dg/coroutines/pr105287.C b/gcc/testsuite/g++.dg/coroutines/pr105287.C
> new file mode 100644
> index 00000000000..9790945287d
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr105287.C
> @@ -0,0 +1,48 @@
> +// { dg-additional-options "-fanalyzer" }
> +// { dg-excess-errors "lots of analyzer output, but no ICE" }
> +namespace std {
> +template <typename _Result> struct coroutine_traits : _Result {};
> +template <typename = void> struct coroutine_handle {
> +  operator coroutine_handle<>();
> +};
> +}
> +struct coro1 {
> +  using handle_type = std::coroutine_handle<>;
> +  coro1(handle_type);
> +  struct suspend_always_prt {
> +    bool await_ready() noexcept;
> +    void await_suspend(handle_type) noexcept;
> +    void await_resume() noexcept;
> +  };
> +  struct promise_type {
> +    std::coroutine_handle<> ch_;
> +    auto get_return_object() { return ch_; }
> +    auto initial_suspend() { return suspend_always_prt{}; }
> +    auto final_suspend() noexcept { return suspend_always_prt{}; }
> +    void unhandled_exception();
> +  };
> +};
> +struct BoolAwaiter {
> +  BoolAwaiter(bool);
> +  bool await_ready();
> +  void await_suspend(std::coroutine_handle<>);
> +  bool await_resume();
> +};
> +struct IntAwaiter {
> +  IntAwaiter(int);
> +  bool await_ready();
> +  void await_suspend(std::coroutine_handle<>);
> +  int await_resume();
> +};
> +coro1 my_coro() {
> + int a = 1;
> + if (a == 0) {
> +   int b = 5;
> +
> + }
> + {
> +   int c = 10;
> + }
> + co_await BoolAwaiter(true) && co_await IntAwaiter(a);
> +
> + }


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] c++, coroutines: Make sure our temporaries are in a bind expr [PR105287]
  2022-04-20  2:14 ` Jason Merrill
@ 2022-04-28  8:28   ` Iain Sandoe
  2022-04-28 11:44     ` Jason Merrill
  0 siblings, 1 reply; 4+ messages in thread
From: Iain Sandoe @ 2022-04-28  8:28 UTC (permalink / raw)
  To: Jason Merrill; +Cc: GCC Patches

Hi Jason,

> On 20 Apr 2022, at 03:14, Jason Merrill <jason@redhat.com> wrote:
> 
> On 4/18/22 10:02, Iain Sandoe wrote:
>> There are a few cases where we can generate a temporary that does not need
>> to be added to the coroutine frame (i.e. these are genuinely ephemeral).  The
>> intent was that unnamed temporaries should not be 'promoted' to coroutine
>> frame entries.  However there was a thinko and these were not actually ever
>> added to the bind expressions being generated for the expanded awaits.  This
>> meant that they were showing in the global namspace, leading to an empty
>> DECL_CONTEXT and the ICE reported.
>> tested on x86_64-darwin, OK for mainline? / Backports? (when?)
>> thanks,
>> Iain
>> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
>> 	PR c++/105287
>> gcc/cp/ChangeLog:
>> 	* coroutines.cc (maybe_promote_temps): Ensure generated temporaries
>> 	are added to the bind expr.
>> 	(add_var_to_bind): Fix local var naming to use portable punctuation.
>> 	(register_local_var_uses): Do not add synthetic names to unnamed
>> 	temporaries.
>> gcc/testsuite/ChangeLog:
>> 	* g++.dg/coroutines/pr105287.C: New test.
>> ---
>>  gcc/cp/coroutines.cc                       | 17 ++++----
>>  gcc/testsuite/g++.dg/coroutines/pr105287.C | 48 ++++++++++++++++++++++
>>  2 files changed, 56 insertions(+), 9 deletions(-)
>>  create mode 100644 gcc/testsuite/g++.dg/coroutines/pr105287.C
>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
>> index a9ce6e050dd..dcc2284171b 100644
>> --- a/gcc/cp/coroutines.cc
>> +++ b/gcc/cp/coroutines.cc
>> @@ -3103,7 +3103,7 @@ maybe_promote_temps (tree *stmt, void *d)
>>  	 If the initializer is a conditional expression, we need to collect
>>  	 and declare any promoted variables nested within it.  DTORs for such
>>  	 variables must be run conditionally too.  */
>> -      if (t->var && DECL_NAME (t->var))
>> +      if (t->var)
>>  	{
>>  	  tree var = t->var;
>>  	  DECL_CHAIN (var) = vlist;
>> @@ -3304,7 +3304,7 @@ add_var_to_bind (tree& bind, tree var_type,
>>    tree b_vars = BIND_EXPR_VARS (bind);
>>    /* Build a variable to hold the condition, this will be included in the
>>       frame as a local var.  */
>> -  char *nam = xasprintf ("%s.%d", nam_root, nam_vers);
>> +  char *nam = xasprintf ("`__%s_%d", nam_root, nam_vers);
> 
> ` is portable?

nope, it’s a typo - apparently accepted by GAS and Darwin’s assembler tho… 
thanks for catching that.

>>    tree newvar = build_lang_decl (VAR_DECL, get_identifier (nam), var_type);
>>    free (nam);
>>    DECL_CHAIN (newvar) = b_vars;
>> @@ -3949,7 +3949,7 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d)
>>  	     scopes with identically named locals and still be able to
>>  	     identify them in the coroutine frame.  */
>>  	  tree lvname = DECL_NAME (lvar);
>> -	  char *buf;
>> +	  char *buf = NULL;
>>    	  /* The outermost bind scope contains the artificial variables that
>>  	     we inject to implement the coro state machine.  We want to be able
>> @@ -3959,14 +3959,13 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d)
>>  	  else if (lvname != NULL_TREE)
>>  	    buf = xasprintf ("%s_%u_%u", IDENTIFIER_POINTER (lvname),
>>  			     lvd->nest_depth, lvd->bind_indx);
>> -	  else
>> -	    buf = xasprintf ("_D%u_%u_%u", DECL_UID (lvar), lvd->nest_depth,
>> -			     lvd->bind_indx);
>>  	  /* TODO: Figure out if we should build a local type that has any
>>  	     excess alignment or size from the original decl.  */
>> -	  local_var.field_id
>> -	    = coro_make_frame_entry (lvd->field_list, buf, lvtype, lvd->loc);
>> -	  free (buf);
>> +	  if (buf) {
> 
> Brace should be on the next line.

that was careless, too much rushing, fixed as below,
OK now?
thanks
Iain

----- 

[PATCH] c++, coroutines: Make sure our temporaries are in a bind expr
 [PR105287]

There are a few cases where we can generate a temporary that does not need
to be added to the coroutine frame (i.e. these are genuinely ephemeral).  The
intent was that unnamed temporaries should not be 'promoted' to coroutine
frame entries.  However there was a thinko and these were not actually ever
added to the bind expressions being generated for the expanded awaits.  This
meant that they were showing in the global namspace, leading to an empty
DECL_CONTEXT and the ICE reported.

Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>

	PR c++/105287

gcc/cp/ChangeLog:

	* coroutines.cc (maybe_promote_temps): Ensure generated temporaries
	are added to the bind expr.
	(add_var_to_bind): Fix local var naming to use portable punctuation.
	(register_local_var_uses): Do not add synthetic names to unnamed
	temporaries.

gcc/testsuite/ChangeLog:

	* g++.dg/coroutines/pr105287.C: New test.
---
 gcc/cp/coroutines.cc                       | 18 ++++----
 gcc/testsuite/g++.dg/coroutines/pr105287.C | 48 ++++++++++++++++++++++
 2 files changed, 57 insertions(+), 9 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/pr105287.C

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index a9ce6e050dd..7e9ff69ae35 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -3103,7 +3103,7 @@ maybe_promote_temps (tree *stmt, void *d)
 	 If the initializer is a conditional expression, we need to collect
 	 and declare any promoted variables nested within it.  DTORs for such
 	 variables must be run conditionally too.  */
-      if (t->var && DECL_NAME (t->var))
+      if (t->var)
 	{
 	  tree var = t->var;
 	  DECL_CHAIN (var) = vlist;
@@ -3304,7 +3304,7 @@ add_var_to_bind (tree& bind, tree var_type,
   tree b_vars = BIND_EXPR_VARS (bind);
   /* Build a variable to hold the condition, this will be included in the
      frame as a local var.  */
-  char *nam = xasprintf ("%s.%d", nam_root, nam_vers);
+  char *nam = xasprintf ("__%s_%d", nam_root, nam_vers);
   tree newvar = build_lang_decl (VAR_DECL, get_identifier (nam), var_type);
   free (nam);
   DECL_CHAIN (newvar) = b_vars;
@@ -3949,7 +3949,7 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d)
 	     scopes with identically named locals and still be able to
 	     identify them in the coroutine frame.  */
 	  tree lvname = DECL_NAME (lvar);
-	  char *buf;
+	  char *buf = NULL;
 
 	  /* The outermost bind scope contains the artificial variables that
 	     we inject to implement the coro state machine.  We want to be able
@@ -3959,14 +3959,14 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d)
 	  else if (lvname != NULL_TREE)
 	    buf = xasprintf ("%s_%u_%u", IDENTIFIER_POINTER (lvname),
 			     lvd->nest_depth, lvd->bind_indx);
-	  else
-	    buf = xasprintf ("_D%u_%u_%u", DECL_UID (lvar), lvd->nest_depth,
-			     lvd->bind_indx);
 	  /* TODO: Figure out if we should build a local type that has any
 	     excess alignment or size from the original decl.  */
-	  local_var.field_id
-	    = coro_make_frame_entry (lvd->field_list, buf, lvtype, lvd->loc);
-	  free (buf);
+	  if (buf)
+	    {
+	      local_var.field_id = coro_make_frame_entry (lvd->field_list, buf,
+							  lvtype, lvd->loc);
+	      free (buf);
+	    }
 	  /* We don't walk any of the local var sub-trees, they won't contain
 	     any bind exprs.  */
 	}
diff --git a/gcc/testsuite/g++.dg/coroutines/pr105287.C b/gcc/testsuite/g++.dg/coroutines/pr105287.C
new file mode 100644
index 00000000000..9790945287d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/pr105287.C
@@ -0,0 +1,48 @@
+// { dg-additional-options "-fanalyzer" }
+// { dg-excess-errors "lots of analyzer output, but no ICE" }
+namespace std {
+template <typename _Result> struct coroutine_traits : _Result {};
+template <typename = void> struct coroutine_handle {
+  operator coroutine_handle<>();
+};
+}
+struct coro1 {
+  using handle_type = std::coroutine_handle<>;
+  coro1(handle_type);
+  struct suspend_always_prt {
+    bool await_ready() noexcept;
+    void await_suspend(handle_type) noexcept;
+    void await_resume() noexcept;
+  };
+  struct promise_type {
+    std::coroutine_handle<> ch_;
+    auto get_return_object() { return ch_; }
+    auto initial_suspend() { return suspend_always_prt{}; }
+    auto final_suspend() noexcept { return suspend_always_prt{}; }
+    void unhandled_exception();
+  };
+};
+struct BoolAwaiter {
+  BoolAwaiter(bool);
+  bool await_ready();
+  void await_suspend(std::coroutine_handle<>);
+  bool await_resume();
+};
+struct IntAwaiter {
+  IntAwaiter(int);
+  bool await_ready();
+  void await_suspend(std::coroutine_handle<>);
+  int await_resume();
+};
+coro1 my_coro() {
+ int a = 1;
+ if (a == 0) {
+   int b = 5;
+   
+ }
+ {
+   int c = 10;
+ }
+ co_await BoolAwaiter(true) && co_await IntAwaiter(a); 
+ 
+ }
-- 


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] c++, coroutines: Make sure our temporaries are in a bind expr [PR105287]
  2022-04-28  8:28   ` Iain Sandoe
@ 2022-04-28 11:44     ` Jason Merrill
  0 siblings, 0 replies; 4+ messages in thread
From: Jason Merrill @ 2022-04-28 11:44 UTC (permalink / raw)
  To: Iain Sandoe; +Cc: GCC Patches

On 4/28/22 04:28, Iain Sandoe wrote:
> Hi Jason,
> 
>> On 20 Apr 2022, at 03:14, Jason Merrill <jason@redhat.com> wrote:
>>
>> On 4/18/22 10:02, Iain Sandoe wrote:
>>> There are a few cases where we can generate a temporary that does not need
>>> to be added to the coroutine frame (i.e. these are genuinely ephemeral).  The
>>> intent was that unnamed temporaries should not be 'promoted' to coroutine
>>> frame entries.  However there was a thinko and these were not actually ever
>>> added to the bind expressions being generated for the expanded awaits.  This
>>> meant that they were showing in the global namspace, leading to an empty
>>> DECL_CONTEXT and the ICE reported.
>>> tested on x86_64-darwin, OK for mainline? / Backports? (when?)
>>> thanks,
>>> Iain
>>> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
>>> 	PR c++/105287
>>> gcc/cp/ChangeLog:
>>> 	* coroutines.cc (maybe_promote_temps): Ensure generated temporaries
>>> 	are added to the bind expr.
>>> 	(add_var_to_bind): Fix local var naming to use portable punctuation.
>>> 	(register_local_var_uses): Do not add synthetic names to unnamed
>>> 	temporaries.
>>> gcc/testsuite/ChangeLog:
>>> 	* g++.dg/coroutines/pr105287.C: New test.
>>> ---
>>>   gcc/cp/coroutines.cc                       | 17 ++++----
>>>   gcc/testsuite/g++.dg/coroutines/pr105287.C | 48 ++++++++++++++++++++++
>>>   2 files changed, 56 insertions(+), 9 deletions(-)
>>>   create mode 100644 gcc/testsuite/g++.dg/coroutines/pr105287.C
>>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
>>> index a9ce6e050dd..dcc2284171b 100644
>>> --- a/gcc/cp/coroutines.cc
>>> +++ b/gcc/cp/coroutines.cc
>>> @@ -3103,7 +3103,7 @@ maybe_promote_temps (tree *stmt, void *d)
>>>   	 If the initializer is a conditional expression, we need to collect
>>>   	 and declare any promoted variables nested within it.  DTORs for such
>>>   	 variables must be run conditionally too.  */
>>> -      if (t->var && DECL_NAME (t->var))
>>> +      if (t->var)
>>>   	{
>>>   	  tree var = t->var;
>>>   	  DECL_CHAIN (var) = vlist;
>>> @@ -3304,7 +3304,7 @@ add_var_to_bind (tree& bind, tree var_type,
>>>     tree b_vars = BIND_EXPR_VARS (bind);
>>>     /* Build a variable to hold the condition, this will be included in the
>>>        frame as a local var.  */
>>> -  char *nam = xasprintf ("%s.%d", nam_root, nam_vers);
>>> +  char *nam = xasprintf ("`__%s_%d", nam_root, nam_vers);
>>
>> ` is portable?
> 
> nope, it’s a typo - apparently accepted by GAS and Darwin’s assembler tho…
> thanks for catching that.
> 
>>>     tree newvar = build_lang_decl (VAR_DECL, get_identifier (nam), var_type);
>>>     free (nam);
>>>     DECL_CHAIN (newvar) = b_vars;
>>> @@ -3949,7 +3949,7 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d)
>>>   	     scopes with identically named locals and still be able to
>>>   	     identify them in the coroutine frame.  */
>>>   	  tree lvname = DECL_NAME (lvar);
>>> -	  char *buf;
>>> +	  char *buf = NULL;
>>>     	  /* The outermost bind scope contains the artificial variables that
>>>   	     we inject to implement the coro state machine.  We want to be able
>>> @@ -3959,14 +3959,13 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d)
>>>   	  else if (lvname != NULL_TREE)
>>>   	    buf = xasprintf ("%s_%u_%u", IDENTIFIER_POINTER (lvname),
>>>   			     lvd->nest_depth, lvd->bind_indx);
>>> -	  else
>>> -	    buf = xasprintf ("_D%u_%u_%u", DECL_UID (lvar), lvd->nest_depth,
>>> -			     lvd->bind_indx);
>>>   	  /* TODO: Figure out if we should build a local type that has any
>>>   	     excess alignment or size from the original decl.  */
>>> -	  local_var.field_id
>>> -	    = coro_make_frame_entry (lvd->field_list, buf, lvtype, lvd->loc);
>>> -	  free (buf);
>>> +	  if (buf) {
>>
>> Brace should be on the next line.
> 
> that was careless, too much rushing, fixed as below,
> OK now?

OK.

> -----
> 
> [PATCH] c++, coroutines: Make sure our temporaries are in a bind expr
>   [PR105287]
> 
> There are a few cases where we can generate a temporary that does not need
> to be added to the coroutine frame (i.e. these are genuinely ephemeral).  The
> intent was that unnamed temporaries should not be 'promoted' to coroutine
> frame entries.  However there was a thinko and these were not actually ever
> added to the bind expressions being generated for the expanded awaits.  This
> meant that they were showing in the global namspace, leading to an empty
> DECL_CONTEXT and the ICE reported.
> 
> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
> 
> 	PR c++/105287
> 
> gcc/cp/ChangeLog:
> 
> 	* coroutines.cc (maybe_promote_temps): Ensure generated temporaries
> 	are added to the bind expr.
> 	(add_var_to_bind): Fix local var naming to use portable punctuation.
> 	(register_local_var_uses): Do not add synthetic names to unnamed
> 	temporaries.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/coroutines/pr105287.C: New test.
> ---
>   gcc/cp/coroutines.cc                       | 18 ++++----
>   gcc/testsuite/g++.dg/coroutines/pr105287.C | 48 ++++++++++++++++++++++
>   2 files changed, 57 insertions(+), 9 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/coroutines/pr105287.C
> 
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index a9ce6e050dd..7e9ff69ae35 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -3103,7 +3103,7 @@ maybe_promote_temps (tree *stmt, void *d)
>   	 If the initializer is a conditional expression, we need to collect
>   	 and declare any promoted variables nested within it.  DTORs for such
>   	 variables must be run conditionally too.  */
> -      if (t->var && DECL_NAME (t->var))
> +      if (t->var)
>   	{
>   	  tree var = t->var;
>   	  DECL_CHAIN (var) = vlist;
> @@ -3304,7 +3304,7 @@ add_var_to_bind (tree& bind, tree var_type,
>     tree b_vars = BIND_EXPR_VARS (bind);
>     /* Build a variable to hold the condition, this will be included in the
>        frame as a local var.  */
> -  char *nam = xasprintf ("%s.%d", nam_root, nam_vers);
> +  char *nam = xasprintf ("__%s_%d", nam_root, nam_vers);
>     tree newvar = build_lang_decl (VAR_DECL, get_identifier (nam), var_type);
>     free (nam);
>     DECL_CHAIN (newvar) = b_vars;
> @@ -3949,7 +3949,7 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d)
>   	     scopes with identically named locals and still be able to
>   	     identify them in the coroutine frame.  */
>   	  tree lvname = DECL_NAME (lvar);
> -	  char *buf;
> +	  char *buf = NULL;
>   
>   	  /* The outermost bind scope contains the artificial variables that
>   	     we inject to implement the coro state machine.  We want to be able
> @@ -3959,14 +3959,14 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d)
>   	  else if (lvname != NULL_TREE)
>   	    buf = xasprintf ("%s_%u_%u", IDENTIFIER_POINTER (lvname),
>   			     lvd->nest_depth, lvd->bind_indx);
> -	  else
> -	    buf = xasprintf ("_D%u_%u_%u", DECL_UID (lvar), lvd->nest_depth,
> -			     lvd->bind_indx);
>   	  /* TODO: Figure out if we should build a local type that has any
>   	     excess alignment or size from the original decl.  */
> -	  local_var.field_id
> -	    = coro_make_frame_entry (lvd->field_list, buf, lvtype, lvd->loc);
> -	  free (buf);
> +	  if (buf)
> +	    {
> +	      local_var.field_id = coro_make_frame_entry (lvd->field_list, buf,
> +							  lvtype, lvd->loc);
> +	      free (buf);
> +	    }
>   	  /* We don't walk any of the local var sub-trees, they won't contain
>   	     any bind exprs.  */
>   	}
> diff --git a/gcc/testsuite/g++.dg/coroutines/pr105287.C b/gcc/testsuite/g++.dg/coroutines/pr105287.C
> new file mode 100644
> index 00000000000..9790945287d
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr105287.C
> @@ -0,0 +1,48 @@
> +// { dg-additional-options "-fanalyzer" }
> +// { dg-excess-errors "lots of analyzer output, but no ICE" }
> +namespace std {
> +template <typename _Result> struct coroutine_traits : _Result {};
> +template <typename = void> struct coroutine_handle {
> +  operator coroutine_handle<>();
> +};
> +}
> +struct coro1 {
> +  using handle_type = std::coroutine_handle<>;
> +  coro1(handle_type);
> +  struct suspend_always_prt {
> +    bool await_ready() noexcept;
> +    void await_suspend(handle_type) noexcept;
> +    void await_resume() noexcept;
> +  };
> +  struct promise_type {
> +    std::coroutine_handle<> ch_;
> +    auto get_return_object() { return ch_; }
> +    auto initial_suspend() { return suspend_always_prt{}; }
> +    auto final_suspend() noexcept { return suspend_always_prt{}; }
> +    void unhandled_exception();
> +  };
> +};
> +struct BoolAwaiter {
> +  BoolAwaiter(bool);
> +  bool await_ready();
> +  void await_suspend(std::coroutine_handle<>);
> +  bool await_resume();
> +};
> +struct IntAwaiter {
> +  IntAwaiter(int);
> +  bool await_ready();
> +  void await_suspend(std::coroutine_handle<>);
> +  int await_resume();
> +};
> +coro1 my_coro() {
> + int a = 1;
> + if (a == 0) {
> +   int b = 5;
> +
> + }
> + {
> +   int c = 10;
> + }
> + co_await BoolAwaiter(true) && co_await IntAwaiter(a);
> +
> + }


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2022-04-28 11:44 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-18 14:02 [PATCH] c++, coroutines: Make sure our temporaries are in a bind expr [PR105287] Iain Sandoe
2022-04-20  2:14 ` Jason Merrill
2022-04-28  8:28   ` Iain Sandoe
2022-04-28 11:44     ` Jason Merrill

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).