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

[Jason Merrill <jason@redhat.com>]

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);
> +
> + }